bug 1593141 - add validity period beginning argument to mozilla::pkix::TrustDomain::CheckRevocation r=jcj
authorDana Keeler <dkeeler@mozilla.com>
Sat, 09 Nov 2019 00:36:23 +0000
changeset 15396 e8f2720c8254a5327260dec70153a3c3b7c745b9
parent 15387 87f35ba4c82f17204c6714f10559a37896f0314f
child 15397 1e22a0c93afe9f46545560c86caedef9dab6cfda
push id3583
push userjjones@mozilla.com
push dateThu, 14 Nov 2019 22:23:57 +0000
reviewersjcj
bugs1593141
bug 1593141 - add validity period beginning argument to mozilla::pkix::TrustDomain::CheckRevocation r=jcj This allows TrustDomain implementations to make decisions based on when the validity period of a certificate began. For instance, if an implementation has revocation information that is valid and complete as of a particular time, but a certificate's validity period begins after that time, the implementation may decide to disregard this revocation information on the basis that the information it has available cannot possibly apply to that certificate. Differential Revision: https://phabricator.services.mozilla.com/D51485
gtests/mozpkix_gtest/pkixbuild_tests.cpp
gtests/mozpkix_gtest/pkixcert_extension_tests.cpp
gtests/mozpkix_gtest/pkixcert_signature_algorithm_tests.cpp
gtests/mozpkix_gtest/pkixcheck_CheckExtendedKeyUsage_tests.cpp
gtests/mozpkix_gtest/pkixcheck_CheckSignatureAlgorithm_tests.cpp
gtests/mozpkix_gtest/pkixgtest.h
lib/mozpkix/include/pkix/pkixtypes.h
lib/mozpkix/lib/pkixbuild.cpp
--- a/gtests/mozpkix_gtest/pkixbuild_tests.cpp
+++ b/gtests/mozpkix_gtest/pkixbuild_tests.cpp
@@ -147,20 +147,24 @@ private:
     rv = checker.Check(derCert, nullptr/*additionalNameConstraints*/,
                        keepGoing);
     if (rv != Success) {
       return rv;
     }
     return Success;
   }
 
-  Result CheckRevocation(EndEntityOrCA, const CertID&, Time, Duration,
+  Result CheckRevocation(EndEntityOrCA, const CertID&, Time,
+                         Time validityBeginning, Duration,
                          /*optional*/ const Input*, /*optional*/ const Input*)
                          override
   {
+    // All of the certificates in this test for which this is called have a
+    // validity period that begins "one day before now".
+    EXPECT_EQ(TimeFromEpochInSeconds(oneDayBeforeNow), validityBeginning);
     return Success;
   }
 
   Result IsChainValid(const DERArray&, Time, const CertPolicyId&) override
   {
     return Success;
   }
 
@@ -296,37 +300,41 @@ public:
     return checker.Check(rootCert, nullptr, keepGoing);
   }
 
   Result IsChainValid(const DERArray&, Time, const CertPolicyId&) override
   {
     return Success;
   }
 
-  Result CheckRevocation(EndEntityOrCA, const CertID&, Time, Duration,
+  Result CheckRevocation(EndEntityOrCA, const CertID&, Time,
+                         Time validityBeginning, Duration,
                          /*optional*/ const Input*, /*optional*/ const Input*)
                          override
   {
+    // All of the certificates in this test for which this is called have a
+    // validity period that begins "one day before now".
+    EXPECT_EQ(TimeFromEpochInSeconds(oneDayBeforeNow), validityBeginning);
     return Success;
   }
 
 private:
   ByteString rootDER;
 };
 
 // A TrustDomain that explicitly fails if CheckRevocation is called.
 class ExpiredCertTrustDomain final : public SingleRootTrustDomain
 {
 public:
   explicit ExpiredCertTrustDomain(ByteString aRootDER)
     : SingleRootTrustDomain(aRootDER)
   {
   }
 
-  Result CheckRevocation(EndEntityOrCA, const CertID&, Time, Duration,
+  Result CheckRevocation(EndEntityOrCA, const CertID&, Time, Time, Duration,
                          /*optional*/ const Input*, /*optional*/ const Input*)
                          override
   {
     ADD_FAILURE();
     return NotReached("CheckRevocation should not be called",
                       Result::FATAL_ERROR_LIBRARY_FAILURE);
   }
 };
@@ -437,20 +445,24 @@ public:
     bool keepGoing;
     EXPECT_EQ(Success,
               checker.Check(issuerInput, nullptr /*additionalNameConstraints*/,
                             keepGoing));
     EXPECT_EQ(expectedKeepGoing, keepGoing);
     return Success;
   }
 
