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

[extension/oauth2clientauth] Enable dynamically reading ClientID and ClientSecret from command #32602

Closed
elikatsis opened this issue Apr 22, 2024 · 4 comments
Labels
enhancement New feature or request extension/oauth2clientauth needs triage New item requiring triage Stale

Comments

@elikatsis
Copy link
Contributor

Component(s)

extension/oauth2clientauth

Is your feature request related to a problem? Please describe.

Hi all!

I have a [non-K8s] cluster which uses an OTEL collector as a systemd service to export telemetry data to a different platform for centralized control. This collector needs to authenticate against this platform by using and OIDC client with credentials which get rotated frequently.

There are multiple services in the platform which have to use this OIDC client, so the credentials are stored a keystore (a local one, AWS Secrets Manager, other). There is more information in this store that the services share, but it is out of scope of this feature.
There is a dedicated service which updates the information in the keystore. As part of this, the service is responsible to keep the OIDC client credentials up-to-date.

All in all, the OTEL collector needs to be using this OIDC client to export the data and, to do this, it needs to fetch the credentials from the keystore.

Describe the solution you'd like

The extension should allow executing commands to retrieve the client credentials. This practice is common in software, mainly when retrieving passwords. For example:

I have implemented a solution which extends the extension's user-facing config with two fields: client_id_cmd and client_secret_cmd. These fields expect a list of strings which represent the command to execute.

The extension will then execute the command when it needs to issue a new token. This is pretty much another source for client ID and secret, just as getting the credentials from files is (see #26117). The required changes are pretty minimal to get this working.

Describe alternatives you've considered

I'm open to suggestions on doing this differently. Nevertheless, I think it's important for this to be an OTEL-native functionality, since it's common practice.

An alternative would be to have a separate service put the credentials in a file and use the read-file feature of the extension. However, this over-complicates things. There are already services/controllers keeping a store in sync, it's not optimal to have yet another service since it will have to be very specific to this case. Also, it could be that in some usecases no another (non-privileged) service is able to access the filesystem of the OTEL collector. Plus, performance-wise, the extension will execute the command only when it needs to renew the token while the extra service would have to run periodically.

It makes sense for the extension to be reading from a store.

Additional context

In a little greater detail, this proposal by @jpkrohling made total sense eventually. Design-wise it is far better and follows the open-closed principle! So I pivoted the design to implement something similar. Since this is not an exposed API nor one that should be used directly anyways, the change is safe.

Moreover, regarding @jpkrohling thoughts on not allowing the collector to execute commands in this review comment:

as mentioned in the issue, get by executing a command

I missed that, and this specific example is unlikely to get accepted: we probably do not want the collector executing commands.

I guess this is a security concern and I hear you. My 2 cents on this is that this is an admin configuration and input. The admin should be able to

  1. run the service as a user with the necessary privileges only,
  2. not allow privilege escalation, and
  3. ensure a safe configuration.

The service shouldn't forbid a functionality in such a case which should be considered a trusted input.

I'm looking forward to your feedback, and I'll open the PR soon for all project stakeholders to take a look!

@elikatsis elikatsis added enhancement New feature or request needs triage New item requiring triage labels Apr 22, 2024
Copy link
Contributor

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@jpkrohling
Copy link
Member

I think we have precedence around executing commands from within the collector with the JMX receiver, but I'm still uneasy about this. I'll review the PR, but I'd like to get clearance from other @open-telemetry/collector-contrib-approvers before merging.

Copy link
Contributor

github-actions bot commented Jul 8, 2024

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

See Adding Labels via Comments if you do not have permissions to add labels yourself.

@jpkrohling
Copy link
Member

I'm closing this for the same reason I closed the PR: #32623 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request extension/oauth2clientauth needs triage New item requiring triage Stale
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants