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

[Xuelei Fan]

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


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.

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(...);
    }

-----------
 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 ...");
+    }

---------
Personally, I would prefer to use unmodifiableList for the protocolNames
variable.


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.

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

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.