Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Full request-line sent with http2 #3570

Closed
bungle opened this issue Feb 14, 2019 · 4 comments
Closed

Full request-line sent with http2 #3570

bungle opened this issue Feb 14, 2019 · 4 comments

Comments

@bungle
Copy link

bungle commented Feb 14, 2019

I did this

curl -x http://example.test:8000/ http://www.example.org:1234/ \
     --http2-prior-knowledge -v

and from that I got this (the relevant part only):

> GET http://www.example.org:1234/ HTTP/2
> Host: www.example.org:1234

Which my Nginx server terminated with:

2019/02/14 [info] client sent invalid :path header: "http://www.example.org:1234/"
                  while reading client request headers

I expected the following

With http2 the client should not send other than path+query with the request line as it is not supported with http2.

I expected:

> GET / HTTP/2
> Host: www.example.org:1234

curl/libcurl version

url 7.63.0 (x86_64-apple-darwin16.7.0) libcurl/7.63.0 OpenSSL/1.0.2q zlib/1.2.8
                                       libidn2/2.1.0 nghttp2/1.35.1
Release-Date: 2018-12-12
Protocols: dict file ftp ftps gopher http https imap imaps ldap ldaps pop3
           pop3s rtsp smb smbs smtp smtps telnet tftp 
Features: AsynchDNS IDN IPv6 Largefile NTLM NTLM_WB SSL libz TLS-SRP
          HTTP2 UnixSockets HTTPS-proxy 

operating system

macOS Sierra 10.12.6

@jay
Copy link
Member

jay commented Feb 16, 2019

Is this allowed or supported in http2? libcurl reports a PROTOCOL_ERROR and nginx 1.10.3 server /var/log/nginx/error.log shows nothing.

curl/lib/http.c

Lines 2513 to 2530 in f3294d9

/* url */
if(conn->bits.httpproxy && !conn->bits.tunnel_proxy) {
char *url = data->change.url;
result = Curl_add_buffer(&req_buffer, url, strlen(url));
}
else if(paste_ftp_userpwd)
result = Curl_add_bufferf(&req_buffer, "ftp://%s:%s@%s",
conn->user, conn->passwd,
path + sizeof("ftp://") - 1);
else {
result = Curl_add_buffer(&req_buffer, path, strlen(path));
if(result)
return result;
if(query)
result = Curl_add_bufferf(&req_buffer, "?%s", query);
}
if(result)
return result;

If I change it to conn->httpversion != 20 then I can get it working [1] so it's possible. It looks like custom host headers would have to be ignored (or sent as actual :host?) to ensure that the actual host is always sent as :authority? I don't know the best way to address this so I tried it parsing the url in http2.c rather than try to predict or have logic whether or not we're at http2 elsewhere...feels wrong though

