Hook up the Push API with GCM's new ability to own encryption keys.

Hook up the Push API with GCM's new ability to own encryption keys.

This CL finishes implementing propagation of the "curve25519dh" attribute
of the PushSubscription interface of the Push API. The public key will be
provided by the GCM Driver, which internally uses the (new) GCMKeyStore.

Encrypted messages cannot yet be received by GCM - that work remains. The
functionality introduced in this CL is also still guarded behind the
"--enable-push-message-payload" command line flag.

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

[Peter Beverloo, 2015-07-16 18:34:39]

Patchset #2 (id:20001) has been deleted

[Peter Beverloo, 2015-07-16 18:40:00]

peter@chromium.org changed reviewers:
+ fgorski@chromium.org, jianli@chromium.org, johnme@chromium.org

[Peter Beverloo, 2015-07-16 18:40:01]

Hi guys -

This is not ready for full review yet, but I'd love some high-level thoughts
about the approach. I'm trying to minimize changes to common GCM Driver code.

A few notes:

    - The GCMEncryptionHandler will get a second public method for decrypting
messages, but I'll do that in a separate CL.

    - Tests will be added. gcm_encryption_handler will get some unit tests, I'll
add a least one browser test for Push. Full end-to-end browser tests will be
done separately.

    - The big remaining task after this is hooking up the receiving side for
both desktop and Android. I'm working with the GCM team to finalize these
details, e.g. propagation of the encryption keys.

[Peter Beverloo, 2015-07-17 12:56:55]

Patchset #2 (id:40001) has been deleted

[Peter Beverloo, 2015-07-17 15:56:34]

On 2015/07/16 18:40:01, Peter Beverloo wrote:
> Hi guys -
> 
> This is not ready for full review yet, but I'd love some high-level thoughts
> about the approach. I'm trying to minimize changes to common GCM Driver code.
> 
> A few notes:
> 
>     - The GCMEncryptionHandler will get a second public method for decrypting
> messages, but I'll do that in a separate CL.
> 
>     - Tests will be added. gcm_encryption_handler will get some unit tests,
I'll
> add a least one browser test for Push. Full end-to-end browser tests will be
> done separately.
> 
>     - The big remaining task after this is hooking up the receiving side for
> both desktop and Android. I'm working with the GCM team to finalize these
> details, e.g. propagation of the encryption keys.

The decryption part can be seen in the following CL:
  https://codereview.chromium.org/1243563002

Both still require a lot of work, but the end-to-end flow is now available :).

[jianli, 2015-07-17 20:55:21]

https://codereview.chromium.org/1231613005/diff/60001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_service_impl.h (right):

https://codereview.chromium.org/1231613005/diff/60001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.h:189: bool
EnableMessagePayloads() const;
EnableMessagePayloads sounds more like an action. How about
IsMessagePayloadsEnabled?

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

