> On 26 Aug 2022, at 07:01, Maxim Dounin <mdounin@mdounin.ru> wrote:
>
> # HG changeset patch
> # User Maxim Dounin <mdounin@mdounin.ru>
> # Date 1661481945 -10800
> # Fri Aug 26 05:45:45 2022 +0300
> # Node ID 2cd8fbeb4edc5a99b725585edc02a16a8a0c503e
> # Parent 069a4813e8d6d7ec662d282a10f5f7062ebd817f
> SSL: disabled saving tickets to session cache.
>
> OpenSSL for TLSv1.3 tries to save tickets into session cache "because some
> applications just want to know about the creation of a session". To avoid
> trashing session cache with useless data, we do not save such sessions now.
>

For the record, BoringSSL doesn't seem to call new_session_cb for TLSv1.3
at all, so there is no way to resume sessions with SSL_OP_NO_TICKET set.
In contrary, OpenSSL emits stateful tickets in this case, which contain
dummy session id used then as a session cache lookup key on server
(much like session ids in TLSv1.2)

OTOH, without SSL_OP_NO_TICKET set, OpenSSL emits self-containing tickets
with enough info to resume session, so nothing to lookup in session cache.
The latter makes impractical storing something in session cache, except
to use the callback for things like tracking "the creation of a session".
Namely, OpenSSL puts session (i.e. something that SSL_get_session returns)
and supplementary info to session ticket message as the ticket value.

With these thoughts in mind, I think log could be clarified to emphasize:
- it's not tickets that are stored in cache
- with SSL_OP_NO_TICKET set TLSv1.3 session are still saved to lookup by id.

> 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
> @@ -3815,6 +3815,22 @@ ngx_ssl_new_session(ngx_ssl_conn_t *ssl_
> ngx_ssl_session_cache_t *cache;
> u_char buf[NGX_SSL_MAX_SESSION_SIZE];
>
> +#ifdef TLS1_3_VERSION
> +
> + /*
> + * OpenSSL for TLSv1.3 tries to save tickets into session cache
> + * "because some applications just want to know about the creation
> + * of a session"; do not cache such sessions
> + */
> +
> + if (SSL_version(ssl_conn) == TLS1_3_VERSION
> + && (SSL_get_options(ssl_conn) & SSL_OP_NO_TICKET) == 0)
> + {
> + return 0;
> + }
> +
> +#endif
> +
> len = i2d_SSL_SESSION(sess, NULL);
>
> /* do not cache too big session */
>

Looks good.

BTW, looking through this code I see that casting NGX_SSL_MAX_SESSION_SIZE,
as seen below the above comment, should be safe to remove, as it stopped
using offsetof since 5ffd76a9ccf3.

# HG changeset patch
# User Sergey Kandaurov <pluknet@nginx.com>
# Date 1663213568 -14400
# Thu Sep 15 07:46:08 2022 +0400
# Node ID 45f239ff9ad31d33b58822eff8686657e0862e44
# Parent bcdd372dd558c6bcef981704b788b3b224173347
SSL: removed cast not needed after 5ffd76a9ccf3.

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
@@ -3842,7 +3842,7 @@ ngx_ssl_new_session(ngx_ssl_conn_t *ssl_

/* do not cache too big session */

- if (len > (int) NGX_SSL_MAX_SESSION_SIZE) {
+ if (len > NGX_SSL_MAX_SESSION_SIZE) {
return 0;
}


--
Sergey Kandaurov

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