From f4e6d36c5667d4a48b7786add755e456ecfc2ead Mon Sep 17 00:00:00 2001 From: Mingtao Yang Date: Wed, 29 Jan 2025 10:26:43 -0800 Subject: [PATCH] Translate several OpenSSL verify error codes into corresponding TLS alerts Summary: Specifically, certificate_expired alerts for expired certificates. Reviewed By: frqiu Differential Revision: D57076898 fbshipit-source-id: bf8a1638dba5ab86c93628aca4ed41bc44e60400 --- .../protocol/DefaultCertificateVerifier.cpp | 23 +++++- .../test/DefaultCertificateVerifierTest.cpp | 73 ++++++++++++++++--- .../fizz/src/fizz/server/ServerProtocol.cpp | 4 + 3 files changed, 88 insertions(+), 12 deletions(-) diff --git a/third-party/fizz/src/fizz/protocol/DefaultCertificateVerifier.cpp b/third-party/fizz/src/fizz/protocol/DefaultCertificateVerifier.cpp index c8e9d96651acd..95afda9550a67 100644 --- 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::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 index 5cfa201fff92d..b60a1468a36df 100644 --- 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 verifier_; }; +template +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 +static void expectThrowWithAlert(F f, AlertDescription ad) { + return expectThrowSuchThat(std::move(f), [ad](const auto& ex) { + auto fe = dynamic_cast(&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 index ccf121724269d..9db31544d3eb0 100644 --- 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("client certificate failure: ", e.what()), + e.getAlert()); } catch (const FizzException&) { throw; } catch (const std::exception& e) {