Separate out the scheme from the GCMMessageCryptographer

Separate out the scheme from the GCMMessageCryptographer

Chrome currently implements draft-ietf-webpush-encryption-03, but
following more discussion and a number of changes the Web Push working
group last week announced last call for draft-ietf-webpush-encryption-08.

In order to be able to support both schemes, this CL refactors the
GCMMessageCryptographer to separate out the operations associated with
the version of the encryption scheme.

In addition, the tests are refactored to easily enable using
parameterized tests for those that can be shared between the drafts. The
reference test that doesn't use the auth_secret has been removed, since
we don't actually ship that functionality.

BUG=679789
Review-Url: https://codereview.chromium.org/2713673002
Cr-Commit-Position: refs/heads/master@{#470067}
Committed: https://chromium.googlesource.com/chromium/src/+/17ae9f3ad85e9ad139bd283908ba4c0529659fce

[Peter Beverloo, 2017-02-22 15:15:58]

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

[Peter Beverloo, 2017-02-22 15:15:59]

+davidben

Hi David, would you mind giving this a review?

In short, we'll want to support the final version (currently in WGLC) of the
encryption scheme soon. This CL prepares us for supporting that.

This looks big, but the changes of interest are isolated:

- Changing the structure of gcm_message_cryptographer.h (+comments)
- Removal of the "auth secret"-less reference test because we don't use it.
- The TestVector tests now use their own fixture so that we can use
TEST_P<Draft03, Draft08> for all the common tests.

The rest of the changes are purely mechanical to support this.

[Peter Beverloo, 2017-02-22 15:16:03]

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

[commit-bot: I haz the power, 2017-02-22 15:16:19]

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-02-22 15:43:19]

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

[commit-bot: I haz the power, 2017-02-22 15:43:20]

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

[Peter Beverloo, 2017-02-22 17:30:26]

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

[commit-bot: I haz the power, 2017-02-22 17:31:15]

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-02-22 18:16:26]

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

[commit-bot: I haz the power, 2017-02-22 18:16:27]

Dry run: This issue passed the CQ dry run.

[Peter Beverloo, 2017-02-22 20:37:36]

Sorry, please hold off for at least today. The third CL can be seen here, but I
need to propagate a bunch of clean-ups back to this one.

https://codereview.chromium.org/2716443002

[Peter Beverloo, 2017-02-23 16:36:32]

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

[commit-bot: I haz the power, 2017-02-23 16:37:01]

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

[Peter Beverloo, 2017-02-23 16:53:46]

This is now ready. The full package can be seen in
https://codereview.chromium.org/2716443002

[commit-bot: I haz the power, 2017-02-23 17:18:07]

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

[commit-bot: I haz the power, 2017-02-23 17:18:08]

Dry run: This issue passed the CQ dry run.

[martijnc, 2017-03-28 16:26:36]

drive-by nits :)

https://codereview.chromium.org/2713673002/diff/40001/components/gcm_driver/c...
File components/gcm_driver/crypto/gcm_message_cryptographer.cc (right):

