Convert the encryption header parsers to be iterator-based.

Convert the encryption header parsers to be iterator-based.

BUG=538576
Committed: https://crrev.com/81a290d32a901677d819e80786f18d09dc30eeb1
Cr-Commit-Position: refs/heads/master@{#402914}

[Peter Beverloo, 2015-12-07 20:55:39]

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

[Peter Beverloo, 2015-12-07 20:55:40]

+rsleevi

[Ryan Sleevi, 2015-12-08 00:22:00]

Is this a precursor to supporting multiple layers of encryption? Or just general
cleanup?

A few mostly nit-y things, otherwise it's looking good.

https://codereview.chromium.org/1509683002/diff/1/components/gcm_driver/crypt...
File components/gcm_driver/crypto/encryption_header_parsers.cc (right):

https://codereview.chromium.org/1509683002/diff/1/components/gcm_driver/crypt...
components/gcm_driver/crypto/encryption_header_parsers.cc:46: // compatibility
with UAs that use other number parsing mechanisms.
This comment leaves me a bit confused.

Do we reject "+" because it's not spec valid? Or because it is, but other UAs
aren't conforming?

https://codereview.chromium.org/1509683002/diff/1/components/gcm_driver/crypt...
components/gcm_driver/crypto/encryption_header_parsers.cc:47: if (value[0] ==
'+')
But is '-' allowed? Or are you relying on the Uint64 to cause that to be
rejected?

https://codereview.chromium.org/1509683002/diff/1/components/gcm_driver/crypt...
components/gcm_driver/crypto/encryption_header_parsers.cc:52: candidate_rs <= 1
/* the record size MUST be greater than 1 */)
Please add braces to this conditional if kept as multi-line.

Also, the inline comment style seems... less common for Chromium code

if (!base::StringToUint64(value, &candidate_rs))
  return false;

// The record size MUST be greater than one byte.
if (candidate_rs <= 1)
  return false;

https://codereview.chromium.org/1509683002/diff/1/components/gcm_driver/crypt...
components/gcm_driver/crypto/encryption_header_parsers.cc:86: 
SPEC BUG: The spec is unclear whether multiple parameters are valid, and if so,
what the behaviour of the client should be.

Presently, you've opted for the "last parameter wins", but in other places, it's
"one and only one instance of this parameter permitted". It would be good for
the spec to be clearer on this.

https://codereview.chromium.org/1509683002/diff/1/components/gcm_driver/crypt...
File components/gcm_driver/crypto/gcm_encryption_provider.cc (right):

https://codereview.chromium.org/1509683002/diff/1/components/gcm_driver/crypt...
components/gcm_driver/crypto/gcm_encryption_provider.cc:111: if
(!crypto_key_header_iterator.dh().size()) {
Use .empty(), not .size(), to test for empty STL containers.

[Peter Beverloo, 2015-12-08 01:02:58]

Thank you. I'll apply the code changes tomorrow, but wanted to reply to your
questions.

> Is this a precursor to supporting multiple layers of encryption? Or just
> general cleanup?

We will not support multiple layers or encryption/compression - this is a
general clean-up I promised you to do.

To cite draft-ietf-webpush-encryption: "The Content-Encoding header field
therefore has exactly one value, which is "aesgcm128". Multiple "aesgcm128"
values are not permitted."

https://codereview.chromium.org/1509683002/diff/1/components/gcm_driver/crypt...
File components/gcm_driver/crypto/encryption_header_parsers.cc (right):

https://codereview.chromium.org/1509683002/diff/1/components/gcm_driver/crypt...
components/gcm_driver/crypto/encryption_header_parsers.cc:46: // compatibility
with UAs that use other number parsing mechanisms.
On 2015/12/08 00:22:00, Ryan Sleevi wrote:
> This comment leaves me a bit confused.
> 
> Do we reject "+" because it's not spec valid? Or because it is, but other UAs
> aren't conforming?

Will rephrase. We're being strict here, the spec defines this to be a "positive
decimal integer". This was based on a review by davidben@ IIRC.

https://codereview.chromium.org/1509683002/diff/1/components/gcm_driver/crypt...
components/gcm_driver/crypto/encryption_header_parsers.cc:47: if (value[0] ==
'+')
On 2015/12/08 00:22:00, Ryan Sleevi wrote:
> But is '-' allowed? Or are you relying on the Uint64 to cause that to be
> rejected?

IteratorRangeToNumber::Invoke will consider the input to be invalid when an '-'
is included for a non-signed type (which uint64_t is), so yes.

https://codereview.chromium.org/1509683002/diff/1/components/gcm_driver/crypt...
components/gcm_driver/crypto/encryption_header_parsers.cc:86: 
On 2015/12/08 00:22:00, Ryan Sleevi wrote:
> SPEC BUG: The spec is unclear whether multiple parameters are valid, and if
so,
> what the behaviour of the client should be.
> 
> Presently, you've opted for the "last parameter wins", but in other places,
it's
> "one and only one instance of this parameter permitted". It would be good for
> the spec to be clearer on this.

I will take an action item to generate a PR against the spec.

[Peter Beverloo, 2015-12-16 21:09:57]

https://codereview.chromium.org/1509683002/diff/1/components/gcm_driver/crypt...
File components/gcm_driver/crypto/encryption_header_parsers.cc (right):

https://codereview.chromium.org/1509683002/diff/1/components/gcm_driver/crypt...
components/gcm_driver/crypto/encryption_header_parsers.cc:46: // compatibility
with UAs that use other number parsing mechanisms.
On 2015/12/08 01:02:57, Peter Beverloo wrote:
> On 2015/12/08 00:22:00, Ryan Sleevi wrote:
> > This comment leaves me a bit confused.
> > 
> > Do we reject "+" because it's not spec valid? Or because it is, but other
UAs
> > aren't conforming?
> 
> Will rephrase. We're being strict here, the spec defines this to be a
"positive
> decimal integer". This was based on a review by davidben@ IIRC.

Done.

https://codereview.chromium.org/1509683002/diff/1/components/gcm_driver/crypt...
components/gcm_driver/crypto/encryption_header_parsers.cc:52: candidate_rs <= 1
/* the record size MUST be greater than 1 */)
On 2015/12/08 00:22:00, Ryan Sleevi wrote:
> Please add braces to this conditional if kept as multi-line.
> 
> Also, the inline comment style seems... less common for Chromium code
> 
> if (!base::StringToUint64(value, &candidate_rs))
>   return false;
> 
> // The record size MUST be greater than one byte.
> if (candidate_rs <= 1)
>   return false;

