Merge pull request #1013 from vmware-tanzu/cli_require_https_issuers

CLI requires HTTPS OIDC issuer, authorize, and token URLS

pkg/oidcclient/login.go

@@ -704,6 +704,11 @@ func (h *handlerState) initOIDCDiscovery() error {
 		return nil
 	}
 
+	// Validate that the issuer URL uses https, or else we cannot trust its discovery endpoint to get the other URLs.
+	if err := validateURLUsesHTTPS(h.issuer, "issuer"); err != nil {
+		return err
+	}
+
 	h.logger.V(debugLogLevel).Info("Pinniped: Performing OIDC discovery", "issuer", h.issuer)
 	var err error
 	h.provider, err = oidc.NewProvider(h.ctx, h.issuer)
@@ -718,6 +723,18 @@ func (h *handlerState) initOIDCDiscovery() error {
 		Scopes:   h.scopes,
 	}
 
+	// Validate that the discovered auth and token URLs use https. The OIDC spec for the authcode flow says:
+	// "Communication with the Authorization Endpoint MUST utilize TLS"
+	// (see https://openid.net/specs/openid-connect-core-1_0.html#AuthorizationEndpoint), and
+	// "Communication with the Token Endpoint MUST utilize TLS"
+	// (see https://openid.net/specs/openid-connect-core-1_0.html#TokenEndpoint).
+	if err := validateURLUsesHTTPS(h.provider.Endpoint().AuthURL, "discovered authorize URL from issuer"); err != nil {
+		return err
+	}
+	if err := validateURLUsesHTTPS(h.provider.Endpoint().TokenURL, "discovered token URL from issuer"); err != nil {
+		return err
+	}
+
 	// Use response_mode=form_post if the provider supports it.
 	var discoveryClaims struct {
 		ResponseModesSupported []string `json:"response_modes_supported"`
@@ -729,6 +746,17 @@ func (h *handlerState) initOIDCDiscovery() error {
 	return nil
 }
 
+func validateURLUsesHTTPS(uri string, uriName string) error {
+	parsed, err := url.Parse(uri)
+	if err != nil {
+		return fmt.Errorf("%s is not a valid URL: %w", uriName, err)
+	}
+	if parsed.Scheme != "https" {
+		return fmt.Errorf("%s must be an https URL, but had scheme %q instead", uriName, parsed.Scheme)
+	}
+	return nil
+}
+
 func stringSliceContains(slice []string, s string) bool {
 	for _, item := range slice {
 		if item == s {

pkg/oidcclient/login_test.go

@@ -6,6 +6,7 @@ package oidcclient
 import (
 	"bytes"
 	"context"
+	"crypto/x509"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -19,6 +20,10 @@ import (
 	"testing"
 	"time"
 
+	"go.pinniped.dev/internal/net/phttp"
+
+	"go.pinniped.dev/internal/testutil/tlsserver"
+
 	"github.com/coreos/go-oidc/v3/oidc"
 	"github.com/golang/mock/gomock"
 	"github.com/stretchr/testify/assert"
@@ -61,6 +66,13 @@ func (m *mockSessionCache) PutToken(key SessionCacheKey, token *oidctypes.Token)
 	m.sawPutTokens = append(m.sawPutTokens, token)
 }
 
+func newClientForServer(server *httptest.Server) *http.Client {
+	pool := x509.NewCertPool()
+	caPEMData := tlsserver.TLSTestServerCA(server)
+	pool.AppendCertsFromPEM(caPEMData)
+	return phttp.Default(pool)
+}
+
 func TestLogin(t *testing.T) { // nolint:gocyclo
 	time1 := time.Date(2035, 10, 12, 13, 14, 15, 16, time.UTC)
 	time1Unix := int64(2075807775)
@@ -76,31 +88,33 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 		IDToken: &oidctypes.IDToken{Token: "test-id-token-with-requested-audience", Expiry: metav1.NewTime(time1.Add(3 * time.Minute))},
 	}
 
-	// Start a test server that returns 500 errors
+	// Start a test server that returns 500 errors.
-	errorServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+	errorServer := tlsserver.TLSTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		http.Error(w, "some discovery error", http.StatusInternalServerError)
-	}))
+	}), nil)
-	t.Cleanup(errorServer.Close)
 
 	// Start a test server that returns discovery data with a broken response_modes_supported value.
 	brokenResponseModeMux := http.NewServeMux()
