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

elikatsis · 2024-04-23T08:25:15Z

Description:
This PR implements the feature described in detail in the issue linked below.

In a nutshell, it extends the oauth2clientauth extension to retrieve ClientID and/or ClientSecret by executing a command whenever a new token is needed for the OAuth flow.
As a result, the extension can use updated credentials (when the old ones expire and they get rotated for example) without the need to restart the OTEL collector, as long as the command returns a different output (e.g., fetch credentials from a secret store).

As part of this PR I also modify the design of the code which is responsible to get the actual client ID and secret values based on an old review comment. I have exposed the details in the tracking issue.

Link to tracking Issue: Closes #32602

Testing:

Unit testing: Extended the unit tests to ensure the functionality
Tested in realistic environments both as a systemd service and a docker container (exporting data with the otlphttp exporter)
The collectors export data to a service which sits behind and OIDC authentication proxy and the oauth2clientauth extension successfully retrieves the credentials from a secret store and authenticates against the service.

Documentation:
I have extended the extension's README file.

elikatsis · 2024-04-25T08:22:10Z

Friendly ping @pavankrish123 @jpkrohling for visibility.
WDYT?

jpkrohling

I feel like some changes to the tests weren't necessary, but this looks good. I'd still like to have extra 👀 from other @open-telemetry/collector-contrib-approvers, given this code spanws arbitrary commands to get values. This could be used as vector together with another vulnerability, to send confidential data to an attacker by making a curl call to a property controlled by the attacker.

I would probably prefer to allow only one argument to the cmd, requiring admins to wrap a cmd+args in a shell script on the machine. This way, we are certain that only those commands are executed.

jpkrohling · 2024-05-07T10:23:19Z

extension/oauth2clientauthextension/extension_test.go

