-
Notifications
You must be signed in to change notification settings - Fork 5.4k
tls: add support for client-side session resumption. #4791
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
Changes from all commits
a5fd6ed
ea18475
b93ad39
b7b3832
8512db4
b698a5a
9aea0b4
fd556b8
18404f4
e21ab80
e4a754b
5f31540
4aeefe4
a09a319
63f2da9
8eb4028
e8da108
5f13c14
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -293,6 +293,12 @@ message UpstreamTlsContext { | |
| // | ||
| // TLS renegotiation is considered insecure and shouldn't be used unless absolutely necessary. | ||
| bool allow_renegotiation = 3; | ||
|
|
||
| // Maximum number of session keys (Pre-Shared Keys for TLSv1.3+, Session IDs and Session Tickets | ||
| // for TLSv1.2 and older) to store for the purpose of session resumption. | ||
| // | ||
| // Defaults to 1, setting this to 0 disables session resumption. | ||
| google.protobuf.UInt32Value max_session_keys = 4; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Q: Should this feature be disabled or enabled by default?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are there any security implications from enabling by default? E.g. are we materially increasing the amount of code that might be subject to compromise in BoringSSL etc. I have zero clue on this, but my inclination would be if there was a tradeoff to sacrifice performance (i.e. the resumption) for improved default security posture.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is a case of a "privacy leak" (passive observer being able to correlate connections from the same user by looking at the session that's being resumed, which is sent unencrypted in Note: TLSv1.3 sends single-use sessions, so the default of 1 is probably too small. Perhaps we could make it vary by default, i.e. 1 if
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the common case is service mesh and middle/edge proxies, so we should optimize for that. It's probably not great to be optimizing for TLS 1.3 quiet yet, I don't think this is universal by far.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I somehow missed this comment earlier. What I meant regarding TLS v1.3 is basically: But I'm fine leaving the default at |
||
| } | ||
|
|
||
| message DownstreamTlsContext { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -275,7 +275,7 @@ std::vector<uint8_t> ContextImpl::parseAlpnProtocols(const std::string& alpn_pro | |
| return out; | ||
| } | ||
|
|
||
| bssl::UniquePtr<SSL> ContextImpl::newSsl(absl::optional<std::string>) const { | ||
| bssl::UniquePtr<SSL> ContextImpl::newSsl(absl::optional<std::string>) { | ||
| return bssl::UniquePtr<SSL>(SSL_new(ctx_.get())); | ||
| } | ||
|
|
||
|
|
@@ -490,16 +490,27 @@ ClientContextImpl::ClientContextImpl(Stats::Scope& scope, const ClientContextCon | |
| TimeSource& time_source) | ||
| : ContextImpl(scope, config, time_source), | ||
| server_name_indication_(config.serverNameIndication()), | ||
| allow_renegotiation_(config.allowRenegotiation()) { | ||
| allow_renegotiation_(config.allowRenegotiation()), | ||
| max_session_keys_(config.maxSessionKeys()) { | ||
| if (!parsed_alpn_protocols_.empty()) { | ||
| int rc = SSL_CTX_set_alpn_protos(ctx_.get(), &parsed_alpn_protocols_[0], | ||
| parsed_alpn_protocols_.size()); | ||
| RELEASE_ASSERT(rc == 0, ""); | ||
| } | ||
|
|
||
| if (max_session_keys_ > 0) { | ||
| SSL_CTX_set_session_cache_mode(ctx_.get(), SSL_SESS_CACHE_CLIENT); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we check or ASSERT errors for these BoringSSL calls?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not for those. |
||
| SSL_CTX_sess_set_new_cb(ctx_.get(), [](SSL* ssl, SSL_SESSION* session) -> int { | ||
| ContextImpl* context_impl = | ||
| static_cast<ContextImpl*>(SSL_CTX_get_ex_data(SSL_get_SSL_CTX(ssl), sslContextIndex())); | ||
| ClientContextImpl* client_context_impl = dynamic_cast<ClientContextImpl*>(context_impl); | ||
| RELEASE_ASSERT(client_context_impl != nullptr, ""); // for Coverity | ||
| return client_context_impl->newSessionKey(session); | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| bssl::UniquePtr<SSL> | ||
| ClientContextImpl::newSsl(absl::optional<std::string> override_server_name) const { | ||
| bssl::UniquePtr<SSL> ClientContextImpl::newSsl(absl::optional<std::string> override_server_name) { | ||
| bssl::UniquePtr<SSL> ssl_con(ContextImpl::newSsl(absl::nullopt)); | ||
|
|
||
| std::string server_name_indication = | ||
|
|
@@ -514,9 +525,51 @@ ClientContextImpl::newSsl(absl::optional<std::string> override_server_name) cons | |
| SSL_set_renegotiate_mode(ssl_con.get(), ssl_renegotiate_freely); | ||
| } | ||
|
|
||
| if (max_session_keys_ > 0) { | ||
| if (session_keys_single_use_) { | ||
| // Stored single-use session keys, use write/write locks. | ||
| absl::WriterMutexLock l(&session_keys_mu_); | ||
| if (!session_keys_.empty()) { | ||
| // Use the most recently stored session key, since it has the highest | ||
| // probability of still being recognized/accepted by the server. | ||
| SSL_SESSION* session = session_keys_.front().get(); | ||
| SSL_set_session(ssl_con.get(), session); | ||
| // Remove single-use session key (TLS 1.3) after first use. | ||
| if (SSL_SESSION_should_be_single_use(session)) { | ||
| session_keys_.pop_front(); | ||
| } | ||
| } | ||
| } else { | ||
| // Never stored single-use session keys, use read/write locks. | ||
| absl::ReaderMutexLock l(&session_keys_mu_); | ||
| if (!session_keys_.empty()) { | ||
| // Use the most recently stored session key, since it has the highest | ||
| // probability of still being recognized/accepted by the server. | ||
| SSL_SESSION* session = session_keys_.front().get(); | ||
| SSL_set_session(ssl_con.get(), session); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return ssl_con; | ||
| } | ||
|
|
||
| int ClientContextImpl::newSessionKey(SSL_SESSION* session) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we are safe for multi cert work, given that the client contexts will continue to have a single cert, but could you comment here on whether in the future, if we support multiple client certs, whether anything needs to change?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think that we'll ever support multiple client certificates that can affect sessions, since client certificates are not revalidated during session resumption by the server. In any case, this would be covered by #5073. |
||
| // In case we ever store single-use session key (TLS 1.3), | ||
| // we need to switch to using write/write locks. | ||
| if (SSL_SESSION_should_be_single_use(session)) { | ||
| session_keys_single_use_ = true; | ||
| } | ||
| absl::WriterMutexLock l(&session_keys_mu_); | ||
| // Evict oldest entries. | ||
| while (session_keys_.size() >= max_session_keys_) { | ||
| session_keys_.pop_back(); | ||
| } | ||
| // Add new session key at the front of the queue, so that it's used first. | ||
| session_keys_.push_front(bssl::UniquePtr<SSL_SESSION>(session)); | ||
| return 1; // Tell BoringSSL that we took ownership of the session. | ||
| } | ||
|
|
||
| ServerContextImpl::ServerContextImpl(Stats::Scope& scope, const ServerContextConfig& config, | ||
| const std::vector<std::string>& server_names, | ||
| TimeSource& time_source) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense (in the future) to have a corresponding server-side setting for how many session tickets a TLS 1.3 server will issue at a time? the RFC says:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but it's currently hardcoded to
static const int kNumTickets = 2in BoringSSL (see: https://boringssl.googlesource.com/boringssl/+/c0c9001440db8121bdc1ff1307b3a9aedf26fcd8/ssl/tls13_server.cc#165). cc @davidben