ZOOKEEPER-4828: Honor ssl.context.supplier.class for client-server TLS - #2433
Merged
Conversation
ZOOKEEPER-4622 replaced the JDK SSLContext based Netty setup with SslContextBuilder, which builds key and trust managers directly from keyStore.location and trustStore.location. As a side effect the documented ssl.context.supplier.class property stopped being honored for the client-server protocol in 3.9.0: it is now only read by X509Util.createSSLContextAndOptions, which the Netty client and server no longer call. Users needing a custom SSLContext, such as a hardware key store or one obtained from a container, are forced to a file based key and trust store for any client-server TLS. Restore the property on both Netty paths by wrapping the supplied SSLContext in a Netty JdkSslContext, the same adapter ZooKeeper used before ZOOKEEPER-4622. Configured protocols, cipher suites, client auth and hostname verification are applied on top of the supplied context, so no new configuration is introduced and the quorum protocol keeps using the identical Supplier<SSLContext>. A supplied SSLContext cannot be used by the OpenSSL providers, which build their own native context, so combining it with a non JDK sslProvider is now rejected with a clear error instead of being silently ignored. Also clear the hostname verification system properties in the X509UtilTest teardown. They leaked between test methods and made context construction depend on test execution order.
Contributor
Author
|
@anmolnar Thanks for the review. JDK 17 passed while only JDK 25 failed on SaslAuthRequiredMultiClientTest.testClientOpWithInvalidSASLPasswordAuthAfterSuccessLogin, which doesn't seem to do any SSL handling so should be unaffected by this PR. This seems to be a flaky test case. How do we go forward here, should the build be re-triggered, or can it be merged eventually in this state? |
PDavid
approved these changes
Aug 10, 2026
PDavid
left a comment
PDavid
left a comment
Contributor
There was a problem hiding this comment.
Many thanks for fixing this, looks good to me. 👍
Comment on lines
+95
to
+96
| System.clearProperty(x509Util.getSslHostnameVerificationEnabledProperty()); | ||
| System.clearProperty(x509Util.getSslClientHostnameVerificationEnabledProperty()); |
Contributor
There was a problem hiding this comment.
Nice that you also fixed this, thanks!
Contributor
|
Merged to master. Thanks @SvenssonWeb ! |
Contributor
Author
|
Great, absolutely! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ZOOKEEPER-4622 (4a79427) replaced the JDK
SSLContextbased Netty setup withSslContextBuilder, which builds key/trust managers straight fromkeyStore.location/trustStore.location. As a side effect, the documentedssl.context.supplier.classproperty stopped being honored for the client-server protocol in 3.9.0:ClientCnxnSocketNettyandNettyServerCnxnFactorynever consult it, onlyFourLetterWordMainstill does. A file based key/trust store is now effectively mandatory for client-server TLS, silently breaking use of a PKCS11 key store or a container suppliedSSLContext.This restores option (2) from the JIRA discussion. The blocker recorded there was that quorum needs a
javax.net.ssl.SSLContextwhile the client needs anio.netty.handler.ssl.SslContext— Netty'sJdkSslContextbridges that, and is the same adapter ZooKeeper used before ZOOKEEPER-4622 removed it, so oneSupplier<SSLContext>still serves both protocols.X509Util.loadSuppliedSSLContext, reused bycreateNettySslContextForClient/ForServerbefore they fall back toSslContextBuilder.sslProvider, since OpenSSL builds its own native context.Also fixes a leaking system property in
X509UtilTest's teardown that made context construction depend on test order.cc @anmolnar @PDavid