Add IDP Discovery integration test for GitHub

2025-12-23 06:15:47 +00:00 · 2024-04-29 14:57:14 -04:00
parent 8a961bfa21
commit 6424f45c19
4 changed files with 156 additions and 22 deletions
--- a/test/integration/supervisor_discovery_test.go
+++ b/test/integration/supervisor_discovery_test.go
@@ -1,4 +1,4 @@
-// Copyright 2020-2023 the Pinniped contributors. All Rights Reserved.
+// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved.
 // SPDX-License-Identifier: Apache-2.0

 package integration
@@ -23,6 +23,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/util/retry"
+	"k8s.io/utils/ptr"
 	"k8s.io/utils/strings/slices"

 	"go.pinniped.dev/generated/latest/apis/supervisor/config/v1alpha1"
@@ -93,7 +94,7 @@ func TestSupervisorOIDCDiscovery_Disruptive(t *testing.T) {
 			// Test that there is no default discovery endpoint available when there are no FederationDomains.
 			requireDiscoveryEndpointsAreNotFound(t, scheme, addr, caBundle, fmt.Sprintf("%s://%s", scheme, addr))

-			// Define several unique issuer strings. Always use https in the issuer name even when we are accessing the http port.
+			// Define several unique issuer URLs. Always use https in the issuer URL even when we are accessing the http port.
 			issuer1 := fmt.Sprintf("https://%s/nested/issuer1", addr)
 			issuer2 := fmt.Sprintf("https://%s/nested/issuer2", addr)
 			issuer3 := fmt.Sprintf("https://%s/issuer3", addr)
@@ -102,7 +103,7 @@ func TestSupervisorOIDCDiscovery_Disruptive(t *testing.T) {
 			issuer6 := fmt.Sprintf("https://%s/issuer6", addr)
 			badIssuer := fmt.Sprintf("https://%s/badIssuer?cannot-use=queries", addr)

-			// When FederationDomain are created in sequence they each cause a discovery endpoint to appear only for as long as the FederationDomain exists.
+			// When FederationDomains are created in sequence they each cause a discovery endpoint to appear only for as long as the FederationDomain exists.
 			config1, jwks1 := requireCreatingFederationDomainCausesDiscoveryEndpointsToAppear(ctx, t, scheme, addr, caBundle, issuer1, client)
 			requireDeletingFederationDomainCausesDiscoveryEndpointsToDisappear(t, config1, client, ns, scheme, addr, caBundle, issuer1)
 			config2, jwks2 := requireCreatingFederationDomainCausesDiscoveryEndpointsToAppear(ctx, t, scheme, addr, caBundle, issuer2, client)
@@ -119,7 +120,7 @@ func TestSupervisorOIDCDiscovery_Disruptive(t *testing.T) {
 			require.NotEqual(t, jwks3.Keys[0]["x"], jwks4.Keys[0]["x"])
 			require.NotEqual(t, jwks3.Keys[0]["y"], jwks4.Keys[0]["y"])

-			// Editing a provider to change the issuer name updates the endpoints that are being served.
+			// Editing a FederationDomain to change the issuer URL updates the endpoints that are being served.
 			updatedConfig4 := editFederationDomainIssuerName(t, config4, client, ns, issuer5)
 			requireDiscoveryEndpointsAreNotFound(t, scheme, addr, caBundle, issuer4)
 			jwks5 := requireDiscoveryEndpointsAreWorking(t, scheme, addr, caBundle, issuer5, nil)
@@ -130,31 +131,37 @@ func TestSupervisorOIDCDiscovery_Disruptive(t *testing.T) {
 			requireDeletingFederationDomainCausesDiscoveryEndpointsToDisappear(t, config3, client, ns, scheme, addr, caBundle, issuer3)
 			requireDeletingFederationDomainCausesDiscoveryEndpointsToDisappear(t, updatedConfig4, client, ns, scheme, addr, caBundle, issuer5)

-			// When the same issuer is added twice, both issuers are marked as duplicates, and neither provider is serving.
+			// When the same issuer URL is added to two FederationDomains, both FederationDomains are marked as duplicates, and neither is serving.
 			config6Duplicate1, _ := requireCreatingFederationDomainCausesDiscoveryEndpointsToAppear(ctx, t, scheme, addr, caBundle, issuer6, client)
 			config6Duplicate2 := testlib.CreateTestFederationDomain(ctx, t, v1alpha1.FederationDomainSpec{Issuer: issuer6}, v1alpha1.FederationDomainPhaseError)
 			requireStatus(t, client, ns, config6Duplicate1.Name, v1alpha1.FederationDomainPhaseError, withFalseConditions([]string{"Ready", "IssuerIsUnique"}))
 			requireStatus(t, client, ns, config6Duplicate2.Name, v1alpha1.FederationDomainPhaseError, withFalseConditions([]string{"Ready", "IssuerIsUnique"}))
 			requireDiscoveryEndpointsAreNotFound(t, scheme, addr, caBundle, issuer6)

-			// If we delete the first duplicate issuer, the second duplicate issuer starts serving.
+			// If we delete the first duplicate FederationDomain, the second duplicate FederationDomain starts serving.
 			requireDelete(t, client, ns, config6Duplicate1.Name)
 			requireWellKnownEndpointIsWorking(t, scheme, addr, caBundle, issuer6, nil)
 			requireStatus(t, client, ns, config6Duplicate2.Name, v1alpha1.FederationDomainPhaseReady, withAllSuccessfulConditions())

-			// When we finally delete all issuers, the endpoint should be down.
+			// When we finally delete all FederationDomains, the discovery endpoints should be down.
 			requireDeletingFederationDomainCausesDiscoveryEndpointsToDisappear(t, config6Duplicate2, client, ns, scheme, addr, caBundle, issuer6)

-			// "Host" headers can be used to send requests to discovery endpoints when the public address is different from the issuer name.
+			// "Host" headers can be used to send requests to discovery endpoints when the public address is different from the issuer URL.
 			issuer7 := "https://some-issuer-host-and-port-that-doesnt-match-public-supervisor-address.com:2684/issuer7"
 			config7, _ := requireCreatingFederationDomainCausesDiscoveryEndpointsToAppear(ctx, t, scheme, addr, caBundle, issuer7, client)
 			requireDeletingFederationDomainCausesDiscoveryEndpointsToDisappear(t, config7, client, ns, scheme, addr, caBundle, issuer7)

-			// When we create a provider with an invalid issuer, the status is set to invalid.
+			// When we create a FederationDomain with an invalid issuer url, the status is set to invalid.
 			badConfig := testlib.CreateTestFederationDomain(ctx, t, v1alpha1.FederationDomainSpec{Issuer: badIssuer}, v1alpha1.FederationDomainPhaseError)
 			requireStatus(t, client, ns, badConfig.Name, v1alpha1.FederationDomainPhaseError, withFalseConditions([]string{"Ready", "IssuerURLValid"}))
 			requireDiscoveryEndpointsAreNotFound(t, scheme, addr, caBundle, badIssuer)
 			requireDeletingFederationDomainCausesDiscoveryEndpointsToDisappear(t, badConfig, client, ns, scheme, addr, caBundle, badIssuer)
+
+			issuer8 := fmt.Sprintf("https://%s/issuer8multipleIDP", addr)
+			config8 := requireIDPsListedByIDPDiscoveryEndpoint(t, env, ctx, kubeClient, ns, scheme, addr, caBundle, issuer8)
+
+			// requireJWKSEndpointIsWorking() will give us a bit of an idea what to do...
+			requireDeletingFederationDomainCausesDiscoveryEndpointsToDisappear(t, config8, client, ns, scheme, addr, caBundle, issuer8)
 		})
 	}
 }
