Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,9 @@ public SslContext createNettySslContextForClient(ZKConfig config)
sslContextBuilder.trustManager(tm);
}

sslContextBuilder.enableOcsp(config.getBoolean(getSslOcspEnabledProperty()));
if (!config.getTriState(getSslOcspEnabledProperty()).isSystem()) {
sslContextBuilder.enableOcsp(config.getBoolean(getSslOcspEnabledProperty()));
}
String[] enabledProtocols = getEnabledProtocols(config);
if (enabledProtocols != null) {
sslContextBuilder.protocols(enabledProtocols);
Expand Down Expand Up @@ -123,7 +125,9 @@ public SslContext createNettySslContextForServer(ZKConfig config, KeyManager key
sslContextBuilder.trustManager(trustManager);
}

sslContextBuilder.enableOcsp(config.getBoolean(getSslOcspEnabledProperty()));
if (!config.getTriState(getSslOcspEnabledProperty()).isSystem()) {
sslContextBuilder.enableOcsp(config.getBoolean(getSslOcspEnabledProperty()));
}
String[] enabledProtocols = getEnabledProtocols(config);
if (enabledProtocols != null) {
sslContextBuilder.protocols(enabledProtocols);
Expand Down Expand Up @@ -189,8 +193,8 @@ private TrustManager getTrustManager(ZKConfig config) throws X509Exception.Trust
getSslTruststorePasswdPathProperty());
String trustStoreType = config.getProperty(getSslTruststoreTypeProperty());

boolean sslCrlEnabled = config.getBoolean(getSslCrlEnabledProperty());
boolean sslOcspEnabled = config.getBoolean(getSslOcspEnabledProperty());
TriState sslCrlEnabled = config.getTriState(getSslCrlEnabledProperty());
TriState sslOcspEnabled = config.getTriState(getSslOcspEnabledProperty());
boolean sslServerHostnameVerificationEnabled = isServerHostnameVerificationEnabled(config);
boolean sslClientHostnameVerificationEnabled = isClientHostnameVerificationEnabled(config);

Expand Down
Copy link
Contributor

Choose a reason for hiding this comment

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

License header is missing.

Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package org.apache.zookeeper.common;

/**
* Represent True / False / System (unset/default) values.
*
*/
public enum TriState {
True,
False,
System;

/**
* @param value the string representation
* @return TriState.true if value equals "true" ignoring case, TriState.System
* if value equals "system", Tristate.False otherwise
*/
public static TriState parse(String value) {
if (value == null) {
return TriState.False;
} else if (value.equalsIgnoreCase("true")) {
return TriState.True;
} else if (value.equalsIgnoreCase("system")) {
return TriState.System;
} else {
return TriState.False;
}
}

public boolean isTrue() {
return this == TriState.True;
}

public boolean isFalse() {
return this == TriState.False;
}

public boolean isSystem() {
return this == TriState.System;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -392,8 +392,8 @@ public SSLContextAndOptions createSSLContextAndOptionsFromConfig(ZKConfig config
String trustStorePasswordProp = getPasswordFromConfigPropertyOrFile(config, sslTruststorePasswdProperty, sslTruststorePasswdPathProperty);
String trustStoreTypeProp = config.getProperty(sslTruststoreTypeProperty);

boolean sslCrlEnabled = config.getBoolean(this.sslCrlEnabledProperty);
boolean sslOcspEnabled = config.getBoolean(this.sslOcspEnabledProperty);
TriState sslCrlEnabled = config.getTriState(this.sslCrlEnabledProperty);
TriState sslOcspEnabled = config.getTriState(this.sslOcspEnabledProperty);
boolean sslServerHostnameVerificationEnabled = isServerHostnameVerificationEnabled(config);
boolean sslClientHostnameVerificationEnabled = isClientHostnameVerificationEnabled(config);
boolean fipsMode = getFipsMode(config);
Expand Down Expand Up @@ -537,8 +537,8 @@ public static X509TrustManager createTrustManager(
String trustStoreLocation,
String trustStorePassword,
String trustStoreTypeProp,
boolean crlEnabled,
boolean ocspEnabled,
TriState crlEnabled,
TriState ocspEnabled,
final boolean serverHostnameVerificationEnabled,
final boolean clientHostnameVerificationEnabled,
final boolean fipsMode) throws TrustManagerException {
Expand All @@ -548,17 +548,19 @@ public static X509TrustManager createTrustManager(
try {
KeyStore ts = loadTrustStore(trustStoreLocation, trustStorePassword, trustStoreTypeProp);
PKIXBuilderParameters pbParams = new PKIXBuilderParameters(ts, new X509CertSelector());
if (crlEnabled || ocspEnabled) {
pbParams.setRevocationEnabled(true);
System.setProperty("com.sun.net.ssl.checkRevocation", "true");
System.setProperty("com.sun.security.enableCRLDP", "true");
if (ocspEnabled) {
Security.setProperty("ocsp.enable", "true");
}
} else {
pbParams.setRevocationEnabled(false);
// Leave CRL/OCSP JVM global properties alone both are set to "system" (represented as null)
if (!crlEnabled.isSystem() || !ocspEnabled.isSystem()) {
if (crlEnabled.isTrue() || ocspEnabled.isTrue()) {
pbParams.setRevocationEnabled(true);
System.setProperty("com.sun.net.ssl.checkRevocation", "true");
System.setProperty("com.sun.security.enableCRLDP", "true");
if (ocspEnabled.isTrue()) {
Security.setProperty("ocsp.enable", "true");
}
} else {
pbParams.setRevocationEnabled(false);
}
Comment on lines +552 to +562
Copy link
Contributor

Choose a reason for hiding this comment

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

One question here:
In the case when both parameters are set to 'system', we won't do anything: not altering system properties and not setting revocation in pbParams. Will that work correctly, I mean will pbParams revocation flag follow the system settings?

Copy link
Contributor

@anmolnar anmolnar Jun 24, 2025

Choose a reason for hiding this comment

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

Doc:

If this flag is true, the default revocation checking mechanism of the underlying PKIX service provider will be used

When a PKIXParameters object is created, this flag is set to true. This setting reflects the most common strategy for checking revocation, since each service provider must support revocation checking to be PKIX compliant. Sophisticated applications should set this flag to false when it is not practical to use a PKIX service provider's default revocation checking mechanism or when an alternative revocation checking mechanism is to be substituted (by also calling the addCertPathChecker or setCertPathCheckers methods).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found the same docs, @anmolnar .

The code agrees that this defaults to the value of "com.sun.net.ssl.checkRevocation".

https://github.com/openjdk/jdk/blob/7c13a2cd9aa5ec9da00084de2388abc189e2f4ef/src/java.base/share/classes/sun/security/validator/PKIXValidator.java#L174

I think that's fine, the goal here is to use the JDK default settings.
This one CAN be overriden separately with the new property if we use a custom truststore.

(Also note that I have slightly changed the logic in the current #2277 PR).

}

// Revocation checking is only supported with the PKIX algorithm
TrustManagerFactory tmf = TrustManagerFactory.getInstance("PKIX");
tmf.init(new CertPathTrustManagerParameters(pbParams));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,18 @@ private void parseProperties(Properties cfg) {
}
}

/**
* Returns {@code Boolean.True} if and only if the property named by the argument
* exists and is case insensitive equal to the string {@code "true"}.
* Returns {@code nuln} if and only if the property named by the argument
* exists and is case insensitive equal to the string {@code "system"}.
* Returns {@code Boolean.False} otherwise.
*/
public TriState getTriState(String key) {
String propertyValue = getProperty(key);
return TriState.parse(propertyValue);
}

/**
* Returns {@code true} if and only if the property named by the argument
* exists and is equal to the string {@code "true"}.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import javax.servlet.http.HttpServletRequest;
import org.apache.zookeeper.KeeperException;
import org.apache.zookeeper.common.ClientX509Util;
import org.apache.zookeeper.common.TriState;
import org.apache.zookeeper.common.X509Exception;
import org.apache.zookeeper.common.X509Exception.KeyManagerException;
import org.apache.zookeeper.common.X509Exception.TrustManagerException;
Expand Down Expand Up @@ -85,8 +86,8 @@ public X509AuthenticationProvider() throws X509Exception {
x509Util.getSslKeystorePasswdPathProperty());
String keyStoreTypeProp = config.getProperty(x509Util.getSslKeystoreTypeProperty());

boolean crlEnabled = Boolean.parseBoolean(config.getProperty(x509Util.getSslCrlEnabledProperty()));
boolean ocspEnabled = Boolean.parseBoolean(config.getProperty(x509Util.getSslOcspEnabledProperty()));
TriState crlEnabled = config.getTriState(x509Util.getSslOcspEnabledProperty());
TriState ocspEnabled = config.getTriState(x509Util.getSslOcspEnabledProperty());
boolean hostnameVerificationEnabled = Boolean.parseBoolean(config.getProperty(x509Util.getSslHostnameVerificationEnabledProperty()));

X509KeyManager km = null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,42 @@ public void testOCSPEnabled(
assertTrue(Boolean.valueOf(Security.getProperty("ocsp.enable")));
}

@ParameterizedTest
@MethodSource("data")
@Timeout(value = 5)
public void testOCSPCRLTransparentFalse(
X509KeyType caKeyType, X509KeyType certKeyType, String keyPassword, Integer paramIndex)
throws Exception {
System.setProperty("com.sun.net.ssl.checkRevocation", Boolean.FALSE.toString());
System.setProperty("com.sun.security.enableCRLDP", Boolean.FALSE.toString());
Security.setProperty("ocsp.enable", Boolean.FALSE.toString());
init(caKeyType, certKeyType, keyPassword, paramIndex);
System.setProperty(x509Util.getSslOcspEnabledProperty(), "system");
System.setProperty(x509Util.getSslCrlEnabledProperty(), "system");
x509Util.getDefaultSSLContext();
assertFalse(Boolean.valueOf(System.getProperty("com.sun.net.ssl.checkRevocation")));
assertFalse(Boolean.valueOf(System.getProperty("com.sun.security.enableCRLDP")));
assertFalse(Boolean.valueOf(Security.getProperty("ocsp.enable")));
}

@ParameterizedTest
@MethodSource("data")
@Timeout(value = 5)
public void testOCSPCRLTransparentTrue(
X509KeyType caKeyType, X509KeyType certKeyType, String keyPassword, Integer paramIndex)
throws Exception {
System.setProperty("com.sun.net.ssl.checkRevocation", Boolean.TRUE.toString());
System.setProperty("com.sun.security.enableCRLDP", Boolean.TRUE.toString());
Security.setProperty("ocsp.enable", Boolean.TRUE.toString());
init(caKeyType, certKeyType, keyPassword, paramIndex);
System.setProperty(x509Util.getSslOcspEnabledProperty(), "system");
System.setProperty(x509Util.getSslCrlEnabledProperty(), "system");
x509Util.getDefaultSSLContext();
assertTrue(Boolean.valueOf(System.getProperty("com.sun.net.ssl.checkRevocation")));
assertTrue(Boolean.valueOf(System.getProperty("com.sun.security.enableCRLDP")));
assertTrue(Boolean.valueOf(Security.getProperty("ocsp.enable")));
}

@ParameterizedTest
@MethodSource("data")
@Timeout(value = 5)
Expand Down Expand Up @@ -373,8 +409,8 @@ public void testLoadPEMTrustStore(
X509TrustManager tm = X509Util.createTrustManager(
x509TestContext.getTrustStoreFile(KeyStoreFileType.PEM).getAbsolutePath(),
x509TestContext.getTrustStorePassword(), KeyStoreFileType.PEM.getPropertyValue(),
false,
false,
TriState.False,
TriState.False,
true,
true,
false);
Expand All @@ -394,8 +430,8 @@ public void testLoadPEMTrustStoreNullPassword(
x509TestContext.getTrustStoreFile(KeyStoreFileType.PEM).getAbsolutePath(),
null,
KeyStoreFileType.PEM.getPropertyValue(),
false,
false,
TriState.False,
TriState.False,
true,
true,
false);
Expand All @@ -413,8 +449,8 @@ public void testLoadPEMTrustStoreAutodetectStoreFileType(
x509TestContext.getTrustStoreFile(KeyStoreFileType.PEM).getAbsolutePath(),
x509TestContext.getTrustStorePassword(),
null, // null StoreFileType means 'autodetect from file extension'
false,
false,
TriState.False,
TriState.False,
true,
true,
false);
Expand Down Expand Up @@ -488,8 +524,8 @@ public void testLoadJKSTrustStore(
x509TestContext.getTrustStoreFile(KeyStoreFileType.JKS).getAbsolutePath(),
x509TestContext.getTrustStorePassword(),
KeyStoreFileType.JKS.getPropertyValue(),
true,
true,
TriState.True,
TriState.True,
true,
true,
false);
Expand All @@ -509,8 +545,8 @@ public void testLoadJKSTrustStoreNullPassword(
x509TestContext.getTrustStoreFile(KeyStoreFileType.JKS).getAbsolutePath(),
null,
KeyStoreFileType.JKS.getPropertyValue(),
false,
false,
TriState.False,
TriState.False,
true,
true,
false);
Expand All @@ -527,8 +563,8 @@ public void testLoadJKSTrustStoreAutodetectStoreFileType(
x509TestContext.getTrustStoreFile(KeyStoreFileType.JKS).getAbsolutePath(),
x509TestContext.getTrustStorePassword(),
null, // null StoreFileType means 'autodetect from file extension'
true,
true,
TriState.True,
TriState.True,
true,
true,
false);
Expand All @@ -546,8 +582,8 @@ public void testLoadJKSTrustStoreWithWrongPassword(
x509TestContext.getTrustStoreFile(KeyStoreFileType.JKS).getAbsolutePath(),
"wrong password",
KeyStoreFileType.JKS.getPropertyValue(),
true,
true,
TriState.True,
TriState.True,
true,
true,
false);
Expand Down Expand Up @@ -621,8 +657,8 @@ public void testLoadPKCS12TrustStore(
X509TrustManager tm = X509Util.createTrustManager(
x509TestContext.getTrustStoreFile(KeyStoreFileType.PKCS12).getAbsolutePath(),
x509TestContext.getTrustStorePassword(), KeyStoreFileType.PKCS12.getPropertyValue(),
true,
true,
TriState.True,
TriState.True,
true,
true,
false);
Expand All @@ -642,8 +678,8 @@ public void testLoadPKCS12TrustStoreNullPassword(
x509TestContext.getTrustStoreFile(KeyStoreFileType.PKCS12).getAbsolutePath(),
null,
KeyStoreFileType.PKCS12.getPropertyValue(),
false,
false,
TriState.False,
TriState.False,
true,
true,
false);
Expand All @@ -660,8 +696,8 @@ public void testLoadPKCS12TrustStoreAutodetectStoreFileType(
x509TestContext.getTrustStoreFile(KeyStoreFileType.PKCS12).getAbsolutePath(),
x509TestContext.getTrustStorePassword(),
null, // null StoreFileType means 'autodetect from file extension'
true,
true,
TriState.True,
TriState.True,
true,
true,
false);
Expand All @@ -679,8 +715,8 @@ public void testLoadPKCS12TrustStoreWithWrongPassword(
x509TestContext.getTrustStoreFile(KeyStoreFileType.PKCS12).getAbsolutePath(),
"wrong password",
KeyStoreFileType.PKCS12.getPropertyValue(),
true,
true,
TriState.True,
TriState.True,
true,
true,
false);
Expand Down
Loading