Add Expect-CT header parsing

Add Expect-CT header parsing

This CL implements parsing of the Expect-CT HTTP header (draft spec at
https://tools.ietf.org/html/draft-ietf-httpbis-expect-ct-00).

BUG=679012
Review-Url: https://codereview.chromium.org/2753703002
Cr-Commit-Position: refs/heads/master@{#466386}
Committed: https://chromium.googlesource.com/chromium/src/+/a57e81656f11b9ec7de47d3fdbe8aa297311b5fa

[estark, 2017-03-14 22:04:26]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 22:05:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-14 22:22:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-14 22:22:08]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[estark, 2017-03-14 22:28:39]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-14 22:29:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-15 00:13:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-15 00:13:37]

Dry run: This issue passed the CQ dry run.

[estark, 2017-04-18 17:07:25]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-18 17:07:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[estark, 2017-04-18 17:36:01]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-18 17:36:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-18 18:55:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-18 18:55:39]

Dry run: This issue passed the CQ dry run.

[estark, 2017-04-18 22:56:56]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-18 22:57:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[estark, 2017-04-18 22:59:14]

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

[estark, 2017-04-18 22:59:15]

mattm: next Expect-CT CL, PTAL. (I know I'm sending you lots of these -- there's
no particular urgency so please don't feel time-pressured! Thanks.)

[commit-bot: I haz the power, 2017-04-19 00:06:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-19 00:06:55]

Dry run: This issue passed the CQ dry run.

[mattm, 2017-04-20 05:48:29]

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

https://codereview.chromium.org/2753703002/diff/120001/net/http/http_security...
net/http/http_security_headers.cc:382: value.begin(), value.end(), ',',
Am I reading the draft wrong or does it define the delimeter as ";"?

https://codereview.chromium.org/2753703002/diff/120001/net/http/http_security...
net/http/http_security_headers.cc:384:
HttpUtil::NameValuePairsIterator::Quotes::NOT_STRICT);
should that be STRICT_QUOTES? 
(from section 2.1 "In particular, UAs must not attempt to fix malformed header
fields.")

https://codereview.chromium.org/2753703002/diff/120001/net/http/http_security...
net/http/http_security_headers.cc:389: name_value_pairs.name_end()),
could make this StringPiece once at the beginning of the while loop instead of
repeating it in each if condition

https://codereview.chromium.org/2753703002/diff/120001/net/http/http_security...
net/http/http_security_headers.cc:396: parsed_max_age = true;
should there be checks that each directive is only present at most once?

