Refactor client cert private key handling.

Refactor client cert private key handling.

This removes the global FetchClientCertPrivateKey function, which needed to be able
to find the private key given just an X509Certificate object. Now, ClientCertStore
returns ClientCertIdentity objects, which contain a certificate and have a method to
asynchronously retrieve the matching SSLPrivateKey object.

Also re-enable SSLClientCertificateSelectorCocoaTest.Basic.

BUG=394131, 313243, 222296
Review-Url: https://codereview.chromium.org/2898573002
Cr-Commit-Position: refs/heads/master@{#480556}
Committed: https://chromium.googlesource.com/chromium/src/+/436ccfe33a1747eb5e4d4b9ed948d498d560bf19

[mattm, 2017-05-20 02:21:35]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-20 02:22:12]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-20 02:53:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-20 02:53:05]

Dry run: Try jobs failed on following builders:
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[mattm, 2017-05-22 20:59:47]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 21:00:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-22 21:04:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-22 21:04:55]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mattm, 2017-05-22 21:11:53]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 21:12:15]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mattm, 2017-05-24 21:23:55]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 21:24:49]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 21:56:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-24 21:56:39]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)

[mattm, 2017-05-24 22:54:40]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 22:55:28]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mattm, 2017-05-24 23:35:22]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 23:36:24]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mattm, 2017-05-25 02:03:46]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-25 02:04:44]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-25 04:05:37]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-25 04:05:42]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[mattm, 2017-05-26 20:49:14]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 20:50:04]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-26 23:16:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-26 23:16:11]

Dry run: This issue passed the CQ dry run.

[mattm, 2017-05-27 01:52:46]

Description was changed from

==========
*WIP* Refactor client cert private key handling.

BUG=394131
==========

to

==========
*WIP* Refactor client cert private key handling.

Also re-enable SSLClientCertificateSelectorCocoaTest.Basic.

BUG=394131,313243,222296
==========

[mattm, 2017-05-27 03:11:43]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-27 03:12:00]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mattm, 2017-05-27 03:15:06]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-27 03:15:19]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mattm, 2017-05-27 03:15:32]

Patchset #11 (id:200001) has been deleted

[commit-bot: I haz the power, 2017-05-27 04:45:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-27 04:45:03]

Dry run: This issue passed the CQ dry run.

[mattm, 2017-05-30 18:06:56]

Description was changed from

==========
*WIP* Refactor client cert private key handling.

Also re-enable SSLClientCertificateSelectorCocoaTest.Basic.

BUG=394131,313243,222296
==========

to

==========
Refactor client cert private key handling.

Removes the global FetchClientCertPrivateKey function which needed to be able to
find the private key given just an X509Certificate object.
Now ClientCertStore returns ClientCertIdentity objects which contain a
certificate and has a method to asynchronously retrieve the matching
SSLPrivateKey object.

Also re-enable SSLClientCertificateSelectorCocoaTest.Basic.

BUG=394131,313243,222296
==========

[mattm, 2017-05-30 18:31:53]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[mattm, 2017-05-30 18:32:19]

Description was changed from

==========
Refactor client cert private key handling.

Removes the global FetchClientCertPrivateKey function which needed to be able to
find the private key given just an X509Certificate object.
Now ClientCertStore returns ClientCertIdentity objects which contain a
certificate and has a method to asynchronously retrieve the matching
SSLPrivateKey object.

Also re-enable SSLClientCertificateSelectorCocoaTest.Basic.

BUG=394131,313243,222296
==========

to

==========
Refactor client cert private key handling.

Removes the global FetchClientCertPrivateKey function which needed to be able to
find the private key given just an X509Certificate object.
Now ClientCertStore returns ClientCertIdentity objects which contain a
certificate and have a method to asynchronously retrieve the matching
SSLPrivateKey object.

Also re-enable SSLClientCertificateSelectorCocoaTest.Basic.

BUG=394131,313243,222296
==========

[commit-bot: I haz the power, 2017-05-30 18:32:33]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mattm, 2017-05-30 18:32:50]

mattm@chromium.org changed reviewers:
+ davidben@chromium.org

[commit-bot: I haz the power, 2017-05-30 21:01:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-30 21:02:00]

Dry run: This issue passed the CQ dry run.

[davidben, 2017-06-01 23:41:26]

Thanks! I did the bits //content and down for this pass, since it's a huge CL. I
haven't looked at //content and above yet but will try to do that tomorrow.

+rsleevi for the two "Ryan question"s (search for the phrase) below.

https://codereview.chromium.org/2898573002/diff/240001/content/public/browser...
File content/public/browser/client_certificate_delegate.h (right):

https://codereview.chromium.org/2898573002/diff/240001/content/public/browser...
content/public/browser/client_certificate_delegate.h:25: // Continue the request
with |cert| and matching |key|. |cert| or |key| may
or => and?

https://codereview.chromium.org/2898573002/diff/240001/content/public/browser...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/2898573002/diff/240001/content/public/browser...
content/public/browser/content_browser_client.h:77: using ClientCertIdentityList
= std::vector<std::unique_ptr<ClientCertIdentity>>;
Do we still like to do this kind of forward decl? I thought we usually just give
up an #include the headers for this.

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/client_cert_st...
File net/ssl/client_cert_store_mac.cc (right):

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/client_cert_st...
net/ssl/client_cert_store_mac.cc:91: // certificate
The last sentence seems off. "is updated to include the intermediates"?

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/client_cert_st...
net/ssl/client_cert_store_mac.cc:127:
identity->SetIntermediates(new_cert->GetIntermediateCertificates());
This feels slightly odd (you could also defer creating the ClientCertIdentity
once you have everything), but that's probably more trouble than is worth it to
reshuffle everything that way, and this way we get to reuse methods on
X509Certificate.

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/client_cert_st...
net/ssl/client_cert_store_mac.cc:192: bool query_keychain,
I suspect the query_keychain boolean silliness could be better done with the
temporary keychain trick plus SecTrustSetKeychains, but, if we bother, that's
for a later CL. :-)

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/client_cert_st...
File net/ssl/client_cert_store_nss_unittest.cc (right):

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/client_cert_st...
net/ssl/client_cert_store_nss_unittest.cc:121:
TestSSLPrivateKeyMatches(ssl_private_key.get(), pkcs8_key);
Nice!

(In the ssl_platform_key_* tests, this function is paired with a supported hash
functions check since TestSSLPrivateKeyMatches only checks whatever the key
advertises, which might be empty. But perhaps that's not worth the trouble.)

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/client_cert_st...
File net/ssl/client_cert_store_win.cc (right):

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/client_cert_st...
net/ssl/client_cert_store_win.cc:188: // pair<X509Certificate,
SSLPrivateKeyCallback>.
I think |cert| should probably be created from bytes out of
cert_context2->pbCertEncoded/cbCertEncoded? Maybe also the intermediates for
good measure?

This is possibly a Ryan question to make sure I'm not totally confused, but the
point of this exercise, as I understand was because cert_context and
cert_context2 are potentially tied to the smartcard driver and thus not
threadsafe or anything sensible (hence all this code running on the IO thread).

On the one hand, this is needed because FetchClientCertPrivateKey might want the
original PCCERT_CONTEXT.

On the other hand, this is not particular desirable because we'd rather
net::X509Certificate be logically byte strings. This avoids any threading
concerns. Notably, although we're trying to move everything to the smartcard
thread, access of the certs themselves is going to happen on the IO thread,
since we need the actual bytes. If we create things from bytes fresh, that means
we know they're not tied to any smartcard (and eventually can safely be made
CRYPTO_BUFFERs).

(Once you do that, you can just remove this TODO.)

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/client_cert_st...
net/ssl/client_cert_store_win.cc:217: const ClientCertListCallback& callback) {
This is probably a Ryan question, but in the same vein as not worrying about the
smartcard's thread-safety, perhaps GetSSLPlatformKeyTaskRunner() should be used
for GetClientCertsImpl too? That way accesses of the smartcard are all on one
thread. (Which perhaps suggests that FetchClientCertPrivateKey should take a
TaskRunner of some sort so we don't need to rely on two files picking the same
one...)

So originally, we had:

GetClientCertsImpl - IO thread
FetchClientCertPrivateKey - IO thread
SSLPlatformKey - IO thread

This avoids threading troubles, but blocks the IO thread. Then we pushed the
signing off-thread and got:

GetClientCertsImpl - IO thread
FetchClientCertPrivateKey - IO thread
SSLPlatformKey - smartcard thread

When we did this, Ryan pointed out that we're now risking problems with buggy
smartcards (though it seems to have been fine?). I assume that's why
GetClientCertsImpl didn't run on a worker thread way back.

Your CL makes it:

GetClientCertsImpl - IO thread
FetchClientCertPrivateKey - smartcard thread
SSLPlatformKey - smartcard thread

But my understanding is we actually want:

GetClientCertsImpl - smartcard thread
FetchClientCertPrivateKey - smartcard thread
SSLPlatformKey - smartcard thread

As a bonus, GetClientCertsImpl won't block the IO thread!

(How this should interact with the non-NULL cert_store_ case, I have no idea!)

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/client_key_sto...
File net/ssl/client_key_store.h (right):

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/client_key_sto...
net/ssl/client_key_store.h:30: // Methods of this singleton can be called from
any thread.
Yeah, these two TODOs are two parts of the same coin. :-) The intent is that
once we have a path from the ClientCertStore implementation to the
SSLPrivateKey, we don't need this "provider" mess and these globals.

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/ssl_cert_reque...
File net/ssl/ssl_cert_request_info.h (right):

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/ssl_cert_reque...
net/ssl/ssl_cert_request_info.h:14: #include "net/ssl/client_cert_identity.h"
Stray #include?

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/ssl_platform_k...
File net/ssl/ssl_platform_key_nss.cc (right):

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/ssl_platform_k...
net/ssl/ssl_platform_key_nss.cc:161:
PK11_FindKeyByAnyCert(certificate->os_cert_handle(), wincx));
Huh. We didn't manage to pass this in before, did we? Do you know in what cases
this might matter? I guess now that we're finally running this step on a
background thread, we can afford to do this.

[davidben, 2017-06-01 23:41:55]

On 2017/06/01 23:41:26, davidben wrote:
> Thanks! I did the bits //content and down for this pass, since it's a huge CL.
I
> haven't looked at //content and above yet but will try to do that tomorrow.

