From a70ce9cef72230e164a209ae821bbc1f2a256a0c Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Thu, 14 Mar 2024 11:25:25 -0700 Subject: [PATCH] Add PINNIPED_SKIP_PRINT_LOGIN_URL env var to CLI --- cmd/pinniped/cmd/login_oidc.go | 21 ++++++++++++++++-- cmd/pinniped/cmd/login_oidc_test.go | 30 +++++++++++++++++-------- pkg/oidcclient/login.go | 34 +++++++++++++---------------- pkg/oidcclient/login_test.go | 21 +++++------------- 4 files changed, 61 insertions(+), 45 deletions(-) diff --git a/cmd/pinniped/cmd/login_oidc.go b/cmd/pinniped/cmd/login_oidc.go index de46034c5..5bfc50db2 100644 --- a/cmd/pinniped/cmd/login_oidc.go +++ b/cmd/pinniped/cmd/login_oidc.go @@ -39,6 +39,18 @@ const ( // which specifies "cli_password" when using an IDE plugin where there is no interactive CLI available. This allows // the user to use one kubeconfig file for both flows. upstreamIdentityProviderFlowEnvVarName = "PINNIPED_UPSTREAM_IDENTITY_PROVIDER_FLOW" + + // When using a browser-based login flow, the user may skip printing the login URL to the screen in the case + // where the browser was launched with the login URL. This can be useful, for example, when using a console-based + // UI like k9s, to avoid having any output to stderr which may confuse the UI. Set this env var to "true" to + // skip printing the URL. + skipPrintLoginURLEnvVarName = "PINNIPED_SKIP_PRINT_LOGIN_URL" + + // Set this env var to "true" to cause debug logs to be printed to stderr. + debugEnvVarName = "PINNIPED_DEBUG" + + // The value to use for true/false env vars to enable the behavior caused by the env var. + envVarTruthyValue = "true" ) //nolint:gochecknoinits @@ -169,6 +181,11 @@ func runOIDCLogin(cmd *cobra.Command, deps oidcLoginCommandDeps, flags oidcLogin oidcclient.WithSessionCache(sessionCache), } + skipPrintLoginURL, _ := deps.lookupEnv(skipPrintLoginURLEnvVarName) + if skipPrintLoginURL == envVarTruthyValue { + opts = append(opts, oidcclient.WithSkipPrintLoginURL()) + } + if flags.listenPort != 0 { opts = append(opts, oidcclient.WithListenPort(flags.listenPort)) } @@ -361,8 +378,8 @@ func tokenCredential(idToken *oidctypes.IDToken) *clientauthv1beta1.ExecCredenti } func SetLogLevel(ctx context.Context, lookupEnv func(string) (string, bool)) (plog.Logger, error) { - debug, _ := lookupEnv("PINNIPED_DEBUG") - if debug == "true" { + debug, _ := lookupEnv(debugEnvVarName) + if debug == envVarTruthyValue { err := plog.ValidateAndSetLogLevelAndFormatGlobally(ctx, plog.LogSpec{Level: plog.LevelDebug, Format: plog.FormatCLI}) if err != nil { return nil, err diff --git a/cmd/pinniped/cmd/login_oidc_test.go b/cmd/pinniped/cmd/login_oidc_test.go index ff7409e54..f023e0757 100644 --- a/cmd/pinniped/cmd/login_oidc_test.go +++ b/cmd/pinniped/cmd/login_oidc_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package cmd @@ -187,6 +187,18 @@ 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", }, + { + name: "PINNIPED_SKIP_PRINT_LOGIN_URL adds an option", + args: []string{ + "--issuer", "test-issuer", + "--client-id", "test-client-id", + "--upstream-identity-provider-type", "oidc", + "--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution + }, + env: map[string]string{"PINNIPED_SKIP_PRINT_LOGIN_URL": "true"}, + wantOptionsCount: 5, + wantStdout: `{"kind":"ExecCredential","apiVersion":"client.authentication.k8s.io/v1beta1","spec":{"interactive":false},"status":{"expirationTimestamp":"3020-10-12T13:14:15Z","token":"test-id-token"}}` + "\n", + }, { name: "oidc upstream type with CLI flow is allowed", args: []string{ @@ -489,8 +501,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 + ` pinniped-login cmd/login_oidc.go:243 Performing OIDC login {"issuer": "test-issuer", "client id": "test-client-id"}`, - nowStr + ` pinniped-login cmd/login_oidc.go:263 No concierge configured, skipping token credential exchange`, + nowStr + ` pinniped-login cmd/login_oidc.go:260 Performing OIDC login {"issuer": "test-issuer", "client id": "test-client-id"}`, + nowStr + ` pinniped-login cmd/login_oidc.go:280 No concierge configured, skipping token credential exchange`, }, }, { @@ -515,14 +527,14 @@ func TestLoginOIDCCommand(t *testing.T) { "--upstream-identity-provider-name", "some-upstream-name", "--upstream-identity-provider-type", "ldap", }, - env: map[string]string{"PINNIPED_DEBUG": "true"}, - wantOptionsCount: 11, + env: map[string]string{"PINNIPED_DEBUG": "true", "PINNIPED_SKIP_PRINT_LOGIN_URL": "true"}, + wantOptionsCount: 12, wantStdout: `{"kind":"ExecCredential","apiVersion":"client.authentication.k8s.io/v1beta1","spec":{"interactive":false},"status":{"token":"exchanged-token"}}` + "\n", wantLogs: []string{ - nowStr + ` pinniped-login cmd/login_oidc.go:243 Performing OIDC login {"issuer": "test-issuer", "client id": "test-client-id"}`, - nowStr + ` pinniped-login cmd/login_oidc.go:253 Exchanging token for cluster credential {"endpoint": "https://127.0.0.1:1234/", "authenticator type": "webhook", "authenticator name": "test-authenticator"}`, - nowStr + ` pinniped-login cmd/login_oidc.go:261 Successfully exchanged token for cluster credential.`, - nowStr + ` pinniped-login cmd/login_oidc.go:268 caching cluster credential for future use.`, + nowStr + ` pinniped-login cmd/login_oidc.go:260 Performing OIDC login {"issuer": "test-issuer", "client id": "test-client-id"}`, + nowStr + ` pinniped-login cmd/login_oidc.go:270 Exchanging token for cluster credential {"endpoint": "https://127.0.0.1:1234/", "authenticator type": "webhook", "authenticator name": "test-authenticator"}`, + nowStr + ` pinniped-login cmd/login_oidc.go:278 Successfully exchanged token for cluster credential.`, + nowStr + ` pinniped-login cmd/login_oidc.go:285 caching cluster credential for future use.`, }, }, } diff --git a/pkg/oidcclient/login.go b/pkg/oidcclient/login.go index 102bc65e9..4fa62ec14 100644 --- a/pkg/oidcclient/login.go +++ b/pkg/oidcclient/login.go @@ -76,9 +76,6 @@ const ( // stdin returns the file descriptor for stdin as an int. func stdin() int { return int(os.Stdin.Fd()) } -// stderr returns the file descriptor for stderr as an int. -func stderr() int { return int(os.Stderr.Fd()) } - type handlerState struct { // Basic parameters. ctx context.Context @@ -94,6 +91,7 @@ type handlerState struct { upstreamIdentityProviderType string cliToSendCredentials bool skipBrowser bool + skipPrintLoginURL bool requestedAudience string httpClient *http.Client @@ -117,7 +115,6 @@ type handlerState struct { getEnv func(key string) string listen func(string, string) (net.Listener, error) stdinIsTTY func() bool - stderrIsTTY func() bool getProvider func(*oauth2.Config, *coreosoidc.Provider, *http.Client) upstreamprovider.UpstreamOIDCIdentityProviderI validateIDToken func(ctx context.Context, provider *coreosoidc.Provider, audience string, token string) (*coreosoidc.IDToken, error) promptForValue func(ctx context.Context, promptLabel string, out io.Writer) (string, error) @@ -198,7 +195,7 @@ func WithSkipBrowserOpen() Option { } } -// WithSkipListen causes the login skip starting the localhost listener, forcing the manual copy/paste login flow. +// WithSkipListen causes the login to skip starting the localhost listener, forcing the manual copy/paste login flow. func WithSkipListen() Option { return func(h *handlerState) error { h.listen = func(string, string) (net.Listener, error) { return nil, nil } @@ -206,6 +203,14 @@ func WithSkipListen() Option { } } +// WithSkipPrintLoginURL causes the login to skip printing the login URL when the browser opens to that URL. +func WithSkipPrintLoginURL() Option { + return func(h *handlerState) error { + h.skipPrintLoginURL = true + return nil + } +} + // SessionCacheKey contains the data used to select a valid session cache entry. type SessionCacheKey struct { Issuer string `json:"issuer"` @@ -298,7 +303,6 @@ func Login(issuer string, clientID string, opts ...Option) (*oidctypes.Token, er getEnv: os.Getenv, listen: net.Listen, stdinIsTTY: func() bool { return term.IsTerminal(stdin()) }, - stderrIsTTY: func() bool { return term.IsTerminal(stderr()) }, getProvider: upstreamoidc.New, validateIDToken: func(ctx context.Context, provider *coreosoidc.Provider, audience string, token string) (*coreosoidc.IDToken, error) { return provider.Verifier(&coreosoidc.Config{ClientID: audience}).Verify(ctx, token) @@ -635,19 +639,11 @@ func (h *handlerState) webBrowserBasedAuth(authorizeOptions *[]oauth2.AuthCodeOp openedBrowser = false } - // Always print the URL for ttys. This is the case when we were invoked by kubectl. - // For a non-tty, when the browser has opened, skip printing this, because printing it may confuse - // a console-based UI program like k9s which invoked this. The browser already has the URL in this case. - // For a non-tty, if the browser did not open, then the user has no way to login without the URL, - // so print it even though it may confuse apps like k9s. - // - // Note that there can be other reasons why stderr is not a tty. For example, when using bash redirects - // to combine stderr into stdout, e.g. "cmd1 2>&1 | cmd2", then stderr is not a tty for cmd1. - // If this hurts someone's ability to write scripts then we may want to instead introduce a command-line - // option or env var to control when this printing is skipped. For now, it seems unlikely that someone - // would be trying to script interactive logins, especially since they could use the CLI-based - // username/password prompts and env vars for scripting). - printAuthorizeURL := h.stderrIsTTY() || !openedBrowser + // Allow optionally skipping printing the login URL, for example because printing it may confuse + // a console-based UI program like k9s which invoked this. If the browser was opened, the browser + // already has the URL. If the browser did not open, then the user has no way to login without the URL, + // so print it anyway, even though it may confuse apps like k9s. + printAuthorizeURL := !openedBrowser || !h.skipPrintLoginURL // Prompt the user to visit the authorize URL, and to paste a manually-copied auth code (if possible). ctx, cancel := context.WithCancel(h.ctx) diff --git a/pkg/oidcclient/login_test.go b/pkg/oidcclient/login_test.go index d6fa3a76f..d7669f74d 100644 --- a/pkg/oidcclient/login_test.go +++ b/pkg/oidcclient/login_test.go @@ -622,7 +622,6 @@ func TestLogin(t *testing.T) { //nolint:gocyclo h.listen = func(string, string) (net.Listener, error) { return nil, fmt.Errorf("some listen error") } h.stdinIsTTY = func() bool { return false } - h.stderrIsTTY = func() bool { return true } return nil } }, @@ -694,7 +693,6 @@ func TestLogin(t *testing.T) { //nolint:gocyclo return nil, fmt.Errorf("some listen error") } h.stdinIsTTY = func() bool { return false } - h.stderrIsTTY = func() bool { return true } return nil } }, @@ -713,7 +711,6 @@ func TestLogin(t *testing.T) { //nolint:gocyclo h.generatePKCE = func() (pkce.Code, error) { return "test-pkce", nil } h.generateNonce = func() (nonce.Nonce, error) { return "test-nonce", nil } h.stdinIsTTY = func() bool { return true } - h.stderrIsTTY = func() bool { return true } require.NoError(t, WithClient(newClientForServer(formPostSuccessServer))(h)) require.NoError(t, WithSkipListen()(h)) h.openURL = func(authorizeURL string) error { @@ -753,7 +750,6 @@ func TestLogin(t *testing.T) { //nolint:gocyclo h.generatePKCE = func() (pkce.Code, error) { return "test-pkce", nil } h.generateNonce = func() (nonce.Nonce, error) { return "test-nonce", nil } h.stdinIsTTY = func() bool { return true } - h.stderrIsTTY = func() bool { return true } require.NoError(t, WithClient(newClientForServer(formPostSuccessServer))(h)) h.listen = func(string, string) (net.Listener, error) { return nil, fmt.Errorf("some listen error") } h.openURL = func(authorizeURL string) error { @@ -793,7 +789,6 @@ func TestLogin(t *testing.T) { //nolint:gocyclo h.generatePKCE = func() (pkce.Code, error) { return "test-pkce", nil } h.generateNonce = func() (nonce.Nonce, error) { return "test-nonce", nil } h.stdinIsTTY = func() bool { return true } - h.stderrIsTTY = func() bool { return true } require.NoError(t, WithClient(newClientForServer(successServer))(h)) @@ -829,7 +824,6 @@ func TestLogin(t *testing.T) { //nolint:gocyclo h.generatePKCE = func() (pkce.Code, error) { return "test-pkce", nil } h.generateNonce = func() (nonce.Nonce, error) { return "test-nonce", nil } h.stdinIsTTY = func() bool { return true } - h.stderrIsTTY = func() bool { return true } require.NoError(t, WithClient(newClientForServer(successServer))(h)) h.openURL = func(_ string) error { go func() { @@ -864,7 +858,6 @@ func TestLogin(t *testing.T) { //nolint:gocyclo h.generateNonce = func() (nonce.Nonce, error) { return "test-nonce", nil } h.stdinIsTTY = func() bool { return true } - h.stderrIsTTY = func() bool { return true } cache := &mockSessionCache{t: t, getReturnsToken: nil} cacheKey := SessionCacheKey{ @@ -929,7 +922,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo wantToken: &testToken, }, { - name: "callback returns success, without stderr tty and with opening the browser, did not show authorize URL on stderr", + name: "callback returns success, with skipPrintLoginURL and with opening the browser, did not show authorize URL on stderr", clientID: "test-client-id", opt: func(t *testing.T) Option { return func(h *handlerState) error { @@ -938,7 +931,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo h.generateNonce = func() (nonce.Nonce, error) { return "test-nonce", nil } h.stdinIsTTY = func() bool { return true } - h.stderrIsTTY = func() bool { return false } // stderr is not a tty + h.skipPrintLoginURL = true cache := &mockSessionCache{t: t, getReturnsToken: nil} cacheKey := SessionCacheKey{ @@ -995,7 +988,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo wantToken: &testToken, }, { - name: "callback returns success, without stderr tty but there was an error when opening the browser, did show authorize URL on stderr", + name: "callback returns success, with skipPrintLoginURL but there was an error when opening the browser, did show authorize URL on stderr", clientID: "test-client-id", opt: func(t *testing.T) Option { return func(h *handlerState) error { @@ -1004,7 +997,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo h.generateNonce = func() (nonce.Nonce, error) { return "test-nonce", nil } h.stdinIsTTY = func() bool { return true } - h.stderrIsTTY = func() bool { return false } // stderr is not a tty + h.skipPrintLoginURL = true cache := &mockSessionCache{t: t, getReturnsToken: nil} cacheKey := SessionCacheKey{ @@ -1073,7 +1066,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo wantToken: &testToken, }, { - name: "callback returns success, without stderr tty and with skipping the browser, did show authorize URL on stderr", + name: "callback returns success, with skipPrintLoginURL and with skipping the browser, did show authorize URL on stderr", clientID: "test-client-id", opt: func(t *testing.T) Option { return func(h *handlerState) error { @@ -1082,7 +1075,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo h.generateNonce = func() (nonce.Nonce, error) { return "test-nonce", nil } h.stdinIsTTY = func() bool { return true } - h.stderrIsTTY = func() bool { return false } // stderr is not a tty + h.skipPrintLoginURL = true cache := &mockSessionCache{t: t, getReturnsToken: nil} cacheKey := SessionCacheKey{ @@ -1157,7 +1150,6 @@ func TestLogin(t *testing.T) { //nolint:gocyclo h.generateNonce = func() (nonce.Nonce, error) { return "test-nonce", nil } h.stdinIsTTY = func() bool { return true } - h.stderrIsTTY = func() bool { return true } // Because response_mode=form_post, the Login function is going to prompt the user // to paste their authcode. This test needs to handle that prompt. @@ -1249,7 +1241,6 @@ func TestLogin(t *testing.T) { //nolint:gocyclo h.generateNonce = func() (nonce.Nonce, error) { return "test-nonce", nil } h.stdinIsTTY = func() bool { return true } - h.stderrIsTTY = func() bool { return true } cache := &mockSessionCache{t: t, getReturnsToken: nil} cacheKey := SessionCacheKey{