From 694e4d6df6885106966bac80edbca04d1a0053da Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Wed, 20 Apr 2022 14:58:09 -0700 Subject: [PATCH] Advertise browser_authcode flow in ldap idp discovery To keep this backwards compatible, this PR changes how the cli deals with ambiguous flows. Previously, if there was more than one flow advertised, the cli would require users to set the flag --upstream-identity-provider-flow. Now it chooses the first one in the list. Signed-off-by: Margo Crawford --- cmd/pinniped/cmd/kubeconfig.go | 15 +++--- cmd/pinniped/cmd/kubeconfig_test.go | 48 +++++++++++++++++-- .../idpdiscovery/idp_discovery_handler.go | 6 +-- .../idp_discovery_handler_test.go | 20 ++++---- 4 files changed, 62 insertions(+), 27 deletions(-) diff --git a/cmd/pinniped/cmd/kubeconfig.go b/cmd/pinniped/cmd/kubeconfig.go index f10fcd43..74280ec5 100644 --- a/cmd/pinniped/cmd/kubeconfig.go +++ b/cmd/pinniped/cmd/kubeconfig.go @@ -233,7 +233,7 @@ func runGetKubeconfig(ctx context.Context, out io.Writer, deps kubeconfigDeps, f // When all the upstream IDP flags are set by the user, then skip discovery and don't validate their input. Maybe they know something // that we can't know, like the name of an IDP that they are going to define in the future. if len(flags.oidc.issuer) > 0 && (flags.oidc.upstreamIDPType == "" || flags.oidc.upstreamIDPName == "" || flags.oidc.upstreamIDPFlow == "") { - if err := discoverSupervisorUpstreamIDP(ctx, &flags); err != nil { + if err := discoverSupervisorUpstreamIDP(ctx, &flags, deps.log); err != nil { return err } } @@ -726,7 +726,7 @@ func hasPendingStrategy(credentialIssuer *configv1alpha1.CredentialIssuer) bool return false } -func discoverSupervisorUpstreamIDP(ctx context.Context, flags *getKubeconfigParams) error { +func discoverSupervisorUpstreamIDP(ctx context.Context, flags *getKubeconfigParams, log logr.Logger) error { httpClient, err := newDiscoveryHTTPClient(flags.oidc.caBundle) if err != nil { return err @@ -758,7 +758,7 @@ func discoverSupervisorUpstreamIDP(ctx context.Context, flags *getKubeconfigPara return err } - selectedIDPFlow, err := selectUpstreamIDPFlow(discoveredIDPFlows, selectedIDPName, selectedIDPType, flags.oidc.upstreamIDPFlow) + selectedIDPFlow, err := selectUpstreamIDPFlow(discoveredIDPFlows, selectedIDPName, selectedIDPType, flags.oidc.upstreamIDPFlow, log) if err != nil { return err } @@ -898,7 +898,7 @@ func selectUpstreamIDPNameAndType(pinnipedIDPs []idpdiscoveryv1alpha1.PinnipedID } } -func selectUpstreamIDPFlow(discoveredIDPFlows []idpdiscoveryv1alpha1.IDPFlow, selectedIDPName string, selectedIDPType idpdiscoveryv1alpha1.IDPType, specifiedFlow string) (idpdiscoveryv1alpha1.IDPFlow, error) { +func selectUpstreamIDPFlow(discoveredIDPFlows []idpdiscoveryv1alpha1.IDPFlow, selectedIDPName string, selectedIDPType idpdiscoveryv1alpha1.IDPType, specifiedFlow string, log logr.Logger) (idpdiscoveryv1alpha1.IDPFlow, error) { switch { case len(discoveredIDPFlows) == 0: // No flows listed by discovery means that we are talking to an old Supervisor from before this feature existed. @@ -922,10 +922,7 @@ func selectUpstreamIDPFlow(discoveredIDPFlows []idpdiscoveryv1alpha1.IDPFlow, se return discoveredIDPFlows[0], nil default: // The user did not specify a flow, and more than one was found. - return "", fmt.Errorf( - "multiple client flows for Supervisor upstream identity provider %q of type %q were found, "+ - "so the --upstream-identity-provider-flow flag must be specified. "+ - "Found these flows: %v", - selectedIDPName, selectedIDPType, discoveredIDPFlows) + log.Info("multiple client flows found, selecting first value as default: "+discoveredIDPFlows[0].String(), "idpName", selectedIDPName, "idpType", selectedIDPType) + return discoveredIDPFlows[0], nil } } diff --git a/cmd/pinniped/cmd/kubeconfig_test.go b/cmd/pinniped/cmd/kubeconfig_test.go index e5c760aa..e5c27797 100644 --- a/cmd/pinniped/cmd/kubeconfig_test.go +++ b/cmd/pinniped/cmd/kubeconfig_test.go @@ -1261,13 +1261,51 @@ func TestGetKubeconfig(t *testing.T) { oidcDiscoveryResponse: happyOIDCDiscoveryResponse, idpsDiscoveryResponse: here.Docf(`{ "pinniped_identity_providers": [ - {"name": "some-oidc-idp", "type": "oidc", "flows": ["flow1", "flow2"]} + {"name": "some-ldap-idp", "type": "ldap", "flows": ["cli_password", "flow2"]} ] }`), - wantError: true, - wantStderr: func(issuerCABundle string, issuerURL string) string { - return `Error: multiple client flows for Supervisor upstream identity provider "some-oidc-idp" of type "oidc" were found, so the --upstream-identity-provider-flow flag must be specified.` + - ` Found these flows: [flow1 flow2]` + "\n" + wantStdout: func(issuerCABundle string, issuerURL string) string { + return here.Docf(` + apiVersion: v1 + clusters: + - cluster: + certificate-authority-data: ZmFrZS1jZXJ0aWZpY2F0ZS1hdXRob3JpdHktZGF0YS12YWx1ZQ== + server: https://fake-server-url-value + name: kind-cluster-pinniped + contexts: + - context: + cluster: kind-cluster-pinniped + user: kind-user-pinniped + name: kind-context-pinniped + current-context: kind-context-pinniped + kind: Config + preferences: {} + users: + - name: kind-user-pinniped + user: + exec: + apiVersion: client.authentication.k8s.io/v1beta1 + args: + - login + - oidc + - --issuer=%s + - --client-id=pinniped-cli + - --scopes=offline_access,openid,pinniped:request-audience + - --ca-bundle-data=%s + - --upstream-identity-provider-name=some-ldap-idp + - --upstream-identity-provider-type=ldap + - --upstream-identity-provider-flow=cli_password + command: '.../path/to/pinniped' + env: [] + installHint: The pinniped CLI does not appear to be installed. See https://get.pinniped.dev/cli + for more details + provideClusterInfo: true + `, + issuerURL, + base64.StdEncoding.EncodeToString([]byte(issuerCABundle))) + }, + wantLogs: func(_ string, _ string) []string { + return []string{"\"level\"=0 \"msg\"=\"multiple client flows found, selecting first value as default: cli_password\" \"idpName\"=\"some-ldap-idp\" \"idpType\"=\"ldap\""} }, }, { diff --git a/internal/oidc/idpdiscovery/idp_discovery_handler.go b/internal/oidc/idpdiscovery/idp_discovery_handler.go index 8949502c..66a974c9 100644 --- a/internal/oidc/idpdiscovery/idp_discovery_handler.go +++ b/internal/oidc/idpdiscovery/idp_discovery_handler.go @@ -1,4 +1,4 @@ -// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // Package idpdiscovery provides a handler for the upstream IDP discovery endpoint. @@ -44,14 +44,14 @@ func responseAsJSON(upstreamIDPs oidc.UpstreamIdentityProvidersLister) ([]byte, r.PinnipedIDPs = append(r.PinnipedIDPs, v1alpha1.PinnipedIDP{ Name: provider.GetName(), Type: v1alpha1.IDPTypeLDAP, - Flows: []v1alpha1.IDPFlow{v1alpha1.IDPFlowCLIPassword}, + Flows: []v1alpha1.IDPFlow{v1alpha1.IDPFlowCLIPassword, v1alpha1.IDPFlowBrowserAuthcode}, }) } for _, provider := range upstreamIDPs.GetActiveDirectoryIdentityProviders() { r.PinnipedIDPs = append(r.PinnipedIDPs, v1alpha1.PinnipedIDP{ Name: provider.GetName(), Type: v1alpha1.IDPTypeActiveDirectory, - Flows: []v1alpha1.IDPFlow{v1alpha1.IDPFlowCLIPassword}, + Flows: []v1alpha1.IDPFlow{v1alpha1.IDPFlowCLIPassword, v1alpha1.IDPFlowBrowserAuthcode}, }) } for _, provider := range upstreamIDPs.GetOIDCIdentityProviders() { diff --git a/internal/oidc/idpdiscovery/idp_discovery_handler_test.go b/internal/oidc/idpdiscovery/idp_discovery_handler_test.go index f5e601bd..b33ab2d8 100644 --- a/internal/oidc/idpdiscovery/idp_discovery_handler_test.go +++ b/internal/oidc/idpdiscovery/idp_discovery_handler_test.go @@ -1,4 +1,4 @@ -// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package idpdiscovery @@ -37,22 +37,22 @@ func TestIDPDiscovery(t *testing.T) { wantContentType: "application/json", wantFirstResponseBodyJSON: here.Doc(`{ "pinniped_identity_providers": [ - {"name": "a-some-ldap-idp", "type": "ldap", "flows": ["cli_password"]}, + {"name": "a-some-ldap-idp", "type": "ldap", "flows": ["cli_password", "browser_authcode"]}, {"name": "a-some-oidc-idp", "type": "oidc", "flows": ["browser_authcode"]}, - {"name": "x-some-idp", "type": "ldap", "flows": ["cli_password"]}, + {"name": "x-some-idp", "type": "ldap", "flows": ["cli_password", "browser_authcode"]}, {"name": "x-some-idp", "type": "oidc", "flows": ["browser_authcode"]}, - {"name": "y-some-ad-idp", "type": "activedirectory", "flows": ["cli_password"]}, - {"name": "z-some-ad-idp", "type": "activedirectory", "flows": ["cli_password"]}, - {"name": "z-some-ldap-idp", "type": "ldap", "flows": ["cli_password"]}, + {"name": "y-some-ad-idp", "type": "activedirectory", "flows": ["cli_password", "browser_authcode"]}, + {"name": "z-some-ad-idp", "type": "activedirectory", "flows": ["cli_password", "browser_authcode"]}, + {"name": "z-some-ldap-idp", "type": "ldap", "flows": ["cli_password", "browser_authcode"]}, {"name": "z-some-oidc-idp", "type": "oidc", "flows": ["browser_authcode", "cli_password"]} ] }`), wantSecondResponseBodyJSON: here.Doc(`{ "pinniped_identity_providers": [ - {"name": "some-other-ad-idp-1", "type": "activedirectory", "flows": ["cli_password"]}, - {"name": "some-other-ad-idp-2", "type": "activedirectory", "flows": ["cli_password"]}, - {"name": "some-other-ldap-idp-1", "type": "ldap", "flows": ["cli_password"]}, - {"name": "some-other-ldap-idp-2", "type": "ldap", "flows": ["cli_password"]}, + {"name": "some-other-ad-idp-1", "type": "activedirectory", "flows": ["cli_password", "browser_authcode"]}, + {"name": "some-other-ad-idp-2", "type": "activedirectory", "flows": ["cli_password", "browser_authcode"]}, + {"name": "some-other-ldap-idp-1", "type": "ldap", "flows": ["cli_password", "browser_authcode"]}, + {"name": "some-other-ldap-idp-2", "type": "ldap", "flows": ["cli_password", "browser_authcode"]}, {"name": "some-other-oidc-idp-1", "type": "oidc", "flows": ["browser_authcode", "cli_password"]}, {"name": "some-other-oidc-idp-2", "type": "oidc", "flows": ["browser_authcode"]} ]