Thanks for the additional review comments. Responses in-line below. Updated webrev: http://cr.openjdk.java.net/~vinnie/8144093/webrev.02/
> On 1 Dec 2015, at 01:32, Bradford Wetmore <bradford.wetm...@oracle.com> wrote: > > > On 11/29/2015 4:08 PM, Vincent Ryan wrote: > > > 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. > > SSLEngineAlpnTest.java > ====================== > > 333: A minor comment here. I think you should test NONE like this: > > if (ap == null) { > throw new Exception( > "Handshake was completed but null was received"); > } > if (expectedAP.equals("NONE")) { > if (!ap.isEmpty()) { > throw new Exception(); > } else { > System.out.println("No ALPN value negotiated, as expected"); > } > } else if (!expectedAP.equals(ap)) { > throw new Exception(expectedAP + > " ALPN value not available on negotiated connection"); > } Done. > > BTW, the indentions here at 331/332/334 are off. The rest are all ok. Corrected. > > We had also talked privately about testing getHandshakeApplicationProtocol(), > but I don't see it here. Let me know if you didn't understand my comments > about adding a X509ExtendedKeyManager (or X509ExtendedTrustManager), or else > please file a RFE for it if it won't be ready for the initial integration. I’ll file an RFE for that. > > 298: This test is not actually calling into checkResult on the server side. > Ooops! You need to check the output of the wrap() before calling unwrap() as > it overwrites the serverResult. You need to put in a similar checkResult() > before doing the flip()s. So checks are required before and after the buffer flips, right? > > > SSLSocketAlpnTest.java > ====================== > > Same concern here as above. Done. > > > On 11/29/2015 10:30 PM, Xuelei Fan wrote: > > 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. > > Correct: > > In that case, the server SHOULD select the most > highly preferred protocol that it supports and that is also > advertised by the client. > > > 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. > > That was the plan if needed. > > Vincent wrote: > > If necessary I think we can add support for controlling this via a > > property, later. > > Or a new API (when possible): a new "preferPeerOrder" method and an update > to the setApplicationProtocols() text: > > Unless configured by the preferPeerOrder() method, the first > matched value becomes the negotiated value. > > On 11/30/2015 3:08 PM, Vincent Ryan wrote: > >> 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? > > Apparently it's not, I thought it was. When unwrapping the final inbound > Finished message around line 1069, it advances the internal state and writes > its own Finished message into the outbound queue, but doesn't actually return > hs=FINISHED until the outbound queue is drained. > > I had to step through it to confirm. So you can remove it. Done. > > Thanks, Vinnie, looks good. > > Brad >
