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

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

It's possible that there is no overlap between the server advertised protocols and SSL client advertised protocols. And Server even can give a empty protocol list in NPN extension in a ServerHello message.
In this case, the SSL client should pick up the first protocol from the next protocol list of SSLConfig.

BUG=131769
TEST=None
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=142098

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

LGTM

http://codereview.chromium.org/10532061/diff/1/net/socket/ssl_client_socket_o...
File net/socket/ssl_client_socket_openssl.cc (right):

http://codereview.chromium.org/10532061/diff/1/net/socket/ssl_client_socket_o...
net/socket/ssl_client_socket_openssl.cc:855: // It's expected that a client will
have a list of protocols that it
I don't think that this comment is very clear. Maybe:

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

http://codereview.chromium.org/10532061/diff/1/net/socket/ssl_client_socket_o...
net/socket/ssl_client_socket_openssl.cc:860: npn_status_ =
kNextProtoUnsupported;
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]

lgtm

http://codereview.chromium.org/10532061/diff/1/net/socket/ssl_client_socket_o...
File net/socket/ssl_client_socket_openssl.cc (right):

http://codereview.chromium.org/10532061/diff/1/net/socket/ssl_client_socket_o...
net/socket/ssl_client_socket_openssl.cc:859: *outlen = 8;
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 :)

http://codereview.chromium.org/10532061/diff/1/net/socket/ssl_client_socket_o...
net/socket/ssl_client_socket_openssl.cc:860: npn_status_ =
kNextProtoUnsupported;
On 2012/06/08 14:23:14, agl wrote:
> 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.

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)

http://codereview.chromium.org/10532061/diff/1/net/socket/ssl_client_socket_o...
net/socket/ssl_client_socket_openssl.cc:877: npn_status_ = kNextProtoNegotiated;
nit: find -> found, since this comment appears after/inside the condition (as
opposed to say, 885/886, which happen outside/before)

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

Patch set 1 LGTM.  I don't know how to solve the problem
that rsleevi pointed out.  It seems to require changing
OpenSSL.

http://codereview.chromium.org/10532061/diff/1/net/socket/ssl_client_socket_o...
File net/socket/ssl_client_socket_openssl.cc (right):

http://codereview.chromium.org/10532061/diff/1/net/socket/ssl_client_socket_o...
net/socket/ssl_client_socket_openssl.cc:341: NULL);
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]

Waiting for comments from kristianm and agl for next step.

http://codereview.chromium.org/10532061/diff/1/net/socket/ssl_client_socket_o...
File net/socket/ssl_client_socket_openssl.cc (right):

http://codereview.chromium.org/10532061/diff/1/net/socket/ssl_client_socket_o...
net/socket/ssl_client_socket_openssl.cc:341: NULL);
@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:
> 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.

http://codereview.chromium.org/10532061/diff/1/net/socket/ssl_client_socket_o...
net/socket/ssl_client_socket_openssl.cc:855: // It's expected that a client will
have a list of protocols that it
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:
> I don't think that this comment is very clear. Maybe:
> 
> // If we don't have any protocols configured, assume "http/1.1".

http://codereview.chromium.org/10532061/diff/1/net/socket/ssl_client_socket_o...
net/socket/ssl_client_socket_openssl.cc:859: *outlen = 8;
On 2012/06/08 18:16:29, Ryan Sleevi wrote:
> 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 :)

Will change in next upload.

http://codereview.chromium.org/10532061/diff/1/net/socket/ssl_client_socket_o...
net/socket/ssl_client_socket_openssl.cc:877: npn_status_ = kNextProtoNegotiated;
will change in next upload.

On 2012/06/08 18:16:29, Ryan Sleevi wrote:
> nit: find -> found, since this comment appears after/inside the condition (as
> opposed to say, 885/886, which happen outside/before)

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

http://codereview.chromium.org/10532061/diff/1/net/socket/ssl_client_socket_o...
File net/socket/ssl_client_socket_openssl.cc (right):

http://codereview.chromium.org/10532061/diff/1/net/socket/ssl_client_socket_o...
net/socket/ssl_client_socket_openssl.cc:341: NULL);
On 2012/06/11 13:27:19, Johnny(Jianning) Ding wrote:
> @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?

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.

http://codereview.chromium.org/10532061/diff/1/net/socket/ssl_client_socket_o...
net/socket/ssl_client_socket_openssl.cc:855: // It's expected that a client will
have a list of protocols that it
On 2012/06/11 13:27:19, Johnny(Jianning) Ding wrote:
> That comment is copied from
> https://technotes.googlecode.com/git/nextprotoneg.html.

