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

19 files changed, 680 insertions, 106 deletions

diff --git a/chrome/app/generated_resources.grd b/chrome/app/generated_resources.grd
index 6ba6970..bc8b8c1 100644
--- a/chrome/app/generated_resources.grd
+++ b/chrome/app/generated_resources.grd
@@ -2941,6 +2941,20 @@ each locale. -->
         &View
       </message>
 
+      <!-- Add Client Certificate Dialog -->
+      <message name="IDS_ADD_CERT_DIALOG_ADD" desc="Title of the button that adds the certificate in the Add Client Certificate dialog">
+        Add Certificate
+      </message>
+      <message name="IDS_ADD_CERT_FAILURE_TITLE" desc="Title of add-certificate error dialog">
+        Client Certificate Error
+      </message>
+      <message name="IDS_ADD_CERT_ERR_INVALID_CERT" desc="Error message when the server returns an invalid client certificate after key generation">
+        The server returned an invalid client certificate.
+      </message>
+      <message name="IDS_ADD_CERT_ERR_FAILED" desc="Generic error message for a failure to add the generated certificate to the cert store/keychain">
+        There was an error while trying to store the client certificate.
+      </message>
+
       <!-- Basic Auth Dialog -->
       <message name="IDS_LOGIN_DIALOG_TITLE" desc="String to be displayed in the title bar of the login prompt dialog">
         Authentication Required
