ZOOKEEPER-4949: Clean up TLS CRL/OCSP configuration

- Enable FIPS style server hostname verification if truststore is not specified
- Make sure tcnative specific enableOCSP method is not called for JRE SSL provider
- Add new config option to enable tcnative specific enableOCSP method
- Add new config option to separetely enable certificate revocation checking for custom truststores
- Add new config option to disable existing implicit certificate revocation checking logic for custom truststores
- Document dependencies of TLS truststore related options

Author: stoty

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

@@ -1687,7 +1687,7 @@ and [SASL authentication for ZooKeeper](https://cwiki.apache.org/confluence/disp
     Specifies the file path to a Java keystore containing the local
     credentials to be used for client and quorum TLS connections, and the
     password to unlock the file.
-    
+
 * *ssl.keyStore.passwordPath* and *ssl.quorum.keyStore.passwordPath* :
     (Java system properties: **zookeeper.ssl.keyStore.passwordPath** and **zookeeper.ssl.quorum.keyStore.passwordPath**)
     **New in 3.8.0:**
@@ -1762,20 +1762,59 @@ and [SASL authentication for ZooKeeper](https://cwiki.apache.org/confluence/disp
     **New in 3.9.4:**
     Specifies whether the client's hostname verification is enabled in client and quorum TLS negotiation process.
     This option requires the corresponding *hostnameVerification* option to be `true`, or it will be ignored.
+    *ssl.clientHostnameVerification* has no effect, as the client does not receive TLS connections.
     Default: true for quorum, false for clients
 
 * *ssl.crl* and *ssl.quorum.crl* :
     (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.
+    This option requires that the *ssl.quorum.trustStore.location* is set, or it will be ignored.
+    Enabling this option will set the JVM global "com.sun.security.enableCRLDP" system property to true.
     Default: false
 
 * *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.
+    This option requires that the *ssl.trustStore.location* is set, or it will be ignored for the client.
+    Enabling this option will set the JVM global "com.sun.security.enableCRLDP" system property to true.
+    Enabling this option will set JVM global "ocsp.enable" security property to true.
     Default: false
 
+* *ssl.revocationEnabled* and *ssl.quorum.revocationEnabled* :
+    (Java system properties: **zookeeper.ssl.revocationEnabled** and **zookeeper.ssl.quorum.revocationEnabled**)
+    **New in 3.10.0:**
+    Specifies whether Certificate Revocation checking is enabled in client and quorum TLS protocols.
+    This option requires that the *ssl.trustStore.location* is set, or it will be ignored for the client.
+    This options has no side effects on JVM global system properties.
+    Default: if the option is not set, or set to the value "default" then the JVM default settings
+    are used.
+
+* *ssl.disableLegacyRevocationLogic* and *ssl.quorum.disableLegacyRevocationLogic* :
+    (Java system properties: **zookeeper.ssl.disableLegacyRevocationLogic** and **zookeeper.ssl.quorum.disableLegacyRevocationLogic**)
+    **New in 3.10.0:**
+    Traditionally, ZK has always explicitly disabled or enabled certificate revocation in the trustmanager based on the values of the
+    *ssl.ocsp* and *ssl.crl* properties. This made it impossible to completely rely on System properties for configuring
+    revocation checking for ZooKeeper.
+    Setting this property disables the logic for implicitly setting the certificate revocation check status on the trustManager,
+    allowing the JVM default behavior to be used.
+    It is recommended to set *ssl.ocsp* and *ssl.crl* to false, and *ssl.disableLegacyRevocationLogic* to true.
+    With those settings, ZK will use the JVM default CRL/OCSP settings, and not change the defaults. *ssl.revocationEnabled*
+    can still be used to enable/disable revocation checking for the custom SSL truststore.
+    This option requires that the *ssl.trustStore.location* is set, or it will be ignored for the client.
+    Default: false.
+
+* *ssl.tcnative.ocsp* and *ssl.quorum.tcnative.ocsp* :
+    (Java system properties: **zookeeper.ssl.tcnative.ocsp** and **zookeeper.ssl.quorum.tcnative.ocsp**)
+    **New in 3.10.0:**
+    Specifies whether OCSP stapling is requested by the client.
+    This option has no effect unless the the OpenSSL tcnative SSL provider with the OpenSSL library is used.
+    Note that Zookeeper uses the the BoringSSL tcnative library by default, so even is the "OpenSSL" SSL provider is requested,
+    this won't do anything unless the default BoringSSL library is replaced with the OpenSSL one.
+    This options has no side effects on JVM global system properties.
+    Default: if the option is not set, or set to the value "default" then the library default is used.
+
 * *ssl.clientAuth* and *ssl.quorum.clientAuth* :
     (Java system properties: **zookeeper.ssl.clientAuth** and **zookeeper.ssl.quorum.clientAuth**)
     **Added in 3.5.5, but broken until 3.5.7:**
@@ -1785,7 +1824,8 @@ and [SASL authentication for ZooKeeper](https://cwiki.apache.org/confluence/disp
      * "want": server will "request" client authentication 
      * "need": server will "require" client authentication
 
-     Default: "need"
+    *ssl.clientHostnameVerification* has no effect.
+    Default: "need"
 
 * *ssl.handshakeDetectionTimeoutMillis* and *ssl.quorum.handshakeDetectionTimeoutMillis* :
     (Java system properties: **zookeeper.ssl.handshakeDetectionTimeoutMillis** and **zookeeper.ssl.quorum.handshakeDetectionTimeoutMillis**)

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

@@ -79,7 +79,19 @@ public SslContext createNettySslContextForClient(ZKConfig config)
             sslContextBuilder.trustManager(tm);
         }
 
-        sslContextBuilder.enableOcsp(config.getBoolean(getSslOcspEnabledProperty()));
+        SslProvider sslProvider = getSslProvider(config);
+        sslContextBuilder.sslProvider(sslProvider);
+        // Note that sslContextBuilder.enableOcsp() doesn't do anything, unless the default BoringSSL
+        // tcnative dependency is replaced with an OpenSsl one.
+        if (sslProvider == SslProvider.OPENSSL || sslProvider == SslProvider.OPENSSL_REFCNT) {
+            sslContextBuilder.enableOcsp(config.getBoolean(getSslOcspEnabledProperty()));
+        }
+        // Explicit option takes precedence if set
+        if (config.getTristate(getSslTcnativeOcspEnabledProperty()).isTrue()) {
+            sslContextBuilder.enableOcsp(true);
+        } else if (config.getTristate(getSslTcnativeOcspEnabledProperty()).isFalse()) {
+            sslContextBuilder.enableOcsp(false);
+        }
         String[] enabledProtocols = getEnabledProtocols(config);
         if (enabledProtocols != null) {
             sslContextBuilder.protocols(enabledProtocols);
@@ -88,11 +100,11 @@ public SslContext createNettySslContextForClient(ZKConfig config)
         if (enabledCiphers != null) {
             sslContextBuilder.ciphers(enabledCiphers);
         }
-        sslContextBuilder.sslProvider(getSslProvider(config));
 
         SslContext sslContext1 = sslContextBuilder.build();
 
-        if (getFipsMode(config) && isServerHostnameVerificationEnabled(config)) {
+        if ((getFipsMode(config) || config.getProperty(getSslKeystoreLocationProperty(), "").isEmpty())
+                && isServerHostnameVerificationEnabled(config)) {
             return addHostnameVerification(sslContext1, "Server");
         } else {
             return sslContext1;
@@ -123,7 +135,16 @@ public SslContext createNettySslContextForServer(ZKConfig config, KeyManager key
             sslContextBuilder.trustManager(trustManager);
         }
 
-        sslContextBuilder.enableOcsp(config.getBoolean(getSslOcspEnabledProperty()));
+        SslProvider sslProvider = getSslProvider(config);
+        sslContextBuilder.sslProvider(sslProvider);
+        if (sslProvider == SslProvider.OPENSSL || sslProvider == SslProvider.OPENSSL_REFCNT) {
+            sslContextBuilder.enableOcsp(config.getBoolean(getSslOcspEnabledProperty()));
+        }
+        if (config.getTristate(getSslTcnativeOcspEnabledProperty()).isTrue()) {
+            sslContextBuilder.enableOcsp(true);
+        } else if (config.getTristate(getSslTcnativeOcspEnabledProperty()).isFalse()) {
+            sslContextBuilder.enableOcsp(false);
+        }
         String[] enabledProtocols = getEnabledProtocols(config);
         if (enabledProtocols != null) {
             sslContextBuilder.protocols(enabledProtocols);
@@ -133,7 +154,6 @@ public SslContext createNettySslContextForServer(ZKConfig config, KeyManager key
         if (enabledCiphers != null) {
             sslContextBuilder.ciphers(enabledCiphers);
         }
-        sslContextBuilder.sslProvider(getSslProvider(config));
 
         SslContext sslContext1 = sslContextBuilder.build();
 
@@ -191,15 +211,21 @@ private TrustManager getTrustManager(ZKConfig config) throws X509Exception.Trust
 
         boolean sslCrlEnabled = config.getBoolean(getSslCrlEnabledProperty());
         boolean sslOcspEnabled = config.getBoolean(getSslOcspEnabledProperty());
+        TriState sslRevocationEnabled = config.getTristate(getSslRevocationEnabledProperty());
+        boolean disableLegacyRevocationLogic = config.getBoolean(getSslDisableLegacyRevocationLogicProperty());
         boolean sslServerHostnameVerificationEnabled = isServerHostnameVerificationEnabled(config);
         boolean sslClientHostnameVerificationEnabled = isClientHostnameVerificationEnabled(config);
 
         if (trustStoreLocation.isEmpty()) {
             LOG.warn("{} not specified", getSslTruststoreLocationProperty());
+            LOG.info("The following options are ignored: {}, {}, {}, {}.\n{} and {} will behave as in FIPS mode.",
+                    getSslCrlEnabledProperty(), getSslOcspEnabledProperty(), getSslRevocationEnabledProperty(),
+                    getSslDisableLegacyRevocationLogicProperty(), getSslHostnameVerificationEnabledProperty(),
+                    getSslClientHostnameVerificationEnabledProperty());
             return null;
         } else {
             return createTrustManager(trustStoreLocation, trustStorePassword, trustStoreType,
-                sslCrlEnabled, sslOcspEnabled, sslServerHostnameVerificationEnabled,
+                sslCrlEnabled, sslOcspEnabled, sslRevocationEnabled, disableLegacyRevocationLogic, sslServerHostnameVerificationEnabled,
                 sslClientHostnameVerificationEnabled, getFipsMode(config));
         }
     }

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

@@ -0,0 +1,53 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.zookeeper.common;
+
+/**
+ * For storing configuration parameters where want to distinguish a default case
+ * in addition to true and false.
+ */
+public enum TriState {
+    True, False, Default;
+
+    public static TriState parse(String value) {
+        if (value == null || value.equalsIgnoreCase("default")) {
+            return TriState.Default;
+        } else if (value.equalsIgnoreCase("true")) {
+            return TriState.True;
+        } else {
+            return TriState.False;
+        }
+    }
+
+    public boolean isTrue() {
+        return this == TriState.True;
+    }
+
+    public boolean isFalse() {
+        return this == TriState.False;
+    }
+
+    public boolean isDefault() {
+        return this == TriState.Default;
+    }
+
+    public boolean isNotDefault() {
+        return this != TriState.Default;
+    }
+}

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

@@ -164,6 +164,9 @@ public io.netty.handler.ssl.ClientAuth toNettyClientAuth() {
     private final String sslClientHostnameVerificationEnabledProperty = getConfigPrefix() + "clientHostnameVerification";
     private final String sslCrlEnabledProperty = getConfigPrefix() + "crl";
     private final String sslOcspEnabledProperty = getConfigPrefix() + "ocsp";
+    private final String sslRevocationEnabledProperty = getConfigPrefix() + "revocationEnabled";
+    private final String sslDisableLegacyRevocationLogic = getConfigPrefix() + "disableLegacyRevocationLogic";
+    private final String sslTcnativeOcspEnabledProperty = getConfigPrefix() + ".tcnative.ocsp";
     private final String sslClientAuthProperty = getConfigPrefix() + "clientAuth";
     private final String sslHandshakeDetectionTimeoutMillisProperty = getConfigPrefix() + "handshakeDetectionTimeoutMillis";
 
@@ -248,6 +251,18 @@ public String getSslOcspEnabledProperty() {
         return sslOcspEnabledProperty;
     }
 
+    public String getSslRevocationEnabledProperty() {
+        return sslRevocationEnabledProperty;
+    }
+
+    public String getSslTcnativeOcspEnabledProperty() {
+        return sslTcnativeOcspEnabledProperty;
+    }
+
+    public String getSslDisableLegacyRevocationLogicProperty() {
+        return sslDisableLegacyRevocationLogic;
+    }
+
     public String getSslClientAuthProperty() {
         return sslClientAuthProperty;
     }
@@ -394,6 +409,8 @@ public SSLContextAndOptions createSSLContextAndOptionsFromConfig(ZKConfig config
 
         boolean sslCrlEnabled = config.getBoolean(this.sslCrlEnabledProperty);
         boolean sslOcspEnabled = config.getBoolean(this.sslOcspEnabledProperty);
+        TriState sslRevocationEnabled = config.getTristate(this.sslRevocationEnabledProperty);
+        boolean disableLegacyRevocationLogic = config.getBoolean(this.sslDisableLegacyRevocationLogic);
         boolean sslServerHostnameVerificationEnabled = isServerHostnameVerificationEnabled(config);
         boolean sslClientHostnameVerificationEnabled = isClientHostnameVerificationEnabled(config);
         boolean fipsMode = getFipsMode(config);
@@ -404,7 +421,7 @@ public SSLContextAndOptions createSSLContextAndOptionsFromConfig(ZKConfig config
             try {
                 trustManagers = new TrustManager[]{
                     createTrustManager(trustStoreLocationProp, trustStorePasswordProp, trustStoreTypeProp, sslCrlEnabled,
-                        sslOcspEnabled, sslServerHostnameVerificationEnabled, sslClientHostnameVerificationEnabled,
+                        sslOcspEnabled, sslRevocationEnabled, disableLegacyRevocationLogic, sslServerHostnameVerificationEnabled, sslClientHostnameVerificationEnabled,
                         fipsMode)};
             } catch (TrustManagerException trustManagerException) {
                 throw new SSLContextException("Failed to create TrustManager", trustManagerException);
@@ -521,6 +538,9 @@ public static X509KeyManager createKeyManager(
      * @param crlEnabled                        enable CRL (certificate revocation list) checks.
      * @param ocspEnabled                       enable OCSP (online certificate status protocol)
      *                                          checks.
+     * @param revocationEnabled                 Enable certificate revocation checks
+     * @param disableLegacyRevocationLogic      if true certificate revocation will only be affected
+     *                                          by revocationEnabled and the JVM system properties
      * @param serverHostnameVerificationEnabled if true, verify hostnames of
      *                                          remote servers that client
      *                                          sockets created by this
@@ -539,6 +559,8 @@ public static X509TrustManager createTrustManager(
         String trustStoreTypeProp,
         boolean crlEnabled,
         boolean ocspEnabled,
+        TriState revocationEnabled,
+        boolean disableLegacyRevocationLogic,
         final boolean serverHostnameVerificationEnabled,
         final boolean clientHostnameVerificationEnabled,
         final boolean fipsMode) throws TrustManagerException {
@@ -549,13 +571,24 @@ public static X509TrustManager createTrustManager(
             KeyStore ts = loadTrustStore(trustStoreLocation, trustStorePassword, trustStoreTypeProp);
             PKIXBuilderParameters pbParams = new PKIXBuilderParameters(ts, new X509CertSelector());
             if (crlEnabled || ocspEnabled) {
-                pbParams.setRevocationEnabled(true);
+                if (!disableLegacyRevocationLogic) {
+                    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 (!disableLegacyRevocationLogic) {
+                    pbParams.setRevocationEnabled(false);
+                }
+            }
+
+            // Setting revocationEnabled explicitly takes precedence
+            if (revocationEnabled.isTrue()) {
+                pbParams.setRevocationEnabled(true);
+            } else if (revocationEnabled.isFalse()) {
                 pbParams.setRevocationEnabled(false);
             }
 

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

@@ -131,6 +131,9 @@ private void putSSLProperties(X509Util x509Util) {
         properties.put(x509Util.getSslHostnameVerificationEnabledProperty(), System.getProperty(x509Util.getSslHostnameVerificationEnabledProperty()));
         properties.put(x509Util.getSslCrlEnabledProperty(), System.getProperty(x509Util.getSslCrlEnabledProperty()));
         properties.put(x509Util.getSslOcspEnabledProperty(), System.getProperty(x509Util.getSslOcspEnabledProperty()));
+        properties.put(x509Util.getSslTcnativeOcspEnabledProperty(), System.getProperty(x509Util.getSslTcnativeOcspEnabledProperty()));
+        properties.put(x509Util.getSslRevocationEnabledProperty(), System.getProperty(x509Util.getSslRevocationEnabledProperty()));
+        properties.put(x509Util.getSslDisableLegacyRevocationLogicProperty(), System.getProperty(x509Util.getSslDisableLegacyRevocationLogicProperty()));
         properties.put(x509Util.getSslClientAuthProperty(), System.getProperty(x509Util.getSslClientAuthProperty()));
         properties.put(x509Util.getSslHandshakeDetectionTimeoutMillisProperty(), System.getProperty(x509Util.getSslHandshakeDetectionTimeoutMillisProperty()));
         properties.put(x509Util.getFipsModeProperty(), System.getProperty(x509Util.getFipsModeProperty()));
@@ -284,4 +287,15 @@ public int getInt(String key, int defaultValue) {
         return defaultValue;
     }
 
+    /**
+     * Returns {@code TriState.True} if and only if the property named by the argument
+     * exists and is equal to the string {@code "true"}.
+     * Returns {@code TriState.Default} if and only if the property named by the argument
+     * does not exist or is equal to the string {@code "default"}.
+     * Returns {@code TriState.False} otherwise.
+     */
+    public TriState getTristate(String key) {
+        String propertyValue = getProperty(key);
+        return TriState.parse(propertyValue);
+    }
 }