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

ZOOKEEPER-4955: Fix intererence with jvm wide ssl properties for ssl.crl and ssl.ocsp
Refs: ZOOKEEPER-4955, #2289, stoty#2
Add more comments about PKIXRevocationChecker options
Rename test methods to reflect where revocation happens
Refactor tests to reflect that revocation checking affects is only enforced in enabled side
Fix missing fallback property for ssl.ocsp
Reviewers: anmolnar, stoty
Author: kezhuw
Closes #2292 from kezhuw/ZOOKEEPER-4955-fix-interference-with-jvm-properties

Author: kezhuw

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

@@ -1768,13 +1768,17 @@ and [SASL authentication for ZooKeeper](https://cwiki.apache.org/confluence/disp
     (Java system properties: **zookeeper.ssl.crl** and **zookeeper.ssl.quorum.crl**)
     **New in 3.5.5:**
     Specifies whether Certificate Revocation List is enabled in client and quorum TLS protocols.
-    Default: false
+    Default: jvm property "com.sun.net.ssl.checkRevocation" since 3.10.0, false otherwise
 
 * *ssl.ocsp* and *ssl.quorum.ocsp* :
     (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.
-    Default: false
+    **Changed in 3.10.0:**
+    Before 3.10.0, *ssl.ocsp* and *ssl.quorum.ocsp* implies *ssl.crl* and *ssl.quorum.crl* correspondingly.
+    After 3.10.0, 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: jvm security property "ocsp.enable" since 3.10.0, false otherwise
 
 * *ssl.clientAuth* and *ssl.quorum.clientAuth* :
     (Java system properties: **zookeeper.ssl.clientAuth** and **zookeeper.ssl.quorum.clientAuth**)

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

@@ -23,6 +23,7 @@
 import io.netty.handler.ssl.SslContext;
 import io.netty.handler.ssl.SslContextBuilder;
 import io.netty.handler.ssl.SslProvider;
+import java.security.Security;
 import java.util.Arrays;
 import javax.net.ssl.KeyManager;
 import javax.net.ssl.SSLEngine;
@@ -148,7 +149,7 @@ public SslContext createNettySslContextForServer(ZKConfig config, KeyManager key
     private SslContextBuilder handleTcnativeOcspStapling(SslContextBuilder builder, ZKConfig config) {
         SslProvider sslProvider = getSslProvider(config);
         boolean tcnative = sslProvider == SslProvider.OPENSSL || sslProvider == SslProvider.OPENSSL_REFCNT;
-        boolean ocspEnabled = config.getBoolean(getSslOcspEnabledProperty());
+        boolean ocspEnabled = config.getBoolean(getSslOcspEnabledProperty(), Boolean.parseBoolean(Security.getProperty("ocsp.enable")));
 
         if (tcnative && ocspEnabled && OpenSsl.isOcspSupported()) {
             builder.enableOcsp(ocspEnabled);
@@ -201,8 +202,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());
+        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);
 

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

@@ -31,11 +31,15 @@
 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.Set;
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.function.Supplier;
 import javax.net.ssl.CertPathTrustManagerParameters;
@@ -392,8 +396,9 @@ 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);
+        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 fipsMode = getFipsMode(config);
@@ -548,13 +553,37 @@ 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");
+            if (crlEnabled) {
+                // 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.
+                // 2. Default "zookeeper.ssl.ocsp" to jvm security property "ocsp.enable" if it is unset.
+                // 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

@@ -18,6 +18,7 @@
 
 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;
@@ -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()));
+            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()));
 
             X509KeyManager km = null;

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()));
     }
 
     /**