[PATCH] Proxy remote server SSL certificate verification

[Maxim Dounin]

Hello!

On Tue, Sep 03, 2013 at 03:32:58PM +0300, Aviram Cohen wrote:

> Thanks for the comments. The new version with all the fixes is
> attached (and also pasted in this mail).

See below for comments.

[...]

> @@ -2382,6 +2408,7 @@ ngx_http_proxy_create_loc_conf(ngx_conf_
>       *     conf->body_set = NULL;
>       *     conf->body_source = { 0, NULL };
>       *     conf->redirects = NULL;
> +     *     conf->ssl_trusted_certificate = NULL;
>       */
> 
>      conf->upstream.store = NGX_CONF_UNSET;

Nitpicking: ssl_trusted_certificate is ngx_str_t, so it's set 
to { 0, NULL } (much like body_source above).

[...]

> +    if (conf->upstream.ssl && conf->upstream.ssl_verify) {
> +        if (conf->ssl_trusted_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");
> +
> +            return NGX_CONF_ERROR;
> +        }
> +    }
> +
> +    if (conf->upstream.ssl
> +        && ngx_ssl_trusted_certificate(cf, conf->upstream.ssl,
> +                                       &conf->ssl_trusted_certificate,
> +                                       conf->ssl_verify_depth)
> +        != NGX_OK)
> +    {
> +        return NGX_CONF_ERROR;
> +    }

Nitpicking: if (conf->upstream.ssl) seems to be common condition, 
and probably conf->upstream.ssl_verify too.  Merging the two under 
something like

    if (conf->upstream.ssl && conf->upstream.ssl_verify) {

        if (conf->ssl_trusted_certificate.len == 0) {
            ...
        }

        if (ngx_ssl_trusted_certificate(...)
            != NGX_OK)
        {
            ...
        }
    }

should be better.

[...]

> @@ -1319,12 +1319,30 @@ ngx_http_upstream_ssl_handshake(ngx_conn
>  {
>      ngx_http_request_t   *r;
>      ngx_http_upstream_t  *u;
> -
> +    X509                 *cert;
> +
>      r = c->data;
>      u = r->upstream;
> 
>      if (c->ssl->handshaked) {
> -
> +        if (u->conf->ssl_verify) {
> +            if (SSL_get_verify_result(c->ssl->connection) != X509_V_OK) {
> +                ngx_log_error(NGX_LOG_ERR, c->log, 0,
> +                              "upstream ssl certificate validation failed");

This seems to lack detailsed error reporting.  Logging at least 
error returned and X509_verify_cert_error_string() as it's done in 
ngx_http_process_request() is really good idea.

> +                goto fail;
> +            }
> +
> +            cert = SSL_get_peer_certificate(c->ssl->connection);
> +
> +            if (cert == NULL) {
> +                ngx_log_error(NGX_LOG_INFO, c->log, 0,
> +                              "upstream sent no required SSL certificate");

The "required" is probably not needed.  And NGX_LOG_INFO logging 
level looks very wrong - while it's used by 
ngx_http_process_request(), it's used in a situation which may be 
easily triggered by broken/malicious clients, so the "info" level 
is appropiate.  No certificate from an upstream server deserves at 
least "error" level (which is already used in your patch by 
previous error message).

> +                goto fail;
> +            }
> +
> +            X509_free(cert);
> +        }
> +
>          if (u->conf->ssl_session_reuse) {
>              u->peer.save_session(&u->peer, u->peer.data);
>          }

One more relatively major point: certificate checking seems to 
lack any peer name validation.  Without it, any certificate issued 
by a trusted certificate authority can be used, making it 
impossible to use certificate verification to prevent MITM if you 
don't control trusted CAs.

I tend to think it's required for initial proxy certificate 
verification.  Though probably there should be a directive to 
switch the off, like in Apache, see 
http://httpd.apache.org/docs/2.4/mod/mod_ssl.html#sslproxycheckpeername.

Additionally, it might be good idea to introduce an ssl_crl 
counterpart for proxy.

> @@ -1332,13 +1350,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);
> 
> +        ngx_http_run_posted_requests(c);
> +
>          return;
>      }
> 
> +fail:
> +    c = r->connection;
> +
>      ngx_http_upstream_next(r, u, NGX_HTTP_UPSTREAM_FT_ERROR);
> 
> +    ngx_http_run_posted_requests(c);
>  }

Nitpicking: I think there should be empty line after "fail:".

BTW, you may want to build patches against mercurial repo, it 
already has the posted requests code here.  See

http://nginx.org/en/docs/contributing_changes.html

for basic tips.

[...]

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