Do not update handshake-related transport settings from the 0-rtt ticket

Summary:
When the server accepts a 0-rtt ticket, it updates the connection's transport settings with the contents of the ticket. This is the value used included in the next ticket it sends the client. However, the handshake layer has a copy of the original transport parameters that was created with the first received packet. This copy in the handshake layer does not get updated. This can cause a mismatch between the value sent to the client in the handshake, and the value encoded inside the ticket.

This change avoid using the 0-rtt ticket for updating any transport settings that are also included in the handshake transport params.

Reviewed By: hanidamlaj

Differential Revision: D51121317

fbshipit-source-id: 55e71965185dff553d16d4c5fbcb1e2f9acdc690

quic/server/QuicServerTransport.cpp

@@ -566,15 +566,15 @@ bool QuicServerTransport::shouldWriteNewSessionTicket() {
 void QuicServerTransport::maybeWriteNewSessionTicket() {
   if (shouldWriteNewSessionTicket() && !ctx_->getSendNewSessionTicket() &&
       serverConn_->serverHandshakeLayer->isHandshakeDone()) {
-    VLOG(7) << "Writing a new session ticket with cwnd="
-            << conn_->congestionController->getCongestionWindow();
     if (conn_->qLogger) {
       conn_->qLogger->addTransportStateUpdate(kWriteNst);
     }
     newSessionTicketWrittenTimestamp_ = Clock::now();
     folly::Optional<uint64_t> cwndHint = folly::none;
     if (conn_->transportSettings.includeCwndHintsInSessionTicket &&
         conn_->congestionController) {
+      VLOG(7) << "Writing a new session ticket with cwnd="
+              << conn_->congestionController->getCongestionWindow();
       cwndHint = conn_->congestionController->getCongestionWindow();
       newSessionTicketWrittenCwndHint_ = cwndHint;
     }

quic/server/handshake/DefaultAppTokenValidator.cpp

@@ -129,14 +129,6 @@ bool DefaultAppTokenValidator::validate(
     VLOG(10) << "Decreased max streams";
     return validated = false;
   }
-
-  // This is an optional parameter in the ticket
-  auto cwndHintBytes =
-      getIntegerParameter(TransportParameterId::cwnd_hint_bytes, params);
-
-  // TODO max ack delay, is this really necessary?
-  // spec says disable_migration should also be in the ticket. It shouldn't.
-
   conn_->transportParamsMatching = true;
 
   if (!validateAndUpdateSourceToken(
@@ -154,18 +146,6 @@ bool DefaultAppTokenValidator::validate(
     return validated = false;
   }
 
-  updateTransportParamsFromTicket(
-      *conn_,
-      *ticketIdleTimeout,
-      *ticketPacketSize,
-      *ticketMaxData,
-      *ticketMaxStreamDataBidiLocal,
-      *ticketMaxStreamDataBidiRemote,
-      *ticketMaxStreamDataUni,
-      *ticketMaxStreamsBidi,
-      *ticketMaxStreamsUni,
-      cwndHintBytes);
-
   return validated;
 }
 

quic/server/handshake/ServerHandshake.cpp

@@ -253,6 +253,10 @@ void ServerHandshake::processPendingEvents() {
   }
 }
 
+const folly::Optional<Buf>& ServerHandshake::getAppToken() const {
+  return state_.appToken();
+}
+
 class ServerHandshake::ActionMoveVisitor : public boost::static_visitor<> {
  public:
   explicit ActionMoveVisitor(ServerHandshake& server) : server_(server) {}

quic/server/handshake/ServerHandshake.h

@@ -213,6 +213,11 @@ class ServerHandshake : public Handshake {
    */
   void processPendingEvents();
 
+  /**
+   * Returns the AppToken seen in session ticket if the session was resumed.
+   */
+  const folly::Optional<Buf>& getAppToken() const;
+
  protected:
   Phase phase_{Phase::Handshake};
 

quic/server/handshake/test/DefaultAppTokenValidatorTest.cpp

@@ -124,11 +124,12 @@ TEST(
   EXPECT_CALL(*quicStats, onZeroRttAccepted());
   EXPECT_TRUE(validator.validate(resState));
 
+  // Transport settings will not be updated by the ticket.
   EXPECT_EQ(
       conn.transportSettings.advertisedInitialConnectionFlowControlWindow,
-      initialMaxData - 1);
-  EXPECT_EQ(conn.flowControlState.windowSize, initialMaxData - 1);
-  EXPECT_EQ(conn.flowControlState.advertisedMaxOffset, initialMaxData - 1);
+      initialMaxData);
+  EXPECT_EQ(conn.flowControlState.windowSize, initialMaxData);
+  EXPECT_EQ(conn.flowControlState.advertisedMaxOffset, initialMaxData);
 }
 
 TEST(DefaultAppTokenValidatorTest, TestInvalidNullAppToken) {

quic/server/state/ServerStateMachine.cpp

@@ -5,6 +5,7 @@
  * LICENSE file in the root directory of this source tree.
  */
 
+#include <quic/fizz/server/handshake/AppToken.h>
 #include <quic/server/handshake/TokenGenerator.h>
 #include <quic/server/state/ServerStateMachine.h>
 
@@ -454,6 +455,8 @@ void updateHandshakeState(QuicServerConnectionState& conn) {
     if (!conn.sentHandshakeDone) {
       sendSimpleFrame(conn, HandshakeDoneFrame());
       conn.sentHandshakeDone = true;
+      maybeUpdateTransportFromAppToken(
+          conn, conn.serverHandshakeLayer->getAppToken());
     }
 
     if (!conn.sentNewTokenFrame &&
@@ -535,39 +538,33 @@ void updateWritableByteLimitOnRecvPacket(QuicServerConnectionState& conn) {
   }
 }
 
-void updateTransportParamsFromTicket(
+void maybeUpdateTransportFromAppToken(
     QuicServerConnectionState& conn,
-    uint64_t idleTimeout,
-    uint64_t maxRecvPacketSize,
-    uint64_t initialMaxData,
-    uint64_t initialMaxStreamDataBidiLocal,
-    uint64_t initialMaxStreamDataBidiRemote,
-    uint64_t initialMaxStreamDataUni,
-    uint64_t initialMaxStreamsBidi,
-    uint64_t initialMaxStreamsUni,
-    folly::Optional<uint64_t> maybeCwndHintBytes) {
-  conn.transportSettings.idleTimeout = std::chrono::milliseconds(idleTimeout);
-  conn.transportSettings.maxRecvPacketSize = maxRecvPacketSize;
-
-  conn.transportSettings.advertisedInitialConnectionFlowControlWindow =
-      initialMaxData;
-  conn.transportSettings.advertisedInitialBidiLocalStreamFlowControlWindow =
-      initialMaxStreamDataBidiLocal;
-  conn.transportSettings.advertisedInitialBidiRemoteStreamFlowControlWindow =
-      initialMaxStreamDataBidiRemote;
-  conn.transportSettings.advertisedInitialUniStreamFlowControlWindow =
-      initialMaxStreamDataUni;
-  updateFlowControlStateWithSettings(
-      conn.flowControlState, conn.transportSettings);
-
-  conn.transportSettings.advertisedInitialMaxStreamsBidi =
-      initialMaxStreamsBidi;
-  conn.transportSettings.advertisedInitialMaxStreamsUni = initialMaxStreamsUni;
-
-  conn.maybeCwndHintBytes = maybeCwndHintBytes;
-
+    const folly::Optional<Buf>& tokenBuf) {
+  if (!tokenBuf) {
+    return;
+  }
+  auto appToken = decodeAppToken(*tokenBuf.value());
+  if (!appToken) {
+    VLOG(10) << "Failed to decode app token";
+    return;
+  }
+  auto& params = appToken->transportParams.parameters;
+  auto maybeCwndHintBytes =
+      getIntegerParameter(TransportParameterId::cwnd_hint_bytes, params);
   if (maybeCwndHintBytes) {
     QUIC_STATS(conn.statsCallback, onCwndHintBytesSample, *maybeCwndHintBytes);
+
+    // Only use the cwndHint if the source address is included in the token
+    DCHECK(conn.peerAddress.isInitialized());
+    auto addressMatches =
+        std::find(
+            appToken->sourceAddresses.begin(),
+            appToken->sourceAddresses.end(),
+            conn.peerAddress.getIPAddress()) != appToken->sourceAddresses.end();
+    if (addressMatches) {
+      conn.maybeCwndHintBytes = maybeCwndHintBytes;
+    }
   }
 }
 

quic/server/state/ServerStateMachine.h

@@ -219,17 +219,9 @@ bool validateAndUpdateSourceToken(
 
 void updateWritableByteLimitOnRecvPacket(QuicServerConnectionState& conn);
 
-void updateTransportParamsFromTicket(
+void maybeUpdateTransportFromAppToken(
     QuicServerConnectionState& conn,
-    uint64_t idleTimeout,
-    uint64_t maxRecvPacketSize,
-    uint64_t initialMaxData,
-    uint64_t initialMaxStreamDataBidiLocal,
-    uint64_t initialMaxStreamDataBidiRemote,
-    uint64_t initialMaxStreamDataUni,
-    uint64_t initialMaxStreamsBidi,
-    uint64_t initialMaxStreamsUni,
-    folly::Optional<uint64_t> maybeCwndHintBytes);
+    const folly::Optional<Buf>& appToken);
 
 void onConnectionMigration(
     QuicServerConnectionState& conn,