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

Vincent Ryan vincent.x.ryan at oracle.com
Tue Dec 1 21:20:59 UTC 2015


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



More information about the security-dev mailing list