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

Should have the option to use access_token instead of id_token #1083

Open
adkafka opened this issue Apr 25, 2024 · 0 comments
Open

Should have the option to use access_token instead of id_token #1083

adkafka opened this issue Apr 25, 2024 · 0 comments
Labels
enhancement New feature or request

Comments

@adkafka
Copy link

adkafka commented Apr 25, 2024

Purpose of the feature (why)

It should be possible to configure this tool to use an access_token instead of an id_token. Many identity providers expose extra claims in access_tokens that are not available in id_tokens. One concrete example is the scp claim in Azure AD that can be used to identify which scopes were requested. In my case, I'm using additional scopes to force the ID provider to trigger MFA, and I'd like to use --oidc-required-claim server-side to verify that the tokens issued have the necessary extra scope. Similiarly, AzureAD also only exposes the amr claim on the access token which more directly indicates if the user used MFA, however the values of that are an array, not a string, so I cannot simply use --oidc-required-claim with that.

The kubernetes docs indicate that we should be using the id_token, not the access_token, but there is more nuance to that. As an example, see this thread: Why use id tokens instead of access tokens for authorization. I can also share anecdotal data that there are other use cases that have come up in my work where we've had to resort to using the access_token instead of the id_token in an OIDC flow in order to access the necessary claims. Lastly, the Azure based kubelogin tool also uses the access_token, not the id_token. See logic here.

I believe the user of this tool should be able to choose to use access_tokens instead, assuming they understand these nuances, even though it is not the suggested path according to the official kubernetes docs.

Your idea (how)

I suggest adding a new CLI flag to the get-token command: --oidc-use-access-token. This will then return the access_token instead of the id_token in this method:

func (c *client) verifyToken(ctx context.Context, token *oauth2.Token, nonce string) (*oidc.TokenSet, error) {
. We will add this additional flag as a parameter to the OIDC client (pkg/oidc/client/client.go). See #1084 for the PR.

The default behavior will be unchanged (using id_token), this new behavior must be opted into.

@adkafka adkafka added the enhancement New feature or request label Apr 25, 2024
adkafka pushed a commit to adkafka/kubelogin that referenced this issue Apr 25, 2024
Implements int128#1083. See
description there for context.
adkafka pushed a commit to adkafka/kubelogin that referenced this issue Apr 25, 2024
Implements int128#1083. See
description there for context.
adkafka pushed a commit to adkafka/kubelogin that referenced this issue Apr 25, 2024
Implements int128#1083. See
description there for context.

In its current form, this PR is barebones functionality. I have not yet
added any tests to confirm this behavior. But I have validated it
locally. Without adding `--oidc-use-access-token`, and `id_token` is
successfully returned. Adding `--oidc-use-access-token` results in an
`access_token` being successfully returned.
adkafka pushed a commit to adkafka/kubelogin that referenced this issue Apr 25, 2024
Implements int128#1083. See
description there for context.

In its current form, this PR is bare bones functionality. I have not yet
added any tests to confirm this behavior. Additionally, we could
consider updtating some of the naming. It is confusing to return a
`TokenSet` where `IDToken` actually has an `accessToken`. I'm open to
feedback on how best to improve this.

However, this PR is functional in its current form. I have validated it
locally. Without adding `--oidc-use-access-token`, and `id_token` is
successfully returned. Adding `--oidc-use-access-token` results in an
`access_token` being successfully returned.
adkafka pushed a commit to adkafka/kubelogin that referenced this issue Apr 25, 2024
Implements int128#1083. See
description there for context.

In its current form, this PR is bare bones functionality. I have not yet
added any tests to confirm this behavior. Additionally, we could
consider updtating some of the naming. It is confusing to return a
`TokenSet` where `IDToken` actually has an `accessToken`. I'm open to
feedback on how best to improve this.

However, this PR is functional. I have validated it locally. Without
adding `--oidc-use-access-token`, and `id_token` is successfully
returned. Adding `--oidc-use-access-token` results in an `access_token`
being successfully returned.
@adkafka adkafka changed the title Should the option to use access_token instead of id_token Should have the option to use access_token instead of id_token Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant