Re: [PATCH] Proxy: add "proxy_ssl_alpn" directive
Hello!
On Mon, Jun 19, 2017 at 07:31:11AM -0700, Piotr Sikora via nginx-devel wrote:
> Hey Maxim,
>
> > It doesn't look like this patch make sense by its own.
> >
> > If this patch is a part of a larger work, please consider
> > submitting a patch series with the whole feature instead.
> > Individual patches which doesn't make much sense
> > by its own are hard to review, and are likely to be rejected.
>
> Actually, it does, the only missing part (from HTTP/2 patchset) is:
>
> case NGX_HTTP_VERSION_11:
> ngx_str_set(&alpn, NGX_HTTP_11_ALPN_ADVERTISE);
> break;
> +
> +#if (NGX_HTTP_V2)
> +case NGX_HTTP_VERSION_20:
> +ngx_str_set(&alpn, NGX_HTTP_V2_ALPN_ADVERTISE);
> +break;
> +#endif
> }
>
> if (ngx_ssl_alpn_protos(cf, plcf->upstream.ssl, &alpn) != NGX_OK) {
>
> ...so this patch is perfectly reviewable on its own.
>
> In case you expected ALPN-negotiation - I didn't add it, since it
> would require rewrite of upstream logic, i.e. u->create_request()
> would need to be called after upstream is already connected, and
> possibly again in case of retries that negotiated different ALPN
> protocol, which would add complexity to the already big patchset.
> Also, I'm not sure how useful this is for upstream connections in
> reverse proxies.
>
> Let me know if that's a must-have, but IMHO we could always add
> "proxy_http_version alpn" in the future, without blocking this and
> HTTP/2 patchset, which effectively implement "prior knowledge".
I don't see how this patch is useful by its own, without
additional HTTP/2 patchset, and it clearly adds additional
user-visible complexity to the proxy module. And hence the
suggestion is to include this patch into the HTTP/2 patchset as it
is clearly a part of this patchset.
Note well that at this point it might be a good idea to implement
HTTP/2 to upstreams as a separate module, as it is done for other
protocols like FastCGI, SCGI, uWSGI, and so on, instead of trying
to built it into the proxy module which is intended to talk HTTP,
not HTTP/2. It will better follow the generic concept we use now,
"one protocol - one module", and will also imply much less
blocking in general.
--
Maxim Dounin
http://nginx.org/
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] Proxy: add "proxy_ssl_alpn" directive
Hey Maxim,
> It doesn't look like this patch make sense by its own.
>
> If this patch is a part of a larger work, please consider
> submitting a patch series with the whole feature instead.
> Individual patches which doesn't make much sense
> by its own are hard to review, and are likely to be rejected.
Actually, it does, the only missing part (from HTTP/2 patchset) is:
case NGX_HTTP_VERSION_11:
ngx_str_set(&alpn, NGX_HTTP_11_ALPN_ADVERTISE);
break;
+
+#if (NGX_HTTP_V2)
+case NGX_HTTP_VERSION_20:
+ngx_str_set(&alpn, NGX_HTTP_V2_ALPN_ADVERTISE);
+break;
+#endif
}
if (ngx_ssl_alpn_protos(cf, plcf->upstream.ssl, &alpn) != NGX_OK) {
...so this patch is perfectly reviewable on its own.
In case you expected ALPN-negotiation - I didn't add it, since it
would require rewrite of upstream logic, i.e. u->create_request()
would need to be called after upstream is already connected, and
possibly again in case of retries that negotiated different ALPN
protocol, which would add complexity to the already big patchset.
Also, I'm not sure how useful this is for upstream connections in
reverse proxies.
Let me know if that's a must-have, but IMHO we could always add
"proxy_http_version alpn" in the future, without blocking this and
HTTP/2 patchset, which effectively implement "prior knowledge".
Best regards,
Piotr Sikora
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] Proxy: add "proxy_ssl_alpn" directive
Hello! On Sat, Jun 03, 2017 at 08:04:02PM -0700, Piotr Sikora via nginx-devel wrote: > # HG changeset patch > # User Piotr Sikora > # Date 1489621682 25200 > # Wed Mar 15 16:48:02 2017 -0700 > # Node ID 7733d946e2651a2486a53d912703e2dfaea30421 > # Parent 716852cce9136d977b81a2d1b8b6f9fbca0dce49 > Proxy: add "proxy_ssl_alpn" directive. > > ALPN is used here only to indicate which version of the HTTP protocol > is going to be used and we doesn't verify that upstream agreed to it. > > Please note that upstream is allowed to reject SSL connection with a > fatal "no_application_protocol" alert if it doesn't support it. > > Signed-off-by: Piotr Sikora It doesn't look like this patch make sense by its own. If this patch is a part of a larger work, please consider submitting a patch series with the whole feature instead. Individual patches which doesn't make much sense by its own are hard to review, and are likely to be rejected. -- Maxim Dounin http://nginx.org/ ___ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel
[PATCH] Proxy: add "proxy_ssl_alpn" directive
# HG changeset patch
# User Piotr Sikora
# Date 1489621682 25200
# Wed Mar 15 16:48:02 2017 -0700
# Node ID 7733d946e2651a2486a53d912703e2dfaea30421
# Parent 716852cce9136d977b81a2d1b8b6f9fbca0dce49
Proxy: add "proxy_ssl_alpn" directive.
ALPN is used here only to indicate which version of the HTTP protocol
is going to be used and we doesn't verify that upstream agreed to it.
Please note that upstream is allowed to reject SSL connection with a
fatal "no_application_protocol" alert if it doesn't support it.
Signed-off-by: Piotr Sikora
diff -r 716852cce913 -r 7733d946e265 src/event/ngx_event_openssl.c
--- a/src/event/ngx_event_openssl.c
+++ b/src/event/ngx_event_openssl.c
@@ -654,6 +654,29 @@ ngx_ssl_ciphers(ngx_conf_t *cf, ngx_ssl_
ngx_int_t
+ngx_ssl_alpn_protos(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_str_t *protos)
+{
+#ifdef TLSEXT_TYPE_application_layer_protocol_negotiation
+
+if (SSL_CTX_set_alpn_protos(ssl->ctx, protos->data, protos->len) != 0) {
+ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0,
+ "SSL_CTX_set_alpn_protos() failed");
+return NGX_ERROR;
+}
+
+return NGX_OK;
+
+#else
+
+ngx_log_error(NGX_LOG_EMERG, cf->log, 0,
+ "nginx was built with OpenSSL that lacks ALPN support");
+return NGX_ERROR;
+
+#endif
+}
+
+
+ngx_int_t
ngx_ssl_client_certificate(ngx_conf_t *cf, ngx_ssl_t *ssl, ngx_str_t *cert,
ngx_int_t depth)
{
diff -r 716852cce913 -r 7733d946e265 src/event/ngx_event_openssl.h
--- a/src/event/ngx_event_openssl.h
+++ b/src/event/ngx_event_openssl.h
@@ -153,6 +153,8 @@ ngx_int_t ngx_ssl_certificate(ngx_conf_t
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_alpn_protos(ngx_conf_t *cf, ngx_ssl_t *ssl,
+ngx_str_t *protos);
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_trusted_certificate(ngx_conf_t *cf, ngx_ssl_t *ssl,
diff -r 716852cce913 -r 7733d946e265 src/http/modules/ngx_http_proxy_module.c
--- a/src/http/modules/ngx_http_proxy_module.c
+++ b/src/http/modules/ngx_http_proxy_module.c
@@ -652,6 +652,13 @@ static ngx_command_t ngx_http_proxy_com
offsetof(ngx_http_proxy_loc_conf_t, ssl_ciphers),
NULL },
+{ ngx_string("proxy_ssl_alpn"),
+ NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_FLAG,
+ ngx_conf_set_flag_slot,
+ NGX_HTTP_LOC_CONF_OFFSET,
+ offsetof(ngx_http_proxy_loc_conf_t, upstream.ssl_alpn),
+ NULL },
+
{ ngx_string("proxy_ssl_name"),
NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE1,
ngx_http_set_complex_value_slot,
@@ -2882,6 +2889,7 @@ ngx_http_proxy_create_loc_conf(ngx_conf_
conf->upstream.intercept_errors = NGX_CONF_UNSET;
#if (NGX_HTTP_SSL)
+conf->upstream.ssl_alpn = NGX_CONF_UNSET;
conf->upstream.ssl_session_reuse = NGX_CONF_UNSET;
conf->upstream.ssl_server_name = NGX_CONF_UNSET;
conf->upstream.ssl_verify = NGX_CONF_UNSET;
@@ -3212,6 +3220,8 @@ ngx_http_proxy_merge_loc_conf(ngx_conf_t
conf->upstream.ssl_name = prev->upstream.ssl_name;
}
+ngx_conf_merge_value(conf->upstream.ssl_alpn,
+ prev->upstream.ssl_alpn, 0);
ngx_conf_merge_value(conf->upstream.ssl_server_name,
prev->upstream.ssl_server_name, 0);
ngx_conf_merge_value(conf->upstream.ssl_verify,
@@ -4320,6 +4330,7 @@ ngx_http_proxy_lowat_check(ngx_conf_t *c
static ngx_int_t
ngx_http_proxy_set_ssl(ngx_conf_t *cf, ngx_http_proxy_loc_conf_t *plcf)
{
+ngx_str_talpn;
ngx_pool_cleanup_t *cln;
plcf->upstream.ssl = ngx_pcalloc(cf->pool, sizeof(ngx_ssl_t));
@@ -4366,6 +4377,24 @@ ngx_http_proxy_set_ssl(ngx_conf_t *cf, n
return NGX_ERROR;
}
+if (plcf->upstream.ssl_alpn) {
+
+switch (plcf->http_version) {
+
+case NGX_HTTP_VERSION_10:
+ngx_str_set(&alpn, NGX_HTTP_10_ALPN_ADVERTISE);
+break;
+
+case NGX_HTTP_VERSION_11:
+ngx_str_set(&alpn, NGX_HTTP_11_ALPN_ADVERTISE);
+break;
+}
+
+if (ngx_ssl_alpn_protos(cf, plcf->upstream.ssl, &alpn) != NGX_OK) {
+return NGX_ERROR;
+}
+}
+
if (plcf->upstream.ssl_verify) {
if (plcf->ssl_trusted_certificate.len == 0) {
ngx_log_error(NGX_LOG_EMERG, cf->log, 0,
diff -r 716852cce913 -r 7733d946e265 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
@@ -17,8 +17,6 @@ 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
stat
