Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add TLS settings to all connection settings #205
base: main
Are you sure you want to change the base?
Add TLS settings to all connection settings #205
Changes from 1 commit
a174175
7849fe0
15fdbc0
41ad8fb
05f2846
2b15b10
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
What is the use case for this flag? Why would you want to remotely specify TLS connection settings and at the same time tell the agent to ignore them?
Generally speaking the use cases are not entirely clear. How are all these parameters supposed to be used? It seems like we borrowed the settings from the Collector which is probably a good starting point but we need to understand if they are applicable to OpAMP-managed configuration.
If we don't have a clear use case for a setting then we should not include it here.
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.
The best use case I can think of for this case is to ease development/testing (allow http connections)
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.
How do we expect this to work? Is OpAMP servers expected to know where on agent's disk these files are located? How would the server know?
This also potentially is an attack vector where malicious server can direct the agent to read an arbitrary file. On its own it may seem benign but if we end up having another weakness where the file that was read for some reasons gets reported to the server this will allow the server to remotely read arbitrary files from agent's machine. This goes against our zero trust recommendations in the Security section.
Should we instead send the content of CA certificate to the agent to use? The agent can save the content locally and use it in subsequent connections.
This can work particularly well for
TelemetryConnectionSettings
andOtherConnectionSettings
, the idea being that OpAMP server has the knowledge about the CA used by telemetry and other connection destinations.And even for
OpAMPConnectionSettings
this may make sense as a flow, where the agent first connects without the TLS certificate, in sort of a bootstraping mode, then accepts server offered CA certificate for future verifications. This is similar to Trust On First Use flow that we already have but in the opposite direction (trust of server, instead of trust of client).If we think this is a valid usage of TLSConnectionSettings I think we should add a corresponding section in the spec and include a sequence diagram that explains it.
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.
When specifying the file path we assume it is present at that location across all agents. It can be provisioned by some other tool (baked into the image, or by a configuration management tool).
But I can see how having a filepath specification goes against our current recommendations. It's specified because it is a common approach to TLS configuration, however I can remove the it if we want to keep our security exposure low.
The content can instead be specified with the
ca_pem
attribute (I'll rename toca_pem_content
to make that clearer).I like the idea of a TOFU like mechanism for accepting CAs from servers, I'll try to add the description for this in the spec.
At elastic we do something a little similar to that, we allow the specification of a CA by a fingerprint hash and add the matching CA to the cert pool when it is found in a connection (implementation here). This assumes that what ever is installing an agent has some additional information about the server and it prevents an agent from blindly trusting any CA. This may be worth breaking out into another issue/pr, what do you think?
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.
Yes, I think we should remove it for now.