Add new command-line flags to the login oidc command

- Also some light prefactoring in login.go to make room for LDAP-style
  login, which is not implemented yet in this commit. TODOs are added.
- And fix a test pollution problem in login_oidc_test.go where it was
  using a real on-disk CLI cache file, so the tests were polluted by
  the contents of that file and would sometimes cause each other to
  fail.

cmd/pinniped/cmd/login_oidc.go

@@ -50,23 +50,25 @@ func oidcLoginCommandRealDeps() oidcLoginCommandDeps {
 }
 
 type oidcLoginFlags struct {
-	issuer                     string
+	issuer                       string
-	clientID                   string
+	clientID                     string
-	listenPort                 uint16
+	listenPort                   uint16
-	scopes                     []string
+	scopes                       []string
-	skipBrowser                bool
+	skipBrowser                  bool
-	sessionCachePath           string
+	sessionCachePath             string
-	caBundlePaths              []string
+	caBundlePaths                []string
-	caBundleData               []string
+	caBundleData                 []string
-	debugSessionCache          bool
+	debugSessionCache            bool
-	requestAudience            string
+	requestAudience              string
-	conciergeEnabled           bool
+	conciergeEnabled             bool
-	conciergeAuthenticatorType string
+	conciergeAuthenticatorType   string
-	conciergeAuthenticatorName string
+	conciergeAuthenticatorName   string
-	conciergeEndpoint          string
+	conciergeEndpoint            string
-	conciergeCABundle          string
+	conciergeCABundle            string
-	conciergeAPIGroupSuffix    string
+	conciergeAPIGroupSuffix      string
-	credentialCachePath        string
+	credentialCachePath          string
+	upstreamIdentityProviderName string
+	upstreamIdentityProviderType string
 }
 
 func oidcLoginCommand(deps oidcLoginCommandDeps) *cobra.Command {
@@ -98,6 +100,8 @@ func oidcLoginCommand(deps oidcLoginCommandDeps) *cobra.Command {
 	cmd.Flags().StringVar(&flags.conciergeCABundle, "concierge-ca-bundle-data", "", "CA bundle to use when connecting to the Concierge")
 	cmd.Flags().StringVar(&flags.conciergeAPIGroupSuffix, "concierge-api-group-suffix", groupsuffix.PinnipedDefaultSuffix, "Concierge API group suffix")
 	cmd.Flags().StringVar(&flags.credentialCachePath, "credential-cache", filepath.Join(mustGetConfigDir(), "credentials.yaml"), "Path to cluster-specific credentials cache (\"\" disables the cache)")
+	cmd.Flags().StringVar(&flags.upstreamIdentityProviderName, "upstream-identity-provider-name", "", "The name of the upstream identity provider used during login with a Supervisor")
+	cmd.Flags().StringVar(&flags.upstreamIdentityProviderType, "upstream-identity-provider-type", "oidc", "The type of the upstream identity provider used during login with a Supervisor (e.g. 'oidc', 'ldap')")
 
 	mustMarkHidden(cmd, "debug-session-cache")
 	mustMarkRequired(cmd, "issuer")
@@ -137,6 +141,23 @@ func runOIDCLogin(cmd *cobra.Command, deps oidcLoginCommandDeps, flags oidcLogin
 		opts = append(opts, oidcclient.WithRequestAudience(flags.requestAudience))
 	}
 
+	if flags.upstreamIdentityProviderName != "" {
+		opts = append(opts, oidcclient.WithUpstreamIdentityProvider(
+			flags.upstreamIdentityProviderName, flags.upstreamIdentityProviderType))
+	}
+
+	switch flags.upstreamIdentityProviderType {
+	case "oidc":
+		// this is the default, so don't need to do anything
+	case "ldap":
+		opts = append(opts, oidcclient.WithLDAPUpstreamIdentityProvider())
+	default:
+		// Surprisingly cobra does not support this kind of flag validation. See https://github.com/spf13/pflag/issues/236
+		return fmt.Errorf(
+			"--upstream-identity-provider-type value not recognized: %s (supported values: oidc, ldap)",
+			flags.upstreamIdentityProviderType)
+	}
+
 	var concierge *conciergeclient.Client
 	if flags.conciergeEnabled {
 		var err error

cmd/pinniped/cmd/login_oidc_test.go

@@ -56,23 +56,25 @@ func TestLoginOIDCCommand(t *testing.T) {
 				  oidc --issuer ISSUER [flags]
 
 				Flags:
-				      --ca-bundle strings                     Path to TLS certificate authority bundle (PEM format, optional, can be repeated)
+				      --ca-bundle strings                        Path to TLS certificate authority bundle (PEM format, optional, can be repeated)
-				      --ca-bundle-data strings                Base64 encoded TLS certificate authority bundle (base64 encoded PEM format, optional, can be repeated)
+				      --ca-bundle-data strings                   Base64 encoded TLS certificate authority bundle (base64 encoded PEM format, optional, can be repeated)
-				      --client-id string                      OpenID Connect client ID (default "pinniped-cli")
+				      --client-id string                         OpenID Connect client ID (default "pinniped-cli")
-				      --concierge-api-group-suffix string     Concierge API group suffix (default "pinniped.dev")
+				      --concierge-api-group-suffix string        Concierge API group suffix (default "pinniped.dev")
-				      --concierge-authenticator-name string   Concierge authenticator name
+				      --concierge-authenticator-name string      Concierge authenticator name
-				      --concierge-authenticator-type string   Concierge authenticator type (e.g., 'webhook', 'jwt')
+				      --concierge-authenticator-type string      Concierge authenticator type (e.g., 'webhook', 'jwt')
-				      --concierge-ca-bundle-data string       CA bundle to use when connecting to the Concierge
+				      --concierge-ca-bundle-data string          CA bundle to use when connecting to the Concierge
-				      --concierge-endpoint string             API base for the Concierge endpoint
+				      --concierge-endpoint string                API base for the Concierge endpoint
-				      --credential-cache string               Path to cluster-specific credentials cache ("" disables the cache) (default "` + cfgDir + `/credentials.yaml")
+				      --credential-cache string                  Path to cluster-specific credentials cache ("" disables the cache) (default "` + cfgDir + `/credentials.yaml")
-				      --enable-concierge                      Use the Concierge to login
+				      --enable-concierge                         Use the Concierge to login
-				  -h, --help                                  help for oidc
+				  -h, --help                                     help for oidc
-				      --issuer string                         OpenID Connect issuer URL
+				      --issuer string                            OpenID Connect issuer URL
-				      --listen-port uint16                    TCP port for localhost listener (authorization code flow only)
+				      --listen-port uint16                       TCP port for localhost listener (authorization code flow only)
-				      --request-audience string               Request a token with an alternate audience using RFC8693 token exchange
+				      --request-audience string                  Request a token with an alternate audience using RFC8693 token exchange
-				      --scopes strings                        OIDC scopes to request during login (default [offline_access,openid,pinniped:request-audience])
+				      --scopes strings                           OIDC scopes to request during login (default [offline_access,openid,pinniped:request-audience])
-				      --session-cache string                  Path to session cache file (default "` + cfgDir + `/sessions.yaml")
+				      --session-cache string                     Path to session cache file (default "` + cfgDir + `/sessions.yaml")
-				      --skip-browser                          Skip opening the browser (just print the URL)
+				      --skip-browser                             Skip opening the browser (just print the URL)
+					  --upstream-identity-provider-name string   The name of the upstream identity provider used during login with a Supervisor
+					  --upstream-identity-provider-type string   The type of the upstream identity provider used during login with a Supervisor (e.g. 'oidc', 'ldap') (default "oidc")
 			`),
 		},
 		{
@@ -134,11 +136,45 @@ func TestLoginOIDCCommand(t *testing.T) {
 				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",
+			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)
+			`),
+		},
+		{
+			name: "oidc upstream type is allowed",
+			args: []string{
+				"--issuer", "test-issuer",
+				"--client-id", "test-client-id",
+				"--upstream-identity-provider-type", "oidc",
+				"--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution
+			},
+			wantOptionsCount: 3,
+			wantStdout:       `{"kind":"ExecCredential","apiVersion":"client.authentication.k8s.io/v1beta1","spec":{},"status":{"expirationTimestamp":"3020-10-12T13:14:15Z","token":"test-id-token"}}` + "\n",
+		},
+		{
+			name: "ldap upstream type 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
+			},
+			wantOptionsCount: 4,
+			wantStdout:       `{"kind":"ExecCredential","apiVersion":"client.authentication.k8s.io/v1beta1","spec":{},"status":{"expirationTimestamp":"3020-10-12T13:14:15Z","token":"test-id-token"}}` + "\n",
+		},
 		{
 			name: "login error",
 			args: []string{
 				"--client-id", "test-client-id",
 				"--issuer", "test-issuer",
+				"--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution
 			},
 			loginErr:         fmt.Errorf("some login error"),
 			wantOptionsCount: 3,
@@ -156,6 +192,7 @@ func TestLoginOIDCCommand(t *testing.T) {
 				"--concierge-authenticator-type", "jwt",
 				"--concierge-authenticator-name", "test-authenticator",
 				"--concierge-endpoint", "https://127.0.0.1:1234/",
+				"--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution
 			},
 			conciergeErr:     fmt.Errorf("some concierge error"),
 			wantOptionsCount: 3,
@@ -169,6 +206,7 @@ func TestLoginOIDCCommand(t *testing.T) {
 			args: []string{
 				"--client-id", "test-client-id",
 				"--issuer", "test-issuer",
+				"--credential-cache", "", // must specify --credential-cache or else the cache file on disk causes test pollution
 			},
 			wantOptionsCount: 3,
 			wantStdout:       `{"kind":"ExecCredential","apiVersion":"client.authentication.k8s.io/v1beta1","spec":{},"status":{"expirationTimestamp":"3020-10-12T13:14:15Z","token":"test-id-token"}}` + "\n",
@@ -190,9 +228,11 @@ func TestLoginOIDCCommand(t *testing.T) {
 				"--concierge-endpoint", "https://127.0.0.1:1234/",
 				"--concierge-ca-bundle-data", base64.StdEncoding.EncodeToString(testCA.Bundle()),
 				"--concierge-api-group-suffix", "some.suffix.com",
-				"--credential-cache", testutil.TempDir(t) + "/credentials.yaml",
+				"--credential-cache", testutil.TempDir(t) + "/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",
 			},
-			wantOptionsCount: 7,
+			wantOptionsCount: 9,
 			wantStdout:       `{"kind":"ExecCredential","apiVersion":"client.authentication.k8s.io/v1beta1","spec":{},"status":{"token":"exchanged-token"}}` + "\n",
 		},
 	}

pkg/oidcclient/login.go

@@ -1,4 +1,4 @@
-// Copyright 2020 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0
 
 // Package oidcclient implements a CLI OIDC login flow.
@@ -54,6 +54,10 @@ type handlerState struct {
 	scopes   []string
 	cache    SessionCache
 
+	upstreamIdentityProviderName string
+	upstreamIdentityProviderType string
+	ldapUpstreamIdentityProvider bool
+
 	requestedAudience string
 
 	httpClient *http.Client
@@ -167,6 +171,30 @@ func WithRequestAudience(audience string) Option {
 	}
 }
 
+// WithLDAPUpstreamIdentityProvider causes the login flow to use CLI 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 is an LDAP provider. It should never be used with non-Supervisor issuers because it will send the user's
+// password as a custom header, which would be ignored but could potentially get logged somewhere by the issuer.
+func WithLDAPUpstreamIdentityProvider() Option {
+	return func(h *handlerState) error {
+		h.ldapUpstreamIdentityProvider = true
+		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.
+// Other issuers will ignore these custom query parameters.
+func WithUpstreamIdentityProvider(upstreamName, upstreamType string) Option {
+	return func(h *handlerState) error {
+		h.upstreamIdentityProviderName = upstreamName
+		h.upstreamIdentityProviderType = upstreamType
+		return nil
+	}
+}
+
 // nopCache is a SessionCache that doesn't actually do anything.
 type nopCache struct{}
 
@@ -281,6 +309,63 @@ func (h *handlerState) baseLogin() (*oidctypes.Token, error) {
 		}
 	}
 
+	// Prepare the common options for the authorization URL. We don't have the redirect URL yet though.
+	authorizeOptions := []oauth2.AuthCodeOption{
+		oauth2.AccessTypeOffline,
+		h.nonce.Param(),
+		h.pkce.Challenge(),
+		h.pkce.Method(),
+	}
+	if h.upstreamIdentityProviderName != "" {
+		authorizeOptions = append(authorizeOptions, oauth2.SetAuthURLParam("upstream_name", h.upstreamIdentityProviderName))
+		authorizeOptions = append(authorizeOptions, oauth2.SetAuthURLParam("upstream_type", h.upstreamIdentityProviderType))
+	}
+
+	// Choose the appropriate authorization and authcode exchange strategy.
+	var authFunc = h.webBrowserBasedAuth
+	if h.ldapUpstreamIdentityProvider {
+		authFunc = h.cliBasedAuth
+	}
+
+	// Perform the authorize request and authcode exchange to get back OIDC tokens.
+	token, err := authFunc(&authorizeOptions)
+
+	// If we got tokens, put them in the cache.
+	if err == nil {
+		h.cache.PutToken(cacheKey, token)
+	}
+
+	return token, err
+}
+
+// Make a direct call to the authorize endpoint 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) {
+	// Make a callback URL even though we won't be listening on this port, because providing a redirect URL is
+	// required for OIDC authorize endpoints, and it must match the allowed redirect URL of the OIDC client
+	// registered on the server.
+	h.oauth2Config.RedirectURL = (&url.URL{
+		Scheme: "http",
+		Host:   h.listenAddr,
+		Path:   h.callbackPath,
+	}).String()
+
+	// Now that we have a redirect URL, we can build the authorize URL.
+	_ = h.oauth2Config.AuthCodeURL(h.state.String(), *authorizeOptions...)
+
+	// TODO prompt for username and password
+	// TODO request the authorizeURL directly using h.httpClient, with the custom username and password headers
+	// TODO error if the response is not a 302
+	// TODO error if the response Location does not include a code param (in this case it could have an error message query param to show)
+	// TODO check the response Location state param to see if it matches, similar to how it is done in handleAuthCodeCallback()
+	// TODO exchange the authcode, similar to how it is done in handleAuthCodeCallback()
+	// TODO return the token or any error encountered along the way
+	return nil, nil
+}
+
+// Open a web browser, or ask the user to open a web browser, to visit the authorize endpoint.
+// Create a localhost callback listener which exchanges the authcode for tokens. Return the tokens or an error.
+func (h *handlerState) webBrowserBasedAuth(authorizeOptions *[]oauth2.AuthCodeOption) (*oidctypes.Token, error) {
 	// Open a TCP listener and update the OAuth2 redirect_uri to match (in case we are using an ephemeral port number).
 	listener, err := net.Listen("tcp", h.listenAddr)
 	if err != nil {
@@ -292,18 +377,14 @@ func (h *handlerState) baseLogin() (*oidctypes.Token, error) {
 		Path:   h.callbackPath,
 	}).String()
 
+	// Now that we have a redirect URL with the listener port, we can build the authorize URL.
+	authorizeURL := h.oauth2Config.AuthCodeURL(h.state.String(), *authorizeOptions...)
+
 	// Start a callback server in a background goroutine.
 	shutdown := h.serve(listener)
 	defer shutdown()
 
 	// Open the authorize URL in the users browser.
-	authorizeURL := h.oauth2Config.AuthCodeURL(
-		h.state.String(),
-		oauth2.AccessTypeOffline,
-		h.nonce.Param(),
-		h.pkce.Challenge(),
-		h.pkce.Method(),
-	)
 	if err := h.openURL(authorizeURL); err != nil {
 		return nil, fmt.Errorf("could not open browser: %w", err)
 	}
@@ -316,7 +397,6 @@ func (h *handlerState) baseLogin() (*oidctypes.Token, error) {
 		if callback.err != nil {
 			return nil, fmt.Errorf("error handling callback: %w", callback.err)
 		}
-		h.cache.PutToken(cacheKey, callback.token)
 		return callback.token, nil
 	}
 }
@@ -447,6 +527,8 @@ func (h *handlerState) handleAuthCodeCallback(w http.ResponseWriter, r *http.Req
 
 	// Check for error response parameters.
 	if errorParam := params.Get("error"); errorParam != "" {
+		// TODO This should also show the value of the optional "error_description" param if it exists.
+		//  See https://openid.net/specs/openid-connect-core-1_0.html#AuthError
 		return httperr.Newf(http.StatusBadRequest, "login failed with code %q", errorParam)
 	}