An implementation of chrome.socket.secure().

chrome/browser/extensions/api/socket/socket_api.cc

Index: chrome/browser/extensions/api/socket/socket_api.cc
diff --git a/chrome/browser/extensions/api/socket/socket_api.cc b/chrome/browser/extensions/api/socket/socket_api.cc
index da173c2952ae873f8068a4db783838efd5bf2225..9b5fe038c7b063636395897eb42ad06813c03137 100644
--- a/chrome/browser/extensions/api/socket/socket_api.cc
+++ b/chrome/browser/extensions/api/socket/socket_api.cc
@@ -12,6 +12,7 @@
 #include "chrome/browser/extensions/api/dns/host_resolver_wrapper.h"
 #include "chrome/browser/extensions/api/socket/socket.h"
 #include "chrome/browser/extensions/api/socket/tcp_socket.h"
+#include "chrome/browser/extensions/api/socket/tls_socket.h"
 #include "chrome/browser/extensions/api/socket/udp_socket.h"
 #include "chrome/browser/extensions/extension_system.h"
 #include "chrome/browser/io_thread.h"
@@ -24,6 +25,10 @@
 #include "net/base/net_errors.h"
 #include "net/base/net_log.h"
 #include "net/base/net_util.h"
+#include "net/socket/client_socket_handle.h"
+#include "net/socket/client_socket_factory.h"
+#include "net/url_request/url_request_context.h"
+#include "net/url_request/url_request_context_getter.h"
 
 namespace extensions {
 
@@ -44,9 +49,32 @@ const char kTCPSocketBindError[] =
     "TCP socket does not support bind. For TCP server please use listen.";
 const char kMulticastSocketTypeError[] =
     "Only UDP socket supports multicast.";
+const char kSecureSocketTypeError[] =
+    "Only TCP sockets are supported for TLS.";
+const char kSocketNotConnectedError[] = "Socket not connected";
 const char kWildcardAddress[] = "*";
 const int kWildcardPort = 0;
 
+namespace {
+// Returns the SSL protocol version (as a uint16) represented by a string.
+// Returns 0 if the string is invalid.  Pulled from
+// chrome/browser/net/ssl_config_service_manager_pref.cc.
+uint16 SSLProtocolVersionFromString(const std::string& version_str) {
+  uint16 version = 0;  // Invalid.
+  if (version_str == "ssl3") {
+    version = net::SSL_PROTOCOL_VERSION_SSL3;
+  } else if (version_str == "tls1") {
+    version = net::SSL_PROTOCOL_VERSION_TLS1;
+  } else if (version_str == "tls1.1") {
+    version = net::SSL_PROTOCOL_VERSION_TLS1_1;
+  } else if (version_str == "tls1.2") {
+    version = net::SSL_PROTOCOL_VERSION_TLS1_2;
+  }
+  return version;
+}
+}  // namespace
+
+
 SocketAsyncApiFunction::SocketAsyncApiFunction() {
 }
 
@@ -76,6 +104,11 @@ Socket* SocketAsyncApiFunction::GetSocket(int api_resource_id) {
   return manager_->Get(extension_->id(), api_resource_id);
 }
 
+void SocketAsyncApiFunction::SetSocket(int api_resource_id,
+                                       Socket* socket) {
+  manager_->Set(extension_->id(), api_resource_id, socket);
+}
+
 base::hash_set<int>* SocketAsyncApiFunction::GetSocketIds() {
   return manager_->GetResourceIds(extension_->id());
 }
@@ -239,6 +272,7 @@ void SocketConnectFunction::AfterDnsLookup(int lookup_result) {
 }
 
 void SocketConnectFunction::StartConnect() {
+  socket_->SetOriginalHostname(hostname_);
   socket_->Connect(resolved_address_, port_,
                    base::Bind(&SocketConnectFunction::OnConnect, this));
 }
@@ -896,4 +930,157 @@ void SocketGetJoinedGroupsFunction::Work() {
   SetResult(values);
 }
 
+SocketSecureFunction::SocketSecureFunction() {}
+SocketSecureFunction::~SocketSecureFunction() {}
+
+bool SocketSecureFunction::Prepare() {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+  VLOG(1) << "chrome.socket.secure: Prepare()";
+  params_ = api::socket::Secure::Params::Create(*args_);
+  EXTENSION_FUNCTION_VALIDATE(params_.get());
+  url_request_getter_ = GetProfile()->GetRequestContext();
+  underlying_socket_ = NULL;
+  return true;
+}
+
+void SocketSecureFunction::Work() {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
+  VLOG(1) << "chrome.socket.secure: Work()";
+  Socket* socket = GetSocket(params_->socket_id);
+  int result = -1;
+  if (!socket) {
+    error_ = kSocketNotFoundError;
+    SetResult(new base::FundamentalValue(result));
+    VLOG(1) << "chrome.socket.secure: Work(): no socket, bolting.";

[Ryan Sleevi, 2013/11/25 17:30:13]

These VLOGs are all included in release/official builds, and thus carry with
them non-trivial cost to build and image size.

It seems like they should all be DVLOGs or (ideally) removed. 

[Lally Singh, 2013/12/05 17:07:12]

All but 1 removed.  The remainder moved to DVLOG (to print the TLS connection's
result code)

+    return;
+  }
+
+  if (socket->GetSocketType() != Socket::TYPE_TCP) {
+    error_ = kSecureSocketTypeError;
+    SetResult(new base::FundamentalValue(result));
+    VLOG(1) << "chrome.socket.secure: Work(): not a TCP socket, bolting.";
+    return;
+  }
+
+  if (!socket->IsConnected()) {
+    error_ = kSocketNotConnectedError;
+    SetResult(new base::FundamentalValue(result));
+    VLOG(1) << "chrome.socket.secure: Work(): unconnected socket, bolting.";
+    return;
+  }
+
+  VLOG(1) << "SocketSecureFunction::Work: Starting up";
+
+  net::IPEndPoint dest_host_port_pair;
+  socket->GetPeerAddress(&dest_host_port_pair);
+  net::HostPortPair host_port(
+      socket->OriginalHostname(),
+      dest_host_port_pair.port());
+
+  // Modeled after content/browser/renderer_host/p2p/socket_host_tcp.cc:154
+  // P2PSocketHostTcpBase::StartTls()

[Ryan Sleevi, 2013/11/25 17:30:13]

Don't do comments like this. It will be out of date within days.

[Lally Singh, 2013/12/05 17:07:12]

Acknowledged.  The file/line# were removed, but the member/method kept.  If
that's still not acceptable, I'll remove the whole thing.

+  scoped_ptr<net::ClientSocketHandle> socket_handle(
+      new net::ClientSocketHandle());
+  // We already know from GetSocketType() above that it's TCP.

[Ryan Sleevi, 2013/11/25 17:30:13]

nit: Please rework the comments here to avoid the "we"

eg:

// |socket| is guaranteed to be a TCPSocket because of the GetSocketType() check
above.

[Lally Singh, 2013/12/05 17:07:12]

This comment was removed, but instances of "we" were removed.

+  TCPSocket *tcp_socket = static_cast<TCPSocket*>(socket);
+  DCHECK(tcp_socket->ClientStream());
+
+  // We keep a pointer (not a lifetime-preserving reference) to the original
+  // TCPClientSocket, to give to TLSSocket upon successful TLS negotiation.
+  // Its properly owned by ssl_socket_, which doesn't pass through certain
+  // APIs that we'd like it to (SetKeepAlive, SetNoDelay).

[Ryan Sleevi, 2013/11/25 17:30:13]

This is a bug. You shouldn't be doing this. Set your KeepAlive / NoDelay
settings before beginning negotiation.

Note that SSLClientSocket expects to be given an *already connected* socket
handle, so you should first do the TCP socket connect, get your TCP callback,
set your options, and *then* hand that off to the SSLSocket.

[Lally Singh, 2013/12/05 17:07:12]

I've disabled the KeepAlive/NoDelay methods.  The socket's already connected
when passed to 
SSLClientSocket.

+  underlying_socket_ = tcp_socket->ClientStream();
+  socket_handle->SetSocket(scoped_ptr<net::StreamSocket>(underlying_socket_));
+
+  // Have the old socket release its holds on the client stream we just gave
+  // away.
+  tcp_socket->Release();
+
+  net::SSLClientSocketContext context;
+  Profile* profile = GetProfile();
+  DCHECK(profile);
+  net::URLRequestContext* url_context =
+      url_request_getter_->GetURLRequestContext();
+
+  DCHECK(url_context);
+  context.cert_verifier = url_context->cert_verifier();
+  context.transport_security_state = url_context->transport_security_state();
+  DCHECK(context.transport_security_state);
+
+  // Fill in the SSL socket params.
+  net::SSLConfig ssl_config;
+  profile->GetSSLConfigService()->GetSSLConfig(&ssl_config);
+  if (params_->options.get() && params_->options->tls_version.get()) {
+    uint16 v_min = 0, v_max = 0;

[Ryan Sleevi, 2013/11/25 17:30:13]

As per the Google C++ style guide, eschew abbreviations. "version_min" and
"version_max" are much more reasonable.

[Lally Singh, 2013/12/05 17:07:12]

Done.

+    api::socket::TLSVersionConstraints *versions =

[Ryan Sleevi, 2013/11/25 17:30:13]

As per http://www.chromium.org/developers/coding-style#Code_Formatting , the *
goes with the type, not the variable name

api::socket::TLSVersionConstraints* versions = ...

[Lally Singh, 2013/12/05 17:07:12]

Done.

+        params_->options->tls_version.get();
+    if (versions->min.get()) {
+      v_min = SSLProtocolVersionFromString(*versions->min.get());
+    }
+    if (versions->max.get()) {
+      v_max = SSLProtocolVersionFromString(*versions->max.get());
+    }
+    if (v_min) {
+      ssl_config.version_min = v_min;
+    }
+    if (v_max) {
+      ssl_config.version_max = v_max;
+    }
+  }
+  net::ClientSocketFactory* socket_factory =
+      net::ClientSocketFactory::GetDefaultFactory();

[Ryan Sleevi, 2013/11/25 17:30:13]

The "ideal" form is to pass this in as a variable somehow, rather than to grab
it from the factory. Just $.02

+
+  VLOG(1) << "SocketSecureFunction::Work: Creating TLS socket";
+
+  // Create the socket.
+  ssl_socket_ = socket_factory->CreateSSLClientSocket(
+      socket_handle.Pass(), host_port, ssl_config, context);

[Ryan Sleevi, 2013/11/25 17:30:13]

BUG: This is broken, because you never connected |socket_handle|.
CreateSSLClientSocket expects a *connected* socket.

[Lally Singh, 2013/12/05 17:07:12]

I set it to underlying_socket above.  That socket was checked to make sure it
was connected (and TCP).  I've since cleaned it up a bit.

+
+  VLOG(1) << "SocketSecureFunction::Work: Trying to connect";
+  // And try to connect.
+  int status = ssl_socket_->Connect(
+      base::Bind(&SocketSecureFunction::TlsConnectDone, this));
+  if (status != net::ERR_IO_PENDING) {
+    TlsConnectDone(status);

[Ryan Sleevi, 2013/11/25 17:30:13]

From an API point, this sort of async-or-recursion tends to cause subtle bugs
regarding lifetimes.

It's typically much better to re-organize this as all of net/ does, into a state
machine-driven loop, rather than to recurse into the function here.

[Lally Singh, 2013/12/05 17:07:12]

I've added a comment on why this can't recurse into another secure() call on
this socket.  Is that the concern you had?

+    VLOG(1) << "SocketSecureFunction::Work: Done. ";
+  } else {
+    VLOG(1) << "SocketSecureFunction::Work: Returning, "
+            << "but expecting a call to TlsConnectDone. ";
+  }
+}
+
+// Override the regular implementation, which would call AsyncWorkCompleted
+// immediately after Work().
+void SocketSecureFunction::AsyncWorkStart() {
+  Work();
+}
+
+void SocketSecureFunction::TlsConnectDone(int result) {
+  SetResult(new base::FundamentalValue(result));
+  VLOG(1) << "chrome.socket.secure: TlsConnectDone(): got back result "
+          << result;
+
+  // No matter how the TLS connection attempt went, the underlying socket's
+  // no longer bound to the original TCPSocket.  It belongs to ssl_socket_,
+  // which we either promote to a new API-accessible socket (via a TLSSocket
+  // wrapper) or delete.
+  if (result == net::OK) {
+    // Wrap the StreamSocket in a TLSSocket (which matches our API).  Set the
+    // handle of the socket to the new value, so that we can use the newly
+    // connected socket for close/SetKeepAlive/etc.
+    TLSSocket* wrapper = new TLSSocket(
+        ssl_socket_.release(), underlying_socket_, extension_->id());
+
+    // This deletes the old TCPSocket for us.
+    SetSocket(params_->socket_id, wrapper);
+  } else {
+    // Failed TLS connection.  This'll delete the underlying TCPClientSocket.
+    ssl_socket_.reset();
+    // This will delete the TCPSocket, which has already released its hold on
+    // the TCPClientSocket.
+    RemoveSocket(params_->socket_id);
+  }
+  AsyncWorkCompleted();
+}
+
 }  // namespace extensions