Refactor client cert private key handling.

chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm

 // Copyright (c) 2012 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.
 
 #import "chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.h"
 
 #import <SecurityInterface/SFChooseIdentityPanel.h>
 #include <stddef.h>
 
 #include <utility>
@@ skipping 54 matching lines @@
   // ConstrainedWindowMacDelegate implementation:
   void OnConstrainedWindowClosed(ConstrainedWindowMac* window) override {
     // |onConstrainedWindowClosed| will delete the sheet which might be still
     // in use higher up the call stack. Wait for the next cycle of the event
     // loop to call this function.
     [controller_ performSelector:@selector(onConstrainedWindowClosed)
                       withObject:nil
                       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
 };
 
 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);
 
   // Not all WebContentses can show modal dialogs.
   //
   // Use the top-level embedder if |contents| is a guest.
   // GetTopLevelWebContents() will return |contents| otherwise.
   // TODO(davidben): Move this hook to the WebContentsDelegate and only try to
   // show a dialog in Browser's implementation. https://crbug.com/456255
   if (web_modal::WebContentsModalDialogManager::FromWebContents(
@@ skipping 75 matching lines @@
   if ((self = [super init])) {
     observer_.reset(new SSLClientAuthObserverCocoaBridge(
         browserContext, certRequestInfo, std::move(delegate), self));
   }
   return self;
 }
 
 - (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();
   }
 
   // See comment at definition; this works around a 10.12 bug.
   ClearTableViewDataSourcesIfNeeded(sheet);
 
   if (!closePending_) {
     // If |closePending_| is already set, |closeSheetWithAnimation:| was called
     // already to cancel the selection rather than continue with no
     // 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:
   NSString* message = l10n_util::GetNSStringF(
       IDS_CLIENT_CERT_DIALOG_TEXT,
       base::ASCIIToUTF16(
           observer_->cert_request_info()->host_and_port.ToString()));
 
   // Create and set up a system choose-identity panel.
   panel_.reset([[SFChooseIdentityPanel alloc] init]);
@@ skipping 24 matching lines @@
   return panel_;
 }
 
 - (void)showSheetForWindow:(NSWindow*)window {
   NSString* title = l10n_util::GetNSString(IDS_CLIENT_CERT_DIALOG_TITLE);
   overlayWindow_.reset([window retain]);
   [panel_ beginSheetForWindow:window
                 modalDelegate:self
                didEndSelector:@selector(sheetDidEnd:returnCode:context:)
                   contextInfo:NULL
-                   identities:base::mac::CFToNSCast(identities_)
+                   identities:base::mac::CFToNSCast(sec_identities_)
                       message:title];
 }
 
 - (void)closeSheetWithAnimation:(BOOL)withAnimation {
   if (!userResponded_) {
     // If the sheet is closed by closing the tab rather than the user explicitly
     // hitting Cancel, |closeSheetWithAnimation:| gets called before
     // |sheetDidEnd:|. In this case, the selection should be canceled rather
     // than continue with no certificate. The |returnCode| parameter to
     // |sheetDidEnd:| is the same in both cases.
@@ skipping 43 matching lines @@
 }
 
 - (void)onConstrainedWindowClosed {
   observer_->StopObserving();
   panel_.reset();
   constrainedWindow_.reset();
   [self release];
 }
 
 @end