cros: Create cryptohome keys for Easy sign-in.

cros: Create cryptohome keys for Easy sign-in.

- Add code to add/remove/get cryptohome keys for Easy sign-in;
- Add an EasyUnlockKeyManager to wrap the operations and provide an API;
- Re-create crypthome keys with pairing data in user prefs on user sign-in;

BUG=394640, 394641
Committed: https://crrev.com/a0e92b0829a31efaa403709216db696a4614e228
Cr-Commit-Position: refs/heads/master@{#294901}

[xiyuan, 2014-09-08 22:09:15]

xiyuan@chromium.org changed reviewers:
+ dkrahn@chromium.org, dzhioev@chromium.org, tbarzic@chromium.org

[xiyuan, 2014-09-08 22:09:16]

Tony, please do overall review.

Darren and Pavel could focus on:
chromeos/cryptohome/*
chromeos/login/auth/*

Thanks.

[xiyuan, 2014-09-08 22:56:29]

Patchset #2 (id:20001) has been deleted

[tbarzic, 2014-09-09 19:32:21]

https://codereview.chromium.org/554043003/diff/40001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/login/easy_unlock/easy_unlock_create_keys_operation.cc
(right):

https://codereview.chromium.org/554043003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_create_keys_operation.cc:159:
tpm_pub_key_,
I think android expect this to be GenericPublicKey proto from securemessage
(probably not relevant for this cl, since tpm_pub_key_ is empty, but we may have
to add something to easy-unlock dbus client to properly format the key)

https://codereview.chromium.org/554043003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_create_keys_operation.cc:240:
if (index == static_cast<int>(devices_.size())) {
I'd rather make key_creation_index_ size_t; and maybe introduce started_ flag
that would be current equivalent of key_creation_index_ != -1

though, I'm fine with this if you prefer it

https://codereview.chromium.org/554043003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_get_keys_operation.cc
(right):

https://codereview.chromium.org/554043003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_get_keys_operation.cc:59:
LOG(ERROR) << "Easy unlock failed to get key data, code=" << return_code;
Are you missing
callback_.Run(false, EasyUnlockDeviceKeyDataList())

https://codereview.chromium.org/554043003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.cc
(right):

https://codereview.chromium.org/554043003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.cc:40: if
(!RemoteDeviceListToDeviceDataList(remote_devices, &devices)) {
Should we remove keys even in case the remote_devices is invalid?

https://codereview.chromium.org/554043003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.cc:48:
create_keys_op_.reset(new EasyUnlockCreateKeysOperation(
Would it make sence to do this in OnKeysRemovedForCreateKeys?

https://codereview.chromium.org/554043003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.cc:161:
DCHECK(!create_keys_op_);
I think you meant DCHECK(create_leys_op_)

https://codereview.chromium.org/554043003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.h
(right):

https://codereview.chromium.org/554043003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.h:42: void
CreateKeys(const UserContext& user_context,
I'd rename this to Refresh/Update keys

https://codereview.chromium.org/554043003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.h:58: static
void DeviceDataToRemoteDeviceDictionary(
do these have to be public? If not we could make them standalone functions in
.cc

https://codereview.chromium.org/554043003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_types.h (right):

https://codereview.chromium.org/554043003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_types.h:23: 
nit: can you add a short comments for these?

https://codereview.chromium.org/554043003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/session/user_session_manager.h (right):

https://codereview.chromium.org/554043003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/session/user_session_manager.h:294: 
can you add a comment?

[dzhioev (left Google), 2014-09-09 20:01:40]

Hello. Xiyuan, can you share with me a design doc describing this feature?

[xiyuan, 2014-09-09 20:15:08]

On 2014/09/09 20:01:40, dzhioev wrote:
> Hello. Xiyuan, can you share with me a design doc describing this feature?

Shared the eng doc to you. Let me know if you could not get it.

[xiyuan, 2014-09-09 20:25:31]

https://codereview.chromium.org/554043003/diff/40001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/login/easy_unlock/easy_unlock_create_keys_operation.cc
(right):

https://codereview.chromium.org/554043003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_create_keys_operation.cc:159:
tpm_pub_key_,
On 2014/09/09 19:32:21, tbarzic wrote:
> I think android expect this to be GenericPublicKey proto from securemessage
> (probably not relevant for this cl, since tpm_pub_key_ is empty, but we may
have
> to add something to easy-unlock dbus client to properly format the key)

Yep. Added a TODO to wrap it in GenericPublicKey proto.

https://codereview.chromium.org/554043003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_create_keys_operation.cc:240:
if (index == static_cast<int>(devices_.size())) {
On 2014/09/09 19:32:21, tbarzic wrote:
> I'd rather make key_creation_index_ size_t; and maybe introduce started_ flag
> that would be current equivalent of key_creation_index_ != -1
> 
> though, I'm fine with this if you prefer it

Done. Changed to size_t. Not really need a started flag though.

https://codereview.chromium.org/554043003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_get_keys_operation.cc
(right):

https://codereview.chromium.org/554043003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_get_keys_operation.cc:59:
LOG(ERROR) << "Easy unlock failed to get key data, code=" << return_code;
On 2014/09/09 19:32:21, tbarzic wrote:
> Are you missing
> callback_.Run(false, EasyUnlockDeviceKeyDataList())

Good catch. Fixed.

https://codereview.chromium.org/554043003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.cc
(right):

https://codereview.chromium.org/554043003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.cc:40: if
(!RemoteDeviceListToDeviceDataList(remote_devices, &devices)) {
On 2014/09/09 19:32:21, tbarzic wrote:
> Should we remove keys even in case the remote_devices is invalid?

Makes sense. Updated to code to use empty |devices| so that we still nuke all
the keys.

https://codereview.chromium.org/554043003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.cc:48:
create_keys_op_.reset(new EasyUnlockCreateKeysOperation(
On 2014/09/09 19:32:21, tbarzic wrote:
> Would it make sence to do this in OnKeysRemovedForCreateKeys?

Done.

https://codereview.chromium.org/554043003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.cc:161:
DCHECK(!create_keys_op_);
On 2014/09/09 19:32:21, tbarzic wrote:
> I think you meant DCHECK(create_leys_op_)

Yep.

https://codereview.chromium.org/554043003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.h
(right):

https://codereview.chromium.org/554043003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.h:42: void
CreateKeys(const UserContext& user_context,
On 2014/09/09 19:32:21, tbarzic wrote:
> I'd rename this to Refresh/Update keys

Done.

https://codereview.chromium.org/554043003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.h:58: static
void DeviceDataToRemoteDeviceDictionary(
On 2014/09/09 19:32:21, tbarzic wrote:
> do these have to be public? If not we could make them standalone functions in
> .cc

Not needed for this CL. But we might need those helpers in EasyUnlockService for
sign-in profile to provide pairing data from cryptohome keys.

https://codereview.chromium.org/554043003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_types.h (right):

https://codereview.chromium.org/554043003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_types.h:23: 
On 2014/09/09 19:32:21, tbarzic wrote:
> nit: can you add a short comments for these?

Done.

https://codereview.chromium.org/554043003/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/session/user_session_manager.h (right):

https://codereview.chromium.org/554043003/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/session/user_session_manager.h:294: 
On 2014/09/09 19:32:21, tbarzic wrote:
> can you add a comment?

Done.

[tbarzic, 2014-09-09 20:52:24]

lgtm

https://codereview.chromium.org/554043003/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.h
(right):

https://codereview.chromium.org/554043003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.h:56: // false
if the conversion fails. Note that EasyUnlockDeviceKeyData contains a
nit: Comment says the method returns false in case of an error, but
DeviceDataToRemoteDeviceDictionary is void :)

https://codereview.chromium.org/554043003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.h:61: static
bool RemoteDeviceDictionaryToDeviceData(
nit: can you add a comment that in case of an error |data| could be filled with
partial result (and not empty); or maybe make sure the data is cleared before
returning false.

(same for RemoteDeviceListToDeviceDataList)

[xiyuan, 2014-09-09 21:13:54]

https://codereview.chromium.org/554043003/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.h
(right):

https://codereview.chromium.org/554043003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.h:56: // false
if the conversion fails. Note that EasyUnlockDeviceKeyData contains a
On 2014/09/09 20:52:24, tbarzic wrote:
> nit: Comment says the method returns false in case of an error, but
> DeviceDataToRemoteDeviceDictionary is void :)

Comments updated.

https://codereview.chromium.org/554043003/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.h:61: static
bool RemoteDeviceDictionaryToDeviceData(
On 2014/09/09 20:52:24, tbarzic wrote:
> nit: can you add a comment that in case of an error |data| could be filled
with
> partial result (and not empty); or maybe make sure the data is cleared before
> returning false.
> 
> (same for RemoteDeviceListToDeviceDataList)

Updated so that the output |data| is only updated on success.
RemoteDeviceListToDeviceDataList is actually okay because its output is only
changed on success.

[xiyuan, 2014-09-09 22:28:35]

Toni, please take another look at PS #6 when you get time.

The followings are changed when playing with the test prototype:
- Added support for concurrent get keys op;
- Changed create key to create new keys first then trim extra;
- Changed to lazily create EasyUnlockKeyManager in UserSessionManager instead of
creating it in update keys; 

Thanks.

[tbarzic, 2014-09-10 19:01:12]

https://codereview.chromium.org/554043003/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.cc
(right):

https://codereview.chromium.org/554043003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.cc:80: const
GetDeviceDataListCallback& callback) {
should this be throttled if there is create_keys_op/remove_keys_op in progress?

https://codereview.chromium.org/554043003/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.h
(right):

https://codereview.chromium.org/554043003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.h:33: typedef
EasyUnlockCreateKeysOperation::CreateKeysCallback CreateKeysCallback;
rename to RefreshKeysCallback

https://codereview.chromium.org/554043003/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/session/user_session_manager.cc (right):

https://codereview.chromium.org/554043003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/user_session_manager.cc:1008:
user_context_,
do we have to check that user_context_ and user_profile belong to the same user?

https://codereview.chromium.org/554043003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/user_session_manager.cc:1009:
*device_list,
suggestion:
you could make EasyUnlockKeyManager::RemoveKeys private and change this to:

device_list ? *device_list : base::ListValue()

[xiyuan, 2014-09-10 20:37:13]

https://codereview.chromium.org/554043003/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.cc
(right):

https://codereview.chromium.org/554043003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.cc:80: const
GetDeviceDataListCallback& callback) {
On 2014/09/10 19:01:11, tbarzic wrote:
> should this be throttled if there is create_keys_op/remove_keys_op in
progress?

This should not happen (at least not for the same user). For simplicity, added a
DCHECK to disallow create/remove keys when trying to get. We could loose it
later if needed.

Also updated the DCHECKs for key creation/removal to ensure there is no pending
operations.

https://codereview.chromium.org/554043003/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.h
(right):

https://codereview.chromium.org/554043003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.h:33: typedef
EasyUnlockCreateKeysOperation::CreateKeysCallback CreateKeysCallback;
On 2014/09/10 19:01:11, tbarzic wrote:
> rename to RefreshKeysCallback

Done.

https://codereview.chromium.org/554043003/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/session/user_session_manager.cc (right):

https://codereview.chromium.org/554043003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/user_session_manager.cc:1008:
user_context_,
On 2014/09/10 19:01:11, tbarzic wrote:
> do we have to check that user_context_ and user_profile belong to the same
user?

The two should match. But to be safe, added a DCHECK to verify the two has the
same user id.

https://codereview.chromium.org/554043003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/session/user_session_manager.cc:1009:
*device_list,
On 2014/09/10 19:01:11, tbarzic wrote:
> suggestion:
> you could make EasyUnlockKeyManager::RemoveKeys private and change this to:
> 
> device_list ? *device_list : base::ListValue()

Cannot do this because the ? trinary expression involves assigning ListValue to
a temp var and it is not allowed for ListValue. I get this error:

../../chrome/browser/chromeos/login/session/user_session_manager.cc:1016:36:
error: calling a private constructor of class 'base::ListValue'
      device_list ? *device_list : base::ListValue(),
                                   ^
../../base/values.h:487:28: note: declared private here
  DISALLOW_COPY_AND_ASSIGN(ListValue);

[tbarzic, 2014-09-10 20:43:40]

lgtm

https://codereview.chromium.org/554043003/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.cc
(right):

https://codereview.chromium.org/554043003/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.cc:80: const
GetDeviceDataListCallback& callback) {
On 2014/09/10 20:37:13, xiyuan wrote:
> On 2014/09/10 19:01:11, tbarzic wrote:
> > should this be throttled if there is create_keys_op/remove_keys_op in
> progress?
> 
> This should not happen (at least not for the same user). For simplicity, added
a
> DCHECK to disallow create/remove keys when trying to get. We could loose it
> later if needed.
> 
> Also updated the DCHECKs for key creation/removal to ensure there is no
pending
> operations.

sg

[Darren Krahn, 2014-09-12 16:35:18]

https://codereview.chromium.org/554043003/diff/140001/chromeos/cryptohome/cry...
File chromeos/cryptohome/cryptohome_parameters.h (right):

https://codereview.chromium.org/554043003/diff/140001/chromeos/cryptohome/cry...
chromeos/cryptohome/cryptohome_parameters.h:34: void SetNumber(int64 number);
style nit: mutators should match var name. E.g. set_number(...).

https://codereview.chromium.org/554043003/diff/140001/chromeos/cryptohome/cry...
chromeos/cryptohome/cryptohome_parameters.h:43: 
This collides with https://codereview.chromium.org/526353002/. You may want to
rebase on that.

https://codereview.chromium.org/554043003/diff/140001/chromeos/cryptohome/hom...
File chromeos/cryptohome/homedir_methods.cc (right):

https://codereview.chromium.org/554043003/diff/140001/chromeos/cryptohome/hom...
chromeos/cryptohome/homedir_methods.cc:63: }
Also collides with https://codereview.chromium.org/526353002/ -- you may want to
coordinate with bartfab@.

[xiyuan, 2014-09-12 19:10:12]

Rebased. Now this CL depends on https://codereview.chromium.org/526353002/.

https://codereview.chromium.org/554043003/diff/140001/chromeos/cryptohome/cry...
File chromeos/cryptohome/cryptohome_parameters.h (right):

https://codereview.chromium.org/554043003/diff/140001/chromeos/cryptohome/cry...
chromeos/cryptohome/cryptohome_parameters.h:34: void SetNumber(int64 number);
On 2014/09/12 16:35:17, Darren Krahn wrote:
> style nit: mutators should match var name. E.g. set_number(...).

Code removed after rebasing.

https://codereview.chromium.org/554043003/diff/140001/chromeos/cryptohome/cry...
chromeos/cryptohome/cryptohome_parameters.h:43: 
On 2014/09/12 16:35:17, Darren Krahn wrote:
> This collides with https://codereview.chromium.org/526353002/. You may want to
> rebase on that.

Rebased.

https://codereview.chromium.org/554043003/diff/140001/chromeos/cryptohome/hom...
File chromeos/cryptohome/homedir_methods.cc (right):

https://codereview.chromium.org/554043003/diff/140001/chromeos/cryptohome/hom...
chromeos/cryptohome/homedir_methods.cc:63: }
On 2014/09/12 16:35:17, Darren Krahn wrote:
> Also collides with https://codereview.chromium.org/526353002/ -- you may want
to
> coordinate with bartfab@.

Rebased.

[Darren Krahn, 2014-09-12 19:12:46]

On 2014/09/12 19:10:12, xiyuan wrote:
> Rebased. Now this CL depends on https://codereview.chromium.org/526353002/.
> 
>
https://codereview.chromium.org/554043003/diff/140001/chromeos/cryptohome/cry...
> File chromeos/cryptohome/cryptohome_parameters.h (right):
> 
>
https://codereview.chromium.org/554043003/diff/140001/chromeos/cryptohome/cry...
> chromeos/cryptohome/cryptohome_parameters.h:34: void SetNumber(int64 number);
> On 2014/09/12 16:35:17, Darren Krahn wrote:
> > style nit: mutators should match var name. E.g. set_number(...).
> 
> Code removed after rebasing.
> 
>
https://codereview.chromium.org/554043003/diff/140001/chromeos/cryptohome/cry...
> chromeos/cryptohome/cryptohome_parameters.h:43: 
> On 2014/09/12 16:35:17, Darren Krahn wrote:
> > This collides with https://codereview.chromium.org/526353002/. You may want
to
> > rebase on that.
> 
> Rebased.
> 
>
https://codereview.chromium.org/554043003/diff/140001/chromeos/cryptohome/hom...
> File chromeos/cryptohome/homedir_methods.cc (right):
> 
>
https://codereview.chromium.org/554043003/diff/140001/chromeos/cryptohome/hom...
> chromeos/cryptohome/homedir_methods.cc:63: }
> On 2014/09/12 16:35:17, Darren Krahn wrote:
> > Also collides with https://codereview.chromium.org/526353002/ -- you may
want
> to
> > coordinate with bartfab@.
> 
> Rebased.

chromeos/cryptohome lgtm

[xiyuan, 2014-09-15 18:37:59]

The CQ bit was checked by xiyuan@chromium.org

[commit-bot: I haz the power, 2014-09-15 19:16:38]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/554043003/180001

[xiyuan, 2014-09-15 20:14:24]

The CQ bit was unchecked by xiyuan@chromium.org

[xiyuan, 2014-09-15 20:21:03]

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

[xiyuan, 2014-09-15 20:21:03]

Ilya, please help with histograms.xml change. Thanks.

[Ilya Sherman, 2014-09-15 20:51:24]

histograms.xml LGTM

[xiyuan, 2014-09-15 20:59:52]

The CQ bit was checked by xiyuan@chromium.org

[commit-bot: I haz the power, 2014-09-15 21:01:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/554043003/200001

[commit-bot: I haz the power, 2014-09-15 22:25:08]

Committed patchset #10 (id:200001) as 3e17116250d542921cb7df9fc70f5c9061a8236a

[commit-bot: I haz the power, 2014-09-15 22:35:09]

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