Do not reuse tokens received in NEW_TOKEN frames for different connection attempts by: 1) add source address token to QuicClientSessionCache, 2) Clear token after use. Protected by FLAGS_quic_reloadable_flag_quic_tls_use_token_in_session_cache. PiperOrigin-RevId: 411536820
diff --git a/quic/core/crypto/quic_client_session_cache.cc b/quic/core/crypto/quic_client_session_cache.cc index 64c3ffd..97f7ee5 100644 --- a/quic/core/crypto/quic_client_session_cache.cc +++ b/quic/core/crypto/quic_client_session_cache.cc
@@ -87,6 +87,12 @@ state->application_state = std::make_unique<ApplicationState>(*iter->second->application_state); } + if (GetQuicReloadableFlag(quic_tls_use_token_in_session_cache) && + !iter->second->token.empty()) { + state->token = iter->second->token; + // Clear token after use. + iter->second->token.clear(); + } return state; } @@ -102,6 +108,18 @@ } } +void QuicClientSessionCache::OnNewTokenReceived(const QuicServerId& server_id, + absl::string_view token) { + if (token.empty()) { + return; + } + auto iter = cache_.Lookup(server_id); + if (iter == cache_.end()) { + return; + } + iter->second->token = token; +} + void QuicClientSessionCache::RemoveExpiredEntries(QuicWallTime now) { auto iter = cache_.begin(); while (iter != cache_.end()) {
diff --git a/quic/core/crypto/quic_client_session_cache.h b/quic/core/crypto/quic_client_session_cache.h index 667594f..38678af 100644 --- a/quic/core/crypto/quic_client_session_cache.h +++ b/quic/core/crypto/quic_client_session_cache.h
@@ -13,6 +13,10 @@ namespace quic { +namespace test { +class QuicClientSessionCachePeer; +} // namespace test + // QuicClientSessionCache maps from QuicServerId to information used to resume // TLS sessions for that server. class QUIC_EXPORT_PRIVATE QuicClientSessionCache : public SessionCache { @@ -32,6 +36,9 @@ void ClearEarlyData(const QuicServerId& server_id) override; + void OnNewTokenReceived(const QuicServerId& server_id, + absl::string_view token) override; + void RemoveExpiredEntries(QuicWallTime now) override; void Clear() override; @@ -39,6 +46,8 @@ size_t size() const { return cache_.Size(); } private: + friend class test::QuicClientSessionCachePeer; + struct QUIC_EXPORT_PRIVATE Entry { Entry(); Entry(Entry&&); @@ -56,6 +65,7 @@ bssl::UniquePtr<SSL_SESSION> sessions[2]; std::unique_ptr<TransportParameters> params; std::unique_ptr<ApplicationState> application_state; + std::string token; // An opaque string received in NEW_TOKEN frame. }; // Creates a new entry and insert into |cache_|.
diff --git a/quic/core/crypto/quic_crypto_client_config.h b/quic/core/crypto/quic_crypto_client_config.h index 694e068..1351729 100644 --- a/quic/core/crypto/quic_crypto_client_config.h +++ b/quic/core/crypto/quic_crypto_client_config.h
@@ -49,6 +49,9 @@ // client received from the server at the application layer that the client // needs to remember when performing a 0-RTT handshake. std::unique_ptr<ApplicationState> application_state = nullptr; + + // Opaque token received in NEW_TOKEN frame if any. + std::string token; }; // SessionCache is an interface for managing storing and retrieving @@ -80,6 +83,10 @@ // associated with |server_id|. virtual void ClearEarlyData(const QuicServerId& server_id) = 0; + // Called when NEW_TOKEN frame is received. + virtual void OnNewTokenReceived(const QuicServerId& server_id, + absl::string_view token) = 0; + // Called to remove expired entries. virtual void RemoveExpiredEntries(QuicWallTime now) = 0;
diff --git a/quic/core/http/end_to_end_test.cc b/quic/core/http/end_to_end_test.cc index ee891dc..5d2d884 100644 --- a/quic/core/http/end_to_end_test.cc +++ b/quic/core/http/end_to_end_test.cc
@@ -47,6 +47,7 @@ #include "quic/test_tools/qpack/qpack_encoder_test_utils.h" #include "quic/test_tools/qpack/qpack_test_utils.h" #include "quic/test_tools/quic_client_peer.h" +#include "quic/test_tools/quic_client_session_cache_peer.h" #include "quic/test_tools/quic_config_peer.h" #include "quic/test_tools/quic_connection_peer.h" #include "quic/test_tools/quic_dispatcher_peer.h" @@ -1702,6 +1703,61 @@ server_thread_->Resume(); client_->Disconnect(); + + // Regression test for b/206087883. + // Mock server crash. + StopServer(); + + // The handshake fails due to idle timeout. + client_->Connect(); + ASSERT_FALSE(client_->client()->WaitForOneRttKeysAvailable()); + client_->WaitForWriteToFlush(); + client_->WaitForResponse(); + ASSERT_FALSE(client_->client()->connected()); + EXPECT_THAT(client_->connection_error(), IsError(QUIC_NETWORK_IDLE_TIMEOUT)); + + // Server restarts. + server_writer_ = new PacketDroppingTestWriter(); + StartServer(); + + // Client re-connect. + client_->Connect(); + ASSERT_TRUE(client_->client()->WaitForOneRttKeysAvailable()); + client_->WaitForWriteToFlush(); + client_->WaitForResponse(); + ASSERT_TRUE(client_->client()->connected()); + client_session = GetClientSession(); + ASSERT_TRUE(client_session); + EXPECT_FALSE(client_session->EarlyDataAccepted()); + EXPECT_FALSE(client_->client()->EarlyDataAccepted()); + server_thread_->Pause(); + server_session = GetServerSession(); + server_connection = GetServerConnection(); + if (!GetQuicReloadableFlag(quic_tls_use_token_in_session_cache)) { + // Address token is reused. + if (server_session != nullptr && server_connection != nullptr) { + // Verify address is validated via validating token received in INITIAL + // packet. + EXPECT_FALSE(server_connection->GetStats() + .address_validated_via_decrypting_packet); + EXPECT_TRUE(server_connection->GetStats().address_validated_via_token); + } else { + ADD_FAILURE() << "Missing server connection"; + } + } else { + // Verify address token is only used once. + if (server_session != nullptr && server_connection != nullptr) { + // Verify address is validated via decrypting packet. + EXPECT_TRUE(server_connection->GetStats() + .address_validated_via_decrypting_packet); + EXPECT_FALSE(server_connection->GetStats().address_validated_via_token); + } else { + ADD_FAILURE() << "Missing server connection"; + } + } + server_thread_->Resume(); + + client_->Disconnect(); } TEST_P(EndToEndTest, AddressTokenRefreshedByServer) { @@ -1721,8 +1777,16 @@ client_->Disconnect(); - std::string old_address_token = - client_crypto_config->LookupOrCreate(server_id)->source_address_token(); + QuicClientSessionCache* session_cache = static_cast<QuicClientSessionCache*>( + client_crypto_config->mutable_session_cache()); + std::string old_address_token; + if (GetQuicReloadableFlag(quic_tls_use_token_in_session_cache)) { + old_address_token = + QuicClientSessionCachePeer::GetToken(session_cache, server_id); + } else { + old_address_token = + client_crypto_config->LookupOrCreate(server_id)->source_address_token(); + } ASSERT_TRUE(!old_address_token.empty()); SetQuicReloadableFlag(quic_add_cached_network_parameters_to_address_token, @@ -1750,8 +1814,14 @@ client_->Disconnect(); - std::string new_address_token = - client_crypto_config->LookupOrCreate(server_id)->source_address_token(); + std::string new_address_token; + if (GetQuicReloadableFlag(quic_tls_use_token_in_session_cache)) { + new_address_token = + QuicClientSessionCachePeer::GetToken(session_cache, server_id); + } else { + new_address_token = + client_crypto_config->LookupOrCreate(server_id)->source_address_token(); + } ASSERT_TRUE(!new_address_token.empty()); ASSERT_NE(new_address_token, old_address_token); } @@ -1772,8 +1842,16 @@ client_->Disconnect(); - std::string old_address_token = - client_crypto_config->LookupOrCreate(server_id)->source_address_token(); + QuicClientSessionCache* session_cache = static_cast<QuicClientSessionCache*>( + client_crypto_config->mutable_session_cache()); + std::string old_address_token; + if (GetQuicReloadableFlag(quic_tls_use_token_in_session_cache)) { + old_address_token = + QuicClientSessionCachePeer::GetToken(session_cache, server_id); + } else { + old_address_token = + client_crypto_config->LookupOrCreate(server_id)->source_address_token(); + } ASSERT_TRUE(!old_address_token.empty()); // Pause the server thread again to blackhole packets from client. @@ -1782,10 +1860,17 @@ EXPECT_FALSE(client_->client()->WaitForOneRttKeysAvailable()); EXPECT_FALSE(client_->client()->connected()); - std::string new_address_token = - client_crypto_config->LookupOrCreate(server_id)->source_address_token(); - // TODO(b/206087883): This currently fails, fix the client and uncomment it. - // ASSERT_TRUE(new_address_token.empty()); + std::string new_address_token; + if (GetQuicReloadableFlag(quic_tls_use_token_in_session_cache)) { + new_address_token = + QuicClientSessionCachePeer::GetToken(session_cache, server_id); + // Verify address token gets cleared. + ASSERT_TRUE(new_address_token.empty()); + } else { + new_address_token = + client_crypto_config->LookupOrCreate(server_id)->source_address_token(); + ASSERT_FALSE(new_address_token.empty()); + } server_thread_->Resume(); }
diff --git a/quic/core/quic_dispatcher_test.cc b/quic/core/quic_dispatcher_test.cc index d4af035..199270e 100644 --- a/quic/core/quic_dispatcher_test.cc +++ b/quic/core/quic_dispatcher_test.cc
@@ -454,7 +454,8 @@ ParsedClientHello parsed_chlo; parsed_chlo.alpns = {ExpectedAlpn()}; parsed_chlo.sni = TestHostname(); - if (address_token_.has_value()) { + if (address_token_.has_value() && + !GetQuicReloadableFlag(quic_tls_use_token_in_session_cache)) { parsed_chlo.retry_token = *address_token_; } return parsed_chlo;
diff --git a/quic/core/quic_flags_list.h b/quic/core/quic_flags_list.h index 4c3a5e8..518b7c3 100644 --- a/quic/core/quic_flags_list.h +++ b/quic/core/quic_flags_list.h
@@ -115,6 +115,8 @@ QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_conservative_cwnd_and_pacing_gains, false) // If true, validate that peer owns the new address once the server detects peer migration or is probed from that address, and also apply anti-amplification limit while sending to that address. QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_server_reverse_validate_new_path3, true) +// If true, when client attempts TLS resumption, use token in session_cache_ instead of cached_states_ in QuicCryptoClientConfig. +QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_tls_use_token_in_session_cache, true) // When receiving STOP_SENDING, send a RESET_STREAM with a matching error code. QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_match_ietf_reset_code, true) // When the flag is true, exit STARTUP after the same number of loss events as PROBE_UP.
diff --git a/quic/core/tls_client_handshaker.cc b/quic/core/tls_client_handshaker.cc index 211dfde..c8306a8 100644 --- a/quic/core/tls_client_handshaker.cc +++ b/quic/core/tls_client_handshaker.cc
@@ -42,10 +42,12 @@ has_application_state_(has_application_state), crypto_config_(crypto_config), tls_connection_(crypto_config->ssl_ctx(), this, session->GetSSLConfig()) { - std::string token = - crypto_config->LookupOrCreate(server_id)->source_address_token(); - if (!token.empty()) { - session->SetSourceAddressTokenToSend(token); + if (!GetQuicReloadableFlag(quic_tls_use_token_in_session_cache)) { + std::string token = + crypto_config->LookupOrCreate(server_id)->source_address_token(); + if (!token.empty()) { + session->SetSourceAddressTokenToSend(token); + } } if (crypto_config->tls_signature_algorithms().has_value()) { SSL_set1_sigalgs_list(ssl(), @@ -125,6 +127,10 @@ } if (cached_state_) { SSL_set_session(ssl(), cached_state_->tls_session.get()); + if (GetQuicReloadableFlag(quic_tls_use_token_in_session_cache) && + !cached_state_->token.empty()) { + session()->SetSourceAddressTokenToSend(cached_state_->token); + } } // Start the handshake. @@ -434,9 +440,15 @@ if (token.empty()) { return; } - QuicCryptoClientConfig::CachedState* cached = - crypto_config_->LookupOrCreate(server_id_); - cached->set_source_address_token(token); + if (GetQuicReloadableFlag(quic_tls_use_token_in_session_cache)) { + if (session_cache_ != nullptr) { + session_cache_->OnNewTokenReceived(server_id_, token); + } + } else { + QuicCryptoClientConfig::CachedState* cached = + crypto_config_->LookupOrCreate(server_id_); + cached->set_source_address_token(token); + } } void TlsClientHandshaker::SetWriteSecret(
diff --git a/quic/test_tools/quic_client_session_cache_peer.h b/quic/test_tools/quic_client_session_cache_peer.h new file mode 100644 index 0000000..6f0c667 --- /dev/null +++ b/quic/test_tools/quic_client_session_cache_peer.h
@@ -0,0 +1,28 @@ +// Copyright (c) 2021 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef QUICHE_QUIC_TEST_TOOLS_QUIC_CLIENT_SESSION_CACHE_PEER_H_ +#define QUICHE_QUIC_TEST_TOOLS_QUIC_CLIENT_SESSION_CACHE_PEER_H_ + +#include "quic/core/crypto/quic_client_session_cache.h" + +namespace quic { +namespace test { + +class QuicClientSessionCachePeer { + public: + static std::string GetToken(QuicClientSessionCache* cache, + const QuicServerId& server_id) { + auto iter = cache->cache_.Lookup(server_id); + if (iter == cache->cache_.end()) { + return {}; + } + return iter->second->token; + } +}; + +} // namespace test +} // namespace quic + +#endif // QUICHE_QUIC_TEST_TOOLS_QUIC_CLIENT_SESSION_CACHE_PEER_H_
diff --git a/quic/test_tools/simple_session_cache.cc b/quic/test_tools/simple_session_cache.cc index d2f5cf8..2d23673 100644 --- a/quic/test_tools/simple_session_cache.cc +++ b/quic/test_tools/simple_session_cache.cc
@@ -48,6 +48,7 @@ } state->transport_params = std::make_unique<TransportParameters>(*it->second.params); + state->token = it->second.token; return state; } @@ -56,6 +57,15 @@ // do anything here. } +void SimpleSessionCache::OnNewTokenReceived(const QuicServerId& server_id, + absl::string_view token) { + auto it = cache_entries_.find(server_id); + if (it == cache_entries_.end()) { + return; + } + it->second.token = token; +} + void SimpleSessionCache::RemoveExpiredEntries(QuicWallTime /*now*/) { // The simple session cache does not support removing expired entries. }
diff --git a/quic/test_tools/simple_session_cache.h b/quic/test_tools/simple_session_cache.h index 54bb0c7..6b4111e 100644 --- a/quic/test_tools/simple_session_cache.h +++ b/quic/test_tools/simple_session_cache.h
@@ -31,6 +31,8 @@ QuicWallTime now, const SSL_CTX* ctx) override; void ClearEarlyData(const QuicServerId& server_id) override; + void OnNewTokenReceived(const QuicServerId& server_id, + absl::string_view token) override; void RemoveExpiredEntries(QuicWallTime now) override; void Clear() override; @@ -39,6 +41,7 @@ bssl::UniquePtr<SSL_SESSION> session; std::unique_ptr<TransportParameters> params; std::unique_ptr<ApplicationState> application_state; + std::string token; }; std::map<QuicServerId, Entry> cache_entries_; };