ZOOKEEPER-4940: Enabling zookeeper.ssl.ocsp with JRE TLS provider errors out by stoty · Pull Request #2270 · apache/zookeeper

stoty · 2025-06-18T04:36:05Z

No description provided.

anmolnar

Please replace the condition. Lgtm.

anmolnar · 2025-06-23T17:12:52Z

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

-        sslContextBuilder.enableOcsp(config.getBoolean(getSslOcspEnabledProperty()));
+        SslProvider sslProvider = getSslProvider(config);
+        sslContextBuilder.sslProvider(sslProvider);
+        if (sslProvider == SslProvider.OPENSSL || sslProvider == SslProvider.OPENSSL_REFCNT) {


Please use OpenSsl.isOcspSupported() instead.

OpenSsl.isOcspSupported() only tells us if the loaded native OpenSSL library supports OCSP, @anmolnar .

That does not help with original problem, ie. when we try to set it when the provider is not set to OpenSSL.

We COULD use OpenSsl.isOcspSupported() to log a warning if we try to set it while it is not supported.
TCnative will just ignore the setting in that case.

Also note that I have slightly changed this logic in the latest #2277 PR.

I've also added code to log a warning in that case in #2277

What about using OpenSsl.isAvailable() and OpenSsl.isOcspSupported() together?

@stoty did you see this?

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 (whether enableOcsp is called or not), 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().

I have some doubts about whether OpenSSL.enableOcsp() does what we would expect it to do, but as this is the API we have, we should use it.

Even if it returns a wrong result (i.e. true for BoringSSL), that only means that the setting will be silently ignored. We can't fix that from our side.

If it errors out only for JRE provider, why don't we check the other way around like provider != JDK? But anyways, let's keep this check if there's no better way.

Checking for JDK would also work.
I prefer checking for the implementations that are known to implement the feature, as this would keep working in the (highly theoretical) case that new provider is added to netty.

Ok, let's go with that.

anmolnar · 2025-06-23T17:13:01Z

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

-        sslContextBuilder.enableOcsp(config.getBoolean(getSslOcspEnabledProperty()));
+        SslProvider sslProvider = getSslProvider(config);
+        sslContextBuilder.sslProvider(sslProvider);
+        if (sslProvider == SslProvider.OPENSSL || sslProvider == SslProvider.OPENSSL_REFCNT) {


stoty · 2025-07-07T11:06:40Z

Included in #2276

…ors out

add new property for tcnative OCSP setting

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

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

anmolnar

I think using TriState property for this is a little bit overkill. We could just set it to "true" if zk property is set to true, otherwise don't do anything. Anyway, lgtm.

anmolnar

Sorry. Please rename the property.

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

stoty · 2025-07-17T05:59:20Z

Thanks for the review @anmolnar .

It turns out that I have misread the tcnative/boringssl code, and current BoringSSL DOES support ocsp stapling.
I have removed the comments and docs stating that BoringSSL does not support stapling.

In any case, I have added checks for OpenSSL.isOcspSupported() and refactored the duplicate logic into a new method.
I have also added more logging and tried to make the logic simpler and easier to follow.

anmolnar · 2025-07-17T18:41:38Z

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

+            // Disabling OCSP for the builder is always safe.
+            // This is the same as the builder default, effectively a noop.


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:

if (tcnative && ocspEnabled && tcnativeOcspStapling && OpenSsl.isOcspSupported()) { builder.enableOcsp(ocspEnabled); }

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?

Yes, it would fix the original issue.

However we want to phase out the exisitng OCSP setting (at least for the client), as it relies on setting a JVM global system property. If we phase that out, there will still be a need to somehow set this property, and that's what the new property is for.

If you prefer, we can remove the new property from this patch and it back in a follow-up patch that deals with the JVM globals.

Thanks, I prefer to go with smaller steps.

I don't think you can get rid of the original property entirely, because there must be a way to set pbParams.setRevocationEnabled(true), but we can discuss that in a different patch.

anmolnar

lgmt. Thanks @stoty !

anmolnar · 2025-07-18T14:16:39Z

Merged to master. branch-3.9 and branch-3.8 have conflicts, please create separate pull requests.

stoty · 2025-07-28T16:38:52Z

Is 3.8 active ?
My last patch was rejected from 3.8

…ors out add docs add new property for tcnative OCSP setting rename property 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 rearrange code to make patch smaller add comment for clarification remove new property Reviewers: anmolnar Author: stoty Closes apache#2270 from stoty/ZOOKEEPER-4940 (cherry picked from commit 9d1d25c)

stoty · 2025-07-28T16:49:45Z

3.8 doesn't have tcnative support, so this is not relevant there.

I've opened #2282 for branch-3.9 @anmolnar .

ZOOKEEPER-4940: Enabling zookeeper.ssl.ocsp with JRE TLS provider errors out add docs add new property for tcnative OCSP setting rename property 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 rearrange code to make patch smaller add comment for clarification remove new property Reviewers: anmolnar Author: stoty Closes #2270 from stoty/ZOOKEEPER-4940 (cherry picked from commit 9d1d25c) Author: stoty Closes #2282 from stoty/ZOOKEEPER-4940-3.9

stoty force-pushed the ZOOKEEPER-4940 branch from a17a00e to 3024190 Compare June 18, 2025 04:50

anmolnar requested changes Jun 23, 2025

View reviewed changes

anmolnar mentioned this pull request Jun 23, 2025

ZOOKEEPER-4942: Add option to preserve JVM TLS certification revocation properties #2271

Closed

stoty closed this Jul 7, 2025

stoty reopened this Jul 7, 2025

stoty added 2 commits July 7, 2025 19:56

ZOOKEEPER-4940: Enabling zookeeper.ssl.ocsp with JRE TLS provider err…

671ecb7

…ors out

add docs

098e7c5

add new property for tcnative OCSP setting

stoty force-pushed the ZOOKEEPER-4940 branch from 3024190 to 098e7c5 Compare July 7, 2025 18:12

stoty mentioned this pull request Jul 7, 2025

ZOOKEEPER-4949: Clean up TLS CRL/OCSP configuration #2277

Open

anmolnar reviewed Jul 8, 2025

View reviewed changes

zookeeper-docs/src/main/resources/markdown/zookeeperAdmin.md Outdated Show resolved Hide resolved

anmolnar reviewed Jul 8, 2025

View reviewed changes

zookeeper-server/src/main/java/org/apache/zookeeper/common/ClientX509Util.java Outdated Show resolved Hide resolved

anmolnar approved these changes Jul 8, 2025

View reviewed changes

anmolnar requested changes Jul 8, 2025

View reviewed changes

stoty added 2 commits July 8, 2025 20:11

rename property

964f353

factor out the stapling handling code to a new method

3af6fda

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

stoty requested a review from anmolnar July 17, 2025 05:59

stoty added 2 commits July 17, 2025 08:02

rearrange code to make patch smaller

ab1065a

add comment for clarification

e2504e1

anmolnar requested changes Jul 17, 2025

View reviewed changes

remove new property

f417041

stoty requested a review from anmolnar July 18, 2025 04:46

anmolnar approved these changes Jul 18, 2025

View reviewed changes

anmolnar merged commit 9d1d25c into apache:master Jul 18, 2025
16 checks passed

stoty mentioned this pull request Jul 28, 2025

ZOOKEEPER-4940: Enabling zookeeper.ssl.ocsp with JRE TLS provider err… #2282

Merged

		// Disabling OCSP for the builder is always safe.
		// This is the same as the builder default, effectively a noop.

Conversation

stoty commented Jun 18, 2025

Uh oh!

anmolnar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stoty Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stoty Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anmolnar Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stoty commented Jul 7, 2025

Uh oh!

Uh oh!

Uh oh!

anmolnar left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anmolnar left a comment

Choose a reason for hiding this comment

Uh oh!

stoty commented Jul 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anmolnar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

anmolnar commented Jul 18, 2025

Uh oh!

stoty commented Jul 28, 2025

Uh oh!

stoty commented Jul 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

stoty Jul 7, 2025 •

edited

Loading

stoty Jul 17, 2025 •

edited

Loading

anmolnar Jul 17, 2025 •

edited

Loading

anmolnar left a comment •

edited

Loading