Mac: implement <keygen> support, including adding generated cert to the Keychain.

BUG=34607
TEST=KeygenHandlerTest.SmokeTest

Review URL: http://codereview.chromium.org/652137

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@40387 0039d316-1c4b-4281-b951-d872f2087c98

12 files changed, 429 insertions, 98 deletions

diff --git a/net/base/cert_database.h b/net/base/cert_database.h
index e78c22e..3eb2836 100644
--- a/net/base/cert_database.h
+++ b/net/base/cert_database.h
@@ -20,9 +20,14 @@ class CertDatabase {
  public:
   CertDatabase();
 
-  // Extract and Store User (Client) Certificate from a data blob.
-  // Return true if successful.
-  bool AddUserCert(const char* data, int len);
+  // Check whether this is a valid user cert that we have the private key for.
+  // Returns OK or a network error code such as ERR_CERT_CONTAINS_ERRORS.
+  int CheckUserCert(X509Certificate* cert);
+
+  // Store user (client) certificate. Assumes CheckUserCert has already passed.
+  // Returns OK, or ERR_ADD_USER_CERT_FAILED if there was a problem saving to
+  // the platform cert database, or possibly other network error codes.
+  int AddUserCert(X509Certificate* cert);
 
  private:
   void Init();
diff --git a/net/base/cert_database_mac.cc b/net/base/cert_database_mac.cc
index 5caf9db..96ab9e5 100644
--- a/net/base/cert_database_mac.cc
+++ b/net/base/cert_database_mac.cc
@@ -4,21 +4,55 @@
 
 #include "net/base/cert_database.h"
 
+#include <Security/Security.h>
+
 #include "base/logging.h"
+#include "net/base/net_errors.h"
 
 namespace net {
 
 CertDatabase::CertDatabase() {
-  NOTIMPLEMENTED();
 }
 
-bool CertDatabase::AddUserCert(const char* data, int len) {
-  NOTIMPLEMENTED();
-  return false;
+void CertDatabase::Init() {
 }
 
-void CertDatabase::Init() {
-  NOTIMPLEMENTED();
+int CertDatabase::CheckUserCert(X509Certificate* cert) {
+  if (!cert)
+    return ERR_CERT_INVALID;
+  if (cert->HasExpired())
+    return ERR_CERT_DATE_INVALID;
+  if (!cert->SupportsSSLClientAuth())
+    return ERR_CERT_INVALID;
+
+  // Verify the Keychain already has the corresponding private key:
+  SecIdentityRef identity = NULL;
+  OSStatus err = SecIdentityCreateWithCertificate(NULL, cert->os_cert_handle(),
+                                                  &identity);
+  if (err == errSecItemNotFound) {
+    LOG(ERROR) << "CertDatabase couldn't find private key for user cert";
+    return ERR_NO_PRIVATE_KEY_FOR_CERT;
+  }
+  if (err != noErr || !identity) {
+    // TODO(snej): Map the error code more intelligently.
+    return ERR_CERT_INVALID;
+  }
+
+  CFRelease(identity);
+  return OK;
+}
+
+int CertDatabase::AddUserCert(X509Certificate* cert) {
+  OSStatus err = SecCertificateAddToKeychain(cert->os_cert_handle(), NULL);
+  switch(err) {
+    case noErr:
+    case errSecDuplicateItem:
+      return OK;
+    default:
+      LOG(ERROR) << "CertDatabase failed to add cert to keychain: " << err;
+      // TODO(snej): Map the error code more intelligently.
+      return ERR_ERR_ADD_USER_CERT_FAILED;
+  }
 }
 
 }  // namespace net
diff --git a/net/base/cert_database_nss.cc b/net/base/cert_database_nss.cc
index e3c1a09..636224f 100644
--- a/net/base/cert_database_nss.cc
+++ b/net/base/cert_database_nss.cc
@@ -15,6 +15,7 @@
 #include "base/logging.h"
 #include "base/scoped_ptr.h"
 #include "base/nss_util.h"
+#include "net/base/net_errors.h"
 
 namespace net {
 
@@ -22,24 +23,11 @@ CertDatabase::CertDatabase() {
   Init();
 }
 
-bool CertDatabase::AddUserCert(const char* data, int len) {
-  CERTCertificate* cert = NULL;
-  PK11SlotInfo* slot = NULL;
-  std::string nickname;
-  bool is_success = true;
-
-  // Make a copy of "data" since CERT_DecodeCertPackage
-  // might modify it.
-  char* data_copy = new char[len];
-  memcpy(data_copy, data, len);
-
-  // Parse into a certificate structure.
-  cert = CERT_DecodeCertFromPackage(data_copy, len);
-  delete [] data_copy;
-  if (!cert) {
-    LOG(ERROR) << "Couldn't create a temporary certificate";
-    return false;
-  }
+int CertDatabase::CheckUserCert(X509Certificate* cert_obj) {
+  if (!cert_obj)
+    return ERR_CERT_INVALID;
+  if (cert_obj->HasExpired())
+    return ERR_CERT_DATE_INVALID;
 
   // Check if the private key corresponding to the certificate exist
   // We shouldn't accept any random client certificate sent by a CA.
@@ -48,22 +36,25 @@ bool CertDatabase::AddUserCert(const char* data, int len) {
   // also imports the certificate if the private key exists. This
   // doesn't seem to be the case.
 
-  slot = PK11_KeyForCertExists(cert, NULL, NULL);
+  CERTCertificate* cert = cert_obj->os_cert_handle();
+  PK11SlotInfo* slot = PK11_KeyForCertExists(cert, NULL, NULL);
   if (!slot) {
     LOG(ERROR) << "No corresponding private key in store";
-    CERT_DestroyCertificate(cert);
-    return false;
+    return ERR_NO_PRIVATE_KEY_FOR_CERT;
   }
   PK11_FreeSlot(slot);
-  slot = NULL;
 
-  // TODO(gauravsh): We also need to make sure another certificate
-  // doesn't already exist for the same private key.
+  return OK;
+}
+
+int CertDatabase::AddUserCert(X509Certificate* cert_obj) {
+  CERTCertificate* cert = cert_obj->os_cert_handle();
+  PK11SlotInfo* slot = NULL;
+  std::string nickname;
 
   // Create a nickname for this certificate.
   // We use the scheme used by Firefox:
   // --> <subject's common name>'s <issuer's common name> ID.
-  //
 
   std::string username, ca_name;
   char* temp_username = CERT_GetCommonName(&cert->subject);
@@ -81,14 +72,12 @@ bool CertDatabase::AddUserCert(const char* data, int len) {
   slot = PK11_ImportCertForKey(cert,
                                const_cast<char*>(nickname.c_str()),
                                NULL);
-  if (slot) {
-    PK11_FreeSlot(slot);
-  } else {
+  if (!slot) {
     LOG(ERROR) << "Couldn't import user certificate.";
-    is_success = false;
+    return ERR_ERR_ADD_USER_CERT_FAILED;
   }
-  CERT_DestroyCertificate(cert);
-  return is_success;
+  PK11_FreeSlot(slot);
+  return OK;
 }
 
 void CertDatabase::Init() {
diff --git a/net/base/cert_database_win.cc b/net/base/cert_database_win.cc
index 5caf9db..7e9d862 100644
--- a/net/base/cert_database_win.cc
+++ b/net/base/cert_database_win.cc
@@ -5,6 +5,7 @@
 #include "net/base/cert_database.h"
 
 #include "base/logging.h"
+#include "net/base/net_errors.h"
 
 namespace net {
 
@@ -12,9 +13,14 @@ CertDatabase::CertDatabase() {
   NOTIMPLEMENTED();
 }
 
-bool CertDatabase::AddUserCert(const char* data, int len) {
+int CertDatabase::CheckUserCert(X509Certificate* cert) {
   NOTIMPLEMENTED();
-  return false;
+  return ERR_NOT_IMPLEMENTED;
+}
+
+int CertDatabase::AddUserCert(X509Certificate* cert) {
+  NOTIMPLEMENTED();
+  return ERR_NOT_IMPLEMENTED;
 }
 
 void CertDatabase::Init() {
diff --git a/net/base/keygen_handler.h b/net/base/keygen_handler.h
index 346b577..1ed023e 100644
--- a/net/base/keygen_handler.h
+++ b/net/base/keygen_handler.h
@@ -10,18 +10,36 @@
 namespace net {
 
 // This class handles keypair generation for generating client
-// certificates via the Netscape <keygen> tag.
+// certificates via the <keygen> tag.
+// <http://dev.w3.org/html5/spec/Overview.html#the-keygen-element>
+// <https://developer.mozilla.org/En/HTML/HTML_Extensions/KEYGEN_Tag>
 
 class KeygenHandler {
  public:
-  KeygenHandler(int key_size_index, const std::string& challenge);
+  // Creates a handler that will generate a key with the given key size
+  // and incorporate the |challenge| into the Netscape SPKAC structure.
+  inline KeygenHandler(int key_size_in_bits, const std::string& challenge);
+
+  // Actually generates the key-pair and the cert request (SPKAC), and returns
+  // a base64-encoded string suitable for use as the form value of <keygen>.
   std::string GenKeyAndSignChallenge();
 
+  // Exposed only for unit tests.
+  void set_stores_key(bool store) { stores_key_ = store;}
+
  private:
-  int key_size_index_;
-  std::string challenge_;
+  int key_size_in_bits_;  // key size in bits (usually 2048)
+  std::string challenge_;  // challenge string sent by server
+  bool stores_key_;  // should the generated key-pair be stored persistently?
 };
 
+KeygenHandler::KeygenHandler(int key_size_in_bits,
+                             const std::string& challenge)
+    : key_size_in_bits_(key_size_in_bits),
+      challenge_(challenge),
+      stores_key_(true) {
+}
+
 }  // namespace net
 
 #endif  // NET_BASE_KEYGEN_HANDLER_H_
diff --git a/net/base/keygen_handler_mac.cc b/net/base/keygen_handler_mac.cc
index f6b4551..e5fd619 100644
--- a/net/base/keygen_handler_mac.cc
+++ b/net/base/keygen_handler_mac.cc
@@ -4,20 +4,249 @@
 
 #include "net/base/keygen_handler.h"
 
+#include <Security/SecAsn1Coder.h>
+#include <Security/SecAsn1Templates.h>
+#include <Security/Security.h>
+
+#include "base/base64.h"
 #include "base/logging.h"
+#include "base/scoped_cftyperef.h"
+
+// These are in Security.framework but not declared in a public header.
+extern const SecAsn1Template kSecAsn1AlgorithmIDTemplate[];
+extern const SecAsn1Template kSecAsn1SubjectPublicKeyInfoTemplate[];
 
 namespace net {
 
-KeygenHandler::KeygenHandler(int key_size_index,
-                             const std::string& challenge)
-    : key_size_index_(key_size_index),
-      challenge_(challenge) {
-  NOTIMPLEMENTED();
-}
+// Declarations of Netscape keygen cert structures for ASN.1 encoding:
+
+struct PublicKeyAndChallenge {
+  CSSM_X509_SUBJECT_PUBLIC_KEY_INFO spki;
+  CSSM_DATA challenge_string;
+};
+
+static const SecAsn1Template kPublicKeyAndChallengeTemplate[] = {
+  {
+    SEC_ASN1_SEQUENCE,
+    0,
+    NULL,
+    sizeof(PublicKeyAndChallenge)
+  },
+  {
+    SEC_ASN1_INLINE,
+    offsetof(PublicKeyAndChallenge, spki),
+    kSecAsn1SubjectPublicKeyInfoTemplate
+  },
+  {
+    SEC_ASN1_INLINE,
+    offsetof(PublicKeyAndChallenge, challenge_string),
+    kSecAsn1IA5StringTemplate
+  },
+  {
+    0
+  }
+};
+
+struct SignedPublicKeyAndChallenge {
+  PublicKeyAndChallenge pkac;
+  CSSM_X509_ALGORITHM_IDENTIFIER signature_algorithm;
+  CSSM_DATA signature;
+};
+
+static const SecAsn1Template kSignedPublicKeyAndChallengeTemplate[] = {
+  {
+    SEC_ASN1_SEQUENCE,
+    0,
+    NULL,
+    sizeof(SignedPublicKeyAndChallenge)
+  },
+  {
+    SEC_ASN1_INLINE,
+    offsetof(SignedPublicKeyAndChallenge, pkac),
+    kPublicKeyAndChallengeTemplate
+  },
+  {
+    SEC_ASN1_INLINE,
+    offsetof(SignedPublicKeyAndChallenge, signature_algorithm),
+    kSecAsn1AlgorithmIDTemplate
+  },
+  {
+    SEC_ASN1_BIT_STRING,
+    offsetof(SignedPublicKeyAndChallenge, signature)
+  },
+  {
+    0
+  }
+};
+
+
+static OSStatus CreateRSAKeyPair(int size_in_bits,
+                                 SecKeyRef* out_pub_key,
+                                 SecKeyRef* out_priv_key);
+static OSStatus SignData(CSSM_DATA data,
+                         SecKeyRef private_key,
+                         CSSM_DATA* signature);
+
 
 std::string KeygenHandler::GenKeyAndSignChallenge() {
-  NOTIMPLEMENTED();
-  return std::string();
+  std::string result;
+  OSStatus err;
+  SecKeyRef public_key = NULL;
+  SecKeyRef private_key = NULL;
+  SecAsn1CoderRef coder = NULL;
+  CSSM_DATA signature = {0, NULL};
+
+  {
+    // Create the key-pair.
+    err = CreateRSAKeyPair(key_size_in_bits_, &public_key, &private_key);
+    if (err)
+      goto failure;
+
+    // Get the public key data (DER sequence of modulus, exponent).
+    CFDataRef key_data = NULL;
+    err = SecKeychainItemExport(public_key, kSecFormatBSAFE, 0, NULL,
+                                &key_data);
+    if (err)
+      goto failure;
+    scoped_cftyperef<CFDataRef> scoped_key_data(key_data);
+
+    // Create an ASN.1 encoder.
+    err = SecAsn1CoderCreate(&coder);
+    if (err)
+      goto failure;
+
+    // Fill in and DER-encode the PublicKeyAndChallenge:
+    SignedPublicKeyAndChallenge spkac;
+    memset(&spkac, 0, sizeof(spkac));
+    spkac.pkac.spki.algorithm.algorithm = CSSMOID_RSA;
+    spkac.pkac.spki.subjectPublicKey.Length =
+        CFDataGetLength(key_data) * 8;  // interpreted as a _bit_ count
+    spkac.pkac.spki.subjectPublicKey.Data =
+        const_cast<uint8_t*>(CFDataGetBytePtr(key_data));
+    spkac.pkac.challenge_string.Length = challenge_.length();
+    spkac.pkac.challenge_string.Data =
+        reinterpret_cast<uint8_t*>(const_cast<char*>(challenge_.data()));
+
+    CSSM_DATA encoded;
+    err = SecAsn1EncodeItem(coder, &spkac.pkac,
+                            kPublicKeyAndChallengeTemplate, &encoded);
+    if (err)
+      goto failure;
+
+    // Compute a signature of the result:
+    err = SignData(encoded, private_key, &signature);
+    if (err)
+      goto failure;
+    spkac.signature.Data = signature.Data;
+    spkac.signature.Length = signature.Length * 8;  // a _bit_ count
+    spkac.signature_algorithm.algorithm = CSSMOID_MD5WithRSA;
+    // TODO(snej): MD5 is weak. Can we use SHA1 instead?
+    // See <https://bugzilla.mozilla.org/show_bug.cgi?id=549460>
+
+    // DER-encode the entire SignedPublicKeyAndChallenge:
+    err = SecAsn1EncodeItem(coder, &spkac,
+                            kSignedPublicKeyAndChallengeTemplate, &encoded);
+    if (err)
+      goto failure;
+
+    // Base64 encode the result.
+    std::string input(reinterpret_cast<char*>(encoded.Data), encoded.Length);
+    base::Base64Encode(input, &result);
+  }
+
+failure:
+  if (err) {
+    LOG(ERROR) << "SSL Keygen failed! OSStatus = " << err;
+  } else {
+    LOG(INFO) << "SSL Keygen succeeded! Output is: " << result;
+  }
+
+  // Remove keys from keychain if asked to during unit testing:
+  if (!stores_key_) {
+    if (public_key)
+      SecKeychainItemDelete(reinterpret_cast<SecKeychainItemRef>(public_key));
+    if (private_key)
+      SecKeychainItemDelete(reinterpret_cast<SecKeychainItemRef>(private_key));
+  }
+
+  // Clean up:
+  free(signature.Data);
+  if (coder)
+    SecAsn1CoderRelease(coder);
+  if (public_key)
+    CFRelease(public_key);
+  if (private_key)
+    CFRelease(private_key);
+  return result;
+}
+
+
+static OSStatus CreateRSAKeyPair(int size_in_bits,
+                                 SecKeyRef* out_pub_key,
+                                 SecKeyRef* out_priv_key) {
+  OSStatus err;
+  SecKeychainRef keychain;
+  err = SecKeychainCopyDefault(&keychain);
+  if (err)
+    return err;
+  scoped_cftyperef<SecKeychainRef> scoped_keychain(keychain);
+  return SecKeyCreatePair(
+      keychain,
+      CSSM_ALGID_RSA,
+      size_in_bits,
+      0LL,
+      // public key usage and attributes:
+      CSSM_KEYUSE_ENCRYPT | CSSM_KEYUSE_VERIFY | CSSM_KEYUSE_WRAP,
+      CSSM_KEYATTR_EXTRACTABLE | CSSM_KEYATTR_PERMANENT,
+      // private key usage and attributes:
+      CSSM_KEYUSE_DECRYPT | CSSM_KEYUSE_SIGN | CSSM_KEYUSE_UNWRAP,                               // private key
+      CSSM_KEYATTR_EXTRACTABLE | CSSM_KEYATTR_PERMANENT |
+          CSSM_KEYATTR_SENSITIVE,
+      NULL,
+      out_pub_key, out_priv_key);
+}
+
+static OSStatus CreateSignatureContext(SecKeyRef key,
+                                       CSSM_ALGORITHMS algorithm,
+                                       CSSM_CC_HANDLE* out_cc_handle) {
+  OSStatus err;
+  const CSSM_ACCESS_CREDENTIALS* credentials = NULL;
+  err = SecKeyGetCredentials(key,
+                             CSSM_ACL_AUTHORIZATION_SIGN,
+                             kSecCredentialTypeDefault,
+                             &credentials);
+  if (err)
+      return err;
+
+  CSSM_CSP_HANDLE csp_handle = 0;
+  err = SecKeyGetCSPHandle(key, &csp_handle);
+  if (err)
+    return err;
+
+  const CSSM_KEY* cssm_key = NULL;
+  err = SecKeyGetCSSMKey(key, &cssm_key);
+  if (err)
+    return err;
+
+  return CSSM_CSP_CreateSignatureContext(csp_handle,
+                                         algorithm,
+                                         credentials,
+                                         cssm_key,
+                                         out_cc_handle);
+}
+
+static OSStatus SignData(CSSM_DATA data,
+                         SecKeyRef private_key,
+                         CSSM_DATA* signature) {
+  CSSM_CC_HANDLE cc_handle;
+  OSStatus err = CreateSignatureContext(private_key,
+                                        CSSM_ALGID_MD5WithRSA,
+                                        &cc_handle);
+  if (err)
+    return err;
+  err = CSSM_SignData(cc_handle, &data, 1, CSSM_ALGID_NONE, signature);
+  CSSM_DeleteContext(cc_handle);
+  return err;
 }
 
 }  // namespace net
diff --git a/net/base/keygen_handler_nss.cc b/net/base/keygen_handler_nss.cc
index 6c17298..d8d9acb 100644
--- a/net/base/keygen_handler_nss.cc
+++ b/net/base/keygen_handler_nss.cc
@@ -7,11 +7,11 @@
 #include <pk11pub.h>
 #include <secmod.h>
 #include <ssl.h>
-#include <nssb64.h>    // NSSBase64_EncodeItem()
 #include <secder.h>    // DER_Encode()
 #include <cryptohi.h>  // SEC_DerSignData()
 #include <keyhi.h>     // SECKEY_CreateSubjectPublicKeyInfo()
 
+#include "base/base64.h"
 #include "base/nss_util.h"
 #include "base/logging.h"
 
@@ -51,21 +51,6 @@ DERTemplate CERTPublicKeyAndChallengeTemplate[] = {
   { 0, }
 };
 
-// This maps displayed strings indicating level of keysecurity in the <keygen>
-// menu to the key size in bits.
-// TODO(gauravsh): Should this mapping be moved else where?
-int RSAkeySizeMap[] = {2048, 1024};
-
-KeygenHandler::KeygenHandler(int key_size_index,
-                             const std::string& challenge)
-    : key_size_index_(key_size_index),
-      challenge_(challenge) {
-  if (key_size_index_ < 0 ||
-      key_size_index_ >=
-          static_cast<int>(sizeof(RSAkeySizeMap) / sizeof(RSAkeySizeMap[0])))
-    key_size_index_ = 0;
-}
-
 // This function is largely copied from the Firefox's
 // <keygen> implementation in security/manager/ssl/src/nsKeygenHandler.cpp
 // FIXME(gauravsh): Do we need a copy of the Mozilla license here?
@@ -73,7 +58,6 @@ KeygenHandler::KeygenHandler(int key_size_index,
 std::string KeygenHandler::GenKeyAndSignChallenge() {
   // Key pair generation mechanism - only RSA is supported at present.
   PRUint32 keyGenMechanism = CKM_RSA_PKCS_KEY_PAIR_GEN;  // from nss/pkcs11t.h
-  char *keystring = NULL;  // Temporary store for result/
 
   // Temporary structures used for generating the result
   // in the right format.
@@ -107,7 +91,7 @@ std::string KeygenHandler::GenKeyAndSignChallenge() {
 
   switch (keyGenMechanism) {
     case CKM_RSA_PKCS_KEY_PAIR_GEN:
-      rsaKeyGenParams.keySizeInBits = RSAkeySizeMap[key_size_index_];
+      rsaKeyGenParams.keySizeInBits = key_size_in_bits_;
       rsaKeyGenParams.pe = DEFAULT_RSA_PUBLIC_EXPONENT;
       keyGenParams = &rsaKeyGenParams;
 
@@ -202,18 +186,14 @@ std::string KeygenHandler::GenKeyAndSignChallenge() {
   }
 
   // Convert the signed public key and challenge into base64/ascii.
-  keystring = NSSBase64_EncodeItem(arena,
-                                   NULL,  // NSS will allocate a buffer for us.
-                                   0,
-                                   &signedItem);
-  if (!keystring) {
+  if (!base::Base64Encode(std::string(reinterpret_cast<char*>(signedItem.data),
+                                      signedItem.len),
+                          &result_blob)) {
     LOG(ERROR) << "Couldn't convert signed public key into base64";
     isSuccess = false;
     goto failure;
   }
 
-  result_blob = keystring;
-
  failure:
   if (!isSuccess) {
     LOG(ERROR) << "SSL Keygen failed!";
@@ -223,11 +203,12 @@ std::string KeygenHandler::GenKeyAndSignChallenge() {
 
   // Do cleanups
   if (privateKey) {
-    // TODO(gauravsh): We still need to maintain the private key because it's
-    // used for certificate enrollment checks.
-
-    // PK11_DestroyTokenObject(privateKey->pkcs11Slot,privateKey->pkcs11ID);
-    // SECKEY_DestroyPrivateKey(privateKey);
+    if (!isSuccess || !stores_key_) {
+      PK11_DestroyTokenObject(privateKey->pkcs11Slot,privateKey->pkcs11ID);
+      SECKEY_DestroyPrivateKey(privateKey);
+    }
+    // On successful keygen we need to keep the private key, of course,
+    // or we won't be able to use the client certificate.
   }
 
   if (publicKey) {
diff --git a/net/base/keygen_handler_unittest.cc b/net/base/keygen_handler_unittest.cc
new file mode 100644
index 0000000..8b0fb09
--- /dev/null
+++ b/net/base/keygen_handler_unittest.cc
@@ -0,0 +1,56 @@
+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "net/base/keygen_handler.h"
+
+#include <string>
+
+#include "base/base64.h"
+#include "base/logging.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace net {
+
+namespace {
+
+TEST(KeygenHandlerTest, SmokeTest) {
+  KeygenHandler handler(2048, "some challenge");
+  handler.set_stores_key(false);  // Don't leave the key-pair behind
+  std::string result = handler.GenKeyAndSignChallenge();
+  LOG(INFO) << "KeygenHandler produced: " << result;
+  ASSERT_GT(result.length(), 0U);
+
+  // Verify it's valid base64:
+  std::string spkac;
+  ASSERT_TRUE(base::Base64Decode(result, &spkac));
+  // In lieu of actually parsing and validating the DER data,
+  // just check that it exists and has a reasonable length.
+  // (It's almost always 590 bytes, but the DER encoding of the random key
+  // and signature could sometimes be a few bytes different.)
+  ASSERT_GE(spkac.length(), 580U);
+  ASSERT_LE(spkac.length(), 600U);
+
+  // NOTE:
+  // The value of |result| can be validated by prefixing 'SPKAC=' to it
+  // and piping it through
+  //   openssl spkac -verify
+  // whose output should look like:
+  //   Netscape SPKI:
+  //     Public Key Algorithm: rsaEncryption
+  //     RSA Public Key: (2048 bit)
+  //     Modulus (2048 bit):
+  //         00:b6:cc:14:c9:43:b5:2d:51:65:7e:11:8b:80:9e: .....
+  //     Exponent: 65537 (0x10001)
+  //     Challenge String: some challenge
+  //     Signature Algorithm: md5WithRSAEncryption
+  //         92:f3:cc:ff:0b:d3:d0:4a:3a:4c:ba:ff:d6:38:7f:a5:4b:b5: .....
+  //   Signature OK
+  //
+  // The value of |spkac| can be ASN.1-parsed with:
+  //    openssl asn1parse -inform DER
+}
+
+}  // namespace
+
+}  // namespace net
diff --git a/net/base/keygen_handler_win.cc b/net/base/keygen_handler_win.cc
index f6b4551..cda0307 100644
--- a/net/base/keygen_handler_win.cc
+++ b/net/base/keygen_handler_win.cc
@@ -8,13 +8,6 @@
 
 namespace net {
 
-KeygenHandler::KeygenHandler(int key_size_index,
-                             const std::string& challenge)
-    : key_size_index_(key_size_index),
-      challenge_(challenge) {
-  NOTIMPLEMENTED();
-}
-
 std::string KeygenHandler::GenKeyAndSignChallenge() {
   NOTIMPLEMENTED();
   return std::string();
diff --git a/net/base/net_error_list.h b/net/base/net_error_list.h
index b676986..4030379 100644
--- a/net/base/net_error_list.h
+++ b/net/base/net_error_list.h
@@ -195,6 +195,8 @@ NET_ERROR(CERT_AUTHORITY_INVALID, -202)
 //
 // MSDN describes this error as follows:
 //   "The SSL certificate contains errors."
+// NOTE: It's unclear how this differs from ERR_CERT_INVALID. For consistency,
+// use that code instead of this one from now on.
 //
 NET_ERROR(CERT_CONTAINS_ERRORS, -203)
 
@@ -349,6 +351,14 @@ NET_ERROR(CACHE_RACE, -406)
 
 // The server's response was insecure (e.g. there was a cert error).
 NET_ERROR(INSECURE_RESPONSE, -501)
+
+// The server responded to a <keygen> with a generated client cert that we
+// don't have the matching private key for.
+NET_ERROR(NO_PRIVATE_KEY_FOR_CERT, -502)
+
+// An error adding to the OS certificate database (e.g. OS X Keychain).
+NET_ERROR(ERR_ADD_USER_CERT_FAILED, -503)
+
 //
 // The FTP PASV command failed.
 NET_ERROR(FTP_PASV_COMMAND_FAILED, -600)
diff --git a/net/base/x509_certificate_nss.cc b/net/base/x509_certificate_nss.cc
index 05ed979..b25688e 100644
--- a/net/base/x509_certificate_nss.cc
+++ b/net/base/x509_certificate_nss.cc
@@ -617,11 +617,16 @@ X509Certificate::OSCertHandle X509Certificate::CreateOSCertHandleFromBytes(
     const char* data, int length) {
   base::EnsureNSSInit();
 
-  SECItem der_cert;
-  der_cert.data = reinterpret_cast<unsigned char*>(const_cast<char*>(data));
-  der_cert.len = length;
-  return CERT_NewTempCertificate(CERT_GetDefaultCertDB(), &der_cert,
-                                 NULL, PR_FALSE, PR_TRUE);
+  // Make a copy of |data| since CERT_DecodeCertPackage might modify it.
+  char* data_copy = new char[length];
+  memcpy(data_copy, data, length);
+
+  // Parse into a certificate structure.
+  CERTCertificate* cert = CERT_DecodeCertFromPackage(data_copy, length);
+  delete [] data_copy;
+  if (!cert)
+    LOG(ERROR) << "Couldn't parse a certificate from " << length << " bytes";
+  return cert;
 }
 
 // static
diff --git a/net/net.gyp b/net/net.gyp
index dffd51f..1f89865 100644
--- a/net/net.gyp
+++ b/net/net.gyp
@@ -604,6 +604,7 @@
         'base/gzip_filter_unittest.cc',
         'base/host_cache_unittest.cc',
         'base/host_resolver_impl_unittest.cc',
+        'base/keygen_handler_unittest.cc',
         'base/load_log_unittest.cc',
         'base/load_log_unittest.h',
         'base/load_log_util_unittest.cc',
@@ -718,9 +719,13 @@
             }],
           ],
         }],
-        # This is needed to trigger the dll copy step on windows.
-        # TODO(mark): Specifying this here shouldn't be necessary.
         [ 'OS == "win"', {
+            'sources!': [
+              # Remove next line when KeygenHandler is implemented for Windows.
+              'base/keygen_handler_unittest.cc',
+            ],
+            # This is needed to trigger the dll copy step on windows.
+            # TODO(mark): Specifying this here shouldn't be necessary.
             'dependencies': [
               '../third_party/icu/icu.gyp:icudata',
             ],