Align the CT Implementation with Policy

net/cert/ct_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/ct_policy_enforcer.h"
 
 #include <memory>
 #include <string>
 
 #include "base/time/time.h"
@@ skipping 45 matching lines @@
 class CTPolicyEnforcerTest : public ::testing::Test {
  public:
   void SetUp() override {
     policy_enforcer_.reset(new CTPolicyEnforcer);
 
     std::string der_test_cert(ct::GetDerEncodedX509Cert());
     chain_ = X509Certificate::CreateFromBytes(der_test_cert.data(),
                                               der_test_cert.size());
     ASSERT_TRUE(chain_.get());
     google_log_id_ = std::string(kGoogleAviatorLogID, crypto::kSHA256Length);
-    non_google_log_id_.assign(crypto::kSHA256Length, 'A');
+    non_google_log_id_.assign(crypto::kSHA256Length, 1);
   }
 
   void FillListWithSCTsOfOrigin(
       ct::SignedCertificateTimestamp::Origin desired_origin,
       size_t num_scts,
       const std::vector<std::string>& desired_log_keys,
       bool timestamp_past_enforcement_date,
       ct::SCTList* verified_scts) {
     for (size_t i = 0; i < num_scts; ++i) {
       scoped_refptr<ct::SignedCertificateTimestamp> sct(
           new ct::SignedCertificateTimestamp());
       sct->origin = desired_origin;
       if (i < desired_log_keys.size())
         sct->log_id = desired_log_keys[i];
       else
-        sct->log_id = non_google_log_id_;
+        sct->log_id = std::string(crypto::kSHA256Length, static_cast<char>(i));
 
       if (timestamp_past_enforcement_date)
         sct->timestamp =
             base::Time::FromUTCExploded({2015, 8, 0, 15, 0, 0, 0, 0});
       else
         sct->timestamp =
             base::Time::FromUTCExploded({2015, 6, 0, 15, 0, 0, 0, 0});
 
       verified_scts->push_back(sct);
     }
   }
 
   void FillListWithSCTsOfOrigin(
       ct::SignedCertificateTimestamp::Origin desired_origin,
       size_t num_scts,
       ct::SCTList* verified_scts) {
     std::vector<std::string> desired_log_ids;
     desired_log_ids.push_back(google_log_id_);
     FillListWithSCTsOfOrigin(desired_origin, num_scts, desired_log_ids, true,
                              verified_scts);
   }
 
-  void FillSCTListWithRepeatedLogID(const std::string& desired_id,
-                                    size_t num_scts,
-                                    bool timestamp_past_enforcement_date,
-                                    ct::SCTList* verified_scts) {
-    std::vector<std::string> desired_log_ids(num_scts, desired_id);
-
-    FillListWithSCTsOfOrigin(
-        ct::SignedCertificateTimestamp::SCT_FROM_TLS_EXTENSION, num_scts,
-        desired_log_ids, timestamp_past_enforcement_date, verified_scts);
-  }
-
   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::SCTList scts;
-
     for (size_t i = 0; i < required_scts - 1; ++i) {
-      FillListWithSCTsOfOrigin(ct::SignedCertificateTimestamp::SCT_EMBEDDED, 1,
+      ct::SCTList scts;

[Ryan Sleevi, 2016/05/02 23:41:24]

Changed because the original code was duplicating the log_id, which shouldn't
have been allowed.

+      FillListWithSCTsOfOrigin(ct::SignedCertificateTimestamp::SCT_EMBEDDED, i,
                                std::vector<std::string>(), false, &scts);
       EXPECT_EQ(ct::CertPolicyCompliance::CERT_POLICY_NOT_ENOUGH_SCTS,
                 policy_enforcer_->DoesConformToCertPolicy(cert.get(), scts,
                                                           BoundNetLog()))
           << " for: " << (end - start).InDays() << " and " << required_scts
           << " scts=" << scts.size() << " i=" << i;
       EXPECT_EQ(ct::EVPolicyCompliance::EV_POLICY_NOT_ENOUGH_SCTS,
                 policy_enforcer_->DoesConformToCTEVPolicy(cert.get(), nullptr,
                                                           scts, BoundNetLog()))
           << " for: " << (end - start).InDays() << " and " << required_scts
           << " scts=" << scts.size() << " i=" << i;
     }
-    FillListWithSCTsOfOrigin(ct::SignedCertificateTimestamp::SCT_EMBEDDED, 1,
-                             std::vector<std::string>(), false, &scts);
+    ct::SCTList scts;
+    FillListWithSCTsOfOrigin(ct::SignedCertificateTimestamp::SCT_EMBEDDED,
+                             required_scts, std::vector<std::string>(), false,
+                             &scts);
     EXPECT_EQ(ct::CertPolicyCompliance::CERT_POLICY_COMPLIES_VIA_SCTS,
               policy_enforcer_->DoesConformToCertPolicy(cert.get(), scts,
                                                         BoundNetLog()))
         << " for: " << (end - start).InDays() << " and " << required_scts
         << " scts=" << scts.size();
     EXPECT_EQ(ct::EVPolicyCompliance::EV_POLICY_COMPLIES_VIA_SCTS,
               policy_enforcer_->DoesConformToCTEVPolicy(cert.get(), nullptr,
                                                         scts, BoundNetLog()))
         << " for: " << (end - start).InDays() << " and " << required_scts
         << " scts=" << scts.size();
   }
 
  protected:
   std::unique_ptr<CTPolicyEnforcer> policy_enforcer_;
   scoped_refptr<X509Certificate> chain_;
   std::string google_log_id_;
   std::string non_google_log_id_;
 };
 
 TEST_F(CTPolicyEnforcerTest,
        DoesNotConformToCTEVPolicyNotEnoughDiverseSCTsAllGoogle) {
   ct::SCTList scts;
-  FillSCTListWithRepeatedLogID(google_log_id_, 2, true, &scts);
+  std::vector<std::string> desired_log_ids(2, google_log_id_);
+
+  FillListWithSCTsOfOrigin(
+      ct::SignedCertificateTimestamp::SCT_FROM_TLS_EXTENSION,
+      desired_log_ids.size(), desired_log_ids, true, &scts);

[Ryan Sleevi, 2016/05/02 23:41:24]

Trying to reduce magic boiler-plate because it took me too damn log to keep
cross-referencing the "helpers".

 
   EXPECT_EQ(ct::CertPolicyCompliance::CERT_POLICY_NOT_DIVERSE_SCTS,
             policy_enforcer_->DoesConformToCertPolicy(chain_.get(), scts,
                                                       BoundNetLog()));
   EXPECT_EQ(ct::EVPolicyCompliance::EV_POLICY_NOT_DIVERSE_SCTS,
             policy_enforcer_->DoesConformToCTEVPolicy(chain_.get(), nullptr,
                                                       scts, BoundNetLog()));
 }
 
 TEST_F(CTPolicyEnforcerTest,
        DoesNotConformToCTEVPolicyNotEnoughDiverseSCTsAllNonGoogle) {
   ct::SCTList scts;
-  FillSCTListWithRepeatedLogID(non_google_log_id_, 2, true, &scts);
+  std::vector<std::string> desired_log_ids(2, non_google_log_id_);
+
+  FillListWithSCTsOfOrigin(
+      ct::SignedCertificateTimestamp::SCT_FROM_TLS_EXTENSION,
+      desired_log_ids.size(), desired_log_ids, true, &scts);
 
   EXPECT_EQ(ct::CertPolicyCompliance::CERT_POLICY_NOT_DIVERSE_SCTS,
             policy_enforcer_->DoesConformToCertPolicy(chain_.get(), scts,
                                                       BoundNetLog()));
   EXPECT_EQ(ct::EVPolicyCompliance::EV_POLICY_NOT_DIVERSE_SCTS,
             policy_enforcer_->DoesConformToCTEVPolicy(chain_.get(), nullptr,
                                                       scts, BoundNetLog()));
 }
 
 TEST_F(CTPolicyEnforcerTest, ConformsToCTEVPolicyIfSCTBeforeEnforcementDate) {
   ct::SCTList scts;
-  FillSCTListWithRepeatedLogID(non_google_log_id_, 2, false, &scts);
+  // This chain_ is valid for 10 years - over 121 months - so requires 5 SCTs.
+  // All 5 SCTs will be from non-Google logs.
+  FillListWithSCTsOfOrigin(ct::SignedCertificateTimestamp::SCT_EMBEDDED, 5,
+                           std::vector<std::string>(), false, &scts);

[Ryan Sleevi, 2016/05/02 23:41:24]

The old test relied on the TLS extension method, which implies sending precerts,
which itself was a broken test that relied on our bugged behaviour.

 
   EXPECT_EQ(ct::CertPolicyCompliance::CERT_POLICY_COMPLIES_VIA_SCTS,
             policy_enforcer_->DoesConformToCertPolicy(chain_.get(), scts,
                                                       BoundNetLog()));
   EXPECT_EQ(ct::EVPolicyCompliance::EV_POLICY_COMPLIES_VIA_SCTS,
             policy_enforcer_->DoesConformToCTEVPolicy(chain_.get(), nullptr,
                                                       scts, BoundNetLog()));
 }
 
 TEST_F(CTPolicyEnforcerTest, ConformsToCTEVPolicyWithNonEmbeddedSCTs) {
@@ skipping 16 matching lines @@
                            &scts);
 
   EXPECT_EQ(ct::CertPolicyCompliance::CERT_POLICY_COMPLIES_VIA_SCTS,
             policy_enforcer_->DoesConformToCertPolicy(chain_.get(), scts,
                                                       BoundNetLog()));
   EXPECT_EQ(ct::EVPolicyCompliance::EV_POLICY_COMPLIES_VIA_SCTS,
             policy_enforcer_->DoesConformToCTEVPolicy(chain_.get(), nullptr,
                                                       scts, BoundNetLog()));
 }
 
