Hello!

On Tue, Jun 13, 2017 at 05:19:48AM -0700, Piotr Sikora via nginx-devel wrote:

> # HG changeset patch
> # User Piotr Sikora <piotrsikora@google.com>
> # Date 1490516709 25200
> # Sun Mar 26 01:25:09 2017 -0700
> # Node ID e2abc3bc3fc12b788d2631d3c47215acdc4ebbe6
> # Parent 6263d68cb96042d8f8974a4a3945226227ce13b9
> HTTP/2: reject HTTP/2 requests with connection-specific headers.
>
> Signed-off-by: Piotr Sikora <piotrsikora@google.com>
>
> diff -r 6263d68cb960 -r e2abc3bc3fc1 src/http/ngx_http_request.c
> --- a/src/http/ngx_http_request.c
> +++ b/src/http/ngx_http_request.c
> @@ -19,6 +19,8 @@ static ngx_int_t ngx_http_alloc_large_he
>
> static ngx_int_t ngx_http_process_header_line(ngx_http_request_t *r,
> ngx_table_elt_t *h, ngx_uint_t offset);
> +static ngx_int_t ngx_http_process_http1_header_line(ngx_http_request_t *r,
> + ngx_table_elt_t *h, ngx_uint_t offset);
> static ngx_int_t ngx_http_process_unique_header_line(ngx_http_request_t *r,
> ngx_table_elt_t *h, ngx_uint_t offset);
> static ngx_int_t ngx_http_process_multi_header_lines(ngx_http_request_t *r,
> @@ -146,7 +148,7 @@ ngx_http_header_t ngx_http_headers_in[]
>
> { ngx_string("Upgrade"),
> offsetof(ngx_http_headers_in_t, upgrade),
> - ngx_http_process_header_line },
> + ngx_http_process_http1_header_line },
>
> #if (NGX_HTTP_GZIP)
> { ngx_string("Accept-Encoding"),
> @@ -161,8 +163,13 @@ ngx_http_header_t ngx_http_headers_in[]
> offsetof(ngx_http_headers_in_t, authorization),
> ngx_http_process_unique_header_line },
>
> - { ngx_string("Keep-Alive"), offsetof(ngx_http_headers_in_t, keep_alive),
> - ngx_http_process_header_line },
> + { ngx_string("Keep-Alive"),
> + offsetof(ngx_http_headers_in_t, keep_alive),
> + ngx_http_process_http1_header_line },
> +
> + { ngx_string("Proxy-Connection"),
> + offsetof(ngx_http_headers_in_t, proxy_connection),
> + ngx_http_process_http1_header_line },

I'm highly sceptical about the whole series in general, and this
patch specifically.

In particular, the "Proxy-Connection" header is not something even
defined by any standard, and even in its non-standard [broken]
meaning never expected to be used in connections to nginx. Not to
mention that Proxy-Authorization, a standard-defined hop-by-hop
(connection-specific in terms of HTTP/2) header, is not checked
anywhere.

Additionally, I really think that disabling upgrades is one of the
big mistakes of HTTP/2. It would be much more logical to
interpret a HTTP/2 stream as a connection to upgrade, and allow to
multiplex arbitrary protocols via a single HTTP/2 connection.

Unless there are practical reasons for these changes, I would
rather reject the series.

--
Maxim Dounin
http://nginx.org/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel