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

 // 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/ssl.h>
@@ skipping 320 matching lines @@
     SSL_CTX_sess_set_new_cb(ssl_ctx_.get(), NewSessionCallbackStatic);
     SSL_CTX_sess_set_remove_cb(ssl_ctx_.get(), RemoveSessionCallbackStatic);
     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);
 #if defined(OPENSSL_NPN_NEGOTIATED)
     // 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);

[wtc, 2012/06/08 21:33:35]

Hmm... this must be the issue that rsleevi pointed out.
The OpenSSL code is:

#ifndef OPENSSL_NO_NEXTPROTONEG
        if (s->ctx->next_proto_select_cb && !s->s3->tmp.finish_md_len)
                {
                /* The client advertises an emtpy extension to indicate its
                 * support for Next Protocol Negotiation */
                if (limit - ret - 4 < 0)
                        return NULL;
                s2n(TLSEXT_TYPE_next_proto_neg,ret);
                s2n(0,ret);
                }
#endif

So if s->ctx->next_proto_select_cb is not null, the
client will send an NPN extension.

This means SSLClientSocketOpenSSL has to be prepared to
do something reasonable when its ssl_config_.next_protos
is empty, and explains why the equivalent code doesn't
exist in SSLClientSocketNSS.

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

@kristianm, according to your comment, seems there is a openssl issue which
stops us to install select callback when ssl_config_.next_proto is not empty.
Would you please give more details?

On 2012/06/08 21:33:35, wtc wrote:

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

Well, I simply didn't add the option to switch the callback on and off on a
per-SSL* basis. You could switch SSL_CTXs, but that seems like a bigger hack.

I think my original suggestion was probably wrong: the code should return
Unsupported when the client's protocol list was empty in order to match NSS.
It's technical wrong because the server clearly *does* support NPN in that case,
but it's the best answer.

 #endif
   }
 
   static int NewSessionCallbackStatic(SSL* ssl, SSL_SESSION* session) {
     return GetInstance()->NewSessionCallback(ssl, session);
   }
 
   int NewSessionCallback(SSL* ssl, SSL_SESSION* session) {
     SSLClientSocketOpenSSL* socket = GetClientSocketFromSSL(ssl);
     session_cache_.OnSessionAdded(socket->host_and_port(),
@@ skipping 493 matching lines @@
 
 // SelectNextProtoCallback is called by OpenSSL during the handshake. If the
 // server supports NPN, selects a protocol from the list that the server
 // provides. According to third_party/openssl/openssl/ssl/ssl_lib.c, the
 // callback can assume that |in| is syntactically valid.
 int SSLClientSocketOpenSSL::SelectNextProtoCallback(unsigned char** out,
                                                     unsigned char* outlen,
                                                     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) {
     for (std::vector<std::string>::const_iterator
              j = ssl_config_.next_protos.begin();
          j != ssl_config_.next_protos.end(); ++j) {
       if (in[i] == j->size() &&
           memcmp(&in[i + 1], j->data(), in[i]) == 0) {
         // 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;
 }
 
 int SSLClientSocketOpenSSL::DoVerifyCert(int result) {
   DCHECK(server_cert_);
   GotoState(STATE_VERIFY_CERT_COMPLETE);
 
   CertStatus cert_status;
@@ skipping 407 matching lines @@
     net_log_.AddByteTransferEvent(NetLog::TYPE_SSL_SOCKET_BYTES_SENT, rv,
                                   user_write_buf_->data());
     return rv;
   }
 
   int err = SSL_get_error(ssl_, rv);
   return MapOpenSSLError(err, err_tracer);
 }
 
 }  // namespace net