Skip to content

Commit

Permalink
Add PINNIPED_SKIP_PRINT_LOGIN_URL env var to CLI
Browse files Browse the repository at this point in the history
  • Loading branch information
@cfryanr
cfryanr committed Mar 14, 2024
1 parent eab6f0d commit a70ce9c
Show file tree
Hide file tree
Showing 4 changed files with 61 additions and 45 deletions.
21 changes: 19 additions & 2 deletions cmd/pinniped/cmd/login_oidc.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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))
}
Expand Down Expand Up @@ -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
Expand Down
30 changes: 21 additions & 9 deletions cmd/pinniped/cmd/login_oidc_test.go
View file
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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`,
},
},
{
Expand All @@ -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.`,
},
},
}
Expand Down
34 changes: 15 additions & 19 deletions pkg/oidcclient/login.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -94,6 +91,7 @@ type handlerState struct {
upstreamIdentityProviderType string
cliToSendCredentials bool
skipBrowser bool
skipPrintLoginURL bool
requestedAudience string
httpClient *http.Client

Expand All @@ -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)
Expand Down Expand Up @@ -198,14 +195,22 @@ 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 }
return nil
}
}

// 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
}

Check warning on line 211 in pkg/oidcclient/login.go

View check run for this annotation

Codecov / codecov/patch

pkg/oidcclient/login.go#L207-L211 

Added lines #L207 - L211 were not covered by tests
}

// SessionCacheKey contains the data used to select a valid session cache entry.
type SessionCacheKey struct {
Issuer string `json:"issuer"`
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
21 changes: 6 additions & 15 deletions pkg/oidcclient/login_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
},
Expand Down Expand Up @@ -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
}
},
Expand All @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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))

Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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 {
Expand All @@ -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{
Expand Down Expand Up @@ -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 {
Expand All @@ -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{
Expand Down Expand Up @@ -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 {
Expand All @@ -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{
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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{
Expand Down

0 comments on commit a70ce9c

Please sign in to comment.