Closed Bug 959664 Opened 6 years ago Closed 6 years ago

Add ALPN (RFC 7301) support to NSS

Categories

(NSS :: Libraries, enhancement, P1)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED
3.15.5

People

(Reporter: mcmanus, Assigned: mcmanus)

References

()

Details

Attachments

(2 files, 3 obsolete files)

+++ 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
also added an option to control NPN. The NPN option defaults to true for backwards compatibility.
Assignee: nobody → mcmanus
Status: NEW → ASSIGNED
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.
Attachment #8359847 - Flags: review?(brian) → review+
Depends on: 959764
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
Attachment #8359847 - Attachment is obsolete: true
Attachment #8359978 - Flags: review+
(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.
Flags: needinfo?(brian)
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.
(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
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.
Attachment #8359978 - Attachment is obsolete: true
Attachment #8361287 - Flags: review+
Attachment #8361287 - Flags: feedback?(mcmanus)
Flags: needinfo?(brian)
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 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 :(
Attachment #8361287 - Flags: feedback?(mcmanus) → feedback+
Checked in in comment 11.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Priority: -- → P1
Target Milestone: --- → 3.15.5
Attached patch Fix comment nits (obsolete) — Splinter Review
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.
Attachment #8362128 - Flags: superreview?(brian)
Attachment #8362128 - Flags: review?(mcmanus)
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.
Attachment #8362128 - Flags: superreview?(brian) → superreview+
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.
Attachment #8362128 - Flags: review?(mcmanus)
Target Milestone: 3.15.5 → 3.16
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
Attachment #8362128 - Attachment is obsolete: true
Attachment #8364827 - Flags: checked-in+
Is this fully supported on both sides ? Are there going to be any tests for this extension ?
Target Milestone: 3.16 → 3.15.5
Summary: Add ALPN support to NSS → Add ALPN (RFC 7301) support to NSS
You need to log in before you can comment on or make changes to this bug.