Done.

https://codereview.chromium.org/1509683002/diff/1/components/gcm_driver/crypt...
components/gcm_driver/crypto/encryption_header_parsers.cc:86: 
On 2015/12/08 01:02:58, Peter Beverloo wrote:
> On 2015/12/08 00:22:00, Ryan Sleevi wrote:
> > SPEC BUG: The spec is unclear whether multiple parameters are valid, and if
> so,
> > what the behaviour of the client should be.
> > 
> > Presently, you've opted for the "last parameter wins", but in other places,
> it's
> > "one and only one instance of this parameter permitted". It would be good
for
> > the spec to be clearer on this.
> 
> I will take an action item to generate a PR against the spec.

https://github.com/martinthomson/http-encryption/pull/6

https://codereview.chromium.org/1509683002/diff/1/components/gcm_driver/crypt...
File components/gcm_driver/crypto/gcm_encryption_provider.cc (right):

https://codereview.chromium.org/1509683002/diff/1/components/gcm_driver/crypt...
components/gcm_driver/crypto/gcm_encryption_provider.cc:111: if
(!crypto_key_header_iterator.dh().size()) {
On 2015/12/08 00:22:00, Ryan Sleevi wrote:
> Use .empty(), not .size(), to test for empty STL containers.

Done.

[Ryan Sleevi, 2015-12-16 21:21:27]

LGTM; a few nits but probably for follow-ups

https://codereview.chromium.org/1509683002/diff/20001/components/gcm_driver/c...
File components/gcm_driver/crypto/encryption_header_parsers.cc (right):

https://codereview.chromium.org/1509683002/diff/20001/components/gcm_driver/c...
components/gcm_driver/crypto/encryption_header_parsers.cc:27: std::string
candidate_salt;
Do you need the temporary? Shouldn't the API contract of Base64UrlDecode leave
the value unmolested, meaning you can just pass the pointer and avoid the swap?

https://codereview.chromium.org/1509683002/diff/20001/components/gcm_driver/c...
components/gcm_driver/crypto/encryption_header_parsers.cc:100: }
If your PR is accepted, won't this be non-spec-compliant?

https://codereview.chromium.org/1509683002/diff/20001/components/gcm_driver/c...
File components/gcm_driver/crypto/encryption_header_parsers_unittest.cc (right):

https://codereview.chromium.org/1509683002/diff/20001/components/gcm_driver/c...
components/gcm_driver/crypto/encryption_header_parsers_unittest.cc:131: };
As an aside (and perhaps not for this CL), the downside with a testing approach
like this is that it doesn't really clarify why things should fail; perhaps some
comments and grouping explanations may help (consider this more a remark than a
request for this CL; probably something as a cleanup CL later)

