Merge cryptohome::RetrievedKeyData with cryptohome::KeyDefinition

Merge cryptohome::RetrievedKeyData with cryptohome::KeyDefinition

cryptohome::RetrievedKeyData and cryptohome::KeyDefinition both represent
subsets of the Key protobuf. This CL merges the two classes and makes the
resulting cryptohome::KeyDefinition contain all of Key's fields.

TBR=pamg (manager_password_service.cc)

BUG=367847
TEST=Updated unit tests

Committed: https://crrev.com/e7e066d00108649cb3761fc5f450a8d9068fa8b3
Cr-Commit-Position: refs/heads/master@{#294823}

[bartfab (slow), 2014-09-02 12:28:45]

bartfab@chromium.org changed reviewers:
+ dkrahn@chromium.org, dzhioev@chromium.org

[bartfab (slow), 2014-09-02 12:28:46]

Hi Darren,

Could you take a look at chromeos/cryptohome/*?

Hi Pavel,

Could you take a look at:

chromeos/login/auth/*
chrome/browser/chromeos/login/*

TBR=bauerb (manager_password_service.cc)

[Darren Krahn, 2014-09-02 18:21:53]

https://codereview.chromium.org/526353002/diff/1/chromeos/cryptohome/cryptoho...
File chromeos/cryptohome/cryptohome_parameters.cc (right):

https://codereview.chromium.org/526353002/diff/1/chromeos/cryptohome/cryptoho...
chromeos/cryptohome/cryptohome_parameters.cc:81: (!bytes || (other.bytes &&
*bytes == *other.bytes)));
Should we verify that both have NULL in !number and !bytes case?

https://codereview.chromium.org/526353002/diff/1/chromeos/cryptohome/cryptoho...
File chromeos/cryptohome/cryptohome_parameters.h (right):

https://codereview.chromium.org/526353002/diff/1/chromeos/cryptohome/cryptoho...
chromeos/cryptohome/cryptohome_parameters.h:36: // It contains authorization
data along with extra parameters like perimissions
old code nit: perimissions -> permissions

https://codereview.chromium.org/526353002/diff/1/chromeos/cryptohome/cryptoho...
chromeos/cryptohome/cryptohome_parameters.h:43: struct AuthorizationData {
IMO, this is under-documented in the protobuf and here's a chance to make it
easier to understand. A description of AuthorizationData and its intended use
would be great.

https://codereview.chromium.org/526353002/diff/1/chromeos/cryptohome/cryptoho...
chromeos/cryptohome/cryptohome_parameters.h:50: Secret(bool encrypt,
A constructor for the way we typically use this would be nice. E.g.

Secret(const std::string& hmac_key);

Also, because this is a struct, would a default constructor be appropriate?

https://codereview.chromium.org/526353002/diff/1/chromeos/cryptohome/cryptoho...
chromeos/cryptohome/cryptohome_parameters.h:65: AuthorizationData();
Same here, a typical usage constructor would be nice (in addition to this one).
E.g.

AuthorizationData(const std::string& hmac_key);

https://codereview.chromium.org/526353002/diff/1/chromeos/cryptohome/cryptoho...
chromeos/cryptohome/cryptohome_parameters.h:74: struct ProviderData {
Same here -- a bit of documentation would be great.

https://codereview.chromium.org/526353002/diff/1/chromeos/cryptohome/cryptoho...
chromeos/cryptohome/cryptohome_parameters.h:89: int privileges);
[optional] Default constructor here too? It just seems intuitive to me that a
struct can be declared like:

MyStruct my_struct;

https://codereview.chromium.org/526353002/diff/1/chromeos/cryptohome/cryptoho...
chromeos/cryptohome/cryptohome_parameters.h:94: void AddSymmetricKey(bool
encrypt,
It's not clear what this method does -- would it be better to call it
AddAuthorizationData or AddAuthorizationKey or AddAuthorizationHMACKey?

It's also not intuitive what the parameters actually mean. AFAIK, only 'sign'
with 'hmac' is supported so we could drop the two bools unless we intend to use
them in the short-medium term.

https://codereview.chromium.org/526353002/diff/1/chromeos/cryptohome/homedir_...
File chromeos/cryptohome/homedir_methods.cc (right):

https://codereview.chromium.org/526353002/diff/1/chromeos/cryptohome/homedir_...
chromeos/cryptohome/homedir_methods.cc:81: if (!key_def.provider_data.empty()) {
nit: conditional not necessary

[dzhioev (left Google), 2014-09-03 17:42:46]

LGTM with nit

https://codereview.chromium.org/526353002/diff/20001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/login/supervised/supervised_user_creation_controller_new.cc
(right):

https://codereview.chromium.org/526353002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/supervised/supervised_user_creation_controller_new.cc:275:
password_key.AddSymmetricKey(true /* encrypt */,
Nit: Is situation when (encrypt == sign) possible? If not, I prefer to have
Add{Encryption/Signature}Key methods instead AddSymmetricKey.

[bartfab (slow), 2014-09-04 10:14:19]

https://codereview.chromium.org/526353002/diff/1/chromeos/cryptohome/cryptoho...
File chromeos/cryptohome/cryptohome_parameters.cc (right):

https://codereview.chromium.org/526353002/diff/1/chromeos/cryptohome/cryptoho...
chromeos/cryptohome/cryptohome_parameters.cc:81: (!bytes || (other.bytes &&
*bytes == *other.bytes)));
On 2014/09/02 18:21:52, Darren Krahn wrote:
> Should we verify that both have NULL in !number and !bytes case?

Done.

https://codereview.chromium.org/526353002/diff/1/chromeos/cryptohome/cryptoho...
File chromeos/cryptohome/cryptohome_parameters.h (right):

https://codereview.chromium.org/526353002/diff/1/chromeos/cryptohome/cryptoho...
chromeos/cryptohome/cryptohome_parameters.h:36: // It contains authorization
data along with extra parameters like perimissions
On 2014/09/02 18:21:53, Darren Krahn wrote:
> old code nit: perimissions -> permissions

Done.

https://codereview.chromium.org/526353002/diff/1/chromeos/cryptohome/cryptoho...
chromeos/cryptohome/cryptohome_parameters.h:43: struct AuthorizationData {
On 2014/09/02 18:21:53, Darren Krahn wrote:
> IMO, this is under-documented in the protobuf and here's a chance to make it
> easier to understand. A description of AuthorizationData and its intended use
> would be great.

Unfortunately, I do not understand it either. I see that everyone always sets
this to |TYPE_HMACSHA256|. I have no idea what this is for and I do not know
when the other value would be used. Is there a doc that explains this somewhere?
I would be happy to distill it into a comment.

https://codereview.chromium.org/526353002/diff/1/chromeos/cryptohome/cryptoho...
chromeos/cryptohome/cryptohome_parameters.h:50: Secret(bool encrypt,
On 2014/09/02 18:21:53, Darren Krahn wrote:
> A constructor for the way we typically use this would be nice. E.g.
> 
> Secret(const std::string& hmac_key);

Our typical use actuall always needs at least |encrypt|, |sign| and
|symmetric_key| and in some cases, the other two arguments as well. We never
construct a Secret with just a |symmetric_key|.

> 
> Also, because this is a struct, would a default constructor be appropriate?

I added default constructors for all structs in this file.

https://codereview.chromium.org/526353002/diff/1/chromeos/cryptohome/cryptoho...
chromeos/cryptohome/cryptohome_parameters.h:65: AuthorizationData();
On 2014/09/02 18:21:53, Darren Krahn wrote:
> Same here, a typical usage constructor would be nice (in addition to this
one).
> E.g.
> 
> AuthorizationData(const std::string& hmac_key);

Done.

https://codereview.chromium.org/526353002/diff/1/chromeos/cryptohome/cryptoho...
chromeos/cryptohome/cryptohome_parameters.h:74: struct ProviderData {
On 2014/09/02 18:21:53, Darren Krahn wrote:
> Same here -- a bit of documentation would be great.

Done.

https://codereview.chromium.org/526353002/diff/1/chromeos/cryptohome/cryptoho...
chromeos/cryptohome/cryptohome_parameters.h:89: int privileges);
On 2014/09/02 18:21:53, Darren Krahn wrote:
> [optional] Default constructor here too? It just seems intuitive to me that a
> struct can be declared like:
> 
> MyStruct my_struct;

Done.

https://codereview.chromium.org/526353002/diff/1/chromeos/cryptohome/cryptoho...
chromeos/cryptohome/cryptohome_parameters.h:94: void AddSymmetricKey(bool
encrypt,
On 2014/09/02 18:21:53, Darren Krahn wrote:
> It's not clear what this method does -- would it be better to call it
> AddAuthorizationData or AddAuthorizationKey or AddAuthorizationHMACKey?
> 
> It's also not intuitive what the parameters actually mean. AFAIK, only 'sign'
> with 'hmac' is supported so we could drop the two bools unless we intend to
use
> them in the short-medium term.

I removed the method as AuthorizationData() has a handy constructor now. We do
use both |sign| and |encrypt| by the way. The two places that create
authorization data (SupervisedUserCreationControllerNew and
ManagerPasswordService) create two keys each, an ecryption key and a signing
key.

https://codereview.chromium.org/526353002/diff/1/chromeos/cryptohome/homedir_...
File chromeos/cryptohome/homedir_methods.cc (right):

https://codereview.chromium.org/526353002/diff/1/chromeos/cryptohome/homedir_...
chromeos/cryptohome/homedir_methods.cc:81: if (!key_def.provider_data.empty()) {
On 2014/09/02 18:21:53, Darren Krahn wrote:
> nit: conditional not necessary

Done.

https://codereview.chromium.org/526353002/diff/20001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/login/supervised/supervised_user_creation_controller_new.cc
(right):

https://codereview.chromium.org/526353002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/supervised/supervised_user_creation_controller_new.cc:275:
password_key.AddSymmetricKey(true /* encrypt */,
On 2014/09/03 17:42:46, dzhioev wrote:
> Nit: Is situation when (encrypt == sign) possible? If not, I prefer to have
> Add{Encryption/Signature}Key methods instead AddSymmetricKey.

Depending on the HMAC used, it may or may not be safe to use the same key for
encryption and signing. Currently, we always have separate keys but this could
change.

[Darren Krahn, 2014-09-04 23:13:24]

chromeos/cryptohome lgtm (w/ a comment for AuthorizationData)

https://codereview.chromium.org/526353002/diff/1/chromeos/cryptohome/cryptoho...
File chromeos/cryptohome/cryptohome_parameters.h (right):

https://codereview.chromium.org/526353002/diff/1/chromeos/cryptohome/cryptoho...
chromeos/cryptohome/cryptohome_parameters.h:43: struct AuthorizationData {
On 2014/09/04 10:14:18, bartfab - very busy til 15 Sep wrote:
> On 2014/09/02 18:21:53, Darren Krahn wrote:
> > IMO, this is under-documented in the protobuf and here's a chance to make it
> > easier to understand. A description of AuthorizationData and its intended
use
> > would be great.
> 
> Unfortunately, I do not understand it either. I see that everyone always sets
> this to |TYPE_HMACSHA256|. I have no idea what this is for and I do not know
> when the other value would be used. Is there a doc that explains this
somewhere?
> I would be happy to distill it into a comment.

I shared the closest doc I could find. My understanding is that this allows a
key to be managed server-side -- or by a supervisor or some entity other than
the user. It is configured with AddKey and verified by cryptohome on UpdateKey.

Currently the only mechanism supported by cryptohome (AFAIK) is HMAC-SHA256 for
signing.

https://codereview.chromium.org/526353002/diff/1/chromeos/cryptohome/cryptoho...
chromeos/cryptohome/cryptohome_parameters.h:94: void AddSymmetricKey(bool
encrypt,
On 2014/09/04 10:14:18, bartfab - very busy til 15 Sep wrote:
> On 2014/09/02 18:21:53, Darren Krahn wrote:
> > It's not clear what this method does -- would it be better to call it
> > AddAuthorizationData or AddAuthorizationKey or AddAuthorizationHMACKey?
> > 
> > It's also not intuitive what the parameters actually mean. AFAIK, only
'sign'
> > with 'hmac' is supported so we could drop the two bools unless we intend to
> use
> > them in the short-medium term.
> 
> I removed the method as AuthorizationData() has a handy constructor now. We do
> use both |sign| and |encrypt| by the way. The two places that create
> authorization data (SupervisedUserCreationControllerNew and
> ManagerPasswordService) create two keys each, an ecryption key and a signing
> key.

Ok, I guess they are just future-proofing, currently the only encryption
mechanism is AES256CBC_HMAC256 and it's not supported by cryptohome.

[xiyuan, 2014-09-12 16:43:02]

Bartosz, when do you plan to land this CL?

I have a https://codereview.chromium.org/554043003/ that needs to set
KeyProviderData for AddKeyEx call and could use this CL as its base.

[chromium-reviews, 2014-09-12 16:49:38]

I am traveling back from MTV to MUC today. I will be able to land this on
Monday.

On 12 September 2014 09:43, <xiyuan@chromium.org> wrote:

> Bartosz, when do you plan to land this CL?
>
> I have a https://codereview.chromium.org/554043003/ that needs to set
> KeyProviderData for AddKeyEx call and could use this CL as its base.
>
> https://codereview.chromium.org/526353002/
>



-- 
Google Germany GmbH
Dienerstr. 12
80331 München

Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
Geschäftsführer: Graham Law, Christine Elizabeth Flores

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[xiyuan, 2014-09-12 16:59:48]

On 2014/09/12 16:49:38, chromium-reviews wrote:
> I am traveling back from MTV to MUC today. I will be able to land this on
> Monday.
> 

Cool. Thanks for the heads up.

I can wait for Monday. So I'll rebase my CL on top of yous then.

[bartfab (slow), 2014-09-15 12:04:44]

https://codereview.chromium.org/526353002/diff/1/chromeos/cryptohome/cryptoho...
File chromeos/cryptohome/cryptohome_parameters.h (right):

https://codereview.chromium.org/526353002/diff/1/chromeos/cryptohome/cryptoho...
chromeos/cryptohome/cryptohome_parameters.h:43: struct AuthorizationData {
On 2014/09/04 23:13:23, Darren Krahn wrote:
> On 2014/09/04 10:14:18, bartfab - very busy til 15 Sep wrote:
> > On 2014/09/02 18:21:53, Darren Krahn wrote:
> > > IMO, this is under-documented in the protobuf and here's a chance to make
it
> > > easier to understand. A description of AuthorizationData and its intended
> use
> > > would be great.
> > 
> > Unfortunately, I do not understand it either. I see that everyone always
sets
> > this to |TYPE_HMACSHA256|. I have no idea what this is for and I do not know
> > when the other value would be used. Is there a doc that explains this
> somewhere?
> > I would be happy to distill it into a comment.
> 
> I shared the closest doc I could find. My understanding is that this allows a
> key to be managed server-side -- or by a supervisor or some entity other than
> the user. It is configured with AddKey and verified by cryptohome on
UpdateKey.
> 
> Currently the only mechanism supported by cryptohome (AFAIK) is HMAC-SHA256
for
> signing.

I cannot find any doc shared with me on that day. If you are OK with it, I will
land the CL as-is and check with Will on the exact purpose of AuthorizationData.
I do not want to add a description that ends up being incorrect.

[bartfab (slow), 2014-09-15 12:05:54]

The CQ bit was checked by bartfab@chromium.org

[commit-bot: I haz the power, 2014-09-15 12:10:34]

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

[commit-bot: I haz the power, 2014-09-15 13:13:00]

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

[commit-bot: I haz the power, 2014-09-15 13:13:02]

Failed to commit the patch.

[bartfab (slow), 2014-09-15 14:38:33]

The CQ bit was checked by bartfab@chromium.org

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

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

[commit-bot: I haz the power, 2014-09-15 14:41:57]

Committed patchset #4 (id:60001) as daa13df1fe2accd1dd1d0130dbbae52697aba7fa

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

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