-
Notifications
You must be signed in to change notification settings - Fork 144
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
Decrypt client TLS certificate key for Elastic Defend #5542
Decrypt client TLS certificate key for Elastic Defend #5542
Conversation
This pull request does not have a backport label. Could you fix it @AndersonQ? 🙏
|
|
716f0a9
to
e00b883
Compare
e00b883
to
d081a2a
Compare
ae1e154
to
673335f
Compare
It adds EndpointTLSComponentModifier which will check if the client certificate key is encrypted, if so, it'll decrypt the key and pass it decrypted to endpoint (Elastic Defend)
673335f
to
2b6e38f
Compare
Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane) |
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 am worried about the performance of this component modifier. Most modifiers should just do some small movements or inject a little bit of extra configration.
This modifier is read a file and decrypting it every time the component model changes, which can be a lot. When using say the kubernetes provider with composable inputs it will recompute the model on every new pod state.
I believe this needs to be changed to only do that work once and only inject the cached content.
good point, even thought I'm not sure if defend would be used on such environments. Anyway I added a local cache for the component modifier and it's also thread-safe as the modifiers might be invoked from the coordinator main goroutine and for diagnostics as well. |
This pull request is now in conflicts. Could you fix it? 🙏
|
/test |
…s-decrypted-cert-key-to-defend
…defend' into 5490-pass-decrypted-cert-key-to-defend
…-cert-key-to-defend
This reverts commit b04b42c.
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.
Thanks for fixing the caching key!
Looks good now.
Quality Gate failedFailed conditions |
* decrypt client mTLS certificate key for Elastic Defend It adds EndpointTLSComponentModifier which will check if the client certificate key is encrypted, if so, it'll decrypt the key and pass it decrypted to endpoint (Elastic Defend) * add cache and tests * fix cache miss test * fix linter * update elastic-agent-libs * better comments * fix comment * mage notice * update elastic-agent-libs * debug test * fix test * Revert "debug test" This reverts commit b04b42c. * make cache key from all paths (cherry picked from commit 1c041a2)
Is there anywhere in our logs or diagnistcs .yaml files where the decrypted key could be logged accidentally (expected-config.yaml maybe)? Did we look through the diagnostics to check for this? The file path likely won't be marked as a secret explicitly because it is obscured by default. |
* decrypt client mTLS certificate key for Elastic Defend It adds EndpointTLSComponentModifier which will check if the client certificate key is encrypted, if so, it'll decrypt the key and pass it decrypted to endpoint (Elastic Defend) * add cache and tests * fix cache miss test * fix linter * update elastic-agent-libs * better comments * fix comment * mage notice * update elastic-agent-libs * debug test * fix test * Revert "debug test" This reverts commit b04b42c. * make cache key from all paths (cherry picked from commit 1c041a2) Co-authored-by: Anderson Queiroz <[email protected]>
Not really, the agent already accepts either a certificate or a filepath in the configuration and everything is correctly handled. The change here does not introduce anything new. So unless something was already leaking, I don't see hot the changes here would create a new leak. However I must say I haven't done explicit test for that |
I don't think we need a test, but a manual spot check searching through diagnostics should be done. Leaked TLS private keys is a bad time for everybody. |
we could add it to as a requirement on one of the manual tests for new releases. It'd cover this and any other change we might make |
What does this PR do?
It adds EndpointTLSComponentModifier which will check if the client certificate key is encrypted, if so, it'll decrypt the key and pass it decrypted to endpoint (Elastic Defend)
Why is it important?
Elastic Defend does not support passphrase-protected certificate key, but the agent does. It'll allow the agent to receive passphrase protected client certificate key and still work when Elastic Defend is installed.
Checklist
[ ] I have made corresponding changes to the documentation[ ] I have made corresponding change to the default configuration files./changelog/fragments
using the changelog tool[ ] I have added an integration test or an E2E testAuthor's checklist
Acceptance Criteria:
fleet.ssl.key
configuration setting within Elastic Defend's configuration.TestEndpointSignedComponentModifier
to include a check for the mTLS client certificate key decryption.Disruptive User Impact
How to test this PR locally
adjust the IPs/hostnames as needed
you might need to build a 8.16 agent out of main:
create 2 TLS certificates
use
elastic-agent-libs/testing/certutil/cmd
to create the certificates. Make sure to use[email protected]+
which supports generating RSA certificates as Elastic Defend only accepts RSA certificates.you should have:
start a elastic stack (considering elastic-cloud)
add a fleet server with mTLS
create a policy with Elastic Defend
add an agent to that policy
Related issues
Questions to ask yourself