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

Vincent Ryan vincent.x.ryan at oracle.com
Tue Dec 1 21:21:11 UTC 2015


Hello Sean,

An empty array is allowed: it means do not use ALPN.
I’ve updated the exception messages to display the offending length in each case.

--- ALPNExtension.java  Tue Dec  1 15:22:02 2015
+++ ALPNExtension.java  Tue Dec  1 14:56:12 2015
@@ -97,11 +97,13 @@
             listLength = s.getInt16(); // list length
             if (listLength < 2 || listLength + 2 != len) {
                 throw new SSLProtocolException(
-                    "Invalid " + type + " extension: incorrect list length");
+                    "Invalid " + type + " extension: incorrect list length " +
+                    "(length=" + listLength + ")");
             }
         } else {
             throw new SSLProtocolException(
-                "Invalid " + type + " extension: too short");
+                "Invalid " + type + " extension: insufficient data " +
+                "(length=" + len + ")");
         }

         int remaining = listLength;
@@ -121,7 +123,8 @@

         if (remaining != 0) {
             throw new SSLProtocolException(
-                "Invalid " + type + " extension: extra data");
+                "Invalid " + type + " extension: extra data " +
+                "(length=" + remaining + ")");
         }
     }




> On 1 Dec 2015, at 13:53, Seán Coffey <sean.coffey at oracle.com> wrote:
> 
> 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.
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/security-dev/attachments/20151201/6a0fbff6/attachment-0001.html>


More information about the security-dev mailing list