diff --git a/lib/http2.c b/lib/http2.c
index 2884c71..5dd943e 100644
--- a/lib/http2.c
+++ b/lib/http2.c
@@ -1835,7 +1835,7 @@ static ssize_t http2_send(struct connectdata *conn, int sockindex,
   nghttp2_nv *nva = NULL;
   size_t nheader;
   size_t i;
-  size_t authority_idx;
+  size_t authority_idx, path_idx, scheme_idx;
   char *hdbuf = (char *)mem;
   char *end, *line_end;
   nghttp2_data_provider data_prd;
@@ -1947,6 +1947,7 @@ static ssize_t http2_send(struct connectdata *conn, int sockindex,
   }
   if(!end || end == hdbuf)
     goto fail;
+  path_idx = 1;
   nva[1].name = (unsigned char *)":path";
   nva[1].namelen = strlen((char *)nva[1].name);
   nva[1].value = (unsigned char *)hdbuf;
@@ -1957,6 +1958,7 @@ static ssize_t http2_send(struct connectdata *conn, int sockindex,
     goto fail;
   }
 
+  scheme_idx = 2;
   nva[2].name = (unsigned char *)":scheme";
   nva[2].namelen = strlen((char *)nva[2].name);
   if(conn->handler->flags & PROTOPT_SSL)
@@ -2030,6 +2032,50 @@ static ssize_t http2_send(struct connectdata *conn, int sockindex,
     ++i;
   }
 
+  /* hack to fix http proxy over http2 */
+  if(conn->bits.httpproxy && !conn->bits.tunnel_proxy) {
+    if(!authority_idx || !path_idx || !scheme_idx) {
+      failf(conn->data, "http2 missing pseudo-headers for proxy modification");
+      goto fail;
+    }
+    for(i = 0; i < nva[path_idx].valuelen; ++i) {
+      if(nva[path_idx].value[i] == ':') {
+        if(i + 2 < nva[path_idx].valuelen &&
+           nva[path_idx].value[i+1] == '/' &&
+           nva[path_idx].value[i+2] == '/')
+          break;
+        else {
+          failf(conn->data, "http2 bad scheme in path for proxy modification");
+          goto fail;
+        }
+      }
+    }
+    if(i == 0 || i == nva[path_idx].valuelen) {
+      failf(conn->data, "http2 missing scheme in path for proxy modification");
+      goto fail;
+    }
+    nva[scheme_idx].valuelen = &nva[path_idx].value[i] -
+                               nva[path_idx].value;
+    nva[scheme_idx].value = nva[path_idx].value;
+    nva[path_idx].valuelen -= &nva[path_idx].value[i+3] -
+                              nva[path_idx].value;
+    nva[path_idx].value = &nva[path_idx].value[i+3];
+    for(i = 0; i < nva[path_idx].valuelen; ++i) {
+      if(nva[path_idx].value[i] == '/')
+        break;
+    }
+    if(i == 0 || i == nva[path_idx].valuelen) {
+      failf(conn->data, "http2 missing host in path for proxy modification");
+      goto fail;
+    }
+    nva[authority_idx].valuelen = &nva[path_idx].value[i] -
+                                  nva[path_idx].value;
+    nva[authority_idx].value = nva[path_idx].value;
+    nva[path_idx].valuelen -= &nva[path_idx].value[i] -
+                              nva[path_idx].value;
+    nva[path_idx].value = &nva[path_idx].value[i];
+  }
+
   /* :authority must come before non-pseudo header fields */
   if(authority_idx != 0 && authority_idx != AUTHORITY_DST_IDX) {
     nghttp2_nv authority = nva[authority_idx];

(Patch now up at branch jay:http2_proxy)

[1]: I used a crude setup of nginx as a forward proxy with appended http2 to the listen line, added a proxy_set_header Proxy-Connection ""; and put it in /etc/nginx/sites-available/default and then restarted the server sudo systemctl reload nginx.service

@bagder
Copy link
Member

bagder commented Feb 16, 2019

I think the primary explanation for this flaw is the outstanding item in the TODO:

"Support HTTP/2 for HTTP(S) proxies"

This simply hasn't been made to work (yet)!

jay added a commit to jay/curl that referenced this issue Feb 16, 2019
- Parse the scheme, host and path from the :path component and then
  use that information to update the respective pseudo-headers.

http2.c constructs an http2 request by parsing a regular http request
that has been constructed in http.c. Normally this request has the
path as the request-uri field (ie /foo in GET /foo HTTP/1.1) so that is
what is used for http2 pseudo-header :path. However in the case of an
http proxy libcurl uses the full URL as the request-uri field
(ie http://test.com/foo in GET http://test.com/foo HTTP/1.1) so that
is not just :path but must be split into :scheme and :authority (host)
which is what this patch does. A side effect of this is that any
user-specified host field is overridden.

Ref: curl#3570 (comment)
Ref: https://curl.haxx.se/docs/todo.html#Support_HTTP_2_for_HTTP_S_proxi
@daurnimator
Copy link

I also ran into this today.

Noted at nghttp2/nghttp2#1304 (comment)

I'm not entirely sure what the behaviour here is meant to be, so in lua-http I added: https://github.com/daurnimator/lua-http/pull/141/files#diff-b4a19dcf32626be03ed2854d0f44ff8eR457

@bagder
Copy link
Member

bagder commented Apr 16, 2019

I close this issue now and refer to the TODO issue again for our still outstanding task to add HTTP/2 support for proxies. The --http2-prior-knowledge option is intended for talking to the server, and doing this over a HTTP/1 proxy I don't think can ever be made to work.

@jay's comments and work above can certainly serve as the beginning of this support, but should be submitted as a regular PR instead when ripe and we can then discuss its merits and the details then.

@bagder bagder closed this as completed Apr 16, 2019
jay added a commit to jay/curl that referenced this issue May 8, 2019
- Do not switch to HTTP/2 for an HTTP proxy that is not tunnelling to
  the destination host.

We already do something similar for HTTPS proxies by not sending h2. [1]

Prior to this change setting CURL_HTTP_VERSION_2_PRIOR_KNOWLEDGE would
incorrectly use HTTP/2 to talk to the proxy, which is not something we
support (yet?). Also it's debatable whether or not that setting should
apply to HTTP/2 proxies.

[1]: curl@17c5d05

Bug: curl#3570
Bug: curl#3832

Closes #xxxx
jay added a commit that referenced this issue May 9, 2019
- Do not switch to HTTP/2 for an HTTP proxy that is not tunnelling to
  the destination host.

We already do something similar for HTTPS proxies by not sending h2. [1]

Prior to this change setting CURL_HTTP_VERSION_2_PRIOR_KNOWLEDGE would
incorrectly use HTTP/2 to talk to the proxy, which is not something we
support (yet?). Also it's debatable whether or not that setting should
apply to HTTP/2 proxies.

[1]: 17c5d05

Bug: #3570
Bug: #3832

Closes #3853
@lock lock bot locked as resolved and limited conversation to collaborators Jul 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

No branches or pull requests

4 participants