gfe-relnote: fix path probe request in IETF QUIC, padded ping with peer address change should not be treated as path probe request. Only a packet contains PATH_CHALLENGE frame is considered as a path probe request. Protected by disabled QUIC version 99. This CL also changes UpdatePacketContent to no-op in IETF version. PiperOrigin-RevId: 296970477 Change-Id: Ida8c7e6a686c08508b936081c812cd14869c2137
diff --git a/quic/core/quic_connection.cc b/quic/core/quic_connection.cc index 0420637..4cfc95d 100644 --- a/quic/core/quic_connection.cc +++ b/quic/core/quic_connection.cc
@@ -1191,13 +1191,6 @@ // response. received_path_challenge_payloads_.push_back(frame.data_buffer); - // For VERSION 99 we define a "Padded PATH CHALLENGE" to be the same thing - // as a PADDED PING -- it will start a connectivity check and prevent - // connection migration. Insofar as the connectivity check and connection - // migration are concerned, logically the PATH CHALLENGE is the same as the - // PING, so as a stopgap, tell the FSM that determines whether we have a - // Padded PING or not that we received a PING. - UpdatePacketContent(FIRST_FRAME_IS_PING); should_last_packet_instigate_acks_ = true; return true; } @@ -1214,7 +1207,6 @@ } // Have received the matching PATH RESPONSE, saved payload no longer valid. transmitted_connectivity_probe_payload_ = nullptr; - UpdatePacketContent(FIRST_FRAME_IS_PING); return true; } @@ -1404,6 +1396,7 @@ } if (IsCurrentPacketConnectivityProbing()) { + DCHECK(!version().HasIetfQuicFrames()); ++stats_.num_connectivity_probing_received; } @@ -1420,6 +1413,7 @@ << IsCurrentPacketConnectivityProbing(); if (IsCurrentPacketConnectivityProbing()) { + DCHECK(!version().HasIetfQuicFrames()); visitor_->OnPacketReceived(last_packet_destination_address_, last_packet_source_address_, /*is_connectivity_probe=*/true); @@ -1436,29 +1430,24 @@ visitor_->OnPacketReceived(last_packet_destination_address_, last_packet_source_address_, /*is_connectivity_probe=*/false); - } else { - // This node is not a client (is a server) AND the received packet was - // NOT connectivity-probing. If the packet had PATH CHALLENGES, send - // appropriate RESPONSE. Then deal with possible peer migration. - if (VersionHasIetfQuicFrames(transport_version()) && - !received_path_challenge_payloads_.empty()) { - // If a PATH CHALLENGE was in a "Padded PING (or PATH CHALLENGE)" - // then it is taken care of above. This handles the case where a PATH - // CHALLENGE appeared someplace else (eg, the peer randomly added a PATH - // CHALLENGE frame to some other packet. - // There was at least one PATH CHALLENGE in the received packet, - // Generate the required PATH RESPONSE. - SendGenericPathProbePacket(nullptr, last_packet_source_address_, - /* is_response= */ true); + } else if (version().HasIetfQuicFrames() && + !received_path_challenge_payloads_.empty()) { + if (current_effective_peer_migration_type_ != NO_CHANGE) { + // TODO(b/150095588): change the stats to + // num_valid_path_challenge_received. + ++stats_.num_connectivity_probing_received; } - - if (last_header_.packet_number == GetLargestReceivedPacket()) { - direct_peer_address_ = last_packet_source_address_; - if (current_effective_peer_migration_type_ != NO_CHANGE) { - // TODO(fayang): When multiple packet number spaces is supported, only - // start peer migration for the application data. - StartEffectivePeerMigration(current_effective_peer_migration_type_); - } + // If the packet contains PATH CHALLENGE, send appropriate RESPONSE. + // There was at least one PATH CHALLENGE in the received packet, + // Generate the required PATH RESPONSE. + SendGenericPathProbePacket(nullptr, last_packet_source_address_, + /* is_response=*/true); + } else if (last_header_.packet_number == GetLargestReceivedPacket()) { + direct_peer_address_ = last_packet_source_address_; + if (current_effective_peer_migration_type_ != NO_CHANGE) { + // TODO(fayang): When multiple packet number spaces is supported, only + // start peer migration for the application data. + StartEffectivePeerMigration(current_effective_peer_migration_type_); } } @@ -3466,39 +3455,28 @@ << server_connection_id_; OwningSerializedPacketPointer probing_packet; - if (!VersionHasIetfQuicFrames(transport_version())) { + if (!version().HasIetfQuicFrames()) { // Non-IETF QUIC, generate a padded ping regardless of whether this is a // request or a response. probing_packet = packet_creator_.SerializeConnectivityProbingPacket(); + } else if (is_response) { + // IETF QUIC path response. + // Respond to path probe request using IETF QUIC PATH_RESPONSE frame. + probing_packet = + packet_creator_.SerializePathResponseConnectivityProbingPacket( + received_path_challenge_payloads_, + /*is_padded=*/false); + received_path_challenge_payloads_.clear(); } else { - if (is_response) { - // Respond using IETF QUIC PATH_RESPONSE frame - if (IsCurrentPacketConnectivityProbing()) { - // Pad the response if the request was a google connectivity probe - // (padded). - probing_packet = - packet_creator_.SerializePathResponseConnectivityProbingPacket( - received_path_challenge_payloads_, /* is_padded = */ true); - received_path_challenge_payloads_.clear(); - } else { - // Do not pad the response if the path challenge was not a google - // connectivity probe. - probing_packet = - packet_creator_.SerializePathResponseConnectivityProbingPacket( - received_path_challenge_payloads_, - /* is_padded = */ false); - received_path_challenge_payloads_.clear(); - } - } else { - // Request using IETF QUIC PATH_CHALLENGE frame - transmitted_connectivity_probe_payload_ = - std::make_unique<QuicPathFrameBuffer>(); - probing_packet = - packet_creator_.SerializePathChallengeConnectivityProbingPacket( - transmitted_connectivity_probe_payload_.get()); - if (!probing_packet) { - transmitted_connectivity_probe_payload_ = nullptr; - } + // IETF QUIC path challenge. + // Send a path probe request using IETF QUIC PATH_CHALLENGE frame. + transmitted_connectivity_probe_payload_ = + std::make_unique<QuicPathFrameBuffer>(); + probing_packet = + packet_creator_.SerializePathChallengeConnectivityProbingPacket( + transmitted_connectivity_probe_payload_.get()); + if (!probing_packet) { + transmitted_connectivity_probe_payload_ = nullptr; } } @@ -3685,6 +3663,14 @@ } void QuicConnection::UpdatePacketContent(PacketContent type) { + // Packet content is tracked to identify connectivity probe in non-IETF + // version, where a connectivity probe is defined as + // - a padded PING packet with peer address change received by server, + // - a padded PING packet on new path received by client. + if (version().HasIetfQuicFrames()) { + return; + } + if (current_packet_content_ == NOT_PADDED_PING) { // We have already learned the current packet is not a connectivity // probing packet. Peer migration should have already been started earlier @@ -3703,11 +3689,9 @@ } } - // In Google QUIC we look for a packet with just a PING and PADDING. - // For IETF QUIC, the packet must consist of just a PATH_CHALLENGE frame, - // followed by PADDING. If the condition is met, mark things as - // connectivity-probing, causing later processing to generate the correct - // response. + // In Google QUIC, we look for a packet with just a PING and PADDING. + // If the condition is met, mark things as connectivity-probing, causing + // later processing to generate the correct response. if (type == SECOND_FRAME_IS_PADDING && current_packet_content_ == FIRST_FRAME_IS_PING) { current_packet_content_ = SECOND_FRAME_IS_PADDING;
diff --git a/quic/core/quic_connection_test.cc b/quic/core/quic_connection_test.cc index c17424e..9753ba1 100644 --- a/quic/core/quic_connection_test.cc +++ b/quic/core/quic_connection_test.cc
@@ -1149,6 +1149,12 @@ QuicSocketAddress peer_address) { QuicFrames frames; frames.push_back(QuicFrame(frame)); + return ProcessFramesPacketWithAddresses(frames, self_address, peer_address); + } + + void ProcessFramesPacketWithAddresses(QuicFrames frames, + QuicSocketAddress self_address, + QuicSocketAddress peer_address) { QuicPacketCreatorPeer::SetSendVersionInPacket( &peer_creator_, QuicPacketCreatorPeer::GetEncryptionLevel(&peer_creator_) < @@ -1984,7 +1990,7 @@ connection_.active_effective_peer_migration_type()); } -TEST_P(QuicConnectionTest, ReceivePaddedPingAtServer) { +TEST_P(QuicConnectionTest, ReceivePathProbeWithNoAddressChangeAtServer) { set_perspective(Perspective::IS_SERVER); QuicPacketCreatorPeer::SetSendVersionInPacket(creator_, false); EXPECT_EQ(Perspective::IS_SERVER, connection_.perspective()); @@ -2016,17 +2022,8 @@ // Process a padded PING or PATH CHALLENGE packet with no peer address change // on server side will be ignored. - OwningSerializedPacketPointer probing_packet; - if (VersionHasIetfQuicFrames(version().transport_version)) { - QuicPathFrameBuffer payload = { - {0xde, 0xad, 0xbe, 0xef, 0xba, 0xdc, 0x0f, 0xfe}}; - probing_packet = - QuicPacketCreatorPeer::SerializePathChallengeConnectivityProbingPacket( - &peer_creator_, &payload); - } else { - probing_packet = QuicPacketCreatorPeer::SerializeConnectivityProbingPacket( - &peer_creator_); - } + OwningSerializedPacketPointer probing_packet = ConstructProbingPacket(); + std::unique_ptr<QuicReceivedPacket> received(ConstructReceivedPacket( QuicEncryptedPacket(probing_packet->encrypted_buffer, probing_packet->encrypted_length), @@ -2091,7 +2088,11 @@ EXPECT_EQ(0u, connection_.GetStats().packets_discarded); } -TEST_P(QuicConnectionTest, ReceiveConnectivityProbingAtServer) { +// Receive a path probe request at the server side, i.e., +// in non-IETF version: receive a padded PING packet with a peer addess change; +// in IETF version: receive a packet contains PATH CHALLENGE with peer address +// change. +TEST_P(QuicConnectionTest, ReceivePathProbingAtServer) { set_perspective(Perspective::IS_SERVER); QuicPacketCreatorPeer::SetSendVersionInPacket(creator_, false); EXPECT_EQ(Perspective::IS_SERVER, connection_.perspective()); @@ -2119,8 +2120,13 @@ EXPECT_EQ(kPeerAddress, connection_.effective_peer_address()); EXPECT_CALL(visitor_, OnConnectionMigration(PORT_CHANGE)).Times(0); - EXPECT_CALL(visitor_, OnPacketReceived(_, _, true)).Times(1); - + if (!GetParam().version.HasIetfQuicFrames()) { + EXPECT_CALL(visitor_, + OnPacketReceived(_, _, /*is_connectivity_probe=*/true)) + .Times(1); + } else { + EXPECT_CALL(visitor_, OnPacketReceived(_, _, _)).Times(0); + } // Process a padded PING packet from a new peer address on server side // is effectively receiving a connectivity probing. const QuicSocketAddress kNewPeerAddress = @@ -2149,7 +2155,90 @@ EXPECT_EQ(kPeerAddress, connection_.effective_peer_address()); } -TEST_P(QuicConnectionTest, ReceiveReorderedConnectivityProbingAtServer) { +// Receive a padded PING packet with a port change on server side. +TEST_P(QuicConnectionTest, ReceivePaddedPingWithPortChangeAtServer) { + set_perspective(Perspective::IS_SERVER); + QuicPacketCreatorPeer::SetSendVersionInPacket(creator_, false); + EXPECT_EQ(Perspective::IS_SERVER, connection_.perspective()); + + // Clear direct_peer_address. + QuicConnectionPeer::SetDirectPeerAddress(&connection_, QuicSocketAddress()); + // Clear effective_peer_address, it is the same as direct_peer_address for + // this test. + QuicConnectionPeer::SetEffectivePeerAddress(&connection_, + QuicSocketAddress()); + EXPECT_FALSE(connection_.effective_peer_address().IsInitialized()); + + QuicFrame frame; + if (GetParam().version.UsesCryptoFrames()) { + frame = QuicFrame(&crypto_frame_); + EXPECT_CALL(visitor_, OnCryptoFrame(_)).Times(AnyNumber()); + } else { + frame = QuicFrame(QuicStreamFrame( + QuicUtils::GetCryptoStreamId(connection_.transport_version()), false, + 0u, quiche::QuicheStringPiece())); + EXPECT_CALL(visitor_, OnStreamFrame(_)).Times(AnyNumber()); + } + ProcessFramePacketWithAddresses(frame, kSelfAddress, kPeerAddress); + EXPECT_EQ(kPeerAddress, connection_.peer_address()); + EXPECT_EQ(kPeerAddress, connection_.effective_peer_address()); + + if (GetParam().version.HasIetfQuicFrames()) { + // In IETF version, a padded PING packet with port change is not taken as + // connectivity probe. + EXPECT_CALL(visitor_, OnConnectionMigration(PORT_CHANGE)).Times(1); + EXPECT_CALL(visitor_, OnPacketReceived(_, _, _)).Times(0); + } else { + // In non-IETF version, process a padded PING packet from a new peer + // address on server side is effectively receiving a connectivity probing. + EXPECT_CALL(visitor_, OnConnectionMigration(PORT_CHANGE)).Times(0); + EXPECT_CALL(visitor_, + OnPacketReceived(_, _, /*is_connectivity_probe=*/true)) + .Times(1); + } + const QuicSocketAddress kNewPeerAddress = + QuicSocketAddress(QuicIpAddress::Loopback6(), /*port=*/23456); + + QuicFrames frames; + // Write a PING frame, which has no data payload. + QuicPingFrame ping_frame; + frames.push_back(QuicFrame(ping_frame)); + + // Add padding to the rest of the packet. + QuicPaddingFrame padding_frame; + frames.push_back(QuicFrame(padding_frame)); + + uint64_t num_probing_received = + connection_.GetStats().num_connectivity_probing_received; + + ProcessFramesPacketWithAddresses(frames, kSelfAddress, kNewPeerAddress); + + if (GetParam().version.HasIetfQuicFrames()) { + // Padded PING with port changen is not considered as connectivity probe but + // a PORT CHANGE. + EXPECT_EQ(num_probing_received, + connection_.GetStats().num_connectivity_probing_received); + EXPECT_EQ(kNewPeerAddress, connection_.peer_address()); + EXPECT_EQ(kNewPeerAddress, connection_.effective_peer_address()); + } else { + EXPECT_EQ(num_probing_received + 1, + connection_.GetStats().num_connectivity_probing_received); + EXPECT_EQ(kPeerAddress, connection_.peer_address()); + EXPECT_EQ(kPeerAddress, connection_.effective_peer_address()); + } + + // Process another packet with the old peer address on server side. + if (GetParam().version.HasIetfQuicFrames()) { + EXPECT_CALL(visitor_, OnConnectionMigration(PORT_CHANGE)).Times(1); + } else { + EXPECT_CALL(visitor_, OnConnectionMigration(PORT_CHANGE)).Times(0); + } + ProcessFramePacketWithAddresses(frame, kSelfAddress, kPeerAddress); + EXPECT_EQ(kPeerAddress, connection_.peer_address()); + EXPECT_EQ(kPeerAddress, connection_.effective_peer_address()); +} + +TEST_P(QuicConnectionTest, ReceiveReorderedPathProbingAtServer) { set_perspective(Perspective::IS_SERVER); QuicPacketCreatorPeer::SetSendVersionInPacket(creator_, false); EXPECT_EQ(Perspective::IS_SERVER, connection_.perspective()); @@ -2181,7 +2270,13 @@ QuicPacketCreatorPeer::SetPacketNumber(&peer_creator_, 4); EXPECT_CALL(visitor_, OnConnectionMigration(PORT_CHANGE)).Times(0); - EXPECT_CALL(visitor_, OnPacketReceived(_, _, true)).Times(1); + if (!GetParam().version.HasIetfQuicFrames()) { + EXPECT_CALL(visitor_, + OnPacketReceived(_, _, /*is_connectivity_probe=*/true)) + .Times(1); + } else { + EXPECT_CALL(visitor_, OnPacketReceived(_, _, _)).Times(0); + } // Process a padded PING packet from a new peer address on server side // is effectively receiving a connectivity probing, even if a newer packet has @@ -2233,7 +2328,13 @@ EXPECT_EQ(kPeerAddress, connection_.effective_peer_address()); EXPECT_CALL(visitor_, OnConnectionMigration(PORT_CHANGE)).Times(0); - EXPECT_CALL(visitor_, OnPacketReceived(_, _, true)).Times(1); + if (!GetParam().version.HasIetfQuicFrames()) { + EXPECT_CALL(visitor_, + OnPacketReceived(_, _, /*is_connectivity_probe=*/true)) + .Times(1); + } else { + EXPECT_CALL(visitor_, OnPacketReceived(_, _, _)).Times(0); + } // Process a padded PING packet from a new peer address on server side // is effectively receiving a connectivity probing. @@ -2305,7 +2406,13 @@ EXPECT_EQ(kPeerAddress, connection_.effective_peer_address()); } -TEST_P(QuicConnectionTest, ReceiveConnectivityProbingAtClient) { +TEST_P(QuicConnectionTest, ReceiveConnectivityProbingResponseAtClient) { + // TODO(b/150095484): add test coverage for IETF to verify that client takes + // PATH RESPONSE with peer address change as correct validation on the new + // path. + if (GetParam().version.HasIetfQuicFrames()) { + return; + } EXPECT_CALL(visitor_, OnSuccessfulVersionNegotiation(_)); set_perspective(Perspective::IS_CLIENT); EXPECT_EQ(Perspective::IS_CLIENT, connection_.perspective()); @@ -2335,7 +2442,13 @@ // Process a padded PING packet with a different self address on client side // is effectively receiving a connectivity probing. EXPECT_CALL(visitor_, OnConnectionMigration(PORT_CHANGE)).Times(0); - EXPECT_CALL(visitor_, OnPacketReceived(_, _, true)).Times(1); + if (!GetParam().version.HasIetfQuicFrames()) { + EXPECT_CALL(visitor_, + OnPacketReceived(_, _, /*is_connectivity_probe=*/true)) + .Times(1); + } else { + EXPECT_CALL(visitor_, OnPacketReceived(_, _, _)).Times(0); + } const QuicSocketAddress kNewSelfAddress = QuicSocketAddress(QuicIpAddress::Loopback6(), /*port=*/23456);