@@ -125,8 +131,6 @@ func TestOAuthClientSettingsCredsConfig(t *testing.T) {
 			settings: &Config{
 				ClientIDFile: testCredsFile,
 				ClientSecret: "testsecret",
-				TokenURL:     "https://example.com/v1/token",


What happened here?

PTAL at my answer at your other comment where I drop all transport TLS settings -related assertions from this test.

jpkrohling · 2024-05-07T10:24:00Z

extension/oauth2clientauthextension/extension_test.go

@@ -186,13 +228,6 @@ func TestOAuthClientSettingsCredsConfig(t *testing.T) {
 			assert.NoError(t, err)
 			assert.Equal(t, test.expectedClientConfig.ClientID, cfg.ClientID)
 			assert.Equal(t, test.expectedClientConfig.ClientSecret, cfg.ClientSecret)
-
-			// test tls settings
-			transport := rc.client.Transport.(*http.Transport)


Why was this removed?

The test TestOAuthClientSettingsCredsConfig, I introduced it in my previous contribution (#26310). I included the TLS settings as a copying-pasting artifact, but I shouldn't have them since it's duplication.
We are testing the transport and TLS configuration [more extensively] in the TestOAuthClientSettings test (current source).
I removed the // test tls settings code to have a single place to test TLS settings (could even be split to a separate test) and, if anything changes on the TLS front, drop the need to touch this unrelated test (for example #32332, #32394).

Moreover, related to your other comment about removing TokenURL and Scopes assignments, my previous contribution had to assign a value to the TokenURL field because at that point we've been validating the configuration when calling newClientAuthenticator(), and a missing TokenURL got this test breaking [kind of falsely].
But, later, #30469 removed this validation logic. So we don't need to configure it anymore for these tests, and I deleted the relevant lines to drop the [now] unnecessary boilerplate.

I'll bring them back if you believe they should be there.

No, I think it's fine, thank you for the explanation. My concern was whether the new code introduced changes in how we handle TLS, which wasn't apparent at first.

Actually, can you revert this change, and send a new PR removing it? Just so that we isolate the changes, ensuring they are not related.

I had it in a separate commit. I pushed a reversion commit. I'll send a new PR when we move forward with this one.

jpkrohling · 2024-05-07T10:27:48Z

extension/oauth2clientauthextension/value_sources.go

+	cmd := exec.Command(cmdArgs[0], cmdArgs[1:]...) // #nosec G204
+	outBytes, err := cmd.Output()
+	if err != nil {
+		return "", err


Suggested change

return "", err

return "", fmt.Errorf("failed to execute the command %q to obtain credential details: %v", err)

Makes sense! I'll push a fixup commit since your suggestion is missing the strings.Join() for %q and also use %w instead of %v.

jpkrohling · 2024-05-07T10:36:21Z

extension/oauth2clientauthextension/README.md

@@ -75,10 +75,16 @@ Following are the configuration fields
 - **client_id_file** - The file path to retrieve the client identifier issued to the client.
  The extension reads this file and updates the client ID used whenever it needs to issue a new token. This enables dynamically changing the client credentials by modifying the file contents when, for example, they need to rotate. <!-- Intended whitespace for compact new line -->  
  This setting takes precedence over `client_id`.
+- **client_id_cmd** - The command to retrieve the client identifier issued to the client. Expects a list of strings.


Is this the only list of strings we have on this file? If so, please provide an example. Add another example in the testdata's config as well please.

We also have the scopes field as a list of strings. Nevertheless, I'll push an extra commit to showcase this.

elikatsis

@jpkrohling thanks a lot for your review!

I pushed two extra commits to address your comments and replied on your comments for the changes in tests.

I would probably prefer to allow only one argument to the cmd, requiring admins to wrap a cmd+args in a shell script on the machine. This way, we are certain that only those commands are executed.

I understand your concerns. However, it's not trivial to do this cmd+args wrapping when deploying the collector as a container, or it would require extra effort and not to use the official community image out-of-the-box.
When deploying the OTEL collector in some other way which allows the software to access a binary from the host filesystem (e.g., a systemd service), a vulnerability which enables the attacker to modify this extension's pointer and run arbitrary commands is probably the last concern since it sounds like there's a bigger security hole in the system.
I am no security expert, though, so I could be very off here :p

I'll also take a look at the JMX receiver you mentioned in the issue.

elikatsis · 2024-05-08T11:26:04Z

extension/oauth2clientauthextension/extension_test.go

@@ -125,8 +131,6 @@ func TestOAuthClientSettingsCredsConfig(t *testing.T) {
 			settings: &Config{
 				ClientIDFile: testCredsFile,
 				ClientSecret: "testsecret",
-				TokenURL:     "https://example.com/v1/token",


PTAL at my answer at your other comment where I drop all transport TLS settings -related assertions from this test.

elikatsis · 2024-05-08T11:55:45Z

extension/oauth2clientauthextension/extension_test.go

@@ -186,13 +228,6 @@ func TestOAuthClientSettingsCredsConfig(t *testing.T) {
 			assert.NoError(t, err)
 			assert.Equal(t, test.expectedClientConfig.ClientID, cfg.ClientID)
 			assert.Equal(t, test.expectedClientConfig.ClientSecret, cfg.ClientSecret)
-
-			// test tls settings
-			transport := rc.client.Transport.(*http.Transport)


The test TestOAuthClientSettingsCredsConfig, I introduced it in my previous contribution (#26310). I included the TLS settings as a copying-pasting artifact, but I shouldn't have them since it's duplication.
We are testing the transport and TLS configuration [more extensively] in the TestOAuthClientSettings test (current source).
I removed the // test tls settings code to have a single place to test TLS settings (could even be split to a separate test) and, if anything changes on the TLS front, drop the need to touch this unrelated test (for example #32332, #32394).

Moreover, related to your other comment about removing TokenURL and Scopes assignments, my previous contribution had to assign a value to the TokenURL field because at that point we've been validating the configuration when calling newClientAuthenticator(), and a missing TokenURL got this test breaking [kind of falsely].
But, later, #30469 removed this validation logic. So we don't need to configure it anymore for these tests, and I deleted the relevant lines to drop the [now] unnecessary boilerplate.

I'll bring them back if you believe they should be there.

elikatsis · 2024-05-08T11:57:02Z

extension/oauth2clientauthextension/value_sources.go

+	cmd := exec.Command(cmdArgs[0], cmdArgs[1:]...) // #nosec G204
+	outBytes, err := cmd.Output()
+	if err != nil {
+		return "", err


Makes sense! I'll push a fixup commit since your suggestion is missing the strings.Join() for %q and also use %w instead of %v.

elikatsis · 2024-05-08T12:18:36Z

extension/oauth2clientauthextension/README.md

@@ -75,10 +75,16 @@ Following are the configuration fields
 - **client_id_file** - The file path to retrieve the client identifier issued to the client.
  The extension reads this file and updates the client ID used whenever it needs to issue a new token. This enables dynamically changing the client credentials by modifying the file contents when, for example, they need to rotate. <!-- Intended whitespace for compact new line -->  
  This setting takes precedence over `client_id`.
+- **client_id_cmd** - The command to retrieve the client identifier issued to the client. Expects a list of strings.


We also have the scopes field as a list of strings. Nevertheless, I'll push an extra commit to showcase this.

Introduce a constructor for clientCredentialsConfig to make the object details more abstract to the rest of the extension.

In preparation for a minor design enhancement, introduce value sources for raw values and values read from files.

Extend the clientCredentialsConfig object with value sources for ClientID and ClientSecret. It does not make actual use of them yet.

Use the recent changes related to value sources to retrieve the client ID and secret during the final Config creation.

Extend the Config object to expect 'client_id_cmd' and 'client_secret_cmd' values as lists of strings to retrieve the client ID and/or secret by executing commands.

Drop parts related to TLS settings and token retrieval already verified in previous tests.

This reverts commit bc4f2c210e70df1520cd6116a9deaa5b6c29b1cf.

jpkrohling · 2024-05-08T13:08:07Z

a vulnerability which enables the attacker to modify this extension's pointer and run arbitrary commands is probably the last concern since it sounds like there's a bigger security hole in the system.

A successful attack is typically the result of using a few attack vectors to exploit a vulnerability. So, while this likely wouldn't introduce a vulnerability, it could be used as a vector in an attack.

If other approvers are OK, with this change, I'm also OK in accepting it.

**Description:** Drop duplicate testing of parts related to TLS settings which already verified more extensively in other tests. This led to relevant changes requiring identical modifications in multiple places (e.g., #32332, #32394). Also remove 'TokenURL' and 'Scopes' from the corresponding configs as they are not required anymore (since #30469). For more details, see also #32623 (comment) Signed-off-by: Ilias Katsakioris <ilias.katsakioris@hpe.com>

github-actions · 2024-05-23T05:20:35Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

elikatsis · 2024-05-23T06:20:01Z

Not stale

github-actions · 2024-06-07T05:20:16Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

elikatsis · 2024-06-07T05:22:39Z

@jpkrohling any update on this?

jpkrohling · 2024-06-18T11:20:47Z

I haven't heard from other approvers. Would you mind joining the SIG Collector and bringing this topic there? Or perhaps ask on #otel-collector on Slack. I'm still not sure I approve this change, at least not until I have more time to think about the possible attack scenarios. If other approvers have a different opinion and want to approve this instead, I won't block it.

github-actions · 2024-07-03T05:20:42Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

jpkrohling · 2024-07-09T08:32:18Z

@elikatsis, I'm sorry for this taking so long, but I think we'll need to close this. I'm VERY happy about your engagement and proposal, and I think this suggestion goes in the right direction. However, I'm not 100% comfortable with allowing this component to execute arbitrary commands on the server. In general, I agree that there's a bigger concern if the Collector can be tricked into doing that, but this feature could be used as a vector in a bigger vulnerability.

My recommendation for your specific case (although I suspect you are doing this already): host a version of this component on your own repository, and build your distribution with this component. This way, you can use this feature and distribute it to other users, even if it's not part of the official distribution. Yes, it's a bit more effort, but I think it's justified given the use-case.

Again, I'm sorry for closing this, but I think it's the right thing for the moment.

elikatsis · 2024-07-11T09:22:28Z

@jpkrohling thank you for taking the time to consider this.

Unfortunately I didn't make it to start a discussion on Slack or a community meeting in time. But, no matter the outcome, it's been a great back and forth, both in this and my previous contribution.

Keep up the good work!

elikatsis requested a review from jpkrohling as a code owner April 23, 2024 08:25

elikatsis requested a review from a team April 23, 2024 08:25

github-actions bot assigned atoulme Apr 23, 2024

github-actions bot added the extension/oauth2clientauth label Apr 23, 2024

github-actions bot requested a review from pavankrish123 April 23, 2024 08:25

jpkrohling reviewed May 7, 2024

View reviewed changes

elikatsis commented May 8, 2024

View reviewed changes

elikatsis added 14 commits May 8, 2024 15:47

Introduce clientCredentialsConfig constructor

eed909f

Introduce a constructor for clientCredentialsConfig to make the object details more abstract to the rest of the extension.

Introduce value sources for the supported value types

a1123fe

In preparation for a minor design enhancement, introduce value sources for raw values and values read from files.

Extend clientCredentialsConfig with value sources

39ef61d

Extend the clientCredentialsConfig object with value sources for ClientID and ClientSecret. It does not make actual use of them yet.

Use the new value sources to retrieve client ID and secret

e2c2ae8

Use the recent changes related to value sources to retrieve the client ID and secret during the final Config creation.

Drop unnecessary references to files

3656822

Move the readCredentialsFile helper to value sources

04e3161

Introduce a value getter which invokes a command

882c4b0

Extend the Config to retrieve ID or secret values from commands

e143245

Extend the Config object to expect 'client_id_cmd' and 'client_secret_cmd' values as lists of strings to retrieve the client ID and/or secret by executing commands.

Extend tests to cover the new CMD options

5379e83

Drop parts tested in other tests

3848248

Drop parts related to TLS settings and token retrieval already verified in previous tests.

Add Changelog entry

6a5adcd

Add explanatory message on command error

9fcc405

Add a testdata config example showcasing the cmd feature

2dcd6c0

Revert "Drop parts tested in other tests"

8e266c6

This reverts commit bc4f2c210e70df1520cd6116a9deaa5b6c29b1cf.

elikatsis mentioned this pull request May 10, 2024

[chore][extension/oauth2clientauth] Drop duplicate testing #32980

Merged

github-actions bot added the Stale label May 23, 2024

github-actions bot removed the Stale label May 24, 2024

github-actions bot added the Stale label Jun 7, 2024

github-actions bot removed the Stale label Jun 8, 2024

github-actions bot added the Stale label Jul 3, 2024

jpkrohling closed this Jul 9, 2024

jpkrohling mentioned this pull request Jul 9, 2024

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

elikatsis commented Apr 23, 2024 •

edited

Loading

elikatsis commented Apr 25, 2024

jpkrohling left a comment

jpkrohling May 7, 2024

elikatsis May 8, 2024

jpkrohling May 7, 2024

elikatsis May 8, 2024

jpkrohling May 8, 2024 •

edited

Loading

jpkrohling May 8, 2024

elikatsis May 8, 2024

jpkrohling May 7, 2024

elikatsis May 8, 2024

jpkrohling May 7, 2024

elikatsis May 8, 2024

elikatsis left a comment

elikatsis May 8, 2024

elikatsis May 8, 2024

elikatsis May 8, 2024

elikatsis May 8, 2024

jpkrohling commented May 8, 2024

github-actions bot commented May 23, 2024

elikatsis commented May 23, 2024

github-actions bot commented Jun 7, 2024

elikatsis commented Jun 7, 2024

jpkrohling commented Jun 18, 2024

github-actions bot commented Jul 3, 2024

jpkrohling commented Jul 9, 2024

elikatsis commented Jul 11, 2024

	return "", err
	return "", fmt.Errorf("failed to execute the command %q to obtain credential details: %v", err)

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

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

Conversation

elikatsis commented Apr 23, 2024 • edited Loading

elikatsis commented Apr 25, 2024

jpkrohling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpkrohling May 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elikatsis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpkrohling commented May 8, 2024

github-actions bot commented May 23, 2024

elikatsis commented May 23, 2024

github-actions bot commented Jun 7, 2024

elikatsis commented Jun 7, 2024

jpkrohling commented Jun 18, 2024

github-actions bot commented Jul 3, 2024

jpkrohling commented Jul 9, 2024

elikatsis commented Jul 11, 2024

elikatsis commented Apr 23, 2024 •

edited

Loading

jpkrohling May 8, 2024 •

edited

Loading