[EasyUnlock] Serialize and store BeaconSeeds along as cryptohome key metadata.

[EasyUnlock] Serialize and store BeaconSeeds as part of the cryptohome key metadata.

For the login flow, we need to be able to use the same BeaconSeeds as those
in the user preferences.

The BeaconSeeds are refreshed everytime the user logs in or unlocks their
Chromebook like the other EasyUnlock metadata.

BUG=715594
Review-Url: https://codereview.chromium.org/2863533003
Cr-Commit-Position: refs/heads/master@{#473352}
Committed: https://chromium.googlesource.com/chromium/src/+/4fdb40570e2c40ffa318f0131b5cbee4900b964f

[Tim Song, 2017-05-04 00:56:42]

Patchset #1 (id:1) has been deleted

[Tim Song, 2017-05-04 02:23:58]

Patchset #1 (id:20001) has been deleted

[Tim Song, 2017-05-04 02:26:46]

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

[Tim Song, 2017-05-04 02:26:46]

Hey Gustavo, I have another CL coming that adds the BeaconSeeds to
RemoteDevices.

However, I need to talk to Kyle, as he has some disagreements about storing the
BeaconSeeds here.

[Tim Song, 2017-05-06 22:53:32]

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

[Tim Song, 2017-05-06 22:53:33]

Hey Ryan, both Gustavo and Kyle are OOO. Can you take a look at this CL?

[Ryan Hansberry, 2017-05-09 02:44:08]

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

https://codereview.chromium.org/2863533003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_get_keys_operation.cc:134:
PA_LOG(WARNING) << "Unknown Easy unlock key data entry, name="
super nit: Not part of this CL but can you fix this to be "EasyUnlock"?

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

https://codereview.chromium.org/2863533003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.cc:163:
PA_LOG(ERROR) << "Expected serialized_beacon_seeds...";
Nit: use a period, not ellipses, and make this error slightly more descriptive.

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

https://codereview.chromium.org/2863533003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_types.cc:15: const char
kEasyUnlockKeyMetaNameSerializedBeaconSeeds[] = "eu.BS";
I don't know much about how this is stored, but these names are really short --
is there any danger of collision with constants elsewhere in Chrome?

https://codereview.chromium.org/2863533003/diff/40001/chrome/browser/signin/c...
File chrome/browser/signin/chrome_proximity_auth_client.cc (right):

https://codereview.chromium.org/2863533003/diff/40001/chrome/browser/signin/c...
chrome/browser/signin/chrome_proximity_auth_client.cc:101: // don't use it here
because the CryptAuthService is not available on the
nit: clarify that you "don't use it here *(as opposed to other methods in this
class)* because the CryptAuthService is not available". This was confusing to me
at first.

https://codereview.chromium.org/2863533003/diff/40001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_service_regular.cc (right):

https://codereview.chromium.org/2863533003/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_regular.cc:148: PA_LOG(INFO) << " 
beacon_seed: " << b64_beacon_seed;
nit: Make the output of this prettier. At least make it a sentence, e.g.
"Retrived BeaconSeed: ", and have a log before the loop introducing it in order
for the indentation to not look unexpected.

https://codereview.chromium.org/2863533003/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_regular.cc:281: PA_LOG(INFO) <<
"Setting RemoteDevices:\n  " << remote_devices_json;
nit: remove the line break.

https://codereview.chromium.org/2863533003/diff/40001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_service_signin_chromeos.cc (right):

