[PATCH] Proxy remote server SSL certificate verification

[Maxim Dounin]

Hello!

On Tue, Aug 27, 2013 at 11:47:38AM +0300, Aviram Cohen wrote:

> Added a new version, with all the required fixes.

This looks better, modulo various style problems.  It also looks 
like verification code isn't complete.

See below for comments.

> 
> diff -Nrpu nginx-1.4.1/src/http/modules/ngx_http_proxy_module.c
> nginx-1.4.1-proxy-ssl-verification/src/http/modules/ngx_http_proxy_module.c
> --- nginx-1.4.1/src/http/modules/ngx_http_proxy_module.c 2013-05-06
> 13:26:50.000000000 +0300
> +++ nginx-1.4.1-proxy-ssl-verification/src/http/modules/ngx_http_proxy_module.c
> 2013-08-26 10:43:15.639557701 +0300
> @@ -511,6 +511,27 @@ static ngx_command_t  ngx_http_proxy_com
>        offsetof(ngx_http_proxy_loc_conf_t, upstream.ssl_session_reuse),
>        NULL },
> 
> +

Style: extra empty line.

> +    { ngx_string("proxy_ssl_verify"),
> +      NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE1,
> +      ngx_conf_set_flag_slot,
> +      NGX_HTTP_LOC_CONF_OFFSET,
> +      offsetof(ngx_http_proxy_loc_conf_t, upstream.ssl_verify),
> +      NULL },
> +
> +    { ngx_string("proxy_ssl_verify_depth"),
> +      NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE1,
> +      ngx_conf_set_num_slot,
> +      NGX_HTTP_LOC_CONF_OFFSET,
> +      offsetof(ngx_http_proxy_loc_conf_t, upstream.ssl_verify_depth),
> +      NULL },
> +
> +    { ngx_string("proxy_ssl_trusted_certificate"),
> +      NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE1,
> +      ngx_conf_set_str_slot,
> +      NGX_HTTP_LOC_CONF_OFFSET,
> +      offsetof(ngx_http_proxy_loc_conf_t, upstream.ssl_certificate),
> +      NULL },
>  #endif

Style: missing empty line before "#endif".

It also might make sense to name the field ssl_trusted_certificate 
to match directive name.

It probably also a good idea to move both certificate and 
verify_depth to ngx_http_proxy_loc_conf_t as they aren't needed in 
upstream configuration.  (At upstream level, only upstream.verify 
is needed/used.)

