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

Bradford Wetmore bradford.wetmore at
Tue Dec 1 01:32:47 UTC 2015

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

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");

BTW, the indentions here at 331/332/334 are off.  The rest are all ok.

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.

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.

Same concern here as above.

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.


    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 
 > Maybe, we can add an option to honor server preference order in the 

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

Thanks, Vinnie, looks good.


More information about the security-dev mailing list