This change loads opencryptoki and uses the TPM for keygen tags.

This change loads opencryptoki and uses the TPM for keygen tags.
on ChromeOS.

After this change, on ChromeOS we will use the TPM to generate keys
for keygen tags in forms.  NSS will also have opencryptoki loaded so
it can talk to the TPM.

BUG=chromium-os:12416, chromium-os:12417
TEST=Generated keys on a ChromeOS device.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=80806

[stevenjb, 2011-03-14 18:41:21]

lgtm, but someone more familiar with NSS should probably look at it also.

http://codereview.chromium.org/6667020/diff/3004/base/nss_util.cc
File base/nss_util.cc (right):

http://codereview.chromium.org/6667020/diff/3004/base/nss_util.cc#newcode58
base/nss_util.cc:58: copied = PR_GetErrorText(error_text);
nit: combine above two lines?

[Greg Spencer (Chromium), 2011-03-14 18:48:09]

http://codereview.chromium.org/6667020/diff/3004/base/nss_util.cc
File base/nss_util.cc (right):

http://codereview.chromium.org/6667020/diff/3004/base/nss_util.cc#newcode58
base/nss_util.cc:58: copied = PR_GetErrorText(error_text);
On 2011/03/14 18:41:21, Steven Bennetts wrote:
> nit: combine above two lines?

Done.

[Greg Spencer (Chromium), 2011-03-15 23:48:37]

[+wtc, who might want to be a reviewer too]

Ping?  kmixter, rginda, comments?

[wtc, 2011-03-16 00:20:45]

gspencer: I will review this tomorrow.

[kmixter1, 2011-03-16 06:33:18]

