Correctly handle SSL Client Authentication requests when connecting...

net/http/http_proxy_client_socket_pool.cc

 // Copyright (c) 2010 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.
 
 #include "net/http/http_proxy_client_socket_pool.h"
 
 #include <algorithm>
 
 #include "base/time.h"
 #include "base/values.h"
@@ skipping 64 matching lines @@
     Delegate* delegate,
     NetLog* net_log)
     : ConnectJob(group_name, timeout_duration, delegate,
                  BoundNetLog::Make(net_log, NetLog::SOURCE_CONNECT_JOB)),
       params_(params),
       tcp_pool_(tcp_pool),
       ssl_pool_(ssl_pool),
       resolver_(host_resolver),
       ALLOW_THIS_IN_INITIALIZER_LIST(
           callback_(this, &HttpProxyConnectJob::OnIOComplete)),
-      using_spdy_(false) {
+      using_spdy_(false),
+      error_response_info_() {

[wtc, 2010/11/11 01:11:35]

Remove this.  We usually omit this kind of member initializer
that takes no argument.

[Ryan Hamilton, 2010/11/11 18:57:00]

Done.

 }
 
 HttpProxyConnectJob::~HttpProxyConnectJob() {}
 
 LoadState HttpProxyConnectJob::GetLoadState() const {
   switch (next_state_) {
     case STATE_TCP_CONNECT:
     case STATE_TCP_CONNECT_COMPLETE:
     case STATE_SSL_CONNECT:
     case STATE_SSL_CONNECT_COMPLETE:
@@ skipping 104 matching lines @@
   }
   next_state_ = STATE_SSL_CONNECT_COMPLETE;
   transport_socket_handle_.reset(new ClientSocketHandle());
   return transport_socket_handle_->Init(
       group_name(), params_->ssl_params(),
       params_->ssl_params()->tcp_params()->destination().priority(),
       &callback_, ssl_pool_, net_log());
 }
 
 int HttpProxyConnectJob::DoSSLConnectComplete(int result) {
-  // TODO(rch): enable support for client auth to the proxy
-  if (result == ERR_SSL_CLIENT_AUTH_CERT_NEEDED)
-    return ERR_PROXY_AUTH_UNSUPPORTED;
+  if (result == ERR_SSL_CLIENT_AUTH_CERT_NEEDED) {
+    error_response_info_ = transport_socket_handle_->ssl_error_response_info();
+    DCHECK(error_response_info_.cert_request_info.get());
+    return ERR_SSL_CLIENT_AUTH_CERT_NEEDED;

[wtc, 2010/11/11 01:11:35]

You can just return 'result' now.

[Ryan Hamilton, 2010/11/11 18:57:00]

Done.

+  }
   if (IsCertificateError(result)) {
     if (params_->ssl_params()->load_flags() & LOAD_IGNORE_ALL_CERT_ERRORS)
       result = OK;
     else
       // TODO(rch): allow the user to deal with proxy cert errors in the
       // same way as server cert errors.
       return ERR_PROXY_CERTIFICATE_INVALID;
   }
   if (result < 0) {
     if (transport_socket_handle_->socket())
       transport_socket_handle_->socket()->Disconnect();
     return ERR_PROXY_CONNECTION_FAILED;
   }
 
   SSLClientSocket* ssl =
       static_cast<SSLClientSocket*>(transport_socket_handle_->socket());
   using_spdy_ = ssl->was_spdy_negotiated();
 
   // Reset the timer to just the length of time allowed for HttpProxy handshake
   // so that a fast SSL connection plus a slow HttpProxy failure doesn't take
   // longer to timeout than it should.
   ResetTimer(base::TimeDelta::FromSeconds(
       kHttpProxyConnectJobTimeoutInSeconds));
   // TODO(rch): If we ever decide to implement a "trusted" SPDY proxy
   // (one that we speak SPDY over SSL to, but to which we send HTTPS
   // request directly instead of through CONNECT tunnels, then we
   // need to add a predicate to this if statement so we fall through
   // to the else case. (HttpProxyClientSocket currently acts as
   // a "trusted" SPDY proxy).
+  LOG(INFO) << "Connected to HTTPS proxy, using spdy: " << (using_spdy_ ? "yes" : "no");

[wtc, 2010/11/11 01:11:35]

Use VLOG(1) instead of LOG(INFO).

This line seems longer than 80 characters.

[Ryan Hamilton, 2010/11/11 18:57:00]

Removed this line.

   if (using_spdy_ && params_->tunnel())
     next_state_ = STATE_SPDY_PROXY_CREATE_STREAM;
   else
     next_state_ = STATE_HTTP_PROXY_CONNECT;
   return result;
 }
 
+void HttpProxyConnectJob::GetAdditionalErrorState(ClientSocketHandle * handle) {
+  if (error_response_info_.cert_request_info) {
+    handle->set_ssl_error_response_info(error_response_info_);
+    handle->set_is_ssl_error(true);
+  }
+}
+
 int HttpProxyConnectJob::DoSpdyProxyCreateStream() {
   DCHECK(using_spdy_);
   DCHECK(params_->tunnel());
 
   HostPortProxyPair pair(params_->destination().host_port_pair(),
                          ProxyServer::Direct());
   SpdySessionPool* spdy_pool = params_->spdy_session_pool();
   scoped_refptr<SpdySession> spdy_session;
   // It's possible that a session to the proxy has recently been created
   if (spdy_pool->HasSession(pair)) {
-    if (transport_socket_handle_->socket())
-      transport_socket_handle_->socket()->Disconnect();
-    transport_socket_handle_->Reset();
+    if (transport_socket_handle_.get()) {

[wtc, 2010/11/11 01:11:35]

Can you explain why you need the null pointer check for
transport_socket_handle_ here?

[Ryan Hamilton, 2010/11/11 18:57:00]

Yes.  If HttpProxyClientSocket::Connect() is called when a SPDY session to the
proxy already exists, then DoSSLConnect will jump to this method without
initializing the transport_socket_handle.

+      if (transport_socket_handle_->socket())
+        transport_socket_handle_->socket()->Disconnect();
+      transport_socket_handle_->Reset();
+    }
     spdy_session = spdy_pool->Get(pair, params_->spdy_settings(), net_log());
   } else {
     // Create a session direct to the proxy itself
     int rv = spdy_pool->GetSpdySessionFromSocket(
         pair, params_->spdy_settings(), transport_socket_handle_.release(),
         net_log(), OK, &spdy_session, /*using_ssl_*/ true);
     if (rv < 0) {
       if (transport_socket_handle_->socket())

[wtc, 2010/11/11 01:11:35]

I pointed this out before: after the transport_socket_handle_.release()
call on line 279 above, transport_socket_handle_ contains
NULL, so transport_socket_handle_->socket() will crash.

[Ryan Hamilton, 2010/11/11 18:57:00]

Nice catch!  It turns out the SpdySession destructor will Disconnect the socket
so we don't need this at all.

[wtc, 2010/11/12 00:12:55]

Good.  Please remove the curly braces as well.

[Ryan Hamilton, 2010/11/12 00:47:30]

Done.

         transport_socket_handle_->socket()->Disconnect();
       return rv;
     }
   }
 
   next_state_ = STATE_SPDY_PROXY_CREATE_STREAM_COMPLETE;
   return spdy_session->CreateStream(params_->request_url(),
                                     params_->destination().priority(),
                                     &spdy_stream_, net_log(), &callback_);
 }
@@ skipping 159 matching lines @@
       list->Append(ssl_pool_->GetInfoAsValue("ssl_socket_pool",
                                              "ssl_socket_pool",
                                              true));
     }
     dict->Set("nested_pools", list);
   }
   return dict;
 }
 
 }  // namespace net