Skip to content
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

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

michel-laterman
Copy link
Contributor

@michel-laterman michel-laterman commented Oct 15, 2024

Add new TLSConnectionSettings across all connection settings a server can offer.

Copy link

linux-foundation-easycla bot commented Oct 15, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the draft @michel-laterman

I left some comments, and I would like @andykellr to also review.

proto/opamp.proto Outdated Show resolved Hide resolved
proto/opamp.proto Outdated Show resolved Hide resolved
proto/opamp.proto Outdated Show resolved Hide resolved
proto/opamp.proto Outdated Show resolved Hide resolved
@michel-laterman michel-laterman force-pushed the feat/tls-proxy-connection-settings branch from e79ec75 to b8770b8 Compare October 16, 2024 16:20
@michel-laterman michel-laterman force-pushed the feat/tls-proxy-connection-settings branch from b8770b8 to 7849fe0 Compare October 16, 2024 16:21
Copy link
Member

@tigrannajaryan tigrannajaryan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would advise to break this down into 2 PRs: one that adds TLSConnectionSettings, the other that adds ProxyConnectionSettings.

If there are known uses cases for other_settings that should likely be the 3rd PR where we can discuss it.

proto/opamp.proto Outdated Show resolved Hide resolved
@michel-laterman michel-laterman changed the title Add other_settings, TLS, and proxy settings to connection settings Add TLS settings to all connection settings Oct 22, 2024
@michel-laterman
Copy link
Contributor Author

I'll recreate the ProxySettings in another pr

Comment on lines 370 to 374
// Optional path to the CA file on disk
string ca_file = 2;

// Alternative to ca_file, Provides CA cert contents as a string
string ca_pem = 3;
Copy link
Member

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 and OtherConnectionSettings, 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.

Copy link
Contributor Author

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 to ca_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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

however I can remove the it if we want to keep our security exposure low.

Yes, I think we should remove it for now.

proto/opamp.proto Outdated Show resolved Hide resolved
@michel-laterman michel-laterman force-pushed the feat/tls-proxy-connection-settings branch from fed1177 to 05f2846 Compare October 23, 2024 19:39
// Status: [Development]
message TLSConnectionSettings {
// Insecure is false by default, if true TLS will be disabled for the connection.
bool insecure = 1;
Copy link
Member

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.

Copy link
Contributor Author

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)

@tigrannajaryan
Copy link
Member

@michel-laterman we had a discussion with other OpAMP approvers/maintainers and decided to follow the Otel spec's requirements for making new proposals, namely to ask for prototypes that demonstrate the new capabilities. We will be formalizing the requirements in this PR: #207

I think it is important for this particular proposal to show how the TLS settings will be used. It is not a trivial change, so a working code would help understand it better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants