[PATCH 11 of 11] SSL: automatic rotation of session ticket keys
Sergey Kandaurov
pluknet at nginx.com
Mon Sep 26 10:17:18 UTC 2022
> On 17 Sep 2022, at 01:08, Maxim Dounin <mdounin at mdounin.ru> wrote:
>
> Hello!
>
> On Thu, Sep 15, 2022 at 09:50:24AM +0400, Sergey Kandaurov wrote:
>
>>> On 26 Aug 2022, at 07:01, Maxim Dounin <mdounin at mdounin.ru> wrote:
>>>
>>> # HG changeset patch
>>> # User Maxim Dounin <mdounin at mdounin.ru>
>>> # Date 1661481958 -10800
>>> # Fri Aug 26 05:45:58 2022 +0300
>>> # Node ID 5c26fe5f6ab0bf4c0d18cae8f6f6483348243d4b
>>> # Parent 2487bf5766f79c813b3397b3bb897424c3590445
>>> SSL: automatic rotation of session ticket keys.
>>>
>>> As long as ssl_session_cache in shared memory is configured, session ticket
>>> keys are now automatically generated in shared memory, and rotated
>>> periodically.
>>
>> It looks odd how session cache is (ab)used to store ticket keys.
>> I understand that it is comfortable to reuse existing shared zone
>> (and not to touch configuration) but probably a more correct way
>> is to create a separate zone to store keys?
>> Something pretty much the same as ssl_session_cache syntax:
>> ssl_session_ticket_key file | shared:name:size
>>
>> What do you think?
>
> I don't think it is a good idea to introduce additional shared
> zones here, especially given that it only needs about 160 bytes of
> shared memory. It really complicates things for users for no real
> reason.
>
> Rather, I was thinking about using nginx_shared_zone, which is
> always available, but it is rather special and will require
> additional integration code. Further, as a single contention
> point it might be a problem, at least unless lock avoidance is
> also implemented (see below). And a single key for all servers
> might not be the best solution either. On the other hand,
> ssl_session_cache in shared memory is readily available and
> logically related, and I see no reasons not to use it to rotate
> ticket keys if it's configured.
>
I've been pondering for a while and tend to agree to go with
the ssl_session_cache route for its simplicity.
I share the dislike to introduce new syntax, while
nginx_shared_zone looks unrelated enough to put SSL bits in.
> If there will be understanding that we need to rotate ticket keys
> in all cases, even without ssl_session_cache in shared memory
> being configured, the idea of using nginx_shared_zone might be
> revisited.
Well, an obvious downside is the need to enable a session cache
which might be undesirable in certain memory constrained systems
if you intend to resume only with session tickets. Even though
the impact is reduced by keeping shared memory zone to the minimum,
(re)using session cache to store only ticket keys nonetheless
requires to allocate memory shared zone of at least 8 page sizes.
OTOH, it quite uncommon to have clients that use tickets only.
>
>>> This can be beneficial from forward secrecy point of view,
>>> and also avoids increased CPU usage after configuration reloads.
>>>
>>
>> You can also mention that this fixes resuming sessions with BoringSSL
>> that implements its own ticket key rotation (if ticket keys callback
>> isn't installed) that doesn't work in multiple workers configuration.
>
> Thanks, forgot about this BoringSSL misfeature. Added the
> following paragraph:
>
> This also helps BoringSSL to properly resume sessions in configurations
> with multiple worker processes and no ssl_session_ticket_key directives,
> as BoringSSL tries to automatically rotate session ticket keys and does
> this independently in different worker processes, thus breaking session
> resumption between worker processes.
Looks good.
>
>>
>>> 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
>>> @@ -74,6 +74,7 @@ static void ngx_ssl_session_rbtree_inser
>>> static int ngx_ssl_ticket_key_callback(ngx_ssl_conn_t *ssl_conn,
>>> unsigned char *name, unsigned char *iv, EVP_CIPHER_CTX *ectx,
>>> HMAC_CTX *hctx, int enc);
>>> +static ngx_int_t ngx_ssl_rotate_ticket_keys(SSL_CTX *ssl_ctx, ngx_log_t *log);
>>> static void ngx_ssl_ticket_keys_cleanup(void *data);
>>> #endif
>>>
>>> @@ -3767,6 +3768,9 @@ ngx_ssl_session_cache_init(ngx_shm_zone_
>>>
>>> ngx_queue_init(&cache->expire_queue);
>>>
>>> + cache->ticket_keys[0].expire = 0;
>>> + cache->ticket_keys[1].expire = 0;
>>> +
>>> len = sizeof(" in SSL session shared cache \"\"") + shm_zone->shm.name.len;
>>>
>>> shpool->log_ctx = ngx_slab_alloc(shpool, len);
>>> @@ -4234,11 +4238,13 @@ ngx_ssl_session_ticket_keys(ngx_conf_t *
>>> ngx_pool_cleanup_t *cln;
>>> ngx_ssl_ticket_key_t *key;
>>>
>>> - if (paths == NULL) {
>>> + if (paths == NULL
>>> + && SSL_CTX_get_ex_data(ssl->ctx, ngx_ssl_session_cache_index) == NULL)
>>> + {
>>> return NGX_OK;
>>> }
>>>
>>> - keys = ngx_array_create(cf->pool, paths->nelts,
>>> + keys = ngx_array_create(cf->pool, paths ? paths->nelts : 2,
>>> sizeof(ngx_ssl_ticket_key_t));
>>> if (keys == NULL) {
>>> return NGX_ERROR;
>>> @@ -4252,6 +4258,34 @@ ngx_ssl_session_ticket_keys(ngx_conf_t *
>>> cln->handler = ngx_ssl_ticket_keys_cleanup;
>>> cln->data = keys;
>>>
>>> + if (SSL_CTX_set_ex_data(ssl->ctx, ngx_ssl_ticket_keys_index, keys) == 0) {
>>> + ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0,
>>> + "SSL_CTX_set_ex_data() failed");
>>> + return NGX_ERROR;
>>> + }
>>> +
>>> + if (SSL_CTX_set_tlsext_ticket_key_cb(ssl->ctx, ngx_ssl_ticket_key_callback)
>>> + == 0)
>>> + {
>>> + ngx_log_error(NGX_LOG_WARN, cf->log, 0,
>>> + "nginx was built with Session Tickets support, however, "
>>> + "now it is linked dynamically to an OpenSSL library "
>>> + "which has no tlsext support, therefore Session Tickets "
>>> + "are not available");
>>> + return NGX_OK;
>>> + }
>>> +
>>> + if (paths == NULL) {
>>> +
>>> + /* placeholder for keys in shared memory */
>>> +
>>> + key = ngx_array_push_n(keys, 2);
>>> + key[0].shared = 1;
>>> + key[1].shared = 1;
>>> +
>>> + return NGX_OK;
>>> + }
>>> +
>>> path = paths->elts;
>>> for (i = 0; i < paths->nelts; i++) {
>>>
>>> @@ -4306,6 +4340,8 @@ ngx_ssl_session_ticket_keys(ngx_conf_t *
>>> goto failed;
>>> }
>>>
>>> + key->shared = 0;
>>> +
>>> if (size == 48) {
>>> key->size = 48;
>>> ngx_memcpy(key->name, buf, 16);
>>> @@ -4327,22 +4363,6 @@ ngx_ssl_session_ticket_keys(ngx_conf_t *
>>> ngx_explicit_memzero(&buf, 80);
>>> }
>>>
>>> - if (SSL_CTX_set_ex_data(ssl->ctx, ngx_ssl_ticket_keys_index, keys) == 0) {
>>> - ngx_ssl_error(NGX_LOG_EMERG, ssl->log, 0,
>>> - "SSL_CTX_set_ex_data() failed");
>>> - return NGX_ERROR;
>>> - }
>>> -
>>> - if (SSL_CTX_set_tlsext_ticket_key_cb(ssl->ctx, ngx_ssl_ticket_key_callback)
>>> - == 0)
>>> - {
>>> - ngx_log_error(NGX_LOG_WARN, cf->log, 0,
>>> - "nginx was built with Session Tickets support, however, "
>>> - "now it is linked dynamically to an OpenSSL library "
>>> - "which has no tlsext support, therefore Session Tickets "
>>> - "are not available");
>>> - }
>>> -
>>> return NGX_OK;
>>>
>>> failed:
>>> @@ -4375,6 +4395,10 @@ ngx_ssl_ticket_key_callback(ngx_ssl_conn
>>> c = ngx_ssl_get_connection(ssl_conn);
>>> ssl_ctx = c->ssl->session_ctx;
>>>
>>> + if (ngx_ssl_rotate_ticket_keys(ssl_ctx, c->log) != NGX_OK) {
>>> + return -1;
>>> + }
>>> +
>>> #ifdef OPENSSL_NO_SHA256
>>> digest = EVP_sha1();
>>> #else
>>> @@ -4493,6 +4517,113 @@ ngx_ssl_ticket_key_callback(ngx_ssl_conn
>>> }
>>>
>>>
>>> +static ngx_int_t
>>> +ngx_ssl_rotate_ticket_keys(SSL_CTX *ssl_ctx, ngx_log_t *log)
>>> +{
>>> + time_t now, expire;
>>> + ngx_array_t *keys;
>>> + ngx_shm_zone_t *shm_zone;
>>> + ngx_slab_pool_t *shpool;
>>> + ngx_ssl_ticket_key_t *key;
>>> + ngx_ssl_session_cache_t *cache;
>>> + u_char buf[80];
>>
>> BTW, the same buf[] variable in ngx_ssl_session_ticket_keys()
>> is placed at the beginning of declaration block.
>> Looks like a chance to improve style consistency (but see below).
>
> There are enough style changes in this series already.
>
>>> +
>>> + keys = SSL_CTX_get_ex_data(ssl_ctx, ngx_ssl_ticket_keys_index);
>>> + if (keys == NULL) {
>>> + return NGX_OK;
>>> + }
>>> +
>>> + key = keys->elts;
>>> +
>>> + if (!key[0].shared) {
>>> + return NGX_OK;
>>> + }
>>> +
>>> + now = ngx_time();
>>> + expire = now + SSL_CTX_get_timeout(ssl_ctx);
>>> +
>>> + shm_zone = SSL_CTX_get_ex_data(ssl_ctx, ngx_ssl_session_cache_index);
>>> +
>>> + cache = shm_zone->data;
>>> + shpool = (ngx_slab_pool_t *) shm_zone->shm.addr;
>>> +
>>> + ngx_shmtx_lock(&shpool->mutex);
>>> +
>>> + key = cache->ticket_keys;
>>> +
>>> + if (key[0].expire == 0) {
>>> +
>>> + /* initialize the current key */
>>> +
>>> + if (RAND_bytes(buf, 80) != 1) {
>>> + ngx_ssl_error(NGX_LOG_ALERT, log, 0, "RAND_bytes() failed");
>>> + ngx_shmtx_unlock(&shpool->mutex);
>>> + return NGX_ERROR;
>>> + }
>>> +
>>> + key->shared = 1;
>>> + key->expire = expire;
>>> + key->size = 80;
>>> + ngx_memcpy(key->name, buf, 16);
>>> + ngx_memcpy(key->hmac_key, buf + 16, 32);
>>> + ngx_memcpy(key->aes_key, buf + 48, 32);
>>> +
>>> + ngx_explicit_memzero(&buf, 80);
>>> +
>>> + ngx_log_debug2(NGX_LOG_DEBUG_EVENT, log, 0,
>>> + "ssl ticket key: \"%*xs\"",
>>> + (size_t) 16, key->name);
>>> + }
>>> +
>>> + if (key[1].expire < now) {
>>> +
>>> + /*
>>> + * if the previous key is no longer needed (or not initialized),
>>> + * replace it with the current key and generate new current key
>>> + */
>>> +
>>> + key[1] = key[0];
>>> +
>>> + if (RAND_bytes(buf, 80) != 1) {
>>> + ngx_ssl_error(NGX_LOG_ALERT, log, 0, "RAND_bytes() failed");
>>> + ngx_shmtx_unlock(&shpool->mutex);
>>> + return NGX_ERROR;
>>> + }
>>> +
>>> + key->shared = 1;
>>> + key->expire = expire;
>>> + key->size = 80;
>>> + ngx_memcpy(key->name, buf, 16);
>>> + ngx_memcpy(key->hmac_key, buf + 16, 32);
>>> + ngx_memcpy(key->aes_key, buf + 48, 32);
>>> +
>>> + ngx_explicit_memzero(&buf, 80);
>>> +
>>> + ngx_log_debug2(NGX_LOG_DEBUG_EVENT, log, 0,
>>> + "ssl ticket key: \"%*xs\"",
>>> + (size_t) 16, key->name);
>>> + }
>>
>> I wonder if cpu intensive ops can be moved from under the lock.
>> At least, buf[] can be removed if write random data directly
>> to ngx_ssl_ticket_key_t. This eliminates 3 memory copies
>> (likely optimized by compiler to a single) and explicit write.
>
> Micro-optimizations are not something important here: new keys are
> generated once per SSL session timeout, that is, once per 300
> seconds by default. As such, the code is kept as close as
> possible to the ticket keys reading in
> ngx_ssl_session_ticket_keys(), and not dependant on the
> ngx_ssl_ticket_key_t structure layout.
Ok, that makes sense.
>
> What can be interesting here is to avoid locking at all unless we
> are going to update keys (change expiration of the current key, or
> switch to the next key).
>
> To do so without races, the code has to maintains 3 keys: current,
> previous, and next. If a worker will switch to the next key
> earlier, other workers will still be able to decrypt new tickets,
> since they will be encrypted with the next key.
>
> But with the SSL session cache in shared memory we have to lock
> shared zone anyway to create sessions, and this was never seen to
> be a performance bottleneck.
Good to know this is not a hot path.
Nonetheless, it is good to preserve a status-quo regarding
the locking and/or keep to the minimum its coverage/impact.
For the record, with this patch and ticket keys rotation enabled,
each ticket operation resembles existing locking in session cache:
- 1 lock per new ticket encryption
- 1 lock per each ticket decryption + 1 lock for possible renew
In TLSv1.3 this is a bit worse:
- 2 locks, each for issuing 2 new tickets
- 2 locks, each for ticket decryption + obligatory ticket renew
With the locking patch, locking acquirement reduces at least to:
- 1 lock total for decryption and renew, as that happens in 1 sec.
- 1 lock for issuing 2 new tickets in TLSv1.3, the same reasons
In loaded scenarios, working with tickets requires obtaining
at most 1 lock per 1 sec. per each worker process.
And by the way, while reviewing this patch, I noticed that
OpenSSL doesn't allow a client to gracefully renew TLSv1.2 session
when the client receives a new session ticket in resumed sessions.
In practice, it is visible when client resumes a not yet expired
session encrypted with not a fresh ticket key (after rotation),
which results in sending a new session ticket.
See ssl_update_cache() for the !s->hit condition.
In the opposite, BoringSSL always allows to renew TLSv1.2 sessions.
Additionally, BoringSSL view a bit differs on a session freshness.
Unlike in OpenSSL, BoringSSL treats sessions as fresh only if
the session's time of issuance + timeout points to the future.
In the opposite, OpenSSL resumes sessions expiring in the current second.
See math in ssl_get_prev_session() vs ssl_session_is_time_valid().
This is an insignificant detail, though.
> Given that, and the added code
> complexity, I've dropped the patch which avoids locking, at least
> for now.
>
> This might worth revisiting though. Patch provided below for
> reference.
>
I see no reasons not to commit it.
Though, it might make sense to let the dust settle down a bit
and commit the locking patch later.
>>
>> 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
>> @@ -4527,7 +4527,6 @@ ngx_ssl_rotate_ticket_keys(SSL_CTX *ssl_
>> ngx_slab_pool_t *shpool;
>> ngx_ssl_ticket_key_t *key;
>> ngx_ssl_session_cache_t *cache;
>> - u_char buf[80];
>>
>> keys = SSL_CTX_get_ex_data(ssl_ctx, ngx_ssl_ticket_keys_index);
>> if (keys == NULL) {
>> @@ -4556,7 +4555,7 @@ ngx_ssl_rotate_ticket_keys(SSL_CTX *ssl_
>>
>> /* initialize the current key */
>>
>> - if (RAND_bytes(buf, 80) != 1) {
>> + if (RAND_bytes(key->name, 80) != 1) {
>> ngx_ssl_error(NGX_LOG_ALERT, log, 0, "RAND_bytes() failed");
>> ngx_shmtx_unlock(&shpool->mutex);
>> return NGX_ERROR;
>> @@ -4565,11 +4564,6 @@ ngx_ssl_rotate_ticket_keys(SSL_CTX *ssl_
>> key->shared = 1;
>> key->expire = expire;
>> key->size = 80;
>> - ngx_memcpy(key->name, buf, 16);
>> - ngx_memcpy(key->hmac_key, buf + 16, 32);
>> - ngx_memcpy(key->aes_key, buf + 48, 32);
>> -
>> - ngx_explicit_memzero(&buf, 80);
>>
>> ngx_log_debug2(NGX_LOG_DEBUG_EVENT, log, 0,
>> "ssl ticket key: \"%*xs\"",
>> @@ -4585,7 +4579,7 @@ ngx_ssl_rotate_ticket_keys(SSL_CTX *ssl_
>>
>> key[1] = key[0];
>>
>> - if (RAND_bytes(buf, 80) != 1) {
>> + if (RAND_bytes(key->name, 80) != 1) {
>> ngx_ssl_error(NGX_LOG_ALERT, log, 0, "RAND_bytes() failed");
>> ngx_shmtx_unlock(&shpool->mutex);
>> return NGX_ERROR;
>> @@ -4594,11 +4588,6 @@ ngx_ssl_rotate_ticket_keys(SSL_CTX *ssl_
>> key->shared = 1;
>> key->expire = expire;
>> key->size = 80;
>> - ngx_memcpy(key->name, buf, 16);
>> - ngx_memcpy(key->hmac_key, buf + 16, 32);
>> - ngx_memcpy(key->aes_key, buf + 48, 32);
>> -
>> - ngx_explicit_memzero(&buf, 80);
>>
>> ngx_log_debug2(NGX_LOG_DEBUG_EVENT, log, 0,
>> "ssl ticket key: \"%*xs\"",
>>
>> Moving RAND_bytes() can be achieved by trying to elide shmem lock
>> and store random data elsewhere if condition is met.
>> But this comes in conflict with buf[] optimization :).
>
> You can't really move RAND_bytes() out of the lock unless you are
> ok with doing unneeded RAND_bytes() calls in workers which weren't
> first to update keys. And see above, you are trying to
> micro-optimize something which happens once per 300 seconds or
> even less frequently.
Agree. Even in advanced cases that don't throw away an unneeded
result, moving RAND_bytes() looks like out of practical interest,
since this requires a storage to save the result between callbacks
for future use, and this apparently affects simplicity and security.
>
>>> +
>>> + /*
>>> + * update expiration of the current key: it is going to be needed
>>> + * at least till the session being created expires
>>> + */
>>> +
>>> + if (expire > key[0].expire) {
>>> + key[0].expire = expire;
>>> + }
>>> +
>>> + /* sync keys to the worker process memory */
>>> +
>>> + ngx_memcpy(keys->elts, cache->ticket_keys,
>>> + 2 * sizeof(ngx_ssl_ticket_key_t));
>>> +
>>> + ngx_shmtx_unlock(&shpool->mutex);
>>> +
>>> + return NGX_OK;
>>> +}
>>> +
>>> +
>>> static void
>>> ngx_ssl_ticket_keys_cleanup(void *data)
>>> {
>>> 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
>>> @@ -147,25 +147,24 @@ struct ngx_ssl_sess_id_s {
>>>
>>>
>>> typedef struct {
>>> + u_char name[16];
>>> + u_char hmac_key[32];
>>> + u_char aes_key[32];
>>> + time_t expire;
>>> + unsigned size:8;
>>> + unsigned shared:1;
>>> +} ngx_ssl_ticket_key_t;
>>> +
>>> +
>>> +typedef struct {
>>> ngx_rbtree_t session_rbtree;
>>> ngx_rbtree_node_t sentinel;
>>> ngx_queue_t expire_queue;
>>> + ngx_ssl_ticket_key_t ticket_keys[2];
>>
>> Again, with the 1st patch applied, this makes using
>> of ticket_keys mutually exclusive to other fields.
>> This suggests that it doesn't belong here.
>
> It look like you are incorrectly assuming that all clients support
> session tickets. While this is true in TLSv1.3, it's not for
> TLSv1.2 and below.
>
> Further, the same ssl_session_cache can be used in multiple server
> blocks with different ssl_session_ticket settings.
See above about memory constrains, but in general I agree.
>
>>> time_t fail_time;
>>> } ngx_ssl_session_cache_t;
>>>
>>>
>>> -#ifdef SSL_CTRL_SET_TLSEXT_TICKET_KEY_CB
>>> -
>>> -typedef struct {
>>> - size_t size;
>>> - u_char name[16];
>>> - u_char hmac_key[32];
>>> - u_char aes_key[32];
>>> -} ngx_ssl_ticket_key_t;
>>> -
>>> -#endif
>>> -
>>> -
>>> #define NGX_SSL_SSLv2 0x0002
>>> #define NGX_SSL_SSLv3 0x0004
>>> #define NGX_SSL_TLSv1 0x0008
>
> Lock avoidance patch below.
>
> # HG changeset patch
> # User Maxim Dounin <mdounin at mdounin.ru>
> # Date 1663356500 -10800
> # Fri Sep 16 22:28:20 2022 +0300
> # Node ID 93cd7641604d85488ee4c6144e51cdeb60caa123
> # Parent 6d94781680d6750268cd697fb72f7b72b73e0df1
> SSL: optimized rotation of session ticket keys.
>
> Instead of syncing keys with shared memory on each ticket operation,
> the code now does this only when the worker is going to change expiration
> of the current key, or going to switch to a new key: that is, usually
> at most once per second.
>
> To do so without races, the code maintains 3 keys: current, previous,
> and next. If a worker will switch to the next key earlier, other workers
> will still be able to decrypt new tickets, since they will be encrypted
> with the next key.
>
> 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
> @@ -3773,6 +3773,7 @@ ngx_ssl_session_cache_init(ngx_shm_zone_
>
> cache->ticket_keys[0].expire = 0;
> cache->ticket_keys[1].expire = 0;
> + cache->ticket_keys[2].expire = 0;
>
> cache->fail_time = 0;
>
> @@ -4249,7 +4250,7 @@ ngx_ssl_session_ticket_keys(ngx_conf_t *
> return NGX_OK;
> }
>
> - keys = ngx_array_create(cf->pool, paths ? paths->nelts : 2,
> + keys = ngx_array_create(cf->pool, paths ? paths->nelts : 3,
> sizeof(ngx_ssl_ticket_key_t));
> if (keys == NULL) {
> return NGX_ERROR;
> @@ -4284,9 +4285,13 @@ ngx_ssl_session_ticket_keys(ngx_conf_t *
>
> /* placeholder for keys in shared memory */
>
> - key = ngx_array_push_n(keys, 2);
> + key = ngx_array_push_n(keys, 3);
> key[0].shared = 1;
> + key[0].expire = 0;
> key[1].shared = 1;
> + key[1].expire = 0;
> + key[2].shared = 1;
> + key[2].expire = 0;
>
> return NGX_OK;
> }
> @@ -4346,6 +4351,7 @@ ngx_ssl_session_ticket_keys(ngx_conf_t *
> }
>
> key->shared = 0;
> + key->expire = 1;
>
> if (size == 48) {
> key->size = 48;
> @@ -4513,7 +4519,7 @@ ngx_ssl_ticket_key_callback(ngx_ssl_conn
>
> /* renew if non-default key */
>
> - if (i != 0) {
> + if (i != 0 && key[i].expire) {
> return 2;
> }
>
> @@ -4544,9 +4550,21 @@ ngx_ssl_rotate_ticket_keys(SSL_CTX *ssl_
> return NGX_OK;
> }
>
> + /*
> + * if we don't need to update expiration of the current key
> + * and the previous key is still needed, don't sync with shared
> + * memory to save some work; in the worst case other worker process
> + * will switch to the next key, but this process will still be able
> + * to decrypt tickets encrypted with it
> + */
> +
> now = ngx_time();
> expire = now + SSL_CTX_get_timeout(ssl_ctx);
>
> + if (key[0].expire >= expire && key[1].expire >= now) {
> + return NGX_OK;
> + }
> +
> shm_zone = SSL_CTX_get_ex_data(ssl_ctx, ngx_ssl_session_cache_index);
>
> cache = shm_zone->data;
> @@ -4566,28 +4584,38 @@ ngx_ssl_rotate_ticket_keys(SSL_CTX *ssl_
> return NGX_ERROR;
> }
>
> - key->shared = 1;
> - key->expire = expire;
> - key->size = 80;
> - ngx_memcpy(key->name, buf, 16);
> - ngx_memcpy(key->hmac_key, buf + 16, 32);
> - ngx_memcpy(key->aes_key, buf + 48, 32);
> + key[0].shared = 1;
> + key[0].expire = expire;
> + key[0].size = 80;
> + ngx_memcpy(key[0].name, buf, 16);
> + ngx_memcpy(key[0].hmac_key, buf + 16, 32);
> + ngx_memcpy(key[0].aes_key, buf + 48, 32);
>
> ngx_explicit_memzero(&buf, 80);
>
> ngx_log_debug2(NGX_LOG_DEBUG_EVENT, log, 0,
> "ssl ticket key: \"%*xs\"",
> - (size_t) 16, key->name);
> + (size_t) 16, key[0].name);
> +
> + /*
> + * copy the current key to the next key, as initialization of
> + * the previous key will replace the current key with the next
> + * key
> + */
> +
> + key[2] = key[0];
> }
>
> if (key[1].expire < now) {
>
> /*
> * if the previous key is no longer needed (or not initialized),
> - * replace it with the current key and generate new current key
> + * replace it with the current key, replace the current key with
> + * the next key, and generate new next key
> */
>
> key[1] = key[0];
> + key[0] = key[2];
>
> if (RAND_bytes(buf, 80) != 1) {
> ngx_ssl_error(NGX_LOG_ALERT, log, 0, "RAND_bytes() failed");
> @@ -4595,18 +4623,18 @@ ngx_ssl_rotate_ticket_keys(SSL_CTX *ssl_
> return NGX_ERROR;
> }
>
> - key->shared = 1;
> - key->expire = expire;
> - key->size = 80;
> - ngx_memcpy(key->name, buf, 16);
> - ngx_memcpy(key->hmac_key, buf + 16, 32);
> - ngx_memcpy(key->aes_key, buf + 48, 32);
> + key[2].shared = 1;
> + key[2].expire = 0;
> + key[2].size = 80;
> + ngx_memcpy(key[2].name, buf, 16);
> + ngx_memcpy(key[2].hmac_key, buf + 16, 32);
> + ngx_memcpy(key[2].aes_key, buf + 48, 32);
>
> ngx_explicit_memzero(&buf, 80);
>
> ngx_log_debug2(NGX_LOG_DEBUG_EVENT, log, 0,
> "ssl ticket key: \"%*xs\"",
> - (size_t) 16, key->name);
> + (size_t) 16, key[2].name);
> }
>
> /*
> 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
> @@ -160,7 +160,7 @@ typedef struct {
> ngx_rbtree_t session_rbtree;
> ngx_rbtree_node_t sentinel;
> ngx_queue_t expire_queue;
> - ngx_ssl_ticket_key_t ticket_keys[2];
> + ngx_ssl_ticket_key_t ticket_keys[3];
> time_t fail_time;
> } ngx_ssl_session_cache_t;
>
Looks good.
--
Sergey Kandaurov
More information about the nginx-devel
mailing list