Send HPKP violation reports when a pin check fails

Send HPKP violation reports when a pin check fails

This CL uses a net::TransportSecurityState::Reporter, instantiated by
ProfileIOData, to send HPKP violation reports. Information included in
the report (such as the validated and server-sent certificate chains)
is passed in to CheckPublicKeyPins().

CL #1: crrev.com/1211363005 (parse report-uri)
CL #2: crrev.com/1212973002 (add net::CertificateReportSender)
CL #3: crrev.com/1212613004 (add net::TransportSecurityReporter)
This is CL #4.

BUG=445793

[estark, 2015-06-26 19:29:00]

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

[estark, 2015-06-26 19:29:01]

This is CL 4/4 split out from https://codereview.chromium.org/1211933005/.

[Ryan Sleevi, 2015-06-26 20:22:19]

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

[Ryan Sleevi, 2015-06-26 20:22:20]

https://codereview.chromium.org/1213783005/diff/1/net/http/transport_security...
File net/http/transport_security_state.h (right):

https://codereview.chromium.org/1213783005/diff/1/net/http/transport_security...
net/http/transport_security_state.h:221: const PublicKeyPinReportStatus
report_status,
same comments re: raw pointers being cool

https://codereview.chromium.org/1213783005/diff/1/net/http/transport_security...
net/http/transport_security_state.h:348: const PublicKeyPinReportStatus
report_status,
ditto

https://codereview.chromium.org/1213783005/diff/1/net/http/transport_security...
File net/http/transport_security_state_unittest.cc (right):

https://codereview.chromium.org/1213783005/diff/1/net/http/transport_security...
net/http/transport_security_state_unittest.cc:118:
CompareCertificateChainWithList(served_certificate_chain,
BUG: You ASSERT_TRUE() in CompareCertificateChainWithList , which will abort
that, but it won't stop processing this code.

I believe you want ASSERT_NO_FATAL_FAILURE() [see
https://code.google.com/p/googletest/wiki/AdvancedGuide#Propagating_Fatal_Fai...
]

https://codereview.chromium.org/1213783005/diff/1/net/http/transport_security...
net/http/transport_security_state_unittest.cc:1209: }
no braces

