Integrate InstanceID with GCM crypto provider

Integrate InstanceID with GCM crypto provider

It's now possible to get per-token encryption information from an
InstanceID; and this is automatically removed when DeleteToken or
DeleteID are called.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1899753002 fixes strict mode violations
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. https://codereview.chromium.org/1953273002 extends the GCMKeyStore
7. this patch
8. https://codereview.chromium.org/1851423003 switches Push to InstanceIDs

BUG=589461
Committed: https://crrev.com/a00cc8ae26668aadb3ac84919c85e8672ed84f28
Cr-Commit-Position: refs/heads/master@{#397388}

[johnme, 2016-04-27 18:01:44]

Description was changed from

==========
Integrate InstanceID with GCM crypto provider

BUG=589461
==========

to

==========
Integrate InstanceID with GCM crypto provider

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1899753002 fixes strict mode violations
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. this patch
7. https://codereview.chromium.org/1851423003 switches Push to InstanceIDs

BUG=589461
==========

[johnme, 2016-04-28 14:08:09]

Description was changed from

==========
Integrate InstanceID with GCM crypto provider

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1899753002 fixes strict mode violations
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. this patch
7. https://codereview.chromium.org/1851423003 switches Push to InstanceIDs

BUG=589461
==========

to

==========
Integrate InstanceID with GCM crypto provider

It's now possible to get per-token encryption information from an
InstanceID; and this is automatically removed when DeleteToken or
DeleteID are called.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1899753002 fixes strict mode violations
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. this patch
7. https://codereview.chromium.org/1851423003 switches Push to InstanceIDs

BUG=589461
==========

[johnme, 2016-04-28 14:09:02]

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

[Peter Beverloo, 2016-05-04 13:58:53]

Hey John,

Thanks for the CL! Firstly, let's split this up in two CLs: one where you
introduce the ability to store keys separately in the key store, one where
you hook up the key store with the InstanceID implementation.

I'm going to focus on the key store part of this CL for my comments, since
that is the meaty part. A few higher level comments:

- The GCMEncryptionProvider doesn't have to know about the distinction
  between the different key storage types supported by the GCMKeyStore.
  Can it just pass through what it knows ([app_id, sender_id]) to the
  GCMKeyStore in all cases, trusting that class to do the right thing?

- You're introducing the concept of a "key owner". The GCM Driver already
  has the established concepts of "app_id" and "sender_id" (even when its
  known as "authorized_entry"), I would like us to build on that.

Could we, instead, consider changing the GCMKeyStore API to provide the
following methods with the following contracts?

:: GCMKeyStore::GetKeys(app_id, authorized_entry, callback)

  1) Loads the key for [app_id, authorized_entry].
  2) If said key does not exist, loads the key for [app_id, ""].

:: GCMKeyStore::CreateKeys(app_id, authorized_entry, callback)

  1) Creates a key for [app_id, authorized_entry].

GCMDriver::GetEncryptionInfo() would deliberately pass an empty string for
the authorized_entry. InstanceID::GetEncryptionInfo() would pass both.

:: GCMKeyStore::RemoveKeys(app_id, authorized_entry, callback)

  1) If |authorized_entry| is "*", removes all non-empty keys associated
     with the |app_id|.
  2) Otherwise, removes the key for [app_id, authorized_entry]

This will support GCMDriver::Unregister (which will pass [app_id, ""]),
InstanceID::DeleteToken (which will pass [app_id, authorized_entry]) as
well as InstanceID::DeleteID (which will pass [app_id, "*"]), without
causing conflicts.

