Send version negotiation in response to short connection IDs This code change is in response to the following spec change: https://github.com/quicwg/base-drafts/pull/4187/files We're now expected to not enforce the 64bit minimum on unknown versions. Protected by FLAGS_quic_reloadable_flag_quic_send_version_negotiation_for_short_connection_ids. PiperOrigin-RevId: 338099578 Change-Id: I9d28a38464fc8050c166d976bb12e7cafd62633c
diff --git a/quic/core/quic_dispatcher.cc b/quic/core/quic_dispatcher.cc index ac40b0a..11c6753 100644 --- a/quic/core/quic_dispatcher.cc +++ b/quic/core/quic_dispatcher.cc
@@ -465,7 +465,16 @@ // connection ID, the dispatcher picks a new one of its expected length. // Therefore we should never receive a connection ID that is smaller // than 64 bits and smaller than what we expect. - if (server_connection_id.length() < kQuicMinimumInitialConnectionIdLength && + bool should_check_short_connection_ids = true; + if (GetQuicReloadableFlag( + quic_send_version_negotiation_for_short_connection_ids)) { + QUIC_RELOADABLE_FLAG_COUNT( + quic_send_version_negotiation_for_short_connection_ids); + should_check_short_connection_ids = + packet_info.version_flag && packet_info.version.IsKnown(); + } + if (should_check_short_connection_ids && + server_connection_id.length() < kQuicMinimumInitialConnectionIdLength && server_connection_id.length() < expected_server_connection_id_length_ && !allow_short_initial_server_connection_ids_) { DCHECK(packet_info.version_flag);
diff --git a/quic/core/quic_dispatcher_test.cc b/quic/core/quic_dispatcher_test.cc index a88eadb..9f692a8 100644 --- a/quic/core/quic_dispatcher_test.cc +++ b/quic/core/quic_dispatcher_test.cc
@@ -471,6 +471,10 @@ void TestTlsMultiPacketClientHello(bool add_reordering); + void TestVersionNegotiationForUnknownVersionInvalidShortInitialConnectionId( + const QuicConnectionId& server_connection_id, + const QuicConnectionId& client_connection_id); + ParsedQuicVersion version_; MockQuicConnectionHelper mock_helper_; MockAlarmFactory mock_alarm_factory_; @@ -1054,7 +1058,7 @@ } TEST_P(QuicDispatcherTestAllVersions, - ProcessPacketWithInvalidShortInitialConnectionId) { + DropPacketWithKnownVersionAndInvalidShortInitialConnectionId) { if (!version_.AllowsVariableLengthConnectionIds()) { return; } @@ -1063,14 +1067,59 @@ QuicSocketAddress client_address(QuicIpAddress::Loopback4(), 1); // dispatcher_ should drop this packet. - EXPECT_CALL(*dispatcher_, CreateQuicSession(_, _, client_address, _, _)) - .Times(0); + EXPECT_CALL(*dispatcher_, CreateQuicSession(_, _, _, _, _)).Times(0); EXPECT_CALL(*time_wait_list_manager_, ProcessPacket(_, _, _, _, _)).Times(0); EXPECT_CALL(*time_wait_list_manager_, AddConnectionIdToTimeWait(_, _, _)) .Times(0); ProcessFirstFlight(client_address, EmptyQuicConnectionId()); } +void QuicDispatcherTestBase:: + TestVersionNegotiationForUnknownVersionInvalidShortInitialConnectionId( + const QuicConnectionId& server_connection_id, + const QuicConnectionId& client_connection_id) { + SetQuicReloadableFlag(quic_send_version_negotiation_for_short_connection_ids, + true); + CreateTimeWaitListManager(); + + QuicSocketAddress client_address(QuicIpAddress::Loopback4(), 1); + + EXPECT_CALL(*dispatcher_, CreateQuicSession(_, _, _, _, _)).Times(0); + EXPECT_CALL(*time_wait_list_manager_, + SendVersionNegotiationPacket( + server_connection_id, client_connection_id, + /*ietf_quic=*/true, + /*use_length_prefix=*/true, _, _, client_address, _)) + .Times(1); + ProcessFirstFlight(ParsedQuicVersion::ReservedForNegotiation(), + client_address, server_connection_id, + client_connection_id); +} + +TEST_P(QuicDispatcherTestOneVersion, + VersionNegotiationForUnknownVersionInvalidShortInitialConnectionId) { + TestVersionNegotiationForUnknownVersionInvalidShortInitialConnectionId( + EmptyQuicConnectionId(), EmptyQuicConnectionId()); +} + +TEST_P(QuicDispatcherTestOneVersion, + VersionNegotiationForUnknownVersionInvalidShortInitialConnectionId2) { + char server_connection_id_bytes[3] = {1, 2, 3}; + QuicConnectionId server_connection_id(server_connection_id_bytes, + sizeof(server_connection_id_bytes)); + TestVersionNegotiationForUnknownVersionInvalidShortInitialConnectionId( + server_connection_id, EmptyQuicConnectionId()); +} + +TEST_P(QuicDispatcherTestOneVersion, + VersionNegotiationForUnknownVersionInvalidShortInitialConnectionId3) { + char client_connection_id_bytes[8] = {1, 2, 3, 4, 5, 6, 7, 8}; + QuicConnectionId client_connection_id(client_connection_id_bytes, + sizeof(client_connection_id_bytes)); + TestVersionNegotiationForUnknownVersionInvalidShortInitialConnectionId( + EmptyQuicConnectionId(), client_connection_id); +} + TEST_P(QuicDispatcherTestOneVersion, VersionsChangeInFlight) { VerifyVersionNotSupported(QuicVersionReservedForNegotiation()); for (ParsedQuicVersion version : CurrentSupportedVersions()) {
diff --git a/quic/core/quic_framer.cc b/quic/core/quic_framer.cc index 19b0cd5..03b27f2 100644 --- a/quic/core/quic_framer.cc +++ b/quic/core/quic_framer.cc
@@ -6671,8 +6671,7 @@ return false; } if (destination_connection_id_length > kQuicMaxConnectionId4BitLength || - destination_connection_id_length < - kQuicMinimumInitialConnectionIdLength) { + destination_connection_id_length < kQuicDefaultConnectionIdLength) { QUIC_BUG << "Invalid connection_id_length"; return false; }
diff --git a/quic/core/quic_versions.cc b/quic/core/quic_versions.cc index d2b5774..98bef7f 100644 --- a/quic/core/quic_versions.cc +++ b/quic/core/quic_versions.cc
@@ -664,6 +664,8 @@ // Enable necessary flags. SetQuicRestartFlag(quic_enable_zero_rtt_for_tls_v2, true); SetQuicReloadableFlag(quic_key_update_supported, true); + SetQuicReloadableFlag(quic_send_version_negotiation_for_short_connection_ids, + true); } void QuicEnableVersion(const ParsedQuicVersion& version) {
diff --git a/quic/test_tools/quic_test_utils.cc b/quic/test_tools/quic_test_utils.cc index b286764..0357ee2 100644 --- a/quic/test_tools/quic_test_utils.cc +++ b/quic/test_tools/quic_test_utils.cc
@@ -1572,11 +1572,6 @@ QUIC_BUG << "Invalid destination_connection_id_length_out"; return false; } - if (*destination_connection_id_length_out < - kQuicMinimumInitialConnectionIdLength) { - QUIC_BUG << "Invalid *destination_connection_id_length_out"; - return false; - } QuicEncryptedPacket encrypted_packet(packet_bytes, packet_length); PacketHeaderFormat format; @@ -1601,12 +1596,6 @@ QUIC_BUG << "Packet is not a long header"; return false; } - if (destination_connection_id.length() < - kQuicMinimumInitialConnectionIdLength) { - QUIC_BUG << "Invalid destination_connection_id length " - << static_cast<int>(destination_connection_id.length()); - return false; - } if (*destination_connection_id_length_out < destination_connection_id.length()) { QUIC_BUG << "destination_connection_id_length_out too small";