https://codereview.chromium.org/2863533003/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_signin_chromeos.cc:100:
std::vector<cryptauth::BeaconSeed> DeserializeBeaconSeeds(
Why should serialization and deserialization logic not live in something more
generic, like in cryptauth?

[Ryan Hansberry, 2017-05-09 02:45:08]

Please also reword the grammar of the first sentence of this CL's description.

[Tim Song, 2017-05-10 18:22:48]

Description was changed from

==========
[EasyUnlock] Serialize and store BeaconSeeds along as cryptohome key metadata.

For the login flow, we need to be able to use the same BeaconSeeds as those
in the user preferences.

The BeaconSeeds are refreshed everytime the user logs in or unlocks their
Chromebook like the other EasyUnlock metadata.
==========

to

==========
[EasyUnlock] Serialize and store BeaconSeeds as part of the cryptohome key
metadata.

For the login flow, we need to be able to use the same BeaconSeeds as those
in the user preferences.

The BeaconSeeds are refreshed everytime the user logs in or unlocks their
Chromebook like the other EasyUnlock metadata.
==========

[Tim Song, 2017-05-10 18:23:27]

Fixed the CL description as well.

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

https://codereview.chromium.org/2863533003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_get_keys_operation.cc:134:
PA_LOG(WARNING) << "Unknown Easy unlock key data entry, name="
On 2017/05/09 02:44:08, Ryan Hansberry wrote:
> super nit: Not part of this CL but can you fix this to be "EasyUnlock"?

Done.

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

https://codereview.chromium.org/2863533003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.cc:163:
PA_LOG(ERROR) << "Expected serialized_beacon_seeds...";
On 2017/05/09 02:44:08, Ryan Hansberry wrote:
> Nit: use a period, not ellipses, and make this error slightly more
descriptive.

Done.

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

https://codereview.chromium.org/2863533003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_types.cc:15: const char
kEasyUnlockKeyMetaNameSerializedBeaconSeeds[] = "eu.BS";
On 2017/05/09 02:44:08, Ryan Hansberry wrote:
> I don't know much about how this is stored, but these names are really short
--
> is there any danger of collision with constants elsewhere in Chrome?

These names are prefixed with "eu" (EasyUnlock), so there shouldn't be any
collisions if other projects doing use the same prefix.

https://codereview.chromium.org/2863533003/diff/40001/chrome/browser/signin/c...
File chrome/browser/signin/chrome_proximity_auth_client.cc (right):

https://codereview.chromium.org/2863533003/diff/40001/chrome/browser/signin/c...
chrome/browser/signin/chrome_proximity_auth_client.cc:101: // don't use it here
because the CryptAuthService is not available on the
On 2017/05/09 02:44:08, Ryan Hansberry wrote:
> nit: clarify that you "don't use it here *(as opposed to other methods in this
> class)* because the CryptAuthService is not available". This was confusing to
me
> at first.

Done.

https://codereview.chromium.org/2863533003/diff/40001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_service_regular.cc (right):

https://codereview.chromium.org/2863533003/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_regular.cc:148: PA_LOG(INFO) << " 
beacon_seed: " << b64_beacon_seed;
On 2017/05/09 02:44:08, Ryan Hansberry wrote:
> nit: Make the output of this prettier. At least make it a sentence, e.g.
> "Retrived BeaconSeed: ", and have a log before the loop introducing it in
order
> for the indentation to not look unexpected.

Done. I just deleted this log; it's a bit unnecessary.

https://codereview.chromium.org/2863533003/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_regular.cc:281: PA_LOG(INFO) <<
"Setting RemoteDevices:\n  " << remote_devices_json;
On 2017/05/09 02:44:08, Ryan Hansberry wrote:
> nit: remove the line break.

The line break makes the JSON more readable, as it will start by itself on a new
line.

https://codereview.chromium.org/2863533003/diff/40001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_service_signin_chromeos.cc (right):

