Introduce the ability to require CT for specific hosts

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 c0dade1b2bb60e590e30e068a9c11aa4f01bce31..4c986351d455c0c6080d6e34bb9aa844c8415d72 100644
--- a/net/http/transport_security_state.h
+++ b/net/http/transport_security_state.h
@@ -53,6 +53,38 @@ class NET_EXPORT TransportSecurityState
     virtual ~Delegate() {}
   };
 
+  class NET_EXPORT RequireCTDelegate {
+   public:
+    // Provides a capability for altering the default handling of Certificate
+    // Transparency information, allowing it to be always required for some
+    // hosts, for some hosts to be opted out of the default policy, or
+    // allowing the TransportSecurityState to apply the default security
+    // policies.
+    enum class CTRequirementLevel {
+      // The host is required to always supply Certificate Transparency
+      // information that complies with the CT policy.
+      REQUIRED,
+
+      // The host is explicitly not required to supply Certificate
+      // Transparency information that complies with the CT policy.
+      NOT_REQUIRED,
+
+      // The delegate makes no statements, positive or negative, about
+      // requiring the host to supply Certificate Transparency information,
+      // allowing the default behaviour to happen.
+      DEFAULT,

[davidben, 2016/06/22 21:44:59]

To confirm, the reason DEFAULT is a notion is because TSS will have internal
logic? Is the intent that Expect-CT and Symantec bits be implemented in TSS and
the admin policy bits be in the delegate?

[Ryan Sleevi, 2016/06/22 22:07:39]

Yes (or eventually the CertVerifier)

+    };
+
+    // Called by the TransportSecurityState, allows the Delegate to override
+    // the default handling of Certificate Transparency requirements, if
+    // desired.
+    virtual CTRequirementLevel IsCTRequiredForHost(

[davidben, 2016/06/22 21:44:59]

Nit: A little weird to have an IsFoo function return something other than a
boolean. Although GetRequirementLevelForHost() is kinda wordy, so maybe this is
better?

[Ryan Sleevi, 2016/06/22 22:07:39]

It's just a tri-state bool ;)

( http://thedailywtf.com/articles/tri-state-boolean )

It's "Yes", "No", "#Yolo"

+        const std::string& hostname) = 0;
+
+   protected:
+    virtual ~RequireCTDelegate() = default;
+  };
+
   // A STSState describes the strict transport security state (required
   // upgrade to HTTPS).
   class NET_EXPORT STSState {
@@ -260,6 +292,17 @@ class NET_EXPORT TransportSecurityState
                           std::string* failure_log);
   bool HasPublicKeyPins(const std::string& host);
 
+  // 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.
+  //
+  // The behavior may be further be altered by setting a RequireCTDelegate
+  // via |SetRequireCTDelegate()|.
+  bool ShouldRequireCT(const std::string& host,
+                       const X509Certificate* validated_certificate_chain,
+                       const HashValueVector& hashes);
+
   // Assign a |Delegate| for persisting the transport security state. If
   // |NULL|, state will not be persisted. The caller retains
   // ownership of |delegate|.
@@ -271,6 +314,16 @@ class NET_EXPORT TransportSecurityState
 
   void SetExpectCTReporter(ExpectCTReporter* expect_ct_reporter);
 
+  // Assigns a delegate responsible for determining whether or not a
+  // connection to a given host should require Certificate Transparency
+  // information that complies with the CT policy provided by a
+  // CTPolicyEnforcer.
+  // If nullptr, no delegate will be consulted.
+  // The caller retains ownership of the |delegate|, and must persist for
+  // the lifetime of this object or until called with nullptr, whichever
+  // occurs first.
+  void SetRequireCTDelegate(RequireCTDelegate* delegate);
+
   // Clears all dynamic data (e.g. HSTS and HPKP data).
   //
   // Does NOT persist changes using the Delegate, as this function is only
@@ -475,9 +528,9 @@ class NET_EXPORT TransportSecurityState
   STSStateMap enabled_sts_hosts_;
   PKPStateMap enabled_pkp_hosts_;
 
-  Delegate* delegate_;
+  Delegate* delegate_ = nullptr;
 
-  ReportSenderInterface* report_sender_;
+  ReportSenderInterface* report_sender_ = nullptr;
 
   // True if static pins should be used.
   bool enable_static_pins_;

[davidben, 2016/06/22 21:44:59]

Why make the pointers = nullptr while the bools are set in the constructor? (I'm
unclear on when we prefer to do which.)

[Ryan Sleevi, 2016/06/22 22:07:39]

I haven't seen any clear guidance in any of the style guides, but the style I
was adopting was trying to be reflective of the policies regarding forward
declarations or sticking implementation details in unnamed namespaces:

What's in the header is, effectively, a contract - and a reason for your
dependencies to change. In this case, I didn't think that dependents should need
to be recompiled if we change how enable_static_* behaves (and, to be fair, it
changes in the ctor depending on compile flags that I don't think we want
leaking into the .h), but the assumption is we'd never "not" null-initialize the
pointers, so we won't end up churning these.

@@ -488,7 +541,9 @@ class NET_EXPORT TransportSecurityState
   // True if static expect-staple state should be used.
   bool enable_static_expect_staple_;
 
-  ExpectCTReporter* expect_ct_reporter_;
+  ExpectCTReporter* expect_ct_reporter_ = nullptr;
+
+  RequireCTDelegate* require_ct_delegate_ = nullptr;
 
   // Keeps track of reports that have been sent recently for
   // rate-limiting.