diff --git a/cmd/pinniped/cmd/kubeconfig.go b/cmd/pinniped/cmd/kubeconfig.go index bceccafa61..daa752af99 100644 --- a/cmd/pinniped/cmd/kubeconfig.go +++ b/cmd/pinniped/cmd/kubeconfig.go @@ -63,8 +63,9 @@ type getKubeconfigOIDCParams struct { clientID string listenPort uint16 scopes []string - skipBrowser bool - skipListen bool + skipBrowser bool + skipListen bool + skipRequireIDTokenOnRefresh bool sessionCachePath string debugSessionCache bool caBundle caBundleFlag @@ -142,6 +143,7 @@ func kubeconfigCommand(deps kubeconfigDeps) *cobra.Command { f.StringSliceVar(&flags.oidc.scopes, "oidc-scopes", []string{oidcapi.ScopeOfflineAccess, oidcapi.ScopeOpenID, oidcapi.ScopeRequestAudience, oidcapi.ScopeUsername, oidcapi.ScopeGroups}, "OpenID Connect scopes to request during login") f.BoolVar(&flags.oidc.skipBrowser, "oidc-skip-browser", false, "During OpenID Connect login, skip opening the browser (just print the URL)") f.BoolVar(&flags.oidc.skipListen, "oidc-skip-listen", false, "During OpenID Connect login, skip starting a localhost callback listener (manual copy/paste flow only)") + f.BoolVar(&flags.oidc.skipRequireIDTokenOnRefresh, "oidc-skip-require-id-token-on-refresh", false, "During OpenID Connect login, skip requiring an ID token in the refresh response (use for OIDC providers that omit id_token on refresh)") f.StringVar(&flags.oidc.sessionCachePath, "oidc-session-cache", "", "Path to OpenID Connect session cache file") f.Var(&flags.oidc.caBundle, "oidc-ca-bundle", "Path to TLS certificate authority bundle (PEM format, optional, can be repeated)") f.BoolVar(&flags.oidc.debugSessionCache, "oidc-debug-session-cache", false, "Print debug logs related to the OpenID Connect session cache") @@ -352,6 +354,9 @@ func newExecConfig(deps kubeconfigDeps, flags getKubeconfigParams) (*clientcmdap if flags.oidc.skipListen { execConfig.Args = append(execConfig.Args, "--skip-listen") } + if flags.oidc.skipRequireIDTokenOnRefresh { + execConfig.Args = append(execConfig.Args, "--skip-require-id-token-on-refresh") + } if flags.oidc.listenPort != 0 { execConfig.Args = append(execConfig.Args, "--listen-port="+strconv.Itoa(int(flags.oidc.listenPort))) } diff --git a/cmd/pinniped/cmd/kubeconfig_test.go b/cmd/pinniped/cmd/kubeconfig_test.go index fb37a2a052..245192fa92 100644 --- a/cmd/pinniped/cmd/kubeconfig_test.go +++ b/cmd/pinniped/cmd/kubeconfig_test.go @@ -187,6 +187,7 @@ func TestGetKubeconfig(t *testing.T) { --oidc-scopes strings OpenID Connect scopes to request during login (default [offline_access,openid,pinniped:request-audience,username,groups]) --oidc-session-cache string Path to OpenID Connect session cache file --oidc-skip-browser During OpenID Connect login, skip opening the browser (just print the URL) + --oidc-skip-require-id-token-on-refresh During OpenID Connect login, skip requiring an ID token in the refresh response (use for OIDC providers that omit id_token on refresh) -o, --output string Output file path (default: stdout) --pinniped-cli-path string Full path or executable name for the Pinniped CLI binary to be embedded in the resulting kubeconfig output (e.g. 'pinniped') (default: full path of the binary used to execute this command) --skip-validation Skip final validation of the kubeconfig (default: false) diff --git a/cmd/pinniped/cmd/login_oidc.go b/cmd/pinniped/cmd/login_oidc.go index 5adabe2ace..d48155883a 100644 --- a/cmd/pinniped/cmd/login_oidc.go +++ b/cmd/pinniped/cmd/login_oidc.go @@ -97,6 +97,7 @@ type oidcLoginFlags struct { upstreamIdentityProviderName string upstreamIdentityProviderType string upstreamIdentityProviderFlow string + skipRequireIDTokenOnRefresh bool } func oidcLoginCommand(deps oidcLoginCommandDeps) *cobra.Command { @@ -152,6 +153,7 @@ func oidcLoginCommand(deps oidcLoginCommandDeps) *cobra.Command { idpdiscoveryv1alpha1.IDPTypeGitHub, )) cmd.Flags().StringVar(&flags.upstreamIdentityProviderFlow, "upstream-identity-provider-flow", "", fmt.Sprintf("The type of client flow to use with the upstream identity provider during login with a Supervisor (e.g. '%s', '%s')", idpdiscoveryv1alpha1.IDPFlowBrowserAuthcode, idpdiscoveryv1alpha1.IDPFlowCLIPassword)) + cmd.Flags().BoolVar(&flags.skipRequireIDTokenOnRefresh, "skip-require-id-token-on-refresh", false, "Skip requiring an ID token in the refresh response (use for OIDC providers that omit id_token on refresh)") // --skip-listen is mainly needed for testing. We'll leave it hidden until we have a non-testing use case. mustMarkHidden(cmd, "skip-listen") @@ -241,6 +243,10 @@ func runOIDCLogin(cmd *cobra.Command, deps oidcLoginCommandDeps, flags oidcLogin opts = append(opts, deps.optionsFactory.WithSkipListen()) } + if flags.skipRequireIDTokenOnRefresh { + opts = append(opts, deps.optionsFactory.WithRequireIDTokenOnRefresh(false)) + } + if len(flags.caBundlePaths) > 0 || len(flags.caBundleData) > 0 { client, err := makeClient(flags.caBundlePaths, flags.caBundleData) if err != nil { @@ -271,7 +277,7 @@ func runOIDCLogin(cmd *cobra.Command, deps oidcLoginCommandDeps, flags oidcLogin if err != nil { return fmt.Errorf("could not complete Pinniped login: %w", err) } - cred := tokenCredential(token.IDToken) + cred := tokenCredential(token) // If the concierge was configured, exchange the credential for a separate short-lived, cluster-specific credential. if concierge != nil { @@ -279,6 +285,9 @@ func runOIDCLogin(cmd *cobra.Command, deps oidcLoginCommandDeps, flags oidcLogin ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) defer cancel() + if token.IDToken == nil { + return fmt.Errorf("the OIDC provider did not return an ID token during login; cannot exchange token with Concierge (--skip-require-id-token-on-refresh is not compatible with --enable-concierge)") + } cred, err = deps.exchangeToken(ctx, concierge, token.IDToken.Token) if err != nil { return fmt.Errorf("could not complete Concierge credential exchange: %w", err) @@ -315,22 +324,30 @@ func makeClient(caBundlePaths []string, caBundleData []string) (*http.Client, er return phttp.Default(pool), nil } -func tokenCredential(idToken *oidctypes.IDToken) *clientauthv1beta1.ExecCredential { +func tokenCredential(token *oidctypes.Token) *clientauthv1beta1.ExecCredential { cred := clientauthv1beta1.ExecCredential{ TypeMeta: metav1.TypeMeta{ Kind: "ExecCredential", APIVersion: "client.authentication.k8s.io/v1beta1", }, - Status: &clientauthv1beta1.ExecCredentialStatus{ - Token: idToken.Token, - }, + Status: &clientauthv1beta1.ExecCredentialStatus{}, } - if !idToken.Expiry.IsZero() { - cred.Status.ExpirationTimestamp = &idToken.Expiry + switch { + case token.IDToken != nil: + applyTokenToStatus(cred.Status, token.IDToken.Token, token.IDToken.Expiry) + case token.AccessToken != nil: + applyTokenToStatus(cred.Status, token.AccessToken.Token, token.AccessToken.Expiry) } return &cred } +func applyTokenToStatus(s *clientauthv1beta1.ExecCredentialStatus, tok string, expiry metav1.Time) { + s.Token = tok + if !expiry.IsZero() { + s.ExpirationTimestamp = &expiry + } +} + func SetLogLevel(ctx context.Context, lookupEnv func(string) (string, bool)) (plog.Logger, error) { debug, _ := lookupEnv(debugEnvVarName) if debug == envVarTruthyValue { diff --git a/cmd/pinniped/cmd/login_oidc_test.go b/cmd/pinniped/cmd/login_oidc_test.go index 10a87fddd8..f78d2da709 100644 --- a/cmd/pinniped/cmd/login_oidc_test.go +++ b/cmd/pinniped/cmd/login_oidc_test.go @@ -101,6 +101,7 @@ func TestLoginOIDCCommand(t *testing.T) { --scopes strings OIDC scopes to request during login (default [offline_access,openid,pinniped:request-audience,username,groups]) --session-cache string Path to session cache file (default "` + cfgDir + `/sessions.yaml") --skip-browser Skip opening the browser (just print the URL) + --skip-require-id-token-on-refresh Skip requiring an ID token in the refresh response (use for OIDC providers that omit id_token on refresh) --upstream-identity-provider-flow string The type of client flow to use with the upstream identity provider during login with a Supervisor (e.g. 'browser_authcode', 'cli_password') --upstream-identity-provider-name string The name of the upstream identity provider used during login with a Supervisor --upstream-identity-provider-type string The type of the upstream identity provider used during login with a Supervisor (e.g. 'oidc', 'ldap', 'activedirectory', 'github') (default "oidc") @@ -274,8 +275,8 @@ func TestLoginOIDCCommand(t *testing.T) { wantOptionsCount: 4, wantStdout: `{"kind":"ExecCredential","apiVersion":"client.authentication.k8s.io/v1beta1","spec":{"interactive":false},"status":{"expirationTimestamp":"3020-10-12T13:14:15Z","token":"test-id-token"}}` + "\n", wantLogs: []string{ - nowStr + ` cmd/login_oidc.go:268 Performing OIDC login {"issuer": "test-issuer", "client id": "test-client-id"}`, - nowStr + ` cmd/login_oidc.go:288 No concierge configured, skipping token credential exchange`, + nowStr + ` cmd/login_oidc.go:274 Performing OIDC login {"issuer": "test-issuer", "client id": "test-client-id"}`, + nowStr + ` cmd/login_oidc.go:297 No concierge configured, skipping token credential exchange`, }, }, { @@ -319,10 +320,10 @@ func TestLoginOIDCCommand(t *testing.T) { wantOptionsCount: 12, wantStdout: `{"kind":"ExecCredential","apiVersion":"client.authentication.k8s.io/v1beta1","spec":{"interactive":false},"status":{"token":"exchanged-token"}}` + "\n", wantLogs: []string{ - nowStr + ` cmd/login_oidc.go:268 Performing OIDC login {"issuer": "test-issuer", "client id": "test-client-id"}`, - nowStr + ` cmd/login_oidc.go:278 Exchanging token for cluster credential {"endpoint": "https://127.0.0.1:1234/", "authenticator type": "webhook", "authenticator name": "test-authenticator"}`, - nowStr + ` cmd/login_oidc.go:286 Successfully exchanged token for cluster credential.`, - nowStr + ` cmd/login_oidc.go:293 caching cluster credential for future use.`, + nowStr + ` cmd/login_oidc.go:274 Performing OIDC login {"issuer": "test-issuer", "client id": "test-client-id"}`, + nowStr + ` cmd/login_oidc.go:284 Exchanging token for cluster credential {"endpoint": "https://127.0.0.1:1234/", "authenticator type": "webhook", "authenticator name": "test-authenticator"}`, + nowStr + ` cmd/login_oidc.go:295 Successfully exchanged token for cluster credential.`, + nowStr + ` cmd/login_oidc.go:302 caching cluster credential for future use.`, }, }, } @@ -395,3 +396,37 @@ func TestLoginOIDCCommand(t *testing.T) { }) } } + +func TestTokenCredential(t *testing.T) { + expiry := metav1.NewTime(time.Date(3020, 10, 12, 13, 14, 15, 0, time.UTC)) + + t.Run("IDToken present uses IDToken", func(t *testing.T) { + token := &oidctypes.Token{ + IDToken: &oidctypes.IDToken{Token: "id-tok", Expiry: expiry}, + AccessToken: &oidctypes.AccessToken{Token: "access-tok", Expiry: expiry}, + } + cred := tokenCredential(token) + require.Equal(t, "id-tok", cred.Status.Token) + require.Equal(t, &expiry, cred.Status.ExpirationTimestamp) + }) + + t.Run("IDToken nil falls back to AccessToken", func(t *testing.T) { + token := &oidctypes.Token{ + IDToken: nil, + AccessToken: &oidctypes.AccessToken{Token: "access-tok", Expiry: expiry}, + } + cred := tokenCredential(token) + require.Equal(t, "access-tok", cred.Status.Token) + require.Equal(t, &expiry, cred.Status.ExpirationTimestamp) + }) + + t.Run("IDToken nil and AccessToken with zero expiry omits ExpirationTimestamp", func(t *testing.T) { + token := &oidctypes.Token{ + IDToken: nil, + AccessToken: &oidctypes.AccessToken{Token: "access-tok"}, + } + cred := tokenCredential(token) + require.Equal(t, "access-tok", cred.Status.Token) + require.Nil(t, cred.Status.ExpirationTimestamp) + }) +} diff --git a/cmd/pinniped/cmd/login_static.go b/cmd/pinniped/cmd/login_static.go index cb5b2267bc..4c0e2daece 100644 --- a/cmd/pinniped/cmd/login_static.go +++ b/cmd/pinniped/cmd/login_static.go @@ -134,7 +134,7 @@ func runStaticLogin(cmd *cobra.Command, deps staticLoginDeps, flags staticLoginP return fmt.Errorf("--token-env variable %q is empty", flags.staticTokenEnvName) } } - cred := tokenCredential(&oidctypes.IDToken{Token: token}) + cred := tokenCredential(&oidctypes.Token{IDToken: &oidctypes.IDToken{Token: token}}) // Look up cached credentials based on a hash of all the CLI arguments, the current token value, and the cluster info. cacheKey := struct { diff --git a/cmd/pinniped/cmd/oidc_client_options.go b/cmd/pinniped/cmd/oidc_client_options.go index bcb5d8d968..25a425d879 100644 --- a/cmd/pinniped/cmd/oidc_client_options.go +++ b/cmd/pinniped/cmd/oidc_client_options.go @@ -28,6 +28,7 @@ type OIDCClientOptions interface { WithRequestAudience(audience string) oidcclient.Option WithLoginFlow(loginFlow v1alpha1.IDPFlow, flowSource string) oidcclient.Option WithUpstreamIdentityProvider(upstreamName, upstreamType string) oidcclient.Option + WithRequireIDTokenOnRefresh(require bool) oidcclient.Option } // clientOptions implements OIDCClientOptions for production use. @@ -82,3 +83,7 @@ func (o *clientOptions) WithLoginFlow(loginFlow v1alpha1.IDPFlow, flowSource str func (o *clientOptions) WithUpstreamIdentityProvider(upstreamName, upstreamType string) oidcclient.Option { return oidcclient.WithUpstreamIdentityProvider(upstreamName, upstreamType) } + +func (o *clientOptions) WithRequireIDTokenOnRefresh(require bool) oidcclient.Option { + return oidcclient.WithRequireIDTokenOnRefresh(require) +} diff --git a/internal/mocks/mockoidcclientoptions/mockoidcclientoptions.go b/internal/mocks/mockoidcclientoptions/mockoidcclientoptions.go index ce44386649..2c1d26f53b 100644 --- a/internal/mocks/mockoidcclientoptions/mockoidcclientoptions.go +++ b/internal/mocks/mockoidcclientoptions/mockoidcclientoptions.go @@ -214,3 +214,17 @@ func (mr *MockOIDCClientOptionsMockRecorder) WithUpstreamIdentityProvider(upstre mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "WithUpstreamIdentityProvider", reflect.TypeOf((*MockOIDCClientOptions)(nil).WithUpstreamIdentityProvider), upstreamName, upstreamType) } + +// WithRequireIDTokenOnRefresh mocks base method. +func (m *MockOIDCClientOptions) WithRequireIDTokenOnRefresh(require bool) oidcclient.Option { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "WithRequireIDTokenOnRefresh", require) + ret0, _ := ret[0].(oidcclient.Option) + return ret0 +} + +// WithRequireIDTokenOnRefresh indicates an expected call of WithRequireIDTokenOnRefresh. +func (mr *MockOIDCClientOptionsMockRecorder) WithRequireIDTokenOnRefresh(require any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "WithRequireIDTokenOnRefresh", reflect.TypeOf((*MockOIDCClientOptions)(nil).WithRequireIDTokenOnRefresh), require) +} diff --git a/pkg/oidcclient/login.go b/pkg/oidcclient/login.go index 732ea3e529..01cc17b606 100644 --- a/pkg/oidcclient/login.go +++ b/pkg/oidcclient/login.go @@ -102,6 +102,7 @@ type handlerState struct { skipPrintLoginURL bool requestedAudience string httpClient *http.Client + requireIDTokenOnRefresh bool // Parameters of the localhost listener. listenAddr string @@ -291,6 +292,16 @@ func WithRequestAudience(audience string) Option { } } +// WithRequireIDTokenOnRefresh controls whether the refresh token flow requires the provider to return an id_token. +// Set to false for providers that omit id_token in their refresh response. +// Defaults to true for backward compatibility. +func WithRequireIDTokenOnRefresh(require bool) Option { + return func(h *handlerState) error { + h.requireIDTokenOnRefresh = require + return nil + } +} + // WithCLISendingCredentials causes the login flow to use CLI-based prompts for username and password and causes the // call to the Issuer's authorize endpoint to be made directly (no web browser) with the username and password on custom // HTTP headers. This is only intended to be used when the issuer is a Pinniped Supervisor and the upstream identity @@ -421,7 +432,8 @@ func Login(issuer string, clientID string, opts ...Option) (*oidctypes.Token, er ctx: context.Background(), logger: &emptyLogger{}, callbacks: make(chan callbackResult, 2), - httpClient: phttp.Default(nil), + httpClient: phttp.Default(nil), + requireIDTokenOnRefresh: true, // Default implementations of external dependencies (to be mocked in tests). generateState: state.Generate, @@ -1224,7 +1236,7 @@ func (h *handlerState) handleRefresh(ctx context.Context, refreshToken *oidctype // The spec is not 100% clear about whether an ID token from the refresh flow should include a nonce, and at least // some providers do not include one, so we skip the nonce validation here (but not other validations). - return upstreamOIDCIdentityProvider.ValidateTokenAndMergeWithUserInfo(ctx, refreshed, "", true, false) + return upstreamOIDCIdentityProvider.ValidateTokenAndMergeWithUserInfo(ctx, refreshed, "", h.requireIDTokenOnRefresh, false) } // handleAuthCodeCallback is used as an http handler, so it does not run in the CLI's main goroutine. diff --git a/pkg/oidcclient/login_test.go b/pkg/oidcclient/login_test.go index 6aee17244e..9140233053 100644 --- a/pkg/oidcclient/login_test.go +++ b/pkg/oidcclient/login_test.go @@ -4065,3 +4065,75 @@ func TestMaybePrintAuditID(t *testing.T) { }) } } + +func TestHandleRefreshWithRequireIDTokenOnRefresh(t *testing.T) { + accessTokenOnly := &oauth2.Token{ + AccessToken: "access-tok", + RefreshToken: "new-refresh-tok", + TokenType: "Bearer", + } + + t.Run("flag enabled, provider omits id_token, refresh succeeds with access token", func(t *testing.T) { + mock := mockUpstream(t) + mock.EXPECT(). + PerformRefresh(gomock.Any(), "old-refresh-tok"). + Return(accessTokenOnly, nil) + mock.EXPECT(). + ValidateTokenAndMergeWithUserInfo(gomock.Any(), accessTokenOnly, nonce.Nonce(""), false, false). + Return(&oidctypes.Token{ + AccessToken: &oidctypes.AccessToken{Token: "access-tok"}, + }, nil) + + h := &handlerState{ + requireIDTokenOnRefresh: false, + logger: &emptyLogger{}, + getProvider: func(_ *oauth2.Config, _ *coreosoidc.Provider, _ *http.Client) upstreamprovider.UpstreamOIDCIdentityProviderI { + return mock + }, + } + token, err := h.handleRefresh(context.Background(), &oidctypes.RefreshToken{Token: "old-refresh-tok"}) + require.NoError(t, err) + require.NotNil(t, token) + require.Nil(t, token.IDToken) + require.Equal(t, "access-tok", token.AccessToken.Token) + }) + + t.Run("flag disabled (default), ValidateTokenAndMergeWithUserInfo called with requireIDToken=true", func(t *testing.T) { + mock := mockUpstream(t) + mock.EXPECT(). + PerformRefresh(gomock.Any(), "old-refresh-tok"). + Return(accessTokenOnly, nil) + mock.EXPECT(). + ValidateTokenAndMergeWithUserInfo(gomock.Any(), accessTokenOnly, nonce.Nonce(""), true, false). + Return(nil, nil) // simulate provider returning nil (triggers browser login) + + h := &handlerState{ + requireIDTokenOnRefresh: true, + logger: &emptyLogger{}, + getProvider: func(_ *oauth2.Config, _ *coreosoidc.Provider, _ *http.Client) upstreamprovider.UpstreamOIDCIdentityProviderI { + return mock + }, + } + token, err := h.handleRefresh(context.Background(), &oidctypes.RefreshToken{Token: "old-refresh-tok"}) + require.NoError(t, err) + require.Nil(t, token) + }) + + t.Run("flag enabled, PerformRefresh itself fails, returns nil to trigger browser login", func(t *testing.T) { + mock := mockUpstream(t) + mock.EXPECT(). + PerformRefresh(gomock.Any(), "old-refresh-tok"). + Return(nil, fmt.Errorf("network error")) + + h := &handlerState{ + requireIDTokenOnRefresh: false, + logger: &emptyLogger{}, + getProvider: func(_ *oauth2.Config, _ *coreosoidc.Provider, _ *http.Client) upstreamprovider.UpstreamOIDCIdentityProviderI { + return mock + }, + } + token, err := h.handleRefresh(context.Background(), &oidctypes.RefreshToken{Token: "old-refresh-tok"}) + require.NoError(t, err) + require.Nil(t, token) + }) +}