Plumbing SSLPrivateKey (//net)

Plumbing SSLPrivateKey (//net)

Plumbing SSLPrivateKey out of URLRequest to allow better unit testing.

BUG=394131
Committed: https://crrev.com/7872fd0f67dc86e4de8eb34124e83364d3e20f62
Cr-Commit-Position: refs/heads/master@{#360663}

[svaldez, 2015-10-30 15:37:02]

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

[svaldez, 2015-10-30 15:37:02]

svaldez@chromium.org changed required reviewers:
+ davidben@chromium.org

[svaldez, 2015-10-30 15:37:03]

//net half

[davidben, 2015-11-04 00:17:38]

https://codereview.chromium.org/1422573008/diff/210001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/1422573008/diff/210001/chrome/browser/BUILD.g...
chrome/browser/BUILD.gn:718: "//third_party/boringssl",
Is this needed?

https://codereview.chromium.org/1422573008/diff/210001/chrome/chrome_browser....
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1422573008/diff/210001/chrome/chrome_browser....
chrome/chrome_browser.gypi:3325:
'../third_party/boringssl/boringssl.gyp:boringssl',
Is this needed?

https://codereview.chromium.org/1422573008/diff/210001/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/1422573008/diff/210001/chromecast/chromecast....
chromecast/chromecast.gyp:452:
'../third_party/boringssl/boringssl.gyp:boringssl',
Is this needed?

https://codereview.chromium.org/1422573008/diff/210001/content/browser/BUILD.gn
File content/browser/BUILD.gn (right):

https://codereview.chromium.org/1422573008/diff/210001/content/browser/BUILD....
content/browser/BUILD.gn:48: "//third_party/boringssl",
Is this needed? (See net/ssl/ssl_platform_key.h comment)

https://codereview.chromium.org/1422573008/diff/210001/content/browser/loader...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/1422573008/diff/210001/content/browser/loader...
content/browser/loader/resource_loader.cc:427: ssl_client_auth_handler_.reset();
Check for null cert.

https://codereview.chromium.org/1422573008/diff/210001/content/browser/loader...
File content/browser/loader/resource_loader_unittest.cc (right):

https://codereview.chromium.org/1422573008/diff/210001/content/browser/loader...
content/browser/loader/resource_loader_unittest.cc:712:
test_client.ContinueWithCertificate(dummy_certs[0].get());
Rather than require the ugly os_cert_handle check, how about you just continue
with no certificate in all these tests? Between the os_cert_handle check, fake
X509Certificate, and the fake URLRequestJob, it's not doing anything with them
anyway.

https://codereview.chromium.org/1422573008/diff/210001/content/content_browse...
File content/content_browser.gypi (right):

https://codereview.chromium.org/1422573008/diff/210001/content/content_browse...
content/content_browser.gypi:27:
'../third_party/boringssl/boringssl.gyp:boringssl',
Not necessary? (See net/ssl/ssl_platform_key.h comment)

https://codereview.chromium.org/1422573008/diff/210001/content/content_tests....
File content/content_tests.gypi (right):

https://codereview.chromium.org/1422573008/diff/210001/content/content_tests....
content/content_tests.gypi:1210:
'../third_party/boringssl/boringssl.gyp:boringssl',
Is this needed?

https://codereview.chromium.org/1422573008/diff/210001/content/test/BUILD.gn
File content/test/BUILD.gn (right):

https://codereview.chromium.org/1422573008/diff/210001/content/test/BUILD.gn#...
content/test/BUILD.gn:166: "//third_party/boringssl",
Is this needed?

https://codereview.chromium.org/1422573008/diff/210001/net/socket/ssl_client_...
File net/socket/ssl_client_socket_openssl.cc (left):

https://codereview.chromium.org/1422573008/diff/210001/net/socket/ssl_client_...
net/socket/ssl_client_socket_openssl.cc:615: private_key_.reset();
Hrm. Actually, I think you can do away with this field completely and just
reference it out of ssl_config_.

https://codereview.chromium.org/1422573008/diff/210001/net/socket/ssl_client_...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/1422573008/diff/210001/net/socket/ssl_client_...
net/socket/ssl_client_socket_openssl.cc:19: #include "base/environment.h"
(Stray change? Guessing a funny rebase.)

https://codereview.chromium.org/1422573008/diff/210001/net/ssl/ssl_platform_k...
File net/ssl/ssl_platform_key.h (right):

https://codereview.chromium.org/1422573008/diff/210001/net/ssl/ssl_platform_k...
net/ssl/ssl_platform_key.h:29: crypto::ScopedEVP_PKEY key);
Does this need to be exported? Everything's just calling
FetchClientCertPrivateKey for now. (Except for
ssl_client_socket_openssl_unittest.cc, but you can keep that as a Record call
for the moment.)

Saves you having to add third_party/boringssl dependencies everywhere just yet.

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

https://codereview.chromium.org/1422573008/diff/210001/net/ssl/ssl_platform_k...
net/ssl/ssl_platform_key_nss.cc:167: return nullptr;
See resource_loader_unittest.cc comment.

[svaldez, 2015-11-04 16:55:52]

https://codereview.chromium.org/1422573008/diff/210001/chrome/browser/BUILD.gn
File chrome/browser/BUILD.gn (right):

https://codereview.chromium.org/1422573008/diff/210001/chrome/browser/BUILD.g...
chrome/browser/BUILD.gn:718: "//third_party/boringssl",
On 2015/11/04 00:17:38, davidben wrote:
> Is this needed?

Done.

https://codereview.chromium.org/1422573008/diff/210001/chrome/chrome_browser....
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1422573008/diff/210001/chrome/chrome_browser....
chrome/chrome_browser.gypi:3325:
'../third_party/boringssl/boringssl.gyp:boringssl',
On 2015/11/04 00:17:38, davidben wrote:
> Is this needed?

Done.

https://codereview.chromium.org/1422573008/diff/210001/chromecast/chromecast.gyp
File chromecast/chromecast.gyp (right):

https://codereview.chromium.org/1422573008/diff/210001/chromecast/chromecast....
chromecast/chromecast.gyp:452:
'../third_party/boringssl/boringssl.gyp:boringssl',
On 2015/11/04 00:17:38, davidben wrote:
> Is this needed?

Done.

https://codereview.chromium.org/1422573008/diff/210001/content/browser/BUILD.gn
File content/browser/BUILD.gn (right):

https://codereview.chromium.org/1422573008/diff/210001/content/browser/BUILD....
content/browser/BUILD.gn:48: "//third_party/boringssl",
On 2015/11/04 00:17:38, davidben wrote:
> Is this needed? (See net/ssl/ssl_platform_key.h comment)

Done.

https://codereview.chromium.org/1422573008/diff/210001/content/browser/loader...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/1422573008/diff/210001/content/browser/loader...
content/browser/loader/resource_loader.cc:427: ssl_client_auth_handler_.reset();
On 2015/11/04 00:17:38, davidben wrote:
> Check for null cert.

Done.

https://codereview.chromium.org/1422573008/diff/210001/content/browser/loader...
File content/browser/loader/resource_loader_unittest.cc (right):

https://codereview.chromium.org/1422573008/diff/210001/content/browser/loader...
content/browser/loader/resource_loader_unittest.cc:712:
test_client.ContinueWithCertificate(dummy_certs[0].get());
On 2015/11/04 00:17:38, davidben wrote:
> Rather than require the ugly os_cert_handle check, how about you just continue
> with no certificate in all these tests? Between the os_cert_handle check, fake
> X509Certificate, and the fake URLRequestJob, it's not doing anything with them
> anyway.

Done.

https://codereview.chromium.org/1422573008/diff/210001/content/content_browse...
File content/content_browser.gypi (right):

https://codereview.chromium.org/1422573008/diff/210001/content/content_browse...
content/content_browser.gypi:27:
'../third_party/boringssl/boringssl.gyp:boringssl',
On 2015/11/04 00:17:38, davidben wrote:
> Not necessary? (See net/ssl/ssl_platform_key.h comment)

Done.

https://codereview.chromium.org/1422573008/diff/210001/content/content_tests....
File content/content_tests.gypi (right):

https://codereview.chromium.org/1422573008/diff/210001/content/content_tests....
content/content_tests.gypi:1210:
'../third_party/boringssl/boringssl.gyp:boringssl',
On 2015/11/04 00:17:38, davidben wrote:
> Is this needed?

Done.

https://codereview.chromium.org/1422573008/diff/210001/content/test/BUILD.gn
File content/test/BUILD.gn (right):

https://codereview.chromium.org/1422573008/diff/210001/content/test/BUILD.gn#...
content/test/BUILD.gn:166: "//third_party/boringssl",
On 2015/11/04 00:17:38, davidben wrote:
> Is this needed?

Done.

https://codereview.chromium.org/1422573008/diff/210001/net/socket/ssl_client_...
File net/socket/ssl_client_socket_openssl.cc (left):

https://codereview.chromium.org/1422573008/diff/210001/net/socket/ssl_client_...
net/socket/ssl_client_socket_openssl.cc:615: private_key_.reset();
On 2015/11/04 00:17:38, davidben wrote:
> Hrm. Actually, I think you can do away with this field completely and just
> reference it out of ssl_config_.

Done.

https://codereview.chromium.org/1422573008/diff/210001/net/socket/ssl_client_...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/1422573008/diff/210001/net/socket/ssl_client_...
net/socket/ssl_client_socket_openssl.cc:19: #include "base/environment.h"
On 2015/11/04 00:17:38, davidben wrote:
> (Stray change? Guessing a funny rebase.)

Done.

https://codereview.chromium.org/1422573008/diff/210001/net/ssl/ssl_platform_k...
File net/ssl/ssl_platform_key.h (right):

https://codereview.chromium.org/1422573008/diff/210001/net/ssl/ssl_platform_k...
net/ssl/ssl_platform_key.h:29: crypto::ScopedEVP_PKEY key);
On 2015/11/04 00:17:38, davidben wrote:
> Does this need to be exported? Everything's just calling
> FetchClientCertPrivateKey for now. (Except for
> ssl_client_socket_openssl_unittest.cc, but you can keep that as a Record call
> for the moment.)
> 
> Saves you having to add third_party/boringssl dependencies everywhere just
yet.

Done.

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

https://codereview.chromium.org/1422573008/diff/210001/net/ssl/ssl_platform_k...
net/ssl/ssl_platform_key_nss.cc:167: return nullptr;
On 2015/11/04 00:17:38, davidben wrote:
> See resource_loader_unittest.cc comment.

Done.

[davidben, 2015-11-04 17:03:01]

https://codereview.chromium.org/1422573008/diff/250001/net/ssl/ssl_platform_k...
File net/ssl/ssl_platform_key_android.cc (right):

https://codereview.chromium.org/1422573008/diff/250001/net/ssl/ssl_platform_k...
net/ssl/ssl_platform_key_android.cc:111: #if false
Try bots are unhappy. I think you want to keep this existing but put it in the
anonymous namespace.

[svaldez, 2015-11-06 21:30:15]

Fixed?

[davidben, 2015-11-06 21:47:45]

lgtm!

https://codereview.chromium.org/1422573008/diff/340001/net/net_common.gypi
File net/net_common.gypi (right):

https://codereview.chromium.org/1422573008/diff/340001/net/net_common.gypi#ne...
net/net_common.gypi:168: 'ssl/ssl_platform_key_task_runner.h',
Missing ssl/ssl_platform_key_task_runner.cc?

https://codereview.chromium.org/1422573008/diff/340001/net/socket/ssl_client_...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/1422573008/diff/340001/net/socket/ssl_client_...
net/socket/ssl_client_socket_openssl.cc:1770: #if defined(OS_NACL)
I think this is no longer necessary. We'll just never call
ContinueWithCertificate. (Would be good to also get rid of the OS_IOS one, but I
think that's not quite a no-op. NaCl right now requires you to call
ContinueWithCertificate(null, null) while iOS does it for you silently. Though
it'd be good to unify that at some point...)

[svaldez, 2015-11-09 17:32:16]

https://codereview.chromium.org/1422573008/diff/340001/net/net_common.gypi
File net/net_common.gypi (right):

https://codereview.chromium.org/1422573008/diff/340001/net/net_common.gypi#ne...
net/net_common.gypi:168: 'ssl/ssl_platform_key_task_runner.h',
On 2015/11/06 21:47:45, davidben wrote:
> Missing ssl/ssl_platform_key_task_runner.cc?

Done.

https://codereview.chromium.org/1422573008/diff/340001/net/socket/ssl_client_...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/1422573008/diff/340001/net/socket/ssl_client_...
net/socket/ssl_client_socket_openssl.cc:1770: #if defined(OS_NACL)
On 2015/11/06 21:47:45, davidben wrote:
> I think this is no longer necessary. We'll just never call
> ContinueWithCertificate. (Would be good to also get rid of the OS_IOS one, but
I
> think that's not quite a no-op. NaCl right now requires you to call
> ContinueWithCertificate(null, null) while iOS does it for you silently. Though
> it'd be good to unify that at some point...)

Done.

[davidben, 2015-11-11 00:00:23]

(still lgtm)

[svaldez, 2015-11-11 14:35:54]

The CQ bit was checked by svaldez@chromium.org

[commit-bot: I haz the power, 2015-11-11 14:36:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1422573008/400001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1422573008/400001

[commit-bot: I haz the power, 2015-11-11 14:43:09]

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

[commit-bot: I haz the power, 2015-11-11 14:43:13]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[svaldez, 2015-11-11 15:23:42]

svaldez@chromium.org changed reviewers:
+ derat@chromium.org, sergeyu@chromium.org, yurys@chromium.org

[svaldez, 2015-11-11 15:23:42]

svaldez@chromium.org changed required reviewers:
+ derat@chromium.org, sergeyu@chromium.org, yurys@chromium.org

[svaldez, 2015-11-11 15:23:43]

Hi,

We're changing the interface for using client certificates in //net and are
updating some of the callers. Could you take a look at the files you have
ownership on and comment/LGTM?

sergeyu:
    remoting/host/token_validator_base.cc
    remoting/host/token_validator_base.h
yurys:
    chrome/browser/devtools/devtools_network_transaction.cc
    chrome/browser/devtools/devtools_network_transaction.h

derat:
    chrome/browser/chromeos/certificate_provider/certificate_provider_service.cc
   
chrome/browser/chromeos/certificate_provider/certificate_provider_service_unittest.cc


Thanks,
Steven

[Daniel Erat, 2015-11-11 15:37:54]

lgtm for c/b/chromeos

[Sergey Ulanov, 2015-11-11 19:05:02]

remoting lgtm, but please see my suggestion

https://codereview.chromium.org/1422573008/diff/420001/remoting/host/token_va...
File remoting/host/token_validator_base.cc (right):

https://codereview.chromium.org/1422573008/diff/420001/remoting/host/token_va...
remoting/host/token_validator_base.cc:159: scoped_refptr<net::SSLPrivateKey>
private_key =
nit: this code would be shorter and more readable as follows:
  net::X509Certificate* cert = (*selected_certs)[i].get();
  if (issuer == kCertIssuerWildCard ||
      issuer == cert->issuer().common_name) {
    request_->ContinueWithCertificate(
        cert, net::FetchClientCertPrivateKey(cert));
  }

This particularly avoids duplication of the (*selected_certs)[i].get()
expression. Alternative

[svaldez, 2015-11-16 21:36:22]

yurys: Any idea if you'll have time to take a look at the devtools change, or
know of someone else that I should ping?

https://codereview.chromium.org/1422573008/diff/420001/remoting/host/token_va...
File remoting/host/token_validator_base.cc (right):

https://codereview.chromium.org/1422573008/diff/420001/remoting/host/token_va...
remoting/host/token_validator_base.cc:159: scoped_refptr<net::SSLPrivateKey>
private_key =
On 2015/11/11 19:05:02, Sergey Ulanov wrote:
> nit: this code would be shorter and more readable as follows:
>   net::X509Certificate* cert = (*selected_certs)[i].get();
>   if (issuer == kCertIssuerWildCard ||
>       issuer == cert->issuer().common_name) {
>     request_->ContinueWithCertificate(
>         cert, net::FetchClientCertPrivateKey(cert));
>   }
> 
> This particularly avoids duplication of the (*selected_certs)[i].get()
> expression. Alternative

Done.

[svaldez, 2015-11-19 15:51:07]

svaldez@chromium.org changed reviewers:
+ dgozman@chromium.org

[svaldez, 2015-11-19 15:51:07]

svaldez@chromium.org changed required reviewers:
+ dgozman@chromium.org
- yurys@chromium.org

[svaldez, 2015-11-19 15:51:08]

dgozman: Any chance you could take a look at the devtools change? Its a small
change to update how we are plumbing SSLPrivateKey through the browser.

[davidben, 2015-11-19 17:01:20]

(It's been a week of waiting on the previous reviewer and this is a completely
mechanical change in that directory. I think a TBR is reasonable.)

[dgozman, 2015-11-19 18:49:07]

chrome/browser/devtools lgtm. Just TBR mechanical changes in clients next time
if you got an approval from the actual API owner (in this case, owner of
http_transaction.h).

https://codereview.chromium.org/1422573008/diff/440001/chrome/browser/devtool...
File chrome/browser/devtools/devtools_network_transaction.h (right):

https://codereview.chromium.org/1422573008/diff/440001/chrome/browser/devtool...
chrome/browser/devtools/devtools_network_transaction.h:31: class SSLPrivateKey;
This should be already included in http_transaction.h, so I'd rather not add it.

[svaldez, 2015-11-19 18:55:34]

The CQ bit was checked by svaldez@chromium.org

[svaldez, 2015-11-19 18:55:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
davidben@chromium.org, sergeyu@chromium.org, derat@chromium.org,
dgozman@chromium.org
Link to the patchset: https://codereview.chromium.org/1422573008/#ps460001
(title: "Removing un-needed forward decl.")

[commit-bot: I haz the power, 2015-11-19 18:56:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1422573008/460001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1422573008/460001

[commit-bot: I haz the power, 2015-11-19 21:11:02]

Committed patchset #25 (id:460001)

[commit-bot: I haz the power, 2015-11-19 21:12:33]

Patchset 25 (id:??) landed as
https://crrev.com/7872fd0f67dc86e4de8eb34124e83364d3e20f62
Cr-Commit-Position: refs/heads/master@{#360663}