Add support for OpenSSL Next Protocol Negotiation

net/socket/ssl_client_socket_openssl.cc

Index: net/socket/ssl_client_socket_openssl.cc
===================================================================
--- net/socket/ssl_client_socket_openssl.cc	(revision 67990)
+++ net/socket/ssl_client_socket_openssl.cc	(working copy)
@@ -19,6 +19,9 @@
 #include "net/base/ssl_cert_request_info.h"
 #include "net/base/ssl_connection_status_flags.h"
 #include "net/base/ssl_info.h"
+// TODO(willchan): Fix this dependency.  It's ugly for net/socket to depend on
+// net/http.

[joth, 2010/12/02 17:46:55]

overlength line

[Kristian_, 2010/12/03 14:58:56]

Done.

+#include "net/http/http_stream_factory.h"

[joth, 2010/12/02 17:46:55]

Istead of doing this please see if we can use SSLConfig::next_protos instead.

[willchan no longer on Chromium, 2010/12/03 01:35:06]

Agreed, this is preferable, primarily in terms of layering, since socket
shouldn't be depending on http.

[Kristian_, 2010/12/03 14:58:56]

Done

 
 namespace net {
 
@@ -175,6 +178,10 @@
     SSL_CTX_set_timeout(ssl_ctx_.get(), kSessionCacheTimeoutSeconds);
     SSL_CTX_sess_set_cache_size(ssl_ctx_.get(), kSessionCacheMaxEntires);
     SSL_CTX_set_client_cert_cb(ssl_ctx_.get(), ClientCertCallback);
+#ifdef OPENSSL_NPN_NEGOTIATED

[willchan no longer on Chromium, 2010/12/03 01:35:06]

In Chromium code, we usually do #if defined(OPENSSL_NPN_NEGOTIATED).

[Kristian_, 2010/12/03 14:58:56]

Done.

+    SSL_CTX_set_next_proto_select_cb(ssl_ctx_.get(), &SelectNextProtoCallback,

[joth, 2010/12/02 17:46:55]

nit: other calls don't use & to get function address

[joth, 2010/12/02 17:46:55]

Problem: a side effect of calling SSL_CTX_set_next_proto_select_cb is to put
openssl into NPN mode (hence make the request and handle the response).
We only want to do this per SSL instance (i.e. per connection), once we have
determined that SSLConfig::next_protos is not empty.

But AFAICT we have to set this for the whole CTX.

We can't easily create 2 CTXs (one with and one with NPN) without also making 2
cache instances. (Sessions may not be shared across CTX)

What am I missing?

[willchan no longer on Chromium, 2010/12/03 01:35:06]

NPN next protos are decided on Chrome startup and never change.

In current Chrome, we always have NPN set.  So not conditioning on
SSLConfig::next_protos right now is OK, but a better way to do it would be to
read from the first SSLConfig::next_protos instance.  I'm fine with having that
be a TODO since it's basically never empty.

[Kristian_, 2010/12/03 14:58:56]

I'm not sure if setting on first run gives us much. We can still get into an
inconsistent state. As long as it is an on/off switch (that can't be turned
off), I think we should just enabled it at start. If this creates problems we
have to turn the feature off anyway.

+        NULL);

[joth, 2010/12/02 17:46:55]

align NULL with first param

[Kristian_, 2010/12/03 14:58:56]

Done.

+#endif
   }
 
   static int NewSessionCallbackStatic(SSL* ssl, SSL_SESSION* session) {
@@ -202,6 +209,42 @@
     return socket->ClientCertRequestCallback(ssl, x509, pkey);
   }
 
+#ifdef OPENSSL_NPN_NEGOTIATED
+  static int SelectNextProtoCallback(SSL* ssl,
+                                     unsigned char** out, unsigned char* outlen,
+                                     const unsigned char* in,
+                                     unsigned int inlen, void* arg) {
+    if (!HttpStreamFactory::next_protos() ||
+        HttpStreamFactory::next_protos()->empty())
+      return SSL_TLSEXT_ERR_OK;

[joth, 2010/12/02 17:46:55]

The docs for SSL_CTX_set_next_proto_select_cb state "The client must select a
protocol."
At the very least we should probably set *outlen = 0 here (and maybe *out =
NULL), as it looks like the openssl library goes ahead and calls malloc(*outlen)
if we return OK.

Although: as per my comments above, shouldn't we using SSLConfig::next_protos
instead? And arrange to only get this callback once we know
SSLConfig::next_protos is not empty.

[willchan no longer on Chromium, 2010/12/03 01:35:06]

You're right.  That'd be better, but if it's too much work, I'm fine with a
TODO.  I defer to agl here.

[Kristian_, 2010/12/03 14:58:56]

The problem is that the contents of SSLConfig::next_proto can be changing, but
the callback can only be set once. What if we set the string on creation to
contain the fallback case? As far as I can see that should never be a problem.

+
+    const std::string& next_protos = *HttpStreamFactory::next_protos();

[joth, 2010/12/02 17:46:55]

maybe add a comment:
CARE: |*out| maybe set to point to a substring within the |next_protos| content
by SSL_select_next_proto, and then accessed after this function returns, hence
it must not be scoped within this function.

+    SSLClientSocketOpenSSL* socket = Get()->GetClientSocketFromSSL(ssl);
+
+    int status = SSL_select_next_proto(
+        out, outlen, in, inlen,
+        reinterpret_cast<const unsigned char*>(next_protos.data()),
+        next_protos.size());
+
+    socket->set_npn_proto(reinterpret_cast<const char*>(*out), *outlen);
+    switch (status) {
+      case OPENSSL_NPN_UNSUPPORTED:
+        socket->set_npn_status(SSLClientSocket::kNextProtoUnsupported);
+        break;
+      case OPENSSL_NPN_NEGOTIATED:
+        socket->set_npn_status(SSLClientSocket::kNextProtoNegotiated);
+        break;
+      case OPENSSL_NPN_NO_OVERLAP:
+        socket->set_npn_status(SSLClientSocket::kNextProtoNoOverlap);
+        break;
+      default:
+        NOTREACHED();

[joth, 2010/12/02 17:46:55]

nit: NOTREACHED() << status;

[Kristian_, 2010/12/03 14:58:56]

Done.

+        break;
+    }
+    return SSL_TLSEXT_ERR_OK;
+  }
+#endif
+
   // This is the index used with SSL_get_ex_data to retrieve the owner
   // SSLClientSocketOpenSSL object from an SSL instance.
   int ssl_socket_data_index_;
@@ -248,6 +291,7 @@
       host_and_port_(host_and_port),
       ssl_config_(ssl_config),
       trying_cached_session_(false),
+      npn_status_(kNextProtoUnsupported),
       net_log_(transport_socket->socket()->NetLog()) {
 }
 
@@ -370,8 +414,8 @@
 
 SSLClientSocket::NextProtoStatus SSLClientSocketOpenSSL::GetNextProto(
     std::string* proto) {
-  proto->clear();
-  return kNextProtoUnsupported;
+  proto->assign(npn_proto_);

[joth, 2010/12/02 17:46:55]

nit: *proto = npn_proto_;
(probably just slightly more canonical for string copy)

[Kristian_, 2010/12/03 14:58:56]

Done.

+  return npn_status_;
 }
 
 void SSLClientSocketOpenSSL::DoReadCallback(int rv) {