Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Missing peer host and port info in SSLEngine for server SslHandler #5290

Closed
ben1222 opened this issue Aug 29, 2024 · 6 comments · Fixed by #5347
Closed

Missing peer host and port info in SSLEngine for server SslHandler #5290

ben1222 opened this issue Aug 29, 2024 · 6 comments · Fixed by #5347
Labels
Milestone

Comments

@ben1222
Copy link
Contributor

ben1222 commented Aug 29, 2024

Version

4.4.9

Context

We have a customized key manager that extends X509ExtendedKeyManager that want to override the public String chooseEngineServerAlias(String keyType, Principal[] issuers, SSLEngine engine) method to choose the server alias partly depending on the peer host address.
However, the engine.getPeerHost() always returns null.

After read related code, I find that netty SslContext.newHandler do support passing in an advisory peer information of peer host and port.
However, in vert.x, when creating SslHandler for server in SslChannelProvider, the peer host and port info is not passed to SslContext.newHandler, result in null for engine.getPeerHost() in X509ExtendedKeyManager.chooseEngineServerAlias.
(The SslChannelProvider do provide peer host and port info when creating client SslHandler)

I tried to pass the peer host and port info from HttpServerWorker to SslChannelProvider.createServerHandler and find the peer host and port are available in the SSLEngine in X509ExtendedKeyManager.chooseEngineServerAlias:

diff --git a/src/main/java/io/vertx/core/http/impl/HttpServerWorker.java b/src/main/java/io/vertx/core/http/impl/HttpServerWorker.java
index cf37c4e8b..82402c858 100644
--- a/src/main/java/io/vertx/core/http/impl/HttpServerWorker.java
+++ b/src/main/java/io/vertx/core/http/impl/HttpServerWorker.java
@@ -35,6 +35,8 @@ import io.vertx.core.impl.VertxInternal;
import io.vertx.core.net.impl.*;
import io.vertx.core.spi.metrics.HttpServerMetrics;

