ZOOKEEPER-4955: Fix intererence with jvm wide ssl properties for ssl.crl and ssl.ocsp (#2339)

This is a cherry-pick commit from
770804b and
45b49a5.

Author: kezhuw

zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md

@@ -1779,6 +1779,10 @@ and [SASL authentication for ZooKeeper](https://cwiki.apache.org/confluence/disp
     (Java system properties: **zookeeper.ssl.ocsp** and **zookeeper.ssl.quorum.ocsp**)
     **New in 3.5.5:**
     Specifies whether Online Certificate Status Protocol is enabled in client and quorum TLS protocols.
+    **Changed in next feature version:**
+    Currently, *ssl.ocsp* and *ssl.quorum.ocsp* implies *ssl.crl* and *ssl.quorum.crl* correspondingly.
+    In next feature release, one has to setup both *ssl.crl* and *ssl.ocsp* (or *ssl.quorum.crl* and *ssl.quorum.ocsp*)
+    to enable OCSP. This is consistent with jdk's method of [Setting up a Java Client to use Client-Driven OCSP](https://docs.oracle.com/javase/8/docs/technotes/guides/security/jsse/ocsp.html#setting-up-a-java-client-to-use-client-driven-ocsp).
     Default: false
 
 * *ssl.clientAuth* and *ssl.quorum.clientAuth* :

zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java

@@ -31,12 +31,16 @@
 import java.security.KeyStore;
 import java.security.NoSuchAlgorithmException;
 import java.security.Security;
+import java.security.cert.CertPathValidator;
 import java.security.cert.PKIXBuilderParameters;
+import java.security.cert.PKIXRevocationChecker;
 import java.security.cert.X509CertSelector;
 import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Objects;
+import java.util.Set;
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.function.Supplier;
 import java.util.stream.Collectors;
@@ -598,12 +602,36 @@ public static X509TrustManager createTrustManager(
             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");
+                // See [RevocationChecker][1] for details. Basically, we are mimicking the legacy path,
+                // which relies significantly on jvm wide properties[2], as that is the path we are routing
+                // before (i.e. no explicit `PKIXRevocationChecker`).
+                //
+                // By reading but not writing these properties, we conform to but not interfere with what
+                // admin set while still keep backward compatibility.
+                // 1. Default "zookeeper.ssl.crl" to jvm property "com.sun.net.ssl.checkRevocation" if it is unset in upcoming feature version.
+                // 2. Default "zookeeper.ssl.ocsp" to jvm security property "ocsp.enable" if it is unset in upcoming feature version.
+                // 3. Set `Option.ONLY_END_ENTITY` for jvm security property "com.sun.security.onlyCheckRevocationOfEECert".
+                // 4. Don't set "com.sun.security.enableCRLDP" as it is always enabled in no legacy path.
+                //
+                // See also:
+                // * https://docs.oracle.com/javase/8/docs/technotes/guides/security/jsse/ocsp.html
+                // * https://docs.oracle.com/javase/8/docs/technotes/guides/security/jsse/JSSERefGuide.html
+                // * https://docs.oracle.com/javase/8/docs/technotes/guides/security/certpath/CertPathProgGuide.html#PKIXRevocationChecker
+                //
+                // [1]: https://github.com/openjdk/jdk/blob/jdk-11%2B28/src/java.base/share/classes/sun/security/provider/certpath/RevocationChecker.java#L124
+                // [2]: https://github.com/openjdk/jdk/blob/jdk-11%2B28/src/java.base/share/classes/sun/security/provider/certpath/RevocationChecker.java#L179
+                Set<PKIXRevocationChecker.Option> options = new HashSet<>();
+                if (!ocspEnabled) {
+                    options.add(PKIXRevocationChecker.Option.NO_FALLBACK);
+                    options.add(PKIXRevocationChecker.Option.PREFER_CRLS);
                 }
+                if (Boolean.parseBoolean(Security.getProperty("com.sun.security.onlyCheckRevocationOfEECert")))  {
+                    options.add(PKIXRevocationChecker.Option.ONLY_END_ENTITY);
+                }
+
+                PKIXRevocationChecker revocationChecker = (PKIXRevocationChecker) CertPathValidator.getInstance("PKIX").getRevocationChecker();
+                revocationChecker.setOptions(options);
+                pbParams.addCertPathChecker(revocationChecker);
             } else {
                 pbParams.setRevocationEnabled(false);
             }

zookeeper-server/src/main/java/org/apache/zookeeper/server/auth/X509AuthenticationProvider.java

@@ -85,8 +85,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()));
+            boolean crlEnabled = config.getBoolean(x509Util.getSslCrlEnabledProperty());
+            boolean ocspEnabled = config.getBoolean(x509Util.getSslOcspEnabledProperty());
             boolean hostnameVerificationEnabled = Boolean.parseBoolean(config.getProperty(x509Util.getSslHostnameVerificationEnabledProperty()));
             boolean clientHostnameVerificationEnabled = x509Util.isClientHostnameVerificationEnabled(config);
             boolean allowReverseDnsLookup = Boolean.parseBoolean(config.getProperty(x509Util.getSslAllowReverseDnsLookupProperty()));

