[9] RFR 8170282: Enable ALPN parameters to be supplied during the TLS handshake

[Vincent Ryan]

Thanks for your detailed review Brad. I’ve generated a new webrev:
    http://cr.openjdk.java.net/~vinnie/8170282/webrev.01/



> On 9 Dec 2016, at 01:34, Bradford Wetmore <Bradford.Wetmore at oracle.com> wrote:
> 
> 
> Hi Vinnie,
> 
> On 12/8/2016 2:18 PM, Vincent Ryan wrote:
>> The Java Servlet Expect Group reported that they have identified a specific HTTP2 server use-case that cannot
>> be easily addressed using the existing ALPN APIs.
>> 
>> This changeset fixes that problem. It supports a new callback mechanism to allow TLS server applications
>> to set an application protocol during the TLS handshake. Specifically it allows the cipher suite chosen by the
>> TLS protocol implementation to be examined by the TLS server application before it sets the application protocol.
>> Additional TLS parameters are also available for inspection in the callback function.
>> 
>> This new mechanism is available only to TLS server applications. TLS clients will continue to use the existing ALPN APIs.
> 
> Technically, the API could be used for NPN (though it's pretty much dead), so that would be a listing the options on the server side, and selection on client side.
> 
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8170282
>> Webrev: http://cr.openjdk.java.net/~vinnie/8170282/webrev.00/
> 
> SSLEngineImpl.java/SSLSocketImpl.java
> =====================================
> Please use the standard handshaker initialization pattern where the Handshaker is initialized with all of the data/mechanisms needed for a complete handshake.  This will will ensure consistent handshake results and avoid potential strange race conditions.  (There's some corresponding API comments below.)
> 
> You would register your callback after the handshaker.setApplicationProtocols() calls at (currently) line 444 in the SSLEngineImpl code.
> 
> 
> SSLEngine.java/SSLSocket.java
> =============================
> I would suggest putting an introduction to this addition in the class description section, that application values can be set using SSLParameters.setApplication...() and selected with the default algorithm, or that a more accurate selection mechanism can be created by registering the callback that could look at any Handshake in progress and make appropriate decisions.
> 
> 1339:
> Registers the callback function that selects an application protocol
> value during the SSL/TLS/DTLS handshake.
> ->
> Registers a callback function that selects an application protocol
> value for a SSL/TLS/DTLS handshake.  The function overrides any values set using {@link SSLParameters#setApplicationProtocols SSLParameters.setApplicationProtocols}.
> 
> and remove the corresponding section from the return docs (in the {@code String section}).
> 
> the function's first argument enables the current
> handshake settings to be inspected.<br>
> ->
> the function's first argument allows the current SSLEngine(SSLSocket) to be inspected, including the handshake session and configuration settings.<br>
> 
> If null is returned, or a value that was not advertised
> then the underlying protocol will determine what action
> to take.
> (For example, ALPN will send a "no_application_protocol"
> alert and terminate the connection.)
> ->
> If the return value is null (no value chosen) or is a value that was not advertised by the peer, the underlying protocol will determine what action to take.  (For example, ALPN will send a "no_application_protocol" alert and terminate the connection.)
> 
> Also, TLS should be configured with parameters that
> ->
> Also, this SSLEngine(SSLSocket) should be configured with parameters that
> 
> @param selector the callback function, or null to de-register.
> ->
> @param selector the callback function, or null to disable the callback functionality.
> 
> Retrieves the callback function that selects an application protocol
> value during the SSL/TLS/DTLS handshake.
> ->
> Retrieves the callback function that selects an application protocol
> value during a SSL/TLS/DTLS handshake.
> 
>    This method should be called by TLS protocol implementations during
>    the TLS handshake. The callback function should not be called until
>    after the cipher suite has been selected.
> 
> I would suggest removing this apiNote entirely.  I expect only applications will call this method, so the first sentence is not necessary since it's up to the implementation how it wants to store the BiFunction.  I expect that when the handshaker is initialized, the current BiFunction will be assigned to it, and thus can't be changed for the current handshake/Handshaker in progress.  The second sentence ties an ordering to the selection of ciphersuite and ALPN value, and will tie our hands if we ever revisit the group protocol/ciphersuite/SNI/ALPN selection discussion.
> 
> ServerHandshaker.java
> =====================
> I was expecting that the ALPN callback logic would be an update for our current ALPN decision logic.  If a callback was set, use it, else look at defined strings from the SSLParameters, else fail.  e.g.
> 
>    ALPNExtension clientHelloALPN = (ALPNExtension)
>        mesg.extensions.get(ExtensionType.EXT_ALPN);
> 
>    if (clientHelloALPN != null) {
>        List<String> protocols = clientHelloALPN.getPeerAPs();
>        if (applicationSelector != null) {
>            applicationProtocol =
>                selector.apply(SSLEngine/SSLSocket, peerAPs);
>        } else if (localApl.length > 0)) {
>            // Intersect the requested and the locally supported,
>            // and save for later.  Use server preference order
>            for (String ap : localApl) {
>                ...deleted...
>            }
>            applicationProtocol = negotiatedValue;
>        }
>        if (negotiatedValue == null) {
>            fatalSE(Alerts.alert_no_application_protocol,
>                new SSLHandshakeException(
>                    "No matching ALPN values"));
>        }
>    }
> 
> Thanks.
> 
> Brad
> 
>