https://codereview.chromium.org/2713673002/diff/40001/components/gcm_driver/c...
components/gcm_driver/crypto/gcm_message_cryptographer.cc:166: switch (version)
{
nit: I think

switch (version) {
  case Version::DRAFT_03:
    ..
    break;
  default:
    NOTREACHED();
}

is more common (and maybe a little more readable).

https://codereview.chromium.org/2713673002/diff/40001/components/gcm_driver/c...
File components/gcm_driver/crypto/gcm_message_cryptographer.h (right):

https://codereview.chromium.org/2713673002/diff/40001/components/gcm_driver/c...
components/gcm_driver/crypto/gcm_message_cryptographer.h:54: // Derives the
pseuro random key (PRK) to use for deriving the content
typo: pseudo

https://codereview.chromium.org/2713673002/diff/40001/components/gcm_driver/c...
File components/gcm_driver/crypto/gcm_message_cryptographer_unittest.cc (right):

https://codereview.chromium.org/2713673002/diff/40001/components/gcm_driver/c...
components/gcm_driver/crypto/gcm_message_cryptographer_unittest.cc:495: // And
verify that decrypting themessageyields the plaintext again.
typo: missing spaces

[Peter Beverloo, 2017-04-07 17:48:22]

Thanks Martijn!

David, this has been open for quite a while and I'd really love to be able to
ship it. (Not only because that'd allow us to coordinate the launch with
Mozilla.) Would you have time to take a look at the sequence of CLs?

https://codereview.chromium.org/2713673002/diff/40001/components/gcm_driver/c...
File components/gcm_driver/crypto/gcm_message_cryptographer.cc (right):

https://codereview.chromium.org/2713673002/diff/40001/components/gcm_driver/c...
components/gcm_driver/crypto/gcm_message_cryptographer.cc:166: switch (version)
{
On 2017/03/28 16:26:36, martijnc wrote:
> nit: I think
> 
> switch (version) {
>   case Version::DRAFT_03:
>     ..
>     break;
>   default:
>     NOTREACHED();
> }
> 
> is more common (and maybe a little more readable).

Not having a `default` clause gives us compile-time verification that all cases
have been handled though, which is a big pro.

https://codereview.chromium.org/2713673002/diff/40001/components/gcm_driver/c...
File components/gcm_driver/crypto/gcm_message_cryptographer.h (right):

https://codereview.chromium.org/2713673002/diff/40001/components/gcm_driver/c...
components/gcm_driver/crypto/gcm_message_cryptographer.h:54: // Derives the
pseuro random key (PRK) to use for deriving the content
On 2017/03/28 16:26:36, martijnc wrote:
> typo: pseudo

Done.

https://codereview.chromium.org/2713673002/diff/40001/components/gcm_driver/c...
File components/gcm_driver/crypto/gcm_message_cryptographer_unittest.cc (right):

https://codereview.chromium.org/2713673002/diff/40001/components/gcm_driver/c...
components/gcm_driver/crypto/gcm_message_cryptographer_unittest.cc:495: // And
verify that decrypting themessageyields the plaintext again.
On 2017/03/28 16:26:36, martijnc wrote:
> typo: missing spaces

What how. Fixed, thanks!

[eroman, 2017-05-05 20:52:25]

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

[eroman, 2017-05-05 20:52:26]

LGTM as a code refactor.

(I didn't review the specific web push draft changes to check if the abstraction
makes sense though).

https://codereview.chromium.org/2713673002/diff/60001/components/gcm_driver/c...
File components/gcm_driver/crypto/gcm_message_cryptographer.cc (right):

https://codereview.chromium.org/2713673002/diff/60001/components/gcm_driver/c...
components/gcm_driver/crypto/gcm_message_cryptographer.cc:57: info_stream <<
"Content-Encoding: auth" << '\x00';
side-note: not sure stringstream makes sense for this (it is a literal). Given
this is a refactor, we can discuss this separately.

https://codereview.chromium.org/2713673002/diff/60001/components/gcm_driver/c...
components/gcm_driver/crypto/gcm_message_cryptographer.cc:230:
DCHECK_EQ(recipient_public_key.size(), 65u);
What layer is responsible for ensuring the parameters are the correct lengths?

https://codereview.chromium.org/2713673002/diff/60001/components/gcm_driver/c...
components/gcm_driver/crypto/gcm_message_cryptographer.cc:253: if
(ciphertext.size() < kAuthenticationTagBytes ||
Was this bound change on ciphertext intentional?

https://codereview.chromium.org/2713673002/diff/60001/components/gcm_driver/c...
components/gcm_driver/crypto/gcm_message_cryptographer.cc:357: //
draft-ietf-httpbis-encryption-encoding defines that the result should
specify draft version / full URL?

https://codereview.chromium.org/2713673002/diff/60001/components/gcm_driver/c...
components/gcm_driver/crypto/gcm_message_cryptographer.cc:359: // is limited to
a single record per draft-ietf-webpush-encryption.
ditto.

[Peter Beverloo, 2017-05-08 16:08:27]

Thank you!

https://codereview.chromium.org/2713673002/diff/60001/components/gcm_driver/c...
File components/gcm_driver/crypto/gcm_message_cryptographer.cc (right):

https://codereview.chromium.org/2713673002/diff/60001/components/gcm_driver/c...
components/gcm_driver/crypto/gcm_message_cryptographer.cc:57: info_stream <<
"Content-Encoding: auth" << '\x00';
On 2017/05/05 20:52:26, eroman wrote:
> side-note: not sure stringstream makes sense for this (it is a literal). Given
> this is a refactor, we can discuss this separately.

That seems good to me. Since the -08 CL does something similar we can unify
changes there.

https://codereview.chromium.org/2713673002/diff/60001/components/gcm_driver/c...
components/gcm_driver/crypto/gcm_message_cryptographer.cc:230:
DCHECK_EQ(recipient_public_key.size(), 65u);
On 2017/05/05 20:52:26, eroman wrote:
> What layer is responsible for ensuring the parameters are the correct lengths?

The caller. We make this even more strict in the subsequent CL that you've
already seen. Conceptually, the caller deals with validating the input, making
this class solely responsible for the cryptographic operations.

https://codereview.chromium.org/2713673002/diff/60001/components/gcm_driver/c...
components/gcm_driver/crypto/gcm_message_cryptographer.cc:253: if
(ciphertext.size() < kAuthenticationTagBytes ||
On 2017/05/05 20:52:26, eroman wrote:
> Was this bound change on ciphertext intentional?

Oops - no, not for this CL. The padding's semantics are changing in the -08
draft which means this check will change, but that's not applicable yet.

Reverted.

https://codereview.chromium.org/2713673002/diff/60001/components/gcm_driver/c...
components/gcm_driver/crypto/gcm_message_cryptographer.cc:357: //
draft-ietf-httpbis-encryption-encoding defines that the result should
On 2017/05/05 20:52:26, eroman wrote:
> specify draft version / full URL?

Done. I'll amend the comment again in the third CL.

https://codereview.chromium.org/2713673002/diff/60001/components/gcm_driver/c...
components/gcm_driver/crypto/gcm_message_cryptographer.cc:359: // is limited to
a single record per draft-ietf-webpush-encryption.
On 2017/05/05 20:52:26, eroman wrote:
> ditto.

Done.

[Peter Beverloo, 2017-05-08 16:09:25]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2017-05-08 16:09:25]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2713673002/#ps80001
(title: "comments")

[commit-bot: I haz the power, 2017-05-08 16:10:08]

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-08 16:27:09]

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

[commit-bot: I haz the power, 2017-05-08 16:27:09]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Peter Beverloo, 2017-05-08 17:29:23]

The CQ bit was checked by peter@chromium.org

[commit-bot: I haz the power, 2017-05-08 17:30:09]

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-08 18:56:29]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1494264563891560,
"parent_rev": "11539c86d2827abf795281446643991e6705790a", "commit_rev":
"17ae9f3ad85e9ad139bd283908ba4c0529659fce"}

