Initial (partial) implementation of HPKP violation reporting

Initial (partial) implementation of HPKP violation reporting

This CL implements some of the core functionality for HPKP violation
reporting, using certificate-report-sending code factored out of
//chrome.

As of this CL, the certificate reporting classes are:
* net::CertificateReportSender: an interface for sending serialized
  reports over the network. Implemented by
  net::CertificateReportSenderImpl and by unit tests that inject mock
  report senders.
* net::TransportSecurityReporter: builds HPKP reports and uses a
  net::CertificateReportSender to send them. net::TransportSecurityState
  uses a net::TransportSecurityReporter to handle HPKP reports.
* CertificateErrorReporter, in //chrome. This class handles the sending
  of Google-specific pinning reports and Safe Browsing Extended
  Reporting invalid cert reports (the latter of which it encrypts when
  applicable). It uses a net::CertificateReportSender to actually send
  reports.
* ChromeFraudulentCertificateReporter can be removed next: it uses a
  CertificateErrorReporter to handle Google-specific pinning reports. We
  can remove it and replace with a ChromeTransportSecurityReporter that
  implements the net::TransportSecurityReporter interface.

Notable to-dos not included in this CL:
* Include an accurate port number on reports generated from QUIC
  connections.
* Write dates in the reports in RFC 3339 format.
* Implement report-only mode.
* Allow report-uri to be relative.
* Unify ChromeFraudulentCertificateReporter with this CL by implementing
  ChromeTransportSecurityReporter and removing
  FraudulentCertificateReporter and ChromeFraudulentCertificateReporter.
* Rate-limit report-sending and detect loops.

BUG=445793

[estark, 2015-06-26 01:35:48]

estark@chromium.org changed reviewers:
+ davidben@chromium.org

[estark, 2015-06-26 01:35:48]

davidben: could you please take a look?

