On 9/23/2015 2:33 AM, Simone Bordet wrote:
Hi,

On Wed, Sep 23, 2015 at 7:04 AM, Bradford Wetmore
<bradford.wetm...@oracle.com> wrote:

This new proposal still requires that ciphers are sorted in a way that
matches the ApplicationProtocol order.
Would be nice if, along with the HTTP/2 blacklist, there is a HTTP/2
comparator that sorts ciphers putting the blacklisted ones at the end.

Hm...is the sample code at the end of the initial class description
insufficient?  Adding a comparator seems a little heavyweight in that it
could require access to the ciphersuite internals and would add a lot of
complexity for this one known use case.  When TLSv1.3 is done, the blacklist
stuff in HTTP/2 goes away.

Sure, but until TLS 1.3 widely deployed, applications will have to
sort the ciphers to put HTTP/2 ones before the blacklisted ones.
Providing this comparator is as trivial as providing
ApplicationProtocol.HTTP2BLACKLIST, so I thought to mention it.

I learned something today: Collections/Arrays.sort()/others are stable. (i.e. equal elements will not be reordered as a result of the sort) My expectation of "equals" was .equals(), not return value == 0.

I was concerned that such a Comparator might reorder valid H2 suites from what was passed in. Thankfully, that's not the case, so I've added this Comparator. There is a warning now about "consistency with equals()", as the Strings obvioulsy won't be equals() and thus sorted sets/maps just won't work. (See the Comparator pages for further discussion.)

I also don't understand why there are 2 methods for the protocol name
? What value does it bring to have 2 methods for the same thing ?

Please see the IANA registry:

http://www.iana.org/assignments/tls-extensiontype-values/tls-extensiontype-values.xhtml#alpn-protocol-ids

for RFC 7301:

     http://www.rfc-editor.org/info/rfc7301

getProtocolName() is the IANA/IETF textual representation of the protocol
name (i.e. "Protocol" column), for example "HTTP/1.1", "SPDY/3", and "HTTP/2
over TLS".  I suppose toString() could be used instead, but thought it might
eventually output additional ALPN value state.  I don't have any concrete
plans at this point.

getNetworkSequence() is the identification sequence for the protocol (i.e.
"Identification Sequence" column), and represents the actual byte
identifiers that will travel the network in an ALPN extension.

     0x68 0x74 0x74 0x70 0x2f 0x31 0x2e 0x31 ("http/1.1")
     0x73 0x70 0x64 0x79 0x2f 0x33 ("spdy/3")
     0x68 0x32 ("h2")

When client wants to send the extension over the network, it grabs the
ApplicationProtocols values from the SSLParameters, then calls
getNetworkSequence() on each ApplicationProtocol to obtain the actual opaque
ProtocolName(1..2^8-1) to send.  Likewise on the server side, we match the
incoming active ALPN opaque values with the list of mutually agreeable ALPN
values.  And of course, send back the final selected value.

Sure, but application will have to implement two methods instead of
one, and AFAIU the JDK implementation is never calling
getProtocolName() since it's just a description for humans.

I think that a textual name will be better than:

    // Output:  javax.net.ssl.ApplicationProtocol$1@1b9e1916

    System.out.println(ApplicationProtocol.H2);

and there's no UTF-8 ambiguity.

I've updated the webrev to include an SSLSocket test variant, and added a
few more comments.

     http://cr.openjdk.java.net/~wetmore/8051498/webrev.14/

Hopefully things are more clear now.  Thanks for your review/comments.

I see now, thanks for the pointers !

Indulge me a bit more below on the Map passed as parameter to
ApplicationProtocol :)

IIUC, by the time we are executing the code that calls
ApplicationProtocol.match(), the TLS protocol is already chosen and
it's available in SSLSession.

Not necessarily. This could also be called before the connection has even been attempted, say if the client wants to determine if the proposed protocols or protocol/ciphersuites it wants to use are even allowed by an ApplicationProtocol.

When remains is the transient value of cipher that is being chosen.
Because we already have modified the API to support the application
protocol transient value (by adding
SSLEngine.getHandshakeApplicationProtocol()) to be used by
KeyManagers, I was wondering if we cannot either:

CipherSuite is more of a Session value, so it should probably be part of the handshakeSSLSession. We could set handshakeSSLSession.getCipherSuite() before calling the ALPN selector, or pass it in as part of the Map.

> A) add: String SSLEngine.getHandshakeCipherSuite(), to be used by
> ApplicationProtocol

That's kind of what we are doing already, but just in the ApplicationProtocol matcher instead so that it doesn't add extra methods to SSLSocket/SSLEngine.

And this doesn't really help the pre-connection situation where you want to query/filter out unacceptable values.

    http://cr.openjdk.java.net/~wetmore/8051498/webrev.15/

1.  New H2BLACKLISTCOMPARATOR

2.  Renamed the HTTP2BLACKLIST to H2BLACKLIST

Brad



Reply via email to