TLS ALPN Proposal

[Bradford Wetmore]

Thanks for the thorough reviews and comments, I really appreciate it and 
always learn something.  FunctionalInterface (@since 1.8) is something I 
haven't really explored yet, so off to the books.

I'm glad this ALPN approach seems worth pursing.  I have several 
different comments I'll combine into this single message.

On 5/22/2015 9:12 AM, Simone Bordet wrote:

>> ExtendedSSLSession:
>> -------------------
>> gets the negotiated protocol String
>
> If I understand correctly, SSLParameters will have methods that can
> only be used by one side of the TLS protocol (e.g. client or server,
> but not both).
> It's already like this for other things (e.g. SNI) so it will work,
> but I had hoped for some kind of reworking in this area too, but I'm
> digressing.

I don't have a better idea/suggestion for this, protocol advertisement 
and selection are two very different things.  At this point, I am 
thinking it should remain in SSLParameters.

I considered putting the selector into the SSLContext initialization:

     sslContext.init(KeyManager, TrustManager, SecureRandom, Selector);

but that means 1 selector for all SSLSockets/SSLEngines that get 
generated from that SSLContext.  That is constricting if the selector 
wanted to keep any connection-specific values.  It could go into 
SSLSocket/SSLEngine, but starting in JDK 6, we've been lumping all set* 
configuration parameters into SSLParameters, so that once configured, 
devs can reuse one SSLParameters object without having to reconstruct it 
(source code or at runtime) from scratch.

BTW, there was a previous question about when to use SSLSocket.set*() 
and sslSocket.setSSLParameters(params).  I gave a partial answer in:

 
http://mail.openjdk.java.net/pipermail/security-dev/2014-November/011430.html

I recently filed:

     https://bugs.openjdk.java.net/browse/JDK-8080799
     Provide guidance on repeated configuration parameter APIs

to address this.  I don't think we need to deprecate the set() methods, 
but having a note here will alleviate confusion.

>> /*
>>   * A callback class on the server side that is responsible for
>>   * choosing which Application Protocol should be used in a SSL/TLS
>>   * connection.
>>   */
>> public abstract class ApplicationProtocolSelector {
>>
>>      /*
>>       * SSLSocket callback to choose which Application Protocol value
>>       * to return to a SSL/TLS client
>>       *
>>       * @param sslSocket the SSLSocket for this connection
>>       * @param protocols the list of protocols advertised by the client
>>       * @param protocolVersion the negotiated protocol version

Egads, this is confusing.  protocols is the ALPN protocols, 
protocolVersion is the TLS version number.  I'll fix this.

>>       * @param ciphersuite the negotiated ciphersuite
>>       * @return a non-empty protocol String to send to the client,
>>       *     or null if no protocol value (i.e. extension) should be sent.
>>       * @throws SSLProtocolException if the connection should be aborted
>>       *     because the the server supports none of the protocols that
>>       *     the client advertised.

Whoops, I put exactly the Exception I didn't want to use!  I was 
originally thinking SSLHandshakeException.  See below for more comments.

>>       */
>>      public String select(SSLSocket sslSocket, String[] protocols,
>>              String protocolVersion, String ciphersuite)
>>              throws SSLProtocolException;
>
> We are currently getting the protocolVersion and the cipherSuite via:
>
> [sslSocket|sslEngine].getHandshakeSession().get[Protocol|CipherSuite]()
>
> If this is correct, perhaps there is no need to pass those 2
> parameters to select() ?

Good point.  Yes, those are no longer needed.

There was a suggestion to have access to the clientHello information in 
order to guide ciphersuite/protocol selection, however, there isn't a 
way to control that part of the internals at this time, so I didn't 
include it.

As an aside, if we do develop the Handshake API (other mail) and allow 
for handshake message modification (not currently proposed, but hinted 
at), the protocols/ciphersuites values could be adjusted before they are 
handled.  e.g. INBOUND callbacks would be triggered after the message 
has been parsed and added to the handshake hash calculation, but before 
clientHello() is actually called.

> I would suggest that if SSLParameters.setApplicationProtocols() takes
> a List<String>, then also select() should take a List<String>, rather
> than a String[].
> Since it's the server picking the protocols, would be handy to have
> the client protocols in a List<String> in order to call contains(),
> etc. on it.

Good points.

> I would suggest to throw SSLException rather than the too specific
> SSLProtocolException (which may also be misleading, since its javadoc
> hints at a "flaw in one of the protocol implementations", while in
> this case it is just a failure to negotiate an *application* protocol
> - perfectly fine from the point of view of the TLS protocol).

See above.  I'll change it to SSLException for now, but I think it 
probably should be SSLHandshakeException.

> Would be great if we could make this class a FunctionalInterface, but
> I guess it's not easy due to lack of commonality between SSLSocket and
> SSLEngine.
> Unless there is a way to abstract something out of those 2 :)

I'll look into Sean's suggest of SSLBase.

