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

@@ -2228,6 +2228,10 @@ folly::Expected<folly::Unit, QuicError> 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<folly::Unit, QuicError> maybeHandleIncomingKeyUpdate(
 folly::Expected<folly::Unit, QuicError> 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<folly::Unit, QuicError> 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 =

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 <typename T>
 struct AckEventMatcherBuilder {
   using Builder = AckEventMatcherBuilder;