[PATCH] Proxy SSL Verify

Maxim Dounin mdounin at mdounin.ru
Fri Sep 16 13:00:00 UTC 2011


Hello!

On Thu, Sep 15, 2011 at 12:23:28PM -0700, W. Andrew Loe III wrote:

> I have a patch working against nginx 1.1.3. I'm not entirely happy
> with having to set verification_failed on ngx_ssl_connection_t,
> however returning 0 from the verify callback did not stop processing
> as documented (http://www.openssl.org/docs/ssl/SSL_CTX_set_verify.html),
> so I captured and check before the handshake is complete. If anyone
> has an alternative solution that is more elegant I will gladly
> refactor.

[...]

> http://trac.nginx.org/nginx/ticket/13

Just a side note: flooding trac with patches doesn't really help.  
It's not really possible to review patches there.  Just posting 
them here is much better idea, possibly linking ticket to mailing 
list discussions as appropriate.

[...awfully corrupted inline patch skipped, below is one from 
attachment; hope they match...]

> diff --git a/src/event/ngx_event_openssl.c b/src/event/ngx_event_openssl.c
> index 259b1d8..05b49dd 100644
> --- a/src/event/ngx_event_openssl.c
> +++ b/src/event/ngx_event_openssl.c
> @@ -216,13 +216,10 @@ ngx_ssl_certificate(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_str_t *cert,
>      return NGX_OK;
>  }
>  
> -
>  ngx_int_t

Please keep 2 blank lines between functions.  (I know this file 
has recent style corruption after ecdh patch, it will be fixed.)

> -ngx_ssl_client_certificate(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_str_t *cert,
> +ngx_ssl_set_verify_options(ngx_ssl_t *ssl, ngx_str_t *cert,
>      ngx_int_t depth)
>  {
> -    STACK_OF(X509_NAME)  *list;
> -
>      SSL_CTX_set_verify(ssl->ctx, SSL_VERIFY_PEER, ngx_http_ssl_verify_callback);
>  
>      SSL_CTX_set_verify_depth(ssl->ctx, depth);
> @@ -231,10 +228,6 @@ ngx_ssl_client_certificate(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_str_t *cert,
>          return NGX_OK;
>      }
>  
> -    if (ngx_conf_full_name(cf->cycle, cert, 1) != NGX_OK) {
> -        return NGX_ERROR;
> -    }
> -

You actually need this for peer certificate as well.

>      if (SSL_CTX_load_verify_locations(ssl->ctx, (char *) cert->data, NULL)
>          == 0)
>      {
> @@ -244,6 +237,23 @@ ngx_ssl_client_certificate(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_str_t *cert,
>          return NGX_ERROR;
>      }
>  
> +    return NGX_OK;
> +}
> +
> +ngx_int_t
> +ngx_ssl_client_certificate(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_str_t *cert,
> +    ngx_int_t depth)
> +{
> +    STACK_OF(X509_NAME)  *list;
> +
> +    if (ngx_ssl_set_verify_options(ssl, cert, depth) != NGX_OK) {
> +        return NGX_ERROR;
> +    }
> +
> +    if (ngx_conf_full_name(cf->cycle, cert, 1) != NGX_OK) {
> +        return NGX_ERROR;
> +    }
> +
>      list = SSL_load_client_CA_file((char *) cert->data);
>  
>      if (list == NULL) {
> @@ -313,11 +323,6 @@ ngx_ssl_crl(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_str_t *crl)
>  static int
>  ngx_http_ssl_verify_callback(int ok, X509_STORE_CTX *x509_store)
>  {
> -#if (NGX_DEBUG)
> -    char              *subject, *issuer;
> -    int                err, depth;
> -    X509              *cert;
> -    X509_NAME         *sname, *iname;
>      ngx_connection_t  *c;
>      ngx_ssl_conn_t    *ssl_conn;
>  
> @@ -326,6 +331,12 @@ ngx_http_ssl_verify_callback(int ok, X509_STORE_CTX *x509_store)
>  
>      c = ngx_ssl_get_connection(ssl_conn);
>  
> +#if (NGX_DEBUG)
> +    char              *subject, *issuer;
> +    int                err, depth;
> +    X509              *cert;
> +    X509_NAME         *sname, *iname;
> +
>      cert = X509_STORE_CTX_get_current_cert(x509_store);
>      err = X509_STORE_CTX_get_error(x509_store);
>      depth = X509_STORE_CTX_get_error_depth(x509_store);
> @@ -350,6 +361,13 @@ ngx_http_ssl_verify_callback(int ok, X509_STORE_CTX *x509_store)
>      }
>  #endif
>  
> +    if (ok != 1)
> +    {
> +        ngx_ssl_error(NGX_LOG_EMERG, c->log, 0, "ngx_http_ssl_verify_callback failed");
> +        c->ssl->verification_failed = 1;
> +        return 0;
> +    }
> +
>      return 1;
>  }
>  
> @@ -575,6 +593,11 @@ ngx_ssl_handshake(ngx_connection_t *c)
>  
>      ngx_log_debug1(NGX_LOG_DEBUG_EVENT, c->log, 0, "SSL_do_handshake: %d", n);
>  
> +    if (c->ssl->verification_failed != NGX_OK)
> +    {
> +      return NGX_ERROR;
> +    }
> +
>      if (n == 1) {
>  
>          if (ngx_handle_read_event(c->read, 0) != NGX_OK) {

You may want to avoid touching ngx_http_ssl_verify_callback() and 
ngx_ssl_handshake().  This will break client cert processing.

In normal https this is checked in ngx_http_process_request().  
Appropriate checks should be done in upstream case as well.

And you anyway need verify checks in upstream code as checking CN 
will require it.

Please also note that code code here have multiple style issues 
("{" should be on line with "if" unless it's multiline, wrong 
indentation).  This is irrelevant as code is wrong anyway, noted 
just to make sure coding style in next iteration will be better.  

> diff --git a/src/event/ngx_event_openssl.h b/src/event/ngx_event_openssl.h
> index 33cab7b..b59baf9 100644
> --- a/src/event/ngx_event_openssl.h
> +++ b/src/event/ngx_event_openssl.h
> @@ -46,6 +46,8 @@ typedef struct {
>      unsigned                    buffer:1;
>      unsigned                    no_wait_shutdown:1;
>      unsigned                    no_send_shutdown:1;
> +
> +    ngx_int_t                   verification_failed;
>  } ngx_ssl_connection_t;

No, please.  See above.

>  
>  
> @@ -96,6 +98,8 @@ ngx_int_t ngx_ssl_init(ngx_log_t *log);
>  ngx_int_t ngx_ssl_create(ngx_ssl_t *ssl, ngx_uint_t protocols, void *data);
>  ngx_int_t ngx_ssl_certificate(ngx_conf_t *cf, ngx_ssl_t *ssl,
>      ngx_str_t *cert, ngx_str_t *key);
> +ngx_int_t ngx_ssl_set_verify_options(ngx_ssl_t *ssl, ngx_str_t *cert,
> +    ngx_int_t depth);
>  ngx_int_t ngx_ssl_client_certificate(ngx_conf_t *cf, ngx_ssl_t *ssl,
>      ngx_str_t *cert, ngx_int_t depth);
>  ngx_int_t ngx_ssl_crl(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_str_t *crl);
> diff --git a/src/http/modules/ngx_http_proxy_module.c b/src/http/modules/ngx_http_proxy_module.c
> index 902cfb8..834301e 100644
> --- a/src/http/modules/ngx_http_proxy_module.c
> +++ b/src/http/modules/ngx_http_proxy_module.c
> @@ -440,7 +440,27 @@ static ngx_command_t  ngx_http_proxy_commands[] = {
>        NGX_HTTP_LOC_CONF_OFFSET,
>        offsetof(ngx_http_proxy_loc_conf_t, upstream.ssl_session_reuse),
>        NULL },
> +    
> +    { ngx_string("proxy_ssl_verify_peer"),
> +      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_peer),
> +      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_ca_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_ca_certificate),
> +      NULL },

As I already said, you may want to call this 
proxy_ssl_peer_certificate to make

    proxy_ssl_verify_peer
    proxy_ssl_peer_certificate

in line with

    ssl_verify_client
    ssl_client_certificate

>  #endif
>  
>        ngx_null_command
> @@ -1697,6 +1717,8 @@ ngx_http_proxy_create_loc_conf(ngx_conf_t *cf)
>      conf->upstream.intercept_errors = NGX_CONF_UNSET;
>  #if (NGX_HTTP_SSL)
>      conf->upstream.ssl_session_reuse = NGX_CONF_UNSET;
> +    conf->upstream.ssl_verify_peer = NGX_CONF_UNSET;
> +    conf->upstream.ssl_verify_depth = NGX_CONF_UNSET_UINT;
>  #endif
>  
>      /* "proxy_cyclic_temp_file" is disabled */
> @@ -1955,6 +1977,22 @@ ngx_http_proxy_merge_loc_conf(ngx_conf_t *cf, void *parent, void *child)
>  #if (NGX_HTTP_SSL)
>      ngx_conf_merge_value(conf->upstream.ssl_session_reuse,
>                                prev->upstream.ssl_session_reuse, 1);
> +    ngx_conf_merge_value(conf->upstream.ssl_verify_peer,
> +                              prev->upstream.ssl_verify_peer, 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_ca_certificate,
> +                              prev->upstream.ssl_ca_certificate, "");
> +
> +    if (conf->upstream.ssl_verify_peer) {
> +      if (conf->upstream.ssl_ca_certificate.len == 0) {
> +        ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
> +            "no \"proxy_ssl_ca_certificate\" is defined for "
> +            "the \"proxy_ssl_verify_peer\" directive");
> +
> +        return NGX_CONF_ERROR;
> +      }
> +    }

As I already said, no 2 spaces indentation, please.

>  #endif
>  
>      ngx_conf_merge_value(conf->redirect, prev->redirect, 1);
> diff --git a/src/http/ngx_http_upstream.c b/src/http/ngx_http_upstream.c
> index 29432dc..474cf0d 100644
> --- a/src/http/ngx_http_upstream.c
> +++ b/src/http/ngx_http_upstream.c
> @@ -1210,6 +1210,15 @@ ngx_http_upstream_ssl_init_connection(ngx_http_request_t *r,
>  {
>      ngx_int_t   rc;
>  
> +    if (ngx_ssl_set_verify_options(u->conf->ssl,
> +          &u->conf->ssl_ca_certificate, u->conf->ssl_verify_depth)
> +        != NGX_OK)
> +    {
> +      ngx_http_upstream_finalize_request(r, u,
> +          NGX_HTTP_INTERNAL_SERVER_ERROR);
> +      return;
> +    }
> +

You want this to be set during config parsing, not at runtime.

>      if (ngx_ssl_create_connection(u->conf->ssl, c,
>                                    NGX_SSL_BUFFER|NGX_SSL_CLIENT)
>          != NGX_OK)
> @@ -4527,3 +4536,4 @@ ngx_http_upstream_init_main_conf(ngx_conf_t *cf, void *conf)
>  
>      return NGX_CONF_OK;
>  }
> +

Unrelated whitespace change.

> diff --git a/src/http/ngx_http_upstream.h b/src/http/ngx_http_upstream.h
> index fa848c0..cc71ba9 100644
> --- a/src/http/ngx_http_upstream.h
> +++ b/src/http/ngx_http_upstream.h
> @@ -177,6 +177,9 @@ typedef struct {
>  #if (NGX_HTTP_SSL)
>      ngx_ssl_t                       *ssl;
>      ngx_flag_t                       ssl_session_reuse;
> +    ngx_flag_t                       ssl_verify_peer;
> +    ngx_uint_t                       ssl_verify_depth;
> +    ngx_str_t                        ssl_ca_certificate;
>  #endif
>  
>      ngx_str_t                        module;

Maxim Dounin



More information about the nginx-devel mailing list