Make CertificateProviderService vend ClientCertIdentities directly.

Make CertificateProviderService vend ClientCertIdentities directly.

Deletes net::ClientKeyStore, of which CertificateProviderService was the last remaining use.

BUG=394131
Review-Url: https://codereview.chromium.org/2937553003
Cr-Commit-Position: refs/heads/master@{#480638}
Committed: https://chromium.googlesource.com/chromium/src/+/bbf7fc0fd86aea1e927337256934b0e0fb82eaf0

[mattm, 2017-06-12 21:14:47]

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

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

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-12 21:20:40]

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

[commit-bot: I haz the power, 2017-06-12 21:20:46]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  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-06-13 19:42:05]

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

[davidben, 2017-06-16 00:28:25]

lgtm, but probably hopefully one of the owners of certificate_provider can
provide a somewhat better review of that code than me. :-)

https://codereview.chromium.org/2937553003/diff/1/chrome/browser/chromeos/cer...
File
chrome/browser/chromeos/certificate_provider/certificate_provider_service.cc
(right):

https://codereview.chromium.org/2937553003/diff/1/chrome/browser/chromeos/cer...
chrome/browser/chromeos/certificate_provider/certificate_provider_service.cc:44:
base::BindOnce(callback, std::move(certs)));
[The sheer amount of code in Chromium that's devoted to thread-hopping is kind
of mind-blowing.]

https://codereview.chromium.org/2937553003/diff/1/chrome/browser/chromeos/cer...
chrome/browser/chromeos/certificate_provider/certificate_provider_service.cc:173:
// extension instead of having to go through service_->certificate_map_ ?
Looks like you have the callback available at UpdateCertificatesAndRun, right?
That'll also fix issues if two extensions advertise the same cert. Though yeah,
maybe separate CL makes sense and probably there's more involved here. *shrug*

https://codereview.chromium.org/2937553003/diff/1/chrome/browser/chromeos/cer...
File
chrome/browser/chromeos/certificate_provider/certificate_provider_service_unittest.cc
(right):

