Hey Vinnie,
question on SSLParameters.setApplicationProtocols(String[] protocols) method
What happens if you pass an empty array into this method. Shouldn't it
throw an IllegalArgumentException ?
====
In ALPNExtension.java :
+ if (listLength < 2 || listLength + 2 != len) {
+ throw new SSLProtocolException(
+ "Invalid " + type + " extension: incorrect list length");
Can you print the listLength value in exception message ?
+ throw new SSLProtocolException(
+ "Invalid " + type + " extension: too short");
Can you print the len value here ?
+ if (remaining != 0) {
+ throw new SSLProtocolException(
+ "Invalid " + type + " extension: extra data");
Can you print remaining value here ?
Regards,
Sean.
On 01/12/2015 01:53, Xuelei Fan wrote:
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.
