From a21a5bca1e31a88a8d127c31372fbc0479800b78 Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Sun, 13 Feb 2022 12:48:20 -0500 Subject: [PATCH 1/7] Update dex docs regarding password grant Signed-off-by: Monis Khan --- site/content/docs/howto/configure-supervisor-with-dex.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/site/content/docs/howto/configure-supervisor-with-dex.md b/site/content/docs/howto/configure-supervisor-with-dex.md index 79a5ea35b..df8876ff7 100644 --- a/site/content/docs/howto/configure-supervisor-with-dex.md +++ b/site/content/docs/howto/configure-supervisor-with-dex.md @@ -95,8 +95,7 @@ spec: # If you would also like to allow your end users to authenticate using # a password grant, then change this to true. - # Password grants with Dex will only work in Dex versions that include - # this bug fix: https://github.com/dexidp/dex/pull/2234 + # Password grant requires Dex v2.31.0 or later allowPasswordGrant: false # Specify how Dex claims are mapped to Kubernetes identities. From b0c36c663356b60e7dc89d6e5bf0fe910d80e6a8 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Tue, 15 Feb 2022 11:19:49 -0800 Subject: [PATCH 2/7] Fix int test that was failing on MacOS, and some small doc changes --- CONTRIBUTING.md | 14 +++++++++++--- GOVERNANCE.md | 4 ++-- hack/prepare-for-integration-tests.sh | 3 ++- test/integration/e2e_test.go | 12 +++++------- 4 files changed, 20 insertions(+), 13 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 39bd6e942..1e0d9aa78 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -72,7 +72,7 @@ Please follow the procedure described in [SECURITY.md](SECURITY.md). ## CLA -We welcome contributions from everyone but we can only accept them if you sign +We welcome contributions from everyone but, we can only accept them if you sign our Contributor License Agreement (CLA). If you would like to contribute and you have not signed it, our CLA-bot will walk you through the process when you open a Pull Request. For questions about the CLA process, see the @@ -82,13 +82,21 @@ tracker. ## Building The [Dockerfile](Dockerfile) at the root of the repo can be used to build and -package the code. After making a change to the code, rebuild the docker image with the following command. +package the server-side code. After making a change to the code, rebuild the +docker image with the following command. ```bash # From the root directory of the repo... docker build . ``` +The Pinniped CLI client can be built for local use with the following command. + +```bash +# From the root directory of the repo... +go build -o pinniped ./cmd/pinniped +``` + ## Testing ### Running Lint @@ -122,7 +130,7 @@ docker build . brew install kind k14s/tap/ytt k14s/tap/kapp kubectl chromedriver nmap && brew cask install docker ``` -1. Create a kind cluster, compile, create container images, and install Pinniped and supporting dependencies using: +1. Create a kind cluster, compile, create container images, and install Pinniped and supporting test dependencies using: ```bash ./hack/prepare-for-integration-tests.sh diff --git a/GOVERNANCE.md b/GOVERNANCE.md index 679648f6c..8de08d8c5 100644 --- a/GOVERNANCE.md +++ b/GOVERNANCE.md @@ -31,7 +31,7 @@ A supermajority is defined as two-thirds of members in the group. A supermajorit Ideally, all project decisions are resolved by consensus. If impossible, any maintainer may call a vote. Unless otherwise specified in this document, any vote will be decided by a supermajority of maintainers. --- -# Proposal Process +# Proposal Process The proposal process is currently being worked on. No formal process is available at this time. You may reach out to the maintainers in the Kubernetes Slack Workspace within the [#pinniped](https://kubernetes.slack.com/archives/C01BW364RJA) channel or on the [Pinniped mailing list](project-pinniped@googlegroups.com) with any questions you may have or to send us your proposals. --- @@ -48,4 +48,4 @@ Lazy consensus does not apply to the process of: --- # Updating Governance -All substantive changes in Governance require a supermajority agreement by all maintainers. \ No newline at end of file +All substantive changes in Governance require a supermajority agreement by all maintainers. diff --git a/hack/prepare-for-integration-tests.sh b/hack/prepare-for-integration-tests.sh index 2b2518991..6fb4e63c1 100755 --- a/hack/prepare-for-integration-tests.sh +++ b/hack/prepare-for-integration-tests.sh @@ -1,6 +1,6 @@ #!/usr/bin/env bash -# Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. +# Copyright 2020-2022 the Pinniped contributors. All Rights Reserved. # SPDX-License-Identifier: Apache-2.0 # @@ -420,6 +420,7 @@ EOF log_note log_note "🚀 Ready to run integration tests! For example..." log_note " cd $pinniped_path" +log_note " ulimit -n 512" log_note ' source /tmp/integration-test-env && go test -v -race -count 1 -timeout 0 ./test/integration' log_note log_note "Using GoLand? Paste the result of this command into GoLand's run configuration \"Environment\"." diff --git a/test/integration/e2e_test.go b/test/integration/e2e_test.go index 2af62cc2f..88c189ba6 100644 --- a/test/integration/e2e_test.go +++ b/test/integration/e2e_test.go @@ -449,8 +449,6 @@ func TestE2EFullIntegration(t *testing.T) { // nolint:gocyclo start := time.Now() kubectlCmd := exec.CommandContext(ctx, "kubectl", "get", "namespace", "--kubeconfig", kubeconfigPath) kubectlCmd.Env = append(os.Environ(), env.ProxyEnv()...) - stdoutPipe, err := kubectlCmd.StdoutPipe() - require.NoError(t, err) ptyFile, err := pty.Start(kubectlCmd) require.NoError(t, err) @@ -492,10 +490,10 @@ func TestE2EFullIntegration(t *testing.T) { // nolint:gocyclo t.Logf("waiting for kubectl to output namespace list") // Read all output from the subprocess until EOF. // Ignore any errors returned because there is always an error on linux. - kubectlStdOutOutputBytes, _ := ioutil.ReadAll(stdoutPipe) - kubectlStdErrOutputBytes, _ := ioutil.ReadAll(ptyFile) - requireKubectlGetNamespaceOutput(t, env, string(kubectlStdOutOutputBytes)) - require.Contains(t, string(kubectlStdErrOutputBytes), "Access token from identity provider has lifetime of less than 3 hours. Expect frequent prompts to log in.") + kubectlOutputBytes, _ := ioutil.ReadAll(ptyFile) + requireKubectlGetNamespaceOutput(t, env, string(kubectlOutputBytes)) + // This warning should be on stderr, but with pty on MacOS it's hard to assert that specifically. + require.Contains(t, string(kubectlOutputBytes), "Access token from identity provider has lifetime of less than 3 hours. Expect frequent prompts to log in.") t.Logf("first kubectl command took %s", time.Since(start).String()) @@ -1013,7 +1011,7 @@ func readFromFileUntilStringIsSeen(t *testing.T, f *os.File, until string) strin return true, nil // found it! finished. } if foundEOF { - return false, fmt.Errorf("reached EOF of subcommand's output without seeing expected string %q", until) + return false, fmt.Errorf("reached EOF of subcommand's output without seeing expected string %q. Output read so far was:\n%s", until, readFromFile) } return false, nil // keep waiting and reading }, 1*time.Minute, 1*time.Second) From 1aa17bd84de348539e633489389722f5f8d7d56d Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Tue, 15 Feb 2022 13:45:04 -0800 Subject: [PATCH 3/7] Check for darwin before relaxing stderr vs stdout assertion in e2e test --- test/integration/e2e_test.go | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/test/integration/e2e_test.go b/test/integration/e2e_test.go index 88c189ba6..ccf8d1147 100644 --- a/test/integration/e2e_test.go +++ b/test/integration/e2e_test.go @@ -17,6 +17,7 @@ import ( "os/exec" "path/filepath" "regexp" + "runtime" "sort" "strings" "sync/atomic" @@ -449,6 +450,13 @@ func TestE2EFullIntegration(t *testing.T) { // nolint:gocyclo start := time.Now() kubectlCmd := exec.CommandContext(ctx, "kubectl", "get", "namespace", "--kubeconfig", kubeconfigPath) kubectlCmd.Env = append(os.Environ(), env.ProxyEnv()...) + var kubectlStdoutPipe io.ReadCloser + if runtime.GOOS != "darwin" { + // For some unknown reason this breaks the pty library on some MacOS machines. + // The problem doesn't reproduce for everyone, so this is just a workaround. + kubectlStdoutPipe, err = kubectlCmd.StdoutPipe() + require.NoError(t, err) + } ptyFile, err := pty.Start(kubectlCmd) require.NoError(t, err) @@ -490,10 +498,19 @@ func TestE2EFullIntegration(t *testing.T) { // nolint:gocyclo t.Logf("waiting for kubectl to output namespace list") // Read all output from the subprocess until EOF. // Ignore any errors returned because there is always an error on linux. - kubectlOutputBytes, _ := ioutil.ReadAll(ptyFile) - requireKubectlGetNamespaceOutput(t, env, string(kubectlOutputBytes)) - // This warning should be on stderr, but with pty on MacOS it's hard to assert that specifically. - require.Contains(t, string(kubectlOutputBytes), "Access token from identity provider has lifetime of less than 3 hours. Expect frequent prompts to log in.") + kubectlPtyOutputBytes, _ := ioutil.ReadAll(ptyFile) + if kubectlStdoutPipe != nil { + // On non-MacOS check that stdout of the CLI contains the expected output. + kubectlStdOutOutputBytes, _ := ioutil.ReadAll(kubectlStdoutPipe) + requireKubectlGetNamespaceOutput(t, env, string(kubectlStdOutOutputBytes)) + } else { + // On MacOS check that the pty (stdout+stderr+stdin) of the CLI contains the expected output. + requireKubectlGetNamespaceOutput(t, env, string(kubectlPtyOutputBytes)) + } + // Due to the GOOS check in the code above, on MacOS the pty will include stdout, and other platforms it will not. + // This warning message is supposed to be printed by the CLI on stderr. + require.Contains(t, string(kubectlPtyOutputBytes), + "Access token from identity provider has lifetime of less than 3 hours. Expect frequent prompts to log in.") t.Logf("first kubectl command took %s", time.Since(start).String()) From 3f4e6cf3677dac08f234c104dd3d421a1962e7e1 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Tue, 15 Feb 2022 16:45:49 -0800 Subject: [PATCH 4/7] Fix a typo in CONTRIBUTING.md from a recent commit: comma in wrong place --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1e0d9aa78..ddef26d0f 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -72,7 +72,7 @@ Please follow the procedure described in [SECURITY.md](SECURITY.md). ## CLA -We welcome contributions from everyone but, we can only accept them if you sign +We welcome contributions from everyone, but we can only accept them if you sign our Contributor License Agreement (CLA). If you would like to contribute and you have not signed it, our CLA-bot will walk you through the process when you open a Pull Request. For questions about the CLA process, see the From b8202d89d94981059d99b2391c7964485a580bdf Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Wed, 16 Feb 2022 09:20:28 -0500 Subject: [PATCH 5/7] Enforce naming convention for browser based tests This allows us to target browser based tests with the regex: go test -v -race -count 1 -timeout 0 ./test/integration -run '/_Browser' New tests that call browsertest.Open will automatically be forced to follow this convention. Signed-off-by: Monis Khan --- test/integration/cli_test.go | 2 +- .../concierge_credentialrequest_test.go | 4 +-- test/integration/e2e_test.go | 4 +-- test/integration/formposthtml_test.go | 2 +- test/integration/supervisor_login_test.go | 2 +- test/testlib/browsertest/browsertest.go | 28 ++++++++++++++++++- 6 files changed, 34 insertions(+), 8 deletions(-) diff --git a/test/integration/cli_test.go b/test/integration/cli_test.go index 263c91543..605f590a8 100644 --- a/test/integration/cli_test.go +++ b/test/integration/cli_test.go @@ -170,7 +170,7 @@ func deserializeWhoAmIRequest(t *testing.T, data string, apiGroupSuffix string) return obj.(*identityv1alpha1.WhoAmIRequest) } -func TestCLILoginOIDC(t *testing.T) { +func TestCLILoginOIDC_Browser(t *testing.T) { env := testlib.IntegrationEnv(t) ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) diff --git a/test/integration/concierge_credentialrequest_test.go b/test/integration/concierge_credentialrequest_test.go index d403a8d09..2bd657ad0 100644 --- a/test/integration/concierge_credentialrequest_test.go +++ b/test/integration/concierge_credentialrequest_test.go @@ -45,10 +45,10 @@ func TestUnsuccessfulCredentialRequest_Parallel(t *testing.T) { require.Equal(t, "authentication failed", *response.Status.Message) } -// TestSuccessfulCredentialRequest cannot run in parallel because runPinnipedLoginOIDC uses a fixed port +// TestSuccessfulCredentialRequest_Browser cannot run in parallel because runPinnipedLoginOIDC uses a fixed port // for its localhost listener via --listen-port=env.CLIUpstreamOIDC.CallbackURL.Port() per oidcLoginCommand. // Since ports are global to the process, tests using oidcLoginCommand must be run serially. -func TestSuccessfulCredentialRequest(t *testing.T) { +func TestSuccessfulCredentialRequest_Browser(t *testing.T) { env := testlib.IntegrationEnv(t).WithCapability(testlib.ClusterSigningKeyIsAvailable) ctx, cancel := context.WithTimeout(context.Background(), 6*time.Minute) diff --git a/test/integration/e2e_test.go b/test/integration/e2e_test.go index 2af62cc2f..decfc14b7 100644 --- a/test/integration/e2e_test.go +++ b/test/integration/e2e_test.go @@ -47,8 +47,8 @@ import ( "go.pinniped.dev/test/testlib/browsertest" ) -// TestE2EFullIntegration tests a full integration scenario that combines the supervisor, concierge, and CLI. -func TestE2EFullIntegration(t *testing.T) { // nolint:gocyclo +// TestE2EFullIntegration_Browser tests a full integration scenario that combines the supervisor, concierge, and CLI. +func TestE2EFullIntegration_Browser(t *testing.T) { // nolint:gocyclo env := testlib.IntegrationEnv(t) ctx, cancelFunc := context.WithTimeout(context.Background(), 10*time.Minute) diff --git a/test/integration/formposthtml_test.go b/test/integration/formposthtml_test.go index d891488e5..7bbadee98 100644 --- a/test/integration/formposthtml_test.go +++ b/test/integration/formposthtml_test.go @@ -25,7 +25,7 @@ import ( ) // safe to run in parallel with serial tests since it only interacts with a test local server, see main_test.go. -func TestFormPostHTML_Parallel(t *testing.T) { +func TestFormPostHTML_Browser_Parallel(t *testing.T) { _ = testlib.IntegrationEnv(t) // Run a mock callback handler, simulating the one running in the CLI. diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index 1fec09894..d8c9d31c6 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -47,7 +47,7 @@ import ( ) // nolint:gocyclo -func TestSupervisorLogin(t *testing.T) { +func TestSupervisorLogin_Browser(t *testing.T) { env := testlib.IntegrationEnv(t) tests := []struct { diff --git a/test/testlib/browsertest/browsertest.go b/test/testlib/browsertest/browsertest.go index c94850d26..d6686ea6d 100644 --- a/test/testlib/browsertest/browsertest.go +++ b/test/testlib/browsertest/browsertest.go @@ -6,6 +6,7 @@ package browsertest import ( "regexp" + "strings" "testing" "time" @@ -20,10 +21,16 @@ const ( operationPollingInterval = 100 * time.Millisecond ) -// Open a webdriver-driven browser and returns an *agouti.Page to control it. The browser will be automatically +// Open a webdriver-driven browser and returns an *agouti.Page to control it. The browser will be automatically // closed at the end of the current test. It is configured for test purposes with the correct HTTP proxy and // in a mode that ignore certificate errors. func Open(t *testing.T) *agouti.Page { + t.Helper() + + // make it trivial to run all browser based tests via: + // go test -v -race -count 1 -timeout 0 ./test/integration -run '/_Browser' + require.Contains(t, rootTestName(t), "_Browser", "browser based tests must contain the string _Browser in their name") + t.Logf("opening browser driver") env := testlib.IntegrationEnv(t) caps := agouti.NewCapabilities() @@ -58,6 +65,25 @@ func Open(t *testing.T) *agouti.Page { return page } +func rootTestName(t *testing.T) string { + switch names := strings.SplitN(t.Name(), "/", 3); len(names) { + case 0: + panic("impossible") + + case 1: + return names[0] + + case 2, 3: + if strings.HasPrefix(names[0], "TestIntegration") { + return names[1] + } + return names[0] + + default: + panic("impossible") + } +} + // WaitForVisibleElements expects the page to contain all the the elements specified by the selectors. It waits for this // to occur and times out, failing the test, if they never appear. func WaitForVisibleElements(t *testing.T, page *agouti.Page, selectors ...string) { From 79467318f48fcf45093ef643ffc29fe64bd8c6bc Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Wed, 16 Feb 2022 10:41:51 -0800 Subject: [PATCH 6/7] CLI requires HTTPS OIDC issuer, authorize, and token URLS --- pkg/oidcclient/login.go | 28 ++ pkg/oidcclient/login_test.go | 555 ++++++++++++++++++++++++----------- 2 files changed, 411 insertions(+), 172 deletions(-) diff --git a/pkg/oidcclient/login.go b/pkg/oidcclient/login.go index afda042a5..39f375d36 100644 --- a/pkg/oidcclient/login.go +++ b/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 { diff --git a/pkg/oidcclient/login_test.go b/pkg/oidcclient/login_test.go index fe6a8ce5a..5a3c176a5 100644 --- a/pkg/oidcclient/login_test.go +++ b/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 - errorServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Start a test server that returns 500 errors. + errorServer := tlsserver.TLSTestServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { http.Error(w, "some discovery error", http.StatusInternalServerError) - })) - t.Cleanup(errorServer.Close) + }), nil) // 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) - t.Cleanup(successServer.Close) + successServer := tlsserver.TLSTestServer(t, providerMux, nil) 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) - t.Cleanup(formPostSuccessServer.Close) + formPostSuccessServer := tlsserver.TLSTestServer(t, formPostProviderMux, nil) 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", - issuer: "test-issuer", + name: "issuer is not https", + 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\""}, - wantErr: `could not perform OIDC discovery for "test-issuer": Get "test-issuer/.well-known/openid-configuration": unsupported protocol scheme ""`, + wantLogs: []string{"\"level\"=4 \"msg\"=\"Pinniped: Performing OIDC discovery\" \"issuer\"=\"" + errorServer.URL + "\""}, + 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,40 +1020,42 @@ 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) { - 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 + + 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 "https://" + 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"` } - }), - })(h)) + 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 + } + }) + 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,45 +1229,45 @@ func TestLogin(t *testing.T) { // nolint:gocyclo 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("Pinniped-Username")) - require.Equal(t, "some-upstream-password", req.Header.Get("Pinniped-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"}, - "pinniped_idp_name": []string{"some-upstream-name"}, - "pinniped_idp_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)) + 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 "https://" + successServer.Listener.Addr().String() + "/.well-known/openid-configuration": + discoveryRequestWasMade = true + return defaultDiscoveryResponse(req) + 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")) + 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"}, + "pinniped_idp_name": []string{"some-upstream-name"}, + "pinniped_idp_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 + } + }) + require.NoError(t, WithClient(client)(h)) return nil } }, @@ -1165,45 +1336,45 @@ func TestLogin(t *testing.T) { // nolint:gocyclo 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("Pinniped-Username")) - require.Equal(t, "some-upstream-password", req.Header.Get("Pinniped-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"}, - "pinniped_idp_name": []string{"some-upstream-name"}, - "pinniped_idp_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)) + 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 "https://" + successServer.Listener.Addr().String() + "/.well-known/openid-configuration": + discoveryRequestWasMade = true + return defaultDiscoveryResponse(req) + 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")) + 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"}, + "pinniped_idp_name": []string{"some-upstream-name"}, + "pinniped_idp_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 + } + }) + require.NoError(t, WithClient(client)(h)) return nil } }, @@ -1276,45 +1447,45 @@ func TestLogin(t *testing.T) { // nolint:gocyclo 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("Pinniped-Username")) - require.Equal(t, "some-upstream-password", req.Header.Get("Pinniped-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"}, - "pinniped_idp_name": []string{"some-upstream-name"}, - "pinniped_idp_type": []string{"ldap"}, - }, req.URL.Query()) - return &http.Response{ - StatusCode: http.StatusSeeOther, - 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)) + 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 "https://" + successServer.Listener.Addr().String() + "/.well-known/openid-configuration": + discoveryRequestWasMade = true + return defaultDiscoveryResponse(req) + 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")) + 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"}, + "pinniped_idp_name": []string{"some-upstream-name"}, + "pinniped_idp_type": []string{"ldap"}, + }, req.URL.Query()) + return &http.Response{ + StatusCode: http.StatusSeeOther, + 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 + } + }) + 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", From e5a60a8c841e180b4d421f9ee50082ce0c048dae Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Wed, 16 Feb 2022 11:09:05 -0800 Subject: [PATCH 7/7] Update a comment --- internal/oidc/provider/formposthtml/form_post.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/oidc/provider/formposthtml/form_post.js b/internal/oidc/provider/formposthtml/form_post.js index 9e4b02036..cb73c8cd4 100644 --- a/internal/oidc/provider/formposthtml/form_post.js +++ b/internal/oidc/provider/formposthtml/form_post.js @@ -53,8 +53,8 @@ window.onload = () => { // Requests made using "no-cors" mode will hide the real response.status by making it 0 // and the real response.ok by making it false. // If the real response was success, then we would like to show the success state. - // If the real response was an error, then we wish we could show the manual - // state, but we have no way to know that, as long as we are making "no-cors" requests. + // If the real response was an error, then we wish we could do something else (maybe show the error?), + // but we have no way to know the real response as long as we are making "no-cors" requests. // For now, show the success status for all responses. // In the future, we could make this request in "cors" mode once old versions of our CLI // which did not handle CORS are upgraded out by our users. That would allow us to use