Build and send HPKP violation reports

Build and send HPKP violation reports

This CL adds code to TransportSecurityState to build HPKP reports, and
sends them with a CertificateReportSender constructed by
ProfileIOData. Calls to CheckPublicKeyPins() indicate whether a report
should be sent and pass necessary reporting information as arguments.

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

BUG=445793
Committed: https://crrev.com/1a66df754cd86315086180e978298f453f8723e7
Cr-Commit-Position: refs/heads/master@{#340687}

[estark, 2015-06-26 19:28:47]

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

[estark, 2015-06-26 19:28:48]

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

[Ryan Sleevi, 2015-06-26 20:08:57]

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

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

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

https://codereview.chromium.org/1212613004/diff/1/net/http/transport_security...
net/http/transport_security_state.h:13: #include "base/basictypes.h"
This should probably be <stdint.h> (noticed when you added uint16_t below)

https://codereview.chromium.org/1212613004/diff/1/net/http/transport_security...
net/http/transport_security_state.h:172: virtual ~Reporter() {}
Is ownership of the reporter transferred?

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

https://codereview.chromium.org/1212613004/diff/1/net/http/transport_security...
net/http/transport_security_state.h:178: GURL* report_uri) = 0;
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.

https://codereview.chromium.org/1212613004/diff/1/net/http/transport_security...
net/http/transport_security_state.h:188: const std::string& effective_hostname,
Should provide a bit of documentation about these parameters - in particular,
this one is a little tricksy

https://codereview.chromium.org/1212613004/diff/1/net/http/transport_security...
net/http/transport_security_state.h:190: const scoped_refptr<X509Certificate>&
validated_certificate_chain,
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.

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

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

https://codereview.chromium.org/1212613004/diff/1/net/http/transport_security...
net/http/transport_security_state.h:178: GURL* report_uri) = 0;
On 2015/06/26 20:08:58, Ryan Sleevi (slow through 7-15 wrote:
> Also, note that there may be multiple report URIs - another thing to consider
> with this interface.

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 20:53:24]

Patchset #2 (id:20001) has been deleted

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

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

https://codereview.chromium.org/1212613004/diff/1/net/http/transport_security...
net/http/transport_security_state.h:13: #include "base/basictypes.h"
On 2015/06/26 20:08:58, Ryan Sleevi (slow through 7-15 wrote:
> This should probably be <stdint.h> (noticed when you added uint16_t below)

Done.

https://codereview.chromium.org/1212613004/diff/1/net/http/transport_security...
net/http/transport_security_state.h:172: virtual ~Reporter() {}
On 2015/06/26 20:08:58, Ryan Sleevi (slow through 7-15 wrote:
> Is ownership of the reporter transferred?
> 
> If not, this should be made protected - that way implementations can be made
> using refcounting (if needed)

Done.

https://codereview.chromium.org/1212613004/diff/1/net/http/transport_security...
net/http/transport_security_state.h:178: GURL* report_uri) = 0;
On 2015/06/26 20:08:58, Ryan Sleevi (slow through 7-15 wrote:
> 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.

Yes, that's the reason. Done.

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

https://codereview.chromium.org/1212613004/diff/1/net/http/transport_security...
net/http/transport_security_state.h:178: GURL* report_uri) = 0;
On 2015/06/26 20:17:15, Ryan Sleevi (slow through 7-15 wrote:
> On 2015/06/26 20:08:58, Ryan Sleevi (slow through 7-15 wrote:
> > Also, note that there may be multiple report URIs - another thing to
consider
> > with this interface.
> 
> 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. 

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).

https://codereview.chromium.org/1212613004/diff/1/net/http/transport_security...
net/http/transport_security_state.h:188: const std::string& effective_hostname,
On 2015/06/26 20:08:58, Ryan Sleevi (slow through 7-15 wrote:
> Should provide a bit of documentation about these parameters - in particular,
> this one is a little tricksy

Done.

https://codereview.chromium.org/1212613004/diff/1/net/http/transport_security...
net/http/transport_security_state.h:190: const scoped_refptr<X509Certificate>&
validated_certificate_chain,
On 2015/06/26 20:08:58, Ryan Sleevi (slow through 7-15 wrote:
> 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.

Done.

[davidben, 2015-07-15 23:38:36]

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

https://codereview.chromium.org/1212613004/diff/60001/net/http/transport_secu...
net/http/transport_security_state.h:188: // |report_uri|. Returns false if a
report should not be sent.
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?

https://codereview.chromium.org/1212613004/diff/60001/net/http/transport_secu...
net/http/transport_security_state.h:220: std::string* serialized_report) = 0;
Can this not be in TransportSecurityState? There's just the one HPKP format,
right?

https://codereview.chromium.org/1212613004/diff/60001/net/http/transport_secu...
net/http/transport_security_state.h:224: const std::string& report) = 0;
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:37]

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

https://codereview.chromium.org/1212613004/diff/60001/net/http/transport_secu...
net/http/transport_security_state.h:188: // |report_uri|. Returns false if a
report should not be sent.
On 2015/07/15 23:38:35, David Benjamin wrote:
> 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?

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.

https://codereview.chromium.org/1212613004/diff/60001/net/http/transport_secu...
net/http/transport_security_state.h:220: std::string* serialized_report) = 0;
On 2015/07/15 23:38:35, David Benjamin wrote:
> Can this not be in TransportSecurityState? There's just the one HPKP format,
> right?

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

https://codereview.chromium.org/1212613004/diff/60001/net/http/transport_secu...
net/http/transport_security_state.h:224: const std::string& report) = 0;
On 2015/07/15 23:38:35, David Benjamin wrote:
> 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.

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:54]

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

https://codereview.chromium.org/1212613004/diff/60001/net/http/transport_secu...
net/http/transport_security_state.h:188: // |report_uri|. Returns false if a
report should not be sent.
On 2015/07/15 23:51:36, estark wrote:
> On 2015/07/15 23:38:35, David Benjamin wrote:
> > 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?
> 
> 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.

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?

https://codereview.chromium.org/1212613004/diff/60001/net/http/transport_secu...
net/http/transport_security_state.h:224: const std::string& report) = 0;
On 2015/07/15 23:51:36, estark wrote:
> On 2015/07/15 23:38:35, David Benjamin wrote:
> > 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.
> 
> 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?

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.)

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

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

https://codereview.chromium.org/1212613004/diff/60001/net/http/transport_secu...
net/http/transport_security_state.h:188: // |report_uri|. Returns false if a
report should not be sent.
On 2015/07/16 00:22:53, David Benjamin wrote:
> On 2015/07/15 23:51:36, estark wrote:
> > On 2015/07/15 23:38:35, David Benjamin wrote:
> > > 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?
> > 
> > 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.
> 
> 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.

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.)

> 
> 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.

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.)

> 
> 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.

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).

> 
> 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?

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

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

https://codereview.chromium.org/1212613004/diff/60001/net/http/transport_secu...
net/http/transport_security_state.h:188: // |report_uri|. Returns false if a
report should not be sent.
On 2015/07/16 01:41:13, estark wrote:
> On 2015/07/16 00:22:53, David Benjamin wrote:
> > On 2015/07/15 23:51:36, estark wrote:
> > > On 2015/07/15 23:38:35, David Benjamin wrote:
> > > > 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?
> > > 
> > > 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.
> > 
> > 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.
> 
> 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.

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.)

> (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.)

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.

> > 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.
> 
> 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.)

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.)

> > 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.
> 
> 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).

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. :-)

> > 
> > 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-17 21:33:14]

Chatted with David a bit more out-of-band. Decided to go with a
GetHPKPReport()/SendHPKPReport() for TransportSecurityState::Reporter for now,
with an open issue to make the server accept JSON-format reports, at which point
we can simplify by getting rid of GetHPKPReport() and preloading the report URIs
for Google properties. So that's what's in patch set #6; PTAL whenever you have
the time.

[davidben, 2015-07-22 21:36:43]

I'm still a little sad we can't fold TransportSecurity::Reporter and
CertificateReportSender into one interface, but I don't see a clear way around
having two interfaces somewhere because of the old format. We can perhaps fold
them together

https://codereview.chromium.org/1212613004/diff/120001/net/http/transport_sec...
File net/http/transport_security_reporter.cc (right):

https://codereview.chromium.org/1212613004/diff/120001/net/http/transport_sec...
net/http/transport_security_reporter.cc:22: return
scoped_ptr<base::ListValue>(new base::ListValue());
You can also write
  return make_scoped_ptr(new base::ListValue());
which is a little shorter.

https://codereview.chromium.org/1212613004/diff/120001/net/http/transport_sec...
net/http/transport_security_reporter.cc:27: for (std::string cert :
pem_encoded_chain)
std::string -> const std::string&

https://codereview.chromium.org/1212613004/diff/120001/net/http/transport_sec...
net/http/transport_security_reporter.cc:28:
result->Append(scoped_ptr<base::Value>(new base::StringValue(cert)));
I think make_scoped_ptr will work here too.

https://codereview.chromium.org/1212613004/diff/120001/net/http/transport_sec...
net/http/transport_security_reporter.cc:65: report.SetString("date-time",
base::Int64ToString(now.ToInternalValue()));
Here, you can just nab this code:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/dow...

(Otherwise, this is enough of spec deviation that we probably should have a bug
on file and not ship without it. Would be unfortunate if the world needs to
parse dates in different ways. Plus our ToInternalValue uses the Windows epoch,
so that'll confuse everyone.)

https://codereview.chromium.org/1212613004/diff/120001/net/http/transport_sec...
net/http/transport_security_reporter.cc:93: NOTREACHED();
Probably also want a
  return false;

(Or perhaps we should just get rid of the HASH_VALUE_TAGS_COUNT. I'm not sure
why it's there; it's never used. Then you could omit the default case and clang
will yell if we miss one...)

https://codereview.chromium.org/1212613004/diff/120001/net/http/transport_sec...
File net/http/transport_security_state.h (right):

https://codereview.chromium.org/1212613004/diff/120001/net/http/transport_sec...
net/http/transport_security_state.h:202: uint16_t port,
hostname and port can be folded together to a HostPortPair. Which, conveniently,
is the structure that SSLClientSocket uses internally.

[davidben, 2015-07-22 21:37:26]

On 2015/07/22 21:36:43, David Benjamin wrote:
> I'm still a little sad we can't fold TransportSecurity::Reporter and
> CertificateReportSender into one interface, but I don't see a clear way around
> having two interfaces somewhere because of the old format. We can perhaps fold
> them together

Finishing my sentence, we can perhaps fold them together after the old format is
gone. (I'm thinking the JSON building code would just be a utility function that
TransportSecurityState calls rather than something it has an interface for.)

[davidben, 2015-07-23 00:03:12]

Hrm, silly question: the current old format is
ChromeFradulentCertificateReporter, right? Any reason not to just leave the old
format via that code and make this new codepath only do HPKP stuff. Then, when
the old one can fold into the standard format on the server, just delete the
ChromeFradulentCertificateReporter without touching this codepath any.

[estark, 2015-07-23 00:03:57]

https://codereview.chromium.org/1212613004/diff/120001/net/http/transport_sec...
File net/http/transport_security_reporter.cc (right):

https://codereview.chromium.org/1212613004/diff/120001/net/http/transport_sec...
net/http/transport_security_reporter.cc:22: return
scoped_ptr<base::ListValue>(new base::ListValue());
On 2015/07/22 21:36:42, David Benjamin wrote:
> You can also write
>   return make_scoped_ptr(new base::ListValue());
> which is a little shorter.

Done.

https://codereview.chromium.org/1212613004/diff/120001/net/http/transport_sec...
net/http/transport_security_reporter.cc:27: for (std::string cert :
pem_encoded_chain)
On 2015/07/22 21:36:42, David Benjamin wrote:
> std::string -> const std::string&

Done.

https://codereview.chromium.org/1212613004/diff/120001/net/http/transport_sec...
net/http/transport_security_reporter.cc:28:
result->Append(scoped_ptr<base::Value>(new base::StringValue(cert)));
On 2015/07/22 21:36:42, David Benjamin wrote:
> I think make_scoped_ptr will work here too.

Done.

https://codereview.chromium.org/1212613004/diff/120001/net/http/transport_sec...
net/http/transport_security_reporter.cc:65: report.SetString("date-time",
base::Int64ToString(now.ToInternalValue()));
On 2015/07/22 21:36:42, David Benjamin wrote:
> Here, you can just nab this code:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/dow...
> 
> (Otherwise, this is enough of spec deviation that we probably should have a
bug
> on file and not ship without it. Would be unfortunate if the world needs to
> parse dates in different ways. Plus our ToInternalValue uses the Windows
epoch,
> so that'll confuse everyone.)

Done. (I just copied and pasted it -- not sure if that's what you meant by nab
it. I could maybe add it to base/time as a utility function?)

https://codereview.chromium.org/1212613004/diff/120001/net/http/transport_sec...
net/http/transport_security_reporter.cc:93: NOTREACHED();
On 2015/07/22 21:36:43, David Benjamin wrote:
> Probably also want a
>   return false;
> 
> (Or perhaps we should just get rid of the HASH_VALUE_TAGS_COUNT. I'm not sure
> why it's there; it's never used. Then you could omit the default case and
clang
> will yell if we miss one...)

Done (the latter).

https://codereview.chromium.org/1212613004/diff/120001/net/http/transport_sec...
File net/http/transport_security_state.h (right):

https://codereview.chromium.org/1212613004/diff/120001/net/http/transport_sec...
net/http/transport_security_state.h:202: uint16_t port,
On 2015/07/22 21:36:43, David Benjamin wrote:
> hostname and port can be folded together to a HostPortPair. Which,
conveniently,
> is the structure that SSLClientSocket uses internally.

Done.

[estark, 2015-07-23 00:12:50]

On 2015/07/22 21:37:26, David Benjamin wrote:
> On 2015/07/22 21:36:43, David Benjamin wrote:
> > I'm still a little sad we can't fold TransportSecurity::Reporter and
> > CertificateReportSender into one interface, but I don't see a clear way
around
> > having two interfaces somewhere because of the old format. We can perhaps
fold
> > them together
> 
> Finishing my sentence, we can perhaps fold them together after the old format
is
> gone. (I'm thinking the JSON building code would just be a utility function
that
> TransportSecurityState calls rather than something it has an interface for.)

Yep, that was my plan (land this, convert the server to accept JSON, remove the
TransportSecurityReporter class.)


On 2015/07/23 00:03:12, David Benjamin wrote:
> Hrm, silly question: the current old format is
> ChromeFradulentCertificateReporter, right? Any reason not to just leave the
old
> format via that code and make this new codepath only do HPKP stuff. Then, when
> the old one can fold into the standard format on the server, just delete the
> ChromeFradulentCertificateReporter without touching this codepath any.

That would work logistically, but the downsides I see are: 1. The
FraudulentCertificateReporter interface is a tad objectionable, so getting rid
of it sooner rather than later is actually desirable; see crbug.com/512461; 2.
I'm not sure I'm convinced it's actually it's any better -- we'll still have two
interfaces in //net (FraudulentCertificateReporter and CertificateReportSender)
so the only improvement is that one of them isn't stapled to
TransportSecurityReporter; and 3. With FraudulentCertificateReporter as-is,
we'll have send-pinning-violation-report checks in two different places (one at
the URLRequest level and one at the socket level), which seems a little weird to
me.

I don't feel too strongly either way, but that's my thinking.

[davidben, 2015-07-23 00:16:02]

On 2015/07/23 00:12:50, estark wrote:
> 
> On 2015/07/23 00:03:12, David Benjamin wrote:
> > Hrm, silly question: the current old format is
> > ChromeFradulentCertificateReporter, right? Any reason not to just leave the
> old
> > format via that code and make this new codepath only do HPKP stuff. Then,
when
> > the old one can fold into the standard format on the server, just delete the
> > ChromeFradulentCertificateReporter without touching this codepath any.
> 
> That would work logistically, but the downsides I see are: 1. The
> FraudulentCertificateReporter interface is a tad objectionable, so getting rid
> of it sooner rather than later is actually desirable; see crbug.com/512461; 2.
> I'm not sure I'm convinced it's actually it's any better -- we'll still have
two
> interfaces in //net (FraudulentCertificateReporter and
CertificateReportSender)
> so the only improvement is that one of them isn't stapled to
> TransportSecurityReporter; and 3. With FraudulentCertificateReporter as-is,
> we'll have send-pinning-violation-report checks in two different places (one
at
> the URLRequest level and one at the socket level), which seems a little weird
to
> me.
> 
> I don't feel too strongly either way, but that's my thinking.

