This is an automated email from the ASF dual-hosted git repository.
andor pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/zookeeper.git
The following commit(s) were added to refs/heads/master by this push:
new c91f6111d ZOOKEEPER-4997: Unify construction of TrustManager to
dedicated method
c91f6111d is described below
commit c91f6111d2f4d115217ba1dc4c28fa5bd26e2c91
Author: Kezhu Wang <[email protected]>
AuthorDate: Wed Dec 17 06:24:33 2025 +0800
ZOOKEEPER-4997: Unify construction of TrustManager to dedicated method
Reviewers: anmolnar
Author: kezhuw
Closes #2340 from kezhuw/ZOOKEEPER-4997-refactor-x509-createTrustManager
---
.../apache/zookeeper/common/ClientX509Util.java | 50 ++------
.../java/org/apache/zookeeper/common/X509Util.java | 131 ++++++++++++++-------
.../server/auth/X509AuthenticationProvider.java | 55 ++-------
.../zookeeper/common/X509UtilCrlPropertyTest.java | 104 ++++++++++++++++
4 files changed, 209 insertions(+), 131 deletions(-)
diff --git
a/zookeeper-server/src/main/java/org/apache/zookeeper/common/ClientX509Util.java
b/zookeeper-server/src/main/java/org/apache/zookeeper/common/ClientX509Util.java
index b035bbfe0..1e50b8425 100644
---
a/zookeeper-server/src/main/java/org/apache/zookeeper/common/ClientX509Util.java
+++
b/zookeeper-server/src/main/java/org/apache/zookeeper/common/ClientX509Util.java
@@ -63,20 +63,14 @@ public String getSslProviderProperty() {
public SslContext createNettySslContextForClient(ZKConfig config)
throws X509Exception.KeyManagerException,
X509Exception.TrustManagerException, SSLException {
- String keyStoreLocation =
config.getProperty(getSslKeystoreLocationProperty(), "");
- String keyStorePassword = getPasswordFromConfigPropertyOrFile(config,
getSslKeystorePasswdProperty(),
- getSslKeystorePasswdPathProperty());
- String keyStoreType = config.getProperty(getSslKeystoreTypeProperty());
-
SslContextBuilder sslContextBuilder = SslContextBuilder.forClient();
- if (keyStoreLocation.isEmpty()) {
- LOG.warn("{} not specified", getSslKeystoreLocationProperty());
- } else {
- sslContextBuilder.keyManager(createKeyManager(keyStoreLocation,
keyStorePassword, keyStoreType));
+ KeyManager km = buildKeyManager(config);
+ if (km != null) {
+ sslContextBuilder.keyManager(km);
}
- TrustManager tm = getTrustManager(config);
+ TrustManager tm = buildTrustManager(config);
if (tm != null) {
sslContextBuilder.trustManager(tm);
}
@@ -103,19 +97,12 @@ public SslContext createNettySslContextForClient(ZKConfig
config)
public SslContext createNettySslContextForServer(ZKConfig config)
throws X509Exception.SSLContextException,
X509Exception.KeyManagerException, X509Exception.TrustManagerException,
SSLException {
- String keyStoreLocation =
config.getProperty(getSslKeystoreLocationProperty(), "");
- String keyStorePassword = getPasswordFromConfigPropertyOrFile(config,
getSslKeystorePasswdProperty(),
- getSslKeystorePasswdPathProperty());
- String keyStoreType = config.getProperty(getSslKeystoreTypeProperty());
-
- if (keyStoreLocation.isEmpty()) {
+ KeyManager km = buildKeyManager(config);
+ if (km == null) {
throw new X509Exception.SSLContextException(
"Keystore is required for SSL server: " +
getSslKeystoreLocationProperty());
}
-
- KeyManager km = createKeyManager(keyStoreLocation, keyStorePassword,
keyStoreType);
-
- return createNettySslContextForServer(config, km,
getTrustManager(config));
+ return createNettySslContextForServer(config, km,
buildTrustManager(config));
}
public SslContext createNettySslContextForServer(ZKConfig config,
KeyManager keyManager, TrustManager trustManager) throws SSLException {
@@ -195,27 +182,4 @@ private Iterable<String> getCipherSuites(final ZKConfig
config) {
public SslProvider getSslProvider(ZKConfig config) {
return
SslProvider.valueOf(config.getProperty(getSslProviderProperty(), "JDK"));
}
-
- private TrustManager getTrustManager(ZKConfig config) throws
X509Exception.TrustManagerException {
- String trustStoreLocation =
config.getProperty(getSslTruststoreLocationProperty(), "");
- String trustStorePassword =
getPasswordFromConfigPropertyOrFile(config, getSslTruststorePasswdProperty(),
- getSslTruststorePasswdPathProperty());
- String trustStoreType =
config.getProperty(getSslTruststoreTypeProperty());
-
- boolean sslCrlEnabled = config.getBoolean(getSslCrlEnabledProperty(),
Boolean.getBoolean("com.sun.net.ssl.checkRevocation"));
- boolean sslOcspEnabled =
config.getBoolean(getSslOcspEnabledProperty(),
Boolean.parseBoolean(Security.getProperty("ocsp.enable")));
- boolean sslServerHostnameVerificationEnabled =
isServerHostnameVerificationEnabled(config);
- boolean sslClientHostnameVerificationEnabled =
isClientHostnameVerificationEnabled(config);
- boolean allowReverseDnsLookup = allowReverseDnsLookup(config);
-
- if (trustStoreLocation.isEmpty()) {
- LOG.warn("{} not specified", getSslTruststoreLocationProperty());
- return null;
- } else {
- return createTrustManager(trustStoreLocation, trustStorePassword,
trustStoreType,
- sslCrlEnabled, sslOcspEnabled,
sslServerHostnameVerificationEnabled,
- sslClientHostnameVerificationEnabled, allowReverseDnsLookup,
- getFipsMode(config));
- }
- }
}
diff --git
a/zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java
b/zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java
index 2ac1ad9ee..fee410cf0 100644
--- a/zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java
+++ b/zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java
@@ -378,57 +378,37 @@ public SSLContextAndOptions
createSSLContextAndOptions(ZKConfig config) throws S
}
public SSLContextAndOptions createSSLContextAndOptionsFromConfig(ZKConfig
config) throws SSLContextException {
- KeyManager[] keyManagers = null;
- TrustManager[] trustManagers = null;
-
- String keyStoreLocationProp =
config.getProperty(sslKeystoreLocationProperty, "");
- String keyStorePasswordProp =
getPasswordFromConfigPropertyOrFile(config, sslKeystorePasswdProperty,
sslKeystorePasswdPathProperty);
- String keyStoreTypeProp = config.getProperty(sslKeystoreTypeProperty);
-
// There are legal states in some use cases for null KeyManager or
TrustManager.
// But if a user wanna specify one, location is required. Password
defaults to empty string if it is not
// specified by the user.
+ KeyManager[] keyManagers = null;
+ TrustManager[] trustManagers = null;
- if (keyStoreLocationProp.isEmpty()) {
- LOG.warn("{} not specified", getSslKeystoreLocationProperty());
- } else {
- try {
- keyManagers = new
KeyManager[]{createKeyManager(keyStoreLocationProp, keyStorePasswordProp,
keyStoreTypeProp)};
- } catch (KeyManagerException keyManagerException) {
- throw new SSLContextException("Failed to create KeyManager",
keyManagerException);
- } catch (IllegalArgumentException e) {
- throw new SSLContextException("Bad value for " +
sslKeystoreTypeProperty + ": " + keyStoreTypeProp, e);
+ try {
+ KeyManager km = buildKeyManager(config);
+ if (km != null) {
+ keyManagers = new KeyManager[]{km};
}
+ } catch (KeyManagerException keyManagerException) {
+ throw new SSLContextException("Failed to create KeyManager",
keyManagerException);
+ } catch (IllegalArgumentException e) {
+ String keyStoreTypeProp =
config.getProperty(sslKeystoreTypeProperty);
+ throw new SSLContextException("Bad value for " +
sslKeystoreTypeProperty + ": " + keyStoreTypeProp, e);
}
- String trustStoreLocationProp =
config.getProperty(sslTruststoreLocationProperty, "");
- String trustStorePasswordProp =
getPasswordFromConfigPropertyOrFile(config, sslTruststorePasswdProperty,
sslTruststorePasswdPathProperty);
- String trustStoreTypeProp =
config.getProperty(sslTruststoreTypeProperty);
-
- boolean sslCrlEnabled = config.getBoolean(this.sslCrlEnabledProperty,
Boolean.getBoolean("com.sun.net.ssl.checkRevocation"));
- boolean sslOcspEnabled =
config.getBoolean(this.sslOcspEnabledProperty,
Boolean.parseBoolean(Security.getProperty("ocsp.enable")));
-
- boolean sslServerHostnameVerificationEnabled =
isServerHostnameVerificationEnabled(config);
- boolean sslClientHostnameVerificationEnabled =
isClientHostnameVerificationEnabled(config);
- boolean allowReverseDnsLookup = allowReverseDnsLookup(config);
- boolean fipsMode = getFipsMode(config);
-
- if (trustStoreLocationProp.isEmpty()) {
- LOG.warn("{} not specified", getSslTruststoreLocationProperty());
- } else {
- try {
- trustManagers = new TrustManager[]{
- createTrustManager(trustStoreLocationProp,
trustStorePasswordProp, trustStoreTypeProp, sslCrlEnabled,
- sslOcspEnabled, sslServerHostnameVerificationEnabled,
sslClientHostnameVerificationEnabled,
- allowReverseDnsLookup, fipsMode)};
- } catch (TrustManagerException trustManagerException) {
- throw new SSLContextException("Failed to create TrustManager",
trustManagerException);
- } catch (IllegalArgumentException e) {
- throw new SSLContextException("Bad value for "
- + sslTruststoreTypeProperty
- + ": "
- + trustStoreTypeProp, e);
+ try {
+ TrustManager tm = buildTrustManager(config);
+ if (tm != null) {
+ trustManagers = new TrustManager[]{tm};
}
+ } catch (TrustManagerException trustManagerException) {
+ throw new SSLContextException("Failed to create TrustManager",
trustManagerException);
+ } catch (IllegalArgumentException e) {
+ String trustStoreTypeProp =
config.getProperty(sslTruststoreTypeProperty);
+ throw new SSLContextException("Bad value for "
+ + sslTruststoreTypeProperty
+ + ": "
+ + trustStoreTypeProp, e);
}
String protocol = config.getProperty(sslProtocolProperty,
DEFAULT_PROTOCOL);
@@ -487,6 +467,18 @@ public String getPasswordFromConfigPropertyOrFile(final
ZKConfig config,
return value;
}
+ public X509KeyManager buildKeyManager(ZKConfig config) throws
KeyManagerException {
+ String keyStoreLocation =
config.getProperty(getSslKeystoreLocationProperty(), "");
+ if (keyStoreLocation.isEmpty()) {
+ LOG.warn("{} not specified for X509KeyManager",
getSslKeystoreLocationProperty());
+ return null;
+ }
+ String keyStorePassword = getPasswordFromConfigPropertyOrFile(config,
getSslKeystorePasswdProperty(),
+ getSslKeystorePasswdPathProperty());
+ String keyStoreType = config.getProperty(getSslKeystoreTypeProperty());
+ return createKeyManager(keyStoreLocation, keyStorePassword,
keyStoreType);
+ }
+
/**
* Creates a key manager by loading the key store from the given file of
* the given type, optionally decrypting it using the given password.
@@ -522,6 +514,59 @@ public static X509KeyManager createKeyManager(
}
}
+ public X509TrustManager buildTrustManager(ZKConfig config) throws
TrustManagerException {
+ String trustStoreLocationProp =
config.getProperty(sslTruststoreLocationProperty, "");
+ if (trustStoreLocationProp.isEmpty()) {
+ LOG.warn("{} not specified for X509TrustManager",
sslTruststoreLocationProperty);
+ return null;
+ }
+
+ String trustStorePasswordProp =
getPasswordFromConfigPropertyOrFile(config, sslTruststorePasswdProperty,
sslTruststorePasswdPathProperty);
+ String trustStoreTypeProp =
config.getProperty(sslTruststoreTypeProperty);
+
+ boolean sslCrlEnabled = config.getBoolean(this.sslCrlEnabledProperty,
Boolean.getBoolean("com.sun.net.ssl.checkRevocation"));
+ boolean sslOcspEnabled =
config.getBoolean(this.sslOcspEnabledProperty,
Boolean.parseBoolean(Security.getProperty("ocsp.enable")));
+
+ boolean sslServerHostnameVerificationEnabled =
isServerHostnameVerificationEnabled(config);
+ boolean sslClientHostnameVerificationEnabled =
isClientHostnameVerificationEnabled(config);
+ boolean allowReverseDnsLookup = allowReverseDnsLookup(config);
+ boolean fipsMode = getFipsMode(config);
+
+ return createTrustManagerInternal(
+ trustStoreLocationProp,
+ trustStorePasswordProp,
+ trustStoreTypeProp,
+ sslCrlEnabled,
+ sslOcspEnabled,
+ sslServerHostnameVerificationEnabled,
+ sslClientHostnameVerificationEnabled,
+ allowReverseDnsLookup,
+ fipsMode);
+ }
+
+ // @VisibleForTesting
+ protected X509TrustManager createTrustManagerInternal(
+ String trustStoreLocation,
+ String trustStorePassword,
+ String trustStoreTypeProp,
+ boolean crlEnabled,
+ boolean ocspEnabled,
+ final boolean serverHostnameVerificationEnabled,
+ final boolean clientHostnameVerificationEnabled,
+ final boolean allowReverseDnsLookup,
+ final boolean fipsMode) throws TrustManagerException {
+ return createTrustManager(
+ trustStoreLocation,
+ trustStorePassword,
+ trustStoreTypeProp,
+ crlEnabled,
+ ocspEnabled,
+ serverHostnameVerificationEnabled,
+ clientHostnameVerificationEnabled,
+ allowReverseDnsLookup,
+ fipsMode);
+ }
+
/**
* Creates a trust manager by loading the trust store from the given file
* of the given type, optionally decrypting it using the given password.
diff --git
a/zookeeper-server/src/main/java/org/apache/zookeeper/server/auth/X509AuthenticationProvider.java
b/zookeeper-server/src/main/java/org/apache/zookeeper/server/auth/X509AuthenticationProvider.java
index 8392db6bc..3bc430468 100644
---
a/zookeeper-server/src/main/java/org/apache/zookeeper/server/auth/X509AuthenticationProvider.java
+++
b/zookeeper-server/src/main/java/org/apache/zookeeper/server/auth/X509AuthenticationProvider.java
@@ -18,7 +18,6 @@
package org.apache.zookeeper.server.auth;
-import java.security.Security;
import java.security.cert.Certificate;
import java.security.cert.CertificateException;
import java.security.cert.X509Certificate;
@@ -80,55 +79,21 @@ public class X509AuthenticationProvider implements
AuthenticationProvider {
public X509AuthenticationProvider() throws X509Exception {
ZKConfig config = new ZKConfig();
try (X509Util x509Util = new ClientX509Util()) {
- String keyStoreLocation =
config.getProperty(x509Util.getSslKeystoreLocationProperty(), "");
- String keyStorePassword =
x509Util.getPasswordFromConfigPropertyOrFile(config,
- x509Util.getSslKeystorePasswdProperty(),
- x509Util.getSslKeystorePasswdPathProperty());
- String keyStoreTypeProp =
config.getProperty(x509Util.getSslKeystoreTypeProperty());
-
- boolean crlEnabled =
config.getBoolean(x509Util.getSslCrlEnabledProperty(),
Boolean.getBoolean("com.sun.net.ssl.checkRevocation"));
- boolean ocspEnabled =
config.getBoolean(x509Util.getSslOcspEnabledProperty(),
Boolean.parseBoolean(Security.getProperty("ocsp.enable")));
- boolean hostnameVerificationEnabled =
Boolean.parseBoolean(config.getProperty(x509Util.getSslHostnameVerificationEnabledProperty()));
- boolean clientHostnameVerificationEnabled =
x509Util.isClientHostnameVerificationEnabled(config);
- boolean allowReverseDnsLookup =
Boolean.parseBoolean(config.getProperty(x509Util.getSslAllowReverseDnsLookupProperty()));
-
X509KeyManager km = null;
X509TrustManager tm = null;
- if (keyStoreLocation.isEmpty()) {
- LOG.warn("keystore not specified for client connection");
- } else {
- try {
- km = X509Util.createKeyManager(keyStoreLocation,
keyStorePassword, keyStoreTypeProp);
- } catch (KeyManagerException e) {
- LOG.error("Failed to create key manager", e);
- }
- }
- String trustStoreLocation =
config.getProperty(x509Util.getSslTruststoreLocationProperty(), "");
- String trustStorePassword =
x509Util.getPasswordFromConfigPropertyOrFile(config,
- x509Util.getSslTruststorePasswdProperty(),
- x509Util.getSslTruststorePasswdPathProperty());
- String trustStoreTypeProp =
config.getProperty(x509Util.getSslTruststoreTypeProperty());
- boolean fipsMode = X509Util.getFipsMode(config);
+ try {
+ km = x509Util.buildKeyManager(config);
+ } catch (KeyManagerException e) {
+ LOG.error("Failed to create key manager", e);
+ }
- if (trustStoreLocation.isEmpty()) {
- LOG.warn("Truststore not specified for client connection");
- } else {
- try {
- tm = X509Util.createTrustManager(
- trustStoreLocation,
- trustStorePassword,
- trustStoreTypeProp,
- crlEnabled,
- ocspEnabled,
- hostnameVerificationEnabled,
- clientHostnameVerificationEnabled,
- allowReverseDnsLookup,
- fipsMode);
- } catch (TrustManagerException e) {
- LOG.error("Failed to create trust manager", e);
- }
+ try {
+ tm = x509Util.buildTrustManager(config);
+ } catch (TrustManagerException e) {
+ LOG.error("Failed to create trust manager", e);
}
+
this.keyManager = km;
this.trustManager = tm;
}
diff --git
a/zookeeper-server/src/test/java/org/apache/zookeeper/common/X509UtilCrlPropertyTest.java
b/zookeeper-server/src/test/java/org/apache/zookeeper/common/X509UtilCrlPropertyTest.java
new file mode 100644
index 000000000..154e738ce
--- /dev/null
+++
b/zookeeper-server/src/test/java/org/apache/zookeeper/common/X509UtilCrlPropertyTest.java
@@ -0,0 +1,104 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.common;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.Mockito.any;
+import static org.mockito.Mockito.anyBoolean;
+import static org.mockito.Mockito.doAnswer;
+import static org.mockito.Mockito.spy;
+import java.security.Security;
+import java.security.cert.CertificateException;
+import java.security.cert.X509Certificate;
+import javax.net.ssl.X509TrustManager;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+
+public class X509UtilCrlPropertyTest {
+ private static class TrustManagerCrlProperties implements X509TrustManager
{
+ private final boolean crlEnabled;
+ private final boolean ocspEnabled;
+
+ public TrustManagerCrlProperties(boolean crlEnabled, boolean
ocspEnabled) {
+ this.crlEnabled = crlEnabled;
+ this.ocspEnabled = ocspEnabled;
+ }
+
+ @Override
+ public X509Certificate[] getAcceptedIssuers() {
+ return new X509Certificate[0];
+ }
+
+ @Override
+ public void checkClientTrusted(X509Certificate[] chain, String
authType) throws CertificateException {
+ }
+
+ @Override
+ public void checkServerTrusted(X509Certificate[] chain, String
authType) throws CertificateException {
+ }
+ }
+
+ @ParameterizedTest
+ @ValueSource(classes = {ClientX509Util.class, QuorumX509Util.class})
+ public void testCrlProperties(Class<? extends X509Util> clazz) throws
Exception {
+ try {
+ X509Util util = spy(clazz.getDeclaredConstructor().newInstance());
+
+ doAnswer(mock -> new TrustManagerCrlProperties(mock.getArgument(3,
Boolean.class), mock.getArgument(4, Boolean.class)))
+ .when(util)
+ .createTrustManagerInternal(any(), any(), any(), anyBoolean(),
anyBoolean(), anyBoolean(), anyBoolean(), anyBoolean(), anyBoolean());
+
+ ZKConfig config = new ZKConfig();
+ config.setProperty(util.getSslTruststoreLocationProperty(),
"no-empty");
+
+ TrustManagerCrlProperties properties = (TrustManagerCrlProperties)
util.buildTrustManager(config);
+
+ assertFalse(properties.crlEnabled);
+ assertFalse(properties.ocspEnabled);
+
+ // given: "com.sun.net.ssl.checkRevocation"
+ System.setProperty("com.sun.net.ssl.checkRevocation", "true");
+ properties = (TrustManagerCrlProperties)
util.buildTrustManager(config);
+ // then: default to "com.sun.net.ssl.checkRevocation"
+ assertTrue(properties.crlEnabled);
+
+ // given: security property "ocsp.enable"
+ Security.setProperty("ocsp.enable", "true");
+ properties = (TrustManagerCrlProperties)
util.buildTrustManager(config);
+ // then: default to "ocsp.enable"
+ assertTrue(properties.ocspEnabled);
+
+ // given: both "zookeeper.ssl.crl" and
"com.sun.net.ssl.checkRevocation"
+ config.setProperty(util.getSslCrlEnabledProperty(), "false");
+ properties = (TrustManagerCrlProperties)
util.buildTrustManager(config);
+ // then: "zookeeper.ssl.crl" take precedence
+ assertFalse(properties.crlEnabled);
+
+ // given: both "zookeeper.ssl.ocsp" and "ocsp.enable"
+ config.setProperty(util.getSslOcspEnabledProperty(), "false");
+ properties = (TrustManagerCrlProperties)
util.buildTrustManager(config);
+ // then: "zookeeper.ssl.ocsp" take precedence
+ assertFalse(properties.ocspEnabled);
+ } finally {
+ System.clearProperty("com.sun.net.ssl.checkRevocation");
+ Security.setProperty("ocsp.enable", "false");
+ }
+ }
+}