diff --git a/chrome/browser/renderer_host/resource_message_filter.cc b/chrome/browser/renderer_host/resource_message_filter.cc
index b7c75e4..94f60a8 100644
--- a/chrome/browser/renderer_host/resource_message_filter.cc
+++ b/chrome/browser/renderer_host/resource_message_filter.cc
@@ -1332,10 +1332,23 @@ void ResourceMessageFilter::OnKeygen(uint32 key_size_index,
                                      const std::string& challenge_string,
                                      const GURL& url,
                                      std::string* signed_public_key) {
-  scoped_ptr<net::KeygenHandler> keygen_handler(
-      new net::KeygenHandler(key_size_index,
-                             challenge_string));
-  *signed_public_key = keygen_handler->GenKeyAndSignChallenge();
+  // Map displayed strings indicating level of keysecurity in the <keygen>
+  // menu to the key size in bits. (See SSLKeyGeneratorChromium.cpp in WebCore.)
+  int key_size_in_bits;
+  switch (key_size_index) {
+    case 0:
+      key_size_in_bits = 2048;
+      break;
+    case 1:
+      key_size_in_bits = 1024;
+      break;
+    default:
+      DCHECK(false) << "Illegal key_size_index " << key_size_index;
+      *signed_public_key = std::string();
+      return;
+  }
+  net::KeygenHandler keygen_handler(key_size_in_bits, challenge_string);
+  *signed_public_key = keygen_handler.GenKeyAndSignChallenge();
 }
 
 void ResourceMessageFilter::OnTranslateText(
diff --git a/chrome/browser/renderer_host/x509_user_cert_resource_handler.cc b/chrome/browser/renderer_host/x509_user_cert_resource_handler.cc
index 0c200a9..3dcc8ee 100644
--- a/chrome/browser/renderer_host/x509_user_cert_resource_handler.cc
+++ b/chrome/browser/renderer_host/x509_user_cert_resource_handler.cc
@@ -10,9 +10,9 @@
 #include "chrome/browser/download/download_file.h"
 #include "chrome/browser/renderer_host/resource_dispatcher_host.h"
 #include "chrome/browser/renderer_host/resource_dispatcher_host_request_info.h"
+#include "chrome/browser/ssl/ssl_add_cert_handler.h"
 #include "chrome/common/resource_response.h"
 #include "chrome/common/url_constants.h"
-#include "net/base/cert_database.h"
 #include "net/base/io_buffer.h"
 #include "net/base/mime_sniffer.h"
 #include "net/base/mime_util.h"
@@ -91,12 +91,18 @@ bool X509UserCertResourceHandler::OnResponseCompleted(
     int request_id,
     const URLRequestStatus& urs,
     const std::string& sec_info) {
+  if (urs.status() != URLRequestStatus::SUCCESS)
+    return false;
+
   // TODO(gauravsh): Verify that 'request_id' was actually a keygen form post
   // and only then import the certificate.
-  scoped_ptr<net::CertDatabase> cert_db(new net::CertDatabase());
   AssembleResource();
-
-  return cert_db->AddUserCert(resource_buffer_->data(), content_length_);
+  scoped_refptr<net::X509Certificate> cert =
+      net::X509Certificate::CreateFromBytes(resource_buffer_->data(),
+                                            content_length_);
+  // The handler will run the UI and delete itself when it's finished.
+  new SSLAddCertHandler(request_, cert);
+  return true;
 }
 
 void X509UserCertResourceHandler::OnRequestClosed() {
diff --git a/chrome/browser/ssl/ssl_add_cert_handler.cc b/chrome/browser/ssl/ssl_add_cert_handler.cc
new file mode 100644
index 0000000..8c75d7d
--- /dev/null
+++ b/chrome/browser/ssl/ssl_add_cert_handler.cc
@@ -0,0 +1,69 @@
+// 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 "chrome/browser/ssl/ssl_add_cert_handler.h"
+
+#include "app/l10n_util.h"
+#include "chrome/browser/browser_list.h"
+#include "chrome/browser/browser.h"
+#include "chrome/browser/browser_window.h"
+#include "chrome/browser/chrome_thread.h"
+#include "chrome/common/platform_util.h"
+#include "grit/generated_resources.h"
+#include "net/base/cert_database.h"
+#include "net/base/net_errors.h"
+#include "net/base/x509_certificate.h"
+#include "net/url_request/url_request.h"
+
+SSLAddCertHandler::SSLAddCertHandler(URLRequest* request,
+                                     net::X509Certificate* cert)
+    : cert_(cert) {
+  // Stay alive until the UI completes and Finished() is called.
+  AddRef();
+  ChromeThread::PostTask(
+      ChromeThread::UI, FROM_HERE,
+      NewRunnableMethod(this, &SSLAddCertHandler::RunUI));
+}
+
+void SSLAddCertHandler::RunUI() {
+  int cert_error;
+  {
+    net::CertDatabase db;
+    cert_error = db.CheckUserCert(cert_);
+  }
+  if (cert_error != net::OK) {
+    // TODO(snej): Map cert_error to a more specific error message.
+    ShowError(l10n_util::GetStringUTF16(IDS_ADD_CERT_ERR_INVALID_CERT));
+    Finished(false);
+    return;
+  }
+  AskToAddCert();
+}
+
+#if !defined(OS_MACOSX)
+void SSLAddCertHandler::AskToAddCert() {
+  // TODO(snej): Someone should add Windows and GTK implementations with UI.
+  Finished(true);
+}
+#endif
+
+void SSLAddCertHandler::Finished(bool add_cert) {
+  if (add_cert) {
+    net::CertDatabase db;
+    int cert_error = db.AddUserCert(cert_);
+    if (cert_error != net::OK) {
+      // TODO(snej): Map cert_error to a more specific error message.
+      ShowError(l10n_util::GetStringUTF16(IDS_ADD_CERT_ERR_FAILED));
+    }
+  }
+  Release();
+}
+
+void SSLAddCertHandler::ShowError(const string16& error) {
+  Browser* browser = BrowserList::GetLastActive();
+  platform_util::SimpleErrorBox(
+      browser ? browser->window()->GetNativeHandle() : NULL,
+      l10n_util::GetStringUTF16(IDS_ADD_CERT_FAILURE_TITLE),
+      error);
+}
diff --git a/chrome/browser/ssl/ssl_add_cert_handler.h b/chrome/browser/ssl/ssl_add_cert_handler.h
new file mode 100644
index 0000000..0680128
--- /dev/null
+++ b/chrome/browser/ssl/ssl_add_cert_handler.h
@@ -0,0 +1,50 @@
+// 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.
+
+#ifndef CHROME_BROWSER_SSL_SSL_ADD_CERT_HANDLER_H_
+#define CHROME_BROWSER_SSL_SSL_ADD_CERT_HANDLER_H_
+
+#include "base/basictypes.h"
+#include "base/ref_counted.h"
+#include "base/string16.h"
+
+namespace net {
+class X509Certificate;
+}
+class URLRequest;
+
+// This class handles adding a newly-generated client cert. It ensures there's a
+// private key for the cert, displays the cert to the user, and adds it upon
+// user approval.
+// It is self-owned and deletes itself when finished.
+class SSLAddCertHandler : public base::RefCountedThreadSafe<SSLAddCertHandler> {
+ public:
+  SSLAddCertHandler(URLRequest* request, net::X509Certificate* cert);
+
+  net::X509Certificate* cert()  { return cert_; }
+
+  // The platform-specific code calls this when it's done, to clean up.
+  // If |addCert| is true, the cert will be added to the CertDatabase.
+  void Finished(bool add_cert);
+
+ private:
+  friend class base::RefCountedThreadSafe<SSLAddCertHandler>;
+
+  // Runs the user interface. Called on the UI thread. Calls AskToAddCert.
+  void RunUI();
+
+  // Platform-specific code that asks the user whether to add the cert.
+  // Called on the UI thread.
+  void AskToAddCert();
+
+  // Utility to display an error message in a dialog box.
+  void ShowError(const string16& error);
+
+  // The cert to add.
+  scoped_refptr<net::X509Certificate> cert_;
+
+  DISALLOW_COPY_AND_ASSIGN(SSLAddCertHandler);
+};
+
+#endif  // CHROME_BROWSER_SSL_SSL_ADD_CERT_HANDLER_H_
diff --git a/chrome/browser/ssl/ssl_add_cert_handler_mac.mm b/chrome/browser/ssl/ssl_add_cert_handler_mac.mm
new file mode 100644
index 0000000..75b8142
--- /dev/null
+++ b/chrome/browser/ssl/ssl_add_cert_handler_mac.mm
@@ -0,0 +1,88 @@
+// 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 "ssl_add_cert_handler.h"
+
+#include <SecurityInterface/SFCertificatePanel.h>
+#include <SecurityInterface/SFCertificateView.h>
+
+#include "app/l10n_util_mac.h"
+#include "base/scoped_nsobject.h"
+#include "chrome/common/logging_chrome.h"
+#include "chrome/browser/browser_list.h"
+#include "chrome/browser/browser.h"
+#include "chrome/browser/browser_window.h"
+#include "grit/generated_resources.h"
+#include "net/base/x509_certificate.h"
+
+@interface SSLAddCertHandlerCocoa : NSObject
+{
+  scoped_refptr<SSLAddCertHandler> handler_;
+}
+
+- (id)initWithHandler:(SSLAddCertHandler*)handler;
+- (void)askToAddCert;
+@end
+
+
+void SSLAddCertHandler::AskToAddCert() {
+  [[[SSLAddCertHandlerCocoa alloc] initWithHandler: this] askToAddCert];
+  // The new object will release itself when the sheet ends.
+}
+
+
+// The actual implementation of the add-client-cert handler is an Obj-C class.
+@implementation SSLAddCertHandlerCocoa
+
+- (id)initWithHandler:(SSLAddCertHandler*)handler {
+  DCHECK(handler && handler->cert());
+  self = [super init];
+  if (self) {
+    handler_ = handler;
+  }
+  return self;
+}
+
+- (void)sheetDidEnd:(SFCertificatePanel*)panel
+         returnCode:(NSInteger)returnCode
+            context:(void*)context {
+  [panel orderOut:self];
+  [panel autorelease];
+  handler_->Finished(returnCode == NSOKButton);
+  [self release];
+}
+
+- (void)askToAddCert {
+  NSWindow* parentWindow = NULL;
+  Browser* browser = BrowserList::GetLastActive();
+  // TODO(snej): Can I get the Browser that issued the request?
+  if (browser) {
+    parentWindow = browser->window()->GetNativeHandle();
+    if ([parentWindow attachedSheet])
+      parentWindow = nil;
+  }
+
+  // Create the cert panel, which will be released in my -sheetDidEnd: method.
+  SFCertificatePanel* panel = [[SFCertificatePanel alloc] init];
+  [panel setDefaultButtonTitle:l10n_util::GetNSString(IDS_ADD_CERT_DIALOG_ADD)];
+  [panel setAlternateButtonTitle:l10n_util::GetNSString(IDS_CANCEL)];
+  SecCertificateRef cert = handler_->cert()->os_cert_handle();
+  NSArray* certs = [NSArray arrayWithObject: (id)cert];
+
+  if (parentWindow) {
+    // Open the cert panel as a sheet on the browser window.
+    [panel beginSheetForWindow:parentWindow
+                 modalDelegate:self
+                didEndSelector:@selector(sheetDidEnd:returnCode:context:)
+                   contextInfo:NULL
+                  certificates:certs
+                     showGroup:NO];
+  } else {
+    // No available browser window, so run independently as a (blocking) dialog.
+    int returnCode = [panel runModalForCertificates:certs showGroup:NO];
+    [self sheetDidEnd:panel returnCode:returnCode context:NULL];
+  }
+}
+
+@end
diff --git a/chrome/chrome_browser.gypi b/chrome/chrome_browser.gypi
index fe76e2e..001b67a 100755
--- a/chrome/chrome_browser.gypi
+++ b/chrome/chrome_browser.gypi
@@ -1746,6 +1746,9 @@
         'browser/spellchecker_mac.mm',
         'browser/spellchecker_platform_engine.h',
         'browser/spellchecker_win.cc',
+        'browser/ssl/ssl_add_cert_handler.cc',
+        'browser/ssl/ssl_add_cert_handler.h',
+        'browser/ssl/ssl_add_cert_handler_mac.mm',
         'browser/ssl/ssl_blocking_page.cc',
         'browser/ssl/ssl_blocking_page.h',
         'browser/ssl/ssl_cert_error_handler.cc',
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',
             ],