Ensure Expect-CT enforcement respects enterprise policies

Previously, Expect-CT would enable hard-fail enforcement of
CT for a domain, even if an Enterprise had disabled such
enforcement (via DisableCertificateTransparencyEnforcementForUrls).

Now, Enterprise Policies can be used to disable Expect-CT
enforcement for domains, which aligns with the priority of
constituencies and helps reduce any friction towards enabling
Expect-CT for split-DNS scenarios.

BUG=827170

Change-Id: I7b5cb3891d5726d3755f6d9e15f87050ebec6c7b
Reviewed-on: https://chromium-review.googlesource.com/986553
Commit-Queue: Ryan Sleevi <rsleevi@chromium.org>
Reviewed-by: David Benjamin <davidben@chromium.org>
Reviewed-by: Emily Stark <estark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546992}

net/http/transport_security_state.cc

@@ -885,6 +885,12 @@ TransportSecurityState::CheckCTRequirements(
   using CTRequirementLevel = RequireCTDelegate::CTRequirementLevel;
   std::string hostname = host_port_pair.host();
 
+  // CT is not required if the certificate does not chain to a publicly
+  // trusted root certificate. Testing can override this, as certain tests
+  // rely on using a non-publicly-trusted root.
+  if (!is_issued_by_known_root && g_ct_required_for_testing == 0)
+    return CT_NOT_REQUIRED;
+
   // A connection is considered compliant if it has sufficient SCTs or if the
   // build is outdated. Other statuses are not considered compliant; this
   // includes COMPLIANCE_DETAILS_NOT_AVAILABLE because compliance must have been
@@ -896,9 +902,9 @@ TransportSecurityState::CheckCTRequirements(
 
   // Check Expect-CT first so that other CT requirements do not prevent
   // Expect-CT reports from being sent.
+  bool required_via_expect_ct = false;
   ExpectCTState state;
-  if (is_issued_by_known_root && IsDynamicExpectCTEnabled() &&
-      GetDynamicExpectCTState(hostname, &state)) {
+  if (IsDynamicExpectCTEnabled() && GetDynamicExpectCTState(hostname, &state)) {
     UMA_HISTOGRAM_ENUMERATION(
         "Net.ExpectCTHeader.PolicyComplianceOnConnectionSetup",
         policy_compliance, ct::CTPolicyCompliance::CT_POLICY_MAX);
@@ -909,17 +915,27 @@ TransportSecurityState::CheckCTRequirements(
                                 served_certificate_chain,
                                 signed_certificate_timestamps);
     }
-    if (state.enforce)
-      return complies ? CT_REQUIREMENTS_MET : CT_REQUIREMENTS_NOT_MET;
+    required_via_expect_ct = state.enforce;
   }
 
   CTRequirementLevel ct_required = CTRequirementLevel::DEFAULT;
-  if (require_ct_delegate_)
+  if (require_ct_delegate_) {
+    // Allow the delegate to override the CT requirement state, including
+    // overriding any Expect-CT enforcement.
     ct_required = require_ct_delegate_->IsCTRequiredForHost(hostname);
-  if (ct_required != CTRequirementLevel::DEFAULT) {
-    if (ct_required == CTRequirementLevel::REQUIRED)
+  }
+  switch (ct_required) {
+    case CTRequirementLevel::REQUIRED:
       return complies ? CT_REQUIREMENTS_MET : CT_REQUIREMENTS_NOT_MET;
-    return CT_NOT_REQUIRED;
+    case CTRequirementLevel::NOT_REQUIRED:
+      return CT_NOT_REQUIRED;
+    case CTRequirementLevel::DEFAULT:
+      if (required_via_expect_ct) {
+        // If Expect-CT is set, short-circuit checking additional policies,
+        // since they will only enable CT requirement, not exclude from it.
+        return complies ? CT_REQUIREMENTS_MET : CT_REQUIREMENTS_NOT_MET;
+      }
+      break;
   }
 
   // Allow unittests to override the default result.

net/http/transport_security_state_unittest.cc

@@ -2224,6 +2224,9 @@ TEST_F(TransportSecurityStateTest, RequireCTConsultsDelegate) {
   hashes.push_back(
       HashValue(X509Certificate::CalculateFingerprint256(cert->cert_buffer())));
 
+  // If CT is required, then the requirements are not met if the CT policy
+  // wasn't met, but are met if the policy was met or the build was out of
+  // date.
   {
     TransportSecurityState state;
     const TransportSecurityState::CTRequirementsStatus original_status =
@@ -2276,6 +2279,8 @@ TEST_F(TransportSecurityStateTest, RequireCTConsultsDelegate) {
             ct::CTPolicyCompliance::CT_POLICY_NOT_ENOUGH_SCTS));
   }
 
+  // If CT is not required, then regardless of the CT state for the host,
+  // it should indicate CT is not required.
   {
     TransportSecurityState state;
     const TransportSecurityState::CTRequirementsStatus original_status =
@@ -2314,6 +2319,8 @@ TEST_F(TransportSecurityStateTest, RequireCTConsultsDelegate) {
             ct::CTPolicyCompliance::CT_POLICY_NOT_ENOUGH_SCTS));
   }
 
+  // If the Delegate is in the default state, then it should return the same
+  // result as if there was no delegate in the first place.
   {
     TransportSecurityState state;
     const TransportSecurityState::CTRequirementsStatus original_status =
@@ -2500,7 +2507,7 @@ TEST_F(TransportSecurityStateTest, RequireCTViaFieldTrial) {
       kFeatureName, base::FeatureList::OVERRIDE_ENABLE_FEATURE, trial.get());
   scoped_feature_list.InitWithFeatureList(std::move(feature_list));
 
-  // It should fail if it doesn't comply with policy...
+  // It should fail if it doesn't comply with policy.
   EXPECT_EQ(TransportSecurityState::CT_REQUIREMENTS_NOT_MET,
             state.CheckCTRequirements(
                 HostPortPair("www.example.com", 443), true, hashes, cert.get(),
@@ -2508,7 +2515,7 @@ TEST_F(TransportSecurityStateTest, RequireCTViaFieldTrial) {
                 TransportSecurityState::DISABLE_EXPECT_CT_REPORTS,
                 ct::CTPolicyCompliance::CT_POLICY_NOT_ENOUGH_SCTS));
 
-  // ... and succeed if it does comply with policy ...
+  // It should succeed if it does comply with policy.
   EXPECT_EQ(TransportSecurityState::CT_REQUIREMENTS_MET,
             state.CheckCTRequirements(
                 HostPortPair("www.example.com", 443), true, hashes, cert.get(),
@@ -2516,13 +2523,21 @@ TEST_F(TransportSecurityStateTest, RequireCTViaFieldTrial) {
                 TransportSecurityState::DISABLE_EXPECT_CT_REPORTS,
                 ct::CTPolicyCompliance::CT_POLICY_COMPLIES_VIA_SCTS));
 
-  // ... or if the build is outdated.
+  // It should succeed if the build is outdated.
   EXPECT_EQ(TransportSecurityState::CT_REQUIREMENTS_MET,
             state.CheckCTRequirements(
                 HostPortPair("www.example.com", 443), true, hashes, cert.get(),
                 cert.get(), SignedCertificateTimestampAndStatusList(),
                 TransportSecurityState::DISABLE_EXPECT_CT_REPORTS,
                 ct::CTPolicyCompliance::CT_POLICY_BUILD_NOT_TIMELY));
+
+  // It should succeed if it was a locally-trusted CA.
+  EXPECT_EQ(TransportSecurityState::CT_NOT_REQUIRED,
+            state.CheckCTRequirements(
+                HostPortPair("www.example.com", 443), false, hashes, cert.get(),
+                cert.get(), SignedCertificateTimestampAndStatusList(),
+                TransportSecurityState::DISABLE_EXPECT_CT_REPORTS,
+                ct::CTPolicyCompliance::CT_POLICY_BUILD_NOT_TIMELY));
 }
 
 // Tests that Certificate Transparency is required for all of the Symantec
@@ -3121,6 +3136,62 @@ TEST_F(TransportSecurityStateTest, CheckCTRequirementsWithExpectCTAndDelegate) {
   EXPECT_EQ(sct_list[0].sct, reporter.signed_certificate_timestamps()[0].sct);
 }
 
+// Tests that for a host that explicitly disabled CT by delegate and is also
+// Expect-CT-enabled, CheckCTRequirements() sends reports.
+TEST_F(TransportSecurityStateTest,
+       CheckCTRequirementsWithExpectCTAndDelegateDisables) {
+  using ::testing::_;
+  using ::testing::Return;
+  using CTRequirementLevel =
+      TransportSecurityState::RequireCTDelegate::CTRequirementLevel;
+
+  const base::Time current_time(base::Time::Now());
+  const base::Time expiry = current_time + base::TimeDelta::FromSeconds(1000);
+  scoped_refptr<X509Certificate> cert1 =
+      ImportCertFromFile(GetTestCertsDirectory(), "ok_cert.pem");
+  ASSERT_TRUE(cert1);
+  scoped_refptr<X509Certificate> cert2 =
+      ImportCertFromFile(GetTestCertsDirectory(), "expired_cert.pem");
+  ASSERT_TRUE(cert2);
+  SignedCertificateTimestampAndStatusList sct_list;
+  MakeTestSCTAndStatus(ct::SignedCertificateTimestamp::SCT_EMBEDDED, "test_log",
+                       std::string(), std::string(), base::Time::Now(),
+                       ct::SCT_STATUS_INVALID_SIGNATURE, &sct_list);
+
+  base::test::ScopedFeatureList feature_list;
+  feature_list.InitAndEnableFeature(
+      TransportSecurityState::kDynamicExpectCTFeature);
+
+  TransportSecurityState state;
+  MockExpectCTReporter reporter;
+  state.SetExpectCTReporter(&reporter);
+  state.AddExpectCT("example.test", expiry, false /* enforce */,
+                    GURL("https://example-report.test"));
+
+  // A connection to an Expect-CT host, which is exempted from the CT
+  // requirements by the delegate, should be reported but not closed.
+  MockRequireCTDelegate never_require_delegate;
+  EXPECT_CALL(never_require_delegate, IsCTRequiredForHost(_))
+      .WillRepeatedly(Return(CTRequirementLevel::NOT_REQUIRED));
+  state.SetRequireCTDelegate(&never_require_delegate);
+  EXPECT_EQ(TransportSecurityState::CT_NOT_REQUIRED,
+            state.CheckCTRequirements(
+                HostPortPair("example.test", 443), true, HashValueVector(),
+                cert1.get(), cert2.get(), sct_list,
+                TransportSecurityState::ENABLE_EXPECT_CT_REPORTS,
+                ct::CTPolicyCompliance::CT_POLICY_NOT_ENOUGH_SCTS));
+  EXPECT_EQ(1u, reporter.num_failures());
+  EXPECT_EQ("example.test", reporter.host_port_pair().host());
+  EXPECT_EQ(443, reporter.host_port_pair().port());
+  EXPECT_EQ(expiry, reporter.expiration());
+  EXPECT_EQ(cert1.get(), reporter.validated_certificate_chain());
+  EXPECT_EQ(cert2.get(), reporter.served_certificate_chain());
+  EXPECT_EQ(sct_list.size(), reporter.signed_certificate_timestamps().size());
+  EXPECT_EQ(sct_list[0].status,
+            reporter.signed_certificate_timestamps()[0].status);
+  EXPECT_EQ(sct_list[0].sct, reporter.signed_certificate_timestamps()[0].sct);
+}
+
 // Tests that the dynamic Expect-CT UMA histogram is recorded correctly.
 TEST_F(TransportSecurityStateTest, DynamicExpectCTUMA) {
   const char kHistogramName[] = "Net.ExpectCTHeader.ParseSuccess";

net/socket/ssl_client_socket_unittest.cc

@@ -3796,6 +3796,10 @@ TEST_F(SSLClientSocketTest, CTRequiredHistogramNonCompliantLocalRoot) {
   cert_verifier_->AddResultForCert(server_cert.get(), verify_result, OK);
 
   // Set up the CT requirement and failure to comply.
+  base::ScopedClosureRunner cleanup(base::BindOnce(
+      &TransportSecurityState::SetShouldRequireCTForTesting, nullptr));
+  bool require_ct = true;
+  TransportSecurityState::SetShouldRequireCTForTesting(&require_ct);
   MockRequireCTDelegate require_ct_delegate;
   transport_security_state_->SetRequireCTDelegate(&require_ct_delegate);
   EXPECT_CALL(require_ct_delegate, IsCTRequiredForHost(_))