From b45c82b8847ff9c76c397c742b9e757e745acda0 Mon Sep 17 00:00:00 2001 From: Joseph Beshay Date: Mon, 24 Feb 2025 12:32:50 -0800 Subject: [PATCH] ACK_EXTENDED frame support Summary: This introduces a new frame type for acks (ACK_EXTENDED) that can carry optional fields depending on the features supported by the peer. The currently supported features set will include ECN count fields, and Receive Timstamp fields. This enables a quic connection to report both ECN counts and receive timestamps, which is not possible otherwise because they use different frame types. Support for the extended ack as well as the set of features that can be included in it is negotiated through a new transport parameter (extended_ack_supported = 0xff0a004). Its value indicates which features are supported by the local transport. The value is an integer which is evaluated against the following bitmasks: ``` ECN_COUNTS = 0x01, RECEIVE_TIMESTAMPS = 0x02, ``` This diff introduces the transport parameter and negotiates the supported features between the peers of the connection. The parameter is cached in the psk cache so the client can remember the server config. It is also encoded inside the 0-rtt ticket so the server can reject it if its local config has changed. The following diffs add reading and writing the frame itself. The ACK_EXTENDED frame itself will have the following format ``` ACK_EXTENDED Frame { Type (i) = 0xB1 // Fields of the existing ACK (type=0x02) frame: Largest Acknowledged (i), ACK Delay (i), ACK Range Count (i), First ACK Range (i), ACK Range (..) ..., Extended Ack Features (i), // Optional ECN counts (if bit 0 is set in Features) [ECN Counts (..)], // Optional Receive Timestamps (if bit 1 is set in Features) [Receive Timestamps (..)] } // Fields from the existing ACK_ECN frame ECN Counts { ECT0 Count (i), ECT1 Count (i), ECN-CE Count (i), } // Fields from the existing ACK_RECEIVE_TIMESTAMPS frame Receive Timestamps { Timestamp Range Count (i), Timestamp Ranges (..) ..., } Timestamp Range { Gap (i), Timestamp Delta Count (i), Timestamp Delta (i) ..., } ``` Reviewed By: sharmafb Differential Revision: D68931151 fbshipit-source-id: 44c8c83d2f434abca97c4e85f0fa7502736cddc1 --- quic/QuicConstants.h | 12 +- quic/client/QuicClientTransportLite.cpp | 18 ++- .../CachedServerTransportParameters.h | 2 + quic/client/handshake/ClientHandshake.cpp | 3 +- quic/client/state/ClientStateMachine.cpp | 11 +- quic/client/state/ClientStateMachine.h | 3 +- quic/client/test/ClientStateMachineTest.cpp | 26 ++++ quic/codec/Decode.cpp | 8 +- quic/codec/Types.cpp | 2 + quic/common/test/TestUtils.cpp | 3 +- quic/handshake/TransportParameters.cpp | 5 + quic/handshake/TransportParameters.h | 1 + quic/logging/QLoggerConstants.cpp | 2 + quic/logging/QLoggerTypes.cpp | 6 +- quic/server/QuicServerTransport.cpp | 1 + quic/server/handshake/AppToken.cpp | 3 + quic/server/handshake/AppToken.h | 1 + .../handshake/DefaultAppTokenValidator.cpp | 10 ++ quic/server/handshake/test/AppTokenTest.cpp | 32 +++-- .../test/DefaultAppTokenValidatorTest.cpp | 130 ++++++++++++------ quic/server/state/ServerStateMachine.cpp | 21 +++ quic/server/test/ServerStateMachineTest.cpp | 22 +++ quic/state/StateData.h | 2 + quic/state/TransportSettings.h | 10 ++ 24 files changed, 277 insertions(+), 57 deletions(-) diff --git a/quic/QuicConstants.h b/quic/QuicConstants.h index 8f6a1cad1..152567346 100644 --- a/quic/QuicConstants.h +++ b/quic/QuicConstants.h @@ -262,13 +262,23 @@ enum class FrameType : uint64_t { GROUP_STREAM_OFF_FIN = 0x37, GROUP_STREAM_OFF_LEN = 0x38, GROUP_STREAM_OFF_LEN_FIN = 0x39, - ACK_RECEIVE_TIMESTAMPS = 0xB0 + ACK_RECEIVE_TIMESTAMPS = 0xB0, + ACK_EXTENDED = 0xB1 }; inline constexpr uint16_t toFrameError(FrameType frame) { return 0x0100 | static_cast(frame); } +enum class ExtendedAckFeatureMask : uint8_t { + // These should use mutually exclusive bits. + ECN_COUNTS = 0x01, + RECEIVE_TIMESTAMPS = 0x02, +}; + +using ExtendedAckFeatureMaskType = + std::underlying_type::type; + enum class TransportErrorCode : uint64_t { NO_ERROR = 0x0000, INTERNAL_ERROR = 0x0001, diff --git a/quic/client/QuicClientTransportLite.cpp b/quic/client/QuicClientTransportLite.cpp index fb8bfdcfe..d411c0b2e 100644 --- a/quic/client/QuicClientTransportLite.cpp +++ b/quic/client/QuicClientTransportLite.cpp @@ -462,6 +462,21 @@ void QuicClientTransportLite::processUdpPacketData( VLOG(10) << "Client received ack frame in packet=" << packetNum << " " << *this; ReadAckFrame& ackFrame = *quicFrame.asReadAckFrame(); + + if (ackFrame.frameType == FrameType::ACK_EXTENDED && + !conn_->transportSettings.advertisedExtendedAckFeatures) { + throw QuicTransportException( + "Received unexpected ACK_EXTENDED frame", + TransportErrorCode::PROTOCOL_VIOLATION); + } else if ( + ackFrame.frameType == FrameType::ACK_RECEIVE_TIMESTAMPS && + !conn_->transportSettings + .maybeAckReceiveTimestampsConfigSentToPeer) { + throw QuicTransportException( + "Received unexpected ACK_RECEIVE_TIMESTAMPS frame", + TransportErrorCode::PROTOCOL_VIOLATION); + } + conn_->lastProcessedAckEvents.emplace_back(processAckFrame( *conn_, pnSpace, @@ -754,7 +769,8 @@ void QuicClientTransportLite::processUdpPacketData( ? conn_->maybePeerAckReceiveTimestampsConfig ->receiveTimestampsExponent : 3, - conn_->peerAdvertisedReliableStreamResetSupport); + conn_->peerAdvertisedReliableStreamResetSupport, + conn_->peerAdvertisedExtendedAckFeatures); if (clientConn_->zeroRttRejected.has_value() && *clientConn_->zeroRttRejected) { diff --git a/quic/client/handshake/CachedServerTransportParameters.h b/quic/client/handshake/CachedServerTransportParameters.h index 4ac45effd..b26b4cece 100644 --- a/quic/client/handshake/CachedServerTransportParameters.h +++ b/quic/client/handshake/CachedServerTransportParameters.h @@ -24,6 +24,8 @@ struct CachedServerTransportParameters { uint64_t initialMaxStreamsUni{0}; uint64_t maxReceiveTimestampsPerAck{0}; uint64_t receiveTimestampsExponent{0}; + // Underlying type is currently uint8_t so this struct is still packed + ExtendedAckFeatureMaskType extendedAckFeatures{0}; bool knobFrameSupport{false}; bool ackReceiveTimestampsEnabled{false}; bool reliableStreamResetSupport{false}; diff --git a/quic/client/handshake/ClientHandshake.cpp b/quic/client/handshake/ClientHandshake.cpp index 1541db3db..eb8019e37 100644 --- a/quic/client/handshake/ClientHandshake.cpp +++ b/quic/client/handshake/ClientHandshake.cpp @@ -46,7 +46,8 @@ void ClientHandshake::connect( cachedServerTransportParams->ackReceiveTimestampsEnabled, cachedServerTransportParams->maxReceiveTimestampsPerAck, cachedServerTransportParams->receiveTimestampsExponent, - cachedServerTransportParams->reliableStreamResetSupport); + cachedServerTransportParams->reliableStreamResetSupport, + cachedServerTransportParams->extendedAckFeatures); updateTransportParamsFromCachedEarlyParams( *conn_, *cachedServerTransportParams); } diff --git a/quic/client/state/ClientStateMachine.cpp b/quic/client/state/ClientStateMachine.cpp index 177b15d65..7d9ff9d04 100644 --- a/quic/client/state/ClientStateMachine.cpp +++ b/quic/client/state/ClientStateMachine.cpp @@ -151,6 +151,10 @@ void processServerInitialParams( static_cast( TransportParameterId::knob_frames_supported), serverParams.parameters); + auto extendedAckFeatures = getIntegerParameter( + static_cast( + TransportParameterId::extended_ack_features), + serverParams.parameters); auto reliableResetTpIter = findParameter( serverParams.parameters, @@ -286,6 +290,7 @@ void processServerInitialParams( } conn.peerAdvertisedKnobFrameSupport = knobFrameSupported.value_or(0) > 0; + conn.peerAdvertisedExtendedAckFeatures = extendedAckFeatures.value_or(0); } void cacheServerInitialParams( @@ -300,7 +305,8 @@ void cacheServerInitialParams( bool peerAdvertisedAckReceiveTimestampsEnabled, uint64_t peerAdvertisedMaxReceiveTimestampsPerAck, uint64_t peerAdvertisedReceiveTimestampsExponent, - bool peerAdvertisedReliableStreamResetSupport) { + bool peerAdvertisedReliableStreamResetSupport, + ExtendedAckFeatureMaskType peerAdvertisedExtendedAckFeatures) { conn.serverInitialParamsSet_ = true; conn.peerAdvertisedInitialMaxData = peerAdvertisedInitialMaxData; conn.peerAdvertisedInitialMaxStreamDataBidiLocal = @@ -328,6 +334,7 @@ void cacheServerInitialParams( } else { conn.maybePeerAckReceiveTimestampsConfig.clear(); } + conn.peerAdvertisedExtendedAckFeatures = peerAdvertisedExtendedAckFeatures; } CachedServerTransportParameters getServerCachedTransportParameters( @@ -360,6 +367,7 @@ CachedServerTransportParameters getServerCachedTransportParameters( transportParams.receiveTimestampsExponent = conn.maybePeerAckReceiveTimestampsConfig->receiveTimestampsExponent; } + transportParams.extendedAckFeatures = conn.peerAdvertisedExtendedAckFeatures; return transportParams; } @@ -395,5 +403,6 @@ void updateTransportParamsFromCachedEarlyParams( } else { conn.maybePeerAckReceiveTimestampsConfig.clear(); } + conn.peerAdvertisedExtendedAckFeatures = transportParams.extendedAckFeatures; } } // namespace quic diff --git a/quic/client/state/ClientStateMachine.h b/quic/client/state/ClientStateMachine.h index b850980d3..41acacf96 100644 --- a/quic/client/state/ClientStateMachine.h +++ b/quic/client/state/ClientStateMachine.h @@ -158,7 +158,8 @@ void cacheServerInitialParams( bool peerAdvertisedAckReceiveTimestampsEnabled, uint64_t peerAdvertisedMaxRecvTimestampsPerAck, uint64_t peerAdvertisedReceiveTimestampsExponent, - bool peerAdvertisedReliableStreamResetSupport); + bool peerAdvertisedReliableStreamResetSupport, + ExtendedAckFeatureMaskType peerAdvertisedExtendedAckSupport); CachedServerTransportParameters getServerCachedTransportParameters( const QuicClientConnectionState& conn); diff --git a/quic/client/test/ClientStateMachineTest.cpp b/quic/client/test/ClientStateMachineTest.cpp index 4d81b8e18..a595b0c58 100644 --- a/quic/client/test/ClientStateMachineTest.cpp +++ b/quic/client/test/ClientStateMachineTest.cpp @@ -32,6 +32,7 @@ constexpr auto initialMaxStreamDataUni = kDefaultStreamFlowControlWindow + 5; constexpr auto initialMaxStreamsBidi = kDefaultMaxStreamsBidirectional + 6; constexpr auto initialMaxStreamsUni = kDefaultMaxStreamsUnidirectional + 7; constexpr auto knobFrameSupport = true; +constexpr auto extendedAckSupport = 3; constexpr auto ackReceiveTimestampsEnabled = true; constexpr auto maxReceiveTimestampsPerAck = 10; constexpr auto ackReceiveTimestampsExponent = 0; @@ -46,6 +47,7 @@ const CachedServerTransportParameters kParams{ initialMaxStreamsUni, maxReceiveTimestampsPerAck, ackReceiveTimestampsExponent, + extendedAckSupport, knobFrameSupport, ackReceiveTimestampsEnabled}; } // namespace @@ -78,6 +80,7 @@ TEST_F(ClientStateMachineTest, TestUpdateTransportParamsNotIgnorePathMTU) { TEST_F(ClientStateMachineTest, TestUpdateTransportParamsFromCachedEarlyParams) { client_->transportSettings.canIgnorePathMTU = true; client_->peerAdvertisedKnobFrameSupport = false; + client_->peerAdvertisedExtendedAckFeatures = 0; client_->maybePeerAckReceiveTimestampsConfig.assign( {.maxReceiveTimestampsPerAck = 10, .receiveTimestampsExponent = 0}); @@ -95,6 +98,7 @@ TEST_F(ClientStateMachineTest, TestUpdateTransportParamsFromCachedEarlyParams) { client_->flowControlState.peerAdvertisedInitialMaxStreamOffsetUni, initialMaxStreamDataUni); EXPECT_EQ(client_->peerAdvertisedKnobFrameSupport, knobFrameSupport); + EXPECT_EQ(client_->peerAdvertisedExtendedAckFeatures, extendedAckSupport); ASSERT_TRUE(client_->maybePeerAckReceiveTimestampsConfig.has_value()); EXPECT_EQ( client_->maybePeerAckReceiveTimestampsConfig.value() @@ -234,6 +238,28 @@ TEST_F(ClientStateMachineTest, TestProcessKnobFramesSupportedParamDisabled) { EXPECT_FALSE(clientConn.peerAdvertisedKnobFrameSupport); } +TEST_F(ClientStateMachineTest, TestProcessExtendedAckSupportedParam) { + QuicClientConnectionState clientConn( + FizzClientQuicHandshakeContext::Builder().build()); + std::vector transportParams; + transportParams.push_back( + encodeIntegerParameter(TransportParameterId::extended_ack_features, 3)); + ServerTransportParameters serverTransportParams = { + std::move(transportParams)}; + processServerInitialParams(clientConn, serverTransportParams, 0); + EXPECT_EQ(clientConn.peerAdvertisedExtendedAckFeatures, 3); +} + +TEST_F(ClientStateMachineTest, TestProcessExtendedAckSupportedParamDefault) { + QuicClientConnectionState clientConn( + FizzClientQuicHandshakeContext::Builder().build()); + std::vector transportParams; + ServerTransportParameters serverTransportParams = { + std::move(transportParams)}; + processServerInitialParams(clientConn, serverTransportParams, 0); + EXPECT_EQ(clientConn.peerAdvertisedExtendedAckFeatures, 0); +} + TEST_F( ClientStateMachineTest, TestProcessReliableStreamResetSupportedParamEnabled) { diff --git a/quic/codec/Decode.cpp b/quic/codec/Decode.cpp index dd07328ba..943579cc2 100644 --- a/quic/codec/Decode.cpp +++ b/quic/codec/Decode.cpp @@ -865,10 +865,16 @@ QuicFrame parseFrame( return QuicFrame(decodeAckFrequencyFrame(cursor)); case FrameType::IMMEDIATE_ACK: return QuicFrame(decodeImmediateAckFrame(cursor)); - case FrameType::ACK_RECEIVE_TIMESTAMPS: + case FrameType::ACK_RECEIVE_TIMESTAMPS: { auto frame = QuicFrame(decodeAckFrameWithReceivedTimestamps( cursor, header, params, FrameType::ACK_RECEIVE_TIMESTAMPS)); return frame; + } + case FrameType::ACK_EXTENDED: + throw QuicTransportException( + folly::to( + "ACK_EXTENDED not yet supported, type=", frameTypeInt->first), + TransportErrorCode::FRAME_ENCODING_ERROR); } } catch (const std::exception& e) { error = true; diff --git a/quic/codec/Types.cpp b/quic/codec/Types.cpp index 6009e6118..2feb2d387 100644 --- a/quic/codec/Types.cpp +++ b/quic/codec/Types.cpp @@ -457,6 +457,8 @@ std::string_view toString(FrameType frame) { return "GROUP_STREAM"; case FrameType::ACK_RECEIVE_TIMESTAMPS: return "ACK_RECEIVE_TIMESTAMPS"; + case quic::FrameType::ACK_EXTENDED: + return "ACK_EXTENDED"; } LOG(WARNING) << "toString has unhandled frame type"; return "UNKNOWN"; diff --git a/quic/common/test/TestUtils.cpp b/quic/common/test/TestUtils.cpp index 24f7149dd..78ebc6622 100644 --- a/quic/common/test/TestUtils.cpp +++ b/quic/common/test/TestUtils.cpp @@ -183,7 +183,8 @@ class AcceptingTicketCipher : public fizz::server::TicketCipher { kDefaultStreamFlowControlWindow, kDefaultStreamFlowControlWindow, kDefaultMaxStreamsBidirectional, - kDefaultMaxStreamsUnidirectional); + kDefaultMaxStreamsUnidirectional, + 0 /*extendedAckSupport*/); appToken.version = QuicVersion::MVFST; resState.appToken = encodeAppToken(appToken); return resState; diff --git a/quic/handshake/TransportParameters.cpp b/quic/handshake/TransportParameters.cpp index 0099a186b..6985b79d3 100644 --- a/quic/handshake/TransportParameters.cpp +++ b/quic/handshake/TransportParameters.cpp @@ -129,6 +129,11 @@ std::vector getSupportedExtTransportParams( customTps.push_back(encodeEmptyParameter(TpId::reliable_stream_reset)); } + if (ts.advertisedExtendedAckFeatures) { + customTps.push_back(encodeIntegerParameter( + TpId::extended_ack_features, ts.advertisedExtendedAckFeatures)); + } + return customTps; } diff --git a/quic/handshake/TransportParameters.h b/quic/handshake/TransportParameters.h index 677c9e7ff..346f776a6 100644 --- a/quic/handshake/TransportParameters.h +++ b/quic/handshake/TransportParameters.h @@ -39,6 +39,7 @@ enum class TransportParameterId : uint64_t { ack_receive_timestamps_enabled = 0xff0a001, max_receive_timestamps_per_ack = 0xff0a002, receive_timestamps_exponent = 0xff0a003, + extended_ack_features = 0xff0a004, stream_groups_enabled = 0x0000ff99, knob_frames_supported = 0x00005178, cwnd_hint_bytes = 0x00007492, diff --git a/quic/logging/QLoggerConstants.cpp b/quic/logging/QLoggerConstants.cpp index b1d403760..91b0c1e22 100644 --- a/quic/logging/QLoggerConstants.cpp +++ b/quic/logging/QLoggerConstants.cpp @@ -94,6 +94,8 @@ folly::StringPiece toQlogString(FrameType frame) { return "group_stream"; case FrameType::ACK_RECEIVE_TIMESTAMPS: return "ack_receive_timestamps"; + case FrameType::ACK_EXTENDED: + return "ack_extended"; } folly::assume_unreachable(); } diff --git a/quic/logging/QLoggerTypes.cpp b/quic/logging/QLoggerTypes.cpp index 6c56b08d6..ded621549 100644 --- a/quic/logging/QLoggerTypes.cpp +++ b/quic/logging/QLoggerTypes.cpp @@ -224,11 +224,13 @@ folly::dynamic ReadAckFrameLog::toDynamic() const { } d["acked_ranges"] = ackRangeDynamic; d["frame_type"] = toQlogString(frameType); - if (frameType == FrameType::ACK_ECN) { + if (frameType == FrameType::ACK_EXTENDED || frameType == FrameType::ACK_ECN) { d["ecn_ect0"] = ecnECT0Count; d["ecn_ect1"] = ecnECT1Count; d["ecn_ce"] = ecnCECount; - } else if (frameType == FrameType::ACK_RECEIVE_TIMESTAMPS) { + } + if (frameType == FrameType::ACK_EXTENDED || + frameType == FrameType::ACK_RECEIVE_TIMESTAMPS) { if (maybeLatestRecvdPacketTime.has_value()) { d["latest_recvd_packet_time"] = maybeLatestRecvdPacketTime.value().count(); diff --git a/quic/server/QuicServerTransport.cpp b/quic/server/QuicServerTransport.cpp index 1349ea05a..e3a484e65 100644 --- a/quic/server/QuicServerTransport.cpp +++ b/quic/server/QuicServerTransport.cpp @@ -564,6 +564,7 @@ void QuicServerTransport::maybeWriteNewSessionTicket() { conn_->transportSettings.advertisedInitialUniStreamFlowControlWindow, conn_->transportSettings.advertisedInitialMaxStreamsBidi, conn_->transportSettings.advertisedInitialMaxStreamsUni, + conn_->transportSettings.advertisedExtendedAckFeatures, cwndHint); appToken.sourceAddresses = serverConn_->tokenSourceAddresses; appToken.version = conn_->version.value(); diff --git a/quic/server/handshake/AppToken.cpp b/quic/server/handshake/AppToken.cpp index 2725beb99..6286dc028 100644 --- a/quic/server/handshake/AppToken.cpp +++ b/quic/server/handshake/AppToken.cpp @@ -18,6 +18,7 @@ TicketTransportParameters createTicketTransportParameters( uint64_t initialMaxStreamDataUni, uint64_t initialMaxStreamsBidi, uint64_t initialMaxStreamsUni, + ExtendedAckFeatureMaskType extendedAckFeatures, Optional cwndHintBytes) { TicketTransportParameters params; params.parameters.push_back( @@ -39,6 +40,8 @@ TicketTransportParameters createTicketTransportParameters( TransportParameterId::initial_max_streams_bidi, initialMaxStreamsBidi)); params.parameters.push_back(encodeIntegerParameter( TransportParameterId::initial_max_streams_uni, initialMaxStreamsUni)); + params.parameters.push_back(encodeIntegerParameter( + TransportParameterId::extended_ack_features, extendedAckFeatures)); if (cwndHintBytes) { params.parameters.push_back((encodeIntegerParameter( TransportParameterId::cwnd_hint_bytes, *cwndHintBytes))); diff --git a/quic/server/handshake/AppToken.h b/quic/server/handshake/AppToken.h index 22ba67dbd..2bd1491ff 100644 --- a/quic/server/handshake/AppToken.h +++ b/quic/server/handshake/AppToken.h @@ -36,6 +36,7 @@ TicketTransportParameters createTicketTransportParameters( uint64_t initialMaxStreamDataUni, uint64_t initialMaxStreamsBidi, uint64_t initialMaxStreamsUni, + ExtendedAckFeatureMaskType extendedAckSupport, Optional cwndHintBytes = none); } // namespace quic diff --git a/quic/server/handshake/DefaultAppTokenValidator.cpp b/quic/server/handshake/DefaultAppTokenValidator.cpp index 2ce93378b..d0cc040fa 100644 --- a/quic/server/handshake/DefaultAppTokenValidator.cpp +++ b/quic/server/handshake/DefaultAppTokenValidator.cpp @@ -129,6 +129,16 @@ bool DefaultAppTokenValidator::validate( VLOG(10) << "Decreased max streams"; return validated = false; } + + auto ticketExtendedAckFeatures = + getIntegerParameter(TransportParameterId::extended_ack_features, params) + .value_or(0); + if (conn_->transportSettings.advertisedExtendedAckFeatures != + ticketExtendedAckFeatures) { + VLOG(10) << "Extended ack support changed"; + return validated = false; + } + conn_->transportParamsMatching = true; if (!validateAndUpdateSourceToken( diff --git a/quic/server/handshake/test/AppTokenTest.cpp b/quic/server/handshake/test/AppTokenTest.cpp index c33c7ded1..fb1d0241d 100644 --- a/quic/server/handshake/test/AppTokenTest.cpp +++ b/quic/server/handshake/test/AppTokenTest.cpp @@ -78,6 +78,14 @@ void expectAppTokenEqual( appToken.transportParams.parameters); EXPECT_EQ(cwndHintBytes, expectedCwndHintBytes); + auto extendedAckSupport = getIntegerParameter( + TransportParameterId::extended_ack_features, + decodedAppToken->transportParams.parameters); + auto expectedExtendedAckSupport = getIntegerParameter( + TransportParameterId::extended_ack_features, + appToken.transportParams.parameters); + EXPECT_EQ(extendedAckSupport, expectedExtendedAckSupport); + EXPECT_EQ( decodedAppToken->sourceAddresses.size(), appToken.sourceAddresses.size()); for (size_t ii = 0; ii < appToken.sourceAddresses.size(); ++ii) { @@ -104,7 +112,8 @@ TEST(AppTokenTest, TestEncodeAndDecodeNoSourceAddresses) { kDefaultStreamFlowControlWindow, kDefaultStreamFlowControlWindow, std::numeric_limits::max(), - std::numeric_limits::max()); + std::numeric_limits::max(), + 2 /* extendedAckSupport */); appToken.version = QuicVersion::MVFST; Buf buf = encodeAppToken(appToken); @@ -121,7 +130,8 @@ TEST(AppTokenTest, TestEncodeAndDecodeSingleIPv6Address) { kDefaultStreamFlowControlWindow, kDefaultStreamFlowControlWindow, std::numeric_limits::max(), - std::numeric_limits::max()); + std::numeric_limits::max(), + 2 /* extendedAckSupport */); appToken.sourceAddresses = { folly::IPAddress("2401:db00:2111:7283:face::46:0")}; appToken.version = QuicVersion::MVFST; @@ -140,7 +150,8 @@ TEST(AppTokenTest, TestEncodeAndDecodeThreeIPv6Addresses) { kDefaultStreamFlowControlWindow, kDefaultStreamFlowControlWindow, std::numeric_limits::max(), - std::numeric_limits::max()); + std::numeric_limits::max(), + 2 /* extendedAckSupport */); appToken.sourceAddresses = { folly::IPAddress("2401:db00:2111:7283:face::46:0"), folly::IPAddress("2401:db00:2111:7283:face::46:1"), @@ -161,7 +172,8 @@ TEST(AppTokenTest, TestEncodeAndDecodeSingleIPv4Address) { kDefaultStreamFlowControlWindow, kDefaultStreamFlowControlWindow, std::numeric_limits::max(), - std::numeric_limits::max()); + std::numeric_limits::max(), + 2 /* extendedAckSupport */); appToken.sourceAddresses = {folly::IPAddress("1.2.3.4")}; appToken.version = QuicVersion::MVFST; Buf buf = encodeAppToken(appToken); @@ -179,7 +191,8 @@ TEST(AppTokenTest, TestEncodeAndDecodeThreeIPv4Addresses) { kDefaultStreamFlowControlWindow, kDefaultStreamFlowControlWindow, std::numeric_limits::max(), - std::numeric_limits::max()); + std::numeric_limits::max(), + 2 /* extendedAckSupport */); appToken.sourceAddresses = { folly::IPAddress("1.2.3.4"), folly::IPAddress("1.2.3.5"), @@ -200,7 +213,8 @@ TEST(AppTokenTest, TestEncodeAndDecodeIPv6AndIPv4Addresses) { kDefaultStreamFlowControlWindow, kDefaultStreamFlowControlWindow, std::numeric_limits::max(), - std::numeric_limits::max()); + std::numeric_limits::max(), + 2 /* extendedAckSupport */); appToken.sourceAddresses = { folly::IPAddress("2401:db00:2111:7283:face::46:0"), folly::IPAddress("1.2.3.4"), @@ -221,7 +235,8 @@ TEST(AppTokenTest, TestEncodeAndDecodeWithAppToken) { kDefaultStreamFlowControlWindow, kDefaultStreamFlowControlWindow, std::numeric_limits::max(), - std::numeric_limits::max()); + std::numeric_limits::max(), + 2 /* extendedAckSupport */); appToken.appParams = folly::IOBuf::copyBuffer("QPACK Params"); appToken.version = QuicVersion::MVFST; Buf buf = encodeAppToken(appToken); @@ -239,7 +254,8 @@ TEST(AppTokenTest, TestEncodeAndDecodeIPv6AndIPv4AddressesWithAppToken) { kDefaultStreamFlowControlWindow, kDefaultStreamFlowControlWindow, std::numeric_limits::max(), - std::numeric_limits::max()); + std::numeric_limits::max(), + 2 /* extendedAckSupport */); appToken.sourceAddresses = { folly::IPAddress("2401:db00:2111:7283:face::46:0"), folly::IPAddress("1.2.3.4"), diff --git a/quic/server/handshake/test/DefaultAppTokenValidatorTest.cpp b/quic/server/handshake/test/DefaultAppTokenValidatorTest.cpp index 504eac648..154f6cc42 100644 --- a/quic/server/handshake/test/DefaultAppTokenValidatorTest.cpp +++ b/quic/server/handshake/test/DefaultAppTokenValidatorTest.cpp @@ -44,7 +44,8 @@ TEST(DefaultAppTokenValidatorTest, TestValidParams) { conn.transportSettings.advertisedInitialBidiRemoteStreamFlowControlWindow, conn.transportSettings.advertisedInitialUniStreamFlowControlWindow, conn.transportSettings.advertisedInitialMaxStreamsBidi, - conn.transportSettings.advertisedInitialMaxStreamsUni); + conn.transportSettings.advertisedInitialMaxStreamsUni, + conn.transportSettings.advertisedExtendedAckFeatures); ResumptionState resState; resState.appToken = encodeAppToken(appToken); @@ -75,7 +76,8 @@ TEST(DefaultAppTokenValidatorTest, TestValidOptionalParameter) { conn.transportSettings.advertisedInitialBidiRemoteStreamFlowControlWindow, conn.transportSettings.advertisedInitialUniStreamFlowControlWindow, conn.transportSettings.advertisedInitialMaxStreamsBidi, - conn.transportSettings.advertisedInitialMaxStreamsUni); + conn.transportSettings.advertisedInitialMaxStreamsUni, + conn.transportSettings.advertisedExtendedAckFeatures); appToken.transportParams.parameters.push_back( encodeIntegerParameter(TransportParameterId::disable_migration, 1)); ResumptionState resState; @@ -112,7 +114,8 @@ TEST( conn.transportSettings.advertisedInitialBidiRemoteStreamFlowControlWindow, conn.transportSettings.advertisedInitialUniStreamFlowControlWindow, conn.transportSettings.advertisedInitialMaxStreamsBidi, - conn.transportSettings.advertisedInitialMaxStreamsUni); + conn.transportSettings.advertisedInitialMaxStreamsUni, + conn.transportSettings.advertisedExtendedAckFeatures); ResumptionState resState; resState.appToken = encodeAppToken(appToken); @@ -197,7 +200,7 @@ TEST(DefaultAppTokenValidatorTest, TestInvalidMissingParams) { params.parameters.push_back(encodeIntegerParameter( TransportParameterId::max_packet_size, conn.transportSettings.maxRecvPacketSize)); - + appToken.sourceAddresses = {conn.peerAddress.getIPAddress()}; ResumptionState resState; resState.appToken = encodeAppToken(appToken); @@ -212,39 +215,45 @@ TEST(DefaultAppTokenValidatorTest, TestInvalidMissingParams) { EXPECT_FALSE(validator.validate(resState)); } -TEST(DefaultAppTokenValidatorTest, TestInvalidRedundantParameter) { - QuicServerConnectionState conn( - FizzServerQuicHandshakeContext::Builder().build()); - conn.peerAddress = folly::SocketAddress("1.2.3.4", 443); - conn.version = QuicVersion::MVFST; - auto quicStats = std::make_shared(); - conn.statsCallback = quicStats.get(); +// This test was not actually testing for redundant parameters. It was passing +// because the check on the source address was invalidating the token. +// The validator currently allows redundant parameters. +// TODO: Update the validator to reject redundant parameters? +// TEST(DefaultAppTokenValidatorTest, TestInvalidRedundantParameter) { +// QuicServerConnectionState conn( +// FizzServerQuicHandshakeContext::Builder().build()); +// conn.peerAddress = folly::SocketAddress("1.2.3.4", 443); +// conn.version = QuicVersion::MVFST; +// auto quicStats = std::make_shared(); +// conn.statsCallback = quicStats.get(); - AppToken appToken; - appToken.transportParams = createTicketTransportParameters( - conn.transportSettings.idleTimeout.count(), - conn.transportSettings.maxRecvPacketSize, - conn.transportSettings.advertisedInitialConnectionFlowControlWindow, - conn.transportSettings.advertisedInitialBidiLocalStreamFlowControlWindow, - conn.transportSettings.advertisedInitialBidiRemoteStreamFlowControlWindow, - conn.transportSettings.advertisedInitialUniStreamFlowControlWindow, - conn.transportSettings.advertisedInitialMaxStreamsBidi, - conn.transportSettings.advertisedInitialMaxStreamsUni); - appToken.transportParams.parameters.push_back( - encodeIntegerParameter(TransportParameterId::idle_timeout, 100)); - ResumptionState resState; - resState.appToken = encodeAppToken(appToken); +// AppToken appToken; +// appToken.transportParams = createTicketTransportParameters( +// conn.transportSettings.idleTimeout.count(), +// conn.transportSettings.maxRecvPacketSize, +// conn.transportSettings.advertisedInitialConnectionFlowControlWindow, +// conn.transportSettings.advertisedInitialBidiLocalStreamFlowControlWindow, +// conn.transportSettings.advertisedInitialBidiRemoteStreamFlowControlWindow, +// conn.transportSettings.advertisedInitialUniStreamFlowControlWindow, +// conn.transportSettings.advertisedInitialMaxStreamsBidi, +// conn.transportSettings.advertisedInitialMaxStreamsUni, +// conn.transportSettings.advertisedExtendedAckFeatures); +// appToken.transportParams.parameters.push_back( +// encodeIntegerParameter(TransportParameterId::idle_timeout, 100)); +// appToken.sourceAddresses = {conn.peerAddress.getIPAddress()}; +// ResumptionState resState; +// resState.appToken = encodeAppToken(appToken); - conn.earlyDataAppParamsValidator = [](const Optional&, - const Buf&) { - EXPECT_TRUE(false); - return true; - }; - DefaultAppTokenValidator validator(&conn); - EXPECT_CALL(*quicStats, onZeroRttAccepted()).Times(0); - EXPECT_CALL(*quicStats, onZeroRttRejected()); - EXPECT_FALSE(validator.validate(resState)); -} +// conn.earlyDataAppParamsValidator = [](const Optional&, +// const Buf&) { +// EXPECT_TRUE(false); +// return true; +// }; +// DefaultAppTokenValidator validator(&conn); +// EXPECT_CALL(*quicStats, onZeroRttAccepted()).Times(0); +// EXPECT_CALL(*quicStats, onZeroRttRejected()); +// EXPECT_FALSE(validator.validate(resState)); +// } TEST(DefaultAppTokenValidatorTest, TestInvalidDecreasedInitialMaxStreamData) { QuicServerConnectionState conn( @@ -266,7 +275,9 @@ TEST(DefaultAppTokenValidatorTest, TestInvalidDecreasedInitialMaxStreamData) { 1, conn.transportSettings.advertisedInitialUniStreamFlowControlWindow + 1, conn.transportSettings.advertisedInitialMaxStreamsBidi, - conn.transportSettings.advertisedInitialMaxStreamsUni); + conn.transportSettings.advertisedInitialMaxStreamsUni, + conn.transportSettings.advertisedExtendedAckFeatures); + appToken.sourceAddresses = {conn.peerAddress.getIPAddress()}; ResumptionState resState; resState.appToken = encodeAppToken(appToken); @@ -296,7 +307,9 @@ TEST(DefaultAppTokenValidatorTest, TestChangedIdleTimeout) { conn.transportSettings.advertisedInitialBidiRemoteStreamFlowControlWindow, conn.transportSettings.advertisedInitialUniStreamFlowControlWindow, conn.transportSettings.advertisedInitialMaxStreamsBidi, - conn.transportSettings.advertisedInitialMaxStreamsUni); + conn.transportSettings.advertisedInitialMaxStreamsUni, + conn.transportSettings.advertisedExtendedAckFeatures); + appToken.sourceAddresses = {conn.peerAddress.getIPAddress()}; ResumptionState resState; resState.appToken = encodeAppToken(appToken); @@ -328,7 +341,9 @@ TEST(DefaultAppTokenValidatorTest, TestDecreasedInitialMaxStreams) { conn.transportSettings.advertisedInitialBidiRemoteStreamFlowControlWindow, conn.transportSettings.advertisedInitialUniStreamFlowControlWindow, conn.transportSettings.advertisedInitialMaxStreamsBidi + 1, - conn.transportSettings.advertisedInitialMaxStreamsUni + 1); + conn.transportSettings.advertisedInitialMaxStreamsUni + 1, + conn.transportSettings.advertisedExtendedAckFeatures); + appToken.sourceAddresses = {conn.peerAddress.getIPAddress()}; ResumptionState resState; resState.appToken = encodeAppToken(appToken); @@ -343,6 +358,38 @@ TEST(DefaultAppTokenValidatorTest, TestDecreasedInitialMaxStreams) { EXPECT_FALSE(validator.validate(resState)); } +TEST(DefaultAppTokenValidatorTest, TestInvalidExtendedAckSupportChanged) { + QuicServerConnectionState conn( + FizzServerQuicHandshakeContext::Builder().build()); + conn.peerAddress = folly::SocketAddress("1.2.3.4", 443); + conn.version = QuicVersion::MVFST; + auto quicStats = std::make_shared(); + conn.statsCallback = quicStats.get(); + + AppToken appToken; + appToken.transportParams = createTicketTransportParameters( + conn.transportSettings.idleTimeout.count(), + conn.transportSettings.maxRecvPacketSize, + conn.transportSettings.advertisedInitialConnectionFlowControlWindow, + conn.transportSettings.advertisedInitialBidiLocalStreamFlowControlWindow, + conn.transportSettings.advertisedInitialBidiRemoteStreamFlowControlWindow, + conn.transportSettings.advertisedInitialUniStreamFlowControlWindow, + conn.transportSettings.advertisedInitialMaxStreamsBidi, + conn.transportSettings.advertisedInitialMaxStreamsUni, + conn.transportSettings.advertisedExtendedAckFeatures + 1); + appToken.sourceAddresses = {conn.peerAddress.getIPAddress()}; + ResumptionState resState; + resState.appToken = encodeAppToken(appToken); + + conn.earlyDataAppParamsValidator = [](const Optional&, + const Buf&) { + EXPECT_TRUE(false); + return true; + }; + DefaultAppTokenValidator validator(&conn); + EXPECT_FALSE(validator.validate(resState)); +} + TEST(DefaultAppTokenValidatorTest, TestInvalidAppParams) { QuicServerConnectionState conn( FizzServerQuicHandshakeContext::Builder().build()); @@ -363,7 +410,9 @@ TEST(DefaultAppTokenValidatorTest, TestInvalidAppParams) { conn.transportSettings.advertisedInitialBidiRemoteStreamFlowControlWindow, conn.transportSettings.advertisedInitialUniStreamFlowControlWindow, conn.transportSettings.advertisedInitialMaxStreamsBidi, - conn.transportSettings.advertisedInitialMaxStreamsUni); + conn.transportSettings.advertisedInitialMaxStreamsUni, + conn.transportSettings.advertisedExtendedAckFeatures); + appToken.sourceAddresses = {conn.peerAddress.getIPAddress()}; ResumptionState resState; resState.appToken = encodeAppToken(appToken); @@ -392,7 +441,8 @@ class SourceAddressTokenTest : public Test { .advertisedInitialBidiRemoteStreamFlowControlWindow, conn_.transportSettings.advertisedInitialUniStreamFlowControlWindow, conn_.transportSettings.advertisedInitialMaxStreamsBidi, - conn_.transportSettings.advertisedInitialMaxStreamsUni); + conn_.transportSettings.advertisedInitialMaxStreamsUni, + conn_.transportSettings.advertisedExtendedAckFeatures); } void encodeAndValidate(bool acceptZeroRtt = true) { diff --git a/quic/server/state/ServerStateMachine.cpp b/quic/server/state/ServerStateMachine.cpp index d307aea4c..71bf7a462 100644 --- a/quic/server/state/ServerStateMachine.cpp +++ b/quic/server/state/ServerStateMachine.cpp @@ -250,6 +250,11 @@ void processClientInitialParams( TransportParameterId::knob_frames_supported), clientParams.parameters); + auto extendedAckFeatures = getIntegerParameter( + static_cast( + TransportParameterId::extended_ack_features), + clientParams.parameters); + auto reliableResetTpIter = findParameter( clientParams.parameters, static_cast( @@ -381,6 +386,7 @@ void processClientInitialParams( } conn.peerAdvertisedKnobFrameSupport = knobFrameSupported.value_or(0) > 0; + conn.peerAdvertisedExtendedAckFeatures = extendedAckFeatures.value_or(0); } void updateHandshakeState(QuicServerConnectionState& conn) { @@ -1113,6 +1119,21 @@ void onServerReadDataFromOpen( << conn; isNonProbingPacket = true; ReadAckFrame& ackFrame = *quicFrame.asReadAckFrame(); + + if (ackFrame.frameType == FrameType::ACK_EXTENDED && + !conn.transportSettings.advertisedExtendedAckFeatures) { + throw QuicTransportException( + "Received unexpected ACK_EXTENDED frame", + TransportErrorCode::PROTOCOL_VIOLATION); + } else if ( + ackFrame.frameType == FrameType::ACK_RECEIVE_TIMESTAMPS && + !conn.transportSettings + .maybeAckReceiveTimestampsConfigSentToPeer) { + throw QuicTransportException( + "Received unexpected ACK_RECEIVE_TIMESTAMPS frame", + TransportErrorCode::PROTOCOL_VIOLATION); + } + conn.lastProcessedAckEvents.emplace_back(processAckFrame( conn, packetNumberSpace, diff --git a/quic/server/test/ServerStateMachineTest.cpp b/quic/server/test/ServerStateMachineTest.cpp index 799ba2f88..fd4075e28 100644 --- a/quic/server/test/ServerStateMachineTest.cpp +++ b/quic/server/test/ServerStateMachineTest.cpp @@ -336,6 +336,28 @@ TEST(ServerStateMachineTest, TestEncodeKnobFrameSupportedParamDisabled) { testing::Eq(TransportParameterId::knob_frames_supported))))); } +TEST(ServerStateMachineTest, TestProcessExtendedAckSupportParam) { + QuicServerConnectionState serverConn( + FizzServerQuicHandshakeContext::Builder().build()); + std::vector transportParams; + transportParams.push_back( + encodeIntegerParameter(TransportParameterId::extended_ack_features, 7)); + ClientTransportParameters clientTransportParams = { + std::move(transportParams)}; + processClientInitialParams(serverConn, clientTransportParams); + EXPECT_EQ(serverConn.peerAdvertisedExtendedAckFeatures, 7); +} + +TEST(ServerStateMachineTest, TestProcessExtendedAckSupportParamNotSent) { + QuicServerConnectionState serverConn( + FizzServerQuicHandshakeContext::Builder().build()); + std::vector transportParams; + ClientTransportParameters clientTransportParams = { + std::move(transportParams)}; + processClientInitialParams(serverConn, clientTransportParams); + EXPECT_EQ(serverConn.peerAdvertisedExtendedAckFeatures, 0); +} + TEST( ServerStateMachineTest, TestProcessReliableStreamResetSupportedParamEnabled) { diff --git a/quic/state/StateData.h b/quic/state/StateData.h index d28012efe..154d94b6b 100644 --- a/quic/state/StateData.h +++ b/quic/state/StateData.h @@ -703,6 +703,8 @@ struct QuicConnectionStateBase : public folly::DelayedDestruction { bool peerAdvertisedReliableStreamResetSupport{false}; bool peerAdvertisedKnobFrameSupport{false}; + + ExtendedAckFeatureMaskType peerAdvertisedExtendedAckFeatures{0}; // Retransmission policies map. folly::F14FastMap retransmissionPolicies; diff --git a/quic/state/TransportSettings.h b/quic/state/TransportSettings.h index 1118f7b07..38aaa77ec 100644 --- a/quic/state/TransportSettings.h +++ b/quic/state/TransportSettings.h @@ -335,6 +335,16 @@ struct TransportSettings { bool dropIngressOnStopSending{false}; bool advertisedReliableResetStreamSupport{false}; bool advertisedKnobFrameSupport{true}; + + // Extended ACK support to advertise to the peer. This is what we expect to + // receive from the peer inside ACK_EXTENDED frames. + // 0 means no support. + // Otherwise the bits of the integer indicate the following: + // - Bit 0: support for ACK_EXTENDED frame with ECN fields + // - Bit 1: support for ACK_EXTENDED frame with receive timestamp fields + // The list of features corresponds to ExtendedAckFeatures in QuicConstants + ExtendedAckFeatureMaskType advertisedExtendedAckFeatures{0}; + bool removeStreamAfterEomCallbackUnset{false}; // Whether to include cwnd hint in new session tickets for 0-rtt bool includeCwndHintsInSessionTicket{false};