> On 26 Aug 2022, at 07:01, Maxim Dounin <mdounin@mdounin.ru> wrote:
>
> # HG changeset patch
> # User Maxim Dounin <mdounin@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?

> 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.

> 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).

> +
> + 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.

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 :).

> +
> + /*
> + * 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.

> 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

--
Sergey Kandaurov

_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org