[PATCH 01 of 11] SSL: disabled saving tickets to session cache

[Maxim Dounin]

Hello!

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