Implement Expect-Staple

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 2988f3ad8a49e9fe072795af4c4b2b9654cb20e0..3f194214ab510480de4468d2c615f21c46604baa 100644
--- a/net/http/transport_security_state.h
+++ b/net/http/transport_security_state.h
@@ -27,6 +27,7 @@ class GURL;
 
 namespace net {
 
+class ExpectStapleReport;
 class HostPortPair;
 class SSLInfo;
 
@@ -385,8 +386,24 @@ class NET_EXPORT TransportSecurityState
                              const HostPortPair& host_port_pair,
                              const SSLInfo& ssl_info);
 
+  // Checks to see if the given |host_port_pair| is in the Expect-Staple preload
+  // list. If the host is preloaded, this parses |ocsp_response|, validates
+  // it against |verified_certificate|, and ensures the OCSP response is valid
+  // at |verify_time| and is no older than |max_age|. If the OCSP response fails
+  // validation, this sends an Expect-Staple report to the preloaded report URI.
+  // The report will contain |unverified_certificate| iff
+  // |is_issued_by_known_root| is true.

[Ryan Sleevi, 2016/06/16 21:49:29]

This also seems a big "SRP" concern. This doesn't have anything to do with
persisting or storing the data in the transport security state - all you really
care about is the domain policy for |host_port_pair|. Building a report,
verifying, sending, etc - that all seems like something that should be done
outside the TSS.

Line 385-387 I'm totally down with, because that speaks to TSS's core thing -
manage domain policies. This method seems to do... a lot more work.

+  void CheckExpectStaple(const HostPortPair& host_port_pair,
+                         const X509Certificate& verified_certificate,
+                         const X509Certificate& unverified_certificate,
+                         bool is_issued_by_known_root,
+                         const base::Time& verify_time,
+                         const base::TimeDelta& max_age,
+                         const std::string& ocsp_response);
+
  private:
   friend class TransportSecurityStateTest;
+  friend class ExpectStapleTest;

[Ryan Sleevi, 2016/06/16 21:49:29]

I block the addition of friends because I'm a terrible person. The only friend
should be this files tester

[dadrian, 2016/06/17 17:26:55]

This is a derived class of this files tester. The other option would be to make
SerializeExpectStapleReport public.

   FRIEND_TEST_ALL_PREFIXES(HttpSecurityHeadersTest, UpdateDynamicPKPOnly);
   FRIEND_TEST_ALL_PREFIXES(HttpSecurityHeadersTest, UpdateDynamicPKPMaxAge0);
   FRIEND_TEST_ALL_PREFIXES(HttpSecurityHeadersTest, NoClobberPins);
@@ -410,6 +427,14 @@ class NET_EXPORT TransportSecurityState
   // information) is timely.
   static bool IsBuildTimely();
 
+  // Helper method for serializing an ExpectStaple report.
+  static bool SerializeExpectStapleReport(

[Ryan Sleevi, 2016/06/17 16:19:56]

DESIGN: Static privates are a design anti-pattern, in that no one can reach into
them but the class, but they require definition in the .h (thus affecting
consumers). The general argument "for" them is "So tests can call into them",
but that means writing tests which aren't black-box tests, and those are
generally bad tests.

So the approach is to move static privates into the .cc file (since that's the
only place they can be called) and in the unnamed namespace.

The exception to this is when a single class spans multiple TUs (for example,
foo.cc and foo_win.cc/foo_linux.cc, with _win/_linux needing to call some core
functionality, and without using inheritance as the platform abstraction layer),
but this is TransportSecurityStatic, so it doesn't rise to that level

[dadrian, 2016/06/17 17:26:55]

It's currently structured like this to be able to inject ExpectStapleReports, in
order to make the verification of the serialized form in tests feasible. I think
this goes back to my earlier point of deciding on how TransportSecurityState
should interact with ExpectStapleReport. Maybe the thing to do here is break out
a reporter, similar to the ChromeExpectCTReporter, but Emily and I were trying
to avoid doing that, especially since most of the useful code is already inside
of an anonymous namespace in transport_security_state.cc anyway.

+      const HostPortPair& host_port_pair,
+      const X509Certificate& unverified_certificate,
+      bool is_issued_by_known_root,
+      const ExpectStapleReport& report,
+      std::string* serialized_report);
+
   // Helper method for actually checking pins.
   bool CheckPublicKeyPinsImpl(
       const HostPortPair& host_port_pair,