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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
db03e78
Add new `--oidc-use-access-token` flag to `get-token`
Apr 25, 2024
859d51b
Fix failing tests
May 21, 2024
f9c3c8a
Add a test to make sure new flag is plumbed through
May 21, 2024
1add63b
Support Access Tokens whose audience differ from the client_id
Jun 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 26 additions & 0 deletions pkg/cmd/cmd_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ func TestCmd_Run(t *testing.T) {
RedirectURLHostname: "localhost",
},
},
UseAccessToken: false,
},
},
"FullOptions": {
Expand Down Expand Up @@ -150,6 +151,30 @@ func TestCmd_Run(t *testing.T) {
RedirectURLHostname: "localhost",
},
},
UseAccessToken: false,
},
},
"AccessToken": {
args: []string{executable,
"get-token",
"--oidc-issuer-url", "https://issuer.example.com",
"--oidc-client-id", "YOUR_CLIENT_ID",
"--oidc-use-access-token=true",
},
in: credentialplugin.Input{
TokenCacheDir: filepath.Join(userHomeDir, ".kube/cache/oidc-login"),
Provider: oidc.Provider{
IssuerURL: "https://issuer.example.com",
ClientID: "YOUR_CLIENT_ID",
},
GrantOptionSet: authentication.GrantOptionSet{
AuthCodeBrowserOption: &authcode.BrowserOption{
BindAddress: defaultListenAddress,
AuthenticationTimeout: defaultAuthenticationTimeoutSec * time.Second,
RedirectURLHostname: "localhost",
},
},
UseAccessToken: true,
},
},
"HomedirExpansion": {
Expand Down Expand Up @@ -180,6 +205,7 @@ func TestCmd_Run(t *testing.T) {
TLSClientConfig: tlsclientconfig.Config{
CACertFilename: []string{filepath.Join(userHomeDir, ".kube/ca.crt")},
},
UseAccessToken: false,
},
},
}
Expand Down
3 changes: 3 additions & 0 deletions pkg/cmd/get_token.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ type getTokenOptions struct {
ClientSecret string
ExtraScopes []string
UsePKCE bool
UseAccessToken bool
TokenCacheDir string
tlsOptions tlsOptions
authenticationOptions authenticationOptions
Expand All @@ -30,6 +31,7 @@ func (o *getTokenOptions) addFlags(f *pflag.FlagSet) {
f.StringVar(&o.ClientSecret, "oidc-client-secret", "", "Client secret of the provider")
f.StringSliceVar(&o.ExtraScopes, "oidc-extra-scope", nil, "Scopes to request to the provider")
f.BoolVar(&o.UsePKCE, "oidc-use-pkce", false, "Force PKCE usage")
f.BoolVar(&o.UseAccessToken, "oidc-use-access-token", false, "Instead of using the id_token, use the access_token to authenticate to Kubernetes")
f.StringVar(&o.TokenCacheDir, "token-cache-dir", defaultTokenCacheDir, "Path to a directory for token cache")
f.BoolVar(&o.ForceRefresh, "force-refresh", false, "If set, refresh the ID token regardless of its expiration time")
o.tlsOptions.addFlags(f)
Expand Down Expand Up @@ -85,6 +87,7 @@ func (cmd *GetToken) New() *cobra.Command {
GrantOptionSet: grantOptionSet,
TLSClientConfig: o.tlsOptions.tlsClientConfig(),
ForceRefresh: o.ForceRefresh,
UseAccessToken: o.UseAccessToken,
}
if err := cmd.GetToken.Do(c.Context(), in); err != nil {
return fmt.Errorf("get-token: %w", err)
Expand Down
3 changes: 3 additions & 0 deletions pkg/cmd/setup.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ type setupOptions struct {
ClientSecret string
ExtraScopes []string
UsePKCE bool
UseAccessToken bool
tlsOptions tlsOptions
authenticationOptions authenticationOptions
}
Expand All @@ -25,6 +26,7 @@ func (o *setupOptions) addFlags(f *pflag.FlagSet) {
f.StringVar(&o.ClientSecret, "oidc-client-secret", "", "Client secret of the provider")
f.StringSliceVar(&o.ExtraScopes, "oidc-extra-scope", nil, "Scopes to request to the provider")
f.BoolVar(&o.UsePKCE, "oidc-use-pkce", false, "Force PKCE usage")
f.BoolVar(&o.UseAccessToken, "oidc-use-access-token", false, "Instead of using the id_token, use the access_token to authenticate to Kubernetes")
o.tlsOptions.addFlags(f)
o.authenticationOptions.addFlags(f)
}
Expand All @@ -50,6 +52,7 @@ func (cmd *Setup) New() *cobra.Command {
ClientSecret: o.ClientSecret,
ExtraScopes: o.ExtraScopes,
UsePKCE: o.UsePKCE,
UseAccessToken: o.UseAccessToken,
GrantOptionSet: grantOptionSet,
TLSClientConfig: o.tlsOptions.tlsClientConfig(),
}
Expand Down
27 changes: 27 additions & 0 deletions pkg/oidc/client/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ type client struct {
logger logger.Interface
supportedPKCEMethods []string
deviceAuthorizationEndpoint string
useAccessToken bool
}

func (c *client) wrapContext(ctx context.Context) context.Context {
Expand Down Expand Up @@ -205,6 +206,32 @@ func (c *client) verifyToken(ctx context.Context, token *oauth2.Token, nonce str
if nonce != "" && nonce != verifiedIDToken.Nonce {
return nil, fmt.Errorf("nonce did not match (wants %s but got %s)", nonce, verifiedIDToken.Nonce)
}

if c.useAccessToken {
accessToken, ok := token.Extra("access_token").(string)
if !ok {
return nil, fmt.Errorf("access_token is missing in the token response: %#v", accessToken)
}

// We intentionally do not perform a ClientID check here because there
// are some use cases in access_tokens where we *expect* the audience
// to differ. For example, one can explicitly set
// `audience=CLUSTER_CLIENT_ID` as an extra auth parameter.
verifier = c.provider.Verifier(&gooidc.Config{ClientID: "", Now: c.clock.Now, SkipClientIDCheck: true})

_, err := verifier.Verify(ctx, accessToken)
Copy link

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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.

Copy link

@jsphpl jsphpl Jun 1, 2024 •
edited

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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).

Copy link

Choose a reason for hiding this comment

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

Sounds good to me.

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

Very good point!

Copy link
Author

Choose a reason for hiding this comment

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

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

if err != nil {
return nil, fmt.Errorf("could not verify the access token: %w", err)
}

// There is no `nonce` to check on the `access_token`. We rely on the
// above `nonce` check on the `id_token`.

return &oidc.TokenSet{
IDToken: accessToken,
RefreshToken: token.RefreshToken,
}, nil
}
Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

return &oidc.TokenSet{
IDToken: idToken,
RefreshToken: token.RefreshToken,
Expand Down
5 changes: 3 additions & 2 deletions pkg/oidc/client/factory.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ var Set = wire.NewSet(
)

type FactoryInterface interface {
New(ctx context.Context, p oidc.Provider, tlsClientConfig tlsclientconfig.Config) (Interface, error)
New(ctx context.Context, p oidc.Provider, tlsClientConfig tlsclientconfig.Config, useAccessToken bool) (Interface, error)
}

type Factory struct {
Expand All @@ -34,7 +34,7 @@ type Factory struct {
}

// New returns an instance of infrastructure.Interface with the given configuration.
func (f *Factory) New(ctx context.Context, p oidc.Provider, tlsClientConfig tlsclientconfig.Config) (Interface, error) {
func (f *Factory) New(ctx context.Context, p oidc.Provider, tlsClientConfig tlsclientconfig.Config, useAccessToken bool) (Interface, error) {
rawTLSClientConfig, err := f.Loader.Load(tlsClientConfig)
if err != nil {
return nil, fmt.Errorf("could not load the TLS client config: %w", err)
Expand Down Expand Up @@ -80,6 +80,7 @@ func (f *Factory) New(ctx context.Context, p oidc.Provider, tlsClientConfig tlsc
logger: f.Logger,
supportedPKCEMethods: supportedPKCEMethods,
deviceAuthorizationEndpoint: deviceAuthorizationEndpoint,
useAccessToken: useAccessToken,
}, nil
}

Expand Down
15 changes: 8 additions & 7 deletions pkg/oidc/client/mock_FactoryInterface.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion pkg/usecases/authentication/authentication.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ type Input struct {
CachedTokenSet *oidc.TokenSet // optional
TLSClientConfig tlsclientconfig.Config
ForceRefresh bool
UseAccessToken bool
}

type GrantOptionSet struct {
Expand Down Expand Up @@ -98,7 +99,7 @@ func (u *Authentication) Do(ctx context.Context, in Input) (*Output, error) {
}

u.Logger.V(1).Infof("initializing an OpenID Connect client")
oidcClient, err := u.ClientFactory.New(ctx, in.Provider, in.TLSClientConfig)
oidcClient, err := u.ClientFactory.New(ctx, in.Provider, in.TLSClientConfig, in.UseAccessToken)
if err != nil {
return nil, fmt.Errorf("oidc error: %w", err)
}
Expand Down
6 changes: 3 additions & 3 deletions pkg/usecases/authentication/authentication_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ func TestAuthentication_Do(t *testing.T) {
}, nil)
mockClientFactory := client.NewMockFactoryInterface(t)
mockClientFactory.EXPECT().
New(ctx, dummyProvider, dummyTLSClientConfig).
New(ctx, dummyProvider, dummyTLSClientConfig, false).
Return(mockClient, nil)
u := Authentication{
ClientFactory: mockClientFactory,
Expand Down Expand Up @@ -143,7 +143,7 @@ func TestAuthentication_Do(t *testing.T) {
}, nil)
mockClientFactory := client.NewMockFactoryInterface(t)
mockClientFactory.EXPECT().
New(ctx, dummyProvider, dummyTLSClientConfig).
New(ctx, dummyProvider, dummyTLSClientConfig, false).
Return(mockClient, nil)
u := Authentication{
ClientFactory: mockClientFactory,
Expand Down Expand Up @@ -190,7 +190,7 @@ func TestAuthentication_Do(t *testing.T) {
}, nil)
mockClientFactory := client.NewMockFactoryInterface(t)
mockClientFactory.EXPECT().
New(ctx, dummyProvider, dummyTLSClientConfig).
New(ctx, dummyProvider, dummyTLSClientConfig, false).
Return(mockClient, nil)
u := Authentication{
ClientFactory: mockClientFactory,
Expand Down
2 changes: 2 additions & 0 deletions pkg/usecases/credentialplugin/get_token.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ type Input struct {
GrantOptionSet authentication.GrantOptionSet
TLSClientConfig tlsclientconfig.Config
ForceRefresh bool
UseAccessToken bool
}

type GetToken struct {
Expand Down Expand Up @@ -92,6 +93,7 @@ func (u *GetToken) Do(ctx context.Context, in Input) error {
CachedTokenSet: cachedTokenSet,
TLSClientConfig: in.TLSClientConfig,
ForceRefresh: in.ForceRefresh,
UseAccessToken: in.UseAccessToken,
}
authenticationOutput, err := u.Authentication.Do(ctx, authenticationInput)
if err != nil {
Expand Down
5 changes: 5 additions & 0 deletions pkg/usecases/setup/stage2.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ type Stage2Input struct {
ClientSecret string
ExtraScopes []string // optional
UsePKCE bool // optional
UseAccessToken bool // optional
ListenAddressArgs []string // non-nil if set by the command arg
GrantOptionSet authentication.GrantOptionSet
TLSClientConfig tlsclientconfig.Config
Expand All @@ -91,6 +92,7 @@ func (u *Setup) DoStage2(ctx context.Context, in Stage2Input) error {
},
GrantOptionSet: in.GrantOptionSet,
TLSClientConfig: in.TLSClientConfig,
UseAccessToken: in.UseAccessToken,
})
if err != nil {
return fmt.Errorf("authentication error: %w", err)
Expand Down Expand Up @@ -128,6 +130,9 @@ func makeCredentialPluginArgs(in Stage2Input) []string {
if in.UsePKCE {
args = append(args, "--oidc-use-pkce")
}
if in.UseAccessToken {
args = append(args, "--oidc-use-access-token")
}
for _, f := range in.TLSClientConfig.CACertFilename {
args = append(args, "--certificate-authority="+f)
}
Expand Down