This API will allow us to limit the need for a "key owner" concept to the
GCMKeyStore, and even then it depends on how we decide to store the actual
key information. (I don't think we even have to do that.)

WDYT?

[johnme, 2016-05-06 12:03:50]

Description was changed from

==========
Integrate InstanceID with GCM crypto provider

It's now possible to get per-token encryption information from an
InstanceID; and this is automatically removed when DeleteToken or
DeleteID are called.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1899753002 fixes strict mode violations
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. this patch
7. https://codereview.chromium.org/1851423003 switches Push to InstanceIDs

BUG=589461
==========

to

==========
Integrate InstanceID with GCM crypto provider

It's now possible to get per-token encryption information from an
InstanceID; and this is automatically removed when DeleteToken or
DeleteID are called.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1899753002 fixes strict mode violations
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. this patch
7. https://codereview.chromium.org/1851423003 switches Push to InstanceIDs

Also depends on https://codereview.chromium.org/1948133003

BUG=589461
==========

[johnme, 2016-05-06 12:22:43]

Description was changed from

==========
Integrate InstanceID with GCM crypto provider

It's now possible to get per-token encryption information from an
InstanceID; and this is automatically removed when DeleteToken or
DeleteID are called.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1899753002 fixes strict mode violations
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. this patch
7. https://codereview.chromium.org/1851423003 switches Push to InstanceIDs

Also depends on https://codereview.chromium.org/1948133003

BUG=589461
==========

to

==========
Integrate InstanceID with GCM crypto provider

It's now possible to get per-token encryption information from an
InstanceID; and this is automatically removed when DeleteToken or
DeleteID are called.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1899753002 fixes strict mode violations
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. https://codereview.chromium.org/1953273002 extends the GCMKeyStore
7. this patch
8. https://codereview.chromium.org/1851423003 switches Push to InstanceIDs

BUG=589461
==========

[johnme, 2016-05-06 12:33:01]

Addressed review comments.

> Thanks for the CL! Firstly, let's split this up in two CLs: one where you
> introduce the ability to store keys separately in the key store, one where
> you hook up the key store with the InstanceID implementation.

Done. Key store changes moved to https://codereview.chromium.org/1953273002

> The GCMEncryptionProvider doesn't have to know about the distinction
> between the different key storage types supported by the GCMKeyStore.
> Can it just pass through what it knows ([app_id, sender_id]) to the
> GCMKeyStore in all cases, trusting that class to do the right thing?

Doneish - it passes through an instance_id_authorized_entity, which is the
empty string for legacy GCM registrations.

> You're introducing the concept of a "key owner". The GCM Driver already
> has the established concepts of "app_id" and "sender_id" (even when its
> known as "authorized_entry"), I would like us to build on that.

Done. No more owner.

> :: GCMKeyStore::GetKeys(app_id, authorized_entry, callback)

Done, though see next point.

> 1) Loads the key for [app_id, authorized_entry].
> 2) If said key does not exist, loads the key for [app_id, ""].

No, this could potentially cause conflicts between legacy GCM registations
and IID tokens for the same app_id. Instead GCMEncryptionProvider's Decrypt
method still tries both.

> :: GCMKeyStore::CreateKeys(app_id, authorized_entry, callback)
>
> 1) Creates a key for [app_id, authorized_entry].
>
> GCMDriver::GetEncryptionInfo() would deliberately pass an empty string for
> the authorized_entry. InstanceID::GetEncryptionInfo() would pass both.

Done.

> :: GCMKeyStore::RemoveKeys(app_id, authorized_entry, callback)
>
> 1) If |authorized_entry| is "*", removes all non-empty keys associated
>    with the |app_id|.
> 2) Otherwise, removes the key for [app_id, authorized_entry]
>
> This will support GCMDriver::Unregister (which will pass [app_id, ""]),
> InstanceID::DeleteToken (which will pass [app_id, authorized_entry]) as
> well as InstanceID::DeleteID (which will pass [app_id, "*"]), without
> causing conflicts.

Done.

