Hello!

On Mon, Jun 20, 2016 at 09:59:55AM +0200, Tim Taubert wrote:

> # HG changeset patch
> # User Tim Taubert <tim@timtaubert.de>
> # Date 1466409485 -7200
> # Mon Jun 20 09:58:05 2016 +0200
> # Node ID 1955931e69e166e26e6e4b7655695810c58a22c8
> # Parent 2c7b488a61fbbd36054bc7410c161ce73b7624b9
> SSL: ngx_ssl_protocol_negotiation() to set up ALPN/NPN handling
>
> This patch adds ngx_ssl_protocol_negotiation() to be called to
> set up ALPN/NPN handling for the respective cryptography library
> used to implement TLS support.

See various comments below. Overral, the patch doesn't look
working.

>
> diff --git a/src/event/ngx_event_openssl.c b/src/event/ngx_event_openssl.c
> --- a/src/event/ngx_event_openssl.c
> +++ b/src/event/ngx_event_openssl.c
> @@ -13,16 +13,27 @@
> #define NGX_SSL_PASSWORD_BUFFER_SIZE 4096
>
>
> typedef struct {
> ngx_uint_t engine; /* unsigned engine:1; */
> } ngx_openssl_conf_t;
>
>
> +#ifdef TLSEXT_TYPE_application_layer_protocol_negotiation
> +static int ngx_ssl_alpn_select(ngx_ssl_conn_t *ssl_conn,
> + const unsigned char **out, unsigned char *outlen, const unsigned char *in,
> + unsigned int inlen, void *arg);
> +#endif
> +
> +#ifdef TLSEXT_TYPE_next_proto_neg
> +static int ngx_ssl_npn_advertised(ngx_ssl_conn_t *ssl_conn,
> + const unsigned char **out, unsigned int *outlen, void *arg);
> +#endif
> +
> static int ngx_ssl_password_callback(char *buf, int size, int rwflag,
> void *userdata);
> static int ngx_ssl_verify_callback(int ok, X509_STORE_CTX *x509_store);
> static void ngx_ssl_info_callback(const ngx_ssl_conn_t *ssl_conn, int where,
> int ret);
> static void ngx_ssl_passwords_cleanup(void *data);
> static void ngx_ssl_handshake_handler(ngx_event_t *ev);
> static ngx_int_t ngx_ssl_handle_recv(ngx_connection_t *c, int n);
> @@ -317,16 +328,101 @@ ngx_ssl_create(ngx_ssl_t *ssl, ngx_uint_
>
> SSL_CTX_set_info_callback(ssl->ctx, ngx_ssl_info_callback);
>
> return NGX_OK;
> }
>
>
> ngx_int_t
> +ngx_ssl_protocol_negotiation(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_str_t *data)

ALPN/NPN is just one of aspects of SSL, and relatively minor one.
Placing this right after ngx_ssl_create() looks wrong for me.

The interface suggested doesn't look generic: it uses
OpenSSL-specific wire-format way to specify supported protocols.
E.g., GnuTLS uses an array of protocol names instead.
Additionally, such an interface can't work as we don't know in
advance if HTTP/2 will be allowed on a listening socket or not,
see below.

The argument name "data" looks too generic. (And the name of the
function can be better, too.)

