Parse HPKP report-uri and persist in TransportSecurityPersister

Parse HPKP report-uri and persist in TransportSecurityPersister

This CL parses the report-uri attribute on HPKP headers and stores them
in TransportSecurityPersister.

This is CL #1.
CL #2: crrev.com/1212973002 (add net::CertificateReportSender)
CL #3: crrev.com/1212613004 (build and send HPKP reports)

BUG=445793
Committed: https://crrev.com/1320e36d908427d615357df1630348bfb38cb5c4
Cr-Commit-Position: refs/heads/master@{#339667}

Committed: https://crrev.com/83487b6773bbda6a46191dede6010ec90e5a9ae8
Cr-Commit-Position: refs/heads/master@{#340490}

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

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

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

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

[Ryan Sleevi, 2015-06-26 19:41:51]

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

[Ryan Sleevi, 2015-06-26 19:41:53]

https://codereview.chromium.org/1211363005/diff/20001/net/http/http_security_...
File net/http/http_security_headers.cc (right):

https://codereview.chromium.org/1211363005/diff/20001/net/http/http_security_...
net/http/http_security_headers.cc:319: if (!HttpUtil::IsQuote(equals.second[0]))
SECURITY BUG: equals.second.empty() may be true, in which case, [0] is an
invalid access.

(See line 300)

https://codereview.chromium.org/1211363005/diff/20001/net/http/http_security_...
File net/http/http_security_headers_unittest.cc (right):

https://codereview.chromium.org/1211363005/diff/20001/net/http/http_security_...
net/http/http_security_headers_unittest.cc:526: backup_pin + ";" + good_pin + ";
  report-uri=\"/foo\"",
Suggestion: Also include a report-uri with an encoded value to make sure it's
handled properly.

https://codereview.chromium.org/1211363005/diff/20001/net/http/http_security_...
net/http/http_security_headers_unittest.cc:526: backup_pin + ";" + good_pin + ";
  report-uri=\"/foo\"",
Suggestion: Also include a > 0x7F character, which from what I can tell, seems
to be valid; just making sure the parser doesn't get hinky

https://codereview.chromium.org/1211363005/diff/20001/net/http/http_security_...
net/http/http_security_headers_unittest.cc:526: backup_pin + ";" + good_pin + ";
  report-uri=\"/foo\"",
Suggestion: include a quoted URL with a ";", to make sure the parser doesn't do
something stupid like split on ';' but actually tokenizes properly.

https://codereview.chromium.org/1211363005/diff/20001/net/http/transport_secu...
File net/http/transport_security_persister_unittest.cc (right):

https://codereview.chromium.org/1211363005/diff/20001/net/http/transport_secu...
net/http/transport_security_persister_unittest.cc:88: std::string
report_uri(kReportUri);
Why the explicit string? Implicit string construction would be fine (e.g. line
103 saying kReportUri)

https://codereview.chromium.org/1211363005/diff/20001/net/http/transport_secu...
File net/http/transport_security_state_unittest.cc (right):

https://codereview.chromium.org/1211363005/diff/20001/net/http/transport_secu...
net/http/transport_security_state_unittest.cc:432: static const char
kReportUri[] = "http://example.com/test";
nit: Rather than constantly duplicating this - stick it in the unnamed namespace
at the top of the file?

I'm ambivalent.

Same remarks as the other tests re: the temp string.

[estark, 2015-06-26 22:42:11]

Thanks Ryan.

https://codereview.chromium.org/1211363005/diff/20001/net/http/http_security_...
File net/http/http_security_headers.cc (right):

https://codereview.chromium.org/1211363005/diff/20001/net/http/http_security_...
net/http/http_security_headers.cc:319: if (!HttpUtil::IsQuote(equals.second[0]))
On 2015/06/26 19:41:52, Ryan Sleevi (slow through 7-15 wrote:
> SECURITY BUG: equals.second.empty() may be true, in which case, [0] is an
> invalid access.
> 
> (See line 300)

Done.

https://codereview.chromium.org/1211363005/diff/20001/net/http/http_security_...
File net/http/http_security_headers_unittest.cc (right):

https://codereview.chromium.org/1211363005/diff/20001/net/http/http_security_...
net/http/http_security_headers_unittest.cc:526: backup_pin + ";" + good_pin + ";
  report-uri=\"/foo\"",
On 2015/06/26 19:41:52, Ryan Sleevi (slow through 7-15 wrote:
> Suggestion: include a quoted URL with a ";", to make sure the parser doesn't
do
> something stupid like split on ';' but actually tokenizes properly.

Done. (It in fact wasn't tokenizing properly!)

https://codereview.chromium.org/1211363005/diff/20001/net/http/http_security_...
net/http/http_security_headers_unittest.cc:526: backup_pin + ";" + good_pin + ";
  report-uri=\"/foo\"",
On 2015/06/26 19:41:52, Ryan Sleevi (slow through 7-15 wrote:
> Suggestion: Also include a > 0x7F character, which from what I can tell, seems
> to be valid; just making sure the parser doesn't get hinky

Done.

https://codereview.chromium.org/1211363005/diff/20001/net/http/http_security_...
net/http/http_security_headers_unittest.cc:526: backup_pin + ";" + good_pin + ";
  report-uri=\"/foo\"",
On 2015/06/26 19:41:52, Ryan Sleevi (slow through 7-15 wrote:
> Suggestion: Also include a report-uri with an encoded value to make sure it's
> handled properly.

I'm actually not sure what we should be doing with a URL encoded value. Should
we just store it in encoded form and pass it straight through to GURL? I can't
seem to find how GURL deals with URL encoding...

https://codereview.chromium.org/1211363005/diff/20001/net/http/transport_secu...
File net/http/transport_security_persister_unittest.cc (right):

https://codereview.chromium.org/1211363005/diff/20001/net/http/transport_secu...
net/http/transport_security_persister_unittest.cc:88: std::string
report_uri(kReportUri);
On 2015/06/26 19:41:52, Ryan Sleevi (slow through 7-15 wrote:
> Why the explicit string? Implicit string construction would be fine (e.g. line
> 103 saying kReportUri)

Done.

https://codereview.chromium.org/1211363005/diff/20001/net/http/transport_secu...
File net/http/transport_security_state_unittest.cc (right):

https://codereview.chromium.org/1211363005/diff/20001/net/http/transport_secu...
net/http/transport_security_state_unittest.cc:432: static const char
kReportUri[] = "http://example.com/test";
On 2015/06/26 19:41:52, Ryan Sleevi (slow through 7-15 wrote:
> nit: Rather than constantly duplicating this - stick it in the unnamed
namespace
> at the top of the file?
> 
> I'm ambivalent.
> 
> Same remarks as the other tests re: the temp string.

Done.

[Ryan Sleevi, 2015-06-27 12:13:59]

https://codereview.chromium.org/1211363005/diff/20001/net/http/http_security_...
File net/http/http_security_headers_unittest.cc (right):

https://codereview.chromium.org/1211363005/diff/20001/net/http/http_security_...
net/http/http_security_headers_unittest.cc:526: backup_pin + ";" + good_pin + ";
  report-uri=\"/foo\"",
On 2015/06/26 22:42:11, estark wrote:
> On 2015/06/26 19:41:52, Ryan Sleevi (slow through 7-15 wrote:
> > Suggestion: Also include a report-uri with an encoded value to make sure
it's
> > handled properly.
> 
> I'm actually not sure what we should be doing with a URL encoded value. Should
> we just store it in encoded form and pass it straight through to GURL? I can't
> seem to find how GURL deals with URL encoding...

HttpUtil::Unquote handles unescaping

https://codereview.chromium.org/1211363005/diff/40001/net/http/http_security_...
File net/http/http_security_headers.cc (right):

https://codereview.chromium.org/1211363005/diff/40001/net/http/http_security_...
net/http/http_security_headers.cc:109: size_t point =
HttpUtil::FindDelimiter(source, 0, delimiter);
Hrm, this was previously dead code, and nothing uses this function - not even
unit tests!

It makes me wonder if we should be tweaking HttpUtil::NameValuePairsIterator to
allow optionally omitted values, which would at least be consistent with all the
other code we use.

[davidben, 2015-06-29 22:38:44]

https://codereview.chromium.org/1211363005/diff/40001/net/http/http_security_...
File net/http/http_security_headers.cc (right):

https://codereview.chromium.org/1211363005/diff/40001/net/http/http_security_...
net/http/http_security_headers.cc:109: size_t point =
HttpUtil::FindDelimiter(source, 0, delimiter);
On 2015/06/27 12:13:59, Ryan Sleevi (slow through 7-15 wrote:
> Hrm, this was previously dead code, and nothing uses this function - not even
> unit tests!
> 
> It makes me wonder if we should be tweaking HttpUtil::NameValuePairsIterator
to
> allow optionally omitted values, which would at least be consistent with all
the
> other code we use.

That sounds reasonable to me, assuming you mean that value-less is some opt-in
mode for NameValuePairsIterator. Too bad there isn't a clear name for this
syntax that specs and NameValuePairsIterator could clearly refer to. There seem
to be a handful of parsing problems with the current code, like the one Emily
caught. We also don't accept quoted max-ages right now, if I'm reading this code
right, but RFC 6797 explicitly allows it quoted. <insert davidben's usual rant
about text-based encodings here>

Although note, by the way, that NameValuePairsIterator also accepts this broken
syntax:
http://crbug.com/39836


[Aside: Is HPKP quiiite the same syntax? I suspect this is a spec error but:

http://tools.ietf.org/html/rfc6797#section-6.1
This cites the "implied *LWS" variant and so it allows spaces (and the
line-folding craziness!) everywhere.

http://tools.ietf.org/html/rfc7469#section-2.1
This cites the RFC 7230 formulation (which says, if obsolete line folding is
supported, it's replaced with one or more SP before further processing) and has
explicit OWS in places. But there isn't OWS around the =.

Anyway, I'm guessing this is a spec error and so it's safe to use an omitted
values variant of NameValuePairsIterator.]

https://codereview.chromium.org/1211363005/diff/40001/net/http/http_security_...
net/http/http_security_headers.cc:319: // report-uris are always quoted.
Not quoting would be insane, but I don't actually see anything in the spec that
requires that. (Spec error? Pins explicitly say they have to be quoted.)

[estark, 2015-07-06 23:42:02]

Hello, I'm back from vacation. Patchset #5 adds an optional-values mode to
NameValuePairsIterator, uses it for HPKP parsing, and deletes the dead code
(FindDelimiter).

On 2015/06/29 22:38:44, David Benjamin wrote:
>
https://codereview.chromium.org/1211363005/diff/40001/net/http/http_security_...
> File net/http/http_security_headers.cc (right):
> 
>
https://codereview.chromium.org/1211363005/diff/40001/net/http/http_security_...
> net/http/http_security_headers.cc:109: size_t point =
> HttpUtil::FindDelimiter(source, 0, delimiter);
> On 2015/06/27 12:13:59, Ryan Sleevi (slow through 7-15 wrote:
> > Hrm, this was previously dead code, and nothing uses this function - not
even
> > unit tests!
> > 
> > It makes me wonder if we should be tweaking HttpUtil::NameValuePairsIterator
> to
> > allow optionally omitted values, which would at least be consistent with all
> the
> > other code we use.
> 
> That sounds reasonable to me, assuming you mean that value-less is some opt-in
> mode for NameValuePairsIterator. Too bad there isn't a clear name for this
> syntax that specs and NameValuePairsIterator could clearly refer to. There
seem
> to be a handful of parsing problems with the current code, like the one Emily
> caught. We also don't accept quoted max-ages right now, if I'm reading this
code
> right, but RFC 6797 explicitly allows it quoted. <insert davidben's usual rant
> about text-based encodings here>

Added a test for quoted max-ages.

> 
> Although note, by the way, that NameValuePairsIterator also accepts this
broken
> syntax:
> http://crbug.com/39836
> 
> 
> [Aside: Is HPKP quiiite the same syntax? I suspect this is a spec error but:
> 
> http://tools.ietf.org/html/rfc6797#section-6.1
> This cites the "implied *LWS" variant and so it allows spaces (and the
> line-folding craziness!) everywhere.
> 
> http://tools.ietf.org/html/rfc7469#section-2.1
> This cites the RFC 7230 formulation (which says, if obsolete line folding is
> supported, it's replaced with one or more SP before further processing) and
has
> explicit OWS in places. But there isn't OWS around the =.
> 
> Anyway, I'm guessing this is a spec error and so it's safe to use an omitted
> values variant of NameValuePairsIterator.]
> 
>
https://codereview.chromium.org/1211363005/diff/40001/net/http/http_security_...
> net/http/http_security_headers.cc:319: // report-uris are always quoted.
> Not quoting would be insane, but I don't actually see anything in the spec
that
> requires that. (Spec error? Pins explicitly say they have to be quoted.)

A couple things have come up so far that are in the category of possible spec
errors or improvements, but I'm not sure what to do with them. I'm just keeping
a list of them so far and doing what seems sensible, and figure I'll deal with
them in a batch later, though I'm not quite sure what "deal with them" means.
(File errata?)

[estark, 2015-07-13 17:33:30]

On 2015/07/06 23:42:02, estark wrote:
> Hello, I'm back from vacation. Patchset #5 adds an optional-values mode to
> NameValuePairsIterator, uses it for HPKP parsing, and deletes the dead code
> (FindDelimiter).
> 
> On 2015/06/29 22:38:44, David Benjamin wrote:
> >
>
https://codereview.chromium.org/1211363005/diff/40001/net/http/http_security_...
> > File net/http/http_security_headers.cc (right):
> > 
> >
>
https://codereview.chromium.org/1211363005/diff/40001/net/http/http_security_...
> > net/http/http_security_headers.cc:109: size_t point =
> > HttpUtil::FindDelimiter(source, 0, delimiter);
> > On 2015/06/27 12:13:59, Ryan Sleevi (slow through 7-15 wrote:
> > > Hrm, this was previously dead code, and nothing uses this function - not
> even
> > > unit tests!
> > > 
> > > It makes me wonder if we should be tweaking
HttpUtil::NameValuePairsIterator
> > to
> > > allow optionally omitted values, which would at least be consistent with
all
> > the
> > > other code we use.
> > 
> > That sounds reasonable to me, assuming you mean that value-less is some
opt-in
> > mode for NameValuePairsIterator. Too bad there isn't a clear name for this
> > syntax that specs and NameValuePairsIterator could clearly refer to. There
> seem
> > to be a handful of parsing problems with the current code, like the one
Emily
> > caught. We also don't accept quoted max-ages right now, if I'm reading this
> code
> > right, but RFC 6797 explicitly allows it quoted. <insert davidben's usual
rant
> > about text-based encodings here>
> 
> Added a test for quoted max-ages.
> 
> > 
> > Although note, by the way, that NameValuePairsIterator also accepts this
> broken
> > syntax:
> > http://crbug.com/39836
> > 
> > 
> > [Aside: Is HPKP quiiite the same syntax? I suspect this is a spec error but:
> > 
> > http://tools.ietf.org/html/rfc6797#section-6.1
> > This cites the "implied *LWS" variant and so it allows spaces (and the
> > line-folding craziness!) everywhere.
> > 
> > http://tools.ietf.org/html/rfc7469#section-2.1
> > This cites the RFC 7230 formulation (which says, if obsolete line folding is
> > supported, it's replaced with one or more SP before further processing) and
> has
> > explicit OWS in places. But there isn't OWS around the =.
> > 
> > Anyway, I'm guessing this is a spec error and so it's safe to use an omitted
> > values variant of NameValuePairsIterator.]
> > 
> >
>
https://codereview.chromium.org/1211363005/diff/40001/net/http/http_security_...
> > net/http/http_security_headers.cc:319: // report-uris are always quoted.
> > Not quoting would be insane, but I don't actually see anything in the spec
> that
> > requires that. (Spec error? Pins explicitly say they have to be quoted.)
> 
> A couple things have come up so far that are in the category of possible spec
> errors or improvements, but I'm not sure what to do with them. I'm just
keeping
> a list of them so far and doing what seems sensible, and figure I'll deal with
> them in a batch later, though I'm not quite sure what "deal with them" means.
> (File errata?)

Friendly ping to davidben and rsleevi

[davidben, 2015-07-15 22:21:07]

Sorry about the delay. A few comments, but they're fairly minor. Thanks for
doing the NameValuePairsIterator thing!

https://codereview.chromium.org/1211363005/diff/100001/net/http/http_security...
File net/http/http_security_headers.cc (right):

https://codereview.chromium.org/1211363005/diff/100001/net/http/http_security...
net/http/http_security_headers.cc:300:
!HttpUtil::IsQuote(name_value_pairs.raw_value()[0]) ||
Maybe expose value_is_quoted() out of NameValuePairsIterator. Then this can just
be.

  if (!name_value_pairs.value_is_quoted())
    return false;
  *report_uri = name_value_pairs.value();
  if (report_uri->empty())
    return false;


[*] Hrm. I bet we should be using base::StringPiece now. Not for this CL though.
"Oh, by the way, while you're here, how about you refactor the universe?" is not
actually reasonable. :-)

https://codereview.chromium.org/1211363005/diff/100001/net/http/http_security...
net/http/http_security_headers.cc:306: *report_uri =
HttpUtil::Unquote(name_value_pairs.value());
Bug? I think this should be Unquote(raw_value) or just value.

https://codereview.chromium.org/1211363005/diff/100001/net/http/http_util.cc
File net/http/http_util.cc (right):

https://codereview.chromium.org/1211363005/diff/100001/net/http/http_util.cc#...
net/http/http_util.cc:902: return valid_ = false;  // Malformed, no name
This was there before, but... sadness. :-(

https://codereview.chromium.org/1211363005/diff/100001/net/http/http_util.cc#...
net/http/http_util.cc:916: value_begin_ = (equals == value_end_) ? equals :
equals + 1;
Nit: I'd maybe do

 (equals == value_end_) ? value_end_ : equals + 1

Just so it's more obvious that you intend for value_begin_/value_end_ to be
empty.

https://codereview.chromium.org/1211363005/diff/100001/net/http/http_util.cc#...
net/http/http_util.cc:925: return valid_ = values_optional_;
This probably wants to be:

  if (equals != value_end_ && value_begin_ == value_end_)

And not conditioned on values_optional_. It doesn't look like "foo=" is legal in
HSTS and HPKP either; tokens can't be empty.

https://codereview.chromium.org/1211363005/diff/100001/net/http/http_util_uni...
File net/http/http_util_unittest.cc (right):

https://codereview.chromium.org/1211363005/diff/100001/net/http/http_util_uni...
net/http/http_util_unittest.cc:1103:
ASSERT_NO_FATAL_FAILURE(CheckNextNameValuePair(&default_parser, false, false,
Hrm. Is a GetNext failure not supposed to be fatal? It doesn't look like there's
any code or tests that want us to be able to recover.

You can probably just do the one

  ASSERT_NO_FATAL_FAILURE(CheckNextNameValuePair(&default_parser, false, false,
std::string(), std::string()));

(Note CheckNextNameValuePair ignores the next two arguments if it's not true
true.)

https://codereview.chromium.org/1211363005/diff/100001/net/http/http_util_uni...
net/http/http_util_unittest.cc:1123: false, "beta", std::string()));
Ditto.

https://codereview.chromium.org/1211363005/diff/100001/net/http/transport_sec...
File net/http/transport_security_persister_unittest.cc (right):

https://codereview.chromium.org/1211363005/diff/100001/net/http/transport_sec...
net/http/transport_security_persister_unittest.cc:187: static const char
kReportUri[] = "http://example.com/test";
Remove this line in favor of the global one? (Confusing the shadow the name
anyway.)

https://codereview.chromium.org/1211363005/diff/100001/net/http/transport_sec...
net/http/transport_security_persister_unittest.cc:227:
TEST_F(TransportSecurityPersisterTest, PublicKeyPinReportUri) {
Isn't this test the same as the one above, or am I missing something? (Maybe
just rename it to PublicKeyPins.)

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

https://codereview.chromium.org/1211363005/diff/100001/net/http/transport_sec...
net/http/transport_security_state.h:134: // sent when this pin is violated.
Nit: Probably better to say

   // A optional uri-reference indicating where reports should be
   // sent when this pin is violated or the empty string when omitted.

Just to be explicit what the optional state is. (Although empty string is pretty
obvious.)

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

https://codereview.chromium.org/1211363005/diff/100001/net/http/transport_sec...
net/http/transport_security_state_unittest.cc:229: report_uri);
Does passing kReportUri in here not work? I would have thought that
std::string's const char * ctor would fire. (Ditto elsewhere in the file.)

[estark, 2015-07-16 00:07:01]

Thanks David.

https://codereview.chromium.org/1211363005/diff/100001/net/http/http_security...
File net/http/http_security_headers.cc (right):

https://codereview.chromium.org/1211363005/diff/100001/net/http/http_security...
net/http/http_security_headers.cc:300:
!HttpUtil::IsQuote(name_value_pairs.raw_value()[0]) ||
On 2015/07/15 22:21:06, David Benjamin wrote:
> Maybe expose value_is_quoted() out of NameValuePairsIterator. Then this can
just
> be.
> 
>   if (!name_value_pairs.value_is_quoted())
>     return false;
>   *report_uri = name_value_pairs.value();
>   if (report_uri->empty())
>     return false;
> 
> 
> [*] Hrm. I bet we should be using base::StringPiece now. Not for this CL
though.
> "Oh, by the way, while you're here, how about you refactor the universe?" is
not
> actually reasonable. :-)

Done.

https://codereview.chromium.org/1211363005/diff/100001/net/http/http_security...
net/http/http_security_headers.cc:306: *report_uri =
HttpUtil::Unquote(name_value_pairs.value());
On 2015/07/15 22:21:06, David Benjamin wrote:
> Bug? I think this should be Unquote(raw_value) or just value.

Done.

https://codereview.chromium.org/1211363005/diff/100001/net/http/http_util.cc
File net/http/http_util.cc (right):

https://codereview.chromium.org/1211363005/diff/100001/net/http/http_util.cc#...
net/http/http_util.cc:916: value_begin_ = (equals == value_end_) ? equals :
equals + 1;
On 2015/07/15 22:21:06, David Benjamin wrote:
> Nit: I'd maybe do
> 
>  (equals == value_end_) ? value_end_ : equals + 1
> 
> Just so it's more obvious that you intend for value_begin_/value_end_ to be
> empty.

Done.

https://codereview.chromium.org/1211363005/diff/100001/net/http/http_util.cc#...
net/http/http_util.cc:925: return valid_ = values_optional_;
On 2015/07/15 22:21:06, David Benjamin wrote:
> This probably wants to be:
> 
>   if (equals != value_end_ && value_begin_ == value_end_)
> 
> And not conditioned on values_optional_. It doesn't look like "foo=" is legal
in
> HSTS and HPKP either; tokens can't be empty.

Done.

https://codereview.chromium.org/1211363005/diff/100001/net/http/http_util_uni...
File net/http/http_util_unittest.cc (right):

https://codereview.chromium.org/1211363005/diff/100001/net/http/http_util_uni...
net/http/http_util_unittest.cc:1103:
ASSERT_NO_FATAL_FAILURE(CheckNextNameValuePair(&default_parser, false, false,
On 2015/07/15 22:21:06, David Benjamin wrote:
> Hrm. Is a GetNext failure not supposed to be fatal? It doesn't look like
there's
> any code or tests that want us to be able to recover.
> 
> You can probably just do the one
> 
>   ASSERT_NO_FATAL_FAILURE(CheckNextNameValuePair(&default_parser, false,
false,
> std::string(), std::string()));
> 
> (Note CheckNextNameValuePair ignores the next two arguments if it's not true
> true.)

Done.

https://codereview.chromium.org/1211363005/diff/100001/net/http/http_util_uni...
net/http/http_util_unittest.cc:1123: false, "beta", std::string()));
On 2015/07/15 22:21:06, David Benjamin wrote:
> Ditto.

Done.

https://codereview.chromium.org/1211363005/diff/100001/net/http/transport_sec...
File net/http/transport_security_persister_unittest.cc (right):

https://codereview.chromium.org/1211363005/diff/100001/net/http/transport_sec...
net/http/transport_security_persister_unittest.cc:187: static const char
kReportUri[] = "http://example.com/test";
On 2015/07/15 22:21:06, David Benjamin wrote:
> Remove this line in favor of the global one? (Confusing the shadow the name
> anyway.)

Done.

https://codereview.chromium.org/1211363005/diff/100001/net/http/transport_sec...
net/http/transport_security_persister_unittest.cc:227:
TEST_F(TransportSecurityPersisterTest, PublicKeyPinReportUri) {
On 2015/07/15 22:21:06, David Benjamin wrote:
> Isn't this test the same as the one above, or am I missing something? (Maybe
> just rename it to PublicKeyPins.)

Er, sorry, don't know what I was thinking there. Done.

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

https://codereview.chromium.org/1211363005/diff/100001/net/http/transport_sec...
net/http/transport_security_state.h:134: // sent when this pin is violated.
On 2015/07/15 22:21:06, David Benjamin wrote:
> Nit: Probably better to say
> 
>    // A optional uri-reference indicating where reports should be
>    // sent when this pin is violated or the empty string when omitted.
> 
> Just to be explicit what the optional state is. (Although empty string is
pretty
> obvious.)

Done.

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

https://codereview.chromium.org/1211363005/diff/100001/net/http/transport_sec...
net/http/transport_security_state_unittest.cc:229: report_uri);
On 2015/07/15 22:21:07, David Benjamin wrote:
> Does passing kReportUri in here not work? I would have thought that
> std::string's const char * ctor would fire. (Ditto elsewhere in the file.)

Done.

[Ryan Sleevi, 2015-07-16 02:25:44]

https://codereview.chromium.org/1211363005/diff/120001/net/http/http_security...
File net/http/http_security_headers.cc (right):

https://codereview.chromium.org/1211363005/diff/120001/net/http/http_security...
net/http/http_security_headers.cc:275: if (name_value_pairs.value().empty() ||
It's worth noting that you force a string coercion here, only to discard it,
only to make a new copy in MaxAgeToInt, which isn't needed because it's a
base::StringPiece.

Oh the silliness abounds here (in the existing code)

Suggestion:

if (!MaxAgeToInt(...begin(), ...end(), &max_age_candidate)) {
  return false;
}

and in MaxAgeToInt(iter begin, iter end, result) {
  const base::StringPiece s(begin, end);
  if (s.empty())
    return false;

  int64_t i = 0;
  ...
}

(since base::StringToInt64 is perfectly happy with a base::StringPiece)

Avoids all copies :)

https://codereview.chromium.org/1211363005/diff/120001/net/http/http_security...
net/http/http_security_headers.cc:283: if
(!ParseAndAppendPin(name_value_pairs.raw_value(), HASH_VALUE_SHA1,
Man, never realized how many copies we were making for this, unnecessarily. Oh
well. Bonus cleanups for more StringPiece's (sorry, brettw@ keeps sending me
cleanups to reduce strings, this stood out as well)

(It's also unfortunate that we don't just update ParseAndAppendPin to expect the
value to be unquoted already, as a base::StringPiece, but that's neither here
nor there)

https://codereview.chromium.org/1211363005/diff/120001/net/http/http_security...
net/http/http_security_headers.cc:304: return false;
So this makes me a little uncomfortable, because usually with API contracts, we
don't modify output variables unless/until we return true - see lines 319-321.

So I think you want a temporary here and to update it down there, to be entirely
consistent.

https://codereview.chromium.org/1211363005/diff/120001/net/http/http_security...
net/http/http_security_headers.cc:304: return false;
BUG: We parse the other fields for validity and conformance, but here, we don't
do anything to validate that report-uri is a valid URL that is understood.

It seems like we should at least run through a GURL and canonicalize it (by
virtue of the string ctor), and then check validity. This also implies that
report_uri should return a GURL (which would thus indicate it's always in a
canonical form).

For example, under the current code, report-uri of "foo" seems like it would be
valid and persisted. Have I missed a check?

https://codereview.chromium.org/1211363005/diff/120001/net/http/http_util.cc
File net/http/http_util.cc (right):

https://codereview.chromium.org/1211363005/diff/120001/net/http/http_util.cc#...
net/http/http_util.cc:874: : NameValuePairsIterator(begin, end, delimiter,
VALUES_NOT_OPTIONAL) {
Had to double check, but yeah, delegated ctors are now allowed in
https://chromium-cpp.appspot.com/. Awesome.

https://codereview.chromium.org/1211363005/diff/120001/net/http/http_util.cc#...
net/http/http_util.cc:942: 
Interesting - we don't complain about empty quoted values.

https://codereview.chromium.org/1211363005/diff/120001/net/http/http_util.h
File net/http/http_util.h (right):

https://codereview.chromium.org/1211363005/diff/120001/net/http/http_util.h#n...
net/http/http_util.h:321: // corresponding equals sign. Default is
VALUES_NOT_OPTIONAL.
This last sentence arguably belongs to the NameValuePairsIterator ctor that
doesn't take a value, not to the definition of the enum itself.

[estark, 2015-07-16 04:52:18]

https://codereview.chromium.org/1211363005/diff/120001/net/http/http_security...
File net/http/http_security_headers.cc (right):

https://codereview.chromium.org/1211363005/diff/120001/net/http/http_security...
net/http/http_security_headers.cc:275: if (name_value_pairs.value().empty() ||
On 2015/07/16 02:25:44, Ryan Sleevi (slow through 7-15 wrote:
> It's worth noting that you force a string coercion here, only to discard it,
> only to make a new copy in MaxAgeToInt, which isn't needed because it's a
> base::StringPiece.
> 
> Oh the silliness abounds here (in the existing code)
> 
> Suggestion:
> 
> if (!MaxAgeToInt(...begin(), ...end(), &max_age_candidate)) {
>   return false;
> }
> 
> and in MaxAgeToInt(iter begin, iter end, result) {
>   const base::StringPiece s(begin, end);
>   if (s.empty())
>     return false;
> 
>   int64_t i = 0;
>   ...
> }
> 
> (since base::StringToInt64 is perfectly happy with a base::StringPiece)
> 
> Avoids all copies :)

Done.

https://codereview.chromium.org/1211363005/diff/120001/net/http/http_security...
net/http/http_security_headers.cc:283: if
(!ParseAndAppendPin(name_value_pairs.raw_value(), HASH_VALUE_SHA1,
On 2015/07/16 02:25:44, Ryan Sleevi (slow through 7-15 wrote:
> Man, never realized how many copies we were making for this, unnecessarily. Oh
> well. Bonus cleanups for more StringPiece's (sorry, brettw@ keeps sending me
> cleanups to reduce strings, this stood out as well)
> 
> (It's also unfortunate that we don't just update ParseAndAppendPin to expect
the
> value to be unquoted already, as a base::StringPiece, but that's neither here
> nor there)

Done, I think -- please check and make sure I did something that makes sense?
(Moved the quoted check outside of ParseAndAppendPin(); passed begin and end
iterators to ParseAndAppendPin() and constructed a StringPiece from them)

https://codereview.chromium.org/1211363005/diff/120001/net/http/http_security...
net/http/http_security_headers.cc:304: return false;
On 2015/07/16 02:25:44, Ryan Sleevi (slow through 7-15 wrote:
> So this makes me a little uncomfortable, because usually with API contracts,
we
> don't modify output variables unless/until we return true - see lines 319-321.
> 
> So I think you want a temporary here and to update it down there, to be
entirely
> consistent.

Done.

https://codereview.chromium.org/1211363005/diff/120001/net/http/http_security...
net/http/http_security_headers.cc:304: return false;
On 2015/07/16 02:25:44, Ryan Sleevi (slow through 7-15 wrote:
> BUG: We parse the other fields for validity and conformance, but here, we
don't
> do anything to validate that report-uri is a valid URL that is understood.
> 
> It seems like we should at least run through a GURL and canonicalize it (by
> virtue of the string ctor), and then check validity. This also implies that
> report_uri should return a GURL (which would thus indicate it's always in a
> canonical form).
> 
> For example, under the current code, report-uri of "foo" seems like it would
be
> valid and persisted. Have I missed a check?

Ah, good point... Done. I'm now passing around GURL references everywhere
instead of strings and just converting to/from strings when
serializing/deserializing to/from disk; do you think that makes sense?

https://codereview.chromium.org/1211363005/diff/120001/net/http/http_util.h
File net/http/http_util.h (right):

https://codereview.chromium.org/1211363005/diff/120001/net/http/http_util.h#n...
net/http/http_util.h:321: // corresponding equals sign. Default is
VALUES_NOT_OPTIONAL.
On 2015/07/16 02:25:44, Ryan Sleevi (slow through 7-15 wrote:
> This last sentence arguably belongs to the NameValuePairsIterator ctor that
> doesn't take a value, not to the definition of the enum itself.

Done.

[Ryan Sleevi, 2015-07-16 05:07:51]

LGTM % inhumane nits

https://codereview.chromium.org/1211363005/diff/160001/net/http/http_security...
File net/http/http_security_headers.cc (right):

https://codereview.chromium.org/1211363005/diff/160001/net/http/http_security...
net/http/http_security_headers.cc:277: if
(base::LowerCaseEqualsASCII(name_value_pairs.name(), "max-age")) {
World's cruelest reviewer nit:

Each of these can be

base::LowerCaseEqualsASCII(name_value_pairs.name_begin(),
name_value_pairs.name_end(), ...))

Avoids multiple copies of the directive name...

[Ryan Sleevi, 2015-07-16 05:11:10]

On 2015/07/16 05:07:51, Ryan Sleevi (slow through 7-15 wrote:
> base::LowerCaseEqualsASCII(name_value_pairs.name_begin(),
> name_value_pairs.name_end(), ...))

Longer-term, we may just want to offer a name_piece / value_piece that returns a
base::StringPiece, which would make much of this so much easier for you, it
seems. But that's (yet another) CL, and more of a cleanup that a necessity.

[Ryan Sleevi, 2015-07-16 05:15:03]

On 2015/07/16 05:11:10, Ryan Sleevi (slow through 7-15 wrote:
> and more of a cleanup that a necessity.

*than* a necessity. As in NO need to deal with that now, in this CL - I've
nitted enough already.

[estark, 2015-07-16 05:18:49]

Haha. Thanks!

https://codereview.chromium.org/1211363005/diff/160001/net/http/http_security...
File net/http/http_security_headers.cc (right):

https://codereview.chromium.org/1211363005/diff/160001/net/http/http_security...
net/http/http_security_headers.cc:277: if
(base::LowerCaseEqualsASCII(name_value_pairs.name(), "max-age")) {
On 2015/07/16 05:07:51, Ryan Sleevi (slow through 7-15 wrote:
> World's cruelest reviewer nit:
> 
> Each of these can be
> 
> base::LowerCaseEqualsASCII(name_value_pairs.name_begin(),
> name_value_pairs.name_end(), ...))
> 
> Avoids multiple copies of the directive name...

Done.

[estark, 2015-07-16 17:34:14]

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

[estark, 2015-07-16 17:34:15]

eroman@chromium.org: can you please review //c/b/ui/webui/net_internals?

(David I'll wait for your l-g-t-m as well before landing.)

[davidben, 2015-07-16 20:13:01]

lgtm

https://codereview.chromium.org/1211363005/diff/180001/net/http/http_util_uni...
File net/http/http_util_unittest.cc (right):

https://codereview.chromium.org/1211363005/diff/180001/net/http/http_util_uni...
net/http/http_util_unittest.cc:1114:
EXPECT_FALSE(values_required_parser.valid());
Nit: This one is probably unnecessary since CheckNextNameValuePair does it
internally. (The block above doesn't do it.)

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

https://codereview.chromium.org/1211363005/diff/180001/net/http/transport_sec...
net/http/transport_security_state.h:135: // sent when this pin is violated, or
empty when omitted.
Super-nit-picky-nit: I think you want to rewrap this text now that you can fit
more on the first line.

[estark, 2015-07-17 00:56:32]

Thanks David.

https://codereview.chromium.org/1211363005/diff/180001/net/http/http_util_uni...
File net/http/http_util_unittest.cc (right):

https://codereview.chromium.org/1211363005/diff/180001/net/http/http_util_uni...
net/http/http_util_unittest.cc:1114:
EXPECT_FALSE(values_required_parser.valid());
On 2015/07/16 20:13:01, David Benjamin wrote:
> Nit: This one is probably unnecessary since CheckNextNameValuePair does it
> internally. (The block above doesn't do it.)

Done.

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

https://codereview.chromium.org/1211363005/diff/180001/net/http/transport_sec...
net/http/transport_security_state.h:135: // sent when this pin is violated, or
empty when omitted.
On 2015/07/16 20:13:01, David Benjamin wrote:
> Super-nit-picky-nit: I think you want to rewrap this text now that you can fit
> more on the first line.

Done.

[estark, 2015-07-17 21:14:21]

On 2015/07/17 00:56:32, estark wrote:
> Thanks David.
> 
>
https://codereview.chromium.org/1211363005/diff/180001/net/http/http_util_uni...
> File net/http/http_util_unittest.cc (right):
> 
>
https://codereview.chromium.org/1211363005/diff/180001/net/http/http_util_uni...
> net/http/http_util_unittest.cc:1114:
> EXPECT_FALSE(values_required_parser.valid());
> On 2015/07/16 20:13:01, David Benjamin wrote:
> > Nit: This one is probably unnecessary since CheckNextNameValuePair does it
> > internally. (The block above doesn't do it.)
> 
> Done.
> 
>
https://codereview.chromium.org/1211363005/diff/180001/net/http/transport_sec...
> File net/http/transport_security_state.h (right):
> 
>
https://codereview.chromium.org/1211363005/diff/180001/net/http/transport_sec...
> net/http/transport_security_state.h:135: // sent when this pin is violated, or
> empty when omitted.
> On 2015/07/16 20:13:01, David Benjamin wrote:
> > Super-nit-picky-nit: I think you want to rewrap this text now that you can
fit
> > more on the first line.
> 
> Done.

Friendly ping to eroman; can you please review net_internals_ui.cc?

[eroman, 2015-07-20 23:30:37]

LGTM

Sorry was vacationing thu/fri last week :)

[estark, 2015-07-21 15:01:20]

The CQ bit was checked by estark@chromium.org

[estark, 2015-07-21 15:01:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
rsleevi@chromium.org, davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/1211363005/#ps200001
(title: "style fixes")

[commit-bot: I haz the power, 2015-07-21 15:02:07]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1211363005/200001

[commit-bot: I haz the power, 2015-07-21 16:39:49]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2015-07-21 16:41:10]

Patchset 11 (id:??) landed as
https://crrev.com/1320e36d908427d615357df1630348bfb38cb5c4
Cr-Commit-Position: refs/heads/master@{#339667}

[Ryan Sleevi, 2015-07-21 22:02:24]

A revert of this CL (patchset #11 id:200001) has been created in
https://codereview.chromium.org/1249823002/ by rsleevi@chromium.org.

The reason for reverting is: Causes Win7 to hang.

[estark, 2015-07-21 22:10:57]

https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%2...
is an example of the BogusPinHeaders tests hanging

[estark, 2015-07-22 00:18:45]

cc'ing palmer@ because he expressed interest, thanks palmer! Feel free to add
yourself to any of the CLs in this series. This one already landed once but is
currently reverted due to mysterious Win7 test hangs.

[estark, 2015-07-27 14:12:11]

The CQ bit was checked by estark@chromium.org

[estark, 2015-07-27 14:12:12]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org,
davidben@chromium.org, rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/1211363005/#ps240001
(title: "GetNext() fix")

[commit-bot: I haz the power, 2015-07-27 14:12:23]

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

[commit-bot: I haz the power, 2015-07-27 17:11:22]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-07-27 17:12:18]

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