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

[Seán Coffey]

Hey Vinnie,

question on SSLParameters.setApplicationProtocols(String[] protocols) method

What happens if you pass an empty array into this method. Shouldn't it 
throw an IllegalArgumentException  ?

====

In ALPNExtension.java :
> +            if (listLength < 2 || listLength + 2 != len) {
> +                throw new SSLProtocolException(
> +                    "Invalid " + type + " extension: incorrect list length");
Can you print the listLength value in exception message ?

> +            throw new SSLProtocolException(
> +                "Invalid " + type + " extension: too short");
Can you print the len value here ?

> +        if (remaining != 0) {
> +            throw new SSLProtocolException(
> +                "Invalid " + type + " extension: extra data");
Can you print remaining value here ?

Regards,
Sean.

On 01/12/2015 01:53, Xuelei Fan wrote:
> 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.