Translate several OpenSSL verify error codes into corresponding TLS a…

…lerts Summary: Specifically, certificate_expired alerts for expired certificates. Reviewed By: frqiu Differential Revision: D57076898 fbshipit-source-id: bf8a1638dba5ab86c93628aca4ed41bc44e60400
facebook · Jan 29, 2025 · f4e6d36 · f4e6d36
1 parent a258c4c
commit f4e6d36
Show file tree

Hide file tree

Showing 3 changed files with 88 additions and 12 deletions.
diff --git a/third-party/fizz/src/fizz/protocol/DefaultCertificateVerifier.cpp b/third-party/fizz/src/fizz/protocol/DefaultCertificateVerifier.cpp
@@ -18,6 +18,21 @@ struct STACK_OF_X509_deleter {
   }
 };
 
+static AlertDescription toTLSAlert(int opensslVerifyErr) {
+  switch (opensslVerifyErr) {
+    case X509_V_ERR_CERT_REVOKED:
+      return AlertDescription::certificate_revoked;
+    case X509_V_ERR_CERT_NOT_YET_VALID:
+    case X509_V_ERR_CERT_HAS_EXPIRED:
+      /**
+       * "A certificate has expired or is not currently valid."
+       */
+      return AlertDescription::certificate_expired;
+    default:
+      return AlertDescription::bad_certificate;
+  }
+}
+
 /* static */ std::unique_ptr<DefaultCertificateVerifier>
 DefaultCertificateVerifier::createFromCAFile(
     VerificationContext context,
@@ -120,9 +135,11 @@ DefaultCertificateVerifier::verifyWithX509StoreCtx(
 
   if (ret != 1) {
     const auto errorInt = X509_STORE_CTX_get_error(ctx.get());
-    std::string errorText =
-        std::string(X509_verify_cert_error_string(errorInt));
-    throw std::runtime_error("certificate verification failed: " + errorText);
+    throw FizzVerificationException(
+        fmt::format(
+            "certificate verification failed: {}",
+            X509_verify_cert_error_string(errorInt)),
+        toTLSAlert(errorInt));
   }
 
   return ctx;

diff --git a/third-party/fizz/src/fizz/protocol/test/DefaultCertificateVerifierTest.cpp b/third-party/fizz/src/fizz/protocol/test/DefaultCertificateVerifierTest.cpp
@@ -44,6 +44,28 @@ class DefaultCertificateVerifierTest : public testing::Test {
   std::unique_ptr<DefaultCertificateVerifier> verifier_;
 };
 
+template <class F1, class F2>
+void expectThrowSuchThat(F1 f, F2 g) {
+  try {
+    f();
+    FAIL() << "Expected to throw";
+  } catch (const std::exception& ex) {
+    g(ex);
+  } catch (...) {
+    FAIL() << "non std::exception thrown";
+  }
+}
+
+template <class F>
+static void expectThrowWithAlert(F f, AlertDescription ad) {
+  return expectThrowSuchThat(std::move(f), [ad](const auto& ex) {
+    auto fe = dynamic_cast<const fizz::FizzException*>(&ex);
+    ASSERT_TRUE(fe);
+    ASSERT_TRUE(fe->getAlert().has_value());
+    EXPECT_EQ(fe->getAlert().value(), ad);
+  });
+}
+
 TEST_F(DefaultCertificateVerifierTest, TestVerifySuccess) {
   verifier_->verify({getPeerCert(leafCertAndKey_)});
 
@@ -69,9 +91,9 @@ TEST_F(DefaultCertificateVerifierTest, TestVerifyWithIntermediates) {
 
 TEST_F(DefaultCertificateVerifierTest, TestVerifySelfSignedCert) {
   auto selfsigned = createCert("self", false, nullptr);
-  EXPECT_THROW(
-      std::ignore = verifier_->verify({getPeerCert(selfsigned)}),
-      std::runtime_error);
+  expectThrowWithAlert(
+      [&] { verifier_->verify({getPeerCert(selfsigned)}); },
+      AlertDescription::bad_certificate);
 }
 
 TEST_F(DefaultCertificateVerifierTest, TestVerifySelfSignedCertWithOverride) {
@@ -90,7 +112,9 @@ TEST_F(DefaultCertificateVerifierTest, TestVerifySelfSignedCertWithOverride) {
 TEST_F(DefaultCertificateVerifierTest, TestVerifyWithIntermediateMissing) {
   auto subauth = createCert("subauth", true, &rootCertAndKey_);
   auto subleaf = createCert("subleaf", false, &subauth);
-  EXPECT_THROW(verifier_->verify({getPeerCert(subleaf)}), std::runtime_error);
+  expectThrowWithAlert(
+      [&] { verifier_->verify({getPeerCert(subleaf)}); },
+      AlertDescription::bad_certificate);
 }
 
 TEST_F(
@@ -101,22 +125,53 @@ TEST_F(
   verifier_->setCustomVerifyCallback(
       &DefaultCertificateVerifierTest::allowSelfSignedLeafCertCallback);
   // The override is irrelevant to the error here. So exception is expected.
-  EXPECT_THROW(verifier_->verify({getPeerCert(subleaf)}), std::runtime_error);
+  expectThrowWithAlert(
+      [&] { verifier_->verify({getPeerCert(subleaf)}); },
+      AlertDescription::bad_certificate);
 }
 
 TEST_F(DefaultCertificateVerifierTest, TestVerifyWithBadIntermediate) {
   auto subauth = createCert("badsubauth", false, &rootCertAndKey_);
   auto subleaf = createCert("badsubleaf", false, &subauth);
-  EXPECT_THROW(verifier_->verify({getPeerCert(subleaf)}), std::runtime_error);
+  expectThrowWithAlert(
+      [&] { verifier_->verify({getPeerCert(subleaf)}); },
+      AlertDescription::bad_certificate);
 }
 
 TEST_F(DefaultCertificateVerifierTest, TestVerifyWithBadRoot) {
   auto newroot = createCert("root2", true, nullptr);
   auto subauth = createCert("subauth2", true, &newroot);
   auto subleaf = createCert("leaf2", false, &subauth);
-  EXPECT_THROW(
-      verifier_->verify({getPeerCert(subleaf), getPeerCert(subauth)}),
-      std::runtime_error);
+  expectThrowWithAlert(
+      [&] { verifier_->verify({getPeerCert(subleaf), getPeerCert(subauth)}); },
+      AlertDescription::bad_certificate);
+}
+TEST_F(DefaultCertificateVerifierTest, TestVerifyWithExpiredLeafTooOld) {
+  auto now = std::chrono::system_clock::now();
+  auto newLeaf = createCert({
+      .cn = "expiredLeaf",
+      .issuer = &rootCertAndKey_,
+      .notBefore = now - std::chrono::hours(24),
+      .notAfter = now - std::chrono::hours(23),
+  });
+
+  expectThrowWithAlert(
+      [&] { verifier_->verify({getPeerCert(newLeaf)}); },
+      AlertDescription::certificate_expired);
+}
+
+TEST_F(DefaultCertificateVerifierTest, TestVerifyWithExpiredLeafTooNew) {
+  auto now = std::chrono::system_clock::now();
+  auto newLeaf = createCert({
+      .cn = "expiredLeaf",
+      .issuer = &rootCertAndKey_,
+      .notBefore = now + std::chrono::hours(24),
+      .notAfter = now + std::chrono::hours(25),
+  });
+
+  expectThrowWithAlert(
+      [&] { verifier_->verify({getPeerCert(newLeaf)}); },
+      AlertDescription::certificate_expired);
 }
 } // namespace test
 } // namespace fizz
diff --git a/third-party/fizz/src/fizz/server/ServerProtocol.cpp b/third-party/fizz/src/fizz/server/ServerProtocol.cpp
@@ -2234,6 +2234,10 @@ AsyncActions EventHandler<
     } else {
       newCert = std::move(leafCert);
     }
+  } catch (const FizzVerificationException& e) {
+    throw FizzVerificationException(
+        folly::to<std::string>("client certificate failure: ", e.what()),
+        e.getAlert());
   } catch (const FizzException&) {
     throw;
   } catch (const std::exception& e) {