Hello!

On Sat, Jun 25, 2022 at 01:02:21AM +0000, Pavel Pautov via nginx-devel wrote:

> > -----Original Message-----
> > From: Maxim Dounin <mdounin@mdounin.ru>
> > Sent: Thursday, June 16, 2022 18:51
> >
> > Hello!
> >
> > On Thu, Jun 16, 2022 at 08:26:48AM +0000, Pavel Pautov via nginx-devel wrote:
> >
> > > Looks like, we've made a full circle then... I've replied to
> > > that suggestion already and in last e-mail (with patch) I note
> > > that moving additional logic into the ngx_http_proxy_set_ssl()
> > > has its own drawbacks, but I can certainly move more stuff into
> > > it.
> > >
> > > So do you envision something like "ngx_http_proxy_set_ssl(cf,
> > > conf, prev, reuse_ssl)"? As previously we've established that
> > > directives merging stays out of ngx_http_proxy_set_ssl (and
> > > reuse_ssl calculation has to happen before it).
> >
> > I don't think further attempts to explain how to write readable
> > code worth the effort.
>
> Too bad.

Yes, that's unfortunate. You may want to consider asking more
experienced developers in your company for mentorship.

> > Please see the patch below. Review and testing appreciated.
>
> The patch seems to contradict some previously discussed points.
> For example, it can actually increase memory usage in certain
> configurations (say, when proxy_ssl_* directives in location
> override server level directives or when proxy_pass
> "https://..." is absent). Or you don't consider this an issue
> anymore?

Quoting the very first review:

: Also, it should be a good idea to avoid creating SSL contexts if
: there is no SSL proxying configured. Or, at very least, make sure
: only one context at the http level is used in such cases, so
: configurations with many servers won't suddenly blow up.

The patch tries to be in line with this, and create at most one
proxy SSL context as long as no SSL proxying is configured (it
fails to do so though, see below). If proxy SSL settings are
redefined at some level, this redefinition adds an SSL context,
and this behaviour is something one might expect from the explicit
proxy_ssl_* directives in the configuration.

> More importantly, it doesn't really solve the use case from
> #1234, i.e. http level proxy_ssl_* directives with many servers
> (as by default http level values are remain unset and thus are
> not equal to server level values).

My bad, mostly focused on the location level and missed that it is
not possible to properly compare http and server level settings
after the merge.

Updated patch below, with additional function called just before
the merge of particular directives. The resulting codes tries to
be close to ngx_http_proxy_set_ssl(), where the relevant settings
are used to create SSL contexts, to be self-explanatory.

Additionally, the patch now avoids creating SSL contexts if there
are no SSL proxying configured. This seems to complicate things
insignificantly given the new code layout, though ensures that
configurations with "proxy_ssl_verify on;" at http or stream level
without proxy_ssl_trusted_certificate set at the same level, as
seen in stream_proxy_ssl_verify.t, won't blow up.

> Also, you effectively compare some directives by value (instead
> of checking the presence), so it might be surprising to the user
> that repeating some directives on inner level increases memory
> consumption and repeating others doesn't.

I don't think this is an issue. SSL contexts can be inherited
from the previous level if proxy_ssl_* directives are not
redefined; if any of them are redefined, this can result in an
additional SSL context. While there are settings which does not
create additional SSL contexts even if set to a different value
(such as proxy_ssl_server_name), the only guarantee we are willing
to provide is that inherited settings will result in optimal
memory usage.

Either way, this is irrelevant with the updated patch.

> My last patch already addresses all of above...

Except you've failed to make it readable.

> Also, it would be nice to avoid all this copy-paste

As already explained, upstream SSL settings are protocol-specific
in the current design, as well as upstream SSL context creation.
While introducing some shared part is possible, it does not seem
to worth the effort, especially given the existing
protocol-specific difference.

> and have the same optimization in the stream module.

Given there are no locations in the stream module, I don't think
this is an issue there, or at least no more than server-side SSL
contexts. On the other hand, it is trivial to add the same
optimization to the stream module, and good from the code
consistency point of view. Added.

