Populate cert_key_types on OpenSSL.

Populate cert_key_types on OpenSSL.

Add a test to ensure it gets plumbed through correctly.

This also rolls third_party/openssl to r270417:
------------------------------------------------------------------------
r269864 | davidben@chromium.org | 2014-05-12 16:21:12 -0400 (Mon, 12 May 2014) | 7 lines

Add SSL_get_client_certificate_types.

Exposes the certificate_types parameter in a CertificateRequest.

BUG=165446
Review URL: https://codereview.chromium.org/254723002
------------------------------------------------------------------------
r270417 | davidben@chromium.org | 2014-05-14 12:27:52 -0400 (Wed, 14 May 2014) | 10 lines

Refactor ssl3_send_client_verify.

The original logic was a confusing spaghetti and mixed up initialization for
all the different cases together. Tidy it up in preparation for having to
adjust this logic later to support asynchronous crypto operations.

BUG=none
R=agl@chromium.org

Review URL: https://codereview.chromium.org//284693002
------------------------------------------------------------------------


BUG=165446
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271765

[davidben, 2014-04-25 00:46:44]

Depends on OpenSSL change here: https://codereview.chromium.org/254723002/

[wtc, 2014-04-25 18:52:39]

Patch set 2 LGTM.

https://codereview.chromium.org/257513008/diff/20001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/257513008/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.cc:1417: // First pass: we know that a
client certificate is needed, but we do not

Hmm... I believe almost the code in the first pass, with the possible exception
of the "return -1" on line 1440, is also usable when USE_OPENSSL_CERTS is not
defined.

So, if we restructure the ifdef here, we should be able to run the new
SSLClientSocketCertRequestInfoTest.ClientKeyTypes test with just #if
defined(USE_OPENSSL).

https://codereview.chromium.org/257513008/diff/20001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_openssl.h (right):

https://codereview.chromium.org/257513008/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.h:201: // List of certificate_type values
for client certificates allowed by the

Nit: "certificate_type" is a little confusing because it looks like a
class/struct member name. I suggest "SSLClientCertType".

https://codereview.chromium.org/257513008/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.h:203: std::vector<SSLClientCertType>
cert_key_types_;

Nit: It seems better to name this member client_cert_types_.

https://codereview.chromium.org/257513008/diff/20001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/257513008/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_unittest.cc:2122: // client_key_types is only
populated on OpenSSL and currently only when

1. Typo: client_key_types => cert_key_types

2. We can fix this in SSLClientSocketNSS by calling
SSL_GetRequestedClientCertificateTypes, but I won't insist on it.