https://codereview.chromium.org/2863533003/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_signin_chromeos.cc:100:
std::vector<cryptauth::BeaconSeed> DeserializeBeaconSeeds(
On 2017/05/09 02:44:08, Ryan Hansberry wrote:
> Why should serialization and deserialization logic not live in something more
> generic, like in cryptauth?

This would require some refactoring. Essentially, these values are stored
outside of the user session (as cryptohome key metadata) where CryptAuthService
is not available. 

Indeed, we should ideally have one serialization algorithm for both these use
cases, but the problem right now is that CryptAuth serializes from the
ExternalDeviceInfo proto, whereas we have some custom data scheme here.

It is a bigger refactoring effort (but probably worth it) to store the
serialized ExternalDeviceInfos in the cryptohome key metadata rather than these
custom fields.

[Tim Song, 2017-05-11 22:28:34]

Description was changed from

==========
[EasyUnlock] Serialize and store BeaconSeeds as part of the cryptohome key
metadata.

For the login flow, we need to be able to use the same BeaconSeeds as those
in the user preferences.

The BeaconSeeds are refreshed everytime the user logs in or unlocks their
Chromebook like the other EasyUnlock metadata.
==========

to

==========
[EasyUnlock] Serialize and store BeaconSeeds as part of the cryptohome key
metadata.

For the login flow, we need to be able to use the same BeaconSeeds as those
in the user preferences.

The BeaconSeeds are refreshed everytime the user logs in or unlocks their
Chromebook like the other EasyUnlock metadata.

BUG=715594
==========

[Ryan Hansberry, 2017-05-15 15:36:41]

Mostly looks good, but I noticed your previous comment about khorimoto@
disagreeing with the seed storage. What was that disagreement about (was it
about storing it in cryptohome outside the user session?), and why do you feel
that your current approach is right?

https://codereview.chromium.org/2863533003/diff/40001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_service_signin_chromeos.cc (right):

https://codereview.chromium.org/2863533003/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_signin_chromeos.cc:100:
std::vector<cryptauth::BeaconSeed> DeserializeBeaconSeeds(
On 2017/05/10 18:23:26, Tim Song wrote:
> On 2017/05/09 02:44:08, Ryan Hansberry wrote:
> > Why should serialization and deserialization logic not live in something
more
> > generic, like in cryptauth?
> 
> This would require some refactoring. Essentially, these values are stored
> outside of the user session (as cryptohome key metadata) where
CryptAuthService
> is not available. 
> 
> Indeed, we should ideally have one serialization algorithm for both these use
> cases, but the problem right now is that CryptAuth serializes from the
> ExternalDeviceInfo proto, whereas we have some custom data scheme here.
> 
> It is a bigger refactoring effort (but probably worth it) to store the
> serialized ExternalDeviceInfos in the cryptohome key metadata rather than
these
> custom fields. 

Got it. Can you please add a comment explaining this background?

https://codereview.chromium.org/2863533003/diff/60001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_service_signin_chromeos.cc (right):

https://codereview.chromium.org/2863533003/diff/60001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_signin_chromeos.cc:130: PA_LOG(ERROR)
<< "Unable to Base64 decode BeaconSeed.";
nit: "unable to decode"

https://codereview.chromium.org/2863533003/diff/60001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_signin_chromeos.cc:491: PA_LOG(ERROR)
<< "Unable base64url decode stored remote device:\n"
nit: "unable to decode"

[sacomoto, 2017-05-15 15:47:46]

On 2017/05/15 15:36:41, Ryan Hansberry wrote:
> Mostly looks good, but I noticed your previous comment about khorimoto@
> disagreeing with the seed storage. What was that disagreement about (was it
> about storing it in cryptohome outside the user session?), and why do you feel
> that your current approach is right?
> 
>
https://codereview.chromium.org/2863533003/diff/40001/chrome/browser/signin/e...
> File chrome/browser/signin/easy_unlock_service_signin_chromeos.cc (right):
> 
>
https://codereview.chromium.org/2863533003/diff/40001/chrome/browser/signin/e...
> chrome/browser/signin/easy_unlock_service_signin_chromeos.cc:100:
> std::vector<cryptauth::BeaconSeed> DeserializeBeaconSeeds(
> On 2017/05/10 18:23:26, Tim Song wrote:
> > On 2017/05/09 02:44:08, Ryan Hansberry wrote:
> > > Why should serialization and deserialization logic not live in something
> more
> > > generic, like in cryptauth?
> > 
> > This would require some refactoring. Essentially, these values are stored
> > outside of the user session (as cryptohome key metadata) where
> CryptAuthService
> > is not available. 
> > 
> > Indeed, we should ideally have one serialization algorithm for both these
use
> > cases, but the problem right now is that CryptAuth serializes from the
> > ExternalDeviceInfo proto, whereas we have some custom data scheme here.
> > 
> > It is a bigger refactoring effort (but probably worth it) to store the
> > serialized ExternalDeviceInfos in the cryptohome key metadata rather than
> these
> > custom fields. 

Not really sure that having an ExternalDeviceInfo proto here would be a big
improvement. As not all fields here are included in that proto (e.g. PSK).

If you want to completely rely on the proto serialization / deserialization
methods, it would probably make sense to define a new proto (only in Chrome, no
need to put it in Google3) for the login information (e.g. LoginDeviceInfo).

Then, on that proto you can add all fields in easy_unlock_key_manager.cc.

> 
> Got it. Can you please add a comment explaining this background?
> 
>
https://codereview.chromium.org/2863533003/diff/60001/chrome/browser/signin/e...
> File chrome/browser/signin/easy_unlock_service_signin_chromeos.cc (right):
> 
>
https://codereview.chromium.org/2863533003/diff/60001/chrome/browser/signin/e...
> chrome/browser/signin/easy_unlock_service_signin_chromeos.cc:130:
PA_LOG(ERROR)
> << "Unable to Base64 decode BeaconSeed.";
> nit: "unable to decode"
> 
>
https://codereview.chromium.org/2863533003/diff/60001/chrome/browser/signin/e...
> chrome/browser/signin/easy_unlock_service_signin_chromeos.cc:491:
PA_LOG(ERROR)
> << "Unable base64url decode stored remote device:\n"
> nit: "unable to decode"

[Tim Song, 2017-05-17 19:18:07]

https://codereview.chromium.org/2863533003/diff/40001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_service_signin_chromeos.cc (right):

https://codereview.chromium.org/2863533003/diff/40001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_signin_chromeos.cc:100:
std::vector<cryptauth::BeaconSeed> DeserializeBeaconSeeds(
On 2017/05/15 15:36:41, Ryan Hansberry wrote:
> On 2017/05/10 18:23:26, Tim Song wrote:
> > On 2017/05/09 02:44:08, Ryan Hansberry wrote:
> > > Why should serialization and deserialization logic not live in something
> more
> > > generic, like in cryptauth?
> > 
> > This would require some refactoring. Essentially, these values are stored
> > outside of the user session (as cryptohome key metadata) where
> CryptAuthService
> > is not available. 
> > 
> > Indeed, we should ideally have one serialization algorithm for both these
use
> > cases, but the problem right now is that CryptAuth serializes from the
> > ExternalDeviceInfo proto, whereas we have some custom data scheme here.
> > 
> > It is a bigger refactoring effort (but probably worth it) to store the
> > serialized ExternalDeviceInfos in the cryptohome key metadata rather than
> these
> > custom fields. 
> 
> Got it. Can you please add a comment explaining this background?

Done.

https://codereview.chromium.org/2863533003/diff/60001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_service_signin_chromeos.cc (right):

https://codereview.chromium.org/2863533003/diff/60001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_signin_chromeos.cc:130: PA_LOG(ERROR)
<< "Unable to Base64 decode BeaconSeed.";
On 2017/05/15 15:36:41, Ryan Hansberry wrote:
> nit: "unable to decode"

Done.

https://codereview.chromium.org/2863533003/diff/60001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_signin_chromeos.cc:491: PA_LOG(ERROR)
<< "Unable base64url decode stored remote device:\n"
On 2017/05/15 15:36:41, Ryan Hansberry wrote:
> nit: "unable to decode"

Done.

[Ryan Hansberry, 2017-05-19 18:13:38]

Apologies for the late response. lgtm.

[Tim Song, 2017-05-19 20:52:48]

The CQ bit was checked by tengs@chromium.org

[commit-bot: I haz the power, 2017-05-19 20:53:58]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-19 22:09:39]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495227168190550,
"parent_rev": "2a552a75bf20b63dc035145a8b2eaef54c1e034c", "commit_rev":
"4fdb40570e2c40ffa318f0131b5cbee4900b964f"}

[commit-bot: I haz the power, 2017-05-19 22:09:53]

Description was changed from

==========
[EasyUnlock] Serialize and store BeaconSeeds as part of the cryptohome key
metadata.

For the login flow, we need to be able to use the same BeaconSeeds as those
in the user preferences.

The BeaconSeeds are refreshed everytime the user logs in or unlocks their
Chromebook like the other EasyUnlock metadata.

BUG=715594
==========

to

==========
[EasyUnlock] Serialize and store BeaconSeeds as part of the cryptohome key
metadata.

For the login flow, we need to be able to use the same BeaconSeeds as those
in the user preferences.

The BeaconSeeds are refreshed everytime the user logs in or unlocks their
Chromebook like the other EasyUnlock metadata.

BUG=715594

Review-Url: https://codereview.chromium.org/2863533003
Cr-Commit-Position: refs/heads/master@{#473352}
Committed:
https://chromium.googlesource.com/chromium/src/+/4fdb40570e2c40ffa318f0131b5c...
==========

[commit-bot: I haz the power, 2017-05-19 22:09:54]

Committed patchset #3 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/4fdb40570e2c40ffa318f0131b5c...