Add server certificate request parameters to be stored in SSLCertRequestInfo.

Add server certificate request parameters to be stored in SSLCertRequestInfo.

Currently SSLCertRequestInfo provides a list of applicable client certificates,
filtered against server request. This patch adds the server criteria to the
class as a part of a larger refactoring effort.

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

[ppi, 2013-01-03 18:59:23]

This is an attempt to implement A/ as described in http://crbug.com/166642.
Could you guys have a look?

[Ryan Sleevi, 2013-01-03 19:15:00]

Mostly this LG, but we should have unit tests.

You should be able to use the TestServer to test this functionality, ensuring
that types & names are being returned correctly.

https://codereview.chromium.org/11739004/diff/13001/net/base/ssl_cert_request...
File net/base/ssl_cert_request_info.h (right):

https://codereview.chromium.org/11739004/diff/13001/net/base/ssl_cert_request...
net/base/ssl_cert_request_info.h:22: // In TLS 1.0, the CertificateRequest
We support TLS 1.1 on (NSS, OpenSSL), which adds several more types:

      enum {
          rsa_sign(1), dss_sign(2), rsa_fixed_dh(3), dss_fixed_dh(4),
       rsa_ephemeral_dh_RESERVED(5), dss_ephemeral_dh_RESERVED(6),
       fortezza_dms_RESERVED(20),
          (255)

      } ClientCertificateType;

https://codereview.chromium.org/11739004/diff/13001/net/base/ssl_cert_request...
net/base/ssl_cert_request_info.h:35: 
nit: drop this blank line

https://codereview.chromium.org/11739004/diff/13001/net/base/ssl_cert_request...
net/base/ssl_cert_request_info.h:57: // removed as being tracked in
http://crbug.com/166642.
s/soon removed/removed soon,/

[ppi, 2013-01-07 10:43:35]

Thanks for the remarks, Ryan!

In patch set 3 I have added missing implementation for openssl socket and the
unit tests, although I verify only the |cert_authorities| field. 

