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.cof...@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....@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.
>
