-
Notifications
You must be signed in to change notification settings - Fork 7.3k
ZOOKEEPER-5045: Fall back to TLSv1.2 default in FIPS mode #2380
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
base: master
Are you sure you want to change the base?
Changes from all commits
72e30cc
b8dc40d
cb391d9
55e134b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -87,27 +87,38 @@ public abstract class X509Util implements Closeable, AutoCloseable { | |
| } | ||
| } | ||
|
|
||
| public static final String DEFAULT_PROTOCOL = defaultTlsProtocol(); | ||
| private static volatile String bestAvailableProtocol; | ||
|
|
||
| /** | ||
| * Return TLSv1.3 or TLSv1.2 depending on Java runtime version being used. | ||
| * 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. | ||
| */ | ||
| private static String defaultTlsProtocol() { | ||
| String defaultProtocol = TLS_1_2; | ||
| public static String defaultTlsProtocol(ZKConfig config) { | ||
| if (getFipsMode(config)) { | ||
| return TLS_1_2; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| } | ||
| return getBestAvailableProtocol(); | ||
| } | ||
|
|
||
| private static String getBestAvailableProtocol() { | ||
| if (bestAvailableProtocol != null) { | ||
| return bestAvailableProtocol; | ||
| } | ||
|
|
||
| String protocol = TLS_1_2; | ||
| List<String> supported = new ArrayList<>(); | ||
| try { | ||
| supported = Arrays.asList(SSLContext.getDefault().getSupportedSSLParameters().getProtocols()); | ||
| // We cannot use the default protocols directly, because the SSLContext factory methods | ||
| // only accept a single protocol | ||
| if (supported.contains(TLS_1_3)) { | ||
| defaultProtocol = TLS_1_3; | ||
| protocol = TLS_1_3; | ||
| } | ||
| } catch (NoSuchAlgorithmException e) { | ||
| // Ignore. | ||
| } | ||
| LOG.info("Default TLS protocol is {}, supported TLS protocols are {}", defaultProtocol, supported); | ||
| return defaultProtocol; | ||
| LOG.info("Default TLS protocol is {}, supported TLS protocols are {}", protocol, supported); | ||
| bestAvailableProtocol = protocol; | ||
| return protocol; | ||
| } | ||
|
|
||
| public static final int DEFAULT_HANDSHAKE_DETECTION_TIMEOUT_MILLIS = 5000; | ||
|
|
@@ -410,8 +421,8 @@ public SSLContextAndOptions createSSLContextAndOptionsFromConfig(ZKConfig config | |
| + ": " | ||
| + trustStoreTypeProp, e); | ||
| } | ||
|
|
||
| String protocol = config.getProperty(sslProtocolProperty, DEFAULT_PROTOCOL); | ||
| String defaultTlsProtocol = defaultTlsProtocol(config); | ||
| String protocol = config.getProperty(sslProtocolProperty, defaultTlsProtocol); | ||
| try { | ||
| SSLContext sslContext = SSLContext.getInstance(protocol); | ||
| sslContext.init(keyManagers, trustManagers, null); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,9 @@ | |
|
|
||
| package org.apache.zookeeper.common; | ||
|
|
||
| import static org.apache.zookeeper.common.X509Util.FIPS_MODE_PROPERTY; | ||
| import static org.apache.zookeeper.common.X509Util.TLS_1_2; | ||
| import static org.apache.zookeeper.common.X509Util.TLS_1_3; | ||
| import static org.junit.jupiter.api.Assertions.assertArrayEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertThrows; | ||
|
|
@@ -90,6 +93,7 @@ public void cleanUp() { | |
| System.clearProperty(x509Util.getSslHandshakeDetectionTimeoutMillisProperty()); | ||
| System.clearProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN_FACTORY); | ||
| System.clearProperty(ZKClientConfig.ZOOKEEPER_CLIENT_CNXN_SOCKET); | ||
| System.clearProperty(FIPS_MODE_PROPERTY); | ||
| x509Util.close(); | ||
| } | ||
|
|
||
|
|
@@ -100,24 +104,39 @@ public void testCreateSSLContextWithoutCustomProtocol( | |
| X509KeyType caKeyType, X509KeyType certKeyType, String keyPassword, Integer paramIndex) | ||
| throws Exception { | ||
| init(caKeyType, certKeyType, keyPassword, paramIndex); | ||
| System.setProperty(FIPS_MODE_PROPERTY, Boolean.FALSE.toString()); | ||
| SSLContext sslContext = x509Util.getDefaultSSLContext(); | ||
| assertEquals(X509Util.DEFAULT_PROTOCOL, sslContext.getProtocol()); | ||
| String defaultTlsProtocol = X509Util.defaultTlsProtocol(new ZKConfig()); | ||
| assertEquals(defaultTlsProtocol, sslContext.getProtocol()); | ||
|
|
||
| // Check that TLSv1.3 is selected in JDKs that support it (OpenJDK 8u272 and later). | ||
| List<String> supported = Arrays.asList(SSLContext.getDefault().getSupportedSSLParameters().getProtocols()); | ||
| if (supported.contains(X509Util.TLS_1_3)) { | ||
| if (supported.contains(TLS_1_3)) { | ||
| // SSLContext protocol. | ||
| assertEquals(X509Util.TLS_1_3, sslContext.getProtocol()); | ||
| assertEquals(TLS_1_3, sslContext.getProtocol()); | ||
| // Enabled protocols. | ||
| List<String> protos = Arrays.asList(sslContext.getDefaultSSLParameters().getProtocols()); | ||
| assertTrue(protos.contains(X509Util.TLS_1_2)); | ||
| assertTrue(protos.contains(X509Util.TLS_1_3)); | ||
| assertTrue(protos.contains(TLS_1_2)); | ||
| assertTrue(protos.contains(TLS_1_3)); | ||
| } else { | ||
| assertEquals(X509Util.TLS_1_2, sslContext.getProtocol()); | ||
| assertArrayEquals(new String[]{X509Util.TLS_1_2}, sslContext.getDefaultSSLParameters().getProtocols()); | ||
| assertEquals(TLS_1_2, sslContext.getProtocol()); | ||
| assertArrayEquals(new String[]{TLS_1_2}, sslContext.getDefaultSSLParameters().getProtocols()); | ||
| } | ||
| } | ||
|
|
||
| @ParameterizedTest | ||
| @MethodSource("data") | ||
| @Timeout(value = 5) | ||
| public void testCreateSSLContextFIPSModeEnabled( | ||
| X509KeyType caKeyType, X509KeyType certKeyType, String keyPassword, Integer paramIndex) | ||
| throws Exception { | ||
| init(caKeyType, certKeyType, keyPassword, paramIndex); | ||
| System.setProperty(FIPS_MODE_PROPERTY, Boolean.TRUE.toString()); | ||
| SSLContext sslContext = x509Util.getDefaultSSLContext(); | ||
| assertEquals(TLS_1_2, sslContext.getProtocol()); | ||
| assertArrayEquals(new String[]{TLS_1_2}, sslContext.getDefaultSSLParameters().getProtocols()); | ||
| } | ||
|
PDavid marked this conversation as resolved.
|
||
|
|
||
| @ParameterizedTest | ||
| @MethodSource("data") | ||
| @Timeout(value = 5) | ||
|
|
@@ -873,4 +892,25 @@ private void testCreateSSLContext_withWrongPasswordFromFile(final String keyPass | |
| x509Util.getDefaultSSLContext(); | ||
| }); | ||
| } | ||
|
|
||
| @ParameterizedTest | ||
| @MethodSource("data") | ||
| @Timeout(value = 5) | ||
| public void testDefaultTlsProtocolWithInvalidDefaultTrustStore( | ||
| X509KeyType caKeyType, X509KeyType certKeyType, String keyPassword, Integer paramIndex) | ||
| throws Exception { | ||
| init(caKeyType, certKeyType, keyPassword, paramIndex); | ||
| System.setProperty("javax.net.ssl.trustStore", "/nonexistent/truststore.jks"); | ||
| System.setProperty("javax.net.ssl.trustStorePassword", "bogus"); | ||
| System.setProperty("javax.net.ssl.trustStoreType", "JKS"); | ||
| try { | ||
| System.setProperty(FIPS_MODE_PROPERTY, Boolean.FALSE.toString()); | ||
| String protocol = X509Util.defaultTlsProtocol(new ZKConfig()); | ||
| assertTrue(TLS_1_2.equals(protocol) || TLS_1_3.equals(protocol)); | ||
| } finally { | ||
| System.clearProperty("javax.net.ssl.trustStore"); | ||
| System.clearProperty("javax.net.ssl.trustStorePassword"); | ||
| System.clearProperty("javax.net.ssl.trustStoreType"); | ||
|
Comment on lines
+911
to
+913
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we can implement a test like this.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. According to the javadoc:
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 |
||
| } | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.