Generate Instance ID locally.

Generate Instance ID locally.

Implement InstanceIDImpl::GetID and InstanceIDImpl::DeleteID. The ID
is not saved to the persistent store yet.

BUG=477084
TEST=new tests

Committed: https://crrev.com/22a3673df2a7734da6f67b962e493043aff87c96
Cr-Commit-Position: refs/heads/master@{#328222}

[jianli, 2015-04-30 20:10:18]

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

[fgorski, 2015-04-30 22:13:12]

https://codereview.chromium.org/1113213002/diff/20001/components/gcm_driver/i...
File components/gcm_driver/instance_id/instance_id_driver_unittest.cc (right):

https://codereview.chromium.org/1113213002/diff/20001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_driver_unittest.cc:72:
EXPECT_FALSE(creation_time1.is_null());
you can take before and after time and check that creation_time1 is between the
two.

https://codereview.chromium.org/1113213002/diff/20001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_driver_unittest.cc:81:
EXPECT_FALSE(id2.empty());
can we check that created ids:
* are of certain length
* consist of a limited set of characters (are URL safe)

https://codereview.chromium.org/1113213002/diff/20001/components/gcm_driver/i...
File components/gcm_driver/instance_id/instance_id_impl.cc (right):

https://codereview.chromium.org/1113213002/diff/20001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_impl.cc:35: EnsureIDGenerated();
Are you sure we should create ID when code queries for creation time? It would
be ok to return null time, as the ID is not created and was never requested.

It also allows to query if ID was at all created.

https://codereview.chromium.org/1113213002/diff/20001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_impl.cc:71:
key_pair_.reset(crypto::RSAPrivateKey::Create(2048));
const kKeyLength?

https://codereview.chromium.org/1113213002/diff/20001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_impl.cc:101: //   
Android-compatible base64 scheme:
move below as point 4

https://codereview.chromium.org/1113213002/diff/20001/components/gcm_driver/i...
File components/gcm_driver/instance_id/instance_id_impl.h (right):

https://codereview.chromium.org/1113213002/diff/20001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_impl.h:45:
scoped_ptr<crypto::RSAPrivateKey> key_pair_;
do we need to keep the key_pair_ around? what for?

[jianli, 2015-05-02 00:30:18]

https://codereview.chromium.org/1113213002/diff/20001/components/gcm_driver/i...
File components/gcm_driver/instance_id/instance_id_driver_unittest.cc (right):

https://codereview.chromium.org/1113213002/diff/20001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_driver_unittest.cc:72:
EXPECT_FALSE(creation_time1.is_null());
On 2015/04/30 22:13:11, fgorski wrote:
> you can take before and after time and check that creation_time1 is between
the
> two.

I don't think this is safe since it might be likely that the time value remains
same on a fast computer.

https://codereview.chromium.org/1113213002/diff/20001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_driver_unittest.cc:81:
EXPECT_FALSE(id2.empty());
On 2015/04/30 22:13:12, fgorski wrote:
> can we check that created ids:
> * are of certain length
> * consist of a limited set of characters (are URL safe)

We don't have a strict guideline for length. Added check for chars.

https://codereview.chromium.org/1113213002/diff/20001/components/gcm_driver/i...
File components/gcm_driver/instance_id/instance_id_impl.cc (right):

https://codereview.chromium.org/1113213002/diff/20001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_impl.cc:35: EnsureIDGenerated();
On 2015/04/30 22:13:12, fgorski wrote:
> Are you sure we should create ID when code queries for creation time? It would
> be ok to return null time, as the ID is not created and was never requested.
> 
> It also allows to query if ID was at all created.

Sounds reasonable. Changed.

https://codereview.chromium.org/1113213002/diff/20001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_impl.cc:71:
key_pair_.reset(crypto::RSAPrivateKey::Create(2048));
On 2015/04/30 22:13:12, fgorski wrote:
> const kKeyLength?

Not needed any more.

https://codereview.chromium.org/1113213002/diff/20001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_impl.cc:101: //   
Android-compatible base64 scheme:
On 2015/04/30 22:13:12, fgorski wrote:
> move below as point 4

Not needed any more.

https://codereview.chromium.org/1113213002/diff/20001/components/gcm_driver/i...
File components/gcm_driver/instance_id/instance_id_impl.h (right):

https://codereview.chromium.org/1113213002/diff/20001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_impl.h:45:
scoped_ptr<crypto::RSAPrivateKey> key_pair_;
On 2015/04/30 22:13:12, fgorski wrote:
> do we need to keep the key_pair_ around? what for?

Not needed any more.

[fgorski, 2015-05-04 16:11:19]

Mostly looks good. Let's figure out the format of generated instance ID.

https://codereview.chromium.org/1113213002/diff/40001/components/gcm_driver/i...
File components/gcm_driver/instance_id/instance_id_impl.cc (right):

https://codereview.chromium.org/1113213002/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_impl.cc:64: id_ =
base::GenerateGUID();
Based on the prior code, shouldn't you start from a 9? and isn't there a
requirement on length?

Could you clarify that with Costin?

[jianli, 2015-05-04 20:14:49]

https://codereview.chromium.org/1113213002/diff/40001/components/gcm_driver/i...
File components/gcm_driver/instance_id/instance_id_impl.cc (right):

https://codereview.chromium.org/1113213002/diff/40001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_impl.cc:64: id_ =
base::GenerateGUID();
On 2015/05/04 16:11:19, fgorski wrote:
> Based on the prior code, shouldn't you start from a 9? and isn't there a
> requirement on length?
> 
> Could you clarify that with Costin?

I've updated the ID generation algorithm based on talk with Costin.

[fgorski, 2015-05-04 20:22:49]

LGTM with nit

https://codereview.chromium.org/1113213002/diff/60001/components/gcm_driver/i...
File components/gcm_driver/instance_id/instance_id_impl.cc (right):

https://codereview.chromium.org/1113213002/diff/60001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_impl.cc:80:
base::StringPiece(reinterpret_cast<const char*>(bytes), 8), &id_);
s/8/kInstanceIDByteLength/