I don't hugely care either. Just seemed it'd be less effort to build the HPKP
path as we want it and not bother touching FraudulentCertificateReporter until
it's time to just remove it, rather than introduce a temporary interface and
then fold the interface away when the server's switched over. Eh, your call. I
suppose you've already written the extra interface version.

[estark, 2015-07-23 08:52:57]

On 2015/07/23 00:16:02, David Benjamin wrote:
> On 2015/07/23 00:12:50, estark wrote:
> > 
> > On 2015/07/23 00:03:12, David Benjamin wrote:
> > > Hrm, silly question: the current old format is
> > > ChromeFradulentCertificateReporter, right? Any reason not to just leave
the
> > old
> > > format via that code and make this new codepath only do HPKP stuff. Then,
> when
> > > the old one can fold into the standard format on the server, just delete
the
> > > ChromeFradulentCertificateReporter without touching this codepath any.
> > 
> > That would work logistically, but the downsides I see are: 1. The
> > FraudulentCertificateReporter interface is a tad objectionable, so getting
rid
> > of it sooner rather than later is actually desirable; see crbug.com/512461;
2.
> > I'm not sure I'm convinced it's actually it's any better -- we'll still have
> two
> > interfaces in //net (FraudulentCertificateReporter and
> CertificateReportSender)
> > so the only improvement is that one of them isn't stapled to
> > TransportSecurityReporter; and 3. With FraudulentCertificateReporter as-is,
> > we'll have send-pinning-violation-report checks in two different places (one
> at
> > the URLRequest level and one at the socket level), which seems a little
weird
> to
> > me.
> > 
> > I don't feel too strongly either way, but that's my thinking.
> 
> I don't hugely care either. Just seemed it'd be less effort to build the HPKP
> path as we want it and not bother touching FraudulentCertificateReporter until
> it's time to just remove it, rather than introduce a temporary interface and
> then fold the interface away when the server's switched over. Eh, your call. I
> suppose you've already written the extra interface version.

Ok, I thought on this a little more and I think you're right. Even though the
extra interface is already done, it'll still be strictly more work from here on
out (because I would still have to implement ChromeTransportSecurityReporter
which I haven't done yet). Also I looked at the server code and am cautiously
optimistic that I can add JSON support without too much trouble (famous last
words...) so hopefully I won't be prolonging FraudulentCertificateReporter's
lifespan by too much.

So, I updated these CLs to combine CertificateReportSender and
CertificateReportSenderImpl into a single CertificateReportSender class that
implements TransportSecurityState::ReportSender (which just has a single Send()
method). Deleted TransportSecurityReporter and moved the report-building code
into a helper GetHPKPReport() function in TransportSecurityState.

The CLs got restructured a little bit so here's the summary:
- 1211363005 is the parsing one that got reverted and that I'm still debugging
- 1212973002 adds CertificateReportSender
- 1212613004 (this one) adds report building code to TransportSecurityState and
wires everything up to send reports when CheckPublicKeyPins() is called

[estark, 2015-07-23 08:53:29]

https://codereview.chromium.org/1212613004/diff/180001/net/http/transport_sec...
File net/http/transport_security_state_unittest.cc (right):

https://codereview.chromium.org/1212613004/diff/180001/net/http/transport_sec...
net/http/transport_security_state_unittest.cc:43: const char kReportUri[] =
"http://example.test/test";
Is everything supposed to be in the net namespace for tests, or just the actual
tests and fixtures? (Should constants and helper functions be in an anonymous
namespace?)

[davidben, 2015-07-24 20:42:56]

This is basically entirely nits.

https://codereview.chromium.org/1212613004/diff/180001/chrome/browser/profile...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/1212613004/diff/180001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.h:71: class CertificateReportSender;
Nit: Alphabetize.

https://codereview.chromium.org/1212613004/diff/180001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.h:561: mutable
scoped_ptr<net::CertificateReportSender> certificate_report_sender_;
This is really obnoxious, but there's a window where certificate_report_sender_
is alive and main_request_context_ isn't. Which actually can trip
AssertNoURLRequests if there are any live reports in flight on shutdown. And the
CertificateReportSender cannot outlive main_request_context_. (At the same time,
transport_security_state_ can't have a pointer to certificate_report_sender_
after it's been destroyed.)

Really the problem is you have a cycle. So you'll need some code in some
destructor to break one of the pointers, probably TSS -> CRS. (Note that
TransportSecurityPersister's destructor calls SetDelegate(NULL) which is why it
can outlive TransportSecurityState.)

Probably just putting something in ~ProfileIOData to disconnect it and either
certificate_report_sender_.reset() early or place it underneath
main_request_context_ so it's destroyed first.

https://codereview.chromium.org/1212613004/diff/180001/net/http/http_security...
File net/http/http_security_headers_unittest.cc (right):

https://codereview.chromium.org/1212613004/diff/180001/net/http/http_security...
net/http/http_security_headers_unittest.cc:682: HostPortPair domain_port(domain,
80);
Nit: 443 might be a slightly more realistic port number than 80. :-) Ditto
below.

https://codereview.chromium.org/1212613004/diff/180001/net/http/transport_sec...
File net/http/transport_security_state.cc (right):

https://codereview.chromium.org/1212613004/diff/180001/net/http/transport_sec...
net/http/transport_security_state.cc:59: exploded.millisecond);
[Verified RFC 3339 allows fractional seconds. I didn't see any language which
suggested we need to strip trailing zeros or anything obnoxious like that.]

https://codereview.chromium.org/1212613004/diff/180001/net/http/transport_sec...
net/http/transport_security_state.cc:82: // break loops
You should probably talk to ttuttle@. Domain Reliability has similar needs (send
reports somewhere, mumble mumble rate-limiting mumble mumble).

(The loops thing is interesting. That hadn't occurred to me. Yuck. :-/)

https://codereview.chromium.org/1212613004/diff/180001/net/http/transport_sec...
net/http/transport_security_state.cc:926: // to have been called, so if we fall
through to here, it's an error.
[This was there before, so no need to do anything for this CL, but I do have to
wonder why the Foo/FooImpl split. Seems we could just switch this one to return
true and remove the HasPublicKeyPins check...]

https://codereview.chromium.org/1212613004/diff/180001/net/http/transport_sec...
net/http/transport_security_state.cc:930: bool passed_pin_check =
pkp_state.CheckPublicKeyPins(hashes, failure_log);
This might be slightly clearer as

 if (pkp_state.Check(...))
   return true;

 if (!report_sender_ || .....)
   return false;

 [Code to send report.]
 return false;

Or maybe some other restatement. Perhaps

  if (!pkp_state.CheckPublic....) {
    if (stuff) {
      Send pin report
    }
    return false;
  }

  return true;

Though that one is more indenting. Dunno.

Mostly just that, at first glance, it looked like you might send a report for
successful pin checks too, but passed_pin_check is always false after the
condition.

https://codereview.chromium.org/1212613004/diff/180001/net/http/transport_sec...
net/http/transport_security_state.cc:934: return passed_pin_check;
Nit: IIRC, multi-line if's get curlies.

https://codereview.chromium.org/1212613004/diff/180001/net/http/transport_sec...
File net/http/transport_security_state_unittest.cc (right):

https://codereview.chromium.org/1212613004/diff/180001/net/http/transport_sec...
net/http/transport_security_state_unittest.cc:43: const char kReportUri[] =
"http://example.test/test";
On 2015/07/23 08:53:29, estark wrote:
> Is everything supposed to be in the net namespace for tests, or just the
actual
> tests and fixtures? (Should constants and helper functions be in an anonymous
> namespace?)

See my second comment here:
https://codereview.chromium.org/1212973002/diff/180001/net/url_request/certif...

https://codereview.chromium.org/1212613004/diff/180001/net/http/transport_sec...
net/http/transport_security_state_unittest.cc:97:
static_cast<base::DictionaryValue*>(value.release()));
You can also do:
  base::DictionaryValue* report_dict;
  ASSERT_TRUE(value->GetAsDictionary(&report_dict));

And then use report_dict as a raw pointer.

https://codereview.chromium.org/1212613004/diff/180001/net/http/transport_sec...
net/http/transport_security_state_unittest.cc:1201: const uint16_t kPort = 443;
Nit: static const

https://codereview.chromium.org/1212613004/diff/180001/net/http/transport_sec...
net/http/transport_security_state_unittest.cc:1242: const base::Time
current_time(base::Time::Now());
Nit: I'd probably just use equals here.

https://codereview.chromium.org/1212613004/diff/180001/net/http/transport_sec...
net/http/transport_security_state_unittest.cc:1254: // No report should have
been sent because of the DO_NOT_SEND_REPORT
DO_NOT_SEND_REPORT -> DISABLE_PIN_REPORTS?

https://codereview.chromium.org/1212613004/diff/180001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/1212613004/diff/180001/net/spdy/spdy_session....
net/spdy/spdy_session.cc:607: ssl_info.cert.get(),
TransportSecurityState::DISABLE_PIN_REPORTS,
Should this send reports? I believe if we get this far, this is a certificate
that's otherwise valid for the name, which suggests yes.

I guess one problem is that CanPool will get queried every single time we
locally compare the session, so long as the other valid session is alive, and so
we might go crazy?

Anyway, if we don't want to send reports, worth a brief comment as to why not.

[estark, 2015-07-25 00:10:31]

https://codereview.chromium.org/1212613004/diff/180001/chrome/browser/profile...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/1212613004/diff/180001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.h:71: class CertificateReportSender;
On 2015/07/24 20:42:55, David Benjamin wrote:
> Nit: Alphabetize.

Done.

https://codereview.chromium.org/1212613004/diff/180001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.h:561: mutable
scoped_ptr<net::CertificateReportSender> certificate_report_sender_;
On 2015/07/24 20:42:55, David Benjamin wrote:
> This is really obnoxious, but there's a window where
certificate_report_sender_
> is alive and main_request_context_ isn't. Which actually can trip
> AssertNoURLRequests if there are any live reports in flight on shutdown. And
the
> CertificateReportSender cannot outlive main_request_context_. (At the same
time,
> transport_security_state_ can't have a pointer to certificate_report_sender_
> after it's been destroyed.)
> 
> Really the problem is you have a cycle. So you'll need some code in some
> destructor to break one of the pointers, probably TSS -> CRS. (Note that
> TransportSecurityPersister's destructor calls SetDelegate(NULL) which is why
it
> can outlive TransportSecurityState.)
> 
> Probably just putting something in ~ProfileIOData to disconnect it and either
> certificate_report_sender_.reset() early or place it underneath
> main_request_context_ so it's destroyed first.

Ah, yeah, mmenke and I were talking about this earlier this week on
crbug.com/512461. Done.

https://codereview.chromium.org/1212613004/diff/180001/net/http/http_security...
File net/http/http_security_headers_unittest.cc (right):

https://codereview.chromium.org/1212613004/diff/180001/net/http/http_security...
net/http/http_security_headers_unittest.cc:682: HostPortPair domain_port(domain,
80);
On 2015/07/24 20:42:55, David Benjamin wrote:
> Nit: 443 might be a slightly more realistic port number than 80. :-) Ditto
> below.

Done.

https://codereview.chromium.org/1212613004/diff/180001/net/http/transport_sec...
File net/http/transport_security_state.cc (right):

https://codereview.chromium.org/1212613004/diff/180001/net/http/transport_sec...
net/http/transport_security_state.cc:59: exploded.millisecond);
On 2015/07/24 20:42:55, David Benjamin wrote:
> [Verified RFC 3339 allows fractional seconds. I didn't see any language which
> suggested we need to strip trailing zeros or anything obnoxious like that.]

Acknowledged.

