Thanks for your review. Comments in-line.
> On 30 Nov 2015, at 06:30, Xuelei Fan <xuelei....@oracle.com> wrote:
>
> 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 …
>
Done.
>
> 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.
I added a comment to show that server-preference is used.
If necessary I think we can add support for controlling this via a property,
later.
>
> 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(...);
> }
I added the exception and more detailed message.
>
> -----------
> 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 ...");
> + }
Done.
>
> ---------
> Personally, I would prefer to use unmodifiableList for the protocolNames
> variable.
It is accessible only to classes in this package.
>
>
> 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.
I concatenated the final sentence.
>
> * 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.
My understanding is the the wrapping is performed here and the unwrapping
performed in processInputRecord(). Isn’t the assignment required in both places?
>
> 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.
>