Er, I haven't looked at things above //content. I have looked at //content.

[davidben, 2017-06-01 23:46:14]

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/client_cert_st...
File net/ssl/client_cert_store_win.cc (right):

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/client_cert_st...
net/ssl/client_cert_store_win.cc:217: const ClientCertListCallback& callback) {
Likewise, should we try to keep thread affinity throughout like this for Mac and
NSS too? It seems a little odd to be happy using the worker pool most of it, but
then switching to GetSSLPlatformKeyTaskRunner once the actual signing kicks in.
Though I suppose nothing's broken yet...

[mattm, 2017-06-02 04:02:10]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-02 04:02:37]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mattm, 2017-06-02 04:04:21]

https://codereview.chromium.org/2898573002/diff/240001/content/public/browser...
File content/public/browser/client_certificate_delegate.h (right):

https://codereview.chromium.org/2898573002/diff/240001/content/public/browser...
content/public/browser/client_certificate_delegate.h:25: // Continue the request
with |cert| and matching |key|. |cert| or |key| may
On 2017/06/01 23:41:25, davidben wrote:
> or => and?

It's possible for |cert| to be non-null and |key| to be null (eg, if
AcquirePrivateKey fails).  Maybe "and/or"?

https://codereview.chromium.org/2898573002/diff/240001/content/public/browser...
File content/public/browser/content_browser_client.h (right):

https://codereview.chromium.org/2898573002/diff/240001/content/public/browser...
content/public/browser/content_browser_client.h:77: using ClientCertIdentityList
= std::vector<std::unique_ptr<ClientCertIdentity>>;
On 2017/06/01 23:41:25, davidben wrote:
> Do we still like to do this kind of forward decl? I thought we usually just
give
> up an #include the headers for this.

I think I did do that in most places, but since content_browser_client.h gets
included so many places I thought maybe it was worth doing here. Fine with just
doing the include too though, if you want.

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/client_cert_st...
File net/ssl/client_cert_store_mac.cc (right):

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/client_cert_st...
net/ssl/client_cert_store_mac.cc:91: // certificate
On 2017/06/01 23:41:25, davidben wrote:
> The last sentence seems off. "is updated to include the intermediates"?

Done.

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/client_cert_st...
net/ssl/client_cert_store_mac.cc:98: identity->certificate()));
Changed this since I noticed now I could actually just get the SecCertificate
out of identity->sec_identity_ref() for this part. (Since this should only be
reached when using real identities due to the query_keychain condition.)

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/client_cert_st...
net/ssl/client_cert_store_mac.cc:127:
identity->SetIntermediates(new_cert->GetIntermediateCertificates());
On 2017/06/01 23:41:25, davidben wrote:
> This feels slightly odd (you could also defer creating the ClientCertIdentity
> once you have everything), but that's probably more trouble than is worth it
to
> reshuffle everything that way, and this way we get to reuse methods on
> X509Certificate.

Yeah, I noticed this oddity too, but it seemed like it would have to uglify
other things to avoid it.

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/client_cert_st...
net/ssl/client_cert_store_mac.cc:192: bool query_keychain,
On 2017/06/01 23:41:25, davidben wrote:
> I suspect the query_keychain boolean silliness could be better done with the
> temporary keychain trick plus SecTrustSetKeychains, but, if we bother, that's
> for a later CL. :-)

Well, aside from crbug.com/671889 :(
(SecTrustSetKeychains does not work on Mac OS 10.12)

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/client_cert_st...
File net/ssl/client_cert_store_nss_unittest.cc (right):

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/client_cert_st...
net/ssl/client_cert_store_nss_unittest.cc:121:
TestSSLPrivateKeyMatches(ssl_private_key.get(), pkcs8_key);
On 2017/06/01 23:41:25, davidben wrote:
> Nice!
> 
> (In the ssl_platform_key_* tests, this function is paired with a supported
hash
> functions check since TestSSLPrivateKeyMatches only checks whatever the key
> advertises, which might be empty. But perhaps that's not worth the trouble.)

Done.

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/client_cert_st...
File net/ssl/client_cert_store_win.cc (right):

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/client_cert_st...
net/ssl/client_cert_store_win.cc:188: // pair<X509Certificate,
SSLPrivateKeyCallback>.
On 2017/06/01 23:41:25, davidben wrote:
> I think |cert| should probably be created from bytes out of
> cert_context2->pbCertEncoded/cbCertEncoded? Maybe also the intermediates for
> good measure?
> 
> This is possibly a Ryan question to make sure I'm not totally confused, but
the
> point of this exercise, as I understand was because cert_context and
> cert_context2 are potentially tied to the smartcard driver and thus not
> threadsafe or anything sensible (hence all this code running on the IO
thread).
> 
> On the one hand, this is needed because FetchClientCertPrivateKey might want
the
> original PCCERT_CONTEXT.
> 
> On the other hand, this is not particular desirable because we'd rather
> net::X509Certificate be logically byte strings. This avoids any threading
> concerns. Notably, although we're trying to move everything to the smartcard
> thread, access of the certs themselves is going to happen on the IO thread,
> since we need the actual bytes. If we create things from bytes fresh, that
means
> we know they're not tied to any smartcard (and eventually can safely be made
> CRYPTO_BUFFERs).
> 
> (Once you do that, you can just remove this TODO.)

I was planning to leave this as is in this CL since it should be followed very
shortly by switching win to X509CertificateBytes.

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/client_cert_st...
net/ssl/client_cert_store_win.cc:217: const ClientCertListCallback& callback) {
On 2017/06/01 23:46:14, davidben wrote:
> Likewise, should we try to keep thread affinity throughout like this for Mac
and
> NSS too? It seems a little odd to be happy using the worker pool most of it,
but
> then switching to GetSSLPlatformKeyTaskRunner once the actual signing kicks
in.
> Though I suppose nothing's broken yet...

Sounds reasonable but I'll wait and see if Ryan chimes in on these questions.
Also probably the GetClientCerts threading changes could be left to a followup
CL?

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/client_key_sto...
File net/ssl/client_key_store.h (right):

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/client_key_sto...
net/ssl/client_key_store.h:30: // Methods of this singleton can be called from
any thread.
On 2017/06/01 23:41:25, davidben wrote:
> Yeah, these two TODOs are two parts of the same coin. :-) The intent is that
> once we have a path from the ClientCertStore implementation to the
> SSLPrivateKey, we don't need this "provider" mess and these globals.

You probably didn't look at the chromeos extensions parts yet, but I didn't see
a good way to actually fully switch to ClientCertIdentities there. 

https://developer.chrome.com/extensions/platformKeys has
selectClientCertificates which returns just DER certs, and has a separate
getKeyPair that takes a DER cert and returns the public/private keys.  So I
ended up leaving that part still doing private key lookups in the old hacky way
:(

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/ssl_cert_reque...
File net/ssl/ssl_cert_request_info.h (right):

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/ssl_cert_reque...
net/ssl/ssl_cert_request_info.h:14: #include "net/ssl/client_cert_identity.h"
On 2017/06/01 23:41:25, davidben wrote:
> Stray #include?

Done.

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/ssl_platform_k...
File net/ssl/ssl_platform_key_nss.cc (right):

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/ssl_platform_k...
net/ssl/ssl_platform_key_nss.cc:161:
PK11_FindKeyByAnyCert(certificate->os_cert_handle(), wincx));
On 2017/06/01 23:41:25, davidben wrote:
> Huh. We didn't manage to pass this in before, did we? Do you know in what
cases
> this might matter? I guess now that we're finally running this step on a
> background thread, we can afford to do this.

Previously we had that ugly UnlockSlot thing where the client cert selector
dialog would try to unlock the slot before calling the result callback and hope
that was good enough. I manually tested that this still works if you have a
password on your NSSDB, should also apply if you have a password protected smart
card though I didn't test that.

[commit-bot: I haz the power, 2017-06-02 04:38:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-02 04:38:30]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[mattm, 2017-06-02 18:05:47]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-02 18:06:32]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-02 21:41:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-02 21:41:51]

Dry run: This issue passed the CQ dry run.

[Ryan Sleevi, 2017-06-05 20:18:12]

rsleevi@chromium.org changed reviewers:
+ rsleevi@chromium.org

[Ryan Sleevi, 2017-06-05 20:18:13]

Adding myself, since David mentioned "Ryan questions" - but I probably won't be
able to get to them until tomorrow morning

[davidben, 2017-06-05 20:21:31]

On 2017/06/05 20:18:13, Ryan Sleevi wrote:
> Adding myself, since David mentioned "Ryan questions" - but I probably won't
be
> able to get to them until tomorrow morning

My bad. I must not have actually added you! Oops.

[davidben, 2017-06-07 23:06:17]

Phew. That was a long CL. :-)

https://codereview.chromium.org/2898573002/diff/240001/chrome/browser/chromeo...
File chrome/browser/chromeos/platform_keys/platform_keys_nss.cc (right):

https://codereview.chromium.org/2898573002/diff/240001/chrome/browser/chromeo...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:554: //
crypto::FindNSSKeyFromPublicKeyInfo.
Ugh. There's some desire to get the certProvider bits routed into here too. This
suggests we should ask them to make an API that does both steps together first.
It'd be more efficient anyway. Less IPC overhead.

https://codereview.chromium.org/2898573002/diff/240001/content/public/browser...
File content/public/browser/client_certificate_delegate.h (right):

https://codereview.chromium.org/2898573002/diff/240001/content/public/browser...
content/public/browser/client_certificate_delegate.h:25: // Continue the request
with |cert| and matching |key|. |cert| or |key| may
On 2017/06/02 04:04:20, mattm wrote:
> On 2017/06/01 23:41:25, davidben wrote:
> > or => and?
> 
> It's possible for |cert| to be non-null and |key| to be null (eg, if
> AcquirePrivateKey fails).  Maybe "and/or"?

Well, cert non-null and key null doesn't mean to continue without a certificate.
It means to fail really really deep in the stack with
ERR_SSL_CLIENT_AUTH_CERT_NO_PRIVATE_KEY, which is kinda weird.

It's this bizarre thing here:
https://docs.google.com/document/d/13qJRJKczwMu6Cqw_xEkGhp_pZrJI7P2s-jNCjVUAg...

Maybe just leave it at |cert| may be nullptr [...] for the time being?

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/client_cert_st...
File net/ssl/client_cert_store_win.cc (right):

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/client_cert_st...
net/ssl/client_cert_store_win.cc:217: const ClientCertListCallback& callback) {
On 2017/06/02 04:04:20, mattm wrote:
> On 2017/06/01 23:46:14, davidben wrote:
> > Likewise, should we try to keep thread affinity throughout like this for Mac
> and
> > NSS too? It seems a little odd to be happy using the worker pool most of it,
> but
> > then switching to GetSSLPlatformKeyTaskRunner once the actual signing kicks
> in.
> > Though I suppose nothing's broken yet...
> 
> Sounds reasonable but I'll wait and see if Ryan chimes in on these questions.
> Also probably the GetClientCerts threading changes could be left to a followup
> CL?

Fair enough. I guess the main reason we might want this (and the above) done
sooner is if we newly expose some threading risks with this change.

(We're already assuming the smartcard can tolerate
CryptAcquireCertificatePrivateKey and usage of the private key on different
threads, but I don't think we're currently assuming that the
potentially-smartcard-associated PCCERT_CONTEXT may be acquired on the IO thread
and then CryptAcquireCertificatePrivateKey'd on the smartcard thread.)

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/client_key_sto...
File net/ssl/client_key_store.h (right):

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/client_key_sto...
net/ssl/client_key_store.h:30: // Methods of this singleton can be called from
any thread.
On 2017/06/02 04:04:20, mattm wrote:
> On 2017/06/01 23:41:25, davidben wrote:
> > Yeah, these two TODOs are two parts of the same coin. :-) The intent is that
> > once we have a path from the ClientCertStore implementation to the
> > SSLPrivateKey, we don't need this "provider" mess and these globals.
> 
> You probably didn't look at the chromeos extensions parts yet, but I didn't
see
> a good way to actually fully switch to ClientCertIdentities there. 
> 
> https://developer.chrome.com/extensions/platformKeys has
> selectClientCertificates which returns just DER certs, and has a separate
> getKeyPair that takes a DER cert and returns the public/private keys.  So I
> ended up leaving that part still doing private key lookups in the old hacky
way
> :(

Hrm. Well, if we could at least avoid a mess of globals and "provider"
interfaces, that would be something. That the leaf operation is a cert => key
lookup is annoying, but not far from how all the other platforms work.

Disjointed incoherent babbling of ideas: CrOS ClientCertStore implementation
could just query the CertificateProviderService and be done with it. Or if the
interface between CrOS's ClientCertStore and CertificateProviderService is still
valuable, we can at least fold the CertProvider and CertKeyProvider interfaces
into one thing. PErhaps rather than the additional_certs mess,
CertificateProviderService can just vend out a bunch of ClientCertIdentities?

(Not suggesting this be in the same CL, of course.)

https://codereview.chromium.org/2898573002/diff/280001/chrome/browser/chromeo...
File chrome/browser/chromeos/net/client_cert_store_chromeos.cc (right):

https://codereview.chromium.org/2898573002/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/net/client_cert_store_chromeos.cc:46: void
ClientCertIdentityCros::AcquirePrivateKey(
Optional: Any reason not to inline this one into the declaration? Not sure what
the style is.

https://codereview.chromium.org/2898573002/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/net/client_cert_store_chromeos.cc:50:
base::WorkerPool::GetTaskRunner(true /* task_is_slow */).get(),
This doesn't actually need to be on a worker thread, does it? It doesn't really
do anything. :-)

https://cs.chromium.org/chromium/src/chrome/browser/chromeos/certificate_prov...

https://codereview.chromium.org/2898573002/diff/280001/chrome/browser/chromeo...
File chrome/browser/chromeos/platform_keys/platform_keys_nss.cc (right):

https://codereview.chromium.org/2898573002/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/platform_keys/platform_keys_nss.cc:554: //
crypto::FindNSSKeyFromPublicKeyInfo.
Sigh. There's some CL floating around to expose the CertProvider ones out of
this API, so we probably need to ask them to fix this before doing that. The
platform API is inherently a singleton, but I feel like requiring a global map
on CertProvider certs is silly.

https://codereview.chromium.org/2898573002/diff/280001/chrome/browser/ssl/ssl...
File chrome/browser/ssl/ssl_client_auth_observer.cc (right):

https://codereview.chromium.org/2898573002/diff/280001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_client_auth_observer.cc:23: CertDetails;
Style nit: using CertDetails = ...;

https://codereview.chromium.org/2898573002/diff/280001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm (right):

https://codereview.chromium.org/2898573002/diff/280001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm:221:
base::Unretained(cert->certificate())));
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?

https://codereview.chromium.org/2898573002/diff/280001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa_browsertest.mm
(left):

https://codereview.chromium.org/2898573002/diff/280001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa_browsertest.mm:86:
mit_davidben_cert_ = net::ImportCertFromFile(certs_dir, "mit.davidben.der");
...that's still in there? Hah.

https://codereview.chromium.org/2898573002/diff/280001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa_browsertest.mm
(right):

https://codereview.chromium.org/2898573002/diff/280001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa_browsertest.mm:134:
return client_cert_list;
Nit: Does return {base::MakeUnique<...>(...), base::MakeUnique<...>(...)} work?

https://codereview.chromium.org/2898573002/diff/280001/chrome/browser/ui/cryp...
File chrome/browser/ui/crypto_module_delegate_nss.cc (right):

