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

cmd/krp/app,pkg/authn: make OIDC token aud agnostic #343

Open
wants to merge 1 commit into
base: sig-auth-acceptance
Choose a base branch
from

Conversation

ibihim
Copy link
Collaborator

@ibihim ibihim commented Dec 11, 2024

What

Make OIDC authenticator audience agnostic.

Why

Mo said so. And I assume the point is that the aud in an ID token will be the client ID and the overwrites the classical meaning of an "audience".

@stlaz
Copy link
Collaborator

stlaz commented Dec 13, 2024

I don't see a reason why we would want to allow OIDC authenticator responses to not contain audiences.

@ibihim
Copy link
Collaborator Author

ibihim commented Dec 17, 2024

I know what you mean. I am not sure as well, but this is how I understood it.

@ibihim ibihim added the sig-auth-acceptance issues created during review for sig-auth-acceptance label Dec 17, 2024
@enj
Copy link

enj commented Dec 17, 2024

I don't see a reason why we would want to allow OIDC authenticator responses to not contain audiences.

That is not what "audience agnostic" means. Audience agnostic means that the authenticator never returns an authenticator.Response with the Audiences field set. This is always true for the OIDC authenticator, meaning it is always audience agnostic. See kubernetes/kubernetes#87612 for the history around this. By using WrapAudienceAgnosticToken, all we are doing is artificially setting the authenticator.Response.Audiences to match the audiences configured for the proxy, which is strictly more correct for the proxy because it only has one concept of an audience, whereas the Kube API server has both "API audiences" and "JWT audiences." FWIW, I don't think any consumer of the authenticator.Response in this codebase cares either way.

@enj
Copy link

enj commented Dec 19, 2024

Hm, this part of WrapAudienceAgnosticToken would make all of this a no-op unless something is also calling WithAudiences.

@ibihim
Copy link
Collaborator Author

ibihim commented Dec 19, 2024

@enj, yes this is a programmatic representation of my confusion. I asked you about that no-op a couple of months ago, but I think I might have missed something.

AFAIU, the audience of the ID token in the OIDC spec is the client ID. It is being checked by the apiserver, if not skipped. This is nice and useful, but it doesn't specify any of our own audiences, like we are used to have in Service Account Tokens (as you highlighted in your references / previous PRs).

This means it wouldn't make sense for us to "verify audiences" as that check:

  1. It is already done by the apiserver / jwtAuthenticator.
  2. It does never answer if it is meant for us or our upstream component.

But I am not quite sure how and where to make our code "audience agnostic" in that use case. The oidcAuthenticator does check the clienID / Audience in the jwtAuthenticator. The WithAuthenticator doesn't care for audience checks, if the response doesn't return audiences: if len(apiAuds) == 0 || len(responseAudiences) == 0 .

Therefore I assumed some ... current no-op... that might be future proof, in case that audiences are enforced at some point and it is already best practice to respect that?

@enj
Copy link

enj commented Dec 19, 2024

I think this all comes down to:

  • How likely are we to accept tokens that somehow target the proxy with a different audience than the OIDC one? This seems highly unlikely to me.
  • Does any of the code that consumes the authentication result care about the audience, now or in the future?

My gut says that either way will be fine, so if you prefer to do #329 since it is simpler, I am fine with that. If you go with this PR, then I think WithAuthenticator should be passed in the OIDC auds we expect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig-auth-acceptance issues created during review for sig-auth-acceptance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants