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

SSL improvements #4468

Merged
merged 26 commits into from
Sep 14, 2022
Merged

SSL improvements #4468

merged 26 commits into from
Sep 14, 2022

Conversation

vietj
Copy link
Member

@vietj vietj commented Aug 30, 2022

A set of improvements for SSL handling

  • SSL configuration is performed
    • on a worker thread instead of event-loop thread
    • lazily in NetClient as validation is asynchronous
  • SslContext is now delegated to an SPI to enable alternative way to create an SslContext (e.g reuse an existing SSLContext) provided on the SSLEngineOptions classes
  • remove the suppliedSSLContext temporary machinery in favor of the SSLEngineOptions configuration mechanism
  • ssl handler (SslHandler or SniHandler) is now created by the SSLHelper instead of the pipeline setup
  • internal cleanups

The SPI will be documented in the Vert.x Advanced Guide

Thanks @MikeEdgar for the SSL blocking contribution

@vietj vietj added this to the 4.3.4 milestone Aug 30, 2022
@vietj vietj self-assigned this Aug 30, 2022
@vietj
Copy link
Member Author

vietj commented Aug 30, 2022

@kabir have a look, it changes the temporary supplied SSLContext machinery for a cleaner API, NetTest#testNetClientInternalTLS shows how the API can be used for NetClient. I will provide a PR for vertx-proton for this matter.

@kabir
Copy link
Contributor

kabir commented Aug 31, 2022

@vietj Looks good. My only concern is what the timeframe is to get this in? Since I have some work further down the chain (in SR Reactive Messaging) which needs merging/adjusting.

CC @cescoffier @ozangunalp

@vietj
Copy link
Member Author

vietj commented Aug 31, 2022 via email

@kabir
Copy link
Contributor

kabir commented Aug 31, 2022

@vietj ok, great - the needed work in SmallRye Reactive Messaging will need updating. But since that isn't merged yet, assuming this comes soon-ish, I can update that PR.

@MikeEdgar
Copy link

@vietj - this looks good to me as it relates to issue #4439. I no longer see the blocked threads during application startup with this branch.

@vietj vietj marked this pull request as ready for review September 2, 2022 09:30
@vietj
Copy link
Member Author

vietj commented Sep 5, 2022

@kabir this is I think the most up to date PR for the feature

Now the vertx-proton and vertx-amqp-client changes are not necessary anymore as the SSLContext can be configured from the SSLEngineOptions, e.g

    AmqpClientOptions options = new AmqpClientOptions()
      .setSsl(true)
      .setHost("localhost")
      .setPort(server.actualPort())
      .setSslEngineOptions(new JdkSSLEngineOptions() {
        @Override
        public SslContextFactory sslContextFactory() {
          return new SslContextFactory() {
            @Override
            public SslContext create() throws SSLException {
              return new JdkSslContext(
                sslContext,
                true,
                null,
                IdentityCipherSuiteFilter.INSTANCE,
                ApplicationProtocolConfig.DISABLED,
                io.netty.handler.ssl.ClientAuth.NONE,
                null,
                false);
            }
          };
        }
      });

    AmqpClient client = AmqpClient.create(vertx, options);

After this is merged, I'll revert the changes in vertx-proton and vertx-amqp-client (but I'll keep the SSL tests).

@kabir
Copy link
Contributor

kabir commented Sep 5, 2022

Thanks @vietj - the changes look good to me

Copy link
Contributor

@InfoSec812 InfoSec812 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some comments from my perspective as a user of the toolkit who is trying to learn more about the internals.

@@ -226,6 +212,26 @@ private void connect(SocketAddress remoteAddress, String serverName, Promise<Net
}

public void connectInternal(ProxyOptions proxyOptions,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be helpful to add JavaDoc to this method and the next?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do

}

private synchronized io.netty.util.concurrent.Future<Channel> listen(SocketAddress localAddress, ContextInternal context) {
private synchronized Future<Channel> listen(SocketAddress localAddress, ContextInternal context) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am having issues tracing through this method and understanding the reasoning. Any chance of adding comments or decomposing into smaller methods?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add comments

Copy link
Contributor

@InfoSec812 InfoSec812 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the additional comments and documentation @vietj - Looks good!

@vietj vietj linked an issue Sep 9, 2022 that may be closed by this pull request
Copy link
Contributor

@tsegismont tsegismont left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vietj great job

@vietj vietj merged commit aa4a1ce into master Sep 14, 2022
@vietj vietj deleted the ssl-helper-rework branch September 14, 2022 07:46
tsegismont added a commit to tsegismont/vertx-sql-client that referenced this pull request Sep 15, 2022
Signed-off-by: Thomas Segismont <tsegismont@gmail.com>
tsegismont added a commit to tsegismont/vertx-sql-client that referenced this pull request Sep 16, 2022
Signed-off-by: Thomas Segismont <tsegismont@gmail.com>
tsegismont added a commit to tsegismont/vertx-sql-client that referenced this pull request Sep 16, 2022
Signed-off-by: Thomas Segismont <tsegismont@gmail.com>
tsegismont added a commit to eclipse-vertx/vertx-sql-client that referenced this pull request Sep 16, 2022
Signed-off-by: Thomas Segismont <tsegismont@gmail.com>

Signed-off-by: Thomas Segismont <tsegismont@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Noisy logging from TCPServerBase#listen during startup
5 participants