From 7e0a3c114d9bae91872065e108b318532d49c4b3 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Thu, 2 May 2024 16:12:12 -0700 Subject: [PATCH] Pinniped CLI and the oidc-client package are now enhanced by pinniped_supported_identity_provider_types Co-authored-by: Joshua Casey --- cmd/pinniped/cmd/kubeconfig_test.go | 4 +- cmd/pinniped/cmd/login_oidc.go | 99 +- cmd/pinniped/cmd/login_oidc_test.go | 340 ++---- cmd/pinniped/cmd/oidc_client_options.go | 85 ++ .../kubecertagent/mocks/mockdynamiccert.go | 13 +- .../mocks/mockpodcommandexecutor.go | 11 +- .../mocks/mockoidcclientoptions/generate.go | 6 + .../mockoidcclientoptions.go | 216 ++++ pkg/oidcclient/login.go | 235 ++++- pkg/oidcclient/login_test.go | 969 ++++++++++++++++-- test/integration/e2e_test.go | 16 +- 11 files changed, 1548 insertions(+), 446 deletions(-) create mode 100644 cmd/pinniped/cmd/oidc_client_options.go create mode 100644 internal/mocks/mockoidcclientoptions/generate.go create mode 100644 internal/mocks/mockoidcclientoptions/mockoidcclientoptions.go diff --git a/cmd/pinniped/cmd/kubeconfig_test.go b/cmd/pinniped/cmd/kubeconfig_test.go index 6d95fa565..656d5126b 100644 --- a/cmd/pinniped/cmd/kubeconfig_test.go +++ b/cmd/pinniped/cmd/kubeconfig_test.go @@ -908,7 +908,7 @@ func TestGetKubeconfig(t *testing.T) { idpsDiscoveryResponse: here.Docf(`{ "pinniped_identity_providers": [ {"name": "some-ldap-idp", "type": "ldap"}, - {"name": "some-oidc-idp", "type": "oidc"} + {"name": "some-oidc-idp", "type": "oidc", "flows": ["flow1", "flow2"]} ] }`), wantLogs: func(issuerCABundle string, issuerURL string) []string { @@ -927,7 +927,7 @@ func TestGetKubeconfig(t *testing.T) { wantStderr: func(issuerCABundle string, issuerURL string) testutil.RequireErrorStringFunc { return testutil.WantExactErrorString(`Error: multiple Supervisor upstream identity providers were found, ` + `so the --upstream-identity-provider-name/--upstream-identity-provider-type flags must be specified. ` + - `Found these upstreams: [{"name":"some-ldap-idp","type":"ldap"},{"name":"some-oidc-idp","type":"oidc"}]` + "\n") + `Found these upstreams: [{"name":"some-ldap-idp","type":"ldap"},{"name":"some-oidc-idp","type":"oidc","flows":["flow1","flow2"]}]` + "\n") }, }, { diff --git a/cmd/pinniped/cmd/login_oidc.go b/cmd/pinniped/cmd/login_oidc.go index 3efa88130..d771b5178 100644 --- a/cmd/pinniped/cmd/login_oidc.go +++ b/cmd/pinniped/cmd/login_oidc.go @@ -12,7 +12,6 @@ import ( "net/http" "os" "path/filepath" - "strings" "time" "github.com/spf13/cobra" @@ -59,9 +58,10 @@ func init() { } type oidcLoginCommandDeps struct { - lookupEnv func(string) (string, bool) - login func(string, string, ...oidcclient.Option) (*oidctypes.Token, error) - exchangeToken func(context.Context, *conciergeclient.Client, string) (*clientauthv1beta1.ExecCredential, error) + lookupEnv func(string) (string, bool) + login func(string, string, ...oidcclient.Option) (*oidctypes.Token, error) + exchangeToken func(context.Context, *conciergeclient.Client, string) (*clientauthv1beta1.ExecCredential, error) + optionsFactory OIDCClientOptions } func oidcLoginCommandRealDeps() oidcLoginCommandDeps { @@ -71,6 +71,7 @@ func oidcLoginCommandRealDeps() oidcLoginCommandDeps { exchangeToken: func(ctx context.Context, client *conciergeclient.Client, token string) (*clientauthv1beta1.ExecCredential, error) { return client.ExchangeToken(ctx, token) }, + optionsFactory: &clientOptions{}, } } @@ -175,39 +176,37 @@ func runOIDCLogin(cmd *cobra.Command, deps oidcLoginCommandDeps, flags oidcLogin // Initialize the login handler. opts := []oidcclient.Option{ - oidcclient.WithContext(cmd.Context()), - oidcclient.WithLogger(plog.Logr()), //nolint:staticcheck // old code with lots of log statements - oidcclient.WithScopes(flags.scopes), - oidcclient.WithSessionCache(sessionCache), + deps.optionsFactory.WithContext(cmd.Context()), + deps.optionsFactory.WithLogger(plog.Logr()), //nolint:staticcheck // old code with lots of log statements + deps.optionsFactory.WithScopes(flags.scopes), + deps.optionsFactory.WithSessionCache(sessionCache), } skipPrintLoginURL, _ := deps.lookupEnv(skipPrintLoginURLEnvVarName) if skipPrintLoginURL == envVarTruthyValue { - opts = append(opts, oidcclient.WithSkipPrintLoginURL()) + opts = append(opts, deps.optionsFactory.WithSkipPrintLoginURL()) } if flags.listenPort != 0 { - opts = append(opts, oidcclient.WithListenPort(flags.listenPort)) + opts = append(opts, deps.optionsFactory.WithListenPort(flags.listenPort)) } if flags.requestAudience != "" { - opts = append(opts, oidcclient.WithRequestAudience(flags.requestAudience)) + opts = append(opts, deps.optionsFactory.WithRequestAudience(flags.requestAudience)) } if flags.upstreamIdentityProviderName != "" { - opts = append(opts, oidcclient.WithUpstreamIdentityProvider( + opts = append(opts, deps.optionsFactory.WithUpstreamIdentityProvider( flags.upstreamIdentityProviderName, flags.upstreamIdentityProviderType)) } - flowOpts, err := flowOptions( - idpdiscoveryv1alpha1.IDPType(flags.upstreamIdentityProviderType), - idpdiscoveryv1alpha1.IDPFlow(flags.upstreamIdentityProviderFlow), - deps, - ) - if err != nil { - return err + requestedFlow, flowSource := idpdiscoveryv1alpha1.IDPFlow(flags.upstreamIdentityProviderFlow), "--upstream-identity-provider-flow" + if flowOverride, hasFlowOverride := deps.lookupEnv(upstreamIdentityProviderFlowEnvVarName); hasFlowOverride { + requestedFlow, flowSource = idpdiscoveryv1alpha1.IDPFlow(flowOverride), upstreamIdentityProviderFlowEnvVarName + } + if requestedFlow != "" { + opts = append(opts, deps.optionsFactory.WithLoginFlow(requestedFlow, flowSource)) } - opts = append(opts, flowOpts...) var concierge *conciergeclient.Client if flags.conciergeEnabled { @@ -225,12 +224,12 @@ func runOIDCLogin(cmd *cobra.Command, deps oidcLoginCommandDeps, flags oidcLogin // --skip-browser skips opening the browser. if flags.skipBrowser { - opts = append(opts, oidcclient.WithSkipBrowserOpen()) + opts = append(opts, deps.optionsFactory.WithSkipBrowserOpen()) } // --skip-listen skips starting the localhost callback listener. if flags.skipListen { - opts = append(opts, oidcclient.WithSkipListen()) + opts = append(opts, deps.optionsFactory.WithSkipListen()) } if len(flags.caBundlePaths) > 0 || len(flags.caBundleData) > 0 { @@ -238,7 +237,7 @@ func runOIDCLogin(cmd *cobra.Command, deps oidcLoginCommandDeps, flags oidcLogin if err != nil { return err } - opts = append(opts, oidcclient.WithClient(client)) + opts = append(opts, deps.optionsFactory.WithClient(client)) } // Look up cached credentials based on a hash of all the CLI arguments and the cluster info. cacheKey := struct { @@ -288,60 +287,6 @@ func runOIDCLogin(cmd *cobra.Command, deps oidcLoginCommandDeps, flags oidcLogin return json.NewEncoder(cmd.OutOrStdout()).Encode(cred) } -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 { - case idpdiscoveryv1alpha1.IDPFlowCLIPassword: - return useCLIFlow, nil - case idpdiscoveryv1alpha1.IDPFlowBrowserAuthcode, "": - return nil, nil // browser authcode flow is the default Option, so don't need to return an Option here - default: - return nil, fmt.Errorf( - "%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 // browser authcode flow is the default Option, so don't need to return an Option here - default: - return nil, fmt.Errorf( - "%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 - return nil, fmt.Errorf( - "--upstream-identity-provider-type value not recognized: %s (supported values: %s)", - requestedIDPType, - strings.Join([]string{ - idpdiscoveryv1alpha1.IDPTypeOIDC.String(), - idpdiscoveryv1alpha1.IDPTypeLDAP.String(), - idpdiscoveryv1alpha1.IDPTypeActiveDirectory.String(), - }, ", "), - ) - } -} - func makeClient(caBundlePaths []string, caBundleData []string) (*http.Client, error) { pool := x509.NewCertPool() for _, p := range caBundlePaths { diff --git a/cmd/pinniped/cmd/login_oidc_test.go b/cmd/pinniped/cmd/login_oidc_test.go index 581660210..5bd6f1884 100644 --- a/cmd/pinniped/cmd/login_oidc_test.go +++ b/cmd/pinniped/cmd/login_oidc_test.go @@ -14,12 +14,16 @@ import ( "time" "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" clientauthv1beta1 "k8s.io/client-go/pkg/apis/clientauthentication/v1beta1" clocktesting "k8s.io/utils/clock/testing" + idpdiscoveryv1alpha1 "go.pinniped.dev/generated/latest/apis/supervisor/idpdiscovery/v1alpha1" + oidcapi "go.pinniped.dev/generated/latest/apis/supervisor/oidc" "go.pinniped.dev/internal/certauthority" "go.pinniped.dev/internal/here" + "go.pinniped.dev/internal/mocks/mockoidcclientoptions" "go.pinniped.dev/internal/plog" "go.pinniped.dev/internal/testutil" "go.pinniped.dev/pkg/conciergeclient" @@ -42,6 +46,13 @@ func TestLoginOIDCCommand(t *testing.T) { require.NoError(t, err) nowStr := now.Local().Format(time.RFC1123) + defaultWantedOptions := func(f *mockoidcclientoptions.MockOIDCClientOptions) { + f.EXPECT().WithContext(gomock.Any()) + f.EXPECT().WithLogger(gomock.Any()) + f.EXPECT().WithScopes([]string{oidcapi.ScopeOfflineAccess, oidcapi.ScopeOpenID, oidcapi.ScopeRequestAudience, oidcapi.ScopeUsername, oidcapi.ScopeGroups}) + f.EXPECT().WithSessionCache(gomock.Any()) + } + tests := []struct { name string args []string @@ -51,6 +62,7 @@ func TestLoginOIDCCommand(t *testing.T) { wantError bool wantStdout string wantStderr string + wantOptions func(f *mockoidcclientoptions.MockOIDCClientOptions) wantOptionsCount int wantLogs []string }{ @@ -109,7 +121,8 @@ func TestLoginOIDCCommand(t *testing.T) { "--issuer", "test-issuer", "--enable-concierge", }, - wantError: true, + wantOptions: defaultWantedOptions, + wantError: true, wantStderr: here.Doc(` Error: invalid Concierge parameters: endpoint must not be empty `), @@ -121,7 +134,8 @@ func TestLoginOIDCCommand(t *testing.T) { "--issuer", "test-issuer", "--ca-bundle", "./does/not/exist", }, - wantError: true, + wantOptions: defaultWantedOptions, + wantError: true, wantStderr: here.Doc(` Error: could not read --ca-bundle: open ./does/not/exist: no such file or directory `), @@ -133,7 +147,8 @@ func TestLoginOIDCCommand(t *testing.T) { "--issuer", "test-issuer", "--ca-bundle-data", "invalid-base64", }, - wantError: true, + wantOptions: defaultWantedOptions, + wantError: true, wantStderr: here.Doc(` Error: could not read --ca-bundle-data: illegal base64 data at input byte 7 `), @@ -148,34 +163,12 @@ func TestLoginOIDCCommand(t *testing.T) { "--concierge-authenticator-name", "test-authenticator", "--concierge-endpoint", "https://127.0.0.1:1234/", }, - wantError: true, + wantOptions: defaultWantedOptions, + wantError: true, wantStderr: here.Doc(` Error: invalid Concierge parameters: invalid API group suffix: a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*') `), }, - { - name: "invalid upstream type is an error", - args: []string{ - "--issuer", "test-issuer", - "--upstream-identity-provider-type", "invalid", - }, - wantError: true, - wantStderr: here.Doc(` - 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{ @@ -184,6 +177,7 @@ func TestLoginOIDCCommand(t *testing.T) { "--upstream-identity-provider-type", "oidc", "--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution }, + wantOptions: defaultWantedOptions, 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", }, @@ -195,269 +189,45 @@ func TestLoginOIDCCommand(t *testing.T) { "--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{ - "--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 - }, - 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 browser flow 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 - }, - 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_SKIP_PRINT_LOGIN_URL": "true"}, + wantOptions: func(f *mockoidcclientoptions.MockOIDCClientOptions) { + defaultWantedOptions(f) + f.EXPECT().WithSkipPrintLoginURL() }, - 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", + name: "--upstream-identity-provider-flow adds an option", 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{ - "--issuer", "test-issuer", - "--client-id", "test-client-id", - "--upstream-identity-provider-type", "oidc", - "--upstream-identity-provider-flow", "foobar", - "--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution - }, - wantError: true, - wantStderr: here.Doc(` - 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{ - "--issuer", "test-issuer", - "--client-id", "test-client-id", - "--upstream-identity-provider-type", "ldap", - "--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution + wantOptions: func(f *mockoidcclientoptions.MockOIDCClientOptions) { + defaultWantedOptions(f) + f.EXPECT().WithLoginFlow(idpdiscoveryv1alpha1.IDPFlowCLIPassword, "--upstream-identity-provider-flow") }, 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: "activedirectory upstream type with default flow is allowed", + name: "PINNIPED_UPSTREAM_IDENTITY_PROVIDER_FLOW adds an option that overrides --upstream-identity-provider-flow", args: []string{ "--issuer", "test-issuer", "--client-id", "test-client-id", - "--upstream-identity-provider-type", "activedirectory", + "--upstream-identity-provider-flow", "ignored-value-from-param", "--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution }, - 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 CLI flow 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": "actual-value-from-env"}, + wantOptions: func(f *mockoidcclientoptions.MockOIDCClientOptions) { + defaultWantedOptions(f) + f.EXPECT().WithLoginFlow(idpdiscoveryv1alpha1.IDPFlow("actual-value-from-env"), "PINNIPED_UPSTREAM_IDENTITY_PROVIDER_FLOW") }, 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 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 - }, - 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{ - "--issuer", "test-issuer", - "--client-id", "test-client-id", - "--upstream-identity-provider-type", "ldap", - "--upstream-identity-provider-flow", "foo", - "--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution - }, - wantError: true, - wantStderr: here.Doc(` - 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{ - "--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 - }, - 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 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 - }, - 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{ - "--issuer", "test-issuer", - "--client-id", "test-client-id", - "--upstream-identity-provider-type", "activedirectory", - "--upstream-identity-provider-flow", "foo", - "--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution - }, - wantError: true, - wantStderr: here.Doc(` - 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{ @@ -466,6 +236,7 @@ func TestLoginOIDCCommand(t *testing.T) { "--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution }, loginErr: fmt.Errorf("some login error"), + wantOptions: defaultWantedOptions, wantOptionsCount: 4, wantError: true, wantStderr: here.Doc(` @@ -484,6 +255,7 @@ func TestLoginOIDCCommand(t *testing.T) { "--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution }, conciergeErr: fmt.Errorf("some concierge error"), + wantOptions: defaultWantedOptions, wantOptionsCount: 4, wantError: true, wantStderr: here.Doc(` @@ -498,11 +270,12 @@ func TestLoginOIDCCommand(t *testing.T) { "--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution }, env: map[string]string{"PINNIPED_DEBUG": "true"}, + wantOptions: defaultWantedOptions, 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: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`, + nowStr + ` pinniped-login cmd/login_oidc.go:259 Performing OIDC login {"issuer": "test-issuer", "client id": "test-client-id"}`, + nowStr + ` pinniped-login cmd/login_oidc.go:279 No concierge configured, skipping token credential exchange`, }, }, { @@ -526,15 +299,30 @@ func TestLoginOIDCCommand(t *testing.T) { "--credential-cache", t.TempDir() + "/credentials.yaml", // must specify --credential-cache or else the cache file on disk causes test pollution "--upstream-identity-provider-name", "some-upstream-name", "--upstream-identity-provider-type", "ldap", + "--upstream-identity-provider-flow", "some-flow-type", + }, + env: map[string]string{"PINNIPED_DEBUG": "true", "PINNIPED_SKIP_PRINT_LOGIN_URL": "true"}, + wantOptions: func(f *mockoidcclientoptions.MockOIDCClientOptions) { + f.EXPECT().WithContext(gomock.Any()) + f.EXPECT().WithLogger(gomock.Any()) + f.EXPECT().WithScopes([]string{oidcapi.ScopeOfflineAccess, oidcapi.ScopeOpenID, oidcapi.ScopeRequestAudience, oidcapi.ScopeUsername, oidcapi.ScopeGroups}) + f.EXPECT().WithSessionCache(gomock.Any()) + f.EXPECT().WithListenPort(uint16(1234)) + f.EXPECT().WithSkipBrowserOpen() + f.EXPECT().WithSkipListen() + f.EXPECT().WithSkipPrintLoginURL() + f.EXPECT().WithClient(gomock.Any()) + f.EXPECT().WithRequestAudience("cluster-1234") + f.EXPECT().WithLoginFlow(idpdiscoveryv1alpha1.IDPFlow("some-flow-type"), "--upstream-identity-provider-flow") + f.EXPECT().WithUpstreamIdentityProvider("some-upstream-name", "ldap") }, - 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: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.`, + nowStr + ` pinniped-login cmd/login_oidc.go:259 Performing OIDC login {"issuer": "test-issuer", "client id": "test-client-id"}`, + nowStr + ` pinniped-login cmd/login_oidc.go:269 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:277 Successfully exchanged token for cluster credential.`, + nowStr + ` pinniped-login cmd/login_oidc.go:284 caching cluster credential for future use.`, }, }, } @@ -543,6 +331,13 @@ func TestLoginOIDCCommand(t *testing.T) { var buf bytes.Buffer ctx := plog.AddZapOverridesToContext(context.Background(), t, &buf, nil, clocktesting.NewFakeClock(now)) + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + optionsFactory := mockoidcclientoptions.NewMockOIDCClientOptions(ctrl) + if tt.wantOptions != nil { + tt.wantOptions(optionsFactory) + } + var gotOptions []oidcclient.Option cmd := oidcLoginCommand(oidcLoginCommandDeps{ lookupEnv: func(s string) (string, bool) { @@ -578,6 +373,7 @@ func TestLoginOIDCCommand(t *testing.T) { }, }, nil }, + optionsFactory: optionsFactory, }) require.NotNil(t, cmd) diff --git a/cmd/pinniped/cmd/oidc_client_options.go b/cmd/pinniped/cmd/oidc_client_options.go new file mode 100644 index 000000000..7b5f098c9 --- /dev/null +++ b/cmd/pinniped/cmd/oidc_client_options.go @@ -0,0 +1,85 @@ +// Copyright 2024 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package cmd + +import ( + "context" + "net/http" + + "github.com/go-logr/logr" + + "go.pinniped.dev/generated/latest/apis/supervisor/idpdiscovery/v1alpha1" + "go.pinniped.dev/pkg/oidcclient" +) + +// OIDCClientOptions is an interface that wraps the creation of Options for the purpose of making them +// more friendly to unit tests. Because the Option type refers to a private struct type, it is hard +// to create mocks for them in tests of other packages. This provides a seam that can be mocked. +type OIDCClientOptions interface { + WithContext(ctx context.Context) oidcclient.Option + WithLogger(logger logr.Logger) oidcclient.Option + WithListenPort(port uint16) oidcclient.Option + WithSkipBrowserOpen() oidcclient.Option + WithSkipListen() oidcclient.Option + WithSkipPrintLoginURL() oidcclient.Option + WithSessionCache(cache oidcclient.SessionCache) oidcclient.Option + WithClient(httpClient *http.Client) oidcclient.Option + WithScopes(scopes []string) oidcclient.Option + WithRequestAudience(audience string) oidcclient.Option + WithLoginFlow(loginFlow v1alpha1.IDPFlow, flowSource string) oidcclient.Option + WithUpstreamIdentityProvider(upstreamName, upstreamType string) oidcclient.Option +} + +// clientOptions implements OIDCClientOptions for production use. +type clientOptions struct{} + +var _ OIDCClientOptions = (*clientOptions)(nil) + +func (o *clientOptions) WithContext(ctx context.Context) oidcclient.Option { + return oidcclient.WithContext(ctx) +} + +func (o *clientOptions) WithLogger(logger logr.Logger) oidcclient.Option { + return oidcclient.WithLogger(logger) +} + +func (o *clientOptions) WithListenPort(port uint16) oidcclient.Option { + return oidcclient.WithListenPort(port) +} + +func (o *clientOptions) WithSkipBrowserOpen() oidcclient.Option { + return oidcclient.WithSkipBrowserOpen() +} + +func (o *clientOptions) WithSkipListen() oidcclient.Option { + return oidcclient.WithSkipListen() +} + +func (o *clientOptions) WithSkipPrintLoginURL() oidcclient.Option { + return oidcclient.WithSkipPrintLoginURL() +} + +func (o *clientOptions) WithSessionCache(cache oidcclient.SessionCache) oidcclient.Option { + return oidcclient.WithSessionCache(cache) +} + +func (o *clientOptions) WithClient(httpClient *http.Client) oidcclient.Option { + return oidcclient.WithClient(httpClient) +} + +func (o *clientOptions) WithScopes(scopes []string) oidcclient.Option { + return oidcclient.WithScopes(scopes) +} + +func (o *clientOptions) WithRequestAudience(audience string) oidcclient.Option { + return oidcclient.WithRequestAudience(audience) +} + +func (o *clientOptions) WithLoginFlow(loginFlow v1alpha1.IDPFlow, flowSource string) oidcclient.Option { + return oidcclient.WithLoginFlow(loginFlow, flowSource) +} + +func (o *clientOptions) WithUpstreamIdentityProvider(upstreamName, upstreamType string) oidcclient.Option { + return oidcclient.WithUpstreamIdentityProvider(upstreamName, upstreamType) +} diff --git a/internal/controller/kubecertagent/mocks/mockdynamiccert.go b/internal/controller/kubecertagent/mocks/mockdynamiccert.go index e22f6d50d..83429341f 100644 --- a/internal/controller/kubecertagent/mocks/mockdynamiccert.go +++ b/internal/controller/kubecertagent/mocks/mockdynamiccert.go @@ -4,6 +4,11 @@ // Code generated by MockGen. DO NOT EDIT. // Source: go.pinniped.dev/internal/dynamiccert (interfaces: Private) +// +// Generated by this command: +// +// mockgen -destination=mockdynamiccert.go -package=mocks -copyright_file=../../../../hack/header.txt -mock_names Private=MockDynamicCertPrivate go.pinniped.dev/internal/dynamiccert Private +// // Package mocks is a generated GoMock package. package mocks @@ -46,7 +51,7 @@ func (m *MockDynamicCertPrivate) AddListener(arg0 dynamiccertificates.Listener) } // AddListener indicates an expected call of AddListener. -func (mr *MockDynamicCertPrivateMockRecorder) AddListener(arg0 interface{}) *gomock.Call { +func (mr *MockDynamicCertPrivateMockRecorder) AddListener(arg0 any) *gomock.Call { mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "AddListener", reflect.TypeOf((*MockDynamicCertPrivate)(nil).AddListener), arg0) } @@ -87,7 +92,7 @@ func (m *MockDynamicCertPrivate) Run(arg0 context.Context, arg1 int) { } // Run indicates an expected call of Run. -func (mr *MockDynamicCertPrivateMockRecorder) Run(arg0, arg1 interface{}) *gomock.Call { +func (mr *MockDynamicCertPrivateMockRecorder) Run(arg0, arg1 any) *gomock.Call { mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Run", reflect.TypeOf((*MockDynamicCertPrivate)(nil).Run), arg0, arg1) } @@ -101,7 +106,7 @@ func (m *MockDynamicCertPrivate) RunOnce(arg0 context.Context) error { } // RunOnce indicates an expected call of RunOnce. -func (mr *MockDynamicCertPrivateMockRecorder) RunOnce(arg0 interface{}) *gomock.Call { +func (mr *MockDynamicCertPrivateMockRecorder) RunOnce(arg0 any) *gomock.Call { mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RunOnce", reflect.TypeOf((*MockDynamicCertPrivate)(nil).RunOnce), arg0) } @@ -115,7 +120,7 @@ func (m *MockDynamicCertPrivate) SetCertKeyContent(arg0, arg1 []byte) error { } // SetCertKeyContent indicates an expected call of SetCertKeyContent. -func (mr *MockDynamicCertPrivateMockRecorder) SetCertKeyContent(arg0, arg1 interface{}) *gomock.Call { +func (mr *MockDynamicCertPrivateMockRecorder) SetCertKeyContent(arg0, arg1 any) *gomock.Call { mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetCertKeyContent", reflect.TypeOf((*MockDynamicCertPrivate)(nil).SetCertKeyContent), arg0, arg1) } diff --git a/internal/controller/kubecertagent/mocks/mockpodcommandexecutor.go b/internal/controller/kubecertagent/mocks/mockpodcommandexecutor.go index 1cdb18f01..3bf7e753b 100644 --- a/internal/controller/kubecertagent/mocks/mockpodcommandexecutor.go +++ b/internal/controller/kubecertagent/mocks/mockpodcommandexecutor.go @@ -4,6 +4,11 @@ // Code generated by MockGen. DO NOT EDIT. // Source: go.pinniped.dev/internal/controller/kubecertagent (interfaces: PodCommandExecutor) +// +// Generated by this command: +// +// mockgen -destination=mockpodcommandexecutor.go -package=mocks -copyright_file=../../../../hack/header.txt go.pinniped.dev/internal/controller/kubecertagent PodCommandExecutor +// // Package mocks is a generated GoMock package. package mocks @@ -41,7 +46,7 @@ func (m *MockPodCommandExecutor) EXPECT() *MockPodCommandExecutorMockRecorder { // Exec mocks base method. func (m *MockPodCommandExecutor) Exec(arg0 context.Context, arg1, arg2, arg3 string, arg4 ...string) (string, error) { m.ctrl.T.Helper() - varargs := []interface{}{arg0, arg1, arg2, arg3} + varargs := []any{arg0, arg1, arg2, arg3} for _, a := range arg4 { varargs = append(varargs, a) } @@ -52,8 +57,8 @@ func (m *MockPodCommandExecutor) Exec(arg0 context.Context, arg1, arg2, arg3 str } // Exec indicates an expected call of Exec. -func (mr *MockPodCommandExecutorMockRecorder) Exec(arg0, arg1, arg2, arg3 interface{}, arg4 ...interface{}) *gomock.Call { +func (mr *MockPodCommandExecutorMockRecorder) Exec(arg0, arg1, arg2, arg3 any, arg4 ...any) *gomock.Call { mr.mock.ctrl.T.Helper() - varargs := append([]interface{}{arg0, arg1, arg2, arg3}, arg4...) + varargs := append([]any{arg0, arg1, arg2, arg3}, arg4...) return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Exec", reflect.TypeOf((*MockPodCommandExecutor)(nil).Exec), varargs...) } diff --git a/internal/mocks/mockoidcclientoptions/generate.go b/internal/mocks/mockoidcclientoptions/generate.go new file mode 100644 index 000000000..33854a5e2 --- /dev/null +++ b/internal/mocks/mockoidcclientoptions/generate.go @@ -0,0 +1,6 @@ +// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package mockoidcclientoptions + +//go:generate go run -v go.uber.org/mock/mockgen -destination=mockoidcclientoptions.go -package=mockoidcclientoptions -copyright_file=../../../hack/header.txt go.pinniped.dev/cmd/pinniped/cmd OIDCClientOptions diff --git a/internal/mocks/mockoidcclientoptions/mockoidcclientoptions.go b/internal/mocks/mockoidcclientoptions/mockoidcclientoptions.go new file mode 100644 index 000000000..231d3f897 --- /dev/null +++ b/internal/mocks/mockoidcclientoptions/mockoidcclientoptions.go @@ -0,0 +1,216 @@ +// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 +// + +// Code generated by MockGen. DO NOT EDIT. +// Source: go.pinniped.dev/cmd/pinniped/cmd (interfaces: OIDCClientOptions) +// +// Generated by this command: +// +// mockgen -destination=mockoidcclientoptions.go -package=mockoidcclientoptions -copyright_file=../../../hack/header.txt go.pinniped.dev/cmd/pinniped/cmd OIDCClientOptions +// + +// Package mockoidcclientoptions is a generated GoMock package. +package mockoidcclientoptions + +import ( + context "context" + http "net/http" + reflect "reflect" + + logr "github.com/go-logr/logr" + v1alpha1 "go.pinniped.dev/generated/latest/apis/supervisor/idpdiscovery/v1alpha1" + oidcclient "go.pinniped.dev/pkg/oidcclient" + gomock "go.uber.org/mock/gomock" +) + +// MockOIDCClientOptions is a mock of OIDCClientOptions interface. +type MockOIDCClientOptions struct { + ctrl *gomock.Controller + recorder *MockOIDCClientOptionsMockRecorder +} + +// MockOIDCClientOptionsMockRecorder is the mock recorder for MockOIDCClientOptions. +type MockOIDCClientOptionsMockRecorder struct { + mock *MockOIDCClientOptions +} + +// NewMockOIDCClientOptions creates a new mock instance. +func NewMockOIDCClientOptions(ctrl *gomock.Controller) *MockOIDCClientOptions { + mock := &MockOIDCClientOptions{ctrl: ctrl} + mock.recorder = &MockOIDCClientOptionsMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockOIDCClientOptions) EXPECT() *MockOIDCClientOptionsMockRecorder { + return m.recorder +} + +// WithClient mocks base method. +func (m *MockOIDCClientOptions) WithClient(arg0 *http.Client) oidcclient.Option { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "WithClient", arg0) + ret0, _ := ret[0].(oidcclient.Option) + return ret0 +} + +// WithClient indicates an expected call of WithClient. +func (mr *MockOIDCClientOptionsMockRecorder) WithClient(arg0 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "WithClient", reflect.TypeOf((*MockOIDCClientOptions)(nil).WithClient), arg0) +} + +// WithContext mocks base method. +func (m *MockOIDCClientOptions) WithContext(arg0 context.Context) oidcclient.Option { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "WithContext", arg0) + ret0, _ := ret[0].(oidcclient.Option) + return ret0 +} + +// WithContext indicates an expected call of WithContext. +func (mr *MockOIDCClientOptionsMockRecorder) WithContext(arg0 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "WithContext", reflect.TypeOf((*MockOIDCClientOptions)(nil).WithContext), arg0) +} + +// WithListenPort mocks base method. +func (m *MockOIDCClientOptions) WithListenPort(arg0 uint16) oidcclient.Option { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "WithListenPort", arg0) + ret0, _ := ret[0].(oidcclient.Option) + return ret0 +} + +// WithListenPort indicates an expected call of WithListenPort. +func (mr *MockOIDCClientOptionsMockRecorder) WithListenPort(arg0 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "WithListenPort", reflect.TypeOf((*MockOIDCClientOptions)(nil).WithListenPort), arg0) +} + +// WithLogger mocks base method. +func (m *MockOIDCClientOptions) WithLogger(arg0 logr.Logger) oidcclient.Option { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "WithLogger", arg0) + ret0, _ := ret[0].(oidcclient.Option) + return ret0 +} + +// WithLogger indicates an expected call of WithLogger. +func (mr *MockOIDCClientOptionsMockRecorder) WithLogger(arg0 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "WithLogger", reflect.TypeOf((*MockOIDCClientOptions)(nil).WithLogger), arg0) +} + +// WithLoginFlow mocks base method. +func (m *MockOIDCClientOptions) WithLoginFlow(arg0 v1alpha1.IDPFlow, arg1 string) oidcclient.Option { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "WithLoginFlow", arg0, arg1) + ret0, _ := ret[0].(oidcclient.Option) + return ret0 +} + +// WithLoginFlow indicates an expected call of WithLoginFlow. +func (mr *MockOIDCClientOptionsMockRecorder) WithLoginFlow(arg0, arg1 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "WithLoginFlow", reflect.TypeOf((*MockOIDCClientOptions)(nil).WithLoginFlow), arg0, arg1) +} + +// WithRequestAudience mocks base method. +func (m *MockOIDCClientOptions) WithRequestAudience(arg0 string) oidcclient.Option { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "WithRequestAudience", arg0) + ret0, _ := ret[0].(oidcclient.Option) + return ret0 +} + +// WithRequestAudience indicates an expected call of WithRequestAudience. +func (mr *MockOIDCClientOptionsMockRecorder) WithRequestAudience(arg0 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "WithRequestAudience", reflect.TypeOf((*MockOIDCClientOptions)(nil).WithRequestAudience), arg0) +} + +// WithScopes mocks base method. +func (m *MockOIDCClientOptions) WithScopes(arg0 []string) oidcclient.Option { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "WithScopes", arg0) + ret0, _ := ret[0].(oidcclient.Option) + return ret0 +} + +// WithScopes indicates an expected call of WithScopes. +func (mr *MockOIDCClientOptionsMockRecorder) WithScopes(arg0 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "WithScopes", reflect.TypeOf((*MockOIDCClientOptions)(nil).WithScopes), arg0) +} + +// WithSessionCache mocks base method. +func (m *MockOIDCClientOptions) WithSessionCache(arg0 oidcclient.SessionCache) oidcclient.Option { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "WithSessionCache", arg0) + ret0, _ := ret[0].(oidcclient.Option) + return ret0 +} + +// WithSessionCache indicates an expected call of WithSessionCache. +func (mr *MockOIDCClientOptionsMockRecorder) WithSessionCache(arg0 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "WithSessionCache", reflect.TypeOf((*MockOIDCClientOptions)(nil).WithSessionCache), arg0) +} + +// WithSkipBrowserOpen mocks base method. +func (m *MockOIDCClientOptions) WithSkipBrowserOpen() oidcclient.Option { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "WithSkipBrowserOpen") + ret0, _ := ret[0].(oidcclient.Option) + return ret0 +} + +// WithSkipBrowserOpen indicates an expected call of WithSkipBrowserOpen. +func (mr *MockOIDCClientOptionsMockRecorder) WithSkipBrowserOpen() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "WithSkipBrowserOpen", reflect.TypeOf((*MockOIDCClientOptions)(nil).WithSkipBrowserOpen)) +} + +// WithSkipListen mocks base method. +func (m *MockOIDCClientOptions) WithSkipListen() oidcclient.Option { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "WithSkipListen") + ret0, _ := ret[0].(oidcclient.Option) + return ret0 +} + +// WithSkipListen indicates an expected call of WithSkipListen. +func (mr *MockOIDCClientOptionsMockRecorder) WithSkipListen() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "WithSkipListen", reflect.TypeOf((*MockOIDCClientOptions)(nil).WithSkipListen)) +} + +// WithSkipPrintLoginURL mocks base method. +func (m *MockOIDCClientOptions) WithSkipPrintLoginURL() oidcclient.Option { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "WithSkipPrintLoginURL") + ret0, _ := ret[0].(oidcclient.Option) + return ret0 +} + +// WithSkipPrintLoginURL indicates an expected call of WithSkipPrintLoginURL. +func (mr *MockOIDCClientOptionsMockRecorder) WithSkipPrintLoginURL() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "WithSkipPrintLoginURL", reflect.TypeOf((*MockOIDCClientOptions)(nil).WithSkipPrintLoginURL)) +} + +// WithUpstreamIdentityProvider mocks base method. +func (m *MockOIDCClientOptions) WithUpstreamIdentityProvider(arg0, arg1 string) oidcclient.Option { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "WithUpstreamIdentityProvider", arg0, arg1) + ret0, _ := ret[0].(oidcclient.Option) + return ret0 +} + +// WithUpstreamIdentityProvider indicates an expected call of WithUpstreamIdentityProvider. +func (mr *MockOIDCClientOptionsMockRecorder) WithUpstreamIdentityProvider(arg0, arg1 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "WithUpstreamIdentityProvider", reflect.TypeOf((*MockOIDCClientOptions)(nil).WithUpstreamIdentityProvider), arg0, arg1) +} diff --git a/pkg/oidcclient/login.go b/pkg/oidcclient/login.go index d82849c2a..bcaba7e50 100644 --- a/pkg/oidcclient/login.go +++ b/pkg/oidcclient/login.go @@ -16,6 +16,7 @@ import ( "net/http" "net/url" "os" + "slices" "sort" "strings" "sync" @@ -27,8 +28,8 @@ import ( "golang.org/x/oauth2" "golang.org/x/term" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/utils/strings/slices" + idpdiscoveryv1alpha1 "go.pinniped.dev/generated/latest/apis/supervisor/idpdiscovery/v1alpha1" oidcapi "go.pinniped.dev/generated/latest/apis/supervisor/oidc" "go.pinniped.dev/internal/federationdomain/upstreamprovider" "go.pinniped.dev/internal/httputil/httperr" @@ -88,8 +89,9 @@ type handlerState struct { // Tracking the usage of some other functional options. upstreamIdentityProviderName string - upstreamIdentityProviderType string + upstreamIdentityProviderType idpdiscoveryv1alpha1.IDPType cliToSendCredentials bool + loginFlow idpdiscoveryv1alpha1.IDPFlow skipBrowser bool skipPrintLoginURL bool requestedAudience string @@ -101,6 +103,7 @@ type handlerState struct { // Generated parameters of a login flow. provider *coreosoidc.Provider + idpDiscovery *idpdiscoveryv1alpha1.IDPDiscoveryResponse oauth2Config *oauth2.Config useFormPost bool state state.State @@ -243,6 +246,9 @@ func WithRequestAudience(audience string) Option { // and by OIDCIdentityProviders which optionally enable the resource owner password credentials grant flow. // This should never be used with non-Supervisor issuers because it will send the user's password to the authorization // endpoint as a custom header, which would be ignored but could potentially get logged somewhere by the issuer. +// +// Deprecated: this option will be removed in a future version of Pinniped. See the WithLoginFlow() option instead. +// If this option is used along with the WithLoginFlow() option, it will cause an error. func WithCLISendingCredentials() Option { return func(h *handlerState) error { h.cliToSendCredentials = true @@ -250,6 +256,38 @@ func WithCLISendingCredentials() Option { } } +// WithLoginFlow chooses the login flow. +// When the argument is equal to idpdiscoveryv1alpha1.IDPFlowCLIPassword, it 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 provider type supports this style of authentication. Currently, this is +// supported by LDAPIdentityProviders, ActiveDirectoryIdentityProviders, and by OIDCIdentityProviders which optionally +// enable the resource owner password credentials grant flow. This should never be used with non-Supervisor issuers +// because it will send the user's password to the authorization endpoint as a custom header, which would be ignored but +// could potentially get logged somewhere by the issuer. +// When the argument is equal to idpdiscoveryv1alpha1.IDPFlowBrowserAuthcode, it will attempt to open a web browser +// and perform the OIDC authcode flow. +// When not used, the default when the issuer is a Pinniped Supervisor will be determined automatically, +// and the default for non-Supervisor issuers will be the browser authcode flow. +func WithLoginFlow(loginFlow idpdiscoveryv1alpha1.IDPFlow, flowSource string) Option { + return func(h *handlerState) error { + switch loginFlow { + case idpdiscoveryv1alpha1.IDPFlowCLIPassword, + idpdiscoveryv1alpha1.IDPFlowBrowserAuthcode: + default: + return fmt.Errorf( + "WithLoginFlow error: loginFlow '%s' from '%s' must be '%s' or '%s'", + loginFlow, + flowSource, + idpdiscoveryv1alpha1.IDPFlowCLIPassword, + idpdiscoveryv1alpha1.IDPFlowBrowserAuthcode, + ) + } + h.loginFlow = loginFlow + return nil + } +} + // WithUpstreamIdentityProvider causes the specified name and type to be sent as custom query parameters to the // issuer's authorize endpoint. This is only intended to be used when the issuer is a Pinniped Supervisor, in which // case it provides a mechanism to choose among several upstream identity providers. @@ -257,7 +295,10 @@ func WithCLISendingCredentials() Option { func WithUpstreamIdentityProvider(upstreamName, upstreamType string) Option { return func(h *handlerState) error { h.upstreamIdentityProviderName = upstreamName - h.upstreamIdentityProviderType = upstreamType + + // Do not perform validation on this cast. + // If possible, dynamic validation against a Pinniped Supervisor's supported IDP types will be performed. + h.upstreamIdentityProviderType = idpdiscoveryv1alpha1.IDPType(upstreamType) return nil } } @@ -304,6 +345,13 @@ func Login(issuer string, clientID string, opts ...Option) (*oidctypes.Token, er } } + if h.cliToSendCredentials { + if h.loginFlow != "" { + return nil, fmt.Errorf("do not use deprecated option WithCLISendingCredentials when using option WithLoginFlow") + } + h.loginFlow = idpdiscoveryv1alpha1.IDPFlowCLIPassword + } + // Copy the configured HTTP client to set a request timeout (the Go default client has no timeout configured). httpClientWithTimeout := *h.httpClient httpClientWithTimeout.Timeout = httpRequestTimeout @@ -426,19 +474,24 @@ func (h *handlerState) baseLogin() (*oidctypes.Token, error) { h.pkce.Challenge(), h.pkce.Method(), } - if h.upstreamIdentityProviderName != "" { - authorizeOptions = append(authorizeOptions, - oauth2.SetAuthURLParam(oidcapi.AuthorizeUpstreamIDPNameParamName, h.upstreamIdentityProviderName), - ) - authorizeOptions = append(authorizeOptions, - oauth2.SetAuthURLParam(oidcapi.AuthorizeUpstreamIDPTypeParamName, h.upstreamIdentityProviderType), - ) + + loginFlow, pinnipedSupervisorOptions, err := h.maybePerformPinnipedSupervisorValidations() + if err != nil { + return nil, err } + h.loginFlow = loginFlow + authorizeOptions = append(authorizeOptions, pinnipedSupervisorOptions...) + + // Preserve the legacy behavior where browser-based auth is preferred + authFunc := h.webBrowserBasedAuth // Choose the appropriate authorization and authcode exchange strategy. - var authFunc = h.webBrowserBasedAuth - if h.cliToSendCredentials { + // Use a switch so that lint will make sure we have full coverage. + switch h.loginFlow { + case idpdiscoveryv1alpha1.IDPFlowCLIPassword: authFunc = h.cliBasedAuth + case idpdiscoveryv1alpha1.IDPFlowBrowserAuthcode: + // NOOP } // Perform the authorize request and authcode exchange to get back OIDC tokens. @@ -452,6 +505,108 @@ func (h *handlerState) baseLogin() (*oidctypes.Token, error) { return token, err } +// maybePerformPinnipedSupervisorValidations will return the flow and some authorization options. +// When the IDP name is unset, it will assume that the server is not a Pinniped Supervisor, and will return immediately. +// Otherwise, when the flow is unset, it will infer the flow from the server, or when the flow is set, it will return that flow unchanged. +// It will also perform additional validations if the issuer is a Pinniped Supervisor. +func (h *handlerState) maybePerformPinnipedSupervisorValidations() (idpdiscoveryv1alpha1.IDPFlow, []oauth2.AuthCodeOption, error) { + loginFlow := h.loginFlow + + if h.upstreamIdentityProviderName == "" { + return loginFlow, nil, nil + } + + if h.idpDiscovery == nil { + return "", nil, fmt.Errorf("upstream identity provider name %q was specified, but OIDC issuer %q does not "+ + "offer Pinniped-style IDP discovery, so it does not appear to be a Pinniped Supervisor; "+ + "specifying an upstream identity provider name is only meant to be used with Pinniped Supervisors", + h.upstreamIdentityProviderName, h.issuer) + } + + // Legacy Pinniped Supervisors do not provide this information. Only run this validation when the information was provided. + if len(h.idpDiscovery.PinnipedSupportedIDPTypes) > 0 { + var supportedIDPTypes []idpdiscoveryv1alpha1.IDPType + for _, idpType := range h.idpDiscovery.PinnipedSupportedIDPTypes { + supportedIDPTypes = append(supportedIDPTypes, idpType.Type) + } + + // Sort by name for repeatability + slices.Sort(supportedIDPTypes) + + if !slices.Contains(supportedIDPTypes, h.upstreamIdentityProviderType) { + convertIDPListToQuotedStringList := func() []string { + var temp []string + for _, idpType := range supportedIDPTypes { + temp = append(temp, fmt.Sprintf("%q", idpType)) + } + return temp + } + return "", nil, fmt.Errorf("unable to find upstream identity provider with type %q, this Pinniped Supervisor supports IDP types [%s]", + h.upstreamIdentityProviderType, + strings.Join(convertIDPListToQuotedStringList(), ", ")) + } + } + + // Find the IDP from discovery by the specified name, type, and maybe flow. + foundIDPIndex := slices.IndexFunc(h.idpDiscovery.PinnipedIDPs, func(idp idpdiscoveryv1alpha1.PinnipedIDP) bool { + return idp.Name == h.upstreamIdentityProviderName && + idp.Type == h.upstreamIdentityProviderType && + (loginFlow == "" || slices.Contains(idp.Flows, loginFlow)) + }) + + // If the IDP was not found... + if foundIDPIndex < 0 { + pinnipedIDPsString, err := json.Marshal(h.idpDiscovery.PinnipedIDPs) + if err != nil { + // This should never happen. Not unit tested. + return "", nil, fmt.Errorf("error marshalling IDP discovery response: %w", err) + } + if loginFlow == "" { + return "", nil, fmt.Errorf( + "unable to find upstream identity provider with name %q and type %q. Found these providers: %s", + h.upstreamIdentityProviderName, + h.upstreamIdentityProviderType, + pinnipedIDPsString, + ) + } + return "", nil, fmt.Errorf( + "unable to find upstream identity provider with name %q and type %q and flow %q. Found these providers: %s", + h.upstreamIdentityProviderName, + h.upstreamIdentityProviderType, + loginFlow, + pinnipedIDPsString, + ) + } + + // If the caller has not requested a specific flow, but has requested a specific IDP, infer the authentication flow + // from the found IDP's discovery information. + if loginFlow == "" { + foundIDP := h.idpDiscovery.PinnipedIDPs[foundIDPIndex] + if len(foundIDP.Flows) == 0 { + // Note that this should not really happen because the Supervisor's IDP discovery endpoint has always listed flows. + return "", nil, fmt.Errorf("unable to infer flow for upstream identity provider with name %q and type %q "+ + "because there were no flows discovered for that provider", + h.upstreamIdentityProviderName, + h.upstreamIdentityProviderType, + ) + } + // The order of the flows returned by the server indicates the server's flow preference, + // so always use the first flow for that IDP from the discovery response. + loginFlow = foundIDP.Flows[0] + } + + var authorizeOptions []oauth2.AuthCodeOption + + authorizeOptions = append(authorizeOptions, + oauth2.SetAuthURLParam(oidcapi.AuthorizeUpstreamIDPNameParamName, h.upstreamIdentityProviderName), + ) + authorizeOptions = append(authorizeOptions, + oauth2.SetAuthURLParam(oidcapi.AuthorizeUpstreamIDPTypeParamName, string(h.upstreamIdentityProviderType)), + ) + + return loginFlow, authorizeOptions, nil +} + // Make a direct call to the authorize endpoint, including the user's username and password on custom http headers, // and parse the authcode from the response. Exchange the authcode for tokens. Return the tokens or an error. func (h *handlerState) cliBasedAuth(authorizeOptions *[]oauth2.AuthCodeOption) (*oidctypes.Token, error) { @@ -759,7 +914,7 @@ func promptForSecret(promptLabel string, out io.Writer) (string, error) { } func (h *handlerState) initOIDCDiscovery() error { - // Make this method idempotent so it can be called in multiple cases with no extra network requests. + // Make this method idempotent, so it can be called in multiple cases with no extra network requests. if h.provider != nil { return nil } @@ -803,6 +958,60 @@ func (h *handlerState) initOIDCDiscovery() error { return fmt.Errorf("could not decode response_modes_supported in OIDC discovery from %q: %w", h.issuer, err) } h.useFormPost = slices.Contains(discoveryClaims.ResponseModesSupported, "form_post") + + return h.maybePerformPinnipedSupervisorIDPDiscovery() +} + +func (h *handlerState) maybePerformPinnipedSupervisorIDPDiscovery() error { + // If this OIDC IDP is a Pinniped Supervisor, it will have a reference to the IDP discovery document. + // Go to that document and retrieve the IDPs. + var pinnipedSupervisorClaims idpdiscoveryv1alpha1.OIDCDiscoveryResponse + if err := h.provider.Claims(&pinnipedSupervisorClaims); err != nil { + return fmt.Errorf("could not decode the Pinniped IDP discovery document URL in OIDC discovery from %q: %w", h.issuer, err) + } + + // This is not an error - it just means that this issuer is not a Pinniped Supervisor. + // Note that this package can be used with OIDC IDPs other than Pinniped Supervisor. + if pinnipedSupervisorClaims.SupervisorDiscovery.PinnipedIDPsEndpoint == "" { + return nil + } + // This check confirms that the issuer is hosting the IDP discovery document, which would always be the case for + // Pinniped Supervisor. Since there are checks above to confirm that the issuer uses HTTPS, IDP discovery will + // always use HTTPS. + if !strings.HasPrefix(pinnipedSupervisorClaims.SupervisorDiscovery.PinnipedIDPsEndpoint, h.issuer) { + return fmt.Errorf("the Pinniped IDP discovery document must always be hosted by the issuer: %q", h.issuer) + } + + idpDiscoveryCtx, idpDiscoveryCtxCancelFunc := context.WithTimeout(h.ctx, httpRequestTimeout) + defer idpDiscoveryCtxCancelFunc() + idpDiscoveryReq, err := http.NewRequestWithContext(idpDiscoveryCtx, http.MethodGet, pinnipedSupervisorClaims.SupervisorDiscovery.PinnipedIDPsEndpoint, nil) + if err != nil { // untested + return fmt.Errorf("could not build IDP Discovery request: %w", err) + } + idpDiscoveryRes, err := h.httpClient.Do(idpDiscoveryReq) + if err != nil { + return fmt.Errorf("IDP Discovery response error: %w", err) + } + defer func() { + _ = idpDiscoveryRes.Body.Close() // We can't do anything if this fails to close + }() + + if idpDiscoveryRes.StatusCode != http.StatusOK { + return fmt.Errorf("unable to fetch IDP discovery data from issuer: unexpected http response status: %s", idpDiscoveryRes.Status) + } + + rawBody, err := io.ReadAll(idpDiscoveryRes.Body) + if err != nil { // untested + return fmt.Errorf("unable to fetch IDP discovery data from issuer: could not read response body: %w", err) + } + + var body idpdiscoveryv1alpha1.IDPDiscoveryResponse + err = json.Unmarshal(rawBody, &body) + if err != nil { + return fmt.Errorf("unable to fetch the Pinniped IDP discovery document: could not parse response JSON: %w", err) + } + + h.idpDiscovery = &body return nil } diff --git a/pkg/oidcclient/login_test.go b/pkg/oidcclient/login_test.go index 7cfded622..5243c3520 100644 --- a/pkg/oidcclient/login_test.go +++ b/pkg/oidcclient/login_test.go @@ -21,7 +21,7 @@ import ( "testing" "time" - "github.com/coreos/go-oidc/v3/oidc" + coreosoidc "github.com/coreos/go-oidc/v3/oidc" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" @@ -29,6 +29,10 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/klog/v2" + idpdiscoveryv1alpha1 "go.pinniped.dev/generated/latest/apis/supervisor/idpdiscovery/v1alpha1" + oidcapi "go.pinniped.dev/generated/latest/apis/supervisor/oidc" + "go.pinniped.dev/internal/federationdomain/endpoints/discovery" + federationdomainoidc "go.pinniped.dev/internal/federationdomain/oidc" "go.pinniped.dev/internal/federationdomain/upstreamprovider" "go.pinniped.dev/internal/httputil/httperr" "go.pinniped.dev/internal/httputil/roundtripper" @@ -189,6 +193,70 @@ func TestLogin(t *testing.T) { //nolint:gocyclo }) }) + // Start a test server that returns IDP discovery at some other location + emptyIDPDiscoveryMux := http.NewServeMux() + emptyIDPDiscoveryServer, emptyIDPDiscoveryServerCA := tlsserver.TestServerIPv4(t, emptyIDPDiscoveryMux, nil) + emptyIDPDiscoveryMux.HandleFunc("/.well-known/openid-configuration", func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet { + http.Error(w, "unexpected method", http.StatusMethodNotAllowed) + return + } + w.Header().Set("content-type", "application/json") + _ = json.NewEncoder(w).Encode(&struct { + Issuer string `json:"issuer"` + AuthURL string `json:"authorization_endpoint"` + TokenURL string `json:"token_endpoint"` + JWKSURL string `json:"jwks_uri"` + ResponseModesSupported []string `json:"response_modes_supported,omitempty"` + SupervisorDiscovery idpdiscoveryv1alpha1.OIDCDiscoveryResponseIDPEndpoint `json:"discovery.supervisor.pinniped.dev/v1alpha1"` + }{ + Issuer: emptyIDPDiscoveryServer.URL, + AuthURL: emptyIDPDiscoveryServer.URL + "/authorize", + TokenURL: emptyIDPDiscoveryServer.URL + "/token", + JWKSURL: emptyIDPDiscoveryServer.URL + "/keys", + ResponseModesSupported: []string{}, + SupervisorDiscovery: idpdiscoveryv1alpha1.OIDCDiscoveryResponseIDPEndpoint{ + PinnipedIDPsEndpoint: "https://example.com" + federationdomainoidc.PinnipedIDPsPathV1Alpha1, + }, + }) + }) + + // Start a test server that has invalid IDP discovery + invalidIDPDiscoveryMux := http.NewServeMux() + invalidIDPDiscoveryServer, invalidIDPDiscoveryServerCA := tlsserver.TestServerIPv4(t, invalidIDPDiscoveryMux, nil) + invalidIDPDiscoveryMux.HandleFunc("/.well-known/openid-configuration", func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet { + http.Error(w, "unexpected method", http.StatusMethodNotAllowed) + return + } + w.Header().Set("content-type", "application/json") + _ = json.NewEncoder(w).Encode(&struct { + Issuer string `json:"issuer"` + AuthURL string `json:"authorization_endpoint"` + TokenURL string `json:"token_endpoint"` + JWKSURL string `json:"jwks_uri"` + ResponseModesSupported []string `json:"response_modes_supported,omitempty"` + SupervisorDiscovery idpdiscoveryv1alpha1.OIDCDiscoveryResponseIDPEndpoint `json:"discovery.supervisor.pinniped.dev/v1alpha1"` + }{ + Issuer: invalidIDPDiscoveryServer.URL, + AuthURL: invalidIDPDiscoveryServer.URL + "/authorize", + TokenURL: invalidIDPDiscoveryServer.URL + "/token", + JWKSURL: invalidIDPDiscoveryServer.URL + "/keys", + ResponseModesSupported: []string{}, + SupervisorDiscovery: idpdiscoveryv1alpha1.OIDCDiscoveryResponseIDPEndpoint{ + PinnipedIDPsEndpoint: invalidIDPDiscoveryServer.URL + federationdomainoidc.PinnipedIDPsPathV1Alpha1, + }, + }) + }) + invalidIDPDiscoveryMux.HandleFunc(federationdomainoidc.PinnipedIDPsPathV1Alpha1, func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet { + http.Error(w, "unexpected method", http.StatusMethodNotAllowed) + return + } + w.Header().Set("content-type", "application/json") + _, _ = fmt.Fprint(w, "not real json") + }) + discoveryHandler := func(server *httptest.Server, responseModes []string) http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { if r.Method != http.MethodGet { @@ -197,17 +265,55 @@ func TestLogin(t *testing.T) { //nolint:gocyclo } w.Header().Set("content-type", "application/json") _ = json.NewEncoder(w).Encode(&struct { - Issuer string `json:"issuer"` - AuthURL string `json:"authorization_endpoint"` - TokenURL string `json:"token_endpoint"` - JWKSURL string `json:"jwks_uri"` - ResponseModesSupported []string `json:"response_modes_supported,omitempty"` + Issuer string `json:"issuer"` + AuthURL string `json:"authorization_endpoint"` + TokenURL string `json:"token_endpoint"` + JWKSURL string `json:"jwks_uri"` + ResponseModesSupported []string `json:"response_modes_supported,omitempty"` + SupervisorDiscovery idpdiscoveryv1alpha1.OIDCDiscoveryResponseIDPEndpoint `json:"discovery.supervisor.pinniped.dev/v1alpha1"` }{ Issuer: server.URL, AuthURL: server.URL + "/authorize", TokenURL: server.URL + "/token", JWKSURL: server.URL + "/keys", ResponseModesSupported: responseModes, + SupervisorDiscovery: idpdiscoveryv1alpha1.OIDCDiscoveryResponseIDPEndpoint{ + PinnipedIDPsEndpoint: server.URL + federationdomainoidc.PinnipedIDPsPathV1Alpha1, + }, + }) + } + } + + idpDiscoveryHandler := func(server *httptest.Server) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodGet { + http.Error(w, "unexpected method", http.StatusMethodNotAllowed) + return + } + w.Header().Set("content-type", "application/json") + _ = json.NewEncoder(w).Encode(idpdiscoveryv1alpha1.IDPDiscoveryResponse{ + PinnipedIDPs: []idpdiscoveryv1alpha1.PinnipedIDP{ + { + Name: "upstream-idp-name-with-cli-password-flow-first", + Type: "upstream-idp-type-with-cli-password-flow-first", + Flows: []idpdiscoveryv1alpha1.IDPFlow{ + idpdiscoveryv1alpha1.IDPFlowCLIPassword, + idpdiscoveryv1alpha1.IDPFlowBrowserAuthcode, + }, + }, + { + Name: "upstream-idp-name-with-browser-authcode-flow-first", + Type: "upstream-idp-type-with-browser-authcode-flow-first", + Flows: []idpdiscoveryv1alpha1.IDPFlow{ + idpdiscoveryv1alpha1.IDPFlowBrowserAuthcode, + idpdiscoveryv1alpha1.IDPFlowCLIPassword, + }, + }, + }, + PinnipedSupportedIDPTypes: []idpdiscoveryv1alpha1.PinnipedSupportedIDPType{ + {Type: "upstream-idp-type-with-cli-password-flow-first"}, + {Type: "upstream-idp-type-with-browser-authcode-flow-first"}, + }, }) } } @@ -299,14 +405,18 @@ func TestLogin(t *testing.T) { //nolint:gocyclo providerMux := http.NewServeMux() successServer, successServerCA := tlsserver.TestServerIPv4(t, providerMux, nil) providerMux.HandleFunc("/.well-known/openid-configuration", discoveryHandler(successServer, nil)) + providerMux.HandleFunc(federationdomainoidc.PinnipedIDPsPathV1Alpha1, idpDiscoveryHandler(successServer)) providerMux.HandleFunc("/token", tokenHandler) // Start a test server that returns a real discovery document and answers refresh requests, _and_ supports form_mode=post. formPostProviderMux := http.NewServeMux() formPostSuccessServer, formPostSuccessServerCA := tlsserver.TestServerIPv4(t, formPostProviderMux, nil) formPostProviderMux.HandleFunc("/.well-known/openid-configuration", discoveryHandler(formPostSuccessServer, []string{"query", "form_post"})) + formPostProviderMux.HandleFunc(federationdomainoidc.PinnipedIDPsPathV1Alpha1, idpDiscoveryHandler(formPostSuccessServer)) formPostProviderMux.HandleFunc("/token", tokenHandler) + // TODO: Use a test server without federationdomainoidc.PinnipedIDPsPathV1Alpha1 (e.g. a non-Supervisor server) + defaultDiscoveryResponse := func(req *http.Request) (*http.Response, error) { // Call the handler function from the test server to calculate the response. handler, _ := providerMux.Handler(req) @@ -330,14 +440,14 @@ func TestLogin(t *testing.T) { //nolint:gocyclo ClientID: "test-client-id", Scopes: []string{"test-scope"}, RedirectURI: "http://localhost:0/callback", - UpstreamProviderName: "some-upstream-name", + UpstreamProviderName: "upstream-idp-name-with-cli-password-flow-first", } t.Cleanup(func() { require.Equal(t, []SessionCacheKey{cacheKey}, cache.sawGetKeys) }) require.NoError(t, WithSessionCache(cache)(h)) - require.NoError(t, WithCLISendingCredentials()(h)) - require.NoError(t, WithUpstreamIdentityProvider("some-upstream-name", "ldap")(h)) + require.NoError(t, WithLoginFlow(idpdiscoveryv1alpha1.IDPFlowCLIPassword, "flowSource")(h)) + require.NoError(t, WithUpstreamIdentityProvider("upstream-idp-name-with-cli-password-flow-first", "upstream-idp-type-with-cli-password-flow-first")(h)) require.NoError(t, WithClient(buildHTTPClientForPEM(successServerCA))(h)) require.NoError(t, WithClient(&http.Client{ @@ -345,6 +455,8 @@ func TestLogin(t *testing.T) { //nolint:gocyclo switch req.URL.Scheme + "://" + req.URL.Host + req.URL.Path { case "https://" + successServer.Listener.Addr().String() + "/.well-known/openid-configuration": return defaultDiscoveryResponse(req) + case "https://" + successServer.Listener.Addr().String() + federationdomainoidc.PinnipedIDPsPathV1Alpha1: + return defaultDiscoveryResponse(req) case "https://" + successServer.Listener.Addr().String() + "/authorize": return authResponse, authError default: @@ -375,6 +487,42 @@ func TestLogin(t *testing.T) { //nolint:gocyclo }, wantErr: "some option error", }, + { + name: "WithLoginFlow option and deprecated WithCLISendingCredentials option cannot be used together (with CLI flow selected)", + opt: func(t *testing.T) Option { + return func(h *handlerState) error { + require.NoError(t, WithCLISendingCredentials()(h)) // This is meant to call a deprecated function + require.NoError(t, WithLoginFlow(idpdiscoveryv1alpha1.IDPFlowCLIPassword, "flowSource")(h)) + return nil + } + }, + wantErr: "do not use deprecated option WithCLISendingCredentials when using option WithLoginFlow", + }, + { + name: "WithLoginFlow option and deprecated WithCLISendingCredentials option cannot be used together (with browser flow selected)", + opt: func(t *testing.T) Option { + return func(h *handlerState) error { + require.NoError(t, WithCLISendingCredentials()(h)) // This is meant to call a deprecated function + require.NoError(t, WithLoginFlow(idpdiscoveryv1alpha1.IDPFlowBrowserAuthcode, "flowSource")(h)) + return nil + } + }, + wantErr: "do not use deprecated option WithCLISendingCredentials when using option WithLoginFlow", + }, + { + name: "WithLoginFlow option rejects a non-enum value", + opt: func(t *testing.T) Option { + return WithLoginFlow("this is not one of the enum values", "some-flow-source") + }, + wantErr: "WithLoginFlow error: loginFlow 'this is not one of the enum values' from 'some-flow-source' must be 'cli_password' or 'browser_authcode'", + }, + { + name: "WithLoginFlow option will not accept empty string either", + opt: func(t *testing.T) Option { + return WithLoginFlow("", "other-flow-source") + }, + wantErr: "WithLoginFlow error: loginFlow '' from 'other-flow-source' must be 'cli_password' or 'browser_authcode'", + }, { name: "error generating state", opt: func(t *testing.T) Option { @@ -512,7 +660,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo return func(h *handlerState) error { require.NoError(t, WithClient(buildHTTPClientForPEM(successServerCA))(h)) - h.getProvider = func(config *oauth2.Config, provider *oidc.Provider, client *http.Client) upstreamprovider.UpstreamOIDCIdentityProviderI { + h.getProvider = func(config *oauth2.Config, provider *coreosoidc.Provider, client *http.Client) upstreamprovider.UpstreamOIDCIdentityProviderI { mock := mockUpstream(t) mock.EXPECT(). ValidateTokenAndMergeWithUserInfo(gomock.Any(), HasAccessToken(testToken.AccessToken.Token), nonce.Nonce(""), true, false). @@ -563,7 +711,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo return func(h *handlerState) error { require.NoError(t, WithClient(buildHTTPClientForPEM(successServerCA))(h)) - h.getProvider = func(config *oauth2.Config, provider *oidc.Provider, client *http.Client) upstreamprovider.UpstreamOIDCIdentityProviderI { + h.getProvider = func(config *oauth2.Config, provider *coreosoidc.Provider, client *http.Client) upstreamprovider.UpstreamOIDCIdentityProviderI { mock := mockUpstream(t) mock.EXPECT(). ValidateTokenAndMergeWithUserInfo(gomock.Any(), HasAccessToken(testToken.AccessToken.Token), nonce.Nonce(""), true, false). @@ -681,6 +829,30 @@ func TestLogin(t *testing.T) { //nolint:gocyclo wantLogs: []string{`"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + insecureAuthURLServer.URL + `"`}, wantErr: `discovered authorize URL from issuer must be an https URL, but had scheme "http" instead`, }, + { + name: "issuer has Pinniped Supervisor's IDP discovery, but from another location", + opt: func(t *testing.T) Option { + return func(h *handlerState) error { + require.NoError(t, WithClient(buildHTTPClientForPEM(emptyIDPDiscoveryServerCA))(h)) + return nil + } + }, + issuer: emptyIDPDiscoveryServer.URL, + wantLogs: []string{`"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + emptyIDPDiscoveryServer.URL + `"`}, + wantErr: fmt.Sprintf(`the Pinniped IDP discovery document must always be hosted by the issuer: %q`, emptyIDPDiscoveryServer.URL), + }, + { + name: "issuer has Pinniped Supervisor's IDP discovery, but it cannot be unmarshaled", + opt: func(t *testing.T) Option { + return func(h *handlerState) error { + require.NoError(t, WithClient(buildHTTPClientForPEM(invalidIDPDiscoveryServerCA))(h)) + return nil + } + }, + issuer: invalidIDPDiscoveryServer.URL, + wantLogs: []string{`"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + invalidIDPDiscoveryServer.URL + `"`}, + wantErr: "unable to fetch the Pinniped IDP discovery document: could not parse response JSON: invalid character 'o' in literal null (expecting 'u')", + }, { name: "listen failure and non-tty stdin", opt: func(t *testing.T) Option { @@ -1219,6 +1391,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo wantToken: &testToken, }, { + // TODO: This test name says that "upstream type" is included in the session cache key but I don't see that below. name: "upstream name and type are included in authorize request and session cache key if upstream name is provided", clientID: "test-client-id", opt: func(t *testing.T) Option { @@ -1235,7 +1408,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo ClientID: "test-client-id", Scopes: []string{"test-scope"}, RedirectURI: "http://localhost:0/callback", - UpstreamProviderName: "some-upstream-name", + UpstreamProviderName: "upstream-idp-name-with-browser-authcode-flow-first", } t.Cleanup(func() { require.Equal(t, []SessionCacheKey{cacheKey}, cache.sawGetKeys) @@ -1243,7 +1416,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo require.Equal(t, []*oidctypes.Token{&testToken}, cache.sawPutTokens) }) require.NoError(t, WithSessionCache(cache)(h)) - require.NoError(t, WithUpstreamIdentityProvider("some-upstream-name", "oidc")(h)) + require.NoError(t, WithUpstreamIdentityProvider("upstream-idp-name-with-browser-authcode-flow-first", "upstream-idp-type-with-browser-authcode-flow-first")(h)) client := buildHTTPClientForPEM(successServerCA) client.Timeout = 10 * time.Second @@ -1267,8 +1440,8 @@ func TestLogin(t *testing.T) { //nolint:gocyclo "state": []string{"test-state"}, "access_type": []string{"offline"}, "client_id": []string{"test-client-id"}, - "pinniped_idp_name": []string{"some-upstream-name"}, - "pinniped_idp_type": []string{"oidc"}, + "pinniped_idp_name": []string{"upstream-idp-name-with-browser-authcode-flow-first"}, + "pinniped_idp_type": []string{"upstream-idp-type-with-browser-authcode-flow-first"}, }, actualParams) parsedActualURL.RawQuery = "" @@ -1289,7 +1462,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo regexp.QuoteMeta(" https://127.0.0.1:") + "[0-9]+" + // random port regexp.QuoteMeta("/authorize?access_type=offline&client_id=test-client-id&code_challenge="+testCodeChallenge+ - "&code_challenge_method=S256&nonce=test-nonce&pinniped_idp_name=some-upstream-name&pinniped_idp_type=oidc"+ + "&code_challenge_method=S256&nonce=test-nonce&pinniped_idp_name=upstream-idp-name-with-browser-authcode-flow-first&pinniped_idp_type=upstream-idp-type-with-browser-authcode-flow-first"+ "&redirect_uri=http%3A%2F%2F127.0.0.1%3A") + "[0-9]+" + // random port regexp.QuoteMeta("%2Fcallback&response_type=code&scope=test-scope&state=test-state") + @@ -1312,7 +1485,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo }, issuer: successServer.URL, wantLogs: []string{`"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + successServer.URL + `"`}, - wantStdErr: "^\nLog in to some-upstream-name\n\n$", + wantStdErr: "^\nLog in to upstream-idp-name-with-cli-password-flow-first\n\n$", wantErr: "error prompting for username: some prompt error", }, { @@ -1327,7 +1500,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo }, issuer: successServer.URL, wantLogs: []string{`"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + successServer.URL + `"`}, - wantStdErr: "^\nLog in to some-upstream-name\n\n$", + wantStdErr: "^\nLog in to upstream-idp-name-with-cli-password-flow-first\n\n$", wantErr: "error prompting for password: some prompt error", }, { @@ -1382,11 +1555,11 @@ func TestLogin(t *testing.T) { //nolint:gocyclo }, issuer: successServer.URL, wantLogs: []string{`"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + successServer.URL + `"`}, - wantStdErr: "^\nLog in to some-upstream-name\n\n$", + wantStdErr: "^\nLog in to upstream-idp-name-with-cli-password-flow-first\n\n$", wantErr: `authorization response error: Get "https://` + successServer.Listener.Addr().String() + `/authorize?access_type=offline&client_id=test-client-id&code_challenge=` + testCodeChallenge + - `&code_challenge_method=S256&nonce=test-nonce&pinniped_idp_name=some-upstream-name&` + - `pinniped_idp_type=ldap&redirect_uri=http%3A%2F%2F127.0.0.1%3A0%2Fcallback&response_type=code` + + `&code_challenge_method=S256&nonce=test-nonce&pinniped_idp_name=upstream-idp-name-with-cli-password-flow-first&` + + `pinniped_idp_type=upstream-idp-type-with-cli-password-flow-first&redirect_uri=http%3A%2F%2F127.0.0.1%3A0%2Fcallback&response_type=code` + `&scope=test-scope&state=test-state": some error fetching authorize endpoint`, }, { @@ -1399,7 +1572,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo }, issuer: successServer.URL, wantLogs: []string{`"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + successServer.URL + `"`}, - wantStdErr: "^\nLog in to some-upstream-name\n\n$", + wantStdErr: "^\nLog in to upstream-idp-name-with-cli-password-flow-first\n\n$", wantErr: `error getting authorization: expected to be redirected, but response status was 502 Bad Gateway`, }, { @@ -1417,7 +1590,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo }, issuer: successServer.URL, wantLogs: []string{`"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + successServer.URL + `"`}, - wantStdErr: "^\nLog in to some-upstream-name\n\n$", + wantStdErr: "^\nLog in to upstream-idp-name-with-cli-password-flow-first\n\n$", wantErr: `login failed with code "access_denied": optional-error-description`, }, { @@ -1435,7 +1608,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo }, issuer: successServer.URL, wantLogs: []string{`"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + successServer.URL + `"`}, - wantStdErr: "^\nLog in to some-upstream-name\n\n$", + wantStdErr: "^\nLog in to upstream-idp-name-with-cli-password-flow-first\n\n$", wantErr: `error getting authorization: redirected to the wrong location: http://other-server.example.com/callback?code=foo&state=test-state`, }, { @@ -1453,7 +1626,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo }, issuer: successServer.URL, wantLogs: []string{`"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + successServer.URL + `"`}, - wantStdErr: "^\nLog in to some-upstream-name\n\n$", + wantStdErr: "^\nLog in to upstream-idp-name-with-cli-password-flow-first\n\n$", wantErr: `login failed with code "access_denied"`, }, { @@ -1469,7 +1642,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo }, issuer: successServer.URL, wantLogs: []string{`"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + successServer.URL + `"`}, - wantStdErr: "^\nLog in to some-upstream-name\n\n$", + wantStdErr: "^\nLog in to upstream-idp-name-with-cli-password-flow-first\n\n$", wantErr: `missing or invalid state parameter in authorization response: http://127.0.0.1:0/callback?code=foo&state=wrong-state`, }, { @@ -1484,7 +1657,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo fmt.Sprintf("http://127.0.0.1:0/callback?code=%s&state=test-state", fakeAuthCode), }}, }, nil) - h.getProvider = func(_ *oauth2.Config, _ *oidc.Provider, _ *http.Client) upstreamprovider.UpstreamOIDCIdentityProviderI { + h.getProvider = func(_ *oauth2.Config, _ *coreosoidc.Provider, _ *http.Client) upstreamprovider.UpstreamOIDCIdentityProviderI { mock := mockUpstream(t) mock.EXPECT(). ExchangeAuthcodeAndValidateTokens( @@ -1497,7 +1670,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo }, issuer: successServer.URL, wantLogs: []string{`"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + successServer.URL + `"`}, - wantStdErr: "^\nLog in to some-upstream-name\n\n$", + wantStdErr: "^\nLog in to upstream-idp-name-with-cli-password-flow-first\n\n$", wantErr: "could not complete authorization code exchange: some authcode exchange or token validation error", }, { @@ -1507,7 +1680,276 @@ func TestLogin(t *testing.T) { //nolint:gocyclo return func(h *handlerState) error { fakeAuthCode := "test-authcode-value" - h.getProvider = func(_ *oauth2.Config, _ *oidc.Provider, _ *http.Client) upstreamprovider.UpstreamOIDCIdentityProviderI { + h.getProvider = func(_ *oauth2.Config, _ *coreosoidc.Provider, _ *http.Client) upstreamprovider.UpstreamOIDCIdentityProviderI { + mock := mockUpstream(t) + mock.EXPECT(). + ExchangeAuthcodeAndValidateTokens( + gomock.Any(), fakeAuthCode, pkce.Code("test-pkce"), nonce.Nonce("test-nonce"), "http://127.0.0.1:0/callback"). + Return(&testToken, nil) + return mock + } + + h.generateState = func() (state.State, error) { return "test-state", nil } + h.generatePKCE = func() (pkce.Code, error) { return "test-pkce", nil } + h.generateNonce = func() (nonce.Nonce, error) { return "test-nonce", nil } + h.getEnv = func(_ string) string { + return "" // asking for any env var returns empty as if it were unset + } + h.promptForValue = func(_ context.Context, promptLabel string, _ io.Writer) (string, error) { + require.Equal(t, "Username: ", promptLabel) + return "some-upstream-username", nil + } + h.promptForSecret = func(promptLabel string, _ io.Writer) (string, error) { + require.Equal(t, "Password: ", promptLabel) + return "some-upstream-password", nil + } + + cache := &mockSessionCache{t: t, getReturnsToken: nil} + cacheKey := SessionCacheKey{ + Issuer: successServer.URL, + ClientID: "test-client-id", + Scopes: []string{"test-scope"}, + RedirectURI: "http://localhost:0/callback", + UpstreamProviderName: "upstream-idp-name-with-cli-password-flow-first", + } + t.Cleanup(func() { + require.Equal(t, []SessionCacheKey{cacheKey}, cache.sawGetKeys) + require.Equal(t, []SessionCacheKey{cacheKey}, cache.sawPutKeys) + require.Equal(t, []*oidctypes.Token{&testToken}, cache.sawPutTokens) + }) + require.NoError(t, WithSessionCache(cache)(h)) + require.NoError(t, WithLoginFlow(idpdiscoveryv1alpha1.IDPFlowCLIPassword, "flowSource")(h)) + require.NoError(t, WithUpstreamIdentityProvider("upstream-idp-name-with-cli-password-flow-first", "upstream-idp-type-with-cli-password-flow-first")(h)) + + discoveryRequestWasMade := false + authorizeRequestWasMade := false + t.Cleanup(func() { + require.True(t, discoveryRequestWasMade, "should have made an discovery request") + require.True(t, authorizeRequestWasMade, "should have made an authorize request") + }) + + client := buildHTTPClientForPEM(successServerCA) + client.Transport = roundtripper.Func(func(req *http.Request) (*http.Response, error) { + switch req.URL.Scheme + "://" + req.URL.Host + req.URL.Path { + case "https://" + successServer.Listener.Addr().String() + "/.well-known/openid-configuration": + discoveryRequestWasMade = true + return defaultDiscoveryResponse(req) + case "https://" + successServer.Listener.Addr().String() + federationdomainoidc.PinnipedIDPsPathV1Alpha1: + return defaultDiscoveryResponse(req) + case "https://" + successServer.Listener.Addr().String() + "/authorize": + authorizeRequestWasMade = true + require.Equal(t, "some-upstream-username", req.Header.Get("Pinniped-Username")) + require.Equal(t, "some-upstream-password", req.Header.Get("Pinniped-Password")) + require.Equal(t, url.Values{ + "code_challenge": []string{testCodeChallenge}, + "code_challenge_method": []string{"S256"}, + "response_type": []string{"code"}, + "scope": []string{"test-scope"}, + "nonce": []string{"test-nonce"}, + "state": []string{"test-state"}, + "access_type": []string{"offline"}, + "client_id": []string{"test-client-id"}, + "redirect_uri": []string{"http://127.0.0.1:0/callback"}, + "pinniped_idp_name": []string{"upstream-idp-name-with-cli-password-flow-first"}, + "pinniped_idp_type": []string{"upstream-idp-type-with-cli-password-flow-first"}, + }, req.URL.Query()) + return &http.Response{ + StatusCode: http.StatusFound, + Header: http.Header{"Location": []string{ + fmt.Sprintf("http://127.0.0.1:0/callback?code=%s&state=test-state", fakeAuthCode), + }}, + }, nil + default: + // Note that "/token" requests should not be made. They are mocked by mocking calls to ExchangeAuthcodeAndValidateTokens(). + require.FailNow(t, fmt.Sprintf("saw unexpected http call from the CLI: %s", req.URL.String())) + return nil, nil + } + }) + require.NoError(t, WithClient(client)(h)) + return nil + } + }, + issuer: successServer.URL, + wantLogs: []string{`"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + successServer.URL + `"`}, + wantStdErr: "^\nLog in to upstream-idp-name-with-cli-password-flow-first\n\n$", + wantToken: &testToken, + }, + { + name: "unable to login with unknown IDP, when Supervisor provides its supported IDP types", + clientID: "test-client-id", + opt: func(t *testing.T) Option { + return func(h *handlerState) error { + h.generateState = func() (state.State, error) { return "test-state", nil } + h.generatePKCE = func() (pkce.Code, error) { return "test-pkce", nil } + h.generateNonce = func() (nonce.Nonce, error) { return "test-nonce", nil } + h.getEnv = func(_ string) string { + return "" // asking for any env var returns empty as if it were unset + } + h.promptForValue = func(_ context.Context, promptLabel string, _ io.Writer) (string, error) { + require.Equal(t, "Username: ", promptLabel) + return "some-upstream-username", nil + } + h.promptForSecret = func(promptLabel string, _ io.Writer) (string, error) { + require.Equal(t, "Password: ", promptLabel) + return "some-upstream-password", nil + } + + cache := &mockSessionCache{t: t, getReturnsToken: nil} + cacheKey := SessionCacheKey{ + Issuer: successServer.URL, + ClientID: "test-client-id", + Scopes: []string{"test-scope"}, + RedirectURI: "http://localhost:0/callback", + UpstreamProviderName: "upstream-idp-name-with-cli-password-flow-first", + } + t.Cleanup(func() { + require.Equal(t, []SessionCacheKey{cacheKey}, cache.sawGetKeys) + require.Equal(t, []SessionCacheKey(nil), cache.sawPutKeys) + require.Equal(t, []*oidctypes.Token(nil), cache.sawPutTokens) + }) + require.NoError(t, WithSessionCache(cache)(h)) + require.NoError(t, WithLoginFlow(idpdiscoveryv1alpha1.IDPFlowCLIPassword, "flowSource")(h)) + require.NoError(t, WithUpstreamIdentityProvider("upstream-idp-name-with-cli-password-flow-first", "INVALID UPSTREAM TYPE")(h)) + + discoveryRequestWasMade := false + idpDiscoveryRequestWasMade := false + t.Cleanup(func() { + require.True(t, discoveryRequestWasMade, "should have made an discovery request") + require.True(t, idpDiscoveryRequestWasMade, "should have made an discovery request") + }) + + client := buildHTTPClientForPEM(successServerCA) + client.Transport = roundtripper.Func(func(req *http.Request) (*http.Response, error) { + switch req.URL.Scheme + "://" + req.URL.Host + req.URL.Path { + case "https://" + successServer.Listener.Addr().String() + "/.well-known/openid-configuration": + discoveryRequestWasMade = true + return defaultDiscoveryResponse(req) + case "https://" + successServer.Listener.Addr().String() + federationdomainoidc.PinnipedIDPsPathV1Alpha1: + idpDiscoveryRequestWasMade = true + return defaultDiscoveryResponse(req) + default: + // Note that "/token" requests should not be made. They are mocked by mocking calls to ExchangeAuthcodeAndValidateTokens(). + require.FailNow(t, fmt.Sprintf("saw unexpected http call from the CLI: %s", req.URL.String())) + return nil, nil + } + }) + require.NoError(t, WithClient(client)(h)) + return nil + } + }, + issuer: successServer.URL, + wantLogs: []string{`"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + successServer.URL + `"`}, + wantErr: `unable to find upstream identity provider with type "INVALID UPSTREAM TYPE", this Pinniped Supervisor supports IDP types ["upstream-idp-type-with-browser-authcode-flow-first", "upstream-idp-type-with-cli-password-flow-first"]`, + }, + { + name: "successful ldap login with prompts for username and password, using deprecated WithCLISendingCredentials option", + clientID: "test-client-id", + opt: func(t *testing.T) Option { + return func(h *handlerState) error { + fakeAuthCode := "test-authcode-value" + + h.getProvider = func(_ *oauth2.Config, _ *coreosoidc.Provider, _ *http.Client) upstreamprovider.UpstreamOIDCIdentityProviderI { + mock := mockUpstream(t) + mock.EXPECT(). + ExchangeAuthcodeAndValidateTokens( + gomock.Any(), fakeAuthCode, pkce.Code("test-pkce"), nonce.Nonce("test-nonce"), "http://127.0.0.1:0/callback"). + Return(&testToken, nil) + return mock + } + + h.generateState = func() (state.State, error) { return "test-state", nil } + h.generatePKCE = func() (pkce.Code, error) { return "test-pkce", nil } + h.generateNonce = func() (nonce.Nonce, error) { return "test-nonce", nil } + h.getEnv = func(_ string) string { + return "" // asking for any env var returns empty as if it were unset + } + h.promptForValue = func(_ context.Context, promptLabel string, _ io.Writer) (string, error) { + require.Equal(t, "Username: ", promptLabel) + return "some-upstream-username", nil + } + h.promptForSecret = func(promptLabel string, _ io.Writer) (string, error) { + require.Equal(t, "Password: ", promptLabel) + return "some-upstream-password", nil + } + + cache := &mockSessionCache{t: t, getReturnsToken: nil} + cacheKey := SessionCacheKey{ + Issuer: successServer.URL, + ClientID: "test-client-id", + Scopes: []string{"test-scope"}, + RedirectURI: "http://localhost:0/callback", + UpstreamProviderName: "upstream-idp-name-with-cli-password-flow-first", + } + t.Cleanup(func() { + require.Equal(t, []SessionCacheKey{cacheKey}, cache.sawGetKeys) + require.Equal(t, []SessionCacheKey{cacheKey}, cache.sawPutKeys) + require.Equal(t, []*oidctypes.Token{&testToken}, cache.sawPutTokens) + }) + require.NoError(t, WithSessionCache(cache)(h)) + require.NoError(t, WithCLISendingCredentials()(h)) // This is meant to call a deprecated function + require.NoError(t, WithUpstreamIdentityProvider("upstream-idp-name-with-cli-password-flow-first", "upstream-idp-type-with-cli-password-flow-first")(h)) + + discoveryRequestWasMade := false + authorizeRequestWasMade := false + t.Cleanup(func() { + require.True(t, discoveryRequestWasMade, "should have made an discovery request") + require.True(t, authorizeRequestWasMade, "should have made an authorize request") + }) + + client := buildHTTPClientForPEM(successServerCA) + client.Transport = roundtripper.Func(func(req *http.Request) (*http.Response, error) { + switch req.URL.Scheme + "://" + req.URL.Host + req.URL.Path { + case "https://" + successServer.Listener.Addr().String() + "/.well-known/openid-configuration": + discoveryRequestWasMade = true + return defaultDiscoveryResponse(req) + case "https://" + successServer.Listener.Addr().String() + federationdomainoidc.PinnipedIDPsPathV1Alpha1: + return defaultDiscoveryResponse(req) + case "https://" + successServer.Listener.Addr().String() + "/authorize": + authorizeRequestWasMade = true + require.Equal(t, "some-upstream-username", req.Header.Get("Pinniped-Username")) + require.Equal(t, "some-upstream-password", req.Header.Get("Pinniped-Password")) + require.Equal(t, url.Values{ + "code_challenge": []string{testCodeChallenge}, + "code_challenge_method": []string{"S256"}, + "response_type": []string{"code"}, + "scope": []string{"test-scope"}, + "nonce": []string{"test-nonce"}, + "state": []string{"test-state"}, + "access_type": []string{"offline"}, + "client_id": []string{"test-client-id"}, + "redirect_uri": []string{"http://127.0.0.1:0/callback"}, + "pinniped_idp_name": []string{"upstream-idp-name-with-cli-password-flow-first"}, + "pinniped_idp_type": []string{"upstream-idp-type-with-cli-password-flow-first"}, + }, req.URL.Query()) + return &http.Response{ + StatusCode: http.StatusFound, + Header: http.Header{"Location": []string{ + fmt.Sprintf("http://127.0.0.1:0/callback?code=%s&state=test-state", fakeAuthCode), + }}, + }, nil + default: + // Note that "/token" requests should not be made. They are mocked by mocking calls to ExchangeAuthcodeAndValidateTokens(). + require.FailNow(t, fmt.Sprintf("saw unexpected http call from the CLI: %s", req.URL.String())) + return nil, nil + } + }) + require.NoError(t, WithClient(client)(h)) + return nil + } + }, + issuer: successServer.URL, + wantLogs: []string{`"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + successServer.URL + `"`}, + wantStdErr: "^\nLog in to upstream-idp-name-with-cli-password-flow-first\n\n$", + wantToken: &testToken, + }, + { + name: "successful ldap login with prompts for username and password, infers flow when not specified", + clientID: "test-client-id", + opt: func(t *testing.T) Option { + return func(h *handlerState) error { + fakeAuthCode := "test-authcode-value" + + h.getProvider = func(_ *oauth2.Config, _ *coreosoidc.Provider, _ *http.Client) upstreamprovider.UpstreamOIDCIdentityProviderI { mock := mockUpstream(t) mock.EXPECT(). ExchangeAuthcodeAndValidateTokens( @@ -1537,7 +1979,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo ClientID: "test-client-id", Scopes: []string{"test-scope"}, RedirectURI: "http://localhost:0/callback", - UpstreamProviderName: "some-upstream-name", + UpstreamProviderName: "upstream-idp-name-with-cli-password-flow-first", } t.Cleanup(func() { require.Equal(t, []SessionCacheKey{cacheKey}, cache.sawGetKeys) @@ -1545,13 +1987,14 @@ func TestLogin(t *testing.T) { //nolint:gocyclo require.Equal(t, []*oidctypes.Token{&testToken}, cache.sawPutTokens) }) require.NoError(t, WithSessionCache(cache)(h)) - require.NoError(t, WithCLISendingCredentials()(h)) - require.NoError(t, WithUpstreamIdentityProvider("some-upstream-name", "ldap")(h)) + require.NoError(t, WithUpstreamIdentityProvider("upstream-idp-name-with-cli-password-flow-first", "upstream-idp-type-with-cli-password-flow-first")(h)) discoveryRequestWasMade := false + idpDiscoveryRequestWasMade := false authorizeRequestWasMade := false t.Cleanup(func() { require.True(t, discoveryRequestWasMade, "should have made an discovery request") + require.True(t, idpDiscoveryRequestWasMade, "should have made an IDP discovery request") require.True(t, authorizeRequestWasMade, "should have made an authorize request") }) @@ -1561,6 +2004,9 @@ func TestLogin(t *testing.T) { //nolint:gocyclo case "https://" + successServer.Listener.Addr().String() + "/.well-known/openid-configuration": discoveryRequestWasMade = true return defaultDiscoveryResponse(req) + case "https://" + successServer.Listener.Addr().String() + federationdomainoidc.PinnipedIDPsPathV1Alpha1: + idpDiscoveryRequestWasMade = true + return defaultDiscoveryResponse(req) case "https://" + successServer.Listener.Addr().String() + "/authorize": authorizeRequestWasMade = true require.Equal(t, "some-upstream-username", req.Header.Get("Pinniped-Username")) @@ -1575,8 +2021,8 @@ func TestLogin(t *testing.T) { //nolint:gocyclo "access_type": []string{"offline"}, "client_id": []string{"test-client-id"}, "redirect_uri": []string{"http://127.0.0.1:0/callback"}, - "pinniped_idp_name": []string{"some-upstream-name"}, - "pinniped_idp_type": []string{"ldap"}, + "pinniped_idp_name": []string{"upstream-idp-name-with-cli-password-flow-first"}, + "pinniped_idp_type": []string{"upstream-idp-type-with-cli-password-flow-first"}, }, req.URL.Query()) return &http.Response{ StatusCode: http.StatusFound, @@ -1596,7 +2042,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo }, issuer: successServer.URL, wantLogs: []string{`"level"=4 "msg"="Pinniped: Performing OIDC discovery" "issuer"="` + successServer.URL + `"`}, - wantStdErr: "^\nLog in to some-upstream-name\n\n$", + wantStdErr: "^\nLog in to upstream-idp-name-with-cli-password-flow-first\n\n$", wantToken: &testToken, }, { @@ -1606,7 +2052,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo return func(h *handlerState) error { fakeAuthCode := "test-authcode-value" - h.getProvider = func(_ *oauth2.Config, _ *oidc.Provider, _ *http.Client) upstreamprovider.UpstreamOIDCIdentityProviderI { + h.getProvider = func(_ *oauth2.Config, _ *coreosoidc.Provider, _ *http.Client) upstreamprovider.UpstreamOIDCIdentityProviderI { mock := mockUpstream(t) mock.EXPECT(). ExchangeAuthcodeAndValidateTokens( @@ -1650,7 +2096,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo require.Equal(t, []*oidctypes.Token{&testToken}, cache.sawPutTokens) }) require.NoError(t, WithSessionCache(cache)(h)) - require.NoError(t, WithCLISendingCredentials()(h)) + require.NoError(t, WithLoginFlow(idpdiscoveryv1alpha1.IDPFlowCLIPassword, "flowSource")(h)) discoveryRequestWasMade := false authorizeRequestWasMade := false @@ -1665,6 +2111,8 @@ func TestLogin(t *testing.T) { //nolint:gocyclo case "https://" + successServer.Listener.Addr().String() + "/.well-known/openid-configuration": discoveryRequestWasMade = true return defaultDiscoveryResponse(req) + case "https://" + successServer.Listener.Addr().String() + federationdomainoidc.PinnipedIDPsPathV1Alpha1: + return defaultDiscoveryResponse(req) case "https://" + successServer.Listener.Addr().String() + "/authorize": authorizeRequestWasMade = true require.Equal(t, "some-upstream-username", req.Header.Get("Pinniped-Username")) @@ -1711,7 +2159,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo return func(h *handlerState) error { fakeAuthCode := "test-authcode-value" - h.getProvider = func(_ *oauth2.Config, _ *oidc.Provider, _ *http.Client) upstreamprovider.UpstreamOIDCIdentityProviderI { + h.getProvider = func(_ *oauth2.Config, _ *coreosoidc.Provider, _ *http.Client) upstreamprovider.UpstreamOIDCIdentityProviderI { mock := mockUpstream(t) mock.EXPECT(). ExchangeAuthcodeAndValidateTokens( @@ -1748,7 +2196,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo ClientID: "test-client-id", Scopes: []string{"test-scope"}, RedirectURI: "http://localhost:0/callback", - UpstreamProviderName: "some-upstream-name", + UpstreamProviderName: "upstream-idp-name-with-cli-password-flow-first", } t.Cleanup(func() { require.Equal(t, []SessionCacheKey{cacheKey}, cache.sawGetKeys) @@ -1756,8 +2204,8 @@ func TestLogin(t *testing.T) { //nolint:gocyclo require.Equal(t, []*oidctypes.Token{&testToken}, cache.sawPutTokens) }) require.NoError(t, WithSessionCache(cache)(h)) - require.NoError(t, WithCLISendingCredentials()(h)) - require.NoError(t, WithUpstreamIdentityProvider("some-upstream-name", "ldap")(h)) + require.NoError(t, WithLoginFlow(idpdiscoveryv1alpha1.IDPFlowCLIPassword, "flowSource")(h)) + require.NoError(t, WithUpstreamIdentityProvider("upstream-idp-name-with-cli-password-flow-first", "upstream-idp-type-with-cli-password-flow-first")(h)) discoveryRequestWasMade := false authorizeRequestWasMade := false @@ -1772,6 +2220,8 @@ func TestLogin(t *testing.T) { //nolint:gocyclo case "https://" + successServer.Listener.Addr().String() + "/.well-known/openid-configuration": discoveryRequestWasMade = true return defaultDiscoveryResponse(req) + case "https://" + successServer.Listener.Addr().String() + federationdomainoidc.PinnipedIDPsPathV1Alpha1: + return defaultDiscoveryResponse(req) case "https://" + successServer.Listener.Addr().String() + "/authorize": authorizeRequestWasMade = true require.Equal(t, "some-upstream-username", req.Header.Get("Pinniped-Username")) @@ -1786,8 +2236,8 @@ func TestLogin(t *testing.T) { //nolint:gocyclo "access_type": []string{"offline"}, "client_id": []string{"test-client-id"}, "redirect_uri": []string{"http://127.0.0.1:0/callback"}, - "pinniped_idp_name": []string{"some-upstream-name"}, - "pinniped_idp_type": []string{"ldap"}, + "pinniped_idp_name": []string{"upstream-idp-name-with-cli-password-flow-first"}, + "pinniped_idp_type": []string{"upstream-idp-type-with-cli-password-flow-first"}, }, req.URL.Query()) return &http.Response{ StatusCode: http.StatusSeeOther, @@ -1811,7 +2261,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo `"level"=4 "msg"="Pinniped: Read username from environment variable" "name"="PINNIPED_USERNAME"`, `"level"=4 "msg"="Pinniped: Read password from environment variable" "name"="PINNIPED_PASSWORD"`, }, - wantStdErr: "^\nLog in to some-upstream-name\n\n$", + wantStdErr: "^\nLog in to upstream-idp-name-with-cli-password-flow-first\n\n$", wantToken: &testToken, }, { @@ -2153,10 +2603,10 @@ func TestLogin(t *testing.T) { //nolint:gocyclo require.NoError(t, WithSessionCache(cache)(h)) require.NoError(t, WithRequestAudience("test-audience")(h)) - h.validateIDToken = func(ctx context.Context, provider *oidc.Provider, audience string, token string) (*oidc.IDToken, error) { + h.validateIDToken = func(ctx context.Context, provider *coreosoidc.Provider, audience string, token string) (*coreosoidc.IDToken, error) { require.Equal(t, "test-audience", audience) require.Equal(t, "test-id-token-with-requested-audience", token) - return &oidc.IDToken{Expiry: testExchangedToken.IDToken.Expiry.Time}, nil + return &coreosoidc.IDToken{Expiry: testExchangedToken.IDToken.Expiry.Time}, nil } return nil } @@ -2196,7 +2646,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo require.NoError(t, WithSessionCache(cache)(h)) require.NoError(t, WithRequestAudience("request-this-test-audience")(h)) - h.validateIDToken = func(ctx context.Context, provider *oidc.Provider, audience string, token string) (*oidc.IDToken, error) { + h.validateIDToken = func(ctx context.Context, provider *coreosoidc.Provider, audience string, token string) (*coreosoidc.IDToken, error) { require.FailNow(t, "should not have performed a token exchange because the cached ID token already had the requested audience") return nil, nil } @@ -2204,7 +2654,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo } }, wantLogs: []string{`"level"=4 "msg"="Pinniped: Found unexpired cached token." "type"="id_token"`}, - wantToken: &oidctypes.Token{ // the same tokens that were pulled from from the cache + wantToken: &oidctypes.Token{ // the same tokens that were pulled from the cache AccessToken: testToken.AccessToken, IDToken: &oidctypes.IDToken{ Token: testToken.IDToken.Token, @@ -2248,7 +2698,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo require.NoError(t, WithSessionCache(cache)(h)) require.NoError(t, WithRequestAudience("test-custom-request-audience")(h)) - h.getProvider = func(config *oauth2.Config, provider *oidc.Provider, client *http.Client) upstreamprovider.UpstreamOIDCIdentityProviderI { + h.getProvider = func(config *oauth2.Config, provider *coreosoidc.Provider, client *http.Client) upstreamprovider.UpstreamOIDCIdentityProviderI { mock := mockUpstream(t) mock.EXPECT(). ValidateTokenAndMergeWithUserInfo(gomock.Any(), HasAccessToken(testToken.AccessToken.Token), nonce.Nonce(""), true, false). @@ -2310,10 +2760,10 @@ func TestLogin(t *testing.T) { //nolint:gocyclo require.NoError(t, WithSessionCache(cache)(h)) require.NoError(t, WithRequestAudience("request-this-test-audience")(h)) - h.validateIDToken = func(ctx context.Context, provider *oidc.Provider, audience string, token string) (*oidc.IDToken, error) { + h.validateIDToken = func(ctx context.Context, provider *coreosoidc.Provider, audience string, token string) (*coreosoidc.IDToken, error) { require.Equal(t, "request-this-test-audience", audience) require.Equal(t, "test-id-token-with-requested-audience", token) - return &oidc.IDToken{Expiry: testExchangedToken.IDToken.Expiry.Time}, nil + return &coreosoidc.IDToken{Expiry: testExchangedToken.IDToken.Expiry.Time}, nil } return nil } @@ -2358,7 +2808,7 @@ func TestLogin(t *testing.T) { //nolint:gocyclo }) h.cache = cache - h.getProvider = func(config *oauth2.Config, provider *oidc.Provider, client *http.Client) upstreamprovider.UpstreamOIDCIdentityProviderI { + h.getProvider = func(config *oauth2.Config, provider *coreosoidc.Provider, client *http.Client) upstreamprovider.UpstreamOIDCIdentityProviderI { mock := mockUpstream(t) mock.EXPECT(). ValidateTokenAndMergeWithUserInfo(gomock.Any(), HasAccessToken(testToken.AccessToken.Token), nonce.Nonce(""), true, false). @@ -2374,10 +2824,10 @@ func TestLogin(t *testing.T) { //nolint:gocyclo require.NoError(t, WithRequestAudience("test-audience")(h)) - h.validateIDToken = func(ctx context.Context, provider *oidc.Provider, audience string, token string) (*oidc.IDToken, error) { + h.validateIDToken = func(ctx context.Context, provider *coreosoidc.Provider, audience string, token string) (*coreosoidc.IDToken, error) { require.Equal(t, "test-audience", audience) require.Equal(t, "test-id-token-with-requested-audience", token) - return &oidc.IDToken{Expiry: testExchangedToken.IDToken.Expiry.Time}, nil + return &coreosoidc.IDToken{Expiry: testExchangedToken.IDToken.Expiry.Time}, nil } return nil } @@ -2534,7 +2984,7 @@ func TestHandlePasteCallback(t *testing.T) { return "invalid", nil } h.oauth2Config = &oauth2.Config{RedirectURL: testRedirectURI} - h.getProvider = func(_ *oauth2.Config, _ *oidc.Provider, _ *http.Client) upstreamprovider.UpstreamOIDCIdentityProviderI { + h.getProvider = func(_ *oauth2.Config, _ *coreosoidc.Provider, _ *http.Client) upstreamprovider.UpstreamOIDCIdentityProviderI { mock := mockUpstream(t) mock.EXPECT(). ExchangeAuthcodeAndValidateTokens(gomock.Any(), "invalid", pkce.Code("test-pkce"), nonce.Nonce("test-nonce"), testRedirectURI). @@ -2561,7 +3011,7 @@ func TestHandlePasteCallback(t *testing.T) { return "valid", nil } h.oauth2Config = &oauth2.Config{RedirectURL: testRedirectURI} - h.getProvider = func(_ *oauth2.Config, _ *oidc.Provider, _ *http.Client) upstreamprovider.UpstreamOIDCIdentityProviderI { + h.getProvider = func(_ *oauth2.Config, _ *coreosoidc.Provider, _ *http.Client) upstreamprovider.UpstreamOIDCIdentityProviderI { mock := mockUpstream(t) mock.EXPECT(). ExchangeAuthcodeAndValidateTokens(gomock.Any(), "valid", pkce.Code("test-pkce"), nonce.Nonce("test-nonce"), testRedirectURI). @@ -2586,7 +3036,7 @@ func TestHandlePasteCallback(t *testing.T) { h.useFormPost = true h.promptForValue = nil // shouldn't get called, so can be nil h.oauth2Config = &oauth2.Config{RedirectURL: testRedirectURI} - h.getProvider = func(_ *oauth2.Config, _ *oidc.Provider, _ *http.Client) upstreamprovider.UpstreamOIDCIdentityProviderI { + h.getProvider = func(_ *oauth2.Config, _ *coreosoidc.Provider, _ *http.Client) upstreamprovider.UpstreamOIDCIdentityProviderI { mock := mockUpstream(t) mock.EXPECT(). ExchangeAuthcodeAndValidateTokens(gomock.Any(), "valid", pkce.Code("test-pkce"), nonce.Nonce("test-nonce"), testRedirectURI). @@ -2821,7 +3271,7 @@ func TestHandleAuthCodeCallback(t *testing.T) { opt: func(t *testing.T) Option { return func(h *handlerState) error { h.oauth2Config = &oauth2.Config{RedirectURL: testRedirectURI} - h.getProvider = func(_ *oauth2.Config, _ *oidc.Provider, _ *http.Client) upstreamprovider.UpstreamOIDCIdentityProviderI { + h.getProvider = func(_ *oauth2.Config, _ *coreosoidc.Provider, _ *http.Client) upstreamprovider.UpstreamOIDCIdentityProviderI { mock := mockUpstream(t) mock.EXPECT(). ExchangeAuthcodeAndValidateTokens(gomock.Any(), "invalid", pkce.Code("test-pkce"), nonce.Nonce("test-nonce"), testRedirectURI). @@ -2845,7 +3295,7 @@ func TestHandleAuthCodeCallback(t *testing.T) { opt: func(t *testing.T) Option { return func(h *handlerState) error { h.oauth2Config = &oauth2.Config{RedirectURL: testRedirectURI} - h.getProvider = func(_ *oauth2.Config, _ *oidc.Provider, _ *http.Client) upstreamprovider.UpstreamOIDCIdentityProviderI { + h.getProvider = func(_ *oauth2.Config, _ *coreosoidc.Provider, _ *http.Client) upstreamprovider.UpstreamOIDCIdentityProviderI { mock := mockUpstream(t) mock.EXPECT(). ExchangeAuthcodeAndValidateTokens(gomock.Any(), "valid", pkce.Code("test-pkce"), nonce.Nonce("test-nonce"), testRedirectURI). @@ -2870,7 +3320,7 @@ func TestHandleAuthCodeCallback(t *testing.T) { opt: func(t *testing.T) Option { return func(h *handlerState) error { h.oauth2Config = &oauth2.Config{RedirectURL: testRedirectURI} - h.getProvider = func(_ *oauth2.Config, _ *oidc.Provider, _ *http.Client) upstreamprovider.UpstreamOIDCIdentityProviderI { + h.getProvider = func(_ *oauth2.Config, _ *coreosoidc.Provider, _ *http.Client) upstreamprovider.UpstreamOIDCIdentityProviderI { mock := mockUpstream(t) mock.EXPECT(). ExchangeAuthcodeAndValidateTokens(gomock.Any(), "valid", pkce.Code("test-pkce"), nonce.Nonce("test-nonce"), testRedirectURI). @@ -2898,7 +3348,7 @@ func TestHandleAuthCodeCallback(t *testing.T) { opt: func(t *testing.T) Option { return func(h *handlerState) error { h.oauth2Config = &oauth2.Config{RedirectURL: testRedirectURI} - h.getProvider = func(_ *oauth2.Config, _ *oidc.Provider, _ *http.Client) upstreamprovider.UpstreamOIDCIdentityProviderI { + h.getProvider = func(_ *oauth2.Config, _ *coreosoidc.Provider, _ *http.Client) upstreamprovider.UpstreamOIDCIdentityProviderI { mock := mockUpstream(t) mock.EXPECT(). ExchangeAuthcodeAndValidateTokens(gomock.Any(), "valid", pkce.Code("test-pkce"), nonce.Nonce("test-nonce"), testRedirectURI). @@ -3017,3 +3467,400 @@ func (m hasAccessTokenMatcher) String() string { func HasAccessToken(expected string) gomock.Matcher { return hasAccessTokenMatcher{expected: expected} } + +func TestMaybePerformPinnipedSupervisorIDPDiscovery(t *testing.T) { + withContextAndProvider := func(t *testing.T, issuerURL string) Option { + return func(h *handlerState) error { + t.Helper() + + cancelCtx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + + h.ctx = cancelCtx + + cancelCtx = coreosoidc.ClientContext(cancelCtx, h.httpClient) + provider, err := coreosoidc.NewProvider(cancelCtx, issuerURL) + require.NoError(t, err) + h.provider = provider + return nil + } + } + + t.Run("with valid IDP discovery information, returns the information", func(t *testing.T) { + issuerMux := http.NewServeMux() + issuerServer, issuerServerCA := tlsserver.TestServerIPv4(t, issuerMux, nil) + + oidcDiscoveryMetadata := discovery.Metadata{ + Issuer: issuerServer.URL, + OIDCDiscoveryResponse: idpdiscoveryv1alpha1.OIDCDiscoveryResponse{ + SupervisorDiscovery: idpdiscoveryv1alpha1.OIDCDiscoveryResponseIDPEndpoint{ + PinnipedIDPsEndpoint: issuerServer.URL + "/some-path-for-pinnipeds-idp-discovery", + }, + }, + } + + idpDiscoveryMetadata := &idpdiscoveryv1alpha1.IDPDiscoveryResponse{ + PinnipedIDPs: []idpdiscoveryv1alpha1.PinnipedIDP{ + { + Name: "some-idp-name", + Type: "some-idp-type", + Flows: []idpdiscoveryv1alpha1.IDPFlow{"some-flow", "some-other-flow"}, + }, + }, + PinnipedSupportedIDPTypes: []idpdiscoveryv1alpha1.PinnipedSupportedIDPType{ + {Type: "type-alpha"}, + {Type: "type-beta"}, + {Type: "type-gamma"}, + }, + } + + issuerMux.HandleFunc("/.well-known/openid-configuration", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("content-type", "application/json") + jsonBytes, err := json.Marshal(&oidcDiscoveryMetadata) + require.NoError(t, err) + _, err = w.Write(jsonBytes) + require.NoError(t, err) + }) + + issuerMux.HandleFunc("/some-path-for-pinnipeds-idp-discovery", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("content-type", "application/json") + jsonBytes, err := json.Marshal(idpDiscoveryMetadata) + require.NoError(t, err) + _, err = w.Write(jsonBytes) + require.NoError(t, err) + }) + + var h handlerState + require.NoError(t, WithClient(buildHTTPClientForPEM(issuerServerCA))(&h)) + require.NoError(t, withContextAndProvider(t, issuerServer.URL)(&h)) + + actualError := h.maybePerformPinnipedSupervisorIDPDiscovery() + require.NoError(t, actualError) + require.Equal(t, idpDiscoveryMetadata, h.idpDiscovery) + }) + + t.Run("when IDP discovery returns 500, return an error", func(t *testing.T) { + issuerMux := http.NewServeMux() + issuerServer, issuerServerCA := tlsserver.TestServerIPv4(t, issuerMux, nil) + + oidcDiscoveryMetadata := discovery.Metadata{ + Issuer: issuerServer.URL, + OIDCDiscoveryResponse: idpdiscoveryv1alpha1.OIDCDiscoveryResponse{ + SupervisorDiscovery: idpdiscoveryv1alpha1.OIDCDiscoveryResponseIDPEndpoint{ + PinnipedIDPsEndpoint: issuerServer.URL + "/some-path-for-pinnipeds-idp-discovery", + }, + }, + } + + issuerMux.HandleFunc("/.well-known/openid-configuration", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("content-type", "application/json") + jsonBytes, err := json.Marshal(&oidcDiscoveryMetadata) + require.NoError(t, err) + _, err = w.Write(jsonBytes) + require.NoError(t, err) + }) + + issuerMux.HandleFunc("/some-path-for-pinnipeds-idp-discovery", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + }) + + var h handlerState + require.NoError(t, WithClient(buildHTTPClientForPEM(issuerServerCA))(&h)) + require.NoError(t, withContextAndProvider(t, issuerServer.URL)(&h)) + + actualError := h.maybePerformPinnipedSupervisorIDPDiscovery() + require.EqualError(t, actualError, "unable to fetch IDP discovery data from issuer: unexpected http response status: 500 Internal Server Error") + require.Empty(t, h.idpDiscovery) + }) + + t.Run("when IDP discovery returns garbled data, return an error", func(t *testing.T) { + issuerMux := http.NewServeMux() + issuerServer, issuerServerCA := tlsserver.TestServerIPv4(t, issuerMux, nil) + + oidcDiscoveryMetadata := discovery.Metadata{ + Issuer: issuerServer.URL, + OIDCDiscoveryResponse: idpdiscoveryv1alpha1.OIDCDiscoveryResponse{ + SupervisorDiscovery: idpdiscoveryv1alpha1.OIDCDiscoveryResponseIDPEndpoint{ + PinnipedIDPsEndpoint: issuerServer.URL + "/some-path-for-pinnipeds-idp-discovery", + }, + }, + } + + issuerMux.HandleFunc("/.well-known/openid-configuration", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("content-type", "application/json") + jsonBytes, err := json.Marshal(&oidcDiscoveryMetadata) + require.NoError(t, err) + _, err = w.Write(jsonBytes) + require.NoError(t, err) + }) + + issuerMux.HandleFunc("/some-path-for-pinnipeds-idp-discovery", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("content-type", "application/json") + _, err := w.Write([]byte("foo")) + require.NoError(t, err) + }) + + var h handlerState + require.NoError(t, WithClient(buildHTTPClientForPEM(issuerServerCA))(&h)) + require.NoError(t, withContextAndProvider(t, issuerServer.URL)(&h)) + + actualError := h.maybePerformPinnipedSupervisorIDPDiscovery() + require.EqualError(t, actualError, "unable to fetch the Pinniped IDP discovery document: could not parse response JSON: invalid character 'o' in literal false (expecting 'a')") + require.Empty(t, h.idpDiscovery) + }) + + t.Run("when http client cannot perform request, return an error", func(t *testing.T) { + issuerMux := http.NewServeMux() + issuerServer, issuerServerCA := tlsserver.TestServerIPv4(t, issuerMux, nil) + + oidcDiscoveryMetadata := discovery.Metadata{ + Issuer: issuerServer.URL, + OIDCDiscoveryResponse: idpdiscoveryv1alpha1.OIDCDiscoveryResponse{ + SupervisorDiscovery: idpdiscoveryv1alpha1.OIDCDiscoveryResponseIDPEndpoint{ + PinnipedIDPsEndpoint: issuerServer.URL + "/some-path-for-pinnipeds-idp-discovery", + }, + }, + } + + issuerMux.HandleFunc("/.well-known/openid-configuration", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("content-type", "application/json") + jsonBytes, err := json.Marshal(&oidcDiscoveryMetadata) + require.NoError(t, err) + _, err = w.Write(jsonBytes) + require.NoError(t, err) + }) + + issuerMux.HandleFunc("/some-path-for-pinnipeds-idp-discovery", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Location", "foo") + w.WriteHeader(http.StatusSeeOther) + }) + + var h handlerState + require.NoError(t, WithClient(buildHTTPClientForPEM(issuerServerCA))(&h)) + require.NoError(t, withContextAndProvider(t, issuerServer.URL)(&h)) + h.httpClient.CheckRedirect = func(_ *http.Request, _ []*http.Request) error { + return fmt.Errorf("redirect error") + } + + actualError := h.maybePerformPinnipedSupervisorIDPDiscovery() + require.EqualError(t, actualError, `IDP Discovery response error: Get "foo": redirect error`) + require.Empty(t, h.idpDiscovery) + }) + + tests := []struct { + name string + pinnipedDiscovery string + wantErr string + }{ + { + name: "when not a Supervisor, returns nothing", + }, + { + name: "when the Supervisor returns empty discovery information, returns nothing", + pinnipedDiscovery: `{"pinniped_identity_providers_endpoint":""}`, + }, + { + name: "when the Supervisor returns invalid discovery information, returns an error", + pinnipedDiscovery: `"not-valid-discovery-claim"`, + wantErr: `could not decode the Pinniped IDP discovery document URL in OIDC discovery from "FAKE-ISSUER": json: cannot unmarshal string into Go struct field OIDCDiscoveryResponse.discovery.supervisor.pinniped.dev/v1alpha1 of type v1alpha1.OIDCDiscoveryResponseIDPEndpoint`, + }, + { + name: "when the Supervisor has invalid pinniped_identity_providers_endpoint, returns an error", + pinnipedDiscovery: `{"pinniped_identity_providers_endpoint":"asdf"}`, + wantErr: `the Pinniped IDP discovery document must always be hosted by the issuer: "FAKE-ISSUER"`, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + issuerMux := http.NewServeMux() + issuerServer, issuerServerCA := tlsserver.TestServerIPv4(t, issuerMux, nil) + + issuerMux.HandleFunc("/.well-known/openid-configuration", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("content-type", "application/json") + _, _ = fmt.Fprintf(w, `{"issuer": %q`, issuerServer.URL) + if len(test.pinnipedDiscovery) > 0 { + _, _ = fmt.Fprintf(w, `, "discovery.supervisor.pinniped.dev/v1alpha1": %s`, test.pinnipedDiscovery) + } + _, _ = fmt.Fprint(w, `}`) + }) + + var h handlerState + h.issuer = "FAKE-ISSUER" + require.NoError(t, WithClient(buildHTTPClientForPEM(issuerServerCA))(&h)) + require.NoError(t, withContextAndProvider(t, issuerServer.URL)(&h)) + + actualError := h.maybePerformPinnipedSupervisorIDPDiscovery() + + if test.wantErr != "" { + require.EqualError(t, actualError, test.wantErr) + return + } + + require.NoError(t, actualError) + }) + } +} + +func TestMaybePerformPinnipedSupervisorValidations(t *testing.T) { + withIDPDiscovery := func(idpDiscovery idpdiscoveryv1alpha1.IDPDiscoveryResponse) Option { + return func(h *handlerState) error { + h.idpDiscovery = &idpDiscovery + return nil + } + } + + withIssuer := func(issuer string) Option { + return func(h *handlerState) error { + h.issuer = issuer + return nil + } + } + + someIDPDiscoveryResponse := idpdiscoveryv1alpha1.IDPDiscoveryResponse{ + PinnipedIDPs: []idpdiscoveryv1alpha1.PinnipedIDP{ + { + Name: "some-upstream-name", + Type: "some-upstream-type", + Flows: []idpdiscoveryv1alpha1.IDPFlow{ + idpdiscoveryv1alpha1.IDPFlowCLIPassword, + idpdiscoveryv1alpha1.IDPFlowBrowserAuthcode, + }, + }, + { + Name: "idp-name-with-cli-password-only-flow", + Type: "idp-type-with-cli-password-only-flow", + Flows: []idpdiscoveryv1alpha1.IDPFlow{ + idpdiscoveryv1alpha1.IDPFlowCLIPassword, + }, + }, + { + Name: "idp-name-with-no-flows", + Type: "idp-type-with-no-flows", + }, + }, + PinnipedSupportedIDPTypes: []idpdiscoveryv1alpha1.PinnipedSupportedIDPType{ + {Type: "some-upstream-type"}, + {Type: "idp-type-with-cli-password-only-flow"}, + {Type: "idp-type-with-no-flows"}, + {Type: "other-supported-type-with-no-idp"}, + }, + } + stringVersionOfSomeIDPDiscoveryResponseIDPs := `[{"name":"some-upstream-name","type":"some-upstream-type","flows":["cli_password","browser_authcode"]},{"name":"idp-name-with-cli-password-only-flow","type":"idp-type-with-cli-password-only-flow","flows":["cli_password"]},{"name":"idp-name-with-no-flows","type":"idp-type-with-no-flows"}]` + + tests := []struct { + name string + options []Option + wantAuthCodeOptions []oauth2.AuthCodeOption + wantLoginFlow idpdiscoveryv1alpha1.IDPFlow + wantErr string + }{ + { + name: "without IDP name, return the specified login flow, nil options, and no error", + options: []Option{ + WithLoginFlow(idpdiscoveryv1alpha1.IDPFlowCLIPassword, "someSource"), + }, + wantLoginFlow: idpdiscoveryv1alpha1.IDPFlowCLIPassword, + wantAuthCodeOptions: nil, + }, + { + name: "with IDP name and IDP type, returns the right AuthCodeOptions and infers the loginFlow", + options: []Option{ + WithUpstreamIdentityProvider("some-upstream-name", "some-upstream-type"), + withIDPDiscovery(someIDPDiscoveryResponse), + }, + wantAuthCodeOptions: []oauth2.AuthCodeOption{ + oauth2.SetAuthURLParam(oidcapi.AuthorizeUpstreamIDPNameParamName, "some-upstream-name"), + oauth2.SetAuthURLParam(oidcapi.AuthorizeUpstreamIDPTypeParamName, "some-upstream-type"), + }, + wantLoginFlow: idpdiscoveryv1alpha1.IDPFlowCLIPassword, + }, + { + name: "when the Supervisor lists pinniped_supported_identity_provider_types and the given upstreamType is not found, return a specific error", + options: []Option{ + WithUpstreamIdentityProvider("some-upstream-name", "NOT_A_DISCOVERED_TYPE"), + withIDPDiscovery(someIDPDiscoveryResponse), + }, + wantErr: `unable to find upstream identity provider with type "NOT_A_DISCOVERED_TYPE", this Pinniped Supervisor supports IDP types ["idp-type-with-cli-password-only-flow", "idp-type-with-no-flows", "other-supported-type-with-no-idp", "some-upstream-type"]`, + }, + { + name: "when the Supervisor does not list pinniped_supported_identity_provider_types (legacy behavior) and the given upstreamType is not found, return a generic error", + options: []Option{ + WithUpstreamIdentityProvider("some-upstream-name", "NOT_A_DISCOVERED_TYPE"), + withIDPDiscovery(func() idpdiscoveryv1alpha1.IDPDiscoveryResponse { + temp := someIDPDiscoveryResponse + temp.PinnipedSupportedIDPTypes = nil + return temp + }()), + }, + wantErr: `unable to find upstream identity provider with name "some-upstream-name" and type "NOT_A_DISCOVERED_TYPE". Found these providers: ` + + stringVersionOfSomeIDPDiscoveryResponseIDPs, + }, + { + name: "when the Supervisor does not have an IDP that matches by name, return an error", + options: []Option{ + WithUpstreamIdentityProvider("INVALID-upstream-name", "some-upstream-type"), + withIDPDiscovery(someIDPDiscoveryResponse), + }, + wantErr: `unable to find upstream identity provider with name "INVALID-upstream-name" and type "some-upstream-type". Found these providers: ` + + stringVersionOfSomeIDPDiscoveryResponseIDPs, + }, + { + name: "when the Supervisor does not have an IDP that matches by type, return an error", + options: []Option{ + WithUpstreamIdentityProvider("some-upstream-name", "other-supported-type-with-no-idp"), + withIDPDiscovery(someIDPDiscoveryResponse), + }, + wantErr: `unable to find upstream identity provider with name "some-upstream-name" and type "other-supported-type-with-no-idp". Found these providers: ` + + stringVersionOfSomeIDPDiscoveryResponseIDPs, + }, + { + name: "when the Supervisor does not have an IDP that matches by flow, return an error", + options: []Option{ + WithUpstreamIdentityProvider("idp-name-with-no-flows", "idp-type-with-no-flows"), + WithLoginFlow(idpdiscoveryv1alpha1.IDPFlowBrowserAuthcode, "flowSource"), + withIDPDiscovery(someIDPDiscoveryResponse), + }, + wantErr: `unable to find upstream identity provider with name "idp-name-with-no-flows" and type "idp-type-with-no-flows" and flow "browser_authcode". Found these providers: ` + + stringVersionOfSomeIDPDiscoveryResponseIDPs, + }, + { + name: "with IDP name and type, without IDP flow, when the Supervisor says that IDP has no flows, return an error", + options: []Option{ + WithUpstreamIdentityProvider("idp-name-with-no-flows", "idp-type-with-no-flows"), + withIDPDiscovery(someIDPDiscoveryResponse), + }, + wantErr: `unable to infer flow for upstream identity provider with name "idp-name-with-no-flows" and type "idp-type-with-no-flows" because there were no flows discovered for that provider`, + }, + { + name: "with IDP name, when issuer does not have Pinniped-style IDP discovery, return error", + options: []Option{ + WithUpstreamIdentityProvider("some-upstream-name", "some-upstream-type"), + withIssuer("https://fake-issuer.com"), + }, + wantErr: `upstream identity provider name "some-upstream-name" was specified, but OIDC issuer "https://fake-issuer.com" does not offer Pinniped-style IDP discovery, so it does not appear to be a Pinniped Supervisor; specifying an upstream identity provider name is only meant to be used with Pinniped Supervisors`, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + var h handlerState + + for _, option := range test.options { + require.NoError(t, option(&h)) + } + + actualLoginFlow, actualAuthCodeOptions, actualError := h.maybePerformPinnipedSupervisorValidations() + + if test.wantErr != "" { + require.EqualError(t, actualError, test.wantErr) + return + } + + require.NoError(t, actualError) + require.Equal(t, test.wantAuthCodeOptions, actualAuthCodeOptions) + require.Equal(t, test.wantLoginFlow, actualLoginFlow) + }) + } +} diff --git a/test/integration/e2e_test.go b/test/integration/e2e_test.go index da42f9e4e..21c28840b 100644 --- a/test/integration/e2e_test.go +++ b/test/integration/e2e_test.go @@ -701,27 +701,15 @@ func TestE2EFullIntegration_Browser(t *testing.T) { ptyFile, err := pty.Start(kubectlCmd) require.NoError(t, err) - // Wait for the subprocess to print the username prompt, then type the user's username. - readFromFileUntilStringIsSeen(t, ptyFile, "Username: ") - _, err = ptyFile.WriteString(env.SupervisorUpstreamOIDC.Username + "\n") - require.NoError(t, err) - - // Wait for the subprocess to print the password prompt, then type the user's password. - readFromFileUntilStringIsSeen(t, ptyFile, "Password: ") - _, err = ptyFile.WriteString(env.SupervisorUpstreamOIDC.Password + "\n") - require.NoError(t, err) - // Read all output from the subprocess until EOF. // Ignore any errors returned because there is always an error on linux. kubectlOutputBytes, _ := io.ReadAll(ptyFile) kubectlOutput := string(kubectlOutputBytes) - // The output should look like an authentication failure, because the OIDCIdentityProvider disallows password grants. + // The output should fail IDP discovery validation, because the OIDCIdentityProvider disallows password grants. t.Log("kubectl command output (expecting a login failed error):\n", kubectlOutput) require.Contains(t, kubectlOutput, - `Error: could not complete Pinniped login: login failed with code "access_denied": `+ - `The resource owner or authorization server denied the request. `+ - `Resource owner password credentials grant is not allowed for this upstream provider according to its configuration.`, + fmt.Sprintf(`could not complete Pinniped login: unable to find upstream identity provider with name "%[1]s" and type "oidc" and flow "cli_password". Found these providers: [{"name":"%[1]s","type":"oidc","flows":["browser_authcode"]}]`, oidcIdentityProvider.Name), ) })