ZOOKEEPER-5045: Fall back to TLSv1.2 default in FIPS mode by PDavid · Pull Request #2380 · apache/zookeeper

PDavid · 2026-05-04T12:29:26Z

No description provided.

anmolnar

@PDavid Thanks for the patch. Overall lgtm. Just a few comments.

Tests when the default truststore of the JVM (javax.net.ssl.trustStore) points to a nonexistent file.

PDavid · 2026-05-05T15:13:08Z

Hmm, X509UtilTest.testCreateSSLContext* tests failed in the PR build. I'll have a look.

anmolnar · 2026-05-05T22:03:07Z

How about the following?
No static field needed.

    private final AtomicReference<String> defaultProtocol = new AtomicReference<>();

    /**
     * Return TLSv1.2 when FIPS mode is enabled.
     * Otherwise, returns TLSv1.3 or TLSv1.2 depending on Java runtime version being used.
     * TLSv1.3 was first introduced in JDK11 and back-ported to OpenJDK 8u272.
     */
    public String defaultTlsProtocol(ZKConfig config) {
        String proto = defaultProtocol.get();
        if (proto != null) {
            return proto;
        }
        String protocol = TLS_1_2;
        if (!getFipsMode(config)) {
            List<String> supported = new ArrayList<>();
            try {
                supported = Arrays.asList(SSLContext.getDefault().getSupportedSSLParameters().getProtocols());
                LOG.info("Supported TLS protocols are {}", supported);
                if (supported.contains(TLS_1_3)) {
                    protocol = TLS_1_3;
                }
            } catch (NoSuchAlgorithmException e) {
                // Ignore.
            }
        }
        if (defaultProtocol.compareAndSet(null, protocol)) {
            LOG.info("Default TLS protocol is {}", protocol);
        } else {
            protocol = defaultProtocol.get();
        }
        return protocol;
    }

anmolnar · 2026-05-05T22:15:30Z

@meszibalu Actually it could be static/singleton, we don't need to calculate it for every single instance, do we?

anmolnar · 2026-05-05T22:25:03Z

+            System.clearProperty("javax.net.ssl.trustStore");
+            System.clearProperty("javax.net.ssl.trustStorePassword");
+            System.clearProperty("javax.net.ssl.trustStoreType");


I don't think we can implement a test like this. SSLContext.getDefault() will cache the problem and subsequent tests will fail even if the properties are reverted.

According to the javadoc:

If a default context was set using the SSLContext.setDefault() method, it is returned. Otherwise, the first call of this method triggers the call SSLContext.getInstance("Default").

And the code in java 25:

public static SSLContext getDefault() throws NoSuchAlgorithmException { SSLContext temporaryContext = defaultContext; if (temporaryContext == null) { temporaryContext = SSLContext.getInstance("Default"); if (!VH_DEFAULT_CONTEXT.compareAndSet(null, temporaryContext)) { temporaryContext = defaultContext; } } return temporaryContext; }

Which means we can call SSLContext.getInstance("Default") and it will NOT cache.

Just a question: So if SSLContext.getDefault() will cache and not create a new SSL context, do we need our own complex caching solution to avoid calling SSLContext.getDefault() on every method call? Or can we just use the original solution which was without the field (e.g. the first commit)? 🤔

OK, I tried to use SSLContext.getInstance("Default") instead of SSLContext.getDefault() but was not able to make the tests work.

I was not able to make this test work, not even with SSLContext.setDefault.

So I removed it. 🤷‍♂️

Me neither. I spent a few hours yesterday with this, but I'm unable to "reset" the property values. All tests are green if run without the new test or alone, but not with it. There's some state change in the background which I couldn't find.

…-static

…tore from X509UtilTest to fix the tests.

anmolnar

Just one more thing.

anmolnar · 2026-05-06T14:48:03Z

    }

-    public static final String DEFAULT_PROTOCOL = defaultTlsProtocol();
+    private final AtomicReference<String> defaultProtocol = new AtomicReference<>();


Make it static, because we shouldn't calculate it for every instance.

because we shouldn't calculate it for every instance.

defaultTlsProtocol should not depend on if it was called in FIPS mode or not. Condense logging so that supported TLS protocols are not logged multiple times.

not needed anymore.

PDavid · 2026-05-07T11:12:54Z

org.apache.zookeeper.server.admin.SnapshotAndRestoreCommandTest.testSnapshotAndRestoreCommand_streaming failed in the Jenkins PR build. I think it is unrelated.

