Add Supervisor upstream LDAP login to the Pinniped CLI

- Also enhance prepare-supervisor-on-kind.sh to allow setup of
  a working LDAP upstream IDP.

go.mod

@@ -31,6 +31,7 @@ require (
 	golang.org/x/net v0.0.0-20210226172049-e18ecbb05110
 	golang.org/x/oauth2 v0.0.0-20200107190931-bf48bf16ab8d
 	golang.org/x/sync v0.0.0-20201207232520-09787c993a3a
+	golang.org/x/term v0.0.0-20210220032956-6a3ed077a48d
 	gopkg.in/square/go-jose.v2 v2.5.1
 	k8s.io/api v0.21.0
 	k8s.io/apimachinery v0.21.0

hack/prepare-supervisor-on-kind.sh

@@ -20,6 +20,34 @@ set -euo pipefail
 ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"
 cd "$ROOT"
 
+use_oidc_upstream=no
+use_ldap_upstream=no
+while (("$#")); do
+  case "$1" in
+  --ldap)
+    use_ldap_upstream=yes
+    shift
+    ;;
+  --oidc)
+    use_oidc_upstream=yes
+    shift
+    ;;
+  -*)
+    log_error "Unsupported flag $1" >&2
+    exit 1
+    ;;
+  *)
+    log_error "Unsupported positional arg $1" >&2
+    exit 1
+    ;;
+  esac
+done
+
+if [[ "$use_oidc_upstream" == "no" && "$use_ldap_upstream" == "no" ]]; then
+  echo "Error: Please use --oidc or --ldap to specify which type of upstream identity provider(s) you would like"
+  exit 1
+fi
+
 # Read the env vars output by hack/prepare-for-integration-tests.sh
 source /tmp/integration-test-env
 
@@ -73,6 +101,7 @@ sleep 5
 echo "Fetching FederationDomain discovery info..."
 https_proxy="$PINNIPED_TEST_PROXY" curl -fLsS --cacert "$root_ca_crt_path" "$issuer/.well-known/openid-configuration" | jq .
 
+if [[ "$use_oidc_upstream" == "yes" ]]; then
   # Make an OIDCIdentityProvider which uses Dex to provide identity.
   cat <<EOF | kubectl apply --namespace "$PINNIPED_TEST_SUPERVISOR_NAMESPACE" -f -
 apiVersion: idp.supervisor.pinniped.dev/v1alpha1
@@ -105,9 +134,50 @@ type: "secrets.pinniped.dev/oidc-client"
 EOF
 
   # Grant the test user some RBAC permissions so we can play with kubectl as that user.
-kubectl create clusterrolebinding test-user-can-view --clusterrole view \
+  kubectl create clusterrolebinding oidc-test-user-can-view --clusterrole view \
     --user "$PINNIPED_TEST_SUPERVISOR_UPSTREAM_OIDC_USERNAME" \
     --dry-run=client --output yaml | kubectl apply -f -
+fi
+
+if [[ "$use_ldap_upstream" == "yes" ]]; then
+  # Make an LDAPIdentityProvider which uses OpenLDAP to provide identity.
+  cat <<EOF | kubectl apply --namespace "$PINNIPED_TEST_SUPERVISOR_NAMESPACE" -f -
+apiVersion: idp.supervisor.pinniped.dev/v1alpha1
+kind: LDAPIdentityProvider
+metadata:
+  name: my-ldap-provider
+spec:
+  host: "$PINNIPED_TEST_LDAP_HOST"
+  tls:
+    certificateAuthorityData: "$PINNIPED_TEST_LDAP_LDAPS_CA_BUNDLE"
+  bind:
+    secretName: my-ldap-service-account
+  userSearch:
+    base: "$PINNIPED_TEST_LDAP_USERS_SEARCH_BASE"
+    filter: "cn={}"
+    attributes:
+      uniqueID: "$PINNIPED_TEST_LDAP_USER_UNIQUE_ID_ATTRIBUTE_NAME"
+      username: "$PINNIPED_TEST_LDAP_USER_EMAIL_ATTRIBUTE_NAME"
+  dryRunAuthenticationUsername: "$PINNIPED_TEST_LDAP_USER_CN"
+EOF
+
+  # Make a Secret for the above LDAPIdentityProvider to describe the bind account.
+  cat <<EOF | kubectl apply --namespace "$PINNIPED_TEST_SUPERVISOR_NAMESPACE" -f -
+apiVersion: v1
+kind: Secret
+metadata:
+  name: my-ldap-service-account
+stringData:
+  username: "$PINNIPED_TEST_LDAP_BIND_ACCOUNT_USERNAME"
+  password: "$PINNIPED_TEST_LDAP_BIND_ACCOUNT_PASSWORD"
+type: "kubernetes.io/basic-auth"
+EOF
+
+  # Grant the test user some RBAC permissions so we can play with kubectl as that user.
+  kubectl create clusterrolebinding ldap-test-user-can-view --clusterrole view \
+    --user "$PINNIPED_TEST_LDAP_USER_EMAIL_ATTRIBUTE_VALUE" \
+    --dry-run=client --output yaml | kubectl apply -f -
+fi
 
 # Make a JWTAuthenticator which respects JWTs from the Supervisor's issuer.
 # The issuer URL must be accessible from within the cluster for OIDC discovery.
