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 PINNIPED_SKIP_PRINT_LOGIN_URL env var to CLI #1897

Merged
merged 2 commits into from
Mar 15, 2024
Merged

Add PINNIPED_SKIP_PRINT_LOGIN_URL env var to CLI #1897

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a70ce9c
Add PINNIPED_SKIP_PRINT_LOGIN_URL env var to CLI
cfryanr Mar 14, 2024
92a082b
Merge branch 'main' into cli_skip_print_url_env_var
joshuatcasey Mar 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 19 additions & 2 deletions cmd/pinniped/cmd/login_oidc.go
View file
Open in desktop
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"
Copy link
Member

Choose a reason for hiding this comment

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

Should we relax this a bit to allow for True or TRUE? The following func should only return true for a very limited number of values as per https://gosamples.dev/string-to-bool/. We could still document using true but this allows for variations.

func isTruthyString(s string) bool {
	b, err := strconv.ParseBool(s)
	return err == nil && b
}

Copy link
Member Author

Choose a reason for hiding this comment

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

We can do that if you'd like. I was copying the style used by the existing PINNIPED_DEBUG env var which is either true or anything else is considered not 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
Open in desktop
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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,9 +76,6 @@
// 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 @@
upstreamIdentityProviderType string
cliToSendCredentials bool
skipBrowser bool
skipPrintLoginURL bool
requestedAudience string
httpClient *http.Client

Expand All @@ -117,7 +115,6 @@
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 @@
}
}

// 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 @@
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 @@
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
Open in desktop
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
Loading