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};