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.

Sign up for GitHub

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
Draft

Add TLS settings to all connection settings #205

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a174175
Add TLS and proxy settings to connection settings
michel-laterman Oct 15, 2024
7849fe0
Change status to Development
michel-laterman Oct 16, 2024
15fdbc0
Only add TLSSettings with this PR
michel-laterman Oct 22, 2024
41ad8fb
Clarify ca attribute names, add ca trust on use to spec
michel-laterman Oct 23, 2024
05f2846
linter fix
michel-laterman Oct 23, 2024
2b15b10
Remove ca_file_path
michel-laterman Nov 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions proto/opamp.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,10 @@ message OpAMPConnectionSettings {
// If this field has no value or is set to 0, the Agent should not send any heartbeats.
// Status: [Development]
uint64 heartbeat_interval_seconds = 4;

// Optional connection specific TLS settings.
// Status: [Development]
TLSConnectionSettings tls = 5;
}

// The TelemetryConnectionSettings message is a collection of fields which comprise an
Expand All @@ -303,6 +307,10 @@ message TelemetryConnectionSettings {
// This field is optional: if omitted the client SHOULD NOT use a client-side certificate.
// This field can be used to perform a client certificate revocation/rotation.
TLSCertificate certificate = 3;

// Optional connection specific TLS settings.
// Status: [Development]
TLSConnectionSettings tls = 4;
}

// The OtherConnectionSettings message is a collection of fields which comprise an
Expand Down Expand Up @@ -348,6 +356,37 @@ message OtherConnectionSettings {
// Other connection settings. These are Agent-specific and are up to the Agent
// interpret.
map<string, string> other_settings = 4;

// Optional connection specific TLS settings.
// Status: [Development]
TLSConnectionSettings tls = 5;
}

// 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)


// Optional path to the CA file on disk
string ca_file = 2;
michel-laterman marked this conversation as resolved.
Show resolved Hide resolved

// 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.


// Load system CA pool alongside any specifed CAs (provided through ca_file or ca_path).
bool include_system_ca_certs_pool = 4;

// skip certificate verification
bool insecure_skip_verify = 5;

// Miniumum accepted TLS version; default "1.2".
string min_version = 6;

// Maxiumum accepted TLS version; default "".
string max_version = 7;

// Explicit list of cipher suites.
repeated string cipher_suites = 8;
}

// Status: [Beta]
Expand Down