> 
>        ngx_null_command
> @@ -2419,8 +2440,11 @@ ngx_http_proxy_create_loc_conf(ngx_conf_
>      conf->upstream.pass_headers = NGX_CONF_UNSET_PTR;
> 
>      conf->upstream.intercept_errors = NGX_CONF_UNSET;
> +
>  #if (NGX_HTTP_SSL)
>      conf->upstream.ssl_session_reuse = NGX_CONF_UNSET;
> +    conf->upstream.ssl_verify = NGX_CONF_UNSET;
> +    conf->upstream.ssl_verify_depth = NGX_CONF_UNSET_UINT;
>  #endif
> 
>      /* "proxy_cyclic_temp_file" is disabled */

Style: please add conf->upstream.ssl_certificate (or whatever) to 
a comment "set by ngx_pcalloc()".

> @@ -2697,6 +2721,30 @@ ngx_http_proxy_merge_loc_conf(ngx_conf_t
>  #if (NGX_HTTP_SSL)
>      ngx_conf_merge_value(conf->upstream.ssl_session_reuse,
>                                prev->upstream.ssl_session_reuse, 1);

Style: if you add empty line before "#endif", please add another 
one after "#if".

> +    ngx_conf_merge_value(conf->upstream.ssl_verify,
> +                              prev->upstream.ssl_verify, 0);
> +    ngx_conf_merge_uint_value(conf->upstream.ssl_verify_depth,
> +                              prev->upstream.ssl_verify_depth, 1);
> +    ngx_conf_merge_str_value(conf->upstream.ssl_certificate,
> +                              prev->upstream.ssl_certificate, "");
> +
> +    if (conf->upstream.ssl_verify) {
> +        if (conf->upstream.ssl_certificate.len == 0) {
> +            ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
> +                               "no \"proxy_ssl_trusted_certificate\" is defined for "
> +                               "the \"proxy_ssl_verify\" directive");

Style: no lines longer than 80 chars, please.

It might also be a good idea to only complain if there is 
conf->upstream.ssl.

> +
> +            return NGX_CONF_ERROR;
> +        }
> +    }
> +
> +    if (conf->upstream.ssl &&
> +        ngx_ssl_trusted_certificate(cf, conf->upstream.ssl,
> +          &conf->upstream.ssl_certificate, conf->upstream.ssl_verify_depth) != NGX_OK)
> +    {

Style:

- no lines longer than 80 chars, please.
- if you wrap long conditions, please put operators a the start of 
  a continuation line.

That is,

    if (conf->upstream.ssl
        && ngx_ssl_trusted_certificate(cf, conf->upstream.ssl,
                                       &conf->upstream.ssl_certificate
                                       conf->upstream.ssl_verify_depth)
           != NGX_OK)
    {
        ...
    }

Additional question is what happens in a configuration like

    location / {
        proxy_pass https://example.com;
        proxy_ssl_verify on;
        proxy_ssl_trusted_ceritifcate example.crt;

        if ($foo) {
            # do nothing
        }
    }

or the same with a nested location instead of "if".  Quick look 
suggest it will result in trusted certs loaded twice (and stale 
alerts later due to how OpenSSL handles this).

> +        return NGX_CONF_ERROR;
> +    }
> +
>  #endif
> 
>      ngx_conf_merge_value(conf->redirect, prev->redirect, 1);
> diff -Nrpu nginx-1.4.1/src/http/ngx_http_upstream.c
> nginx-1.4.1-proxy-ssl-verification/src/http/ngx_http_upstream.c
> --- nginx-1.4.1/src/http/ngx_http_upstream.c 2013-05-06 13:26:50.000000000 +0300
> +++ nginx-1.4.1-proxy-ssl-verification/src/http/ngx_http_upstream.c
> 2013-08-26 10:44:35.323558884 +0300
> @@ -1324,7 +1324,13 @@ ngx_http_upstream_ssl_handshake(ngx_conn
>      u = r->upstream;
> 
>      if (c->ssl->handshaked) {
> -
> +        if (u->conf->ssl_verify && SSL_get_verify_result(c->ssl->connection) != X509_V_OK) {
> +            ngx_log_error(NGX_LOG_ERR, c->log, 0, "upstream ssl certificate validation failed");
> +            c = r->connection;
> +            ngx_http_upstream_next(r, u, NGX_HTTP_UPSTREAM_FT_ERROR);
> +            goto fail;
> +        }
> +

Style: 80 chars.

Please also note that SSL_get_verify_result() will return 
X509_V_OK if there is no certificate at all, quote from 
SSL_get_verify_result manpage:

       If no peer certificate was presented, the returned result code is
       X509_V_OK. This is because no verification error occurred, it does
       however not indicate success. SSL_get_verify_result() is only useful in
       connection with SSL_get_peer_certificate(3).

Please take a look at relevant code at ngx_http_process_request().  
It also seems to have better error reporting.

>          if (u->conf->ssl_session_reuse) {
>              u->peer.save_session(&u->peer, u->peer.data);
>          }
> @@ -1332,13 +1338,21 @@ ngx_http_upstream_ssl_handshake(ngx_conn
>          c->write->handler = ngx_http_upstream_handler;
>          c->read->handler = ngx_http_upstream_handler;
> 
> +        c = r->connection;
> +
>          ngx_http_upstream_send_request(r, u);
> 
> +fail:
> +        ngx_http_run_posted_requests(c);
> +
>          return;
>      }

Probably just adding

    ngx_http_run_posted_requests(c);
    return;

instead of "goto" above would be more readable...

> 
> +    c = r->connection;
> +
>      ngx_http_upstream_next(r, u, NGX_HTTP_UPSTREAM_FT_ERROR);
> 
> +    ngx_http_run_posted_requests(c);
>  }

... or, alternatively, "fail:" may be moved before these lines 
(and "c = ..." and "ngx_http_upstream_next(...)" lines before 
"goto" removed), resulting in less code duplication.

[...]

> On Thu, Aug 22, 2013 at 5:00 PM, Aviram Cohen <aviram at adallom.com> wrote:
> > Hello!
> >
> > I have a couple of questions regarding the two last comments:

[...]

Sorry, I somehow missed this message.  Glad to see you've 
succcessfully found answers.

-- 
Maxim Dounin
http://nginx.org/en/donation.html