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

[Xuelei Fan]

Looks fine to me.  Thanks for the update.

Xuelei

On 12/1/2015 7:08 AM, Vincent Ryan wrote:
> 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.
>>
>