kezhuw commented on code in PR #2289:
URL: https://github.com/apache/zookeeper/pull/2289#discussion_r2262197297
##########
zookeeper-server/src/main/java/org/apache/zookeeper/common/X509Util.java:
##########
@@ -541,21 +548,32 @@ public static X509TrustManager createTrustManager(
boolean ocspEnabled,
final boolean serverHostnameVerificationEnabled,
final boolean clientHostnameVerificationEnabled,
- final boolean fipsMode) throws TrustManagerException {
+ final boolean fipsMode,
+ final SslRevocationEnabled revocationEnabled) throws
TrustManagerException {
if (trustStorePassword == null) {
trustStorePassword = "";
}
try {
KeyStore ts = loadTrustStore(trustStoreLocation,
trustStorePassword, trustStoreTypeProp);
PKIXBuilderParameters pbParams = new PKIXBuilderParameters(ts, new
X509CertSelector());
if (crlEnabled || ocspEnabled) {
- pbParams.setRevocationEnabled(true);
+ if (revocationEnabled == SslRevocationEnabled.LEGACY) {
+ 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 {
+ if (revocationEnabled == SslRevocationEnabled.LEGACY) {
+ pbParams.setRevocationEnabled(false);
+ }
+ }
+
+ if (revocationEnabled == SslRevocationEnabled.TRUE) {
+ pbParams.setRevocationEnabled(true);
+ } else if (revocationEnabled == SslRevocationEnabled.FALSE) {
Review Comment:
I agree to most of above comments but one.
> That's only true sometimes.
> It is enabled by default in SunJCE, but (only by looking at the code, not
tested) it is disabled by default in BouncyCastle. There also other
Cryptography providers where the behaviour may be different.
How could cryptography providers override the default in jdk ?
https://github.com/openjdk/jdk/blob/jdk-11%2B28/src/java.base/share/classes/java/security/cert/PKIXParameters.java#L86-L92
```java
public class PKIXParameters implements CertPathParameters {
private Set<TrustAnchor> unmodTrustAnchors;
private Date date;
private List<PKIXCertPathChecker> certPathCheckers;
private String sigProvider;
private boolean revocationEnabled = true;
```
> The case for having a default NoOp option is that there are already
mechanisms and defaults in the JVM that can be used to set this up.
We are using explicitly `PKIXBuilderParameters`, according to the
documentation, `com.sun.net.ssl.checkRevocation` is useless in our path.
https://docs.oracle.com/javase/8/docs/technotes/guides/security/jsse/JSSERefGuide.html
states that:
> If the init(KeyStore ks) method is used, then default PKIX parameters are
used with the exception that revocation checking is disabled. It can be enabled
by setting the com.sun.net.ssl.checkRevocation system property to true. This
setting requires that the CertPath implementation can locate revocation
information by itself. The PKIX implementation in the provider can do this in
many cases but requires that the system property com.sun.security.enableCRLDP
be set to true.
https://docs.oracle.com/javase/8/docs/technotes/guides/security/jsse/ocsp.html
states that:
> Client-driven OCSP is enabled by enabling revocation checking and enabling
OCSP.
>
> To configure a Java client to use client-driven OCSP, the Java client must
already be set up to connect to a > server using TLS.
>
> Enable revocation checking. You can do this in two different ways.
> Set the system property com.sun.net.ssl.checkRevocation to true.
> Use the setRevocationEnabled method on PKIXParameters.
> Enable client-driven OCSP:
>
> Set the Security Property ocsp.enable to true.
>
> Both steps are necessary. The ocsp.enable setting has no effect unless
revocation checking is enabled.
I dropped `pbParams.setRevocationEnabled(true)` and
`System.setProperty("com.sun.net.ssl.checkRevocation", "true")` in
https://github.com/kezhuw/zookeeper/commit/826b33862832f04f0171a47a03b420f955fa0b9f
and [`QuorumSSLTest`
passes](https://github.com/kezhuw/zookeeper/actions/runs/16824320595/job/47657245027#step:6:541).
This conforms to the doc that `PKIXParameters::revocationEnabled` is default
to `true` and `com.sun.net.ssl.checkRevocation` is not consulted in case of
explicitly pkix parameters.
--
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]
