Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1267513003:
Add parsing for Public-Key-Pins-Report-Only header (Closed)
DescriptionAdd parsing for Public-Key-Pins-Report-Only header
Adds the ability to parse HPKP Report-Only headers, which will later be
processed on the requests on which they are received.
BUG=445793
Committed: https://crrev.com/640590d4170cd3d2ac9dec78d655ef064ec1b4a0
Cr-Commit-Position: refs/heads/master@{#341441}
Patch Set 1 #
Total comments: 8
Patch Set 2 : rename test function; add includeSubdomains to RO parsing #
Total comments: 3
Patch Set 3 : fix includeSubdomains comment #
Messages
Total messages: 16 (4 generated)
estark@chromium.org changed reviewers: + davidben@chromium.org
davidben, could you PTAL? This CL does the parsing and crrev.com/1266723003 does the processing/sending for Public-Key-Pins-Report-Only.
rsleevi@chromium.org changed reviewers: + rsleevi@chromium.org
https://codereview.chromium.org/1267513003/diff/1/net/http/http_security_head... File net/http/http_security_headers.h (right): https://codereview.chromium.org/1267513003/diff/1/net/http/http_security_head... net/http/http_security_headers.h:76: GURL* report_uri); Is there any reason to introduce a separate method? Given that the PKP-RO spec uses the same directive parsing, it seems perfectly fine to use ParsePKPHeader. (2.1.2 indicates max-age should just be ignored, 2.1.3 leaves includeSubDomains as optional - _but meaningful_, and 2.1.4 of course encourages report-uri for RO) https://codereview.chromium.org/1267513003/diff/1/net/http/http_security_head... File net/http/http_security_headers_unittest.cc (right): https://codereview.chromium.org/1267513003/diff/1/net/http/http_security_head... net/http/http_security_headers_unittest.cc:71: bool ParseBothHPKPHeaders(const std::string& value, Naming wise, this feels a bit odd - you're not parsing both headers simultaneously, except that's what seems to read as. RFC 7469 calls these "Public-Key-Directives" ABNF, but on a grammar nit, perhaps "ParseEitherHPKPHeader" ? Or simply "ParseHPKPHeader", since both PKP and PKP-RO *are* HPKP headers :)
https://codereview.chromium.org/1267513003/diff/1/net/http/http_security_head... File net/http/http_security_headers.h (right): https://codereview.chromium.org/1267513003/diff/1/net/http/http_security_head... net/http/http_security_headers.h:76: GURL* report_uri); On 2015/07/30 01:43:50, Ryan Sleevi wrote: > Is there any reason to introduce a separate method? Given that the PKP-RO spec > uses the same directive parsing, it seems perfectly fine to use ParsePKPHeader. > > (2.1.2 indicates max-age should just be ignored, 2.1.3 leaves includeSubDomains > as optional - _but meaningful_, and 2.1.4 of course encourages report-uri for > RO) The main reason is that ParseHPKPHeader() does some validation that doesn't apply to PKP-RO, namely requiring max-age and checking IsPinListValid(). If we wanted to use ParseHPKPHeader() for both, we could pull those validation steps out and have the caller do them. But there were two things that seemed weird about that: 1. it seemed weird to have ParseHPKPHeader() doing some of the validation (like report_uri.is_valid()) but not all, and 2. ParseHPKPHeader() would have to take an additional parameter or somehow indicate whether a max-age was parsed, so that the caller can check that a max-age is present if required. So I started to get a fitting-a-square-peg-in-a-round-hole kind of feeling. However, I did overlook the fact that we can't just ignore includeSubdomains for PKP-RO because it needs to be included in the report, so if we stick with the two separate functions I'll add an |include_subdomains| parameter to ParseHPKPReportOnlyHeader(). https://codereview.chromium.org/1267513003/diff/1/net/http/http_security_head... File net/http/http_security_headers_unittest.cc (right): https://codereview.chromium.org/1267513003/diff/1/net/http/http_security_head... net/http/http_security_headers_unittest.cc:71: bool ParseBothHPKPHeaders(const std::string& value, On 2015/07/30 01:43:50, Ryan Sleevi wrote: > Naming wise, this feels a bit odd - you're not parsing both headers > simultaneously, except that's what seems to read as. > > RFC 7469 calls these "Public-Key-Directives" ABNF, but on a grammar nit, perhaps > "ParseEitherHPKPHeader" ? Or simply "ParseHPKPHeader", since both PKP and PKP-RO > *are* HPKP headers :) I thought it might be weird to shadow net::ParseHPKPHeader()... but maybe that's okay? Otherwise I'm fine with ParseEitherHPKPHeader().
https://codereview.chromium.org/1267513003/diff/1/net/http/http_security_head... File net/http/http_security_headers.h (right): https://codereview.chromium.org/1267513003/diff/1/net/http/http_security_head... net/http/http_security_headers.h:76: GURL* report_uri); On 2015/07/30 02:43:49, estark wrote: > The main reason is that ParseHPKPHeader() does some validation that doesn't > apply to PKP-RO, namely requiring max-age and checking IsPinListValid(). Both of these apply. UAs MUST ignore any header fields containing directives, or other header field value data, that do not conform to the syntax defined in this specification. In particular, UAs must not attempt to fix malformed header fields. Or is your point the presence check (that max-age is REQUIRED)? The IsPinListValid argument makes sense though. > If we > wanted to use ParseHPKPHeader() for both, we could pull those validation steps > out and have the caller do them. But there were two things that seemed weird > about that: 1. it seemed weird to have ParseHPKPHeader() doing some of the > validation (like report_uri.is_valid()) but not all, and 2. ParseHPKPHeader() > would have to take an additional parameter or somehow indicate whether a max-age > was parsed, so that the caller can check that a max-age is present if required. > So I started to get a fitting-a-square-peg-in-a-round-hole kind of feeling. Fair point. > However, I did overlook the fact that we can't just ignore includeSubdomains for > PKP-RO because it needs to be included in the report, so if we stick with the > two separate functions I'll add an |include_subdomains| parameter to > ParseHPKPReportOnlyHeader(). Yeah, I think your argument for why two functions makes sense.
https://codereview.chromium.org/1267513003/diff/1/net/http/http_security_head... File net/http/http_security_headers_unittest.cc (right): https://codereview.chromium.org/1267513003/diff/1/net/http/http_security_head... net/http/http_security_headers_unittest.cc:71: bool ParseBothHPKPHeaders(const std::string& value, On 2015/07/30 02:43:49, estark wrote: > On 2015/07/30 01:43:50, Ryan Sleevi wrote: > > Naming wise, this feels a bit odd - you're not parsing both headers > > simultaneously, except that's what seems to read as. > > > > RFC 7469 calls these "Public-Key-Directives" ABNF, but on a grammar nit, > perhaps > > "ParseEitherHPKPHeader" ? Or simply "ParseHPKPHeader", since both PKP and > PKP-RO > > *are* HPKP headers :) > > I thought it might be weird to shadow net::ParseHPKPHeader()... but maybe that's > okay? Otherwise I'm fine with ParseEitherHPKPHeader(). ParsesAsHPKPHeader ? :)
https://codereview.chromium.org/1267513003/diff/1/net/http/http_security_head... File net/http/http_security_headers.h (right): https://codereview.chromium.org/1267513003/diff/1/net/http/http_security_head... net/http/http_security_headers.h:76: GURL* report_uri); On 2015/07/30 02:52:25, Ryan Sleevi wrote: > On 2015/07/30 02:43:49, estark wrote: > > The main reason is that ParseHPKPHeader() does some validation that doesn't > > apply to PKP-RO, namely requiring max-age and checking IsPinListValid(). > > Both of these apply. > > UAs MUST ignore any header fields containing directives, or other > header field value data, that do not conform to the syntax > defined in this specification. In particular, UAs must not > attempt to fix malformed header fields. > > Or is your point the presence check (that max-age is REQUIRED)? Yep, sorry, I meant the presence check. > > The IsPinListValid argument makes sense though. > > > If we > > wanted to use ParseHPKPHeader() for both, we could pull those validation steps > > out and have the caller do them. But there were two things that seemed weird > > about that: 1. it seemed weird to have ParseHPKPHeader() doing some of the > > validation (like report_uri.is_valid()) but not all, and 2. ParseHPKPHeader() > > would have to take an additional parameter or somehow indicate whether a > max-age > > was parsed, so that the caller can check that a max-age is present if > required. > > So I started to get a fitting-a-square-peg-in-a-round-hole kind of feeling. > > Fair point. > > > However, I did overlook the fact that we can't just ignore includeSubdomains > for > > PKP-RO because it needs to be included in the report, so if we stick with the > > two separate functions I'll add an |include_subdomains| parameter to > > ParseHPKPReportOnlyHeader(). > > Yeah, I think your argument for why two functions makes sense. Cool, thanks. https://codereview.chromium.org/1267513003/diff/1/net/http/http_security_head... File net/http/http_security_headers_unittest.cc (right): https://codereview.chromium.org/1267513003/diff/1/net/http/http_security_head... net/http/http_security_headers_unittest.cc:71: bool ParseBothHPKPHeaders(const std::string& value, On 2015/07/30 02:53:32, Ryan Sleevi wrote: > On 2015/07/30 02:43:49, estark wrote: > > On 2015/07/30 01:43:50, Ryan Sleevi wrote: > > > Naming wise, this feels a bit odd - you're not parsing both headers > > > simultaneously, except that's what seems to read as. > > > > > > RFC 7469 calls these "Public-Key-Directives" ABNF, but on a grammar nit, > > perhaps > > > "ParseEitherHPKPHeader" ? Or simply "ParseHPKPHeader", since both PKP and > > PKP-RO > > > *are* HPKP headers :) > > > > I thought it might be weird to shadow net::ParseHPKPHeader()... but maybe > that's > > okay? Otherwise I'm fine with ParseEitherHPKPHeader(). > > ParsesAsHPKPHeader ? :) Done.
lgtm https://codereview.chromium.org/1267513003/diff/20001/net/http/http_security_... File net/http/http_security_headers.cc (right): https://codereview.chromium.org/1267513003/diff/20001/net/http/http_security_... net/http/http_security_headers.cc:355: GURL candidate_report_uri; [Incidentally, this "candidate" silliness is why I like BoringSSL's convention that output parameters always get an out_ prefix. But Chromium doesn't do that, so I'm just griping. Ignore me.] https://codereview.chromium.org/1267513003/diff/20001/net/http/http_security_... net/http/http_security_headers.cc:383: // headers. and includeSubdomains are -> is? (Hrm. Looks like you did that in the other CL, actually.)
Thanks, David. Ryan, do you want to have another look before I land? https://codereview.chromium.org/1267513003/diff/20001/net/http/http_security_... File net/http/http_security_headers.cc (right): https://codereview.chromium.org/1267513003/diff/20001/net/http/http_security_... net/http/http_security_headers.cc:383: // headers. On 2015/07/31 19:53:23, David Benjamin wrote: > and includeSubdomains are -> is? > > (Hrm. Looks like you did that in the other CL, actually.) Yeah, just fixed it here too.
lgtm
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 Link to the patchset: https://codereview.chromium.org/1267513003/#ps40001 (title: "fix includeSubdomains comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1267513003/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1267513003/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/640590d4170cd3d2ac9dec78d655ef064ec1b4a0 Cr-Commit-Position: refs/heads/master@{#341441} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