> +{
> + // Store protocols to offer.

No C99-style comments, please.

> + ssl->proto_neg.len = data->len;
> + ssl->proto_neg.data = ngx_pstrdup(cf->pool, data);
> + if (ssl->proto_neg.data == NULL) {
> + return NGX_ERROR;
> + }

Why ngx_pstrdup() is needed here?

> +
> +#ifdef TLSEXT_TYPE_application_layer_protocol_negotiation
> + SSL_CTX_set_alpn_select_cb(ssl->ctx, ngx_ssl_alpn_select, &ssl->proto_neg);
> +#endif
> +
> +#ifdef TLSEXT_TYPE_next_proto_neg
> + SSL_CTX_set_next_protos_advertised_cb(ssl->ctx, ngx_ssl_npn_advertised,
> + &ssl->proto_neg);
> +#endif
> +
> + return NGX_OK;
> +}
> +
> +
> +#ifdef TLSEXT_TYPE_application_layer_protocol_negotiation
> +
> +static int
> +ngx_ssl_alpn_select(ngx_ssl_conn_t *ssl_conn, const unsigned char **out,
> + unsigned char *outlen, const unsigned char *in, unsigned int inlen,
> + void *arg)
> +{
> + ngx_str_t *proto_neg = arg;
> +
> +#if (NGX_DEBUG)
> + unsigned int i;
> + ngx_connection_t *c = ngx_ssl_get_connection(ssl_conn);

Indentation of variables here looks wrong. General rule is "2
spaces after the longest type".

Please don't use variable definition and initialization at the
same time (except in a few very specific cases).

> +
> + for (i = 0; i < inlen; i += in[i] + 1) {
> + ngx_log_debug2(NGX_LOG_DEBUG_HTTP, c->log, 0,

NGX_LOG_DEBUG_HTTP is wrong here and in other places.

> + "SSL ALPN supported by client: %*s",
> + (size_t) in[i], &in[i + 1]);
> + }
> +#endif
> +
> + if (SSL_select_next_proto((unsigned char **) out, outlen, proto_neg->data,
> + proto_neg->len, in, inlen)
> + != OPENSSL_NPN_NEGOTIATED)
> + {
> + return SSL_TLSEXT_ERR_NOACK;
> + }
> +
> +#if (NGX_DEBUG)
> + ngx_log_debug2(NGX_LOG_DEBUG_HTTP, c->log, 0,
> + "SSL ALPN selected: %*s", (size_t) *outlen, *out);
> +#endif
> +
> + return SSL_TLSEXT_ERR_OK;
> +}
> +
> +#endif
> +
> +
> +#ifdef TLSEXT_TYPE_next_proto_neg
> +
> +static int
> +ngx_ssl_npn_advertised(ngx_ssl_conn_t *ssl_conn,
> + const unsigned char **out, unsigned int *outlen, void *arg)
> +{
> + ngx_str_t *proto_neg = arg;
> +
> +#if (NGX_DEBUG)
> + ngx_connection_t *c;
> +
> + c = ngx_ssl_get_connection(ssl_conn);
> + ngx_log_debug0(NGX_LOG_DEBUG_HTTP, c->log, 0, "SSL NPN advertised");
> +#endif
> +
> + *out = proto_neg->data;
> + *outlen = proto_neg->len;
> +
> + return SSL_TLSEXT_ERR_OK;
> +}
> +
> +#endif
> +
> +
> +ngx_int_t
> ngx_ssl_certificates(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_array_t *certs,
> ngx_array_t *keys, ngx_array_t *passwords)
> {
> ngx_str_t *cert, *key;
> ngx_uint_t i;
>
> cert = certs->elts;
> key = keys->elts;
> diff --git a/src/event/ngx_event_openssl.h b/src/event/ngx_event_openssl.h
> --- a/src/event/ngx_event_openssl.h
> +++ b/src/event/ngx_event_openssl.h
> @@ -53,16 +53,17 @@
> #define ngx_ssl_session_t SSL_SESSION
> #define ngx_ssl_conn_t SSL
>
>
> typedef struct {
> SSL_CTX *ctx;
> ngx_log_t *log;
> size_t buffer_size;
> + ngx_str_t proto_neg;

I can't say I like the name. Also it's not clear why this field
is at all needed.

> } ngx_ssl_t;
>
>
> typedef struct {
> ngx_ssl_conn_t *connection;
> SSL_CTX *session_ctx;
>
> ngx_int_t last;
> @@ -135,16 +136,18 @@ typedef struct {
> #define NGX_SSL_BUFFER 1
> #define NGX_SSL_CLIENT 2
>
> #define NGX_SSL_BUFSIZE 16384
>
>
> 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_protocol_negotiation(ngx_conf_t *cf, ngx_ssl_t *ssl,
> + ngx_str_t *data);
> ngx_int_t ngx_ssl_certificates(ngx_conf_t *cf, ngx_ssl_t *ssl,
> ngx_array_t *certs, ngx_array_t *keys, ngx_array_t *passwords);
> ngx_int_t ngx_ssl_certificate(ngx_conf_t *cf, ngx_ssl_t *ssl,
> ngx_str_t *cert, ngx_str_t *key, ngx_array_t *passwords);
> ngx_int_t ngx_ssl_ciphers(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_str_t *ciphers,
> ngx_uint_t prefer_server_ciphers);
> ngx_int_t ngx_ssl_client_certificate(ngx_conf_t *cf, ngx_ssl_t *ssl,
> ngx_str_t *cert, ngx_int_t depth);
> diff --git a/src/http/modules/ngx_http_ssl_module.c b/src/http/modules/ngx_http_ssl_module.c
> --- a/src/http/modules/ngx_http_ssl_module.c
> +++ b/src/http/modules/ngx_http_ssl_module.c
> @@ -15,27 +15,16 @@ typedef ngx_int_t (*ngx_ssl_variable_han
>
>
> #define NGX_DEFAULT_CIPHERS "HIGH:!aNULL:!MD5"
> #define NGX_DEFAULT_ECDH_CURVE "auto"
>
> #define NGX_HTTP_NPN_ADVERTISE "\x08http/1.1"
>
>
> -#ifdef TLSEXT_TYPE_application_layer_protocol_negotiation
> -static int ngx_http_ssl_alpn_select(ngx_ssl_conn_t *ssl_conn,
> - const unsigned char **out, unsigned char *outlen,
> - const unsigned char *in, unsigned int inlen, void *arg);
> -#endif
> -
> -#ifdef TLSEXT_TYPE_next_proto_neg
> -static int ngx_http_ssl_npn_advertised(ngx_ssl_conn_t *ssl_conn,
> - const unsigned char **out, unsigned int *outlen, void *arg);
> -#endif
> -
> static ngx_int_t ngx_http_ssl_static_variable(ngx_http_request_t *r,
> ngx_http_variable_value_t *v, uintptr_t data);
> static ngx_int_t ngx_http_ssl_variable(ngx_http_request_t *r,
> ngx_http_variable_value_t *v, uintptr_t data);
>
> static ngx_int_t ngx_http_ssl_add_variables(ngx_conf_t *cf);
> static void *ngx_http_ssl_create_srv_conf(ngx_conf_t *cf);
> static char *ngx_http_ssl_merge_srv_conf(ngx_conf_t *cf,
> @@ -308,113 +297,21 @@ static ngx_http_variable_t ngx_http_ssl
> (uintptr_t) ngx_ssl_get_client_verify, NGX_HTTP_VAR_CHANGEABLE, 0 },
>
> { ngx_null_string, NULL, NULL, 0, 0, 0 }
> };
>
>
> static ngx_str_t ngx_http_ssl_sess_id_ctx = ngx_string("HTTP");
>
> -
> -#ifdef TLSEXT_TYPE_application_layer_protocol_negotiation
> -
> -static int
> -ngx_http_ssl_alpn_select(ngx_ssl_conn_t *ssl_conn, const unsigned char **out,
> - unsigned char *outlen, const unsigned char *in, unsigned int inlen,
> - void *arg)
> -{
> - unsigned int srvlen;
> - unsigned char *srv;
> -#if (NGX_DEBUG)
> - unsigned int i;
> -#endif
> #if (NGX_HTTP_V2)
> - ngx_http_connection_t *hc;
> -#endif
> -#if (NGX_HTTP_V2 || NGX_DEBUG)
> - ngx_connection_t *c;
> -
> - c = ngx_ssl_get_connection(ssl_conn);
> -#endif
> -
> -#if (NGX_DEBUG)
> - for (i = 0; i < inlen; i += in[i] + 1) {
> - ngx_log_debug2(NGX_LOG_DEBUG_HTTP, c->log, 0,
> - "SSL ALPN supported by client: %*s",
> - (size_t) in[i], &in[i + 1]);
> - }
> -#endif
> -
> -#if (NGX_HTTP_V2)
> - hc = c->data;
> -
> - if (hc->addr_conf->http2) {
> - srv =
> - (unsigned char *) NGX_HTTP_V2_ALPN_ADVERTISE NGX_HTTP_NPN_ADVERTISE;
> - srvlen = sizeof(NGX_HTTP_V2_ALPN_ADVERTISE NGX_HTTP_NPN_ADVERTISE) - 1;
> -
> - } else
> +static ngx_str_t ngx_http_ssl_proto_neg_h2 = ngx_string(
> + NGX_HTTP_V2_ALPN_ADVERTISE NGX_HTTP_NPN_ADVERTISE);
> #endif
> - {
> - srv = (unsigned char *) NGX_HTTP_NPN_ADVERTISE;
> - srvlen = sizeof(NGX_HTTP_NPN_ADVERTISE) - 1;
> - }
> -
> - if (SSL_select_next_proto((unsigned char **) out, outlen, srv, srvlen,
> - in, inlen)
> - != OPENSSL_NPN_NEGOTIATED)
> - {
> - return SSL_TLSEXT_ERR_NOACK;
> - }
> -
> - ngx_log_debug2(NGX_LOG_DEBUG_HTTP, c->log, 0,
> - "SSL ALPN selected: %*s", (size_t) *outlen, *out);
> -
> - return SSL_TLSEXT_ERR_OK;
> -}
> -
> -#endif
> -
> -
> -#ifdef TLSEXT_TYPE_next_proto_neg
> -
> -static int
> -ngx_http_ssl_npn_advertised(ngx_ssl_conn_t *ssl_conn,
> - const unsigned char **out, unsigned int *outlen, void *arg)
> -{
> -#if (NGX_HTTP_V2 || NGX_DEBUG)
> - ngx_connection_t *c;
> -
> - c = ngx_ssl_get_connection(ssl_conn);
> - ngx_log_debug0(NGX_LOG_DEBUG_HTTP, c->log, 0, "SSL NPN advertised");
> -#endif
> -
> -#if (NGX_HTTP_V2)
> - {
> - ngx_http_connection_t *hc;
> -
> - hc = c->data;
> -
> - if (hc->addr_conf->http2) {
> - *out =
> - (unsigned char *) NGX_HTTP_V2_NPN_ADVERTISE NGX_HTTP_NPN_ADVERTISE;
> - *outlen = sizeof(NGX_HTTP_V2_NPN_ADVERTISE NGX_HTTP_NPN_ADVERTISE) - 1;
> -
> - return SSL_TLSEXT_ERR_OK;
> - }
> - }
> -#endif
> -
> - *out = (unsigned char *) NGX_HTTP_NPN_ADVERTISE;
> - *outlen = sizeof(NGX_HTTP_NPN_ADVERTISE) - 1;
> -
> - return SSL_TLSEXT_ERR_OK;
> -}
> -
> -#endif
> +static ngx_str_t ngx_http_ssl_proto_neg_h1 = ngx_string(NGX_HTTP_NPN_ADVERTISE);

Defining C strings and then defining ngx_str_t using these strings
looks wierd. It may be a better idea to just define appropriate
strings here, much like it's done with ngx_http_ssl_sess_id_ctx.

>
>
> static ngx_int_t
> ngx_http_ssl_static_variable(ngx_http_request_t *r,
> ngx_http_variable_value_t *v, uintptr_t data)
> {
> ngx_ssl_variable_handler_pt handler = (ngx_ssl_variable_handler_pt) data;
>
> @@ -536,16 +433,17 @@ ngx_http_ssl_create_srv_conf(ngx_conf_t
>
> return sscf;
> }
>
>
> static char *
> ngx_http_ssl_merge_srv_conf(ngx_conf_t *cf, void *parent, void *child)
> {
> + ngx_str_t *proto_neg = &ngx_http_ssl_proto_neg_h1;
> ngx_http_ssl_srv_conf_t *prev = parent;
> ngx_http_ssl_srv_conf_t *conf = child;
>
> ngx_pool_cleanup_t *cln;

This looks wrong at least from style point of view. The prev/conf
case is of very specific cases when variables can be declared and
initialized at the same time. All other variables are expected to
be defined after an empty line (see "cln"), and initialized
separately.

>
> if (conf->enable == NGX_CONF_UNSET) {
> if (prev->enable == NGX_CONF_UNSET) {
> conf->enable = 0;
> @@ -660,24 +558,27 @@ ngx_http_ssl_merge_srv_conf(ngx_conf_t *
> ngx_log_error(NGX_LOG_WARN, cf->log, 0,
> "nginx was built with SNI support, however, now it is linked "
> "dynamically to an OpenSSL library which has no tlsext support, "
> "therefore SNI is not available");
> }
>
> #endif
>
> -#ifdef TLSEXT_TYPE_application_layer_protocol_negotiation
> - SSL_CTX_set_alpn_select_cb(conf->ssl.ctx, ngx_http_ssl_alpn_select, NULL);
> -#endif
> +#if (NGX_HTTP_V2)
> + {
> + ngx_connection_t *c = ngx_ssl_get_connection(conf->ssl.ctx);
> + ngx_http_connection_t *hc = c->data;
>
> -#ifdef TLSEXT_TYPE_next_proto_neg
> - SSL_CTX_set_next_protos_advertised_cb(conf->ssl.ctx,
> - ngx_http_ssl_npn_advertised, NULL);
> + if (hc->addr_conf->http2) {
> + proto_neg = &ngx_http_ssl_proto_neg_h2;
> + }
> + }

This looks completely wrong: no connection is expected to be
available during configuration merging. And compilation fails
accordingly:

src/http/modules/ngx_http_ssl_module.c:568:54: error: incompatible pointer types
passing 'SSL_CTX *' (aka 'struct ssl_ctx_st *') to parameter of type
'const SSL *' (aka 'const struct ssl_st *')
[-Werror,-Wincompatible-pointer-types]
ngx_connection_t *c = ngx_ssl_get_connection(conf->ssl.ctx);
^~~~~~~~~~~~~

> #endif
> + ngx_ssl_protocol_negotiation(cf, &conf->ssl, proto_neg);
>
> cln = ngx_pool_cleanup_add(cf->pool, 0);
> if (cln == NULL) {
> return NGX_CONF_ERROR;
> }
>
> cln->handler = ngx_ssl_cleanup_ctx;
> cln->data = &conf->ssl;
>
> _______________________________________________
> nginx-devel mailing list
> nginx-devel@nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel

--
Maxim Dounin
http://nginx.org/

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