https://codereview.chromium.org/2753703002/diff/120001/net/http/http_security...
net/http/http_security_headers.cc:400: "enforce")) {
check that no value was given for enforce?

https://codereview.chromium.org/2753703002/diff/120001/net/http/http_security...
net/http/http_security_headers.cc:410: parsed_report_uri =
GURL(name_value_pairs.value());
could use a StringPiece for the GURL param too

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

https://codereview.chromium.org/2753703002/diff/120001/net/http/http_security...
net/http/http_security_headers_unittest.cc:1130:
EXPECT_TRUE(ParseExpectCTHeader(",, max-age=394082038  , enfoRce, ,",
shouldn't this be invalid? (separators can only come between directives, and
directives must have a directive-name, which is a token, which must be at least
1 char)

[estark, 2017-04-20 22:33:24]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 22:33:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[estark, 2017-04-20 22:36:09]

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

https://codereview.chromium.org/2753703002/diff/120001/net/http/http_security...
net/http/http_security_headers.cc:382: value.begin(), value.end(), ',',
On 2017/04/20 05:48:29, mattm wrote:
> Am I reading the draft wrong or does it define the delimeter as ";"?

No, my bad, I haven't published an updated draft since it changed to comma in
https://github.com/httpwg/http-extensions/pull/327

https://codereview.chromium.org/2753703002/diff/120001/net/http/http_security...
net/http/http_security_headers.cc:384:
HttpUtil::NameValuePairsIterator::Quotes::NOT_STRICT);
On 2017/04/20 05:48:29, mattm wrote:
> should that be STRICT_QUOTES? 
> (from section 2.1 "In particular, UAs must not attempt to fix malformed header
> fields.")

Done.

https://codereview.chromium.org/2753703002/diff/120001/net/http/http_security...
net/http/http_security_headers.cc:389: name_value_pairs.name_end()),
On 2017/04/20 05:48:29, mattm wrote:
> could make this StringPiece once at the beginning of the while loop instead of
> repeating it in each if condition

Done.

https://codereview.chromium.org/2753703002/diff/120001/net/http/http_security...
net/http/http_security_headers.cc:396: parsed_max_age = true;
On 2017/04/20 05:48:29, mattm wrote:
> should there be checks that each directive is only present at most once?

Done.

https://codereview.chromium.org/2753703002/diff/120001/net/http/http_security...
net/http/http_security_headers.cc:400: "enforce")) {
On 2017/04/20 05:48:29, mattm wrote:
> check that no value was given for enforce?

Done.

https://codereview.chromium.org/2753703002/diff/120001/net/http/http_security...
net/http/http_security_headers.cc:410: parsed_report_uri =
GURL(name_value_pairs.value());
On 2017/04/20 05:48:29, mattm wrote:
> could use a StringPiece for the GURL param too

Done.

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

https://codereview.chromium.org/2753703002/diff/120001/net/http/http_security...
net/http/http_security_headers_unittest.cc:1130:
EXPECT_TRUE(ParseExpectCTHeader(",, max-age=394082038  , enfoRce, ,",
On 2017/04/20 05:48:29, mattm wrote:
> shouldn't this be invalid? (separators can only come between directives, and
> directives must have a directive-name, which is a token, which must be at
least
> 1 char)

You would think so, but buried in RFC 7230, it says "For compatibility with
legacy list rules, a recipient MUST parse and ignore a reasonable number of
empty list elements: enough to handle common mistakes by senders that merge
values, but not so much that they could be used as a denial-of-service
mechanism."(https://tools.ietf.org/html/rfc7230#section-7)

I added a comment with the bit about "parse and ignore a reasonable number of
empty list elements." I don't think the DoS concern applies here, so I think
it's reasonable to match HSTS/HPKP and accept empty elements.

[commit-bot: I haz the power, 2017-04-20 22:36:13]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-20 22:36:13]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[mattm, 2017-04-21 05:15:53]

lgtm

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

https://codereview.chromium.org/2753703002/diff/140001/net/http/http_security...
net/http/http_security_headers.cc:408: if (!name_value_pairs.value().empty()) {
nit: don't need the braces on this one

[mmoroz, 2017-04-21 13:17:57]

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

[mmoroz, 2017-04-21 13:17:58]

Thanks Emily for adding a fuzz target together with a new feature, that's
awesome!

https://codereview.chromium.org/2753703002/diff/140001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/2753703002/diff/140001/net/BUILD.gn#newcode5719
net/BUILD.gn:5719: dict =
"data/fuzzer_dictionaries/net_http_security_headers_fuzzer.dict"
I believe it also would be great to have a `seed_corpus` attribute here pointing
to a directory with a few test inputs (something like test inputs from
http_security_headers_unittest.cc).

[estark, 2017-04-21 16:32:20]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-21 16:32:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[estark, 2017-04-21 16:36:37]

The CQ bit was checked by estark@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-21 16:37:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[estark, 2017-04-21 16:37:31]

https://codereview.chromium.org/2753703002/diff/140001/net/BUILD.gn
File net/BUILD.gn (right):

https://codereview.chromium.org/2753703002/diff/140001/net/BUILD.gn#newcode5719
net/BUILD.gn:5719: dict =
"data/fuzzer_dictionaries/net_http_security_headers_fuzzer.dict"
On 2017/04/21 13:17:58, mmoroz wrote:
> I believe it also would be great to have a `seed_corpus` attribute here
pointing
> to a directory with a few test inputs (something like test inputs from
> http_security_headers_unittest.cc).

Thanks for the tip! I added a seed corpus of the de-duped unit test values from
http_security_headers_unittest.cc. (The HSTS/HPKP/Expect-CT headers are similar
enough that I just used the same corpus for all of them.)

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

https://codereview.chromium.org/2753703002/diff/140001/net/http/http_security...
net/http/http_security_headers.cc:408: if (!name_value_pairs.value().empty()) {
On 2017/04/21 05:15:53, mattm wrote:
> nit: don't need the braces on this one

Done.

[commit-bot: I haz the power, 2017-04-21 17:39:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-21 17:39:10]

Dry run: This issue passed the CQ dry run.

[mmoroz, 2017-04-21 17:52:19]

LGTM for fuzzing stuff, thanks!

[estark, 2017-04-21 17:53:04]

The CQ bit was checked by estark@chromium.org

[estark, 2017-04-21 17:53:05]

The patchset sent to the CQ was uploaded after l-g-t-m from mattm@chromium.org
Link to the patchset: https://codereview.chromium.org/2753703002/#ps180001
(title: "add txt extensions to seed corpus")

[commit-bot: I haz the power, 2017-04-21 17:53:31]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-21 18:01:11]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1492797184902240,
"parent_rev": "2c711f7b56b771b19f2c3d1a458815d0eeafe6df", "commit_rev":
"a57e81656f11b9ec7de47d3fdbe8aa297311b5fa"}

[commit-bot: I haz the power, 2017-04-21 18:01:25]

Description was changed from

==========
Add Expect-CT header parsing

This CL implements parsing of the Expect-CT HTTP header (draft spec at
https://tools.ietf.org/html/draft-ietf-httpbis-expect-ct-00).

BUG=679012
==========

to

==========
Add Expect-CT header parsing

This CL implements parsing of the Expect-CT HTTP header (draft spec at
https://tools.ietf.org/html/draft-ietf-httpbis-expect-ct-00).

BUG=679012

Review-Url: https://codereview.chromium.org/2753703002
Cr-Commit-Position: refs/heads/master@{#466386}
Committed:
https://chromium.googlesource.com/chromium/src/+/a57e81656f11b9ec7de47d3fdbe8...
==========

[commit-bot: I haz the power, 2017-04-21 18:01:27]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/a57e81656f11b9ec7de47d3fdbe8...