[jianli, 2015-05-04 21:32:27]

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

[jianli, 2015-05-04 21:32:28]

agl, could you please review the dependency on crypto? Thanks.

[agl, 2015-05-04 21:55:54]

https://codereview.chromium.org/1113213002/diff/80001/components/gcm_driver/i...
File components/gcm_driver/instance_id/instance_id_impl.cc (right):

https://codereview.chromium.org/1113213002/diff/80001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_impl.cc:67: // 1) Generates the
cryptographically secure random number in 8 bytes.
You can't have a cryptographically secure, 60-bit number. How about making
kInstanceIDByteLength 16? 124 is reasonable.

https://codereview.chromium.org/1113213002/diff/80001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_impl.cc:69:
crypto::RandBytes(bytes, kInstanceIDByteLength);
s/kInstanceIDByteLength/sizeof(bytes)/

https://codereview.chromium.org/1113213002/diff/80001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_impl.cc:71: // 2) Transforms the
first 4 bits to 0x7.
Comment should explain why.

https://codereview.chromium.org/1113213002/diff/80001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_impl.cc:72: uint8 b0 = bytes[0];
bytes[0] &= 0x0f;
bytes[0] |= 0x70;

?

[jianli, 2015-05-04 22:07:20]

https://codereview.chromium.org/1113213002/diff/80001/components/gcm_driver/i...
File components/gcm_driver/instance_id/instance_id_impl.cc (right):

https://codereview.chromium.org/1113213002/diff/80001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_impl.cc:67: // 1) Generates the
cryptographically secure random number in 8 bytes.
On 2015/05/04 21:55:54, agl wrote:
> You can't have a cryptographically secure, 60-bit number. How about making
> kInstanceIDByteLength 16? 124 is reasonable.

Updated the comment per discussion.

https://codereview.chromium.org/1113213002/diff/80001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_impl.cc:69:
crypto::RandBytes(bytes, kInstanceIDByteLength);
On 2015/05/04 21:55:54, agl wrote:
> s/kInstanceIDByteLength/sizeof(bytes)/

Done.

https://codereview.chromium.org/1113213002/diff/80001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_impl.cc:71: // 2) Transforms the
first 4 bits to 0x7.
On 2015/05/04 21:55:54, agl wrote:
> Comment should explain why.

Done.

https://codereview.chromium.org/1113213002/diff/80001/components/gcm_driver/i...
components/gcm_driver/instance_id/instance_id_impl.cc:72: uint8 b0 = bytes[0];
On 2015/05/04 21:55:54, agl wrote:
> bytes[0] &= 0x0f;
> bytes[0] |= 0x70;
> 
> ?

Done.

[agl, 2015-05-04 22:19:25]

LGTM for crypto.

[jianli, 2015-05-04 22:20:09]

The CQ bit was checked by jianli@chromium.org

[jianli, 2015-05-04 22:20:09]

The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org
Link to the patchset: https://codereview.chromium.org/1113213002/#ps100001
(title: "Address more feedback")

[commit-bot: I haz the power, 2015-05-04 22:20:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1113213002/100001

[commit-bot: I haz the power, 2015-05-04 23:01:40]

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

[commit-bot: I haz the power, 2015-05-04 23:01:41]

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

[jianli, 2015-05-04 23:20:42]

The CQ bit was checked by jianli@chromium.org

[commit-bot: I haz the power, 2015-05-04 23:21:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1113213002/100001

[commit-bot: I haz the power, 2015-05-05 00:02:04]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-05-05 00:03:00]

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