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

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