You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by "stoty (via GitHub)" <gi...@apache.org> on 2025/06/18 08:22:54 UTC
[PR] ZOOKEEPER-4942: Add option to preserve JVM TLS certification revocation properties [zookeeper]
stoty opened a new pull request, #2271:
URL: https://github.com/apache/zookeeper/pull/2271
This patch implements Option B discussed in the ticket.
If the solution is accepted, then we will also need to update the docs.
--
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-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ZOOKEEPER-4942: Add option to preserve JVM TLS certification revocation properties [zookeeper]
Posted by "stoty (via GitHub)" <gi...@apache.org>.
stoty commented on code in PR #2271:
URL: https://github.com/apache/zookeeper/pull/2271#discussion_r2189966523
##########
zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java:
##########
@@ -548,17 +548,19 @@ public static X509TrustManager createTrustManager(
try {
KeyStore ts = loadTrustStore(trustStoreLocation, trustStorePassword, trustStoreTypeProp);
PKIXBuilderParameters pbParams = new PKIXBuilderParameters(ts, new X509CertSelector());
- if (crlEnabled || ocspEnabled) {
- pbParams.setRevocationEnabled(true);
- System.setProperty("com.sun.net.ssl.checkRevocation", "true");
- System.setProperty("com.sun.security.enableCRLDP", "true");
- if (ocspEnabled) {
- Security.setProperty("ocsp.enable", "true");
- }
- } else {
- pbParams.setRevocationEnabled(false);
+ // Leave CRL/OCSP JVM global properties alone both are set to "system" (represented as null)
+ if (!crlEnabled.isSystem() || !ocspEnabled.isSystem()) {
+ if (crlEnabled.isTrue() || ocspEnabled.isTrue()) {
+ pbParams.setRevocationEnabled(true);
+ System.setProperty("com.sun.net.ssl.checkRevocation", "true");
+ System.setProperty("com.sun.security.enableCRLDP", "true");
+ if (ocspEnabled.isTrue()) {
+ Security.setProperty("ocsp.enable", "true");
+ }
+ } else {
+ pbParams.setRevocationEnabled(false);
+ }
Review Comment:
I found the same docs, @anmolnar .
The code agrees that this defaults to the value of "com.sun.net.ssl.checkRevocation".
https://github.com/openjdk/jdk/blob/7c13a2cd9aa5ec9da00084de2388abc189e2f4ef/src/java.base/share/classes/sun/security/validator/PKIXValidator.java#L174
I think that's fine, the goal here is to use the JDK default settings.
This one CAN be overriden separately with the new property if we use a custom truststore.
(Also note that I have slightly changed the logic in the current #2277 PR).
--
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-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ZOOKEEPER-4942: Add option to preserve JVM TLS certification revocation properties [zookeeper]
Posted by "anmolnar (via GitHub)" <gi...@apache.org>.
anmolnar commented on code in PR #2271:
URL: https://github.com/apache/zookeeper/pull/2271#discussion_r2163676682
##########
zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java:
##########
@@ -548,17 +548,19 @@ public static X509TrustManager createTrustManager(
try {
KeyStore ts = loadTrustStore(trustStoreLocation, trustStorePassword, trustStoreTypeProp);
PKIXBuilderParameters pbParams = new PKIXBuilderParameters(ts, new X509CertSelector());
- if (crlEnabled || ocspEnabled) {
- pbParams.setRevocationEnabled(true);
- System.setProperty("com.sun.net.ssl.checkRevocation", "true");
- System.setProperty("com.sun.security.enableCRLDP", "true");
- if (ocspEnabled) {
- Security.setProperty("ocsp.enable", "true");
- }
- } else {
- pbParams.setRevocationEnabled(false);
+ // Leave CRL/OCSP JVM global properties alone both are set to "system" (represented as null)
+ if (!crlEnabled.isSystem() || !ocspEnabled.isSystem()) {
+ if (crlEnabled.isTrue() || ocspEnabled.isTrue()) {
+ pbParams.setRevocationEnabled(true);
+ System.setProperty("com.sun.net.ssl.checkRevocation", "true");
+ System.setProperty("com.sun.security.enableCRLDP", "true");
+ if (ocspEnabled.isTrue()) {
+ Security.setProperty("ocsp.enable", "true");
+ }
+ } else {
+ pbParams.setRevocationEnabled(false);
+ }
Review Comment:
Doc:
> If this flag is true, the default revocation checking mechanism of the underlying PKIX service provider will be used
> When a PKIXParameters object is created, this flag is set to true. This setting reflects the most common strategy for checking revocation, since each service provider must support revocation checking to be PKIX compliant. Sophisticated applications should set this flag to false when it is not practical to use a PKIX service provider's default revocation checking mechanism or when an alternative revocation checking mechanism is to be substituted (by also calling the addCertPathChecker or setCertPathCheckers methods).
--
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-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ZOOKEEPER-4942: Add option to preserve JVM TLS certification revocation properties [zookeeper]
Posted by "anmolnar (via GitHub)" <gi...@apache.org>.
anmolnar commented on code in PR #2271:
URL: https://github.com/apache/zookeeper/pull/2271#discussion_r2155513248
##########
zookeeper-server/src/main/java/org/apache/zookeeper/common/TriState.java:
##########
Review Comment:
License header is missing.
--
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-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ZOOKEEPER-4942: Add option to preserve JVM TLS certification revocation properties [zookeeper]
Posted by "anmolnar (via GitHub)" <gi...@apache.org>.
anmolnar commented on code in PR #2271:
URL: https://github.com/apache/zookeeper/pull/2271#discussion_r2163666292
##########
zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java:
##########
@@ -548,17 +548,19 @@ public static X509TrustManager createTrustManager(
try {
KeyStore ts = loadTrustStore(trustStoreLocation, trustStorePassword, trustStoreTypeProp);
PKIXBuilderParameters pbParams = new PKIXBuilderParameters(ts, new X509CertSelector());
- if (crlEnabled || ocspEnabled) {
- pbParams.setRevocationEnabled(true);
- System.setProperty("com.sun.net.ssl.checkRevocation", "true");
- System.setProperty("com.sun.security.enableCRLDP", "true");
- if (ocspEnabled) {
- Security.setProperty("ocsp.enable", "true");
- }
- } else {
- pbParams.setRevocationEnabled(false);
+ // Leave CRL/OCSP JVM global properties alone both are set to "system" (represented as null)
+ if (!crlEnabled.isSystem() || !ocspEnabled.isSystem()) {
+ if (crlEnabled.isTrue() || ocspEnabled.isTrue()) {
+ pbParams.setRevocationEnabled(true);
+ System.setProperty("com.sun.net.ssl.checkRevocation", "true");
+ System.setProperty("com.sun.security.enableCRLDP", "true");
+ if (ocspEnabled.isTrue()) {
+ Security.setProperty("ocsp.enable", "true");
+ }
+ } else {
+ pbParams.setRevocationEnabled(false);
+ }
Review Comment:
One question here:
In the case when both parameters are set to 'system', we won't do anything: not altering system properties and not setting revocation in `pbParams`. Will that work correctly, I mean will `pbParams` revocation flag follow the system settings?
--
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-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ZOOKEEPER-4942: Add option to preserve JVM TLS certification revocation properties [zookeeper]
Posted by "stoty (via GitHub)" <gi...@apache.org>.
stoty closed pull request #2271: ZOOKEEPER-4942: Add option to preserve JVM TLS certification revocation properties
URL: https://github.com/apache/zookeeper/pull/2271
--
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-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ZOOKEEPER-4942: Add option to preserve JVM TLS certification revocation properties [zookeeper]
Posted by "stoty (via GitHub)" <gi...@apache.org>.
stoty commented on PR #2271:
URL: https://github.com/apache/zookeeper/pull/2271#issuecomment-3044535724
#2277 is the current patch
--
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-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ZOOKEEPER-4942: Add option to preserve JVM TLS certification revocation properties [zookeeper]
Posted by "anmolnar (via GitHub)" <gi...@apache.org>.
anmolnar commented on code in PR #2271:
URL: https://github.com/apache/zookeeper/pull/2271#discussion_r2163676682
##########
zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java:
##########
@@ -548,17 +548,19 @@ public static X509TrustManager createTrustManager(
try {
KeyStore ts = loadTrustStore(trustStoreLocation, trustStorePassword, trustStoreTypeProp);
PKIXBuilderParameters pbParams = new PKIXBuilderParameters(ts, new X509CertSelector());
- if (crlEnabled || ocspEnabled) {
- pbParams.setRevocationEnabled(true);
- System.setProperty("com.sun.net.ssl.checkRevocation", "true");
- System.setProperty("com.sun.security.enableCRLDP", "true");
- if (ocspEnabled) {
- Security.setProperty("ocsp.enable", "true");
- }
- } else {
- pbParams.setRevocationEnabled(false);
+ // Leave CRL/OCSP JVM global properties alone both are set to "system" (represented as null)
+ if (!crlEnabled.isSystem() || !ocspEnabled.isSystem()) {
+ if (crlEnabled.isTrue() || ocspEnabled.isTrue()) {
+ pbParams.setRevocationEnabled(true);
+ System.setProperty("com.sun.net.ssl.checkRevocation", "true");
+ System.setProperty("com.sun.security.enableCRLDP", "true");
+ if (ocspEnabled.isTrue()) {
+ Security.setProperty("ocsp.enable", "true");
+ }
+ } else {
+ pbParams.setRevocationEnabled(false);
+ }
Review Comment:
Doc:
> When a PKIXParameters object is created, this flag is set to true. This setting reflects the most common strategy for checking revocation, since each service provider must support revocation checking to be PKIX compliant. Sophisticated applications should set this flag to false when it is not practical to use a PKIX service provider's default revocation checking mechanism or when an alternative revocation checking mechanism is to be substituted (by also calling the addCertPathChecker or setCertPathCheckers methods).
--
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-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org