https://codereview.chromium.org/1231613005/diff/60001/components/gcm_driver/c...
components/gcm_driver/crypto/gcm_encryption_handler.cc:35:
key_store_->CreateKeys(
If GCMEncryptionHandler is the only consumer of GCMKeyStore, it seems to be
better to combine GetKey and CreateKey into one to simplify the logic here.

https://codereview.chromium.org/1231613005/diff/60001/components/gcm_driver/c...
components/gcm_driver/crypto/gcm_encryption_handler.cc:41:
DCHECK_EQ(KeyPair::ECDH_CURVE_25519, pair.type());
nit: include base/logging.h

https://codereview.chromium.org/1231613005/diff/60001/components/gcm_driver/c...
components/gcm_driver/crypto/gcm_encryption_handler.cc:44:
callback.Run(std::vector<uint8_t>(public_key.begin(), public_key.end()));
Can we avoid the conversion?

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

https://codereview.chromium.org/1231613005/diff/60001/components/gcm_driver/c...
components/gcm_driver/crypto/gcm_encryption_handler.h:27: // Interface through
which the GCM Driver deals with encryption. Responsible for
"Interface" sounds a bit misleading. How about "bridge"?

https://codereview.chromium.org/1231613005/diff/60001/components/gcm_driver/c...
components/gcm_driver/crypto/gcm_encryption_handler.h:29: class
GCMEncryptionHandler {
Handler suffix sounds not good enough to indicate what this class is intended
for. How about GCMEncryptionChannel, or GCMEncryptionProvider?

https://codereview.chromium.org/1231613005/diff/60001/components/gcm_driver/c...
components/gcm_driver/crypto/gcm_encryption_handler.h:36:
scoped_refptr<base::SequencedTaskRunner> blocking_task_runner);
const scoped_refptr<>&

https://codereview.chromium.org/1231613005/diff/60001/components/gcm_driver/c...
components/gcm_driver/crypto/gcm_encryption_handler.h:41: void
GetPublicEncryptionKey(const std::string& app_id,
I think the shorter name GetPublicKey is simpler and easier to understand.

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

https://codereview.chromium.org/1231613005/diff/60001/components/gcm_driver/c...
components/gcm_driver/crypto/gcm_key_store.cc:170:
DCHECK(blocking_task_runner_);
This DCHECK should be moved to constructor.

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

https://codereview.chromium.org/1231613005/diff/60001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.cc:18: const char kEncryptionDirectoryName[] =
"Encryption";
Should this be defined in chrome/common/chrome_constants and passed as parameter
to GCMDriver?

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

https://codereview.chromium.org/1231613005/diff/60001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.h:132: void GetPublicEncryptionKey(
GetPublicKey

https://codereview.chromium.org/1231613005/diff/60001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.h:134: const GCMEncryptionHandler::KeyCallback&
callback);
We should try avoid referring something defined in another class that belongs to
the implementation detail.

[Peter Beverloo, 2015-07-20 17:55:10]

Patchset #3 (id:80001) has been deleted

[Peter Beverloo, 2015-07-20 17:55:54]

Thank you! This should now be ready for formal review.

https://codereview.chromium.org/1231613005/diff/60001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_service_impl.h (right):

https://codereview.chromium.org/1231613005/diff/60001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.h:189: bool
EnableMessagePayloads() const;
On 2015/07/17 20:55:21, jianli wrote:
> EnableMessagePayloads sounds more like an action. How about
> IsMessagePayloadsEnabled?

Done.

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

https://codereview.chromium.org/1231613005/diff/60001/components/gcm_driver/c...
components/gcm_driver/crypto/gcm_encryption_handler.cc:35:
key_store_->CreateKeys(
On 2015/07/17 20:55:21, jianli wrote:
> If GCMEncryptionHandler is the only consumer of GCMKeyStore, it seems to be
> better to combine GetKey and CreateKey into one to simplify the logic here.

Agreed. I implemented the distinction because it'd make key rotation a lot
easier. However, since this will impact the tests, I will do so in a separate
CL. (Together with Ryan's post-commit comments.)

https://codereview.chromium.org/1231613005/diff/60001/components/gcm_driver/c...
components/gcm_driver/crypto/gcm_encryption_handler.cc:41:
DCHECK_EQ(KeyPair::ECDH_CURVE_25519, pair.type());
On 2015/07/17 20:55:21, jianli wrote:
> nit: include base/logging.h

Done.

https://codereview.chromium.org/1231613005/diff/60001/components/gcm_driver/c...
components/gcm_driver/crypto/gcm_encryption_handler.cc:44:
callback.Run(std::vector<uint8_t>(public_key.begin(), public_key.end()));
On 2015/07/17 20:55:21, jianli wrote:
> Can we avoid the conversion?

Moved it to PushMessagingServiceImpl (as we're using WebVector/WebArrayBuffer in
Blink) - this allows the GCMDriver and below to use an std::string.

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

https://codereview.chromium.org/1231613005/diff/60001/components/gcm_driver/c...
components/gcm_driver/crypto/gcm_encryption_handler.h:27: // Interface through
which the GCM Driver deals with encryption. Responsible for
On 2015/07/17 20:55:21, jianli wrote:
> "Interface" sounds a bit misleading. How about "bridge"?

I called it a provider per your next comment.

https://codereview.chromium.org/1231613005/diff/60001/components/gcm_driver/c...
components/gcm_driver/crypto/gcm_encryption_handler.h:29: class
GCMEncryptionHandler {
On 2015/07/17 20:55:21, jianli wrote:
> Handler suffix sounds not good enough to indicate what this class is intended
> for. How about GCMEncryptionChannel, or GCMEncryptionProvider?

Done. (GCMEncryptionProvider)

https://codereview.chromium.org/1231613005/diff/60001/components/gcm_driver/c...
components/gcm_driver/crypto/gcm_encryption_handler.h:36:
scoped_refptr<base::SequencedTaskRunner> blocking_task_runner);
On 2015/07/17 20:55:21, jianli wrote:
> const scoped_refptr<>&

Done.

https://codereview.chromium.org/1231613005/diff/60001/components/gcm_driver/c...
components/gcm_driver/crypto/gcm_encryption_handler.h:41: void
GetPublicEncryptionKey(const std::string& app_id,
On 2015/07/17 20:55:21, jianli wrote:
> I think the shorter name GetPublicKey is simpler and easier to understand.

Done.

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

https://codereview.chromium.org/1231613005/diff/60001/components/gcm_driver/c...
components/gcm_driver/crypto/gcm_key_store.cc:170:
DCHECK(blocking_task_runner_);
On 2015/07/17 20:55:21, jianli wrote:
> This DCHECK should be moved to constructor.

Done.

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

https://codereview.chromium.org/1231613005/diff/60001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.cc:18: const char kEncryptionDirectoryName[] =
"Encryption";
On 2015/07/17 20:55:21, jianli wrote:
> Should this be defined in chrome/common/chrome_constants and passed as
parameter
> to GCMDriver?

This is an implementation detail - chrome_constants gives us gcm store
directory, which the encryption-specific data could be considered a dependent
database of (for Desktop). Using a sub-directory for that is fine.

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

https://codereview.chromium.org/1231613005/diff/60001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.h:132: void GetPublicEncryptionKey(
On 2015/07/17 20:55:21, jianli wrote:
> GetPublicKey

Done.

https://codereview.chromium.org/1231613005/diff/60001/components/gcm_driver/g...
components/gcm_driver/gcm_driver.h:134: const GCMEncryptionHandler::KeyCallback&
callback);
On 2015/07/17 20:55:21, jianli wrote:
> We should try avoid referring something defined in another class that belongs
to
> the implementation detail.

Done.

[jianli, 2015-07-20 21:36:33]

lgtm

https://codereview.chromium.org/1231613005/diff/100001/components/gcm_driver/...
File components/gcm_driver/fake_gcm_driver.h (right):

https://codereview.chromium.org/1231613005/diff/100001/components/gcm_driver/...
components/gcm_driver/fake_gcm_driver.h:17: FakeGCMDriver(
nit: add explicit

https://codereview.chromium.org/1231613005/diff/100001/components/gcm_driver/...
File components/gcm_driver/gcm_driver.cc (right):

https://codereview.chromium.org/1231613005/diff/100001/components/gcm_driver/...
components/gcm_driver/gcm_driver.cc:37: if (blocking_task_runner) {
Please comment under what circumstance that blocking_task_runner could be null.

[Peter Beverloo, 2015-07-21 14:11:29]

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

[Peter Beverloo, 2015-07-21 14:11:30]

+thakis for /chrome/browser/BUILD.gn addition (analogous to
gcm_driver/instance_id).

https://codereview.chromium.org/1231613005/diff/100001/components/gcm_driver/...
File components/gcm_driver/fake_gcm_driver.h (right):

https://codereview.chromium.org/1231613005/diff/100001/components/gcm_driver/...
components/gcm_driver/fake_gcm_driver.h:17: FakeGCMDriver(
On 2015/07/20 21:36:33, jianli wrote:
> nit: add explicit

Done.

https://codereview.chromium.org/1231613005/diff/100001/components/gcm_driver/...
File components/gcm_driver/gcm_driver.cc (right):

https://codereview.chromium.org/1231613005/diff/100001/components/gcm_driver/...
components/gcm_driver/gcm_driver.cc:37: if (blocking_task_runner) {
On 2015/07/20 21:36:33, jianli wrote:
> Please comment under what circumstance that blocking_task_runner could be
null.

Done.

[Nico, 2015-07-21 15:03:07]

chrome build files lgtm

[johnme, 2015-07-21 15:37:26]

chrome/browser/push_messaging/* and chrome/test/data/push_messaging/push_test.js
lgtm once you make sure we always call DecreasePushSubscriptionCount (and with a
few nits)

https://codereview.chromium.org/1231613005/diff/120001/chrome/browser/push_me...
File chrome/browser/push_messaging/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/1231613005/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_service_impl.cc:317: //
GetPublicEncryptionKey methods ----------------------------------------------
Nit: Ideally this section would be between "Subscribe and GetPermissionStatus
methods" and "Unsubscribe methods" in the cc file as well.

https://codereview.chromium.org/1231613005/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_service_impl.cc:347:
callback.Run(success, std::vector<uint8_t>(public_key.begin(),
On platforms where char is signed, this might cause a conversion. Though I guess
it'll wrap correctly, so that's probably ok.

https://codereview.chromium.org/1231613005/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_service_impl.cc:509:
SubscribeEndWithError(callback, status);
Why are you removing the call to `DecreasePushSubscriptionCount(1, true /*
was_pending */);` in the error case?

https://codereview.chromium.org/1231613005/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_service_impl.cc:518:
SubscribeEndWithError(
You need to call `DecreasePushSubscriptionCount(1, true /* was_pending */);`
here too. In fact you might find it easier to just call it once from the top of
DidSubscribe.

https://codereview.chromium.org/1231613005/diff/120001/chrome/browser/push_me...
File chrome/browser/push_messaging/push_messaging_service_impl.h (right):

https://codereview.chromium.org/1231613005/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_service_impl.h:168: //
GetPublicEncryptionKey method ---------------------------------------------
Nit: GetPublicEncryptionKey is declared between Subscribe and Unsubscribe (above
and in the superclass), so please put GetPublicEncryptionKey method between
Subscribe methods and Unsubscribe methods for consistency.

https://codereview.chromium.org/1231613005/diff/120001/chrome/test/data/push_...
File chrome/test/data/push_messaging/push_test.js (right):

https://codereview.chromium.org/1231613005/diff/120001/chrome/test/data/push_...
chrome/test/data/push_messaging/push_test.js:111: return
swRegistration.pushManager.subscribe(pushSubscriptionOptions)
Nit: the intent would probably be clearer if you s/subscribe/getSubscription/

[Peter Beverloo, 2015-07-21 15:51:08]

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

[Peter Beverloo, 2015-07-21 15:51:08]

All done, thanks! :)

+avi for a silly mistake I made in push_messaging_status.h in a previous patch.

https://codereview.chromium.org/1231613005/diff/120001/chrome/browser/push_me...
File chrome/browser/push_messaging/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/1231613005/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_service_impl.cc:317: //
GetPublicEncryptionKey methods ----------------------------------------------
On 2015/07/21 15:37:25, johnme wrote:
> Nit: Ideally this section would be between "Subscribe and GetPermissionStatus
> methods" and "Unsubscribe methods" in the cc file as well.

Done.

https://codereview.chromium.org/1231613005/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_service_impl.cc:347:
callback.Run(success, std::vector<uint8_t>(public_key.begin(),
On 2015/07/21 15:37:25, johnme wrote:
> On platforms where char is signed, this might cause a conversion. Though I
guess
> it'll wrap correctly, so that's probably ok.

Ack - yay for protobufs storing any binary data as strings :).

https://codereview.chromium.org/1231613005/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_service_impl.cc:509:
SubscribeEndWithError(callback, status);
On 2015/07/21 15:37:26, johnme wrote:
> Why are you removing the call to `DecreasePushSubscriptionCount(1, true /*
> was_pending */);` in the error case?

Oops - done.

https://codereview.chromium.org/1231613005/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_service_impl.cc:518:
SubscribeEndWithError(
On 2015/07/21 15:37:26, johnme wrote:
> You need to call `DecreasePushSubscriptionCount(1, true /* was_pending */);`
> here too. In fact you might find it easier to just call it once from the top
of
> DidSubscribe.

Done.

https://codereview.chromium.org/1231613005/diff/120001/chrome/browser/push_me...
File chrome/browser/push_messaging/push_messaging_service_impl.h (right):

https://codereview.chromium.org/1231613005/diff/120001/chrome/browser/push_me...
chrome/browser/push_messaging/push_messaging_service_impl.h:168: //
GetPublicEncryptionKey method ---------------------------------------------
On 2015/07/21 15:37:26, johnme wrote:
> Nit: GetPublicEncryptionKey is declared between Subscribe and Unsubscribe
(above
> and in the superclass), so please put GetPublicEncryptionKey method between
> Subscribe methods and Unsubscribe methods for consistency.

Done.

https://codereview.chromium.org/1231613005/diff/120001/chrome/test/data/push_...
File chrome/test/data/push_messaging/push_test.js (right):

https://codereview.chromium.org/1231613005/diff/120001/chrome/test/data/push_...
chrome/test/data/push_messaging/push_test.js:111: return
swRegistration.pushManager.subscribe(pushSubscriptionOptions)
On 2015/07/21 15:37:26, johnme wrote:
> Nit: the intent would probably be clearer if you s/subscribe/getSubscription/

Done.

[Avi (use Gerrit), 2015-07-21 17:16:46]

lgtm

yeeeeerp

[Peter Beverloo, 2015-07-24 10:27:54]

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

[commit-bot: I haz the power, 2015-07-24 10:28:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1231613005/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1231613005/160001

[commit-bot: I haz the power, 2015-07-24 12:29:47]

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

[commit-bot: I haz the power, 2015-07-24 12:29:48]

Dry run: This issue passed the CQ dry run.

[Peter Beverloo, 2015-07-24 12:30:17]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2015-07-24 12:30:17]

The patchset sent to the CQ was uploaded after l-g-t-m from jianli@chromium.org,
thakis@chromium.org, johnme@chromium.org
Link to the patchset: https://codereview.chromium.org/1231613005/#ps160001
(title: " ")

[commit-bot: I haz the power, 2015-07-24 12:30:21]

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

[commit-bot: I haz the power, 2015-07-24 12:35:00]

Committed patchset #6 (id:160001)

[commit-bot: I haz the power, 2015-07-24 12:35:33]

Patchset 6 (id:??) landed as
https://crrev.com/23695aaa72c3c7c5176456c5ca4d3f7617be42fd
Cr-Commit-Position: refs/heads/master@{#340263}

[Peter Beverloo, 2015-07-24 13:15:39]

A revert of this CL (patchset #6 id:160001) has been created in
https://codereview.chromium.org/1259613002/ by peter@chromium.org.

The reason for reverting is: I broke the build. Sorry!

http://build.chromium.org/p/chromium.linux/builders/Linux%20GN%20Clobber/buil...
.

[Peter Beverloo, 2015-07-28 16:07:11]

The CQ bit was checked by peter@chromium.org

[Peter Beverloo, 2015-07-28 16:07:12]

The patchset sent to the CQ was uploaded after l-g-t-m from johnme@chromium.org,
avi@chromium.org, thakis@chromium.org, jianli@chromium.org
Link to the patchset: https://codereview.chromium.org/1231613005/#ps180001
(title: " ")

[commit-bot: I haz the power, 2015-07-28 16:07:35]

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

[commit-bot: I haz the power, 2015-07-28 16:20:02]

Committed patchset #7 (id:180001)

[commit-bot: I haz the power, 2015-07-28 16:20:49]

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