+import java.net.InetSocketAddress;
+import java.net.SocketAddress;
import java.nio.charset.StandardCharsets;
import java.util.List;
import java.util.function.BiConsumer;
@@ -131,7 +133,12 @@ public class HttpServerWorker implements BiConsumer<Channel, SslChannelProvider>
   private void configurePipeline(Channel ch, SslChannelProvider sslChannelProvider) {
     ChannelPipeline pipeline = ch.pipeline();
     if (options.isSsl()) {
-      pipeline.addLast("ssl", sslChannelProvider.createServerHandler());
+      SocketAddress remoteAddress = ch.remoteAddress();
+      if (remoteAddress instanceof InetSocketAddress) {
+        pipeline.addLast("ssl", sslChannelProvider.createServerHandler(((InetSocketAddress) remoteAddress).getHostString(), ((InetSocketAddress) remoteAddress).getPort()));
+      } else {
+        pipeline.addLast("ssl", sslChannelProvider.createServerHandler());
+      }
       ChannelPromise p = ch.newPromise();
       pipeline.addLast("handshaker", new SslHandshakeCompletionHandler(p));
       p.addListener(future -> {
diff --git a/src/main/java/io/vertx/core/net/impl/SslChannelProvider.java b/src/main/java/io/vertx/core/net/impl/SslChannelProvider.java
index 290bf8c23..cb5aba5d1 100644
--- a/src/main/java/io/vertx/core/net/impl/SslChannelProvider.java
+++ b/src/main/java/io/vertx/core/net/impl/SslChannelProvider.java
@@ -144,17 +144,21 @@ public class SslChannelProvider {
   }

   public ChannelHandler createServerHandler() {
+    return createServerHandler(null, -1);
+  }
+
+  public ChannelHandler createServerHandler(String peerHost, int peerPort) {
     if (sni) {
       return createSniHandler();
     } else {
-      return createServerSslHandler(useAlpn);
+      return createServerSslHandler(useAlpn, peerHost, peerPort);
     }
   }

-  private SslHandler createServerSslHandler(boolean useAlpn) {
+  private SslHandler createServerSslHandler(boolean useAlpn, String peerHost, int peerPort) {
     SslContext sslContext = sslServerContext(useAlpn);
     Executor delegatedTaskExec = useWorkerPool ? workerPool : ImmediateExecutor.INSTANCE;
-    SslHandler sslHandler = sslContext.newHandler(ByteBufAllocator.DEFAULT, delegatedTaskExec);
+    SslHandler sslHandler = sslContext.newHandler(ByteBufAllocator.DEFAULT, peerHost, peerPort, delegatedTaskExec);
     sslHandler.setHandshakeTimeout(sslHandshakeTimeout, sslHandshakeTimeoutUnit);
     return sslHandler;
   }

There are a few other places calling SslChannelProvider.createServerHandler so although this change works in my use case, a more complete fix may be needed.

@ben1222 ben1222 added the bug label Aug 29, 2024
@vietj
Copy link
Member

vietj commented Aug 29, 2024

@ben1222 can you provide a reproducer for this using the vertx tests so we are covered, that would help

@ben1222
Copy link
Contributor Author

ben1222 commented Aug 30, 2024

@vietj I tried to create a unit test under Http1xTLSTest:

public class Http1xTLSTest extends HttpTLSTest {
  private static final Logger LOG = LogManager.getLogger(Http1xTLSTest.class);

  @Test
  public void testTLSServerSSLEnginePeerHost() throws Exception {
    testTLS(Cert.NONE, Trust.SERVER_JKS, () -> {
      try {
        return KeyCertOptions.wrap(new MyKeyManager((X509KeyManager) Cert.SERVER_JKS.get().getKeyManagerFactory(vertx).getKeyManagers()[0]));
      } catch (Exception e) {
        throw new RuntimeException(e);
      }
    }, Trust.NONE).pass();
  }


  private static class MyKeyManager extends X509ExtendedKeyManager {
    private final X509KeyManager wrapped;

    MyKeyManager(X509KeyManager wrapped) {
      this.wrapped = wrapped;
    }

    @Override
    public String chooseEngineClientAlias(String[] keyType, Principal[] issuers, SSLEngine engine) {
      throw new UnsupportedOperationException();
    }

    @Override
    public String chooseEngineServerAlias(String keyType, Principal[] issuers, SSLEngine engine) {
      LOG.info("In chooseEngineServerAlias, keyType: {}, issuers: {}, peer host: {}, peer port: {}",
        keyType, issuers, engine.getPeerHost(), engine.getPeerPort());
      if (engine.getPeerHost() == null || engine.getPeerPort() == -1) {
        throw new RuntimeException("Missing peer host/port");
      }
      return wrapped.chooseServerAlias(keyType, issuers, null);
    }

    @Override
    public String chooseClientAlias(String[] keyType, Principal[] issuers, Socket socket) {
      throw new UnsupportedOperationException();
    }

    @Override
    public String chooseServerAlias(String keyType, Principal[] issuers, Socket socket) {
      throw new UnsupportedOperationException();
    }

    @Override
    public String[] getClientAliases(String s, Principal[] principals) {
      throw new UnsupportedOperationException();
    }

    @Override
    public String[] getServerAliases(String s, Principal[] principals) {
      return wrapped.getServerAliases(s, principals);
    }

    @Override
    public X509Certificate[] getCertificateChain(String s) {
      LOG.info("In getCertificateChain, s: {}", s);
      return wrapped.getCertificateChain(s);
    }

    @Override
    public PrivateKey getPrivateKey(String s) {
      LOG.info("In getPrivateKey, s: {}", s);
      return wrapped.getPrivateKey(s);
    }
  }

//...
}

Currently it will fail with:

Starting test: Http1xTLSTest#testTLSServerSSLEnginePeerHost 
2024-08-30 00:02:25,606 [vert.x-eventloop-thread-2] INFO io.vertx.core.http.Http1xTLSTest Http1xTLSTest$MyKeyManager.chooseEngineServerAlias:296 null - In chooseEngineServerAlias, keyType: EC, issuers: null, peer host: null, peer port: -1
java.lang.RuntimeException: Missing peer host/port
	at io.vertx.core.http.Http1xTLSTest$MyKeyManager.chooseEngineServerAlias(Http1xTLSTest.java:299)
	at java.base/sun.security.ssl.X509Authentication$X509PossessionGenerator.createServerPossession(X509Authentication.java:293)
	at java.base/sun.security.ssl.X509Authentication$X509PossessionGenerator.createPossession(X509Authentication.java:214)
	at java.base/sun.security.ssl.X509Authentication.createPossession(X509Authentication.java:90)
	at java.base/sun.security.ssl.CertificateMessage$T13CertificateProducer.choosePossession(CertificateMessage.java:1081)
	at java.base/sun.security.ssl.CertificateMessage$T13CertificateProducer.onProduceCertificate(CertificateMessage.java:970)
	at java.base/sun.security.ssl.CertificateMessage$T13CertificateProducer.produce(CertificateMessage.java:961)
	at java.base/sun.security.ssl.SSLHandshake.produce(SSLHandshake.java:436)
...
java.lang.AssertionError: Should not fail Failed to create SSL connection
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.junit.Assert.assertFalse(Assert.java:65)
	at io.vertx.test.core.AsyncTestBase.assertFalse(AsyncTestBase.java:259)
	at io.vertx.core.http.HttpTLSTest.access$300(HttpTLSTest.java:68)
	at io.vertx.core.http.HttpTLSTest$TLSTest.lambda$run$10(HttpTLSTest.java:1312)
	at io.vertx.core.impl.future.FutureImpl$2.onFailure(FutureImpl.java:117)
...

With the changes in HttpServerWorker and SslChannelProvider, it succeeds:

Starting test: Http1xTLSTest#testTLSServerSSLEnginePeerHost 
2024-08-30 00:10:35,108 [vert.x-eventloop-thread-2] INFO io.vertx.core.http.Http1xTLSTest Http1xTLSTest$MyKeyManager.chooseEngineServerAlias:294 null - In chooseEngineServerAlias, keyType: EC, issuers: null, peer host: 127.0.0.1, peer port: 48470
2024-08-30 00:10:35,111 [vert.x-eventloop-thread-2] INFO io.vertx.core.http.Http1xTLSTest Http1xTLSTest$MyKeyManager.chooseEngineServerAlias:294 null - In chooseEngineServerAlias, keyType: EC, issuers: null, peer host: 127.0.0.1, peer port: 48470
2024-08-30 00:10:35,112 [vert.x-eventloop-thread-2] INFO io.vertx.core.http.Http1xTLSTest Http1xTLSTest$MyKeyManager.chooseEngineServerAlias:294 null - In chooseEngineServerAlias, keyType: EC, issuers: null, peer host: 127.0.0.1, peer port: 48470
2024-08-30 00:10:35,112 [vert.x-eventloop-thread-2] INFO io.vertx.core.http.Http1xTLSTest Http1xTLSTest$MyKeyManager.chooseEngineServerAlias:294 null - In chooseEngineServerAlias, keyType: RSA, issuers: null, peer host: 127.0.0.1, peer port: 48470
2024-08-30 00:10:35,112 [vert.x-eventloop-thread-2] INFO io.vertx.core.http.Http1xTLSTest Http1xTLSTest$MyKeyManager.getPrivateKey:330 null - In getPrivateKey, s: test-store
2024-08-30 00:10:35,113 [vert.x-eventloop-thread-2] INFO io.vertx.core.http.Http1xTLSTest Http1xTLSTest$MyKeyManager.getCertificateChain:324 null - In getCertificateChain, s: test-store

@vietj
Copy link
Member

vietj commented Aug 30, 2024

do you mind contributing a pull request to the 4.x branch and master branch ?

@ben1222
Copy link
Contributor Author

ben1222 commented Sep 3, 2024

@vietj I can have a try. Do I need to go through some process before sending the pull request? I see the contributing guideline mentioned about signing ECA?
Since there are a few other places (the NetServerImpl, NetSocketImpl) calling SslChannelProvider.createServerHandler, I'll try to also update them to pass the peer host info to SslChannelProvider.createServerHandler, is that ok?

@vietj
Copy link
Member

vietj commented Sep 4, 2024

you should sign the Eclipse Agreement indeed

everything should be updated and tested in master and 4.x branches

@ben1222
Copy link
Contributor Author

ben1222 commented Oct 8, 2024

@vietj I signed ECA and opened pull requests on 4.x branch (#5346) and master branch (#5347), please review.

@vietj vietj added this to the 4.5.11 milestone Oct 24, 2024
@vietj vietj closed this as completed Oct 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants