On 12/15/2016 5:15 PM, Bradford Wetmore 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.
More safe to use activated().

Xuelei

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




Reply via email to