https://codereview.chromium.org/1213783005/diff/1/net/http/transport_security...
net/http/transport_security_state_unittest.cc:1254: CheckHPKPReport(report,
kHost, kPort, expiry, true, kHost, cert1, cert2,
You don't propogate failures here either - re: ASSERT_NO_FATAL

[estark, 2015-07-09 21:51:22]

Patchset #2 (id:20001) has been deleted

[estark, 2015-07-09 21:52:42]

Patchset #2 (id:40001) has been deleted

[estark, 2015-07-09 22:18:42]

https://codereview.chromium.org/1213783005/diff/1/net/http/transport_security...
File net/http/transport_security_state.h (right):

https://codereview.chromium.org/1213783005/diff/1/net/http/transport_security...
net/http/transport_security_state.h:221: const PublicKeyPinReportStatus
report_status,
On 2015/06/26 20:22:19, Ryan Sleevi (slow through 7-15 wrote:
> same comments re: raw pointers being cool

Done.

https://codereview.chromium.org/1213783005/diff/1/net/http/transport_security...
net/http/transport_security_state.h:348: const PublicKeyPinReportStatus
report_status,
On 2015/06/26 20:22:19, Ryan Sleevi (slow through 7-15 wrote:
> ditto

Done.

https://codereview.chromium.org/1213783005/diff/1/net/http/transport_security...
File net/http/transport_security_state_unittest.cc (right):

https://codereview.chromium.org/1213783005/diff/1/net/http/transport_security...
net/http/transport_security_state_unittest.cc:118:
CompareCertificateChainWithList(served_certificate_chain,
On 2015/06/26 20:22:19, Ryan Sleevi (slow through 7-15 wrote:
> BUG: You ASSERT_TRUE() in CompareCertificateChainWithList , which will abort
> that, but it won't stop processing this code.
> 
> I believe you want ASSERT_NO_FATAL_FAILURE() [see
>
https://code.google.com/p/googletest/wiki/AdvancedGuide#Propagating_Fatal_Fai...
> ]

Done.

https://codereview.chromium.org/1213783005/diff/1/net/http/transport_security...
net/http/transport_security_state_unittest.cc:1209: }
On 2015/06/26 20:22:19, Ryan Sleevi (slow through 7-15 wrote:
> no braces

Done.

https://codereview.chromium.org/1213783005/diff/1/net/http/transport_security...
net/http/transport_security_state_unittest.cc:1254: CheckHPKPReport(report,
kHost, kPort, expiry, true, kHost, cert1, cert2,
On 2015/06/26 20:22:20, Ryan Sleevi (slow through 7-15 wrote:
> You don't propogate failures here either - re: ASSERT_NO_FATAL

Done.

[Ryan Sleevi, 2015-07-10 16:40:00]

One big design question: Does this preclude preloaded report uris?

https://codereview.chromium.org/1213783005/diff/80001/net/http/transport_secu...
File net/http/transport_security_state.cc (right):

https://codereview.chromium.org/1213783005/diff/80001/net/http/transport_secu...
net/http/transport_security_state.cc:845: if
(!reporter_->GetHPKPReportUri(dynamic_state, &report_uri))
Does this mean we can't preload report-uris? Seems to be.

Perhaps reshuffling this to indicate which PKPState was used?

https://codereview.chromium.org/1213783005/diff/80001/net/http/transport_secu...
File net/http/transport_security_state.h (right):

https://codereview.chromium.org/1213783005/diff/80001/net/http/transport_secu...
net/http/transport_security_state.h:234: SEND_PUBLIC_KEY_PIN_REPORT
Perhaps for brevity/readability this should be

DISABLE_REPORTING
ENABLE_REPORTING

I guess the two issues I have that make me feel uneasy are the "PUBLIC_KEY_PIN"
(which feels overly verbose) but also the "SEND_..._REPORT", which implies a
report will always be sent in the check, but it's not (only if the check
mismatches)

There's probably other ways to paint the bikeshed with those two concerns, so go
to town on naming (since I agree even my naming is weird)

[estark, 2015-07-10 19:33:30]

https://codereview.chromium.org/1213783005/diff/80001/net/http/transport_secu...
File net/http/transport_security_state.cc (right):

https://codereview.chromium.org/1213783005/diff/80001/net/http/transport_secu...
net/http/transport_security_state.cc:845: if
(!reporter_->GetHPKPReportUri(dynamic_state, &report_uri))
On 2015/07/10 16:40:00, Ryan Sleevi (slow through 7-15 wrote:
> Does this mean we can't preload report-uris? Seems to be.
> 
> Perhaps reshuffling this to indicate which PKPState was used?

Done.

https://codereview.chromium.org/1213783005/diff/80001/net/http/transport_secu...
File net/http/transport_security_state.h (right):

https://codereview.chromium.org/1213783005/diff/80001/net/http/transport_secu...
net/http/transport_security_state.h:234: SEND_PUBLIC_KEY_PIN_REPORT
On 2015/07/10 16:40:00, Ryan Sleevi (slow through 7-15 wrote:
> Perhaps for brevity/readability this should be
> 
> DISABLE_REPORTING
> ENABLE_REPORTING
> 
> I guess the two issues I have that make me feel uneasy are the
"PUBLIC_KEY_PIN"
> (which feels overly verbose) but also the "SEND_..._REPORT", which implies a
> report will always be sent in the check, but it's not (only if the check
> mismatches)
> 
> There's probably other ways to paint the bikeshed with those two concerns, so
go
> to town on naming (since I agree even my naming is weird)

Renamed to ENABLE_PIN_REPORTS/DISABLE_PIN_REPORTS?

[Ryan Sleevi, 2015-08-08 00:30:25]

Is this waiting on me? Does it need to be rebased?

[estark, 2015-08-08 00:35:39]

On 2015/08/08 00:30:25, Ryan Sleevi wrote:
> Is this waiting on me? Does it need to be rebased?

This CL isn't needed anymore (which is why it's Closed -- though maybe I should
have just deleted it to be less confusing). I shuffled things around so that
this kind of got folded into the previous CL in the series.

[Ryan Sleevi, 2015-08-08 00:37:03]

On 2015/08/08 00:35:39, estark wrote:
> On 2015/08/08 00:30:25, Ryan Sleevi wrote:
> > Is this waiting on me? Does it need to be rebased?
> 
> This CL isn't needed anymore (which is why it's Closed -- though maybe I
should
> have just deleted it to be less confusing). I shuffled things around so that
> this kind of got folded into the previous CL in the series.

Nah, it's my fault; was using email instead of code review to drive my reviews
:)