Certificate Transparency: Correct month calculation.

net/cert/cert_policy_enforcer_unittest.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/cert/cert_policy_enforcer.h"
 
 #include <string>
 
 #include "base/memory/scoped_ptr.h"
 #include "base/version.h"
@@ skipping 49 matching lines @@
       int num_scts,
       ct::CTVerifyResult* result) {
     for (int i = 0; i < num_scts; ++i) {
       scoped_refptr<ct::SignedCertificateTimestamp> sct(
           new ct::SignedCertificateTimestamp());
       sct->origin = desired_origin;
       result->verified_scts.push_back(sct);
     }
   }
 
+  void CheckCertificateCompliesWithExactNumberOfEmbeddedSCTs(
+      const base::Time& start,
+      const base::Time& end,
+      size_t required_scts) {
+    scoped_refptr<X509Certificate> cert(
+        new X509Certificate("subject", "issuer", start, end));
+    ct::CTVerifyResult result;
+    for (size_t j = 0; j < required_scts - 1; ++j) {

[davidben, 2015/03/25 15:00:54]

Nit: j -> i

[Eran Messeri, 2015/03/25 18:27:28]

Done.

+      FillResultWithSCTsOfOrigin(ct::SignedCertificateTimestamp::SCT_EMBEDDED,
+                                 1, &result);
+      EXPECT_FALSE(policy_enforcer_->DoesConformToCTEVPolicy(
+          cert.get(), nullptr, result, BoundNetLog()))
+          << " for: " << (end - start).InDays() << " and " << required_scts
+          << " scts=" << result.verified_scts.size() << " j=" << j;
+    }
+    FillResultWithSCTsOfOrigin(ct::SignedCertificateTimestamp::SCT_EMBEDDED, 1,
+                               &result);
+    EXPECT_TRUE(policy_enforcer_->DoesConformToCTEVPolicy(
+        cert.get(), nullptr, result, BoundNetLog()))
+        << " for: " << (end - start).InDays() << " and " << required_scts
+        << " scts=" << result.verified_scts.size();
+  }
+
  protected:
   scoped_ptr<CertPolicyEnforcer> policy_enforcer_;
   scoped_refptr<X509Certificate> chain_;
 };
 
 TEST_F(CertPolicyEnforcerTest, ConformsToCTEVPolicyWithNonEmbeddedSCTs) {
   ct::CTVerifyResult result;
   FillResultWithSCTsOfOrigin(
       ct::SignedCertificateTimestamp::SCT_FROM_TLS_EXTENSION, 2, &result);
 
@@ skipping 53 matching lines @@
       no_valid_dates_cert.get(), nullptr, result, BoundNetLog()));
   // ... but should be OK if whitelisted.
   scoped_refptr<ct::EVCertsWhitelist> whitelist(
       new DummyEVCertsWhitelist(true, true));
   EXPECT_TRUE(policy_enforcer_->DoesConformToCTEVPolicy(
       chain_.get(), whitelist.get(), result, BoundNetLog()));
 }
 
 TEST_F(CertPolicyEnforcerTest,
        ConformsToPolicyExactNumberOfSCTsForValidityPeriod) {
-  // Test multiple validity periods: Over 27 months, Over 15 months (but less
-  // than 27 months),
-  // Less than 15 months.
-  const size_t validity_period[] = {12, 19, 30, 50};
-  const size_t needed_scts[] = {2, 3, 4, 5};
+  // Test multiple validity periods:
+  // Under 15 months
+  // Over 15 months, less than 27 months
+  // Over 27 months, less than 39 months
+  // Over 39 months
+  const size_t validity_period[] = {417 /* 14 months */,

[davidben, 2015/03/25 15:00:54]

time_t? (Though see comment below.)

[Eran Messeri, 2015/03/25 18:27:28]

Left at size_t but grouped the number of required scts with this array.

+                                    458 /* exactly 15 months */,
+                                    460 /* over 15 months by a few days */,
+                                    823 /* exactly 27 months */,
+                                    826 /* over 27 months by a few days */,
+                                    1188 /* exactly 39 months */,
+                                    1190 /* over 39 months by a few days */};
+  const size_t needed_scts[] = {2, 3, 3, 3, 4, 4, 5};
 
+  // Fixed start time - Wed Mar 25 11:45:03 GMT 2015
+  base::Time fixed_start(base::Time::FromTimeT(1427283904));

[davidben, 2015/03/25 15:00:54]

Since this is all test code, it might be more understandable if you used
base::Time::FromString or base::Time::FromUTCExploded. And likewise for the
validity_period bits above. Maybe also wrap it in a struct to pair the
validity_period and the needed_scts together.

[Ryan Sleevi, 2015/03/25 16:54:23]

+1

Easier to just make a set of base::Time::FromUTCExploded in a set of test
structures

const struct TestData {
  base::Time validity_start;
  base::Time validity_end;
  int scts_present;
  bool expected_result;
} kTestData[] = {
  { base::Time::FromUTCExploded({2012, 1, 1, ... }),
    base::Time::FromUTCExploded({2014, 1, 1, ... }),
    3,
    false },
  { base::Time....

};

If you make the definition of the TestData (perhaps better named) struct outside
of this function, you can also declare

struct BetterName {
  // stuff
};

void PrintTo(const BetterName& object, ::std::ostream* os) {
  *os << "Validity Start: " << object.validity_start
        << "Validity End: " << object.validity_end
        << ...
}

TEST_F(CertPolicyEnforcerTest,
            Conforms....) {
  const BetterName test_data[] = {
    { ... }
  };
}

[Eran Messeri, 2015/03/25 18:27:28]

Done as you suggested - since the TestData structure is only used in one test
method I've defined it there.

   for (int i = 0; i < 3; ++i) {

[davidben, 2015/03/25 15:00:54]

While you're here, maybe put a SCOPED_TRACE(i) so you know which one failed.

[davidben, 2015/03/25 15:00:54]

BUG: 3 -> arraysize(validity_period)

(It looks like the old one got this wrong too...)

[Ryan Sleevi, 2015/03/25 16:54:23]

+1

[Ryan Sleevi, 2015/03/25 16:54:23]

s/int/size_t/

[Eran Messeri, 2015/03/25 18:27:28]

I do - the detail about the number of days is included in the
EXPECT_TRUE/EXPECT_FALSE printout.

[Eran Messeri, 2015/03/25 18:27:28]

Done.

[Eran Messeri, 2015/03/25 18:27:28]

Done.

     size_t curr_validity = validity_period[i];
-    scoped_refptr<X509Certificate> cert(new X509Certificate(
-        "subject", "issuer", base::Time::Now(),
-        base::Time::Now() + base::TimeDelta::FromDays(31 * curr_validity)));
-    size_t curr_required_scts = needed_scts[i];
-    ct::CTVerifyResult result;
-    for (size_t j = 0; j < curr_required_scts - 1; ++j) {
-      FillResultWithSCTsOfOrigin(ct::SignedCertificateTimestamp::SCT_EMBEDDED,
-                                 1, &result);
-      EXPECT_FALSE(policy_enforcer_->DoesConformToCTEVPolicy(
-          cert.get(), nullptr, result, BoundNetLog()))
-          << " for: " << curr_validity << " and " << curr_required_scts
-          << " scts=" << result.verified_scts.size() << " j=" << j;
-    }
-    FillResultWithSCTsOfOrigin(ct::SignedCertificateTimestamp::SCT_EMBEDDED, 1,
-                               &result);
-    EXPECT_TRUE(policy_enforcer_->DoesConformToCTEVPolicy(
-        cert.get(), nullptr, result, BoundNetLog()));
+    CheckCertificateCompliesWithExactNumberOfEmbeddedSCTs(
+        fixed_start, fixed_start + base::TimeDelta::FromDays(curr_validity),
+        needed_scts[i]);
   }
 }
 
 TEST_F(CertPolicyEnforcerTest, ConformsToPolicyByEVWhitelistPresence) {
   scoped_refptr<ct::EVCertsWhitelist> whitelist(
       new DummyEVCertsWhitelist(true, true));
 
   ct::CTVerifyResult result;
   FillResultWithSCTsOfOrigin(ct::SignedCertificateTimestamp::SCT_EMBEDDED, 1,
                              &result);
@@ skipping 16 matching lines @@
   ct::CTVerifyResult result;
   FillResultWithSCTsOfOrigin(ct::SignedCertificateTimestamp::SCT_EMBEDDED, 1,
                              &result);
   EXPECT_FALSE(policy_enforcer_->DoesConformToCTEVPolicy(
       chain_.get(), nullptr, result, BoundNetLog()));
 }
 
 }  // namespace
 
 }  // namespace net