https://codereview.chromium.org/1509683002/diff/20001/components/gcm_driver/c...
components/gcm_driver/crypto/encryption_header_parsers_unittest.cc:298:
TEST(EncryptionHeaderParsersTest, InvalidHeadersResetOutput) {
Some comments here could help

[Peter Beverloo, 2015-12-16 21:35:35]

Thank you - all done.

https://codereview.chromium.org/1509683002/diff/20001/components/gcm_driver/c...
File components/gcm_driver/crypto/encryption_header_parsers.cc (right):

https://codereview.chromium.org/1509683002/diff/20001/components/gcm_driver/c...
components/gcm_driver/crypto/encryption_header_parsers.cc:27: std::string
candidate_salt;
On 2015/12/16 21:21:27, Ryan Sleevi wrote:
> Do you need the temporary? Shouldn't the API contract of Base64UrlDecode leave
> the value unmolested, meaning you can just pass the pointer and avoid the
swap?

Doh. I wrote that API. Fixed.

https://codereview.chromium.org/1509683002/diff/20001/components/gcm_driver/c...
components/gcm_driver/crypto/encryption_header_parsers.cc:100: }
On 2015/12/16 21:21:27, Ryan Sleevi wrote:
> If your PR is accepted, won't this be non-spec-compliant?

Yes, but it'll be an easy fix. I'm not in a hurry with this CL, so let's keep
this open until Martin replies.

https://codereview.chromium.org/1509683002/diff/20001/components/gcm_driver/c...
File components/gcm_driver/crypto/encryption_header_parsers_unittest.cc (right):

https://codereview.chromium.org/1509683002/diff/20001/components/gcm_driver/c...
components/gcm_driver/crypto/encryption_header_parsers_unittest.cc:131: };
On 2015/12/16 21:21:27, Ryan Sleevi wrote:
> As an aside (and perhaps not for this CL), the downside with a testing
approach
> like this is that it doesn't really clarify why things should fail; perhaps
some
> comments and grouping explanations may help (consider this more a remark than
a
> request for this CL; probably something as a cleanup CL later)

Realistically, I'm unlikely to touch this code anytime soon, so better to do it
right now. Added comments and grouping where appropriate.

https://codereview.chromium.org/1509683002/diff/20001/components/gcm_driver/c...
components/gcm_driver/crypto/encryption_header_parsers_unittest.cc:298:
TEST(EncryptionHeaderParsersTest, InvalidHeadersResetOutput) {
On 2015/12/16 21:21:27, Ryan Sleevi wrote:
> Some comments here could help

Done.

[Ryan Sleevi, 2016-03-11 17:54:03]

Old CL poke :)

[Peter Beverloo, 2016-03-11 17:55:59]

On 2016/03/11 17:54:03, Ryan Sleevi wrote:
> Old CL poke :)

I haven't forgotten :-). Trying to find a nice way to do the
no-repeated-parameters checks, I much like the new code and it'd be a shame to
crash nine lines of boolean checks (per function) within it...

[Ryan Sleevi, 2016-05-16 20:55:25]

On 2016/03/11 17:55:59, Peter Beverloo wrote:
> On 2016/03/11 17:54:03, Ryan Sleevi wrote:
> > Old CL poke :)
> 
> I haven't forgotten :-). Trying to find a nice way to do the
> no-repeated-parameters checks, I much like the new code and it'd be a shame to
> crash nine lines of boolean checks (per function) within it...

2 month poke? :)

[Peter Beverloo, 2016-06-29 18:47:50]

Rebased, and putting this on the queue. I'll do the non-repeatable params check
in a follow-up CL, together with a spec change[1] that I also want included in
M53.

[1] https://github.com/webpush-wg/webpush-encryption/pull/7

[Peter Beverloo, 2016-06-29 18:48:01]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2016-06-29 18:48:01]

The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/1509683002/#ps60001
(title: "Convert the encryption header parsers to be iterator-based.")

[commit-bot: I haz the power, 2016-06-29 18:49:17]

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, 2016-06-29 20:00:32]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-29 20:01:28]

Description was changed from

==========
Convert the encryption header parsers to be iterator-based.

BUG=538576
==========

to

==========
Convert the encryption header parsers to be iterator-based.

BUG=538576

Committed: https://crrev.com/81a290d32a901677d819e80786f18d09dc30eeb1
Cr-Commit-Position: refs/heads/master@{#402914}
==========

[commit-bot: I haz the power, 2016-06-29 20:01:30]

Patchset 4 (id:??) landed as
https://crrev.com/81a290d32a901677d819e80786f18d09dc30eeb1
Cr-Commit-Position: refs/heads/master@{#402914}