Teach the GCM Driver how to decrypt incoming messages.

Teach the GCM Driver how to decrypt incoming messages.

This CL teaches the GCM Driver how it can decrypt incoming messages
according to draft-thomson-webpush-encryption-01 and
draft-thomson-http-encryption-01.

Only the Web Push protocol is supported, decryption for messages
sent through the regular GCM protocol will be supported later.

While this makes end-to-end encryption work, we do *not* enable this
functionality by default yet, pending resolution of some ongoing
discussions with the IETF and W3C Push working groups.

BUG=486040
Committed: https://crrev.com/add31f6fb477348d2b6a755cc8b6107a7fcb5339
Cr-Commit-Position: refs/heads/master@{#353256}

[johnme, 2015-07-21 13:52:20]

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

[johnme, 2015-07-21 13:52:21]

https://codereview.chromium.org/1243563002/diff/20001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/1243563002/diff/20001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:199: data =
message.raw_data.size();
Remove the .size()

[Peter Beverloo, 2015-08-03 18:52:07]

Patchset #3 (id:40001) has been deleted

[Peter Beverloo, 2015-08-03 18:55:33]

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

[Peter Beverloo, 2015-08-03 18:55:34]

+johnme, jianli

This should be ready for a first round of review. Code Review fails to realize,
but this depends on the header CL:

https://codereview.chromium.org/1244803002/

https://codereview.chromium.org/1243563002/diff/20001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/1243563002/diff/20001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:199: data =
message.raw_data.size();
On 2015/07/21 13:52:20, johnme wrote:
> Remove the .size()

Done.

[jianli, 2015-08-04 00:49:30]

It seems that we also support decrypting data stored in kEncryptionDataProperty
on desktop. But there is not any test to cover that. Could you please add one?

https://codereview.chromium.org/1243563002/diff/60001/components/gcm_driver/c...
File components/gcm_driver/crypto/encryption_header_parsers.h (right):

https://codereview.chromium.org/1243563002/diff/60001/components/gcm_driver/c...
components/gcm_driver/crypto/encryption_header_parsers.h:41: // Decodes |input|
as a base64url string into |output|.
Please also comment on the returned boolean value.

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

https://codereview.chromium.org/1243563002/diff/60001/components/gcm_driver/c...
components/gcm_driver/crypto/gcm_encryption_provider.cc:20: const char
kEncryptionProperty[] = "encryption";
All these constants, including the previously added one should be wrapped inside
an anonymous namespace.

https://codereview.chromium.org/1243563002/diff/60001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver.h (right):

https://codereview.chromium.org/1243563002/diff/60001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.h:291: void DispatchDecryptedMessage(const
std::string& app_id,
This method might also dispatch an error event in the future. So the name
DispatchDecryptedMessage does not reflect this potential new behavior. How about
DispatchMessageUponDecryption?

[Peter Beverloo, 2015-09-25 16:37:35]

Thank you for the review! I have updated this CL and it's ready for another
look.

Two notes:
  - Decryption from non-Web Push messages is not included for now, I will create
this in a later CL. (GCM may be able to do this for us.)

  - I haven't written a full end-to-end test at the level of the
GCMEncryptionProvider yet, and will add that unit test on Monday.

https://codereview.chromium.org/1243563002/diff/60001/components/gcm_driver/c...
File components/gcm_driver/crypto/encryption_header_parsers.h (right):

https://codereview.chromium.org/1243563002/diff/60001/components/gcm_driver/c...
components/gcm_driver/crypto/encryption_header_parsers.h:41: // Decodes |input|
as a base64url string into |output|.
On 2015/08/04 00:49:30, jianli wrote:
> Please also comment on the returned boolean value.

This doesn't apply anymore.

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

https://codereview.chromium.org/1243563002/diff/60001/components/gcm_driver/c...
components/gcm_driver/crypto/gcm_encryption_provider.cc:20: const char
kEncryptionProperty[] = "encryption";
On 2015/08/04 00:49:30, jianli wrote:
> All these constants, including the previously added one should be wrapped
inside
> an anonymous namespace.

Done for consistency, but please do note that constants will be file-scoped
*unless* they're explicitly being externalized using an "extern" keyword
elsewhere.

https://codereview.chromium.org/1243563002/diff/60001/components/gcm_driver/g...
File components/gcm_driver/gcm_driver.h (right):

https://codereview.chromium.org/1243563002/diff/60001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.h:291: void DispatchDecryptedMessage(const
std::string& app_id,
On 2015/08/04 00:49:30, jianli wrote:
> This method might also dispatch an error event in the future. So the name
> DispatchDecryptedMessage does not reflect this potential new behavior. How
about
> DispatchMessageUponDecryption?

This doesn't apply anymore due to re-using DispatchMessage().

[Peter Beverloo, 2015-09-28 13:53:36]

Patchset #7 (id:140001) has been deleted

[Peter Beverloo, 2015-09-28 14:02:59]

Patchset #7 (id:160001) has been deleted

[Peter Beverloo, 2015-10-01 15:05:15]

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

[Peter Beverloo, 2015-10-01 15:05:16]

+mvanouwerkerk for review as well (friendly ping to others?)

[Michael van Ouwerkerk, 2015-10-02 14:30:42]

Just some nits. lgtm

https://codereview.chromium.org/1243563002/diff/200001/components/gcm_driver/...
File components/gcm_driver/crypto/gcm_encryption_provider.cc (right):

https://codereview.chromium.org/1243563002/diff/200001/components/gcm_driver/...
components/gcm_driver/crypto/gcm_encryption_provider.cc:17: #include
"crypto/curve25519.h"
I thought we settled on curve P-256?

https://codereview.chromium.org/1243563002/diff/200001/components/gcm_driver/...
components/gcm_driver/crypto/gcm_encryption_provider.cc:87: // Callers are
expected to call IsEncryptedMessage() prior to this method.
Why not DCHECK(IsEncryptedMessage()) instead of checking parts of what its
implementation does?

https://codereview.chromium.org/1243563002/diff/200001/components/gcm_driver/...
components/gcm_driver/crypto/gcm_encryption_provider.cc:117: DLOG(ERROR) <<
"Invalid values supplied in the Encryption header";
Encryption-Key

https://codereview.chromium.org/1243563002/diff/200001/components/gcm_driver/...
File components/gcm_driver/crypto/gcm_encryption_provider.h (right):

https://codereview.chromium.org/1243563002/diff/200001/components/gcm_driver/...
components/gcm_driver/crypto/gcm_encryption_provider.h:51:
DECRYPTION_FAILURE_UNKNOWN
nit: Consider making this the first value as it might not always be the last.

https://codereview.chromium.org/1243563002/diff/200001/components/gcm_driver/...
components/gcm_driver/crypto/gcm_encryption_provider.h:85:
FRIEND_TEST_ALL_PREFIXES(GCMEncryptionProviderTest, EncryptionRoundTrip);
Why not friend class GCMEncryptionProviderTest? It doesn't lead to a long list
of FRIEND_TEST_ALL_PREFIXES.

https://codereview.chromium.org/1243563002/diff/200001/components/gcm_driver/...
File components/gcm_driver/crypto/gcm_encryption_provider_unittest.cc (right):

https://codereview.chromium.org/1243563002/diff/200001/components/gcm_driver/...
components/gcm_driver/crypto/gcm_encryption_provider_unittest.cc:261:
base::RunLoop().RunUntilIdle();
nit: document what these loop runs are for.

https://codereview.chromium.org/1243563002/diff/200001/components/gcm_driver/...
File components/gcm_driver/gcm_driver.cc (right):

https://codereview.chromium.org/1243563002/diff/200001/components/gcm_driver/...
components/gcm_driver/gcm_driver.cc:20: void DecryptionFailedCallback(
nit: Can we add some Info level logging for when this happens? Or some UMA?

https://codereview.chromium.org/1243563002/diff/200001/components/gcm_driver/...
File components/gcm_driver/gcm_driver.h (right):

https://codereview.chromium.org/1243563002/diff/200001/components/gcm_driver/...
components/gcm_driver/gcm_driver.h:271: // If the |message| has been encrypted,
it will be asynchronously decrypted
nit: This left me wondering whether it will be dispatched synchronously when it
is not encrypted.

[Peter Beverloo, 2015-10-02 15:01:22]

Thank you for the review Michael!!

https://codereview.chromium.org/1243563002/diff/200001/components/gcm_driver/...
File components/gcm_driver/crypto/gcm_encryption_provider.cc (right):

https://codereview.chromium.org/1243563002/diff/200001/components/gcm_driver/...
components/gcm_driver/crypto/gcm_encryption_provider.cc:17: #include
"crypto/curve25519.h"
On 2015/10/02 14:30:41, Michael van Ouwerkerk wrote:
> I thought we settled on curve P-256?

Yes, but we'll switch over in M48. (The branch point is today, and I'd like to
switch over all of the GCM component at once.)

https://codereview.chromium.org/1243563002/diff/200001/components/gcm_driver/...
components/gcm_driver/crypto/gcm_encryption_provider.cc:87: // Callers are
expected to call IsEncryptedMessage() prior to this method.
On 2015/10/02 14:30:41, Michael van Ouwerkerk wrote:
> Why not DCHECK(IsEncryptedMessage()) instead of checking parts of what its
> implementation does?

|message| is a const reference to IncomingMessage, whereas operator[] (which
we'd use if we were certain that the keys existed) requires a non-const object.

http://www.cplusplus.com/reference/map/map/operator[]/

The std::map<>.at() method is new in C++11, and cannot be used yet.

Since we need to use the iterators in either case, I think it's best to be
explicit about it rather than implicitly checking them through a different
method?

https://codereview.chromium.org/1243563002/diff/200001/components/gcm_driver/...
components/gcm_driver/crypto/gcm_encryption_provider.cc:117: DLOG(ERROR) <<
"Invalid values supplied in the Encryption header";
On 2015/10/02 14:30:41, Michael van Ouwerkerk wrote:
> Encryption-Key

Done.

https://codereview.chromium.org/1243563002/diff/200001/components/gcm_driver/...
File components/gcm_driver/crypto/gcm_encryption_provider.h (right):

https://codereview.chromium.org/1243563002/diff/200001/components/gcm_driver/...
components/gcm_driver/crypto/gcm_encryption_provider.h:51:
DECRYPTION_FAILURE_UNKNOWN
On 2015/10/02 14:30:41, Michael van Ouwerkerk wrote:
> nit: Consider making this the first value as it might not always be the last.

Done.

https://codereview.chromium.org/1243563002/diff/200001/components/gcm_driver/...
components/gcm_driver/crypto/gcm_encryption_provider.h:85:
FRIEND_TEST_ALL_PREFIXES(GCMEncryptionProviderTest, EncryptionRoundTrip);
On 2015/10/02 14:30:42, Michael van Ouwerkerk wrote:
> Why not friend class GCMEncryptionProviderTest? It doesn't lead to a long list
> of FRIEND_TEST_ALL_PREFIXES.

Unfortunately that's not how gtest works. The fixture is called
GCMEncryptionProviderTest, each test using the fixture has its own class called
(something like) GCMEncryptionProviderTest__NameOfTest, which extends the
fixture.

Friending a class does not friend all derived classes as well, so that
unfortunately wouldn't work. The only test touching private behaviour is
EncryptionRoundTrip, as it depends on the |key_store_| accessor.

https://codereview.chromium.org/1243563002/diff/200001/components/gcm_driver/...
File components/gcm_driver/crypto/gcm_encryption_provider_unittest.cc (right):

https://codereview.chromium.org/1243563002/diff/200001/components/gcm_driver/...
components/gcm_driver/crypto/gcm_encryption_provider_unittest.cc:261:
base::RunLoop().RunUntilIdle();
On 2015/10/02 14:30:42, Michael van Ouwerkerk wrote:
> nit: document what these loop runs are for.

Done.

https://codereview.chromium.org/1243563002/diff/200001/components/gcm_driver/...
File components/gcm_driver/gcm_driver.cc (right):

https://codereview.chromium.org/1243563002/diff/200001/components/gcm_driver/...
components/gcm_driver/gcm_driver.cc:20: void DecryptionFailedCallback(
On 2015/10/02 14:30:42, Michael van Ouwerkerk wrote:
> nit: Can we add some Info level logging for when this happens? Or some UMA?

Each path towards reaching this message will already output a DLOG(ERROR), so
this should be obvious. We'll need an |onpusherror| event prior to shipping as
well, which will be covered by UMA.

https://codereview.chromium.org/1243563002/diff/200001/components/gcm_driver/...
File components/gcm_driver/gcm_driver.h (right):

https://codereview.chromium.org/1243563002/diff/200001/components/gcm_driver/...
components/gcm_driver/gcm_driver.h:271: // If the |message| has been encrypted,
it will be asynchronously decrypted
On 2015/10/02 14:30:42, Michael van Ouwerkerk wrote:
> nit: This left me wondering whether it will be dispatched synchronously when
it
> is not encrypted.

Done.

[jianli, 2015-10-06 22:30:43]

lgtm

https://codereview.chromium.org/1243563002/diff/220001/components/gcm_driver/...
File components/gcm_driver/gcm_driver.cc (right):

https://codereview.chromium.org/1243563002/diff/220001/components/gcm_driver/...
components/gcm_driver/gcm_driver.cc:20: void DecryptionFailedCallback(
nit: this should be placed after the constant line.

[Peter Beverloo, 2015-10-09 10:13:04]

Thank you Jian!

https://codereview.chromium.org/1243563002/diff/220001/components/gcm_driver/...
File components/gcm_driver/gcm_driver.cc (right):

https://codereview.chromium.org/1243563002/diff/220001/components/gcm_driver/...
components/gcm_driver/gcm_driver.cc:20: void DecryptionFailedCallback(
On 2015/10/06 22:30:43, jianli wrote:
> nit: this should be placed after the constant line.

Done.

[Peter Beverloo, 2015-10-09 10:13:16]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2015-10-09 10:13:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
mvanouwerkerk@chromium.org, jianli@chromium.org
Link to the patchset: https://codereview.chromium.org/1243563002/#ps240001
(title: "address comment")

[commit-bot: I haz the power, 2015-10-09 10:13:52]

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

[commit-bot: I haz the power, 2015-10-09 10:19:00]

Committed patchset #10 (id:240001)

[commit-bot: I haz the power, 2015-10-09 10:19:52]

Patchset 10 (id:??) landed as
https://crrev.com/add31f6fb477348d2b6a755cc8b6107a7fcb5339
Cr-Commit-Position: refs/heads/master@{#353256}