-
Notifications
You must be signed in to change notification settings - Fork 134
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
Apply mTLS config from policy #4770
Conversation
This pull request does not have a backport label. Could you fix it @pchila? 🙏
NOTE: |
This pull request is now in conflicts. Could you fix it? 🙏
|
d51dcc1
to
a457ba3
Compare
Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane) |
5b7af1a
to
bd9fe64
Compare
changelog/fragments/1716490302-Load-fleet.ssl.certificate_authorities-from-agent-policy.yaml
Outdated
Show resolved
Hide resolved
changelog/fragments/1716490302-Load-fleet.ssl.certificate_authorities-from-agent-policy.yaml
Outdated
Show resolved
Hide resolved
f43e4f3
to
93760d4
Compare
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 looks really good. Has good testing and code coverage.
Just a few comments, that should be cleaned up.
internal/pkg/agent/application/actions/handlers/handler_action_policy_change.go
Outdated
Show resolved
Hide resolved
internal/pkg/agent/application/actions/handlers/handler_action_policy_change.go
Outdated
Show resolved
Hide resolved
internal/pkg/agent/application/actions/handlers/handler_action_policy_change.go
Outdated
Show resolved
Hide resolved
internal/pkg/agent/application/actions/handlers/handler_action_policy_change.go
Outdated
Show resolved
Hide resolved
// generate a certificate for elastic-agent from the same CA as the proxy | ||
_, agentPair, err := certutil.GenerateChildCert("localhost", []net.IP{net.IPv6loopback, net.IPv6zero, net.ParseIP("127.0.0.1")}, caKey, caCert) |
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.
[Suggestion]
I like to have different CAs for the client and server/proxy just for the sake of testing it as thorough as possible.
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.
Done in 78edd2e
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.
[Blocker]
It's missing the tests to ensure the precedence is correct:
- enroll args with mTLS > policy with no TLS
- policy mTLS > enroll args with mTLS
Also, tests to ensure a broken config isn't applied:
- wrong CA
- wrong certificate / key
right now, there is only:
- policy mTLS > no mTLS enroll args
which is a sub case of "policy mTLS > enroll args with mTLS"
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.
Another thing, ideally there should also be a test to validate the certificate, its key and the CA can be a path
53b3f82
to
b97e6af
Compare
--insecure
flag should not be required during enroll/install because we have an http
Fleet URL
#4896
changelog/fragments/1716490302-Load-fleet.ssl.certificate_authorities-from-agent-policy.yaml
Outdated
Show resolved
Hide resolved
Quality Gate passedIssues Measures |
What does this PR do?
This PR implements reading and applying TLS configuration for Fleet client using CA, certificate and key included in Fleet policy.
This PR:
Note to reviewers: refactor of ProxyURL integration tests has been moved to PR #4813 , so for initial review you can have a look at this set of commits or wait till PR #4813 is merged and this PR rebased onto the new mainPR #4813 has been merged and this change has been rebase on top of the new
main
.Why is it important?
Configuring TLS via the policy allows agent to connect to Fleet (possibly via a proxy) using custom CAs or enabling mTLS (certificate verification of both the client and the server).
Checklist
[ ] I have made corresponding changes to the documentation[ ] I have made corresponding change to the default configuration files./changelog/fragments
using the changelog toolDisruptive User Impact
How to test this PR locally
In order to test this PR we need:
Related issues
fleet.ssl.certificate_authorities
from agent policy #2247fleet.ssl.certificate
andfleet.ssl.key
from agent policy #2248Questions to ask yourself