@@ -134,17 +204,30 @@ go build ./cmd/pinniped
 ./pinniped get kubeconfig --oidc-skip-browser >kubeconfig
 
 # Clear the local CLI cache to ensure that the kubectl command below will need to perform a fresh login.
-rm -f "$HOME"/.config/pinniped/sessions.yaml
+rm -f "$HOME/.config/pinniped/sessions.yaml"
+rm -f "$HOME/.config/pinniped/credentials.yaml"
 
 echo
 echo "Ready! 🚀"
+
+if [[ "$use_oidc_upstream" == "yes" ]]; then
+  echo
   echo "To be able to access the login URL shown below, start Chrome like this:"
   echo "    open -a \"Google Chrome\" --args --proxy-server=\"$PINNIPED_TEST_PROXY\""
   echo "Then use these credentials at the Dex login page:"
   echo "    Username: $PINNIPED_TEST_SUPERVISOR_UPSTREAM_OIDC_USERNAME"
   echo "    Password: $PINNIPED_TEST_SUPERVISOR_UPSTREAM_OIDC_PASSWORD"
+fi
 
-# Perform a login using the kubectl plugin. This should print the URL to be followed for the Dex login page.
+if [[ "$use_ldap_upstream" == "yes" ]]; then
+  echo
+  echo "When prompted for username and password by the CLI, use these values:"
+  echo "    Username: $PINNIPED_TEST_LDAP_USER_CN"
+  echo "    Password: $PINNIPED_TEST_LDAP_USER_PASSWORD"
+fi
+
+# Perform a login using the kubectl plugin. This should print the URL to be followed for the Dex login page
+# if using an OIDC upstream, or should prompt on the CLI for username/password if using an LDAP upstream.
 echo
 echo "Running: https_proxy=\"$PINNIPED_TEST_PROXY\" no_proxy=\"127.0.0.1\" kubectl --kubeconfig ./kubeconfig get pods -A"
 https_proxy="$PINNIPED_TEST_PROXY" no_proxy="127.0.0.1" kubectl --kubeconfig ./kubeconfig get pods -A

internal/oidc/auth/auth_handler.go

