I’ve made the change to use activated() rather than started() in SSLEngineImpl
and SSLSocketImpl
and also the suggestions for ServerHandshaker.
I’ve also updated the tests to check
SSLEngine/SSLSocket.getHandshakeApplicationProtocolSelector.
All tests pass.
Thanks for all the review comments.
> On 16 Dec 2016, at 01:15, Bradford Wetmore <bradford.wetm...@oracle.com>
> wrote:
>
> (Xuelei, question for you below)
>
> Hi Vinnie,
>
> There is no test yet for SSLEngine/SSLSocket:
>
> public BiFunction<SSLEngine, List<String>, String>
> getHandshakeApplicationProtocolSelector() {
>
> Tests are passing at 100% with latest jdk.patch.
>
>
> SSLSocketImpl.java
> ==================
> 2672: Sorry, but I think what you want here is:
>
> if ((handshaker != null) && !handshaker.started()) {
> ->
> if ((handshaker != null) && !handshaker.activated()) {
>
> Xuelei can confirm. BTW, I just filed:
>
> 8171337: Check for
> correct SSLEngineImpl/SSLSocketImpl.setSSLParameters
> handshaker update method
>
> as I think setSSLParameters may be using the wrong method.
>
>
> SSLEngineImpl.java
> ==================
> 2275: I misspoke earlier today. Please add a similar change that you made
> in SSLSocket (2671-2674).
>
> if ((handshaker != null) && !handshaker.activated()) {
>
>
> ServerHandshaker.java
> =====================
> 536: Minor nit: suggest renaming hasCallback to something a little more
> descriptive. By the time you drop 400 lines, you may have forgotten the
> variable meaning. hasAPCallback?
>
> 535: A comments describing your current logic would be nice.
>
> if (there is a callback) {
> use the callback
> } else {
> use the list
> }
>
> Rest looks good! Thanks.
>
> Brad
>
>
>
> On 12/15/2016 4:39 PM, Vincent Ryan wrote:
>> 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/
>>
>>
>>
>>> On 9 Dec 2016, at 23:27, Bradford Wetmore <bradford.wetm...@oracle.com
>>> <mailto: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 <mailto: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
>>>>>
>>>>>
>>>>
>>
