Fix SSLClientSocketOpenSSL error-handling for Channel ID.

net/socket/ssl_client_socket_openssl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // OpenSSL binding for SSLClientSocket. The class layout and general principle
 // of operation is derived from SSLClientSocketNSS.
 
 #include "net/socket/ssl_client_socket_openssl.h"
 
 #include <openssl/err.h>
@@ skipping 109 matching lines @@
   friend struct DefaultSingletonTraits<SSLContext>;
 
   SSLContext() {
     crypto::EnsureOpenSSLInit();
     ssl_socket_data_index_ = SSL_get_ex_new_index(0, 0, 0, 0, 0);
     DCHECK_NE(ssl_socket_data_index_, -1);
     ssl_ctx_.reset(SSL_CTX_new(SSLv23_client_method()));
     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,
     // but that is an OpenSSL issue.
     SSL_CTX_set_next_proto_select_cb(ssl_ctx_.get(), SelectNextProtoCallback,
                                      NULL);
   }
 
   static std::string GetSessionCacheKey(const SSL* ssl) {
     SSLClientSocketOpenSSL* socket = GetInstance()->GetClientSocketFromSSL(ssl);
     DCHECK(socket);
     return GetSocketSessionCacheKey(*socket);
   }
 
   static SSLSessionCacheOpenSSL::Config kDefaultSessionCacheConfig;
 
   static int ClientCertCallback(SSL* ssl, X509** x509, EVP_PKEY** pkey) {
     SSLClientSocketOpenSSL* socket = GetInstance()->GetClientSocketFromSSL(ssl);
     CHECK(socket);
     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()));
     SSLClientSocketOpenSSL* socket = GetInstance()->GetClientSocketFromSSL(ssl);
     CHECK(socket);
 
     return socket->CertVerifyCallback(store_ctx);
   }
 
   static int SelectNextProtoCallback(SSL* ssl,
@@ skipping 182 matching lines @@
       server_bound_cert_service_(context.server_bound_cert_service),
       ssl_(NULL),
       transport_bio_(NULL),
       transport_(transport_socket.Pass()),
       host_and_port_(host_and_port),
       ssl_config_(ssl_config),
       ssl_session_cache_shard_(context.ssl_session_cache_shard),
       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()) {}
 
 SSLClientSocketOpenSSL::~SSLClientSocketOpenSSL() {
   Disconnect();
 }
 
 void SSLClientSocketOpenSSL::GetSSLCertRequestInfo(
     SSLCertRequestInfo* cert_request_info) {
   cert_request_info->host_and_port = host_and_port_;
@@ skipping 97 matching lines @@
 
   pending_read_error_ = kNoPendingReadResult;
   transport_write_error_ = OK;
 
   server_cert_verify_result_.Reset();
   completed_handshake_ = false;
 
   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 {
   // If the handshake has not yet completed.
   if (!completed_handshake_)
     return false;
   // If an asynchronous operation is still pending.
   if (user_read_buf_.get() || user_write_buf_.get())
     return true;
 
@@ skipping 331 matching lines @@
     DCHECK(got_cert);
     net_log_.AddEvent(
         NetLog::TYPE_SSL_CERTIFICATES_RECEIVED,
         base::Bind(&NetLogX509CertificateCallback,
                    base::Unretained(server_cert_.get())));
     GotoState(STATE_VERIFY_CERT);
   } else {
     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);
     }
 
     // If not done, stay in this state
     if (net_error == ERR_IO_PENDING) {
       GotoState(STATE_HANDSHAKE);
     } else {
       LOG(ERROR) << "handshake failed; returned " << rv
                  << ", SSL error code " << ssl_error
                  << ", net_error " << net_error;
       net_log_.AddEvent(
           NetLog::TYPE_SSL_HANDSHAKE_ERROR,
           CreateNetLogSSLErrorCallback(net_error, ssl_error));
     }
   }
   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);
 
   CertStatus cert_status;
   if (ssl_config_.IsAllowedBadCert(server_cert_.get(), &cert_status)) {
     VLOG(1) << "Received an expected bad cert with status: " << cert_status;
     server_cert_verify_result_.Reset();
     server_cert_verify_result_.cert_status = cert_status;
     server_cert_verify_result_.verified_cert = server_cert_;
@@ skipping 124 matching lines @@
     // (This is a quirk carried over from the windows
     // implementation.  It makes reading the logs a bit harder.)
     // State handlers can and often do call GotoState just
     // to stay in the current state.
     State state = next_handshake_state_;
     GotoState(STATE_NONE);
     switch (state) {
       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);
        break;
       case STATE_VERIFY_CERT_COMPLETE:
         rv = DoVerifyCertComplete(rv);
         break;
       case STATE_NONE:
       default:
         rv = ERR_UNEXPECTED;
         NOTREACHED() << "unexpected state" << state;
         break;
@@ skipping 306 matching lines @@
 #else  // !defined(USE_OPENSSL_CERTS)
     // OS handling of client certificates is not yet implemented.
     NOTIMPLEMENTED();
 #endif  // defined(USE_OPENSSL_CERTS)
   }
 
   // Send no client certificate.
   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
     // because we verify after the handshake.
     return 1;
   }
 
   CHECK(server_cert_.get());
 
   PeerCertificateChain chain(store_ctx->untrusted);
@@ skipping 56 matching lines @@
   DVLOG(2) << "next protocol: '" << npn_proto_ << "' status: " << npn_status_;
   return SSL_TLSEXT_ERR_OK;
 }
 
 scoped_refptr<X509Certificate>
 SSLClientSocketOpenSSL::GetUnverifiedServerCertificateChain() const {
   return server_cert_;
 }
 
 }  // namespace net