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/oidcauthextension] oidc ignore client/audience support #36569

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

zeck-ops
Copy link

Adds support for ignoring the audience/clientid in OIDC.
go-oidc has a config option for this, and the PR lets the collector use it.

Fixes 36568

I built a custom collector, and tested with a couple of aws cognito app id / client ids and JWTs from them with and without the option enabled. Also added a unit test.

Not sure what I should add to the readme, or if other documentation needs updated. Figured I would find out of the property names seemed ok first.

@zeck-ops zeck-ops requested review from jpkrohling and a team as code owners November 27, 2024 15:14
Copy link

linux-foundation-easycla bot commented Nov 27, 2024

CLA Not Signed

Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

Besides the previous comments, this PR would also need user-facing documentation (README), and a changelog entry. Take a look at the contributing guidelines at the root of this repo for instructions on what to do.

extension/oidcauthextension/config.go Outdated Show resolved Hide resolved
@zeck-ops zeck-ops changed the title oidc ignore client/audience support [extension/oidcauthextension] oidc ignore client/audience support Nov 27, 2024
@zeck-ops
Copy link
Author

Besides the previous comments, this PR would also need user-facing documentation (README), and a changelog entry. Take a look at the contributing guidelines at the root of this repo for instructions on what to do.

I have made my first attempt at those changes now. Hopefully they are close to correct.

Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

Nice, it looks great, thank you! I think you still need to sign the CLA. I'll approve CI to run as well, there might be a failure or two (linters!), but from my side, this is LGTM.

@zeck-ops
Copy link
Author

zeck-ops commented Dec 3, 2024

Cool! CLA is in progress. Fixed a lint failure.

@jpkrohling
Copy link
Member

I started the build again. Ping me once the CI is green and I'll merge this.

@zeck-ops
Copy link
Author

zeck-ops commented Dec 4, 2024

I started the build again. Ping me once the CI is green and I'll merge this.

@jpkrohling I think I have it now. There was another lint issue related to formatting, I believe, that should now be fixed. The error was confusing to me, and the linter is crashing locally. Hopefully I got it. Not super experienced with Go.

Thanks for the help.

@jpkrohling
Copy link
Member

CI started again.

@zeck-ops
Copy link
Author

zeck-ops commented Dec 5, 2024

CI started again.

@jpkrohling Missed a lint error. Fixed now.

I did get the linter running by temporarily locally bumping to golangci-lint v1.60.1
(although this causes some lint errors to appear in extension/solarwindsapmsettingsextension/config_test.go, so I dunno how close the behavior is to the project's version). I don't know why the version in the project crashes for me. Quite sorry for all the inconvenience. ☹️

@jpkrohling
Copy link
Member

CI started 🤞🏽

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

Successfully merging this pull request may close these issues.

[extension/oidcauthextension] Fixes oidc extension skip client id check
3 participants