https://codereview.chromium.org/1212613004/diff/180001/net/http/transport_sec...
net/http/transport_security_state.cc:82: // break loops
On 2015/07/24 20:42:55, David Benjamin wrote:
> You should probably talk to ttuttle@. Domain Reliability has similar needs
(send
> reports somewhere, mumble mumble rate-limiting mumble mumble).
> 
> (The loops thing is interesting. That hadn't occurred to me. Yuck. :-/)

Acknowledged.

https://codereview.chromium.org/1212613004/diff/180001/net/http/transport_sec...
net/http/transport_security_state.cc:926: // to have been called, so if we fall
through to here, it's an error.
On 2015/07/24 20:42:55, David Benjamin wrote:
> [This was there before, so no need to do anything for this CL, but I do have
to
> wonder why the Foo/FooImpl split. Seems we could just switch this one to
return
> true and remove the HasPublicKeyPins check...]

Acknowledged.

https://codereview.chromium.org/1212613004/diff/180001/net/http/transport_sec...
net/http/transport_security_state.cc:930: bool passed_pin_check =
pkp_state.CheckPublicKeyPins(hashes, failure_log);
On 2015/07/24 20:42:55, David Benjamin wrote:
> This might be slightly clearer as
> 
>  if (pkp_state.Check(...))
>    return true;
> 
>  if (!report_sender_ || .....)
>    return false;
> 
>  [Code to send report.]
>  return false;
> 
> Or maybe some other restatement. Perhaps
> 
>   if (!pkp_state.CheckPublic....) {
>     if (stuff) {
>       Send pin report
>     }
>     return false;
>   }
> 
>   return true;
> 
> Though that one is more indenting. Dunno.
> 
> Mostly just that, at first glance, it looked like you might send a report for
> successful pin checks too, but passed_pin_check is always false after the
> condition.

Done.

https://codereview.chromium.org/1212613004/diff/180001/net/http/transport_sec...
net/http/transport_security_state.cc:934: return passed_pin_check;
On 2015/07/24 20:42:55, David Benjamin wrote:
> Nit: IIRC, multi-line if's get curlies.

Done.

https://codereview.chromium.org/1212613004/diff/180001/net/http/transport_sec...
File net/http/transport_security_state_unittest.cc (right):

https://codereview.chromium.org/1212613004/diff/180001/net/http/transport_sec...
net/http/transport_security_state_unittest.cc:43: const char kReportUri[] =
"http://example.test/test";
On 2015/07/24 20:42:55, David Benjamin wrote:
> On 2015/07/23 08:53:29, estark wrote:
> > Is everything supposed to be in the net namespace for tests, or just the
> actual
> > tests and fixtures? (Should constants and helper functions be in an
anonymous
> > namespace?)
> 
> See my second comment here:
>
https://codereview.chromium.org/1212973002/diff/180001/net/url_request/certif...

Done. I left the existing test fixtures and tests in net because I didn't feel
like going through and adding all the net:: qualifiers in this CL, but I can do
that in a follow-up if so desired.

https://codereview.chromium.org/1212613004/diff/180001/net/http/transport_sec...
net/http/transport_security_state_unittest.cc:97:
static_cast<base::DictionaryValue*>(value.release()));
On 2015/07/24 20:42:55, David Benjamin wrote:
> You can also do:
>   base::DictionaryValue* report_dict;
>   ASSERT_TRUE(value->GetAsDictionary(&report_dict));
> 
> And then use report_dict as a raw pointer.

Done.

https://codereview.chromium.org/1212613004/diff/180001/net/http/transport_sec...
net/http/transport_security_state_unittest.cc:1201: const uint16_t kPort = 443;
On 2015/07/24 20:42:55, David Benjamin wrote:
> Nit: static const

Done.

https://codereview.chromium.org/1212613004/diff/180001/net/http/transport_sec...
net/http/transport_security_state_unittest.cc:1242: const base::Time
current_time(base::Time::Now());
On 2015/07/24 20:42:55, David Benjamin wrote:
> Nit: I'd probably just use equals here.

Done.

https://codereview.chromium.org/1212613004/diff/180001/net/http/transport_sec...
net/http/transport_security_state_unittest.cc:1254: // No report should have
been sent because of the DO_NOT_SEND_REPORT
On 2015/07/24 20:42:55, David Benjamin wrote:
> DO_NOT_SEND_REPORT -> DISABLE_PIN_REPORTS?

Done.

https://codereview.chromium.org/1212613004/diff/180001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/1212613004/diff/180001/net/spdy/spdy_session....
net/spdy/spdy_session.cc:607: ssl_info.cert.get(),
TransportSecurityState::DISABLE_PIN_REPORTS,
On 2015/07/24 20:42:55, David Benjamin wrote:
> Should this send reports? I believe if we get this far, this is a certificate
> that's otherwise valid for the name, which suggests yes.
> 
> I guess one problem is that CanPool will get queried every single time we
> locally compare the session, so long as the other valid session is alive, and
so
> we might go crazy?

Yeah... just because the certificate is valid in every other way, I don't think
it means we should treat it as a violation. Ryan had an example (which I might
mangle in the retelling) about inbox.google.com having a cert that's valid for
inbox.g.com and mail.g.com, but mail.g.com having its own cert and pins. It
seems weird to send a report for mail.g.com just because the browser happened to
have an inbox.google.com connection lying around (especially because there's
nothing a site operator can or should do with such a report -- it's not a
misconfiguration, not an attacker, nothing to fix... it's just something that
can happen in completely normal operation).

> 
> Anyway, if we don't want to send reports, worth a brief comment as to why not.

Done.

[estark, 2015-07-25 00:11:27]

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

[estark, 2015-07-25 00:11:28]

mmenke, could you please review the ProfileIOData changes?

[davidben, 2015-07-25 01:48:58]

https://codereview.chromium.org/1212613004/diff/180001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/1212613004/diff/180001/net/spdy/spdy_session....
net/spdy/spdy_session.cc:607: ssl_info.cert.get(),
TransportSecurityState::DISABLE_PIN_REPORTS,
On 2015/07/25 00:10:31, estark wrote:
> On 2015/07/24 20:42:55, David Benjamin wrote:
> > Should this send reports? I believe if we get this far, this is a
certificate
> > that's otherwise valid for the name, which suggests yes.
> > 
> > I guess one problem is that CanPool will get queried every single time we
> > locally compare the session, so long as the other valid session is alive,
and
> so
> > we might go crazy?
> 
> Yeah... just because the certificate is valid in every other way, I don't
think
> it means we should treat it as a violation. Ryan had an example (which I might
> mangle in the retelling) about http://inbox.google.com having a cert that's
valid for
> http://inbox.g.com and http://mail.g.com, but http://mail.g.com having its own
cert and pins. It
> seems weird to send a report for http://mail.g.com just because the browser
happened to
> have an http://inbox.google.com connection lying around (especially because
there's
> nothing a site operator can or should do with such a report -- it's not a
> misconfiguration, not an attacker, nothing to fix... it's just something that
> can happen in completely normal operation).

Hrm. Okay, so it's specifically that the two certificates are owned by the same
entity but that one entity decided to pin on something else. That... seems sort
of a contrived situation, but I suppose it's possible. I could imagine it being
more likely if you have wildcards. It also seems a slightly weird deployment
strategy. There's no point in inbox.google.com and mail.google.com having
different sets of keys in this case; if inbox.google.com's key is compromised,
mail.google.com is still bust, pinning aside.