https://codereview.chromium.org/2898573002/diff/280001/chrome/browser/ui/cryp...
chrome/browser/ui/crypto_module_delegate_nss.cc:66: crypto::ScopedPK11Slot
ChromeNSSCryptoModuleDelegate::RequestSlot() {
BTW, this slot was only used for <keygen> which is gone now. That means you can
get rid of the NSSCryptoModuleDelegate interface, which means you can get rid of
slot_, the constructor parameter,
ChromeNSSCryptoModuleDelegate::CreateForResourceContext, and everything!

https://codereview.chromium.org/2898573002/diff/280001/chrome/browser/ui/view...
File chrome/browser/ui/views/platform_keys_certificate_selector_chromeos.cc
(right):

https://codereview.chromium.org/2898573002/diff/280001/chrome/browser/ui/view...
chrome/browser/ui/views/platform_keys_certificate_selector_chromeos.cc:50: const
net::CertificateList& certs) {
Optional: Same question about whether this should just be inlined into the
definition.

https://codereview.chromium.org/2898573002/diff/280001/chrome/browser/ui/view...
File chrome/browser/ui/views/ssl_client_certificate_selector.cc (right):

https://codereview.chromium.org/2898573002/diff/280001/chrome/browser/ui/view...
chrome/browser/ui/views/ssl_client_certificate_selector.cc:25: #include
"chrome/browser/ui/crypto_module_password_dialog_nss.h"
No longer used? (Yay!)

https://codereview.chromium.org/2898573002/diff/280001/chrome/browser/ui/view...
chrome/browser/ui/views/ssl_client_certificate_selector.cc:78:
&SSLClientCertificateSelector::GotPrivateKey, base::Unretained(this),
Existing issue, but I'm not sure base::Unretained works here. What if the window
is closed while all this is going on?

https://codereview.chromium.org/2898573002/diff/280001/chrome/browser/ui/view...
File chrome/browser/ui/views/ssl_client_certificate_selector_browsertest.cc
(right):

https://codereview.chromium.org/2898573002/diff/280001/chrome/browser/ui/view...
chrome/browser/ui/views/ssl_client_certificate_selector_browsertest.cc:78:
net::ClientCertIdentityList cert_identity_list;
Nit: Are we allowed to write something like this these days? Ditto throughout.

net::ClientCertIdentityList cert_identity_list = {cert_identity_1_->Copy(), 
                                                  cert_identity_2_->Copy()};

(Without the = ? I'm not sure what the style is.)

Or perhaps even:

selector_ = new SSLClientCertificateSelector(
    browser()->tab_strip_model()->GetActiveWebContents(),
    auth_requestor_->cert_request_info_,
    {cert_identity_1_->Copy(), cert_identity_2_->Copy()},
    auth_requestor_->CreateDelegate());

https://codereview.chromium.org/2898573002/diff/280001/net/ssl/client_cert_st...
File net/ssl/client_cert_store_win.cc (right):

https://codereview.chromium.org/2898573002/diff/280001/net/ssl/client_cert_st...
net/ssl/client_cert_store_win.cc:42: private_key_callback) override;
Optional: What's the reason for defining this function out-of-line? I'm never
clear on what our style is w.r.t. these things.

https://codereview.chromium.org/2898573002/diff/280001/net/ssl/client_cert_st...
net/ssl/client_cert_store_win.cc:53: base::Bind(&FetchClientCertPrivateKey,
Optional: Given this function isn't tested as part of
ssl_platform_key_win_unittest.cc anyway, we probably could just inline it into
ClientCertIdentityWin::AcquirePrivateKey. Dunno if that makes things any
cleaner.

[Ryan Sleevi, 2017-06-08 18:13:47]

Only responded to the Ryan questions

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/client_cert_st...
File net/ssl/client_cert_store_win.cc (right):

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/client_cert_st...
net/ssl/client_cert_store_win.cc:188: // pair<X509Certificate,
SSLPrivateKeyCallback>.
On 2017/06/02 04:04:20, mattm wrote:
> On 2017/06/01 23:41:25, davidben wrote:
> > I think |cert| should probably be created from bytes out of
> > cert_context2->pbCertEncoded/cbCertEncoded? Maybe also the intermediates for
> > good measure?
> > 
> > This is possibly a Ryan question to make sure I'm not totally confused, but
> the
> > point of this exercise, as I understand was because cert_context and
> > cert_context2 are potentially tied to the smartcard driver and thus not
> > threadsafe or anything sensible (hence all this code running on the IO
> thread).

|cert_context2| is not - line 150 handles that.

The "issue" was that |cert_context| is tied to the HCERTSTORE that it came from
(and thus the HCERTSTORE can't be closed while the cert context is still live,
which can cause smart card difficulties). So we clone the |cert_context| into
the default HCERTSTORE (and the intermediates) to break the HCERTSTORE linkage,
while still maintaining the non-serialized attributes (such as the underlying
smartcard handle). 

That said, because of X509CertificateCache::InsertOrUpdate, we wouldn't actually
keep the smartcard context in the null-store cert if we already had a
(different) null-store cert.

But totally happy to defer this cleanup; the current code isn't accomplishing
what it (originally) set out to do, but that hasn't caused issues.

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/client_cert_st...
net/ssl/client_cert_store_win.cc:217: const ClientCertListCallback& callback) {
On 2017/06/01 23:41:25, davidben wrote:
> Your CL makes it:
> 
> GetClientCertsImpl - IO thread
> FetchClientCertPrivateKey - smartcard thread
> SSLPlatformKey - smartcard thread

Right, so the risk I tried to capture in previous contexts was this:
- Prompting can happen in the CryptAcquire() call for some smart cards
- Prompting can happen in the CryptSignHash() call for other smart cards

Either of which can trigger UI and need to spin message pumps and all that
goodness.

HCERTSTORE is "supposed" to be thread-safe (and for MSFT it is, for others, it's
not). NCRYPT_KEY_HANDLE does not have such guarantees (IIRC). PCCERT_CONTEXT
does not (since it's backed by an HCERTSTORE)

I think the thread jumping may be better deferred until post-OSCertHandle, so
that we don't have to run the risk of using/accessing the PCCERT_CONTEXT (which
is held/lives on the IO thread) on other threads.

I missed where FetchClientCertPrivateKey moved off the IO thread, but if we're
using a PCCERT_CONTEXT, then we may be in trouble in doing so because of the
X509CertificateCache and the underlying HCERTSTORE affinity.

[mattm, 2017-06-08 21:40:45]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[davidben, 2017-06-08 21:40:49]

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/client_cert_st...
File net/ssl/client_cert_store_win.cc (right):

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/client_cert_st...
net/ssl/client_cert_store_win.cc:217: const ClientCertListCallback& callback) {
On 2017/06/08 18:13:47, Ryan Sleevi wrote:
> On 2017/06/01 23:41:25, davidben wrote:
> > Your CL makes it:
> > 
> > GetClientCertsImpl - IO thread
> > FetchClientCertPrivateKey - smartcard thread
> > SSLPlatformKey - smartcard thread
> 
> Right, so the risk I tried to capture in previous contexts was this:
> - Prompting can happen in the CryptAcquire() call for some smart cards
> - Prompting can happen in the CryptSignHash() call for other smart cards
> 
> Either of which can trigger UI and need to spin message pumps and all that
> goodness.
> 
> HCERTSTORE is "supposed" to be thread-safe (and for MSFT it is, for others,
it's
> not). NCRYPT_KEY_HANDLE does not have such guarantees (IIRC). PCCERT_CONTEXT
> does not (since it's backed by an HCERTSTORE)
> 
> I think the thread jumping may be better deferred until post-OSCertHandle, so
> that we don't have to run the risk of using/accessing the PCCERT_CONTEXT
(which
> is held/lives on the IO thread) on other threads.
> 
> I missed where FetchClientCertPrivateKey moved off the IO thread, but if we're
> using a PCCERT_CONTEXT, then we may be in trouble in doing so because of the
> X509CertificateCache and the underlying HCERTSTORE affinity.

This CL currently moves FetchClientCertPrivateKey to the background thread up at
the top of the file. So that means this CL makes the PCCERT_CONTEXT accessd on
the IO thread (GetClientCertsImpl) and then the background thread
(FetchClientCertPrivateKey). I take it then this is "supposed" to work but does
not work for other smartcards.

I didn't realize NCRYPT_KEY_HANDLE simply didn't work at all. Even before this
CL we were doing FetchClientCertPrivateKey on IO thread and the signing on the
background thread.

How about this then: In the interest of not doing everything in one already
giant CL, let's make ClientCertIdentityWin, for now, call
FetchClientCertPrivateKey on the IO thread. (Which probably means
ClientCertIdentityWin needs to be passed in a TaskRunner since AcquirePrivateKey
is driven by the UI code now.) This means the threads are the same as before.
It's apparently violating NCRYPTO_SIGN_HANDLE's guarantees, but no differently
then we always were.

Then we can have a follow-up CL that fixes all the threading in one go: make
both GetClientCertsImpl and FetchClientCertPrivateKey join the signing code on
the smartcard thread and avoid introducing awkward intermediate stages.

[commit-bot: I haz the power, 2017-06-08 21:41:33]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ryan Sleevi, 2017-06-08 21:46:16]

On 2017/06/08 21:40:49, davidben wrote:
> I didn't realize NCRYPT_KEY_HANDLE simply didn't work at all. Even before this
> CL we were doing FetchClientCertPrivateKey on IO thread and the signing on the
> background thread.

Apologies - I explained it poorly.

NCRYPT_KEY_HANDLE *should* work if it's exclusively used thread to thread (e.g.
it's never concurrently accessed on more than one thread)
HCERTSTORE is 'supposed' to work if concurrently accessed (internal locking and
such), but third-party HCERTSTOREs did not properly implement that API.
PCCERT_CONTEXT relies on the HCERTSTORE backing it, and thus suffers from the
thread-unsafety aspect - particularly and especially when reading/writing
attributes that are manifest in the HCERTSTORE (which CryptAcquire* tickles)

X509Certificate backs its OSCertHandles (ergo PCCERT_CONTEXTs) on the IO thread,
so things get all sorts of hinky, because it potentially means multi-threaded
access to a shared PCCERT_CONTEXT & HCERTSTORE. However, since we acquire on the
IO thread, we maintain the invariant of only tickling the HCERTSTORE on the IO
thread, and the UI thread (when prompting) clones the certs out of the
HCERTSTORE to try to make sure they don't tickle the same cert.


> How about this then: In the interest of not doing everything in one already
> giant CL, let's make ClientCertIdentityWin, for now, call
> FetchClientCertPrivateKey on the IO thread. (Which probably means
> ClientCertIdentityWin needs to be passed in a TaskRunner since
AcquirePrivateKey
> is driven by the UI code now.) This means the threads are the same as before.
> It's apparently violating NCRYPTO_SIGN_HANDLE's guarantees, but no differently
> then we always were.
> 
> Then we can have a follow-up CL that fixes all the threading in one go: make
> both GetClientCertsImpl and FetchClientCertPrivateKey join the signing code on
> the smartcard thread and avoid introducing awkward intermediate stages.


Yeah, SG. Hopefully with the above clarifications, provides a bit easier
guidance on a transition path :)

[mattm, 2017-06-08 21:47:56]

https://codereview.chromium.org/2898573002/diff/240001/content/public/browser...
File content/public/browser/client_certificate_delegate.h (right):

https://codereview.chromium.org/2898573002/diff/240001/content/public/browser...
content/public/browser/client_certificate_delegate.h:25: // Continue the request
with |cert| and matching |key|. |cert| or |key| may
On 2017/06/07 23:06:16, davidben wrote:
> On 2017/06/02 04:04:20, mattm wrote:
> > On 2017/06/01 23:41:25, davidben wrote:
> > > or => and?
> > 
> > It's possible for |cert| to be non-null and |key| to be null (eg, if
> > AcquirePrivateKey fails).  Maybe "and/or"?
> 
> Well, cert non-null and key null doesn't mean to continue without a
certificate.
> It means to fail really really deep in the stack with
> ERR_SSL_CLIENT_AUTH_CERT_NO_PRIVATE_KEY, which is kinda weird.
> 
> It's this bizarre thing here:
>
https://docs.google.com/document/d/13qJRJKczwMu6Cqw_xEkGhp_pZrJI7P2s-jNCjVUAg...
> 
> Maybe just leave it at |cert| may be nullptr [...] for the time being?

Done, thanks.

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/client_cert_st...
File net/ssl/client_cert_store_win.cc (right):

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/client_cert_st...
net/ssl/client_cert_store_win.cc:217: const ClientCertListCallback& callback) {
On 2017/06/08 18:13:47, Ryan Sleevi wrote:
> On 2017/06/01 23:41:25, davidben wrote:
> > Your CL makes it:
> > 
> > GetClientCertsImpl - IO thread
> > FetchClientCertPrivateKey - smartcard thread
> > SSLPlatformKey - smartcard thread
> 
> Right, so the risk I tried to capture in previous contexts was this:
> - Prompting can happen in the CryptAcquire() call for some smart cards
> - Prompting can happen in the CryptSignHash() call for other smart cards
> 
> Either of which can trigger UI and need to spin message pumps and all that
> goodness.
> 
> HCERTSTORE is "supposed" to be thread-safe (and for MSFT it is, for others,
it's
> not). NCRYPT_KEY_HANDLE does not have such guarantees (IIRC). PCCERT_CONTEXT
> does not (since it's backed by an HCERTSTORE)
> 
> I think the thread jumping may be better deferred until post-OSCertHandle, so
> that we don't have to run the risk of using/accessing the PCCERT_CONTEXT
(which
> is held/lives on the IO thread) on other threads.
> 
> I missed where FetchClientCertPrivateKey moved off the IO thread, but if we're
> using a PCCERT_CONTEXT, then we may be in trouble in doing so because of the
> X509CertificateCache and the underlying HCERTSTORE affinity.

So for context:

Before this CL, net::FetchClientCertPrivateKey is called by 
ResourceLoader::ContinueWithCertificate on the IO thread.

In the CL (up to patch set 14):
ClientCertIdentity::AcquirePrivateKey is called by
SSLClientCertificateSelector::Accept on the UI thread, 
(or directly by ChromeContentBrowserClient::SelectClientCertificate in the case
of policy auto select, also on UI thread).
AcquirePrivateKey then posts to FetchClientCertPrivateKey on the smartcard
thread.


I've changed it (in patchset 15) to instead have GetClientCertsImpl get a handle
for the thread it's on and pass that to the ClientCertIdentityWin, so that the
PCCERT_CONTEXT will only be accessed on the same thread it was created. 
That means currently it'll stay on the IO thread, but later, after the
use_byte_certs change, when GetClientCertsImpl gets moved to the smartcard
thread it'll go there too.

https://codereview.chromium.org/2898573002/diff/280001/chrome/browser/chromeo...
File chrome/browser/chromeos/net/client_cert_store_chromeos.cc (right):

https://codereview.chromium.org/2898573002/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/net/client_cert_store_chromeos.cc:46: void
ClientCertIdentityCros::AcquirePrivateKey(
On 2017/06/07 23:06:16, davidben wrote:
> Optional: Any reason not to inline this one into the declaration? Not sure
what
> the style is.

Done.

https://codereview.chromium.org/2898573002/diff/280001/chrome/browser/chromeo...
chrome/browser/chromeos/net/client_cert_store_chromeos.cc:50:
base::WorkerPool::GetTaskRunner(true /* task_is_slow */).get(),
On 2017/06/07 23:06:16, davidben wrote:
> This doesn't actually need to be on a worker thread, does it? It doesn't
really
> do anything. :-)
> 
>
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/certificate_prov...

Hm, yeah, guess so.

It feels a little weird to depend on that when using a general interface like
clientkeystore, but since it's really just a leftover, ok.

https://codereview.chromium.org/2898573002/diff/280001/chrome/browser/ssl/ssl...
File chrome/browser/ssl/ssl_client_auth_observer.cc (right):

https://codereview.chromium.org/2898573002/diff/280001/chrome/browser/ssl/ssl...
chrome/browser/ssl/ssl_client_auth_observer.cc:23: CertDetails;
On 2017/06/07 23:06:16, davidben wrote:
> Style nit: using CertDetails = ...;

Done.

https://codereview.chromium.org/2898573002/diff/280001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm (right):

https://codereview.chromium.org/2898573002/diff/280001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa.mm:221:
base::Unretained(cert->certificate())));
On 2017/06/07 23:06:16, davidben wrote:
> 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?

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