-	brokenResponseModeServer := httptest.NewServer(brokenResponseModeMux)
+	brokenResponseModeServer := tlsserver.TLSTestServer(t, brokenResponseModeMux, nil)
 	brokenResponseModeMux.HandleFunc("/.well-known/openid-configuration", func(w http.ResponseWriter, r *http.Request) {
 		w.Header().Set("content-type", "application/json")
 		type providerJSON struct {
 			Issuer                 string `json:"issuer"`
+			AuthURL                string `json:"authorization_endpoint"`
+			TokenURL               string `json:"token_endpoint"`
 			ResponseModesSupported string `json:"response_modes_supported"` // Wrong type (should be []string).
 		}
 		_ = json.NewEncoder(w).Encode(&providerJSON{
 			Issuer:                 brokenResponseModeServer.URL,
+			AuthURL:                brokenResponseModeServer.URL + "/authorize",
+			TokenURL:               brokenResponseModeServer.URL + "/token",
 			ResponseModesSupported: "invalid",
 		})
 	})
-	t.Cleanup(brokenResponseModeServer.Close)
 
-	// Start a test server that returns discovery data with a broken token URL
+	// Start a test server that returns discovery data with a broken token URL.
 	brokenTokenURLMux := http.NewServeMux()
-	brokenTokenURLServer := httptest.NewServer(brokenTokenURLMux)
+	brokenTokenURLServer := tlsserver.TLSTestServer(t, brokenTokenURLMux, nil)
 	brokenTokenURLMux.HandleFunc("/.well-known/openid-configuration", func(w http.ResponseWriter, r *http.Request) {
 		w.Header().Set("content-type", "application/json")
 		type providerJSON struct {
@@ -116,7 +130,63 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 			JWKSURL:  brokenTokenURLServer.URL + "/keys",
 		})
 	})
