Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

stoty
Copy link
Contributor

@stoty stoty commented Jul 7, 2025

  • 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

- 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
@stoty stoty force-pushed the ZOOKEEPER-4949 branch from 52cca79 to d97ecd1 Compare July 7, 2025 11:15
@stoty
Copy link
Contributor Author

stoty commented Jul 7, 2025

These are all the CRL/OCSP config related changes in a single PR, @anmolnar .

@stoty
Copy link
Contributor Author

stoty commented Jul 7, 2025

The failures look like network / resource starvation issues.

@anmolnar
Copy link
Contributor

anmolnar commented Jul 7, 2025

I'm sorry @stoty , but this is the 5th patch that you create on this topic and it's getting impossible for me to follow. I think it would be beneficial to discuss and address the issues separately. I don't really like "clean up" style patches, especially when there're significant changes in the logic. This is not cleaning up.

So, I can identify the following issues:

  • setting OCSP/CRL Java properties explicitly to "true" on the client side while we want to follow system default. I don't want to change this behaviour on the server side at all. This is strictly a client side improvement.
  • setting OCSP stapling in OpenSSL provider only. There should be a way to check availability in Netty for this rather than relying on enum values. How about using OpenSsl.isAvailable() and OpenSsl.isOcspSupported() flags together?
  • using built-in hostname verification even if Fips style is disabled and we don't have trustStore defined

Is the above accurate?

@stoty
Copy link
Contributor Author

stoty commented Jul 7, 2025

I'm sorry @stoty , but this is the 5th patch that you create on this topic and it's getting impossible for me to follow. I think it would be beneficial to discuss and address the issues separately. I don't really like "clean up" style patches, especially when there're significant changes in the logic. This is not cleaning up.

So, I can identify the following issues:

  • setting OCSP/CRL Java properties explicitly to "true" on the client side while we want to follow system default. I don't want to change this behaviour on the server side at all. This is strictly a client side improvement.

Mostly.
We can discuss the implementation details in the separate patch I'm going to open.

As for "this is strictly a client side improvement" : This is only really a problem on the client side.
As everything is backwards compatible, I would prefer to also make these changes on the server side, as the TLS code is structured so that it's harder to make client side only changes than to make the same changes on the server side.

  • setting OCSP stapling in OpenSSL provider only. There should be a way to check availability in Netty for this rather than relying on enum values. How about using OpenSsl.isAvailable() and OpenSsl.isOcspSupported() flags together?

Yes, that's ZOOKEEPER-4940. However, OpenSsl.is* is NOT helpful here.
We don't care whether OpenSSL is loaded or not, or what it supports.
None of that matters if the SSL provider is set to JRE, and tcnative is never used.

Please see the reopened #2270 for ZOOKEEPER-4940 for the standalone patch.

  • using built-in hostname verification even if Fips style is disabled and we don't have trustStore defined

Check

Is the above accurate?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants