Add new `--oidc-use-access-token` flag to `get-token` #1084

adkafka · 2024-04-25T23:29:23Z

Implements #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 updating 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, changes to this may effect which token is cached and validated. So I'm hesitant to make this change unless we think it is necessary.

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.

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.

v0lkc · 2024-05-14T10:51:41Z

pkg/oidc/client/client.go

+			IDToken:      accessToken,
+			RefreshToken: token.RefreshToken,
+		}, nil
+	}


Don't you think it should be enough to select the other token here?

kubelogin/pkg/oidc/client/client.go

Line 196 in 94ca6bc

idToken, ok := token.Extra("id_token").(string)

The way how it's implemented now would actually lead to a verification of both tokens if --oidc-use-access-token is set, which is in my opinion not necessary.

That is a valid alternative approach. I chose not do that initially here because the id_token has a nonce while the access_token does not. So if we only validate the access_token, we will lose the nonce check. It seemed preferred to me to maintain this nonce check but use the access_token.

adkafka · 2024-05-21T21:09:52Z

I realize I have some tests failing because I haven't updated the mocks (mockery --all --inpackage --with-expecter). Fixing that and adding at least one test shortly.

EDIT: This actually isn't so easy. The version of mockery used previously (v2.13.1) seems incompatible with our current go version. We are running into this issue: https://vektra.github.io/mockery/v2.35/notes/#internal-error-package-without-types-was-imported

Needed to plumb through our new parameter `UseAccessToken` to the mocks as well.

adkafka · 2024-05-21T22:12:16Z

pkg/oidc/client/mock_FactoryInterface.go

-// New provides a mock function with given fields: ctx, p, tlsClientConfig
-func (_m *MockFactoryInterface) New(ctx context.Context, p oidc.Provider, tlsClientConfig tlsclientconfig.Config) (Interface, error) {
+// New provides a mock function with given fields: ctx, p, tlsClientConfig, useAccessToken
+func (_m *MockFactoryInterface) New(ctx context.Context, p oidc.Provider, tlsClientConfig tlsclientconfig.Config, useAccessToken bool) (Interface, error) {


Note that I had to manually make these changes, which is not great. I couldn't get mockery v2.13.1 to work on my machine. I believe we have to upgrade to a newer version of mockery to work with this version of Go. I was getting this error:

$ ~/Downloads/mockery_2.13.1_Darwin_x86_64/mockery --all --inpackage --with-expecter 21 May 24 15:11 PDT INF Starting mockery dry-run=false version=v2.13.1 21 May 24 15:11 PDT INF Walking dry-run=false version=v2.13.1 2024/05/21 15:11:45 internal error: package "context" without types was imported from "github.com/int128/kubelogin/integration_test/httpdriver"

Which is mentioned here: https://vektra.github.io/mockery/v2.35/notes/#error-no-go-files-found-in-root-search-path. I decided not to tackle this in this PR. We can discuss doing so in a follow-up PR if that makes sense.

adkafka · 2024-05-21T22:16:13Z

@int128 Are you able to take a look at this PR when you have a chance? I'm happy to update if you have suggestions. I wasn't sure if there are any better unit tests to add than the ones I added. And I'm unsure if it is worth updating TokenSet fields such that we actually store the AccessToken as opposed to overloading the IDToken field. Thanks!

jsphpl · 2024-05-31T17:55:32Z

pkg/oidc/client/client.go

+		if !ok {
+			return nil, fmt.Errorf("access_token is missing in the token response: %#v", accessToken)
+		}
+		_, err := verifier.Verify(ctx, accessToken)


I think there is no need to verify the token on the client. We're not granting the user any access based on the content of the token. Instead we're just passing the token on to the k8s API, where it has to be verified.

If we stick with verifying the token, the correct audience (client_id) would have to be passed to the verifier in line 201. The aud of the access_token is not necessarily the client_id requesting the token.

Addition: The OIDC spec does not mandate the format of an access token. So my comment about the audience when verifying the access token is irrelevant.

https://openid.net/specs/openid-connect-core-1_0.html#CodeFlowTokenValidation describes the verification of the access token using the at_hash claim. This of course requires a valid signature of the id token. Which brings us back to the question whether we should do any verification of the tokens at all, given we're the client and not the resource server.

I think there is no need to verify the token on the client. We're not granting the user any access based on the content of the token. Instead we're just passing the token on to the k8s API, where it has to be verified.

I see your point here. Kubelogin is only passing the token to kubernetes, so we don't gain much by validating it. However, we currently validate the id_token, so my inclination is to follow that precedent in this PR. If we want to add a CLI flag to disable local token verification, I think that could make sense. But I'd argue that is out of scope of this particular issue / PR.

If we stick with verifying the token, the correct audience (client_id) would have to be passed to the verifier in line 201. The aud of the access_token is not necessarily the client_id requesting the token.

Interesting. In my case, the client_id is the aud for both the id_token and the access_token. Is this a common case worth supporting? Or do we think it is sufficient to (in a separate PR) add a flag that disables local token validation?

Addition: The OIDC spec does not mandate the format of an access token. So my comment about the audience when verifying the access token is irrelevant.

https://openid.net/specs/openid-connect-core-1_0.html#CodeFlowTokenValidation describes the verification of the access token using the at_hash claim. This of course requires a valid signature of the id token. Which brings us back to the question whether we should do any verification of the tokens at all, given we're the client and not the resource server.

Correct me if I'm wrong, but that validation only applies to access tokens acquired from the "authorization" endpoint. In my flow at least, we are using the "Authentication Code" flow. First, we make a call to /authorize to optian an authorization code, then we make a call to /token to obtain the id_token and access_token. In my case, the id_token will only populate the at_hash claim if the token is obtained using the "implicit" flow, where the token is returned directly from the /authorize call.

Is this a common case worth supporting?

Many OIDC IdP will issue an access token with the same structure as an OIDC ID Token. But as far as my understanding goes, this is incidental and not mandated by the spec. So validating an access token the same way one would validate an ID token might or might not work. The access token could be an opaque string that only has meaning to the resource server.

If the access token is a JWT after the OIDC spec (as with Azure, Auth0, etc.), the only reasonable value for aud, in my opinion, would be the client ID of the resource server because that's who the access token is meant for.

So I would vote for either a) not verify the access token at all or b) only verify it using the at_hash, if present. Tendency towards a) because b) implies verifying the ID token, which, in my opinion, is pointless on the client side.

