Plumbing SSLPrivateKey

Plumbing SSLPrivateKey

Plumbing SSLPrivateKey to content/ to allow for better unit testing of
the net/ code.

BUG=394131

[svaldez, 2015-09-09 16:32:40]

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

[svaldez, 2015-09-09 16:32:41]

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

[svaldez, 2015-09-09 16:32:41]

Clean version of the CL.

Still missing the JNI/Windows X509Certificate changes, but wanted to submit
those as a separate patchset once you've had a chance to review this part.

[davidben, 2015-09-25 20:10:13]

Sorry this took so long! Here's a first pass through it. For the most part, this
looks good. One high-level comment that might require some surgery though:

So I do like doing part of this separately from the big Windows messiness, but
it might be good to do it in a slightly different order.

This isn't quiiiiite a no-op w.r.t. the Windows threading problem because now we
CACPK on the UI thread instead of the IO thread, whereas the smartcard-backed
X509Certificate is still created on and largely accessed the IO thread, apart
from the operations we've since pushed to the background thread.

How about this?

CL #1: Land the //net portions of this CL so that SSLPrivateKey is out of
URLRequest, but keep the OpenSSLClientKeyStore and RecordPrivateKey mess for
now. Expose net::FetchBlah and call it in //content, just before calling
URLRequest::ContinueWithCertificate. //content's API and stuff outside of
//content is unchanged.

CL #2: Actually, if you feel like it, that's enough for //net-level tests
already.

CL #3: The remainder of this CL (so //content and above) and the Windows
craziness.

https://codereview.chromium.org/1304143010/diff/40001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/1304143010/diff/40001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1985: #if !defined(OS_ANDROID)
I'd probably just wrap the whole thing.

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

https://codereview.chromium.org/1304143010/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/certificate_provider/certificate_provider_service.cc:227:
}
Reorder to match declaration

https://codereview.chromium.org/1304143010/diff/40001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_network_transaction.h (right):

https://codereview.chromium.org/1304143010/diff/40001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction.h:16: #include
"net/ssl/ssl_private_key.h"
Forward decl / unnecessary because this is part of HttpTransaction

https://codereview.chromium.org/1304143010/diff/40001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_client_auth_observer.cc (right):

https://codereview.chromium.org/1304143010/diff/40001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_client_auth_observer.cc:55:
FetchClientCertPrivateKey(certificate);
[See comment at top of review. If I forget to type it, come yell at me.]

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

https://codereview.chromium.org/1304143010/diff/40001/content/browser/loader/...
content/browser/loader/resource_loader_unittest.cc:41: #include
"net/ssl/ssl_platform_key.h"
No longer necessary?

https://codereview.chromium.org/1304143010/diff/40001/net/BUILD.gn
File net/BUILD.gn (left):

https://codereview.chromium.org/1304143010/diff/40001/net/BUILD.gn#oldcode241
net/BUILD.gn:241: 
Stray change?

https://codereview.chromium.org/1304143010/diff/40001/net/http/failing_http_t...
File net/http/failing_http_transaction_factory.cc (right):

https://codereview.chromium.org/1304143010/diff/40001/net/http/failing_http_t...
net/http/failing_http_transaction_factory.cc:19: #include
"net/ssl/ssl_private_key.h"
Forward decl

https://codereview.chromium.org/1304143010/diff/40001/net/http/http_cache_tra...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/1304143010/diff/40001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:46: #include "net/ssl/ssl_private_key.h"
Not necessary

https://codereview.chromium.org/1304143010/diff/40001/net/http/http_cache_tra...
File net/http/http_cache_transaction.h (right):

https://codereview.chromium.org/1304143010/diff/40001/net/http/http_cache_tra...
net/http/http_cache_transaction.h:34: #include "net/ssl/ssl_private_key.h"
Forward decl

https://codereview.chromium.org/1304143010/diff/40001/net/http/http_network_t...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1304143010/diff/40001/net/http/http_network_t...
net/http/http_network_transaction.cc:63: #include "net/ssl/ssl_private_key.h"
[THIS one is actually necessary]

https://codereview.chromium.org/1304143010/diff/40001/net/http/http_network_t...
File net/http/http_network_transaction.h (right):

https://codereview.chromium.org/1304143010/diff/40001/net/http/http_network_t...
net/http/http_network_transaction.h:28: #include "net/ssl/ssl_private_key.h"
Forward decl (or perhaps just not necessary since it's coming from
HttpTransaction anyway. Never been clear on whether we explicitly forward decl
there or no)

https://codereview.chromium.org/1304143010/diff/40001/net/http/http_transacti...
File net/http/http_transaction.h (right):

https://codereview.chromium.org/1304143010/diff/40001/net/http/http_transacti...
net/http/http_transaction.h:16: #include "net/ssl/ssl_private_key.h"
Forward decl

https://codereview.chromium.org/1304143010/diff/40001/net/http/http_transacti...
File net/http/http_transaction_test_util.cc (right):

https://codereview.chromium.org/1304143010/diff/40001/net/http/http_transacti...
net/http/http_transaction_test_util.cc:26: #include "net/ssl/ssl_private_key.h"
Not necessary

https://codereview.chromium.org/1304143010/diff/40001/net/http/http_transacti...
File net/http/http_transaction_test_util.h (right):

https://codereview.chromium.org/1304143010/diff/40001/net/http/http_transacti...
net/http/http_transaction_test_util.h:30: #include "net/ssl/ssl_private_key.h"
Forward decl / unnecessary because HttpTransaction, not sure

https://codereview.chromium.org/1304143010/diff/40001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/1304143010/diff/40001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:1780: private_key_ =
ssl_config_.client_pkey.Pass();
I think you want it without the .Pass(). We probably shouldn't modify
ssl_config_ here.

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/client_cert_sto...
File net/ssl/client_cert_store_win.cc (right):

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/client_cert_sto...
net/ssl/client_cert_store_win.cc:135: // CertificateList to be a
pair<X509Certificate, SSLPrivateKeyCallback>
Nit: Rather than "Fake X509Certificate", I would maybe say:

TODO(svaldez): cert wraps cert_context2 which may be backed by a smartcard with
threading difficulties. Instead, create a fresh X509Certificate with
CreateFromBytes and route cert_context2 into the SSLPrivateKey. Probably change
CertificateList to be a pair<X509Certificate, SSLPrivateKeyCAllback>.
https://crbug.com/<bug-here>

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_config.h
File net/ssl/ssl_config.h (right):

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_config.h#ne...
net/ssl/ssl_config.h:173: scoped_refptr<SSLPrivateKey> client_pkey;
I don't have a strong opinion, but 'pkey' is sort of an odd abbreviation. I
mostly associate it with OpenSSL's EVP_PKEY because they somewhat
over-abbreviate. I'd probably do client_private_key_ and client_key_.

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_key.h
File net/ssl/ssl_platform_key.h (right):

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key.h:12: #include "net/ssl/ssl_platform_key_task_runner.h"
Unnecessary? (Move to cc files.)

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key.h:24: class SSLPlatformKeyTaskRunner;
Unnecessary?

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key.h:30: NET_EXPORT scoped_refptr<SSLPrivateKey>
WrapPrivateKey(
I'd probably call this WrapEVP_PKEY or WrapOpenSSLPrivateKey since
net::WrapPrivateKey is a fairly generic name.

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key.h:39: X509Certificate* certificate);
This doesn't exist on OS_ANDROID, right? Probably should be #else.

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
File net/ssl/ssl_platform_key_mac.cc (right):

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key_mac.cc:220: if (!certificate) {
Any reason not to have the caller handle null? This seems the sort of function
that doesn't really have a reason to allow null when you ask it to do something
with a certificate.

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
File net/ssl/ssl_platform_key_nss.cc (right):

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key_nss.cc:162: if (!certificate ||
!certificate->os_cert_handle()) {
Ditto about handling in caller.

(I don't think os_cert_handle being missing is possible, is it?)

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key_nss.cc:166: LOG(ERROR) << "Cert Handle: " <<
certificate->os_cert_handle() << "\n";
Probably want to drop these two.

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
File net/ssl/ssl_platform_key_task_runner.cc (right):

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key_task_runner.cc:1: // Copyright (c) 2015 The Chromium
Authors. All rights reserved.
Some of the older files still have the (c) sign, but the new copyright header
doesn't.

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key_task_runner.cc:11: // thread to avoid problems with
buggy smartcards.
I'd probably put a comment of this sort into the header file instead, so that if
you're looking at the header, you know what this class is for.

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
File net/ssl/ssl_platform_key_task_runner.h (right):

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key_task_runner.h:1: // Copyright (c) 2015 The Chromium
Authors. All rights reserved.
Some of the older files still have the (c) sign, but the new copyright header
doesn't.

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key_task_runner.h:10: namespace net {
Nit: newline

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key_task_runner.h:14: ~SSLPlatformKeyTaskRunner();
Rather than have everyone do a LazyInstance, perhaps just this should just have
a GetSSLPlatformKeyTaskRunner() method and implement the LazyInstance once.

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key_task_runner.h:23: };
Nit: newline

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
File net/ssl/ssl_platform_key_win.cc (right):

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key_win.cc:325: if (!certificate) {
Ditto about caller's problem.

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/threaded_ssl_pr...
File net/ssl/threaded_ssl_private_key.h (right):

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/threaded_ssl_pr...
net/ssl/threaded_ssl_private_key.h:65: ~ThreadedSSLPrivateKey() override;
Update cc file to match header order.

https://codereview.chromium.org/1304143010/diff/40001/net/url_request/url_fet...
File net/url_request/url_fetcher_core.cc (right):

https://codereview.chromium.org/1304143010/diff/40001/net/url_request/url_fet...
net/url_request/url_fetcher_core.cc:27: #include "net/ssl/ssl_private_key.h"
I don't think this include is actually needed, is it?

https://codereview.chromium.org/1304143010/diff/40001/net/url_request/url_fet...
net/url_request/url_fetcher_core.cc:441: request->ContinueWithCertificate(NULL,
NULL);
Nit: While you're here, want to switch these both to nullptr?

https://codereview.chromium.org/1304143010/diff/40001/net/url_request/url_req...
File net/url_request/url_request.cc (right):

https://codereview.chromium.org/1304143010/diff/40001/net/url_request/url_req...
net/url_request/url_request.cc:33: #include "net/ssl/ssl_private_key.h"
I don't think you need this include when using raw pointers.

https://codereview.chromium.org/1304143010/diff/40001/net/url_request/url_req...
File net/url_request/url_request.h (right):

https://codereview.chromium.org/1304143010/diff/40001/net/url_request/url_req...
net/url_request/url_request.h:34: #include "net/ssl/ssl_private_key.h"
This can be a forward decl.

https://codereview.chromium.org/1304143010/diff/40001/net/url_request/url_req...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/1304143010/diff/40001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:45: #include "net/ssl/ssl_private_key.h"
Nit: unnecessary

https://codereview.chromium.org/1304143010/diff/40001/net/url_request/url_req...
File net/url_request/url_request_http_job.h (right):

https://codereview.chromium.org/1304143010/diff/40001/net/url_request/url_req...
net/url_request/url_request_http_job.h:25: #include "net/ssl/ssl_private_key.h"
Nit: forward decl.

https://codereview.chromium.org/1304143010/diff/40001/net/url_request/url_req...
File net/url_request/url_request_job.cc (right):

https://codereview.chromium.org/1304143010/diff/40001/net/url_request/url_req...
net/url_request/url_request_job.cc:28: #include "net/ssl/ssl_private_key.h"
Nit unnecessary

https://codereview.chromium.org/1304143010/diff/40001/net/url_request/url_req...
File net/url_request/url_request_job.h (right):

https://codereview.chromium.org/1304143010/diff/40001/net/url_request/url_req...
net/url_request/url_request_job.h:25: #include "net/ssl/ssl_private_key.h"
Nit: forward decl

https://codereview.chromium.org/1304143010/diff/40001/net/url_request/url_req...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/1304143010/diff/40001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:78: #include "net/ssl/ssl_private_key.h"
Nit: unnecessary

https://codereview.chromium.org/1304143010/diff/40001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:7968: r->ContinueWithCertificate(NULL,
NULL);
Nit: while you're here, want to make this nullptr?

https://codereview.chromium.org/1304143010/diff/40001/remoting/host/token_val...
File remoting/host/token_validator_base.h (right):

https://codereview.chromium.org/1304143010/diff/40001/remoting/host/token_val...
remoting/host/token_validator_base.h:12: #include "net/ssl/ssl_private_key.h"
Unnecessary includes? (The two key ones.)

[svaldez, 2015-09-28 16:54:54]

Fixed most of the issues modulo Platform Key null checking.

I think the plan is to fix up this review, and then split it out into the net
stack stuff, content+Windows, Android JNI?

https://codereview.chromium.org/1304143010/diff/40001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/1304143010/diff/40001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1985: #if !defined(OS_ANDROID)
On 2015/09/25 20:10:11, David Benjamin wrote:
> I'd probably just wrap the whole thing.

Done.

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

https://codereview.chromium.org/1304143010/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/certificate_provider/certificate_provider_service.cc:227:
}
On 2015/09/25 20:10:11, David Benjamin wrote:
> Reorder to match declaration

Done.

https://codereview.chromium.org/1304143010/diff/40001/chrome/browser/devtools...
File chrome/browser/devtools/devtools_network_transaction.h (right):

https://codereview.chromium.org/1304143010/diff/40001/chrome/browser/devtools...
chrome/browser/devtools/devtools_network_transaction.h:16: #include
"net/ssl/ssl_private_key.h"
On 2015/09/25 20:10:11, David Benjamin wrote:
> Forward decl / unnecessary because this is part of HttpTransaction

Done.

https://codereview.chromium.org/1304143010/diff/40001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_client_auth_observer.cc (right):

https://codereview.chromium.org/1304143010/diff/40001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_client_auth_observer.cc:55:
FetchClientCertPrivateKey(certificate);
On 2015/09/25 20:10:11, David Benjamin wrote:
> [See comment at top of review. If I forget to type it, come yell at me.]

Acknowledged.

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

https://codereview.chromium.org/1304143010/diff/40001/content/browser/loader/...
content/browser/loader/resource_loader_unittest.cc:41: #include
"net/ssl/ssl_platform_key.h"
On 2015/09/25 20:10:11, David Benjamin wrote:
> No longer necessary?

Done.

https://codereview.chromium.org/1304143010/diff/40001/net/BUILD.gn
File net/BUILD.gn (left):

https://codereview.chromium.org/1304143010/diff/40001/net/BUILD.gn#oldcode241
net/BUILD.gn:241: 
On 2015/09/25 20:10:11, David Benjamin wrote:
> Stray change?

Done.

https://codereview.chromium.org/1304143010/diff/40001/net/http/failing_http_t...
File net/http/failing_http_transaction_factory.cc (right):

https://codereview.chromium.org/1304143010/diff/40001/net/http/failing_http_t...
net/http/failing_http_transaction_factory.cc:19: #include
"net/ssl/ssl_private_key.h"
On 2015/09/25 20:10:11, David Benjamin wrote:
> Forward decl

Done.

https://codereview.chromium.org/1304143010/diff/40001/net/http/http_cache_tra...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/1304143010/diff/40001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:46: #include "net/ssl/ssl_private_key.h"
On 2015/09/25 20:10:11, David Benjamin wrote:
> Not necessary

Done.

https://codereview.chromium.org/1304143010/diff/40001/net/http/http_cache_tra...
File net/http/http_cache_transaction.h (right):

https://codereview.chromium.org/1304143010/diff/40001/net/http/http_cache_tra...
net/http/http_cache_transaction.h:34: #include "net/ssl/ssl_private_key.h"
On 2015/09/25 20:10:11, David Benjamin wrote:
> Forward decl

Done.

https://codereview.chromium.org/1304143010/diff/40001/net/http/http_network_t...
File net/http/http_network_transaction.cc (right):

https://codereview.chromium.org/1304143010/diff/40001/net/http/http_network_t...
net/http/http_network_transaction.cc:63: #include "net/ssl/ssl_private_key.h"
On 2015/09/25 20:10:11, David Benjamin wrote:
> [THIS one is actually necessary]

Done.

https://codereview.chromium.org/1304143010/diff/40001/net/http/http_network_t...
File net/http/http_network_transaction.h (right):

https://codereview.chromium.org/1304143010/diff/40001/net/http/http_network_t...
net/http/http_network_transaction.h:28: #include "net/ssl/ssl_private_key.h"
On 2015/09/25 20:10:11, David Benjamin wrote:
> Forward decl (or perhaps just not necessary since it's coming from
> HttpTransaction anyway. Never been clear on whether we explicitly forward decl
> there or no)

Done.

https://codereview.chromium.org/1304143010/diff/40001/net/http/http_transacti...
File net/http/http_transaction.h (right):

https://codereview.chromium.org/1304143010/diff/40001/net/http/http_transacti...
net/http/http_transaction.h:16: #include "net/ssl/ssl_private_key.h"
On 2015/09/25 20:10:11, David Benjamin wrote:
> Forward decl

Done.

https://codereview.chromium.org/1304143010/diff/40001/net/http/http_transacti...
File net/http/http_transaction_test_util.cc (right):

https://codereview.chromium.org/1304143010/diff/40001/net/http/http_transacti...
net/http/http_transaction_test_util.cc:26: #include "net/ssl/ssl_private_key.h"
On 2015/09/25 20:10:11, David Benjamin wrote:
> Not necessary

Done.

https://codereview.chromium.org/1304143010/diff/40001/net/http/http_transacti...
File net/http/http_transaction_test_util.h (right):

https://codereview.chromium.org/1304143010/diff/40001/net/http/http_transacti...
net/http/http_transaction_test_util.h:30: #include "net/ssl/ssl_private_key.h"
On 2015/09/25 20:10:11, David Benjamin wrote:
> Forward decl / unnecessary because HttpTransaction, not sure

Done.

https://codereview.chromium.org/1304143010/diff/40001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/1304143010/diff/40001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:1780: private_key_ =
ssl_config_.client_pkey.Pass();
On 2015/09/25 20:10:11, David Benjamin wrote:
> I think you want it without the .Pass(). We probably shouldn't modify
> ssl_config_ here.

Done.

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/client_cert_sto...
File net/ssl/client_cert_store_win.cc (right):

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/client_cert_sto...
net/ssl/client_cert_store_win.cc:135: // CertificateList to be a
pair<X509Certificate, SSLPrivateKeyCallback>
On 2015/09/25 20:10:11, David Benjamin wrote:
> Nit: Rather than "Fake X509Certificate", I would maybe say:
> 
> TODO(svaldez): cert wraps cert_context2 which may be backed by a smartcard
with
> threading difficulties. Instead, create a fresh X509Certificate with
> CreateFromBytes and route cert_context2 into the SSLPrivateKey. Probably
change
> CertificateList to be a pair<X509Certificate, SSLPrivateKeyCAllback>.
> https://crbug.com/%3Cbug-here%3E

Done.

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_config.h
File net/ssl/ssl_config.h (right):

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_config.h#ne...
net/ssl/ssl_config.h:173: scoped_refptr<SSLPrivateKey> client_pkey;
On 2015/09/25 20:10:11, David Benjamin wrote:
> I don't have a strong opinion, but 'pkey' is sort of an odd abbreviation. I
> mostly associate it with OpenSSL's EVP_PKEY because they somewhat
> over-abbreviate. I'd probably do client_private_key_ and client_key_.

Done.

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_key.h
File net/ssl/ssl_platform_key.h (right):

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key.h:12: #include "net/ssl/ssl_platform_key_task_runner.h"
On 2015/09/25 20:10:11, David Benjamin wrote:
> Unnecessary? (Move to cc files.)

Done.

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key.h:24: class SSLPlatformKeyTaskRunner;
On 2015/09/25 20:10:12, David Benjamin wrote:
> Unnecessary?

Done.

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key.h:30: NET_EXPORT scoped_refptr<SSLPrivateKey>
WrapPrivateKey(
On 2015/09/25 20:10:11, David Benjamin wrote:
> I'd probably call this WrapEVP_PKEY or WrapOpenSSLPrivateKey since
> net::WrapPrivateKey is a fairly generic name.

Done.

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key.h:39: X509Certificate* certificate);
On 2015/09/25 20:10:12, David Benjamin wrote:
> This doesn't exist on OS_ANDROID, right? Probably should be #else.

Done.

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
File net/ssl/ssl_platform_key_mac.cc (right):

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key_mac.cc:220: if (!certificate) {
On 2015/09/25 20:10:12, David Benjamin wrote:
> Any reason not to have the caller handle null? This seems the sort of function
> that doesn't really have a reason to allow null when you ask it to do
something
> with a certificate.

Then we'd have to check both at the caller that the argument isn't null and also
check that the private key returned isn't null. A null cert and a null private
key should continue down the same code path, since they both mean we can't use
this cert.

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
File net/ssl/ssl_platform_key_nss.cc (right):

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key_nss.cc:162: if (!certificate ||
!certificate->os_cert_handle()) {
On 2015/09/25 20:10:12, David Benjamin wrote:
> Ditto about handling in caller.
> 
> (I don't think os_cert_handle being missing is possible, is it?)

The handle could theoretically be closed or destructed? Same reasoning for
handling null here.

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key_nss.cc:166: LOG(ERROR) << "Cert Handle: " <<
certificate->os_cert_handle() << "\n";
On 2015/09/25 20:10:12, David Benjamin wrote:
> Probably want to drop these two.

Done.

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
File net/ssl/ssl_platform_key_task_runner.cc (right):

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key_task_runner.cc:1: // Copyright (c) 2015 The Chromium
Authors. All rights reserved.
On 2015/09/25 20:10:12, David Benjamin wrote:
> Some of the older files still have the (c) sign, but the new copyright header
> doesn't.

Done.

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key_task_runner.cc:11: // thread to avoid problems with
buggy smartcards.
On 2015/09/25 20:10:12, David Benjamin wrote:
> I'd probably put a comment of this sort into the header file instead, so that
if
> you're looking at the header, you know what this class is for.

Done.

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
File net/ssl/ssl_platform_key_task_runner.h (right):

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key_task_runner.h:1: // Copyright (c) 2015 The Chromium
Authors. All rights reserved.
On 2015/09/25 20:10:12, David Benjamin wrote:
> Some of the older files still have the (c) sign, but the new copyright header
> doesn't.

Done.

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key_task_runner.h:10: namespace net {
On 2015/09/25 20:10:12, David Benjamin wrote:
> Nit: newline

Done.

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key_task_runner.h:14: ~SSLPlatformKeyTaskRunner();
On 2015/09/25 20:10:12, David Benjamin wrote:
> Rather than have everyone do a LazyInstance, perhaps just this should just
have
> a GetSSLPlatformKeyTaskRunner() method and implement the LazyInstance once.

Done.

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key_task_runner.h:23: };
On 2015/09/25 20:10:12, David Benjamin wrote:
> Nit: newline

Done.

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
File net/ssl/ssl_platform_key_win.cc (right):

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key_win.cc:325: if (!certificate) {
On 2015/09/25 20:10:12, David Benjamin wrote:
> Ditto about caller's problem.

Ditto.

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/threaded_ssl_pr...
File net/ssl/threaded_ssl_private_key.h (right):

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/threaded_ssl_pr...
net/ssl/threaded_ssl_private_key.h:65: ~ThreadedSSLPrivateKey() override;
On 2015/09/25 20:10:12, David Benjamin wrote:
> Update cc file to match header order.

Done.

https://codereview.chromium.org/1304143010/diff/40001/net/url_request/url_fet...
File net/url_request/url_fetcher_core.cc (right):

https://codereview.chromium.org/1304143010/diff/40001/net/url_request/url_fet...
net/url_request/url_fetcher_core.cc:27: #include "net/ssl/ssl_private_key.h"
On 2015/09/25 20:10:12, David Benjamin wrote:
> I don't think this include is actually needed, is it?

Done.

https://codereview.chromium.org/1304143010/diff/40001/net/url_request/url_fet...
net/url_request/url_fetcher_core.cc:441: request->ContinueWithCertificate(NULL,
NULL);
On 2015/09/25 20:10:12, David Benjamin wrote:
> Nit: While you're here, want to switch these both to nullptr?

Done.

https://codereview.chromium.org/1304143010/diff/40001/net/url_request/url_req...
File net/url_request/url_request.cc (right):

https://codereview.chromium.org/1304143010/diff/40001/net/url_request/url_req...
net/url_request/url_request.cc:33: #include "net/ssl/ssl_private_key.h"
On 2015/09/25 20:10:12, David Benjamin wrote:
> I don't think you need this include when using raw pointers.

Done.

https://codereview.chromium.org/1304143010/diff/40001/net/url_request/url_req...
File net/url_request/url_request.h (right):

https://codereview.chromium.org/1304143010/diff/40001/net/url_request/url_req...
net/url_request/url_request.h:34: #include "net/ssl/ssl_private_key.h"
On 2015/09/25 20:10:12, David Benjamin wrote:
> This can be a forward decl.

Done.

https://codereview.chromium.org/1304143010/diff/40001/net/url_request/url_req...
File net/url_request/url_request_http_job.cc (right):

https://codereview.chromium.org/1304143010/diff/40001/net/url_request/url_req...
net/url_request/url_request_http_job.cc:45: #include "net/ssl/ssl_private_key.h"
On 2015/09/25 20:10:12, David Benjamin wrote:
> Nit: unnecessary

Done.

https://codereview.chromium.org/1304143010/diff/40001/net/url_request/url_req...
File net/url_request/url_request_http_job.h (right):

https://codereview.chromium.org/1304143010/diff/40001/net/url_request/url_req...
net/url_request/url_request_http_job.h:25: #include "net/ssl/ssl_private_key.h"
On 2015/09/25 20:10:12, David Benjamin wrote:
> Nit: forward decl.

Done.

https://codereview.chromium.org/1304143010/diff/40001/net/url_request/url_req...
File net/url_request/url_request_job.cc (right):

https://codereview.chromium.org/1304143010/diff/40001/net/url_request/url_req...
net/url_request/url_request_job.cc:28: #include "net/ssl/ssl_private_key.h"
On 2015/09/25 20:10:12, David Benjamin wrote:
> Nit unnecessary

Done.

https://codereview.chromium.org/1304143010/diff/40001/net/url_request/url_req...
File net/url_request/url_request_job.h (right):

https://codereview.chromium.org/1304143010/diff/40001/net/url_request/url_req...
net/url_request/url_request_job.h:25: #include "net/ssl/ssl_private_key.h"
On 2015/09/25 20:10:12, David Benjamin wrote:
> Nit: forward decl

Done.

https://codereview.chromium.org/1304143010/diff/40001/net/url_request/url_req...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/1304143010/diff/40001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:78: #include "net/ssl/ssl_private_key.h"
On 2015/09/25 20:10:12, David Benjamin wrote:
> Nit: unnecessary

Done.

https://codereview.chromium.org/1304143010/diff/40001/net/url_request/url_req...
net/url_request/url_request_unittest.cc:7968: r->ContinueWithCertificate(NULL,
NULL);
On 2015/09/25 20:10:12, David Benjamin wrote:
> Nit: while you're here, want to make this nullptr?

Done.

https://codereview.chromium.org/1304143010/diff/40001/remoting/host/token_val...
File remoting/host/token_validator_base.h (right):

https://codereview.chromium.org/1304143010/diff/40001/remoting/host/token_val...
remoting/host/token_validator_base.h:12: #include "net/ssl/ssl_private_key.h"
On 2015/09/25 20:10:12, David Benjamin wrote:
> Unnecessary includes? (The two key ones.)

Done.

[davidben, 2015-10-13 20:32:16]

(Note two comments are an an earlier patch set.)

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
File net/ssl/ssl_platform_key_mac.cc (right):

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key_mac.cc:220: if (!certificate) {
On 2015/09/28 16:54:53, svaldez wrote:
> On 2015/09/25 20:10:12, David Benjamin wrote:
> > Any reason not to have the caller handle null? This seems the sort of
function
> > that doesn't really have a reason to allow null when you ask it to do
> something
> > with a certificate.
> 
> Then we'd have to check both at the caller that the argument isn't null and
also
> check that the private key returned isn't null. A null cert and a null private
> key should continue down the same code path, since they both mean we can't use
> this cert.

In general, APIs shouldn't silently try to handle null unless what the function
does inherently wants a nullable type. I'm not sure this does. There also aren't
that many callers of FetchClientCertPrivateKey which actually can have a null
input it seems. Just the SSLClientAuthObserver thing as far as I can tell.

There's even more implementations of this function, actually. :-)

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
File net/ssl/ssl_platform_key_nss.cc (right):

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key_nss.cc:162: if (!certificate ||
!certificate->os_cert_handle()) {
On 2015/09/28 16:54:53, svaldez wrote:
> On 2015/09/25 20:10:12, David Benjamin wrote:
> > Ditto about handling in caller.
> > 
> > (I don't think os_cert_handle being missing is possible, is it?)
> 
> The handle could theoretically be closed or destructed? Same reasoning for
> handling null here.

But the handle is owned by the X509Certificate. We shouldn't be destroying those
I think.

https://codereview.chromium.org/1304143010/diff/80001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/1304143010/diff/80001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1973: 
Probably add a comment to the #ifdef that Android can't support
CONTENT_SETTINGS_TYPE_AUTO_SELECT_CERTIFICATE because the platform doesn't
provide the list of matching erts.

https://codereview.chromium.org/1304143010/diff/80001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1974: #if !defined(OS_ANDROID)
Nit: Move this up to line 1958 I think.

https://codereview.chromium.org/1304143010/diff/80001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1991:
FetchClientCertPrivateKey(all_client_certs[i].get()).get());
net:: prefix. Ugh, I bet this is compiling because of ADL.

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

https://codereview.chromium.org/1304143010/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/certificate_provider/certificate_provider_service.cc:166:
private_key->swap(new_key);
private_key = new SSLPrivateKey(...)

https://codereview.chromium.org/1304143010/diff/80001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_client_auth_observer.cc (right):

https://codereview.chromium.org/1304143010/diff/80001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_client_auth_observer.cc:55:
FetchClientCertPrivateKey(certificate);
net:: prefix. Ugh, I bet this is compiling because of ADL.

https://codereview.chromium.org/1304143010/diff/80001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_client_auth_observer.cc:88:
FetchClientCertPrivateKey(cert_details->second);
(Ditto.)

https://codereview.chromium.org/1304143010/diff/80001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1304143010/diff/80001/chrome/chrome_browser.g...
chrome/chrome_browser.gypi:3325:
'../third_party/boringssl/boringssl.gyp:boringssl',
Corresponding change in chrome/browser/BUILD.gn

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

https://codereview.chromium.org/1304143010/diff/80001/content/browser/loader/...
content/browser/loader/resource_loader_unittest.cc:713:
test_client.ContinueWithCertificate(dummy_certs[0].get(), nullptr);
Given that this call does absolutely nothing with the certificate, I would
actually drop cert too and pass in nullptr. Maybe explicitly say "Continue the
request with no certificate."

Also helps in trying to get rid of that very confusing X509Certificate
constructor that creates a broken X509Certificate. I'm guessing this is why you
wanted to add an os_cert_handle check.

https://codereview.chromium.org/1304143010/diff/80001/content/browser/loader/...
content/browser/loader/resource_loader_unittest.cc:741:
test_client.ContinueWithCertificate(cert.get(), nullptr);
Ditto

https://codereview.chromium.org/1304143010/diff/80001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_client_auth_handler.h (right):

https://codereview.chromium.org/1304143010/diff/80001/content/browser/ssl/ssl...
content/browser/ssl/ssl_client_auth_handler.h:40: // may be nullptr.
Nit: wrapping.

https://codereview.chromium.org/1304143010/diff/80001/content/content_tests.gypi
File content/content_tests.gypi (right):

https://codereview.chromium.org/1304143010/diff/80001/content/content_tests.g...
content/content_tests.gypi:1186:
'../third_party/boringssl/boringssl.gyp:boringssl',
Corresponding change in content/test/BUILD.gn

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

https://codereview.chromium.org/1304143010/diff/80001/content/public/browser/...
content/public/browser/client_certificate_delegate.h:28: // remembered for
future requests to the domain.
Nit: wrapping

https://codereview.chromium.org/1304143010/diff/80001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/1304143010/diff/80001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:1788: }
Oh hrm. That hadn't occurred to me. It's kind of confusing that, if FetchBlah
fails, we'd like to have the URLRequest signal that error code, but right now
it's implemented by pushing the null all the way down and bouncing it back up.
Maybe that's fine? The alternative is that ClientCertificateDelegate gets a
CancelWithError() hook or something. Dunno. Let's leave that as-is for now.

Maybe tweak the comment to something like:

// The caller supplied a null private key. Fail the handshake and surface and
surface an appropriate error to the caller.

https://codereview.chromium.org/1304143010/diff/80001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl_unittest.cc (right):

https://codereview.chromium.org/1304143010/diff/80001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl_unittest.cc:238:
net::WrapOpenSSLPrivateKey(client_private_key.Pass());
Ooh. We could basically promote these tests to run on everything in a follow-up
CL having a EVP_PKEY -> SSLPrivateKey bridge available in test code everywhere.

https://codereview.chromium.org/1304143010/diff/80001/net/ssl/ssl_client_auth...
File net/ssl/ssl_client_auth_cache_unittest.cc (right):

https://codereview.chromium.org/1304143010/diff/80001/net/ssl/ssl_client_auth...
net/ssl/ssl_client_auth_cache_unittest.cc:14: TEST(SSLClientAuthCacheTest,
LookupAddRemove) {
It would probably be good if these tests had some dummy SSLPrivateKeys in there
rather than storing null everywhere. It can be a really silly impl that just
NOT_IMPLEMENTED()'s everything.

https://codereview.chromium.org/1304143010/diff/80001/net/ssl/ssl_client_auth...
net/ssl/ssl_client_auth_cache_unittest.cc:39: cache.Add(server1, cert1.get(),
NULL);
nullptr, etc. :-)

https://codereview.chromium.org/1304143010/diff/80001/net/ssl/ssl_platform_key.h
File net/ssl/ssl_platform_key.h (right):

https://codereview.chromium.org/1304143010/diff/80001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key.h:34: // buggy smartcards.
What is g_platform_key_task_runner? This header doesn't include or reference it.

Actually, I think as a caller of this function, you don't even care about this
behavior, so you can just write "and returns an SSLPrivateKey backed by the
platform key"

https://codereview.chromium.org/1304143010/diff/80001/net/ssl/ssl_platform_ke...
File net/ssl/ssl_platform_key_win.cc (left):

https://codereview.chromium.org/1304143010/diff/80001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key_win.cc:19: #include "base/lazy_instance.h"
Still needed for this file.

[svaldez, 2015-10-14 15:06:19]

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
File net/ssl/ssl_platform_key_mac.cc (right):

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key_mac.cc:220: if (!certificate) {
On 2015/10/13 20:32:15, David Benjamin wrote:
> On 2015/09/28 16:54:53, svaldez wrote:
> > On 2015/09/25 20:10:12, David Benjamin wrote:
> > > Any reason not to have the caller handle null? This seems the sort of
> function
> > > that doesn't really have a reason to allow null when you ask it to do
> > something
> > > with a certificate.
> > 
> > Then we'd have to check both at the caller that the argument isn't null and
> also
> > check that the private key returned isn't null. A null cert and a null
private
> > key should continue down the same code path, since they both mean we can't
use
> > this cert.
> 
> In general, APIs shouldn't silently try to handle null unless what the
function
> does inherently wants a nullable type. I'm not sure this does. There also
aren't
> that many callers of FetchClientCertPrivateKey which actually can have a null
> input it seems. Just the SSLClientAuthObserver thing as far as I can tell.
> 
> There's even more implementations of this function, actually. :-)

Done.

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
File net/ssl/ssl_platform_key_nss.cc (right):

https://codereview.chromium.org/1304143010/diff/40001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key_nss.cc:162: if (!certificate ||
!certificate->os_cert_handle()) {
On 2015/10/13 20:32:15, David Benjamin wrote:
> On 2015/09/28 16:54:53, svaldez wrote:
> > On 2015/09/25 20:10:12, David Benjamin wrote:
> > > Ditto about handling in caller.
> > > 
> > > (I don't think os_cert_handle being missing is possible, is it?)
> > 
> > The handle could theoretically be closed or destructed? Same reasoning for
> > handling null here.
> 
> But the handle is owned by the X509Certificate. We shouldn't be destroying
those
> I think.

Its only owned insofar as we have a pointer to the data, but modulo caching, I
think that the OS could still access this data, and I think when I was testing
this came up as a problem.

https://codereview.chromium.org/1304143010/diff/80001/chrome/browser/chrome_c...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/1304143010/diff/80001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1973: 
On 2015/10/13 20:32:15, David Benjamin wrote:
> Probably add a comment to the #ifdef that Android can't support
> CONTENT_SETTINGS_TYPE_AUTO_SELECT_CERTIFICATE because the platform doesn't
> provide the list of matching erts.

Done.

https://codereview.chromium.org/1304143010/diff/80001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1974: #if !defined(OS_ANDROID)
On 2015/10/13 20:32:15, David Benjamin wrote:
> Nit: Move this up to line 1958 I think.

Done.

https://codereview.chromium.org/1304143010/diff/80001/chrome/browser/chrome_c...
chrome/browser/chrome_content_browser_client.cc:1991:
FetchClientCertPrivateKey(all_client_certs[i].get()).get());
On 2015/10/13 20:32:15, David Benjamin wrote:
> net:: prefix. Ugh, I bet this is compiling because of ADL.

Done.

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

https://codereview.chromium.org/1304143010/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/certificate_provider/certificate_provider_service.cc:166:
private_key->swap(new_key);
On 2015/10/13 20:32:15, David Benjamin wrote:
> private_key = new SSLPrivateKey(...)

Done.

https://codereview.chromium.org/1304143010/diff/80001/chrome/browser/ssl/ssl_...
File chrome/browser/ssl/ssl_client_auth_observer.cc (right):

https://codereview.chromium.org/1304143010/diff/80001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_client_auth_observer.cc:55:
FetchClientCertPrivateKey(certificate);
On 2015/10/13 20:32:15, David Benjamin wrote:
> net:: prefix. Ugh, I bet this is compiling because of ADL.

Done.

https://codereview.chromium.org/1304143010/diff/80001/chrome/browser/ssl/ssl_...
chrome/browser/ssl/ssl_client_auth_observer.cc:88:
FetchClientCertPrivateKey(cert_details->second);
On 2015/10/13 20:32:15, David Benjamin wrote:
> (Ditto.)

Done.

https://codereview.chromium.org/1304143010/diff/80001/chrome/chrome_browser.gypi
File chrome/chrome_browser.gypi (right):

https://codereview.chromium.org/1304143010/diff/80001/chrome/chrome_browser.g...
chrome/chrome_browser.gypi:3325:
'../third_party/boringssl/boringssl.gyp:boringssl',
On 2015/10/13 20:32:15, David Benjamin wrote:
> Corresponding change in chrome/browser/BUILD.gn

Done.

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

https://codereview.chromium.org/1304143010/diff/80001/content/browser/loader/...
content/browser/loader/resource_loader_unittest.cc:713:
test_client.ContinueWithCertificate(dummy_certs[0].get(), nullptr);
On 2015/10/13 20:32:15, David Benjamin wrote:
> Given that this call does absolutely nothing with the certificate, I would
> actually drop cert too and pass in nullptr. Maybe explicitly say "Continue the
> request with no certificate."
> 
> Also helps in trying to get rid of that very confusing X509Certificate
> constructor that creates a broken X509Certificate. I'm guessing this is why
you
> wanted to add an os_cert_handle check.

Done.

https://codereview.chromium.org/1304143010/diff/80001/content/browser/loader/...
content/browser/loader/resource_loader_unittest.cc:741:
test_client.ContinueWithCertificate(cert.get(), nullptr);
On 2015/10/13 20:32:15, David Benjamin wrote:
> Ditto

Done.

https://codereview.chromium.org/1304143010/diff/80001/content/browser/ssl/ssl...
File content/browser/ssl/ssl_client_auth_handler.h (right):

https://codereview.chromium.org/1304143010/diff/80001/content/browser/ssl/ssl...
content/browser/ssl/ssl_client_auth_handler.h:40: // may be nullptr.
On 2015/10/13 20:32:15, David Benjamin wrote:
> Nit: wrapping.

Done.

https://codereview.chromium.org/1304143010/diff/80001/content/content_tests.gypi
File content/content_tests.gypi (right):

https://codereview.chromium.org/1304143010/diff/80001/content/content_tests.g...
content/content_tests.gypi:1186:
'../third_party/boringssl/boringssl.gyp:boringssl',
On 2015/10/13 20:32:16, David Benjamin wrote:
> Corresponding change in content/test/BUILD.gn

Done.

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

https://codereview.chromium.org/1304143010/diff/80001/content/public/browser/...
content/public/browser/client_certificate_delegate.h:28: // remembered for
future requests to the domain.
On 2015/10/13 20:32:16, David Benjamin wrote:
> Nit: wrapping

Done.

https://codereview.chromium.org/1304143010/diff/80001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/1304143010/diff/80001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:1788: }
On 2015/10/13 20:32:16, David Benjamin wrote:
> Oh hrm. That hadn't occurred to me. It's kind of confusing that, if FetchBlah
> fails, we'd like to have the URLRequest signal that error code, but right now
> it's implemented by pushing the null all the way down and bouncing it back up.
> Maybe that's fine? The alternative is that ClientCertificateDelegate gets a
> CancelWithError() hook or something. Dunno. Let's leave that as-is for now.
> 
> Maybe tweak the comment to something like:
> 
> // The caller supplied a null private key. Fail the handshake and surface and
> surface an appropriate error to the caller.

Done.

https://codereview.chromium.org/1304143010/diff/80001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl_unittest.cc (right):

https://codereview.chromium.org/1304143010/diff/80001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl_unittest.cc:238:
net::WrapOpenSSLPrivateKey(client_private_key.Pass());
On 2015/10/13 20:32:16, David Benjamin wrote:
> Ooh. We could basically promote these tests to run on everything in a
follow-up
> CL having a EVP_PKEY -> SSLPrivateKey bridge available in test code
everywhere.

Acknowledged.

https://codereview.chromium.org/1304143010/diff/80001/net/ssl/ssl_client_auth...
File net/ssl/ssl_client_auth_cache_unittest.cc (right):

https://codereview.chromium.org/1304143010/diff/80001/net/ssl/ssl_client_auth...
net/ssl/ssl_client_auth_cache_unittest.cc:14: TEST(SSLClientAuthCacheTest,
LookupAddRemove) {
On 2015/10/13 20:32:16, David Benjamin wrote:
> It would probably be good if these tests had some dummy SSLPrivateKeys in
there
> rather than storing null everywhere. It can be a really silly impl that just
> NOT_IMPLEMENTED()'s everything.

Done.

https://codereview.chromium.org/1304143010/diff/80001/net/ssl/ssl_client_auth...
net/ssl/ssl_client_auth_cache_unittest.cc:39: cache.Add(server1, cert1.get(),
NULL);
On 2015/10/13 20:32:16, David Benjamin wrote:
> nullptr, etc. :-)

Done.

https://codereview.chromium.org/1304143010/diff/80001/net/ssl/ssl_platform_key.h
File net/ssl/ssl_platform_key.h (right):

https://codereview.chromium.org/1304143010/diff/80001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key.h:34: // buggy smartcards.
On 2015/10/13 20:32:16, David Benjamin wrote:
> What is g_platform_key_task_runner? This header doesn't include or reference
it.
> 
> Actually, I think as a caller of this function, you don't even care about this
> behavior, so you can just write "and returns an SSLPrivateKey backed by the
> platform key"

Done.

https://codereview.chromium.org/1304143010/diff/80001/net/ssl/ssl_platform_ke...
File net/ssl/ssl_platform_key_win.cc (left):

https://codereview.chromium.org/1304143010/diff/80001/net/ssl/ssl_platform_ke...
net/ssl/ssl_platform_key_win.cc:19: #include "base/lazy_instance.h"
On 2015/10/13 20:32:16, David Benjamin wrote:
> Still needed for this file.

Done.

[davidben, 2015-10-29 20:45:40]

Sorry, didn't realize you were waiting on me to say something. This looks good
apart from the threading/CL-splitting issues mentioned earlier.

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

https://codereview.chromium.org/1304143010/diff/140001/content/test/BUILD.gn#...
content/test/BUILD.gn:585: ]
[Assuming this was gn format's fault, though I thought it formatted it like the
LHS...]

[svaldez, 2015-10-30 15:06:15]

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

https://codereview.chromium.org/1304143010/diff/140001/content/test/BUILD.gn#...
content/test/BUILD.gn:585: ]
On 2015/10/29 20:45:40, davidben wrote:
> [Assuming this was gn format's fault, though I thought it formatted it like
the
> LHS...]

Acknowledged.

[svaldez, 2016-02-11 16:18:42]

I think that this is supposed to be the next piece to commit? Android fixes are
waiting on this so that we don't have to pipe the jobject keys into a keystore
and then removing them again.

The CAPCK Windows stuff could possibly happen first.