https://codereview.chromium.org/2898573002/diff/280001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa_browsertest.mm
(left):

https://codereview.chromium.org/2898573002/diff/280001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa_browsertest.mm:86:
mit_davidben_cert_ = net::ImportCertFromFile(certs_dir, "mit.davidben.der");
On 2017/06/07 23:06:16, davidben wrote:
> ...that's still in there? Hah.

yep, still hanging on. Looks like 3 more refs beyond this one.

https://codereview.chromium.org/2898573002/diff/280001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa_browsertest.mm
(right):

https://codereview.chromium.org/2898573002/diff/280001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa_browsertest.mm:134:
return client_cert_list;
On 2017/06/07 23:06:16, davidben wrote:
> Nit: Does return {base::MakeUnique<...>(...), base::MakeUnique<...>(...)}
work?

Unfortunately not, I did try that but afaict initializer lists always copy, so
don't work with move-only types.

https://codereview.chromium.org/2898573002/diff/280001/chrome/browser/ui/cryp...
File chrome/browser/ui/crypto_module_delegate_nss.cc (right):

https://codereview.chromium.org/2898573002/diff/280001/chrome/browser/ui/cryp...
chrome/browser/ui/crypto_module_delegate_nss.cc:66: crypto::ScopedPK11Slot
ChromeNSSCryptoModuleDelegate::RequestSlot() {
On 2017/06/07 23:06:17, davidben wrote:
> BTW, this slot was only used for <keygen> which is gone now. That means you
can
> get rid of the NSSCryptoModuleDelegate interface, which means you can get rid
of
> slot_, the constructor parameter,
> ChromeNSSCryptoModuleDelegate::CreateForResourceContext, and everything!

Acknowledged.

https://codereview.chromium.org/2898573002/diff/280001/chrome/browser/ui/view...
File chrome/browser/ui/views/platform_keys_certificate_selector_chromeos.cc
(right):

https://codereview.chromium.org/2898573002/diff/280001/chrome/browser/ui/view...
chrome/browser/ui/views/platform_keys_certificate_selector_chromeos.cc:50: const
net::CertificateList& certs) {
On 2017/06/07 23:06:17, davidben wrote:
> Optional: Same question about whether this should just be inlined into the
> definition.

Done.

https://codereview.chromium.org/2898573002/diff/280001/chrome/browser/ui/view...
File chrome/browser/ui/views/ssl_client_certificate_selector.cc (right):

https://codereview.chromium.org/2898573002/diff/280001/chrome/browser/ui/view...
chrome/browser/ui/views/ssl_client_certificate_selector.cc:25: #include
"chrome/browser/ui/crypto_module_password_dialog_nss.h"
On 2017/06/07 23:06:17, davidben wrote:
> No longer used? (Yay!)

Yep! removed.

https://codereview.chromium.org/2898573002/diff/280001/chrome/browser/ui/view...
chrome/browser/ui/views/ssl_client_certificate_selector.cc:78:
&SSLClientCertificateSelector::GotPrivateKey, base::Unretained(this),
On 2017/06/07 23:06:17, davidben wrote:
> Existing issue, but I'm not sure base::Unretained works here. What if the
window
> is closed while all this is going on?

Yep. Crash :/

I think at one point these used to be modal or something so that couldn't
happen? Or maybe it always was broken. dunno.

Might as well fix while I'm here.

https://codereview.chromium.org/2898573002/diff/280001/chrome/browser/ui/view...
File chrome/browser/ui/views/ssl_client_certificate_selector_browsertest.cc
(right):

https://codereview.chromium.org/2898573002/diff/280001/chrome/browser/ui/view...
chrome/browser/ui/views/ssl_client_certificate_selector_browsertest.cc:78:
net::ClientCertIdentityList cert_identity_list;
On 2017/06/07 23:06:17, davidben wrote:
> Nit: Are we allowed to write something like this these days? Ditto throughout.
> 
> net::ClientCertIdentityList cert_identity_list = {cert_identity_1_->Copy(), 
>                                                   cert_identity_2_->Copy()};
> 
> (Without the = ? I'm not sure what the style is.)
> 
> Or perhaps even:
> 
> selector_ = new SSLClientCertificateSelector(
>     browser()->tab_strip_model()->GetActiveWebContents(),
>     auth_requestor_->cert_request_info_,
>     {cert_identity_1_->Copy(), cert_identity_2_->Copy()},
>     auth_requestor_->CreateDelegate());

(Answered on the other comment, but doesn't work due to non-copyable types.)

https://codereview.chromium.org/2898573002/diff/280001/net/ssl/client_cert_st...
File net/ssl/client_cert_store_win.cc (right):

https://codereview.chromium.org/2898573002/diff/280001/net/ssl/client_cert_st...
net/ssl/client_cert_store_win.cc:42: private_key_callback) override;
On 2017/06/07 23:06:17, davidben wrote:
> Optional: What's the reason for defining this function out-of-line? I'm never
> clear on what our style is w.r.t. these things.

Done.

https://codereview.chromium.org/2898573002/diff/280001/net/ssl/client_cert_st...
net/ssl/client_cert_store_win.cc:53: base::Bind(&FetchClientCertPrivateKey,
On 2017/06/07 23:06:17, davidben wrote:
> Optional: Given this function isn't tested as part of
> ssl_platform_key_win_unittest.cc anyway, we probably could just inline it into
> ClientCertIdentityWin::AcquirePrivateKey. Dunno if that makes things any
> cleaner.

hm, dunno. I kinda like leaving it in ssl_platform_key_win. 
One is just because of the weirdness of needing to choose between the CNG or
CAPI versions, kinda seems nicer to keep that hidden.
Also it keeps things more consistent between the different impls.

[davidben, 2017-06-08 21:49:53]

On 2017/06/08 21:46:16, Ryan Sleevi wrote:
> On 2017/06/08 21:40:49, davidben wrote:
> > I didn't realize NCRYPT_KEY_HANDLE simply didn't work at all. Even before
this
> > CL we were doing FetchClientCertPrivateKey on IO thread and the signing on
the
> > background thread.
> 
> Apologies - I explained it poorly.
> 
> NCRYPT_KEY_HANDLE *should* work if it's exclusively used thread to thread
(e.g.
> it's never concurrently accessed on more than one thread)
> HCERTSTORE is 'supposed' to work if concurrently accessed (internal locking
and
> such), but third-party HCERTSTOREs did not properly implement that API.
> PCCERT_CONTEXT relies on the HCERTSTORE backing it, and thus suffers from the
> thread-unsafety aspect - particularly and especially when reading/writing
> attributes that are manifest in the HCERTSTORE (which CryptAcquire* tickles)

Ah, cool. I do not believe we are using NCRYPT_KEY_HANDLE exclusively thread to
thread. But if it has any thread affinity, we'll be sad, for now, until
everything is punted onto on thread.

> X509Certificate backs its OSCertHandles (ergo PCCERT_CONTEXTs) on the IO
thread,
> so things get all sorts of hinky, because it potentially means multi-threaded
> access to a shared PCCERT_CONTEXT & HCERTSTORE. However, since we acquire on
the
> IO thread, we maintain the invariant of only tickling the HCERTSTORE on the IO
> thread, and the UI thread (when prompting) clones the certs out of the
> HCERTSTORE to try to make sure they don't tickle the same cert.
> 
> 
> > How about this then: In the interest of not doing everything in one already
> > giant CL, let's make ClientCertIdentityWin, for now, call
> > FetchClientCertPrivateKey on the IO thread. (Which probably means
> > ClientCertIdentityWin needs to be passed in a TaskRunner since
> AcquirePrivateKey
> > is driven by the UI code now.) This means the threads are the same as
before.
> > It's apparently violating NCRYPTO_SIGN_HANDLE's guarantees, but no
differently
> > then we always were.
> > 
> > Then we can have a follow-up CL that fixes all the threading in one go: make
> > both GetClientCertsImpl and FetchClientCertPrivateKey join the signing code
on
> > the smartcard thread and avoid introducing awkward intermediate stages.
> 
> 
> Yeah, SG. Hopefully with the above clarifications, provides a bit easier
> guidance on a transition path :)

Yup. Thanks!

Now part of me wonders if there'll be any noticeable jank reduction once the
client cert lookup is moved off-thread. That'd be fun.

[commit-bot: I haz the power, 2017-06-08 22:06:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-08 22:06:58]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[mattm, 2017-06-08 22:34:24]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-08 22:35:22]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-09 00:33:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-09 00:33:49]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[mattm, 2017-06-09 00:39:17]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-09 00:40:00]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-09 02:53:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-09 02:54:00]

Dry run: This issue passed the CQ dry run.