@@ -494,6 +501,7 @@ func requireCreatingFederationDomainCausesDiscoveryEndpointsToAppear(
 	return newFederationDomain, jwksResult
 }

+// TODO: consider renaming this since it does not actually check all discovery endpoints (example: idp discovery is not tested).
 func requireDiscoveryEndpointsAreWorking(t *testing.T, supervisorScheme, supervisorAddress, supervisorCABundle, issuerName string, dnsOverrides map[string]string) *ExpectedJWKSResponseFormat {
 	requireWellKnownEndpointIsWorking(t, supervisorScheme, supervisorAddress, supervisorCABundle, issuerName, dnsOverrides)
 	jwksResult := requireJWKSEndpointIsWorking(t, supervisorScheme, supervisorAddress, supervisorCABundle, issuerName, dnsOverrides)
@@ -738,3 +746,95 @@ func newHTTPClient(t *testing.T, caBundle string, dnsOverrides map[string]string

 	return c
 }
+
+func requireIDPsListedByIDPDiscoveryEndpoint(
+	t *testing.T,
+	env *testlib.TestEnv,
+	ctx context.Context,
+	kubeClient kubernetes.Interface,
+	ns, supervisorScheme, supervisorAddress, supervisorCABundle, issuerName string) *v1alpha1.FederationDomain {
+	// github
+	gitHubIDPSecretName := "github-idp-secret" //nolint:gosec // this is not a credential
+	_, err := kubeClient.CoreV1().Secrets(ns).Create(ctx, &corev1.Secret{
+		Type:     "secrets.pinniped.dev/github-client",
+		TypeMeta: metav1.TypeMeta{},
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      gitHubIDPSecretName,
+			Namespace: ns,
+		},
+		StringData: map[string]string{
+			"clientID":     "foo",
+			"clientSecret": "bar",
+		},
+	}, metav1.CreateOptions{})
+	require.NoError(t, err)
+	t.Cleanup(func() {
+		_ = kubeClient.CoreV1().Secrets(ns).Delete(ctx, gitHubIDPSecretName, metav1.DeleteOptions{})
+	})
+
+	ghIDP := testlib.CreateGitHubIdentityProvider(t, idpv1alpha1.GitHubIdentityProviderSpec{
+		Client: idpv1alpha1.GitHubClientSpec{
+			SecretName: gitHubIDPSecretName,
+		},
+		AllowAuthentication: idpv1alpha1.GitHubAllowAuthenticationSpec{
+			Organizations: idpv1alpha1.GitHubOrganizationsSpec{
+				Policy: ptr.To(idpv1alpha1.GitHubAllowedAuthOrganizationsPolicyAllGitHubUsers),
+			},
+		},
+	}, idpv1alpha1.GitHubPhaseReady)
+
+	// TODO: add ldap to prove it shows up in the IDP discovery API
+	// TODO: add oidc to prove it shows up in the IDP discovery API
+	// TODO: add ad to prove it shows up in the IDP discovery API
+
+	federationDomainConfig := testlib.CreateTestFederationDomain(ctx, t, v1alpha1.FederationDomainSpec{
+		Issuer: issuerName,
+		IdentityProviders: []v1alpha1.FederationDomainIdentityProvider{{
+			DisplayName: ghIDP.Name,
+			ObjectRef: corev1.TypedLocalObjectReference{
+				APIGroup: ptr.To("idp.supervisor." + env.APIGroupSuffix),
+				Kind:     "GitHubIdentityProvider",
+				Name:     ghIDP.Name,
+			},
+		}},
+	}, v1alpha1.FederationDomainPhaseReady)
+
+	requireDiscoveryEndpointsAreWorking(t, supervisorScheme, supervisorAddress, supervisorCABundle, issuerName, nil)
+	issuer8URL, err := url.Parse(issuerName)
+	require.NoError(t, err)
+	wellKnownURL := wellKnownURLForIssuer(supervisorScheme, supervisorAddress, issuer8URL.Path)
+	_, wellKnownResponseBody := requireSuccessEndpointResponse(t, wellKnownURL, issuerName, supervisorCABundle, nil) //nolint:bodyclose
+
+	type WellKnownResponse struct {
+		Issuer                string `json:"issuer"`
+		AuthorizationEndpoint string `json:"authorization_endpoint"`
+		TokenEndpoint         string `json:"token_endpoint"`
+		JWKSUri               string `json:"jwks_uri"`
+		DiscoverySupervisor   struct {
+			IdentityProvidersEndpoint string `json:"pinniped_identity_providers_endpoint"`
+		} `json:"discovery.supervisor.pinniped.dev/v1alpha1"`
+	}
+	var wellKnownResponse WellKnownResponse
+	err = json.Unmarshal([]byte(wellKnownResponseBody), &wellKnownResponse)
+	require.NoError(t, err)
+	discoveryIDPEndpoint := wellKnownResponse.DiscoverySupervisor.IdentityProvidersEndpoint
+	_, discoveryIDPResponseBody := requireSuccessEndpointResponse(t, discoveryIDPEndpoint, issuerName, supervisorCABundle, nil) //nolint:bodyclose
+	type IdentityProviderListResponse struct {
+		IdentityProviders []struct {
+			Name string `json:"name"`
+			Type string `json:"type"`
+		} `json:"pinniped_identity_providers"`
+	}
+	var identityProviderListResponse IdentityProviderListResponse
+	err = json.Unmarshal([]byte(discoveryIDPResponseBody), &identityProviderListResponse)
+	require.NoError(t, err)
+
+	allIDPs := []string{ghIDP.Name}
+	require.Equal(t, len(identityProviderListResponse.IdentityProviders), 1, "all IDPs should be listed by idp discovery endpoint")
+	for _, provider := range identityProviderListResponse.IdentityProviders {
+		require.True(t, slices.Contains(allIDPs, provider.Name))
+		require.Contains(t, allIDPs, provider.Name, fmt.Sprintf("provider name should be listed in IDP discovery: %s", provider.Name))
+	}
+
+	return federationDomainConfig
+}
--- a/test/testlib/client.go
+++ b/test/testlib/client.go
@@ -186,7 +186,7 @@ func CreateTestWebhookAuthenticator(
 	defer cancel()

 	webhook, err := webhooks.Create(createContext, &auth1alpha1.WebhookAuthenticator{
-		ObjectMeta: testObjectMeta(t, "webhook"),
+		ObjectMeta: TestObjectMeta(t, "webhook"),
 		Spec:       *webhookSpec,
 	}, metav1.CreateOptions{})
 	require.NoError(t, err, "could not create test WebhookAuthenticator")
@@ -295,7 +295,7 @@ func CreateTestJWTAuthenticator(
 	defer cancel()

 	jwtAuthenticator, err := jwtAuthenticators.Create(createContext, &auth1alpha1.JWTAuthenticator{
-		ObjectMeta: testObjectMeta(t, "jwt-authenticator"),
+		ObjectMeta: TestObjectMeta(t, "jwt-authenticator"),
 		Spec:       spec,
 	}, metav1.CreateOptions{})
 	require.NoError(t, err, "could not create test JWTAuthenticator")
@@ -369,7 +369,7 @@ func CreateTestFederationDomain(

 	federationDomainsClient := NewSupervisorClientset(t).ConfigV1alpha1().FederationDomains(testEnv.SupervisorNamespace)
 	federationDomain, err := federationDomainsClient.Create(createContext, &configv1alpha1.FederationDomain{
-		ObjectMeta: testObjectMeta(t, "oidc-provider"),
+		ObjectMeta: TestObjectMeta(t, "oidc-provider"),
 		Spec:       spec,
 	}, metav1.CreateOptions{})
 	require.NoError(t, err, "could not create test FederationDomain")
@@ -460,7 +460,7 @@ func CreateTestSecret(t *testing.T, namespace string, baseName string, secretTyp
 	defer cancel()

 	created, err := client.CoreV1().Secrets(namespace).Create(ctx, &corev1.Secret{
-		ObjectMeta: testObjectMeta(t, baseName),
+		ObjectMeta: TestObjectMeta(t, baseName),
 		Type:       secretType,
 		StringData: stringData,
 	}, metav1.CreateOptions{})
@@ -482,7 +482,7 @@ func CreateTestSecretBytes(t *testing.T, namespace string, baseName string, secr
 	defer cancel()

 	created, err := client.CoreV1().Secrets(namespace).Create(ctx, &corev1.Secret{
-		ObjectMeta: testObjectMeta(t, baseName),
+		ObjectMeta: TestObjectMeta(t, baseName),
 		Type:       secretType,
 		Data:       data,
 	}, metav1.CreateOptions{})
@@ -587,7 +587,7 @@ func createOIDCClientSecret(t *testing.T, forOIDCClient *configv1alpha1.OIDCClie

 func CreateTestOIDCIdentityProvider(t *testing.T, spec idpv1alpha1.OIDCIdentityProviderSpec, expectedPhase idpv1alpha1.OIDCIdentityProviderPhase) *idpv1alpha1.OIDCIdentityProvider {
 	t.Helper()
-	return CreateTestOIDCIdentityProviderWithObjectMeta(t, spec, testObjectMeta(t, "upstream-oidc-idp"), expectedPhase)
+	return CreateTestOIDCIdentityProviderWithObjectMeta(t, spec, TestObjectMeta(t, "upstream-oidc-idp"), expectedPhase)
 }

 func CreateTestOIDCIdentityProviderWithObjectMeta(t *testing.T, spec idpv1alpha1.OIDCIdentityProviderSpec, objectMeta metav1.ObjectMeta, expectedPhase idpv1alpha1.OIDCIdentityProviderPhase) *idpv1alpha1.OIDCIdentityProvider {
@@ -640,7 +640,7 @@ func CreateTestLDAPIdentityProvider(t *testing.T, spec idpv1alpha1.LDAPIdentityP

 	// Create the LDAPIdentityProvider using GenerateName to get a random name.
 	created, err := upstreams.Create(ctx, &idpv1alpha1.LDAPIdentityProvider{
-		ObjectMeta: testObjectMeta(t, "upstream-ldap-idp"),
+		ObjectMeta: TestObjectMeta(t, "upstream-ldap-idp"),
 		Spec:       spec,
 	}, metav1.CreateOptions{})
 	require.NoError(t, err)
@@ -681,7 +681,7 @@ func CreateTestActiveDirectoryIdentityProvider(t *testing.T, spec idpv1alpha1.Ac

 	// Create the ActiveDirectoryIdentityProvider using GenerateName to get a random name.
 	created, err := upstreams.Create(ctx, &idpv1alpha1.ActiveDirectoryIdentityProvider{
-		ObjectMeta: testObjectMeta(t, "upstream-ad-idp"),
+		ObjectMeta: TestObjectMeta(t, "upstream-ad-idp"),
 		Spec:       spec,
 	}, metav1.CreateOptions{})
 	require.NoError(t, err)
@@ -711,6 +711,41 @@ func CreateTestActiveDirectoryIdentityProvider(t *testing.T, spec idpv1alpha1.Ac
 	return result
 }

+func CreateGitHubIdentityProvider(t *testing.T, spec idpv1alpha1.GitHubIdentityProviderSpec, expectedPhase idpv1alpha1.GitHubIdentityProviderPhase) *idpv1alpha1.GitHubIdentityProvider {
+	t.Helper()
+	env := IntegrationEnv(t)
+	client := NewSupervisorClientset(t)
+	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute)
+	defer cancel()
+
+	upstreams := client.IDPV1alpha1().GitHubIdentityProviders(env.SupervisorNamespace)
+
+	// Create the GitHubIdentityProvider using GenerateName to get a random name.
+	created, err := upstreams.Create(ctx, &idpv1alpha1.GitHubIdentityProvider{
+		ObjectMeta: TestObjectMeta(t, "upstream-github-idp"),
+		Spec:       spec,
+	}, metav1.CreateOptions{})
+	require.NoError(t, err)
+
+	// Always clean this up after this point.
+	t.Cleanup(func() {
+		t.Logf("cleaning up test GitHubIdentityProvider %s/%s", created.Namespace, created.Name)
+		err := upstreams.Delete(context.Background(), created.Name, metav1.DeleteOptions{})
+		require.NoError(t, err)
+	})
+	t.Logf("created test GitHubIdentityProvider %s", created.Name)
+
+	// Wait for the GitHubIdentityProvider to enter the expected phase (or time out).
+	var result *idpv1alpha1.GitHubIdentityProvider
+	RequireEventuallyf(t, func(requireEventually *require.Assertions) {
+		var err error
+		result, err = upstreams.Get(ctx, created.Name, metav1.GetOptions{})
+		requireEventually.NoErrorf(err, "error while getting GitHubIdentityProvider %s/%s", created.Namespace, created.Name)
+		requireEventually.Equal(expectedPhase, result.Status.Phase)
+	}, 60*time.Second, 1*time.Second, "expected the GitHubIdentityProvider to go into phase %s, GitHubIdentityProvider was: %s", expectedPhase, Sdump(result))
+	return result
+}
+
 func CreateTestClusterRoleBinding(t *testing.T, subject rbacv1.Subject, roleRef rbacv1.RoleRef) *rbacv1.ClusterRoleBinding {
 	t.Helper()
 	client := NewKubernetesClientset(t)
@@ -721,7 +756,7 @@ func CreateTestClusterRoleBinding(t *testing.T, subject rbacv1.Subject, roleRef

 	// Create the ClusterRoleBinding using GenerateName to get a random name.
 	created, err := clusterRoles.Create(ctx, &rbacv1.ClusterRoleBinding{
-		ObjectMeta: testObjectMeta(t, "cluster-role"),
+		ObjectMeta: TestObjectMeta(t, "cluster-role"),
 		Subjects:   []rbacv1.Subject{subject},
 		RoleRef:    roleRef,
 	}, metav1.CreateOptions{})
@@ -772,7 +807,7 @@ func CreatePod(ctx context.Context, t *testing.T, name, namespace string, spec c
 	ctx, cancel := context.WithTimeout(ctx, podCreateTimeout+time.Minute)
 	defer cancel()

-	created, err := pods.Create(ctx, &corev1.Pod{ObjectMeta: testObjectMeta(t, name), Spec: spec}, metav1.CreateOptions{})
+	created, err := pods.Create(ctx, &corev1.Pod{ObjectMeta: TestObjectMeta(t, name), Spec: spec}, metav1.CreateOptions{})
 	require.NoError(t, err)
 	t.Logf("created test Pod %s", created.Name)

@@ -890,7 +925,7 @@ func WaitForGitHubIdentityProviderStatusConditions(
 	}, 60*time.Second, 1*time.Second, "wanted conditions for GitHubIdentityProvider %q", gitHubIDPName)
 }

-func testObjectMeta(t *testing.T, baseName string) metav1.ObjectMeta {
+func TestObjectMeta(t *testing.T, baseName string) metav1.ObjectMeta {
 	return metav1.ObjectMeta{
 		GenerateName: fmt.Sprintf("test-%s-", baseName),
 		Labels:       map[string]string{"pinniped.dev/test": ""},