SSL contexts reuse across locations

Sergey Kandaurov pluknet at nginx.com
Tue Jun 28 14:13:31 UTC 2022


> On 28 Jun 2022, at 09:26, Pavel Pautov via nginx-devel <nginx-devel at nginx.org> wrote:
> 
> Hi,
> 
> The patch seems fine and is somewhat similar to what I've posted before.
> 
> I guess, the copy-paste can be addressed some time later by someone else.

I agree, the patch looks good to me,
tested in various configurations (including if() block, etc.)

> 
>> -----Original Message-----
>> From: Maxim Dounin <mdounin at mdounin.ru>
>> Sent: Saturday, June 25, 2022 22:48
>> To: Pavel Pautov via nginx-devel <nginx-devel at nginx.org>
>> Subject: Re: SSL contexts reuse across locations
>> 
>> EXTERNAL MAIL: nginx-devel-bounces at nginx.org
>> 
>> Hello!
>> 
>> On Sat, Jun 25, 2022 at 01:02:21AM +0000, Pavel Pautov via nginx-devel wrote:
>> 
>>>> -----Original Message-----
>>>> From: Maxim Dounin <mdounin at 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.

Well, redefining proxy_ssl_* directives by itself doesn't result in
additional SSL context, it can be created in locations with proxy_pass.
In my tests with basic configuration, without proxy_ssl_certificate
and proxy_ssl_trusted_certificate and all that, on OpenSSL 3.0.x,
creating SSL context results in allocation of by order of 10k memory
(tested using FreeBSD ktrace + utrace(2) malloc(3) records.)
In configuration levels with just redefined proxy_ssl_* directives,
it's an additional sizeof(ngx_ssl_t) allocation, currently of 24 bytes.
So, for example, if proxy_ssl_* directives are defined at http level,
and then redefined at server level only, this results in one context
per server (given that it has proxy_pass in locations).
Just to make it clear.

>> 
>> 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 at 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 at 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)
>>     {
> [...]
> 
> _______________________________________________
> nginx-devel mailing list -- nginx-devel at nginx.org
> To unsubscribe send an email to nginx-devel-leave at nginx.org

-- 
Sergey Kandaurov



More information about the nginx-devel mailing list