Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't cache stateless tickets in TLSv1.3 #6293

Closed
wants to merge 1 commit into from

Conversation

mattcaswell
Copy link
Member

In TLSv1.2 and below we always cache new sessions by default on the server
side in the internal cache (even when we're using session tickets). This is
in order to support resumption from a session id.

In TLSv1.3 there is no session id. It is only possible to resume using the
ticket. Therefore, in the default case, there is no point in caching the
session in the internal store.

There is still a reason to call the external cache new session callback
because applications may be using the callbacks just to know about when
sessions are created (and not necessarily implementing a full cache). If
the application also implements the remove session callback then we are
forced to also store it in the internal cache so that we can create
timeout events. Otherwise the external cache could just fill up
indefinitely.

This mostly addresses the issue described in #5628. That issue also proposes
having an option to not create full stateless tickets when using the
internal cache. That aspect hasn't been addressed yet.

In TLSv1.2 and below we always cache new sessions by default on the server
side in the internal cache (even when we're using session tickets). This is
in order to support resumption from a session id.

In TLSv1.3 there is no session id. It is only possible to resume using the
ticket. Therefore, in the default case,  there is no point in caching the
session in the internal store.

There is still a reason to call the external cache new session callback
because applications may be using the callbacks just to know about when
sessions are created (and not necessarily implementing a full cache). If
the application also implements the remove session callback then we are
forced to also store it in the internal cache so that we can create
timeout events. Otherwise the external cache could just fill up
indefinitely.

This mostly addresses the issue described in openssl#5628. That issue also proposes
having an option to not create full stateless tickets when using the
internal cache. That aspect hasn't been addressed yet.
@mattcaswell mattcaswell added branch: master Merge to master branch branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch labels May 18, 2018
@mattcaswell mattcaswell added this to the 1.1.1 milestone May 18, 2018
Copy link
Contributor

@richsalz richsalz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you for the detailed comments.

@richsalz richsalz added the approval: done This pull request has the required number of approvals label May 18, 2018
@Ralith
Copy link
Contributor

Ralith commented May 20, 2018

Won't this change cause the check at https://github.com/openssl/openssl/blob/master/ssl/statem/extensions_srvr.c#L1154 to always fail?

@mattcaswell
Copy link
Member Author

Won't this change cause the check at https://github.com/openssl/openssl/blob/master/ssl/statem/extensions_srvr.c#L1154 to always fail?

No, because we still cache the session if early data is switched on in this PR, i.e. we check the value of s->max_early_data and if it is greater than 0 then we still add it to the cache.

@mattcaswell
Copy link
Member Author

Pushed. Thanks.

levitte pushed a commit that referenced this pull request May 21, 2018
In TLSv1.2 and below we always cache new sessions by default on the server
side in the internal cache (even when we're using session tickets). This is
in order to support resumption from a session id.

In TLSv1.3 there is no session id. It is only possible to resume using the
ticket. Therefore, in the default case,  there is no point in caching the
session in the internal store.

There is still a reason to call the external cache new session callback
because applications may be using the callbacks just to know about when
sessions are created (and not necessarily implementing a full cache). If
the application also implements the remove session callback then we are
forced to also store it in the internal cache so that we can create
timeout events. Otherwise the external cache could just fill up
indefinitely.

This mostly addresses the issue described in #5628. That issue also proposes
having an option to not create full stateless tickets when using the
internal cache. That aspect hasn't been addressed yet.

Reviewed-by: Rich Salz <rsalz@openssl.org>
Reviewed-by: Ben Kaduk <kaduk@mit.edu>
(Merged from #6293)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: done This pull request has the required number of approvals branch: master Merge to master branch branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants