stoty commented on code in PR #2289:
URL: https://github.com/apache/zookeeper/pull/2289#discussion_r2260852403
##########
zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java:
##########
@@ -541,21 +548,32 @@ public static X509TrustManager createTrustManager(
boolean ocspEnabled,
final boolean serverHostnameVerificationEnabled,
final boolean clientHostnameVerificationEnabled,
- final boolean fipsMode) throws TrustManagerException {
+ final boolean fipsMode,
+ final SslRevocationEnabled revocationEnabled) throws
TrustManagerException {
if (trustStorePassword == null) {
trustStorePassword = "";
}
try {
KeyStore ts = loadTrustStore(trustStoreLocation,
trustStorePassword, trustStoreTypeProp);
PKIXBuilderParameters pbParams = new PKIXBuilderParameters(ts, new
X509CertSelector());
if (crlEnabled || ocspEnabled) {
- pbParams.setRevocationEnabled(true);
+ if (revocationEnabled == SslRevocationEnabled.LEGACY) {
+ 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 {
+ if (revocationEnabled == SslRevocationEnabled.LEGACY) {
+ pbParams.setRevocationEnabled(false);
+ }
+ }
+
+ if (revocationEnabled == SslRevocationEnabled.TRUE) {
+ pbParams.setRevocationEnabled(true);
+ } else if (revocationEnabled == SslRevocationEnabled.FALSE) {
Review Comment:
> `revocationEnabled` is default to `true` in java since java-8. In old
path, it clears `revocationEnabled`.
That's only true sometimes.
It is enabled by default in SunJCE, but (only by looking at the code, not
tested) it is disabled by default in BouncyCastle. There also other
Cryptography providers where the behaviour may be different.
>
> I don't think it deserves `JAVA_DEFAULT` for a public default. I think we
could shape the candidates:
>
> 1. `legacy`: behaves same as old, this should be the default in case of
backport.
> 2. `true` or `false` to enable/disalbe revocation. `true` could be the
default.
The case for having a default NoOp option is that there are already
mechanisms and defaults in the JVM that can be used to set this up.
There can be any number of HTTP clients (and servers) in an application that
includes the ZK client, and a user may want to threat these in a unified way.
By not exposing a JVM default options, we force the user to always
separately consider the ZK clients.
(Servers are less of a problem, as those are standlone processes)
"Yes, you have set up your JVM system properties to enable/disable CRL
globally, but now you also need to make a separate change for the ZK config
anyway" is not user / admin friendly.
My preference would be defaulting to "java_default" for 3.10 and "legacy"
for backports.
>
> *
https://devdocs.io/openjdk~8/java/security/cert/pkixparameters#setRevocationEnabled-boolean-
> *
https://devdocs.io/openjdk~21/java.base/java/security/cert/pkixparameters#setRevocationEnabled(boolean)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
