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.wetm...@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
>
>
