ZOOKEEPER-4942: Add option to preserve JVM TLS certification revocation properties #2271
Conversation
There was a problem hiding this comment.
License header is missing.
| 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); | ||
| } |
There was a problem hiding this 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?
There was a problem hiding this 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).
There was a problem hiding this comment.
I found the same docs, @anmolnar .
The code agrees that this defaults to the value of "com.sun.net.ssl.checkRevocation".
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).
|
#2277 is the current patch |
2 participants
This patch implements Option B discussed in the ticket.
If the solution is accepted, then we will also need to update the docs.