-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Add trust configuration for cross cluster api keys #134893
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
6c6f51d to
8930ae6
Compare
4615266 to
c8388c4
Compare
|
Pinging @elastic/es-security (Team:Security) |
|
Hi @jfreden, I've created a changelog YAML for you. |
| logger.trace("Loading trust config with settings [{}]", settings); | ||
| try { | ||
| // Only load a trust manager if trust is configured to avoid using key store as trust store | ||
| if (settingsHaveTrustConfig(settings)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would move this check into SslConfiguration - it seems like something useful to push up.
That is,
var sslConfig = loadSslConfig(environment, settings);
if (sslConfig.hasExplicitTrustConfig()) {
....
}
It like that this PR reduces the amount of custom certificate handling we have. If there are opportunities to push more SSL/certificate related things up that's generally going to be good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved the hasExplicitConfig check into SslTrustConfig. While doing that I realized that we might want to allow the default jdk trust store as a valid config. Do you agree with that? So if you configure nothing, that's what you'll get.
The thing I was trying to protect against with that check was that the customer doesn't accidentally trust the key store setup for signing, since it doesn't feel like I would expect that to happen.
| var trustConfig = sslConfig.trustConfig(); | ||
| final X509ExtendedTrustManager newTrustManager = trustConfig.createTrustManager(); | ||
| if (newTrustManager.getAcceptedIssuers().length == 0) { | ||
| logger.warn("Cross cluster API Key trust configuration [{}] has no accepted certificate issuers", this, trustConfig); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has one {} but two args.
| logger.warn("Cross cluster API Key trust configuration [{}] has no accepted certificate issuers", this, trustConfig); | |
| logger.warn("Cross cluster API Key trust configuration [{}] has no accepted certificate issuers", trustConfig); |
| } | ||
| } | ||
|
|
||
| public Collection<Path> getDependentFiles() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be worth being explicit about which method handles trust and which handles signing.
| public Collection<Path> getDependentFiles() { | |
| public Collection<Path> getDependentTrustFiles() { |
or some other name of your choosing.
| return sslConfig == null ? Collections.emptyList() : sslConfig.getDependentFiles(); | ||
| } | ||
|
|
||
| public Collection<Path> getDependentFiles(String clusterAlias) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| public Collection<Path> getDependentFiles(String clusterAlias) { | |
| public Collection<Path> getDependentSigningFiles(String clusterAlias) { |
| var authTrustManager = trustManager.get(); | ||
| if (authTrustManager == null) { | ||
| logger.warn("No trust manager found"); | ||
| throw new IllegalStateException("No trust manager found"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a plausible scenario if someone updates the cluster settings to remove the trust config, right?
In that case I think these messages need to be a bit more explicit about what's happened.
Something like
Cannot verify signed cross-cluster headers because [cluster.remote.signing] has not trust configuration
|
|
||
| try { | ||
| // Make sure the provided certificate chain is trusted | ||
| authTrustManager.checkClientTrusted(signature.certificates(), signature.certificates()[0].getPublicKey().getAlgorithm()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's worth a trace log here
| authTrustManager.checkClientTrusted(signature.certificates(), signature.certificates()[0].getPublicKey().getAlgorithm()); | |
| var leaf = signature.certificates()[0]; | |
| if (logger.isTraceEnabled()) { | |
| logger.trace("checking signing chain (len={}) [{}] with leaf subject [{}] using algorithm [{}]", | |
| signature.certificates().length, | |
| Arrays.stream(signature.certificates()).map(CrossClusterApiKeySignatureManager::calculateFingerprint).collect(Collectors.joining(",")), | |
| leaf.getSubjectX500Principal().getName(X500Principal.RFC2253), | |
| leaf.getPublicKey().getAlgorithm | |
| ); | |
| } | |
| authTrustManager.checkClientTrusted(signature.certificates(),left.getPublicKey().getAlgorithm()); |
| // Make sure the provided certificate chain is trusted | ||
| authTrustManager.checkClientTrusted(signature.certificates(), signature.certificates()[0].getPublicKey().getAlgorithm()); | ||
| // TODO Make sure the signing certificate belongs to the correct DN (the configured api key cert identity) | ||
| // TODO Make sure the signing certificate is valid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can never remember how much the trust manager does for you here. I think it does more than you'd expect from the name, but maybe not everything we need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left the todo very broad to make sure I verify everything when we do the "certificate belongs to the correct DN" check.
| CertificateFactory certFactory = CertificateFactory.getInstance("X.509"); | ||
| final Certificate cert = certFactory.generateCertificate(bais); | ||
| if (cert instanceof X509Certificate x509) { | ||
| certificate = x509; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could just be a return (rather than needing a local variable).
| + fingerprint() | ||
| + "), " | ||
| + "certificates=" | ||
| + Arrays.toString(Arrays.stream(certificateChain).map(this::certificateToString).toArray(String[]::new)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it better to use Collectors.joining(",") rather than converting to an array here?
|
|
||
| var manager = new CrossClusterApiKeySignatureManager(TestEnvironment.newEnvironment(builder.build())); | ||
| var signature = manager.signerForClusterAlias("my_remote").sign("a_header"); | ||
| assertFalse(manager.verifier().verify(signature, "another_header")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also have tests for:
- Signed with a different key
- Manipulate signature string
?
…-dls
* upstream/main: (55 commits)
Mute org.elasticsearch.upgrades.MatchOnlyTextRollingUpgradeIT testIndexing {upgradedNodes=1} elastic#135525
Address es819 tsdb doc values format performance bug (elastic#135505)
Remove obsolete --add-opens from JDK API extractor tool (elastic#135445)
[CI] Fix MergeWithFailureIT (elastic#135447)
Increase wait time in AdaptiveAllocationsScalerServiceTests (elastic#135510)
ES|QL: Handle multi values in FUSE (elastic#135448)
Mute org.elasticsearch.upgrades.SyntheticSourceRollingUpgradeIT testIndexing {upgradedNodes=1} elastic#135512
Add trust configuration for cross cluster api keys (elastic#134893)
ESQL: Fix flakiness in SessionUtilsTests (elastic#135375)
Mute org.elasticsearch.upgrades.LogsdbIndexingRollingUpgradeIT testIndexing {upgradedNodes=1} elastic#135511
Mute org.elasticsearch.upgrades.MatchOnlyTextRollingUpgradeIT testIndexing {upgradedNodes=2} elastic#135325
Require all functions to provide examples (elastic#135094)
Mute org.elasticsearch.upgrades.SyntheticSourceRollingUpgradeIT testIndexing {upgradedNodes=2} elastic#135344
Mute org.elasticsearch.upgrades.TextRollingUpgradeIT testIndexing {upgradedNodes=1} elastic#135236
Mute org.elasticsearch.upgrades.TextRollingUpgradeIT testIndexing {upgradedNodes=2} elastic#135238
Mute org.elasticsearch.upgrades.LogsdbIndexingRollingUpgradeIT testIndexing {upgradedNodes=2} elastic#135327
Mute org.elasticsearch.upgrades.MatchOnlyTextRollingUpgradeIT testIndexing {upgradedNodes=3} elastic#135324
Mute org.elasticsearch.upgrades.StandardToLogsDbIndexModeRollingUpgradeIT testLogsIndexing {upgradedNodes=3} elastic#135315
ESQL: Handle right hand side of Inline Stats coming optimized with LocalRelation shortcut (elastic#135011)
Mute org.elasticsearch.upgrades.TextRollingUpgradeIT testIndexing {upgradedNodes=3} elastic#135237
...
This PR is a follow up to #134137 and #134893. It adds serialization and verification of the new header: - The signing configurations are now used to generate a signature that's passed as a header for cross cluster api keys. - The signature headers are deserializaed and validated on the server side and auth fails if the validation fails. This PR does not use the certificate identity that was added in #134604 to verify that the identity in the passed leaf certificate belongs to the signed cross cluster API key by matching it against the API key certificate identity pattern. That will be done in a follow up PR to keep the scope of this PR manageable.
This PR adds trust configuration for cross-cluster API keys.
The actual verification of certificate dn to cross cluster api keys will be added in subsequent PRs.
The new configurations are: