<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">Hello Sean,<div class=""><br class=""><div class="">An empty array is allowed: it means do not use ALPN.</div><div class="">I’ve updated the exception messages to display the offending length in each case.</div><div class=""><br class=""></div><div class=""><div style="margin: 0px; font-size: 11px; font-family: Menlo; color: rgb(52, 189, 38);" class="">--- ALPNExtension.java Tue Dec 1 15:22:02 2015</div><div style="margin: 0px; font-size: 11px; font-family: Menlo; color: rgb(52, 189, 38);" class="">+++ ALPNExtension.java Tue Dec 1 14:56:12 2015</div><div style="margin: 0px; font-size: 11px; font-family: Menlo; color: rgb(206, 121, 36);" class="">@@ -97,11 +97,13 @@</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""> listLength = s.getInt16(); // list length</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""> if (listLength < 2 || listLength + 2 != len) {</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""> throw new SSLProtocolException(</div><div style="margin: 0px; font-size: 11px; font-family: Menlo; color: rgb(213, 59, 211);" class="">- "Invalid " + type + " extension: incorrect list length");</div><div style="margin: 0px; font-size: 11px; font-family: Menlo; color: rgb(52, 187, 199);" class="">+ "Invalid " + type + " extension: incorrect list length " +</div><div style="margin: 0px; font-size: 11px; font-family: Menlo; color: rgb(52, 187, 199);" class="">+ "(length=" + listLength + ")");</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""> }</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""> } else {</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""> throw new SSLProtocolException(</div><div style="margin: 0px; font-size: 11px; font-family: Menlo; color: rgb(213, 59, 211);" class="">- "Invalid " + type + " extension: too short");</div><div style="margin: 0px; font-size: 11px; font-family: Menlo; color: rgb(52, 187, 199);" class="">+ "Invalid " + type + " extension: insufficient data " +</div><div style="margin: 0px; font-size: 11px; font-family: Menlo; color: rgb(52, 187, 199);" class="">+ "(length=" + len + ")");</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""> }</div><div style="margin: 0px; font-size: 11px; font-family: Menlo; min-height: 13px;" class=""><br class=""></div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""> int remaining = listLength;</div><div style="margin: 0px; font-size: 11px; font-family: Menlo; color: rgb(206, 121, 36);" class="">@@ -121,7 +123,8 @@</div><div style="margin: 0px; font-size: 11px; font-family: Menlo; min-height: 13px;" class=""><br class=""></div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""> if (remaining != 0) {</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""> throw new SSLProtocolException(</div><div style="margin: 0px; font-size: 11px; font-family: Menlo; color: rgb(213, 59, 211);" class="">- "Invalid " + type + " extension: extra data");</div><div style="margin: 0px; font-size: 11px; font-family: Menlo; color: rgb(52, 187, 199);" class="">+ "Invalid " + type + " extension: extra data " +</div><div style="margin: 0px; font-size: 11px; font-family: Menlo; color: rgb(52, 187, 199);" class="">+ "(length=" + remaining + ")");</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""> }</div><div style="margin: 0px; font-size: 11px; font-family: Menlo;" class=""> }</div><div style="margin: 0px; font-size: 11px; font-family: Menlo; min-height: 13px;" class=""><br class=""></div></div><div style="margin: 0px; font-size: 11px; font-family: Menlo; min-height: 13px;" class=""><br class=""></div><div class=""><br class=""></div><div class=""><br class=""><div><blockquote type="cite" class=""><div class="">On 1 Dec 2015, at 13:53, Seán Coffey <<a href="mailto:sean.coffey@oracle.com" class="">sean.coffey@oracle.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class="">Hey Vinnie,<br class=""><br class="">question on SSLParameters.setApplicationProtocols(String[] protocols) method<br class=""><br class="">What happens if you pass an empty array into this method. Shouldn't it throw an IllegalArgumentException ?<br class=""><br class="">====<br class=""><br class="">In ALPNExtension.java :<br class=""><blockquote type="cite" class="">+ if (listLength < 2 || listLength + 2 != len) {<br class="">+ throw new SSLProtocolException(<br class="">+ "Invalid " + type + " extension: incorrect list length");<br class=""></blockquote>Can you print the listLength value in exception message ?<br class=""><br class=""><blockquote type="cite" class="">+ throw new SSLProtocolException(<br class="">+ "Invalid " + type + " extension: too short");<br class=""></blockquote>Can you print the len value here ?<br class=""><br class=""><blockquote type="cite" class="">+ if (remaining != 0) {<br class="">+ throw new SSLProtocolException(<br class="">+ "Invalid " + type + " extension: extra data");<br class=""></blockquote>Can you print remaining value here ?<br class=""><br class="">Regards,<br class="">Sean.<br class=""><br class="">On 01/12/2015 01:53, Xuelei Fan wrote:<br class=""><blockquote type="cite" class="">Looks fine to me. Thanks for the update.<br class=""><br class="">Xuelei<br class=""><br class="">On 12/1/2015 7:08 AM, Vincent Ryan wrote:<br class=""><blockquote type="cite" class="">Thanks for your review. Comments in-line.<br class=""><br class=""><blockquote type="cite" class="">On 30 Nov 2015, at 06:30, Xuelei Fan <<a href="mailto:xuelei.fan@oracle.com" class="">xuelei.fan@oracle.com</a>> wrote:<br class=""><br class="">Looks fine to me. Just a few minor comments.<br class=""><br class="">ServerHandshaker.java<br class="">=====================<br class="">There is a typo of the first line.<br class="">- /* Copyright (c) 1996, 2015, ...<br class="">+ /*<br class="">+ * Copyright (c) 1996, 2015 …<br class=""><br class=""></blockquote>Done.<br class=""><br class=""><blockquote type="cite" class="">line 538-546<br class="">------------<br class=""> String negotiatedValue = null;<br class=""> List<String> protocols = clientHelloALPN.getPeerAPs();<br class=""><br class=""> for (String ap : localApl) {<br class=""> if (protocols.contains(ap)) {<br class=""> negotiatedValue = ap;<br class=""> break;<br class=""> }<br class=""> }<br class=""><br class="">I think the above order prefer the server preference order of<br class="">application protocols. It's OK per Section 3.2 of RFC 7301.<br class=""><br class="">However RFC 7301 also defines the client preference order. Looks like<br class="">the client preference order is not necessary. It's a little bit<br class="">confusing, and may result in different behaviors for different TLS vendors.<br class=""><br class="">Maybe, we can add an option to honor server preference order in the future.<br class=""></blockquote>I added a comment to show that server-preference is used.<br class="">If necessary I think we can add support for controlling this via a property, later.<br class=""><br class=""><br class=""><blockquote type="cite" class="">ALPNExtension.java<br class="">==================<br class="">96 listLength = s.getInt16(); // list length<br class=""><br class="">The code would throws SSLException if the remaining input is not<br class="">sufficient. I was wondering, SSLProtocolException may be more suitable<br class="">here. May be nice to check the "len" argument value if sufficient for<br class="">the list length. Otherwise, a SSLProtocolException would be thrown.<br class=""><br class=""> if (len >= 2) {<br class=""> listLength = s.getInt16(); // list length<br class=""> ...<br class=""> } else {<br class=""> throw new SSLProtocolException(...);<br class=""> }<br class=""></blockquote>I added the exception and more detailed message.<br class=""><br class=""><blockquote type="cite" class="">-----------<br class="">104 byte[] bytes = s.getBytes8();<br class="">105 String p =<br class="">106 new String(bytes, StandardCharsets.UTF_8); // app protocol<br class=""><br class="">Do you want to check that the app protocol cannot be empty?<br class=""><br class=""> // opaque ProtocolName<1..2^8-1>; // RFC 7301<br class=""> byte[] bytes = s.getBytes8();<br class="">+ if (bytes.length == 0) {<br class="">+ throw new SSLProtocolException("Empty ...");<br class="">+ }<br class=""></blockquote>Done.<br class=""><br class=""><br class=""><blockquote type="cite" class="">---------<br class="">Personally, I would prefer to use unmodifiableList for the protocolNames<br class="">variable.<br class=""></blockquote>It is accessible only to classes in this package.<br class=""><br class=""><br class=""><blockquote type="cite" class=""><br class="">HelloExtensions.java<br class="">====================<br class="">line 34-37:<br class="">* This file contains all the classes relevant to TLS Extensions for the<br class="">* ClientHello and ServerHello messages. The extension mechanism and<br class="">* several extensions are defined in RFC 3546. Additional extensions are<br class="">* defined in the ECC RFC 4492.The ALPN extension is defined in RFC7301.<br class=""><br class="">I may put the "Additional extensions ..." at the end. BTW, as you are<br class="">here already, would you mind update RFC 3546 to RFC 6066? RFC 6066 is<br class="">the newest revision of RFC 3546.<br class=""></blockquote>I concatenated the final sentence.<br class=""><br class=""><br class=""><blockquote type="cite" class="">* This file contains all the classes relevant to TLS Extensions for the<br class="">* ClientHello and ServerHello messages. The extension mechanism and<br class="">* several extensions are defined in RFC 6066. The ALPN extension is<br class="">* defined in RFC 7301. Additional extensions are defined in the ECC<br class="">* RFC 4492.<br class=""><br class="">SSLEngineImpl.java<br class="">==================<br class=""><br class="">line 1411-1412:<br class="">private Ciphertext writeRecord(ByteBuffer[] appData,<br class=""> int offset, int length, ByteBuffer netData) throws IOException {<br class=""> ...<br class=""> if (...) {<br class=""> ...<br class=""> // set connection ALPN value<br class=""> applicationProtocol =<br class=""> handshaker.getHandshakeApplicationProtocol();<br class=""> ...<br class=""> }<br class="">}<br class=""><br class="">Is it redundant to set applicationProtocol here. I was wondering, the<br class="">value should set in the processInputRecord() method.<br class=""></blockquote>My understanding is the the wrapping is performed here and the unwrapping<br class="">performed in processInputRecord(). Isn’t the assignment required in both places?<br class=""><br class=""><br class=""><br class=""><blockquote type="cite" class="">Cheers,<br class="">Xuelei<br class=""><br class="">On 11/30/2015 8:08 AM, Vincent Ryan wrote:<br class=""><blockquote type="cite" class="">Hello,<br class=""><br class="">Following on from Brad’s recent email, here is the full webrev of the<br class="">API and the implementation classes for ALPN:<br class=""> <a href="http://cr.openjdk.java.net/~vinnie/8144093/webrev.00/" class="">http://cr.openjdk.java.net/~vinnie/8144093/webrev.00/</a><br class=""><br class="">In adds the implementation classes (sun/security/ssl) to the public API<br class="">classes (javax/net/ssl) which have already been agreed.<br class="">Some basic tests (test/javax/net/ssl) are also included.<br class=""><br class="">Please send any code review comments by close-of-business on Tuesday 1<br class="">December.<br class="">Thanks.<br class=""></blockquote></blockquote></blockquote></blockquote><br class=""></div></blockquote></div><br class=""></div></div></body></html>