> > # HG changeset patch
> > # User Maxim Dounin <mdounin@mdounin.ru>
> > # Date 1655429915 -10800
> > # Fri Jun 17 04:38:35 2022 +0300
> > # Node ID e4a0eeb3edba037f0d090023a2242bda2f8dcb03
> > # Parent e23a385cd0ec866a3eb1d8c9c956991e1ed50d78
> > Upstream: optimized use of SSL contexts (ticket #1234).
> [...]
> >
> > #if (NGX_HTTP_SSL)
> > - u->ssl = (glcf->upstream.ssl != NULL);
> > + u->ssl = glcf->ssl;
>
> I like this change, with it additional 'shared_ssl' pointer can
> be removed from my patch.

Well, "shared_ssl" shouldn't have been added in the first place.

Updated patch below. Review and testing appreciated.

# HG changeset patch
# User Maxim Dounin <mdounin@mdounin.ru>
# Date 1656218606 -10800
# Sun Jun 26 07:43:26 2022 +0300
# Node ID f45142380cee37e71dab76d77ead279cf9b2f4e9
# Parent fecd73db563fb64108f7669eca419badb2aba633
Upstream: optimized use of SSL contexts (ticket #1234).

To ensure optimal use of memory, SSL contexts for proxying are now
inherited from previous levels as long as relevant proxy_ssl_* directives
are not redefined.

Further, when no proxy_ssl_* directives are redefined in a server block,
we now preserve plcf->upstream.ssl in the "http" section configuration
to inherit it to all servers.

Similar changes made in uwsgi, grpc, and stream proxy.

diff --git a/src/http/modules/ngx_http_grpc_module.c b/src/http/modules/ngx_http_grpc_module.c
--- a/src/http/modules/ngx_http_grpc_module.c
+++ b/src/http/modules/ngx_http_grpc_module.c
@@ -209,6 +209,8 @@ static char *ngx_http_grpc_ssl_password_
ngx_command_t *cmd, void *conf);
static char *ngx_http_grpc_ssl_conf_command_check(ngx_conf_t *cf, void *post,
void *data);
+static ngx_int_t ngx_http_grpc_merge_ssl(ngx_conf_t *cf,
+ ngx_http_grpc_loc_conf_t *conf, ngx_http_grpc_loc_conf_t *prev);
static ngx_int_t ngx_http_grpc_set_ssl(ngx_conf_t *cf,
ngx_http_grpc_loc_conf_t *glcf);
#endif
@@ -562,7 +564,7 @@ ngx_http_grpc_handler(ngx_http_request_t
ctx->host = glcf->host;

#if (NGX_HTTP_SSL)
- u->ssl = (glcf->upstream.ssl != NULL);
+ u->ssl = glcf->ssl;

if (u->ssl) {
ngx_str_set(&u->schema, "grpcs://");
@@ -4463,6 +4465,10 @@ ngx_http_grpc_merge_loc_conf(ngx_conf_t

#if (NGX_HTTP_SSL)

+ if (ngx_http_grpc_merge_ssl(cf, conf, prev) != NGX_OK) {
+ return NGX_CONF_ERROR;
+ }
+
ngx_conf_merge_value(conf->upstream.ssl_session_reuse,
prev->upstream.ssl_session_reuse, 1);

@@ -4524,7 +4530,7 @@ ngx_http_grpc_merge_loc_conf(ngx_conf_t
conf->grpc_values = prev->grpc_values;

#if (NGX_HTTP_SSL)
- conf->upstream.ssl = prev->upstream.ssl;
+ conf->ssl = prev->ssl;
#endif
}

@@ -4874,17 +4880,63 @@ ngx_http_grpc_ssl_conf_command_check(ngx


static ngx_int_t
+ngx_http_grpc_merge_ssl(ngx_conf_t *cf, ngx_http_grpc_loc_conf_t *conf,
+ ngx_http_grpc_loc_conf_t *prev)
+{
+ ngx_uint_t preserve;
+
+ if (conf->ssl_protocols == 0
+ && conf->ssl_ciphers.data == NULL
+ && conf->upstream.ssl_certificate == NGX_CONF_UNSET_PTR
+ && conf->upstream.ssl_certificate_key == NGX_CONF_UNSET_PTR
+ && conf->upstream.ssl_passwords == NGX_CONF_UNSET_PTR
+ && conf->upstream.ssl_verify == NGX_CONF_UNSET
+ && conf->ssl_verify_depth == NGX_CONF_UNSET_UINT
+ && conf->ssl_trusted_certificate.data == NULL
+ && conf->ssl_crl.data == NULL
+ && conf->upstream.ssl_session_reuse == NGX_CONF_UNSET
+ && conf->ssl_conf_commands == NGX_CONF_UNSET_PTR)
+ {
+ if (prev->upstream.ssl) {
+ conf->upstream.ssl = prev->upstream.ssl;
+ return NGX_OK;
+ }
+
+ preserve = 1;
+
+ } else {
+ preserve = 0;
+ }
+
+ conf->upstream.ssl = ngx_pcalloc(cf->pool, sizeof(ngx_ssl_t));
+ if (conf->upstream.ssl == NULL) {
+ return NGX_ERROR;
+ }
+
+ conf->upstream.ssl->log = cf->log;
+
+ /*
+ * special handling to preserve conf->upstream.ssl
+ * in the "http" section to inherit it to all servers
+ */
+
+ if (preserve) {
+ prev->upstream.ssl = conf->upstream.ssl;
+ }
+
+ return NGX_OK;
+}
+
+
+static ngx_int_t
ngx_http_grpc_set_ssl(ngx_conf_t *cf, ngx_http_grpc_loc_conf_t *glcf)
{
ngx_pool_cleanup_t *cln;

- glcf->upstream.ssl = ngx_pcalloc(cf->pool, sizeof(ngx_ssl_t));
- if (glcf->upstream.ssl == NULL) {
- return NGX_ERROR;
+ if (glcf->upstream.ssl->ctx) {
+ return NGX_OK;
}

- glcf->upstream.ssl->log = cf->log;
-
if (ngx_ssl_create(glcf->upstream.ssl, glcf->ssl_protocols, NULL)
!= NGX_OK)
{
diff --git a/src/http/modules/ngx_http_proxy_module.c b/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
@@ -236,6 +236,8 @@ static ngx_int_t ngx_http_proxy_rewrite_
ngx_http_proxy_rewrite_t *pr, ngx_str_t *regex, ngx_uint_t caseless);

#if (NGX_HTTP_SSL)
+static ngx_int_t ngx_http_proxy_merge_ssl(ngx_conf_t *cf,
+ ngx_http_proxy_loc_conf_t *conf, ngx_http_proxy_loc_conf_t *prev);
static ngx_int_t ngx_http_proxy_set_ssl(ngx_conf_t *cf,
ngx_http_proxy_loc_conf_t *plcf);
#endif
@@ -959,7 +961,7 @@ ngx_http_proxy_handler(ngx_http_request_
ctx->vars = plcf->vars;
u->schema = plcf->vars.schema;
#if (NGX_HTTP_SSL)
- u->ssl = (plcf->upstream.ssl != NULL);
+ u->ssl = plcf->ssl;
#endif

} else {
@@ -3724,6 +3726,10 @@ ngx_http_proxy_merge_loc_conf(ngx_conf_t

#if (NGX_HTTP_SSL)

+ if (ngx_http_proxy_merge_ssl(cf, conf, prev) != NGX_OK) {
+ return NGX_CONF_ERROR;
+ }
+
ngx_conf_merge_value(conf->upstream.ssl_session_reuse,
prev->upstream.ssl_session_reuse, 1);

@@ -3857,7 +3863,7 @@ ngx_http_proxy_merge_loc_conf(ngx_conf_t
conf->proxy_values = prev->proxy_values;

#if (NGX_HTTP_SSL)
- conf->upstream.ssl = prev->upstream.ssl;
+ conf->ssl = prev->ssl;
#endif
}

@@ -4923,16 +4929,62 @@ ngx_http_proxy_ssl_conf_command_check(ng


static ngx_int_t
+ngx_http_proxy_merge_ssl(ngx_conf_t *cf, ngx_http_proxy_loc_conf_t *conf,
+ ngx_http_proxy_loc_conf_t *prev)
+{
+ ngx_uint_t preserve;
+
+ if (conf->ssl_protocols == 0
+ && conf->ssl_ciphers.data == NULL
+ && conf->upstream.ssl_certificate == NGX_CONF_UNSET_PTR
+ && conf->upstream.ssl_certificate_key == NGX_CONF_UNSET_PTR
+ && conf->upstream.ssl_passwords == NGX_CONF_UNSET_PTR
+ && conf->upstream.ssl_verify == NGX_CONF_UNSET
+ && conf->ssl_verify_depth == NGX_CONF_UNSET_UINT
+ && conf->ssl_trusted_certificate.data == NULL
+ && conf->ssl_crl.data == NULL
+ && conf->upstream.ssl_session_reuse == NGX_CONF_UNSET
+ && conf->ssl_conf_commands == NGX_CONF_UNSET_PTR)
+ {
+ if (prev->upstream.ssl) {
+ conf->upstream.ssl = prev->upstream.ssl;
+ return NGX_OK;
+ }
+
+ preserve = 1;
+
+ } else {
+ preserve = 0;
+ }
+
+ conf->upstream.ssl = ngx_pcalloc(cf->pool, sizeof(ngx_ssl_t));
+ if (conf->upstream.ssl == NULL) {
+ return NGX_ERROR;
+ }
+
+ conf->upstream.ssl->log = cf->log;
+
+ /*
+ * special handling to preserve conf->upstream.ssl
+ * in the "http" section to inherit it to all servers
+ */
+
+ if (preserve) {
+ prev->upstream.ssl = conf->upstream.ssl;
+ }
+
+ return NGX_OK;
+}
+
+
+static ngx_int_t
ngx_http_proxy_set_ssl(ngx_conf_t *cf, ngx_http_proxy_loc_conf_t *plcf)
{
ngx_pool_cleanup_t *cln;

- plcf->upstream.ssl = ngx_pcalloc(cf->pool, sizeof(ngx_ssl_t));
- if (plcf->upstream.ssl == NULL) {
- return NGX_ERROR;
- }
-
- plcf->upstream.ssl->log = cf->log;
+ if (plcf->upstream.ssl->ctx) {
+ return NGX_OK;
+ }

if (ngx_ssl_create(plcf->upstream.ssl, plcf->ssl_protocols, NULL)
!= NGX_OK)
diff --git a/src/http/modules/ngx_http_uwsgi_module.c b/src/http/modules/ngx_http_uwsgi_module.c
--- a/src/http/modules/ngx_http_uwsgi_module.c
+++ b/src/http/modules/ngx_http_uwsgi_module.c
@@ -96,6 +96,8 @@ static char *ngx_http_uwsgi_ssl_password
ngx_command_t *cmd, void *conf);
static char *ngx_http_uwsgi_ssl_conf_command_check(ngx_conf_t *cf, void *post,
void *data);
+static ngx_int_t ngx_http_uwsgi_merge_ssl(ngx_conf_t *cf,
+ ngx_http_uwsgi_loc_conf_t *conf, ngx_http_uwsgi_loc_conf_t *prev);
static ngx_int_t ngx_http_uwsgi_set_ssl(ngx_conf_t *cf,
ngx_http_uwsgi_loc_conf_t *uwcf);
#endif
@@ -668,7 +670,7 @@ ngx_http_uwsgi_handler(ngx_http_request_
if (uwcf->uwsgi_lengths == NULL) {

#if (NGX_HTTP_SSL)
- u->ssl = (uwcf->upstream.ssl != NULL);
+ u->ssl = uwcf->ssl;

if (u->ssl) {
ngx_str_set(&u->schema, "suwsgi://");
@@ -1865,6 +1867,10 @@ ngx_http_uwsgi_merge_loc_conf(ngx_conf_t

#if (NGX_HTTP_SSL)

+ if (ngx_http_uwsgi_merge_ssl(cf, conf, prev) != NGX_OK) {
+ return NGX_CONF_ERROR;
+ }
+
ngx_conf_merge_value(conf->upstream.ssl_session_reuse,
prev->upstream.ssl_session_reuse, 1);

@@ -1927,7 +1933,7 @@ ngx_http_uwsgi_merge_loc_conf(ngx_conf_t
conf->uwsgi_values = prev->uwsgi_values;

#if (NGX_HTTP_SSL)
- conf->upstream.ssl = prev->upstream.ssl;
+ conf->ssl = prev->ssl;
#endif
}

@@ -2455,17 +2461,63 @@ ngx_http_uwsgi_ssl_conf_command_check(ng


static ngx_int_t
+ngx_http_uwsgi_merge_ssl(ngx_conf_t *cf, ngx_http_uwsgi_loc_conf_t *conf,
+ ngx_http_uwsgi_loc_conf_t *prev)
+{
+ ngx_uint_t preserve;
+
+ if (conf->ssl_protocols == 0
+ && conf->ssl_ciphers.data == NULL
+ && conf->upstream.ssl_certificate == NGX_CONF_UNSET_PTR
+ && conf->upstream.ssl_certificate_key == NGX_CONF_UNSET_PTR
+ && conf->upstream.ssl_passwords == NGX_CONF_UNSET_PTR
+ && conf->upstream.ssl_verify == NGX_CONF_UNSET
+ && conf->ssl_verify_depth == NGX_CONF_UNSET_UINT
+ && conf->ssl_trusted_certificate.data == NULL
+ && conf->ssl_crl.data == NULL
+ && conf->upstream.ssl_session_reuse == NGX_CONF_UNSET
+ && conf->ssl_conf_commands == NGX_CONF_UNSET_PTR)
+ {
+ if (prev->upstream.ssl) {
+ conf->upstream.ssl = prev->upstream.ssl;
+ return NGX_OK;
+ }
+
+ preserve = 1;
+
+ } else {
+ preserve = 0;
+ }
+
+ conf->upstream.ssl = ngx_pcalloc(cf->pool, sizeof(ngx_ssl_t));
+ if (conf->upstream.ssl == NULL) {
+ return NGX_ERROR;
+ }
+
+ conf->upstream.ssl->log = cf->log;
+
+ /*
+ * special handling to preserve conf->upstream.ssl
+ * in the "http" section to inherit it to all servers
+ */
+
+ if (preserve) {
+ prev->upstream.ssl = conf->upstream.ssl;
+ }
+
+ return NGX_OK;
+}
+
+
+static ngx_int_t
ngx_http_uwsgi_set_ssl(ngx_conf_t *cf, ngx_http_uwsgi_loc_conf_t *uwcf)
{
ngx_pool_cleanup_t *cln;

- uwcf->upstream.ssl = ngx_pcalloc(cf->pool, sizeof(ngx_ssl_t));
- if (uwcf->upstream.ssl == NULL) {
- return NGX_ERROR;
+ if (uwcf->upstream.ssl->ctx) {
+ return NGX_OK;
}

- uwcf->upstream.ssl->log = cf->log;
-
if (ngx_ssl_create(uwcf->upstream.ssl, uwcf->ssl_protocols, NULL)
!= NGX_OK)
{
diff --git a/src/stream/ngx_stream_proxy_module.c b/src/stream/ngx_stream_proxy_module.c
--- a/src/stream/ngx_stream_proxy_module.c
+++ b/src/stream/ngx_stream_proxy_module.c
@@ -103,6 +103,8 @@ static void ngx_stream_proxy_ssl_handsha
static void ngx_stream_proxy_ssl_save_session(ngx_connection_t *c);
static ngx_int_t ngx_stream_proxy_ssl_name(ngx_stream_session_t *s);
static ngx_int_t ngx_stream_proxy_ssl_certificate(ngx_stream_session_t *s);
+static ngx_int_t ngx_stream_proxy_merge_ssl(ngx_conf_t *cf,
+ ngx_stream_proxy_srv_conf_t *conf, ngx_stream_proxy_srv_conf_t *prev);
static ngx_int_t ngx_stream_proxy_set_ssl(ngx_conf_t *cf,
ngx_stream_proxy_srv_conf_t *pscf);

@@ -801,7 +803,7 @@ ngx_stream_proxy_init_upstream(ngx_strea

#if (NGX_STREAM_SSL)

- if (pc->type == SOCK_STREAM && pscf->ssl) {
+ if (pc->type == SOCK_STREAM && pscf->ssl_enable) {

if (u->proxy_protocol) {
if (ngx_stream_proxy_send_proxy_protocol(s) != NGX_OK) {
@@ -2150,6 +2152,10 @@ ngx_stream_proxy_merge_srv_conf(ngx_conf

#if (NGX_STREAM_SSL)

+ if (ngx_stream_proxy_merge_ssl(cf, conf, prev) != NGX_OK) {
+ return NGX_CONF_ERROR;
+ }
+
ngx_conf_merge_value(conf->ssl_enable, prev->ssl_enable, 0);

ngx_conf_merge_value(conf->ssl_session_reuse,
@@ -2199,17 +2205,63 @@ ngx_stream_proxy_merge_srv_conf(ngx_conf
#if (NGX_STREAM_SSL)

static ngx_int_t
+ngx_stream_proxy_merge_ssl(ngx_conf_t *cf, ngx_stream_proxy_srv_conf_t *conf,
+ ngx_stream_proxy_srv_conf_t *prev)
+{
+ ngx_uint_t preserve;
+
+ if (conf->ssl_protocols == 0
+ && conf->ssl_ciphers.data == NULL
+ && conf->ssl_certificate == NGX_CONF_UNSET_PTR
+ && conf->ssl_certificate_key == NGX_CONF_UNSET_PTR
+ && conf->ssl_passwords == NGX_CONF_UNSET_PTR
+ && conf->ssl_verify == NGX_CONF_UNSET
+ && conf->ssl_verify_depth == NGX_CONF_UNSET_UINT
+ && conf->ssl_trusted_certificate.data == NULL
+ && conf->ssl_crl.data == NULL
+ && conf->ssl_session_reuse == NGX_CONF_UNSET
+ && conf->ssl_conf_commands == NGX_CONF_UNSET_PTR)
+ {
+ if (prev->ssl) {
+ conf->ssl = prev->ssl;
+ return NGX_OK;
+ }
+
+ preserve = 1;
+
+ } else {
+ preserve = 0;
+ }
+
+ conf->ssl = ngx_pcalloc(cf->pool, sizeof(ngx_ssl_t));
+ if (conf->ssl == NULL) {
+ return NGX_ERROR;
+ }
+
+ conf->ssl->log = cf->log;
+
+ /*
+ * special handling to preserve conf->ssl
+ * in the "stream" section to inherit it to all servers
+ */
+
+ if (preserve) {
+ prev->ssl = conf->ssl;
+ }
+
+ return NGX_OK;
+}
+
+
+static ngx_int_t
ngx_stream_proxy_set_ssl(ngx_conf_t *cf, ngx_stream_proxy_srv_conf_t *pscf)
{
ngx_pool_cleanup_t *cln;

- pscf->ssl = ngx_pcalloc(cf->pool, sizeof(ngx_ssl_t));
- if (pscf->ssl == NULL) {
- return NGX_ERROR;
+ if (pscf->ssl->ctx) {
+ return NGX_OK;
}

- pscf->ssl->log = cf->log;
-
if (ngx_ssl_create(pscf->ssl, pscf->ssl_protocols, NULL) != NGX_OK) {
return NGX_ERROR;
}

--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org