Fix SSLClientSocketOpenSSL error-handling for Channel ID.

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 4ff8d438e965b1fdf0b5b7f8b4ecdb75e1d14d66..26c6a1e0ec22f3a7afe90a3bc72751fc98fd8002 100644
--- a/net/socket/ssl_client_socket_openssl.cc
+++ b/net/socket/ssl_client_socket_openssl.cc
@@ -127,7 +127,6 @@ class SSLClientSocketOpenSSL::SSLContext {
     session_cache_.Reset(ssl_ctx_.get(), kDefaultSessionCacheConfig);
     SSL_CTX_set_cert_verify_callback(ssl_ctx_.get(), CertVerifyCallback, NULL);
     SSL_CTX_set_client_cert_cb(ssl_ctx_.get(), ClientCertCallback);
-    SSL_CTX_set_channel_id_cb(ssl_ctx_.get(), ChannelIDCallback);
     SSL_CTX_set_verify(ssl_ctx_.get(), SSL_VERIFY_PEER, NULL);
     // TODO(kristianm): Only select this if ssl_config_.next_proto is not empty.
     // It would be better if the callback were not a global setting,
@@ -150,12 +149,6 @@ class SSLClientSocketOpenSSL::SSLContext {
     return socket->ClientCertRequestCallback(ssl, x509, pkey);
   }
 
-  static void ChannelIDCallback(SSL* ssl, EVP_PKEY** pkey) {
-    SSLClientSocketOpenSSL* socket = GetInstance()->GetClientSocketFromSSL(ssl);
-    CHECK(socket);
-    socket->ChannelIDRequestCallback(ssl, pkey);
-  }
-
   static int CertVerifyCallback(X509_STORE_CTX *store_ctx, void *arg) {
     SSL* ssl = reinterpret_cast<SSL*>(X509_STORE_CTX_get_ex_data(
         store_ctx, SSL_get_ex_data_X509_STORE_CTX_idx()));
@@ -358,7 +351,6 @@ SSLClientSocketOpenSSL::SSLClientSocketOpenSSL(
       trying_cached_session_(false),
       next_handshake_state_(STATE_NONE),
       npn_status_(kNextProtoUnsupported),
-      channel_id_request_return_value_(ERR_UNEXPECTED),
       channel_id_xtn_negotiated_(false),
       net_log_(transport_->socket()->NetLog()) {}
 
@@ -476,6 +468,9 @@ void SSLClientSocketOpenSSL::Disconnect() {
   cert_authorities_.clear();
   cert_key_types_.clear();
   client_auth_cert_needed_ = false;
+
+  channel_id_xtn_negotiated_ = false;
+  channel_id_request_handle_.Cancel();
 }
 
 bool SSLClientSocketOpenSSL::IsConnected() const {
@@ -827,9 +822,10 @@ int SSLClientSocketOpenSSL::DoHandshake() {
     int ssl_error = SSL_get_error(ssl_, rv);
 
     if (ssl_error == SSL_ERROR_WANT_CHANNEL_ID_LOOKUP) {
-      // The server supports TLS channel id and the lookup is asynchronous.
-      // Retrieve the error from the call to |server_bound_cert_service_|.
-      net_error = channel_id_request_return_value_;
+      // The server supports channel ID. Stop to look one up before returning to
+      // the handshake.
+      GotoState(STATE_CHANNEL_ID_LOOKUP);
+      return OK;
     } else {

[wtc, 2014/06/19 22:00:11]

Nit: remove the "else" because now the "if" branch ends with a return statement.

[davidben, 2014/06/19 22:36:38]

Done.

       net_error = MapOpenSSLError(ssl_error, err_tracer);
     }
@@ -849,6 +845,50 @@ int SSLClientSocketOpenSSL::DoHandshake() {
   return net_error;
 }
 
+int SSLClientSocketOpenSSL::DoChannelIDLookup() {
+  channel_id_xtn_negotiated_ = true;

[wtc, 2014/06/19 22:00:11]

Nit: this probably can be moved to line 827.

[davidben, 2014/06/19 22:36:38]

Done.

+
+  GotoState(STATE_CHANNEL_ID_LOOKUP_COMPLETE);
+  return server_bound_cert_service_->GetOrCreateDomainBoundCert(
+      host_and_port_.host(),
+      &channel_id_private_key_,
+      &channel_id_cert_,
+      base::Bind(&SSLClientSocketOpenSSL::OnHandshakeIOComplete,
+                 base::Unretained(this)),
+      &channel_id_request_handle_);
+}
+
+int SSLClientSocketOpenSSL::DoChannelIDLookupComplete(int result) {
+  if (result < 0)
+    return result;
+
+  DCHECK_LT(0u, channel_id_private_key_.size());
+  // Decode key.
+  std::vector<uint8> encrypted_private_key_info;
+  std::vector<uint8> subject_public_key_info;
+  encrypted_private_key_info.assign(
+      channel_id_private_key_.data(),
+      channel_id_private_key_.data() + channel_id_private_key_.size());
+  subject_public_key_info.assign(
+      channel_id_cert_.data(),
+      channel_id_cert_.data() + channel_id_cert_.size());
+  scoped_ptr<crypto::ECPrivateKey> ec_private_key(
+      crypto::ECPrivateKey::CreateFromEncryptedPrivateKeyInfo(
+          ServerBoundCertService::kEPKIPassword,
+          encrypted_private_key_info,
+            subject_public_key_info));

[wtc, 2014/06/19 22:00:11]

Nit: indentation is wrong.

[davidben, 2014/06/19 22:36:38]

Done.

+  if (!ec_private_key) {
+    LOG(ERROR) << "Failed to import Channel ID.";
+    return ERR_CHANNEL_ID_IMPORT_FAILED;
+  }
+  set_channel_id_sent(true);
+  SSL_set1_tls_channel_id(ssl_, ec_private_key->key());

[wtc, 2014/06/19 22:00:11]

I assume SSL_set1_tls_channel_id will call EVP_PKEY_dup on its second argument?

[davidben, 2014/06/19 22:36:38]

Yeah. I think set1 vs set may be the OpenSSL convention for "takes a reference"?
Anyway, it hits this function:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/openss...

For completeness, this is equivalent to what we were doing before because
tlsext_channel_id_enabled was already set by SSL_enable_tls_channel_id:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/openss...

And this is the code that calls the callback:

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/openss...

I suppose for completeness, we really should be checking the return value there,
in case OpenSSL rejects it for being the wrong size. Fixed.

+
+  // Return to the handshake.
+  GotoState(STATE_HANDSHAKE);
+  return OK;
+}
+
 int SSLClientSocketOpenSSL::DoVerifyCert(int result) {
   DCHECK(server_cert_.get());
   GotoState(STATE_VERIFY_CERT_COMPLETE);
@@ -993,6 +1033,13 @@ int SSLClientSocketOpenSSL::DoHandshakeLoop(int last_io_result) {
       case STATE_HANDSHAKE:
         rv = DoHandshake();
         break;
+      case STATE_CHANNEL_ID_LOOKUP:
+        DCHECK_EQ(OK, rv);
+        rv = DoChannelIDLookup();
+       break;
+      case STATE_CHANNEL_ID_LOOKUP_COMPLETE:
+        rv = DoChannelIDLookupComplete(rv);
+        break;
       case STATE_VERIFY_CERT:
         DCHECK(rv == OK);

[wtc, 2014/06/19 22:00:11]

Please also change this to DCHECK_EQ(OK, rv) for consistency.

[davidben, 2014/06/19 22:36:38]

Done.

         rv = DoVerifyCert(rv);
@@ -1321,46 +1368,6 @@ int SSLClientSocketOpenSSL::ClientCertRequestCallback(SSL* ssl,
   return 0;
 }
 
-void SSLClientSocketOpenSSL::ChannelIDRequestCallback(SSL* ssl,
-                                                      EVP_PKEY** pkey) {
-  DVLOG(3) << "OpenSSL ChannelIDRequestCallback called";
-  DCHECK_EQ(ssl, ssl_);
-  DCHECK(!*pkey);
-
-  channel_id_xtn_negotiated_ = true;
-  if (!channel_id_private_key_.size()) {
-    channel_id_request_return_value_ =
-        server_bound_cert_service_->GetOrCreateDomainBoundCert(
-            host_and_port_.host(),
-            &channel_id_private_key_,
-            &channel_id_cert_,
-            base::Bind(&SSLClientSocketOpenSSL::OnHandshakeIOComplete,
-                       base::Unretained(this)),
-            &channel_id_request_handle_);
-    if (channel_id_request_return_value_ != OK)
-      return;
-  }
-
-  // Decode key.
-  std::vector<uint8> encrypted_private_key_info;
-  std::vector<uint8> subject_public_key_info;
-  encrypted_private_key_info.assign(
-      channel_id_private_key_.data(),
-      channel_id_private_key_.data() + channel_id_private_key_.size());
-  subject_public_key_info.assign(
-      channel_id_cert_.data(),
-      channel_id_cert_.data() + channel_id_cert_.size());
-  scoped_ptr<crypto::ECPrivateKey> ec_private_key(
-      crypto::ECPrivateKey::CreateFromEncryptedPrivateKeyInfo(
-          ServerBoundCertService::kEPKIPassword,
-          encrypted_private_key_info,
-          subject_public_key_info));
-  if (!ec_private_key)
-    return;
-  set_channel_id_sent(true);
-  *pkey = EVP_PKEY_dup(ec_private_key->key());
-}
-
 int SSLClientSocketOpenSSL::CertVerifyCallback(X509_STORE_CTX* store_ctx) {
   if (!completed_handshake_) {
     // If the first handshake hasn't completed then we accept any certificates