Request for review: 8144093: JEP 244/8051498 - TLS Application-Layer Protocol Negotiation Extension

[Vincent Ryan]

Thanks for your review. Comments in-line.

> On 30 Nov 2015, at 06:30, Xuelei Fan <xuelei.fan at oracle.com> wrote:
> 
> Looks fine to me.  Just a few minor comments.
> 
> ServerHandshaker.java
> =====================
> There is a typo of the first line.
> - /* Copyright (c) 1996, 2015, ...
> + /*
> +  * Copyright (c) 1996, 2015 …
> 
Done.

> 
> line 538-546
> ------------
>   String negotiatedValue = null;
>   List<String> protocols = clientHelloALPN.getPeerAPs();
> 
>   for (String ap : localApl) {
>       if (protocols.contains(ap)) {
>           negotiatedValue = ap;
>           break;
>       }
>   }
> 
> I think the above order prefer the server preference order of
> application protocols.  It's OK per Section 3.2 of RFC 7301.
> 
> However RFC 7301 also defines the client preference order.  Looks like
> the client preference order is not necessary.  It's a little bit
> confusing, and may result in different behaviors for different TLS vendors.
> 
> Maybe, we can add an option to honor server preference order in the future.

I added a comment to show that server-preference is used.
If necessary I think we can add support for controlling this via a property, later.


> 
> ALPNExtension.java
> ==================
> 96    listLength = s.getInt16(); // list length
> 
> The code would throws SSLException if the remaining input is not
> sufficient.  I was wondering, SSLProtocolException may be more suitable
> here.  May be nice to check the "len" argument value if sufficient for
> the list length.  Otherwise, a SSLProtocolException would be thrown.
> 
>    if (len >= 2) {
>        listLength = s.getInt16(); // list length
>        ...
>    } else {
>        throw new SSLProtocolException(...);
>    }

I added the exception and more detailed message.

> 
> -----------
> 104   byte[] bytes = s.getBytes8();
> 105   String p =
> 106       new String(bytes, StandardCharsets.UTF_8); // app protocol
> 
> Do you want to check that the app protocol cannot be empty?
> 
>     // opaque ProtocolName<1..2^8-1>;  // RFC 7301
>     byte[] bytes = s.getBytes8();
> +    if (bytes.length == 0) {
> +        throw new SSLProtocolException("Empty ...");
> +    }

Done.


> 
> ---------
> Personally, I would prefer to use unmodifiableList for the protocolNames
> variable.

It is accessible only to classes in this package.


> 
> 
> HelloExtensions.java
> ====================
> line 34-37:
> * This file contains all the classes relevant to TLS Extensions for the
> * ClientHello and ServerHello messages. The extension mechanism and
> * several extensions are defined in RFC 3546. Additional extensions are
> * defined in the ECC RFC 4492.The ALPN extension is defined in RFC7301.
> 
> I may put the "Additional extensions ..." at the end.  BTW, as you are
> here already, would you mind update RFC 3546 to RFC 6066?  RFC 6066 is
> the newest revision of RFC 3546.

I concatenated the final sentence.


> 
> * This file contains all the classes relevant to TLS Extensions for the
> * ClientHello and ServerHello messages. The extension mechanism and
> * several extensions are defined in RFC 6066. The ALPN extension is
> * defined in RFC 7301.  Additional extensions are defined in the ECC
> * RFC 4492.
> 
> SSLEngineImpl.java
> ==================
> 
> line 1411-1412:
> private Ciphertext writeRecord(ByteBuffer[] appData,
>    int offset, int length, ByteBuffer netData) throws IOException {
>    ...
>  if (...) {
>    ...
>    // set connection ALPN value
>    applicationProtocol =
>        handshaker.getHandshakeApplicationProtocol();
>    ...
>  }
> }
> 
> Is it redundant to set applicationProtocol here.  I was wondering, the
> value should set in the processInputRecord() method.

My understanding is the the wrapping is performed here and the unwrapping 
performed in processInputRecord(). Isn’t the assignment required in both places?



> 
> Cheers,
> Xuelei
> 
> On 11/30/2015 8:08 AM, Vincent Ryan wrote:
>> Hello,
>> 
>> Following on from Brad’s recent email, here is the full webrev of the
>> API and the implementation classes for ALPN:
>>  http://cr.openjdk.java.net/~vinnie/8144093/webrev.00/
>> 
>> In adds the implementation classes (sun/security/ssl) to the public API
>> classes (javax/net/ssl) which have already been agreed.
>> Some basic tests (test/javax/net/ssl) are also included.
>> 
>> Please send any code review comments by close-of-business on Tuesday 1
>> December.
>> Thanks.
>