Use a stable device-user identifier for CryptAuth enrollments.

Use a stable device-user identifier for CryptAuth enrollments.

This id should not change if the same user is removed and added to the same
device.

BUG=455515
TEST=removed account, added account back and id is still the same

Committed: https://crrev.com/7df5c5cf5459cf393fe775a6bf508b801ef19fe1
Cr-Commit-Position: refs/heads/master@{#318001}

[Tim Song, 2015-02-09 18:47:04]

tengs@chromium.org changed reviewers:
+ isherman@chromium.org, xiyuan@chromium.org

[xiyuan, 2015-02-09 19:25:40]

lgtm

[Ilya Sherman, 2015-02-10 01:31:05]

Should there also be a TODO somewhere to remove the old way of computing the ID,
which was per-user?

https://codereview.chromium.org/877033005/diff/1/chrome/common/extensions/api...
File chrome/common/extensions/api/easy_unlock_private.idl (right):

https://codereview.chromium.org/877033005/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/easy_unlock_private.idl:147: // An stable
identifier for the user and device. If a user is removed and
nit: "An" -> "A"

https://codereview.chromium.org/877033005/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/877033005/diff/1/chrome/common/pref_names.cc#...
chrome/common/pref_names.cc:2257: // Device ID registered with CryptAuth stored
in local state.
The device id isn't directly reported to CryptAuth -- right?  It looks like
we're going to report different id's per user -- or am I misunderstanding?

https://codereview.chromium.org/877033005/diff/1/components/proximity_auth/cr...
File components/proximity_auth/cryptauth/cryptauth_enrollment_utils.cc (right):

https://codereview.chromium.org/877033005/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/cryptauth_enrollment_utils.cc:6: #include
"components/proximity_auth/cryptauth/cryptauth_enrollment_utils.h"
nit: This include should be first.

https://codereview.chromium.org/877033005/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/cryptauth_enrollment_utils.cc:19: return
base::MD5DigestToBase16(digest);
MD5 is somewhat frowned upon these days, as secure hash functions go.  Any
reason not to use something more secure?  I realize it doesn't super matter for
our use-case, but it seems like it wouldn't hurt.

[Tim Song, 2015-02-10 22:19:41]

New patchsets have been uploaded after l-g-t-m from xiyuan@chromium.org

[Tim Song, 2015-02-10 22:20:33]

The old way of computing the device id was done in the app. I'll make CL
changing it to use the userInfo deviceUserId instead.

https://codereview.chromium.org/877033005/diff/1/chrome/common/extensions/api...
File chrome/common/extensions/api/easy_unlock_private.idl (right):

https://codereview.chromium.org/877033005/diff/1/chrome/common/extensions/api...
chrome/common/extensions/api/easy_unlock_private.idl:147: // An stable
identifier for the user and device. If a user is removed and
On 2015/02/10 01:31:05, Ilya Sherman wrote:
> nit: "An" -> "A"

Done.

https://codereview.chromium.org/877033005/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/877033005/diff/1/chrome/common/pref_names.cc#...
chrome/common/pref_names.cc:2257: // Device ID registered with CryptAuth stored
in local state.
On 2015/02/10 01:31:05, Ilya Sherman wrote:
> The device id isn't directly reported to CryptAuth -- right?  It looks like
> we're going to report different id's per user -- or am I misunderstanding?

Done.

https://codereview.chromium.org/877033005/diff/1/components/proximity_auth/cr...
File components/proximity_auth/cryptauth/cryptauth_enrollment_utils.cc (right):

https://codereview.chromium.org/877033005/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/cryptauth_enrollment_utils.cc:6: #include
"components/proximity_auth/cryptauth/cryptauth_enrollment_utils.h"
On 2015/02/10 01:31:05, Ilya Sherman wrote:
> nit: This include should be first.

Done.

https://codereview.chromium.org/877033005/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/cryptauth_enrollment_utils.cc:19: return
base::MD5DigestToBase16(digest);
On 2015/02/10 01:31:05, Ilya Sherman wrote:
> MD5 is somewhat frowned upon these days, as secure hash functions go.  Any
> reason not to use something more secure?  I realize it doesn't super matter
for
> our use-case, but it seems like it wouldn't hurt.

I mainly did it because MD5 is fast, but I switched to SHA1 as speed isn't
really important here either.

[Ilya Sherman, 2015-02-10 23:20:14]

LGMT % a final comment:

https://codereview.chromium.org/877033005/diff/1/components/proximity_auth/cr...
File components/proximity_auth/cryptauth/cryptauth_enrollment_utils.cc (right):

https://codereview.chromium.org/877033005/diff/1/components/proximity_auth/cr...
components/proximity_auth/cryptauth/cryptauth_enrollment_utils.cc:19: return
base::MD5DigestToBase16(digest);
On 2015/02/10 22:20:33, Tim Song wrote:
> On 2015/02/10 01:31:05, Ilya Sherman wrote:
> > MD5 is somewhat frowned upon these days, as secure hash functions go.  Any
> > reason not to use something more secure?  I realize it doesn't super matter
> for
> > our use-case, but it seems like it wouldn't hurt.
> 
> I mainly did it because MD5 is fast, but I switched to SHA1 as speed isn't
> really important here either.

As long as we're using SHA, why not use SHA-256, via //crypto/sha2.h?

[Tim Song, 2015-02-11 01:33:59]

I don't really want to add a dependency to crypto just to use SHA256 instead.

[Tim Song, 2015-02-11 01:34:38]

tengs@chromium.org changed reviewers:
+ asargent@chromium.org

[Tim Song, 2015-02-11 01:34:38]

+asargent@ for .idl file

[asargent_no_longer_on_chrome, 2015-02-12 20:43:41]

chrome/common/extensions/api/easy_unlock_private.idl lgtm

[Tim Song, 2015-02-25 07:03:47]

The CQ bit was checked by tengs@chromium.org

[Tim Song, 2015-02-25 07:03:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
asargent@chromium.org, xiyuan@chromium.org
Link to the patchset: https://codereview.chromium.org/877033005/#ps60001 (title:
"accidently removed files")

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

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

[commit-bot: I haz the power, 2015-02-25 07:21:43]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-25 07:22:35]

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