> On 28 Sep 2022, at 22:37, Maxim Dounin <mdounin@mdounin.ru> wrote:
>
> Hello!
>
> On Mon, Sep 26, 2022 at 02:17:18PM +0400, Sergey Kandaurov wrote:
>
>>> On 17 Sep 2022, at 01:08, Maxim Dounin <mdounin@mdounin.ru> wrote:
>>>
>>> On Thu, Sep 15, 2022 at 09:50:24AM +0400, Sergey Kandaurov wrote:
>>>
>>>>> 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?
>>>
>>> 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.
>
> I don't think that nginx_shared_zone is unrelated, it's a basic
> shared zone used for multiple nginx-wide tasks, including mostly
> module-specific ones, such as stub_status. I don't see why it
> shouldn't contain some keying material used for SSL ticket keys.
>

Well, from the point of view of a basic shared zone for various tasks,
this sounds convincing. If carefully separate storage logic for
nginx_shared_zone (i.e. pure allocation in ngx_event_module_init()
e.g. as currently done for stub_status) and SSL-related logic that
uses this storage, then this approach is quite promising.

> But using it certainly would require some additional work, notably
> a) mutex initialization for proper locking (since there is no
> shared pool in the zone) and b) some key derivation logic, as we
> probably want to avoid using identical keys for all server blocks.

Agree.

>
>>> 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.
>
> I rather see ticket key rotation as a free feature you'll now get
> if you have ssl_session_cache enabled. If there will be enough
> demand for ticket key rotation without ssl_session_cache enabled,
> using nginx_shared_zone might be revisited, see above.
>

Ok.

> [...]
>
>>>>> +
>>>>> + 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
>
> Note that previously (before the 1st patch from this series) with
> ssl_session_cache enabled and TLSv1.3 this used to result in 2
> sessions being created and both saved to the session cache, with
> corresponding 2 locks.
>
>> 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.
>
> Correct.
>
>> 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.
>
> You mean on the client side? Yes, it looks like
> ngx_ssl_new_client_session() won't be called for such a new
> session ticket, and updated ticket will be never saved. This
> might need to be worked around.

Yes, I mean the client side.

>
> This should be safe with the key rotation logic introduced in this
> patch though, given that the previous key is preserved till the
> last ticket encrypted with it is expected to expire.
>
> One of the possible solutions might be to avoid re-encryption of
> tickets with the new key, as the old key is anyway expected to be
> available till the session expires.

I don't think it's worth the effort. If I got you right, and
as far as I understand, re-encrypting the ticket essentially
means sending a fresh session (renewal). Avoid doing that will
result in eventual session expiration and a full SSL handshake.
Please correct me if I'm wrong.

>
>> 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.
>
> Current code preserves the previous key till key expiration is in
> the past, and this should be long enough for all cases.
>
>>> 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.
>
> Well, if it's already reviewed I see no reasons to delay the
> commit.

Yes, it looks good for me.
As well as other patches in the series.

--
Sergey Kandaurov

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