Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(227)

Issue 5686002: NSS: PKCS 11 password prompt. (Closed)

Created:
10 years ago by mattm
Modified:
9 years, 7 months ago
Reviewers:
wtc, Evan Stade
CC:
chromium-reviews, Paweł Hajdan Jr., cbentzel+watch_chromium.org, ben+cc_chromium.org, darin-cc_chromium.org, brettw-cc_chromium.org, davidben, bulach
Visibility:
Public.

Description

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

Patch Set 1 : #

Total comments: 66

Patch Set 2 : rebase #

Patch Set 3 : addressing review comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+817 lines, -88 lines) Patch
M base/base.gypi View 1 1 chunk +1 line, -0 lines 0 comments Download
A base/crypto/pk11_blocking_password_delegate.h View 1 2 1 chunk +34 lines, -0 lines 0 comments Download
M base/nss_util.cc View 1 2 3 chunks +23 lines, -0 lines 0 comments Download
M chrome/app/generated_resources.grd View 1 2 1 chunk +26 lines, -0 lines 0 comments Download
M chrome/browser/certificate_manager_model.h View 1 2 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/certificate_manager_model.cc View 1 2 1 chunk +3 lines, -2 lines 0 comments Download
M chrome/browser/dom_ui/options/certificate_manager_handler.h View 1 2 4 chunks +14 lines, -6 lines 0 comments Download
M chrome/browser/dom_ui/options/certificate_manager_handler.cc View 1 2 4 chunks +36 lines, -1 line 0 comments Download
A chrome/browser/gtk/pk11_password_dialog.cc View 1 2 1 chunk +218 lines, -0 lines 0 comments Download
M chrome/browser/gtk/ssl_client_certificate_selector.cc View 1 2 4 chunks +19 lines, -4 lines 0 comments Download
M chrome/browser/renderer_host/render_message_filter.cc View 1 2 2 chunks +10 lines, -0 lines 0 comments Download
A chrome/browser/ui/pk11_password_dialog.h View 1 2 1 chunk +69 lines, -0 lines 0 comments Download
A chrome/browser/ui/pk11_password_dialog_nss.cc View 1 2 1 chunk +123 lines, -0 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 6 chunks +8 lines, -0 lines 0 comments Download
M net/base/cert_database.h View 1 2 2 chunks +9 lines, -2 lines 0 comments Download
M net/base/cert_database_nss.cc View 1 2 2 chunks +18 lines, -2 lines 0 comments Download
M net/base/cert_database_nss_unittest.cc View 1 2 15 chunks +27 lines, -22 lines 0 comments Download
A net/base/crypto_module.h View 1 2 1 chunk +51 lines, -0 lines 0 comments Download
A net/base/crypto_module_nss.cc View 1 2 1 chunk +28 lines, -0 lines 0 comments Download
M net/base/keygen_handler.h View 1 2 3 chunks +24 lines, -12 lines 0 comments Download
A net/base/keygen_handler.cc View 1 2 1 chunk +29 lines, -0 lines 0 comments Download
M net/base/keygen_handler_nss.cc View 1 2 2 chunks +28 lines, -1 line 0 comments Download
M net/net.gyp View 1 2 4 chunks +5 lines, -0 lines 0 comments Download
M net/third_party/mozilla_security_manager/nsKeygenHandler.h View 1 2 2 chunks +3 lines, -0 lines 0 comments Download
M net/third_party/mozilla_security_manager/nsKeygenHandler.cpp View 5 chunks +1 line, -25 lines 0 comments Download
M net/third_party/mozilla_security_manager/nsPKCS12Blob.h View 1 2 2 chunks +4 lines, -2 lines 0 comments Download
M net/third_party/mozilla_security_manager/nsPKCS12Blob.cpp View 1 2 chunks +4 lines, -8 lines 0 comments Download

Messages

Total messages: 4 (0 generated)
mattm
Evan for UI bits, Wan-Teh for the rest. Notes: Like in davidben's original, the keygen ...
10 years ago (2010-12-09 23:43:30 UTC) #1
Evan Stade
gtk bits look ok http://codereview.chromium.org/5686002/diff/4001/chrome/browser/gtk/pk11_password_dialog.cc File chrome/browser/gtk/pk11_password_dialog.cc (right): http://codereview.chromium.org/5686002/diff/4001/chrome/browser/gtk/pk11_password_dialog.cc#newcode74 chrome/browser/gtk/pk11_password_dialog.cc:74: class PK11PasswordDialog { it's confusing ...
10 years ago (2010-12-10 04:17:04 UTC) #2
wtc
LGTM. Impressive work! Please address my comments below before checking this in. Note especially the ...
10 years ago (2010-12-15 20:54:36 UTC) #3
mattm
9 years, 11 months ago (2011-01-12 01:22:07 UTC) #4
> 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.

Powered by Google App Engine
This is Rietveld 408576698