Check Expect-CT at connection setup

net/http/transport_security_state.cc

Index: net/http/transport_security_state.cc
diff --git a/net/http/transport_security_state.cc b/net/http/transport_security_state.cc
index 4486954966631398bc9cc155e1b6435fa2138475..81177fab52458f655813f060f5c0457013f3f235 100644
--- a/net/http/transport_security_state.cc
+++ b/net/http/transport_security_state.cc
@@ -50,7 +50,7 @@ const size_t kReportCacheKeyLength = 16;
 // Points to the active transport security state source.
 const TransportSecurityStateSource* g_hsts_source = &kHSTSSource;
 
-// Override for ShouldRequireCT() for unit tests. Possible values:
+// Override for CheckCTRequirements() for unit tests. Possible values:
 //  -1: Unless a delegate says otherwise, do not require CT.
 //   0: Use the default implementation (e.g. production)
 //   1: Unless a delegate says otherwise, require CT.
@@ -860,26 +860,57 @@ bool TransportSecurityState::HasPublicKeyPins(const std::string& host) {
   return false;
 }
 
-bool TransportSecurityState::ShouldRequireCT(
-    const std::string& hostname,
+bool TransportSecurityState::CheckCTRequirements(

[mattm, 2017/05/04 01:57:11]

do you think it's worth returning an enum value so there's no confusion over
what true / false mean?

[estark, 2017/05/04 04:03:25]

Yes, I think that makes sense. Done.

+    const net::HostPortPair& host_port_pair,
+    bool is_issued_by_known_root,
+    const HashValueVector& public_key_hashes,
     const X509Certificate* validated_certificate_chain,
-    const HashValueVector& public_key_hashes) {
+    const X509Certificate* served_certificate_chain,
+    const SignedCertificateTimestampAndStatusList&
+        signed_certificate_timestamps,
+    const ExpectCTReportStatus report_status,
+    ct::CertPolicyCompliance cert_policy_compliance) {
   using CTRequirementLevel = RequireCTDelegate::CTRequirementLevel;
+  std::string hostname = host_port_pair.host();
+
+  // If the connection complies with CT policy, then no further checks are
+  // necessary.
+  if (cert_policy_compliance ==
+          ct::CertPolicyCompliance::CERT_POLICY_COMPLIES_VIA_SCTS ||
+      cert_policy_compliance ==
+          ct::CertPolicyCompliance::CERT_POLICY_BUILD_NOT_TIMELY) {
+    return true;
+  }
+
+  // Check Expect-CT first so that other CT requirements do not prevent
+  // Expect-CT reports from being sent.
+  ExpectCTState state;
+  if (is_issued_by_known_root && IsDynamicExpectCTEnabled() &&
+      GetDynamicExpectCTState(hostname, &state)) {
+    if (expect_ct_reporter_ && !state.report_uri.is_empty() &&
+        report_status == ENABLE_EXPECT_CT_REPORTS) {
+      expect_ct_reporter_->OnExpectCTFailed(
+          host_port_pair, state.report_uri, validated_certificate_chain,
+          served_certificate_chain, signed_certificate_timestamps);
+    }
+    if (state.enforce)
+      return false;
+  }
 
   CTRequirementLevel ct_required = CTRequirementLevel::DEFAULT;
   if (require_ct_delegate_)
     ct_required = require_ct_delegate_->IsCTRequiredForHost(hostname);
   if (ct_required != CTRequirementLevel::DEFAULT)
-    return ct_required == CTRequirementLevel::REQUIRED;
+    return ct_required != CTRequirementLevel::REQUIRED;
 
   // Allow unittests to override the default result.
   if (g_ct_required_for_testing)
     return g_ct_required_for_testing == 1;

[mattm, 2017/05/04 01:57:11]

should that be !=  now?

[estark, 2017/05/04 04:03:25]

Fixed with the new enum return value.

 
   // Until CT is required for all secure hosts on the Internet, this should
-  // remain false. It is provided to simplify the various short-circuit
+  // remain true. It is provided to simplify the various short-circuit
   // returns below.
-  bool default_response = false;
+  bool default_response = true;

[mattm, 2017/05/04 01:57:11]

since you're touching this anyway, make this const?

[estark, 2017/05/04 04:03:24]

Done.

 
 // FieldTrials are not supported in Native Client apps.
 #if !defined(OS_NACL)
@@ -1413,8 +1444,10 @@ void TransportSecurityState::ProcessExpectCTHeader(
       return;
     ExpectCTState state;
     if (GetStaticExpectCTState(host_port_pair.host(), &state)) {
-      expect_ct_reporter_->OnExpectCTFailed(host_port_pair, state.report_uri,
-                                            ssl_info);
+      expect_ct_reporter_->OnExpectCTFailed(
+          host_port_pair, state.report_uri, ssl_info.cert.get(),
+          ssl_info.unverified_cert.get(),
+          ssl_info.signed_certificate_timestamps);
     }
     return;
   }
@@ -1447,8 +1480,10 @@ void TransportSecurityState::ProcessExpectCTHeader(
     // processing the header.
     if (expect_ct_reporter_ && !report_uri.is_empty() &&
         !GetDynamicExpectCTState(host_port_pair.host(), &state)) {
-      expect_ct_reporter_->OnExpectCTFailed(host_port_pair, report_uri,
-                                            ssl_info);
+      expect_ct_reporter_->OnExpectCTFailed(
+          host_port_pair, report_uri, ssl_info.cert.get(),
+          ssl_info.unverified_cert.get(),
+          ssl_info.signed_certificate_timestamps);
     }
     return;
   }