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 0e9e762bdb4ffbd250aba2ddfd7bf74f39279b2a..8c9782f382eb6968aaf35d1ae7175a9aa948ea39 100644
--- a/net/http/transport_security_state.h
+++ b/net/http/transport_security_state.h
@@ -5,12 +5,13 @@
 #ifndef NET_HTTP_TRANSPORT_SECURITY_STATE_H_
 #define NET_HTTP_TRANSPORT_SECURITY_STATE_H_
 
+#include <stdint.h>
+
 #include <map>
 #include <string>
 #include <utility>
 #include <vector>
 
-#include "base/basictypes.h"
 #include "base/gtest_prod_util.h"
 #include "base/threading/non_thread_safe.h"
 #include "base/time/time.h"
@@ -18,6 +19,8 @@
 #include "net/cert/x509_cert_types.h"
 #include "net/cert/x509_certificate.h"
 
+class GURL;
+
 namespace net {
 
 class SSLInfo;
@@ -45,9 +48,6 @@ class NET_EXPORT TransportSecurityState
     virtual ~Delegate() {}
   };
 
-  TransportSecurityState();
-  ~TransportSecurityState();
-
   // A STSState describes the strict transport security state (required
   // upgrade to HTTPS).
   class NET_EXPORT STSState {
@@ -178,10 +178,62 @@ class NET_EXPORT TransportSecurityState
     std::map<std::string, PKPState>::const_iterator end_;
   };
 
+  class NET_EXPORT Reporter {
+   public:
+    // Allows the reporter to override the reporting state in some cases
+    // (for example, if reports should always be sent for certain
+    // hostnames regardless of the HPKP state). 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.

[davidben, 2015/07/15 23:38:35]

It seems Ryan partially asked this, but where are we planning on doing anything
but the default here? Is this for some kind of corp policy or something?

[estark, 2015/07/15 23:51:36]

Hmm, sorry, maybe it would have been clearer if I added a fifth CL to this
series where I actually used this.

The plan is to implement this interface in //chrome
(ChromeTransportSecurityReporter) and use it to special-case Google properties
to match what ChromeFraudulentCertificateReporter currently does.

[davidben, 2015/07/16 00:22:53]

Ah, gotcha. Would it work to model that as another part of the preloaded data?
This way we don't need two different "is this Google?" checks and it interacts
better with different pinning sources. I suppose if we were to do that, it'd be
slightly weird to use a different format.

Though the different format is odd anyway. What happens if a Google property
sends a PKP header that uses report-uri? It'd be weird for that to use the
cert_logger.proto format since every other browser will interpret it as the JSON
format. Also would be weird to override the report-uri with a hard-coded one.

How unhappy would the server people be if they had to use the JSON one? If we
have a standards-based mechanism, I'd think it's better to use it rather than a
Google-specific thing, especially if they're equivalent, just with a different
syntax. Plus, if they ever want reports from other browsers, they'll need to
support the standards-based syntax anyway.

Alternative, if we do really need the other format, what about this model:

- HPKP reporting is one universe and report_uri always means HPKP reporting.
That just goes through the one SendReport(GURL, string) API rather than three.

- We have a second OnStaticPinningViolation() method that gives you the
parameters of BuildHPKPReport. Maybe the static pins have a bool
report_violations bit that funnels to an embedder-controlled location if we
don't want to report non-Google static pin violations.

Observers do what they like with that and the Chrome-specific code will use the
proto format and toss it to the pre-defined Google URL. If you want the proto
codepath to hit dynamic pins too, you could make it OnPinningViolation, but I
kinda like separating the two universe. I dunno, maybe we later add a way to set
pins based on admin policy. That probably shouldn't fold into this logic. Or if
a user uses net-internals to inject random pins with a random report-uri.

(But I think this is slightly silly and using the one JSON format seems much
saner overall.)


Thoughts?

[estark, 2015/07/16 01:41:13]

I was thinking that the ChromeTransportSecurityReporter would always let
report-uri take precedence; so it would always return pkp_state.report_uri if
non-empty, and return the hard-coded report-uri for Google properties if
pkp_state.report_uri is empty.

(We could also possibly combine GetHPKPReportUri() and BuildHPKPReport() into
one GetHPKPReport() method that returns both the serialized report and the URL
to send it to, which would avoid having two different "is it google?" checks.)
The problem is that the server people might be nonexistent. :) I'll ask around a
bit more, but my impression so far has definitely been that I don't think anyone
has been actively maintaining or working on the server side for some time. If we
want to use the JSON format, I can probably do the server-side myself, but I
would be sad to block all of HPKP reporting on it. (See comments below about how
we can maybe start with the two formats and transition to JSON once the server
supports it.)
Hmm, Ryan commented somewhere in one of these CLs that all static pins should be
able to have report-uris. So I think static pinning violations should go through
the same SendReport(GURL, string) API, but maybe OnStaticPinningViolation() can
also take whether a report was sent as an argument? e.g.

