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");
}
BTW, the indentions here at 331/332/334 are off. The rest are all ok.
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.
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.
SSLSocketAlpnTest.java
======================
Same concern here as above.
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.
Thanks, Vinnie, looks good.
Brad