[commit-bot: I haz the power, 2017-05-08 18:56:40]

Description was changed from

==========
Separate out the scheme from the GCMMessageCryptographer

Chrome currently implements draft-ietf-webpush-encryption-03, but
following more discussion and a number of changes the Web Push working
group last week announced last call for draft-ietf-webpush-encryption-08.

In order to be able to support both schemes, this CL refactors the
GCMMessageCryptographer to separate out the operations associated with
the version of the encryption scheme.

In addition, the tests are refactored to easily enable using
parameterized tests for those that can be shared between the drafts. The
reference test that doesn't use the auth_secret has been removed, since
we don't actually ship that functionality.

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

to

==========
Separate out the scheme from the GCMMessageCryptographer

Chrome currently implements draft-ietf-webpush-encryption-03, but
following more discussion and a number of changes the Web Push working
group last week announced last call for draft-ietf-webpush-encryption-08.

In order to be able to support both schemes, this CL refactors the
GCMMessageCryptographer to separate out the operations associated with
the version of the encryption scheme.

In addition, the tests are refactored to easily enable using
parameterized tests for those that can be shared between the drafts. The
reference test that doesn't use the auth_secret has been removed, since
we don't actually ship that functionality.

BUG=679789

Review-Url: https://codereview.chromium.org/2713673002
Cr-Commit-Position: refs/heads/master@{#470067}
Committed:
https://chromium.googlesource.com/chromium/src/+/17ae9f3ad85e9ad139bd283908ba...
==========

[commit-bot: I haz the power, 2017-05-08 18:56:41]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/17ae9f3ad85e9ad139bd283908ba...