https://codereview.chromium.org/257513008/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_unittest.cc:2125:
TEST_F(SSLClientSocketCertRequestInfoTest, ClientKeyTypes) {

Nit: ClientKeyTypes => CertKeyTypes or ClientCertKeyTypes

https://codereview.chromium.org/257513008/diff/20001/net/test/spawned_test_se...
File net/test/spawned_test_server/base_test_server.h (right):

https://codereview.chromium.org/257513008/diff/20001/net/test/spawned_test_se...
net/test/spawned_test_server/base_test_server.h:105: enum ClientKeyType {

Nit: I suggest naming this type "ClientCertType" because TLS refers to this as
"certificate_types" as opposed to "key_types".

Update: I believe this enum type is not used, so it can be simply deleted.

https://codereview.chromium.org/257513008/diff/20001/net/tools/testserver/tes...
File net/tools/testserver/testserver.py (right):

https://codereview.chromium.org/257513008/diff/20001/net/tools/testserver/tes...
net/tools/testserver/testserver.py:190: for key_type in ssl_client_cert_types:

Nit: key_type => cert_type ?

https://codereview.chromium.org/257513008/diff/20001/third_party/tlslite/tlsl...
File third_party/tlslite/tlslite/constants.py (right):

https://codereview.chromium.org/257513008/diff/20001/third_party/tlslite/tlsl...
third_party/tlslite/tlslite/constants.py:23: ecdsa_fixed_ecdh = 66  # RFC 4492

Nit: rather than referencing RFC 4492, you can just reference the IANA registry:
http://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml#tls-param...

[davidben, 2014-04-25 20:52:31]

https://codereview.chromium.org/257513008/diff/20001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/257513008/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.cc:1417: // First pass: we know that a
client certificate is needed, but we do not
On 2014/04/25 18:52:40, wtc wrote:
> 
> Hmm... I believe almost the code in the first pass, with the possible
exception
> of the "return -1" on line 1440, is also usable when USE_OPENSSL_CERTS is not
> defined.
> 
> So, if we restructure the ifdef here, we should be able to run the new
> SSLClientSocketCertRequestInfoTest.ClientKeyTypes test with just #if
> defined(USE_OPENSSL).

Hrm. Yeah, looks like it. I suspect it was done this way just because the client
auth code is otherwise not hooked up to anything? I dunno. Not sure what the
plan is. (Ryan?)

https://codereview.chromium.org/257513008/diff/20001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_openssl.h (right):

https://codereview.chromium.org/257513008/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.h:201: // List of certificate_type values
for client certificates allowed by the
On 2014/04/25 18:52:40, wtc wrote:
> 
> Nit: "certificate_type" is a little confusing because it looks like a
> class/struct member name. I suggest "SSLClientCertType".

Done.

https://codereview.chromium.org/257513008/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.h:203: std::vector<SSLClientCertType>
cert_key_types_;
On 2014/04/25 18:52:40, wtc wrote:
> 
> Nit: It seems better to name this member client_cert_types_.

I named it cert_key_types mostly to match in name in SSLCertRequestInfo. I
dunno, our naming is pretty inconsistent. SSLClientCertType/cert_key_types.
cert_authorities/client_authorities.

https://codereview.chromium.org/257513008/diff/20001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/257513008/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_unittest.cc:2122: // client_key_types is only
populated on OpenSSL and currently only when
On 2014/04/25 18:52:40, wtc wrote:
> 
> 1. Typo: client_key_types => cert_key_types

Done.

> 2. We can fix this in SSLClientSocketNSS by calling
> SSL_GetRequestedClientCertificateTypes, but I won't insist on it.

Oh hey. Did we just add it and never hook it up? Maybe let's do that in a
follow-up since the code later in the path doesn't consume the value either.
Strangely, Android's client auth code does consume the value even though it's
never populated.

https://codereview.chromium.org/257513008/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_unittest.cc:2125:
TEST_F(SSLClientSocketCertRequestInfoTest, ClientKeyTypes) {
On 2014/04/25 18:52:40, wtc wrote:
> 
> Nit: ClientKeyTypes => CertKeyTypes or ClientCertKeyTypes

Done.

https://codereview.chromium.org/257513008/diff/20001/net/test/spawned_test_se...
File net/test/spawned_test_server/base_test_server.h (right):

https://codereview.chromium.org/257513008/diff/20001/net/test/spawned_test_se...
net/test/spawned_test_server/base_test_server.h:105: enum ClientKeyType {
On 2014/04/25 18:52:40, wtc wrote:
> 
> Nit: I suggest naming this type "ClientCertType" because TLS refers to this as
> "certificate_types" as opposed to "key_types".
> 
> Update: I believe this enum type is not used, so it can be simply deleted.

Oops. Removed.

https://codereview.chromium.org/257513008/diff/20001/net/tools/testserver/tes...
File net/tools/testserver/testserver.py (right):

https://codereview.chromium.org/257513008/diff/20001/net/tools/testserver/tes...
net/tools/testserver/testserver.py:190: for key_type in ssl_client_cert_types:
On 2014/04/25 18:52:40, wtc wrote:
> 
> Nit: key_type => cert_type ?

Done.

https://codereview.chromium.org/257513008/diff/20001/third_party/tlslite/tlsl...
File third_party/tlslite/tlslite/constants.py (right):

https://codereview.chromium.org/257513008/diff/20001/third_party/tlslite/tlsl...
third_party/tlslite/tlslite/constants.py:23: ecdsa_fixed_ecdh = 66  # RFC 4492
On 2014/04/25 18:52:40, wtc wrote:
> 
> Nit: rather than referencing RFC 4492, you can just reference the IANA
registry:
>
http://www.iana.org/assignments/tls-parameters/tls-parameters.xhtml#tls-param...

Done.

[Ryan Sleevi, 2014-04-26 01:47:33]

https://codereview.chromium.org/257513008/diff/20001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/257513008/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.cc:1417: // First pass: we know that a
client certificate is needed, but we do not
On 2014/04/25 20:52:31, David Benjamin wrote:
> On 2014/04/25 18:52:40, wtc wrote:
> > 
> > Hmm... I believe almost the code in the first pass, with the possible
> exception
> > of the "return -1" on line 1440, is also usable when USE_OPENSSL_CERTS is
not
> > defined.
> > 
> > So, if we restructure the ifdef here, we should be able to run the new
> > SSLClientSocketCertRequestInfoTest.ClientKeyTypes test with just #if
> > defined(USE_OPENSSL).
> 
> Hrm. Yeah, looks like it. I suspect it was done this way just because the
client
> auth code is otherwise not hooked up to anything? I dunno. Not sure what the
> plan is. (Ryan?)

Yeah, when we support platform native auth, this will all move from being gated
behind USE_OPENSSL_CERTS, with the exception of line 1454/1455, which will need
to shim OS cert into OpenSSL cert. That is, platform-native auth will be
implemented using an OpenSSLClientKeyStore that vends an EVP_PKEY that talks to
the OS-layer for signing when OpenSSL calls into it.

I'm fine making some of that change here.

[wtc, 2014-04-26 12:36:04]

Patch set 3 LGTM.

https://codereview.chromium.org/257513008/diff/20001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/257513008/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_unittest.cc:2122: // client_key_types is only
populated on OpenSSL and currently only when
On 2014/04/25 20:52:31, David Benjamin wrote:
> 
> > 2. We can fix this in SSLClientSocketNSS by calling
> > SSL_GetRequestedClientCertificateTypes, but I won't insist on it.
> 
> Oh hey. Did we just add it and never hook it up?

SSL_GetRequestedClientCertificateTypes was used before, when origin-bound
certificates were used as SSL client certificates during the handshake.

> Maybe let's do that in a
> follow-up since the code later in the path doesn't consume the value either.

Yes, that is certainly fine.

[davidben, 2014-04-30 21:48:16]

https://codereview.chromium.org/257513008/diff/20001/net/socket/ssl_client_so...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/257513008/diff/20001/net/socket/ssl_client_so...
net/socket/ssl_client_socket_openssl.cc:1417: // First pass: we know that a
client certificate is needed, but we do not
On 2014/04/26 01:47:33, Ryan Sleevi wrote:
> On 2014/04/25 20:52:31, David Benjamin wrote:
> > On 2014/04/25 18:52:40, wtc wrote:
> > > 
> > > Hmm... I believe almost the code in the first pass, with the possible
> > exception
> > > of the "return -1" on line 1440, is also usable when USE_OPENSSL_CERTS is
> not
> > > defined.
> > > 
> > > So, if we restructure the ifdef here, we should be able to run the new
> > > SSLClientSocketCertRequestInfoTest.ClientKeyTypes test with just #if
> > > defined(USE_OPENSSL).
> > 
> > Hrm. Yeah, looks like it. I suspect it was done this way just because the
> client
> > auth code is otherwise not hooked up to anything? I dunno. Not sure what the
> > plan is. (Ryan?)
> 
> Yeah, when we support platform native auth, this will all move from being
gated
> behind USE_OPENSSL_CERTS, with the exception of line 1454/1455, which will
need
> to shim OS cert into OpenSSL cert. That is, platform-native auth will be
> implemented using an OpenSSLClientKeyStore that vends an EVP_PKEY that talks
to
> the OS-layer for signing when OpenSSL calls into it.
> 
> I'm fine making some of that change here.

Alright. Moved stuff out from there.

[wtc, 2014-04-30 22:03:04]

Patch set 5 LGTM.

[davidben, 2014-05-16 19:05:55]

Updated to included an openssl roll, mostly for try coverage. I can split the
roll out into a separate CL when landing if that's preferred.

[davidben, 2014-05-20 17:37:37]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2014-05-20 17:38:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/davidben@chromium.org/257513008/160001

[commit-bot: I haz the power, 2014-05-20 21:51:49]

Change committed as 271765