Add a parser for messages with a Web Push Protocol-based payload

Add a parser for messages with a Web Push Protocol-based payload

This will help to avoid doing manual parsing and verification of the
inbound messages in multiple places.

BUG=679789
Review-Url: https://codereview.chromium.org/2888763006
Cr-Commit-Position: refs/heads/master@{#473889}
Committed: https://chromium.googlesource.com/chromium/src/+/006f9034020134314605f8a64b0b3f925d1b6012

[Peter Beverloo, 2017-05-18 18:46:13]

peter@chromium.org changed reviewers:
+ eroman@chromium.org, johnme@chromium.org

[Peter Beverloo, 2017-05-18 18:46:14]

+eroman, johnme

This is just generalizing parsing of the binary header. Only used in the test
right now, but fairly straightforward.

[johnme, 2017-05-22 16:18:39]

lgtm with some nits/questions

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

https://codereview.chromium.org/2888763006/diff/1/components/gcm_driver/crypt...
components/gcm_driver/crypto/message_payload_parser.cc:17: constexpr size_t
kUncompressedPointSize = 65;
Nit: perhaps stick to "keyid" for consistency with the spec and class comment?

https://codereview.chromium.org/2888763006/diff/1/components/gcm_driver/crypt...
components/gcm_driver/crypto/message_payload_parser.cc:24: kSaltSize +
sizeof(uint32_t) + sizeof(uint8_t) + kUncompressedPointSize +
Nit: perhaps use sizeof(record_size_) instead of sizeof(uint32_t) for
consistency with below?

https://codereview.chromium.org/2888763006/diff/1/components/gcm_driver/crypt...
components/gcm_driver/crypto/message_payload_parser.cc:36:
base::ReadBigEndian(message.data(), &record_size_);
Do we need to worry about the "This would cause SIGBUS on ARMv5 or earlier and
ARMv6-M." comment?

https://codereview.chromium.org/2888763006/diff/1/components/gcm_driver/crypt...
components/gcm_driver/crypto/message_payload_parser.cc:49: public_key_ =
message.substr(0, kUncompressedPointSize);
It's a little surprising that you mention in the class comment that this must
start with 0x04, but then don't actually check that when validating the header.

https://codereview.chromium.org/2888763006/diff/1/components/gcm_driver/crypt...
components/gcm_driver/crypto/message_payload_parser.cc:54: if
(ciphertext_.size() < kMinimumRecordSize)
Shouldn't this be `(ciphertext_.size() != record_size_)`?

https://codereview.chromium.org/2888763006/diff/1/components/gcm_driver/crypt...
File components/gcm_driver/crypto/message_payload_parser.h (right):

https://codereview.chromium.org/2888763006/diff/1/components/gcm_driver/crypt...
components/gcm_driver/crypto/message_payload_parser.h:29: // Specific to Web
Push encryption, the `keyid` parameter of this header must
I take it that overrides <<A "keyid" parameter SHOULD be a UTF-8 [RFC3629]
encoded string, particularly where the identifier might need to appear in a
textual form.>> from
https://tools.ietf.org/html/draft-ietf-httpbis-encryption-encoding-08#section...

https://codereview.chromium.org/2888763006/diff/1/components/gcm_driver/crypt...
components/gcm_driver/crypto/message_payload_parser.h:36: // Ownership of the
|message| remains with the caller, and must outlive this
Seems a little bit hairy (albeit nice and efficient). Shame we don't have ref
counted strings in chromium :-| Oh well.

https://codereview.chromium.org/2888763006/diff/1/components/gcm_driver/crypt...
components/gcm_driver/crypto/message_payload_parser.h:70: DCHECK(is_valid_);
Nit: One or more of these should probably be CHECKs since they seem to have
security implications?

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

https://codereview.chromium.org/2888763006/diff/1/components/gcm_driver/crypt...
components/gcm_driver/crypto/message_payload_parser_unittest.cc:18: // rs (4
bytes)
Nit: add "in network byte order"

https://codereview.chromium.org/2888763006/diff/1/components/gcm_driver/crypt...
components/gcm_driver/crypto/message_payload_parser_unittest.cc:36: // Creates
an std::string for the |kValidMessage| constant.
Nit: s/an/a/

https://codereview.chromium.org/2888763006/diff/1/components/gcm_driver/crypt...
components/gcm_driver/crypto/message_payload_parser_unittest.cc:44:
EXPECT_TRUE(parser.IsValid());
Consider exercising the getters here?

[Peter Beverloo, 2017-05-22 17:25:13]

Thanks! PTAL John

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

https://codereview.chromium.org/2888763006/diff/1/components/gcm_driver/crypt...
components/gcm_driver/crypto/message_payload_parser.cc:17: constexpr size_t
kUncompressedPointSize = 65;
On 2017/05/22 16:18:39, johnme wrote:
> Nit: perhaps stick to "keyid" for consistency with the spec and class comment?

I've updated the header instead.

https://codereview.chromium.org/2888763006/diff/1/components/gcm_driver/crypt...
components/gcm_driver/crypto/message_payload_parser.cc:24: kSaltSize +
sizeof(uint32_t) + sizeof(uint8_t) + kUncompressedPointSize +
On 2017/05/22 16:18:39, johnme wrote:
> Nit: perhaps use sizeof(record_size_) instead of sizeof(uint32_t) for
> consistency with below?

That's a private class member, so it'd be a bit awkward to..

https://codereview.chromium.org/2888763006/diff/1/components/gcm_driver/crypt...
components/gcm_driver/crypto/message_payload_parser.cc:36:
base::ReadBigEndian(message.data(), &record_size_);
On 2017/05/22 16:18:39, johnme wrote:
> Do we need to worry about the "This would cause SIGBUS on ARMv5 or earlier and
> ARMv6-M." comment?

