factor out the stapling handling code to a new method

use and honor OpenSSL.isOcspSupported()
Add more log messages
Remove comments about BoringSSL not supporting OCSP stapling

Author: stoty

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

@@ -1780,9 +1780,6 @@ and [SASL authentication for ZooKeeper](https://cwiki.apache.org/confluence/disp
     (Java system properties: **zookeeper.ssl.tcnative.ocsp.stapling** and **zookeeper.ssl.quorum.tcnative.ocsp.stapling**)
     **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.
 

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

@@ -80,21 +80,8 @@ public SslContext createNettySslContextForClient(ZKConfig config)
             sslContextBuilder.trustManager(tm);
         }
 
-        SslProvider sslProvider = getSslProvider(config);
-        sslContextBuilder.sslProvider(sslProvider);
-        if (sslProvider == SslProvider.OPENSSL || sslProvider == SslProvider.OPENSSL_REFCNT) {
-            boolean ocspEnabled = config.getBoolean(getSslOcspEnabledProperty());
-            logTcnativeOcsp(ocspEnabled);
-            // Set it even in unsupported, tcnative will just ignore it
-            sslContextBuilder.enableOcsp(ocspEnabled);
-        }
-        // Explicit option takes precedence if set
-        if (config.getTristate(getSslTcnativeOcspStaplingEnabledProperty()).isTrue()) {
-            logTcnativeOcsp(true);
-            sslContextBuilder.enableOcsp(true);
-        } else if (config.getTristate(getSslTcnativeOcspStaplingEnabledProperty()).isFalse()) {
-            sslContextBuilder.enableOcsp(false);
-        }
+        sslContextBuilder.sslProvider(getSslProvider(config));
+        handleTcnativeOcspStapling(sslContextBuilder, config);
         String[] enabledProtocols = getEnabledProtocols(config);
         if (enabledProtocols != null) {
             sslContextBuilder.protocols(enabledProtocols);
@@ -113,12 +100,29 @@ public SslContext createNettySslContextForClient(ZKConfig config)
         }
     }
 
-    private void logTcnativeOcsp(boolean enable) {
-        if (enable && !OpenSsl.isOcspSupported()) {
-            // SslContextBuilder.enableOcsp() doesn't do anything, unless the default BoringSSL
-            // tcnative dependency is replaced with an OpenSsl one.
-            LOG.warn("Trying to enable OCSP for tcnative OpenSSL provider, but it is not supported. The setting will be ignored", OpenSsl.versionString());
+    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());
+        TriState tcnativeOcspStapling = config.getTristate(getSslTcnativeOcspStaplingEnabledProperty());
+
+        if (tcnative && ocspEnabled && tcnativeOcspStapling.isDefault() && OpenSsl.isOcspSupported()) {
+            // Maintain old behaviour (mostly, we also check for OpenSsl.isOcspSupported())
+            builder.enableOcsp(ocspEnabled);
+        } else if (tcnativeOcspStapling.isTrue()) {
+            if (!tcnative) {
+                // Don't override the explicit setting, let it error out
+                LOG.error("Trying to enable OpenSSL OCSP stapling for non-OpenSSL TLS provider. "
+                        + "This is going to fail. Please fix the TLS configuration");
+            } else if (!OpenSsl.isOcspSupported()) {
+                LOG.warn("Trying to enable OpenSSL OCSP stapling for OpenSSL provider {} which does not support it. "
+                        + "This is either going to be ignored or fail.", OpenSsl.versionString());
+            }
+            builder.enableOcsp(true);
+        } else if (tcnativeOcspStapling.isFalse()) {
+            builder.enableOcsp(false);
         }
+        return builder;
     }
 
     public SslContext createNettySslContextForServer(ZKConfig config)
@@ -144,17 +148,8 @@ public SslContext createNettySslContextForServer(ZKConfig config, KeyManager key
         if (trustManager != null) {
             sslContextBuilder.trustManager(trustManager);
         }
-
-        SslProvider sslProvider = getSslProvider(config);
-        sslContextBuilder.sslProvider(sslProvider);
-        if (sslProvider == SslProvider.OPENSSL || sslProvider == SslProvider.OPENSSL_REFCNT) {
-            sslContextBuilder.enableOcsp(config.getBoolean(getSslOcspEnabledProperty()));
-        }
-        if (config.getTristate(getSslTcnativeOcspStaplingEnabledProperty()).isTrue()) {
-            sslContextBuilder.enableOcsp(true);
-        } else if (config.getTristate(getSslTcnativeOcspStaplingEnabledProperty()).isFalse()) {
-            sslContextBuilder.enableOcsp(false);
-        }
+        sslContextBuilder.sslProvider(getSslProvider(config));
+        handleTcnativeOcspStapling(sslContextBuilder, config);
         String[] enabledProtocols = getEnabledProtocols(config);
         if (enabledProtocols != null) {
             sslContextBuilder.protocols(enabledProtocols);