net: support ALPN.

net: support ALPN.

BUG=254178
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=209878

[wtc, 2013-07-01 23:07:13]

Added rsleevi as a second reviewer.

[Ryan Sleevi, 2013-07-02 00:41:42]

This LGTM. It took me a bit to realize that ss->ssl3.nextProto contains the
*negotiated* protocol, whereas ss->opt.nextProtoNego contains the NPN/ALPN data.


This works so long as NPN and ALPN remain wire compatible. Which is certainly
the case today.

https://codereview.chromium.org/18346010/diff/1/net/third_party/nss/ssl/ssl.h
File net/third_party/nss/ssl/ssl.h (right):

https://codereview.chromium.org/18346010/diff/1/net/third_party/nss/ssl/ssl.h...
net/third_party/nss/ssl/ssl.h:208: * support ALPN.
nit: I'm not sure I fully parse this comment. Does this mean
SSL_SetNextProtoCallback *also* supports ALPN? This function is documented as
being an *alternative* to SSL_SetNextProtoCallbac, so the "in addition" is
confusing.

[agl, 2013-07-02 17:43:01]

Have also added a new error to catch the case where a server tries to negotiate
both NPN and ALPN on the same connection.

https://codereview.chromium.org/18346010/diff/1/net/third_party/nss/ssl/ssl.h
File net/third_party/nss/ssl/ssl.h (right):

https://codereview.chromium.org/18346010/diff/1/net/third_party/nss/ssl/ssl.h...
net/third_party/nss/ssl/ssl.h:208: * support ALPN.
On 2013/07/02 00:41:43, Ryan Sleevi wrote:
> nit: I'm not sure I fully parse this comment. Does this mean
> SSL_SetNextProtoCallback *also* supports ALPN? This function is documented as
> being an *alternative* to SSL_SetNextProtoCallbac, so the "in addition" is
> confusing.

You're right, that was nonsense.

" * Using this function also allows the client to transparently support ALPN.
 * The same set of protocols will be advertised via ALPN and, if the server
 * uses ALPN to select a protocol, SSL_GetNextProto will return
 * SSL_NEXT_PROTO_SELECTED as the state.
"

[Ryan Sleevi, 2013-07-02 18:36:48]

https://codereview.chromium.org/18346010/diff/10001/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/ssl3ext.c (right):

https://codereview.chromium.org/18346010/diff/10001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3ext.c:662: return SECFailure;
This won't actually abort the connection - it will just cause ALPN || NPN to
used, but not both.

Is that what you intended?

See
https://code.google.com/p/chromium/codesearch#chromium/src/net/third_party/ns...

[agl, 2013-07-02 18:42:22]

https://codereview.chromium.org/18346010/diff/10001/net/third_party/nss/ssl/s...
File net/third_party/nss/ssl/ssl3ext.c (right):

https://codereview.chromium.org/18346010/diff/10001/net/third_party/nss/ssl/s...
net/third_party/nss/ssl/ssl3ext.c:662: return SECFailure;
On 2013/07/02 18:36:48, Ryan Sleevi wrote:
> This won't actually abort the connection - it will just cause ALPN || NPN to
> used, but not both.
> 
> Is that what you intended?

Hmm, I was planning on aborting the connection but that doesn't seem possible
and I don't think it's worth twisting the existing code too much to make it
happen.

As such, it doesn't matter what the error code is so I've removed the new error
code.

[Ryan Sleevi, 2013-07-02 19:13:10]

LGTM

https://codereview.chromium.org/18346010/diff/22001/net/socket/ssl_client_soc...
File net/socket/ssl_client_socket_nss.cc (right):

https://codereview.chromium.org/18346010/diff/22001/net/socket/ssl_client_soc...
net/socket/ssl_client_socket_nss.cc:2501: case SSL_NEXT_PROTO_SELECTED:
This will affect the histograms for NPN, as well as enable things like false
start.

I think this is OK for the initial landing, since we really do want the rest of
the codepaths to be the same for ALPN vs NPN, but it may be good to do a
follow-up CL to make sure we're not mis-reporting anything.

[commit-bot: I haz the power, 2013-07-02 21:14:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/agl@chromium.org/18346010/22001

[commit-bot: I haz the power, 2013-07-03 04:06:25]

Change committed as 209878

[wtc, 2013-08-01 19:35:24]

https://chromiumcodereview.appspot.com/18346010/diff/22001/net/third_party/ns...
File net/third_party/nss/ssl/sslt.h (right):

https://chromiumcodereview.appspot.com/18346010/diff/22001/net/third_party/ns...
net/third_party/nss/ssl/sslt.h:199: ssl_application_layer_protocol   = 16,

The enum for ALPN should also end in _xtn like the other enums.
This requires abbreviation to keep the enum name short, for example,
ssl_app_layer_proto_nego_xtn.