> This API will allow us to limit the need for a "key owner" concept to the
> GCMKeyStore, and even then it depends on how we decide to store the actual
> key information. (I don't think we even have to do that.)

Even the store no longer uses owners, since I used separate fields in the
proto. The only remnant is DatabaseKey in gcm_key_store.cc, since app_id is
no longer a valid unique key for database objects.

[Peter Beverloo, 2016-05-11 16:05:00]

Thank you, John!

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

https://codereview.chromium.org/1923953002/diff/100001/components/gcm_driver/...
components/gcm_driver/gcm_driver.h:242: GCMEncryptionProvider*
encryption_provider_internal() {
(1) Please move this after GetInstanceIDHandlerInternal() to group them
together.
(2) Please rename this to GetEncryptionProviderInternal() for consistency. As
such, also move to .cc.
(3) Have a brief half-sentence describing *what* it is.

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

https://codereview.chromium.org/1923953002/diff/100001/components/gcm_driver/...
components/gcm_driver/instance_id/instance_id.cc:37: DeleteTokenCallback
wrapped_callback =
It would be good to document why this only applies to the "GCM" scope.

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

https://codereview.chromium.org/1923953002/diff/100001/components/gcm_driver/...
components/gcm_driver/instance_id/instance_id.h:24: // Must match Java
GoogleCloudMessaging.INSTANCE_ID_SCOPE.
Please document *why* you could pass this to getToken().

https://codereview.chromium.org/1923953002/diff/100001/components/gcm_driver/...
components/gcm_driver/instance_id/instance_id.h:103: // InstanceID tokens; use
GCMDriver::GetEncryptionInfo for GCM registrations.
This is a non-static method on the InstanceID class, that last sentence seems
fairly self-explanatory to me? How real is the risk that someone would misuse
this?

Also, I'm going to have to ask you to do the reverse of the other CL here
(sorry!)- it's consistent in this file to document methods as:

|param_name|: description

Best to match that.

https://codereview.chromium.org/1923953002/diff/100001/components/gcm_driver/...
File components/gcm_driver/instance_id/instance_id_impl.h (right):

https://codereview.chromium.org/1923953002/diff/100001/components/gcm_driver/...
components/gcm_driver/instance_id/instance_id_impl.h:72: gcm::InstanceIDHandler*
handler_;
It's a bit unfortunate that this class will now end up having two pointers to
exactly the same thing (GCMDriverDesktop).

Would you consider reverting the changes you did as part of this CL instead?

https://codereview.chromium.org/1785613006/diff/60001/components/gcm_driver/i...

[Peter Beverloo, 2016-05-11 16:05:31]

lgtm % the duplicated pointer in InstanceIdImpl

[johnme, 2016-05-13 16:25:45]

Addressed review nits - thanks!

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

https://codereview.chromium.org/1923953002/diff/100001/components/gcm_driver/...
components/gcm_driver/gcm_driver.h:242: GCMEncryptionProvider*
encryption_provider_internal() {
On 2016/05/11 16:04:59, Peter Beverloo wrote:
> (1) Please move this after GetInstanceIDHandlerInternal() to group them
> together.
> (2) Please rename this to GetEncryptionProviderInternal() for consistency. As
> such, also move to .cc.
> (3) Have a brief half-sentence describing *what* it is.

Done.

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

https://codereview.chromium.org/1923953002/diff/100001/components/gcm_driver/...
components/gcm_driver/instance_id/instance_id.cc:37: DeleteTokenCallback
wrapped_callback =
On 2016/05/11 16:05:00, Peter Beverloo wrote:
> It would be good to document why this only applies to the "GCM" scope.

Done.

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

https://codereview.chromium.org/1923953002/diff/100001/components/gcm_driver/...
components/gcm_driver/instance_id/instance_id.h:24: // Must match Java
GoogleCloudMessaging.INSTANCE_ID_SCOPE.
On 2016/05/11 16:05:00, Peter Beverloo wrote:
> Please document *why* you could pass this to getToken().

Done. Also moved comment to .cc file (for consistency with most comments in
Chrome, and because it the "must match" part is best paired with the
definition).

https://codereview.chromium.org/1923953002/diff/100001/components/gcm_driver/...
components/gcm_driver/instance_id/instance_id.h:103: // InstanceID tokens; use
GCMDriver::GetEncryptionInfo for GCM registrations.
On 2016/05/11 16:05:00, Peter Beverloo wrote:
> This is a non-static method on the InstanceID class, that last sentence seems
> fairly self-explanatory to me? How real is the risk that someone would misuse
> this?

Fair. I'll keep the reverse comment though, since it's more likely that someone
might accidentally use the GCMDriver one for an IID.

> Also, I'm going to have to ask you to do the reverse of the other CL here
> (sorry!)- it's consistent in this file to document methods as:
> 
> |param_name|: description
> 
> Best to match that.

Done.

https://codereview.chromium.org/1923953002/diff/100001/components/gcm_driver/...
File components/gcm_driver/instance_id/instance_id_impl.h (right):

https://codereview.chromium.org/1923953002/diff/100001/components/gcm_driver/...
components/gcm_driver/instance_id/instance_id_impl.h:72: gcm::InstanceIDHandler*
handler_;
On 2016/05/11 16:05:00, Peter Beverloo wrote:
> It's a bit unfortunate that this class will now end up having two pointers to
> exactly the same thing (GCMDriverDesktop).
> 
> Would you consider reverting the changes you did as part of this CL instead?
> 
>
https://codereview.chromium.org/1785613006/diff/60001/components/gcm_driver/i...

Done. There's now a Handler() method.

[johnme, 2016-05-13 16:25:57]

The CQ bit was checked by johnme@chromium.org

[johnme, 2016-05-13 16:25:57]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org
Link to the patchset: https://codereview.chromium.org/1923953002/#ps120001
(title: "Address review nits")

[commit-bot: I haz the power, 2016-05-13 16:26:10]

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

[commit-bot: I haz the power, 2016-05-13 16:35:27]

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

[commit-bot: I haz the power, 2016-05-13 16:35:28]

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

[johnme, 2016-05-24 17:30:13]

The CQ bit was checked by johnme@chromium.org

[johnme, 2016-05-24 17:30:14]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org
Link to the patchset: https://codereview.chromium.org/1923953002/#ps140001
(title: "Fix typo")

[commit-bot: I haz the power, 2016-05-24 17:30:48]

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

[commit-bot: I haz the power, 2016-05-24 19:04:15]

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

[commit-bot: I haz the power, 2016-05-24 19:04:18]

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

[johnme, 2016-06-02 12:57:58]

The CQ bit was checked by johnme@chromium.org

[johnme, 2016-06-02 12:57:59]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org
Link to the patchset: https://codereview.chromium.org/1923953002/#ps160001
(title: "RunUntilIdle after deleting GCMDriver")

[commit-bot: I haz the power, 2016-06-02 12:58:13]

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

[commit-bot: I haz the power, 2016-06-02 13:58:15]

Description was changed from

==========
Integrate InstanceID with GCM crypto provider

It's now possible to get per-token encryption information from an
InstanceID; and this is automatically removed when DeleteToken or
DeleteID are called.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1899753002 fixes strict mode violations
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. https://codereview.chromium.org/1953273002 extends the GCMKeyStore
7. this patch
8. https://codereview.chromium.org/1851423003 switches Push to InstanceIDs

BUG=589461
==========

to

==========
Integrate InstanceID with GCM crypto provider

It's now possible to get per-token encryption information from an
InstanceID; and this is automatically removed when DeleteToken or
DeleteID are called.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1899753002 fixes strict mode violations
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. https://codereview.chromium.org/1953273002 extends the GCMKeyStore
7. this patch
8. https://codereview.chromium.org/1851423003 switches Push to InstanceIDs

BUG=589461
==========

[commit-bot: I haz the power, 2016-06-02 13:58:16]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-06-02 14:00:00]

Description was changed from

==========
Integrate InstanceID with GCM crypto provider

It's now possible to get per-token encryption information from an
InstanceID; and this is automatically removed when DeleteToken or
DeleteID are called.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1899753002 fixes strict mode violations
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. https://codereview.chromium.org/1953273002 extends the GCMKeyStore
7. this patch
8. https://codereview.chromium.org/1851423003 switches Push to InstanceIDs

BUG=589461
==========

to

==========
Integrate InstanceID with GCM crypto provider

It's now possible to get per-token encryption information from an
InstanceID; and this is automatically removed when DeleteToken or
DeleteID are called.

Part of a series of patches:
1. https://codereview.chromium.org/1832833002 adds InstanceIDWithSubtype
2. https://codereview.chromium.org/1830983002 adds JNI bindings
3. https://codereview.chromium.org/1829023002 adds fake and test
4. https://codereview.chromium.org/1899753002 fixes strict mode violations
5. https://codereview.chromium.org/1854093002 enables InstanceID by default
6. https://codereview.chromium.org/1953273002 extends the GCMKeyStore
7. this patch
8. https://codereview.chromium.org/1851423003 switches Push to InstanceIDs

BUG=589461

Committed: https://crrev.com/a00cc8ae26668aadb3ac84919c85e8672ed84f28
Cr-Commit-Position: refs/heads/master@{#397388}
==========

[commit-bot: I haz the power, 2016-06-02 14:00:01]

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