Certificate Transparency: Require SCTs for EV certificates.

net/cert/cert_policy_enforcer_unittest.cc

Index: net/cert/cert_policy_enforcer_unittest.cc
diff --git a/net/cert/cert_policy_enforcer_unittest.cc b/net/cert/cert_policy_enforcer_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..88d264617458e2ed7918c492df029c5cc0815e4e
--- /dev/null
+++ b/net/cert/cert_policy_enforcer_unittest.cc
@@ -0,0 +1,190 @@
+// 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 "net/cert/ct_ev_whitelist.h"
+#include "net/cert/ct_verify_result.h"
+#include "net/cert/x509_certificate.h"
+#include "net/test/cert_test_util.h"
+#include "net/test/ct_test_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace net {
+
+namespace {
+
+class DummyEVCertsWhitelist : public ct::EVCertsWhitelist {
+ public:
+  explicit DummyEVCertsWhitelist(bool always_return)
+      : canned_response_(always_return) {}
+
+  bool IsValid() const override { return true; }
+
+  bool ContainsCertificateHash(
+      const std::string& certificate_hash) const override {
+    return canned_response_;
+  }
+
+ protected:
+  ~DummyEVCertsWhitelist() override {}
+
+ private:
+  bool canned_response_;
+};

[Ryan Sleevi, 2014/11/28 15:27:44]

Seems like this could/should be replaced with GMock, since that's what this
really is.

class MockEVCertsWhitelist : public ct::EVCertsWhitelist {
 public:
  MockEVCertsWhitelist() {
    ON_CALL(*this, IsValid()).WillByDefault(Return(true));
    ON_CALL(*this, ContainsCertificateHash(_)).WillByDefault(Return(false));
  }
  MOCK_CONST_METHOD0(IsValid(), bool());
  MOCK_CONST_METHOD1(ContainsCertificateHash(), bool(const std::string&));
};

[Eran Messeri, 2014/12/01 13:59:03]

Done with a slight variation: I couldn't use ON_CALL on the constructed object
because it's a ref-counted class, so I passed in the return parameters  to the
c'tor.

+
+class CertPolicyEnforcerTest : public ::testing::Test {
+ public:
+  virtual void SetUp() override {
+    policy_enforcer_.reset(new CertPolicyEnforcer(5, true));
+
+    std::string der_test_cert(ct::GetDerEncodedX509Cert());
+    chain_ = X509Certificate::CreateFromBytes(der_test_cert.data(),
+                                              der_test_cert.length());

[Ryan Sleevi, 2014/11/28 15:27:44]

s/.length()/.size()/

[Eran Messeri, 2014/12/01 13:59:03]

Done.

+    ASSERT_TRUE(chain_.get());
+    whitelist_ = new DummyEVCertsWhitelist(false);

[Ryan Sleevi, 2014/11/28 15:27:44]

Nuke this in favour of consistency among the tests (each initializing whitelist
themselves?)

The asymmetry of 177 makes it weird.

This also lets you override how IsValid() works and to ALSO make sure that an EV
whitelist that isn't valid is ignored.

Tests for:
1) No whitelist (== nullptr) - 123 - 161 indirectly test this, but not really
2) Whitelist !IsValid() - No such test exists for this right now 
3) Whitelist IsValid(), !ContainsCertificateHash() - implicitly being tested by
70 - 175
4) Whitelist IsValid(), ContainsCertificateHash() - being tested by 177-186

Note that for each of the EXPECT_FALSE(Conforms(...)), I'd like to see an
EXPECT_TRUE(Conforms(...)) with a different MockEVCertsWhitelist (or you can
just use the existing one and replace the ON_CALL).

This is to make sure our whitelist is good for _all_ cases (namely, that we
always check the whitelist first, and the whitelist overrides any other
requirements we may have)

[Eran Messeri, 2014/12/01 13:59:03]

Done, per your suggestion, particularly:
(1) used nullptr as whitelist in all tests that expect success.
(2) tested nullptr whitelist in a test of its own (IgnoresNullEVWhitelist)
(3) Explicit test for !IsValid whitelist (IgnoresInvalidEVWhitelist)
(4) Explicit test for IsValid, ContainsCertificateHash (
ConformsToPolicyByEVWhitelistPresence)
(5) In all tests that expect failure, added a second EXPECT_TRUE with a
whitelist where IsValid and ContainsCertificateHash return true.
(6) In one of the tests also added the use of a valid whitelist where
ContainsCertificateHash returns false.

