From 6e461821d6e14d15b6a2d3c6bf40157e7cfe3f14 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Thu, 2 Jun 2022 10:30:03 -0700 Subject: [PATCH 1/3] Allow PINNIPED_UPSTREAM_IDENTITY_PROVIDER_FLOW env var to override flow Env var may be used with CLI to override the flow selected by the --upstream-identity-provider-flow CLI flag. --- cmd/pinniped/cmd/login_oidc.go | 36 +++++-- cmd/pinniped/cmd/login_oidc_test.go | 149 ++++++++++++++++++++++++++-- 2 files changed, 172 insertions(+), 13 deletions(-) diff --git a/cmd/pinniped/cmd/login_oidc.go b/cmd/pinniped/cmd/login_oidc.go index 8f9378f5..b31f8dd6 100644 --- a/cmd/pinniped/cmd/login_oidc.go +++ b/cmd/pinniped/cmd/login_oidc.go @@ -32,6 +32,15 @@ import ( "go.pinniped.dev/pkg/oidcclient/oidctypes" ) +const ( + // The user may override the flow selection made by `--upstream-identity-provider-flow` using an env var. + // This allows the user to override their default flow selected inside their Pinniped-compatible kubeconfig file. + // A user might want to use this env var, for example, to choose the "browser_authcode" flow when using a kubeconfig + // 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" +) + // nolint: gochecknoinits func init() { loginCmd.AddCommand(oidcLoginCommand(oidcLoginCommandRealDeps())) @@ -165,6 +174,7 @@ func runOIDCLogin(cmd *cobra.Command, deps oidcLoginCommandDeps, flags oidcLogin flowOpts, err := flowOptions( idpdiscoveryv1alpha1.IDPType(flags.upstreamIdentityProviderType), idpdiscoveryv1alpha1.IDPFlow(flags.upstreamIdentityProviderFlow), + deps, ) if err != nil { return err @@ -250,9 +260,21 @@ func runOIDCLogin(cmd *cobra.Command, deps oidcLoginCommandDeps, flags oidcLogin return json.NewEncoder(cmd.OutOrStdout()).Encode(cred) } -func flowOptions(requestedIDPType idpdiscoveryv1alpha1.IDPType, requestedFlow idpdiscoveryv1alpha1.IDPFlow) ([]oidcclient.Option, error) { +func flowOptions( + requestedIDPType idpdiscoveryv1alpha1.IDPType, + requestedFlow idpdiscoveryv1alpha1.IDPFlow, + deps oidcLoginCommandDeps, +) ([]oidcclient.Option, error) { useCLIFlow := []oidcclient.Option{oidcclient.WithCLISendingCredentials()} + // If the env var is set to override the --upstream-identity-provider-type flag, then override it. + flowOverride, hasFlowOverride := deps.lookupEnv(upstreamIdentityProviderFlowEnvVarName) + flowSource := "--upstream-identity-provider-flow" + if hasFlowOverride { + requestedFlow = idpdiscoveryv1alpha1.IDPFlow(flowOverride) + flowSource = upstreamIdentityProviderFlowEnvVarName + } + switch requestedIDPType { case idpdiscoveryv1alpha1.IDPTypeOIDC: switch requestedFlow { @@ -262,19 +284,21 @@ func flowOptions(requestedIDPType idpdiscoveryv1alpha1.IDPType, requestedFlow id return nil, nil // browser authcode flow is the default Option, so don't need to return an Option here default: return nil, fmt.Errorf( - "--upstream-identity-provider-flow value not recognized for identity provider type %q: %s (supported values: %s)", - requestedIDPType, requestedFlow, strings.Join([]string{idpdiscoveryv1alpha1.IDPFlowBrowserAuthcode.String(), idpdiscoveryv1alpha1.IDPFlowCLIPassword.String()}, ", ")) + "%s value not recognized for identity provider type %q: %s (supported values: %s)", + flowSource, requestedIDPType, requestedFlow, + strings.Join([]string{idpdiscoveryv1alpha1.IDPFlowBrowserAuthcode.String(), idpdiscoveryv1alpha1.IDPFlowCLIPassword.String()}, ", ")) } case idpdiscoveryv1alpha1.IDPTypeLDAP, idpdiscoveryv1alpha1.IDPTypeActiveDirectory: switch requestedFlow { case idpdiscoveryv1alpha1.IDPFlowCLIPassword, "": return useCLIFlow, nil case idpdiscoveryv1alpha1.IDPFlowBrowserAuthcode: - return nil, nil + return nil, nil // browser authcode flow is the default Option, so don't need to return an Option here default: return nil, fmt.Errorf( - "--upstream-identity-provider-flow value not recognized for identity provider type %q: %s (supported values: %s)", - requestedIDPType, requestedFlow, strings.Join([]string{idpdiscoveryv1alpha1.IDPFlowCLIPassword.String(), idpdiscoveryv1alpha1.IDPFlowBrowserAuthcode.String()}, ", ")) + "%s value not recognized for identity provider type %q: %s (supported values: %s)", + flowSource, requestedIDPType, requestedFlow, + strings.Join([]string{idpdiscoveryv1alpha1.IDPFlowCLIPassword.String(), idpdiscoveryv1alpha1.IDPFlowBrowserAuthcode.String()}, ", ")) } default: // Surprisingly cobra does not support this kind of flag validation. See https://github.com/spf13/pflag/issues/236 diff --git a/cmd/pinniped/cmd/login_oidc_test.go b/cmd/pinniped/cmd/login_oidc_test.go index 492891e2..2e4fbd45 100644 --- a/cmd/pinniped/cmd/login_oidc_test.go +++ b/cmd/pinniped/cmd/login_oidc_test.go @@ -148,7 +148,7 @@ func TestLoginOIDCCommand(t *testing.T) { `), }, { - name: "invalid upstream type", + name: "invalid upstream type is an error", args: []string{ "--issuer", "test-issuer", "--upstream-identity-provider-type", "invalid", @@ -158,6 +158,18 @@ func TestLoginOIDCCommand(t *testing.T) { Error: --upstream-identity-provider-type value not recognized: invalid (supported values: oidc, ldap, activedirectory) `), }, + { + name: "invalid upstream type when flow override env var is used is still an error", + args: []string{ + "--issuer", "test-issuer", + "--upstream-identity-provider-type", "invalid", + }, + env: map[string]string{"PINNIPED_UPSTREAM_IDENTITY_PROVIDER_FLOW": "browser_authcode"}, + wantError: true, + wantStderr: here.Doc(` + Error: --upstream-identity-provider-type value not recognized: invalid (supported values: oidc, ldap, activedirectory) + `), + }, { name: "oidc upstream type with default flow is allowed", args: []string{ @@ -193,6 +205,32 @@ 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: "oidc upstream type with CLI flow in flow override env var is allowed", + args: []string{ + "--issuer", "test-issuer", + "--client-id", "test-client-id", + "--upstream-identity-provider-type", "oidc", + "--upstream-identity-provider-flow", "browser_authcode", + "--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution + }, + env: map[string]string{"PINNIPED_UPSTREAM_IDENTITY_PROVIDER_FLOW": "cli_password"}, + 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 with browser flow in flow override env var is allowed", + args: []string{ + "--issuer", "test-issuer", + "--client-id", "test-client-id", + "--upstream-identity-provider-type", "oidc", + "--upstream-identity-provider-flow", "cli_password", + "--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution + }, + env: map[string]string{"PINNIPED_UPSTREAM_IDENTITY_PROVIDER_FLOW": "browser_authcode"}, + 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: "oidc upstream type with unsupported flow is an error", args: []string{ @@ -207,6 +245,21 @@ func TestLoginOIDCCommand(t *testing.T) { Error: --upstream-identity-provider-flow value not recognized for identity provider type "oidc": foobar (supported values: browser_authcode, cli_password) `), }, + { + name: "oidc upstream type with unsupported flow in flow override env var is an error", + args: []string{ + "--issuer", "test-issuer", + "--client-id", "test-client-id", + "--upstream-identity-provider-type", "oidc", + "--upstream-identity-provider-flow", "browser_authcode", + "--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution + }, + env: map[string]string{"PINNIPED_UPSTREAM_IDENTITY_PROVIDER_FLOW": "foo"}, + wantError: true, + wantStderr: here.Doc(` + Error: PINNIPED_UPSTREAM_IDENTITY_PROVIDER_FLOW value not recognized for identity provider type "oidc": foo (supported values: browser_authcode, cli_password) + `), + }, { name: "ldap upstream type with default flow is allowed", args: []string{ @@ -253,6 +306,32 @@ 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: "ldap upstream type with CLI flow in flow override env var is allowed", + args: []string{ + "--issuer", "test-issuer", + "--client-id", "test-client-id", + "--upstream-identity-provider-type", "ldap", + "--upstream-identity-provider-flow", "browser_authcode", + "--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution + }, + env: map[string]string{"PINNIPED_UPSTREAM_IDENTITY_PROVIDER_FLOW": "cli_password"}, + 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: "ldap upstream type with browser_authcode flow in flow override env var is allowed", + args: []string{ + "--issuer", "test-issuer", + "--client-id", "test-client-id", + "--upstream-identity-provider-type", "ldap", + "--upstream-identity-provider-flow", "cli_password", + "--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution + }, + env: map[string]string{"PINNIPED_UPSTREAM_IDENTITY_PROVIDER_FLOW": "browser_authcode"}, + 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: "ldap upstream type with unsupported flow is an error", args: []string{ @@ -267,6 +346,21 @@ func TestLoginOIDCCommand(t *testing.T) { Error: --upstream-identity-provider-flow value not recognized for identity provider type "ldap": foo (supported values: cli_password, browser_authcode) `), }, + { + name: "ldap upstream type with unsupported flow in flow override env var is an error", + args: []string{ + "--issuer", "test-issuer", + "--client-id", "test-client-id", + "--upstream-identity-provider-type", "ldap", + "--upstream-identity-provider-flow", "browser_authcode", + "--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution + }, + env: map[string]string{"PINNIPED_UPSTREAM_IDENTITY_PROVIDER_FLOW": "foo"}, + wantError: true, + wantStderr: here.Doc(` + Error: PINNIPED_UPSTREAM_IDENTITY_PROVIDER_FLOW value not recognized for identity provider type "ldap": foo (supported values: cli_password, browser_authcode) + `), + }, { name: "active directory upstream type with CLI flow is allowed", args: []string{ @@ -291,6 +385,32 @@ 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: "active directory upstream type with CLI flow in flow override env var is allowed", + args: []string{ + "--issuer", "test-issuer", + "--client-id", "test-client-id", + "--upstream-identity-provider-type", "activedirectory", + "--upstream-identity-provider-flow", "browser_authcode", + "--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution + }, + env: map[string]string{"PINNIPED_UPSTREAM_IDENTITY_PROVIDER_FLOW": "cli_password"}, + 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: "active directory upstream type with browser_authcode in flow override env var is allowed", + args: []string{ + "--issuer", "test-issuer", + "--client-id", "test-client-id", + "--upstream-identity-provider-type", "activedirectory", + "--upstream-identity-provider-flow", "cli_password", + "--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution + }, + env: map[string]string{"PINNIPED_UPSTREAM_IDENTITY_PROVIDER_FLOW": "browser_authcode"}, + 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: "active directory upstream type with unsupported flow is an error", args: []string{ @@ -305,6 +425,21 @@ func TestLoginOIDCCommand(t *testing.T) { Error: --upstream-identity-provider-flow value not recognized for identity provider type "activedirectory": foo (supported values: cli_password, browser_authcode) `), }, + { + name: "active directory upstream type with unsupported flow in flow override env var is an error", + args: []string{ + "--issuer", "test-issuer", + "--client-id", "test-client-id", + "--upstream-identity-provider-type", "activedirectory", + "--upstream-identity-provider-flow", "browser_authcode", + "--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution + }, + env: map[string]string{"PINNIPED_UPSTREAM_IDENTITY_PROVIDER_FLOW": "foo"}, + wantError: true, + wantStderr: here.Doc(` + Error: PINNIPED_UPSTREAM_IDENTITY_PROVIDER_FLOW value not recognized for identity provider type "activedirectory": foo (supported values: cli_password, browser_authcode) + `), + }, { name: "login error", args: []string{ @@ -348,8 +483,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:222 Performing OIDC login {"issuer": "test-issuer", "client id": "test-client-id"}`, - nowStr + ` pinniped-login cmd/login_oidc.go:242 No concierge configured, skipping token credential exchange`, + nowStr + ` pinniped-login cmd/login_oidc.go:232 Performing OIDC login {"issuer": "test-issuer", "client id": "test-client-id"}`, + nowStr + ` pinniped-login cmd/login_oidc.go:252 No concierge configured, skipping token credential exchange`, }, }, { @@ -378,10 +513,10 @@ func TestLoginOIDCCommand(t *testing.T) { wantOptionsCount: 11, 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:222 Performing OIDC login {"issuer": "test-issuer", "client id": "test-client-id"}`, - nowStr + ` pinniped-login cmd/login_oidc.go:232 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:240 Successfully exchanged token for cluster credential.`, - nowStr + ` pinniped-login cmd/login_oidc.go:247 caching cluster credential for future use.`, + nowStr + ` pinniped-login cmd/login_oidc.go:232 Performing OIDC login {"issuer": "test-issuer", "client id": "test-client-id"}`, + nowStr + ` pinniped-login cmd/login_oidc.go:242 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:250 Successfully exchanged token for cluster credential.`, + nowStr + ` pinniped-login cmd/login_oidc.go:257 caching cluster credential for future use.`, }, }, } From cb8685b9427399d5625ea0d1870e537fa1cbd1ed Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Thu, 2 Jun 2022 11:27:54 -0700 Subject: [PATCH 2/3] Add e2e test for PINNIPED_UPSTREAM_IDENTITY_PROVIDER_FLOW env var --- test/integration/e2e_test.go | 69 ++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/test/integration/e2e_test.go b/test/integration/e2e_test.go index 0e16bde2..1c07b48e 100644 --- a/test/integration/e2e_test.go +++ b/test/integration/e2e_test.go @@ -53,6 +53,15 @@ import ( func TestE2EFullIntegration_Browser(t *testing.T) { env := testlib.IntegrationEnv(t) + // Avoid allowing PINNIPED_UPSTREAM_IDENTITY_PROVIDER_FLOW to interfere with these tests. + originalFlowEnvVarValue, flowOverrideEnvVarSet := os.LookupEnv("PINNIPED_UPSTREAM_IDENTITY_PROVIDER_FLOW") + if flowOverrideEnvVarSet { + require.NoError(t, os.Unsetenv("PINNIPED_UPSTREAM_IDENTITY_PROVIDER_FLOW")) + t.Cleanup(func() { + require.NoError(t, os.Setenv("PINNIPED_UPSTREAM_IDENTITY_PROVIDER_FLOW", originalFlowEnvVarValue)) + }) + } + topSetupCtx, cancelFunc := context.WithTimeout(context.Background(), 5*time.Minute) defer cancelFunc() @@ -1015,6 +1024,66 @@ func TestE2EFullIntegration_Browser(t *testing.T) { expectedGroups, ) }) + + // Add an LDAP upstream IDP and try using it to authenticate during kubectl commands, using the env var to choose the browser flow. + t.Run("with Supervisor LDAP upstream IDP and browser flow selected by env var override with with form_post automatic authcode delivery to CLI", func(t *testing.T) { + testCtx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) + t.Cleanup(cancel) + + tempDir := testutil.TempDir(t) // per-test tmp dir to avoid sharing files between tests + + // Start a fresh browser driver because we don't want to share cookies between the various tests in this file. + page := browsertest.Open(t) + + expectedUsername := env.SupervisorUpstreamLDAP.TestUserMailAttributeValue + expectedGroups := env.SupervisorUpstreamLDAP.TestUserDirectGroupsDNs + + setupClusterForEndToEndLDAPTest(t, expectedUsername, env) + + // Use a specific session cache for this test. + sessionCachePath := tempDir + "/test-sessions.yaml" + + kubeconfigPath := runPinnipedGetKubeconfig(t, env, pinnipedExe, tempDir, []string{ + "get", "kubeconfig", + "--concierge-api-group-suffix", env.APIGroupSuffix, + "--concierge-authenticator-type", "jwt", + "--concierge-authenticator-name", authenticator.Name, + "--oidc-skip-browser", + "--oidc-ca-bundle", testCABundlePath, + "--upstream-identity-provider-flow", "cli_password", // put cli_password in the kubeconfig, so we can override it with the env var + "--oidc-session-cache", sessionCachePath, + }) + + // Override the --upstream-identity-provider-flow flag from the kubeconfig using the env var. + require.NoError(t, os.Setenv("PINNIPED_UPSTREAM_IDENTITY_PROVIDER_FLOW", "browser_authcode")) + t.Cleanup(func() { + require.NoError(t, os.Unsetenv("PINNIPED_UPSTREAM_IDENTITY_PROVIDER_FLOW")) + }) + + // Run "kubectl get namespaces" which should trigger a browser login via the plugin. + kubectlCmd := exec.CommandContext(testCtx, "kubectl", "get", "namespace", "--kubeconfig", kubeconfigPath, "-v", "6") + kubectlCmd.Env = append(os.Environ(), env.ProxyEnv()...) + + // Run the kubectl command, wait for the Pinniped CLI to print the authorization URL, and open it in the browser. + kubectlOutputChan := startKubectlAndOpenAuthorizationURLInBrowser(testCtx, t, kubectlCmd, page) + + // Confirm that we got to the Supervisor's login page, fill out the form, and submit the form. + browsertest.LoginToUpstreamLDAP(t, page, downstream.Spec.Issuer, + expectedUsername, env.SupervisorUpstreamLDAP.TestUserPassword) + + formpostExpectSuccessState(t, page) + + requireKubectlGetNamespaceOutput(t, env, waitForKubectlOutput(t, kubectlOutputChan)) + + requireUserCanUseKubectlWithoutAuthenticatingAgain(testCtx, t, env, + downstream, + kubeconfigPath, + sessionCachePath, + pinnipedExe, + expectedUsername, + expectedGroups, + ) + }) } func startKubectlAndOpenAuthorizationURLInBrowser(testCtx context.Context, t *testing.T, kubectlCmd *exec.Cmd, page *agouti.Page) chan string { From fd9d641b5c2096ed05a1dd6ae98f9d2e5e092bc0 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Mon, 6 Jun 2022 09:47:50 -0700 Subject: [PATCH 3/3] Add doc for PINNIPED_UPSTREAM_IDENTITY_PROVIDER_FLOW env var --- site/content/docs/howto/login.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/site/content/docs/howto/login.md b/site/content/docs/howto/login.md index b2ae46a0..61034590 100644 --- a/site/content/docs/howto/login.md +++ b/site/content/docs/howto/login.md @@ -125,6 +125,11 @@ will depend on which type of identity provider was configured. Unlike the optional flow for OIDC providers described above, this optional flow does not need to be configured in the LDAPIdentityProvider or ActiveDirectoryIdentityProvider resource, so it is always available for end-users. +The flow selected by the `--upstream-identity-provider-flow` CLI flag may be overridden by using the +`PINNIPED_UPSTREAM_IDENTITY_PROVIDER_FLOW` environment variable for the CLI at runtime. This environment variable +may be set to the same values as the CLI flag (`browser_authcode` or `cli_password`). This allows a user to switch +flows based on their needs without editing their kubeconfig file. + Once the user completes authentication, the `kubectl` command will automatically continue and complete the user's requested command. For the example above, `kubectl` would list the cluster's namespaces.