From 371f19df8e329229ef5eac7731f08992aaffe6b8 Mon Sep 17 00:00:00 2001 From: Joseph Beshay Date: Tue, 20 May 2025 14:04:58 -0700 Subject: [PATCH] Accommodate old MVFST clients when attempting to intiate key updates Summary: Some old MVFST clients did not support key updates. This change prevents the server from initiating key updates to clients using the MVFST quic version until the client has initiated a key update. This ensures that key updates are triggered for clients that support them. Reviewed By: mjoras Differential Revision: D75022528 fbshipit-source-id: 7af2d65becbfefee347cbe98ccdfc01323cd153c --- quic/api/QuicTransportFunctions.cpp | 15 +++ quic/api/test/QuicTypedTransportTest.cpp | 111 +++++++++++++++++++++++ 2 files changed, 126 insertions(+) diff --git a/quic/api/QuicTransportFunctions.cpp b/quic/api/QuicTransportFunctions.cpp index 31c73bdbb..1b6d4b7aa 100644 --- a/quic/api/QuicTransportFunctions.cpp +++ b/quic/api/QuicTransportFunctions.cpp @@ -2228,6 +2228,10 @@ folly::Expected maybeHandleIncomingKeyUpdate( } conn.readCodec->setNextOneRttReadCipher( std::move(nextOneRttReadCipherResult.value())); + + // The peer has initiated a key update. We should use the regular key + // update interval if we are initiating key updates. + conn.transportSettings.firstKeyUpdatePacketCount.reset(); } return folly::unit; } @@ -2235,6 +2239,14 @@ folly::Expected maybeHandleIncomingKeyUpdate( folly::Expected maybeInitiateKeyUpdate( QuicConnectionStateBase& conn) { if (conn.transportSettings.initiateKeyUpdate) { + if (conn.nodeType == QuicNodeType::Server && conn.version.has_value() && + conn.version.value() == QuicVersion::MVFST && + conn.transportSettings.firstKeyUpdatePacketCount.has_value()) { + // Some old versions of MVFST did not support key updates. + // So as the server, do not attempt to initiate key updates if the client + // hasn't initiated one yet. + return folly::unit; + } auto packetsBeforeNextUpdate = conn.transportSettings.firstKeyUpdatePacketCount ? conn.transportSettings.firstKeyUpdatePacketCount.value() @@ -2244,6 +2256,9 @@ folly::Expected maybeInitiateKeyUpdate( conn.readCodec->canInitiateKeyUpdate()) { QUIC_STATS(conn.statsCallback, onKeyUpdateAttemptInitiated); conn.readCodec->advanceOneRttReadPhase(); + + //  We have initiated a key update. We should use the regular key + // update from now on. conn.transportSettings.firstKeyUpdatePacketCount.reset(); auto nextOneRttWriteCipherResult = diff --git a/quic/api/test/QuicTypedTransportTest.cpp b/quic/api/test/QuicTypedTransportTest.cpp index e0023299f..02e0980a6 100644 --- a/quic/api/test/QuicTypedTransportTest.cpp +++ b/quic/api/test/QuicTypedTransportTest.cpp @@ -1450,6 +1450,13 @@ TYPED_TEST(QuicTypedTransportAfterStartTest, InitiateKeyUpdateFailure) { * Initiate the first key update - Successful attempt */ TYPED_TEST(QuicTypedTransportAfterStartTest, InitiateFirstKeyUpdateSuccess) { + // Use QUIC_V1 for the server since MVFST has special handling to accommodate + // older client versions without key update support. That behavior is covered + // in another test + if (this->getConn().nodeType == QuicNodeType::Server) { + this->getNonConstConn().version = QuicVersion::QUIC_V1; + } + ASSERT_EQ(this->getConn().oneRttWritePhase, ProtectionType::KeyPhaseZero); auto numberOfWrittenPacketsInPhase = this->getConn().oneRttWritePacketsSentInCurrentPhase; @@ -1523,6 +1530,110 @@ TYPED_TEST(QuicTypedTransportAfterStartTest, InitiateFirstKeyUpdateSuccess) { this->destroyTransport(); } +/** + * As a server, do not initiate key updates for QuicVersion::MVFST unless the + * client has performed a key update. + * (Old versions of MVFST did not support key updates) + */ +TYPED_TEST( + QuicTypedTransportAfterStartTest, + ServerInitiateKeyUpdateForMvfstClient) { + if (this->getConn().nodeType != QuicNodeType::Server) { + // This test is for the server behavior only + return; + } + + ASSERT_EQ(this->getConn().oneRttWritePhase, ProtectionType::KeyPhaseZero); + this->getNonConstConn().version = QuicVersion::MVFST; + + auto numberOfWrittenPacketsInPhase = + this->getConn().oneRttWritePacketsSentInCurrentPhase; + + auto streamId = this->getTransport()->createBidirectionalStream().value(); + + { + // Send and receive a packet in the current phase + this->getTransport()->writeChain( + streamId, IOBuf::copyBuffer("hello"), false); + this->loopForWrites(); + EXPECT_EQ( + this->getConn().oneRttWritePacketsSentInCurrentPhase, + ++numberOfWrittenPacketsInPhase); + + this->deliverPacket(this->buildPeerPacketWithStreamData( + streamId, IOBuf::copyBuffer("hello2"), ProtectionType::KeyPhaseZero)); + + // Both read and writer ciphers should be in phase zero. + EXPECT_EQ( + this->getConn().readCodec->getCurrentOneRttReadPhase(), + ProtectionType::KeyPhaseZero); + EXPECT_EQ(this->getConn().oneRttWritePhase, ProtectionType::KeyPhaseZero); + } + + { + // First key update is due but it shouldn't trigger one because connection + // is for QuicVersion::MVFST and the peer hasn't initiated a key update yet + + QuicConnectionStateBase& conn = this->getNonConstConn(); + conn.transportSettings.initiateKeyUpdate = true; + // Trigger this update as a first update + conn.transportSettings.firstKeyUpdatePacketCount = + numberOfWrittenPacketsInPhase; + // Periodic interval is high enough not to be triggered + conn.transportSettings.keyUpdatePacketCountInterval = + kDefaultKeyUpdatePacketCountInterval; + + // A key update should be triggered after this write is completed. + this->getTransport()->writeChain( + streamId, IOBuf::copyBuffer("hello3"), false); + this->loopForWrites(); + + EXPECT_EQ( + this->getConn().oneRttWritePacketsSentInCurrentPhase, + ++numberOfWrittenPacketsInPhase); + + // Both read and writer ciphers should still be at Phase Zero + EXPECT_EQ( + this->getConn().readCodec->getCurrentOneRttReadPhase(), + ProtectionType::KeyPhaseZero); + EXPECT_EQ(this->getConn().oneRttWritePhase, ProtectionType::KeyPhaseZero); + } + + // Clear firstKeyUpdatePacketCount to indicate that a key update was performed + // by the peer. + this->getNonConstConn().transportSettings.firstKeyUpdatePacketCount.reset(); + + { + // Now that the peer had initiated a key update, the server can trigger one + // too. + QuicConnectionStateBase& conn = this->getNonConstConn(); + conn.transportSettings.initiateKeyUpdate = true; + // Indicate that a regular update is due + conn.transportSettings.keyUpdatePacketCountInterval = + numberOfWrittenPacketsInPhase; + + // A key update should be triggered after this write is completed. + this->getTransport()->writeChain( + streamId, IOBuf::copyBuffer("hello3"), false); + this->loopForWrites(); + + numberOfWrittenPacketsInPhase = 0; + EXPECT_EQ( + this->getConn().oneRttWritePacketsSentInCurrentPhase, + numberOfWrittenPacketsInPhase); + + // Read and writer ciphers should advance to Key Phase One + EXPECT_EQ( + this->getConn().readCodec->getCurrentOneRttReadPhase(), + ProtectionType::KeyPhaseOne); + EXPECT_EQ(this->getConn().oneRttWritePhase, ProtectionType::KeyPhaseOne); + } + + this->getNonConstConn().outstandings.reset(); + + this->destroyTransport(); +} + template struct AckEventMatcherBuilder { using Builder = AckEventMatcherBuilder;