From 6424f45c194aad71af96479a59f0335c4977fb0e Mon Sep 17 00:00:00 2001 From: "Benjamin A. Petersen" Date: Mon, 29 Apr 2024 14:57:14 -0400 Subject: [PATCH] Add IDP Discovery integration test for GitHub --- cmd/pinniped/cmd/login_oidc.go | 2 +- .../github_upstream_watcher_test.go | 1 - test/integration/supervisor_discovery_test.go | 118 ++++++++++++++++-- test/testlib/client.go | 57 +++++++-- 4 files changed, 156 insertions(+), 22 deletions(-) diff --git a/cmd/pinniped/cmd/login_oidc.go b/cmd/pinniped/cmd/login_oidc.go index 4369d12ab..6c8ae0bb0 100644 --- a/cmd/pinniped/cmd/login_oidc.go +++ b/cmd/pinniped/cmd/login_oidc.go @@ -304,7 +304,7 @@ func flowOptions( } switch requestedIDPType { - case idpdiscoveryv1alpha1.IDPTypeOIDC: + case idpdiscoveryv1alpha1.IDPTypeOIDC, idpdiscoveryv1alpha1.IDPTypeGitHub: switch requestedFlow { case idpdiscoveryv1alpha1.IDPFlowCLIPassword: return useCLIFlow, nil diff --git a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go index fa8401714..110e8bcc4 100644 --- a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go @@ -1787,7 +1787,6 @@ func TestController(t *testing.T) { compareTLSClientConfigWithinHttpClients(t, phttp.Default(certPool), actualIDP.GetHttpClient()) require.Equal(t, tt.wantResultingCache[i].OAuth2Config, actualIDP.GetOAuth2Config()) - } // Verify the status conditions as reported in Kubernetes diff --git a/test/integration/supervisor_discovery_test.go b/test/integration/supervisor_discovery_test.go index 09f35bd6c..ef2967fdc 100644 --- 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 +} diff --git a/test/testlib/client.go b/test/testlib/client.go index feda851b9..1f820c03a 100644 --- 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": ""},