Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
5528003:
Add support for OpenSSL Next Protocol Negotiation (Closed)
|
Created:
10 years ago by Kristian_ Modified:
9 years, 7 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, darin-cc_chromium.org Base URL:
http://src.chromium.org/svn/trunk/src/ Visibility:
Public. |
DescriptionAdd support for OpenSSL Next Protocol Negotiation
Bug=none
Test=none
Patch Set 1 #Patch Set 2 : '' #Patch Set 3 : '' #
Total comments: 23
Patch Set 4 : '' #
Total comments: 4
Patch Set 5 : '' #Patch Set 6 : '' #Messages
Total messages: 12 (0 generated)
Thanks for doing this Kristian. Got a couple nits, and question about the usage of SSLConfig::next_protos and how we can dynamically enable NPN mode. http://codereview.chromium.org/5528003/diff/2002/net/socket/ssl_client_socket... File net/socket/ssl_client_socket_openssl.cc (right): http://codereview.chromium.org/5528003/diff/2002/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:23: // net/http. overlength line http://codereview.chromium.org/5528003/diff/2002/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:24: #include "net/http/http_stream_factory.h" Istead of doing this please see if we can use SSLConfig::next_protos instead. http://codereview.chromium.org/5528003/diff/2002/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:182: SSL_CTX_set_next_proto_select_cb(ssl_ctx_.get(), &SelectNextProtoCallback, nit: other calls don't use & to get function address http://codereview.chromium.org/5528003/diff/2002/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:182: SSL_CTX_set_next_proto_select_cb(ssl_ctx_.get(), &SelectNextProtoCallback, 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? http://codereview.chromium.org/5528003/diff/2002/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:183: NULL); align NULL with first param http://codereview.chromium.org/5528003/diff/2002/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:219: return SSL_TLSEXT_ERR_OK; 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. http://codereview.chromium.org/5528003/diff/2002/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:221: const std::string& next_protos = *HttpStreamFactory::next_protos(); 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. http://codereview.chromium.org/5528003/diff/2002/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:241: NOTREACHED(); nit: NOTREACHED() << status; http://codereview.chromium.org/5528003/diff/2002/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:417: proto->assign(npn_proto_); nit: *proto = npn_proto_; (probably just slightly more canonical for string copy) http://codereview.chromium.org/5528003/diff/2002/net/socket/ssl_client_socket... File net/socket/ssl_client_socket_openssl.h (right): http://codereview.chromium.org/5528003/diff/2002/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.h:72: void set_npn_status(NextProtoStatus status) { npn_status_ = status; } could you put these up next to host_and_port(), as the section down here is for virtual overrides.
Should definitely make sure to get agl's lgtm, since he's much more familiar with OpenSSL than I am. http://codereview.chromium.org/5528003/diff/2002/net/socket/ssl_client_socket... File net/socket/ssl_client_socket_openssl.cc (right): http://codereview.chromium.org/5528003/diff/2002/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:24: #include "net/http/http_stream_factory.h" On 2010/12/02 17:46:55, joth wrote: > Istead of doing this please see if we can use SSLConfig::next_protos instead. Agreed, this is preferable, primarily in terms of layering, since socket shouldn't be depending on http. http://codereview.chromium.org/5528003/diff/2002/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:181: #ifdef OPENSSL_NPN_NEGOTIATED In Chromium code, we usually do #if defined(OPENSSL_NPN_NEGOTIATED). http://codereview.chromium.org/5528003/diff/2002/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:182: SSL_CTX_set_next_proto_select_cb(ssl_ctx_.get(), &SelectNextProtoCallback, On 2010/12/02 17:46:55, joth wrote: > 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? 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. http://codereview.chromium.org/5528003/diff/2002/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:219: return SSL_TLSEXT_ERR_OK; On 2010/12/02 17:46:55, joth wrote: > 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. 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.
OK, so I think the simplest thing for now is we don't register the callback
until the first non-empty SSLConfig::next_protos is received, and if we ever
do subsequently find find ourselves in the callback with an
empty SSLConfig::next_protos just fallback to a hard-coded 'HTTP' value.
WIth a TODO.
agl - is there a reason this feature isn't enabled per-connection (that is,
per SSL)?
I'm thinking something like making a dedicated member in struct ssl_st
(SSL), that controls if the client will send the NPN request:-
#ifndef OPENSSL_NO_NEXTPROTONEG
/* Next protocol negotiation. For the client, this is the protocol
that
* we sent in NextProtocol and is set when handling ServerHello
* extensions.
*
* For a server, this is the client's selected_protocol from
* NextProtocol and is set when handling the NextProtocol message,
* before the Finished message. */
unsigned char *next_proto_negotiated;
unsigned char next_proto_negotiated_len;
* unsigned char next_proto_enabled;*
...
#endif
ssl_lib.c:
SSL_new()...
* // NPN defaults to enabled if a callback was registered when the SSL
was created*
* s->next_proto_negotiated = ctx->next_proto_select_cb ? 1 : 0;*
*
*
*
*
*void SSL_set_next_proto_enabled(SSL* s, char enabled)*
* {*
* s->next_proto_enabled = enabled;*
* }*
t1_lib.c:
#ifndef OPENSSL_NO_NEXTPROTONEG
if (*s->next_proto_enabled && *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
On 3 December 2010 01:35, <willchan@chromium.org> wrote:
> Should definitely make sure to get agl's lgtm, since he's much more
> familiar
> with OpenSSL than I am.
>
>
>
>
>
http://codereview.chromium.org/5528003/diff/2002/net/socket/ssl_client_socket...
> File net/socket/ssl_client_socket_openssl.cc (right):
>
>
>
http://codereview.chromium.org/5528003/diff/2002/net/socket/ssl_client_socket...
> net/socket/ssl_client_socket_openssl.cc:24: #include
> "net/http/http_stream_factory.h"
> On 2010/12/02 17:46:55, joth wrote:
>
>> Istead of doing this please see if we can use SSLConfig::next_protos
>>
> instead.
>
> Agreed, this is preferable, primarily in terms of layering, since socket
> shouldn't be depending on http.
>
>
>
http://codereview.chromium.org/5528003/diff/2002/net/socket/ssl_client_socket...
> net/socket/ssl_client_socket_openssl.cc:181: #ifdef
> OPENSSL_NPN_NEGOTIATED
> In Chromium code, we usually do #if defined(OPENSSL_NPN_NEGOTIATED).
>
>
>
>
http://codereview.chromium.org/5528003/diff/2002/net/socket/ssl_client_socket...
> net/socket/ssl_client_socket_openssl.cc:182:
> SSL_CTX_set_next_proto_select_cb(ssl_ctx_.get(),
> &SelectNextProtoCallback,
> On 2010/12/02 17:46:55, joth wrote:
>
>> 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?
>>
>
> 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.
>
>
>
>
http://codereview.chromium.org/5528003/diff/2002/net/socket/ssl_client_socket...
> net/socket/ssl_client_socket_openssl.cc:219: return SSL_TLSEXT_ERR_OK;
> On 2010/12/02 17:46:55, joth wrote:
>
>> 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.
>>
>
> 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.
>
>
> http://codereview.chromium.org/5528003/
>
http://codereview.chromium.org/5528003/diff/2002/net/socket/ssl_client_socket... File net/socket/ssl_client_socket_openssl.cc (right): http://codereview.chromium.org/5528003/diff/2002/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:23: // net/http. On 2010/12/02 17:46:55, joth wrote: > overlength line Done. http://codereview.chromium.org/5528003/diff/2002/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:24: #include "net/http/http_stream_factory.h" On 2010/12/02 17:46:55, joth wrote: > Istead of doing this please see if we can use SSLConfig::next_protos instead. Done http://codereview.chromium.org/5528003/diff/2002/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:181: #ifdef OPENSSL_NPN_NEGOTIATED On 2010/12/03 01:35:06, willchan wrote: > In Chromium code, we usually do #if defined(OPENSSL_NPN_NEGOTIATED). Done. http://codereview.chromium.org/5528003/diff/2002/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:182: SSL_CTX_set_next_proto_select_cb(ssl_ctx_.get(), &SelectNextProtoCallback, On 2010/12/03 01:35:06, willchan wrote: > On 2010/12/02 17:46:55, joth wrote: > > 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? > > 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. 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. http://codereview.chromium.org/5528003/diff/2002/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:183: NULL); On 2010/12/02 17:46:55, joth wrote: > align NULL with first param Done. http://codereview.chromium.org/5528003/diff/2002/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:219: return SSL_TLSEXT_ERR_OK; On 2010/12/02 17:46:55, joth wrote: > 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. 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. http://codereview.chromium.org/5528003/diff/2002/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:241: NOTREACHED(); On 2010/12/02 17:46:55, joth wrote: > nit: NOTREACHED() << status; Done. http://codereview.chromium.org/5528003/diff/2002/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.cc:417: proto->assign(npn_proto_); On 2010/12/02 17:46:55, joth wrote: > nit: *proto = npn_proto_; > (probably just slightly more canonical for string copy) Done. http://codereview.chromium.org/5528003/diff/2002/net/socket/ssl_client_socket... File net/socket/ssl_client_socket_openssl.h (right): http://codereview.chromium.org/5528003/diff/2002/net/socket/ssl_client_socket... net/socket/ssl_client_socket_openssl.h:72: void set_npn_status(NextProtoStatus status) { npn_status_ = status; } On 2010/12/02 17:46:55, joth wrote: > could you put these up next to host_and_port(), as the section down here is for > virtual overrides. Done, actually removed.
http://codereview.chromium.org/5528003/diff/16001/net/socket/ssl_client_socke... File net/socket/ssl_client_socket_openssl.cc (right): http://codereview.chromium.org/5528003/diff/16001/net/socket/ssl_client_socke... net/socket/ssl_client_socket_openssl.cc:592: *outlen = 0; *out = "http/1.1"; *outlen = 8; http://codereview.chromium.org/5528003/diff/16001/net/socket/ssl_client_socke... net/socket/ssl_client_socket_openssl.cc:603: //case OPENSSL_NPN_UNSUPPORTED: Why commented out?
http://codereview.chromium.org/5528003/diff/16001/net/socket/ssl_client_socke... File net/socket/ssl_client_socket_openssl.cc (right): http://codereview.chromium.org/5528003/diff/16001/net/socket/ssl_client_socke... net/socket/ssl_client_socket_openssl.cc:592: *outlen = 0; On 2010/12/03 15:17:37, agl wrote: > *out = "http/1.1"; > *outlen = 8; I added this in later patch, should I change to this or leave it like it is there? http://codereview.chromium.org/5528003/diff/16001/net/socket/ssl_client_socke... net/socket/ssl_client_socket_openssl.cc:603: //case OPENSSL_NPN_UNSUPPORTED: On 2010/12/03 15:17:37, agl wrote: > Why commented out? Thanks, it was to test compile on ubuntu where they are not defined.
Any updates?
On Wed, Dec 8, 2010 at 2:39 PM, <kristianm@google.com> wrote: > Any updates? Are you waiting for me? If so, why? AGL
I said to wait for your LGTM rather than mine. On Wed, Dec 8, 2010 at 11:50 AM, Adam Langley <agl@chromium.org> wrote: > On Wed, Dec 8, 2010 at 2:39 PM, <kristianm@google.com> wrote: > > Any updates? > > Are you waiting for me? If so, why? > > > AGL >
On Wed, Dec 8, 2010 at 2:54 PM, William Chan (陈智昌) <willchan@chromium.org> wrote: > I said to wait for your LGTM rather than mine. LGTM (p.s. *never* wait > one business day for a reply from me. If you haven't gotten one by then, please ping.) AGL
On 2010/12/08 19:58:36, agl wrote: > On Wed, Dec 8, 2010 at 2:54 PM, William Chan (陈智昌) > <mailto:willchan@chromium.org> wrote: > > I said to wait for your LGTM rather than mine. > > LGTM > > (p.s. *never* wait > one business day for a reply from me. If you > haven't gotten one by then, please ping.) > > > AGL Thank you all, will ping quicker next time.
|