Looking at code search, it doesn't look like we support ARMv6 anymore? Other
uses of base::ReadBigEndian aren't worried about this.

https://codereview.chromium.org/2888763006/diff/1/components/gcm_driver/crypt...
components/gcm_driver/crypto/message_payload_parser.cc:49: public_key_ =
message.substr(0, kUncompressedPointSize);
On 2017/05/22 16:18:39, johnme wrote:
> It's a little surprising that you mention in the class comment that this must
> start with 0x04, but then don't actually check that when validating the
header.

Done.

https://codereview.chromium.org/2888763006/diff/1/components/gcm_driver/crypt...
components/gcm_driver/crypto/message_payload_parser.cc:54: if
(ciphertext_.size() < kMinimumRecordSize)
On 2017/05/22 16:18:39, johnme wrote:
> Shouldn't this be `(ciphertext_.size() != record_size_)`?

No, because record_size_ can be larger too. (We default to 4096.)

https://codereview.chromium.org/2888763006/diff/1/components/gcm_driver/crypt...
File components/gcm_driver/crypto/message_payload_parser.h (right):

https://codereview.chromium.org/2888763006/diff/1/components/gcm_driver/crypt...
components/gcm_driver/crypto/message_payload_parser.h:29: // Specific to Web
Push encryption, the `keyid` parameter of this header must
On 2017/05/22 16:18:39, johnme wrote:
> I take it that overrides <<A "keyid" parameter SHOULD be a UTF-8 [RFC3629]
> encoded string, particularly where the identifier might need to appear in a
> textual form.>> from
>
https://tools.ietf.org/html/draft-ietf-httpbis-encryption-encoding-08#section...

Yes.

https://codereview.chromium.org/2888763006/diff/1/components/gcm_driver/crypt...
components/gcm_driver/crypto/message_payload_parser.h:36: // Ownership of the
|message| remains with the caller, and must outlive this
On 2017/05/22 16:18:39, johnme wrote:
> Seems a little bit hairy (albeit nice and efficient). Shame we don't have ref
> counted strings in chromium :-| Oh well.

Agreed. This doesn't really make usage prettier either, and in practice we'll
need the copy. Updated.

https://codereview.chromium.org/2888763006/diff/1/components/gcm_driver/crypt...
components/gcm_driver/crypto/message_payload_parser.h:70: DCHECK(is_valid_);
On 2017/05/22 16:18:39, johnme wrote:
> Nit: One or more of these should probably be CHECKs since they seem to have
> security implications?

Done.

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

https://codereview.chromium.org/2888763006/diff/1/components/gcm_driver/crypt...
components/gcm_driver/crypto/message_payload_parser_unittest.cc:18: // rs (4
bytes)
On 2017/05/22 16:18:39, johnme wrote:
> Nit: add "in network byte order"

Done.

https://codereview.chromium.org/2888763006/diff/1/components/gcm_driver/crypt...
components/gcm_driver/crypto/message_payload_parser_unittest.cc:36: // Creates
an std::string for the |kValidMessage| constant.
On 2017/05/22 16:18:39, johnme wrote:
> Nit: s/an/a/

No, "an" is correct :). Esteedee.

https://codereview.chromium.org/2888763006/diff/1/components/gcm_driver/crypt...
components/gcm_driver/crypto/message_payload_parser_unittest.cc:44:
EXPECT_TRUE(parser.IsValid());
On 2017/05/22 16:18:39, johnme wrote:
> Consider exercising the getters here?

Done.

[eroman, 2017-05-22 17:46:03]

lgtm

[Peter Beverloo, 2017-05-23 12:17:46]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2017-05-23 12:17:47]

The patchset sent to the CQ was uploaded after l-g-t-m from johnme@chromium.org
Link to the patchset: https://codereview.chromium.org/2888763006/#ps20001
(title: "Add a parser for messages with a Web Push Protocol-based payload")

[Peter Beverloo, 2017-05-23 12:17:52]

The CQ bit was unchecked by peter@chromium.org

[Peter Beverloo, 2017-05-23 12:37:15]

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

[commit-bot: I haz the power, 2017-05-23 12:37:37]

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-05-23 13:58:08]

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

[commit-bot: I haz the power, 2017-05-23 13:58:09]

Dry run: This issue passed the CQ dry run.

[Peter Beverloo, 2017-05-23 14:32:50]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2017-05-23 14:32:51]

The patchset sent to the CQ was uploaded after l-g-t-m from johnme@chromium.org,
eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2888763006/#ps40001
(title: "rebase")

[commit-bot: I haz the power, 2017-05-23 14:33:11]

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-05-23 14:37:52]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1495549970943660,
"parent_rev": "795c4f100e123db3775930dd6e170a9b56b12657", "commit_rev":
"006f9034020134314605f8a64b0b3f925d1b6012"}

[commit-bot: I haz the power, 2017-05-23 14:38:12]

Description was changed from

==========
Add a parser for messages with a Web Push Protocol-based payload

This will help to avoid doing manual parsing and verification of the
inbound messages in multiple places.

BUG=679789
==========

to

==========
Add a parser for messages with a Web Push Protocol-based payload

This will help to avoid doing manual parsing and verification of the
inbound messages in multiple places.

BUG=679789

Review-Url: https://codereview.chromium.org/2888763006
Cr-Commit-Position: refs/heads/master@{#473889}
Committed:
https://chromium.googlesource.com/chromium/src/+/006f9034020134314605f8a64b0b...
==========

[commit-bot: I haz the power, 2017-05-23 14:38:12]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/006f9034020134314605f8a64b0b...