Split test environment variables so there's a specific supervisor upstream client.

Prior to this we re-used the CLI testing client to test the authorize flow of the supervisor, but they really need to be separate upstream clients. For example, the supervisor client should be a non-public client with a client secret and a different callback endpoint.

Signed-off-by: Matt Moyer <moyerm@vmware.com>

hack/prepare-for-integration-tests.sh

@@ -295,9 +295,16 @@ export PINNIPED_TEST_PROXY=http://127.0.0.1:12346
 export PINNIPED_TEST_CLI_OIDC_ISSUER=https://dex.dex.svc.cluster.local/dex
 export PINNIPED_TEST_CLI_OIDC_ISSUER_CA_BUNDLE="${test_ca_bundle_pem}"
 export PINNIPED_TEST_CLI_OIDC_CLIENT_ID=pinniped-cli
-export PINNIPED_TEST_CLI_OIDC_LOCALHOST_PORT=48095
+export PINNIPED_TEST_CLI_OIDC_CALLBACK_URL=http://127.0.0.1:48095/callback
 export PINNIPED_TEST_CLI_OIDC_USERNAME=pinny@example.com
 export PINNIPED_TEST_CLI_OIDC_PASSWORD=password
+export PINNIPED_TEST_SUPERVISOR_UPSTREAM_OIDC_ISSUER=https://dex.dex.svc.cluster.local/dex
+export PINNIPED_TEST_SUPERVISOR_UPSTREAM_OIDC_ISSUER_CA_BUNDLE="${test_ca_bundle_pem}"
+export PINNIPED_TEST_SUPERVISOR_UPSTREAM_OIDC_CLIENT_ID=pinniped-supervisor
+export PINNIPED_TEST_SUPERVISOR_UPSTREAM_OIDC_CLIENT_SECRET=pinniped-supervisor-secret
+export PINNIPED_TEST_SUPERVISOR_UPSTREAM_OIDC_CALLBACK_URL=https://127.0.0.1:12345/some/path/callback
+export PINNIPED_TEST_SUPERVISOR_UPSTREAM_OIDC_USERNAME=pinny@example.com
+export PINNIPED_TEST_SUPERVISOR_UPSTREAM_OIDC_PASSWORD=password
 
 read -r -d '' PINNIPED_TEST_CLUSTER_CAPABILITY_YAML << PINNIPED_TEST_CLUSTER_CAPABILITY_YAML_EOF || true
 ${pinniped_cluster_capability_file_content}

test/deploy/dex/dex.yaml

@@ -24,6 +24,12 @@ staticClients:
   redirectURIs:
   - #@ "http://127.0.0.1:" + str(data.values.ports.cli) + "/callback"
   - #@ "http://[::1]:" + str(data.values.ports.cli) + "/callback"
+- id: pinniped-supervisor
+  name: 'Pinniped Supervisor'
+  secret: pinniped-supervisor-secret
+  redirectURIs:
+    - #@ "http://127.0.0.1:" + str(data.values.ports.cli) + "/callback"
+    - #@ "http://[::1]:" + str(data.values.ports.cli) + "/callback"
 enablePasswordDB: true
 staticPasswords:
 - username: "pinny"

test/integration/cli_test.go

@@ -11,11 +11,11 @@ import (
 	"fmt"
 	"io"
 	"io/ioutil"
+	"net/url"
 	"os"
 	"os/exec"
 	"path/filepath"
 	"regexp"
-	"strconv"
 	"strings"
 	"testing"
 	"time"
@@ -118,7 +118,7 @@ type loginProviderPatterns struct {
 
 func getLoginProvider(t *testing.T) *loginProviderPatterns {
 	t.Helper()
-	issuer := library.IntegrationEnv(t).OIDCUpstream.Issuer
+	issuer := library.IntegrationEnv(t).CLITestUpstream.Issuer
 	for _, p := range []loginProviderPatterns{
 		{
 			Name:                "Okta",
@@ -270,13 +270,13 @@ func TestCLILoginOIDC(t *testing.T) {
 
 	// Fill in the username and password and click "submit".
 	t.Logf("logging into %s", loginProvider.Name)
-	require.NoError(t, page.First(loginProvider.UsernameSelector).Fill(env.OIDCUpstream.Username))
+	require.NoError(t, page.First(loginProvider.UsernameSelector).Fill(env.CLITestUpstream.Username))
-	require.NoError(t, page.First(loginProvider.PasswordSelector).Fill(env.OIDCUpstream.Password))
+	require.NoError(t, page.First(loginProvider.PasswordSelector).Fill(env.CLITestUpstream.Password))
 	require.NoError(t, page.First(loginProvider.LoginButtonSelector).Click())
 
 	// Wait for the login to happen and us be redirected back to a localhost callback.
 	t.Logf("waiting for redirect to localhost callback")
-	callbackURLPattern := regexp.MustCompile(`\Ahttp://127.0.0.1:` + strconv.Itoa(env.OIDCUpstream.LocalhostPort) + `/.+\z`)
+	callbackURLPattern := regexp.MustCompile(`\A` + regexp.QuoteMeta(env.CLITestUpstream.CallbackURL) + `\?.+\z`)
 	waitForURL(t, page, callbackURLPattern)
 
 	// Wait for the "pre" element that gets rendered for a `text/plain` page, and
@@ -313,9 +313,9 @@ func TestCLILoginOIDC(t *testing.T) {
 	require.NoError(t, err)
 	claims := map[string]interface{}{}
 	require.NoError(t, json.Unmarshal(jws.UnsafePayloadWithoutVerification(), &claims))
-	require.Equal(t, env.OIDCUpstream.Issuer, claims["iss"])
+	require.Equal(t, env.CLITestUpstream.Issuer, claims["iss"])
-	require.Equal(t, env.OIDCUpstream.ClientID, claims["aud"])
+	require.Equal(t, env.CLITestUpstream.ClientID, claims["aud"])
-	require.Equal(t, env.OIDCUpstream.Username, claims["email"])
+	require.Equal(t, env.CLITestUpstream.Username, claims["email"])
 	require.NotEmpty(t, claims["nonce"])
 
 	// Run the CLI again with the same session cache and login parameters.
@@ -334,10 +334,10 @@ func TestCLILoginOIDC(t *testing.T) {
 	t.Logf("overwriting cache to remove valid ID token")
 	cache := filesession.New(sessionCachePath)
 	cacheKey := oidcclient.SessionCacheKey{
-		Issuer:      env.OIDCUpstream.Issuer,
+		Issuer:      env.CLITestUpstream.Issuer,
-		ClientID:    env.OIDCUpstream.ClientID,
+		ClientID:    env.CLITestUpstream.ClientID,
 		Scopes:      []string{"email", "offline_access", "openid", "profile"},
-		RedirectURI: fmt.Sprintf("http://localhost:%d/callback", env.OIDCUpstream.LocalhostPort),
+		RedirectURI: strings.ReplaceAll(env.CLITestUpstream.CallbackURL, "127.0.0.1", "localhost"),
 	}
 	cached := cache.GetToken(cacheKey)
 	require.NotNil(t, cached)
@@ -378,10 +378,24 @@ func waitForVisibleElements(t *testing.T, page *agouti.Page, selectors ...string
 }
 
 func waitForURL(t *testing.T, page *agouti.Page, pat *regexp.Regexp) {
-	require.Eventually(t, func() bool {
+	var lastURL string
+	require.Eventuallyf(t,
+		func() bool {
 			url, err := page.URL()
-		return err == nil && pat.MatchString(url)
+			if err == nil && pat.MatchString(url) {
-	}, 10*time.Second, 100*time.Millisecond)
+				return true
+			}
+			if url != lastURL {
+				t.Logf("saw URL %s", url)
+				lastURL = url
+			}
+			return false
+		},
+		10*time.Second,
+		100*time.Millisecond,
+		"expected to browse to %s, but never got there",
+		pat,
+	)
 }
 
 func readAndExpectEmpty(r io.Reader) (err error) {
@@ -407,18 +421,20 @@ func spawnTestGoroutine(t *testing.T, f func() error) {
 
 func oidcLoginCommand(ctx context.Context, t *testing.T, pinnipedExe string, sessionCachePath string) *exec.Cmd {
 	env := library.IntegrationEnv(t)
+	callbackURL, err := url.Parse(env.CLITestUpstream.CallbackURL)
+	require.NoError(t, err)
 	cmd := exec.CommandContext(ctx, pinnipedExe, "login", "oidc",
-		"--issuer", env.OIDCUpstream.Issuer,
+		"--issuer", env.CLITestUpstream.Issuer,
-		"--client-id", env.OIDCUpstream.ClientID,
+		"--client-id", env.CLITestUpstream.ClientID,
-		"--listen-port", strconv.Itoa(env.OIDCUpstream.LocalhostPort),
+		"--listen-port", callbackURL.Port(),
 		"--session-cache", sessionCachePath,
 		"--skip-browser",
 	)
 
 	// If there is a custom CA bundle, pass it via --ca-bundle and a temporary file.
-	if env.OIDCUpstream.CABundle != "" {
+	if env.CLITestUpstream.CABundle != "" {
 		path := filepath.Join(t.TempDir(), "test-ca.pem")
-		require.NoError(t, ioutil.WriteFile(path, []byte(env.OIDCUpstream.CABundle), 0600))
+		require.NoError(t, ioutil.WriteFile(path, []byte(env.CLITestUpstream.CABundle), 0600))
 		cmd.Args = append(cmd.Args, "--ca-bundle", path)
 	}
 

test/integration/supervisor_login_test.go

@@ -68,15 +68,13 @@ func TestSupervisorLogin(t *testing.T) {
 	require.Equal(t, http.StatusUnprocessableEntity, rsp.StatusCode)
 
 	// Create upstream OIDC provider.
-	testClientID := "test-client-id"
-	testClientSecret := "test-client-secret"
 	spec := idpv1alpha1.UpstreamOIDCProviderSpec{
-		Issuer: env.OIDCUpstream.Issuer,
+		Issuer: env.SupervisorTestUpstream.Issuer,
 		TLS: &idpv1alpha1.TLSSpec{
-			CertificateAuthorityData: base64.StdEncoding.EncodeToString([]byte(env.OIDCUpstream.CABundle)),
+			CertificateAuthorityData: base64.StdEncoding.EncodeToString([]byte(env.SupervisorTestUpstream.CABundle)),
 		},
 		Client: idpv1alpha1.OIDCClient{
-			SecretName: makeTestClientCredsSecret(t, testClientID, testClientSecret).Name,
+			SecretName: makeTestClientCredsSecret(t, env.SupervisorTestUpstream.ClientID, env.SupervisorTestUpstream.ClientSecret).Name,
 		},
 	}
 	upstream := makeTestUpstream(t, spec, idpv1alpha1.PhaseReady)
@@ -94,7 +92,7 @@ func TestSupervisorLogin(t *testing.T) {
 		ctx,
 		t,
 		upstream.Spec.Issuer,
-		testClientID,
+		env.SupervisorTestUpstream.ClientID,
 		upstreamRedirectURI,
 		rsp.Header.Get("Location"),
 	)
@@ -147,9 +145,9 @@ func requireValidRedirectLocation(
 			return url.Parse(env.Proxy)
 		}
 	}
-	if env.OIDCUpstream.CABundle != "" {
+	if env.SupervisorTestUpstream.CABundle != "" {
 		transport.TLSClientConfig = &tls.Config{RootCAs: x509.NewCertPool()}
-		transport.TLSClientConfig.RootCAs.AppendCertsFromPEM([]byte(env.OIDCUpstream.CABundle))
+		transport.TLSClientConfig.RootCAs.AppendCertsFromPEM([]byte(env.SupervisorTestUpstream.CABundle))
 	}
 
 	ctx = oidc.ClientContext(ctx, &http.Client{Transport: &transport})

test/integration/supervisor_upstream_test.go

@@ -48,9 +48,9 @@ func TestSupervisorUpstreamOIDCDiscovery(t *testing.T) {
 	t.Run("valid", func(t *testing.T) {
 		t.Parallel()
 		spec := v1alpha1.UpstreamOIDCProviderSpec{
-			Issuer: env.OIDCUpstream.Issuer,
+			Issuer: env.SupervisorTestUpstream.Issuer,
 			TLS: &v1alpha1.TLSSpec{
-				CertificateAuthorityData: base64.StdEncoding.EncodeToString([]byte(env.OIDCUpstream.CABundle)),
+				CertificateAuthorityData: base64.StdEncoding.EncodeToString([]byte(env.SupervisorTestUpstream.CABundle)),
 			},
 			AuthorizationConfig: v1alpha1.OIDCAuthorizationConfig{
 				AdditionalScopes: []string{"email", "profile"},

test/library/env.go

@@ -6,7 +6,6 @@ package library
 import (
 	"io/ioutil"
 	"os"
-	"strconv"
 	"strings"
 	"testing"
 
@@ -46,14 +45,18 @@ type TestEnv struct {
 		ExpectedGroups   []string `json:"expectedGroups"`
 	} `json:"testUser"`
 
-	OIDCUpstream struct {
+	CLITestUpstream        TestOIDCUpstream `json:"cliOIDCUpstream"`
+	SupervisorTestUpstream TestOIDCUpstream `json:"supervisorOIDCUpstream"`
+}
+
+type TestOIDCUpstream struct {
 	Issuer       string `json:"issuer"`
 	CABundle     string `json:"caBundle" `
 	ClientID     string `json:"clientID"`
-		LocalhostPort int    `json:"localhostPort"`
+	ClientSecret string `json:"clientSecret"`
+	CallbackURL  string `json:"callback"`
 	Username     string `json:"username"`
 	Password     string `json:"password"`
-	} `json:"oidcUpstream"`
 }
 
 // IntegrationEnv gets the integration test environment from OS environment variables. This
@@ -130,12 +133,24 @@ func loadEnvVars(t *testing.T, result *TestEnv) {
 	require.NotEmpty(t, result.SupervisorCustomLabels, "PINNIPED_TEST_SUPERVISOR_CUSTOM_LABELS cannot be empty")
 	result.Proxy = os.Getenv("PINNIPED_TEST_PROXY")
 
-	result.OIDCUpstream.Issuer = needEnv(t, "PINNIPED_TEST_CLI_OIDC_ISSUER")
+	result.CLITestUpstream = TestOIDCUpstream{
-	result.OIDCUpstream.CABundle = os.Getenv("PINNIPED_TEST_CLI_OIDC_ISSUER_CA_BUNDLE")
+		Issuer:      needEnv(t, "PINNIPED_TEST_CLI_OIDC_ISSUER"),
-	result.OIDCUpstream.ClientID = needEnv(t, "PINNIPED_TEST_CLI_OIDC_CLIENT_ID")
+		CABundle:    os.Getenv("PINNIPED_TEST_CLI_OIDC_ISSUER_CA_BUNDLE"),
-	result.OIDCUpstream.LocalhostPort, _ = strconv.Atoi(needEnv(t, "PINNIPED_TEST_CLI_OIDC_LOCALHOST_PORT"))
+		ClientID:    needEnv(t, "PINNIPED_TEST_CLI_OIDC_CLIENT_ID"),
-	result.OIDCUpstream.Username = needEnv(t, "PINNIPED_TEST_CLI_OIDC_USERNAME")
+		CallbackURL: needEnv(t, "PINNIPED_TEST_CLI_OIDC_CALLBACK_URL"),
-	result.OIDCUpstream.Password = needEnv(t, "PINNIPED_TEST_CLI_OIDC_PASSWORD")
+		Username:    needEnv(t, "PINNIPED_TEST_CLI_OIDC_USERNAME"),
+		Password:    needEnv(t, "PINNIPED_TEST_CLI_OIDC_PASSWORD"),
+	}
+
+	result.SupervisorTestUpstream = TestOIDCUpstream{
+		Issuer:       needEnv(t, "PINNIPED_TEST_SUPERVISOR_UPSTREAM_OIDC_ISSUER"),
+		CABundle:     os.Getenv("PINNIPED_TEST_SUPERVISOR_UPSTREAM_OIDC_ISSUER_CA_BUNDLE"),
+		ClientID:     needEnv(t, "PINNIPED_TEST_SUPERVISOR_UPSTREAM_OIDC_CLIENT_ID"),
+		ClientSecret: needEnv(t, "PINNIPED_TEST_SUPERVISOR_UPSTREAM_OIDC_CLIENT_SECRET"),
+		CallbackURL:  needEnv(t, "PINNIPED_TEST_SUPERVISOR_UPSTREAM_OIDC_CALLBACK_URL"),
+		Username:     needEnv(t, "PINNIPED_TEST_SUPERVISOR_UPSTREAM_OIDC_USERNAME"),
+		Password:     needEnv(t, "PINNIPED_TEST_SUPERVISOR_UPSTREAM_OIDC_PASSWORD"),
+	}
 }
 
 func (e *TestEnv) HasCapability(cap Capability) bool {