[9] RFR 8170282: Enable ALPN parameters to be supplied during the TLS handshake
Bradford Wetmore
bradford.wetmore at oracle.com
Fri Dec 16 01:15:24 UTC 2016
(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.wetmore at oracle.com
>> <mailto:bradford.wetmore at 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.Wetmore at oracle.com <mailto: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
>>>>
>>>>
>>>
>
More information about the security-dev
mailing list