[PATCH] SSL: add "{proxy, uwsgi}_ssl_verify" and supporting directives

[Maxim Dounin]

Hello!

On Tue, Feb 04, 2014 at 10:54:47PM -0800, Piotr Sikora wrote:

> # HG changeset patch
> # User Piotr Sikora <piotr at cloudflare.com>
> # Date 1391582213 28800
> #      Tue Feb 04 22:36:53 2014 -0800
> # Node ID e7704dcea76c83708cd8bf01709e15dc658871ae
> # Parent  f0129ac05ced1ee418fa97dbbae35f3c0b831992
> SSL: add "{proxy,uwsgi}_ssl_verify" and supporting directives.

Nitpicking:

It may be better to write about "proxy_..." directives, and 
mention uwsgi counterparts separately and/or introduce them in a 
separate patch.  Cryptic names to mention all the modules aren't 
very readable and hardly searchable, so we generally try to avoid 
them now.

> Verify SSL certificate when connecting to an SSL upstream.
> 
> "{proxy,uwsgi}_ssl_verify" directives support 3 modes:
> - off - don't verify upstream's SSL certificate (default),
> - on - verify validity and trust of upstream's SSL certificate,
> - server_name - same as above, but when SNI is used, also verify
>   that it matches one of the hostnames in the certificate. This
>   mode requires OpenSSL-1.0.2+.

I don't really like this approach of a special "server_name" value 
and SNI dependency, it looks counterintuitive.  Peer name 
verification should be done by default, and probably there should 
be a separate option to turn it off if needed for some reason.

I believe the main reason for SNI dependency is a name to verify 
against.  In case of proxy, shouldn't it be $proxy_host by 
default?

Something like this:

    proxy_ssl_verify on;
    proxy_ssl_name $proxy_host;

or this:

    proxy_ssl_verify on;
    proxy_ssl_verify_name off;

And the same name probably may be used for SNI, with an 
additional flag to switch it on, like this:

    proxy_ssl_sni on;
    proxy_ssl_name $proxy_host;

(Well, it might be better to introduce something more generic to 
also resolve default proxy_cache_key vs. "proxy_set_header Host" 
issue, but I don't see any obvious solution yet.)

What do you think?

[...]

> diff -r f0129ac05ced -r e7704dcea76c src/http/modules/ngx_http_upstream_keepalive_module.c
> --- a/src/http/modules/ngx_http_upstream_keepalive_module.c	Tue Feb 04 22:36:41 2014 -0800
> +++ b/src/http/modules/ngx_http_upstream_keepalive_module.c	Tue Feb 04 22:36:53 2014 -0800
> @@ -51,6 +51,7 @@ typedef struct {
>  
>  #if (NGX_HTTP_SSL)
>      ngx_str_t                          server_name;
> +    unsigned                           verify:2;
>  #endif
>  
>  } ngx_http_upstream_keepalive_cache_t;
> @@ -250,6 +251,7 @@ ngx_http_upstream_get_keepalive_peer(ngx
>                  || ngx_strncmp(pc->server_name.data, item->server_name.data,
>                                 pc->server_name.len)
>                     == 0)
> +            && (pc->verify <= item->verify)
>  #endif
>              )
>          {
> @@ -372,6 +374,8 @@ ngx_http_upstream_free_keepalive_peer(ng
>                     pc->server_name.len);
>      }
>  
> +    item->verify = pc->verify;
> +
>  #endif
>  
>      if (c->read->ready) {

Not sure if it's needed at all.  I think we can safely assume that 
verification options are the same in all cases.

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