-	t.Cleanup(brokenTokenURLServer.Close)
+
+	// Start a test server that returns discovery data with an insecure token URL.
+	insecureTokenURLMux := http.NewServeMux()
+	insecureTokenURLServer := tlsserver.TLSTestServer(t, insecureTokenURLMux, nil)
+	insecureTokenURLMux.HandleFunc("/.well-known/openid-configuration", func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("content-type", "application/json")
+		type providerJSON struct {
+			Issuer   string `json:"issuer"`
+			AuthURL  string `json:"authorization_endpoint"`
+			TokenURL string `json:"token_endpoint"`
+			JWKSURL  string `json:"jwks_uri"`
+		}
+		_ = json.NewEncoder(w).Encode(&providerJSON{
+			Issuer:   insecureTokenURLServer.URL,
+			AuthURL:  insecureTokenURLServer.URL + "/authorize",
+			TokenURL: "http://insecure-issuer.com",
+			JWKSURL:  insecureTokenURLServer.URL + "/keys",
+		})
+	})
+
+	// Start a test server that returns discovery data with a broken authorize URL.
+	brokenAuthURLMux := http.NewServeMux()
+	brokenAuthURLServer := tlsserver.TLSTestServer(t, brokenAuthURLMux, nil)
+	brokenAuthURLMux.HandleFunc("/.well-known/openid-configuration", func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("content-type", "application/json")
+		type providerJSON struct {
+			Issuer   string `json:"issuer"`
+			AuthURL  string `json:"authorization_endpoint"`
+			TokenURL string `json:"token_endpoint"`
+			JWKSURL  string `json:"jwks_uri"`
+		}
+		_ = json.NewEncoder(w).Encode(&providerJSON{
+			Issuer:   brokenAuthURLServer.URL,
+			AuthURL:  `%`,
+			TokenURL: brokenAuthURLServer.URL + "/token",
+			JWKSURL:  brokenAuthURLServer.URL + "/keys",
+		})
+	})
+
+	// Start a test server that returns discovery data with an insecure authorize URL.
+	insecureAuthURLMux := http.NewServeMux()
+	insecureAuthURLServer := tlsserver.TLSTestServer(t, insecureAuthURLMux, nil)
+	insecureAuthURLMux.HandleFunc("/.well-known/openid-configuration", func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("content-type", "application/json")
+		type providerJSON struct {
+			Issuer   string `json:"issuer"`
+			AuthURL  string `json:"authorization_endpoint"`
+			TokenURL string `json:"token_endpoint"`
+			JWKSURL  string `json:"jwks_uri"`
+		}
+		_ = json.NewEncoder(w).Encode(&providerJSON{
+			Issuer:   insecureAuthURLServer.URL,
+			AuthURL:  "http://insecure-issuer.com",
+			TokenURL: insecureAuthURLServer.URL + "/token",
+			JWKSURL:  insecureAuthURLServer.URL + "/keys",
+		})
+	})
 
 	discoveryHandler := func(server *httptest.Server, responseModes []string) http.HandlerFunc {
 		return func(w http.ResponseWriter, r *http.Request) {
@@ -225,15 +295,13 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 
 	// Start a test server that returns a real discovery document and answers refresh requests.
 	providerMux := http.NewServeMux()
-	successServer := httptest.NewServer(providerMux)
+	successServer := tlsserver.TLSTestServer(t, providerMux, nil)
-	t.Cleanup(successServer.Close)
 	providerMux.HandleFunc("/.well-known/openid-configuration", discoveryHandler(successServer, nil))
 	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 := httptest.NewServer(formPostProviderMux)
+	formPostSuccessServer := tlsserver.TLSTestServer(t, formPostProviderMux, nil)
-	t.Cleanup(formPostSuccessServer.Close)
 	formPostProviderMux.HandleFunc("/.well-known/openid-configuration", discoveryHandler(formPostSuccessServer, []string{"query", "form_post"}))
 	formPostProviderMux.HandleFunc("/token", tokenHandler)
 
@@ -265,13 +333,14 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 		require.NoError(t, WithSessionCache(cache)(h))
 		require.NoError(t, WithCLISendingCredentials()(h))
 		require.NoError(t, WithUpstreamIdentityProvider("some-upstream-name", "ldap")(h))
+		require.NoError(t, WithClient(newClientForServer(successServer))(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":
+				case "https://" + successServer.Listener.Addr().String() + "/.well-known/openid-configuration":
 					return defaultDiscoveryResponse(req)
-				case "http://" + successServer.Listener.Addr().String() + "/authorize":
+				case "https://" + 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()))
@@ -331,11 +400,36 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 			wantErr: "some error generating PKCE",
 		},
 		{
-			name:     "session cache hit but token expired",
+			name:     "issuer is not https",
-			issuer:   "test-issuer",
+			issuer:   "http://insecure-issuer.com",
 			clientID: "test-client-id",
 			opt: func(t *testing.T) Option {
 				return func(h *handlerState) error {
+					return nil
+				}
+			},
+			wantLogs: nil,
+			wantErr:  `issuer must be an https URL, but had scheme "http" instead`,
+		},
+		{
+			name:     "issuer is not a valid URL",
+			issuer:   "%",
+			clientID: "test-client-id",
+			opt: func(t *testing.T) Option {
+				return func(h *handlerState) error {
+					return nil
+				}
+			},
+			wantLogs: nil,
+			wantErr:  `issuer is not a valid URL: parse "%": invalid URL escape "%"`,
+		},
+		{
+			name:     "session cache hit but token expired",
+			issuer:   errorServer.URL,
+			clientID: "test-client-id",
+			opt: func(t *testing.T) Option {
+				return func(h *handlerState) error {
+					require.NoError(t, WithClient(newClientForServer(errorServer))(h))
 					cache := &mockSessionCache{t: t, getReturnsToken: &oidctypes.Token{
 						IDToken: &oidctypes.IDToken{
 							Token:  "test-id-token",
@@ -344,7 +438,7 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 					}}
 					t.Cleanup(func() {
 						require.Equal(t, []SessionCacheKey{{
-							Issuer:      "test-issuer",
+							Issuer:      errorServer.URL,
 							ClientID:    "test-client-id",
 							Scopes:      []string{"test-scope"},
 							RedirectURI: "http://localhost:0/callback",
@@ -354,8 +448,8 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 					return WithSessionCache(cache)(h)
 				}
 			},
-			wantLogs: []string{"\"level\"=4 \"msg\"=\"Pinniped: Performing OIDC discovery\"  \"issuer\"=\"test-issuer\""},
+			wantLogs: []string{"\"level\"=4 \"msg\"=\"Pinniped: Performing OIDC discovery\"  \"issuer\"=\"" + errorServer.URL + "\""},
-			wantErr:  `could not perform OIDC discovery for "test-issuer": Get "test-issuer/.well-known/openid-configuration": unsupported protocol scheme ""`,
+			wantErr:  "could not perform OIDC discovery for \"" + errorServer.URL + "\": 500 Internal Server Error: some discovery error\n",
 		},
 		{
 			name:     "session cache hit with valid token",
@@ -382,7 +476,10 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 		{
 			name: "discovery failure due to 500 error",
 			opt: func(t *testing.T) Option {
-				return func(h *handlerState) error { return nil }
+				return func(h *handlerState) error {
+					require.NoError(t, WithClient(newClientForServer(errorServer))(h))
+					return nil
+				}
 			},
 			issuer:   errorServer.URL,
 			wantLogs: []string{"\"level\"=4 \"msg\"=\"Pinniped: Performing OIDC discovery\"  \"issuer\"=\"" + errorServer.URL + "\""},
@@ -391,7 +488,10 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 		{
 			name: "discovery failure due to invalid response_modes_supported",
 			opt: func(t *testing.T) Option {
-				return func(h *handlerState) error { return nil }
+				return func(h *handlerState) error {
+					require.NoError(t, WithClient(newClientForServer(brokenResponseModeServer))(h))
+					return nil
+				}
 			},
 			issuer:   brokenResponseModeServer.URL,
 			wantLogs: []string{"\"level\"=4 \"msg\"=\"Pinniped: Performing OIDC discovery\"  \"issuer\"=\"" + brokenResponseModeServer.URL + "\""},
@@ -403,6 +503,8 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 			clientID: "test-client-id",
 			opt: func(t *testing.T) Option {
 				return func(h *handlerState) error {
+					require.NoError(t, WithClient(newClientForServer(successServer))(h))
+
 					h.getProvider = func(config *oauth2.Config, provider *oidc.Provider, client *http.Client) provider.UpstreamOIDCIdentityProviderI {
 						mock := mockUpstream(t)
 						mock.EXPECT().
@@ -450,6 +552,8 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 			clientID: "test-client-id",
 			opt: func(t *testing.T) Option {
 				return func(h *handlerState) error {
+					require.NoError(t, WithClient(newClientForServer(successServer))(h))
+
 					h.getProvider = func(config *oauth2.Config, provider *oidc.Provider, client *http.Client) provider.UpstreamOIDCIdentityProviderI {
 						mock := mockUpstream(t)
 						mock.EXPECT().
@@ -490,6 +594,8 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 			clientID: "not-the-test-client-id",
 			opt: func(t *testing.T) Option {
 				return func(h *handlerState) error {
+					require.NoError(t, WithClient(newClientForServer(successServer))(h))
+
 					cache := &mockSessionCache{t: t, getReturnsToken: &oidctypes.Token{
 						IDToken: &oidctypes.IDToken{
 							Token:  "expired-test-id-token",
@@ -517,10 +623,59 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 			// Expect this to fall through to the authorization code flow, so it fails here.
 			wantErr: "login failed: must have either a localhost listener or stdin must be a TTY",
 		},
+		{
+			name: "issuer has invalid token URL",
+			opt: func(t *testing.T) Option {
+				return func(h *handlerState) error {
+					require.NoError(t, WithClient(newClientForServer(brokenTokenURLServer))(h))
+					return nil
+				}
+			},
+			issuer:   brokenTokenURLServer.URL,
+			wantLogs: []string{`"level"=4 "msg"="Pinniped: Performing OIDC discovery"  "issuer"="` + brokenTokenURLServer.URL + `"`},
+			wantErr:  `discovered token URL from issuer is not a valid URL: parse "%": invalid URL escape "%"`,
+		},
+		{
+			name: "issuer has insecure token URL",
+			opt: func(t *testing.T) Option {
+				return func(h *handlerState) error {
+					require.NoError(t, WithClient(newClientForServer(insecureTokenURLServer))(h))
+					return nil
+				}
+			},
+			issuer:   insecureTokenURLServer.URL,
+			wantLogs: []string{`"level"=4 "msg"="Pinniped: Performing OIDC discovery"  "issuer"="` + insecureTokenURLServer.URL + `"`},
+			wantErr:  `discovered token URL from issuer must be an https URL, but had scheme "http" instead`,
+		},
+		{
+			name: "issuer has invalid authorize URL",
+			opt: func(t *testing.T) Option {
+				return func(h *handlerState) error {
+					require.NoError(t, WithClient(newClientForServer(brokenAuthURLServer))(h))
+					return nil
+				}
+			},
+			issuer:   brokenAuthURLServer.URL,
+			wantLogs: []string{`"level"=4 "msg"="Pinniped: Performing OIDC discovery"  "issuer"="` + brokenAuthURLServer.URL + `"`},
+			wantErr:  `discovered authorize URL from issuer is not a valid URL: parse "%": invalid URL escape "%"`,
+		},
+		{
+			name: "issuer has insecure authorize URL",
+			opt: func(t *testing.T) Option {
+				return func(h *handlerState) error {
+					require.NoError(t, WithClient(newClientForServer(insecureAuthURLServer))(h))
+					return nil
+				}
+			},
+			issuer:   insecureAuthURLServer.URL,
+			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: "listen failure and non-tty stdin",
 			opt: func(t *testing.T) Option {
 				return func(h *handlerState) error {
+					require.NoError(t, WithClient(newClientForServer(successServer))(h))
 					h.listen = func(net string, addr string) (net.Listener, error) {
 						assert.Equal(t, "tcp", net)
 						assert.Equal(t, "localhost:0", addr)
@@ -544,6 +699,7 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 			name: "listening disabled and manual prompt fails",
 			opt: func(t *testing.T) Option {
 				return func(h *handlerState) error {
+					require.NoError(t, WithClient(newClientForServer(formPostSuccessServer))(h))
 					require.NoError(t, WithSkipListen()(h))
 					h.isTTY = func(fd int) bool { return true }
 					h.openURL = func(authorizeURL string) error {
@@ -570,6 +726,7 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 			name: "listen success and manual prompt succeeds",
 			opt: func(t *testing.T) Option {
 				return func(h *handlerState) error {
+					require.NoError(t, WithClient(newClientForServer(formPostSuccessServer))(h))
 					h.listen = func(string, string) (net.Listener, error) { return nil, fmt.Errorf("some listen error") }
 					h.isTTY = func(fd int) bool { return true }
 					h.openURL = func(authorizeURL string) error {
@@ -596,6 +753,8 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 			name: "timeout waiting for callback",
 			opt: func(t *testing.T) Option {
 				return func(h *handlerState) error {
+					require.NoError(t, WithClient(newClientForServer(successServer))(h))
+
 					ctx, cancel := context.WithCancel(h.ctx)
 					h.ctx = ctx
 
@@ -614,6 +773,7 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 			name: "callback returns error",
 			opt: func(t *testing.T) Option {
 				return func(h *handlerState) error {
+					require.NoError(t, WithClient(newClientForServer(successServer))(h))
 					h.openURL = func(_ string) error {
 						go func() {
 							h.callbacks <- callbackResult{err: fmt.Errorf("some callback error")}
@@ -649,7 +809,10 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 						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))
+
+					client := newClientForServer(successServer)
+					client.Timeout = 10 * time.Second
+					require.NoError(t, WithClient(client)(h))
 
 					h.openURL = func(actualURL string) error {
 						parsedActualURL, err := url.Parse(actualURL)
@@ -710,7 +873,10 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 						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))
+
+					client := newClientForServer(formPostSuccessServer)
+					client.Timeout = 10 * time.Second
+					require.NoError(t, WithClient(client)(h))
 
 					h.openURL = func(actualURL string) error {
 						parsedActualURL, err := url.Parse(actualURL)
@@ -772,9 +938,12 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 						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))
 
+					client := newClientForServer(successServer)
+					client.Timeout = 10 * time.Second
+					require.NoError(t, WithClient(client)(h))
+
 					h.openURL = func(actualURL string) error {
 						parsedActualURL, err := url.Parse(actualURL)
 						require.NoError(t, err)
@@ -851,10 +1020,11 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 			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) {
+					client := newClientForServer(successServer)
+					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":
+						case "https://" + successServer.Listener.Addr().String() + "/.well-known/openid-configuration":
 							type providerJSON struct {
 								Issuer   string `json:"issuer"`
 								AuthURL  string `json:"authorization_endpoint"`
@@ -877,14 +1047,15 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 							require.FailNow(t, fmt.Sprintf("saw unexpected http call from the CLI: %s", req.URL.String()))
 							return nil, nil
 						}
-						}),
+					})
-					})(h))
+					require.NoError(t, WithClient(client)(h))
+
 					return nil
 				}
 			},
 			issuer:   successServer.URL,
 			wantLogs: []string{"\"level\"=4 \"msg\"=\"Pinniped: Performing OIDC discovery\"  \"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&pinniped_idp_name=some-upstream-name&pinniped_idp_type=ldap&redirect_uri=http%3A%2F%2F127.0.0.1%3A0%2Fcallback&response_type=code&scope=test-scope&state=test-state": invalid URL escape "%"`,
+			wantErr:  `discovered authorize URL from issuer is not a valid URL: parse "%": invalid URL escape "%"`,
 		},
 		{
 			name:     "ldap login when there is an error calling the authorization endpoint",
@@ -896,7 +1067,7 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 			},
 			issuer:   successServer.URL,
 			wantLogs: []string{"\"level\"=4 \"msg\"=\"Pinniped: Performing OIDC discovery\"  \"issuer\"=\"" + successServer.URL + "\""},
-			wantErr: `authorization response error: Get "http://` + successServer.Listener.Addr().String() +
+			wantErr: `authorization response error: Get "https://` + 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&pinniped_idp_name=some-upstream-name&pinniped_idp_type=ldap&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`,
 		},
 		{
@@ -1058,13 +1229,13 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 						require.True(t, authorizeRequestWasMade, "should have made an authorize request")
 					})
 
-					require.NoError(t, WithClient(&http.Client{
+					client := newClientForServer(successServer)
-						Transport: roundtripper.Func(func(req *http.Request) (*http.Response, error) {
+					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":
+						case "https://" + successServer.Listener.Addr().String() + "/.well-known/openid-configuration":
 							discoveryRequestWasMade = true
 							return defaultDiscoveryResponse(req)
-							case "http://" + successServer.Listener.Addr().String() + "/authorize":
+						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"))
@@ -1095,8 +1266,8 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 							require.FailNow(t, fmt.Sprintf("saw unexpected http call from the CLI: %s", req.URL.String()))
 							return nil, nil
 						}
-						}),
+					})
-					})(h))
+					require.NoError(t, WithClient(client)(h))
 					return nil
 				}
 			},
@@ -1165,13 +1336,13 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 						require.True(t, authorizeRequestWasMade, "should have made an authorize request")
 					})
 
-					require.NoError(t, WithClient(&http.Client{
+					client := newClientForServer(successServer)
-						Transport: roundtripper.Func(func(req *http.Request) (*http.Response, error) {
+					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":
+						case "https://" + successServer.Listener.Addr().String() + "/.well-known/openid-configuration":
 							discoveryRequestWasMade = true
 							return defaultDiscoveryResponse(req)
-							case "http://" + successServer.Listener.Addr().String() + "/authorize":
+						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"))
@@ -1202,8 +1373,8 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 							require.FailNow(t, fmt.Sprintf("saw unexpected http call from the CLI: %s", req.URL.String()))
 							return nil, nil
 						}
-						}),
+					})
-					})(h))
+					require.NoError(t, WithClient(client)(h))
 					return nil
 				}
 			},
@@ -1276,13 +1447,13 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 						require.True(t, authorizeRequestWasMade, "should have made an authorize request")
 					})
 
-					require.NoError(t, WithClient(&http.Client{
+					client := newClientForServer(successServer)
-						Transport: roundtripper.Func(func(req *http.Request) (*http.Response, error) {
+					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":
+						case "https://" + successServer.Listener.Addr().String() + "/.well-known/openid-configuration":
 							discoveryRequestWasMade = true
 							return defaultDiscoveryResponse(req)
-							case "http://" + successServer.Listener.Addr().String() + "/authorize":
+						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"))
@@ -1313,8 +1484,8 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 							require.FailNow(t, fmt.Sprintf("saw unexpected http call from the CLI: %s", req.URL.String()))
 							return nil, nil
 						}
-						}),
+					})
-					})(h))
+					require.NoError(t, WithClient(client)(h))
 					return nil
 				}
 			},
@@ -1342,6 +1513,7 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 						}}, cache.sawGetKeys)
 						require.Empty(t, cache.sawPutTokens)
 					})
+					require.NoError(t, WithClient(newClientForServer(errorServer))(h))
 					require.NoError(t, WithSessionCache(cache)(h))
 					require.NoError(t, WithRequestAudience("cluster-1234")(h))
 					return nil
@@ -1352,6 +1524,33 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 				"\"level\"=4 \"msg\"=\"Pinniped: Performing OIDC discovery\"  \"issuer\"=\"" + errorServer.URL + "\""},
 			wantErr: fmt.Sprintf("failed to exchange token: could not perform OIDC discovery for %q: 500 Internal Server Error: some discovery error\n", errorServer.URL),
 		},
+		{
+			name:     "with requested audience, session cache hit with valid token, but token URL is insecure",
+			issuer:   insecureTokenURLServer.URL,
+			clientID: "test-client-id",
+			opt: func(t *testing.T) Option {
+				return func(h *handlerState) error {
+					cache := &mockSessionCache{t: t, getReturnsToken: &testToken}
+					t.Cleanup(func() {
+						require.Equal(t, []SessionCacheKey{{
+							Issuer:      insecureTokenURLServer.URL,
+							ClientID:    "test-client-id",
+							Scopes:      []string{"test-scope"},
+							RedirectURI: "http://localhost:0/callback",
+						}}, cache.sawGetKeys)
+						require.Empty(t, cache.sawPutTokens)
+					})
+					require.NoError(t, WithClient(newClientForServer(insecureTokenURLServer))(h))
+					require.NoError(t, WithSessionCache(cache)(h))
+					require.NoError(t, WithRequestAudience("cluster-1234")(h))
+					return nil
+				}
+			},
+			wantLogs: []string{"\"level\"=4 \"msg\"=\"Pinniped: Found unexpired cached token.\"",
+				"\"level\"=4 \"msg\"=\"Pinniped: Performing RFC8693 token exchange\"  \"requestedAudience\"=\"cluster-1234\"",
+				"\"level\"=4 \"msg\"=\"Pinniped: Performing OIDC discovery\"  \"issuer\"=\"" + insecureTokenURLServer.URL + "\""},
+			wantErr: `failed to exchange token: discovered token URL from issuer must be an https URL, but had scheme "http" instead`,
+		},
 		{
 			name:     "with requested audience, session cache hit with valid token, but token URL is invalid",
 			issuer:   brokenTokenURLServer.URL,
@@ -1368,6 +1567,7 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 						}}, cache.sawGetKeys)
 						require.Empty(t, cache.sawPutTokens)
 					})
+					require.NoError(t, WithClient(newClientForServer(brokenTokenURLServer))(h))
 					require.NoError(t, WithSessionCache(cache)(h))
 					require.NoError(t, WithRequestAudience("cluster-1234")(h))
 					return nil
@@ -1376,7 +1576,7 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 			wantLogs: []string{"\"level\"=4 \"msg\"=\"Pinniped: Found unexpired cached token.\"",
 				"\"level\"=4 \"msg\"=\"Pinniped: Performing RFC8693 token exchange\"  \"requestedAudience\"=\"cluster-1234\"",
 				"\"level\"=4 \"msg\"=\"Pinniped: Performing OIDC discovery\"  \"issuer\"=\"" + brokenTokenURLServer.URL + "\""},
-			wantErr: `failed to exchange token: could not build RFC8693 request: parse "%": invalid URL escape "%"`,
+			wantErr: `failed to exchange token: discovered token URL from issuer is not a valid URL: parse "%": invalid URL escape "%"`,
 		},
 		{
 			name:     "with requested audience, session cache hit with valid token, but token exchange request fails",
@@ -1394,6 +1594,7 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 						}}, cache.sawGetKeys)
 						require.Empty(t, cache.sawPutTokens)
 					})
+					require.NoError(t, WithClient(newClientForServer(successServer))(h))
 					require.NoError(t, WithSessionCache(cache)(h))
 					require.NoError(t, WithRequestAudience("test-audience-produce-invalid-http-response")(h))
 					return nil
@@ -1420,6 +1621,7 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 						}}, cache.sawGetKeys)
 						require.Empty(t, cache.sawPutTokens)
 					})
+					require.NoError(t, WithClient(newClientForServer(successServer))(h))
 					require.NoError(t, WithSessionCache(cache)(h))
 					require.NoError(t, WithRequestAudience("test-audience-produce-http-400")(h))
 					return nil
@@ -1446,6 +1648,7 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 						}}, cache.sawGetKeys)
 						require.Empty(t, cache.sawPutTokens)
 					})
+					require.NoError(t, WithClient(newClientForServer(successServer))(h))
 					require.NoError(t, WithSessionCache(cache)(h))
 					require.NoError(t, WithRequestAudience("test-audience-produce-invalid-content-type")(h))
 					return nil
@@ -1472,6 +1675,7 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 						}}, cache.sawGetKeys)
 						require.Empty(t, cache.sawPutTokens)
 					})
+					require.NoError(t, WithClient(newClientForServer(successServer))(h))
 					require.NoError(t, WithSessionCache(cache)(h))
 					require.NoError(t, WithRequestAudience("test-audience-produce-wrong-content-type")(h))
 					return nil
@@ -1498,6 +1702,7 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 						}}, cache.sawGetKeys)
 						require.Empty(t, cache.sawPutTokens)
 					})
+					require.NoError(t, WithClient(newClientForServer(successServer))(h))
 					require.NoError(t, WithSessionCache(cache)(h))
 					require.NoError(t, WithRequestAudience("test-audience-produce-invalid-json")(h))
 					return nil
@@ -1524,6 +1729,7 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 						}}, cache.sawGetKeys)
 						require.Empty(t, cache.sawPutTokens)
 					})
+					require.NoError(t, WithClient(newClientForServer(successServer))(h))
 					require.NoError(t, WithSessionCache(cache)(h))
 					require.NoError(t, WithRequestAudience("test-audience-produce-invalid-tokentype")(h))
 					return nil
@@ -1550,6 +1756,7 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 						}}, cache.sawGetKeys)
 						require.Empty(t, cache.sawPutTokens)
 					})
+					require.NoError(t, WithClient(newClientForServer(successServer))(h))
 					require.NoError(t, WithSessionCache(cache)(h))
 					require.NoError(t, WithRequestAudience("test-audience-produce-invalid-issuedtokentype")(h))
 					return nil
@@ -1576,6 +1783,7 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 						}}, cache.sawGetKeys)
 						require.Empty(t, cache.sawPutTokens)
 					})
+					require.NoError(t, WithClient(newClientForServer(successServer))(h))
 					require.NoError(t, WithSessionCache(cache)(h))
 					require.NoError(t, WithRequestAudience("test-audience-produce-invalid-jwt")(h))
 					return nil
@@ -1602,6 +1810,7 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 						}}, cache.sawGetKeys)
 						require.Empty(t, cache.sawPutTokens)
 					})
+					require.NoError(t, WithClient(newClientForServer(successServer))(h))
 					require.NoError(t, WithSessionCache(cache)(h))
 					require.NoError(t, WithRequestAudience("test-audience")(h))
 
@@ -1624,6 +1833,8 @@ func TestLogin(t *testing.T) { // nolint:gocyclo
 			clientID: "test-client-id",
 			opt: func(t *testing.T) Option {
 				return func(h *handlerState) error {
+					require.NoError(t, WithClient(newClientForServer(successServer))(h))
+
 					cache := &mockSessionCache{t: t, getReturnsToken: &oidctypes.Token{
 						IDToken: &oidctypes.IDToken{
 							Token:  "expired-test-id-token",