From db03e78dc4b83a66f0f64980e626051018c91452 Mon Sep 17 00:00:00 2001 From: Adam kafka Date: Thu, 25 Apr 2024 15:37:00 -0700 Subject: [PATCH 1/4] Add new `--oidc-use-access-token` flag to `get-token` Implements https://github.com/int128/kubelogin/issues/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. --- pkg/cmd/get_token.go | 3 +++ pkg/cmd/setup.go | 3 +++ pkg/oidc/client/client.go | 20 +++++++++++++++++++ pkg/oidc/client/factory.go | 5 +++-- pkg/usecases/authentication/authentication.go | 3 ++- pkg/usecases/credentialplugin/get_token.go | 2 ++ pkg/usecases/setup/stage2.go | 5 +++++ 7 files changed, 38 insertions(+), 3 deletions(-) diff --git a/pkg/cmd/get_token.go b/pkg/cmd/get_token.go index bff4bc07..3e7c9f67 100644 --- a/pkg/cmd/get_token.go +++ b/pkg/cmd/get_token.go @@ -18,6 +18,7 @@ type getTokenOptions struct { ClientSecret string ExtraScopes []string UsePKCE bool + UseAccessToken bool TokenCacheDir string tlsOptions tlsOptions authenticationOptions authenticationOptions @@ -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) @@ -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) diff --git a/pkg/cmd/setup.go b/pkg/cmd/setup.go index 4e90c2d2..0f6f813d 100644 --- a/pkg/cmd/setup.go +++ b/pkg/cmd/setup.go @@ -15,6 +15,7 @@ type setupOptions struct { ClientSecret string ExtraScopes []string UsePKCE bool + UseAccessToken bool tlsOptions tlsOptions authenticationOptions authenticationOptions } @@ -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) } @@ -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(), } diff --git a/pkg/oidc/client/client.go b/pkg/oidc/client/client.go index 268b63ee..4505c1fb 100644 --- a/pkg/oidc/client/client.go +++ b/pkg/oidc/client/client.go @@ -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 { @@ -205,6 +206,25 @@ 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) + } + _, err := verifier.Verify(ctx, accessToken) + 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 + } return &oidc.TokenSet{ IDToken: idToken, RefreshToken: token.RefreshToken, diff --git a/pkg/oidc/client/factory.go b/pkg/oidc/client/factory.go index 4a9ee332..5c9fc610 100644 --- a/pkg/oidc/client/factory.go +++ b/pkg/oidc/client/factory.go @@ -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 { @@ -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) @@ -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 } diff --git a/pkg/usecases/authentication/authentication.go b/pkg/usecases/authentication/authentication.go index 5fa44b72..060de4e3 100644 --- a/pkg/usecases/authentication/authentication.go +++ b/pkg/usecases/authentication/authentication.go @@ -36,6 +36,7 @@ type Input struct { CachedTokenSet *oidc.TokenSet // optional TLSClientConfig tlsclientconfig.Config ForceRefresh bool + UseAccessToken bool } type GrantOptionSet struct { @@ -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) } diff --git a/pkg/usecases/credentialplugin/get_token.go b/pkg/usecases/credentialplugin/get_token.go index 7db246db..979238b0 100644 --- a/pkg/usecases/credentialplugin/get_token.go +++ b/pkg/usecases/credentialplugin/get_token.go @@ -38,6 +38,7 @@ type Input struct { GrantOptionSet authentication.GrantOptionSet TLSClientConfig tlsclientconfig.Config ForceRefresh bool + UseAccessToken bool } type GetToken struct { @@ -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 { diff --git a/pkg/usecases/setup/stage2.go b/pkg/usecases/setup/stage2.go index ced1b648..ae9bf2eb 100644 --- a/pkg/usecases/setup/stage2.go +++ b/pkg/usecases/setup/stage2.go @@ -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 @@ -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) @@ -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) } From 859d51b40ba3a9e006f1d2e309ed51183488d74f Mon Sep 17 00:00:00 2001 From: Adam kafka Date: Tue, 21 May 2024 13:47:59 -0700 Subject: [PATCH 2/4] Fix failing tests Needed to plumb through our new parameter `UseAccessToken` to the mocks as well. --- pkg/oidc/client/mock_FactoryInterface.go | 15 ++++++++------- .../authentication/authentication_test.go | 6 +++--- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/pkg/oidc/client/mock_FactoryInterface.go b/pkg/oidc/client/mock_FactoryInterface.go index 71a44861..cd815fb4 100644 --- a/pkg/oidc/client/mock_FactoryInterface.go +++ b/pkg/oidc/client/mock_FactoryInterface.go @@ -24,13 +24,13 @@ func (_m *MockFactoryInterface) EXPECT() *MockFactoryInterface_Expecter { return &MockFactoryInterface_Expecter{mock: &_m.Mock} } -// 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) { ret := _m.Called(ctx, p, tlsClientConfig) var r0 Interface - if rf, ok := ret.Get(0).(func(context.Context, oidc.Provider, tlsclientconfig.Config) Interface); ok { - r0 = rf(ctx, p, tlsClientConfig) + if rf, ok := ret.Get(0).(func(context.Context, oidc.Provider, tlsclientconfig.Config, bool) Interface); ok { + r0 = rf(ctx, p, tlsClientConfig, useAccessToken) } else { if ret.Get(0) != nil { r0 = ret.Get(0).(Interface) @@ -38,8 +38,8 @@ func (_m *MockFactoryInterface) New(ctx context.Context, p oidc.Provider, tlsCli } var r1 error - if rf, ok := ret.Get(1).(func(context.Context, oidc.Provider, tlsclientconfig.Config) error); ok { - r1 = rf(ctx, p, tlsClientConfig) + if rf, ok := ret.Get(1).(func(context.Context, oidc.Provider, tlsclientconfig.Config, bool) error); ok { + r1 = rf(ctx, p, tlsClientConfig, useAccessToken) } else { r1 = ret.Error(1) } @@ -56,7 +56,8 @@ type MockFactoryInterface_New_Call struct { // - ctx context.Context // - p oidc.Provider // - tlsClientConfig tlsclientconfig.Config -func (_e *MockFactoryInterface_Expecter) New(ctx interface{}, p interface{}, tlsClientConfig interface{}) *MockFactoryInterface_New_Call { +// - useAccessToken bool +func (_e *MockFactoryInterface_Expecter) New(ctx interface{}, p interface{}, tlsClientConfig interface{}, useAccessToken bool) *MockFactoryInterface_New_Call { return &MockFactoryInterface_New_Call{Call: _e.mock.On("New", ctx, p, tlsClientConfig)} } diff --git a/pkg/usecases/authentication/authentication_test.go b/pkg/usecases/authentication/authentication_test.go index 63faba0c..8ba2c910 100644 --- a/pkg/usecases/authentication/authentication_test.go +++ b/pkg/usecases/authentication/authentication_test.go @@ -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, @@ -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, @@ -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, From f9c3c8aee74ff81784c5d5e1b8c2cbcc76aff8a5 Mon Sep 17 00:00:00 2001 From: Adam kafka Date: Tue, 21 May 2024 15:07:07 -0700 Subject: [PATCH 3/4] Add a test to make sure new flag is plumbed through --- pkg/cmd/cmd_test.go | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/pkg/cmd/cmd_test.go b/pkg/cmd/cmd_test.go index 8e9f9ca1..6c9c4be7 100644 --- a/pkg/cmd/cmd_test.go +++ b/pkg/cmd/cmd_test.go @@ -123,6 +123,7 @@ func TestCmd_Run(t *testing.T) { RedirectURLHostname: "localhost", }, }, + UseAccessToken: false, }, }, "FullOptions": { @@ -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": { @@ -180,6 +205,7 @@ func TestCmd_Run(t *testing.T) { TLSClientConfig: tlsclientconfig.Config{ CACertFilename: []string{filepath.Join(userHomeDir, ".kube/ca.crt")}, }, + UseAccessToken: false, }, }, } From 1add63b506925f34181007c871086e86e7954b6a Mon Sep 17 00:00:00 2001 From: Adam kafka Date: Mon, 17 Jun 2024 16:05:55 -0700 Subject: [PATCH 4/4] Support Access Tokens whose audience differ from the client_id 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. --- pkg/oidc/client/client.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/pkg/oidc/client/client.go b/pkg/oidc/client/client.go index 4505c1fb..5da05759 100644 --- a/pkg/oidc/client/client.go +++ b/pkg/oidc/client/client.go @@ -212,6 +212,13 @@ func (c *client) verifyToken(ctx context.Context, token *oauth2.Token, nonce str 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) if err != nil { return nil, fmt.Errorf("could not verify the access token: %w", err)