Hello!

On Thu, Sep 15, 2022 at 09:36:31AM +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 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.

It looks like you are trying to introduce "stateful tickets" and
"self-containing tickets" terms, which is somewhat confusing
unless carefully explained. OpenSSL itself tries to use terms
"stateful tickets" and "stateless tickets", with the similar
drawbacks.

A better explanation would be to follow generic term "session
ticket", as originally introduced in RFC 4507 for TLS session
resumption without server-side state.

In these terms (as always used before introduction of TLSv1.3 and
currently used in many places, including nginx own documentation
and the source code) there are two basic mechanisms to resume
sessions: server-side session cache and session tickets (used to
resume sessions without server-side state).

Without SSL_OP_NO_TICKET set, OpenSSL uses tickets as long as
supported by the client.

With SSL_OP_NO_TICKET set, OpenSSL does not use tickets, and uses
server-side session cache instead (if configured).

The only difference between TLSv1.3 and previous protocols is how
session ids are sent to the client if server-side session cache is
used. In case of SSL and TLS up to and including TLSv1.2, session
ids are sent in the dedicated fields of the ServerHello and
ClientHello handshake messages. In case of TLSv1.3, dedicated
fields were removed, so session ids are sent in the
NewSessionTicket messages ("a database lookup key" in terms of RFC
8446).

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

Hope it is clear enough now.

>
> > 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;
> }
>

Looks good.

(Especially given that the cast is not used in similar tests in
the upstream zone code.)

--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org