diff --git a/fizz/protocol/DefaultCertificateVerifier.cpp b/fizz/protocol/DefaultCertificateVerifier.cpp index 95afda9550..bf7ac57b6f 100644 --- a/fizz/protocol/DefaultCertificateVerifier.cpp +++ b/fizz/protocol/DefaultCertificateVerifier.cpp @@ -28,6 +28,18 @@ static AlertDescription toTLSAlert(int opensslVerifyErr) { * "A certificate has expired or is not currently valid." */ return AlertDescription::certificate_expired; + case X509_V_ERR_CERT_CHAIN_TOO_LONG: + case X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT: + case X509_V_ERR_PATH_LENGTH_EXCEEDED: + case X509_V_ERR_INVALID_CA: + case X509_V_ERR_SELF_SIGNED_CERT_IN_CHAIN: + case X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT: + case X509_V_ERR_UNABLE_TO_GET_ISSUER_CERT_LOCALLY: + /** + * All of these conditions indicate that building a path to a trusted + * anchor failed. + */ + return AlertDescription::unknown_ca; default: return AlertDescription::bad_certificate; } diff --git a/fizz/protocol/test/DefaultCertificateVerifierTest.cpp b/fizz/protocol/test/DefaultCertificateVerifierTest.cpp index b60a1468a3..f02cb61ab7 100644 --- a/fizz/protocol/test/DefaultCertificateVerifierTest.cpp +++ b/fizz/protocol/test/DefaultCertificateVerifierTest.cpp @@ -93,7 +93,7 @@ TEST_F(DefaultCertificateVerifierTest, TestVerifySelfSignedCert) { auto selfsigned = createCert("self", false, nullptr); expectThrowWithAlert( [&] { verifier_->verify({getPeerCert(selfsigned)}); }, - AlertDescription::bad_certificate); + AlertDescription::unknown_ca); } TEST_F(DefaultCertificateVerifierTest, TestVerifySelfSignedCertWithOverride) { @@ -114,7 +114,7 @@ TEST_F(DefaultCertificateVerifierTest, TestVerifyWithIntermediateMissing) { auto subleaf = createCert("subleaf", false, &subauth); expectThrowWithAlert( [&] { verifier_->verify({getPeerCert(subleaf)}); }, - AlertDescription::bad_certificate); + AlertDescription::unknown_ca); } TEST_F( @@ -124,10 +124,13 @@ TEST_F( auto subleaf = createCert("subleaf", false, &subauth); verifier_->setCustomVerifyCallback( &DefaultCertificateVerifierTest::allowSelfSignedLeafCertCallback); - // The override is irrelevant to the error here. So exception is expected. + // The override asserts that self signed certs (chain length = 1) are + // accepted. But since this certificate is not self signed, this override + // should effectively be a no-op and normal certificate verification + // should take place. expectThrowWithAlert( [&] { verifier_->verify({getPeerCert(subleaf)}); }, - AlertDescription::bad_certificate); + AlertDescription::unknown_ca); } TEST_F(DefaultCertificateVerifierTest, TestVerifyWithBadIntermediate) { @@ -135,7 +138,7 @@ TEST_F(DefaultCertificateVerifierTest, TestVerifyWithBadIntermediate) { auto subleaf = createCert("badsubleaf", false, &subauth); expectThrowWithAlert( [&] { verifier_->verify({getPeerCert(subleaf)}); }, - AlertDescription::bad_certificate); + AlertDescription::unknown_ca); } TEST_F(DefaultCertificateVerifierTest, TestVerifyWithBadRoot) { @@ -144,7 +147,7 @@ TEST_F(DefaultCertificateVerifierTest, TestVerifyWithBadRoot) { auto subleaf = createCert("leaf2", false, &subauth); expectThrowWithAlert( [&] { verifier_->verify({getPeerCert(subleaf), getPeerCert(subauth)}); }, - AlertDescription::bad_certificate); + AlertDescription::unknown_ca); } TEST_F(DefaultCertificateVerifierTest, TestVerifyWithExpiredLeafTooOld) { auto now = std::chrono::system_clock::now(); diff --git a/fizz/test/HandshakeTest.cpp b/fizz/test/HandshakeTest.cpp index 50fe0d96f8..06ad3229f2 100644 --- a/fizz/test/HandshakeTest.cpp +++ b/fizz/test/HandshakeTest.cpp @@ -416,6 +416,10 @@ TEST_F(HandshakeTest, CertRequestPermitNoCert) { } TEST_F(HandshakeTest, CertRequestBadCert) { + /** + * A server requires client authentication, and the client presents a + * self signed certificate to the server. + */ serverContext_->setClientAuthMode(ClientAuthMode::Required); auto badCert = createCert("foo", false, nullptr); std::vector certVec; @@ -425,7 +429,7 @@ TEST_F(HandshakeTest, CertRequestBadCert) { std::make_shared>( std::move(badCert.key), std::move(certVec))); clientContext_->setClientCertManager(std::move(certMgr)); - expectServerError("alert: bad_certificate", "client certificate failure"); + expectServerError("alert: unknown_ca", "client certificate failure"); doHandshake(); }