Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8566056:
This applies GUIDs to certificate and key nicknames when (Closed)
|
Created:
9 years, 1 month ago by Greg Spencer (Chromium) Modified:
9 years ago CC:
chromium-reviews, cbentzel+watch_chromium.org, nkostylev+watch_chromium.org, darin-cc_chromium.org, Paweł Hajdan Jr., stevenjb+watch_chromium.org, davemoore+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionThis applies GUIDs to certificate and key nicknames when
imported via ONC.
It also centralizes the label creation for nicknames and certificates
so that we can better control their values.
BUG=chromium-os:19403
TEST=Ran new unit tests, imported certs into certificate store via ONC.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113993
Patch Set 1 #Patch Set 2 : minor fixes #
Total comments: 6
Patch Set 3 : Review edits #
Total comments: 35
Patch Set 4 : More review changes #
Total comments: 57
Patch Set 5 : Reorganizing to not need CKA_LABEL #Patch Set 6 : Fix memory leak #
Total comments: 9
Patch Set 7 : Final review changes #Messages
Total messages: 19 (0 generated)
There's still one issue here, but maybe you have an idea of how to get around
it.
The nickname string on the certificate structure is set by my code, but it
doesn't get saved to the PKCS#11 database underneath. I've set the nickname on
the public and private keys if they exist, but public keys are only created for
some kinds of certs (e.g. I don't think they're created for CA certs), and only
client certs have private keys anyhow. So the nicknames for the certs end up in
the PKCS#11 database as "token name:label" instead of just "label" (I'm using a
GUID as the label when I create them, so it appears as something like
"OncNetworkParserTest token:{f998f760-272b-6939-4c2beffe428697aa}")
To fix this, I think I need to implement the unimplemented PK11_TypeCert case
here: http://goo.gl/LL2aT
It's a three-line implementation.
But implementing that would require Chrome to build nss on Linux (we already do
on Windows/Mac), but since Linux distros have nss already, it could cause
compatibility problems. There are also other slightly more involved fixes, but
they all involve modifying NSS source code. I thought about the ultimate hack
of defining my own local version of PK11_WriteRawAttributes using NSS internal
data structures, but discarded that as pure evil and prone to rot.
Having the name be "token:label" isn't a showstopper for us: since we're using
GUIDs, we can just search for the GUID in the nickname, and since it's unique we
don't have to worry that we got the wrong one coincidentally (or that the GUID
includes the token name), so I'm tempted to not worry about it, but it does make
renaming things or looking up exact matches or non GUID nicknames problematic.
Just a random drive-by: I'm sure you'll want to get wtc to review the net/ changes. For my own $.02, it feels weird/wrong to bring in the msm dependency to x509_certificate. Certificate labels (and types) are more aligned with a particular database store, not an individual certificate, IMO. It also creates more API divergence between the platforms, which I've been subtlety trying to reduce when possible. My own goal would be that X509Certificate has a single, consistent interface across platforms, and the platform-specific bits can be in a separate space (such as x509_util_[nss/mac/win/openssl]), since such implementations are inherently non-portable. Still though, in this particular case, it feels more suited for the CertDatabase code than X509Certificate - especially since 'labels' are inherently tied to keychain/CAPI store on OS X & Win. http://codereview.chromium.org/8566056/diff/3001/net/base/x509_certificate_ns... File net/base/x509_certificate_nss.cc (right): http://codereview.chromium.org/8566056/diff/3001/net/base/x509_certificate_ns... net/base/x509_certificate_nss.cc:691: // to the given label. Rather than having to extract and encode the DER from the token, would it be better to use the PK11_ReadRawAttribute() and compare with the CKA_ID? Since you should have the same CKA_ID between the CKO_PUBLIC_KEY, CKO_PRIVATE_KEY, and CKO_CERTIFICATE SECItem key_id; SECStatus rv = PK11_ReadRawAttribute(cert_handle->slot, cert_handle->pkcs11ID, CKA_ID, &key_id); // Alternatively, // SECItem* key_id = PK11_GetLowLevelKeyIDForCert( // cert_handle->slot, cert, NULL); SECKEYPublicKeyList pubkey_list = NULL; if (rv == SECSuccess && key_id.len) pubkey_list = ... if (pubkey_list) { SECItem node_id; for(... node = PUBKEY_LIST_HEAD(pubkey_list); ...) { SECStatus rv = PK11_ReadRawAttribute(PK11_TypePubKey, node, CKA_ID, &node_id); if (rv != SECSuccess) continue; // Really, an error if (SECITEM_CompareItem(&key_id, &node_id) == 0) PK11_SetPublicKeyNickname(node->key, label.c_str()); SECITEM_FreeItem(&node_id, false); } SECKEY_DestroyPublicKeyList(pubkey_list) } It's a shame too, because NSS has pk11_FindObjectByTemplate exposed via a very convenient (but private) API PK11_MatchItem. With PK11_MatchItem, it would simply be CK_OBJECT_HANDLE key_handle = PK11_MatchItem(cert_handle->slot, cert_handle->pkcs11ID, CKO_PUBLIC_KEY); if (key_handle != CK_INVALID_HANDLE) PK11_SetPublicKeyNickname(key_handle, label.c_str()); http://codereview.chromium.org/8566056/diff/3001/net/base/x509_certificate_ns... net/base/x509_certificate_nss.cc:1082: int len = strlen(nickname); Since |nickname| is optional (see line 1052), I believe this means that you're potentially doing strlen(NULL). Perhaps a short-circuit before here for when there is no nickname? http://codereview.chromium.org/8566056/diff/3001/net/third_party/mozilla_secu... File net/third_party/mozilla_security_manager/nsPKCS12Blob.cpp (right): http://codereview.chromium.org/8566056/diff/3001/net/third_party/mozilla_secu... net/third_party/mozilla_security_manager/nsPKCS12Blob.cpp:250: break; BUG: You will end up leaking |cert| here, as the break jumps to line 257, bypassing the matching free call on line 254 for the allocation on line 220.
On 2011/11/22 00:59:03, Ryan Sleevi wrote: > I'm sure you'll want to get wtc to review the net/ changes. Yes, that's my plan, now that he's back in the office. > For my own $.02, it feels weird/wrong to bring in the msm dependency > to x509_certificate. Certificate labels (and types) are more aligned > with a particular database store, not an individual certificate, > IMO. OK, so what you'd want to see here is something like moving GetCertificateType (the only thing that depends on msm) into the cert_database.h API? Or would you rather it moved into x509_util instead? > It also creates more API divergence between the platforms, which > I've been subtlety trying to reduce when possible. My own goal would > be that X509Certificate has a single, consistent interface across > platforms, and the platform-specific bits can be in a separate space > (such as x509_util_[nss/mac/win/openssl]), since such > implementations are inherently non-portable. Still though, in this > particular case, it feels more suited for the CertDatabase code than > X509Certificate - especially since 'labels' are inherently tied to > keychain/CAPI store on OS X & Win. I agree with you on the divergence, but it occured to me that doing that limits the x509_certificate API to the least common denominator, which is why I chose the #ifdef method. To be clear, you think it would be better if I moved all of the label related code into the x509_util namespace and made standalone functions that take certificates as arguments? (I'm not opposed: just clarifying) http://codereview.chromium.org/8566056/diff/3001/net/base/x509_certificate_ns... File net/base/x509_certificate_nss.cc (right): http://codereview.chromium.org/8566056/diff/3001/net/base/x509_certificate_ns... net/base/x509_certificate_nss.cc:691: // to the given label. On 2011/11/22 00:59:03, Ryan Sleevi wrote: > Rather than having to extract and encode the DER from the token, would it be > better to use the PK11_ReadRawAttribute() and compare with the CKA_ID? Since you > should have the same CKA_ID between the CKO_PUBLIC_KEY, CKO_PRIVATE_KEY, and > CKO_CERTIFICATE That's a great idea, except that there's no way to get the CKA_ID from a cert. PK11_ReadRawAttribute isn't implemented for PK11_TypeCert, and PK11_ReadAttribute is a private interface (in your sample, the first PK11_ReadRawAttribute function that takes a slot and pkcs11ID doesn't exist). http://codereview.chromium.org/8566056/diff/3001/net/base/x509_certificate_ns... net/base/x509_certificate_nss.cc:1082: int len = strlen(nickname); On 2011/11/22 00:59:03, Ryan Sleevi wrote: > Since |nickname| is optional (see line 1052), I believe this means that you're > potentially doing strlen(NULL). > > Perhaps a short-circuit before here for when there is no nickname? Good catch. Fixed. http://codereview.chromium.org/8566056/diff/3001/net/third_party/mozilla_secu... File net/third_party/mozilla_security_manager/nsPKCS12Blob.cpp (right): http://codereview.chromium.org/8566056/diff/3001/net/third_party/mozilla_secu... net/third_party/mozilla_security_manager/nsPKCS12Blob.cpp:250: break; On 2011/11/22 00:59:03, Ryan Sleevi wrote: > BUG: You will end up leaking |cert| here, as the break jumps to line 257, > bypassing the matching free call on line 254 for the allocation on line 220. Thanks. Fixed.
Review comments on Patch Set 3: I only reviewed the changes under src/net. I like your cleanup of the code. But I am not sure about some of the changes in x509_certificate_nss.cc related to certificate nicknames. Please search for my comments below marked with "BUG" and "IMPORTANT". http://codereview.chromium.org/8566056/diff/7001/net/base/cert_database.h File net/base/cert_database.h (right): http://codereview.chromium.org/8566056/diff/7001/net/base/cert_database.h#new... net/base/cert_database.h:127: CertificateList* imported_certs); Please document the new imported_certs output parameter. You can copy your comment from net/third_party/mozilla_security_manager/nsPKCS12Blob.h. http://codereview.chromium.org/8566056/diff/7001/net/base/cert_database.h#new... net/base/cert_database.h:189: // on success. ("label" here refers to the NSS attribute CKA_LABEL, Nit: NSS attribute => PKCS #11 attribute http://codereview.chromium.org/8566056/diff/7001/net/base/cert_database.h#new... net/base/cert_database.h:195: // certificates that match). This is a substring match, so if the Doing an exact match in DeleteCertAndKeyByLabel but a substring match in ListCertsWithLabel is a non-obvious inconsistency. It seems that this class just needs to provide ListCerts(), and you can do the substring match of labels in Chrome OS ONC code. http://codereview.chromium.org/8566056/diff/7001/net/base/x509_certificate.cc File net/base/x509_certificate.cc (right): http://codereview.chromium.org/8566056/diff/7001/net/base/x509_certificate.cc... net/base/x509_certificate.cc:310: X509Certificate* X509Certificate::CreateFromBytesWithNickname( This probably should be moved to x509_certificate_nss.cc, but I know why you put it here (to be next to the similar CreateFromBytes method). http://codereview.chromium.org/8566056/diff/7001/net/base/x509_certificate.h File net/base/x509_certificate.h (right): http://codereview.chromium.org/8566056/diff/7001/net/base/x509_certificate.h#... net/base/x509_certificate.h:34: #include <net/base/cert_type.h> This should be #include "net/base/cert_type.h" <foo.h> means foo.h is a system header. http://codereview.chromium.org/8566056/diff/7001/net/base/x509_certificate.h#... net/base/x509_certificate.h:132: // underlying crypto library. The returned pointer MUST be stored in a Nit: we don't have a convention of capitalizing MUST for emphasis in our header file comments. http://codereview.chromium.org/8566056/diff/7001/net/base/x509_certificate.h#... net/base/x509_certificate.h:157: static X509Certificate* CreateFromBytesWithNickname(const char* data, It would be nice to point out the difference between this method and the CreateFromBytes method above, and to explain why this method is necessary. Similarly for the CreateOSCertHandleFromBytesWithNickname method below. http://codereview.chromium.org/8566056/diff/7001/net/base/x509_certificate.h#... net/base/x509_certificate.h:215: // Set/get the label of this certificate (the equivalent of NSS's Nit: Set/get => Sets/gets NSS's => PKCS #11's http://codereview.chromium.org/8566056/diff/7001/net/base/x509_certificate.h#... net/base/x509_certificate.h:219: std::string GetLabel(); I believe GetLabel() can be 'const': std::string GetLabel() const; http://codereview.chromium.org/8566056/diff/7001/net/base/x509_certificate.h#... net/base/x509_certificate.h:223: CertType GetCertificateType() const; Nit: GetCertificateType => GetCertType http://codereview.chromium.org/8566056/diff/7001/net/base/x509_certificate.h#... net/base/x509_certificate.h:451: const char* data, int length, const char* nickname); Nit: List one parameter on each line. See http://www.chromium.org/developers/coding-style#TOC-Code-formatting Follow the "OK3" example, and note the "BadWrappingStyle" example. http://codereview.chromium.org/8566056/diff/7001/net/base/x509_certificate_ns... File net/base/x509_certificate_nss.cc (right): http://codereview.chromium.org/8566056/diff/7001/net/base/x509_certificate_ns... net/base/x509_certificate_nss.cc:675: bool SetPKCS11Nicknames(CERTCertificate* cert_handle, Please document this function. http://codereview.chromium.org/8566056/diff/7001/net/base/x509_certificate_ns... net/base/x509_certificate_nss.cc:786: switch(GetCertificateType()) { Nit: add a space after 'switch'. IMPORTANT: in this "else" branch, the returned string is not the CKA_LABEL attribute of the certificate object, so this contradicts the comment for GetLabel() in x509_certificate.h. In this case, this function actually builds (not gets) an appropriate label for the certificate object. http://codereview.chromium.org/8566056/diff/7001/net/base/x509_certificate_ns... net/base/x509_certificate_nss.cc:815: result = subject().GetDisplayName(); Nit: we can just use the subject_ member when inside this class. http://codereview.chromium.org/8566056/diff/7001/net/base/x509_certificate_ns... net/base/x509_certificate_nss.cc:1084: PORT_ArenaRelease(result->arena, result->nickname); BUG: the fact that result->nickname is allocated from result->arena is an implementation detail. (I believe it is done here: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/pki/pki3... ) We should not depend on this implementation detail. Also, it is wrong to call PORT_ArenaRelease here. The second argument of PORT_ArenaRelease is a "mark" in the arena, but result->nickname is not a mark. It is not necessary to release the result->nickname memory; it will be released when the entire arena is freed. Finally, it seems wrong change the 'nickname' field of a CERTCertificate structure. I think the 'nickname' field should be treated as read-only. It may need to stay consistent with something else inside NSS. http://codereview.chromium.org/8566056/diff/7001/net/base/x509_certificate_ns... net/base/x509_certificate_nss.cc:1089: result->nickname = arena_nickname; You can use the PORT_ArenaStrdup function. http://codereview.chromium.org/8566056/diff/7001/net/third_party/mozilla_secu... File net/third_party/mozilla_security_manager/nsPKCS12Blob.cpp (right): http://codereview.chromium.org/8566056/diff/7001/net/third_party/mozilla_secu... net/third_party/mozilla_security_manager/nsPKCS12Blob.cpp:141: // do a no-op, since I've already got Unicode. Hah! In general it's not necessary to fix minor comment problems in third-party code. We have forked these files, so this is fine. http://codereview.chromium.org/8566056/diff/7001/net/third_party/mozilla_secu... net/third_party/mozilla_security_manager/nsPKCS12Blob.cpp:215: continue; BUG: I would expect that a certificate without a corresponding private key should also be added to imported_certs.
On 2011/11/21 22:08:55, Greg Spencer (Chromium) wrote: > > To fix this, I think I need to implement the unimplemented PK11_TypeCert case > here: http://goo.gl/LL2aT > It's a three-line implementation. > > But implementing that would require Chrome to build nss on Linux As we discussed in person yesterday, I suggest that we submit your three-line change to NSS upstream, apply that patch to the NSS package in Chrome OS, and add #if defined(OS_CHROMEOS) to the Chrome (ONC related?) code that needs to set and get certificate labels.
Ryan, can you confirm that this is what you wanted? On 2011/11/29 00:21:49, Greg Spencer (Chromium) wrote: > On 2011/11/22 00:59:03, Ryan Sleevi wrote: > > I'm sure you'll want to get wtc to review the net/ changes. > > Yes, that's my plan, now that he's back in the office. > > > For my own $.02, it feels weird/wrong to bring in the msm dependency > > to x509_certificate. Certificate labels (and types) are more aligned > > with a particular database store, not an individual certificate, > > IMO. > > OK, so what you'd want to see here is something like moving > GetCertificateType (the only thing that depends on msm) into the > cert_database.h API? Or would you rather it moved into x509_util > instead? > > > It also creates more API divergence between the platforms, which > > I've been subtlety trying to reduce when possible. My own goal would > > be that X509Certificate has a single, consistent interface across > > platforms, and the platform-specific bits can be in a separate space > > (such as x509_util_[nss/mac/win/openssl]), since such > > implementations are inherently non-portable. Still though, in this > > particular case, it feels more suited for the CertDatabase code than > > X509Certificate - especially since 'labels' are inherently tied to > > keychain/CAPI store on OS X & Win. > > I agree with you on the divergence, but it occured to me that doing > that limits the x509_certificate API to the least common denominator, > which is why I chose the #ifdef method. To be clear, you think it > would be better if I moved all of the label related code into the > x509_util namespace and made standalone functions that take > certificates as arguments? (I'm not opposed: just clarifying) > > http://codereview.chromium.org/8566056/diff/3001/net/base/x509_certificate_ns... > File net/base/x509_certificate_nss.cc (right): > > http://codereview.chromium.org/8566056/diff/3001/net/base/x509_certificate_ns... > net/base/x509_certificate_nss.cc:691: // to the given label. > On 2011/11/22 00:59:03, Ryan Sleevi wrote: > > Rather than having to extract and encode the DER from the token, would it be > > better to use the PK11_ReadRawAttribute() and compare with the CKA_ID? Since > you > > should have the same CKA_ID between the CKO_PUBLIC_KEY, CKO_PRIVATE_KEY, and > > CKO_CERTIFICATE > > That's a great idea, except that there's no way to get the CKA_ID from a cert. > PK11_ReadRawAttribute isn't implemented for PK11_TypeCert, and > PK11_ReadAttribute is a private interface (in your sample, the first > PK11_ReadRawAttribute function that takes a slot and pkcs11ID doesn't exist). > > http://codereview.chromium.org/8566056/diff/3001/net/base/x509_certificate_ns... > net/base/x509_certificate_nss.cc:1082: int len = strlen(nickname); > On 2011/11/22 00:59:03, Ryan Sleevi wrote: > > Since |nickname| is optional (see line 1052), I believe this means that you're > > potentially doing strlen(NULL). > > > > Perhaps a short-circuit before here for when there is no nickname? > > Good catch. Fixed. > > http://codereview.chromium.org/8566056/diff/3001/net/third_party/mozilla_secu... > File net/third_party/mozilla_security_manager/nsPKCS12Blob.cpp (right): > > http://codereview.chromium.org/8566056/diff/3001/net/third_party/mozilla_secu... > net/third_party/mozilla_security_manager/nsPKCS12Blob.cpp:250: break; > On 2011/11/22 00:59:03, Ryan Sleevi wrote: > > BUG: You will end up leaking |cert| here, as the break jumps to line 257, > > bypassing the matching free call on line 254 for the allocation on line 220. > > Thanks. Fixed.
On 2011/11/29 00:21:49, Greg Spencer (Chromium) wrote: > On 2011/11/22 00:59:03, Ryan Sleevi wrote: > > I'm sure you'll want to get wtc to review the net/ changes. > > Yes, that's my plan, now that he's back in the office. > > > For my own $.02, it feels weird/wrong to bring in the msm dependency > > to x509_certificate. Certificate labels (and types) are more aligned > > with a particular database store, not an individual certificate, > > IMO. > > OK, so what you'd want to see here is something like moving > GetCertificateType (the only thing that depends on msm) into the > cert_database.h API? Or would you rather it moved into x509_util > instead? I think the former more than the latter, but either work. For example, on the CryptoAPI side, there is CertSetCertificateContextProperty with CERT_FRIENDLY_NAME_PROP_ID, but the property is actually stored in the HCERTSTORE (CERT_CONTEXT->pCertStore) - and HCERTSTORE is the "certificate database" type. On the OS X front, Keychain services exposes SecKeychainItemModifyAttributesAndData, which takes a SecKeychainItemRef, with attributes such as kSecLabelItemAttr / kSecDescriptionItemAttr. A SecKeychainItemRef always implies a SecKeychainRef, and a SecKeychainRef is the logical "certificate database type". I certainly haven't been involved with any of the larger API design in Chromium, but from what I've seen in the codebase, it seems that when things get into a whole host of platform specific cases, with no overlap between the abstractions, the tend to get broken into _mac/_win/_posix/_linux/_android/_etc specific implementations. The canonical example would be the various files under base/mac, base/nix, base/win, etc. If it doesn't make sense for the CertDatabase - such as there is never/will never be a need for a cross-platform implementation, then it seems better suited for the _util_nss.h, so that it's clear it's intentionally a platform-specific implementation, not a generic abstraction. > > > It also creates more API divergence between the platforms, which > > I've been subtlety trying to reduce when possible. My own goal would > > be that X509Certificate has a single, consistent interface across > > platforms, and the platform-specific bits can be in a separate space > > (such as x509_util_[nss/mac/win/openssl]), since such > > implementations are inherently non-portable. Still though, in this > > particular case, it feels more suited for the CertDatabase code than > > X509Certificate - especially since 'labels' are inherently tied to > > keychain/CAPI store on OS X & Win. > > I agree with you on the divergence, but it occured to me that doing > that limits the x509_certificate API to the least common denominator, > which is why I chose the #ifdef method. To be clear, you think it > would be better if I moved all of the label related code into the > x509_util namespace and made standalone functions that take > certificates as arguments? (I'm not opposed: just clarifying) I think limiting X509Certificate to the common, platform-agnostic abstraction is a good thing, based on it's wide usage through the code. I would like to see all of the #ifdefs, outside of the OSCertHandle, go away, and I am working towards that goal. TL;DR explanation: Right now X509Certificate is responsible for a number of things and appears in code for various reasons: - Want to simply scope a platform-specific certificate type (ala scoped_ptr_malloc) - Basic parsing of subject/issuer/validity dates for display (although most of the display stuff is extracted now into platform-specific headers) - Containing all of the validation logic for SSL *server* certificates (nothing for client certs, nothing for intermediate certs - both use cases that we do want to support at some point) - A grab bag for some platform-specific helpers for one or two special cases (primarily on the _mac side, related to client certs) The first two responsibilities make sense - hide some of the platform specific ugliness for common tasks, and make it easy to get access to the platform-specific bits when needed. The latter two seem like an API smell. Perhaps more importantly though, X509Certificates are immutable on creation (mod exposing the OSCertHandle). A big part of this is thread-safety (X509Certificates* are used and shared across multiple threads - refcountedthreadsafe and all). Having methods like GetLabel/SetLabel that can mutate the X509Certificate seems very inconsistent with this. Especially if this is something CrOS specific, rather than a generic abstraction, it seems better to stick it in a platform-specific header & implementation, rather than add to the confusion.
Please take another look, some things have changed. From Ryan's comments, I've moved most of the NSS specific code and APIs into x509_util_nss, and refactored things a bit. There are still a couple of additions to x509_certificate that are NSS specific (CreateFromBytesWithNickname, CreateOSCertHandleFromBytesWithNickname), but they are there because it's the only way to pass the nickname to the creation functions, which is the only way to set the nickname parameter of the certificates. I've also addressed Wan-Teh's comments and fixed the bugs he pointed out. (Some comments no longer applied after the refactor.) http://codereview.chromium.org/8566056/diff/7001/net/base/cert_database.h File net/base/cert_database.h (right): http://codereview.chromium.org/8566056/diff/7001/net/base/cert_database.h#new... net/base/cert_database.h:127: CertificateList* imported_certs); On 2011/11/29 23:13:57, wtc wrote: > > Please document the new imported_certs output parameter. > You can copy your comment from > net/third_party/mozilla_security_manager/nsPKCS12Blob.h. Done. http://codereview.chromium.org/8566056/diff/7001/net/base/cert_database.h#new... net/base/cert_database.h:189: // on success. ("label" here refers to the NSS attribute CKA_LABEL, On 2011/11/29 23:13:57, wtc wrote: > > Nit: NSS attribute => PKCS #11 attribute Done. I've also moved these functions into the onc parser because it's the only client at the moment, and it keeps NSS specific code out of this header. http://codereview.chromium.org/8566056/diff/7001/net/base/cert_database.h#new... net/base/cert_database.h:195: // certificates that match). This is a substring match, so if the On 2011/11/29 23:13:57, wtc wrote: > > Doing an exact match in DeleteCertAndKeyByLabel but a substring > match in ListCertsWithLabel is a non-obvious inconsistency. > > It seems that this class just needs to provide ListCerts(), > and you can do the substring match of labels in Chrome OS > ONC code. I've moved these into OncNetworkParser, since it keeps NSS specific calls out of this API, and they both do substring matches now. http://codereview.chromium.org/8566056/diff/7001/net/base/x509_certificate.cc File net/base/x509_certificate.cc (right): http://codereview.chromium.org/8566056/diff/7001/net/base/x509_certificate.cc... net/base/x509_certificate.cc:310: X509Certificate* X509Certificate::CreateFromBytesWithNickname( On 2011/11/29 23:13:57, wtc wrote: > > This probably should be moved to x509_certificate_nss.cc, > but I know why you put it here (to be next to the similar > CreateFromBytes method). Ahh, good point. I moved it there. http://codereview.chromium.org/8566056/diff/7001/net/base/x509_certificate.h File net/base/x509_certificate.h (right): http://codereview.chromium.org/8566056/diff/7001/net/base/x509_certificate.h#... net/base/x509_certificate.h:34: #include <net/base/cert_type.h> On 2011/11/29 23:13:57, wtc wrote: > > This should be > #include "net/base/cert_type.h" > > <foo.h> means foo.h is a system header. Whoops, yeah, I know that. :) Fixed. http://codereview.chromium.org/8566056/diff/7001/net/base/x509_certificate.h#... net/base/x509_certificate.h:132: // underlying crypto library. The returned pointer MUST be stored in a On 2011/11/29 23:13:57, wtc wrote: > > Nit: we don't have a convention of capitalizing MUST for > emphasis in our header file comments. OK, fixed. http://codereview.chromium.org/8566056/diff/7001/net/base/x509_certificate.h#... net/base/x509_certificate.h:157: static X509Certificate* CreateFromBytesWithNickname(const char* data, On 2011/11/29 23:13:57, wtc wrote: > > It would be nice to point out the difference between this > method and the CreateFromBytes method above, and to explain > why this method is necessary. > > Similarly for the CreateOSCertHandleFromBytesWithNickname > method below. Done. http://codereview.chromium.org/8566056/diff/7001/net/base/x509_certificate.h#... net/base/x509_certificate.h:215: // Set/get the label of this certificate (the equivalent of NSS's On 2011/11/29 23:13:57, wtc wrote: > > Nit: Set/get => Sets/gets > NSS's => PKCS #11's Done. http://codereview.chromium.org/8566056/diff/7001/net/base/x509_certificate.h#... net/base/x509_certificate.h:219: std::string GetLabel(); On 2011/11/29 23:13:57, wtc wrote: > > I believe GetLabel() can be 'const': > std::string GetLabel() const; Yes, it should be. But I've moved these functions into x509_util_nss, so they're standalone functions now. http://codereview.chromium.org/8566056/diff/7001/net/base/x509_certificate.h#... net/base/x509_certificate.h:223: CertType GetCertificateType() const; On 2011/11/29 23:13:57, wtc wrote: > > Nit: GetCertificateType => GetCertType Done. http://codereview.chromium.org/8566056/diff/7001/net/base/x509_certificate.h#... net/base/x509_certificate.h:451: const char* data, int length, const char* nickname); On 2011/11/29 23:13:57, wtc wrote: > > Nit: List one parameter on each line. See > http://www.chromium.org/developers/coding-style#TOC-Code-formatting > > Follow the "OK3" example, and note the "BadWrappingStyle" > example. OK, done. Thanks, I had forgotten that rule, and we have LOTS of counter examples in the code to help me forget (like CreateOSCertHandlesFromBytes, for instance. I fixed that one too). :) http://codereview.chromium.org/8566056/diff/7001/net/base/x509_certificate_ns... File net/base/x509_certificate_nss.cc (right): http://codereview.chromium.org/8566056/diff/7001/net/base/x509_certificate_ns... net/base/x509_certificate_nss.cc:675: bool SetPKCS11Nicknames(CERTCertificate* cert_handle, On 2011/11/29 23:13:57, wtc wrote: > > Please document this function. This function has been merged back into SetLabel. http://codereview.chromium.org/8566056/diff/7001/net/base/x509_certificate_ns... net/base/x509_certificate_nss.cc:786: switch(GetCertificateType()) { On 2011/11/29 23:13:57, wtc wrote: > > Nit: add a space after 'switch'. > > IMPORTANT: in this "else" branch, the returned string is > not the CKA_LABEL attribute of the certificate object, so > this contradicts the comment for GetLabel() in > x509_certificate.h. In this case, this function actually > builds (not gets) an appropriate label for the certificate > object. True. I've split this into two separate functions. One that gets the Label that is currently set on the certificate, and one that gets the "Default" label (which on ChromeOS is the label if set, and the generated name if not, and on other platforms is the generated name). They're both part of x509_util_nss http://codereview.chromium.org/8566056/diff/7001/net/base/x509_certificate_ns... net/base/x509_certificate_nss.cc:815: result = subject().GetDisplayName(); On 2011/11/29 23:13:57, wtc wrote: > > Nit: we can just use the subject_ member when inside this class. Good point, but in the new incarnation I had to call subject() because it's not a member function anymore. http://codereview.chromium.org/8566056/diff/7001/net/base/x509_certificate_ns... net/base/x509_certificate_nss.cc:1084: PORT_ArenaRelease(result->arena, result->nickname); On 2011/11/29 23:13:57, wtc wrote: > BUG: the fact that result->nickname is allocated from > result->arena is an implementation detail. OK, I've changed the way I do things here. I no longer try and keep the nickname field up to date, and instead rely solely on the CKA_LABEL attribute (via GetLabel). http://codereview.chromium.org/8566056/diff/7001/net/base/x509_certificate_ns... net/base/x509_certificate_nss.cc:1089: result->nickname = arena_nickname; On 2011/11/29 23:13:57, wtc wrote: > > You can use the PORT_ArenaStrdup function. This code went away. http://codereview.chromium.org/8566056/diff/7001/net/third_party/mozilla_secu... File net/third_party/mozilla_security_manager/nsPKCS12Blob.cpp (right): http://codereview.chromium.org/8566056/diff/7001/net/third_party/mozilla_secu... net/third_party/mozilla_security_manager/nsPKCS12Blob.cpp:215: continue; On 2011/11/29 23:13:57, wtc wrote: > > BUG: I would expect that a certificate without a corresponding > private key should also be added to imported_certs. Oooh. Nice catch. Done.
Ping. We'd like to get this in for R17.
Patch Set 4 LGTM. High-level comments: 1. I don't think we fully understand how NSS manages the nicknames of certificates. I have several comments marked with "IMPORTANT" about this. 2. A risk of the GetDefaultCertificateLabel function is that GetCertType may return the wrong result. This is why I asked you to add CHECKs (not DCHECKs) below. To eliminate this risk, you can add a "CertType cert_type" parameter to GetDefaultCertificateLabel. http://codereview.chromium.org/8566056/diff/19017/chrome/browser/chromeos/cro... File chrome/browser/chromeos/cros/onc_network_parser.cc (left): http://codereview.chromium.org/8566056/diff/19017/chrome/browser/chromeos/cro... chrome/browser/chromeos/cros/onc_network_parser.cc:66: Nit: you should keep this blank line to match the blank line on line 24 above. http://codereview.chromium.org/8566056/diff/19017/chrome/browser/chromeos/cro... File chrome/browser/chromeos/cros/onc_network_parser.cc (right): http://codereview.chromium.org/8566056/diff/19017/chrome/browser/chromeos/cro... chrome/browser/chromeos/cros/onc_network_parser.cc:284: return NULL; IMPORTANT: why do you set the label on x509_cert when you already pass the label to net::X509Certificate::CreateFromBytesWithNickname() above? http://codereview.chromium.org/8566056/diff/19017/chrome/browser/chromeos/cro... chrome/browser/chromeos/cros/onc_network_parser.cc:315: if (!net::x509_util::SetLabel(x509_cert, guid)) IMPORTANT: this is a sign that we don't fully understand what's going on. I don't understand why we have to set the label three times (once at certificate creation and twice afterwards). I believe the "token:" prefix is added by NSS, and the underlying CKA_LABEL attribute does NOT have "token:". http://codereview.chromium.org/8566056/diff/19017/chrome/browser/chromeos/cro... chrome/browser/chromeos/cros/onc_network_parser.cc:353: if (imported_certs.size() == 0ul) { Nit: you should be able to use 0 and 1 without the ul suffix here. http://codereview.chromium.org/8566056/diff/19017/chrome/browser/chromeos/cro... chrome/browser/chromeos/cros/onc_network_parser.cc:366: if (!net::x509_util::SetLabel(cert_result.get(), guid)) IMPORTANT: you can specify the certificate "friendly name" when you generate the PKCS #12 file. I hope (but haven't verified) NSS honors the certificate friendly name in a PKCS #12 file. http://codereview.chromium.org/8566056/diff/19017/chrome/browser/chromeos/cro... chrome/browser/chromeos/cros/onc_network_parser.cc:401: if (net::x509_util::GetLabel(iter->get()).find(label) != std::string::npos) IMPORTANT: document that DeleteCertAndKeyByLabel and ListCertsWithLabel perform substring match of |label|. This is not obvious. http://codereview.chromium.org/8566056/diff/19017/chrome/browser/chromeos/cro... chrome/browser/chromeos/cros/onc_network_parser.cc:402: new_list.push_back(*iter); Nit: why don't you just push *iter to |result| directly? http://codereview.chromium.org/8566056/diff/19017/net/base/cert_database_nss.cc File net/base/cert_database_nss.cc (right): http://codereview.chromium.org/8566056/diff/19017/net/base/cert_database_nss.... net/base/cert_database_nss.cc:72: slot = PK11_ImportCertForKey( Please add a CHECK (non-debug assertion) here to assert that x509_util::GetCertType(root) is USER_CERT. http://codereview.chromium.org/8566056/diff/19017/net/base/x509_certificate.h File net/base/x509_certificate.h (right): http://codereview.chromium.org/8566056/diff/19017/net/base/x509_certificate.h... net/base/x509_certificate.h:34: #include "net/base/cert_type.h" Remove this #include. http://codereview.chromium.org/8566056/diff/19017/net/base/x509_certificate.h... net/base/x509_certificate.h:155: // This function differs from CreateFromBytesWithNickname in that it takes a Typo: CreateFromBytesWithNickname => CreateFromBytes http://codereview.chromium.org/8566056/diff/19017/net/base/x509_certificate.h... net/base/x509_certificate.h:158: // nicknames. "NSS is the only certificate store that supports nicknames" is confusing because the Windows system certificate store also supports certificate nicknames (called "friendly names"). http://codereview.chromium.org/8566056/diff/19017/net/base/x509_certificate_n... File net/base/x509_certificate_nss.cc (left): http://codereview.chromium.org/8566056/diff/19017/net/base/x509_certificate_n... net/base/x509_certificate_nss.cc:669: Nit: add this blank line back. This matches the blank line on line 35 above. http://codereview.chromium.org/8566056/diff/19017/net/base/x509_certificate_n... File net/base/x509_certificate_nss.cc (right): http://codereview.chromium.org/8566056/diff/19017/net/base/x509_certificate_n... net/base/x509_certificate_nss.cc:9: #include <keyhi.h> The new code you added doesn't seem to require <keyhi.h>. So I believe this #include can be removed. http://codereview.chromium.org/8566056/diff/19017/net/base/x509_util_nss.cc File net/base/x509_util_nss.cc (left): http://codereview.chromium.org/8566056/diff/19017/net/base/x509_util_nss.cc#o... net/base/x509_util_nss.cc:169: Nit: keep this blank line. This blank line matches the blank line at line 25 above. http://codereview.chromium.org/8566056/diff/19017/net/base/x509_util_nss.cc File net/base/x509_util_nss.cc (right): http://codereview.chromium.org/8566056/diff/19017/net/base/x509_util_nss.cc#n... net/base/x509_util_nss.cc:340: #endif This behavior (lines 335-340) is difficult to document precisely in prose. Why is this behavior necessary for Chrome OS. http://codereview.chromium.org/8566056/diff/19017/net/base/x509_util_nss.cc#n... net/base/x509_util_nss.cc:344: char *nickname = CERT_MakeCANickname(cert->os_cert_handle()); Nit: put '*' next to 'char'. http://codereview.chromium.org/8566056/diff/19017/net/base/x509_util_nss.cc#n... net/base/x509_util_nss.cc:351: // We use the scheme used by Firefox: Nit: extraneous space before "We". http://codereview.chromium.org/8566056/diff/19017/net/base/x509_util_nss.cc#n... net/base/x509_util_nss.cc:374: } Nit: this case doesn't need curly braces because it doesn't use local variables. http://codereview.chromium.org/8566056/diff/19017/net/base/x509_util_nss.cc#n... net/base/x509_util_nss.cc:393: sec_label.type = siUTF8String; PK11_WriteRawAttribute does not use the 'type' field of this SECItem, so I suggest we just set it to the default siBuffer. http://codereview.chromium.org/8566056/diff/19017/net/base/x509_util_nss.cc#n... net/base/x509_util_nss.cc:400: &sec_label); IMPORTANT: my concern about this solution is that the NSS CERTCertificate structure has a 'nickname' field: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certdb/c... but your code does not update the 'nickname' field, so it will become out of sync with the CKA_LABEL attribute of the underlying PKCS #11 certificate object. Perhaps this is why NSS does not support setting the CKA_LABEL attribute on a PKCS #11 certificate object. I think a better solution might be to only set the CKA_LABEL attribute on the private key object. Once you find the right private key object by the label, NSS can find the corresponding certificate object. Will this solution work for you? http://codereview.chromium.org/8566056/diff/19017/net/base/x509_util_nss.cc#n... net/base/x509_util_nss.cc:403: << label; Nit: please add curly braces because the statement is two lines. http://codereview.chromium.org/8566056/diff/19017/net/base/x509_util_nss.cc#n... net/base/x509_util_nss.cc:457: &sec_label); Why don't you just use the 'nickname' field of the NSS CERTCertificate structure? You can strip the "<token-name>:" prefix if necessary. http://codereview.chromium.org/8566056/diff/19017/net/base/x509_util_nss.h File net/base/x509_util_nss.h (right): http://codereview.chromium.org/8566056/diff/19017/net/base/x509_util_nss.h#ne... net/base/x509_util_nss.h:40: // Gets which type of certificate this is, based on the certificate's Nit: Gets => Returns http://codereview.chromium.org/8566056/diff/19017/net/base/x509_util_nss.h#ne... net/base/x509_util_nss.h:47: // Set/get the label of this certificate. This corresponds to the CKA_LABEL Nit: Set/get => Sets/gets http://codereview.chromium.org/8566056/diff/19017/net/base/x509_util_nss.h#ne... net/base/x509_util_nss.h:48: // attribute of the certificate. Setting the label on the certificate also Nit: add "PKCS #11" before "attribute". http://codereview.chromium.org/8566056/diff/19017/net/base/x509_util_nss.h#ne... net/base/x509_util_nss.h:52: std::string GetLabel(const X509Certificate* cert); These two functions should be named SetCertificateLabel GetCertificateLabel or SetCertLabel GetCertLabel The closely-related GetDefaultCertificateLabel function should be moved down here. http://codereview.chromium.org/8566056/diff/19017/net/base/x509_util_nss.h#ne... net/base/x509_util_nss.h:54: } // namespace x509_util Nit: add a blank line before this line. http://codereview.chromium.org/8566056/diff/19017/net/third_party/mozilla_sec... File net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp (right): http://codereview.chromium.org/8566056/diff/19017/net/third_party/mozilla_sec... net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp:84: SECStatus srv = PK11_ImportCert( Please add a CHECK (non-debug assertion) here to assert that net::x509_util::GetCertType(root) is net::CA_CERT. http://codereview.chromium.org/8566056/diff/19017/net/third_party/mozilla_sec... net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp:141: SECStatus srv = PK11_ImportCert( Please add a CHECK (non-debug assertion) here to assert that net::x509_util::GetCertType(cert) is net::CA_CERT. http://codereview.chromium.org/8566056/diff/19017/net/third_party/mozilla_sec... net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp:174: SECStatus srv = PK11_ImportCert( Please add a CHECK (non-debug assertion) here to assert that net::x509_util::GetCertType(cert) is net::SERVER_CERT. http://codereview.chromium.org/8566056/diff/19017/net/third_party/mozilla_sec... File net/third_party/mozilla_security_manager/nsPKCS12Blob.cpp (right): http://codereview.chromium.org/8566056/diff/19017/net/third_party/mozilla_sec... net/third_party/mozilla_security_manager/nsPKCS12Blob.cpp:236: continue; BUG: we also need to call CERT_DestroyCertificate(cert); before 'continue'.
Thanks for your great review, Wan-Teh. I think this finally addresses all the issues we spoke of. I now set the nicknames only at creation time for server and CA certs, and I treat client certs separately. For client certs, I set the nickname right after creation using the PK11_SetPrivateKeyNickname function, and then when searching for certificates, I look at the private key for each certificate (if any) as well. This should address the problem of trying to set the nickname field on the cert itself, but still allow me to use the GUID to look up certificates. http://codereview.chromium.org/8566056/diff/19017/chrome/browser/chromeos/cro... File chrome/browser/chromeos/cros/onc_network_parser.cc (left): http://codereview.chromium.org/8566056/diff/19017/chrome/browser/chromeos/cro... chrome/browser/chromeos/cros/onc_network_parser.cc:66: On 2011/12/08 00:07:43, wtc wrote: > > Nit: you should keep this blank line to match the blank line > on line 24 above. Done. http://codereview.chromium.org/8566056/diff/19017/chrome/browser/chromeos/cro... File chrome/browser/chromeos/cros/onc_network_parser.cc (right): http://codereview.chromium.org/8566056/diff/19017/chrome/browser/chromeos/cro... chrome/browser/chromeos/cros/onc_network_parser.cc:284: return NULL; I've removed the Label setting code, and I'm now just relying on the nickname passed in to the creation routines. http://codereview.chromium.org/8566056/diff/19017/chrome/browser/chromeos/cro... chrome/browser/chromeos/cros/onc_network_parser.cc:353: if (imported_certs.size() == 0ul) { On 2011/12/08 00:07:43, wtc wrote: > > Nit: you should be able to use 0 and 1 without the ul suffix > here. Yes, I know that, but for some reason I was getting an error here at one point during my process (or I wouldn't have added the "ul" in the first place), but now that seems to not be the case anymore. http://codereview.chromium.org/8566056/diff/19017/chrome/browser/chromeos/cro... chrome/browser/chromeos/cros/onc_network_parser.cc:366: if (!net::x509_util::SetLabel(cert_result.get(), guid)) On 2011/12/08 00:07:43, wtc wrote: > > IMPORTANT: you can specify the certificate "friendly name" > when you generate the PKCS #12 file. I hope (but haven't verified) > NSS honors the certificate friendly name in a PKCS #12 file. That may be true, but I don't have control over the certificate creation itself. The user will import their own pre-generated certificates into the Spigots tool (the network configuration tool that generates the ONC files), so I don't have the chance to set it. http://codereview.chromium.org/8566056/diff/19017/chrome/browser/chromeos/cro... chrome/browser/chromeos/cros/onc_network_parser.cc:401: if (net::x509_util::GetLabel(iter->get()).find(label) != std::string::npos) On 2011/12/08 00:07:43, wtc wrote: > > IMPORTANT: document that DeleteCertAndKeyByLabel and > ListCertsWithLabel perform substring match of |label|. > This is not obvious. I switched this so that they are using exact matches, and documented that. http://codereview.chromium.org/8566056/diff/19017/chrome/browser/chromeos/cro... chrome/browser/chromeos/cros/onc_network_parser.cc:402: new_list.push_back(*iter); On 2011/12/08 00:07:43, wtc wrote: > > Nit: why don't you just push *iter to |result| directly? Because if there were failures we could abort and not have already modified |result|. I tend to program these sorts of things so that output parameters aren't modified until I know that the modification is going to completely succeed. It also means I don't have to call "clear" on |result|. And swap is a very fast operation. Regardless, there's no failure mode here, so I changed it as you suggested. http://codereview.chromium.org/8566056/diff/19017/net/base/cert_database_nss.cc File net/base/cert_database_nss.cc (right): http://codereview.chromium.org/8566056/diff/19017/net/base/cert_database_nss.... net/base/cert_database_nss.cc:72: slot = PK11_ImportCertForKey( On 2011/12/08 00:07:43, wtc wrote: > > Please add a CHECK (non-debug assertion) here to assert that > x509_util::GetCertType(root) is USER_CERT. Now that we are supplying the cert type to GetDefaultNickname (as it is now called), I don't think the CHECK is needed. http://codereview.chromium.org/8566056/diff/19017/net/base/x509_certificate.h File net/base/x509_certificate.h (right): http://codereview.chromium.org/8566056/diff/19017/net/base/x509_certificate.h... net/base/x509_certificate.h:34: #include "net/base/cert_type.h" On 2011/12/08 00:07:43, wtc wrote: > > Remove this #include. Done. http://codereview.chromium.org/8566056/diff/19017/net/base/x509_certificate.h... net/base/x509_certificate.h:155: // This function differs from CreateFromBytesWithNickname in that it takes a On 2011/12/08 00:07:43, wtc wrote: > > Typo: CreateFromBytesWithNickname => CreateFromBytes Done. http://codereview.chromium.org/8566056/diff/19017/net/base/x509_certificate.h... net/base/x509_certificate.h:158: // nicknames. On 2011/12/08 00:07:43, wtc wrote: > > "NSS is the only certificate store that supports nicknames" > is confusing because the Windows system certificate store > also supports certificate nicknames (called "friendly names"). I removed that statement from the comment. http://codereview.chromium.org/8566056/diff/19017/net/base/x509_certificate_n... File net/base/x509_certificate_nss.cc (left): http://codereview.chromium.org/8566056/diff/19017/net/base/x509_certificate_n... net/base/x509_certificate_nss.cc:669: On 2011/12/08 00:07:43, wtc wrote: > > Nit: add this blank line back. This matches the blank line > on line 35 above. Done. http://codereview.chromium.org/8566056/diff/19017/net/base/x509_certificate_n... File net/base/x509_certificate_nss.cc (right): http://codereview.chromium.org/8566056/diff/19017/net/base/x509_certificate_n... net/base/x509_certificate_nss.cc:9: #include <keyhi.h> On 2011/12/08 00:07:43, wtc wrote: > > The new code you added doesn't seem to require <keyhi.h>. > So I believe this #include can be removed. Done. http://codereview.chromium.org/8566056/diff/19017/net/base/x509_util_nss.cc File net/base/x509_util_nss.cc (left): http://codereview.chromium.org/8566056/diff/19017/net/base/x509_util_nss.cc#o... net/base/x509_util_nss.cc:169: On 2011/12/08 00:07:43, wtc wrote: > > Nit: keep this blank line. This blank line matches the > blank line at line 25 above. Done. http://codereview.chromium.org/8566056/diff/19017/net/base/x509_util_nss.cc File net/base/x509_util_nss.cc (right): http://codereview.chromium.org/8566056/diff/19017/net/base/x509_util_nss.cc#n... net/base/x509_util_nss.cc:340: #endif On 2011/12/08 00:07:43, wtc wrote: > > This behavior (lines 335-340) is difficult to document > precisely in prose. Why is this behavior necessary for > Chrome OS. We talked about this: I've removed this from the function, and instead I return a default nickname if there is one. http://codereview.chromium.org/8566056/diff/19017/net/base/x509_util_nss.cc#n... net/base/x509_util_nss.cc:344: char *nickname = CERT_MakeCANickname(cert->os_cert_handle()); On 2011/12/08 00:07:43, wtc wrote: > > Nit: put '*' next to 'char'. Done. http://codereview.chromium.org/8566056/diff/19017/net/base/x509_util_nss.cc#n... net/base/x509_util_nss.cc:351: // We use the scheme used by Firefox: On 2011/12/08 00:07:43, wtc wrote: > > Nit: extraneous space before "We". Fixed. http://codereview.chromium.org/8566056/diff/19017/net/base/x509_util_nss.cc#n... net/base/x509_util_nss.cc:374: } On 2011/12/08 00:07:43, wtc wrote: > > Nit: this case doesn't need curly braces because it doesn't > use local variables. Done. http://codereview.chromium.org/8566056/diff/19017/net/base/x509_util_nss.cc#n... net/base/x509_util_nss.cc:393: sec_label.type = siUTF8String; On 2011/12/08 00:07:43, wtc wrote: > > PK11_WriteRawAttribute does not use the 'type' field of this > SECItem, so I suggest we just set it to the default siBuffer. Done. http://codereview.chromium.org/8566056/diff/19017/net/base/x509_util_nss.cc#n... net/base/x509_util_nss.cc:400: &sec_label); On 2011/12/08 00:07:43, wtc wrote: > > IMPORTANT: my concern about this solution is that the NSS > CERTCertificate structure has a 'nickname' field: > http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/certdb/c... > > but your code does not update the 'nickname' field, so it > will become out of sync with the CKA_LABEL attribute of the > underlying PKCS #11 certificate object. I've stopped trying to set the nickname later, and instead I rely on setting the nickname at creation time for server and CA certs. For client certs, I set the nickname on the private key (which has a PK11 interface). > I think a better solution might be to only set the CKA_LABEL > attribute on the private key object. Once you find the right > private key object by the label, NSS can find the corresponding > certificate object. Will this solution work for you? Yes, this is what I have adopted for client certificates. Server and CA certs have their nicknames set at creation time. http://codereview.chromium.org/8566056/diff/19017/net/base/x509_util_nss.cc#n... net/base/x509_util_nss.cc:403: << label; On 2011/12/08 00:07:43, wtc wrote: > > Nit: please add curly braces because the statement is two > lines. Done. http://codereview.chromium.org/8566056/diff/19017/net/base/x509_util_nss.cc#n... net/base/x509_util_nss.cc:457: &sec_label); On 2011/12/08 00:07:43, wtc wrote: > > Why don't you just use the 'nickname' field of the NSS > CERTCertificate structure? You can strip the "<token-name>:" > prefix if necessary. For server and CA certs, this works because I can supply the nickname at creation time. For client certs, it's not so simple. I have switched to using this model for server and CA certs, and for storing it on the private key for client certs. http://codereview.chromium.org/8566056/diff/19017/net/third_party/mozilla_sec... File net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp (right): http://codereview.chromium.org/8566056/diff/19017/net/third_party/mozilla_sec... net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp:84: SECStatus srv = PK11_ImportCert( On 2011/12/08 00:07:43, wtc wrote: > > Please add a CHECK (non-debug assertion) here to assert that > net::x509_util::GetCertType(root) is net::CA_CERT. Now that we're passing the certificate type instead of detecting it, this is not necessary. http://codereview.chromium.org/8566056/diff/19017/net/third_party/mozilla_sec... net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp:141: SECStatus srv = PK11_ImportCert( On 2011/12/08 00:07:43, wtc wrote: > > Please add a CHECK (non-debug assertion) here to assert that > net::x509_util::GetCertType(cert) is net::CA_CERT. Now that we pass the certificate type, this is not necessary. http://codereview.chromium.org/8566056/diff/19017/net/third_party/mozilla_sec... net/third_party/mozilla_security_manager/nsNSSCertificateDB.cpp:174: SECStatus srv = PK11_ImportCert( On 2011/12/08 00:07:43, wtc wrote: > > Please add a CHECK (non-debug assertion) here to assert that > net::x509_util::GetCertType(cert) is net::SERVER_CERT. Now that we pass the certificate type, this is not necessary. http://codereview.chromium.org/8566056/diff/19017/net/third_party/mozilla_sec... File net/third_party/mozilla_security_manager/nsPKCS12Blob.cpp (right): http://codereview.chromium.org/8566056/diff/19017/net/third_party/mozilla_sec... net/third_party/mozilla_security_manager/nsPKCS12Blob.cpp:236: continue; On 2011/12/08 00:07:43, wtc wrote: > > BUG: we also need to call > CERT_DestroyCertificate(cert); > before 'continue'. Done.
Patch Set 6 LGTM. The main issue is the special case code (first two lines) at the beginning of X509Certificate::GetDefaultNickname method. I propose two possible ways to avoid it below. http://codereview.chromium.org/8566056/diff/19017/chrome/browser/chromeos/cro... File chrome/browser/chromeos/cros/onc_network_parser.cc (right): http://codereview.chromium.org/8566056/diff/19017/chrome/browser/chromeos/cro... chrome/browser/chromeos/cros/onc_network_parser.cc:353: if (imported_certs.size() == 0ul) { On 2011/12/09 18:51:38, Greg Spencer (Chromium) wrote: > > Yes, I know that, but for some reason I was getting an error here at one point > during my process (or I wouldn't have added the "ul" in the first place), but > now that seems to not be the case anymore. I suspect you may have used a DCHECK_EQ here before. DCHECKS requires the "ul" suffix for disambiguation. http://codereview.chromium.org/8566056/diff/36003/chrome/browser/chromeos/cro... File chrome/browser/chromeos/cros/onc_network_parser.cc (right): http://codereview.chromium.org/8566056/diff/36003/chrome/browser/chromeos/cro... chrome/browser/chromeos/cros/onc_network_parser.cc:423: success = cert_database.ImportCACerts(cert_list, trust, &failures); Another approach to avoid adding the default_nickname_ member to X509Certificate is to add a 'nickname' input argument to the cert_database.ImportCACerts() method. If |nickname| is NULL, then cert_database.ImportCACerts() uses the (generated) default nickname. Similarly for cert_database.ImportServerCert(). http://codereview.chromium.org/8566056/diff/36003/chrome/browser/chromeos/cro... chrome/browser/chromeos/cros/onc_network_parser.cc:668: ::strchr(iter->get()->os_cert_handle()->nickname, ':'); You should explain why you are looking for ':' here. http://codereview.chromium.org/8566056/diff/36003/chrome/browser/chromeos/cro... chrome/browser/chromeos/cros/onc_network_parser.cc:672: result->push_back(*iter); IMPORTANT: should we add a 'continue' statement here? Otherwise *iter may be added to 'result' again on line 686 below. http://codereview.chromium.org/8566056/diff/36003/chrome/browser/chromeos/cro... File chrome/browser/chromeos/cros/onc_network_parser.h (right): http://codereview.chromium.org/8566056/diff/36003/chrome/browser/chromeos/cro... chrome/browser/chromeos/cros/onc_network_parser.h:130: net::CertificateList* result); Nit: you should also rename the first input parameter to |nickname| in these two functions, now that you have renamed the functions. Nit: I think "exact match" doesn't need to be mentioned. (A substring match would be non-obvious.) http://codereview.chromium.org/8566056/diff/36003/net/base/x509_certificate_n... File net/base/x509_certificate_nss.cc (right): http://codereview.chromium.org/8566056/diff/36003/net/base/x509_certificate_n... net/base/x509_certificate_nss.cc:703: cert->default_nickname_ = nickname; IMPORTANT: the default_nickname_ may not be necessary. Please add if (nickname) { CHECK(strcmp(cert_handle_->nickname, nickname) == 0); | else { CHECK(!cert_handle_->nickname); } If the CHECK never fails, then I think we can replace lines 709-710 with if (cert_handle_->nickname) return std::string(cert_handle_->nickname);
http://codereview.chromium.org/8566056/diff/36003/chrome/browser/chromeos/cro... File chrome/browser/chromeos/cros/onc_network_parser.cc (right): http://codereview.chromium.org/8566056/diff/36003/chrome/browser/chromeos/cro... chrome/browser/chromeos/cros/onc_network_parser.cc:423: success = cert_database.ImportCACerts(cert_list, trust, &failures); On 2011/12/10 00:58:43, wtc wrote: > Another approach to avoid adding the default_nickname_ > member to X509Certificate is to add a 'nickname' input > argument to the cert_database.ImportCACerts() method. > > If |nickname| is NULL, then cert_database.ImportCACerts() uses > the (generated) default nickname. > > Similarly for cert_database.ImportServerCert(). Well, the problem is that the multiplicity of the arguments to ImportCACerts isn't the same as the nickname then: you have a list of certs, and only one nickname. I think I'd rather leave the default_nickname_ member for now, because then they match (one default nickname for each cert). http://codereview.chromium.org/8566056/diff/36003/chrome/browser/chromeos/cro... chrome/browser/chromeos/cros/onc_network_parser.cc:668: ::strchr(iter->get()->os_cert_handle()->nickname, ':'); On 2011/12/10 00:58:43, wtc wrote: > > You should explain why you are looking for ':' here. Done. http://codereview.chromium.org/8566056/diff/36003/chrome/browser/chromeos/cro... chrome/browser/chromeos/cros/onc_network_parser.cc:672: result->push_back(*iter); On 2011/12/10 00:58:43, wtc wrote: > > IMPORTANT: should we add a 'continue' statement here? > Otherwise *iter may be added to 'result' again on line 686 > below. Good catch, thanks! http://codereview.chromium.org/8566056/diff/36003/net/base/x509_certificate_n... File net/base/x509_certificate_nss.cc (right): http://codereview.chromium.org/8566056/diff/36003/net/base/x509_certificate_n... net/base/x509_certificate_nss.cc:703: cert->default_nickname_ = nickname; On 2011/12/10 00:58:43, wtc wrote: > > IMPORTANT: the default_nickname_ may not be necessary. > > Please add > if (nickname) { > CHECK(strcmp(cert_handle_->nickname, nickname) == 0); > | else { > CHECK(!cert_handle_->nickname); > } > > If the CHECK never fails, then I think we can replace lines > 709-710 with > if (cert_handle_->nickname) > return std::string(cert_handle_->nickname); That would be nice, but unfortunately, CERT_NewTempCertificate doesn't initialize the nickname field properly for client certs. It sets the nickname on the *inner* NSSCertificate object (as the tempName field), but doesn't set it on the CERTCertificate it returns (nickname is still NULL). I see this as a bug in NSS, but I'm not sure what the intentions are at each level of the onion of structures. Perhaps for a temporary client certificate, it's appropriate to only set the inner NSSCertificate tempName. For Server and CA certs, it seems to do the right thing, and if I remove CreateOSCertHandleFromBytesWithNickname (as I could do if sending down the nickname did nothing), then they don't work because their nickname fields aren't set at import time. The CERTCertificate nickname gets properly set when the temporary client certificate is imported, because that uses the default nickname field to get it's value.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gspencer@chromium.org/8566056/43001
Presubmit check for 8566056-43001 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit ERRORS ** Missing LGTM from an OWNER for: chrome/browser/chromeos/cros/onc_network_parser_unittest.cc,chrome/browser/chromeos/cros/onc_network_parser.cc,chrome/browser/chromeos/cros/onc_network_parser.h,chrome/browser/chromeos/cros/network_library.cc Presubmit checks took 2.3s to calculate.
Zel, I need an OWNERS approval for chrome/browser/chromeos/cros changes. (I'm also thinking I might want to put myself in the OWNERS file in that dir...)
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gspencer@chromium.org/8566056/43001
Change committed as 113993 |