The first half is, but it doesn't make sense in this context.

> 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.

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.

[joth, 2012-06-11 18:08:30]

On 11 June 2012 09:02, <agl@chromium.org> wrote:

>
> http://codereview.chromium.**org/10532061/diff/1/net/**
>
socket/ssl_client_socket_**openssl.cc<http://codereview.chromium.org/10532061/diff/1/net/socket/ssl_client_socket_openssl.cc>
> File net/socket/ssl_client_socket_**openssl.cc (right):
>
> http://codereview.chromium.**org/10532061/diff/1/net/**
>
socket/ssl_client_socket_**openssl.cc#newcode341<http://codereview.chromium.org/10532061/diff/1/net/socket/ssl_client_socket_openssl.cc#newcode341>
> net/socket/ssl_client_socket_**openssl.cc:341: NULL);
> On 2012/06/11 13:27:19, Johnny(Jianning) Ding wrote:
>
>> @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?
>>
>
> Well, I simply didn't add the option to switch the callback on and off
> on a per-SSL* basis.


FWIW, here's the patch where Krtstian and I set out to add per-SSL
selection for the extension
http://codereview.chromium.org/5692003/

jnd: that ^^ review thread probably answers your question for kristianm too.



> 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.
>
>
> http://codereview.chromium.**org/10532061/diff/1/net/**
>
socket/ssl_client_socket_**openssl.cc#newcode855<http://codereview.chromium.org/10532061/diff/1/net/socket/ssl_client_socket_openssl.cc#newcode855>
> net/socket/ssl_client_socket_**openssl.cc:855: // It's expected that a
> client will have a list of protocols that it
> On 2012/06/11 13:27:19, Johnny(Jianning) Ding wrote:
>
>> That comment is copied from
>>
https://technotes.googlecode.**com/git/nextprotoneg.html<https://technotes.go...
>> .
>>
>
> The first half is, but it doesn't make sense in this context.
>
>
>  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.
>>
>
> 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.
>
>
http://codereview.chromium.**org/10532061/<http://codereview.chromium.org/105...
>

[wtc, 2012-06-12 17:50:29]

http://codereview.chromium.org/10532061/diff/1/net/socket/ssl_client_socket_o...
File net/socket/ssl_client_socket_openssl.cc (right):

http://codereview.chromium.org/10532061/diff/1/net/socket/ssl_client_socket_o...
net/socket/ssl_client_socket_openssl.cc:855: // It's expected that a client will
have a list of protocols that it

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]

http://codereview.chromium.org/10532061/diff/1/net/socket/ssl_client_socket_o...
File net/socket/ssl_client_socket_openssl.cc (right):

http://codereview.chromium.org/10532061/diff/1/net/socket/ssl_client_socket_o...
net/socket/ssl_client_socket_openssl.cc:855: // It's expected that a client will
have a list of protocols that it

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:
> 
> 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.

[agl, 2012-06-13 14:37:47]

LGTM

[wtc, 2012-06-13 18:01:52]

Patch set 2 LGTM.

agl: I have a question for you.

jnd: I suggest a small comment change.

http://codereview.chromium.org/10532061/diff/7001/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_openssl.cc (right):

http://codereview.chromium.org/10532061/diff/7001/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_openssl.cc:861:
const_cast<char*>(kDefaultSupportedNPNProtocol));

agl: it seems that these const_casts when assigning values
to |*out| could be avoided if the |out| parameter were
declared as
    const unsigned char** out,

Although OpenSSL passes |&selected| to the callback, where
|selected| is declared as:
    unsigned char *selected;
OpenSSL uses |selected| as a const pointer.

But I guess it is too late to change the prototype of
next_proto_select_cb now.

http://codereview.chromium.org/10532061/diff/7001/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_openssl.cc:875: // We found a match.

Nit: it's better to move this comment into the if statement.
(That's the original location of this comment.)

[Johnny(Jianning) Ding, 2012-06-14 04:50:13]

http://codereview.chromium.org/10532061/diff/7001/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_openssl.cc (right):

http://codereview.chromium.org/10532061/diff/7001/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_openssl.cc:875: // We found a match.
On 2012/06/13 18:01:52, wtc wrote:
> 
> Nit: it's better to move this comment into the if statement.
> (That's the original location of this comment.)

Done. It was to address Ryan's comment, it said "this comment appears
after/inside the condition (as
opposed to say, 885/886, which happen outside/before)"