stoty commented on code in PR #2289:
URL: https://github.com/apache/zookeeper/pull/2289#discussion_r2262370296
##########
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:
Thanks for correcting me.
Your findings above also support that the current code (specifically
explicitly calling pbParams.setRevocationEnabled(false) when the ZK crl
properties are not set)
are not fit for the client library use case.
There are many possible ways to fix this by adding/modifying the behaviour
of config properties.
IMO my proposal of directly exposing the setRevocationEnabled() property and
disabling the old behaviour
(i.e setting the system/security properties and calling
setRevocationEnabled() based on the values of the old crl and ocsp ZK
properties) by default (depending on branch) is the most straightforward.
The only problem I have with my proposed changes is that old defauit "false"
value for the old crl and ocsp ZK properties becomes effectively a no-op. We
could add a third "java_default" like state and default to that, but that would
actually behave exactly like the "false", so it may not be worth it.
Here are some possible option sets, and the settings for making ZK leave the
revaction settings and JVM System/Security proprties alone:
current patch:
```
ssl.revocationEnabled: java_default (true/false/java_default/legacy)
ssl.crl: false (true/false)
ssl.ocsp: false (true/false)
```
Alternative 1 :
add a third state to ocsp and crl properties, but don't add explicit
override for setRevocationEnabled()
This won't let the user override the system defaults on the custom
truststore, so this loses functionality
```
ssl.crl: java_default (true/false/java_default)
ssl.ocsp: java_default (true/false/java_default)
```
Alternative 2 :
add a third state to ocsp and crl properties, and also an explicit override
for setRevocationEnabled()
```
ssl.revocationEnabled: java_default (true/false/java_default/legacy)
ssl.crl: java_default (true/false/java_default)
ssl.ocsp: java_default (true/false/java/default)
```
Alternative 3:
Separate options for disabling setting global property and revocationEnabled
logic
and only using simple booleans:
```
ssl.set.revocation.enabled f (t/f)
ss.set.revocation.disabled f (t/f)
ssl.crl: false (true/false)
ssl.ocsp: false (true/false)
ssl.crl.populate-java-property: false (t/f)
ssl.ocsp.populate-java-property: false (t/f)
ssl.revocation.setRevocatioEnable-based-on-crl-and-ocsp: false (t/f)
```
Or some combination of the above.
Which one do you prefer ?
--
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]