|cert_key_types| is currently not being set in any of the sockets implementation
and also I believe that current TestServer implementation does not allow to
specify the cert key types allowed for client authentication. If this is
important and feasible to add, I would prefer to work on that in a separate CL
(if that's ok with you).

Could you guys have another look at the patch?

https://codereview.chromium.org/11739004/diff/13001/net/base/ssl_cert_request...
File net/base/ssl_cert_request_info.h (right):

https://codereview.chromium.org/11739004/diff/13001/net/base/ssl_cert_request...
net/base/ssl_cert_request_info.h:22: // In TLS 1.0, the CertificateRequest
On 2013/01/03 19:15:00, Ryan Sleevi wrote:
> We support TLS 1.1 on (NSS, OpenSSL), which adds several more types:
> 
>       enum {
>           rsa_sign(1), dss_sign(2), rsa_fixed_dh(3), dss_fixed_dh(4),
>        rsa_ephemeral_dh_RESERVED(5), dss_ephemeral_dh_RESERVED(6),
>        fortezza_dms_RESERVED(20),
>           (255)
> 
>       } ClientCertificateType;

Thanks, done.

https://codereview.chromium.org/11739004/diff/13001/net/base/ssl_cert_request...
net/base/ssl_cert_request_info.h:35: 
On 2013/01/03 19:15:00, Ryan Sleevi wrote:
> nit: drop this blank line

Thanks, done.

https://codereview.chromium.org/11739004/diff/13001/net/base/ssl_cert_request...
net/base/ssl_cert_request_info.h:57: // removed as being tracked in
http://crbug.com/166642.
On 2013/01/03 19:15:00, Ryan Sleevi wrote:
> s/soon removed/removed soon,/

Thanks, done.

[digit1, 2013-01-07 11:24:10]

That's a pretty nice patch :)

https://chromiumcodereview.appspot.com/11739004/diff/30002/net/base/ssl_cert_...
File net/base/ssl_cert_request_info.h (right):

https://chromiumcodereview.appspot.com/11739004/diff/30002/net/base/ssl_cert_...
net/base/ssl_cert_request_info.h:35: //   } CertificateRequest;
For the record, TLS 1.2 extends the CertificateRequest structure with a new
field named "supported_signature_algorithms" (see 7.4.4. at
http://www.ietf.org/rfc/rfc5246.txt).

I'm not sure if the current win/mac/ios/nss implementations support 1.2 though.
If one of them does, this change would result in a regression once
'client_certs' is effectively removed and the filtering moved to a different
API.

I'd suggest adding a 'cert_signature_algorithms' field here as a
std::vector<SSLClientSignatureAlgorithm>, where SSLClientSignatureAlgorithm is a
new struct type defined according to section A.4.1. of the TLS 1.2 RFC (I don't
think there is already an appropriate structure for this in the net code).

https://chromiumcodereview.appspot.com/11739004/diff/30002/net/socket/ssl_cli...
File net/socket/ssl_client_socket_unittest.cc (right):

https://chromiumcodereview.appspot.com/11739004/diff/30002/net/socket/ssl_cli...
net/socket/ssl_client_socket_unittest.cc:1041:
EXPECT_EQ(std::string(reinterpret_cast<const char*>(kThawteDN), kThawteLen),
I'd suggest using EXPECT_STREQ() instead, which will pretty-print the content of
strings in case of error. Same for line 1043 below.

[Ryan Sleevi, 2013-01-07 15:46:06]

On Jan 7, 2013 3:24 AM, <digit@chromium.org> wrote:
>
> That's a pretty nice patch :)
>
>
>
https://chromiumcodereview.appspot.com/11739004/diff/30002/net/base/ssl_cert_...
> File net/base/ssl_cert_request_info.h (right):
>
>
https://chromiumcodereview.appspot.com/11739004/diff/30002/net/base/ssl_cert_...
> net/base/ssl_cert_request_info.h:35: //   } CertificateRequest;
> For the record, TLS 1.2 extends the CertificateRequest structure with a
> new field named "supported_signature_algorithms" (see 7.4.4. at
> http://www.ietf.org/rfc/rfc5246.txt).
>
> I'm not sure if the current win/mac/ios/nss implementations support 1.2
> though. If one of them does, this change would result in a regression
> once 'client_certs' is effectively removed and the filtering moved to a
> different API.
>
> I'd suggest adding a 'cert_signature_algorithms' field here as a
> std::vector<SSLClientSignatureAlgorithm>, where
> SSLClientSignatureAlgorithm is a new struct type defined according to
> section A.4.1. of the TLS 1.2 RFC (I don't think there is already an
> appropriate structure for this in the net code).

Don't do this now. I intentionally only said TLS 1.1, because that's all
our code currently supports.

We'll revisit this once we know better what our 1.2 code and constants look
like.

>
>
https://chromiumcodereview.appspot.com/11739004/diff/30002/net/socket/ssl_cli...
> File net/socket/ssl_client_socket_unittest.cc (right):
>
>
https://chromiumcodereview.appspot.com/11739004/diff/30002/net/socket/ssl_cli...
> net/socket/ssl_client_socket_unittest.cc:1041:
> EXPECT_EQ(std::string(reinterpret_cast<const char*>(kThawteDN),
> kThawteLen),
> I'd suggest using EXPECT_STREQ() instead, which will pretty-print the
> content of strings in case of error. Same for line 1043 below.
>
> https://chromiumcodereview.appspot.com/11739004/

[ppi, 2013-01-07 18:18:48]

Thanks for the remarks David and for the clarification Ryan!

Ryan, do you think I could proceed with committing patch set 3?

https://chromiumcodereview.appspot.com/11739004/diff/30002/net/socket/ssl_cli...
File net/socket/ssl_client_socket_unittest.cc (right):

https://chromiumcodereview.appspot.com/11739004/diff/30002/net/socket/ssl_cli...
net/socket/ssl_client_socket_unittest.cc:1041:
EXPECT_EQ(std::string(reinterpret_cast<const char*>(kThawteDN), kThawteLen),
On 2013/01/07 11:24:10, digit1 wrote:
> I'd suggest using EXPECT_STREQ() instead, which will pretty-print the content
of
> strings in case of error. Same for line 1043 below.

I think that would cause troubles whenever the string contains nulls, since the
assertion requires C-style strings.

[Ryan Sleevi, 2013-01-07 18:37:58]

https://chromiumcodereview.appspot.com/11739004/diff/30002/net/socket/ssl_cli...
File net/socket/ssl_client_socket_unittest.cc (right):

https://chromiumcodereview.appspot.com/11739004/diff/30002/net/socket/ssl_cli...
net/socket/ssl_client_socket_unittest.cc:947:
GetCertRequest(net::TestServer::SSLOptions ssl_options) {
Indent: For overly long function names, we don't indent the wrap like this

ReturnType
SomeFunction(SomeVariables) {
}

https://chromiumcodereview.appspot.com/11739004/diff/30002/net/socket/ssl_cli...
net/socket/ssl_client_socket_unittest.cc:951: EXPECT_TRUE(test_server.Start());
ASSERT_TRUE

https://chromiumcodereview.appspot.com/11739004/diff/30002/net/socket/ssl_cli...
net/socket/ssl_client_socket_unittest.cc:954:
EXPECT_TRUE(test_server.GetAddressList(&addr));
ASSERT_TRUE

https://chromiumcodereview.appspot.com/11739004/diff/30002/net/socket/ssl_cli...
net/socket/ssl_client_socket_unittest.cc:968: transport,
test_server.host_port_pair(), ssl_config));
1) You don't modify ssl_config, so why create the temporary?
2) super minor pedantry - the indent style for this function is inconsistent
with all the other unit tests

https://chromiumcodereview.appspot.com/11739004/diff/30002/net/socket/ssl_cli...
net/socket/ssl_client_socket_unittest.cc:974:
scoped_refptr<net::SSLCertRequestInfo> requestInfo =
style: naming: requestInfo -> request_info

https://chromiumcodereview.appspot.com/11739004/diff/30002/net/socket/ssl_cli...
net/socket/ssl_client_socket_unittest.cc:992: #endif
design: This sort of dependency on the implementation of TestServer is something
better expressed in the TestServer interface.

It seems like we should just make client_authorities always expect paths
relative to DIR_SOURCE, so that callers on all platforms can use
GetTestCertsDirectoryRelative(..)

https://chromiumcodereview.appspot.com/11739004/diff/30002/net/socket/ssl_cli...
net/socket/ssl_client_socket_unittest.cc:999:
scoped_refptr<net::SSLCertRequestInfo> requestInfo =
style: requestInfo -> request_info

https://chromiumcodereview.appspot.com/11739004/diff/30002/net/socket/ssl_cli...
net/socket/ssl_client_socket_unittest.cc:1001: EXPECT_EQ(0,
static_cast<int>(requestInfo->cert_authorities.size()));
nit: 
EXPECT_EQ(0u, request_info->cert_authorities.size())

https://chromiumcodereview.appspot.com/11739004/diff/30002/net/socket/ssl_cli...
net/socket/ssl_client_socket_unittest.cc:1016: const size_t kThawteLen = 78;
NACK:
const size_t kThawteLen = sizeof(kThawteDN)

Do not hardcode sizes. The same on line 1031

https://chromiumcodereview.appspot.com/11739004/diff/30002/net/socket/ssl_cli...
net/socket/ssl_client_socket_unittest.cc:1040: EXPECT_EQ(2,
static_cast<int>(requestInfo->cert_authorities.size()));
ASSERT_EQ, since you're about to de-reference.

https://chromiumcodereview.appspot.com/11739004/diff/30002/net/socket/ssl_cli...
net/socket/ssl_client_socket_unittest.cc:1041:
EXPECT_EQ(std::string(reinterpret_cast<const char*>(kThawteDN), kThawteLen),
Since the data here is binary, even using the std::string overload of EXPECT_EQ
will lead to meaningless output on the console.

EXPECT_EQ(0, request_info->cert_authorities[0].compare(0, reinterpret_cast<const
char*>(kThawteDN), kThawteLen);

[ppi, 2013-01-08 17:00:38]

Thanks for the careful review, Ryan!

I have addressed your remarks in patch set 4 and in the comments below. Could
you please have another look?

https://codereview.chromium.org/11739004/diff/30002/net/socket/ssl_client_soc...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/11739004/diff/30002/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_unittest.cc:947:
GetCertRequest(net::TestServer::SSLOptions ssl_options) {
On 2013/01/07 18:37:58, Ryan Sleevi wrote:
> Indent: For overly long function names, we don't indent the wrap like this
> 
> ReturnType
> SomeFunction(SomeVariables) {
> }

Fixed, thanks!

https://codereview.chromium.org/11739004/diff/30002/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_unittest.cc:951: EXPECT_TRUE(test_server.Start());
On 2013/01/07 18:37:58, Ryan Sleevi wrote:
> ASSERT_TRUE

We cannot use hard assertions in non-void returning functions
(http://code.google.com/p/googletest/wiki/AdvancedGuide#Assertion_Placement -
despite the first sentence of the paragraph). I have made the function return
NULL whenever server setup fails in patch set 4. I feel it might be better than
obscuring the interface by adding scoped_refptr<...>* argument. What do you
think?

https://codereview.chromium.org/11739004/diff/30002/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_unittest.cc:968: transport,
test_server.host_port_pair(), ssl_config));
On 2013/01/07 18:37:58, Ryan Sleevi wrote:
> 1) You don't modify ssl_config, so why create the temporary?
> 2) super minor pedantry - the indent style for this function is inconsistent
> with all the other unit tests

Thanks, fixed both in patch set 4.

https://codereview.chromium.org/11739004/diff/30002/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_unittest.cc:974:
scoped_refptr<net::SSLCertRequestInfo> requestInfo =
On 2013/01/07 18:37:58, Ryan Sleevi wrote:
> style: naming: requestInfo -> request_info

Thanks, sorry about that!

https://codereview.chromium.org/11739004/diff/30002/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_unittest.cc:992: #endif
On 2013/01/07 18:37:58, Ryan Sleevi wrote:
> design: This sort of dependency on the implementation of TestServer is
something
> better expressed in the TestServer interface.
> 
> It seems like we should just make client_authorities always expect paths
> relative to DIR_SOURCE, so that callers on all platforms can use
> GetTestCertsDirectoryRelative(..)

I agree - I can see no reasons for LocalTestServer and RemoteTestServer to
implement the common interface differently. I would prefer to address this in a
separate CL though, as I am not sure how big/deep the necessary work will be -
would that be ok with you?

https://codereview.chromium.org/11739004/diff/30002/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_unittest.cc:999:
scoped_refptr<net::SSLCertRequestInfo> requestInfo =
On 2013/01/07 18:37:58, Ryan Sleevi wrote:
> style: requestInfo -> request_info

Thanks, sorry about that!

https://codereview.chromium.org/11739004/diff/30002/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_unittest.cc:1001: EXPECT_EQ(0,
static_cast<int>(requestInfo->cert_authorities.size()));
On 2013/01/07 18:37:58, Ryan Sleevi wrote:
> nit: 
> EXPECT_EQ(0u, request_info->cert_authorities.size())

Thanks, done!

https://codereview.chromium.org/11739004/diff/30002/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_unittest.cc:1016: const size_t kThawteLen = 78;
On 2013/01/07 18:37:58, Ryan Sleevi wrote:
> NACK:
> const size_t kThawteLen = sizeof(kThawteDN)
> 
> Do not hardcode sizes. The same on line 1031

Thanks, done!

https://codereview.chromium.org/11739004/diff/30002/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_unittest.cc:1040: EXPECT_EQ(2,
static_cast<int>(requestInfo->cert_authorities.size()));
On 2013/01/07 18:37:58, Ryan Sleevi wrote:
> ASSERT_EQ, since you're about to de-reference.

