Add host and port to SSLEngineFactory #527

wsargent · 2014-04-08T01:40:46Z

Fixes #513

… host and port

slandelle · 2014-04-08T07:59:09Z

providers/netty/src/main/java/org/asynchttpclient/providers/netty/NettyAsyncHttpProvider.java

@@ -72,6 +72,9 @@ public void close() {

    @Override
    public <T> ListenableFuture<T> execute(Request request, final AsyncHandler<T> asyncHandler) throws IOException {
+        final URI uri = request.getURI();
+        channels.configureProcessor(requestSender, closed, uri.getHost(), uri.getPort());


Are you sure this is right? uri.getPort will return -1 if using the scheme default port. I think you should be using AsyncHttpProviderUtils.getHost/getPort

wsargent · 2014-04-08T18:33:17Z

Good catch. Fixing the host and port options.

wsargent · 2014-04-08T18:34:50Z

Grizzly looks like a harder fix: the relevant code for creating an SSLEngine is in SSLEngineConfigurator.createSSLEngine() but it does not take a peerName or peerPort, and is called from SSLBaseFilter.handleRead inside a couple of blocks of code.

wsargent · 2014-04-08T18:44:54Z

What we CAN do is make sure that a correctly configured SSLEngine is already in place by the time it does a handleRead:

    @Override
    public NextAction handleRead(final FilterChainContext ctx)
    throws IOException {
        final Connection connection = ctx.getConnection();
        final SSLConnectionContext sslCtx = obtainSslConnectionContext(connection);
        SSLEngine sslEngine = sslCtx.getSslEngine();

by ensuring SSL_CTX_ATTR is already set:


    static SSLConnectionContext obtainSslConnectionContext(
            final Connection connection) {
        SSLConnectionContext sslCtx = SSL_CTX_ATTR.get(connection);
        if (sslCtx == null) {
            sslCtx = new SSLConnectionContext(connection);
            SSL_CTX_ATTR.set(connection, sslCtx);
        }

        return sslCtx;
    }

slandelle · 2014-04-08T18:51:59Z

I'm completely clueless on the Grizzly side.
If you can't fix it, I say we leave it as is, maybe disable the failing tests on the grizzly side, and open issues so they get tracked and someone with Grizzly skills can fix them.

…t problem)

slandelle · 2014-04-08T19:09:50Z

@wsargent I just chatted with @jfarcand : let's do as I suggested: disable failing Grizzly tests and open new issues targeting only Grizzly and documenting what you've done so far. Are you fine with that?

oleksiys · 2014-04-08T19:16:43Z

I'm not very familiar with Grizzly AHC code, but IMO the following can work:

extend SSLEngineConfigurator passed to the SSLFilter with additional
createSSLEngine(String, int) method
override SSLFilter.handshake method to call the new
SSLEngineConfigurator.createSSLEngine(String, int) method, created on the
1st step.

WBR,
Alexey.

wsargent · 2014-04-08T19:22:34Z

@oleksiys I'll give that a shot.

wsargent · 2014-04-08T19:35:05Z

@oleksiys Doesn't quite work -- the handshakeContextAttr is private and final to SSLFilter. I might have to replace it with something extending from SSLBaseFilter.

oleksiys · 2014-04-08T19:39:33Z

let me check, are you working on 1.8.x branch?

wsargent · 2014-04-08T19:40:54Z

@oleksiys No, this is on master. When it works on master, then can maybe think about backporting it, but not until then.

oleksiys · 2014-04-08T20:13:04Z

@wsargent check this gist https://gist.github.com/anonymous/cc11a4806512c3be3c98. Please let me know if it works for you. thanks!

slandelle · 2014-04-11T07:34:28Z

@wsargent Just saw your tweet. Did you fork AHC (as PR are not merged yet)? WDYT about @oleksiys 's suggestion? Do you want to give it a try, or do you want me to merge what you've done so for only for the Netty provider?

wsargent · 2014-04-11T17:34:26Z

I have not forked

playframework/playframework#2229 got merged into playframework:master, and should be in Play 2.3-M2. Play 2.3 is not out yet, so there's still time to upgrade / patch etc.. It is still using AHC-1.8.x right now.

I've had my hands full writing up better documentation and covering the last few holes. I saw @oleksiys's suggestion but haven't had time to look at it.

If you want to give it a try, YES, please let me know how it works.

I have detailed how to test hostname verification here: http://tersesystems.com/2014/03/31/testing-hostname-verification/

Note that running the AHC tests for me still breaks unless I apply the SSL handshake listener fix in #525

slandelle · 2014-04-11T18:47:24Z

If you want to give it a try, YES, please let me know how it works.

No way :) I also have my hands full and have to focus on the Netty provider.
I'm going to do as I suggested: pick your changes for the Netty provider. Maybe @oleksiys will fix the Grizzly part.

slandelle · 2014-04-11T19:13:25Z

This PR conflicts with #526. Fun...

slandelle · 2014-04-11T19:36:12Z

@oleksiys I have a problem with your patch. Where do HOST and PORT come from? https://gist.github.com/anonymous/cc11a4806512c3be3c98#file-gistfile1-java-L99

slandelle · 2014-04-11T20:02:55Z

@oleksiys I chatted with @rlubke and he told me to try:

InetSocketAddress peerAddress = (InetSocketAddress) connection.getPeerAddress();
String host = peerAddress.getHostString();
int port = peerAddress.getPort();
sslEngine = ((HostPortAwareSSLEngineConfigurator) sslEngineConfigurator)
        .createSSLEngine(host, port);

Sadly, tests stil fail. :(

slandelle · 2014-04-11T20:10:20Z

@wsargent If I apply your patches, I have tons of tests failing on the Netty side too.

oleksiys · 2014-04-11T21:26:58Z

@slandelle Ryan's suggestion sounds reasonable. Not sure why the test is failing. Can I ask you to put these changes on some branch, so I can reproduce the failure and work on it?

Thank you!

slandelle · 2014-04-11T21:34:24Z

@oleksiys Thanks a lot for getting in touch. I've just pushed a branch named ssl-wip with the changes proposed by @wsargent and yours. Note that the tests also fail on the Netty provider, so I can't tell what's wrong (the proposed changes, the tests?).

Cheers!

wsargent · 2014-04-11T21:42:03Z

Try the SSL handshake listener fix in
#525 to fix the
tests. If the hostname verification happens before the SSL handshake, you
will get sslSession.isValid() == false.

slandelle · 2014-04-11T21:46:01Z

@wsargent I've already merged #525, both on master and on the ssl-wip branches!

oleksiys · 2014-04-21T22:42:12Z

hi guys,
do the tests work now?
On my local machine I made them work (both Grizzly and Netty) by reverting (reworking) SslUtils changes (0347bec), the diff is here [1].

[1] https://gist.github.com/oleksiys/baa7ca013f35cc9541b2

wsargent · 2014-04-21T23:38:06Z

Were you able to fix the test by merging

#525

?

oleksiys · 2014-04-22T00:26:38Z

I haven't checked that, but slandelle said he merged #525, but the test issue was still there.

slandelle · 2014-06-13T11:15:44Z

providers/netty/src/main/java/org/asynchttpclient/providers/netty/NettyAsyncHttpProvider.java

+        final URI uri = request.getURI();
+        final String host = AsyncHttpProviderUtils.getHost(uri);
+        final int port = AsyncHttpProviderUtils.getPort(uri);
+        channels.configureProcessor(requestSender, closed, host, port);


So you're reconfiguring the bootstraps' ChannelInitializer on EVERY request?!

slandelle · 2014-06-13T11:56:23Z

I checked the Netty part and it seems very racy and inefficient to me.
You're creating a new ChannelInitializer for the 4 bootstraps on every request. But if you try to send 2 requests to differents (host, port) from different threads, there's a chance you end up with the wrong SSLEngine.

I think the only solution, is to have a custom non shareable handler that replaces itself with a properly configured SslHandler.

@wsargent WDYT?

) ### Motivation With Xcode 13.2, and therefore Swift 5.5.2, Swift Concurrecy is supported on older Apple OSs. async/await suport will no longer be available on Swift before `5.5.2` but this isn't a breaking change because we have not yet made anything of it public. ### Changes - replace all `#if compiler(>=5.5) && canImport(_Concurrency)` with `#if compiler(>=5.5.2) && canImport(_Concurrency)` - replace all `available(macOS 12.0, iOS 15.0, watchOS 8.0, tvOS 15.0, *)` with `available(macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0, *)`

Add host and port to SSLEngineFactory, change the providers to accept…

a0562d0

… host and port

slandelle reviewed Apr 8, 2014
View reviewed changes

Use AsyncHttpProviderUtils.getHost/getPort (avoids -1 for default por…

17f7956

…t problem)

slandelle reviewed Jun 13, 2014
View reviewed changes

slandelle added Enhancement labels Jun 13, 2014

slandelle closed this in 4beb9c8 Jun 13, 2014

slandelle removed Enhancement labels Jun 13, 2014

wsargent deleted the fix-513 branch August 5, 2014 13:03

wsargent mentioned this pull request Aug 5, 2014

SSL/TLS: Add client-side certificate verification to akka-remote akka/akka#13874

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add host and port to SSLEngineFactory #527

Add host and port to SSLEngineFactory #527

wsargent commented Apr 8, 2014

slandelle Apr 8, 2014

wsargent commented Apr 8, 2014

wsargent commented Apr 8, 2014

wsargent commented Apr 8, 2014

slandelle commented Apr 8, 2014

slandelle commented Apr 8, 2014

oleksiys commented Apr 8, 2014

wsargent commented Apr 8, 2014

wsargent commented Apr 8, 2014

oleksiys commented Apr 8, 2014

wsargent commented Apr 8, 2014

oleksiys commented Apr 8, 2014

slandelle commented Apr 11, 2014

wsargent commented Apr 11, 2014

slandelle commented Apr 11, 2014

slandelle commented Apr 11, 2014

slandelle commented Apr 11, 2014

slandelle commented Apr 11, 2014

slandelle commented Apr 11, 2014

oleksiys commented Apr 11, 2014

slandelle commented Apr 11, 2014

wsargent commented Apr 11, 2014

slandelle commented Apr 11, 2014

oleksiys commented Apr 21, 2014

wsargent commented Apr 21, 2014

oleksiys commented Apr 22, 2014

slandelle Jun 13, 2014

slandelle commented Jun 13, 2014

Add host and port to SSLEngineFactory #527

Add host and port to SSLEngineFactory #527

Conversation

wsargent commented Apr 8, 2014

slandelle Apr 8, 2014

Choose a reason for hiding this comment

wsargent commented Apr 8, 2014

wsargent commented Apr 8, 2014

wsargent commented Apr 8, 2014

slandelle commented Apr 8, 2014

slandelle commented Apr 8, 2014

oleksiys commented Apr 8, 2014

wsargent commented Apr 8, 2014

wsargent commented Apr 8, 2014

oleksiys commented Apr 8, 2014

wsargent commented Apr 8, 2014

oleksiys commented Apr 8, 2014

slandelle commented Apr 11, 2014

wsargent commented Apr 11, 2014

slandelle commented Apr 11, 2014

slandelle commented Apr 11, 2014

slandelle commented Apr 11, 2014

slandelle commented Apr 11, 2014

slandelle commented Apr 11, 2014

oleksiys commented Apr 11, 2014

slandelle commented Apr 11, 2014

wsargent commented Apr 11, 2014

slandelle commented Apr 11, 2014

oleksiys commented Apr 21, 2014

wsargent commented Apr 21, 2014

oleksiys commented Apr 22, 2014

slandelle Jun 13, 2014

Choose a reason for hiding this comment

slandelle commented Jun 13, 2014