|
|
DescriptionBuild 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}
Patch Set 1 #
Total comments: 12
Patch Set 2 : rebase #Patch Set 3 : rsleevi comments #
Total comments: 10
Patch Set 4 : rebase #Patch Set 5 : rebase #Patch Set 6 : combine GetHPKPReportUri() and BuildHPKPReport() into GetHPKPReport() #
Total comments: 12
Patch Set 7 : davidben commens #Patch Set 8 : rebase #Patch Set 9 : move report building code to TransportSecurityState; wire up to CheckPublicKeyPins #
Total comments: 30
Patch Set 10 : rebase #Patch Set 11 : davidben comments #
Total comments: 4
Patch Set 12 : rebase #Patch Set 13 : remove unnecessary net::'s #
Total comments: 1
Messages
Total messages: 42 (7 generated)
estark@chromium.org changed reviewers: + davidben@chromium.org
This is CL 3/4 split out from https://codereview.chromium.org/1211933005/.
rsleevi@chromium.org changed reviewers: + rsleevi@chromium.org
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.
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.
Patchset #2 (id:20001) has been deleted
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.
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.
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?
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.)
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?
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? >
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.
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.
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.)
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.
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.
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.
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.
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
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?)
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.
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@chromium.org changed reviewers: + mmenke@chromium.org
mmenke, could you please review the ProfileIOData changes?
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.
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.
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.
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.
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?
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?
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.
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?
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).
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.
The CQ bit was checked by estark@chromium.org
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")
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
Message was sent while issue was closed.
Committed patchset #13 (id:260001)
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/1a66df754cd86315086180e978298f453f8723e7 Cr-Commit-Position: refs/heads/master@{#340687}
Message was sent while issue was closed.
thestig@chromium.org changed reviewers: + thestig@chromium.org
Message was sent while issue was closed.
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. |