ClientCertStoreChromeOS: support additional non-platform certs.

net/ssl/client_cert_store_chromeos.h

 // Copyright 2013 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 NET_SSL_CLIENT_CERT_STORE_CHROMEOS_H_
 #define NET_SSL_CLIENT_CERT_STORE_CHROMEOS_H_
 
 #include <string>
 
 #include "base/memory/scoped_ptr.h"
@@ skipping 15 matching lines @@
     // Must be called at most once.
     virtual bool Init(const base::Closure& callback) = 0;
 
     // Returns true if |cert| is allowed to be used as a client certificate
     // (e.g. for a certain browser context or user).
     // This is only called once initialization is finished, see Init().
     virtual bool IsCertAllowed(
         const scoped_refptr<X509Certificate>& cert) const = 0;
   };
 
-  // This ClientCertStore will return only client certs that pass the filter
-  // |cert_filter|.
+  // This ClientCertStore will return client certs from NSS certificate
+  // databases that pass the filter |cert_filter| and unconditionally the certs
+  // from |additional_certs|.
   ClientCertStoreChromeOS(
+      const CertificateList& additional_certs,

[Ryan Sleevi, 2015/08/07 23:51:09]

Are you sure this is the right pattern? I don't believe it is.

It forces your ClientCertStoreChromeOS to know all of the certificates at load
time, and prevents you from ever changing them. That certainly doesn't match the
NSS guarantees, nor necessarily that of smart cards (e.g. where authenticating
to a hostile card may reveal additional certificates). Are you sure it's what
you want?

[pneubeck (no reviews), 2015/08/10 12:09:56]

Yeah, I thought about that point as well.
As far as I understand it, the (only) call chain is
 URLRequestHttpJob::OnStartCompleted ->
 URLRequestJob::NotifyCertificateRequested ->
 URLRequest::NotifyCertificateRequested ->
 ResourceLoader::OnCertificateRequested ->
 ResourceContext::CreateClientCertStore
(https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/lo...
)

which is executed everytime for the server requesting a client cert.
Of course, there's the chance that a certificate becomes available after the
cert store is created and before the certificates are retrieved from the cert
store. But that chance always exists. We can at most try to make the window as
small as possible.
My expectation would be that it is already rather small with the current
implementation.
Is there reason to expect otherwise?

On the other hand, to retrieve the cert list in GetClientCerts will end up in
more callback complexity again, that we should avoid if there's no clear
benefit.

[davidben, 2015/08/10 21:54:00]

Honestly, this whole ClientCertStore thing is pretty weird. :-) It shouldn't be
in //net and it's really confusing that it's actually a single query, and not a
handle to the actual store, as it should be.

Fixing all that right now is probably not reasonable. That said, there is still
the root issue of whether the certificate list is synchronously available. This
leaks all the way up to the extension API itself, so we probably need to get
that right right now.

Should the API be certificateProvider.pubishClientCertificates or should the
certificate provider provide an asynchronous API to query the smartcard
as-needed? Certainly the latter matches how smartcards typically work. The
former would require that loading the driver query the smartcard on startup and
that Chrome maintain a list in-memory to mirror that list. And perhaps the
smartcard doesn't have any way to get notified when a new certificate is
installed (Ryan, is that a capability that smartcards typically have?) and would
have to poll just in case we add a new one. This seems poor.

If we do go with an asynchronous API in the certificate provider, I would
propose there be a CertFilter::GetAdditionalCertificates that returns a
CertificateList asynchronously. Or perhaps we go ahead and move
ClientCertStoreChromeOS into //chrome now---really ClientCertStore shouldn't be
in //net at all---and then you can avoid this ClientCertStore / CertFilter
split.

[pneubeck (no reviews), 2015/08/13 12:25:24]

Done.

       scoped_ptr<CertFilter> cert_filter,
       const PasswordDelegateFactory& password_delegate_factory);
   ~ClientCertStoreChromeOS() override;
 
   // ClientCertStoreNSS:
   void GetClientCerts(const SSLCertRequestInfo& cert_request_info,
                       CertificateList* selected_certs,
                       const base::Closure& callback) override;
 
  protected:
   // ClientCertStoreNSS:
   void GetClientCertsImpl(CERTCertList* cert_list,
                           const SSLCertRequestInfo& request,
                           bool query_nssdb,
                           CertificateList* selected_certs) override;
 
  private:
   void CertFilterInitialized(const SSLCertRequestInfo* request,
                              CertificateList* selected_certs,
                              const base::Closure& callback);
 
+  const CertificateList additional_certs_;
   scoped_ptr<CertFilter> cert_filter_;
 
   DISALLOW_COPY_AND_ASSIGN(ClientCertStoreChromeOS);
 };
 
 }  // namespace net
 
 #endif  // NET_SSL_CLIENT_CERT_STORE_CHROMEOS_H_