Skip to content

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

Open
PDavid wants to merge 4 commits intoapache:masterfrom
PDavid:ZOOKEEPER-5045
Open

ZOOKEEPER-5045: Fall back to TLSv1.2 default in FIPS mode#2380
PDavid wants to merge 4 commits intoapache:masterfrom
PDavid:ZOOKEEPER-5045

Conversation

@PDavid
Copy link
Copy Markdown
Contributor

@PDavid PDavid commented May 4, 2026

No description provided.

@PDavid PDavid force-pushed the ZOOKEEPER-5045 branch 2 times, most recently from bb72473 to 72ddee6 Compare May 4, 2026 13:29
@PDavid PDavid marked this pull request as ready for review May 4, 2026 13:36
Copy link
Copy Markdown
Contributor

@anmolnar anmolnar left a comment

Choose a reason for hiding this comment

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

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

public static String defaultTlsProtocol(ZKConfig config) {
if (getFipsMode(config)) {
LOG.info("FIPS mode is enabled. Fall back to TLSv1.2 as the default protocol.");
return TLS_1_2;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm still not sure about whether this should be 1.3 or 1.2

@PDavid PDavid force-pushed the ZOOKEEPER-5045 branch from 72ddee6 to 72e30cc Compare May 4, 2026 13:58
Comment thread zookeeper-server/src/test/java/org/apache/zookeeper/common/X509UtilTest.java Outdated
PDavid added 3 commits May 5, 2026 14:48
@PDavid
Copy link
Copy Markdown
Contributor Author

PDavid commented May 5, 2026

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

@anmolnar
Copy link
Copy Markdown
Contributor

anmolnar commented May 5, 2026

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
Copy link
Copy Markdown
Contributor

anmolnar commented May 5, 2026

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

Comment on lines +911 to +913
System.clearProperty("javax.net.ssl.trustStore");
System.clearProperty("javax.net.ssl.trustStorePassword");
System.clearProperty("javax.net.ssl.trustStoreType");
Copy link
Copy Markdown
Contributor

@anmolnar anmolnar May 5, 2026

Choose a reason for hiding this comment

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants