anmolnar commented on code in PR #2270:
URL: https://github.com/apache/zookeeper/pull/2270#discussion_r2214039840
##########
zookeeper-server/src/main/java/org/apache/zookeeper/common/ClientX509Util.java:
##########
@@ -144,6 +145,33 @@ 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());
+ 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()) {
+ // Disabling OCSP for the builder is always safe.
+ // This is the same as the builder default, effectively a noop.
Review Comment:
I was thinking about this too. Why do we bother with TriState property if
the default is obviously that the feature is disabled? We're trying to cover a
highly unlikely case if ever in the future the enabled OCSP Stapling feature
will become the default.
Instead - strictly fixing the bug only - we could do:
```java
if (tcnative && ocspEnabled && tcnativeOcspStapling &&
OpenSsl.isOcspSupported()) {
builder.enableOcsp(ocspEnabled);
}
```
##########
zookeeper-server/src/main/java/org/apache/zookeeper/common/ClientX509Util.java:
##########
@@ -144,6 +145,33 @@ 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());
+ 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()) {
+ // Disabling OCSP for the builder is always safe.
+ // This is the same as the builder default, effectively a noop.
Review Comment:
We could do even less if not introducing the new config setting for
stapling. If OCSP is enabled in the config and `tcnative` is in use, then we
enabled OCSP stapling. This is the simplest bugfix I can think of and it covers
all mentioned - currently broken - use cases as well and it's fully backward
compatible.
wdyt?
--
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
