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

Need clarification on how OpAMP sends sensitive credential data to the client #80

Closed
dsvanlani opened this issue May 4, 2022 · 5 comments
Labels
required-for-stable Required to be resolved before 1.0

Comments

@dsvanlani
Copy link
Contributor

This was first discussed in the workgroup on 5/3/2022.

Issue

How do we handle sending sensitive credentials to the client? Consider this use case.

A client uses an exporter that needs an API Key to send data. It does not want to store the key in its configuration directly but rather in a file located on disk. OpAMP should establish a way to send this sensitive data so that a client can recognize it, store it, and not report it back in its EffectiveConfig status message. Additionally, OpAMP should have the capability of sending a message that would update these credentials.

It's not clear in the spec how these credentials should be sent.

Discussed solutions

  1. Send credentials in AgentRemoteConfig message.

In this case the client would need to know to redact certain fields of its EffectiveConfig status message.

  1. Send credentials in ConnectionSettingsOffers Message.

Similar to above but the client wouldn't be required to send its EffectiveConfig after this message. The message might need to be expanded to be able to send arbitrary key-value pairs, leaving it up to the client to know what to do with them.

@tigrannajaryan
Copy link
Member

We discussed the possibility to add arbitrary key/values to ConnectionSettings:

message ConnectionSettings {
   map<string,string> other_settings = 7;
}

A few things that we need to clarify with this approach:

  • ConnectionSettings also apply to OpAMP and OTLP connections, but they are pointless there. Should we have a different ConnectionSettingsForOther (or whatever we name it) message just for other_connections?
  • Do we need to need to keep all other fields ConnectionSettingsForOther if it is a separate message? If we keep them do we need to explain that other_settings should be used when the exiting built-in fields are not a good fit?

@tigrannajaryan
Copy link
Member

I looked at my initial prototype implementation where I actually use the OpAMP connection settings and own metrics connection settings and the code doesn't benefit in any way from the fact that ConnectionSettings is a shared message.

I am leaning towards having dedicated messages for each connection type, e.g.

message OpAMPConnectionSettings {
    string destination_endpoint = 1;
    Headers headers = 2;
    TLSCertificate certificate = 3;
    // in the future maybe we can add a field to specify the transport type for OpAMP (i.e. HTTP or WebSocket)
    ...
}

message TelemetryConnectionSettings {
    string destination_endpoint = 1;
    Headers headers = 2;
    TLSCertificate certificate = 3;
    // in the future maybe we can add a field to specify the transport type for OTLP (i.e. HTTP or gRPC)
    // or even add support for non-OTLP destinations.
}

message OtherConnectionSettings {
    string destination_endpoint = 1;
    Headers headers = 2;
    TLSCertificate certificate = 3;
    map<string,string> other_settings = 4; // agent-specific settings
    ...
}

Splitting the messages allows them to carry only the fields that are applicable to the particular destination type and allows to evolve them separately in the future.

@andykellr @dsvanlani what do you think?

@tigrannajaryan
Copy link
Member

@andykellr @pmm-sumo @dsvanlani here is a draft that shows what it looks like in Go: open-telemetry/opamp-go#82

What do you think?

tigrannajaryan added a commit to tigrannajaryan/opamp-spec that referenced this issue May 26, 2022
This changes splits connection settings by the type of the connection so that
each type has a corresponding message that records the appropriate data.

See proposal here open-telemetry#80 (comment)

Contributes to open-telemetry#80
tigrannajaryan added a commit to tigrannajaryan/opamp-spec that referenced this issue May 26, 2022
This changes splits connection settings by the type of the connection so that
each type has a corresponding message that records the appropriate data.

See proposal here open-telemetry#80 (comment)

I also deleted proxy-related settings since it wasn't clear when and how when
can be used. These can be added later when the use case is clear.

Contributes to open-telemetry#80
tigrannajaryan added a commit that referenced this issue May 26, 2022
This changes splits connection settings by the type of the connection so that
each type has a corresponding message that records the appropriate data.

See proposal here #80 (comment)

I also deleted proxy-related settings since it wasn't clear when and how when
can be used. These can be added later when the use case is clear.

Contributes to #80
@tigrannajaryan
Copy link
Member

@andykellr @dsvanlani now that we have the other_settings map is there anything else is needed for this issue?

@tigrannajaryan tigrannajaryan added the required-for-stable Required to be resolved before 1.0 label May 31, 2022
@andykellr
Copy link
Contributor

This should work well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
required-for-stable Required to be resolved before 1.0
Projects
None yet
Development

No branches or pull requests

3 participants