I know it's big, unfortunately, so below is an outline of some of the major plot
points. (I could try to split up into smaller CLs if you think that would be
helpful, but I didn't think that it split up in a particularly natural way.) I'm
also going to be on vacation the week of 6/29, so there's no particular rush.

* CertificateErrorReporter (in //chrome, used for sending Google-specific
pinning violations and Safe Browsing Extended Reporting cert reports) used to
contain logic for sending requests over the network. That logic has now been
factored out into net::CertificateReportSenderImpl, which implements the new
net::CertificateReportSender interface. net::CertificateReportSender is used by
both CertificateErrorReporter (in //chrome) and the new class that builds and
sends HPKP reports (in //net). I separated the CertificateReportSenderImpl from
CertificateReportSender so that tests could inject mock implementations of this
interface -- not sure if that pattern is right.

* Added net::TransportSecurityState::Reporter interface, implemented by
net::TransportSecurityReporter. Currently, ProfileIOData instantiates a
net::TransportSecurityReporter and sets it on the TransportSecurityState,
similar to how the persister works. In a future CL, I would like to create
ChromeTransportSecurityReporter (an implementation of
net::TransportSecurityState::Reporter) that basically wraps
net::TransportSecurityReporter PLUS handles the Google-specific pinning reports,
and have ProfileIOData instantiate a ChromeTransportSecurityReporter. This will
replace ChromeFraudulentCertificateReporter, and CertificateErrorReporter can
become for Safe Browsing Extended Reporting reports only (which will reduce some
confusion in that class).

* The rest of the CL is more straightforward: parsing report-uri in
http_security_headers, persisting it in transport_security_persister, updating
and adding tests.

[Ryan Sleevi, 2015-06-26 10:57:57]

rsleevi@chromium.org changed reviewers:
+ rsleevi@chromium.org

[Ryan Sleevi, 2015-06-26 10:57:58]

Suggested splits:
- The header parsing for report-uri and the persistence can be one CL (a small
CL)
- CertReportSender can be split into a single CL (just introducing the interface
& tests)
- The TransportSecurityReporter can be introduced in a single CL (it's unclear
whether the TSS needs to depend on it yet; I'll have to dig through this CL
more). If the integration is still needed, we can allow a nullptr TSR to be
used, which just won't send reports - keeping all the existing reporting code in
place
- Possibly split the Chrome piece into a single CL
- Then finally glue it all together

This will hopefully make it much, much easier to review - you can kick off all
four/five reviews simultaneously, but can respond to each segment in isolation
for each logical piece. It looks like you've got some nice cutpoints in the code
- that is, that a git rebase -i should be able to split these commits into
chunks, which you can then create git branches off of to upload CLs.

Just that things like the parsing of report_uri are totally self-contained and
the sort of ideal 'simple, small CL' (although it may end up larger for things
like the net-internals update), and then we can build on that.

[estark, 2015-06-26 19:27:18]

On 2015/06/26 10:57:58, Ryan Sleevi (slow through 7-15 wrote:
> Suggested splits:
> - The header parsing for report-uri and the persistence can be one CL (a small
> CL)
> - CertReportSender can be split into a single CL (just introducing the
interface
> & tests)
> - The TransportSecurityReporter can be introduced in a single CL (it's unclear
> whether the TSS needs to depend on it yet; I'll have to dig through this CL
> more). If the integration is still needed, we can allow a nullptr TSR to be
> used, which just won't send reports - keeping all the existing reporting code
in
> place
> - Possibly split the Chrome piece into a single CL
> - Then finally glue it all together
> 
> This will hopefully make it much, much easier to review - you can kick off all
> four/five reviews simultaneously, but can respond to each segment in isolation
> for each logical piece. It looks like you've got some nice cutpoints in the
code
> - that is, that a git rebase -i should be able to split these commits into
> chunks, which you can then create git branches off of to upload CLs.
> 
> Just that things like the parsing of report_uri are totally self-contained and
> the sort of ideal 'simple, small CL' (although it may end up larger for things
> like the net-internals update), and then we can build on that.

Ok, I'll give that a try.
#1: https://codereview.chromium.org/1211363005/ (parse report-uri)
#2: https://codereview.chromium.org/1212973002/ (add
net::CertificateReportSender)
#3: https://codereview.chromium.org/1212613004/ (add
net::TransportSecurityReporter)
#4: https://codereview.chromium.org/1213783005/ (send HPKP violation reports
when a pin check fails)

[Ryan Sleevi, 2015-06-26 20:24:07]

Just to close out a note that became clearer when reviewing the split up CLs - I
don't think we want to allow report-uri's to be relative. RFC 7469 doesn't say
they need to be (as far as I can read), and it would certainly add a whole host
of non-deterministic complexity (since we don't know what URI a given socket
will be used for, and then the whole storage of URIs for a host, etc)

So don't plan that one on the roadmap :)

[estark, 2015-06-26 20:29:08]

On 2015/06/26 20:24:07, Ryan Sleevi (slow through 7-15 wrote:
> Just to close out a note that became clearer when reviewing the split up CLs -
I
> don't think we want to allow report-uri's to be relative. RFC 7469 doesn't say
> they need to be (as far as I can read), and it would certainly add a whole
host
> of non-deterministic complexity (since we don't know what URI a given socket
> will be used for, and then the whole storage of URIs for a host, etc)
> 
> So don't plan that one on the roadmap :)

Oh, really? I was going off your comment on the design doc about report-uri
possibly being a uri-reference to match CSP report-uri. I'd be totally fine with
keeping it absolute though.

[Ryan Sleevi, 2015-06-26 20:34:05]

On 2015/06/26 20:29:08, estark wrote:
> Oh, really? I was going off your comment on the design doc about report-uri
> possibly being a uri-reference to match CSP report-uri. I'd be totally fine
with
> keeping it absolute though.

Yeah, it wasn't until I thought through in practice how it'd actually work - and
in seeing your code for sending reports - that I realized it would HAVE to be
absolute as the only sensible answer. Because it's a socket-level event rather
than request-level (that is, to support report-uri, we'd have to move it to the
URLRequest, and all of that weirdness we discussed comes in again)

[estark, 2015-06-26 20:48:36]

On 2015/06/26 20:34:05, Ryan Sleevi (slow through 7-15 wrote:
> On 2015/06/26 20:29:08, estark wrote:
> > Oh, really? I was going off your comment on the design doc about report-uri
> > possibly being a uri-reference to match CSP report-uri. I'd be totally fine
> with
> > keeping it absolute though.
> 
> Yeah, it wasn't until I thought through in practice how it'd actually work -
and
> in seeing your code for sending reports - that I realized it would HAVE to be
> absolute as the only sensible answer. Because it's a socket-level event rather
> than request-level (that is, to support report-uri, we'd have to move it to
the
> URLRequest, and all of that weirdness we discussed comes in again)

That reminds me: one thing I recently realized is that I think the Report-Only
implementation will actually have to be URLRequest-level, because Report-Only
applies to the request that the header was received on, and to only that
request. So I was imagining we might add something like
TransportSecurityState::CheckReportOnlyPublicKeyPins() that parses the
Report-Only header and checks it and sends a report. But such a function will
have to be called in URLRequestHttpJob::ProcessPublicKeyPinsHeader(), which
means that we'd been doing report-only PKP checks at the URLRequest level, but
real PKP checks at the socket level. Is that an argument that we should in fact
be doing the real PKP checks at the URLRequest level too?

[Ryan Sleevi, 2015-06-26 20:53:11]

On 2015/06/26 20:48:36, estark wrote:
> Is that an argument that we should in fact
> be doing the real PKP checks at the URLRequest level too?

Talk to David about the socket pools, but I suspect the answer is probably "No".

But this mostly goes back to the trade-offs and weird edge cases that we
discussed a few weeks ago - both have some viability.

That said, I think regardless of what we do for 'dynamic' PKP, we'll still want
static PKP baked into the Socket, since we do have those that don't go through
the URLRequest layer but do want pinning when talking to Google (e.g. GCM/Sync)