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

feat: export unexpected audience error #406

Closed
wants to merge 1 commit into from

Conversation

crenshaw-dev
Copy link

@crenshaw-dev crenshaw-dev commented Dec 15, 2023

Argo CD loops over an OIDC provider's configured allowed audiences, attempting to verify each one. If verification fails, we try more providers. https://github.com/argoproj/argo-cd/blob/0b35e2f1fe27f395e6106a7466d58911c4f7ec9c/util/oidc/provider.go#L108-L120

Since we don't know exactly why token verification fails, we have no way to know whether we should short-circuit the loop. After all, if the token is invalid for some reason besides a disallowed audience, it's a waste of time trying to validate against other audiences.

Knowing that the verification failed due to an invalid audience would allow us to do two things:

  1. short-circuit the loop if we encounter an error unrelated to audiences
  2. log only the relevant error message rather than continuing the loop and logging an unrelated error about unmatching audiences

Yucky workaround I'm using for now: argoproj/argo-cd#16625

Copy link
Collaborator

@ericchiang ericchiang left a comment

Choose a reason for hiding this comment

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

The main issue I have is that at this point the token isn't verified, so users should NOT be inspecting the audience for an unverified token without caution. We do this as a shortcut in the code, but have strict verification later on.

Maybe, audience should be renamed to "InsecureAudience"

// InsecureAudience is the audiences specified in the token. Note that the token may or
// may not have passed signature validation a this point, so these values should NOT be
// used outside of debugging.
InsecureAudience []string

But if a value can't be used safely is it worth exporting?

Broadly, there are probably better ways to handle this on your end. E.g. you'd probably better served by "SkipClientIDCheck" and then performing checks on the Audience yourself? That way, you're not relying on error inspection or what point in the verification the internal library is doing things.

https://pkg.go.dev/github.com/coreos/go-oidc/v3/oidc#Config

@crenshaw-dev
Copy link
Author

@ericchiang agreed, skipping the client ID check and then performing it in Argo CD is probably better. Thanks for the suggestion!

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

Successfully merging this pull request may close these issues.

2 participants