if (static_pin_violation) {
    bool report_sent = false;
    if (!pkp_state.report_uri.empty()) {
        SendReport(...);
        report_sent = true;
    }
    // Toss is to embedder, who will send proto-format reports if a report
hasn't already been sent
    OnStaticPinningViolation(/* BuildHPKPReport args */, report_sent);
}

The only thing I don't like about this approach is that I'm not sure how we
would transition to the JSON format for the Google reports while continuing to
special-case the report uri for Google. In other words, what I like about
ChromeTransportSecurityReporter is that once the server accepts JSON format, we
can just remove the custom code in
ChromeTransportSecurityReporter::BuildHPKPReport(), i.e. continue to override
the report URL for Google properties but not do anything special for the report
format.

In the OnStaticPinningViolation() world, once the server starts accepting JSON,
we could remove the OnStaticPinningViolation() observer in chrome but would then
have to figure out a way to let the embedder override the report_uri (or
actually literally set the report-uri in the preloaded data... but that seems
error-prone to me). Or we could leaving the OnStaticPinningViolation() observer
and figure out a way to let chrome use the JSON-HPKP-report-building code (maybe
it can just be a static method on TransportSecurityState).

[davidben, 2015/07/16 22:39:19]

Ah, okay. How would you then know in BuildHPKPReport to use the protobuf? Check
the report-uri against the hard-coded value? That seems mildly hacky. (I dunno,
maybe someone actually set that to their report-uri.)
That would be much better for avoiding the mixup between normal report-uri and
the old format. Although it would still be possible for a dynamic
report-uri-less Google pin to hit our servers, which is probably not desirable.

If Google servers ever send a standards-based dynamic header, I think it's
reasonable to say that using the standards-based format is a dependency.

If a user puts one in via net-internals or if we do admin policy or extensions
or whatever other crazy sources, I don't think it makes sense to send those
reports to our old mechanism.

Though I don't feel very strongly about having that separation, so if you prefer
a merged GetHPKPReport() method, that works for me.
Fair enough. Yeah, I agree there's no need to block on it. I do think it makes
sense to consider the old format legacy and at least have transitioning to the
standards one be an open task somewhere, presuming the standards one does
everything we need it to. (We have a lot of Google-specific logic in Chrome and
the more we can lean on standards-based mechanisms that we have to support
anyway, the better.)
I was thinking we'd literally set report-uri in the preloaded data and the
delete the other mechanism, yeah. Maybe run with both for a bit as a trial, I
dunno. I would have suggested just surfacing it as a preloaded report_uri now if
the formats weren't different. :-) We can always change the preloads; there's Go
program somewhere in agl's Github[*].

That seems the simplest end state, once the formats are unified: we have the one
kind of PKP state and the one codepath for reports. There's two possible
sources, dynamic and static. But they both feed into teh same mechanism.

[*] Don't ask. Dumb rules about what languages Chromium can and can't have. :-)

+    virtual bool GetHPKPReportUri(const PKPState& pkp_state,
+                                  GURL* report_uri) = 0;
+
+    // Builds a serialized HPKP violation report in
+    // |serialized_report|. The information included in the report is:
+    //
+    // - The |hostname| and |port| to which the request was sent that
+    //   triggered this report.
+    // - |expiry|, the time at which the HPKP state that triggered this
+    //   report will expire.
+    // - |include_subdomains|, indicating whether the includeSubdomains
+    //   directive was observed for this pin.
+    // - |effective_hostname|, the hostname that was noted for the
+    //   pin. This can be different than |hostname| if, for example,
+    //   the pin was for foo.com with includeSubdomains and the request
+    //   that triggered the report was example.foo.com.
+    // - |served_certificate_chain| and |validated_certificate_chain|,
+    //   the certificate chains as received by the client and as built
+    //   during certificate verification.
+    // - The |spki_hashes| to which the |effective_hostname| is pinned.
+    //
+    // 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,
+        const X509Certificate* served_certificate_chain,
+        const X509Certificate* validated_certificate_chain,
+        const HashValueVector& spki_hashes,
+        std::string* serialized_report) = 0;

[davidben, 2015/07/15 23:38:35]

Can this not be in TransportSecurityState? There's just the one HPKP format,
right?

[estark, 2015/07/15 23:51:37]

ChromeTransportSecurityReporter will have two report formats: JSON for standard
HPKP reports, cert_logger.proto for Google properties.

+
+    // Sends the given serialized |report| to |report_uri|.
+    virtual void SendHPKPReport(const GURL& report_uri,
+                                const std::string& report) = 0;

[davidben, 2015/07/15 23:38:35]

If we could get rid of GetHPKPReportUri or do it differently, this interface
would just be CertificateReportSender, which seems tidier. It seems the only
interface that actually needs to be here, at least based on these CLs, is to
invert the TransportSecurityState -> URLRequest dependency. (Which can then
double as the point you mock stuff out for tests.)

Alternatively, if we keep GetHPKPReportUri, is there a need for
CertificateReportSender as an interface? I'm a little puzzled what it's for.

[estark, 2015/07/15 23:51:36]

Here is what I'm thinking:
- TransportSecurityState uses a TSS::Reporter. Right now this is implemented as
TransportSecurityReporter. The next step after this series of CLs is to
introduce ChromeTransportSecurityReporter and use that. ChromeTSReporter
special-cases Google properties and uses a TransportSecurityReporter to handle
non-Google reports.
- TransportSecurityReporter uses a CertificateReporterSender to send reports.
CertificateReportSender is a separate interface because there is other cert
reporting code (CertificateErrorReporter in //chrome/browser/net) that also uses
a CertificateReportSender. CertificateReportSenderImpl is separate so that
CertificateReportSender can be mocked out for tests.

Does that clarify things at all?

[davidben, 2015/07/16 00:22:53]

Ah, okay, so the CertificateReportSender is less because this is some glue thing
that //net stuff needs and more because a bunch of certificate-related classes
need to mock out a "POST this string to this URL" interface. I dunno, that seems
slightly strange, but I don't have a concrete alternative that I like much
better. (URLFetchers are mockable, so I suppose you could pass in a
URLFetcherFactory. It's a much much larger interface though, so that might not
be the best plan.)

+
+   protected:
+    virtual ~Reporter() {}
+  };
+
+  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 left to tests.
+  // 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
+  // left to tests.
   bool ShouldSSLErrorsBeFatal(const std::string& host);
   bool ShouldUpgradeToSSL(const std::string& host);
   bool CheckPublicKeyPins(const std::string& host,
@@ -197,6 +249,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
@@ -350,6 +404,8 @@ class NET_EXPORT TransportSecurityState
 
   Delegate* delegate_;
 
+  Reporter* reporter_;
+
   // True if static pins should be used.
   bool enable_static_pins_;