959664 - Add ALPN (RFC 7301) support to NSS

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P1)

Description

[Patrick McManus [:mcmanus]]

+++ This bug was initially created as a clone of Bug #890994 +++

From agl originally: http://src.chromium.org/viewvc/chrome?view=revision&revision=216503

added an SSL option to control its negotiation

Comment 1

[Patrick McManus [:mcmanus]]

also added an option to control NPN. The NPN option defaults to true for backwards compatibility.

Comment 2

[Patrick McManus [:mcmanus]]

Attachment: ALPN support for NSS based on agl patches

From agl originally: http://src.chromium.org/viewvc/chrome?view=revision&revision=216503

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8359847 [details] [diff] [review]
ALPN support for NSS based on agl patches

Review of attachment 8359847 [details] [diff] [review]:
-----------------------------------------------------------------

Please make the changes I suggest below and attach a new version of the patch. I would like to get this checked into NSS 3.15.5 beta 2 today so I can merge it into m-c today before tonight's nightly build.

::: security/nss/lib/ssl/ssl.h
@@ +161,5 @@
>   */
>  #define SSL_CBC_RANDOM_IV 23
>  #define SSL_ENABLE_OCSP_STAPLING       24 /* Request OCSP stapling (client) */
> +#define SSL_ENABLE_NEXT_PROTOCOL_NEGOTIATION 25 /* Client use NPN */
> +#define SSL_ENABLE_ALTERNATE_PROTOCOL_NEGOTIATION 26 /* Client use ALPN */

Note that the approach I mentioned in bug 890994 comment 5 would have also been OK and probably better. it is true that the third argument to SSL_OptionSet is a PRBool, but we already have support passing in non-boolean values for it due to the SSL_ENABLE_RENEGOTIATION option. So, we should instead just change the type of that parameter to PRUInt32 or whatever PRBool is typedefed to be. (In a separate bug.)

However, this way is OK for now. If other NSS peers object to this, we can change to the other way and adjust Gecko, before NSS 3.15.5 is released.

Please add a comment giving the default values for these and the reasoning. Note that even if the options are enabled, the extensions won't actually be sent unless the application calls ssl3_SendNextProto or the other function like it.

@@ +215,5 @@
> + *
> + * Since NPN uses the first protocol as the fallback protocol, when sending an
> + * ALPN extension, the first protocol is moved to the end of the list. This
> + * indicates that the fallback protocol is the least preferred. The other
> + * protocols should be in preference order.

I am confused by this. It seems like either ALPN should reverse all the protocols or I am misunderstanding things. It seems strange to just move the first protocol to the last place.

::: security/nss/lib/ssl/ssl3ext.c
@@ +613,5 @@
>      SECItem result = { siBuffer, resultBuffer, 0 };
>  
>      PORT_Assert(!ss->firstHsDone);
>  
> +    if (ssl3_ExtensionNegotiated(ss, ssl_app_layer_protocol_xtn)) {

Please add a comment:

If the server negotiated ALPN then it has already told us what protocol to use, so it doesn't make sense for us to try to negotiate a different one by sending the NPN handshake message. However, if we've negotiated NPN then we're required to send the NPN handshake message. Thus, these two extensions cannot both be negotiated on the same connection.

@@ +675,5 @@
> +
> +    name_list_len = ((PRUint16) d[0]) << 8 |
> +	            ((PRUint16) d[1]);
> +    if (name_list_len != data->len - 2 ||
> +	d[2] != data->len - 3) {

nit: This condition should be on one line.

@@ +731,5 @@
> +
> +    /* Renegotiations do not send this extension. */
> +    if (!ss->opt.enableALPN || !ss->opt.nextProtoNego.data || ss->firstHsDone) {
> +	return 0;
> +    }

Adding the !ss->opt.enableALPN condition was the only change you made to this function, form AGL's patch, right?

@@ +757,5 @@
> +		memcpy(alpn_protos, &ss->opt.nextProtoNego.data[i], len - i);
> +		memcpy(alpn_protos + len - i, ss->opt.nextProtoNego.data, i);
> +	    } else {
> +		/* This seems to be invalid data so we'll send as-is. */
> +		memcpy(alpn_protos, ss->opt.nextProtoNego.data, len);

IMO, it would be better to fail on invalid data. We can make this change in a follow-up patch before NSS 3.15.5 is released.

@@ +763,5 @@
> +	}
> +
> +	rv = ssl3_AppendHandshakeNumber(ss, ssl_app_layer_protocol_xtn, 2);
> +	if (rv != SECSuccess)
> +	    goto loser;

NIT: please use braces around single-line conditional statements. (3x around gotos in this function, and once around PORT_Free below).

::: security/nss/lib/ssl/sslsock.c
@@ +79,5 @@
>      PR_FALSE,   /* enableFalseStart   */
>      PR_TRUE,    /* cbcRandomIV        */
> +    PR_FALSE,   /* enableOCSPStapling */
> +    PR_TRUE,    /* enableNPN          */
> +    PR_FALSE    /* enableALPN         */

I agree that these are the right defaults for now. But, we should expect that one or both may change soon or not-so-soon.

Comment 4

[Patrick McManus [:mcmanus]]

https://tbpl.mozilla.org/?tree=Try&rev=763586e7b019

Comment 5

[Patrick McManus [:mcmanus]]

Attachment: ALPN support for NSS based on agl patches

From agl originally: http://src.chromium.org/viewvc/chrome?view=revision&revision=216503

Comment 6

[Patrick McManus [:mcmanus]]

Thanks Brian.

(In reply to Brian Smith (:briansmith, was :bsmith; Please NEEDINFO? me if you want a response) from comment #3)

> should instead just change the type of that parameter to PRUInt32 or
> whatever PRBool is typedefed to be. (In a separate bug.)

filed 959764

> 
> @@ +215,5 @@
> > + *
> > + * Since NPN uses the first protocol as the fallback protocol, when sending an
> > + * ALPN extension, the first protocol is moved to the end of the list. This
> > + * indicates that the fallback protocol is the least preferred. The other
> > + * protocols should be in preference order.
> 
> I am confused by this. It seems like either ALPN should reverse all the
> protocols or I am misunderstanding things. It seems strange to just move the
> first protocol to the last place.

strange as though it may be - I think Adam's comment captures the reality. There is a mailing list thread about it somewhere. The fallback for NPN is essentially special cased and logically at the wrong end of the (otherwise rightly ordered) list and ALPN corrects that at the protocol level.. but we haven't changed the calling interface so ALPN requires the fixup code.

> @@ +731,5 @@
> > +
> > +    /* Renegotiations do not send this extension. */
> > +    if (!ss->opt.enableALPN || !ss->opt.nextProtoNego.data || ss->firstHsDone) {
> > +	return 0;
> > +    }
> 
> Adding the !ss->opt.enableALPN condition was the only change you made to
> this function, form AGL's patch, right?

correct

Comment 7

[Patrick McManus [:mcmanus]]

(In reply to Brian Smith (:briansmith, was :bsmith; Please NEEDINFO? me if you want a response) from comment #3)

> Please make the changes I suggest below and attach a new version of the
> patch. I would like to get this checked into NSS 3.15.5 beta 2 today so I
> can merge it into m-c today before tonight's nightly build.
> 

let me know if we need further changes. thanks.

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8359847 [details] [diff] [review]
ALPN support for NSS based on agl patches

Review of attachment 8359847 [details] [diff] [review]:
-----------------------------------------------------------------

I will fix the nits when I check it in.

Comment 9

[Patrick McManus [:mcmanus]]

(In reply to Brian Smith (:briansmith, :bsmith; NEEDINFO? for response) from comment #8)
> Comment on attachment 8359847 [details] [diff] [review]
> ALPN support for NSS based on agl patches
> 
> Review of attachment 8359847 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I will fix the nits when I check it in.

I'm a little confused because you are commenting on an obsolete patch.. I uploaded a new one with identified nits addressed the same as the review day.


but either way, I have no objection

Comment 10

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: add-ALPN-to-NSS.patch

Please double-check the changes I made to the comments and naming.

I used the names "NPN" and "ALPN" since those names are what everybody will call these extensions. Also, you had used the name "AlternateProtocol" but ALPN actually stands for "Application Layer Protocol" negotiation, so the new name is clearer.

I also corrected some whitespace issues.

Comment 11

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8361287 [details] [diff] [review]
add-ALPN-to-NSS.patch

http://hg.mozilla.org/projects/nss/rev/7fa7518797e8

In the NSS conference call today, we discussed that the way that SSL_SetNextProtoNego function works would not be good to use for ALPN in the long term if ALPN becomes an IETF proposed standard. It requires the application to give NSS the protocols in the wrong order for ALPN (but the right order for NPN) to compensate for NSS's "fixing" of the order. We should create a new variant of this function that takes the protocols in the ALPN-specified order.

Comment 12

[Patrick McManus [:mcmanus]]

Comment on attachment 8361287 [details] [diff] [review]
add-ALPN-to-NSS.patch

Those comments are great thanks.

Yes, calling it Alternate-Protocol is a mental block for me on that one. not the first time :(

Comment 13

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Checked in in comment 11.

Comment 14

[Wan-Teh Chang]

Attachment: Fix comment nits

1. ssl.h: the comments refer to "protocol negotiation". Since TLS
has its own protocol negotiation, this is ambiguous. I made up the
term "upper layer protocol negotiation" that applies to both NPN
and ALPN. I am also fine with using "application layer protocol
negotiation" for both NPN and ALPN. Other suggestions are welcome.

2. ssl3ext.c: fold comment lines longer than 80 characters.

Comment 15

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8362128 [details] [diff] [review]
Fix comment nits

Review of attachment 8362128 [details] [diff] [review]:
-----------------------------------------------------------------

Personally, I think that the comments in ssl.h were clearer the way I worded them. It is true there is technically some ambiguity but nobody is going to be confused by that ambiguity. Introducing new terms like "upper layer" or using "application layer" to refer to NPN seems confusing; the cure is worse than the illness.

Comment 16

[Patrick McManus [:mcmanus]]

Comment on attachment 8362128 [details] [diff] [review]
Fix comment nits

both versions reflect reality, so either way is fine by me. I'll let the peers and owners of nss decide what they'd like to go with.

Comment 17

[Wan-Teh Chang]

Attachment: Fix comment nits, v2

I decided to use "application layer protocol negotiation" because I
found the NPN spec (https://technotes.googlecode.com/git/nextprotoneg.html)
also uses this term.

Patch checked in: https://hg.mozilla.org/projects/nss/rev/9b0a336cb896

Comment 18

[Julien Pierre]

Is this fully supported on both sides ? Are there going to be any tests for this extension ?