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