anmolnar · 2026-05-07T13:57:28Z

Merged to master branch. Thanks @PDavid and @meszibalu !
Please create separate pull request for branch-3.9

PDavid · 2026-05-07T14:00:05Z

Many thanks for the reviews @anmolnar and @meszibalu! 👍

PDavid force-pushed the ZOOKEEPER-5045 branch 2 times, most recently from bb72473 to 72ddee6 Compare May 4, 2026 13:29

PDavid marked this pull request as ready for review May 4, 2026 13:36

anmolnar requested changes May 4, 2026

View reviewed changes

Comment thread zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java

Comment thread zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java

ZOOKEEPER-5045: Fall back to TLSv1.2 default in FIPS mode

72e30cc

PDavid force-pushed the ZOOKEEPER-5045 branch from 72ddee6 to 72e30cc Compare May 4, 2026 13:58

meszibalu reviewed May 5, 2026

View reviewed changes

Comment thread zookeeper-server/src/test/java/org/apache/zookeeper/common/X509UtilTest.java

Comment thread zookeeper-server/src/test/java/org/apache/zookeeper/common/X509UtilTest.java Outdated

Comment thread zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java

PDavid added 3 commits May 5, 2026 14:48

ZOOKEEPER-5045: Lazy init for default TLS protocol

b8dc40d

ZOOKEEPER-5045: Fix test method name

cb391d9

ZOOKEEPER-5045: Added test with invalid default truststore

55e134b

Tests when the default truststore of the JVM (javax.net.ssl.trustStore) points to a nonexistent file.

anmolnar reviewed May 5, 2026

View reviewed changes

ZOOKEEPER-5043: Use AtomicReference, change defaultTlsProtocol to non…

8a8f99c

…-static

PDavid force-pushed the ZOOKEEPER-5045 branch from 73d53b9 to 8a8f99c Compare May 6, 2026 10:03

ZOOKEEPER-5043: Remove testDefaultTlsProtocolWithInvalidDefaultTrustS…

9718c50

…tore from X509UtilTest to fix the tests.

PDavid requested review from anmolnar and meszibalu May 6, 2026 13:53

anmolnar requested changes May 6, 2026

View reviewed changes

ZOOKEEPER-5045: Changed X509Util.defaultProtocol to static

ae1adcc

because we shouldn't calculate it for every instance.

PDavid requested a review from anmolnar May 6, 2026 19:39

PDavid added 2 commits May 7, 2026 08:28

ZOOKEEPER-5045: Improve defaultTlsProtocol

3f7c09f

defaultTlsProtocol should not depend on if it was called in FIPS mode or not. Condense logging so that supported TLS protocols are not logged multiple times.

ZOOKEEPER-5045: Remove X509Util.resetDefaultProtocol

27090e1

not needed anymore.

meszibalu approved these changes May 7, 2026

View reviewed changes

anmolnar approved these changes May 7, 2026

View reviewed changes

anmolnar merged commit d4e15d3 into apache:master May 7, 2026
16 checks passed

PDavid deleted the ZOOKEEPER-5045 branch May 7, 2026 13:59

PDavid mentioned this pull request May 7, 2026

ZOOKEEPER-5045: Fall back to TLSv1.2 default in FIPS mode #2383

Open

Conversation

PDavid commented May 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anmolnar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PDavid commented May 5, 2026

Uh oh!

anmolnar commented May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anmolnar commented May 5, 2026

Uh oh!

anmolnar May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meszibalu May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PDavid May 6, 2026

Choose a reason for hiding this comment

Uh oh!

PDavid May 6, 2026

Choose a reason for hiding this comment

Uh oh!

PDavid May 6, 2026

Choose a reason for hiding this comment

Uh oh!

anmolnar May 6, 2026

Choose a reason for hiding this comment

Uh oh!

anmolnar left a comment

Choose a reason for hiding this comment

Uh oh!

anmolnar May 6, 2026

Choose a reason for hiding this comment

Uh oh!

PDavid commented May 7, 2026

Uh oh!

Uh oh!

anmolnar commented May 7, 2026

Uh oh!

PDavid commented May 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

PDavid commented May 4, 2026 •

edited

Loading

anmolnar commented May 5, 2026 •

edited

Loading

anmolnar May 5, 2026 •

edited

Loading

meszibalu May 6, 2026 •

edited

Loading