Check Expect-CT at connection setup

net/http/transport_security_state.h

 // Copyright (c) 2012 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.
 
 #ifndef NET_HTTP_TRANSPORT_SECURITY_STATE_H_
 #define NET_HTTP_TRANSPORT_SECURITY_STATE_H_
 
 #include <stdint.h>
 
 #include <map>
 #include <string>
 
 #include "base/callback.h"
 #include "base/feature_list.h"
 #include "base/gtest_prod_util.h"
 #include "base/macros.h"
 #include "base/strings/string_piece.h"
 #include "base/threading/non_thread_safe.h"
 #include "base/time/time.h"
 #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"
 
 namespace net {
 
 class HostPortPair;
 class SSLInfo;
 class X509Certificate;
 
 void NET_EXPORT_PRIVATE SetTransportSecurityStateSourceForTesting(
@@ skipping 270 matching lines @@
   };
 
   // An interface for building and asynchronously sending reports when a
   // site expects valid Certificate Transparency information but it
   // wasn't supplied.
   class NET_EXPORT ExpectCTReporter {
    public:
     // 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() {}
   };
 
   // Indicates whether or not a public key pin check should send a
   // 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;
 
   TransportSecurityState();
   ~TransportSecurityState();
 
   // These functions search for static and dynamic STS and PKP states, and
   // invoke the functions of the same name on them. These functions are the
   // primary public interface; direct access to STS and PKP states is best
@@ skipping 24 matching lines @@
   //    for OCSP responses behind MITM proxies are not useful to site owners.
   void CheckExpectStaple(const HostPortPair& host_port_pair,
                          const SSLInfo& ssl_info,
                          base::StringPiece ocsp_response);
 
   // Returns true if connections to |host|, using the validated certificate
   // |validated_certificate_chain|, are expected to be accompanied with
   // 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
   // ownership of |delegate|.
   // Note: This is only used for serializing/deserializing the
   // TransportSecurityState.
   void SetDelegate(Delegate* delegate);
 
   void SetReportSender(ReportSenderInterface* report_sender);
 
@@ skipping 271 matching lines @@
   // rate-limiting.
   ExpiringCache<std::string, bool, base::TimeTicks, std::less<base::TimeTicks>>
       sent_reports_cache_;
 
   DISALLOW_COPY_AND_ASSIGN(TransportSecurityState);
 };
 
 }  // namespace net
 
 #endif  // NET_HTTP_TRANSPORT_SECURITY_STATE_H_