zookeeper-server/src/test/java/org/apache/zookeeper/common/X509TestContext.java

@@ -80,7 +80,7 @@ public class X509TestContext {
      * @throws GeneralSecurityException
      * @throws OperatorCreationException
      */
-    private X509TestContext(File tempDir, KeyPair trustStoreKeyPair, long trustStoreCertExpirationMillis, String trustStorePassword, KeyPair keyStoreKeyPair, long keyStoreCertExpirationMillis, String keyStorePassword, Boolean hostnameVerification) throws IOException, GeneralSecurityException, OperatorCreationException {
+    private X509TestContext(File tempDir, KeyPair trustStoreKeyPair, long trustStoreCertExpirationMillis, String trustStorePassword, KeyPair keyStoreKeyPair, long keyStoreCertExpirationMillis, String keyStorePassword, Boolean hostnameVerification) throws Exception {
         if (Security.getProvider(BouncyCastleProvider.PROVIDER_NAME) == null) {
             throw new IllegalStateException("BC Security provider was not found");
         }
@@ -425,7 +425,7 @@ public Builder() {
          * @throws GeneralSecurityException
          * @throws OperatorCreationException
          */
-        public X509TestContext build() throws IOException, GeneralSecurityException, OperatorCreationException {
+        public X509TestContext build() throws Exception {
             KeyPair trustStoreKeyPair = X509TestHelpers.generateKeyPair(trustStoreKeyType);
             KeyPair keyStoreKeyPair = X509TestHelpers.generateKeyPair(keyStoreKeyType);
             return new X509TestContext(tempDir, trustStoreKeyPair, trustStoreCertExpirationMillis, trustStorePassword, keyStoreKeyPair, keyStoreCertExpirationMillis, keyStorePassword, hostnameVerification);

zookeeper-server/src/test/java/org/apache/zookeeper/common/X509TestHelpers.java

@@ -36,11 +36,14 @@
 import java.security.cert.X509Certificate;
 import java.security.spec.ECGenParameterSpec;
 import java.security.spec.RSAKeyGenParameterSpec;
+import java.time.Duration;
 import java.util.Date;
 import org.bouncycastle.asn1.DERIA5String;
 import org.bouncycastle.asn1.DEROctetString;
 import org.bouncycastle.asn1.pkcs.PKCSObjectIdentifiers;
 import org.bouncycastle.asn1.x500.X500Name;
+import org.bouncycastle.asn1.x500.X500NameBuilder;
+import org.bouncycastle.asn1.x500.style.BCStyle;
 import org.bouncycastle.asn1.x509.AlgorithmIdentifier;
 import org.bouncycastle.asn1.x509.BasicConstraints;
 import org.bouncycastle.asn1.x509.ExtendedKeyUsage;
@@ -53,6 +56,7 @@
 import org.bouncycastle.cert.X509CertificateHolder;
 import org.bouncycastle.cert.X509v3CertificateBuilder;
 import org.bouncycastle.cert.jcajce.JcaX509CertificateConverter;
+import org.bouncycastle.cert.jcajce.JcaX509v3CertificateBuilder;
 import org.bouncycastle.crypto.params.AsymmetricKeyParameter;
 import org.bouncycastle.crypto.util.PrivateKeyFactory;
 import org.bouncycastle.jce.provider.BouncyCastleProvider;
@@ -85,6 +89,27 @@ public class X509TestHelpers {
     // Per RFC 5280 section 4.1.2.2, X509 certificates can use up to 20 bytes == 160 bits for serial numbers.
     private static final int SERIAL_NUMBER_MAX_BITS = 20 * Byte.SIZE;
 
+    public static X509Certificate newSelfSignedCert(String name, KeyPair keyPair) throws IOException, OperatorCreationException, GeneralSecurityException {
+        X500NameBuilder caNameBuilder = new X500NameBuilder(BCStyle.INSTANCE);
+        caNameBuilder.addRDN(BCStyle.CN, name);
+        return newSelfSignedCACert(caNameBuilder.build(), keyPair, Duration.ofDays(1).toMillis());
+    }
+
+    @FunctionalInterface
+    public interface CertificateCustomization {
+        void customize(X509v3CertificateBuilder builder) throws Exception;
+    }
+
+    public static X509Certificate newCert(X509Certificate caCert, KeyPair caKeyPair, String name, PublicKey certPublicKey, CertificateCustomization customization) throws Exception {
+        X500NameBuilder nameBuilder = new X500NameBuilder(BCStyle.INSTANCE);
+        nameBuilder.addRDN(BCStyle.CN, name);
+        return newCert(caCert, caKeyPair, nameBuilder.build(), certPublicKey, Duration.ofDays(1).toMillis(), customization);
+    }
+
+    public static X509Certificate newCert(X509Certificate caCert, KeyPair caKeyPair, String name, PublicKey certPublicKey) throws Exception {
+        return newCert(caCert, caKeyPair, name, certPublicKey, null);
+    }
+
     /**
      * Uses the private key of the given key pair to create a self-signed CA certificate with the public half of the
      * key pair and the given subject and expiration. The issuer of the new cert will be equal to the subject.
@@ -102,6 +127,11 @@ public class X509TestHelpers {
      */
     public static X509Certificate newSelfSignedCACert(
             X500Name subject, KeyPair keyPair, long expirationMillis) throws IOException, OperatorCreationException, GeneralSecurityException {
+        return newSelfSignedCACert(subject, keyPair, expirationMillis, null);
+    }
+
+    public static X509Certificate newSelfSignedCACert(
+            X500Name subject, KeyPair keyPair, long expirationMillis, CertificateCustomization customization) throws IOException, OperatorCreationException, GeneralSecurityException {
         Date now = new Date();
         X509v3CertificateBuilder builder = initCertBuilder(subject, // for self-signed certs, issuer == subject
                                                            now, new Date(now.getTime()
@@ -129,7 +159,12 @@ now, new Date(now.getTime()
      * @throws GeneralSecurityException
      */
     public static X509Certificate newCert(
-            X509Certificate caCert, KeyPair caKeyPair, X500Name certSubject, PublicKey certPublicKey, long expirationMillis) throws IOException, OperatorCreationException, GeneralSecurityException {
+            X509Certificate caCert, KeyPair caKeyPair, X500Name certSubject, PublicKey certPublicKey, long expirationMillis) throws Exception {
+        return newCert(caCert, caKeyPair, certSubject, certPublicKey, expirationMillis, null);
+    }
+
+    public static X509Certificate newCert(
+            X509Certificate caCert, KeyPair caKeyPair, X500Name certSubject, PublicKey certPublicKey, long expirationMillis, CertificateCustomization customization) throws Exception {
         if (!caKeyPair.getPublic().equals(caCert.getPublicKey())) {
             throw new IllegalArgumentException("CA private key does not match the public key in the CA cert");
         }
@@ -143,6 +178,9 @@ public static X509Certificate newCert(
         builder.addExtension(Extension.extendedKeyUsage, true, new ExtendedKeyUsage(new KeyPurposeId[]{KeyPurposeId.id_kp_serverAuth, KeyPurposeId.id_kp_clientAuth}));
 
         builder.addExtension(Extension.subjectAlternativeName, false, getLocalhostSubjectAltNames());
+        if (customization != null) {
+            customization.customize(builder);
+        }
         return buildAndSignCertificate(caKeyPair.getPrivate(), builder);
     }
 
@@ -172,7 +210,7 @@ private static GeneralNames getLocalhostSubjectAltNames() throws UnknownHostExce
      */
     private static X509v3CertificateBuilder initCertBuilder(
             X500Name issuer, Date notBefore, Date notAfter, X500Name subject, PublicKey subjectPublicKey) {
-        return new X509v3CertificateBuilder(issuer, new BigInteger(SERIAL_NUMBER_MAX_BITS, PRNG), notBefore, notAfter, subject, SubjectPublicKeyInfo.getInstance(subjectPublicKey.getEncoded()));
+        return new JcaX509v3CertificateBuilder(issuer, new BigInteger(SERIAL_NUMBER_MAX_BITS, PRNG), notBefore, notAfter, subject, SubjectPublicKeyInfo.getInstance(subjectPublicKey.getEncoded()));
     }
 
     /**

zookeeper-server/src/test/java/org/apache/zookeeper/common/X509UtilTest.java

@@ -20,7 +20,6 @@
 
 import static org.junit.jupiter.api.Assertions.assertArrayEquals;
 import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import io.netty.buffer.UnpooledByteBufAllocator;
@@ -32,7 +31,6 @@
 import java.net.Socket;
 import java.nio.file.Path;
 import java.security.NoSuchAlgorithmException;
-import java.security.Security;
 import java.util.Arrays;
 import java.util.List;
 import java.util.concurrent.Callable;
@@ -90,10 +88,6 @@ public void cleanUp() {
         System.clearProperty(x509Util.getCipherSuitesProperty());
         System.clearProperty(x509Util.getSslProtocolProperty());
         System.clearProperty(x509Util.getSslHandshakeDetectionTimeoutMillisProperty());
-        System.clearProperty("com.sun.net.ssl.checkRevocation");
-        System.clearProperty("com.sun.security.enableCRLDP");
-        Security.setProperty("ocsp.enable", Boolean.FALSE.toString());
-        Security.setProperty("com.sun.security.enableCRLDP", Boolean.FALSE.toString());
         System.clearProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN_FACTORY);
         System.clearProperty(ZKClientConfig.ZOOKEEPER_CLIENT_CNXN_SOCKET);
         x509Util.close();
@@ -227,49 +221,6 @@ public void testCreateSSLContextWithCustomCipherSuites(
         assertArrayEquals(customCipherSuites, sslSocket.getEnabledCipherSuites());
     }
 
-    // It would be great to test the value of PKIXBuilderParameters#setRevocationEnabled but it does not appear to be
-    // possible
-    @ParameterizedTest
-    @MethodSource("data")
-    @Timeout(value = 5)
-    public void testCRLEnabled(
-            X509KeyType caKeyType, X509KeyType certKeyType, String keyPassword, Integer paramIndex)
-            throws Exception {
-        init(caKeyType, certKeyType, keyPassword, paramIndex);
-        System.setProperty(x509Util.getSslCrlEnabledProperty(), "true");
-        x509Util.getDefaultSSLContext();
-        assertTrue(Boolean.valueOf(System.getProperty("com.sun.net.ssl.checkRevocation")));
-        assertTrue(Boolean.valueOf(System.getProperty("com.sun.security.enableCRLDP")));
-        assertFalse(Boolean.valueOf(Security.getProperty("ocsp.enable")));
-    }
-
-    @ParameterizedTest
-    @MethodSource("data")
-    @Timeout(value = 5)
-    public void testCRLDisabled(
-            X509KeyType caKeyType, X509KeyType certKeyType, String keyPassword, Integer paramIndex)
-            throws Exception {
-        init(caKeyType, certKeyType, keyPassword, paramIndex);
-        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 testOCSPEnabled(
-            X509KeyType caKeyType, X509KeyType certKeyType, String keyPassword, Integer paramIndex)
-            throws Exception {
-        init(caKeyType, certKeyType, keyPassword, paramIndex);
-        System.setProperty(x509Util.getSslOcspEnabledProperty(), "true");
-        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)