Refactor client cert private key handling.

chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm

Index: chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm
diff --git a/chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm b/chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm
index 2143bfe73cfab2baec6bd910aff7db19becc68a8..ceb4c888005af0d8edaf8a50ddf5c436d593de3b 100644
--- a/chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm
+++ b/chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm
@@ -72,6 +72,13 @@ class SSLClientAuthObserverCocoaBridge : public SSLClientAuthObserver,
                       afterDelay:0];
   }
 
+  void GotPrivateKey(
+      base::scoped_nsobject<SSLClientCertificateSelectorCocoa> controller_ref,
+      net::X509Certificate* cert,
+      scoped_refptr<net::SSLPrivateKey> private_key) {
+    CertificateSelected(cert, private_key.get());
+  }
+
  private:
   SSLClientCertificateSelectorCocoa* controller_;  // weak
 };
@@ -81,7 +88,7 @@ namespace chrome {
 void ShowSSLClientCertificateSelector(
     content::WebContents* contents,
     net::SSLCertRequestInfo* cert_request_info,
-    net::CertificateList client_certs,
+    net::ClientCertIdentityList client_certs,
     std::unique_ptr<content::ClientCertificateDelegate> delegate) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
@@ -177,13 +184,13 @@ void ClearTableViewDataSourcesIfNeeded(NSWindow*) {}
 - (void)sheetDidEnd:(NSWindow*)sheet
          returnCode:(NSInteger)returnCode
             context:(void*)context {
-  net::X509Certificate* cert = NULL;
+  net::ClientCertIdentity* cert = nullptr;
   if (returnCode == NSFileHandlingPanelOKButton) {
-    CFRange range = CFRangeMake(0, CFArrayGetCount(identities_));
+    CFRange range = CFRangeMake(0, CFArrayGetCount(sec_identities_));
     CFIndex index =
-        CFArrayGetFirstIndexOfValue(identities_, range, [panel_ identity]);
+        CFArrayGetFirstIndexOfValue(sec_identities_, range, [panel_ identity]);
     if (index != -1)
-      cert = certificates_[index].get();
+      cert = cert_identities_[index].get();
     else
       NOTREACHED();
   }
@@ -197,30 +204,40 @@ void ClearTableViewDataSourcesIfNeeded(NSWindow*) {}
     // certificate. Otherwise, tell the backend which identity (or none) the
     // user selected.
     userResponded_ = YES;
-    observer_->CertificateSelected(cert);
+
+    // Remove the observer before we try acquiring private key, otherwise we
+    // might act on a notification while waiting for the callback, causing us
+    // to delete ourself before the callback gets called.
+    observer_->StopObserving();
+
+    if (cert) {
+      // Pass a scoped_nsobject handle for |self| to ensure |observer_| and
+      // |cert_identities_| stay alive until the private key is acquired.
+      cert->AcquirePrivateKey(
+          base::Bind(&SSLClientAuthObserverCocoaBridge::GotPrivateKey,
+                     base::Unretained(observer_.get()),
+                     base::scoped_nsobject<SSLClientCertificateSelectorCocoa>(
+                         [self retain]),
+                     base::Unretained(cert->certificate())));

[davidben, 2017/06/07 23:06:16]

Optional: It's a little odd here that you're calling AcquirePrivateKey when
you've already got the SecIdentityRef anyway. Since we need the SecIdentityRef
escape hatch anyway, AcquirePrivateKey being this asynchronous thing has no
actual purpose in life. You could just have it go call
CreateSSLPrivateKeyForSecIdentity.

(Then the only reason for the CFArrayGetIndexOfValue business is to get the
X509Certificate out. We could pull it out of the SecIdentityRef but I suppose
that requires a conversion, so keeping the X509Certificate around is useful.)

On NSS/CrOS/Windows, ClientCertIdentity is an abstraction boundary between the
platform-specific cert store code and the platform-independent UI code. But on
Mac, it's just a convenient handle to get from //content embedder to //content
back out to //content embedder because the client cert lookup happens inside
//content right now.

Hrm, actually, as I write this I realize that's not quite true.
ClientCertIdentity is also an abstraction between the platform-specific bits and
the platform-independent client cert policy stuff. So we can't, say, just take
AcquirePrivateKey on Mac altogether...

I dunno, what do you think?

[mattm, 2017/06/08 21:47:55]

Yeah, I think that makes sense. If this code is already cheating to get the
SecIdentityRef, might as well use it.

+    } else {
+      observer_->CertificateSelected(nullptr, nullptr);
+    }
 
     constrainedWindow_->CloseWebContentsModalDialog();
   }
 }
 
 - (void)displayForWebContents:(content::WebContents*)webContents
-                  clientCerts:(net::CertificateList)inputClientCerts {
+                  clientCerts:(net::ClientCertIdentityList)inputClientCerts {
+  cert_identities_ = std::move(inputClientCerts);
   // Create an array of CFIdentityRefs for the certificates:
-  size_t numCerts = inputClientCerts.size();
-  identities_.reset(CFArrayCreateMutable(
-      kCFAllocatorDefault, numCerts, &kCFTypeArrayCallBacks));
+  size_t numCerts = cert_identities_.size();
+  sec_identities_.reset(CFArrayCreateMutable(kCFAllocatorDefault, numCerts,
+                                             &kCFTypeArrayCallBacks));
   for (size_t i = 0; i < numCerts; ++i) {
-    base::ScopedCFTypeRef<SecCertificateRef> cert(
-        net::x509_util::CreateSecCertificateFromX509Certificate(
-            inputClientCerts[i].get()));
-    if (!cert)
-      continue;
-    SecIdentityRef identity;
-    if (SecIdentityCreateWithCertificate(NULL, cert, &identity) == noErr) {
-      CFArrayAppendValue(identities_, identity);
-      CFRelease(identity);
-      certificates_.push_back(inputClientCerts[i]);
-    }
+    DCHECK(cert_identities_[i]->sec_identity_ref());
+    CFArrayAppendValue(sec_identities_,
+                       cert_identities_[i]->sec_identity_ref());
   }
 
   // Get the message to display:
@@ -265,7 +282,7 @@ void ClearTableViewDataSourcesIfNeeded(NSWindow*) {}
                 modalDelegate:self
                didEndSelector:@selector(sheetDidEnd:returnCode:context:)
                   contextInfo:NULL
-                   identities:base::mac::CFToNSCast(identities_)
+                   identities:base::mac::CFToNSCast(sec_identities_)
                       message:title];
 }