SSLCertRequestInfo: Add |valid_cas| and |valid_key_types|

net/socket/ssl_client_socket_openssl.cc

Index: net/socket/ssl_client_socket_openssl.cc
diff --git a/net/socket/ssl_client_socket_openssl.cc b/net/socket/ssl_client_socket_openssl.cc
index 37b494a7a825bc6462c444314f38303aecda11ce..6901864346f61217120a2b7cee7b042655655cbd 100644
--- a/net/socket/ssl_client_socket_openssl.cc
+++ b/net/socket/ssl_client_socket_openssl.cc
@@ -638,7 +638,42 @@ bool SSLClientSocketOpenSSL::GetSSLInfo(SSLInfo* ssl_info) {
 void SSLClientSocketOpenSSL::GetSSLCertRequestInfo(
     SSLCertRequestInfo* cert_request_info) {
   cert_request_info->host_and_port = host_and_port_.ToString();
-  cert_request_info->client_certs = client_certs_;
+  cert_request_info->no_client_certs = true;
+  cert_request_info->client_certs.clear();
+  cert_request_info->valid_cas.clear();
+  cert_request_info->valid_key_types.clear();
+
+  // Convert the list of CA Principals to encoded form.
+  // Note that SSL_get_client_CA_list() doesn't increment the
+  // reference count of the returned list items, there is no
+  // need to used a scoped type here.
+  STACK_OF(X509_NAME)* client_cas = SSL_get_client_CA_list(ssl_);
+  if (client_cas != NULL) {
+    int count = 0;
+    for (int n = 0; n < sk_X509_NAME_num(client_cas); ++n) {
+      X509_NAME* ca_name = sk_X509_NAME_value(client_cas, n);
+      if (ca_name == NULL)
+        continue;
+
+      unsigned char* encoded_name = NULL;
+      int encoded_len = i2d_X509_NAME(ca_name, &encoded_name);
+      if (encoded_len > 0) {
+        // push an empty string in the vector, then assign it the
+        // encoded content, this avoids an extra copy.
+        cert_request_info->valid_cas.push_back(std::string());

[Ryan Sleevi, 2012/12/11 21:30:24]

I think this comment is ambiguous - it suggests that the assignment is not
copying |encoded_name|, even though there's no standards-compliant way it could.

Further, it discounts compiler-optimizations with regards to RVO and placements.
That is, it's legal for it to recognize that

.push_back(std::string(reinterpret_cast<...>, ...)) is creating a temporary, and
to have the std::string() invoked as part of the new type used for push_back

*in general*, such cleverness is strongly avoided in Chromium code unless it's a
demonstrated necessity. The cognitive overhead (and both the comment and the
assign required me to double check what's going on here) is not worth the
perceived benefits.

[digit1, 2012/12/11 23:05:31]

I understand, it really means that a
.push_back(std::string(encoded_name,encoded_len)) would generate two memory
copies of the data, in the absence of C++11 move semantics. It will be removed
anyway (see below).

 
Ah, someone actually recommended this technique in a previous unrelated patch.
I'm completely ok doing this the standard way :)

+        cert_request_info->valid_cas[count].assign(
+            reinterpret_cast<const char*>(encoded_name),
+            static_cast<size_t>(encoded_len));
+        count++;

[Ryan Sleevi, 2012/12/11 21:30:24]

post-increment

+        OPENSSL_free(encoded_name);
+      }
+    }
+  }
+
+  // There is no OpenSSL API to retrieve the list of certificate key
+  // types from the "CertificateRequest" message for now, so hard-code
+  // RSA, which is by far the most common one. crbug.com/165446

[Ryan Sleevi, 2012/12/11 21:30:24]

http:// linkify

[digit1, 2012/12/11 23:05:31]

Sure, will do.

+  cert_request_info->valid_key_types.push_back(CLIENT_CERT_RSA_SIGN);
 }
 
 int SSLClientSocketOpenSSL::ExportKeyingMaterial(
@@ -760,7 +795,6 @@ void SSLClientSocketOpenSSL::Disconnect() {
   server_cert_verify_result_.Reset();
   completed_handshake_ = false;
 
-  client_certs_.clear();
   client_auth_cert_needed_ = false;
 }