>> public class ExtendedSSLSession implements SSLSession {
>>
>>        ...deleted...
>>
>>        /**
>>         * Gets the application protocol negotiated for this connection.
>>         *
>>         * @returns the application protocol name or null if none was
>>         *     negotiated.
>>         */
>>        public String getApplicationProtocol() { };
>> }
>
> How would this case be covered:
>
> * client sends protocol list "foo","bar"
> * server sends back "bax".
> * client has to decide what to do.

Are you positive this was a valid mode of operation?  From RFC 7301:

    With ALPN, the client sends the list of supported application
    protocols as part of the TLS ClientHello message.  The server chooses
    a protocol and sends the selected protocol as part of the TLS
    ServerHello message.

This says to me that it needs to be one of the offered protocols. 
Granted, it's not explictly stated "it must be one of the offered", but 
allowing for that doesn't make sense to me.  Should the server really be 
allowed to select a protocol that the client didn't offer, and then 
realistically expect the client to support it?  If the server supported 
it, the client should have offered in the first place.

I was originally thinking that we should throw an Exception if the 
negotiated protocol is not one of those sent, but maybe that's better 
left to the application when it looks at the ALPN value after 
handshaking is completed.

> The only chance to call ExtendedSSLSession.getApplicationProtocol()
> would be *after* the handshake is completed, right ?

I think this point is moot, but to answer your question.

For SSLEngine, it could be done at any time.

For SSLSocket, if the connection needed to use the KeyManager(client 
auth)/TrustManager(server cert verification), then it could call during 
that point.

> Do you plan to hardcode the abort of the handshake with an alert_120,
> or give the application a chance (just asking) ?

Originally, yes.  To support what you suggest, we would need to add a 
level/description to the SSLException class.  I'd been debating that 
anyway, but this RFC was pretty clear that only 120 is needed.

>> There was also some internal discussion about whether the values should be
>> Strings or byte arrays.  The ALPN RFC only discusses bytes, and a
>> String.toBytes("US-ASCII") would limit the API to ASCII strings.
>>
>> Lastly, we could also add some convenience values for well-known values.
>> e.g.:
>>
>>      public static final AP_HTTP_1.1 = "http/1.1";
>>
>> or in byte form:
>>
>>      public static final AP_H2 = "h2".getBytes(""US-ASCII");
>>
>> I refrained from including SPDY/*, since they are on their way out now, and
>> NAT/STUN since there hasn't been any call for it so far.
>
> I agree that only http/1.1 and h2 deserve a constant.

Ok.

Sean wrote:

 > There's actually a bunch of methods in common, so one possibility is
 > to create a new interface containing the common methods (say SSLBase
 > for now for lack of a better name). Then you could change SSLEngine
 > and SSLSocket to implement SSLBase (which should be a compatible
 > change), and then have a single method on ApplicationProtocolSelector.

Worth looking into.  Thanks for the suggestion.

Simon wrote:

 > For HTTP/2, however, the choice of the protocol will depend on the TLS
 > version and the cipher, so we will need a way to obtain that
 > information.

Yes, that's why I originally included the negotiated 
protocol/ciphersuite (er...soon to be sslBase.getHandshakeSession()). 
HTTP/2 must be TLSv1.2 and the ciphersuite must not be on the blacklist.

 > Typically, in the code where you pass the ApplicationProtocolSelector
 > you have the SSLEngine available, e.g.:
 >
 > sslEngine.getSSLParameters().setApplicationProtocolSelector(new
 > ApplicationProtocolSelector()
 > {
 >    public String select(List<String> clientProtocols)
 >    {
 >        // Here sslEngine will be in lexical scope
 >    }
 > });
 >
 > In this way, ApplicationProtocolSelector won't depend on SSLEngine or
 > SSLSocket, and can be reduced to a FunctionalInterface.

But during the callback, does the application have access to the SSLEngine?

Unless I've haven't grasped how FunctionalInterfaces would have access 
to SSLEngine/SSLSocket objects here (admittedly I'm flying a little 
blind here), the application might want to make the protocol decision 
based on other factors such as network origin.  Our original 
X509TrustManager didn't have access to the Socket/Engine, and it was 
commonly requested.

 > RFC 7301 hints that protocol string could be UTF-8 encoded:
 > http://tools.ietf.org/html/rfc7301#section-6

Thanks for the pointer.  We could add to the javadoc that it will be 
handled as UTF-8?

Weijun wrote:

 > But in the RFC the name is in uppercase and chars in string are all
 > lowercases.
 > ...deleted...
 > - Compare with equalsIgnoreCase()

Not following here, the spec is specific about the over-the-wire byte 
values, and http/1.1 != Http/1.1.

The conversion inside SSLParameters could be done as:

     String s = new String(ba, "UTF-8");
     byte[] ba = s.getBytes("UTF-8");

I would love any thoughts on the Handshake API approach, but understand 
it's a lot to parse, and ALPN should take priority for the immediate future.

I should have an update out early next week, and hopefully can move this 
JEP to "Proposed to Target" if there are no other major comments.

Thanks again,

Brad