Check Expect-CT at connection setup

net/http/transport_security_state.h

Index: net/http/transport_security_state.h
diff --git a/net/http/transport_security_state.h b/net/http/transport_security_state.h
index f2d182c901031b6ed833c0f30a014c610d309048..c9f2ab548908bf9222db69828897dfbc11d17ec8 100644
--- a/net/http/transport_security_state.h
+++ b/net/http/transport_security_state.h
@@ -20,6 +20,7 @@
 #include "net/base/expiring_cache.h"
 #include "net/base/hash_value.h"
 #include "net/base/net_export.h"
+#include "net/cert/signed_certificate_timestamp_and_status.h"
 #include "net/http/transport_security_state_source.h"
 #include "url/gurl.h"
 
@@ -310,9 +311,13 @@ class NET_EXPORT TransportSecurityState
     // Called when the host in |host_port_pair| has opted in to have
     // reports about Expect CT policy violations sent to |report_uri|,
     // and such a violation has occurred.
-    virtual void OnExpectCTFailed(const net::HostPortPair& host_port_pair,
-                                  const GURL& report_uri,
-                                  const net::SSLInfo& ssl_info) = 0;
+    virtual void OnExpectCTFailed(
+        const net::HostPortPair& host_port_pair,
+        const GURL& report_uri,
+        const X509Certificate* served_certificate_chain,
+        const X509Certificate* validated_certificate_chain,
+        const SignedCertificateTimestampAndStatusList&
+            signed_certificate_timestamps) = 0;
 
    protected:
     virtual ~ExpectCTReporter() {}
@@ -322,6 +327,13 @@ class NET_EXPORT TransportSecurityState
   // report if a violation is detected.
   enum PublicKeyPinReportStatus { ENABLE_PIN_REPORTS, DISABLE_PIN_REPORTS };
 
+  // Indicates whether or not an Expect-CT check should send a report if a
+  // violation is detected.
+  enum ExpectCTReportStatus {
+    ENABLE_EXPECT_CT_REPORTS,
+    DISABLE_EXPECT_CT_REPORTS
+  };
+
   // Feature that controls whether Expect-CT HTTP headers are parsed, processed,
   // and stored.
   static const base::Feature kDynamicExpectCTFeature;
@@ -366,11 +378,21 @@ class NET_EXPORT TransportSecurityState
   // valid Certificate Transparency information that complies with the
   // connection's CTPolicyEnforcer.
   //
+  // This method checks Expect-CT state for |host| if |issued_by_known_root| is
+  // true. If Expect-CT is configured for |host| and the connection is not
+  // compliant and |report_status| is ENABLE_EXPECT_CT_REPORTS, then a report
+  // will be sent.

[mattm, 2017/05/02 23:32:52]

It seems weird that a method called "ShouldRequireCT" has this side effect. Also
whether the connection is compliant isn't checked by ShouldRequireCT, it just
assumes the caller only calls ShouldRequireCT if the connection isn't compliant.

[estark, 2017/05/03 00:13:20]

Yeah, I thought about that and hoped that the extra documentation + ReportStatus
parameter might mitigate the weirdness. Here are a few other options:
a.) Have the method take a CertPolicyCompliance and immediately return true if
it is compliant. This would remove the issue where we assume the caller only
calls it for noncompliant connections. For the report-sending side effect, we
could rename the method to ShouldRequireCTAndMaybeSendReport or
CheckCTRequirements (mirroring CheckPublicKeyPins which also sends reports)
or... something else?
b.) Make Expect-CT a separate method entirely (CheckExpectCT?), and have callers
call both ShouldRequireCT and CheckExpectCT. I don't like this because it seems
error-prone to ask callers to check two separate methods to determine if a
connection should be closed because of CT.
b) Leave the connection-enforcement logic in ShouldRequireCT but make a separate
method MaybeSendExpectCT report to do the report-sending. I don't like this much
either because it would be easy for a caller to forget to call MaybeSendExpectCT
after calling ShouldRequireCT.

Of these, I like (a) best; WDYT?

[mattm, 2017/05/03 00:37:07]

(a) sgtm too. CheckCTRequirements seems reasonable. (I think the
ShouldRequireCT... name would then be a bit weird if it does the compliance
check too.)

[estark, 2017/05/04 01:18:30]

Done.

+  //
   // The behavior may be further be altered by setting a RequireCTDelegate
   // via |SetRequireCTDelegate()|.
-  bool ShouldRequireCT(const std::string& host,
+  bool ShouldRequireCT(const net::HostPortPair& host_port_pair,
+                       bool is_issued_by_known_root,
+                       const HashValueVector& public_key_hashes,
                        const X509Certificate* validated_certificate_chain,
-                       const HashValueVector& hashes);
+                       const X509Certificate* served_certificate_chain,

[mattm, 2017/05/02 23:32:52]

confusing that the order is different here than in OnExpectCTFailed

[estark, 2017/05/04 01:18:30]

Done (flipped the order in OnExpectCTFailed)

+                       const SignedCertificateTimestampAndStatusList&
+                           signed_certificate_timestamps,
+                       const ExpectCTReportStatus report_status);
 
   // Assign a |Delegate| for persisting the transport security state. If
   // |NULL|, state will not be persisted. The caller retains