[PATCH] Proxy: always emit "Host" header first

[Maxim Dounin]

Hello!

On Sat, Jun 03, 2017 at 08:03:57PM -0700, Piotr Sikora via nginx-devel wrote:

> # HG changeset patch
> # User Piotr Sikora <piotrsikora at google.com>
> # Date 1489618489 25200
> #      Wed Mar 15 15:54:49 2017 -0700
> # Node ID e472b23fdc387943ea90fb2f0ae415d9d104edc7
> # Parent  716852cce9136d977b81a2d1b8b6f9fbca0dce49
> Proxy: always emit "Host" header first.
> 
> Signed-off-by: Piotr Sikora <piotrsikora at google.com>
> 
> diff -r 716852cce913 -r e472b23fdc38 src/http/modules/ngx_http_proxy_module.c
> --- a/src/http/modules/ngx_http_proxy_module.c
> +++ b/src/http/modules/ngx_http_proxy_module.c
> @@ -3412,7 +3412,7 @@ ngx_http_proxy_init_headers(ngx_conf_t *
>      uintptr_t                    *code;
>      ngx_uint_t                    i;
>      ngx_array_t                   headers_names, headers_merged;
> -    ngx_keyval_t                 *src, *s, *h;
> +    ngx_keyval_t                 *host, *src, *s, *h;
>      ngx_hash_key_t               *hk;
>      ngx_hash_init_t               hash;
>      ngx_http_script_compile_t     sc;
> @@ -3444,11 +3444,33 @@ ngx_http_proxy_init_headers(ngx_conf_t *
>          return NGX_ERROR;
>      }
>  
> +    h = default_headers;
> +
> +    if (h->key.len != sizeof("Host") - 1
> +        || ngx_strcasecmp(h->key.data, (u_char *) "Host") != 0)
> +    {
> +        return NGX_ERROR;
> +    }
> +
> +    host = ngx_array_push(&headers_merged);
> +    if (host == NULL) {
> +        return NGX_ERROR;
> +    }
> +
> +    *host = *h++;
> +
>      if (conf->headers_source) {
>  
>          src = conf->headers_source->elts;
>          for (i = 0; i < conf->headers_source->nelts; i++) {
>  
> +            if (src[i].key.len == sizeof("Host") - 1
> +                && ngx_strcasecmp(src[i].key.data, (u_char *) "Host") == 0)
> +            {
> +                *host = src[i];
> +                continue;
> +            }
> +
>              s = ngx_array_push(&headers_merged);
>              if (s == NULL) {
>                  return NGX_ERROR;
> @@ -3458,8 +3480,6 @@ ngx_http_proxy_init_headers(ngx_conf_t *
>          }
>      }
>  
> -    h = default_headers;
> -
>      while (h->key.len) {
>  
>          src = headers_merged.elts;

I don't think I like this.  Trying to prioritize headers based on 
some mostly external knowledge in a function which is intended to 
merge headers user-supplied and default headers doesn't look wise.  
Not to mention that it depends on the "Host" header being first in 
the default headers lists.

Instead, consider the following variants:

- emitting default headers first, than user-supplied ones;

- emitting the Host header separately from other headers, via a 
  dedicated code in ngx_http_proxy_create_request().

Note well that I'm not really sure that any of the above variants 
is in fact better than the current behaviour.  The current 
behaviour is to follow header order specified in the 
configuration, and it makes it possible to redefine order of 
standard headers if needed.

I suspect this and other patches you've send at the same time are 
in fact parts of your work to introduce HTTP/2 support to upstream 
servers.  Please consider submitting a patch series with the whole 
feature instead.  Individual patches which doesn't make much sense 
by its own are hard to review, and are likely to be rejected.

-- 
Maxim Dounin
http://nginx.org/