NSS: move the first protocol to the end of the ALPN extension.

NSS: move the first protocol to the end of the ALPN extension.

NPN defines the first protocol in the client's list to be the protocol that the
client will select when there's no overlap - i.e. it's the fallback protocol.
However, then the NPN data is used for ALPN, it was becoming the most
preferable protocol because ALPN says that they are in preference order.

This change causes NSS to be aware that the data that is set by
SSL_SetNextProtoNego is in NPN's order and thus the first element should be
moved to the end when sending an ALPN extension.

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

[wtc, 2013-08-05 22:23:38]

On 2013/08/05 17:59:29, agl wrote:
> NPN defines the first protocol in the client's list to be the protocol that
the
> client will select when there's no overlap - i.e. it's the fallback protocol.

Are you sure this is the meaning of the first protocol in the client's list for
NPN?

Here is a quote from the NPN technical note:

  Protocol selection

  It's expected that a client will have a list of protocols that it supports,
  in preference order, and will only select a protocol if the server supports
  it. ...

So the NPN technical note also says the client's list is in preference order.

[wtc, 2013-08-05 22:37:35]

Patch set 1 LGTM.

I wonder if we should clarify this issue in the NPN technical
note. Also, we should document this better in the comments
for SSL_SetNextProtoNego in ssl.h.

https://codereview.chromium.org/22237002/diff/1/net/third_party/nss/ssl/ssl3e...
File net/third_party/nss/ssl/ssl3ext.c (right):

https://codereview.chromium.org/22237002/diff/1/net/third_party/nss/ssl/ssl3e...
net/third_party/nss/ssl/ssl3ext.c:742: * list. */

This policy implies the fallback protocol is the least preferred by the client.
While this is true for SPDY and HTTP/1.1, is this true in general?

https://codereview.chromium.org/22237002/diff/1/net/third_party/nss/ssl/ssl3e...
net/third_party/nss/ssl/ssl3ext.c:744: const size_t len =
ss->opt.nextProtoNego.len;

Nit: |len| can be an unsigned int. I wonder if passing a size_t to
ssl3_AppendHandshakeVariable may result in a compiler warning about
value truncation if size_t is 64 bits.

https://codereview.chromium.org/22237002/diff/1/net/third_party/nss/ssl/ssl3e...
net/third_party/nss/ssl/ssl3ext.c:752: /* Each protocol string is prefixed with
a single byte length. */

Nit: the variable |i| can be declared inside this block.

https://codereview.chromium.org/22237002/diff/1/net/third_party/nss/ssl/ssl3e...
net/third_party/nss/ssl/ssl3ext.c:756: memcpy(alpn_protos + len-i,
ss->opt.nextProtoNego.data, i);

Nit: len-i => len - i

Are you trying to avoid a line wrap?

[agl, 2013-08-06 17:06:43]

Please let me know if the plans for the API are ok with you.

https://codereview.chromium.org/22237002/diff/1/net/third_party/nss/ssl/ssl3e...
File net/third_party/nss/ssl/ssl3ext.c (right):

https://codereview.chromium.org/22237002/diff/1/net/third_party/nss/ssl/ssl3e...
net/third_party/nss/ssl/ssl3ext.c:742: * list. */
On 2013/08/05 22:37:35, wtc wrote:
> 
> This policy implies the fallback protocol is the least preferred by the
client.
> While this is true for SPDY and HTTP/1.1, is this true in general?

I can imagine a case where it's not true, but it would be odd.

Despite what the NPN technote suggests, NPN clients have, in practice, always
used the server's preference. Thus there was only a "fallback protocol" and
"everything else" for NPN.

For ALPN, that's no longer true. The NSS API at the moment is making NSS and
ALPN transparent and I think that means that we need to do this. When we come to
removing NSS support in 2015 I think we should add a new API function which
takes the ALPN protocols in ALPN order. But subtly changing the semantics of the
existing NSS call seems bad.

The alternative is to have an explicit ALPN API now, but I think that's worse
for promoting the use of ALPN, which is the goal for the moment.

https://codereview.chromium.org/22237002/diff/1/net/third_party/nss/ssl/ssl3e...
net/third_party/nss/ssl/ssl3ext.c:744: const size_t len =
ss->opt.nextProtoNego.len;
On 2013/08/05 22:37:35, wtc wrote:
> 
> Nit: |len| can be an unsigned int. I wonder if passing a size_t to
> ssl3_AppendHandshakeVariable may result in a compiler warning about
> value truncation if size_t is 64 bits.

Changed to unsigned int (and |i|).

https://codereview.chromium.org/22237002/diff/1/net/third_party/nss/ssl/ssl3e...
net/third_party/nss/ssl/ssl3ext.c:752: /* Each protocol string is prefixed with
a single byte length. */
On 2013/08/05 22:37:35, wtc wrote:
> 
> Nit: the variable |i| can be declared inside this block.

Done.

https://codereview.chromium.org/22237002/diff/1/net/third_party/nss/ssl/ssl3e...
net/third_party/nss/ssl/ssl3ext.c:756: memcpy(alpn_protos + len-i,
ss->opt.nextProtoNego.data, i);
On 2013/08/05 22:37:35, wtc wrote:
> 
> Nit: len-i => len - i
> 
> Are you trying to avoid a line wrap?

It was just the logical grouping in my mind. Changed.

[wtc, 2013-08-06 18:08:56]

Patch set 2 LGTM. Your plans for the API are ok with me. Thanks.

Is the plan to remove NPN support in 2015 documented somewhere?
I didn't find "2015" in draft-friedl-tls-applayerprotoneg-02.

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

https://codereview.chromium.org/22237002/diff/9001/net/third_party/nss/ssl/ss...
net/third_party/nss/ssl/ssl.h:213: * indicates that the protocol is the least
preferred.

Nit: the protocol => the fallback protocol ?

We should also point out that the other protocols need to be in preference
order.

[commit-bot: I haz the power, 2013-08-08 16:03:46]

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

[commit-bot: I haz the power, 2013-08-09 00:16:40]

Change committed as 216503