+  }
+
+  void FillResultWithSCTsOfOrigin(
+      ct::SignedCertificateTimestamp::Origin desired_origin,
+      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);
+    }
+  }
+
+ protected:
+  scoped_ptr<CertPolicyEnforcer> policy_enforcer_;
+  scoped_refptr<X509Certificate> chain_;
+  scoped_refptr<ct::EVCertsWhitelist> whitelist_;
+};
+
+TEST_F(CertPolicyEnforcerTest, ConformsToCTEVPolicyWithNonEmbeddedSCTs) {
+  ct::CTVerifyResult result;
+  FillResultWithSCTsOfOrigin(
+      ct::SignedCertificateTimestamp::SCT_FROM_TLS_EXTENSION, 2, &result);
+
+  EXPECT_TRUE(policy_enforcer_->DoesConformToCTEVPolicy(
+      chain_.get(), whitelist_.get(), result));
+}
+
+TEST_F(CertPolicyEnforcerTest, ConformsToCTEVPolicyWithEmbeddedSCTs) {
+  // This chain_ is valid for 10 years - over 121 months - so requires 5 SCTs.
+  ct::CTVerifyResult result;
+  FillResultWithSCTsOfOrigin(ct::SignedCertificateTimestamp::SCT_EMBEDDED, 5,
+                             &result);
+
+  EXPECT_TRUE(policy_enforcer_->DoesConformToCTEVPolicy(
+      chain_.get(), whitelist_.get(), result));
+}
+
+TEST_F(CertPolicyEnforcerTest, ConformsToCTEVPolicyMixedOriginSCTs) {
+  ct::CTVerifyResult result;
+  FillResultWithSCTsOfOrigin(
+      ct::SignedCertificateTimestamp::SCT_FROM_TLS_EXTENSION, 2, &result);
+  result.verified_scts[1]->origin =
+      ct::SignedCertificateTimestamp::SCT_EMBEDDED;
+  EXPECT_TRUE(policy_enforcer_->DoesConformToCTEVPolicy(
+      chain_.get(), whitelist_.get(), result));
+}
+
+TEST_F(CertPolicyEnforcerTest, DoesNotConformToCTEVPolicyNotEnoughSCTs) {
+  // This chain_ is valid for 10 years - over 121 months - so requires 5 SCTs.
+  // However, as there are only two logs, two SCTs will be required - supply one
+  // to guarantee the test fails.
+  ct::CTVerifyResult result;
+  FillResultWithSCTsOfOrigin(ct::SignedCertificateTimestamp::SCT_EMBEDDED, 1,
+                             &result);
+
+  EXPECT_FALSE(policy_enforcer_->DoesConformToCTEVPolicy(
+      chain_.get(), whitelist_.get(), result));
+}
+
+TEST_F(CertPolicyEnforcerTest, DoesNotEnforceCTPolicyIfNotRequired) {
+  scoped_ptr<CertPolicyEnforcer> enforcer(new CertPolicyEnforcer(3, false));
+
+  ct::CTVerifyResult result;
+  FillResultWithSCTsOfOrigin(ct::SignedCertificateTimestamp::SCT_EMBEDDED, 1,
+                             &result);
+  // Expect true despite the chain not having enough SCTs as the policy
+  // is not enforced.
+  EXPECT_TRUE(enforcer->DoesConformToCTEVPolicy(chain_.get(), whitelist_.get(),
+                                                result));
+}
+
+TEST_F(CertPolicyEnforcerTest, DoesNotConformToPolicyInvalidDates) {
+  scoped_refptr<X509Certificate> no_valid_dates_cert(new X509Certificate(
+      "subject", "issuer", base::Time(), base::Time::Now()));
+  ct::CTVerifyResult result;
+  FillResultWithSCTsOfOrigin(ct::SignedCertificateTimestamp::SCT_EMBEDDED, 5,
+                             &result);
+  EXPECT_FALSE(policy_enforcer_->DoesConformToCTEVPolicy(
+      no_valid_dates_cert.get(), NULL, result));
+}
+
+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};
+
+  for (int i = 0; i < 3; ++i) {
+    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(), NULL, result))
+          << " 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(), NULL, result));
+  }
+}
+
+TEST_F(CertPolicyEnforcerTest,
+       ConformsToPolicyButDoesNotRequireMoreThanNumLogs) {
+  scoped_ptr<CertPolicyEnforcer> enforcer(new CertPolicyEnforcer(2, true));
+
+  ct::CTVerifyResult result;
+  FillResultWithSCTsOfOrigin(ct::SignedCertificateTimestamp::SCT_EMBEDDED, 2,
+                             &result);
+  // Expect true despite the chain not having enough SCTs according to the
+  // policy
+  // since we only have 2 logs.
+  EXPECT_TRUE(enforcer->DoesConformToCTEVPolicy(chain_.get(), whitelist_.get(),
+                                                result));
+}
+
+TEST_F(CertPolicyEnforcerTest, ConformsToPolicyByEVWhitelistPresence) {
+  scoped_refptr<ct::EVCertsWhitelist> whitelist =
+      new DummyEVCertsWhitelist(true);
+
+  ct::CTVerifyResult result;
+  FillResultWithSCTsOfOrigin(ct::SignedCertificateTimestamp::SCT_EMBEDDED, 1,
+                             &result);
+  EXPECT_TRUE(policy_enforcer_->DoesConformToCTEVPolicy(
+      chain_.get(), whitelist.get(), result));
+}
+
+}  // namespace
+
+}  // namespace net