NSS: PKCS 11 password prompt.

NSS: PKCS 11 password prompt.

This was based off of davidben's WIP cl http://codereview.chromium.org/3186021/show.

BUG=42073
TEST=add password to NSS DB with "certutil -d sql:.pki/nssdb -W", try client auth, <keygen>, cert manager

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

[mattm, 2010-12-09 23:43:30]

Evan for UI bits, Wan-Teh for the rest.

Notes:  Like in davidben's original, the keygen handling can pop up a "page not
responding" if you take too long, since we don't know which tab the request came
from.  I'll have to look into the webkit side of this next to fix that (and once
we do that we can also make the dialog a constrained dialog instead of modal.)

[Evan Stade, 2010-12-10 04:17:04]

gtk bits look ok

http://codereview.chromium.org/5686002/diff/4001/chrome/browser/gtk/pk11_pass...
File chrome/browser/gtk/pk11_password_dialog.cc (right):

http://codereview.chromium.org/5686002/diff/4001/chrome/browser/gtk/pk11_pass...
chrome/browser/gtk/pk11_password_dialog.cc:74: class PK11PasswordDialog {
it's confusing that this class is defined here when there is a header file
called pk11_password_dialog.h. Can you change the name of the header file?

http://codereview.chromium.org/5686002/diff/4001/chrome/browser/gtk/pk11_pass...
chrome/browser/gtk/pk11_password_dialog.cc:177: if (response_id ==
GTK_RESPONSE_ACCEPT) {
no {}

http://codereview.chromium.org/5686002/diff/4001/chrome/browser/ui/pk11_passw...
File chrome/browser/ui/pk11_password_dialog_nss.cc (right):

http://codereview.chromium.org/5686002/diff/4001/chrome/browser/ui/pk11_passw...
chrome/browser/ui/pk11_password_dialog_nss.cc:63: //
http://mxr.mozilla.org/mozilla/source/security/nss/lib/pk11wrap/pk11auth.c#577
80 (i think people will still understand the url with a line break)

[wtc, 2010-12-15 20:54:36]

LGTM.  Impressive work!

Please address my comments below before checking this in.
Note especially the comments marked with "BUG" or "IMPORTANT".

General comments:

1. I didn't review pk11_password_dialog.cc.  Please make
sure estate reviewed it.

2. PK11Slot is NSS-specific.  PKCS #11 is an industry
standard, but among the crypto libraries Chromium uses,
NSS is the only one that uses PKCS #11.

So PK11Slot is inappropriate in the cross-platform Chromium
API.  We should call it something more generic, such as
CryptoModule, or move it to NSS-specific part of the API.

3. It is unfortunate that PK11SlotList conflicts with an
NSS type with the same name.  We should avoid this.

4. "PK11" should be avoided when appropriate.  In comments,
say "PKCS #11".  In code (identifiers), use "PKCS11".
You can do this renaming later.

5. Using "unlock" to describe authenticating/logging into
a security device is confusing.  But I agree it seems more
user-friendly.  I still prefer using "authenticate" or
"log in".  Note that there is a recent email about the
appropriate term for "log in".  Please consider that, too.
You can do this renaming later.

6. "certificate enrollment" should be replaced by
"new user certificate import".  "Certificate enrollment"
refers to the entire process of getting a new user
certificate from a CA, including not only the import
of the newly issued certificate, but also the key
generation part.
You can do this renaming later.

Detailed comments follow.

http://codereview.chromium.org/5686002/diff/4001/base/crypto/pk11_blocking_pa...
File base/crypto/pk11_blocking_password_delegate.h (right):

http://codereview.chromium.org/5686002/diff/4001/base/crypto/pk11_blocking_pa...
base/crypto/pk11_blocking_password_delegate.h:21: // Requests a password to
unlock |slot|. The interface is
Typo: |slot| => |slot_name|
      |retry| is PR_TRUE => |retry| is true

http://codereview.chromium.org/5686002/diff/4001/base/crypto/pk11_blocking_pa...
base/crypto/pk11_blocking_password_delegate.h:25: // Caller should set
|cancelled| to true if the user cancelled instead of
I think "Caller" should be changed to "An implementation of
this interface".

|cancelled| => |*cancelled|

http://codereview.chromium.org/5686002/diff/4001/base/nss_util.cc
File base/nss_util.cc (right):

http://codereview.chromium.org/5686002/diff/4001/base/nss_util.cc#newcode85
base/nss_util.cc:85: return PL_strdup(password.c_str());
BUG: use PORT_Strdup instead of PL_strdup because NSS will
free this string with PORT_Free.

Nit: we should zero the |password| string after use, so that
we don't leave unnecessary copies of the password in the
address space.

http://codereview.chromium.org/5686002/diff/4001/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/5686002/diff/4001/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:6385: Unlock Security Device
"Unlock" may not be the best word here.  How about
"Authenticate to" or "Log in to"?

Note: a "locked" security device could refer to a state
the device enters after the user enters a wrong password
too many times.

http://codereview.chromium.org/5686002/diff/4001/chrome/browser/dom_ui/option...
File chrome/browser/dom_ui/options/certificate_manager_handler.cc (right):

http://codereview.chromium.org/5686002/diff/4001/chrome/browser/dom_ui/option...
chrome/browser/dom_ui/options/certificate_manager_handler.cc:615: int
read_errno, std::string data) {
Should the second argument be
  const std::string& data
?

http://codereview.chromium.org/5686002/diff/4001/chrome/browser/dom_ui/option...
chrome/browser/dom_ui/options/certificate_manager_handler.cc:626: data_ = data;
It's too bad we have to copy the data.  If it's too much
work to avoid this copying, don't worry about it since this
is an uncommon operation.

http://codereview.chromium.org/5686002/diff/4001/chrome/browser/dom_ui/option...
chrome/browser/dom_ui/options/certificate_manager_handler.cc:631: slot_ =
slots[0];
IMPORTANT: Is the first slot (slots[0]) the right slot to use
for not only Chrome but also the unit tests?

http://codereview.chromium.org/5686002/diff/4001/chrome/browser/dom_ui/option...
File chrome/browser/dom_ui/options/certificate_manager_handler.h (right):

http://codereview.chromium.org/5686002/diff/4001/chrome/browser/dom_ui/option...
chrome/browser/dom_ui/options/certificate_manager_handler.h:84: //  import with
previously entered password
Nit: if the slot is already unlocked/authenticated, it seems
that we can remove "with previously entered password".

http://codereview.chromium.org/5686002/diff/4001/chrome/browser/dom_ui/option...
chrome/browser/dom_ui/options/certificate_manager_handler.h:146: // wait for
file to be read, etc.
Could you describe what data_ is?  The data contained in
the file?

http://codereview.chromium.org/5686002/diff/4001/chrome/browser/gtk/ssl_clien...
File chrome/browser/gtk/ssl_client_certificate_selector.cc (right):

http://codereview.chromium.org/5686002/diff/4001/chrome/browser/gtk/ssl_clien...
chrome/browser/gtk/ssl_client_certificate_selector.cc:311: net::X509Certificate*
cert = GetSelectedCert();
Can |cert| be passed to this method as a parameter, so
that this method doesn't need to call GetSelectedCert()
again?

http://codereview.chromium.org/5686002/diff/4001/chrome/browser/gtk/ssl_clien...
chrome/browser/gtk/ssl_client_certificate_selector.cc:349:
cert_request_info_->host_and_port,  // TODO(mattm): strip port part?
The port part is important.

http://codereview.chromium.org/5686002/diff/4001/chrome/browser/ui/pk11_passw...
File chrome/browser/ui/pk11_password_dialog.h (right):

http://codereview.chromium.org/5686002/diff/4001/chrome/browser/ui/pk11_passw...
chrome/browser/ui/pk11_password_dialog.h:42: const std::string& host,
You can avoid the issue of whether this string can contain
a port by naming this parameter |server| or |server_name|.

http://codereview.chromium.org/5686002/diff/4001/chrome/browser/ui/pk11_passw...
File chrome/browser/ui/pk11_password_dialog_nss.cc (right):

http://codereview.chromium.org/5686002/diff/4001/chrome/browser/ui/pk11_passw...
chrome/browser/ui/pk11_password_dialog_nss.cc:16: // Basically an asynchronous
implementation of NSS's PK11_DoPassword.
Please note that we're missing a side effect of PK11_DoPassword: the
nssTrustDomain_UpdateCachedTokenCerts
call for an "unfriendly" slot:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/pk11wrap...

This may or may not matter.

http://codereview.chromium.org/5686002/diff/4001/chrome/browser/ui/pk11_passw...
chrome/browser/ui/pk11_password_dialog_nss.cc:101:
!PK11_IsLoggedIn(slot->os_slot_handle(), NULL)) {
Please add a comment to note that we can pass NULL as the
second (wincx) argument to PK11_IsLoggedIn because we don't
call PK11_SetIsLoggedInFunc.  That argument is only used
by PK11_Global.isLoggedIn.

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

http://codereview.chromium.org/5686002/diff/4001/net/base/cert_database.h#new...
net/base/cert_database.h:79: void ListTokensForPKCS12(PK11SlotList* slots)
const;
Nit: ListTokensForPKCS12 => ListTokensForPKCS12Import ?

http://codereview.chromium.org/5686002/diff/4001/net/base/cert_database.h#new...
net/base/cert_database.h:81: // Import certificates and private keys from PKCS
#12 blob.
Add "into the slot".

http://codereview.chromium.org/5686002/diff/4001/net/base/cert_database_nss.cc
File net/base/cert_database_nss.cc (right):

http://codereview.chromium.org/5686002/diff/4001/net/base/cert_database_nss.c...
net/base/cert_database_nss.cc:114: slot_list = PK11_GetAllTokens(CKM_RSA_PKCS,
BUG: I think you should pass CKM_INVALID_MECHANISM instead
of CKM_RSA_PKCS as the first argument.  See
http://mxr.mozilla.org/security/ident?i=PK11_GetAllTokens

Add a null pointer check for slot_list.

http://codereview.chromium.org/5686002/diff/4001/net/base/cert_database_nss.c...
net/base/cert_database_nss.cc:130: net::PK11Slot* slot, const std::string& data,
const string16& password) {
Nit: in this form, we probably should list one parameter per
line.

http://codereview.chromium.org/5686002/diff/4001/net/base/cert_database_nss_u...
File net/base/cert_database_nss_unittest.cc (right):

http://codereview.chromium.org/5686002/diff/4001/net/base/cert_database_nss_u...
net/base/cert_database_nss_unittest.cc:135: scoped_refptr<PK11Slot>
slot_wrapper_;
Nit: we only need one of slot_ and slot_wrapper_.
I suggest defining slot_ as
  scoped_refptr<PK11Slot> slot_;
and replacing the original slot_.get() with
  slot_->os_slot_handle()

At least consider adding a TODO comment for this.

http://codereview.chromium.org/5686002/diff/4001/net/base/keygen_handler.cc
File net/base/keygen_handler.cc (right):

http://codereview.chromium.org/5686002/diff/4001/net/base/keygen_handler.cc#n...
net/base/keygen_handler.cc:9: #endif  // defined(USE_NSS)
Nit: I usully omit this kind of #endif comment for short
ifdef blocks to reduce visual noise.  I'll leave it to your
personal taste.

http://codereview.chromium.org/5686002/diff/4001/net/base/keygen_handler.cc#n...
net/base/keygen_handler.cc:14: // PK11PasswordCallback can be forward-declared
on platforms which use NSS.
Typo: PK11PasswordCallback => PK11BlockingPasswordDelegate?

http://codereview.chromium.org/5686002/diff/4001/net/base/keygen_handler.h
File net/base/keygen_handler.h (right):

http://codereview.chromium.org/5686002/diff/4001/net/base/keygen_handler.h#ne...
net/base/keygen_handler.h:16: class PK11BlockingPasswordDelegate;
Nit: put this forward declaration inside #if defined(USE_NSS).

http://codereview.chromium.org/5686002/diff/4001/net/base/keygen_handler.h#ne...
net/base/keygen_handler.h:44: // On NSS, the token may be unauthenticated. We
pass the blocking delegate for
Question: does this mean there is also a non-blocking
password delegate type?

Nit: this comment should make it clearer that it's OK to
block because GenKeyAndSignChallenge runs on a worker thread.
The comment says "for simplicity", which may confuse people
into thinking we made a trade-off for simplicity.

http://codereview.chromium.org/5686002/diff/4001/net/base/keygen_handler.h#ne...
net/base/keygen_handler.h:56: // The callback for requesting a password to the
PKCS#11 store.
Nit: store => token

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

http://codereview.chromium.org/5686002/diff/4001/net/base/keygen_handler_nss....
net/base/keygen_handler_nss.cc:23: // TODO(mattm): allow choosing which slot to
store the generated key?
Nit: store the generated key => generate and store the key

http://codereview.chromium.org/5686002/diff/4001/net/base/keygen_handler_nss....
net/base/keygen_handler_nss.cc:26: LOG(ERROR) << "Couldn't get Internal key
slot!";
Nit: lowercase "internal".

http://codereview.chromium.org/5686002/diff/4001/net/base/keygen_handler_nss....
net/base/keygen_handler_nss.cc:33: LOG(ERROR) << "Couldn't authenticate to PK11
token!";
Nit: this probably should also say "internal key slot" for
consistency with the error message above, because both refer
to the same slot/token.

At least change "PK11" to "PKCS #11" or just remove "PK11".

http://codereview.chromium.org/5686002/diff/4001/net/base/pk11_slot.h
File net/base/pk11_slot.h (right):

http://codereview.chromium.org/5686002/diff/4001/net/base/pk11_slot.h#newcode24
net/base/pk11_slot.h:24: class PK11Slot : public
base::RefCountedThreadSafe<PK11Slot> {
PK11Slot may not need to be ref-counted.

http://codereview.chromium.org/5686002/diff/4001/net/net.gyp
File net/net.gyp (right):

http://codereview.chromium.org/5686002/diff/4001/net/net.gyp#newcode100
net/net.gyp:100: 'base/keygen_handler.cc',
Nit: list .cc before .h.

http://codereview.chromium.org/5686002/diff/4001/net/third_party/mozilla_secu...
File net/third_party/mozilla_security_manager/nsKeygenHandler.h (right):

http://codereview.chromium.org/5686002/diff/4001/net/third_party/mozilla_secu...
net/third_party/mozilla_security_manager/nsKeygenHandler.h:59: //   slot: a slot
to store the key in, should be authenticated
Nit: store the key => generate the key

because we may not store the key.

http://codereview.chromium.org/5686002/diff/4001/net/third_party/mozilla_secu...
File net/third_party/mozilla_security_manager/nsPKCS12Blob.h (right):

http://codereview.chromium.org/5686002/diff/4001/net/third_party/mozilla_secu...
net/third_party/mozilla_security_manager/nsPKCS12Blob.h:59: // Import
certificate from PKCS#12 blob.
Nit: add "into the slot".

[mattm, 2011-01-12 01:22:07]

> 1. I didn't review pk11_password_dialog.cc.  Please make
> sure estate reviewed it.

He did

> 2. PK11Slot is NSS-specific.  PKCS #11 is an industry
> standard, but among the crypto libraries Chromium uses,
> NSS is the only one that uses PKCS #11.
>
> So PK11Slot is inappropriate in the cross-platform Chromium
> API.  We should call it something more generic, such as
> CryptoModule, or move it to NSS-specific part of the API.

done

> 3. It is unfortunate that PK11SlotList conflicts with an
> NSS type with the same name.  We should avoid this.

done

> 4. "PK11" should be avoided when appropriate.  In comments,
> say "PKCS #11".  In code (identifiers), use "PKCS11".
> You can do this renaming later.

Will do in followup.

> 5. Using "unlock" to describe authenticating/logging into
> a security device is confusing.  But I agree it seems more
> user-friendly.  I still prefer using "authenticate" or
> "log in".  Note that there is a recent email about the
> appropriate term for "log in".  Please consider that, too.
> You can do this renaming later.

done.

> 6. "certificate enrollment" should be replaced by
> "new user certificate import".  "Certificate enrollment"
> refers to the entire process of getting a new user
> certificate from a CA, including not only the import
> of the newly issued certificate, but also the key
> generation part.
> You can do this renaming later.

Will do in followup.

http://codereview.chromium.org/5686002/diff/4001/base/crypto/pk11_blocking_pa...
File base/crypto/pk11_blocking_password_delegate.h (right):

http://codereview.chromium.org/5686002/diff/4001/base/crypto/pk11_blocking_pa...
base/crypto/pk11_blocking_password_delegate.h:21: // Requests a password to
unlock |slot|. The interface is
On 2010/12/15 20:54:36, wtc wrote:
> Typo: |slot| => |slot_name|
>       |retry| is PR_TRUE => |retry| is true

Done.

http://codereview.chromium.org/5686002/diff/4001/base/crypto/pk11_blocking_pa...
base/crypto/pk11_blocking_password_delegate.h:25: // Caller should set
|cancelled| to true if the user cancelled instead of
On 2010/12/15 20:54:36, wtc wrote:
> I think "Caller" should be changed to "An implementation of
> this interface".
> 
> |cancelled| => |*cancelled|

Done.

http://codereview.chromium.org/5686002/diff/4001/base/nss_util.cc
File base/nss_util.cc (right):

http://codereview.chromium.org/5686002/diff/4001/base/nss_util.cc#newcode85
base/nss_util.cc:85: return PL_strdup(password.c_str());
On 2010/12/15 20:54:36, wtc wrote:
> BUG: use PORT_Strdup instead of PL_strdup because NSS will
> free this string with PORT_Free.
> 
> Nit: we should zero the |password| string after use, so that
> we don't leave unnecessary copies of the password in the
> address space.

Done.

http://codereview.chromium.org/5686002/diff/4001/chrome/app/generated_resourc...
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/5686002/diff/4001/chrome/app/generated_resourc...
chrome/app/generated_resources.grd:6385: Unlock Security Device
On 2010/12/15 20:54:36, wtc wrote:
> "Unlock" may not be the best word here.  How about
> "Authenticate to" or "Log in to"?
> 
> Note: a "locked" security device could refer to a state
> the device enters after the user enters a wrong password
> too many times.

Done.

http://codereview.chromium.org/5686002/diff/4001/chrome/browser/dom_ui/option...
File chrome/browser/dom_ui/options/certificate_manager_handler.cc (right):

http://codereview.chromium.org/5686002/diff/4001/chrome/browser/dom_ui/option...
chrome/browser/dom_ui/options/certificate_manager_handler.cc:615: int
read_errno, std::string data) {
On 2010/12/15 20:54:36, wtc wrote:
> Should the second argument be
>   const std::string& data
> ?

Unfortunately it can't, since it has to match the type returned by the
CancelableRequest, and that can't be a reference since it is returning a
temporary.  This should probably be refactored to have the caller pass in the
buffer to write to, but like you mention below, it's a pretty rare operation on
relatively small files.

http://codereview.chromium.org/5686002/diff/4001/chrome/browser/dom_ui/option...
chrome/browser/dom_ui/options/certificate_manager_handler.cc:631: slot_ =
slots[0];
On 2010/12/15 20:54:36, wtc wrote:
> IMPORTANT: Is the first slot (slots[0]) the right slot to use
> for not only Chrome but also the unit tests?

It should be fine for chrome, even chromeos, since it requests only RW slots, so
it won't be confused by the ro slot that chromeos has.

For unittests it could be a problem, bu unfortunately I don't currently have
unittests for the domui parts.

http://codereview.chromium.org/5686002/diff/4001/chrome/browser/dom_ui/option...
File chrome/browser/dom_ui/options/certificate_manager_handler.h (right):

http://codereview.chromium.org/5686002/diff/4001/chrome/browser/dom_ui/option...
chrome/browser/dom_ui/options/certificate_manager_handler.h:84: //  import with
previously entered password
On 2010/12/15 20:54:36, wtc wrote:
> Nit: if the slot is already unlocked/authenticated, it seems
> that we can remove "with previously entered password".

This is referring to the password for decrypting the pkcs#12 file.

http://codereview.chromium.org/5686002/diff/4001/chrome/browser/dom_ui/option...
chrome/browser/dom_ui/options/certificate_manager_handler.h:146: // wait for
file to be read, etc.
On 2010/12/15 20:54:36, wtc wrote:
> Could you describe what data_ is?  The data contained in
> the file?

renamed to file_data_

http://codereview.chromium.org/5686002/diff/4001/chrome/browser/gtk/pk11_pass...
File chrome/browser/gtk/pk11_password_dialog.cc (right):

http://codereview.chromium.org/5686002/diff/4001/chrome/browser/gtk/pk11_pass...
chrome/browser/gtk/pk11_password_dialog.cc:74: class PK11PasswordDialog {
On 2010/12/10 04:17:04, Evan Stade wrote:
> it's confusing that this class is defined here when there is a header file
> called pk11_password_dialog.h. Can you change the name of the header file?

I thought this was a somewhat common pattern to have "ShowBlah" in some
platform-independent header file and then the actual implementation in the
platform-dependent .cc file(s).

http://codereview.chromium.org/5686002/diff/4001/chrome/browser/gtk/pk11_pass...
chrome/browser/gtk/pk11_password_dialog.cc:177: if (response_id ==
GTK_RESPONSE_ACCEPT) {
On 2010/12/10 04:17:04, Evan Stade wrote:
> no {}

Done.

http://codereview.chromium.org/5686002/diff/4001/chrome/browser/gtk/ssl_clien...
File chrome/browser/gtk/ssl_client_certificate_selector.cc (right):

http://codereview.chromium.org/5686002/diff/4001/chrome/browser/gtk/ssl_clien...
chrome/browser/gtk/ssl_client_certificate_selector.cc:311: net::X509Certificate*
cert = GetSelectedCert();
On 2010/12/15 20:54:36, wtc wrote:
> Can |cert| be passed to this method as a parameter, so
> that this method doesn't need to call GetSelectedCert()
> again?

Could be done, but UnlockSlotIfNecessary and UnlockCertSlotIfNecessary both take
the same callback type, so it'd need to be templated or just use void*.  I'll
just add a TODO about it for now.

http://codereview.chromium.org/5686002/diff/4001/chrome/browser/gtk/ssl_clien...
chrome/browser/gtk/ssl_client_certificate_selector.cc:349:
cert_request_info_->host_and_port,  // TODO(mattm): strip port part?
On 2010/12/15 20:54:36, wtc wrote:
> The port part is important.

Done.

http://codereview.chromium.org/5686002/diff/4001/chrome/browser/ui/pk11_passw...
File chrome/browser/ui/pk11_password_dialog.h (right):

http://codereview.chromium.org/5686002/diff/4001/chrome/browser/ui/pk11_passw...
chrome/browser/ui/pk11_password_dialog.h:42: const std::string& host,
On 2010/12/15 20:54:36, wtc wrote:
> You can avoid the issue of whether this string can contain
> a port by naming this parameter |server| or |server_name|.

Done.

http://codereview.chromium.org/5686002/diff/4001/chrome/browser/ui/pk11_passw...
File chrome/browser/ui/pk11_password_dialog_nss.cc (right):

http://codereview.chromium.org/5686002/diff/4001/chrome/browser/ui/pk11_passw...
chrome/browser/ui/pk11_password_dialog_nss.cc:16: // Basically an asynchronous
implementation of NSS's PK11_DoPassword.
On 2010/12/15 20:54:36, wtc wrote:
> Please note that we're missing a side effect of PK11_DoPassword: the
> nssTrustDomain_UpdateCachedTokenCerts
> call for an "unfriendly" slot:
>
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/pk11wrap...
> 
> This may or may not matter.

Added a note.

http://codereview.chromium.org/5686002/diff/4001/chrome/browser/ui/pk11_passw...
chrome/browser/ui/pk11_password_dialog_nss.cc:63: //
http://mxr.mozilla.org/mozilla/source/security/nss/lib/pk11wrap/pk11auth.c#577
On 2010/12/10 04:17:04, Evan Stade wrote:
> 80 (i think people will still understand the url with a line break)

The style guide has an exception for long urls

http://codereview.chromium.org/5686002/diff/4001/chrome/browser/ui/pk11_passw...
chrome/browser/ui/pk11_password_dialog_nss.cc:101:
!PK11_IsLoggedIn(slot->os_slot_handle(), NULL)) {
On 2010/12/15 20:54:36, wtc wrote:
> Please add a comment to note that we can pass NULL as the
> second (wincx) argument to PK11_IsLoggedIn because we don't
> call PK11_SetIsLoggedInFunc.  That argument is only used
> by PK11_Global.isLoggedIn.

Done.

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

http://codereview.chromium.org/5686002/diff/4001/net/base/cert_database.h#new...
net/base/cert_database.h:79: void ListTokensForPKCS12(PK11SlotList* slots)
const;
On 2010/12/15 20:54:36, wtc wrote:
> Nit: ListTokensForPKCS12 => ListTokensForPKCS12Import ?

Done.

http://codereview.chromium.org/5686002/diff/4001/net/base/cert_database.h#new...
net/base/cert_database.h:81: // Import certificates and private keys from PKCS
#12 blob.
On 2010/12/15 20:54:36, wtc wrote:
> Add "into the slot".

Done.

http://codereview.chromium.org/5686002/diff/4001/net/base/cert_database_nss.cc
File net/base/cert_database_nss.cc (right):

http://codereview.chromium.org/5686002/diff/4001/net/base/cert_database_nss.c...
net/base/cert_database_nss.cc:114: slot_list = PK11_GetAllTokens(CKM_RSA_PKCS,
On 2010/12/15 20:54:36, wtc wrote:
> BUG: I think you should pass CKM_INVALID_MECHANISM instead
> of CKM_RSA_PKCS as the first argument.  See
> http://mxr.mozilla.org/security/ident?i=PK11_GetAllTokens

Done.

> Add a null pointer check for slot_list.

Done.

http://codereview.chromium.org/5686002/diff/4001/net/base/cert_database_nss.c...
net/base/cert_database_nss.cc:130: net::PK11Slot* slot, const std::string& data,
const string16& password) {
On 2010/12/15 20:54:36, wtc wrote:
> Nit: in this form, we probably should list one parameter per
> line.

Done.

http://codereview.chromium.org/5686002/diff/4001/net/base/cert_database_nss_u...
File net/base/cert_database_nss_unittest.cc (right):

http://codereview.chromium.org/5686002/diff/4001/net/base/cert_database_nss_u...
net/base/cert_database_nss_unittest.cc:135: scoped_refptr<PK11Slot>
slot_wrapper_;
On 2010/12/15 20:54:36, wtc wrote:
> Nit: we only need one of slot_ and slot_wrapper_.
> I suggest defining slot_ as
>   scoped_refptr<PK11Slot> slot_;
> and replacing the original slot_.get() with
>   slot_->os_slot_handle()
> 
> At least consider adding a TODO comment for this.

Done.

http://codereview.chromium.org/5686002/diff/4001/net/base/keygen_handler.cc
File net/base/keygen_handler.cc (right):

http://codereview.chromium.org/5686002/diff/4001/net/base/keygen_handler.cc#n...
net/base/keygen_handler.cc:9: #endif  // defined(USE_NSS)
On 2010/12/15 20:54:36, wtc wrote:
> Nit: I usully omit this kind of #endif comment for short
> ifdef blocks to reduce visual noise.  I'll leave it to your
> personal taste.

Done.

http://codereview.chromium.org/5686002/diff/4001/net/base/keygen_handler.cc#n...
net/base/keygen_handler.cc:14: // PK11PasswordCallback can be forward-declared
on platforms which use NSS.
On 2010/12/15 20:54:36, wtc wrote:
> Typo: PK11PasswordCallback => PK11BlockingPasswordDelegate?

Done.

http://codereview.chromium.org/5686002/diff/4001/net/base/keygen_handler.h
File net/base/keygen_handler.h (right):

http://codereview.chromium.org/5686002/diff/4001/net/base/keygen_handler.h#ne...
net/base/keygen_handler.h:16: class PK11BlockingPasswordDelegate;
On 2010/12/15 20:54:36, wtc wrote:
> Nit: put this forward declaration inside #if defined(USE_NSS).

Done.

http://codereview.chromium.org/5686002/diff/4001/net/base/keygen_handler.h#ne...
net/base/keygen_handler.h:44: // On NSS, the token may be unauthenticated. We
pass the blocking delegate for
On 2010/12/15 20:54:36, wtc wrote:
> Question: does this mean there is also a non-blocking
> password delegate type?

no, just the alternate strategy of using the non-blocking UnlockSlotIfNecessary
instead of a password delegate.

> Nit: this comment should make it clearer that it's OK to
> block because GenKeyAndSignChallenge runs on a worker thread.
> The comment says "for simplicity", which may confuse people
> into thinking we made a trade-off for simplicity.

done.

http://codereview.chromium.org/5686002/diff/4001/net/base/keygen_handler.h#ne...
net/base/keygen_handler.h:56: // The callback for requesting a password to the
PKCS#11 store.
On 2010/12/15 20:54:36, wtc wrote:
> Nit: store => token

Done.

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

http://codereview.chromium.org/5686002/diff/4001/net/base/keygen_handler_nss....
net/base/keygen_handler_nss.cc:23: // TODO(mattm): allow choosing which slot to
store the generated key?
On 2010/12/15 20:54:36, wtc wrote:
> Nit: store the generated key => generate and store the key

Done.

http://codereview.chromium.org/5686002/diff/4001/net/base/keygen_handler_nss....
net/base/keygen_handler_nss.cc:26: LOG(ERROR) << "Couldn't get Internal key
slot!";
On 2010/12/15 20:54:36, wtc wrote:
> Nit: lowercase "internal".

Done.

http://codereview.chromium.org/5686002/diff/4001/net/base/keygen_handler_nss....
net/base/keygen_handler_nss.cc:33: LOG(ERROR) << "Couldn't authenticate to PK11
token!";
On 2010/12/15 20:54:36, wtc wrote:
> Nit: this probably should also say "internal key slot" for
> consistency with the error message above, because both refer
> to the same slot/token.
> 
> At least change "PK11" to "PKCS #11" or just remove "PK11".

Done.

http://codereview.chromium.org/5686002/diff/4001/net/net.gyp
File net/net.gyp (right):

http://codereview.chromium.org/5686002/diff/4001/net/net.gyp#newcode100
net/net.gyp:100: 'base/keygen_handler.cc',
On 2010/12/15 20:54:36, wtc wrote:
> Nit: list .cc before .h.

Done.

http://codereview.chromium.org/5686002/diff/4001/net/third_party/mozilla_secu...
File net/third_party/mozilla_security_manager/nsKeygenHandler.h (right):

http://codereview.chromium.org/5686002/diff/4001/net/third_party/mozilla_secu...
net/third_party/mozilla_security_manager/nsKeygenHandler.h:59: //   slot: a slot
to store the key in, should be authenticated
On 2010/12/15 20:54:36, wtc wrote:
> Nit: store the key => generate the key
> 
> because we may not store the key.

Done.

http://codereview.chromium.org/5686002/diff/4001/net/third_party/mozilla_secu...
File net/third_party/mozilla_security_manager/nsPKCS12Blob.h (right):

http://codereview.chromium.org/5686002/diff/4001/net/third_party/mozilla_secu...
net/third_party/mozilla_security_manager/nsPKCS12Blob.h:59: // Import
certificate from PKCS#12 blob.
On 2010/12/15 20:54:36, wtc wrote:
> Nit: add "into the slot".

Done.