+TEST_F(CTPolicyEnforcerTest, ConformsToCTEVPolicyWithPooledNonEmbeddedSCTs) {
+  ct::SCTList scts;
+  std::vector<std::string> desired_logs;
+
+  // One Google log, delivered via OCSP.
+  desired_logs.clear();
+  desired_logs.push_back(google_log_id_);
+  FillListWithSCTsOfOrigin(
+      ct::SignedCertificateTimestamp::SCT_FROM_OCSP_RESPONSE,
+      desired_logs.size(), desired_logs, true, &scts);
+
+  // One non-Google log, delivered via TLS.
+  desired_logs.clear();
+  desired_logs.push_back(non_google_log_id_);
+  FillListWithSCTsOfOrigin(
+      ct::SignedCertificateTimestamp::SCT_FROM_TLS_EXTENSION,
+      desired_logs.size(), desired_logs, true, &scts);
+
+  EXPECT_EQ(ct::CertPolicyCompliance::CERT_POLICY_COMPLIES_VIA_SCTS,
+            policy_enforcer_->DoesConformToCertPolicy(chain_.get(), scts,
+                                                      BoundNetLog()));
+  EXPECT_EQ(ct::EVPolicyCompliance::EV_POLICY_COMPLIES_VIA_SCTS,
+            policy_enforcer_->DoesConformToCTEVPolicy(chain_.get(), nullptr,
+                                                      scts, BoundNetLog()));
+}
+
+TEST_F(CTPolicyEnforcerTest, ConformsToCTEVPolicyWithPooledEmbeddedSCTs) {
+  ct::SCTList scts;
+  std::vector<std::string> desired_logs;
+
+  // One Google log, delivered embedded.
+  desired_logs.clear();
+  desired_logs.push_back(google_log_id_);
+  FillListWithSCTsOfOrigin(ct::SignedCertificateTimestamp::SCT_EMBEDDED,
+                           desired_logs.size(), desired_logs, true, &scts);
+
+  // One non-Google log, delivered via OCSP.
+  desired_logs.clear();
+  desired_logs.push_back(non_google_log_id_);
+  FillListWithSCTsOfOrigin(
+      ct::SignedCertificateTimestamp::SCT_FROM_OCSP_RESPONSE,
+      desired_logs.size(), desired_logs, true, &scts);
+
+  EXPECT_EQ(ct::CertPolicyCompliance::CERT_POLICY_COMPLIES_VIA_SCTS,
+            policy_enforcer_->DoesConformToCertPolicy(chain_.get(), scts,
+                                                      BoundNetLog()));
+  EXPECT_EQ(ct::EVPolicyCompliance::EV_POLICY_COMPLIES_VIA_SCTS,
+            policy_enforcer_->DoesConformToCTEVPolicy(chain_.get(), nullptr,
+                                                      scts, BoundNetLog()));
+}
+
 TEST_F(CTPolicyEnforcerTest, DoesNotConformToCTEVPolicyNotEnoughSCTs) {
   scoped_refptr<ct::EVCertsWhitelist> non_including_whitelist(
       new DummyEVCertsWhitelist(true, false));
   // 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::SCTList scts;
-  FillListWithSCTsOfOrigin(ct::SignedCertificateTimestamp::SCT_EMBEDDED, 1,
+  FillListWithSCTsOfOrigin(ct::SignedCertificateTimestamp::SCT_EMBEDDED, 2,

[Ryan Sleevi, 2016/05/02 23:41:24]

The change to 2 here (and later on) is to ensure that the *diversity*
requirement is always met.

The new policy specifies diversity before amount, since that's even more
important.

                            &scts);
 
   EXPECT_EQ(ct::CertPolicyCompliance::CERT_POLICY_NOT_ENOUGH_SCTS,
             policy_enforcer_->DoesConformToCertPolicy(chain_.get(), scts,
                                                       BoundNetLog()));
   EXPECT_EQ(
       ct::EVPolicyCompliance::EV_POLICY_NOT_ENOUGH_SCTS,
       policy_enforcer_->DoesConformToCTEVPolicy(
           chain_.get(), non_including_whitelist.get(), scts, BoundNetLog()));
 
   // ... but should be OK if whitelisted.
   scoped_refptr<ct::EVCertsWhitelist> whitelist(
       new DummyEVCertsWhitelist(true, true));
   EXPECT_EQ(ct::EVPolicyCompliance::EV_POLICY_COMPLIES_VIA_WHITELIST,
             policy_enforcer_->DoesConformToCTEVPolicy(
                 chain_.get(), whitelist.get(), scts, BoundNetLog()));
 }
 
+TEST_F(CTPolicyEnforcerTest,
+       DoesNotConformToCTEVPolicyNotEnoughUniqueEmbeddedLogs) {
+  ct::SCTList scts;
+  std::vector<std::string> desired_logs;
+
+  // One Google Log.
+  desired_logs.clear();
+  desired_logs.push_back(google_log_id_);
+  FillListWithSCTsOfOrigin(ct::SignedCertificateTimestamp::SCT_EMBEDDED,
+                           desired_logs.size(), desired_logs, true, &scts);
+
+  // Two distinct non-Google logs.
+  desired_logs.clear();
+  desired_logs.push_back(std::string(crypto::kSHA256Length, 'A'));
+  desired_logs.push_back(std::string(crypto::kSHA256Length, 'B'));
+  FillListWithSCTsOfOrigin(ct::SignedCertificateTimestamp::SCT_EMBEDDED,
+                           desired_logs.size(), desired_logs, true, &scts);
+
+  // Two unique SCTs from the same non-Google log.
+  desired_logs.clear();
+  desired_logs.push_back(std::string(crypto::kSHA256Length, 'C'));
+  desired_logs.push_back(std::string(crypto::kSHA256Length, 'C'));
+  FillListWithSCTsOfOrigin(ct::SignedCertificateTimestamp::SCT_EMBEDDED,
+                           desired_logs.size(), desired_logs, true, &scts);
+
+  // This chain_ is valid for 10 years - over 121 months - so requires 5 SCTs.
+  // However, there are only 4 SCTs are from distinct logs.
+  EXPECT_EQ(ct::CertPolicyCompliance::CERT_POLICY_NOT_ENOUGH_SCTS,
+            policy_enforcer_->DoesConformToCertPolicy(chain_.get(), scts,
+                                                      BoundNetLog()));
+  EXPECT_EQ(ct::EVPolicyCompliance::EV_POLICY_NOT_ENOUGH_SCTS,
+            policy_enforcer_->DoesConformToCTEVPolicy(chain_.get(), nullptr,
+                                                      scts, BoundNetLog()));
+}
+
 // TODO(estark): fix this test so that it can check if
 // |no_valid_dates_cert| is on the whitelist without
 // crashing. https://crbug.com/582740
 TEST_F(CTPolicyEnforcerTest, DISABLED_DoesNotConformToPolicyInvalidDates) {
   scoped_refptr<X509Certificate> no_valid_dates_cert(new X509Certificate(
       "subject", "issuer", base::Time(), base::Time::Now()));
   ct::SCTList scts;
   FillListWithSCTsOfOrigin(ct::SignedCertificateTimestamp::SCT_EMBEDDED, 5,
                            &scts);
   ASSERT_TRUE(no_valid_dates_cert);
@@ skipping 54 matching lines @@
         kTestData[i].validity_start, kTestData[i].validity_end,
         kTestData[i].scts_required);
   }
 }
 
 TEST_F(CTPolicyEnforcerTest, ConformsToPolicyByEVWhitelistPresence) {
   scoped_refptr<ct::EVCertsWhitelist> whitelist(
       new DummyEVCertsWhitelist(true, true));
 
   ct::SCTList scts;
-  FillListWithSCTsOfOrigin(ct::SignedCertificateTimestamp::SCT_EMBEDDED, 1,
+  FillListWithSCTsOfOrigin(ct::SignedCertificateTimestamp::SCT_EMBEDDED, 2,
                            &scts);
   EXPECT_EQ(ct::CertPolicyCompliance::CERT_POLICY_NOT_ENOUGH_SCTS,
             policy_enforcer_->DoesConformToCertPolicy(chain_.get(), scts,
                                                       BoundNetLog()));
   EXPECT_EQ(ct::EVPolicyCompliance::EV_POLICY_COMPLIES_VIA_WHITELIST,
             policy_enforcer_->DoesConformToCTEVPolicy(
                 chain_.get(), whitelist.get(), scts, BoundNetLog()));
 }
 
 TEST_F(CTPolicyEnforcerTest, IgnoresInvalidEVWhitelist) {
   scoped_refptr<ct::EVCertsWhitelist> whitelist(
       new DummyEVCertsWhitelist(false, true));
 
   ct::SCTList scts;
-  FillListWithSCTsOfOrigin(ct::SignedCertificateTimestamp::SCT_EMBEDDED, 1,
+  FillListWithSCTsOfOrigin(ct::SignedCertificateTimestamp::SCT_EMBEDDED, 2,
                            &scts);
   EXPECT_EQ(ct::EVPolicyCompliance::EV_POLICY_NOT_ENOUGH_SCTS,
             policy_enforcer_->DoesConformToCTEVPolicy(
                 chain_.get(), whitelist.get(), scts, BoundNetLog()));
 }
 
 TEST_F(CTPolicyEnforcerTest, IgnoresNullEVWhitelist) {
   ct::SCTList scts;
-  FillListWithSCTsOfOrigin(ct::SignedCertificateTimestamp::SCT_EMBEDDED, 1,
+  FillListWithSCTsOfOrigin(ct::SignedCertificateTimestamp::SCT_EMBEDDED, 2,
                            &scts);
   EXPECT_EQ(ct::EVPolicyCompliance::EV_POLICY_NOT_ENOUGH_SCTS,
             policy_enforcer_->DoesConformToCTEVPolicy(chain_.get(), nullptr,
                                                       scts, BoundNetLog()));
 }
 
 }  // namespace
 
 }  // namespace net