Mostly nits but one concern about init'ing the user pin.

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc
File base/nss_util.cc (right):

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode48
base/nss_util.cc:48: static const char kHardwareTokenName[] = "Initialized by
CrOS";
If we support real hardware tokens this may seem confusing.  We seem to call it
the TPM-backed or TPM-wrapped token.  opencryptoki and TrouSers refer to it as
just the "TPM token".  Maybe kTpmTokenName?

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode53
base/nss_util.cc:53: static const FilePath::CharType kReadOnlyCertDB[] =
I don't understand what this does - is this only for testing?  And if so should
it be submitted?

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode104
base/nss_util.cc:104: if (std::string(PK11_GetTokenName(slot)) ==
std::string(kHardwareTokenName))
nit: constructing to std::string on rhs isn't necessary.

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode153
base/nss_util.cc:153: // AutoSECMODListLock is in scope.
Should the name of the class contain "read" since it's a read lock specifically
we're getting?

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode170
base/nss_util.cc:170: bool has_lock() { return lock_ != NULL; }
nit: const function

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode171
base/nss_util.cc:171: private:
nit: One line above private according to style guide?

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode182
base/nss_util.cc:182: int slot_count = item->module->loaded ?
item->module->slotCount : 0;
My brief reading of NSS says that it internally will crash if item->module is
NULL, so I assume it's ok to also make the non-NULL assumption here.  May be
worth noting the assumption is valid or DCHECKing.

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode259
base/nss_util.cc:259: PK11SlotInfo* GetRsaSlot() {
Seems like "RSA" applies to a software slot as well.  GetHardwareSlot or
GetTpmBackedSlot?

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode332
base/nss_util.cc:332: "database: " << GetNssError();
nice

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode410
base/nss_util.cc:410: VLOG(1) << "NSS_Shutdown failed; see
http://crosbug.com/4609";
crbug.com/4609

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode416
base/nss_util.cc:416: SECMODModule *InitDefaultRootCerts() {
nit: star in wrong place (even though you just moved this function)

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode454
base/nss_util.cc:454: PK11_InitPin(rsa_slot.get(),
If we incorrectly get here, executing this means we will clear the user's TPM
slot.  Seems like this code may be helpful for testing but should never be hit
in production.  Remove or only run for DEBUG builds?

[rginda, 2011-03-16 17:09:37]

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc
File base/nss_util.cc (right):

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode48
base/nss_util.cc:48: static const char kHardwareTokenName[] = "Initialized by
CrOS";
On 2011/03/16 06:33:18, kmixter1 wrote:
> If we support real hardware tokens this may seem confusing.  We seem to call
it
> the TPM-backed or TPM-wrapped token.  opencryptoki and TrouSers refer to it as
> just the "TPM token".  Maybe kTpmTokenName?

s/Name/Label/ would be more PKCS11'y.

[wtc, 2011-03-16 18:41:43]

LGTM.  All of my comments below are just nits.

- I suggest renaming some variables and functions.
- Move some code inside #if defined(USE_NSS) blocks.
- Minor improvements such as using strcmp() to compare
  C strings.

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc
File base/nss_util.cc (right):

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode41
base/nss_util.cc:41: namespace {
Nit: please add a blank line after this line.

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode43
base/nss_util.cc:43: static const char kNssDatabaseName[] = "Real NSS database";
Why do you need "Real"?  Is there a "Fake NSS database" too?

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode48
base/nss_util.cc:48: static const char kHardwareTokenName[] = "Initialized by
CrOS";
I agree with kmixter1 that these should be named "kTPMTokenName", kTPMUserPIN,
kTPMSecurityOfficerPIN.

On 2011/03/16 17:09:37, rginda wrote:
>
> s/Name/Label/ would be more PKCS11'y.

Nit: the NSS function is named PK11_GetTokenName, so it'd be
better to use "Name" here for consistency.

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode50
base/nss_util.cc:50: static const char kHardwareTokenSecurityOfficerPin[] =
"000000";
Nit: in the crypto and net code we usually use all caps for
abbreviations, so please try to use "NSS" and "PIN" in
identifiers.

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode57
base/nss_util.cc:57: std::string GetNssError() {
Nit: please rename this function GetNSSErrorMessage or
GetNSSErrorString.  "Error" alone usually means "error code".

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode102
base/nss_util.cc:102: // If we get asked for a password for the hardware token,
then
Nit: hardware token => TPM

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode104
base/nss_util.cc:104: if (std::string(PK11_GetTokenName(slot)) ==
std::string(kHardwareTokenName))
Just use strcmp() to compare the two C strings here and at
line 185 below.

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode150
base/nss_util.cc:150: #endif  // defined(USE_NSS)
Please move this #endif to include AutoSECMODListLock and
FindTokenByName.

The USE_NSS macro is defined for Linux and Chrome OS, which
uses the complete NSS, but undefined for Mac and Windows,
which uses a stripped-down subset of NSS.  FindTokenByNam
won't be used by Mac and Windows.

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode159
base/nss_util.cc:159: LOG(ERROR) << "AutoSECMODListLock: Unable to obtain
lock.";
Nit: "obtain lock" could be misinterpreted to mean "acquire lock".

I suggest "lock is not initialized".

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode170
base/nss_util.cc:170: bool has_lock() { return lock_ != NULL; }
Is the has_lock function useful?  "lock_initialized" would
be a better name.  ("has lock" could also mean "owns lock".)

It may be better to remove the lock_ null pointer check
and just let the AutoSECMODListLock constructor crash
inside the SECMOD_GetReadLock call.

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode254
base/nss_util.cc:254: EnsureRsaPinInit();
This function should be renamed EnsureTPMPINInit, and
GetRsaSlot should be renamed GetTPMSlot.

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode429
base/nss_util.cc:429: SECMODModule* LoadModule(const char* name,
Put the LoadModule function inside the USE_NSS block
above.

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode576
base/nss_util.cc:576: PK11SlotInfo* GetRsaKeySlot() {
This function should be named GetPreferredKeySlot or
GetPermanentKeySlot.

I wonder if we can just change GetDefaultKeySlot() to
use the TPM slot if it exists.

http://codereview.chromium.org/6667020/diff/4002/base/nss_util_internal.h
File base/nss_util_internal.h (right):

http://codereview.chromium.org/6667020/diff/4002/base/nss_util_internal.h#new...
base/nss_util_internal.h:22: // hardware key is present, will return NULL.  On
other OSs, This is
Nit: hardware key => hardware TPM

http://codereview.chromium.org/6667020/diff/4002/net/base/keygen_handler_nss.cc
File net/base/keygen_handler_nss.cc (right):

http://codereview.chromium.org/6667020/diff/4002/net/base/keygen_handler_nss....
net/base/keygen_handler_nss.cc:33: LOG(ERROR) << "Couldn't authenticate to RSA
key slot!";
The "RSA key slot" strings in this file should be changed
to "preferred key slot" or "permanent key slot".

[Greg Spencer (Chromium), 2011-03-16 21:37:51]

I think I've addressed everyone's issues.  Please take another look.

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc
File base/nss_util.cc (right):

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode41
base/nss_util.cc:41: namespace {
On 2011/03/16 18:41:44, wtc wrote:
> Nit: please add a blank line after this line.

Done.

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode43
base/nss_util.cc:43: static const char kNssDatabaseName[] = "Real NSS database";
On 2011/03/16 18:41:44, wtc wrote:
> Why do you need "Real"?  Is there a "Fake NSS database" too?

This is what it was always called, I just moved it here.  In any case, the
reason it's called that is because before the cryptohome is mounted, we have to
open a read-only non-persistent database.  Once it is mounted, we open the r/w
database that we actually use (which is this one).  I think the name is fairly
appropriate, but I didn't name it.

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode48
base/nss_util.cc:48: static const char kHardwareTokenName[] = "Initialized by
CrOS";
On 2011/03/16 18:41:44, wtc wrote:
> I agree with kmixter1 that these should be named "kTPMTokenName", kTPMUserPIN,
> kTPMSecurityOfficerPIN.
> 
> On 2011/03/16 17:09:37, rginda wrote:
> >
> > s/Name/Label/ would be more PKCS11'y.
> 
> Nit: the NSS function is named PK11_GetTokenName, so it'd be
> better to use "Name" here for consistency.

Changed the names to the ones wtc suggests.

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode50
base/nss_util.cc:50: static const char kHardwareTokenSecurityOfficerPin[] =
"000000";
On 2011/03/16 18:41:44, wtc wrote:
> Nit: in the crypto and net code we usually use all caps for
> abbreviations, so please try to use "NSS" and "PIN" in
> identifiers.

Done: changed to mirror other crypto code.  Note that this isn't typical for the
rest of Google: even acronyms use camel case (because of names like
"GetDNSSSLURI()": "GetDnsSslUri()" is clearer).

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode53
base/nss_util.cc:53: static const FilePath::CharType kReadOnlyCertDB[] =
On 2011/03/16 06:33:18, kmixter1 wrote:
> I don't understand what this does - is this only for testing?  And if so
should
> it be submitted?

Yes, it's only for testing, it's the location where we look for the fake root
for testing.  I didn't actually add this at all: I just moved it from down below
because it made sense to collect all the constants into one location.  It's been
checked in for a while.

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode57
base/nss_util.cc:57: std::string GetNssError() {
On 2011/03/16 18:41:44, wtc wrote:
> Nit: please rename this function GetNSSErrorMessage or
> GetNSSErrorString.  "Error" alone usually means "error code".

Done.

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode102
base/nss_util.cc:102: // If we get asked for a password for the hardware token,
then
On 2011/03/16 18:41:44, wtc wrote:
> Nit: hardware token => TPM

Done.

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode104
base/nss_util.cc:104: if (std::string(PK11_GetTokenName(slot)) ==
std::string(kHardwareTokenName))
On 2011/03/16 18:41:44, wtc wrote:
> Just use strcmp() to compare the two C strings here and at
> line 185 below.

I'm going to continue to use std::string operator== here because GetTPMTokenName
returns one, and because in the future when we get the name from the dbus API to
cryptohomed, it will continue to return one (i.e. I don't want to use the
constant here because it's just one more place I'll have to get rid of it anyhow
when the dbus API comes in).

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode150
base/nss_util.cc:150: #endif  // defined(USE_NSS)
On 2011/03/16 18:41:44, wtc wrote:
> Please move this #endif to include AutoSECMODListLock and
> FindTokenByName.
> 
> The USE_NSS macro is defined for Linux and Chrome OS, which
> uses the complete NSS, but undefined for Mac and Windows,
> which uses a stripped-down subset of NSS.  FindTokenByNam
> won't be used by Mac and Windows.

Done.

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode153
base/nss_util.cc:153: // AutoSECMODListLock is in scope.
On 2011/03/16 06:33:18, kmixter1 wrote:
> Should the name of the class contain "read" since it's a read lock
specifically
> we're getting?

Done.

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode170
base/nss_util.cc:170: bool has_lock() { return lock_ != NULL; }
On 2011/03/16 06:33:18, kmixter1 wrote:
> nit: const function

Done.

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode171
base/nss_util.cc:171: private:
On 2011/03/16 06:33:18, kmixter1 wrote:
> nit: One line above private according to style guide?

Done.

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode182
base/nss_util.cc:182: int slot_count = item->module->loaded ?
item->module->slotCount : 0;
On 2011/03/16 06:33:18, kmixter1 wrote:
> My brief reading of NSS says that it internally will crash if item->module is
> NULL, so I assume it's ok to also make the non-NULL assumption here.  May be
> worth noting the assumption is valid or DCHECKing.

Done.

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode254
base/nss_util.cc:254: EnsureRsaPinInit();
On 2011/03/16 18:41:44, wtc wrote:
> This function should be renamed EnsureTPMPINInit, and
> GetRsaSlot should be renamed GetTPMSlot.

Done.

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode259
base/nss_util.cc:259: PK11SlotInfo* GetRsaSlot() {
On 2011/03/16 06:33:18, kmixter1 wrote:
> Seems like "RSA" applies to a software slot as well.  GetHardwareSlot or
> GetTpmBackedSlot?

Renamed to GetTPMSlot

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode410
base/nss_util.cc:410: VLOG(1) << "NSS_Shutdown failed; see
http://crosbug.com/4609";
On 2011/03/16 06:33:18, kmixter1 wrote:
> crbug.com/4609

Whoops! Nice catch.  Done.

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode416
base/nss_util.cc:416: SECMODModule *InitDefaultRootCerts() {
On 2011/03/16 06:33:18, kmixter1 wrote:
> nit: star in wrong place (even though you just moved this function)

Done.

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode429
base/nss_util.cc:429: SECMODModule* LoadModule(const char* name,
On 2011/03/16 18:41:44, wtc wrote:
> Put the LoadModule function inside the USE_NSS block
> above.

Done.

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode454
base/nss_util.cc:454: PK11_InitPin(rsa_slot.get(),
On 2011/03/16 06:33:18, kmixter1 wrote:
> If we incorrectly get here, executing this means we will clear the user's TPM
> slot.  Seems like this code may be helpful for testing but should never be hit
> in production.  Remove or only run for DEBUG builds?

So, this will be removed as soon as we can ask cryptohomed if it has initialized
it.  It also won't clear the TPM because it won't happen if the TPM is already
initialized.  I'll add a TODO, however.

http://codereview.chromium.org/6667020/diff/4002/base/nss_util.cc#newcode576
base/nss_util.cc:576: PK11SlotInfo* GetRsaKeySlot() {
On 2011/03/16 18:41:44, wtc wrote:
> This function should be named GetPreferredKeySlot or
> GetPermanentKeySlot.

Yeah, I used RSA because I was trying to imply that it was the preferred slot on
the given platform for RSA keys.  I like GetPreferredKeySlot better.

> I wonder if we can just change GetDefaultKeySlot() to
> use the TPM slot if it exists.

No, because in some cases (inserting CA certs, for instance), we don't actually
want to use the TPM (mainly for performance).

http://codereview.chromium.org/6667020/diff/4002/base/nss_util_internal.h
File base/nss_util_internal.h (right):

http://codereview.chromium.org/6667020/diff/4002/base/nss_util_internal.h#new...
base/nss_util_internal.h:22: // hardware key is present, will return NULL.  On
other OSs, This is
On 2011/03/16 18:41:44, wtc wrote:
> Nit: hardware key => hardware TPM

Done.

http://codereview.chromium.org/6667020/diff/4002/net/base/keygen_handler_nss.cc
File net/base/keygen_handler_nss.cc (right):

http://codereview.chromium.org/6667020/diff/4002/net/base/keygen_handler_nss....
net/base/keygen_handler_nss.cc:33: LOG(ERROR) << "Couldn't authenticate to RSA
key slot!";
On 2011/03/16 18:41:44, wtc wrote:
> The "RSA key slot" strings in this file should be changed
> to "preferred key slot" or "permanent key slot".

Done.

[wtc, 2011-03-17 19:34:22]

LGTM.  I reviewed Patch Set 6 carefully so I apologize for
the number of comments below.  Most of them are just nits.
The two more serious ones are marked with "BUG".

Thank you for the code cleanup you did as part of this CL.

http://codereview.chromium.org/6667020/diff/14001/base/nss_util.cc
File base/nss_util.cc (right):

http://codereview.chromium.org/6667020/diff/14001/base/nss_util.cc#newcode44
base/nss_util.cc:44: static const char kNSSDatabaseName[] = "Real NSS database";
Nit: some of these constants, such as kNSSDatabaseName and
kReadOnlyCertDB, are used in only one function.  It may be
better to define those constants in those functions so that
the reader has everything he needs in one place to understand
the code. This is a matter of preference and a judgment call
on the trade-off with defining related constants in one place.

http://codereview.chromium.org/6667020/diff/14001/base/nss_util.cc#newcode51
base/nss_util.cc:51: static const char kTPMSecurityOfficerPin[] = "000000";
Nit: Pin => PIN
in lines 50 and 51.

http://codereview.chromium.org/6667020/diff/14001/base/nss_util.cc#newcode61
base/nss_util.cc:61: scoped_ptr<char> error_text(new
char[PR_GetErrorTextLength() + 1]);
BUG: scoped_ptr => scoped_array

The scoped_ptr destructor uses 'delete', but we need
'delete[]' here.

http://codereview.chromium.org/6667020/diff/14001/base/nss_util.cc#newcode65
base/nss_util.cc:65: result = StringPrintf("NSS Error code: %d", PR_GetError());
Nit: Error => error

http://codereview.chromium.org/6667020/diff/14001/base/nss_util.cc#newcode161
base/nss_util.cc:161: if (lock_)
Nit: delete the if (lock_) test because it's also deleted
in the constructor.

http://codereview.chromium.org/6667020/diff/14001/base/nss_util.cc#newcode173
base/nss_util.cc:173: for(SECMODModuleList* item = head; item != NULL; item =
item->next) {
Nit: add a space after 'for'.

http://codereview.chromium.org/6667020/diff/14001/base/nss_util.cc#newcode175
base/nss_util.cc:175: DCHECK(item->module); // This shouldn't happen or NSS
would crash anyhow.
Defintely remove DCHECK(item) on line 174 because it's
guaranteed by the condition of the for loop (item != NULL).

I would omit DCHECK(item->module) on line 175; we need to
trust NSS can implement a simple C linked list correctly :-)
But it's not a big deal.

http://codereview.chromium.org/6667020/diff/14001/base/nss_util.cc#newcode181
base/nss_util.cc:181: }
Nit: omit curly braces around one-liner if bodies.

http://codereview.chromium.org/6667020/diff/14001/base/nss_util.cc#newcode227
base/nss_util.cc:227: // the cert DB and the "default" crypto key provider,
which are
Nit: "the cert DB" here is the fake root CA cert DB, right?
If so, let's say that.

http://codereview.chromium.org/6667020/diff/14001/base/nss_util.cc#newcode282
base/nss_util.cc:282: PK11SlotInfo* GetDefaultKeySlot() {
We should rename this method to point out that this key
slot is an NSS DB, now that the TPM can also serve as
a key slot.

I guess we can simply rename it GetDefaultNSSKeySlot
to match the same-named free function.  Or
GetDefaultDBSlot.

http://codereview.chromium.org/6667020/diff/14001/base/nss_util.cc#newcode438
base/nss_util.cc:438: // in const interfaces.
This is an oversight.  I just filed NSS bug 642546:
https://bugzilla.mozilla.org/show_bug.cgi?id=642546
Please cite the NSS bug number in the comment.

http://codereview.chromium.org/6667020/diff/14001/base/nss_util.cc#newcode457
base/nss_util.cc:457: AutoNSSWriteLock lock;
BUG: the NSSWriteLock is not necessary here because you
are modifying the TPM, not the (real) NSS database.

http://codereview.chromium.org/6667020/diff/14001/base/nss_util.cc#newcode588
base/nss_util.cc:588: return g_nss_singleton.Get().GetDefaultKeySlot();
Nit: just call GetDefaultNSSKeySlot()?

http://codereview.chromium.org/6667020/diff/14001/base/nss_util.h
File base/nss_util.h (right):

http://codereview.chromium.org/6667020/diff/14001/base/nss_util.h#newcode9
base/nss_util.h:9: #include <string>
Nit: please add a blank line to separate C++ system headers
from Chrome's headers.

http://codereview.chromium.org/6667020/diff/14001/net/base/keygen_handler_nss.cc
File net/base/keygen_handler_nss.cc (right):

http://codereview.chromium.org/6667020/diff/14001/net/base/keygen_handler_nss...
net/base/keygen_handler_nss.cc:33: LOG(ERROR) << "Couldn't authenticate to
preferred key slot!";
Question: do you also need to change the PKCS #12 import code
to use GetPreferredKeySlot instead of GetDefaultNSSKeySlot?

[Greg Spencer (Chromium), 2011-03-17 22:41:09]

http://codereview.chromium.org/6667020/diff/14001/base/nss_util.cc
File base/nss_util.cc (right):

http://codereview.chromium.org/6667020/diff/14001/base/nss_util.cc#newcode44
base/nss_util.cc:44: static const char kNSSDatabaseName[] = "Real NSS database";
On 2011/03/17 19:34:22, wtc wrote:
> Nit: some of these constants, such as kNSSDatabaseName and
> kReadOnlyCertDB, are used in only one function.  It may be
> better to define those constants in those functions so that
> the reader has everything he needs in one place to understand
> the code. This is a matter of preference and a judgment call
> on the trade-off with defining related constants in one place.

I think I prefer all the constants in one place.

http://codereview.chromium.org/6667020/diff/14001/base/nss_util.cc#newcode51
base/nss_util.cc:51: static const char kTPMSecurityOfficerPin[] = "000000";
On 2011/03/17 19:34:22, wtc wrote:
> Nit: Pin => PIN
> in lines 50 and 51.

Done, although "Pin" is consistent with PK11_InitPin.

http://codereview.chromium.org/6667020/diff/14001/base/nss_util.cc#newcode61
base/nss_util.cc:61: scoped_ptr<char> error_text(new
char[PR_GetErrorTextLength() + 1]);
On 2011/03/17 19:34:22, wtc wrote:
> BUG: scoped_ptr => scoped_array
> 
> The scoped_ptr destructor uses 'delete', but we need
> 'delete[]' here.

Whoa.  Good catch, thanks.  I knew that already, I just typed the wrong thing.

http://codereview.chromium.org/6667020/diff/14001/base/nss_util.cc#newcode65
base/nss_util.cc:65: result = StringPrintf("NSS Error code: %d", PR_GetError());
On 2011/03/17 19:34:22, wtc wrote:
> Nit: Error => error

Done.

http://codereview.chromium.org/6667020/diff/14001/base/nss_util.cc#newcode161
base/nss_util.cc:161: if (lock_)
On 2011/03/17 19:34:22, wtc wrote:
> Nit: delete the if (lock_) test because it's also deleted
> in the constructor.

Done.

http://codereview.chromium.org/6667020/diff/14001/base/nss_util.cc#newcode173
base/nss_util.cc:173: for(SECMODModuleList* item = head; item != NULL; item =
item->next) {
On 2011/03/17 19:34:22, wtc wrote:
> Nit: add a space after 'for'.

Done.

http://codereview.chromium.org/6667020/diff/14001/base/nss_util.cc#newcode175
base/nss_util.cc:175: DCHECK(item->module); // This shouldn't happen or NSS
would crash anyhow.
On 2011/03/17 19:34:22, wtc wrote:
> Defintely remove DCHECK(item) on line 174 because it's
> guaranteed by the condition of the for loop (item != NULL).
> 
> I would omit DCHECK(item->module) on line 175; we need to
> trust NSS can implement a simple C linked list correctly :-)
> But it's not a big deal.

Yeah, I think I agree with you.  I didn't have this check in here originally,
but added it because Ken wanted more assurance.  I think that I'm going to
assume that NSS will crash long before this if a module is NULL.

http://codereview.chromium.org/6667020/diff/14001/base/nss_util.cc#newcode181
base/nss_util.cc:181: }
On 2011/03/17 19:34:22, wtc wrote:
> Nit: omit curly braces around one-liner if bodies.

Sorry, long standing habit.

http://codereview.chromium.org/6667020/diff/14001/base/nss_util.cc#newcode227
base/nss_util.cc:227: // the cert DB and the "default" crypto key provider,
which are
On 2011/03/17 19:34:22, wtc wrote:
> Nit: "the cert DB" here is the fake root CA cert DB, right?
> If so, let's say that.

Done.

http://codereview.chromium.org/6667020/diff/14001/base/nss_util.cc#newcode282
base/nss_util.cc:282: PK11SlotInfo* GetDefaultKeySlot() {
On 2011/03/17 19:34:22, wtc wrote:
> We should rename this method to point out that this key
> slot is an NSS DB, now that the TPM can also serve as
> a key slot.
> 
> I guess we can simply rename it GetDefaultNSSKeySlot
> to match the same-named free function.  Or
> GetDefaultDBSlot.
> 

Done.  Renamed to GetDefaultNSSKeySlot.

http://codereview.chromium.org/6667020/diff/14001/base/nss_util.cc#newcode457
base/nss_util.cc:457: AutoNSSWriteLock lock;
On 2011/03/17 19:34:22, wtc wrote:
> BUG: the NSSWriteLock is not necessary here because you
> are modifying the TPM, not the (real) NSS database.

OK, Removed.  It wasn't clear to me what parts of the NSS API would touch the DB
and what wouldn't, so I was erring on the side of caution.

http://codereview.chromium.org/6667020/diff/14001/base/nss_util.cc#newcode588
base/nss_util.cc:588: return g_nss_singleton.Get().GetDefaultKeySlot();
On 2011/03/17 19:34:22, wtc wrote:
> Nit: just call GetDefaultNSSKeySlot()?

Done.

[Greg Spencer (Chromium), 2011-03-17 22:50:10]

http://codereview.chromium.org/6667020/diff/18001/base/nss_util.cc
File base/nss_util.cc (right):

http://codereview.chromium.org/6667020/diff/18001/base/nss_util.cc#newcode338
base/nss_util.cc:338: if (nodb_init) {
FYI: Rietveld's diff algorithm got very confused here by the indent change
introduced by an outside change that I had to merge.

In reality, there are no changed lines in this function other than the
"GetNSSErrorMessage()" calls for the error messages.

[kmixter1, 2011-03-18 03:53:26]

LGTM - though have you checked if entd-installed certs show up in certificate
manager?

[wtc, 2011-03-18 18:36:32]

LGTM!

I only reviewed the diffs between Patch Sets 6 and 7.

http://codereview.chromium.org/6667020/diff/18001/base/nss_util.cc
File base/nss_util.cc (right):

http://codereview.chromium.org/6667020/diff/18001/base/nss_util.cc#newcode21
base/nss_util.cc:21: #include "base/environment.h"
Nit: list "base/environment.h" after "base/crypto/scoped_nss_types.h"?

http://codereview.chromium.org/6667020/diff/18001/base/nss_util.cc#newcode369
base/nss_util.cc:369: VLOG(1) << "Initializing NSS without a persistent
database.";
LOG(WARNING) seems appropriate for this log message.

http://codereview.chromium.org/6667020/diff/18001/base/nss_util.cc#newcode448
base/nss_util.cc:448: // const interface for this.  Bug
Nit: SECMOD is a C API, so function overloading is not
available to provide a second const interface.

We should say "but SECMOD doesn't properly declare input
string arguments as const."

[Greg Spencer (Chromium), 2011-03-18 20:43:11]

http://codereview.chromium.org/6667020/diff/18001/base/nss_util.cc
File base/nss_util.cc (right):

http://codereview.chromium.org/6667020/diff/18001/base/nss_util.cc#newcode21
base/nss_util.cc:21: #include "base/environment.h"
On 2011/03/18 18:36:32, wtc wrote:
> Nit: list "base/environment.h" after "base/crypto/scoped_nss_types.h"?

Sorry, bad merge.  Fixed.

http://codereview.chromium.org/6667020/diff/18001/base/nss_util.cc#newcode369
base/nss_util.cc:369: VLOG(1) << "Initializing NSS without a persistent
database.";
On 2011/03/18 18:36:32, wtc wrote:
> LOG(WARNING) seems appropriate for this log message.

Well, I had that, but this happens every time ChromeOS starts, so it's really
pretty expected.  It can give some debugging info though, which is why I didn't
remove it entirely.

http://codereview.chromium.org/6667020/diff/18001/base/nss_util.cc#newcode448
base/nss_util.cc:448: // const interface for this.  Bug
On 2011/03/18 18:36:32, wtc wrote:
> Nit: SECMOD is a C API, so function overloading is not
> available to provide a second const interface.
> 
> We should say "but SECMOD doesn't properly declare input
> string arguments as const."

OK, done.  I wasn't thinking it would be an overloaded interface (I realize it's
a C API), just an interface in the more general "'I' part of API" sense.

[Greg Spencer (Chromium), 2011-03-18 20:44:02]

On 2011/03/18 03:53:26, kmixter1 wrote:
> LGTM - though have you checked if entd-installed certs show up in certificate
> manager?

They don't yet.  They should show up after my certificate manager changes that
I'm working on in parallel.

[stevenjb, 2011-04-05 23:57:25]

Still lgtm, but since I've been working on making all DBus calls asynchronous,
added some forward thinking comments. We can address these when we change this
to use the TPM initialization in cryptohomed, but I thought I would mention them
while I was thinking them.

http://codereview.chromium.org/6667020/diff/12002/base/nss_util.cc
File base/nss_util.cc (right):

http://codereview.chromium.org/6667020/diff/12002/base/nss_util.cc#newcode237
base/nss_util.cc:237: "/usr/lib/opencryptoki/libopencryptoki.so",
nit: Maybe define this as a constant at the top of the file?

http://codereview.chromium.org/6667020/diff/12002/base/nss_util.cc#newcode250
base/nss_util.cc:250: // see that cryptohomed has initialized the PINs already.
We probably want the DBUS calls in a function, so maybe move this comment to
EnsureTPMInit?
Depending on where this gets called from, we may need to make this asyncronous,
along the lines of:
chromeos::RequestTPMInit(&NSSInitSingleton::TPMInitCallback, this)
Not sure if it makes sense to make that change now though.

http://codereview.chromium.org/6667020/diff/12002/base/nss_util.cc#newcode259
base/nss_util.cc:259: return std::string(kTPMTokenName);
This should be provided to the callback passed to RequestTPMInit. 
i.e. TPMInitCallback(const std::string& token_slot, token_name, etc...)
Not sure if we want to mock that model out now or later.

[Greg Spencer (Chromium), 2011-04-06 21:34:45]

OK, I've split "GetDefaultNSSKeySlot()" into a distinction between "public" and
"private" storage locations.  There is no difference on non-ChromeOS platforms
at the moment.  On ChromeOS, if Opencryptoki has been loaded, then private
refers to the TPM, and public refers to the software DB.

I also added a temporary command-line switch for Chrome that will allow us to
manually turn on loading of Opencryptki so that I can check this in without
breaking Google-A for ToT (and also avoid being blocked by the cryptohomed work)

If one of you could look over the changes since patch set 8, I'd appreciate it. 
I'd like to get this in today if I can.

[stevenjb, 2011-04-07 00:44:15]

lgtm

[wtc, 2011-04-07 05:56:33]

The renaming makes the code more confusing to me.  Fortunately
you wrote good comments to explain the new names.

I'm surprised that you didn't check in Patch Set 8.  I'm
going on a one-week vacation on Friday, so I won't be able
to spend more time on this.  You can go ahead and check this
in to make the M12 deadline for CrOS.  I'll try to think of
better names in M13.

http://codereview.chromium.org/6667020/diff/30017/base/nss_util.cc
File base/nss_util.cc (right):

http://codereview.chromium.org/6667020/diff/30017/base/nss_util.cc#newcode536
base/nss_util.cc:536: PK11SlotInfo* tpm_db_slot_;
This should be just tpm_slot_.  TPM is not a database.

software_db_slot_ is confusing, too.

http://codereview.chromium.org/6667020/diff/30017/base/nss_util_internal.h
File base/nss_util_internal.h (right):

http://codereview.chromium.org/6667020/diff/30017/base/nss_util_internal.h#ne...
base/nss_util_internal.h:26: PK11SlotInfo* GetPrivateNSSKeySlot();
The use of "Public" vs. "Private" here is confusing.
I'll think about better names.  Probably GetNSSDBSlot()
and GetPreferredPrivateKeySlot().

http://codereview.chromium.org/6667020/diff/30017/net/base/cert_database.h
File net/base/cert_database.h (right):

http://codereview.chromium.org/6667020/diff/30017/net/base/cert_database.h#ne...
net/base/cert_database.h:102: CryptoModule* GetPrivateModule() const;
Same here -- the use of "Public" vs. "Private" is confusing.
Perhaps GetNSSDBModule and GetPreferredPrivateKeyModule.

[Greg Spencer (Chromium), 2011-04-07 16:46:28]

On 2011/04/07 05:56:33, wtc wrote:
> I'm surprised that you didn't check in Patch Set 8.  I'm
> going on a one-week vacation on Friday, so I won't be able
> to spend more time on this.  You can go ahead and check this
> in to make the M12 deadline for CrOS.  I'll try to think of
> better names in M13.

OK, thanks.  Sorry for the surprise: the reason I couldn't check it in was
because there can be only one open session for opencryptoki at a time when doing
token initialization, and if I were to check it in as it was, then it would
break Google-A on ToT, and we wanted to avoid that.

http://codereview.chromium.org/6667020/diff/30017/base/nss_util.cc
File base/nss_util.cc (right):

http://codereview.chromium.org/6667020/diff/30017/base/nss_util.cc#newcode536
base/nss_util.cc:536: PK11SlotInfo* tpm_db_slot_;
On 2011/04/07 05:56:33, wtc wrote:
> This should be just tpm_slot_.  TPM is not a database.
> 
> software_db_slot_ is confusing, too.

removed _db from both.  I agree that they are not DBs.

http://codereview.chromium.org/6667020/diff/30017/base/nss_util_internal.h
File base/nss_util_internal.h (right):

http://codereview.chromium.org/6667020/diff/30017/base/nss_util_internal.h#ne...
base/nss_util_internal.h:26: PK11SlotInfo* GetPrivateNSSKeySlot();
On 2011/04/07 05:56:33, wtc wrote:
> The use of "Public" vs. "Private" here is confusing.
> I'll think about better names.  Probably GetNSSDBSlot()
> and GetPreferredPrivateKeySlot().

Well, but this enables us to implement our policy clearly: our policy is to
place all certs/keys with private matter into the TPM, and all
public-matter-only objects into the sofware slot (for speed).  When I read the
code that uses this, the names make it clear that the right policy is being
followed.

I realize that public and private are overloaded terms between crypto and C++,
but at the least I would like it to be obvious from the names they are both
using NSS, and the circumstances under which to use them.  "GetNSSDBSlot"
doesn't say anything about why you would choose that slot over the other one,
since the TPM is managed by NSS here too.

I also like the symmetry of GetPrivate/GetPublic, because it implies that if you
don't use one, you use the other one.