Deprecate quic_prober_uses_length_prefixed_connection_ids PiperOrigin-RevId: 337975193 Change-Id: I5c365092604eaf42055c1f6b6e52153aaaeb1716
diff --git a/quic/core/quic_dispatcher_test.cc b/quic/core/quic_dispatcher_test.cc index 259c734..bfeb0d4 100644 --- a/quic/core/quic_dispatcher_test.cc +++ b/quic/core/quic_dispatcher_test.cc
@@ -1206,37 +1206,7 @@ "Please add new RejectDeprecatedVersion tests above this assert " "when deprecating versions"); -TEST_P(QuicDispatcherTestOneVersion, VersionNegotiationProbeOld) { - SetQuicFlag(FLAGS_quic_prober_uses_length_prefixed_connection_ids, false); - QuicSocketAddress client_address(QuicIpAddress::Loopback4(), 1); - CreateTimeWaitListManager(); - char packet[1200]; - char destination_connection_id_bytes[] = {0x56, 0x4e, 0x20, 0x70, - 0x6c, 0x7a, 0x20, 0x21}; - EXPECT_TRUE(QuicFramer::WriteClientVersionNegotiationProbePacket( - packet, sizeof(packet), destination_connection_id_bytes, - sizeof(destination_connection_id_bytes))); - QuicEncryptedPacket encrypted(packet, sizeof(packet), false); - std::unique_ptr<QuicReceivedPacket> received_packet( - ConstructReceivedPacket(encrypted, mock_helper_.GetClock()->Now())); - QuicConnectionId client_connection_id = EmptyQuicConnectionId(); - QuicConnectionId server_connection_id( - destination_connection_id_bytes, sizeof(destination_connection_id_bytes)); - bool ietf_quic = true; - bool use_length_prefix = - GetQuicFlag(FLAGS_quic_prober_uses_length_prefixed_connection_ids); - EXPECT_CALL( - *time_wait_list_manager_, - SendVersionNegotiationPacket(server_connection_id, client_connection_id, - ietf_quic, use_length_prefix, _, _, _, _)) - .Times(1); - EXPECT_CALL(*dispatcher_, CreateQuicSession(_, _, _, _, _)).Times(0); - - dispatcher_->ProcessPacket(server_address_, client_address, *received_packet); -} - TEST_P(QuicDispatcherTestOneVersion, VersionNegotiationProbe) { - SetQuicFlag(FLAGS_quic_prober_uses_length_prefixed_connection_ids, true); QuicSocketAddress client_address(QuicIpAddress::Loopback4(), 1); CreateTimeWaitListManager(); char packet[1200]; @@ -1251,13 +1221,10 @@ QuicConnectionId client_connection_id = EmptyQuicConnectionId(); QuicConnectionId server_connection_id( destination_connection_id_bytes, sizeof(destination_connection_id_bytes)); - bool ietf_quic = true; - bool use_length_prefix = - GetQuicFlag(FLAGS_quic_prober_uses_length_prefixed_connection_ids); - EXPECT_CALL( - *time_wait_list_manager_, - SendVersionNegotiationPacket(server_connection_id, client_connection_id, - ietf_quic, use_length_prefix, _, _, _, _)) + EXPECT_CALL(*time_wait_list_manager_, + SendVersionNegotiationPacket( + server_connection_id, client_connection_id, + /*ietf_quic=*/true, /*use_length_prefix=*/true, _, _, _, _)) .Times(1); EXPECT_CALL(*dispatcher_, CreateQuicSession(_, _, _, _, _)).Times(0); @@ -1288,53 +1255,7 @@ std::vector<std::unique_ptr<QuicEncryptedPacket>> packets_; }; -TEST_P(QuicDispatcherTestOneVersion, VersionNegotiationProbeEndToEndOld) { - SetQuicFlag(FLAGS_quic_prober_uses_length_prefixed_connection_ids, false); - - SavingWriter* saving_writer = new SavingWriter(); - // dispatcher_ takes ownership of saving_writer. - QuicDispatcherPeer::UseWriter(dispatcher_.get(), saving_writer); - - QuicTimeWaitListManager* time_wait_list_manager = new QuicTimeWaitListManager( - saving_writer, dispatcher_.get(), mock_helper_.GetClock(), - &mock_alarm_factory_); - // dispatcher_ takes ownership of time_wait_list_manager. - QuicDispatcherPeer::SetTimeWaitListManager(dispatcher_.get(), - time_wait_list_manager); - char packet[1200] = {}; - char destination_connection_id_bytes[] = {0x56, 0x4e, 0x20, 0x70, - 0x6c, 0x7a, 0x20, 0x21}; - EXPECT_TRUE(QuicFramer::WriteClientVersionNegotiationProbePacket( - packet, sizeof(packet), destination_connection_id_bytes, - sizeof(destination_connection_id_bytes))); - QuicEncryptedPacket encrypted(packet, sizeof(packet), false); - std::unique_ptr<QuicReceivedPacket> received_packet( - ConstructReceivedPacket(encrypted, mock_helper_.GetClock()->Now())); - EXPECT_CALL(*dispatcher_, CreateQuicSession(_, _, _, _, _)).Times(0); - - QuicSocketAddress client_address(QuicIpAddress::Loopback4(), 1); - dispatcher_->ProcessPacket(server_address_, client_address, *received_packet); - ASSERT_EQ(1u, saving_writer->packets()->size()); - - char source_connection_id_bytes[255] = {}; - uint8_t source_connection_id_length = sizeof(source_connection_id_bytes); - std::string detailed_error = "foobar"; - EXPECT_TRUE(QuicFramer::ParseServerVersionNegotiationProbeResponse( - (*(saving_writer->packets()))[0]->data(), - (*(saving_writer->packets()))[0]->length(), source_connection_id_bytes, - &source_connection_id_length, &detailed_error)); - EXPECT_EQ("", detailed_error); - - // The source connection ID of the probe response should match the - // destination connection ID of the probe request. - quiche::test::CompareCharArraysWithHexError( - "parsed probe", source_connection_id_bytes, source_connection_id_length, - destination_connection_id_bytes, sizeof(destination_connection_id_bytes)); -} - TEST_P(QuicDispatcherTestOneVersion, VersionNegotiationProbeEndToEnd) { - SetQuicFlag(FLAGS_quic_prober_uses_length_prefixed_connection_ids, true); - SavingWriter* saving_writer = new SavingWriter(); // dispatcher_ takes ownership of saving_writer. QuicDispatcherPeer::UseWriter(dispatcher_.get(), saving_writer);
diff --git a/quic/core/quic_framer.cc b/quic/core/quic_framer.cc index a5cc8bd..884f7eb 100644 --- a/quic/core/quic_framer.cc +++ b/quic/core/quic_framer.cc
@@ -6676,9 +6676,6 @@ QUIC_BUG << "Invalid connection_id_length"; return false; } - const bool use_length_prefix = - GetQuicFlag(FLAGS_quic_prober_uses_length_prefixed_connection_ids); - const uint8_t last_version_byte = use_length_prefix ? 0xda : 0xba; // clang-format off const unsigned char packet_start_bytes[] = { // IETF long header with fixed bit set, type initial, all-0 encrypted bits. @@ -6686,7 +6683,7 @@ // Version, part of the IETF space reserved for negotiation. // This intentionally differs from QuicVersionReservedForNegotiation() // to allow differentiating them over the wire. - 0xca, 0xba, 0xda, last_version_byte, + 0xca, 0xba, 0xda, 0xda, }; // clang-format on static_assert(sizeof(packet_start_bytes) == 5, "bad packet_start_bytes size"); @@ -6699,8 +6696,8 @@ QuicConnectionId destination_connection_id(destination_connection_id_bytes, destination_connection_id_length); if (!AppendIetfConnectionIds( - /*version_flag=*/true, use_length_prefix, destination_connection_id, - EmptyQuicConnectionId(), &writer)) { + /*version_flag=*/true, /*use_length_prefix=*/true, + destination_connection_id, EmptyQuicConnectionId(), &writer)) { QUIC_BUG << "Failed to write connection IDs"; return false; } @@ -6784,40 +6781,15 @@ *detailed_error = "Packet is not a version negotiation packet"; return false; } - const bool use_length_prefix = - GetQuicFlag(FLAGS_quic_prober_uses_length_prefixed_connection_ids); + QuicConnectionId destination_connection_id, source_connection_id; - if (use_length_prefix) { - if (!reader.ReadLengthPrefixedConnectionId(&destination_connection_id)) { - *detailed_error = "Failed to read destination connection ID"; - return false; - } - if (!reader.ReadLengthPrefixedConnectionId(&source_connection_id)) { - *detailed_error = "Failed to read source connection ID"; - return false; - } - } else { - uint8_t expected_server_connection_id_length = 0, - destination_connection_id_length = 0, - source_connection_id_length = 0; - if (!ProcessAndValidateIetfConnectionIdLength( - &reader, UnsupportedQuicVersion(), Perspective::IS_CLIENT, - /*should_update_expected_server_connection_id_length=*/true, - &expected_server_connection_id_length, - &destination_connection_id_length, &source_connection_id_length, - detailed_error)) { - return false; - } - if (!reader.ReadConnectionId(&destination_connection_id, - destination_connection_id_length)) { - *detailed_error = "Failed to read destination connection ID"; - return false; - } - if (!reader.ReadConnectionId(&source_connection_id, - source_connection_id_length)) { - *detailed_error = "Failed to read source connection ID"; - return false; - } + if (!reader.ReadLengthPrefixedConnectionId(&destination_connection_id)) { + *detailed_error = "Failed to read destination connection ID"; + return false; + } + if (!reader.ReadLengthPrefixedConnectionId(&source_connection_id)) { + *detailed_error = "Failed to read source connection ID"; + return false; } if (destination_connection_id.length() != 0) {
diff --git a/quic/core/quic_framer_test.cc b/quic/core/quic_framer_test.cc index d9baee2..f0f2de0 100644 --- a/quic/core/quic_framer_test.cc +++ b/quic/core/quic_framer_test.cc
@@ -14003,106 +14003,7 @@ CheckFramingBoundaries(packet, QUIC_INVALID_PACKET_HEADER); } -TEST_P(QuicFramerTest, WriteClientVersionNegotiationProbePacketOld) { - SetQuicFlag(FLAGS_quic_prober_uses_length_prefixed_connection_ids, false); - // clang-format off - static const char expected_packet[1200] = { - // IETF long header with fixed bit set, type initial, all-0 encrypted bits. - 0xc0, - // Version, part of the IETF space reserved for negotiation. - 0xca, 0xba, 0xda, 0xba, - // Destination connection ID length 8, source connection ID length 0. - 0x50, - // 8-byte destination connection ID. - 0x56, 0x4e, 0x20, 0x70, 0x6c, 0x7a, 0x20, 0x21, - // 8 bytes of zeroes followed by 8 bytes of ones to ensure that this does - // not parse with any known version. - 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, - 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, - // 2 bytes of zeroes to pad to 16 byte boundary. - 0x00, 0x00, - // A polite greeting in case a human sees this in tcpdump. - 0x54, 0x68, 0x69, 0x73, 0x20, 0x70, 0x61, 0x63, - 0x6b, 0x65, 0x74, 0x20, 0x6f, 0x6e, 0x6c, 0x79, - 0x20, 0x65, 0x78, 0x69, 0x73, 0x74, 0x73, 0x20, - 0x74, 0x6f, 0x20, 0x74, 0x72, 0x69, 0x67, 0x67, - 0x65, 0x72, 0x20, 0x49, 0x45, 0x54, 0x46, 0x20, - 0x51, 0x55, 0x49, 0x43, 0x20, 0x76, 0x65, 0x72, - 0x73, 0x69, 0x6f, 0x6e, 0x20, 0x6e, 0x65, 0x67, - 0x6f, 0x74, 0x69, 0x61, 0x74, 0x69, 0x6f, 0x6e, - 0x2e, 0x20, 0x50, 0x6c, 0x65, 0x61, 0x73, 0x65, - 0x20, 0x72, 0x65, 0x73, 0x70, 0x6f, 0x6e, 0x64, - 0x20, 0x77, 0x69, 0x74, 0x68, 0x20, 0x61, 0x20, - 0x56, 0x65, 0x72, 0x73, 0x69, 0x6f, 0x6e, 0x20, - 0x4e, 0x65, 0x67, 0x6f, 0x74, 0x69, 0x61, 0x74, - 0x69, 0x6f, 0x6e, 0x20, 0x70, 0x61, 0x63, 0x6b, - 0x65, 0x74, 0x20, 0x69, 0x6e, 0x64, 0x69, 0x63, - 0x61, 0x74, 0x69, 0x6e, 0x67, 0x20, 0x77, 0x68, - 0x61, 0x74, 0x20, 0x76, 0x65, 0x72, 0x73, 0x69, - 0x6f, 0x6e, 0x73, 0x20, 0x79, 0x6f, 0x75, 0x20, - 0x73, 0x75, 0x70, 0x70, 0x6f, 0x72, 0x74, 0x2e, - 0x20, 0x54, 0x68, 0x61, 0x6e, 0x6b, 0x20, 0x79, - 0x6f, 0x75, 0x20, 0x61, 0x6e, 0x64, 0x20, 0x68, - 0x61, 0x76, 0x65, 0x20, 0x61, 0x20, 0x6e, 0x69, - 0x63, 0x65, 0x20, 0x64, 0x61, 0x79, 0x2e, 0x00, - }; - // clang-format on - char packet[1200]; - char destination_connection_id_bytes[] = {0x56, 0x4e, 0x20, 0x70, - 0x6c, 0x7a, 0x20, 0x21}; - EXPECT_TRUE(QuicFramer::WriteClientVersionNegotiationProbePacket( - packet, sizeof(packet), destination_connection_id_bytes, - sizeof(destination_connection_id_bytes))); - quiche::test::CompareCharArraysWithHexError("constructed packet", packet, - sizeof(packet), expected_packet, - sizeof(expected_packet)); - QuicEncryptedPacket encrypted(reinterpret_cast<const char*>(packet), - sizeof(packet), false); - // Make sure we fail to parse this packet for the version under test. - if (!framer_.version().HasIetfInvariantHeader()) { - // We can only parse the connection ID with an IETF parser. - EXPECT_FALSE(framer_.ProcessPacket(encrypted)); - return; - } - EXPECT_TRUE(framer_.ProcessPacket(encrypted)); - ASSERT_TRUE(visitor_.header_.get()); - QuicConnectionId probe_payload_connection_id( - reinterpret_cast<const char*>(destination_connection_id_bytes), - sizeof(destination_connection_id_bytes)); - EXPECT_EQ(probe_payload_connection_id, - visitor_.header_.get()->destination_connection_id); - - PacketHeaderFormat format = GOOGLE_QUIC_PACKET; - QuicLongHeaderType long_packet_type = INVALID_PACKET_TYPE; - bool version_present = false, has_length_prefix = false; - QuicVersionLabel version_label = 0; - ParsedQuicVersion parsed_version = QuicVersionReservedForNegotiation(); - QuicConnectionId destination_connection_id = TestConnectionId(0x33); - QuicConnectionId source_connection_id = TestConnectionId(0x34); - bool retry_token_present = true; - absl::string_view retry_token; - std::string detailed_error = "foobar"; - - QuicErrorCode parse_result = QuicFramer::ParsePublicHeaderDispatcher( - encrypted, kQuicDefaultConnectionIdLength, &format, &long_packet_type, - &version_present, &has_length_prefix, &version_label, &parsed_version, - &destination_connection_id, &source_connection_id, &retry_token_present, - &retry_token, &detailed_error); - EXPECT_THAT(parse_result, IsQuicNoError()); - EXPECT_EQ(IETF_QUIC_LONG_HEADER_PACKET, format); - EXPECT_TRUE(version_present); - EXPECT_FALSE(has_length_prefix); - EXPECT_EQ(0xcabadaba, version_label); - EXPECT_EQ(QUIC_VERSION_UNSUPPORTED, parsed_version.transport_version); - EXPECT_EQ(probe_payload_connection_id, destination_connection_id); - EXPECT_EQ(EmptyQuicConnectionId(), source_connection_id); - EXPECT_FALSE(retry_token_present); - EXPECT_EQ(absl::string_view(), retry_token); - EXPECT_EQ("", detailed_error); -} - TEST_P(QuicFramerTest, WriteClientVersionNegotiationProbePacket) { - SetQuicFlag(FLAGS_quic_prober_uses_length_prefixed_connection_ids, true); // clang-format off static const char expected_packet[1200] = { // IETF long header with fixed bit set, type initial, all-0 encrypted bits. @@ -14328,39 +14229,7 @@ EXPECT_EQ("", detailed_error); } -TEST_P(QuicFramerTest, ParseServerVersionNegotiationProbeResponseOld) { - SetQuicFlag(FLAGS_quic_prober_uses_length_prefixed_connection_ids, false); - // clang-format off - const char packet[] = { - // IETF long header with fixed bit set, type initial, all-0 encrypted bits. - 0xc0, - // Version of 0, indicating version negotiation. - 0x00, 0x00, 0x00, 0x00, - // Destination connection ID length 0, source connection ID length 8. - 0x05, - // 8-byte source connection ID. - 0x56, 0x4e, 0x20, 0x70, 0x6c, 0x7a, 0x20, 0x21, - // A few supported versions. - 0xaa, 0xaa, 0xaa, 0xaa, - QUIC_VERSION_BYTES, - }; - // clang-format on - char probe_payload_bytes[] = {0x56, 0x4e, 0x20, 0x70, 0x6c, 0x7a, 0x20, 0x21}; - char parsed_probe_payload_bytes[255] = {}; - uint8_t parsed_probe_payload_length = sizeof(parsed_probe_payload_bytes); - std::string parse_detailed_error = ""; - EXPECT_TRUE(QuicFramer::ParseServerVersionNegotiationProbeResponse( - reinterpret_cast<const char*>(packet), sizeof(packet), - reinterpret_cast<char*>(parsed_probe_payload_bytes), - &parsed_probe_payload_length, &parse_detailed_error)); - EXPECT_EQ("", parse_detailed_error); - quiche::test::CompareCharArraysWithHexError( - "parsed probe", parsed_probe_payload_bytes, parsed_probe_payload_length, - probe_payload_bytes, sizeof(probe_payload_bytes)); -} - TEST_P(QuicFramerTest, ParseServerVersionNegotiationProbeResponse) { - SetQuicFlag(FLAGS_quic_prober_uses_length_prefixed_connection_ids, true); // clang-format off const char packet[] = { // IETF long header with fixed bit set, type initial, all-0 encrypted bits. @@ -14391,7 +14260,6 @@ } TEST_P(QuicFramerTest, ParseClientVersionNegotiationProbePacket) { - SetQuicFlag(FLAGS_quic_prober_uses_length_prefixed_connection_ids, true); char packet[1200]; char input_destination_connection_id_bytes[] = {0x56, 0x4e, 0x20, 0x70, 0x6c, 0x7a, 0x20, 0x21}; @@ -14413,52 +14281,6 @@ } TEST_P(QuicFramerTest, WriteServerVersionNegotiationProbeResponse) { - SetQuicFlag(FLAGS_quic_prober_uses_length_prefixed_connection_ids, true); - char packet[1200]; - size_t packet_length = sizeof(packet); - char input_source_connection_id_bytes[] = {0x56, 0x4e, 0x20, 0x70, - 0x6c, 0x7a, 0x20, 0x21}; - ASSERT_TRUE(WriteServerVersionNegotiationProbeResponse( - packet, &packet_length, input_source_connection_id_bytes, - sizeof(input_source_connection_id_bytes))); - char parsed_source_connection_id_bytes[255] = {0}; - uint8_t parsed_source_connection_id_length = - sizeof(parsed_source_connection_id_bytes); - std::string detailed_error; - ASSERT_TRUE(QuicFramer::ParseServerVersionNegotiationProbeResponse( - packet, packet_length, parsed_source_connection_id_bytes, - &parsed_source_connection_id_length, &detailed_error)) - << detailed_error; - quiche::test::CompareCharArraysWithHexError( - "parsed destination connection ID", parsed_source_connection_id_bytes, - parsed_source_connection_id_length, input_source_connection_id_bytes, - sizeof(input_source_connection_id_bytes)); -} - -TEST_P(QuicFramerTest, ParseClientVersionNegotiationProbePacketOld) { - SetQuicFlag(FLAGS_quic_prober_uses_length_prefixed_connection_ids, false); - char packet[1200]; - char input_destination_connection_id_bytes[] = {0x56, 0x4e, 0x20, 0x70, - 0x6c, 0x7a, 0x20, 0x21}; - ASSERT_TRUE(QuicFramer::WriteClientVersionNegotiationProbePacket( - packet, sizeof(packet), input_destination_connection_id_bytes, - sizeof(input_destination_connection_id_bytes))); - char parsed_destination_connection_id_bytes[255] = {0}; - uint8_t parsed_destination_connection_id_length = - sizeof(parsed_destination_connection_id_bytes); - ASSERT_TRUE(ParseClientVersionNegotiationProbePacket( - packet, sizeof(packet), parsed_destination_connection_id_bytes, - &parsed_destination_connection_id_length)); - quiche::test::CompareCharArraysWithHexError( - "parsed destination connection ID", - parsed_destination_connection_id_bytes, - parsed_destination_connection_id_length, - input_destination_connection_id_bytes, - sizeof(input_destination_connection_id_bytes)); -} - -TEST_P(QuicFramerTest, WriteServerVersionNegotiationProbeResponseOld) { - SetQuicFlag(FLAGS_quic_prober_uses_length_prefixed_connection_ids, false); char packet[1200]; size_t packet_length = sizeof(packet); char input_source_connection_id_bytes[] = {0x56, 0x4e, 0x20, 0x70,
diff --git a/quic/test_tools/quic_test_utils.cc b/quic/test_tools/quic_test_utils.cc index 5321868..9f58985 100644 --- a/quic/test_tools/quic_test_utils.cc +++ b/quic/test_tools/quic_test_utils.cc
@@ -1531,12 +1531,11 @@ } QuicConnectionId source_connection_id(source_connection_id_bytes, source_connection_id_length); - const bool use_length_prefix = - GetQuicFlag(FLAGS_quic_prober_uses_length_prefixed_connection_ids); std::unique_ptr<QuicEncryptedPacket> encrypted_packet = QuicFramer::BuildVersionNegotiationPacket( source_connection_id, EmptyQuicConnectionId(), - /*ietf_quic=*/true, use_length_prefix, ParsedQuicVersionVector{}); + /*ietf_quic=*/true, /*use_length_prefix=*/true, + ParsedQuicVersionVector{}); if (!encrypted_packet) { QUIC_BUG << "Failed to create version negotiation packet"; return false;