Update OwnerKeyUtils API

In real usage, we won't be exporting public keys to a file, so refactor and update the API to reflect this.  Also, add a method to find a private key, given its associated public key.

BUG=chromium-os:4485
TEST=unit tests

[Chris Masone, 2010-07-28 00:43:55]

On 2010/07/27 02:34:49, cmasone wrote:
> 

ping :-)

[Chris Masone, 2010-07-28 17:07:13]

On 2010/07/28 00:43:55, cmasone wrote:
> On 2010/07/27 02:34:49, cmasone wrote:
> > 
> 
> ping :-)

+gauravsh

[wtc, 2010-07-28 17:52:16]

LGTM.  I have some suggested changes below.  Note in particular
the comments marked with "BUG" and "API DESIGN ISSUE".

http://codereview.chromium.org/3078001/diff/1/2
File chrome/browser/chromeos/login/owner_key_utils.cc (right):

http://codereview.chromium.org/3078001/diff/1/2#newcode14
chrome/browser/chromeos/login/owner_key_utils.cc:14: #include <vector>
Nit: I don't see where std::vector is used in this file.

http://codereview.chromium.org/3078001/diff/1/2#newcode75
chrome/browser/chromeos/login/owner_key_utils.cc:75: static bool
FormatPublicKeyForExport(SECKEYPublicKey* key,
Nit: "Format something for export" simply means encode it.
So you could also just name this method EncodePublicKey.

http://codereview.chromium.org/3078001/diff/1/2#newcode80
chrome/browser/chromeos/login/owner_key_utils.cc:80: static const char
kOwnerKeyFile[];
Delete this.  This has been moved to the base class.

http://codereview.chromium.org/3078001/diff/1/2#newcode90
chrome/browser/chromeos/login/owner_key_utils.cc:90: OwnerKeyUtils*
OwnerKeyUtils::Create() {
Should we move OwnerKeyUtils::Create() to the OwnerKeyUtils
section above?

http://codereview.chromium.org/3078001/diff/1/2#newcode185
chrome/browser/chromeos/login/owner_key_utils.cc:185: bool ok = false;
BUG: ok is initialized to false and never changed.
So this function always returns false.

http://codereview.chromium.org/3078001/diff/1/2#newcode193
chrome/browser/chromeos/login/owner_key_utils.cc:193: // send the data over
dbus.
I see no code to back up this comment.  Perhaps this comment
is a TODO?

If the code to send the data over dbus indeed hasn't been
written, then it's correct for the function to return false.

http://codereview.chromium.org/3078001/diff/1/2#newcode210
chrome/browser/chromeos/login/owner_key_utils.cc:210: LOG(ERROR) << "key is too
big! " << to_export.size();
Nit: to_export.size() => to_export.length()
for consistency.

http://codereview.chromium.org/3078001/diff/1/2#newcode308
chrome/browser/chromeos/login/owner_key_utils.cc:308: if (!key)
Unless you want to allow passing a NULL 'key' to this function,
a DCHECK(key) here would catch programming errors.

http://codereview.chromium.org/3078001/diff/1/2#newcode329
chrome/browser/chromeos/login/owner_key_utils.cc:329: SECITEM_ZfreeItem(ck_id,
PR_TRUE);
Nit: it should be OK to use SECITEM_FreeItem on ck_id,
because ck_id is generated from the public key.  Note
that you're using SECITEM_FreeItem to free 'der' (the
SubjectPublicKeyInfo) at line 303 above.

SECITEM_ZfreeItem is used when the SECItem contains
sensitive info such as private/secret keys or passwords.

http://codereview.chromium.org/3078001/diff/1/3
File chrome/browser/chromeos/login/owner_key_utils.h (right):

http://codereview.chromium.org/3078001/diff/1/3#newcode30
chrome/browser/chromeos/login/owner_key_utils.h:30: // The place outside the
owner's encrypted home directory where her
"place" doesn't say what kOwnerKeyFile is.  Is it the pathname
of the owner key file?

http://codereview.chromium.org/3078001/diff/1/3#newcode32
chrome/browser/chromeos/login/owner_key_utils.h:32: static const char
kOwnerKeyFile[];
API DESIGN ISSUE: It is strange for kOwnerKeyFile to be
available in the base class but the closely related
GetOwnerKeyFilePath method to be abstract (= 0).

http://codereview.chromium.org/3078001/diff/1/3#newcode64
chrome/browser/chromeos/login/owner_key_utils.h:64: virtual bool
ExportPublicKey(SECKEYPublicKey* key) = 0;
Perhaps this method should be named ExportPublicKeyToDBus?
The fact that it exports the public key to (or via) DBus
is not obvious at all, since there is no parameter that
suggests DBus is involved.

http://codereview.chromium.org/3078001/diff/1/3#newcode85
chrome/browser/chromeos/login/owner_key_utils.h:85: // memory and ALSO remove
them from the NSS token.
Nit: token => database

"token" is correct, but I prefer a more accessible term.

http://codereview.chromium.org/3078001/diff/1/4
File chrome/browser/chromeos/login/owner_key_utils_unittest.cc (right):

http://codereview.chromium.org/3078001/diff/1/4#newcode76
chrome/browser/chromeos/login/owner_key_utils_unittest.cc:76: // an ID from the
key, and then use that ID to query the token in the
This comment needs updating as some of the code it describes
("We'll create an ID from the key...") has been moved to the
new utils_->FindPrivateKey method.  I think you can just delete
the second sentence in the comment block.

[gauravsh, 2010-07-28 20:47:51]

LGTM

http://codereview.chromium.org/3078001/diff/1/2
File chrome/browser/chromeos/login/owner_key_utils.cc (right):

http://codereview.chromium.org/3078001/diff/1/2#newcode65
chrome/browser/chromeos/login/owner_key_utils.cc:65: // struct for |key_der|. 
|key_der->data| should be initialized to NULL
so I just noticed this comment. Shouldn't you be enforcing and checking for
these 2 conditions (key_der->data and len being 0) using DCHECK or similar in
the method implementation?

http://codereview.chromium.org/3078001/diff/1/2#newcode316
chrome/browser/chromeos/login/owner_key_utils.cc:316: if (NULL == slot)
nit: consistency of NULL checks - you use a !pointer style check in most other
places but explicitly do a comparison to NULL in this and the check below.

[Chris Masone, 2010-07-28 21:24:47]

committing...

http://codereview.chromium.org/3078001/diff/1/2
File chrome/browser/chromeos/login/owner_key_utils.cc (right):

http://codereview.chromium.org/3078001/diff/1/2#newcode14
chrome/browser/chromeos/login/owner_key_utils.cc:14: #include <vector>
On 2010/07/28 17:52:18, wtc wrote:
> Nit: I don't see where std::vector is used in this file.

Done.

http://codereview.chromium.org/3078001/diff/1/2#newcode65
chrome/browser/chromeos/login/owner_key_utils.cc:65: // struct for |key_der|. 
|key_der->data| should be initialized to NULL
On 2010/07/28 20:47:51, gauravsh wrote:
> so I just noticed this comment. Shouldn't you be enforcing and checking for
> these 2 conditions (key_der->data and len being 0) using DCHECK or similar in
> the method implementation?

Done.

http://codereview.chromium.org/3078001/diff/1/2#newcode75
chrome/browser/chromeos/login/owner_key_utils.cc:75: static bool
FormatPublicKeyForExport(SECKEYPublicKey* key,
On 2010/07/28 17:52:18, wtc wrote:
> Nit: "Format something for export" simply means encode it.
> So you could also just name this method EncodePublicKey.

Done.

http://codereview.chromium.org/3078001/diff/1/2#newcode80
chrome/browser/chromeos/login/owner_key_utils.cc:80: static const char
kOwnerKeyFile[];
On 2010/07/28 17:52:18, wtc wrote:
> Delete this.  This has been moved to the base class.

Actually, this is the one I want to keep :-)

http://codereview.chromium.org/3078001/diff/1/2#newcode90
chrome/browser/chromeos/login/owner_key_utils.cc:90: OwnerKeyUtils*
OwnerKeyUtils::Create() {
On 2010/07/28 17:52:18, wtc wrote:
> Should we move OwnerKeyUtils::Create() to the OwnerKeyUtils
> section above?

Can't, because I need OwnerKeyUtilsImpl.  Tried a fwd decl, but no dice.  Added
a comment to this effect.

http://codereview.chromium.org/3078001/diff/1/2#newcode185
chrome/browser/chromeos/login/owner_key_utils.cc:185: bool ok = false;
On 2010/07/28 17:52:18, wtc wrote:
> BUG: ok is initialized to false and never changed.
> So this function always returns false.

See below

http://codereview.chromium.org/3078001/diff/1/2#newcode193
chrome/browser/chromeos/login/owner_key_utils.cc:193: // send the data over
dbus.
On 2010/07/28 17:52:18, wtc wrote:
> I see no code to back up this comment.  Perhaps this comment
> is a TODO?

You're right, which is why I always return false.  Made this comment a TODO.

> 
> If the code to send the data over dbus indeed hasn't been
> written, then it's correct for the function to return false.

http://codereview.chromium.org/3078001/diff/1/2#newcode210
chrome/browser/chromeos/login/owner_key_utils.cc:210: LOG(ERROR) << "key is too
big! " << to_export.size();
On 2010/07/28 17:52:18, wtc wrote:
> Nit: to_export.size() => to_export.length()
> for consistency.

Done.

http://codereview.chromium.org/3078001/diff/1/2#newcode308
chrome/browser/chromeos/login/owner_key_utils.cc:308: if (!key)
On 2010/07/28 17:52:18, wtc wrote:
> Unless you want to allow passing a NULL 'key' to this function,
> a DCHECK(key) here would catch programming errors.

Done.

http://codereview.chromium.org/3078001/diff/1/2#newcode316
chrome/browser/chromeos/login/owner_key_utils.cc:316: if (NULL == slot)
On 2010/07/28 20:47:51, gauravsh wrote:
> nit: consistency of NULL checks - you use a !pointer style check in most other
> places but explicitly do a comparison to NULL in this and the check below. 
> 
> 

Done.

http://codereview.chromium.org/3078001/diff/1/2#newcode329
chrome/browser/chromeos/login/owner_key_utils.cc:329: SECITEM_ZfreeItem(ck_id,
PR_TRUE);
On 2010/07/28 17:52:18, wtc wrote:
> Nit: it should be OK to use SECITEM_FreeItem on ck_id,
> because ck_id is generated from the public key.  Note
> that you're using SECITEM_FreeItem to free 'der' (the
> SubjectPublicKeyInfo) at line 303 above.
> 
> SECITEM_ZfreeItem is used when the SECItem contains
> sensitive info such as private/secret keys or passwords.

Done.

http://codereview.chromium.org/3078001/diff/1/3
File chrome/browser/chromeos/login/owner_key_utils.h (right):

http://codereview.chromium.org/3078001/diff/1/3#newcode30
chrome/browser/chromeos/login/owner_key_utils.h:30: // The place outside the
owner's encrypted home directory where her
On 2010/07/28 17:52:18, wtc wrote:
> "place" doesn't say what kOwnerKeyFile is.  Is it the pathname
> of the owner key file?

It's the fully-qualified filename.  Also, this is in OwnerKeyUtilsImpl now.

http://codereview.chromium.org/3078001/diff/1/3#newcode32
chrome/browser/chromeos/login/owner_key_utils.h:32: static const char
kOwnerKeyFile[];
On 2010/07/28 17:52:18, wtc wrote:
> API DESIGN ISSUE: It is strange for kOwnerKeyFile to be
> available in the base class but the closely related
> GetOwnerKeyFilePath method to be abstract (= 0).


True.  I meant it to be in the child class.  It got shuffled around in a bad
refactor/merge.  Fixed now.

http://codereview.chromium.org/3078001/diff/1/3#newcode64
chrome/browser/chromeos/login/owner_key_utils.h:64: virtual bool
ExportPublicKey(SECKEYPublicKey* key) = 0;
On 2010/07/28 17:52:18, wtc wrote:
> Perhaps this method should be named ExportPublicKeyToDBus?
> The fact that it exports the public key to (or via) DBus
> is not obvious at all, since there is no parameter that
> suggests DBus is involved.

ExportPublicKeyViaDbus() is the new name.

http://codereview.chromium.org/3078001/diff/1/3#newcode85
chrome/browser/chromeos/login/owner_key_utils.h:85: // memory and ALSO remove
them from the NSS token.
On 2010/07/28 17:52:18, wtc wrote:
> Nit: token => database
> 
> "token" is correct, but I prefer a more accessible term.

Done.

http://codereview.chromium.org/3078001/diff/1/4
File chrome/browser/chromeos/login/owner_key_utils_unittest.cc (right):

http://codereview.chromium.org/3078001/diff/1/4#newcode76
chrome/browser/chromeos/login/owner_key_utils_unittest.cc:76: // an ID from the
key, and then use that ID to query the token in the
On 2010/07/28 17:52:18, wtc wrote:
> This comment needs updating as some of the code it describes
> ("We'll create an ID from the key...") has been moved to the
> new utils_->FindPrivateKey method.  I think you can just delete
> the second sentence in the comment block.

Done.