Use SSLPrivateKey for testing client authentication

Use SSLPrivateKey for testing client authentication

BUG=51132
Committed: https://crrev.com/0e6a4c6d35b9081b2bbbba9b07591e64369e0604
Cr-Commit-Position: refs/heads/master@{#372422}

[svaldez, 2016-01-26 17:00:10]

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

[davidben, 2016-01-28 22:24:10]

Want to staple this to BUG=51132? Looking forward to closing that ancient bug...
(I guess we probably also want to flesh out the URLRequest-level test. Right now
we only have a ContinueWithCertificate(nullptr, nullptr) one.)

https://codereview.chromium.org/1639463003/diff/100001/net/socket/ssl_client_...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/1639463003/diff/100001/net/socket/ssl_client_...
net/socket/ssl_client_socket_unittest.cc:56: #include <openssl/bn.h>
Unused?

https://codereview.chromium.org/1639463003/diff/100001/net/socket/ssl_client_...
net/socket/ssl_client_socket_unittest.cc:59: #include <openssl/rsa.h>
Unused?

https://codereview.chromium.org/1639463003/diff/100001/net/socket/ssl_client_...
net/socket/ssl_client_socket_unittest.cc:61: #include <utility>
Already included up top.

https://codereview.chromium.org/1639463003/diff/100001/net/socket/ssl_client_...
net/socket/ssl_client_socket_unittest.cc:63: #include "crypto/openssl_util.h"
Unused?

https://codereview.chromium.org/1639463003/diff/100001/net/socket/ssl_client_...
net/socket/ssl_client_socket_unittest.cc:3259: // These client auth tests are
currently dependent on OpenSSL's struct X509.
// Client auth is not supported in NSS ports.

or something like that.

https://codereview.chromium.org/1639463003/diff/100001/net/socket/ssl_client_...
net/socket/ssl_client_socket_unittest.cc:3269: scoped_refptr<SSLPrivateKey>*
pkey) {
Why not just return a scoped_refptr<Thingy>?

https://codereview.chromium.org/1639463003/diff/100001/net/socket/ssl_client_...
net/socket/ssl_client_socket_unittest.cc:3273: << filepath.value() << ": " <<
strerror(errno);
I'd drop the errno bit. I'm not sure that works on Windows anyway.

https://codereview.chromium.org/1639463003/diff/100001/net/socket/ssl_client_...
net/socket/ssl_client_socket_unittest.cc:3277:
const_cast<char*>(reinterpret_cast<const char*>(data.data())),
I think you only need the const_cast.

https://codereview.chromium.org/1639463003/diff/100001/net/socket/ssl_client_...
net/socket/ssl_client_socket_unittest.cc:3279: if (!bio.get()) {
!bio also works.

https://codereview.chromium.org/1639463003/diff/100001/net/socket/ssl_client_...
net/socket/ssl_client_socket_unittest.cc:3283: EVP_PKEY* result =
PEM_read_bio_PrivateKey(bio.get(), NULL, NULL, NULL);
crypto::ScopedEVP_PKEY result(PEM_read_bio_PrivateKey(bio.get(), nullptr,
nullptr, nullptr));
if (!result) {
  ...
}
*pkey = WrapOpenSSLPrivateKey(std::move(result));

https://codereview.chromium.org/1639463003/diff/100001/net/socket/ssl_client_...
net/socket/ssl_client_socket_unittest.cc:3293: class
SSLClientSocketOpenSSLClientAuthTest : public PlatformTest {
I think this class is more-or-less a subset of SSLClientSocketTest.
CheckSSLClientSocketSentCert isn't in there, but you can just inline that, TBH.

https://codereview.chromium.org/1639463003/diff/100001/net/socket/ssl_client_...
net/socket/ssl_client_socket_unittest.cc:3388:
TEST_F(SSLClientSocketOpenSSLClientAuthTest, NoCert) {
Why is this test in the anonymous namespace and not the rest?

https://codereview.chromium.org/1639463003/diff/100001/net/socket/ssl_client_...
net/socket/ssl_client_socket_unittest.cc:3406: // an empty certificate. It
should refuse the connection.
an empty -> no

https://codereview.chromium.org/1639463003/diff/100001/net/socket/ssl_client_...
net/socket/ssl_client_socket_unittest.cc:3406: // an empty certificate. It
should refuse the connection.
It doesn't seem to actually refuse the connection...

https://codereview.chromium.org/1639463003/diff/100001/net/socket/ssl_client_...
net/socket/ssl_client_socket_unittest.cc:3415: base::FilePath certs_dir =
GetTestCertsDirectory();
This looks unused.

https://codereview.chromium.org/1639463003/diff/100001/net/socket/ssl_client_...
net/socket/ssl_client_socket_unittest.cc:3418: ssl_config.client_cert = NULL;
nullptr

https://codereview.chromium.org/1639463003/diff/100001/net/ssl/ssl_platform_k...
File net/ssl/ssl_platform_key_test.h (right):

https://codereview.chromium.org/1639463003/diff/100001/net/ssl/ssl_platform_k...
net/ssl/ssl_platform_key_test.h:6: #define NET_SSL_SSL_PLATFORM_KEY_TEST_H_
Maybe test_ssl_private_key.h or ssl_private_key_openssl.h? It's not really
provided by the platform anymore. Having the _test suffix rather than a test_
prefix is a bit ambiguous w.r.t. whether it's actually a test or a helper
thingy. (TestWebContents vs WebContentsTest.)

https://codereview.chromium.org/1639463003/diff/100001/net/ssl/ssl_platform_k...
net/ssl/ssl_platform_key_test.h:8: #include <openssl/digest.h>
Not used?

[svaldez, 2016-01-28 22:32:27]

Description was changed from

==========
Use SSLPrivateKey for testing client authentication

BUG=
==========

to

==========
Use SSLPrivateKey for testing client authentication

BUG=51132
==========

[svaldez, 2016-01-29 17:19:30]

https://codereview.chromium.org/1639463003/diff/100001/net/socket/ssl_client_...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/1639463003/diff/100001/net/socket/ssl_client_...
net/socket/ssl_client_socket_unittest.cc:56: #include <openssl/bn.h>
On 2016/01/28 22:24:10, davidben (slow) wrote:
> Unused?

Done.

https://codereview.chromium.org/1639463003/diff/100001/net/socket/ssl_client_...
net/socket/ssl_client_socket_unittest.cc:59: #include <openssl/rsa.h>
On 2016/01/28 22:24:09, davidben (slow) wrote:
> Unused?

Done.

https://codereview.chromium.org/1639463003/diff/100001/net/socket/ssl_client_...
net/socket/ssl_client_socket_unittest.cc:61: #include <utility>
On 2016/01/28 22:24:10, davidben (slow) wrote:
> Already included up top.

Done.

https://codereview.chromium.org/1639463003/diff/100001/net/socket/ssl_client_...
net/socket/ssl_client_socket_unittest.cc:63: #include "crypto/openssl_util.h"
On 2016/01/28 22:24:10, davidben (slow) wrote:
> Unused?

Done.

https://codereview.chromium.org/1639463003/diff/100001/net/socket/ssl_client_...
net/socket/ssl_client_socket_unittest.cc:3259: // These client auth tests are
currently dependent on OpenSSL's struct X509.
On 2016/01/28 22:24:09, davidben (slow) wrote:
> // Client auth is not supported in NSS ports.
> 
> or something like that.

Done.

https://codereview.chromium.org/1639463003/diff/100001/net/socket/ssl_client_...
net/socket/ssl_client_socket_unittest.cc:3269: scoped_refptr<SSLPrivateKey>*
pkey) {
On 2016/01/28 22:24:09, davidben (slow) wrote:
> Why not just return a scoped_refptr<Thingy>?

Done.

https://codereview.chromium.org/1639463003/diff/100001/net/socket/ssl_client_...
net/socket/ssl_client_socket_unittest.cc:3273: << filepath.value() << ": " <<
strerror(errno);
On 2016/01/28 22:24:10, davidben (slow) wrote:
> I'd drop the errno bit. I'm not sure that works on Windows anyway.

Done.

https://codereview.chromium.org/1639463003/diff/100001/net/socket/ssl_client_...
net/socket/ssl_client_socket_unittest.cc:3277:
const_cast<char*>(reinterpret_cast<const char*>(data.data())),
On 2016/01/28 22:24:10, davidben (slow) wrote:
> I think you only need the const_cast.

Done.

https://codereview.chromium.org/1639463003/diff/100001/net/socket/ssl_client_...
net/socket/ssl_client_socket_unittest.cc:3279: if (!bio.get()) {
On 2016/01/28 22:24:10, davidben (slow) wrote:
> !bio also works.

Done.

https://codereview.chromium.org/1639463003/diff/100001/net/socket/ssl_client_...
net/socket/ssl_client_socket_unittest.cc:3283: EVP_PKEY* result =
PEM_read_bio_PrivateKey(bio.get(), NULL, NULL, NULL);
On 2016/01/28 22:24:09, davidben (slow) wrote:
> crypto::ScopedEVP_PKEY result(PEM_read_bio_PrivateKey(bio.get(), nullptr,
> nullptr, nullptr));
> if (!result) {
>   ...
> }
> *pkey = WrapOpenSSLPrivateKey(std::move(result));

Done.

https://codereview.chromium.org/1639463003/diff/100001/net/socket/ssl_client_...
net/socket/ssl_client_socket_unittest.cc:3293: class
SSLClientSocketOpenSSLClientAuthTest : public PlatformTest {
On 2016/01/28 22:24:09, davidben (slow) wrote:
> I think this class is more-or-less a subset of SSLClientSocketTest.
> CheckSSLClientSocketSentCert isn't in there, but you can just inline that,
TBH.

Done.

https://codereview.chromium.org/1639463003/diff/100001/net/socket/ssl_client_...
net/socket/ssl_client_socket_unittest.cc:3388:
TEST_F(SSLClientSocketOpenSSLClientAuthTest, NoCert) {
On 2016/01/28 22:24:10, davidben (slow) wrote:
> Why is this test in the anonymous namespace and not the rest?

Done.

https://codereview.chromium.org/1639463003/diff/100001/net/socket/ssl_client_...
net/socket/ssl_client_socket_unittest.cc:3406: // an empty certificate. It
should refuse the connection.
On 2016/01/28 22:24:10, davidben (slow) wrote:
> an empty -> no

This sends an empty cert (send_client_cert is true).

https://codereview.chromium.org/1639463003/diff/100001/net/socket/ssl_client_...
net/socket/ssl_client_socket_unittest.cc:3415: base::FilePath certs_dir =
GetTestCertsDirectory();
On 2016/01/28 22:24:09, davidben (slow) wrote:
> This looks unused.

Done.

https://codereview.chromium.org/1639463003/diff/100001/net/socket/ssl_client_...
net/socket/ssl_client_socket_unittest.cc:3418: ssl_config.client_cert = NULL;
On 2016/01/28 22:24:09, davidben (slow) wrote:
> nullptr

Done.

https://codereview.chromium.org/1639463003/diff/100001/net/ssl/ssl_platform_k...
File net/ssl/ssl_platform_key_test.h (right):

https://codereview.chromium.org/1639463003/diff/100001/net/ssl/ssl_platform_k...
net/ssl/ssl_platform_key_test.h:6: #define NET_SSL_SSL_PLATFORM_KEY_TEST_H_
On 2016/01/28 22:24:10, davidben (slow) wrote:
> Maybe test_ssl_private_key.h or ssl_private_key_openssl.h? It's not really
> provided by the platform anymore. Having the _test suffix rather than a test_
> prefix is a bit ambiguous w.r.t. whether it's actually a test or a helper
> thingy. (TestWebContents vs WebContentsTest.)

Done.

https://codereview.chromium.org/1639463003/diff/100001/net/ssl/ssl_platform_k...
net/ssl/ssl_platform_key_test.h:8: #include <openssl/digest.h>
On 2016/01/28 22:24:10, davidben (slow) wrote:
> Not used?

Done.

[davidben, 2016-01-29 19:19:56]

lgtm! TESTS!

https://codereview.chromium.org/1639463003/diff/140001/net/socket/ssl_client_...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/1639463003/diff/140001/net/socket/ssl_client_...
net/socket/ssl_client_socket_unittest.cc:3318: 
Probably worth an EXPECT_FALSE (or whichever it actually is) on
ssl_info.client_cert_sent here.

[svaldez, 2016-01-29 19:54:16]

The CQ bit was checked by svaldez@chromium.org

[svaldez, 2016-01-29 19:54:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/1639463003/#ps160001
(title: "Adding check.")

[commit-bot: I haz the power, 2016-01-29 19:55:51]

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

[commit-bot: I haz the power, 2016-01-29 21:17:18]

Description was changed from

==========
Use SSLPrivateKey for testing client authentication

BUG=51132
==========

to

==========
Use SSLPrivateKey for testing client authentication

BUG=51132
==========

[commit-bot: I haz the power, 2016-01-29 21:17:19]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-01-29 21:18:25]

Description was changed from

==========
Use SSLPrivateKey for testing client authentication

BUG=51132
==========

to

==========
Use SSLPrivateKey for testing client authentication

BUG=51132

Committed: https://crrev.com/0e6a4c6d35b9081b2bbbba9b07591e64369e0604
Cr-Commit-Position: refs/heads/master@{#372422}
==========

[commit-bot: I haz the power, 2016-01-29 21:18:26]

Patchset 9 (id:??) landed as
https://crrev.com/0e6a4c6d35b9081b2bbbba9b07591e64369e0604
Cr-Commit-Position: refs/heads/master@{#372422}