@@ -89,6 +89,7 @@ func handleAuthRequestForLDAPUpstream(
 	if username == "" || password == "" {
 		// Return an error according to OIDC spec 3.1.2.6 (second paragraph).
 		err := errors.WithStack(fosite.ErrAccessDenied.WithHintf("Missing or blank username or password."))
+		plog.Info("authorize response error", oidc.FositeErrorForLog(err)...)
 		oauthHelper.WriteAuthorizeError(w, authorizeRequester, err)
 		return nil
 	}
@@ -102,6 +103,7 @@ func handleAuthRequestForLDAPUpstream(
 		plog.Debug("failed upstream LDAP authentication", "upstreamName", ldapUpstream.GetName())
 		// Return an error according to OIDC spec 3.1.2.6 (second paragraph).
 		err = errors.WithStack(fosite.ErrAccessDenied.WithHintf("Username/password not accepted by LDAP provider."))
+		plog.Info("authorize response error", oidc.FositeErrorForLog(err)...)
 		oauthHelper.WriteAuthorizeError(w, authorizeRequester, err)
 		return nil
 	}

pkg/oidcclient/login.go

@@ -5,13 +5,16 @@
 package oidcclient
 
 import (
+	"bufio"
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"mime"
 	"net"
 	"net/http"
 	"net/url"
+	"os"
 	"sort"
 	"strings"
 	"time"
@@ -19,6 +22,7 @@ import (
 	"github.com/coreos/go-oidc/v3/oidc"
 	"github.com/pkg/browser"
 	"golang.org/x/oauth2"
+	"golang.org/x/term"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	"go.pinniped.dev/internal/httputil/httperr"
@@ -44,6 +48,16 @@ const (
 	// overallTimeout is the overall time that a login is allowed to take. This includes several user interactions, so
 	// we set this to be relatively long.
 	overallTimeout = 90 * time.Minute
+
+	supervisorAuthorizeUpstreamNameParam      = "upstream_name"
+	supervisorAuthorizeUpstreamTypeParam      = "upstream_type"
+	supervisorAuthorizeUpstreamUsernameHeader = "X-Pinniped-Upstream-Username"
+	supervisorAuthorizeUpstreamPasswordHeader = "X-Pinniped-Upstream-Password" // nolint:gosec // this is not a credential
+
+	defaultLDAPUsernamePrompt = "Username: "
+	defaultLDAPPasswordPrompt = "Password: "
+
+	httpLocationHeaderName = "Location"
 )
 
 type handlerState struct {
@@ -80,6 +94,8 @@ type handlerState struct {
 	openURL         func(string) error
 	getProvider     func(*oauth2.Config, *oidc.Provider, *http.Client) provider.UpstreamOIDCIdentityProviderI
 	validateIDToken func(ctx context.Context, provider *oidc.Provider, audience string, token string) (*oidc.IDToken, error)
+	promptForValue  func(promptLabel string) (string, error)
+	promptForSecret func(promptLabel string) (string, error)
 
 	callbacks chan callbackResult
 }
@@ -103,7 +119,7 @@ func WithContext(ctx context.Context) Option {
 
 // WithListenPort specifies a TCP listen port on localhost, which will be used for the redirect_uri and to handle the
 // authorization code callback. By default, a random high port will be chosen which requires the authorization server
-// to support wildcard port numbers as described by https://tools.ietf.org/html/rfc8252:
+// to support wildcard port numbers as described by https://tools.ietf.org/html/rfc8252#section-7.3:
 //
 // The authorization server MUST allow any port to be specified at the
 // time of the request for loopback IP redirect URIs, to accommodate
@@ -223,6 +239,8 @@ func Login(issuer string, clientID string, opts ...Option) (*oidctypes.Token, er
 		validateIDToken: func(ctx context.Context, provider *oidc.Provider, audience string, token string) (*oidc.IDToken, error) {
 			return provider.Verifier(&oidc.Config{ClientID: audience}).Verify(ctx, token)
 		},
+		promptForValue:  promptForValue,
+		promptForSecret: promptForSecret,
 	}
 	for _, opt := range opts {
 		if err := opt(&h); err != nil {
@@ -317,8 +335,8 @@ func (h *handlerState) baseLogin() (*oidctypes.Token, error) {
 		h.pkce.Method(),
 	}
 	if h.upstreamIdentityProviderName != "" {
-		authorizeOptions = append(authorizeOptions, oauth2.SetAuthURLParam("upstream_name", h.upstreamIdentityProviderName))
+		authorizeOptions = append(authorizeOptions, oauth2.SetAuthURLParam(supervisorAuthorizeUpstreamNameParam, h.upstreamIdentityProviderName))
-		authorizeOptions = append(authorizeOptions, oauth2.SetAuthURLParam("upstream_type", h.upstreamIdentityProviderType))
+		authorizeOptions = append(authorizeOptions, oauth2.SetAuthURLParam(supervisorAuthorizeUpstreamTypeParam, h.upstreamIdentityProviderType))
 	}
 
 	// Choose the appropriate authorization and authcode exchange strategy.
@@ -341,26 +359,103 @@ func (h *handlerState) baseLogin() (*oidctypes.Token, error) {
 // 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) {
+	// Ask the user for their username and password.
+	username, err := h.promptForValue(defaultLDAPUsernamePrompt)
+	if err != nil {
+		return nil, fmt.Errorf("error prompting for username: %w", err)
+	}
+	password, err := h.promptForSecret(defaultLDAPPasswordPrompt)
+	if err != nil {
+		return nil, fmt.Errorf("error prompting for password: %w", err)
+	}
+
 	// 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.
+	// registered on the server. The Supervisor oauth client does not have "localhost" in the allowed redirect
+	// URI list, so use 127.0.0.1.
+	localhostAddr := strings.ReplaceAll(h.listenAddr, "localhost", "127.0.0.1")
 	h.oauth2Config.RedirectURL = (&url.URL{
 		Scheme: "http",
-		Host:   h.listenAddr,
+		Host:   localhostAddr,
 		Path:   h.callbackPath,
 	}).String()
 
 	// Now that we have a redirect URL, we can build the authorize URL.
-	_ = h.oauth2Config.AuthCodeURL(h.state.String(), *authorizeOptions...)
+	authorizeURL := h.oauth2Config.AuthCodeURL(h.state.String(), *authorizeOptions...)
 
-	// TODO prompt for username and password
+	// Don't follow redirects automatically because we want to handle redirects here.
-	// TODO request the authorizeURL directly using h.httpClient, with the custom username and password headers
+	h.httpClient.CheckRedirect = func(req *http.Request, via []*http.Request) error {
-	// TODO error if the response is not a 302
+		return http.ErrUseLastResponse
-	// 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()
+	// Send an authorize request.
-	// TODO return the token or any error encountered along the way
+	authCtx, authorizeCtxCancelFunc := context.WithTimeout(context.Background(), httpRequestTimeout)
-	return nil, nil
+	defer authorizeCtxCancelFunc()
+	authReq, err := http.NewRequestWithContext(authCtx, http.MethodGet, authorizeURL, nil)
+	if err != nil {
+		return nil, fmt.Errorf("could not build authorize request: %w", err)
+	}
+	authReq.Header.Set(supervisorAuthorizeUpstreamUsernameHeader, username)
+	authReq.Header.Set(supervisorAuthorizeUpstreamPasswordHeader, password)
+	authRes, err := h.httpClient.Do(authReq)
+	if err != nil {
+		return nil, fmt.Errorf("authorization response error: %w", err)
+	}
+	err = authRes.Body.Close() // don't need the response body
+	if err != nil {
+		return nil, fmt.Errorf("could not close authorize response body: %w", err)
+	}
+
+	// A successful authorization always results in a 302.
+	if authRes.StatusCode != http.StatusFound {
+		return nil, fmt.Errorf(
+			"error getting authorization: expected to be redirected, but response status was %s", authRes.Status)
+	}
+	rawLocation := authRes.Header.Get(httpLocationHeaderName)
+	location, err := url.Parse(rawLocation)
+	if err != nil {
+		// This shouldn't be possible in practice because httpClient.Do() already parses the Location header.
+		return nil, fmt.Errorf("error getting authorization: could not parse redirect location: %w", err)
+	}
+
+	// Check that the redirect was to the expected location.
+	if location.Scheme != "http" || location.Host != localhostAddr || location.Path != h.callbackPath {
+		return nil, fmt.Errorf("error getting authorization: redirected to the wrong location: %s", rawLocation)
+	}
+
+	// Get the auth code or return the error from the server.
+	authCode := location.Query().Get("code")
+	if authCode == "" {
+		requiredErrorCode := location.Query().Get("error")
+		optionalErrorDescription := location.Query().Get("error_description")
+		if optionalErrorDescription == "" {
+			return nil, fmt.Errorf("login failed with code %q", requiredErrorCode)
+		}
+		return nil, fmt.Errorf("login failed with code %q: %s", requiredErrorCode, optionalErrorDescription)
+	}
+
+	// Validate OAuth2 state and fail if it's incorrect (to block CSRF).
+	if err := h.state.Validate(location.Query().Get("state")); err != nil {
+		return nil, fmt.Errorf("missing or invalid state parameter in authorization response: %s", rawLocation)
+	}
+
+	// Exchange the authorization code for access, ID, and refresh tokens and perform required
+	// validations on the returned ID token.
+	tokenCtx, tokenCtxCancelFunc := context.WithTimeout(context.Background(), httpRequestTimeout)
+	defer tokenCtxCancelFunc()
+	token, err := h.getProvider(h.oauth2Config, h.provider, h.httpClient).
+		ExchangeAuthcodeAndValidateTokens(
+			tokenCtx,
+			authCode,
+			h.pkce,
+			h.nonce,
+			h.oauth2Config.RedirectURL,
+		)
+	if err != nil {
+		return nil, fmt.Errorf("error during authorization code exchange: %w", err)
+	}
+
+	return token, nil
 }
 
 // Open a web browser, or ask the user to open a web browser, to visit the authorize endpoint.
@@ -401,6 +496,41 @@ func (h *handlerState) webBrowserBasedAuth(authorizeOptions *[]oauth2.AuthCodeOp
 	}
 }
 
+func promptForValue(promptLabel string) (string, error) {
+	if !term.IsTerminal(0) {
+		return "", errors.New("stdin is not connected to a terminal")
+	}
+	_, err := fmt.Fprint(os.Stderr, promptLabel)
+	if err != nil {
+		return "", fmt.Errorf("could not print prompt to stderr: %w", err)
+	}
+	text, _ := bufio.NewReader(os.Stdin).ReadString('\n')
+	text = strings.ReplaceAll(text, "\n", "")
+	return text, nil
+}
+
+func promptForSecret(promptLabel string) (string, error) {
+	if !term.IsTerminal(0) {
+		return "", errors.New("stdin is not connected to a terminal")
+	}
+	_, err := fmt.Fprint(os.Stderr, promptLabel)
+	if err != nil {
+		return "", fmt.Errorf("could not print prompt to stderr: %w", err)
+	}
+	password, err := term.ReadPassword(0)
+	if err != nil {
+		return "", fmt.Errorf("could not read password: %w", err)
+	}
+	// term.ReadPassword swallows the newline that was typed by the user, so to
+	// avoid the next line of output from happening on same line as the password
+	// prompt, we need to print a newline.
+	_, err = fmt.Fprint(os.Stderr, "\n")
+	if err != nil {
+		return "", fmt.Errorf("could not print newline to stderr: %w", err)
+	}
+	return string(password), err
+}
+
 func (h *handlerState) initOIDCDiscovery() error {
 	// Make this method idempotent so it can be called in multiple cases with no extra network requests.
 	if h.provider != nil {

pkg/oidcclient/login_test.go

@@ -6,10 +6,13 @@ package oidcclient
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
+	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
+	"strings"
 	"testing"
 	"time"
 
@@ -21,6 +24,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	"go.pinniped.dev/internal/httputil/httperr"
+	"go.pinniped.dev/internal/httputil/roundtripper"
 	"go.pinniped.dev/internal/mocks/mockupstreamoidcidentityprovider"
 	"go.pinniped.dev/internal/oidc/provider"
 	"go.pinniped.dev/internal/testutil"
@@ -51,7 +55,7 @@ func (m *mockSessionCache) PutToken(key SessionCacheKey, token *oidctypes.Token)
 	m.sawPutTokens = append(m.sawPutTokens, token)
 }
 
-func TestLogin(t *testing.T) {
+func TestLogin(t *testing.T) { // nolint:gocyclo
 	time1 := time.Date(2035, 10, 12, 13, 14, 15, 16, time.UTC)
 	time1Unix := int64(2075807775)
 	require.Equal(t, time1Unix, time1.Add(2*time.Minute).Unix())
@@ -198,6 +202,51 @@ func TestLogin(t *testing.T) {
 		require.NoError(t, json.NewEncoder(w).Encode(&response))
 	})
 
+	defaultDiscoveryResponse := func(req *http.Request) (*http.Response, error) { // nolint:unparam
+		// Call the handler function from the test server to calculate the response.
+		handler, _ := providerMux.Handler(req)
+		recorder := httptest.NewRecorder()
+		handler.ServeHTTP(recorder, req)
+		return recorder.Result(), nil
+	}
+
+	defaultLDAPTestOpts := func(t *testing.T, h *handlerState, authResponse *http.Response, authError error) error { // nolint:unparam
+		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.promptForValue = func(promptLabel string) (string, error) { return "some-upstream-username", nil }
+		h.promptForSecret = func(promptLabel string) (string, error) { 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",
+		}
+		t.Cleanup(func() {
+			require.Equal(t, []SessionCacheKey{cacheKey}, cache.sawGetKeys)
+		})
+		require.NoError(t, WithSessionCache(cache)(h))
+		require.NoError(t, WithLDAPUpstreamIdentityProvider()(h))
+		require.NoError(t, WithUpstreamIdentityProvider("some-upstream-name", "ldap")(h))
+
+		require.NoError(t, WithClient(&http.Client{
+			Transport: roundtripper.Func(func(req *http.Request) (*http.Response, error) {
+				switch req.URL.Scheme + "://" + req.URL.Host + req.URL.Path {
+				case "http://" + successServer.Listener.Addr().String() + "/.well-known/openid-configuration":
+					return defaultDiscoveryResponse(req)
+				case "http://" + successServer.Listener.Addr().String() + "/authorize":
+					return authResponse, authError
+				default:
+					require.FailNow(t, fmt.Sprintf("saw unexpected http call from the CLI: %s", req.URL.String()))
+					return nil, nil
+				}
+			}),
+		})(h))
+		return nil
+	}
+
 	tests := []struct {
 		name      string
 		opt       func(t *testing.T) Option
@@ -512,6 +561,345 @@ func TestLogin(t *testing.T) {
 			issuer:    successServer.URL,
 			wantToken: &testToken,
 		},
+		{
+			name:     "upstream name and type are included in authorize request if upstream name is provided",
+			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 }
+
+					cache := &mockSessionCache{t: t, getReturnsToken: nil}
+					cacheKey := SessionCacheKey{
+						Issuer:      successServer.URL,
+						ClientID:    "test-client-id",
+						Scopes:      []string{"test-scope"},
+						RedirectURI: "http://localhost:0/callback",
+					}
+					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, WithClient(&http.Client{Timeout: 10 * time.Second})(h))
+					require.NoError(t, WithUpstreamIdentityProvider("some-upstream-name", "oidc")(h))
+
+					h.openURL = func(actualURL string) error {
+						parsedActualURL, err := url.Parse(actualURL)
+						require.NoError(t, err)
+						actualParams := parsedActualURL.Query()
+
+						require.Contains(t, actualParams.Get("redirect_uri"), "http://127.0.0.1:")
+						actualParams.Del("redirect_uri")
+
+						require.Equal(t, url.Values{
+							// This is the PKCE challenge which is calculated as base64(sha256("test-pkce")). For example:
+							// $ echo -n test-pkce | shasum -a 256 | cut -d" " -f1 | xxd -r -p | base64 | cut -d"=" -f1
+							// VVaezYqum7reIhoavCHD1n2d+piN3r/mywoYj7fCR7g
+							"code_challenge":        []string{"VVaezYqum7reIhoavCHD1n2d-piN3r_mywoYj7fCR7g"},
+							"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"},
+							"upstream_name":         []string{"some-upstream-name"},
+							"upstream_type":         []string{"oidc"},
+						}, actualParams)
+
+						parsedActualURL.RawQuery = ""
+						require.Equal(t, successServer.URL+"/authorize", parsedActualURL.String())
+
+						go func() {
+							h.callbacks <- callbackResult{token: &testToken}
+						}()
+						return nil
+					}
+					return nil
+				}
+			},
+			issuer:    successServer.URL,
+			wantToken: &testToken,
+		},
+		{
+			name:     "ldap login when prompting for username returns an error",
+			clientID: "test-client-id",
+			opt: func(t *testing.T) Option {
+				return func(h *handlerState) error {
+					_ = defaultLDAPTestOpts(t, h, nil, nil)
+					h.promptForValue = func(promptLabel string) (string, error) {
+						require.Equal(t, "Username: ", promptLabel)
+						return "", errors.New("some prompt error")
+					}
+					return nil
+				}
+			},
+			issuer:  successServer.URL,
+			wantErr: "error prompting for username: some prompt error",
+		},
+		{
+			name:     "ldap login when prompting for password returns an error",
+			clientID: "test-client-id",
+			opt: func(t *testing.T) Option {
+				return func(h *handlerState) error {
+					_ = defaultLDAPTestOpts(t, h, nil, nil)
+					h.promptForSecret = func(promptLabel string) (string, error) { return "", errors.New("some prompt error") }
+					return nil
+				}
+			},
+			issuer:  successServer.URL,
+			wantErr: "error prompting for password: some prompt error",
+		},
+		{
+			name:     "ldap login when there is a problem with parsing the authorize URL",
+			clientID: "test-client-id",
+			opt: func(t *testing.T) Option {
+				return func(h *handlerState) error {
+					_ = defaultLDAPTestOpts(t, h, nil, nil)
+					require.NoError(t, WithClient(&http.Client{
+						Transport: roundtripper.Func(func(req *http.Request) (*http.Response, error) {
+							switch req.URL.Scheme + "://" + req.URL.Host + req.URL.Path {
+							case "http://" + successServer.Listener.Addr().String() + "/.well-known/openid-configuration":
+								type providerJSON struct {
+									Issuer   string `json:"issuer"`
+									AuthURL  string `json:"authorization_endpoint"`
+									TokenURL string `json:"token_endpoint"`
+									JWKSURL  string `json:"jwks_uri"`
+								}
+								jsonResponseBody, err := json.Marshal(&providerJSON{
+									Issuer:   successServer.URL,
+									AuthURL:  "%", // this is not a legal URL!
+									TokenURL: successServer.URL + "/token",
+									JWKSURL:  successServer.URL + "/keys",
+								})
+								require.NoError(t, err)
+								return &http.Response{
+									StatusCode: http.StatusOK,
+									Header:     http.Header{"content-type": []string{"application/json"}},
+									Body:       ioutil.NopCloser(strings.NewReader(string(jsonResponseBody))),
+								}, nil
+							default:
+								require.FailNow(t, fmt.Sprintf("saw unexpected http call from the CLI: %s", req.URL.String()))
+								return nil, nil
+							}
+						}),
+					})(h))
+					return nil
+				}
+			},
+			issuer:  successServer.URL,
+			wantErr: `could not build authorize request: parse "%?access_type=offline&client_id=test-client-id&code_challenge=VVaezYqum7reIhoavCHD1n2d-piN3r_mywoYj7fCR7g&code_challenge_method=S256&nonce=test-nonce&redirect_uri=http%3A%2F%2F127.0.0.1%3A0%2Fcallback&response_type=code&scope=test-scope&state=test-state&upstream_name=some-upstream-name&upstream_type=ldap": invalid URL escape "%"`,
+		},
+		{
+			name:     "ldap login when there is an error calling the authorization endpoint",
+			clientID: "test-client-id",
+			opt: func(t *testing.T) Option {
+				return func(h *handlerState) error {
+					return defaultLDAPTestOpts(t, h, nil, errors.New("some error fetching authorize endpoint"))
+				}
+			},
+			issuer: successServer.URL,
+			wantErr: `authorization response error: Get "http://` + successServer.Listener.Addr().String() +
+				`/authorize?access_type=offline&client_id=test-client-id&code_challenge=VVaezYqum7reIhoavCHD1n2d-piN3r_mywoYj7fCR7g&code_challenge_method=S256&nonce=test-nonce&redirect_uri=http%3A%2F%2F127.0.0.1%3A0%2Fcallback&response_type=code&scope=test-scope&state=test-state&upstream_name=some-upstream-name&upstream_type=ldap": some error fetching authorize endpoint`,
+		},
+		{
+			name:     "ldap login when the OIDC provider authorization endpoint returns something other than a 302 redirect",
+			clientID: "test-client-id",
+			opt: func(t *testing.T) Option {
+				return func(h *handlerState) error {
+					return defaultLDAPTestOpts(t, h, &http.Response{StatusCode: http.StatusBadGateway, Status: "502 Bad Gateway"}, nil)
+				}
+			},
+			issuer:  successServer.URL,
+			wantErr: `error getting authorization: expected to be redirected, but response status was 502 Bad Gateway`,
+		},
+		{
+			name:     "ldap login when the OIDC provider authorization endpoint redirect has an error and error description",
+			clientID: "test-client-id",
+			opt: func(t *testing.T) Option {
+				return func(h *handlerState) error {
+					return defaultLDAPTestOpts(t, h, &http.Response{
+						StatusCode: http.StatusFound,
+						Header: http.Header{"Location": []string{
+							"http://127.0.0.1:0/callback?error=access_denied&error_description=optional-error-description",
+						}},
+					}, nil)
+				}
+			},
+			issuer:  successServer.URL,
+			wantErr: `login failed with code "access_denied": optional-error-description`,
+		},
+		{
+			name:     "ldap login when the OIDC provider authorization endpoint redirects us to a different server",
+			clientID: "test-client-id",
+			opt: func(t *testing.T) Option {
+				return func(h *handlerState) error {
+					return defaultLDAPTestOpts(t, h, &http.Response{
+						StatusCode: http.StatusFound,
+						Header: http.Header{"Location": []string{
+							"http://other-server.example.com/callback?code=foo&state=test-state",
+						}},
+					}, nil)
+				}
+			},
+			issuer:  successServer.URL,
+			wantErr: `error getting authorization: redirected to the wrong location: http://other-server.example.com/callback?code=foo&state=test-state`,
+		},
+		{
+			name:     "ldap login when the OIDC provider authorization endpoint redirect has an error but no error description",
+			clientID: "test-client-id",
+			opt: func(t *testing.T) Option {
+				return func(h *handlerState) error {
+					return defaultLDAPTestOpts(t, h, &http.Response{
+						StatusCode: http.StatusFound,
+						Header: http.Header{"Location": []string{
+							"http://127.0.0.1:0/callback?error=access_denied",
+						}},
+					}, nil)
+				}
+			},
+			issuer:  successServer.URL,
+			wantErr: `login failed with code "access_denied"`,
+		},
+		{
+			name:     "ldap login when the OIDC provider authorization endpoint redirect has the wrong state value",
+			clientID: "test-client-id",
+			opt: func(t *testing.T) Option {
+				return func(h *handlerState) error {
+					return defaultLDAPTestOpts(t, h, &http.Response{
+						StatusCode: http.StatusFound,
+						Header:     http.Header{"Location": []string{"http://127.0.0.1:0/callback?code=foo&state=wrong-state"}},
+					}, nil)
+				}
+			},
+			issuer:  successServer.URL,
+			wantErr: `missing or invalid state parameter in authorization response: http://127.0.0.1:0/callback?code=foo&state=wrong-state`,
+		},
+		{
+			name:     "ldap login when there is an error exchanging the authcode or validating the tokens",
+			clientID: "test-client-id",
+			opt: func(t *testing.T) Option {
+				return func(h *handlerState) error {
+					fakeAuthCode := "test-authcode-value"
+					_ = defaultLDAPTestOpts(t, h, &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)
+					h.getProvider = func(_ *oauth2.Config, _ *oidc.Provider, _ *http.Client) provider.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(nil, errors.New("some authcode exchange or token validation error"))
+						return mock
+					}
+					return nil
+				}
+			},
+			issuer:  successServer.URL,
+			wantErr: "error during authorization code exchange: some authcode exchange or token validation error",
+		},
+		{
+			name:     "successful ldap login",
+			clientID: "test-client-id",
+			opt: func(t *testing.T) Option {
+				return func(h *handlerState) error {
+					fakeAuthCode := "test-authcode-value"
+
+					h.getProvider = func(_ *oauth2.Config, _ *oidc.Provider, _ *http.Client) provider.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.promptForValue = func(promptLabel string) (string, error) {
+						require.Equal(t, "Username: ", promptLabel)
+						return "some-upstream-username", nil
+					}
+					h.promptForSecret = func(promptLabel string) (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",
+					}
+					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, WithLDAPUpstreamIdentityProvider()(h))
+					require.NoError(t, WithUpstreamIdentityProvider("some-upstream-name", "ldap")(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")
+					})
+
+					require.NoError(t, WithClient(&http.Client{
+						Transport: roundtripper.Func(func(req *http.Request) (*http.Response, error) {
+							switch req.URL.Scheme + "://" + req.URL.Host + req.URL.Path {
+							case "http://" + successServer.Listener.Addr().String() + "/.well-known/openid-configuration":
+								discoveryRequestWasMade = true
+								return defaultDiscoveryResponse(req)
+							case "http://" + successServer.Listener.Addr().String() + "/authorize":
+								authorizeRequestWasMade = true
+								require.Equal(t, "some-upstream-username", req.Header.Get("X-Pinniped-Upstream-Username"))
+								require.Equal(t, "some-upstream-password", req.Header.Get("X-Pinniped-Upstream-Password"))
+								require.Equal(t, url.Values{
+									// This is the PKCE challenge which is calculated as base64(sha256("test-pkce")). For example:
+									// $ echo -n test-pkce | shasum -a 256 | cut -d" " -f1 | xxd -r -p | base64 | cut -d"=" -f1
+									// VVaezYqum7reIhoavCHD1n2d+piN3r/mywoYj7fCR7g
+									"code_challenge":        []string{"VVaezYqum7reIhoavCHD1n2d-piN3r_mywoYj7fCR7g"},
+									"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"},
+									"upstream_name":         []string{"some-upstream-name"},
+									"upstream_type":         []string{"ldap"},
+								}, 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
+							}
+						}),
+					})(h))
+					return nil
+				}
+			},
+			issuer:    successServer.URL,
+			wantToken: &testToken,
+		},
 		{
 			name:     "with requested audience, session cache hit with valid token, but discovery fails",
 			clientID: "test-client-id",