ClientCertStoreChromeOS: support additional non-platform certs.

net/ssl/client_cert_store_chromeos.h

Index: net/ssl/client_cert_store_chromeos.h
diff --git a/net/ssl/client_cert_store_chromeos.h b/net/ssl/client_cert_store_chromeos.h
index 2bc359cd9a5f3a0c251bacf4281bc90d2d52c1ed..c58f7ba3f9043ddfabe4871cf451b117086dce33 100644
--- a/net/ssl/client_cert_store_chromeos.h
+++ b/net/ssl/client_cert_store_chromeos.h
@@ -33,9 +33,11 @@ class NET_EXPORT ClientCertStoreChromeOS : public ClientCertStoreNSS {
         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;
@@ -57,6 +59,7 @@ class NET_EXPORT ClientCertStoreChromeOS : public ClientCertStoreNSS {
                              CertificateList* selected_certs,
                              const base::Closure& callback);
 
+  const CertificateList additional_certs_;
   scoped_ptr<CertFilter> cert_filter_;
 
   DISALLOW_COPY_AND_ASSIGN(ClientCertStoreChromeOS);