Thanks Brad for those review comments. I’ve make some implementation changes and updated the existing ALPN tests. No public API changes.
A new webrev is available at: http://cr.openjdk.java.net/~vinnie/8170282/webrev.02/ <http://cr.openjdk.java.net/~vinnie/8170282/webrev.02/> > On 9 Dec 2016, at 23:27, Bradford Wetmore <bradford.wetm...@oracle.com> wrote: > > API looks good. > > SSLEngineImpl/SSLSocketImpl.java > ================================ > 212/229: I might suggest a more descriptive variable name, like > applicationSelector. "selector" is a bit ambiguous. > > 450/1379: > getHandshakeApplicationProtocolSelector()); > -> > selector); > > Xuelei wrote: > > > This method would work in server side only. You mention this point > > in the apiNote part. I'd like to spec this point in the beginning > > part. > > If you do something like this, then you need to be careful to mention > something like "application protocols such as ALPN would do this on the > server side..." NPN is the reverse, where the server offers the strings, and > the client selects. > > > and application developer know what kind of information can be > > retrieved from the handshake session reliably. > > Whatever you put in here, make sure it can be changed later in case we are > able revisit the selection mechanism. > > > The current application protocol selection scenarios looks like: > > In my previous response, I suggested a different approach where everything > ALPN is done together. I think it may be similar to your N1-4. > > I look forward to the ServerHandshaker change next week. > > Brad > > > On 12/9/2016 1:08 PM, Vincent Ryan wrote: >> 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 >>> >>> >>
