Skip to content

Commit

Permalink
Additional WIP that does resolve some TODOs but adds more and needs a…
Browse files Browse the repository at this point in the history 
… lot more testing
  • Loading branch information
@joshuatcasey
joshuatcasey committed May 6, 2024
1 parent f90def1 commit 1d7f77c
Show file tree
Hide file tree
Showing 6 changed files with 498 additions and 427 deletions.
112 changes: 6 additions & 106 deletions cmd/pinniped/cmd/login_oidc.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import (
"net/http"
"os"
"path/filepath"
"strings"
"time"

"github.com/spf13/cobra"
Expand Down Expand Up @@ -201,43 +200,13 @@ func runOIDCLogin(cmd *cobra.Command, deps oidcLoginCommandDeps, flags oidcLogin
flags.upstreamIdentityProviderName, flags.upstreamIdentityProviderType))
}

// TODO This is the caller, but note that OIDC discovery has not happened yet at this point. :(
// It hasn't even constructed the http client for it yet. Nor has it checked to see if
// it already has a cached cluster-specific credential that it can return without ever talking to a server.
// This is too early to discover if the requested IDP type is supported by the Supervisor, and also
// too early to discover if that IDP type supports the requested flow or not.
// So this code could be replaced by code which simply passes through (as a new Option) the value of the
// flow type flag, possibly overridden by the value of the env var, or possibly nothing when the flow type
// flag and env var are both blank.
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...)

// TODO: something like this
//requestedFlow := idpdiscoveryv1alpha1.IDPFlow(flags.upstreamIdentityProviderFlow)
//flowOverride, hasFlowOverride := deps.lookupEnv(upstreamIdentityProviderFlowEnvVarName)
//if hasFlowOverride {
// requestedFlow = idpdiscoveryv1alpha1.IDPFlow(flowOverride)
//}
//if requestedFlow != "" {
// switch requestedFlow {
// case idpdiscoveryv1alpha1.IDPFlowCLIPassword,
// idpdiscoveryv1alpha1.IDPFlowBrowserAuthcode:
// default:
// return fmt.Errorf(
// "WithLoginFlow parameter must be %s or %s",
// idpdiscoveryv1alpha1.IDPFlowCLIPassword,
// idpdiscoveryv1alpha1.IDPFlowBrowserAuthcode,
// )
// }
// opts = append(opts, deps.optionsFactory.WithLoginFlow(requestedFlow))
//}

var concierge *conciergeclient.Client
if flags.conciergeEnabled {
Expand Down Expand Up @@ -327,75 +296,6 @@ func runOIDCLogin(cmd *cobra.Command, deps oidcLoginCommandDeps, flags oidcLogin
return json.NewEncoder(cmd.OutOrStdout()).Encode(cred)
}

// flowOptions takes in the requested IDP type, the requested flow type (which can be blank), and the env var which
// can override the requested flow type.
// It determines whether the CLI should prompt for the user's username/password (known as the "CLI flow"), which is
// returned as an oidcclient.Option. Returning nil means that the CLI should not prompt for username/password.
// It also tries to make nice error messages when the requested IDP type or requested flow values are not understood.
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
}

// TODO This switch statement means that we have to change the CLI every time we add a new IDP type to the Supervisor server. :(
// If we added a new flow type, then CLI changes would be expected. But adding a new IDP type should ideally
// not require any CLI functionality changes (except maybe some command-line flag help text messages).
// Rather than hardcode knowledge of which IDP types are supported, and which flow types are supported by each IDP,
// and which is the default flow type for each IDP type, all that knowledge should come from a discovery request.
// Default flow type for each IDP type can be determined by the ordering of the flow types in the discovery response.
// (See https://github.com/vmware-tanzu/pinniped/blob/v0.29.0/internal/federationdomain/endpoints/idpdiscovery/idp_discovery_handler_test.go#L56-L57.)
// Keeping the nice error messages would only be possible if the CLI used discovery to check what the Supervisor supports.
// We would need to be careful to also support old Supervisors that don't advertise which IDP types are supported
// by retaining the old CLI behavior for that case (assume that the old Supervisor supports only OIDC, LDAP, and AD).
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 {
Expand Down
Loading

0 comments on commit 1d7f77c

Please sign in to comment.