Ensure clientAuthRequested is set when negotiated PSK does not have c…

…erts

Summary:
The `Finished` handler assumes that `clientAuthRequested` is always set. Prior
to this diff, if a CachedPsk was used for a resumption connection where the
CachedPsk itself does not have a `serverCert` or a `clientCert` set (which can
happen if the PSK serialization/deserialization does not support encoding/decoding
the certificate), then this would not be set, causing the handshake to fail
even though the client successfully negotiated a resumption.

This diff changes the behavior so that if a PSK was successfully negotiated
(but the certs are null), then this would be treated as a `NotRequested`
client auth state. Since this is a resumed connection anyway, there would not
be a client certificate sent, so this field is effectively a no-op wrt TLS
logic.

Reviewed By: knekritz

Differential Revision: D56281894

fbshipit-source-id: b528d745e8e17e9328063b490d646aee79956cde

fizz/client/ClientProtocol.cpp

@@ -1297,9 +1297,7 @@ EventHandler<ClientTypes, StateEnum::ExpectingServerHello, Event::ServerHello>::
   }
 
   // Servers cannot accept GREASE PSK
-  if (echStatus == ECHStatus::Rejected &&
-      (negotiatedPsk.type == PskType::External ||
-       negotiatedPsk.type == PskType::Resumption)) {
+  if (echStatus == ECHStatus::Rejected && isPskAccepted(negotiatedPsk.type)) {
     throw FizzException(
         "ech rejected but server accepted psk",
         AlertDescription::illegal_parameter);
@@ -1346,10 +1344,12 @@ EventHandler<ClientTypes, StateEnum::ExpectingServerHello, Event::ServerHello>::
       folly::IOBuf::copyBuffer(handshakeReadSecret.secret);
 
   folly::Optional<ClientAuthType> authType;
-  if (negotiatedPsk.clientCert) {
-    authType = ClientAuthType::Stored;
-  } else if (negotiatedPsk.serverCert) {
-    authType = ClientAuthType::NotRequested;
+  if (isPskAccepted(negotiatedPsk.type)) {
+    if (negotiatedPsk.clientCert) {
+      authType = ClientAuthType::Stored;
+    } else {
+      authType = ClientAuthType::NotRequested;
+    }
   }
 
   std::chrono::system_clock::time_point handshakeTime;
@@ -1803,8 +1803,7 @@ Actions EventHandler<
         }
       });
 
-  if (state.pskType() == PskType::Resumption ||
-      state.pskType() == PskType::External) {
+  if (isPskAccepted(*state.pskType())) {
     return actions(
         std::move(mutateState),
         MutateState(&Transition<StateEnum::ExpectingFinished>));

fizz/client/test/ClientProtocolTest.cpp

@@ -173,6 +173,7 @@ class ClientProtocolTest : public ProtocolTest<ClientTypes, Actions> {
     state_.version() = TestProtocolVersion;
     state_.cipher() = CipherSuite::TLS_AES_128_GCM_SHA256;
     state_.state() = StateEnum::ExpectingEncryptedExtensions;
+    state_.pskType() = PskType::NotAttempted;
     state_.requestedExtensions() = std::vector<ExtensionType>(
         {ExtensionType::supported_versions,
          ExtensionType::key_share,
@@ -5478,6 +5479,31 @@ TEST_F(ClientProtocolTest, TestWaitForDataSizeHint) {
   auto wfd = expectAction<WaitForData>(actions);
   EXPECT_EQ(wfd.recordSizeHint, 17);
 }
+
+TEST_F(ClientProtocolTest, TestPskWithoutCerts) {
+  // Because CachedPsks can be serialized, and because certificates may fail
+  // to serialize for whatever reason, there may be an instance where a client
+  // uses a deserialized cached psk that does not contain either a client or
+  // a server certificate, but the PSK itself is valid (and the server accepted
+  // the offered PSK).
+  setupExpectingServerHello();
+
+  CachedPsk psk = getCachedPsk();
+  psk.clientCert = nullptr;
+  psk.serverCert = nullptr;
+
+  state_.attemptedPsk() = psk;
+
+  auto actions = detail::processEvent(state_, TestMessages::serverHelloPsk());
+  processStateMutations(actions);
+
+  ASSERT_TRUE(state_.pskType().has_value());
+  EXPECT_EQ(state_.pskType().value(), PskType::Resumption);
+
+  ASSERT_TRUE(state_.clientAuthRequested().has_value());
+  EXPECT_EQ(state_.clientAuthRequested().value(), ClientAuthType::NotRequested);
+}
+
 } // namespace test
 } // namespace client
 } // namespace fizz

fizz/protocol/Types.h

@@ -23,6 +23,22 @@ enum class PskType {
   Resumption
 };
 
+/**
+ * `isPskAccepted` returning true implies that the handshake negotiated a PSK to
+ * be used.
+ */
+inline bool isPskAccepted(PskType t) {
+  switch (t) {
+    case PskType::NotSupported:
+    case PskType::NotAttempted:
+    case PskType::Rejected:
+      return false;
+    case PskType::External:
+    case PskType::Resumption:
+      return true;
+  }
+}
+
 /**
  * Encryption level for the TLS layer.
  */