Hello!

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

> # HG changeset patch
> # User Piotr Sikora <piotr@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/

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