https://codereview.chromium.org/2937553003/diff/1/chrome/browser/chromeos/cer...
chrome/browser/chromeos/certificate_provider/certificate_provider_service_unittest.cc:49:
void SavePrivateKey(scoped_refptr<net::SSLPrivateKey>* out_key,
Nit: vaguely odd that this and the above function do exactly the same thing but
are named differently.

https://codereview.chromium.org/2937553003/diff/1/chrome/browser/chromeos/cer...
chrome/browser/chromeos/certificate_provider/certificate_provider_service_unittest.cc:459:
std::unique_ptr<net::ClientCertIdentity> cert(ProvideDefaultCert());
ASSERT_TRUE(cert)?

https://codereview.chromium.org/2937553003/diff/1/chrome/browser/chromeos/cer...
chrome/browser/chromeos/certificate_provider/certificate_provider_service_unittest.cc:496:
std::unique_ptr<net::ClientCertIdentity> cert(ProvideDefaultCert());
Ditto.

[mattm, 2017-06-16 22:34:55]

(I forgot to upload the rebase patchset, but the rebase only changes BUILD.gn
anyway, so whatevs)

https://codereview.chromium.org/2937553003/diff/1/chrome/browser/chromeos/cer...
File
chrome/browser/chromeos/certificate_provider/certificate_provider_service.cc
(right):

https://codereview.chromium.org/2937553003/diff/1/chrome/browser/chromeos/cer...
chrome/browser/chromeos/certificate_provider/certificate_provider_service.cc:173:
// extension instead of having to go through service_->certificate_map_ ?
On 2017/06/16 00:28:25, davidben wrote:
> Looks like you have the callback available at UpdateCertificatesAndRun, right?
> That'll also fix issues if two extensions advertise the same cert. Though
yeah,
> maybe separate CL makes sense and probably there's more involved here. *shrug*

Yeah I think the complication will be handling extensions that get removed while
a ClientCertIdentity is in flight but before the private key is acquired. Right
now certificate_map_ handles all that. I guess it probably shouldn't be too hard
to check directly if the extension still exists. On the other hand, I also
suppose checking that is a bit of questionable benefit since there's still the
possibility that the extension gets unloaded between AcquirePrivateKey and the
actual signing operation ..

The other issue is that doing this wouldn't buy much unless we remove the other
dependencies on certificate_map_ as well.

So yeah, probably better to leave this to a separate change.

https://codereview.chromium.org/2937553003/diff/1/chrome/browser/chromeos/cer...
File
chrome/browser/chromeos/certificate_provider/certificate_provider_service_unittest.cc
(right):

https://codereview.chromium.org/2937553003/diff/1/chrome/browser/chromeos/cer...
chrome/browser/chromeos/certificate_provider/certificate_provider_service_unittest.cc:49:
void SavePrivateKey(scoped_refptr<net::SSLPrivateKey>* out_key,
On 2017/06/16 00:28:25, davidben wrote:
> Nit: vaguely odd that this and the above function do exactly the same thing
but
> are named differently.

Done.

https://codereview.chromium.org/2937553003/diff/1/chrome/browser/chromeos/cer...
chrome/browser/chromeos/certificate_provider/certificate_provider_service_unittest.cc:459:
std::unique_ptr<net::ClientCertIdentity> cert(ProvideDefaultCert());
On 2017/06/16 00:28:25, davidben wrote:
> ASSERT_TRUE(cert)?

Done.

https://codereview.chromium.org/2937553003/diff/1/chrome/browser/chromeos/cer...
chrome/browser/chromeos/certificate_provider/certificate_provider_service_unittest.cc:496:
std::unique_ptr<net::ClientCertIdentity> cert(ProvideDefaultCert());
On 2017/06/16 00:28:25, davidben wrote:
> Ditto.

Done.

[mattm, 2017-06-16 22:39:43]

mattm@chromium.org changed reviewers:
+ emaxx@chromium.org, thestig@chromium.org

[mattm, 2017-06-16 22:39:43]

emaxx: chrome/browser/chromeos/certificate_provider/*
thestig: chrome/browser/certificate_manager_model.*

(note: this is a followup CL to https://codereview.chromium.org/2898573002/)

[Lei Zhang, 2017-06-16 23:03:25]

lgtm

https://codereview.chromium.org/2937553003/diff/20001/chrome/browser/certific...
File chrome/browser/certificate_manager_model.cc (right):

https://codereview.chromium.org/2937553003/diff/20001/chrome/browser/certific...
chrome/browser/certificate_manager_model.cc:147: extension_cert_list_.clear();
Is the size of |new_cert_identities| typically large enough that it would help
to reserve(new_cert_identities.size()) before the for-loop?

[emaxx, 2017-06-19 13:59:11]

LGTM with nits.

https://codereview.chromium.org/2937553003/diff/20001/chrome/browser/chromeos...
File
chrome/browser/chromeos/certificate_provider/certificate_provider_service.cc
(right):

https://codereview.chromium.org/2937553003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/certificate_provider/certificate_provider_service.cc:132:
~ClientCertIdentity() override = default;
nit: Is this required?

https://codereview.chromium.org/2937553003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/certificate_provider/certificate_provider_service.cc:139:
scoped_refptr<net::SSLPrivateKey> GetCertificateKeyOnServiceThread(
nit: Rename to AcquirePrivateKeyOnServiceThread (to show how it's related to
AcquirePrivateKey)?

https://codereview.chromium.org/2937553003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/certificate_provider/certificate_provider_service.cc:155:
base::Unretained(this), base::Unretained(certificate())),
nit: Please add a comment why it's safe to use base::Unretained here (I see the
explanation in net/ssl/client_cert_identity.h, but it's non-obvious here in this
file).

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

Patchset #3 (id:40001) has been deleted

[mattm, 2017-06-19 21:21:03]

https://codereview.chromium.org/2937553003/diff/20001/chrome/browser/certific...
File chrome/browser/certificate_manager_model.cc (right):

https://codereview.chromium.org/2937553003/diff/20001/chrome/browser/certific...
chrome/browser/certificate_manager_model.cc:147: extension_cert_list_.clear();
On 2017/06/16 23:03:24, Lei Zhang wrote:
> Is the size of |new_cert_identities| typically large enough that it would help
> to reserve(new_cert_identities.size()) before the for-loop?

Hm, probably not usually, but wouldn't hurt.
Done.

https://codereview.chromium.org/2937553003/diff/20001/chrome/browser/chromeos...
File
chrome/browser/chromeos/certificate_provider/certificate_provider_service.cc
(right):

https://codereview.chromium.org/2937553003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/certificate_provider/certificate_provider_service.cc:132:
~ClientCertIdentity() override = default;
On 2017/06/19 13:59:11, emaxx wrote:
> nit: Is this required?

nope, removed.

https://codereview.chromium.org/2937553003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/certificate_provider/certificate_provider_service.cc:139:
scoped_refptr<net::SSLPrivateKey> GetCertificateKeyOnServiceThread(
On 2017/06/19 13:59:11, emaxx wrote:
> nit: Rename to AcquirePrivateKeyOnServiceThread (to show how it's related to
> AcquirePrivateKey)?

Done.

https://codereview.chromium.org/2937553003/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/certificate_provider/certificate_provider_service.cc:155:
base::Unretained(this), base::Unretained(certificate())),
On 2017/06/19 13:59:11, emaxx wrote:
> nit: Please add a comment why it's safe to use base::Unretained here (I see
the
> explanation in net/ssl/client_cert_identity.h, but it's non-obvious here in
this
> file).

Done.

[mattm, 2017-06-19 22:01:01]

The CQ bit was checked by mattm@chromium.org

[mattm, 2017-06-19 22:01:02]

The patchset sent to the CQ was uploaded after l-g-t-m from
davidben@chromium.org, thestig@chromium.org, emaxx@chromium.org
Link to the patchset: https://codereview.chromium.org/2937553003/#ps80001
(title: "review changes for comments 11 & 12")

[commit-bot: I haz the power, 2017-06-19 22:01:33]

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 23:38:23]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1497909661568920,
"parent_rev": "ec0881e77a76c2ce666e7e0b0a5187b728014f15", "commit_rev":
"06e613863d5edcc50101146650c9c9ec1435e7b2"}

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

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1497909661568920,
"parent_rev": "7c7cbfd6e77d731d0ee45b9c25e1c8a8ab7d93ba", "commit_rev":
"bbf7fc0fd86aea1e927337256934b0e0fb82eaf0"}

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

Description was changed from

==========
Make CertificateProviderService vend ClientCertIdentities directly.

Deletes net::ClientKeyStore, of which CertificateProviderService was the last
remaining use.

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

to

==========
Make CertificateProviderService vend ClientCertIdentities directly.

Deletes net::ClientKeyStore, of which CertificateProviderService was the last
remaining use.

BUG=394131

Review-Url: https://codereview.chromium.org/2937553003
Cr-Commit-Position: refs/heads/master@{#480638}
Committed:
https://chromium.googlesource.com/chromium/src/+/bbf7fc0fd86aea1e927337256934...
==========

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

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/bbf7fc0fd86aea1e927337256934...