Select the first protocol from the next protocol list of SSLConfig if If we didn't find a protocol.

net/socket/ssl_client_socket_openssl.cc

Index: net/socket/ssl_client_socket_openssl.cc
===================================================================
--- net/socket/ssl_client_socket_openssl.cc	(revision 141188)
+++ net/socket/ssl_client_socket_openssl.cc	(working copy)
@@ -852,17 +852,17 @@
                                                     const unsigned char* in,
                                                     unsigned int inlen) {
 #if defined(OPENSSL_NPN_NEGOTIATED)
+  // It's expected that a client will have a list of protocols that it

[agl, 2012/06/08 14:23:14]

I don't think that this comment is very clear. Maybe:

// If we don't have any protocols configured, assume "http/1.1".

[Johnny(Jianning) Ding, 2012/06/11 13:27:19]

That comment is copied from
https://technotes.googlecode.com/git/nextprotoneg.html. It doesn't mention the
case that client doesn't provide the supported protocol list and http/1.1 is the
default option for that case. As the question Ryan raised, what's right behavior
in case of empty client protocol list?

On 2012/06/08 14:23:14, agl wrote:

[agl, 2012/06/11 16:02:02]

The first half is, but it doesn't make sense in this context.
If the client doesn't have a list of protocols then it shouldn't have sent the
extension. But, since it will with this code, this is a reasonable behaviour.

[wtc, 2012/06/12 17:50:30]

jnd: I suggest simply removing this comment.

You can also say something like:
  // If a client doesn't have a list of protocols that it supports,
  // it will still send the NPN extension.  If the server supports
  // NPN, choosing "http/1.1" is the best answer.

[Johnny(Jianning) Ding, 2012/06/13 09:31:20]

Thanks for all your comments. Seems we should keep using current way. Please
reexamine patchset#2.

On 2012/06/12 17:50:30, wtc wrote:

+  // supports. If not, which means NPN is not supported.
   if (ssl_config_.next_protos.empty()) {
     *out = reinterpret_cast<uint8*>(const_cast<char*>("http/1.1"));
     *outlen = 8;

[Ryan Sleevi, 2012/06/08 18:16:29]

nit: I think I preferred Joth's suggestion (use a static const char[] +
arraysize()). At least then I don't have to count the characters to make sure
I'm right :)

[Johnny(Jianning) Ding, 2012/06/11 13:27:19]

Will change in next upload.

-    npn_status_ = SSLClientSocket::kNextProtoUnsupported;
+    npn_status_ = kNextProtoUnsupported;

[agl, 2012/06/08 14:23:14]

This should be NoOverlap, not Unsupported. If we hit this callback then the
server does support NPN, we just don't have any protocols in common.

[Ryan Sleevi, 2012/06/08 18:16:29]

agl: This does change the behaviour then between the _openssl and _nss
implementations. The _nss implementation only negotiates NPN if
!next_protos.empty(), and by default initializes npn_status to
kNextProtoUnsupported.

If we do want to change this to NoOverlap, then I think the _nss implementation
should also be changed to always negotiate NPN.

It's not clear to me from reading the NPN draft which behaviour is correct
(always negotiating NPN, even for single/implied protocol, or conditionally
negotiating NPN only when multi-protocol)

     return SSL_TLSEXT_ERR_OK;
   }
 
   // Assume there's no overlap between our protocols and the server's list.
-  int status = OPENSSL_NPN_NO_OVERLAP;
-  *out = const_cast<unsigned char*>(in) + 1;
-  *outlen = in[0];
+  npn_status_ = kNextProtoNoOverlap;
 
   // For each protocol in server preference order, see if we support it.
   for (unsigned int i = 0; i < inlen; i += in[i] + 1) {
@@ -874,27 +874,23 @@
         // We find a match.
         *out = const_cast<unsigned char*>(in) + i + 1;
         *outlen = in[i];
-        status = OPENSSL_NPN_NEGOTIATED;
+        npn_status_ = kNextProtoNegotiated;

[Ryan Sleevi, 2012/06/08 18:16:29]

nit: find -> found, since this comment appears after/inside the condition (as
opposed to say, 885/886, which happen outside/before)

[Johnny(Jianning) Ding, 2012/06/11 13:27:19]

will change in next upload.

On 2012/06/08 18:16:29, Ryan Sleevi wrote:

         break;
       }
     }
-    if (status == OPENSSL_NPN_NEGOTIATED)
+    if (npn_status_ == kNextProtoNegotiated)
       break;
   }
 
+  // If we didn't find a protocol, we select the first one from our list.
+  if (npn_status_ == kNextProtoNoOverlap) {
+    *out = reinterpret_cast<uint8*>(const_cast<char*>(
+        ssl_config_.next_protos[0].data()));
+    *outlen = ssl_config_.next_protos[0].size();
+  }
+
   npn_proto_.assign(reinterpret_cast<const char*>(*out), *outlen);
   server_protos_.assign(reinterpret_cast<const char*>(in), inlen);
-  switch (status) {
-    case OPENSSL_NPN_NEGOTIATED:
-      npn_status_ = SSLClientSocket::kNextProtoNegotiated;
-      break;
-    case OPENSSL_NPN_NO_OVERLAP:
-      npn_status_ = SSLClientSocket::kNextProtoNoOverlap;
-      break;
-    default:
-      NOTREACHED() << status;
-      break;
-  }
   DVLOG(2) << "next protocol: '" << npn_proto_ << "' status: " << npn_status_;
 #endif
   return SSL_TLSEXT_ERR_OK;