Add CRL checks#1474
Conversation
|
|
||
| FileSystemStoreInfo trust = supplyStoreInfo(resolvePath, options.trust); | ||
| supplyTrust = (aliases, cacerts) -> newTrustFactory(trust, aliases, cacerts); | ||
| supplyTrust = (aliases, cacerts, crlChecks) -> newTrustFactory(trust, aliases, cacerts, crlChecks); |
There was a problem hiding this comment.
This forced API change suggests that the decision to enforce CRL checks might belong in the vault, not the TLS binding, what do you think?
jfallows
left a comment
jfallows
left a comment
There was a problem hiding this comment.
We need negative test coverage to verify that revoked client certificate cannot be used to successfully complete a TLS handshake.
| ENGINE_CACERTS_STORE_PASS = config.property("cacerts.store.pass"); | ||
| ENGINE_ERROR_REPORTER = config.property(ErrorReporter.class, "error.reporter", | ||
| EngineConfiguration::decodeErrorReporter, EngineConfiguration::defaultErrorReporter); | ||
| ENGINE_REVOCATION = config.property("revocation"); |
There was a problem hiding this comment.
We need a name with more context for clarity.
| ENGINE_REVOCATION = config.property("revocation"); | |
| ENGINE_CERTIFICATE_REVOCATION_STRATEGY = config.property("certificate.revocation.strategy"); |
Suggest making this more strongly typed with an enum, say RevocationStrategy in io.aklivity.zilla.runtime.engine.security package.
public enum RevocationStrategy
{
CRL,
NONE
}
...and default to NONE.
We can also consider using this type (from lowercase string) in various vault options instead of string, encouraging consistency across different vault implementations.
jfallows
left a comment
jfallows
left a comment
There was a problem hiding this comment.
Looks good, just need the negative IT to verify that a revoked certificate cannot be used to successfully complete TLS handshake.
jfallows
left a comment
jfallows
left a comment
There was a problem hiding this comment.
Let's merge this now, given that you have already verified end-to-end.
2 participants
No description provided.