Move validity checks from factory to peer cert verification

Summary: As title says these checks belong in peer cert verification instead of the factory where they were before. Reviewed By: sotodel, knekritz Differential Revision: D55970433 fbshipit-source-id: 945a4628d0242a237e364be841a6bc807b5ad3cb
facebookincubator · Apr 16, 2024 · b8fc0ef · b8fc0ef
1 parent dde25ea
commit b8fc0ef
Show file tree

Hide file tree

Showing 9 changed files with 338 additions and 214 deletions.
diff --git a/fizz/extensions/delegatedcred/DelegatedCredentialFactory.cpp b/fizz/extensions/delegatedcred/DelegatedCredentialFactory.cpp
@@ -53,11 +53,12 @@ std::shared_ptr<PeerCert> makeCredential(
 }
 } // namespace
 
-static const auto kMaxDelegatedCredentialLifetime = std::chrono::hours(24 * 7);
-
-std::shared_ptr<PeerCert> DelegatedCredentialFactory::makePeerCert(
+std::shared_ptr<PeerCert> DelegatedCredentialFactory::makePeerCertStatic(
     CertificateEntry entry,
-    const std::shared_ptr<Clock>& clock) {
+    bool leaf) {
+  if (!leaf || entry.extensions.empty()) {
+    return CertUtils::makePeerCert(std::move(entry.cert_data));
+  }
   auto parentCert = CertUtils::makePeerCert(entry.cert_data->clone());
   auto parentX509 = parentCert->getX509();
   auto credential = getExtension<DelegatedCredential>(entry.extensions);
@@ -67,44 +68,14 @@ std::shared_ptr<PeerCert> DelegatedCredentialFactory::makePeerCert(
     return std::move(parentCert);
   }
 
-  // Check validity period first.
-  auto notBefore = X509_get0_notBefore(parentX509.get());
-  auto notBeforeTime =
-      folly::ssl::OpenSSLCertUtils::asnTimeToTimepoint(notBefore);
-  auto credentialExpiresTime =
-      notBeforeTime + std::chrono::seconds(credential->valid_time);
-  auto now = clock->getCurrentTime();
-  if (now >= credentialExpiresTime) {
-    throw FizzException(
-        "credential is no longer valid", AlertDescription::illegal_parameter);
-  }
-
-  // Credentials may be valid for max 1 week according to spec
-  if (credentialExpiresTime - now > kMaxDelegatedCredentialLifetime) {
-    throw FizzException(
-        "credential validity is longer than a week from now",
-        AlertDescription::illegal_parameter);
-  }
-
-  // Check extensions on cert
-  DelegatedCredentialUtils::checkExtensions(parentX509);
-
   // Create credential
   return makeCredential(std::move(credential.value()), std::move(parentX509));
 }
 
 std::shared_ptr<PeerCert> DelegatedCredentialFactory::makePeerCert(
     CertificateEntry entry,
     bool leaf) const {
-  if (!leaf || entry.extensions.empty()) {
-    return CertUtils::makePeerCert(std::move(entry.cert_data));
-  }
-  return makePeerCert(std::move(entry), clock_);
-}
-
-void DelegatedCredentialFactory::setClock(std::shared_ptr<Clock> clock) {
-  clock_ = clock;
+  return makePeerCertStatic(std::move(entry), leaf);
 }
-
 } // namespace extensions
 } // namespace fizz
diff --git a/fizz/extensions/delegatedcred/DelegatedCredentialFactory.h b/fizz/extensions/delegatedcred/DelegatedCredentialFactory.h
@@ -10,7 +10,6 @@
 
 #include <fizz/extensions/delegatedcred/Types.h>
 #include <fizz/protocol/OpenSSLFactory.h>
-#include <fizz/protocol/clock/SystemClock.h>
 
 namespace fizz {
 namespace extensions {
@@ -25,14 +24,9 @@ class DelegatedCredentialFactory : public OpenSSLFactory {
   std::shared_ptr<PeerCert> makePeerCert(CertificateEntry entry, bool leaf)
       const override;
 
-  void setClock(std::shared_ptr<Clock> clock);
-
-  static std::shared_ptr<PeerCert> makePeerCert(
+  static std::shared_ptr<PeerCert> makePeerCertStatic(
       CertificateEntry entry,
-      const std::shared_ptr<Clock>& clock);
-
- private:
-  std::shared_ptr<Clock> clock_ = std::make_shared<SystemClock>();
+      bool leaf);
 };
 } // namespace extensions
 } // namespace fizz
diff --git a/fizz/extensions/delegatedcred/DelegatedCredentialUtils.cpp b/fizz/extensions/delegatedcred/DelegatedCredentialUtils.cpp
@@ -36,6 +36,7 @@ void DelegatedCredentialUtils::checkExtensions(
 
 namespace {
 static constexpr folly::StringPiece kDelegatedOid{"1.3.6.1.4.1.44363.44"};
+static const auto kMaxDelegatedCredentialLifetime = std::chrono::hours(24 * 7);
 
 folly::ssl::ASN1ObjUniquePtr generateCredentialOid() {
   folly::ssl::ASN1ObjUniquePtr oid;
@@ -157,5 +158,28 @@ DelegatedCredential DelegatedCredentialUtils::generateCredential(
 
   return cred;
 }
+
+void DelegatedCredentialUtils::checkCredentialTimeValidity(
+    const folly::ssl::X509UniquePtr& parentCert,
+    const DelegatedCredential& credential,
+    const std::shared_ptr<Clock>& clock) {
+  auto notBefore = X509_get0_notBefore(parentCert.get());
+  auto notBeforeTime =
+      folly::ssl::OpenSSLCertUtils::asnTimeToTimepoint(notBefore);
+  auto credentialExpiresTime =
+      notBeforeTime + std::chrono::seconds(credential.valid_time);
+  auto now = clock->getCurrentTime();
+  if (now >= credentialExpiresTime) {
+    throw FizzException(
+        "credential is no longer valid", AlertDescription::illegal_parameter);
+  }
+
+  // Credentials may be valid for max 1 week according to spec
+  if (credentialExpiresTime - now > kMaxDelegatedCredentialLifetime) {
+    throw FizzException(
+        "credential validity is longer than a week from now",
+        AlertDescription::illegal_parameter);
+  }
+}
 } // namespace extensions
 } // namespace fizz
diff --git a/fizz/extensions/delegatedcred/DelegatedCredentialUtils.h b/fizz/extensions/delegatedcred/DelegatedCredentialUtils.h
@@ -10,6 +10,7 @@
 
 #include <fizz/extensions/delegatedcred/Types.h>
 #include <fizz/protocol/Certificate.h>
+#include <fizz/protocol/clock/Clock.h>
 #include <folly/ssl/OpenSSLPtrTypes.h>
 
 namespace fizz {
@@ -29,6 +30,11 @@ class DelegatedCredentialUtils {
    */
   static bool hasDelegatedExtension(const folly::ssl::X509UniquePtr& cert);
 
+  static void checkCredentialTimeValidity(
+      const folly::ssl::X509UniquePtr& parentCert,
+      const DelegatedCredential& credential,
+      const std::shared_ptr<Clock>& clock);
+
   /**
    * Constructs the buffer used for verifying the signature on the
    * delegated credential.

diff --git a/fizz/extensions/delegatedcred/PeerDelegatedCredential-inl.h b/fizz/extensions/delegatedcred/PeerDelegatedCredential-inl.h
@@ -6,10 +6,12 @@
  *  LICENSE file in the root directory of this source tree.
  */
 #include <fizz/extensions/delegatedcred/DelegatedCredentialUtils.h>
+#include <fizz/protocol/clock/SystemClock.h>
 #include <folly/ssl/OpenSSLCertUtils.h>
 
 namespace fizz {
 namespace extensions {
+
 template <KeyType T>
 PeerDelegatedCredential<T>::PeerDelegatedCredential(
     folly::ssl::X509UniquePtr cert,
@@ -32,14 +34,16 @@ void PeerDelegatedCredential<T>::verify(
         "certificate verify didn't use credential's algorithm",
         AlertDescription::illegal_parameter);
   }
+  auto x509 = OpenSSLPeerCertImpl<T>::getX509();
+  // Check extensions on cert
+  DelegatedCredentialUtils::checkExtensions(x509);
+  DelegatedCredentialUtils::checkCredentialTimeValidity(
+      x509, credential_, clock_);
 
   // Verify signature
-  auto parentCert = std::make_unique<OpenSSLPeerCertImpl<T>>(
-      OpenSSLPeerCertImpl<T>::getX509());
   auto credSignBuf = DelegatedCredentialUtils::prepareSignatureBuffer(
-      credential_,
-      folly::ssl::OpenSSLCertUtils::derEncode(
-          *OpenSSLPeerCertImpl<T>::getX509()));
+      credential_, folly::ssl::OpenSSLCertUtils::derEncode(*x509));
+  auto parentCert = std::make_unique<OpenSSLPeerCertImpl<T>>(std::move(x509));
 
   try {
     parentCert->verify(
@@ -63,6 +67,5 @@ template <KeyType T>
 SignatureScheme PeerDelegatedCredential<T>::getExpectedScheme() const {
   return credential_.expected_verify_scheme;
 }
-
 } // namespace extensions
 } // namespace fizz
diff --git a/fizz/extensions/delegatedcred/PeerDelegatedCredential.h b/fizz/extensions/delegatedcred/PeerDelegatedCredential.h
@@ -10,6 +10,7 @@
 
 #include <fizz/extensions/delegatedcred/Types.h>
 #include <fizz/protocol/OpenSSLPeerCertImpl.h>
+#include <fizz/protocol/clock/SystemClock.h>
 
 namespace fizz {
 namespace extensions {
@@ -32,8 +33,14 @@ class PeerDelegatedCredential : public OpenSSLPeerCertImpl<T> {
 
   SignatureScheme getExpectedScheme() const;
 
+  /* for testing only */
+  void setClock(std::shared_ptr<Clock> clock) {
+    clock_ = clock;
+  }
+
  private:
   DelegatedCredential credential_;
+  std::shared_ptr<Clock> clock_ = std::make_shared<SystemClock>();
 };
 } // namespace extensions
 } // namespace fizz

diff --git a/fizz/extensions/delegatedcred/test/DelegatedCredentialFactoryTest.cpp b/fizz/extensions/delegatedcred/test/DelegatedCredentialFactoryTest.cpp
@@ -14,7 +14,6 @@
 #include <fizz/extensions/delegatedcred/DelegatedCredentialFactory.h>
 #include <fizz/extensions/delegatedcred/PeerDelegatedCredential.h>
 #include <fizz/protocol/OpenSSLPeerCertImpl.h>
-#include <fizz/protocol/clock/test/Mocks.h>
 
 using namespace folly;
 
@@ -49,62 +48,10 @@ FghrPnYCODq235mY2A==
 -----END CERTIFICATE-----
 )";
 
-StringPiece kCertNoDelegatedExtension = R"(
------BEGIN CERTIFICATE-----
-MIIB2DCCAX6gAwIBAgIJAIEdGlbc/R/BMAoGCCqGSM49BAMCMEIxCzAJBgNVBAYT
-AlhYMRUwEwYDVQQHDAxEZWZhdWx0IENpdHkxHDAaBgNVBAoME0RlZmF1bHQgQ29t
-cGFueSBMdGQwHhcNMTkwNjEwMjExNjEwWhcNMjAwNjA5MjExNjEwWjBCMQswCQYD
-VQQGEwJYWDEVMBMGA1UEBwwMRGVmYXVsdCBDaXR5MRwwGgYDVQQKDBNEZWZhdWx0
-IENvbXBhbnkgTHRkMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAE7cnufRVxTjuK
-xcBV4IQ35xkjIIICtm05sLlsriVbIcMj55kAVZHkEHnezgEIq6io8BRy+4m8gXYs
-X52uCjXmMKNdMFswHQYDVR0OBBYEFBXp/0ABAVMfEpQFDi0K8MK5HEUsMB8GA1Ud
-IwQYMBaAFBXp/0ABAVMfEpQFDi0K8MK5HEUsMAwGA1UdEwQFMAMBAf8wCwYDVR0P
-BAQDAgHmMAoGCCqGSM49BAMCA0gAMEUCIQDV1EqSNhLD+v6XhSGmoCxw6mnuHv2p
-wLj4M2gxgs6VmQIgBLB8W3th4WlHE7+C6YPeX6n834ceZ5dBi6FQLYKgpXY=
------END CERTIFICATE-----
-)";
-
-StringPiece kCertNoKeyUsage = R"(
------BEGIN CERTIFICATE-----
-MIIB3DCCAYKgAwIBAgIJAIueLstLYzuHMAoGCCqGSM49BAMCMEIxCzAJBgNVBAYT
-AlhYMRUwEwYDVQQHDAxEZWZhdWx0IENpdHkxHDAaBgNVBAoME0RlZmF1bHQgQ29t
-cGFueSBMdGQwHhcNMTkwNjEwMjExNjM1WhcNMjAwNjA5MjExNjM1WjBCMQswCQYD
-VQQGEwJYWDEVMBMGA1UEBwwMRGVmYXVsdCBDaXR5MRwwGgYDVQQKDBNEZWZhdWx0
-IENvbXBhbnkgTHRkMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEOiX1bVPJgpse
-oV8UxhOZ5kBynFLu0Iazp8axzziiILdW3j9dG7z2sCBaPiJntn7whfEOwD41mcJq
-UNssOm18IKNhMF8wHQYDVR0OBBYEFHdcgbW5+8piYNQdNy2dnXGuPvBQMB8GA1Ud
-IwQYMBaAFHdcgbW5+8piYNQdNy2dnXGuPvBQMAwGA1UdEwQFMAMBAf8wDwYJKwYB
-BAGC2kssBAIFADAKBggqhkjOPQQDAgNIADBFAiEAl01tUrb6x6E/SsRuPOKteKHZ
-qf+khcoJYtl3FQiBNrECIHP6Qxr3ZXysyHi0uGfkGqPVVLN9Knl7DXVo6CYVQKKl
------END CERTIFICATE-----
-)";
-
-StringPiece kCertWrongKeyUsage = R"(
------BEGIN CERTIFICATE-----
-MIIB6TCCAY+gAwIBAgIJAMfSoojFcEPAMAoGCCqGSM49BAMCMEIxCzAJBgNVBAYT
-AlhYMRUwEwYDVQQHDAxEZWZhdWx0IENpdHkxHDAaBgNVBAoME0RlZmF1bHQgQ29t
-cGFueSBMdGQwHhcNMTkwNjEwMjEzMDQwWhcNMjAwNjA5MjEzMDQwWjBCMQswCQYD
-VQQGEwJYWDEVMBMGA1UEBwwMRGVmYXVsdCBDaXR5MRwwGgYDVQQKDBNEZWZhdWx0
-IENvbXBhbnkgTHRkMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEgyT1AYOI+lLL
-SGKWkPAIXOkQtYXMdhz1+YmvlD4O/tUpedv9UzKb+6YQ/pZj0Kg+IzilwIJSL+aT
-SAG8fLAzS6NuMGwwHQYDVR0OBBYEFHB8ZwSEBZitRX6GbOxW0+ProEX4MB8GA1Ud
-IwQYMBaAFHB8ZwSEBZitRX6GbOxW0+ProEX4MAwGA1UdEwQFMAMBAf8wCwYDVR0P
-BAQDAgFmMA8GCSsGAQQBgtpLLAQCBQAwCgYIKoZIzj0EAwIDSAAwRQIgUMEK+LqG
-x+n8WLnzTOj0bIQOfyerUEIRnEiDzwZtM2ACIQDyDF4Gibr14YHUs8ZbP3l37A9c
-wNMYE5qiSvpfGd77lw==
------END CERTIFICATE-----
-)";
-
 class DelegatedCredentialFactoryTest : public Test {
  public:
   void SetUp() override {
     CryptoUtils::init();
-    clock_ = std::make_shared<MockClock>();
-    factory_.setClock(clock_);
-    // Default return time to June 10, 2019 6:08:58 PM
-    ON_CALL(*clock_, getCurrentTime())
-        .WillByDefault(Return(std::chrono::system_clock::time_point(
-            std::chrono::seconds(1560190138))));
   }
 
   CertificateEntry generateEntry() const {
@@ -131,7 +78,6 @@ class DelegatedCredentialFactoryTest : public Test {
   }
 
   DelegatedCredentialFactory factory_;
-  std::shared_ptr<MockClock> clock_;
 };
 
 TEST_F(DelegatedCredentialFactoryTest, TestCredentialParsing) {
@@ -148,81 +94,13 @@ TEST_F(DelegatedCredentialFactoryTest, TestCredentialParsingNonLeaf) {
   EXPECT_TRUE(typeid(*cert) == typeid(OpenSSLPeerCertImpl<KeyType::P256>));
 }
 
-TEST_F(DelegatedCredentialFactoryTest, TestCredentialNoX509Extension) {
-  // "Wrong" certs have different time, adjust to compensate.
-  // Date: 06/27/2019 @ 7:32pm (UTC)
-  EXPECT_CALL(*clock_, getCurrentTime())
-      .WillOnce(Return(std::chrono::system_clock::time_point(
-          std::chrono::seconds(1561663956))));
-  auto entry = generateEntry();
-  entry.cert_data = getCertData(kCertNoDelegatedExtension);
-  // Note: factory doesn't do signature checks, so we expect this to fail due to
-  // the x509 extension missing.
-  expectThrows(
-      [&]() { factory_.makePeerCert(std::move(entry), true); },
-      "cert is missing DelegationUsage extension");
-}
-
-TEST_F(DelegatedCredentialFactoryTest, TestCredentialNoKeyUsage) {
-  // "Wrong" certs have different time, adjust to compensate.
-  // Date: 06/27/2019 @ 7:32pm (UTC)
-  EXPECT_CALL(*clock_, getCurrentTime())
-      .WillOnce(Return(std::chrono::system_clock::time_point(
-          std::chrono::seconds(1561663956))));
-  auto entry = generateEntry();
-  entry.cert_data = getCertData(kCertNoKeyUsage);
-  // Expect the DelegationUsage check to pass, but missing keyusage
-  expectThrows(
-      [&]() { factory_.makePeerCert(std::move(entry), true); },
-      "cert is missing KeyUsage extension");
-}
-
-TEST_F(DelegatedCredentialFactoryTest, TestCredentialWrongKeyUsage) {
-  // "Wrong" certs have different time, adjust to compensate.
-  // Date: 06/27/2019 @ 7:32pm (UTC)
-  EXPECT_CALL(*clock_, getCurrentTime())
-      .WillOnce(Return(std::chrono::system_clock::time_point(
-          std::chrono::seconds(1561663956))));
-  auto entry = generateEntry();
-  entry.cert_data = getCertData(kCertWrongKeyUsage);
-  // This one has keyusage extension, but it lacks the digitalsignature usage
-  expectThrows(
-      [&]() { factory_.makePeerCert(std::move(entry), true); },
-      "cert lacks digital signature key usage");
-}
-
 TEST_F(DelegatedCredentialFactoryTest, TestCredentialNoCertEntryExtension) {
   auto entry = generateEntry();
   entry.extensions.clear();
   auto cert = factory_.makePeerCert(std::move(entry), true);
   EXPECT_TRUE(cert);
   EXPECT_TRUE(typeid(*cert) == typeid(OpenSSLPeerCertImpl<KeyType::P256>));
 }
-
-TEST_F(DelegatedCredentialFactoryTest, TestCertExpiredCredential) {
-  auto entry = generateEntry();
-  auto credential = getExtension<DelegatedCredential>(entry.extensions);
-  // Make it expire immediately after cert is valid
-  credential->valid_time = 0;
-  entry.extensions.clear();
-  entry.extensions.push_back(encodeExtension(*credential));
-  expectThrows(
-      [&]() { factory_.makePeerCert(std::move(entry), true); },
-      "credential is no longer valid");
-}
-
-TEST_F(DelegatedCredentialFactoryTest, TestCredentialValidForTooLong) {
-  auto entry = generateEntry();
-  auto credential = getExtension<DelegatedCredential>(entry.extensions);
-  // Add an extra week to the valid time to make it longer than the max
-  credential->valid_time += 604800;
-  entry.extensions.clear();
-  entry.extensions.push_back(encodeExtension(*credential));
-  expectThrows(
-      [&]() { factory_.makePeerCert(std::move(entry), true); },
-      "credential validity is longer than a week from now");
-}
-
 } // namespace test
 } // namespace extensions
 } // namespace fizz