[mattm, 2017-06-12 21:28:30]

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/client_cert_st...
File net/ssl/client_cert_store_win.cc (right):

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/client_cert_st...
net/ssl/client_cert_store_win.cc:217: const ClientCertListCallback& callback) {
On 2017/06/08 21:47:55, mattm wrote:
> On 2017/06/08 18:13:47, Ryan Sleevi wrote:
> > On 2017/06/01 23:41:25, davidben wrote:
> > > Your CL makes it:
> > > 
> > > GetClientCertsImpl - IO thread
> > > FetchClientCertPrivateKey - smartcard thread
> > > SSLPlatformKey - smartcard thread
> > 
> > Right, so the risk I tried to capture in previous contexts was this:
> > - Prompting can happen in the CryptAcquire() call for some smart cards
> > - Prompting can happen in the CryptSignHash() call for other smart cards
> > 
> > Either of which can trigger UI and need to spin message pumps and all that
> > goodness.
> > 
> > HCERTSTORE is "supposed" to be thread-safe (and for MSFT it is, for others,
> it's
> > not). NCRYPT_KEY_HANDLE does not have such guarantees (IIRC). PCCERT_CONTEXT
> > does not (since it's backed by an HCERTSTORE)
> > 
> > I think the thread jumping may be better deferred until post-OSCertHandle,
so
> > that we don't have to run the risk of using/accessing the PCCERT_CONTEXT
> (which
> > is held/lives on the IO thread) on other threads.
> > 
> > I missed where FetchClientCertPrivateKey moved off the IO thread, but if
we're
> > using a PCCERT_CONTEXT, then we may be in trouble in doing so because of the
> > X509CertificateCache and the underlying HCERTSTORE affinity.
> 
> So for context:
> 
> Before this CL, net::FetchClientCertPrivateKey is called by 
> ResourceLoader::ContinueWithCertificate on the IO thread.
> 
> In the CL (up to patch set 14):
> ClientCertIdentity::AcquirePrivateKey is called by
> SSLClientCertificateSelector::Accept on the UI thread, 
> (or directly by ChromeContentBrowserClient::SelectClientCertificate in the
case
> of policy auto select, also on UI thread).
> AcquirePrivateKey then posts to FetchClientCertPrivateKey on the smartcard
> thread.
> 
> 
> I've changed it (in patchset 15) to instead have GetClientCertsImpl get a
handle
> for the thread it's on and pass that to the ClientCertIdentityWin, so that the
> PCCERT_CONTEXT will only be accessed on the same thread it was created. 
> That means currently it'll stay on the IO thread, but later, after the
> use_byte_certs change, when GetClientCertsImpl gets moved to the smartcard
> thread it'll go there too.

FYI, I have a followup CL (after this CL and after the CL for
use_byte_certs+=win) which moves the client cert lookup (and key acquisition)
onto the SSLPlatformKeyTaskRunner:
https://codereview.chromium.org/2927193003/

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/client_key_sto...
File net/ssl/client_key_store.h (right):

https://codereview.chromium.org/2898573002/diff/240001/net/ssl/client_key_sto...
net/ssl/client_key_store.h:30: // Methods of this singleton can be called from
any thread.
On 2017/06/07 23:06:16, davidben wrote:
> On 2017/06/02 04:04:20, mattm wrote:
> > On 2017/06/01 23:41:25, davidben wrote:
> > > Yeah, these two TODOs are two parts of the same coin. :-) The intent is
that
> > > once we have a path from the ClientCertStore implementation to the
> > > SSLPrivateKey, we don't need this "provider" mess and these globals.
> > 
> > You probably didn't look at the chromeos extensions parts yet, but I didn't
> see
> > a good way to actually fully switch to ClientCertIdentities there. 
> > 
> > https://developer.chrome.com/extensions/platformKeys has
> > selectClientCertificates which returns just DER certs, and has a separate
> > getKeyPair that takes a DER cert and returns the public/private keys.  So I
> > ended up leaving that part still doing private key lookups in the old hacky
> way
> > :(
> 
> Hrm. Well, if we could at least avoid a mess of globals and "provider"
> interfaces, that would be something. That the leaf operation is a cert => key
> lookup is annoying, but not far from how all the other platforms work.
> 
> Disjointed incoherent babbling of ideas: CrOS ClientCertStore implementation
> could just query the CertificateProviderService and be done with it. Or if the
> interface between CrOS's ClientCertStore and CertificateProviderService is
still
> valuable, we can at least fold the CertProvider and CertKeyProvider interfaces
> into one thing. PErhaps rather than the additional_certs mess,
> CertificateProviderService can just vend out a bunch of ClientCertIdentities?
> 
> (Not suggesting this be in the same CL, of course.)

FYI, I have a followup CL to delete ClientKeyStore and have CertProviderService
return ClientCertIdenties directly:
https://codereview.chromium.org/2937553003/

[davidben, 2017-06-13 18:43:52]

lgtm!

[mattm, 2017-06-13 19:40:00]

mattm@chromium.org changed reviewers:
+ ellyjones@chromium.org, emaxx@chromium.org, lcwu@chromium.org,
pfeldman@chromium.org, pkasting@chromium.org, sergeyu@chromium.org,
sgurun@chromium.org, tedchoc@chromium.org

[mattm, 2017-06-13 19:40:03]

+owners:
sgurun: android_webview
emaxx: chrome/browser/chromeos/platform_keys
rsleevi: chrome/browser/ssl
tedchoc: chrome/browser/ui/android
ellyjones: chrome/browser/ui/cocoa
pkasting: chrome/browser/ui/crypto_module_delegate_nss.h &
chrome/browser/ui/views
lcwu: chromecast
pfeldman: chrome/browser/devtools & content/
sergeyu: remoting/

[Elly Fong-Jones, 2017-06-13 19:45:35]

c/b/ui/cocoa/* lgtm

https://codereview.chromium.org/2898573002/diff/360001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa_browsertest.mm
(right):

https://codereview.chromium.org/2898573002/diff/360001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa_browsertest.mm:37:
// present in 10.5 through 10.8.
is this comment current? 10.5 through 10.8?

[Sergey Ulanov, 2017-06-13 19:49:59]

remoting lgtm

[halliwell, 2017-06-13 22:37:12]

On 2017/06/13 19:49:59, Sergey Ulanov wrote:
> remoting lgtm

chromecast/ lgtm

[sgurun-gerrit only, 2017-06-13 22:48:34]

https://codereview.chromium.org/2898573002/diff/360001/android_webview/browse...
File android_webview/browser/aw_contents_client_bridge.cc (right):

https://codereview.chromium.org/2898573002/diff/360001/android_webview/browse...
android_webview/browser/aw_contents_client_bridge.cc:259: // TODO(mattm): make
this a unique_ptr and get rid of the guard stuff.
why not do it in this CL?

[mattm, 2017-06-13 23:40:59]

https://codereview.chromium.org/2898573002/diff/360001/android_webview/browse...
File android_webview/browser/aw_contents_client_bridge.cc (right):

https://codereview.chromium.org/2898573002/diff/360001/android_webview/browse...
android_webview/browser/aw_contents_client_bridge.cc:259: // TODO(mattm): make
this a unique_ptr and get rid of the guard stuff.
On 2017/06/13 22:48:34, sgurun wrote:
> why not do it in this CL?

Done.

https://codereview.chromium.org/2898573002/diff/360001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa_browsertest.mm
(right):

https://codereview.chromium.org/2898573002/diff/360001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/ssl_client_certificate_selector_cocoa_browsertest.mm:37:
// present in 10.5 through 10.8.
On 2017/06/13 19:45:35, Elly Fong-Jones wrote:
> is this comment current? 10.5 through 10.8?

Ah, that was just copied from ssl_client_certificate_selector_cocoa.mm. I'll
update them.

[sgurun-gerrit only, 2017-06-13 23:43:15]

aw lgtm

[mattm, 2017-06-13 23:47:44]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-13 23:48:21]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[emaxx, 2017-06-14 00:36:44]

lgtm for chrome/browser/chromeos/platform_keys

[commit-bot: I haz the power, 2017-06-14 01:43:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-14 01:43:12]

Dry run: This issue passed the CQ dry run.

[mattm, 2017-06-15 01:33:47]

ping pfeldman, rsleevi, tedchoc, pkasting

[Ted C, 2017-06-15 14:15:56]

On 2017/06/15 01:33:47, mattm wrote:
> ping pfeldman, rsleevi, tedchoc, pkasting

chrome/browser/ui/android - lgtm

[Peter Kasting, 2017-06-15 22:13:41]

https://codereview.chromium.org/2898573002/diff/400001/chrome/browser/ui/view...
File chrome/browser/ui/views/certificate_selector.cc (right):

https://codereview.chromium.org/2898573002/diff/400001/chrome/browser/ui/view...
chrome/browser/ui/views/certificate_selector.cc:116:
CertificateSelector::CertificateSelector(net::ClientCertIdentityList identities,
I think we probably still want to pass by reference, especially since we're not
always moving the whole vector below?  For example, see guidance at
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/string...
.

https://codereview.chromium.org/2898573002/diff/400001/chrome/browser/ui/view...
chrome/browser/ui/views/certificate_selector.cc:256: return
std::move(identities_[selected]);
Don't we also need to do identities_.erase()?  Otherwise we leave a dead slot in
the vector.  I mean, maybe that doesn't cause problems today, if nobody calls
this until the vector won't be used anymore anyway, but it feels like an unsafe
pattern.

https://codereview.chromium.org/2898573002/diff/400001/chrome/browser/ui/view...
chrome/browser/ui/views/certificate_selector.cc:289: if (sender ==
view_cert_button_ && GetSelectedCert()) {
Nit: I liked that the old code avoided calling AnExpensiveLookingFunctionName()
repeatedly but stored it in a temp.

https://codereview.chromium.org/2898573002/diff/400001/chrome/browser/ui/view...
File chrome/browser/ui/views/certificate_selector.h (right):

https://codereview.chromium.org/2898573002/diff/400001/chrome/browser/ui/view...
chrome/browser/ui/views/certificate_selector.h:56: // Take ownership of the
currently selected certificate or null if none is
Nit: Take -> Takes (see note about imperative comments in
http://google.github.io/styleguide/cppguide.html#Function_Comments )

I don't know what it means to "take ownership of...null".  In general, this
comment sounds like this class takes ownership of something, but instead the
implementation says it's doing the opposite.  I'm not sure how best to clarify
this.  I can't think of a better function name, so maybe it's just the comment
that's confusing me?  Maybe "Like GetSelectedCert(), but passes ownership of the
selected cert to the caller."?

https://codereview.chromium.org/2898573002/diff/400001/chrome/browser/ui/view...
chrome/browser/ui/views/certificate_selector.h:57: // selected.  Must be called
after |InitWithText()|.
Nit: No need for || on function names (the () is their signifier)

https://codereview.chromium.org/2898573002/diff/400001/chrome/browser/ui/view...
File chrome/browser/ui/views/platform_keys_certificate_selector_chromeos.cc
(right):

https://codereview.chromium.org/2898573002/diff/400001/chrome/browser/ui/view...
chrome/browser/ui/views/platform_keys_certificate_selector_chromeos.cc:28: //
platformKeys API currently only deals in certificates, not identities.
Nit: Leading "The"

https://codereview.chromium.org/2898573002/diff/400001/chrome/browser/ui/view...
chrome/browser/ui/views/platform_keys_certificate_selector_chromeos.cc:41:
private_key_callback.Run(nullptr);
It doesn't make sense to me to execute any code after a NOTREACHED().  This is
basically like handling DCHECK failure, which the styleguide bans.  If it's
NOTREACHED(), it's truly never reached.

https://codereview.chromium.org/2898573002/diff/400001/chrome/browser/ui/view...
File chrome/browser/ui/views/ssl_client_certificate_selector.h (right):

https://codereview.chromium.org/2898573002/diff/400001/chrome/browser/ui/view...
chrome/browser/ui/views/ssl_client_certificate_selector.h:56:
base::WeakPtrFactory<SSLClientCertificateSelector> weak_ptr_factory_;
Nit: Optionally, init here using "{this}" to avoid doing it in the .cc file. 
(Would be more compelling if there were more members or multiple constructors.)

https://codereview.chromium.org/2898573002/diff/400001/chrome/browser/ui/view...
File chrome/browser/ui/views/ssl_client_certificate_selector_browsertest.cc
(right):

https://codereview.chromium.org/2898573002/diff/400001/chrome/browser/ui/view...
chrome/browser/ui/views/ssl_client_certificate_selector_browsertest.cc:80:
cert_identity_list.push_back(cert_identity_2_->Copy());
Nit: Or use {} to put these two items in the list as it's declared, avoiding
push_back(). (multiple places)

[mattm, 2017-06-16 03:28:37]

Patchset #22 (id:440001) has been deleted

[mattm, 2017-06-16 03:30:43]

https://codereview.chromium.org/2898573002/diff/400001/chrome/browser/ui/view...
File chrome/browser/ui/views/certificate_selector.cc (right):

https://codereview.chromium.org/2898573002/diff/400001/chrome/browser/ui/view...
chrome/browser/ui/views/certificate_selector.cc:116:
CertificateSelector::CertificateSelector(net::ClientCertIdentityList identities,
On 2017/06/15 22:13:41, Peter Kasting wrote:
> I think we probably still want to pass by reference, especially since we're
not
> always moving the whole vector below?  For example, see guidance at
>
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/string...
> .

I don't think that post is relevant, since the list isn't copyable.

This class is self-owning and completes asynchronously, the caller may not
continue to exist, so we can't depend on it to keep the list alive if it were
passed by reference, so we'd always have to move some or all the elements out of
the list. In the chromeos case it might be tiny bit more efficient(all you'd be
avoiding is a std::move(std::vector) which should be pretty cheap), but the
non-chromeos case would be clearly less efficient.

https://codereview.chromium.org/2898573002/diff/400001/chrome/browser/ui/view...
chrome/browser/ui/views/certificate_selector.cc:256: return
std::move(identities_[selected]);
On 2017/06/15 22:13:40, Peter Kasting wrote:
> Don't we also need to do identities_.erase()?  Otherwise we leave a dead slot
in
> the vector.  I mean, maybe that doesn't cause problems today, if nobody calls
> this until the vector won't be used anymore anyway, but it feels like an
unsafe
> pattern.

I think the only reasonable thing is that once a cert is accepted no further
accesses should happen. Otherwise things just get weird. I've attempted to
refactor and fix some issues(eg disabling the dialog controls while waiting for
the async callback).

https://codereview.chromium.org/2898573002/diff/400001/chrome/browser/ui/view...
chrome/browser/ui/views/certificate_selector.cc:289: if (sender ==
view_cert_button_ && GetSelectedCert()) {
On 2017/06/15 22:13:40, Peter Kasting wrote:
> Nit: I liked that the old code avoided calling
AnExpensiveLookingFunctionName()
> repeatedly but stored it in a temp.

Done.

https://codereview.chromium.org/2898573002/diff/400001/chrome/browser/ui/view...
File chrome/browser/ui/views/certificate_selector.h (right):

https://codereview.chromium.org/2898573002/diff/400001/chrome/browser/ui/view...
chrome/browser/ui/views/certificate_selector.h:56: // Take ownership of the
currently selected certificate or null if none is
On 2017/06/15 22:13:41, Peter Kasting wrote:
> Nit: Take -> Takes (see note about imperative comments in
> http://google.github.io/styleguide/cppguide.html#Function_Comments )

done
 
> I don't know what it means to "take ownership of...null".  In general, this
> comment sounds like this class takes ownership of something, but instead the
> implementation says it's doing the opposite.  I'm not sure how best to clarify
> this.  I can't think of a better function name, so maybe it's just the comment
> that's confusing me?  Maybe "Like GetSelectedCert(), but passes ownership of
the
> selected cert to the caller."?

Changed up the interface a bit, wdyt?

https://codereview.chromium.org/2898573002/diff/400001/chrome/browser/ui/view...
chrome/browser/ui/views/certificate_selector.h:57: // selected.  Must be called
after |InitWithText()|.
On 2017/06/15 22:13:41, Peter Kasting wrote:
> Nit: No need for || on function names (the () is their signifier)

Acknowledged.

https://codereview.chromium.org/2898573002/diff/400001/chrome/browser/ui/view...
File chrome/browser/ui/views/platform_keys_certificate_selector_chromeos.cc
(right):

https://codereview.chromium.org/2898573002/diff/400001/chrome/browser/ui/view...
chrome/browser/ui/views/platform_keys_certificate_selector_chromeos.cc:28: //
platformKeys API currently only deals in certificates, not identities.
On 2017/06/15 22:13:41, Peter Kasting wrote:
> Nit: Leading "The"

Done.

https://codereview.chromium.org/2898573002/diff/400001/chrome/browser/ui/view...
chrome/browser/ui/views/platform_keys_certificate_selector_chromeos.cc:41:
private_key_callback.Run(nullptr);
On 2017/06/15 22:13:41, Peter Kasting wrote:
> It doesn't make sense to me to execute any code after a NOTREACHED().  This is
> basically like handling DCHECK failure, which the styleguide bans.  If it's
> NOTREACHED(), it's truly never reached.

Done.

https://codereview.chromium.org/2898573002/diff/400001/chrome/browser/ui/view...
File chrome/browser/ui/views/ssl_client_certificate_selector.h (right):

https://codereview.chromium.org/2898573002/diff/400001/chrome/browser/ui/view...
chrome/browser/ui/views/ssl_client_certificate_selector.h:56:
base::WeakPtrFactory<SSLClientCertificateSelector> weak_ptr_factory_;
On 2017/06/15 22:13:41, Peter Kasting wrote:
> Nit: Optionally, init here using "{this}" to avoid doing it in the .cc file. 
> (Would be more compelling if there were more members or multiple
constructors.)

Done.

https://codereview.chromium.org/2898573002/diff/400001/chrome/browser/ui/view...
File chrome/browser/ui/views/ssl_client_certificate_selector_browsertest.cc
(right):

https://codereview.chromium.org/2898573002/diff/400001/chrome/browser/ui/view...
chrome/browser/ui/views/ssl_client_certificate_selector_browsertest.cc:80:
cert_identity_list.push_back(cert_identity_2_->Copy());
On 2017/06/15 22:13:41, Peter Kasting wrote:
> Nit: Or use {} to put these two items in the list as it's declared, avoiding
> push_back(). (multiple places)

Unfortunately, initializer lists appear to be incompatible with move-only types

[mattm, 2017-06-16 03:33:41]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-16 03:33:53]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mattm, 2017-06-16 03:55:12]

https://codereview.chromium.org/2898573002/diff/400001/chrome/browser/ui/view...
File chrome/browser/ui/views/certificate_selector.cc (right):

https://codereview.chromium.org/2898573002/diff/400001/chrome/browser/ui/view...
chrome/browser/ui/views/certificate_selector.cc:116:
CertificateSelector::CertificateSelector(net::ClientCertIdentityList identities,
On 2017/06/16 03:30:42, mattm wrote:
> On 2017/06/15 22:13:41, Peter Kasting wrote:
> > I think we probably still want to pass by reference, especially since we're
> not
> > always moving the whole vector below?  For example, see guidance at
> >
>
https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/string...
> > .
> 
> I don't think that post is relevant, since the list isn't copyable.
> 
> This class is self-owning and completes asynchronously, the caller may not
> continue to exist, so we can't depend on it to keep the list alive if it were
> passed by reference, so we'd always have to move some or all the elements out
of
> the list. In the chromeos case it might be tiny bit more efficient(all you'd
be
> avoiding is a std::move(std::vector) which should be pretty cheap), but the
> non-chromeos case would be clearly less efficient.

Scratch that last part, you could of course still move the whole list and not
just individual elements, if a reference was passed in.

Still, I think this way is clearer since you have to put the std::move at the
call site, it makes it obvious that the caller is giving up ownership of the
contents of the list.

[commit-bot: I haz the power, 2017-06-16 05:44:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-16 05:44:09]

Dry run: This issue passed the CQ dry run.

[Ryan Sleevi, 2017-06-16 16:05:04]

Description was changed from

==========
Refactor client cert private key handling.

Removes the global FetchClientCertPrivateKey function which needed to be able to
find the private key given just an X509Certificate object.
Now ClientCertStore returns ClientCertIdentity objects which contain a
certificate and have a method to asynchronously retrieve the matching
SSLPrivateKey object.

Also re-enable SSLClientCertificateSelectorCocoaTest.Basic.

BUG=394131,313243,222296
==========

to

==========
Refactor client cert private key handling.

This removes the global FetchClientCertPrivateKey function, which needed to be
able
to find the private key given just an X509Certificate object. Now,
ClientCertStore
returns ClientCertIdentity objects, which contain a certificate and have a
method to
asynchronously retrieve the matching SSLPrivateKey object.

Also re-enable SSLClientCertificateSelectorCocoaTest.Basic.

BUG=394131,313243,222296
==========

[Ryan Sleevi, 2017-06-16 16:10:54]

lgtm

[Peter Kasting, 2017-06-16 23:29:53]

https://codereview.chromium.org/2898573002/diff/400001/chrome/browser/ui/view...
File chrome/browser/ui/views/ssl_client_certificate_selector_browsertest.cc
(right):

https://codereview.chromium.org/2898573002/diff/400001/chrome/browser/ui/view...
chrome/browser/ui/views/ssl_client_certificate_selector_browsertest.cc:80:
cert_identity_list.push_back(cert_identity_2_->Copy());
On 2017/06/16 03:30:43, mattm wrote:
> On 2017/06/15 22:13:41, Peter Kasting wrote:
> > Nit: Or use {} to put these two items in the list as it's declared, avoiding
> > push_back(). (multiple places)
> 
> Unfortunately, initializer lists appear to be incompatible with move-only
types

Yeah, StackOverflow have some ideas here but all of them are uglier than just
doing this manually.

https://codereview.chromium.org/2898573002/diff/460001/chrome/browser/ui/view...
File chrome/browser/ui/views/certificate_selector.cc (right):

https://codereview.chromium.org/2898573002/diff/460001/chrome/browser/ui/view...
chrome/browser/ui/views/certificate_selector.cc:247:
CHECK_LT(static_cast<size_t>(selected), identities_.size());
Nit: Seems like this should be DCHECK?  (I think the old code was wrong)

https://codereview.chromium.org/2898573002/diff/460001/chrome/browser/ui/view...
chrome/browser/ui/views/certificate_selector.cc:263:
CHECK_LT(static_cast<size_t>(selected), identities_.size());
Nit: Seems like this should be DCHECK too?

https://codereview.chromium.org/2898573002/diff/460001/chrome/browser/ui/view...
chrome/browser/ui/views/certificate_selector.cc:264: return
AcceptCertificate(std::move(identities_[selected]));
So, if AcceptCertificate() returns false, then we leave the dialog onscreen but
with various things disabled?

It seems like maybe a better solution might be to not disable anything above,
and not return a bool, but instead Close() the view from here, and then just
AcceptCertificate().  (Note that Close() does a sync hide + async close, so it
won't delete any data structures until the callstack returns.)

I think in that case we can nuke the |certificate_accepted_| variable too.

The only question mark here would be if closing the dialog also deletes this
object, and you need to read from this object after this callstack would have
otherwise returned.  If both these are the case, then the ideal solution would
be to either pass along the data you'd have read out of this object (so it can
be deleted now), or make this object's lifetime able to extend past the dialog's
lifetime, or split the data that needs to live longer into a separate model
object that can live longer -- but in any case, to avoid leaving the dialog
onscreen, since that's less responsive UI.

https://codereview.chromium.org/2898573002/diff/460001/chrome/browser/ui/view...
File chrome/browser/ui/views/certificate_selector.h (right):

https://codereview.chromium.org/2898573002/diff/460001/chrome/browser/ui/view...
chrome/browser/ui/views/certificate_selector.h:102: bool certificate_accepted_ =
false;
Nit: Not necessary if you don't want since the file is already inconsistent, but
it would be nice to init the other basic types below if possible (in particular,
|table_| and |view_cert_button_|), so we don't have some things inited in the
declaration and some in the definition (except where necessary).

https://codereview.chromium.org/2898573002/diff/460001/chrome/browser/ui/view...
File chrome/browser/ui/views/platform_keys_certificate_selector_chromeos.cc
(right):

https://codereview.chromium.org/2898573002/diff/460001/chrome/browser/ui/view...
chrome/browser/ui/views/platform_keys_certificate_selector_chromeos.cc:103:
.Run(make_scoped_refptr(identity->certificate()));
Possible future improvement: Switch the callback type to a OnceCallback so it
will get reset automatically.

[mattm, 2017-06-17 03:08:37]

Patchset #23 (id:480001) has been deleted

[mattm, 2017-06-17 03:18:24]

Patchset #23 (id:500001) has been deleted

[mattm, 2017-06-17 03:19:22]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-17 03:19:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mattm, 2017-06-17 03:20:07]

https://codereview.chromium.org/2898573002/diff/460001/chrome/browser/ui/view...
File chrome/browser/ui/views/certificate_selector.cc (right):

https://codereview.chromium.org/2898573002/diff/460001/chrome/browser/ui/view...
chrome/browser/ui/views/certificate_selector.cc:247:
CHECK_LT(static_cast<size_t>(selected), identities_.size());
On 2017/06/16 23:29:52, Peter Kasting wrote:
> Nit: Seems like this should be DCHECK?  (I think the old code was wrong)

Done.

https://codereview.chromium.org/2898573002/diff/460001/chrome/browser/ui/view...
chrome/browser/ui/views/certificate_selector.cc:263:
CHECK_LT(static_cast<size_t>(selected), identities_.size());
On 2017/06/16 23:29:52, Peter Kasting wrote:
> Nit: Seems like this should be DCHECK too?

Done.

https://codereview.chromium.org/2898573002/diff/460001/chrome/browser/ui/view...
chrome/browser/ui/views/certificate_selector.cc:264: return
AcceptCertificate(std::move(identities_[selected]));
On 2017/06/16 23:29:52, Peter Kasting wrote:
> So, if AcceptCertificate() returns false, then we leave the dialog onscreen
but
> with various things disabled?
> 
> It seems like maybe a better solution might be to not disable anything above,
> and not return a bool, but instead Close() the view from here, and then just
> AcceptCertificate().  (Note that Close() does a sync hide + async close, so it
> won't delete any data structures until the callstack returns.)

Done. (Though I just did it by returning true, I think that's basically
equivalent to calling Close() directly?)

> I think in that case we can nuke the |certificate_accepted_| variable too.
> 
> The only question mark here would be if closing the dialog also deletes this
> object, and you need to read from this object after this callstack would have
> otherwise returned.  If both these are the case, then the ideal solution would
> be to either pass along the data you'd have read out of this object (so it can
> be deleted now), or make this object's lifetime able to extend past the
dialog's
> lifetime, or split the data that needs to live longer into a separate model
> object that can live longer -- but in any case, to avoid leaving the dialog
> onscreen, since that's less responsive UI.

Yeah, the selector needed to continue to exist until the private key was
retrieved. I refactored it to split out the part that needs to stay alive, so
now the dialog can close immediately.

https://codereview.chromium.org/2898573002/diff/460001/chrome/browser/ui/view...
File chrome/browser/ui/views/certificate_selector.h (right):

https://codereview.chromium.org/2898573002/diff/460001/chrome/browser/ui/view...
chrome/browser/ui/views/certificate_selector.h:102: bool certificate_accepted_ =
false;
On 2017/06/16 23:29:52, Peter Kasting wrote:
> Nit: Not necessary if you don't want since the file is already inconsistent,
but
> it would be nice to init the other basic types below if possible (in
particular,
> |table_| and |view_cert_button_|), so we don't have some things inited in the
> declaration and some in the definition (except where necessary).

Done.

https://codereview.chromium.org/2898573002/diff/460001/chrome/browser/ui/view...
File chrome/browser/ui/views/platform_keys_certificate_selector_chromeos.cc
(right):

https://codereview.chromium.org/2898573002/diff/460001/chrome/browser/ui/view...
chrome/browser/ui/views/platform_keys_certificate_selector_chromeos.cc:103:
.Run(make_scoped_refptr(identity->certificate()));
On 2017/06/16 23:29:52, Peter Kasting wrote:
> Possible future improvement: Switch the callback type to a OnceCallback so it
> will get reset automatically.

Acknowledged.

[Peter Kasting, 2017-06-17 03:32:24]

LGTM!  Thanks for the thoughtful refactors here, I think we came out somewhere
good.

[mattm, 2017-06-17 03:43:42]

The CQ bit was checked by mattm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-17 03:43:53]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-17 05:05:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-17 05:05:30]

Dry run: This issue passed the CQ dry run.

[pfeldman, 2017-06-19 17:36:07]

lgtm

[mattm, 2017-06-19 18:06:09]

The CQ bit was checked by mattm@chromium.org

[mattm, 2017-06-19 18:06:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
sergeyu@chromium.org, davidben@chromium.org, ellyjones@chromium.org,
halliwell@chromium.org, sgurun@chromium.org, tedchoc@chromium.org,
emaxx@chromium.org, rsleevi@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2898573002/#ps540001
(title: "removed no longer needed forward declaration")

[commit-bot: I haz the power, 2017-06-19 18:06:23]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-19 20:24:12]

CQ is committing da patch.

Bot data: {"patchset_id": 540001, "attempt_start_ts": 1497895569208430,
"parent_rev": "9d7e219c5242cd3a3cb38c982de807ac52d3e0ee", "commit_rev":
"c9009d864f6ad318386de874aa439a98edc20bba"}

[commit-bot: I haz the power, 2017-06-19 20:24:31]

CQ is committing da patch.

Bot data: {"patchset_id": 540001, "attempt_start_ts": 1497895569208430,
"parent_rev": "808540a1356b6cc5bebaf0eabc15a333823985df", "commit_rev":
"436ccfe33a1747eb5e4d4b9ed948d498d560bf19"}

[commit-bot: I haz the power, 2017-06-19 20:24:42]

Description was changed from

==========
Refactor client cert private key handling.

This removes the global FetchClientCertPrivateKey function, which needed to be
able
to find the private key given just an X509Certificate object. Now,
ClientCertStore
returns ClientCertIdentity objects, which contain a certificate and have a
method to
asynchronously retrieve the matching SSLPrivateKey object.

Also re-enable SSLClientCertificateSelectorCocoaTest.Basic.

BUG=394131,313243,222296
==========

to

==========
Refactor client cert private key handling.

This removes the global FetchClientCertPrivateKey function, which needed to be
able
to find the private key given just an X509Certificate object. Now,
ClientCertStore
returns ClientCertIdentity objects, which contain a certificate and have a
method to
asynchronously retrieve the matching SSLPrivateKey object.

Also re-enable SSLClientCertificateSelectorCocoaTest.Basic.

BUG=394131,313243,222296

Review-Url: https://codereview.chromium.org/2898573002
Cr-Commit-Position: refs/heads/master@{#480556}
Committed:
https://chromium.googlesource.com/chromium/src/+/436ccfe33a1747eb5e4d4b9ed948...
==========

[commit-bot: I haz the power, 2017-06-19 20:24:46]

Committed patchset #24 (id:540001) as
https://chromium.googlesource.com/chromium/src/+/436ccfe33a1747eb5e4d4b9ed948...