Thanks, done!

https://codereview.chromium.org/11739004/diff/30002/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_unittest.cc:1041:
EXPECT_EQ(std::string(reinterpret_cast<const char*>(kThawteDN), kThawteLen),
On 2013/01/07 18:37:58, Ryan Sleevi wrote:
> Since the data here is binary, even using the std::string overload of
EXPECT_EQ
> will lead to meaningless output on the console.
> 
> EXPECT_EQ(0, request_info->cert_authorities[0].compare(0,
reinterpret_cast<const
> char*>(kThawteDN), kThawteLen);

The ASCII content of DN fields is readable in the console output - this was
useful to me when I was writing/debugging the patch (in my first draft I have
mistaken cert Issuer with Subject when extracting the test data and knew
immediately what's wrong after seeing the console output). Do you think it can
stay this way?

[Ryan Sleevi, 2013-01-09 00:33:39]

lgtm

https://codereview.chromium.org/11739004/diff/28006/net/socket/ssl_client_soc...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/11739004/diff/28006/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_unittest.cc:1002: ASSERT_TRUE(request_info !=
NULL);
ASSERT_TRUE(request_info) should suffice

[commit-bot: I haz the power, 2013-01-09 10:06:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ppi@chromium.org/11739004/48001

[ppi, 2013-01-09 10:08:14]

Thanks, Ryan! I have changed the assertion and I am proceeding with a commit.

https://codereview.chromium.org/11739004/diff/28006/net/socket/ssl_client_soc...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/11739004/diff/28006/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_unittest.cc:1002: ASSERT_TRUE(request_info !=
NULL);
On 2013/01/09 00:33:39, Ryan Sleevi wrote:
> ASSERT_TRUE(request_info) should suffice

Thanks, done in patch set 5.

[commit-bot: I haz the power, 2013-01-09 13:27:02]

Retried try job too often on mac_rel for step(s) browser_tests

[commit-bot: I haz the power, 2013-01-09 13:27:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ppi@chromium.org/11739004/48001

[commit-bot: I haz the power, 2013-01-09 16:01:25]

Change committed as 175807