Build and send HPKP violation reports

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 17885ba80f14dcfc625bc554e92c5174638041ae..876f9881dff2d170cbbc3296dd8f2408bd5708b5 100644
--- a/net/http/transport_security_state.h
+++ b/net/http/transport_security_state.h
@@ -45,9 +45,6 @@ class NET_EXPORT TransportSecurityState
     virtual ~Delegate() {}
   };
 
-  TransportSecurityState();
-  ~TransportSecurityState();
-
   // A DomainState describes the transport security state (required upgrade
   // to HTTPS, and/or any public key pins).
   //
@@ -170,6 +167,38 @@ class NET_EXPORT TransportSecurityState
     std::map<std::string, DomainState>::const_iterator end_;
   };
 
+  class NET_EXPORT Reporter {
+   public:
+    virtual ~Reporter() {}

[Ryan Sleevi, 2015/06/26 20:08:58]

Is ownership of the reporter transferred?

If not, this should be made protected - that way implementations can be made
using refcounting (if needed)

[estark, 2015/07/09 21:45:26]

Done.

+
+    // Returns true if a violation report should be sent for the host in
+    // the given |pkp_state|, and returns the report destination URI in
+    // |report_uri|. Returns false if a report should not be sent.
+    virtual bool GetHPKPReportUri(const DomainState::PKPState& pkp_state,
+                                  GURL* report_uri) = 0;

[Ryan Sleevi, 2015/06/26 20:08:58]

It's unclear why this is a property of the Reporter, rather than being something
the TSS handles (at least, from the header)

Without having read anything else (literally, this is the first file I opened),
I suspect this is to allow the Reporter to override the reporting URL for some
state; in which case, we should document that.

Also, note that there may be multiple report URIs - another thing to consider
with this interface.

[Ryan Sleevi, 2015/06/26 20:17:15]

Bah, bad spec language. I can't tell if I'm lying or telling the truth

http://tools.ietf.org/html/rfc7469#section-2.1
   2.  With the exception of pin-directives with the same pin-directive-
       name (see below), a given directive MUST NOT appear more than
       once in a given header field.  Directives are either optional or
       required, as stipulated in their definitions.

   directive             = directive-name [ "=" directive-value ]


This leaves it ambiguous as to whether we're talking directive-name or
directive.

That is, I would expect that

report-uri="http://www.google.com/foo"; report-uri="http://www.google.com/foo"
is invalid

but, as written
report-uri="http://www.google.com/foo"; report-uri="http://www.google.com/bar"
may be seen as valid (because they're the same directive-name, but different
directive-values, ergo different directives)


It may help if you take a look at the spec, since I think I'm too close to it to
recall all the nuance, to see if you can find other language one way or the
other on the interpretation. Chris may recall as well, but language-lawyer'ing
the spec should help. 

[estark, 2015/07/09 21:45:26]

I read Section 2.1 #2 as meaning that there shouldn't be multiple report-uris
(i.e. "directive" is a typo for "directive-name"). For two reasons:

1. If we read "directive" to mean "directive", then that also implies that there
can be multiple max-age directives as long as they have distinct values, which
a.) doesn't make sense (it doesn't seem like multiple max-ages ought to be
allowed), and b.) if it were the intention, it would be a fairly big omission in
the spec to not specify how multiple max-age directives should be handled.

2. The "With the exception of pin-directives" clause wouldn't really appear to
be necessary if we read "directive" to mean "directive". That is, there doesn't
seem to be a need to explicitly allow a pin directive (pin-sha256=blah) to
repeat, whereas there is a need to allow a pin directive name (pin-sha256) to
repeat (namely, pin and backup pin).

So, I'm not opposed to doing multiple report-uris, but if we interpret the spec
to mean that multiple report-uris are allowed, then we should also assume that
multiple max-ages are allowed (and I don't particularly want to implement
multiple max-ages because it seems kinda silly).

[estark, 2015/07/09 21:45:26]

Yes, that's the reason. Done.

+
+    // Builds a serialized HPKP violation report in
+    // |serialized_report|. Returns true on success and false on
+    // failure.
+    virtual bool BuildHPKPReport(
+        const std::string& hostname,
+        uint16_t port,
+        const base::Time& expiry,
+        bool include_subdomains,
+        const std::string& effective_hostname,

[Ryan Sleevi, 2015/06/26 20:08:58]

Should provide a bit of documentation about these parameters - in particular,
this one is a little tricksy

[estark, 2015/07/09 21:45:26]

Done.

+        const scoped_refptr<X509Certificate>& served_certificate_chain,
+        const scoped_refptr<X509Certificate>& validated_certificate_chain,

[Ryan Sleevi, 2015/06/26 20:08:58]

You should be able to pass these just as naked X509Certificate*'s here - this is
a synchronous call, and you're not requiring that the callee take ownership, so
it's fine to pass the raw pointers.

[estark, 2015/07/09 21:45:26]

Done.

+        const HashValueVector& spki_hashes,
+        std::string* serialized_report) = 0;
+
+    // Sends the given serialized |report| to |report_uri|.
+    virtual void SendHPKPReport(const GURL& report_uri,
+                                const std::string& report) = 0;
+  };
+
+  TransportSecurityState();
+  ~TransportSecurityState();
+
   // These functions search for static and dynamic DomainStates, and invoke the
   // functions of the same name on them. These functions are the primary public
   // interface; direct access to DomainStates is best left to tests.
@@ -188,6 +217,8 @@ class NET_EXPORT TransportSecurityState
   // TransportSecurityState.
   void SetDelegate(Delegate* delegate);
 
+  void SetReporter(Reporter* reporter);
+
   // Clears all dynamic data (e.g. HSTS and HPKP data).
   //
   // Does NOT persist changes using the Delegate, as this function is only
@@ -331,6 +362,8 @@ class NET_EXPORT TransportSecurityState
 
   Delegate* delegate_;
 
+  Reporter* reporter_;
+
   // True if static pins should be used.
   bool enable_static_pins_;