In my flow at least, we are using the "Authentication Code" flow

Section 3.1.3.8. of the same Document describes the same procedure for the auth code flow. The at_hash claim is optional in that case.

So I would vote for either a) not verify the access token at all or b) only verify it using the at_hash, if present. Tendency towards a) because b) implies verifying the ID token, which, in my opinion, is pointless on the client side.

Given that we currently verify the ID token in this application and changing that is out of scope of this PR, I don't think option a) fits well in here. As I mentioned earlier, I'd like this change to match the existing precedence set by this application, at least until the maintainer indicates otherwise. Perhaps a good in-between we can consider is to check the signature of the access_token but not verify the aud claim matches the client_id. I think we can accomplish this by creating a different verifier for the access_token. Here is the one we create for the id_token:

https://github.com/int128/kubelogin/blob/f9c3c8aee74ff81784c5d5e1b8c2cbcc76aff8a5/pkg/oidc/client/client.go#L201C25-L201C33

We can instead set ClientID to a blank string and set SkipClientIDCheck to true. See config parameters here. As the docs for go-oidc state, there are some cases where the aud will not match the client_id. This use of an access_token seems to fit that use case

Section 3.1.3.8. of the same Document describes the same procedure for the auth code flow. The at_hash claim is optional in that case.

Agreed. Using the at_hash route for validation is a possibility. But that field is optional, so it won't work to validate the token in all cases. If I understand correctly, this validation route (using at_hash) is primarily useful when the access_token is NOT a signed JWT. If the access_token is a signed JWT (which it will have to be to successfully use this code path for kubernetes), then it is preferable to me to validate the JWT the way we are currently (with the potential caveat above about intentionally not checking the aud claim).

Sounds good to me.

which it will have to be to successfully use this code path for kubernetes

Very good point!

@jsphpl I just pushed 1add63b which should resolve this issue.

jsphpl · 2024-05-31T18:04:49Z

I can confirm that the --oidc-use-access-token works as intended.

However, I had to disable verification of the access_token because I passed --oidc-auth-request-extra-params="audience=CLUSTER_CLIENT_ID". Verifying the access_token is not necessary anyway, see my comments above.

Btw. verifying the id_token isn't necessay either, because we're just the client. Verification has to happen on the resource server (k8s API). We're not granting the user any access – we're merely passing the token to the resource server. Or are we?

As noted in the PR, there are some cases where the access token `aud` field will not be the `client_id`. To allow for these, we use a different token verifier that will not verify that claim.

adkafka · 2024-06-17T23:07:54Z

However, I had to disable verification of the access_token because I passed --oidc-auth-request-extra-params="audience=CLUSTER_CLIENT_ID". Verifying the access_token is not necessary anyway, see my comments above.

This should be resolved now, after I pushed 1add63b.

v0lkc reviewed May 14, 2024

View reviewed changes

adkafka mentioned this pull request May 21, 2024

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

Open

Adam kafka added 2 commits May 21, 2024 15:07

Fix failing tests

859d51b

Needed to plumb through our new parameter `UseAccessToken` to the mocks as well.

Add a test to make sure new flag is plumbed through

f9c3c8a

adkafka commented May 21, 2024

View reviewed changes

jsphpl reviewed May 31, 2024

View reviewed changes

Support Access Tokens whose audience differ from the client_id

1add63b

As noted in the PR, there are some cases where the access token `aud` field will not be the `client_id`. To allow for these, we use a different token verifier that will not verify that claim.

adkafka requested a review from jsphpl June 17, 2024 23:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new `--oidc-use-access-token` flag to `get-token` #1084

Add new `--oidc-use-access-token` flag to `get-token` #1084

adkafka commented Apr 25, 2024 •

edited

v0lkc May 14, 2024

adkafka May 14, 2024

adkafka commented May 21, 2024 •

edited

adkafka May 21, 2024

adkafka commented May 21, 2024

jsphpl May 31, 2024

jsphpl May 31, 2024

jsphpl Jun 1, 2024 •

edited

adkafka Jun 3, 2024

jsphpl Jun 4, 2024

adkafka Jun 5, 2024

jsphpl Jun 5, 2024

adkafka Jun 17, 2024

jsphpl commented May 31, 2024

adkafka commented Jun 17, 2024

Add new --oidc-use-access-token flag to get-token #1084

Are you sure you want to change the base?

Add new --oidc-use-access-token flag to get-token #1084

Conversation

adkafka commented Apr 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adkafka commented May 21, 2024 • edited

Choose a reason for hiding this comment

adkafka commented May 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsphpl Jun 1, 2024 • edited

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

jsphpl commented May 31, 2024

adkafka commented Jun 17, 2024

Add new `--oidc-use-access-token` flag to `get-token` #1084

Add new `--oidc-use-access-token` flag to `get-token` #1084

adkafka commented Apr 25, 2024 •

edited

adkafka commented May 21, 2024 •

edited

jsphpl Jun 1, 2024 •

edited