If the two certificates weren't owned by the same entity though, it's definitely
still a pinning violation. If the certificate is valid for mail.google.com and
evil.com, it can be used to speak for mail.google.com.

Actually, doesn't this mean that, if I have a bad certificate for example.com,
which sets a pin, I just need to ensure that bad certificate is also good for a
site I own and then, whenever I mount an attack, I make sure I have a SPDY
session to my domain open first? In fact, that's really easy. I make my server
shut off the connection if SNI = mail.google.com and only send the cert on SNI =
evil.com.

Then, though I'm still beaten by pinning (for users who have already observe the
pin), I at least won't get reported.

[davidben, 2015-07-25 01:49:45]

On 2015/07/25 01:48:58, David Benjamin wrote:
>
https://codereview.chromium.org/1212613004/diff/180001/net/spdy/spdy_session.cc
> File net/spdy/spdy_session.cc (right):
> 
>
https://codereview.chromium.org/1212613004/diff/180001/net/spdy/spdy_session....
> net/spdy/spdy_session.cc:607: ssl_info.cert.get(),
> TransportSecurityState::DISABLE_PIN_REPORTS,
> On 2015/07/25 00:10:31, estark wrote:
> > On 2015/07/24 20:42:55, David Benjamin wrote:
> > > Should this send reports? I believe if we get this far, this is a
> certificate
> > > that's otherwise valid for the name, which suggests yes.
> > > 
> > > I guess one problem is that CanPool will get queried every single time we
> > > locally compare the session, so long as the other valid session is alive,
> and
> > so
> > > we might go crazy?
> > 
> > Yeah... just because the certificate is valid in every other way, I don't
> think
> > it means we should treat it as a violation. Ryan had an example (which I
might
> > mangle in the retelling) about http://inbox.google.com having a cert that's
> valid for
> > http://inbox.g.com and http://mail.g.com, but http://mail.g.com having its
own
> cert and pins. It
> > seems weird to send a report for http://mail.g.com just because the browser
> happened to
> > have an http://inbox.google.com connection lying around (especially because
> there's
> > nothing a site operator can or should do with such a report -- it's not a
> > misconfiguration, not an attacker, nothing to fix... it's just something
that
> > can happen in completely normal operation).
> 
> Hrm. Okay, so it's specifically that the two certificates are owned by the
same
> entity but that one entity decided to pin on something else. That... seems
sort
> of a contrived situation, but I suppose it's possible. I could imagine it
being
> more likely if you have wildcards. It also seems a slightly weird deployment
> strategy. There's no point in http://inbox.google.com and
http://mail.google.com having
> different sets of keys in this case; if inbox.google.com's key is compromised,
> http://mail.google.com is still bust, pinning aside.
> 
> If the two certificates weren't owned by the same entity though, it's
definitely
> still a pinning violation. If the certificate is valid for
http://mail.google.com and
> http://evil.com, it can be used to speak for http://mail.google.com.
> 
> Actually, doesn't this mean that, if I have a bad certificate for
http://example.com,

s/example.com/mail.google.com/. I apparently switched examples partway through.

> which sets a pin, I just need to ensure that bad certificate is also good for
a
> site I own and then, whenever I mount an attack, I make sure I have a SPDY
> session to my domain open first? In fact, that's really easy. I make my server
> shut off the connection if SNI = http://mail.google.com and only send the cert
on SNI =
> http://evil.com.
> 
> Then, though I'm still beaten by pinning (for users who have already observe
the
> pin), I at least won't get reported.

[estark, 2015-07-25 06:22:33]

On 2015/07/25 01:49:45, David Benjamin wrote:
> On 2015/07/25 01:48:58, David Benjamin wrote:
> >
>
https://codereview.chromium.org/1212613004/diff/180001/net/spdy/spdy_session.cc
> > File net/spdy/spdy_session.cc (right):
> > 
> >
>
https://codereview.chromium.org/1212613004/diff/180001/net/spdy/spdy_session....
> > net/spdy/spdy_session.cc:607: ssl_info.cert.get(),
> > TransportSecurityState::DISABLE_PIN_REPORTS,
> > On 2015/07/25 00:10:31, estark wrote:
> > > On 2015/07/24 20:42:55, David Benjamin wrote:
> > > > Should this send reports? I believe if we get this far, this is a
> > certificate
> > > > that's otherwise valid for the name, which suggests yes.
> > > > 
> > > > I guess one problem is that CanPool will get queried every single time
we
> > > > locally compare the session, so long as the other valid session is
alive,
> > and
> > > so
> > > > we might go crazy?
> > > 
> > > Yeah... just because the certificate is valid in every other way, I don't
> > think
> > > it means we should treat it as a violation. Ryan had an example (which I
> might
> > > mangle in the retelling) about http://inbox.google.com having a cert
that's
> > valid for
> > > http://inbox.g.com and http://mail.g.com, but http://mail.g.com having its
> own
> > cert and pins. It
> > > seems weird to send a report for http://mail.g.com just because the
browser
> > happened to
> > > have an http://inbox.google.com connection lying around (especially
because
> > there's
> > > nothing a site operator can or should do with such a report -- it's not a
> > > misconfiguration, not an attacker, nothing to fix... it's just something
> that
> > > can happen in completely normal operation).
> > 
> > Hrm. Okay, so it's specifically that the two certificates are owned by the
> same
> > entity but that one entity decided to pin on something else. That... seems
> sort
> > of a contrived situation, but I suppose it's possible. I could imagine it
> being
> > more likely if you have wildcards. It also seems a slightly weird deployment
> > strategy. There's no point in http://inbox.google.com and
> http://mail.google.com having
> > different sets of keys in this case; if inbox.google.com's key is
compromised,
> > http://mail.google.com is still bust, pinning aside.
> > 
> > If the two certificates weren't owned by the same entity though, it's
> definitely
> > still a pinning violation. If the certificate is valid for
> http://mail.google.com and
> > http://evil.com, it can be used to speak for http://mail.google.com.
> > 
> > Actually, doesn't this mean that, if I have a bad certificate for
> http://example.com,
> 
> s/example.com/mail.google.com/. I apparently switched examples partway
through.
> 
> > which sets a pin, I just need to ensure that bad certificate is also good
for
> a
> > site I own and then, whenever I mount an attack, I make sure I have a SPDY
> > session to my domain open first? In fact, that's really easy. I make my
server
> > shut off the connection if SNI = http://mail.google.com and only send the
cert
> on SNI =
> > http://evil.com.
> > 
> > Then, though I'm still beaten by pinning (for users who have already observe
> the
> > pin), I at least won't get reported.

But if your goal is to not get reported, wouldn't you just block the report from
being sent? I guess the browser could retry over long periods of time if a
report wasn't sent successfully, which could make it theoretically harder for an
attacker to truly block a report from ever getting sent, but I think,
fundamentally, reporting is designed for detecting and debugging
misconfigurations, not for preventing attacks.

[mmenke, 2015-07-27 15:36:09]

On 2015/07/25 06:22:33, estark wrote:
> On 2015/07/25 01:49:45, David Benjamin wrote:
> > On 2015/07/25 01:48:58, David Benjamin wrote:
> > >
> >
>
https://codereview.chromium.org/1212613004/diff/180001/net/spdy/spdy_session.cc
> > > File net/spdy/spdy_session.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1212613004/diff/180001/net/spdy/spdy_session....
> > > net/spdy/spdy_session.cc:607: ssl_info.cert.get(),
> > > TransportSecurityState::DISABLE_PIN_REPORTS,
> > > On 2015/07/25 00:10:31, estark wrote:
> > > > On 2015/07/24 20:42:55, David Benjamin wrote:
> > > > > Should this send reports? I believe if we get this far, this is a
> > > certificate
> > > > > that's otherwise valid for the name, which suggests yes.
> > > > > 
> > > > > I guess one problem is that CanPool will get queried every single time
> we
> > > > > locally compare the session, so long as the other valid session is
> alive,
> > > and
> > > > so
> > > > > we might go crazy?
> > > > 
> > > > Yeah... just because the certificate is valid in every other way, I
don't
> > > think
> > > > it means we should treat it as a violation. Ryan had an example (which I
> > might
> > > > mangle in the retelling) about http://inbox.google.com having a cert
> that's
> > > valid for
> > > > http://inbox.g.com and http://mail.g.com, but http://mail.g.com having
its
> > own
> > > cert and pins. It
> > > > seems weird to send a report for http://mail.g.com just because the
> browser
> > > happened to
> > > > have an http://inbox.google.com connection lying around (especially
> because
> > > there's
> > > > nothing a site operator can or should do with such a report -- it's not
a
> > > > misconfiguration, not an attacker, nothing to fix... it's just something
> > that
> > > > can happen in completely normal operation).
> > > 
> > > Hrm. Okay, so it's specifically that the two certificates are owned by the
> > same
> > > entity but that one entity decided to pin on something else. That... seems
> > sort
> > > of a contrived situation, but I suppose it's possible. I could imagine it
> > being
> > > more likely if you have wildcards. It also seems a slightly weird
deployment
> > > strategy. There's no point in http://inbox.google.com and
> > http://mail.google.com having
> > > different sets of keys in this case; if inbox.google.com's key is
> compromised,
> > > http://mail.google.com is still bust, pinning aside.
> > > 
> > > If the two certificates weren't owned by the same entity though, it's
> > definitely
> > > still a pinning violation. If the certificate is valid for
> > http://mail.google.com and
> > > http://evil.com, it can be used to speak for http://mail.google.com.
> > > 
> > > Actually, doesn't this mean that, if I have a bad certificate for
> > http://example.com,
> > 
> > s/example.com/mail.google.com/. I apparently switched examples partway
> through.
> > 
> > > which sets a pin, I just need to ensure that bad certificate is also good
> for
> > a
> > > site I own and then, whenever I mount an attack, I make sure I have a SPDY
> > > session to my domain open first? In fact, that's really easy. I make my
> server
> > > shut off the connection if SNI = http://mail.google.com and only send the
> cert
> > on SNI =
> > > http://evil.com.
> > > 
> > > Then, though I'm still beaten by pinning (for users who have already
observe
> > the
> > > pin), I at least won't get reported.
> 
> But if your goal is to not get reported, wouldn't you just block the report
from
> being sent? I guess the browser could retry over long periods of time if a
> report wasn't sent successfully, which could make it theoretically harder for
an
> attacker to truly block a report from ever getting sent, but I think,
> fundamentally, reporting is designed for detecting and debugging
> misconfigurations, not for preventing attacks.

profiles/ LGTM.

[davidben, 2015-07-27 17:08:55]

On 2015/07/25 06:22:33, estark wrote:
> On 2015/07/25 01:49:45, David Benjamin wrote:
> > On 2015/07/25 01:48:58, David Benjamin wrote:
> > >
> >
>
https://codereview.chromium.org/1212613004/diff/180001/net/spdy/spdy_session.cc
> > > File net/spdy/spdy_session.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1212613004/diff/180001/net/spdy/spdy_session....
> > > net/spdy/spdy_session.cc:607: ssl_info.cert.get(),
> > > TransportSecurityState::DISABLE_PIN_REPORTS,
> > > On 2015/07/25 00:10:31, estark wrote:
> > > > On 2015/07/24 20:42:55, David Benjamin wrote:
> > > > > Should this send reports? I believe if we get this far, this is a
> > > certificate
> > > > > that's otherwise valid for the name, which suggests yes.
> > > > > 
> > > > > I guess one problem is that CanPool will get queried every single time
> we
> > > > > locally compare the session, so long as the other valid session is
> alive,
> > > and
> > > > so
> > > > > we might go crazy?
> > > > 
> > > > Yeah... just because the certificate is valid in every other way, I
don't
> > > think
> > > > it means we should treat it as a violation. Ryan had an example (which I
> > might
> > > > mangle in the retelling) about http://inbox.google.com having a cert
> that's
> > > valid for
> > > > http://inbox.g.com and http://mail.g.com, but http://mail.g.com having
its
> > own
> > > cert and pins. It
> > > > seems weird to send a report for http://mail.g.com just because the
> browser
> > > happened to
> > > > have an http://inbox.google.com connection lying around (especially
> because
> > > there's
> > > > nothing a site operator can or should do with such a report -- it's not
a
> > > > misconfiguration, not an attacker, nothing to fix... it's just something
> > that
> > > > can happen in completely normal operation).
> > > 
> > > Hrm. Okay, so it's specifically that the two certificates are owned by the
> > same
> > > entity but that one entity decided to pin on something else. That... seems
> > sort
> > > of a contrived situation, but I suppose it's possible. I could imagine it
> > being
> > > more likely if you have wildcards. It also seems a slightly weird
deployment
> > > strategy. There's no point in http://inbox.google.com and
> > http://mail.google.com having
> > > different sets of keys in this case; if inbox.google.com's key is
> compromised,
> > > http://mail.google.com is still bust, pinning aside.
> > > 
> > > If the two certificates weren't owned by the same entity though, it's
> > definitely
> > > still a pinning violation. If the certificate is valid for
> > http://mail.google.com and
> > > http://evil.com, it can be used to speak for http://mail.google.com.
> > > 
> > > Actually, doesn't this mean that, if I have a bad certificate for
> > http://example.com,
> > 
> > s/example.com/mail.google.com/. I apparently switched examples partway
> through.
> > 
> > > which sets a pin, I just need to ensure that bad certificate is also good
> for
> > a
> > > site I own and then, whenever I mount an attack, I make sure I have a SPDY
> > > session to my domain open first? In fact, that's really easy. I make my
> server
> > > shut off the connection if SNI = http://mail.google.com and only send the
> cert
> > on SNI =
> > > http://evil.com.
> > > 
> > > Then, though I'm still beaten by pinning (for users who have already
observe
> > the
> > > pin), I at least won't get reported.
> 
> But if your goal is to not get reported, wouldn't you just block the report
from
> being sent? I guess the browser could retry over long periods of time if a
> report wasn't sent successfully, which could make it theoretically harder for
an
> attacker to truly block a report from ever getting sent, but I think,
> fundamentally, reporting is designed for detecting and debugging
> misconfigurations, not for preventing attacks.

Is reporting not also used for detecting misissued certificates though? I guess
the question is whether a pin means:

a) I will not [ask a CA to] issue a certificate for this domain which violates
the pin as long as it is in effect. If you see one such certificate, you should
let me know because something went wrong.

b) I will not use a certificate for this domain which violates the pin as long
as it is in effect. If you see one such certificate *served from this hostname*,
you should let me know because something went wrong.


Or put another way: if armed with a pin which I believe to be valid, is it
meaningful for me to search the CT log for all certificates which violate that
pin and report them?

[estark, 2015-07-27 17:34:36]

On 2015/07/27 17:08:55, David Benjamin wrote:
> <snip>
> 
> Is reporting not also used for detecting misissued certificates though?

I'm sure it will turn out to be useful for that purpose in some cases, but I
don't think we should view it or try to sell it as a reliable way to detect
misissuance.

> I guess
> the question is whether a pin means:
> 
> a) I will not [ask a CA to] issue a certificate for this domain which violates
> the pin as long as it is in effect. If you see one such certificate, you
should
> let me know because something went wrong.
> 
> b) I will not use a certificate for this domain which violates the pin as long
> as it is in effect. If you see one such certificate *served from this
hostname*,
> you should let me know because something went wrong.

Based on the implementation and, more importantly, the spec, it definitely seems
like the latter to me. Forget the SPDY pooling complication for a moment. If
A.com serves a certificate that is valid for A.com and B.com, and that violates
a previously received pin for B.com, we won't detect the B.com violation when
served from A.com, because we only check pins for the hostname that the cert was
served from.

> 
> 
> Or put another way: if armed with a pin which I believe to be valid, is it
> meaningful for me to search the CT log for all certificates which violate that
> pin and report them?

I don't think so, unless I'm misunderstanding something. Suppose foo.blah.com
and bar.blah.com are both deployed with a *.blah.com certificate. One day the
site operator decides to put some new super-security-sensitive content on
foo.blah.com, so they buy a separate foo.blah.com certificate and deploy HPKP
for it. *.blah.com would still show up in the CT logs and appear to violate the
foo.blah.com pin, but shouldn't be reported, right?

[davidben, 2015-07-27 19:57:01]

On 2015/07/27 17:34:36, estark wrote:
> Based on the implementation and, more importantly, the spec, it definitely
seems
> like the latter to me. Forget the SPDY pooling complication for a moment. If
> A.com serves a certificate that is valid for A.com and B.com, and that
violates
> a previously received pin for B.com, we won't detect the B.com violation when
> served from A.com, because we only check pins for the hostname that the cert
was
> served from.

That's true. If we want to send pin reports on SPDY, we arguably should be
checking pins on all certificates we see. (Though we don't do this sort of
cross-name pooling with HTTP/1.1.)

> > Or put another way: if armed with a pin which I believe to be valid, is it
> > meaningful for me to search the CT log for all certificates which violate
that
> > pin and report them?
> 
> I don't think so, unless I'm misunderstanding something. Suppose
http://foo.blah.com
> and http://bar.blah.com are both deployed with a *.blah.com certificate. One
day the
> site operator decides to put some new super-security-sensitive content on
> http://foo.blah.com, so they buy a separate http://foo.blah.com certificate
and deploy HPKP
> for it. *.blah.com would still show up in the CT logs and appear to violate
the
> http://foo.blah.com pin, but shouldn't be reported, right?

Hrm. That seems a somewhat odd deployment strategy. But if the *.blah.com
private key is compromised, unless the pin is preloaded or the user has already
seen the pin, foo.blah.com is still bust for some clients.

(If anything, it seems the *.blah.com private key should be the higher-value key
and foo.blah.com is a lower-value key.)


Anyhow. I dug up the thread & design doc and it seems you all already had a
meeting with Ryan to decide this, so this lgtm. If we want to asynchronously
change this behavior later, we certainly can.

https://codereview.chromium.org/1212613004/diff/220001/net/http/transport_sec...
File net/http/transport_security_state_unittest.cc (right):

https://codereview.chromium.org/1212613004/diff/220001/net/http/transport_sec...
net/http/transport_security_state_unittest.cc:50: : public
net::TransportSecurityState::ReportSender {
No need for these 'net::'s.

[Ryan Sleevi, 2015-07-28 01:15:17]

LGTM, one q tho

https://codereview.chromium.org/1212613004/diff/220001/net/http/transport_sec...
File net/http/transport_security_state.cc (right):

https://codereview.chromium.org/1212613004/diff/220001/net/http/transport_sec...
net/http/transport_security_state.cc:651: TransportSecurityState::ReportSender*
report_sender) {
drive by: Curious why this is a method and not a ctor param?

[estark, 2015-07-28 04:09:45]

Thanks David and Ryan.

https://codereview.chromium.org/1212613004/diff/220001/net/http/transport_sec...
File net/http/transport_security_state.cc (right):

https://codereview.chromium.org/1212613004/diff/220001/net/http/transport_sec...
net/http/transport_security_state.cc:651: TransportSecurityState::ReportSender*
report_sender) {
On 2015/07/28 01:15:17, Ryan Sleevi wrote:
> drive by: Curious why this is a method and not a ctor param?

We could pass an initial value in the constructor, but we need the setter so
that we can set the reporter to nullptr, so that the reporter can be destroyed
before the TransportSecurityState to break the circular dependency
(URLRequestContext has a TSS which has a reporter which has the
URLRequestContext).

[estark, 2015-07-28 13:52:39]

https://codereview.chromium.org/1212613004/diff/220001/net/http/transport_sec...
File net/http/transport_security_state_unittest.cc (right):

https://codereview.chromium.org/1212613004/diff/220001/net/http/transport_sec...
net/http/transport_security_state_unittest.cc:50: : public
net::TransportSecurityState::ReportSender {
On 2015/07/27 19:57:01, David Benjamin wrote:
> No need for these 'net::'s.

Done.

[estark, 2015-07-28 13:52:52]

The CQ bit was checked by estark@chromium.org

[estark, 2015-07-28 13:52:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
davidben@chromium.org, rsleevi@chromium.org, mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1212613004/#ps260001
(title: "remove unnecessary net::'s")

[commit-bot: I haz the power, 2015-07-28 13:53:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1212613004/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1212613004/260001

[commit-bot: I haz the power, 2015-07-28 15:24:15]

Committed patchset #13 (id:260001)

[commit-bot: I haz the power, 2015-07-28 15:25:04]

Patchset 13 (id:??) landed as
https://crrev.com/1a66df754cd86315086180e978298f453f8723e7
Cr-Commit-Position: refs/heads/master@{#340687}

[Lei Zhang, 2015-07-28 20:27:25]

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

[Lei Zhang, 2015-07-28 20:27:26]

https://codereview.chromium.org/1212613004/diff/260001/net/http/transport_sec...
File net/http/transport_security_state.cc (right):

https://codereview.chromium.org/1212613004/diff/260001/net/http/transport_sec...
net/http/transport_security_state.cc:564: : delegate_(NULL),
enable_static_pins_(true) {
I think DrMemory's complaint is not initializing |report_sender_| here.