-  Result CheckRevocation(EndEntityOrCA, const CertID&, Time, Duration,
+  Result CheckRevocation(EndEntityOrCA, const CertID&, Time,
+                         Time validityBeginning, Duration,
                          /*optional*/ const Input*, /*optional*/ const Input*)
                          override
   {
+    // All of the certificates in this test for which this is called have a
+    // validity period that begins "one day before now".
+    EXPECT_EQ(TimeFromEpochInSeconds(oneDayBeforeNow), validityBeginning);
     return Success;
   }
 
   Result IsChainValid(const DERArray&, Time, const CertPolicyId&) override
   {
     return Success;
   }
 
@@ -660,20 +672,24 @@ private:
                      keepGoing);
       if (rv != Success) {
         return rv;
       }
     }
     return Success;
   }
 
-  Result CheckRevocation(EndEntityOrCA, const CertID&, Time, Duration,
+  Result CheckRevocation(EndEntityOrCA, const CertID&, Time,
+                         Time validityBeginning, Duration,
                          /*optional*/ const Input*,
                          /*optional*/ const Input*) override
   {
+    // All of the certificates in this test for which this is called have a
+    // validity period that begins "one day before now".
+    EXPECT_EQ(TimeFromEpochInSeconds(oneDayBeforeNow), validityBeginning);
     return Success;
   }
 
   Result IsChainValid(const DERArray&, Time, const CertPolicyId&) override
   {
     return Success;
   }
 
@@ -718,17 +734,17 @@ TEST_F(pkixbuild, BadEmbeddedSCTWithMult
                            nullptr/*stapledOCSPResponse*/));
 }
 
 // Same as a MultiplePathTrustDomain, but the end-entity is revoked.
 class RevokedEndEntityTrustDomain final : public MultiplePathTrustDomain
 {
 public:
   Result CheckRevocation(EndEntityOrCA endEntityOrCA, const CertID&, Time,
-                         Duration, /*optional*/ const Input*,
+                         Time, Duration, /*optional*/ const Input*,
                          /*optional*/ const Input*) override
   {
     if (endEntityOrCA == EndEntityOrCA::MustBeEndEntity) {
       return Result::ERROR_REVOKED_CERTIFICATE;
     }
     return Success;
   }
 };
@@ -823,20 +839,24 @@ private:
       rv = checker.Check(certInput, nullptr, keepGoing);
       if (rv != Success || !keepGoing) {
         return rv;
       }
     }
     return Success;
   }
 
