stoty commented on code in PR #2271:
URL: https://github.com/apache/zookeeper/pull/2271#discussion_r2189966523


##########
zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java:
##########
@@ -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);
+                   }

Review Comment:
   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).
   



-- 
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

Reply via email to