This applies GUIDs to certificate and key nicknames when

net/base/x509_certificate_nss.cc

Index: net/base/x509_certificate_nss.cc
diff --git a/net/base/x509_certificate_nss.cc b/net/base/x509_certificate_nss.cc
index bbb5cef4b656bb88cfcb4bab30323010296f5545..8cb57fba55929dd6a021855259d2291f03db802f 100644
--- a/net/base/x509_certificate_nss.cc
+++ b/net/base/x509_certificate_nss.cc
@@ -6,6 +6,7 @@
 
 #include <cert.h>
 #include <cryptohi.h>
+#include <keyhi.h>
 #include <nss.h>
 #include <pk11pub.h>
 #include <prerror.h>
@@ -24,10 +25,14 @@
 #include "net/base/asn1_util.h"
 #include "net/base/cert_status_flags.h"
 #include "net/base/cert_verify_result.h"
+#include "net/base/cert_type.h"
 #include "net/base/crl_set.h"
 #include "net/base/ev_root_ca_metadata.h"
 #include "net/base/net_errors.h"
 #include "net/base/x509_util_nss.h"
+#include "net/third_party/mozilla_security_manager/nsNSSCertTrust.h"
+
+namespace msm = mozilla_security_manager;
 
 namespace net {
 
@@ -667,6 +672,55 @@ void AppendPublicKeyHashes(CERTCertList* cert_list,
   hashes->push_back(CertPublicKeyHash(root_cert));
 }
 
+bool SetPKCS11Nicknames(CERTCertificate* cert_handle,

[wtc, 2011/11/29 23:13:57]

Please document this function.

[Greg Spencer (Chromium), 2011/12/02 18:50:07]

This function has been merged back into SetLabel.

+                        const std::string& label) {
+  DCHECK(cert_handle);
+
+  // If the slot isn't initialized, then do nothing.
+  if (!cert_handle->slot)
+    return true;
+
+  // As far as I can tell, there is no API for PKCS11 that allows one
+  // to get the valid public key (that has a valid PKCS11 slot and id)
+  // associated with a certificate.  So, instead, I extract the public
+  // key using CERT_ExtractPublicKey (which returns a key that has no
+  // slot or pkcs11 id set), and then we iterate through all of the
+  // existing public keys (which do have this information), and look
+  // for one that has the same DER encoding as the public key
+  // extracted from this certificate.  I then set the key's nickname
+  // to the given label.
+  SECKEYPublicKey* public_key = CERT_ExtractPublicKey(cert_handle);
+  if (!public_key)
+    return false;
+
+  SECKEYPublicKeyList* pubkey_list =
+      PK11_ListPublicKeysInSlot(cert_handle->slot, NULL);
+
+  // If there are no public keys, that's OK.
+  if (pubkey_list) {
+    for (SECKEYPublicKeyListNode* node = PUBKEY_LIST_HEAD(pubkey_list);
+        !PUBKEY_LIST_END(node, pubkey_list);
+        node = PUBKEY_LIST_NEXT(node)) {
+      SECItem* der_encoded = PK11_DEREncodePublicKey(node->key);
+      if (SECITEM_CompareItem(der_encoded,
+                              &cert_handle->derPublicKey) == SECEqual)
+        PK11_SetPublicKeyNickname(node->key, label.c_str());
+      SECITEM_FreeItem(der_encoded, PR_TRUE);
+    }
+    SECKEY_DestroyPublicKeyList(pubkey_list);
+  }
+  SECKEY_DestroyPublicKey(public_key);
+
+  // Now set the nickname on the private key (if there is one)
+  SECKEYPrivateKey* private_key =
+      PK11_FindPrivateKeyFromCert(cert_handle->slot, cert_handle, NULL);
+  if (private_key) {
+    PK11_SetPrivateKeyNickname(private_key, label.c_str());
+    SECKEY_DestroyPrivateKey(private_key);
+  }
+  return true;
+}
+
 }  // namespace
 
 void X509Certificate::Initialize() {
@@ -707,6 +761,79 @@ X509Certificate* X509Certificate::CreateSelfSigned(
   return x509_cert;
 }
 
+bool X509Certificate::SetLabel(const std::string& label) {
+  if (!SetPKCS11Nicknames(cert_handle_, label))
+    return false;
+
+  // Now set the cert's nickname pointer.
+  char* nickname = reinterpret_cast<char*>(
+      PORT_ArenaAlloc(cert_handle_->arena, label.size() + 1));
+  if (!nickname)
+    return false;
+  PORT_Strcpy(nickname, label.c_str());
+  if (cert_handle_->nickname)
+    PORT_ArenaRelease(cert_handle_->arena, cert_handle_->nickname);
+  cert_handle_->nickname = nickname;
+  return true;
+}
+
+std::string X509Certificate::GetLabel() {
+  std::string result;
+  if (cert_handle_->nickname) {
+    // If the cert already has a nickname set, we use that.
+    result = std::string(cert_handle_->nickname);
+  } else {
+    switch(GetCertificateType()) {

[wtc, 2011/11/29 23:13:57]

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.

[Greg Spencer (Chromium), 2011/12/02 18:50:07]

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

+      case CA_CERT: {
+        char *nickname = CERT_MakeCANickname(cert_handle_);
+        result = nickname;
+        PORT_Free(nickname);
+        break;
+      }
+      case USER_CERT: {
+        // Create a nickname for this user certificate.
+        //  We use the scheme used by Firefox:
+        // --> <subject's common name>'s <issuer's common name> ID.
+        // TODO(gspencer): internationalize this: it's wrong to
+        // hard code English.
+
+        std::string username, ca_name;
+        char* temp_username = CERT_GetCommonName(&cert_handle_->subject);
+        char* temp_ca_name = CERT_GetCommonName(&cert_handle_->issuer);
+        if (temp_username) {
+          username = temp_username;
+          PORT_Free(temp_username);
+        }
+        if (temp_ca_name) {
+          ca_name = temp_ca_name;
+          PORT_Free(temp_ca_name);
+        }
+        result = username + "'s " + ca_name + " ID";
+        break;
+      }
+      case SERVER_CERT: {
+        result = subject().GetDisplayName();

[wtc, 2011/11/29 23:13:57]

Nit: we can just use the subject_ member when inside this class.

[Greg Spencer (Chromium), 2011/12/02 18:50:07]

Good point, but in the new incarnation I had to call subject() because it's not
a member function anymore.

+        break;
+      }
+      case UNKNOWN_CERT:
+      default:
+        break;
+    }
+  }
+  return result;
+}
+
+CertType X509Certificate::GetCertificateType() const {
+  msm::nsNSSCertTrust trust(cert_handle_->trust);
+  if (trust.HasAnyUser())
+    return USER_CERT;
+  if (trust.HasAnyCA() || CERT_IsCACert(cert_handle_, NULL))
+    return CA_CERT;
+  if (trust.HasPeer(PR_TRUE, PR_FALSE, PR_FALSE))
+    return SERVER_CERT;
+  return UNKNOWN_CERT;
+}
+
 void X509Certificate::GetSubjectAltName(
     std::vector<std::string>* dns_names,
     std::vector<std::string>* ip_addrs) const {
@@ -922,6 +1049,13 @@ bool X509Certificate::IsSameOSCert(X509Certificate::OSCertHandle a,
 // static
 X509Certificate::OSCertHandle X509Certificate::CreateOSCertHandleFromBytes(
     const char* data, int length) {
+  return CreateOSCertHandleFromBytesWithNickname(data, length, NULL);
+}
+
+// static
+X509Certificate::OSCertHandle
+X509Certificate::CreateOSCertHandleFromBytesWithNickname(
+    const char* data, int length, const char* nickname) {
   if (length < 0)
     return NULL;
 
@@ -936,8 +1070,25 @@ X509Certificate::OSCertHandle X509Certificate::CreateOSCertHandleFromBytes(
   der_cert.type = siDERCertBuffer;
 
   // Parse into a certificate structure.
-  return CERT_NewTempCertificate(CERT_GetDefaultCertDB(), &der_cert, NULL,
-                                 PR_FALSE, PR_TRUE);
+  X509Certificate::OSCertHandle result =
+      CERT_NewTempCertificate(CERT_GetDefaultCertDB(), &der_cert,
+                              const_cast<char*>(nickname),
+                              PR_FALSE, PR_TRUE);
+
+  // CERT_NewTempCertificate doesn't always fill in the nickname,
+  // so we have to (if there is one).
+  if (!nickname)
+    return result;
+
+  if (result->nickname)
+    PORT_ArenaRelease(result->arena, result->nickname);

[wtc, 2011/11/29 23:13:57]

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.

[Greg Spencer (Chromium), 2011/12/02 18:50:07]

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).

+  int len = strlen(nickname);
+  char *arena_nickname = reinterpret_cast<char*>(
+      PORT_ArenaAlloc(result->arena, len + 1));
+  PORT_Strncpy(arena_nickname, nickname, len);
+  result->nickname = arena_nickname;

[wtc, 2011/11/29 23:13:57]

You can use the PORT_ArenaStrdup function.

[Greg Spencer (Chromium), 2011/12/02 18:50:07]

This code went away.

+
+  return result;
 }
 
 // static