-  Result CheckRevocation(EndEntityOrCA, const CertID&, Time, Duration,
+  Result CheckRevocation(EndEntityOrCA, const CertID&, Time,
+                         Time validityBeginning, Duration,
                          /*optional*/ const Input*, /*optional*/ const Input*)
                          override
   {
+    // All of the certificates in this test for which this is called have a
+    // validity period that begins "one day before now".
+    EXPECT_EQ(TimeFromEpochInSeconds(oneDayBeforeNow), validityBeginning);
     return Success;
   }
 
   Result IsChainValid(const DERArray&, Time, const CertPolicyId&) override
   {
     return Success;
   }
 
--- a/gtests/mozpkix_gtest/pkixcert_extension_tests.cpp
+++ b/gtests/mozpkix_gtest/pkixcert_extension_tests.cpp
@@ -65,17 +65,17 @@ class TrustEverythingTrustDomain final :
 private:
   Result GetCertTrust(EndEntityOrCA, const CertPolicyId&, Input,
                       /*out*/ TrustLevel& trustLevel) override
   {
     trustLevel = TrustLevel::TrustAnchor;
     return Success;
   }
 
-  Result CheckRevocation(EndEntityOrCA, const CertID&, Time, Duration,
+  Result CheckRevocation(EndEntityOrCA, const CertID&, Time, Time, Duration,
                          /*optional*/ const Input*, /*optional*/ const Input*)
                          override
   {
     return Success;
   }
 
   Result IsChainValid(const DERArray&, Time, const CertPolicyId&) override
   {
--- a/gtests/mozpkix_gtest/pkixcert_signature_algorithm_tests.cpp
+++ b/gtests/mozpkix_gtest/pkixcert_signature_algorithm_tests.cpp
@@ -87,17 +87,17 @@ private:
     Result rv = issuerCert.Init(issuerDER->data(), issuerDER->length());
     if (rv != Success) {
       return rv;
     }
     bool keepGoing;
     return checker.Check(issuerCert, nullptr, keepGoing);
   }
 
-  Result CheckRevocation(EndEntityOrCA, const CertID&, Time, Duration,
+  Result CheckRevocation(EndEntityOrCA, const CertID&, Time, Time, Duration,
                          const Input*, const Input*) override
   {
     return Success;
   }
 
   Result IsChainValid(const DERArray&, Time, const CertPolicyId&) override
   {
     return Success;
--- a/gtests/mozpkix_gtest/pkixcheck_CheckExtendedKeyUsage_tests.cpp
+++ b/gtests/mozpkix_gtest/pkixcheck_CheckExtendedKeyUsage_tests.cpp
@@ -553,17 +553,17 @@ private:
     Result rv = derCert.Init(mIssuerCertDER.data(), mIssuerCertDER.length());
     if (rv != Success) {
       return rv;
     }
     bool keepGoing;
     return checker.Check(derCert, nullptr, keepGoing);
   }
 
-  Result CheckRevocation(EndEntityOrCA, const CertID&, Time, Duration,
+  Result CheckRevocation(EndEntityOrCA, const CertID&, Time, Time, Duration,
                          const Input*, const Input*) override
   {
     return Success;
   }
 
   Result IsChainValid(const DERArray&, Time, const CertPolicyId&) override
   {
     return Success;
--- a/gtests/mozpkix_gtest/pkixcheck_CheckSignatureAlgorithm_tests.cpp
+++ b/gtests/mozpkix_gtest/pkixcheck_CheckSignatureAlgorithm_tests.cpp
@@ -297,17 +297,17 @@ public:
 
     bool keepGoing;
     EXPECT_EQ(Success, checker.Check(issuerInput, nullptr, keepGoing));
     EXPECT_FALSE(keepGoing);
 
     return Success;
   }
 
-  Result CheckRevocation(EndEntityOrCA, const CertID&, Time, Duration,
+  Result CheckRevocation(EndEntityOrCA, const CertID&, Time, Time, Duration,
                          /*optional*/ const Input*,
                          /*optional*/ const Input*) override
   {
     return Success;
   }
 
   Result IsChainValid(const DERArray&, Time, const CertPolicyId&) override
   {
--- a/gtests/mozpkix_gtest/pkixgtest.h
+++ b/gtests/mozpkix_gtest/pkixgtest.h
@@ -95,17 +95,17 @@ class EverythingFailsByDefaultTrustDomai
   }
 
   Result FindIssuer(Input, IssuerChecker&, Time) override {
     ADD_FAILURE();
     return NotReached("FindIssuer should not be called",
                       Result::FATAL_ERROR_LIBRARY_FAILURE);
   }
 
-  Result CheckRevocation(EndEntityOrCA, const CertID&, Time, Duration,
+  Result CheckRevocation(EndEntityOrCA, const CertID&, Time, Time, Duration,
                          /*optional*/ const Input*,
                          /*optional*/ const Input*) override {
     ADD_FAILURE();
     return NotReached("CheckRevocation should not be called",
                       Result::FATAL_ERROR_LIBRARY_FAILURE);
   }
 
   Result IsChainValid(const DERArray&, Time, const CertPolicyId&) override {
--- a/lib/mozpkix/include/pkix/pkixtypes.h
+++ b/lib/mozpkix/include/pkix/pkixtypes.h
@@ -273,16 +273,17 @@ class TrustDomain {
   // wrong to assume that the certificate chain is valid.
   //
   // certChain.GetDER(0) is the trust anchor.
   virtual Result IsChainValid(const DERArray& certChain, Time time,
                               const CertPolicyId& requiredPolicy) = 0;
 
   virtual Result CheckRevocation(EndEntityOrCA endEntityOrCA,
                                  const CertID& certID, Time time,
+                                 Time validityBeginning,
                                  Duration validityDuration,
                                  /*optional*/ const Input* stapledOCSPresponse,
                                  /*optional*/ const Input* aiaExtension) = 0;
 
   // Check that the given digest algorithm is acceptable for use in signatures.
   //
   // Return Success if the algorithm is acceptable,
   // Result::ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED if the algorithm is not
--- a/lib/mozpkix/lib/pkixbuild.cpp
+++ b/lib/mozpkix/lib/pkixbuild.cpp
@@ -247,17 +247,18 @@ PathBuildingStep::Check(Input potentialI
     // This should never fail. If we're here, we've already parsed the validity
     // and checked that the given time is in the certificate's validity period.
     rv = ParseValidity(subject.GetValidity(), &notBefore, &notAfter);
     if (rv != Success) {
       return rv;
     }
     Duration validityDuration(notAfter, notBefore);
     rv = trustDomain.CheckRevocation(subject.endEntityOrCA, certID, time,
-                                     validityDuration, stapledOCSPResponse,
+                                     notBefore, validityDuration,
+                                     stapledOCSPResponse,
                                      subject.GetAuthorityInfoAccess());
     if (rv != Success) {
       // Since this is actually a problem with the current subject certificate
       // (rather than the issuer), it doesn't make sense to keep going; all
       // paths through this certificate will fail.
       Result savedRv = RecordResult(rv, keepGoing);
       keepGoing = false;
       return savedRv;