stoty commented on code in PR #2270:
URL: https://github.com/apache/zookeeper/pull/2270#discussion_r2212148314
##########
zookeeper-server/src/main/java/org/apache/zookeeper/common/ClientX509Util.java:
##########
@@ -79,7 +79,11 @@ public SslContext createNettySslContextForClient(ZKConfig
config)
sslContextBuilder.trustManager(tm);
}
-
sslContextBuilder.enableOcsp(config.getBoolean(getSslOcspEnabledProperty()));
+ SslProvider sslProvider = getSslProvider(config);
+ sslContextBuilder.sslProvider(sslProvider);
+ if (sslProvider == SslProvider.OPENSSL || sslProvider ==
SslProvider.OPENSSL_REFCNT) {
Review Comment:
OpenSsl.isAvailable() is completely irrelevant. It only tells if the OpenSSL
provider is present on the classpath (perhaps with some sanity checks).
We COULD make a different check for it, but Netty will handle this and error
out anyway, so I don't see any added value.
**The actual error that we want to fix is the error that we get when netty
is configured to use the JRE PROVIDER and
we're calling enableOCSP().**
OpenSsl.isOcspSupported() is actually broken:
` sslCtx = SSLContext.make(SSL.SSL_PROTOCOL_TLSV1_2, SSL.SSL_MODE_SERVER);
SSLContext.enableOcsp(sslCtx, false);
supportsOcsp = true;
`
As tcnative will simply ignore enableOcsp() if the native library does not
support it, that call will return true for BoringSSL, so it only really tells
us wheter calling enableOcsp() would cause the SSLContextContext creation to
fail.
Additionally, all enableOcsp() does is set a member in the builder, the
actual check only happens at build time.
While this won't do anything useful for now, we can add a check for this in
case tcnative behaviour changes in the future and some relevant check is added
in enableOcsp()
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
