Looks fine to me. Thanks for the update.
Xuelei
On 12/1/2015 7:08 AM, Vincent Ryan wrote:
> 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.
>>
>
