From 0c7e95539ff07ffcce834d62d94e661fc6a53b64 Mon Sep 17 00:00:00 2001 From: "Benjamin A. Petersen" Date: Wed, 3 Apr 2024 11:14:13 -0400 Subject: [PATCH 01/16] Add GitHub to FederationDomain IdP ListerFinder --- .../types_supervisor_idp_discovery.go.tmpl | 3 +- .../types_supervisor_idp_discovery.go | 3 +- .../types_supervisor_idp_discovery.go | 3 +- .../types_supervisor_idp_discovery.go | 3 +- .../types_supervisor_idp_discovery.go | 3 +- .../types_supervisor_idp_discovery.go | 3 +- .../types_supervisor_idp_discovery.go | 3 +- .../types_supervisor_idp_discovery.go | 3 +- .../types_supervisor_idp_discovery.go | 3 +- .../types_supervisor_idp_discovery.go | 3 +- .../types_supervisor_idp_discovery.go | 3 +- ...domain_identity_providers_lister_finder.go | 9 ++ .../idplister/upstream_idp_lister.go | 7 +- .../resolved_github_provider.go | 94 +++++++++++++++++++ .../resolved_github_provider_test.go | 1 + internal/psession/pinniped_session.go | 12 +++ 16 files changed, 144 insertions(+), 12 deletions(-) create mode 100644 internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go create mode 100644 internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go diff --git a/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go.tmpl b/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go.tmpl index ea0550904..ed1836768 100644 --- a/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go.tmpl +++ b/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go.tmpl @@ -1,4 +1,4 @@ -// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package v1alpha1 @@ -16,6 +16,7 @@ const ( IDPTypeLDAP IDPType = "ldap" IDPTypeActiveDirectory IDPType = "activedirectory" + IDPTypeGitHub IDPType = "github" IDPFlowCLIPassword IDPFlow = "cli_password" IDPFlowBrowserAuthcode IDPFlow = "browser_authcode" ) diff --git a/generated/1.21/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go b/generated/1.21/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go index ea0550904..ed1836768 100644 --- a/generated/1.21/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go +++ b/generated/1.21/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go @@ -1,4 +1,4 @@ -// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package v1alpha1 @@ -16,6 +16,7 @@ const ( IDPTypeLDAP IDPType = "ldap" IDPTypeActiveDirectory IDPType = "activedirectory" + IDPTypeGitHub IDPType = "github" IDPFlowCLIPassword IDPFlow = "cli_password" IDPFlowBrowserAuthcode IDPFlow = "browser_authcode" ) diff --git a/generated/1.22/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go b/generated/1.22/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go index ea0550904..ed1836768 100644 --- a/generated/1.22/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go +++ b/generated/1.22/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go @@ -1,4 +1,4 @@ -// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package v1alpha1 @@ -16,6 +16,7 @@ const ( IDPTypeLDAP IDPType = "ldap" IDPTypeActiveDirectory IDPType = "activedirectory" + IDPTypeGitHub IDPType = "github" IDPFlowCLIPassword IDPFlow = "cli_password" IDPFlowBrowserAuthcode IDPFlow = "browser_authcode" ) diff --git a/generated/1.23/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go b/generated/1.23/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go index ea0550904..ed1836768 100644 --- a/generated/1.23/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go +++ b/generated/1.23/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go @@ -1,4 +1,4 @@ -// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package v1alpha1 @@ -16,6 +16,7 @@ const ( IDPTypeLDAP IDPType = "ldap" IDPTypeActiveDirectory IDPType = "activedirectory" + IDPTypeGitHub IDPType = "github" IDPFlowCLIPassword IDPFlow = "cli_password" IDPFlowBrowserAuthcode IDPFlow = "browser_authcode" ) diff --git a/generated/1.24/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go b/generated/1.24/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go index ea0550904..ed1836768 100644 --- a/generated/1.24/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go +++ b/generated/1.24/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go @@ -1,4 +1,4 @@ -// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package v1alpha1 @@ -16,6 +16,7 @@ const ( IDPTypeLDAP IDPType = "ldap" IDPTypeActiveDirectory IDPType = "activedirectory" + IDPTypeGitHub IDPType = "github" IDPFlowCLIPassword IDPFlow = "cli_password" IDPFlowBrowserAuthcode IDPFlow = "browser_authcode" ) diff --git a/generated/1.25/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go b/generated/1.25/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go index ea0550904..ed1836768 100644 --- a/generated/1.25/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go +++ b/generated/1.25/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go @@ -1,4 +1,4 @@ -// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package v1alpha1 @@ -16,6 +16,7 @@ const ( IDPTypeLDAP IDPType = "ldap" IDPTypeActiveDirectory IDPType = "activedirectory" + IDPTypeGitHub IDPType = "github" IDPFlowCLIPassword IDPFlow = "cli_password" IDPFlowBrowserAuthcode IDPFlow = "browser_authcode" ) diff --git a/generated/1.26/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go b/generated/1.26/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go index ea0550904..ed1836768 100644 --- a/generated/1.26/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go +++ b/generated/1.26/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go @@ -1,4 +1,4 @@ -// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package v1alpha1 @@ -16,6 +16,7 @@ const ( IDPTypeLDAP IDPType = "ldap" IDPTypeActiveDirectory IDPType = "activedirectory" + IDPTypeGitHub IDPType = "github" IDPFlowCLIPassword IDPFlow = "cli_password" IDPFlowBrowserAuthcode IDPFlow = "browser_authcode" ) diff --git a/generated/1.27/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go b/generated/1.27/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go index ea0550904..ed1836768 100644 --- a/generated/1.27/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go +++ b/generated/1.27/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go @@ -1,4 +1,4 @@ -// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package v1alpha1 @@ -16,6 +16,7 @@ const ( IDPTypeLDAP IDPType = "ldap" IDPTypeActiveDirectory IDPType = "activedirectory" + IDPTypeGitHub IDPType = "github" IDPFlowCLIPassword IDPFlow = "cli_password" IDPFlowBrowserAuthcode IDPFlow = "browser_authcode" ) diff --git a/generated/1.28/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go b/generated/1.28/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go index ea0550904..ed1836768 100644 --- a/generated/1.28/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go +++ b/generated/1.28/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go @@ -1,4 +1,4 @@ -// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package v1alpha1 @@ -16,6 +16,7 @@ const ( IDPTypeLDAP IDPType = "ldap" IDPTypeActiveDirectory IDPType = "activedirectory" + IDPTypeGitHub IDPType = "github" IDPFlowCLIPassword IDPFlow = "cli_password" IDPFlowBrowserAuthcode IDPFlow = "browser_authcode" ) diff --git a/generated/1.29/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go b/generated/1.29/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go index ea0550904..ed1836768 100644 --- a/generated/1.29/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go +++ b/generated/1.29/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go @@ -1,4 +1,4 @@ -// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package v1alpha1 @@ -16,6 +16,7 @@ const ( IDPTypeLDAP IDPType = "ldap" IDPTypeActiveDirectory IDPType = "activedirectory" + IDPTypeGitHub IDPType = "github" IDPFlowCLIPassword IDPFlow = "cli_password" IDPFlowBrowserAuthcode IDPFlow = "browser_authcode" ) diff --git a/generated/latest/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go b/generated/latest/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go index ea0550904..ed1836768 100644 --- a/generated/latest/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go +++ b/generated/latest/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go @@ -1,4 +1,4 @@ -// Copyright 2021-2022 the Pinniped contributors. All Rights Reserved. +// Copyright 2021-2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package v1alpha1 @@ -16,6 +16,7 @@ const ( IDPTypeLDAP IDPType = "ldap" IDPTypeActiveDirectory IDPType = "activedirectory" + IDPTypeGitHub IDPType = "github" IDPFlowCLIPassword IDPFlow = "cli_password" IDPFlowBrowserAuthcode IDPFlow = "browser_authcode" ) diff --git a/internal/federationdomain/federationdomainproviders/federation_domain_identity_providers_lister_finder.go b/internal/federationdomain/federationdomainproviders/federation_domain_identity_providers_lister_finder.go index 358d57921..1c0e398aa 100644 --- a/internal/federationdomain/federationdomainproviders/federation_domain_identity_providers_lister_finder.go +++ b/internal/federationdomain/federationdomainproviders/federation_domain_identity_providers_lister_finder.go @@ -11,6 +11,7 @@ import ( "go.pinniped.dev/internal/federationdomain/idplister" "go.pinniped.dev/internal/federationdomain/resolvedprovider" + "go.pinniped.dev/internal/federationdomain/resolvedprovider/resolvedgithub" "go.pinniped.dev/internal/federationdomain/resolvedprovider/resolvedldap" "go.pinniped.dev/internal/federationdomain/resolvedprovider/resolvedoidc" "go.pinniped.dev/internal/idtransform" @@ -144,6 +145,7 @@ func (u *FederationDomainIdentityProvidersListerFinder) GetIdentityProviders() [ cachedOIDCProviders := u.wrappedLister.GetOIDCIdentityProviders() cachedLDAPProviders := u.wrappedLister.GetLDAPIdentityProviders() cachedADProviders := u.wrappedLister.GetActiveDirectoryIdentityProviders() + cachedGitHubProviders := u.wrappedLister.GetGitHubIdentityProviders() providers := []resolvedprovider.FederationDomainResolvedIdentityProvider{} // Every configured identityProvider on the FederationDomain uses an objetRef to an underlying IDP CR that might // be available as a provider in the wrapped cache. For each configured identityProvider/displayName... @@ -184,6 +186,13 @@ func (u *FederationDomainIdentityProvidersListerFinder) GetIdentityProviders() [ }) } } + for _, p := range cachedGitHubProviders { + if idp.UID == p.GetResourceUID() { + providers = append(providers, &resolvedgithub.FederationDomainResolvedGitHubIdentityProvider{ + // TODO: fill this out. + }) + } + } } return providers } diff --git a/internal/federationdomain/idplister/upstream_idp_lister.go b/internal/federationdomain/idplister/upstream_idp_lister.go index 38b5e27eb..0084b472c 100644 --- a/internal/federationdomain/idplister/upstream_idp_lister.go +++ b/internal/federationdomain/idplister/upstream_idp_lister.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 idplister @@ -19,8 +19,13 @@ type UpstreamActiveDirectoryIdentityProviderLister interface { GetActiveDirectoryIdentityProviders() []upstreamprovider.UpstreamLDAPIdentityProviderI } +type UpstreamGitHubIdentityProviderLister interface { + GetGitHubIdentityProviders() []upstreamprovider.UpstreamGithubIdentityProviderI +} + type UpstreamIdentityProvidersLister interface { UpstreamOIDCIdentityProvidersLister UpstreamLDAPIdentityProvidersLister UpstreamActiveDirectoryIdentityProviderLister + UpstreamGitHubIdentityProviderLister } diff --git a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go new file mode 100644 index 000000000..673af6578 --- /dev/null +++ b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go @@ -0,0 +1,94 @@ +package resolvedgithub + +import ( + "context" + + "go.pinniped.dev/generated/latest/apis/supervisor/idpdiscovery/v1alpha1" + "go.pinniped.dev/internal/federationdomain/resolvedprovider" + "go.pinniped.dev/internal/federationdomain/upstreamprovider" + "go.pinniped.dev/internal/idtransform" + "go.pinniped.dev/internal/psession" + "go.pinniped.dev/pkg/oidcclient/nonce" + "go.pinniped.dev/pkg/oidcclient/pkce" +) + +// FederationDomainResolvedGitHubIdentityProvider respresents a FederationDomainIdentityProvider which has +// been resolved dynamically based on the currently loaded IDP CRs to include the provider.UpstreamGitHubIdentityProviderI +// and other metadata about the provider. +type FederationDomainResolvedGitHubIdentityProvider struct { + DisplayName string + Provider upstreamprovider.UpstreamGithubIdentityProviderI + SessionProviderType psession.ProviderType + Transforms *idtransform.TransformationPipeline +} + +var _ resolvedprovider.FederationDomainResolvedIdentityProvider = (*FederationDomainResolvedGitHubIdentityProvider)(nil) + +func (p *FederationDomainResolvedGitHubIdentityProvider) GetDisplayName() string { + return p.DisplayName +} + +func (p *FederationDomainResolvedGitHubIdentityProvider) GetProvider() upstreamprovider.UpstreamIdentityProviderI { + return p.Provider +} + +func (p *FederationDomainResolvedGitHubIdentityProvider) GetSessionProviderType() psession.ProviderType { + return p.SessionProviderType +} + +func (p *FederationDomainResolvedGitHubIdentityProvider) GetIDPDiscoveryType() v1alpha1.IDPType { + return v1alpha1.IDPTypeGitHub +} + +func (p *FederationDomainResolvedGitHubIdentityProvider) GetIDPDiscoveryFlows() []v1alpha1.IDPFlow { + // TODO: implement + return []v1alpha1.IDPFlow{} +} + +func (p *FederationDomainResolvedGitHubIdentityProvider) GetTransforms() *idtransform.TransformationPipeline { + return p.Transforms +} + +func (p *FederationDomainResolvedGitHubIdentityProvider) CloneIDPSpecificSessionDataFromSession(session *psession.CustomSessionData) interface{} { + if session.GitHub == nil { + return nil + } + return session.GitHub.Clone() +} + +func (p *FederationDomainResolvedGitHubIdentityProvider) ApplyIDPSpecificSessionDataToSession(session *psession.CustomSessionData, idpSpecificSessionData interface{}) { + session.GitHub = idpSpecificSessionData.(*psession.GitHubSessionData) +} + +func (p *FederationDomainResolvedGitHubIdentityProvider) UpstreamAuthorizeRedirectURL(state *resolvedprovider.UpstreamAuthorizeRequestState, downstreamIssuerURL string) (string, error) { + // TODO: implement + return "", nil +} + +func (p *FederationDomainResolvedGitHubIdentityProvider) Login( + ctx context.Context, + submittedUsername string, + submittedPassword string, +) (*resolvedprovider.Identity, *resolvedprovider.IdentityLoginExtras, error) { + // TODO: implement + return nil, nil, nil +} + +func (p *FederationDomainResolvedGitHubIdentityProvider) LoginFromCallback( + ctx context.Context, + authCode string, + pkce pkce.Code, + nonce nonce.Nonce, + redirectURI string, +) (*resolvedprovider.Identity, *resolvedprovider.IdentityLoginExtras, error) { + // TODO: implement + return nil, nil, nil +} + +func (p *FederationDomainResolvedGitHubIdentityProvider) UpstreamRefresh( + ctx context.Context, + identity *resolvedprovider.Identity, +) (refreshedIdentity *resolvedprovider.RefreshedIdentity, err error) { + // TODO: implement + return nil, nil +} diff --git a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go new file mode 100644 index 000000000..c36c1bea7 --- /dev/null +++ b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go @@ -0,0 +1 @@ +package resolvedgithub diff --git a/internal/psession/pinniped_session.go b/internal/psession/pinniped_session.go index 63f5ebe79..e27b489b0 100644 --- a/internal/psession/pinniped_session.go +++ b/internal/psession/pinniped_session.go @@ -74,6 +74,9 @@ type CustomSessionData struct { // Only used when ProviderType == "activedirectory". ActiveDirectory *ActiveDirectorySessionData `json:"activedirectory,omitempty"` + + // Only used when ProviderType == "github". + GitHub *GitHubSessionData `json:"github,omitempty"` } type ProviderType string @@ -140,6 +143,15 @@ func (s *ActiveDirectorySessionData) Clone() *ActiveDirectorySessionData { } } +type GitHubSessionData struct { + // TODO: flesh this out +} + +func (s *GitHubSessionData) Clone() *GitHubSessionData { + dataCopy := *s // this shortcut works because all fields in this type are currently strings (no pointers) + return &dataCopy +} + // NewPinnipedSession returns a new empty session. func NewPinnipedSession() *PinnipedSession { return &PinnipedSession{ From 44edba6f756b0cf22e2f75c8bad2f8200d111361 Mon Sep 17 00:00:00 2001 From: "Benjamin A. Petersen" Date: Wed, 3 Apr 2024 15:25:51 -0400 Subject: [PATCH 02/16] Add tests for Github in FederationDomain ListerFinder --- cmd/pinniped/cmd/login_oidc.go | 3 + .../github_upstream_watcher.go | 34 ++--- .../github_upstream_watcher_test.go | 14 ++- ...domain_identity_providers_lister_finder.go | 5 +- ...n_identity_providers_lister_finder_test.go | 118 +++++++++++++++--- .../resolved_github_provider.go | 28 +++-- .../resolved_github_provider_test.go | 3 + .../resolvedldap/resolved_ldap_provider.go | 6 + .../upstreamprovider/upsteam_provider.go | 1 + .../authorizationcode/authorizationcode.go | 3 +- internal/psession/pinniped_session.go | 4 +- .../githubtestutil/testgithubprovider.go | 8 -- .../oidctestutil/testgithubprovider.go | 77 ++++++++++++ .../testutil/testidplister/testidplister.go | 50 ++++++++ internal/upstreamgithub/upstreamgithub.go | 77 ++++++++---- .../upstreamgithub/upstreamgithub_test.go | 2 + 16 files changed, 349 insertions(+), 84 deletions(-) delete mode 100644 internal/testutil/githubtestutil/testgithubprovider.go create mode 100644 internal/testutil/oidctestutil/testgithubprovider.go diff --git a/cmd/pinniped/cmd/login_oidc.go b/cmd/pinniped/cmd/login_oidc.go index 5bfc50db2..34f51706e 100644 --- a/cmd/pinniped/cmd/login_oidc.go +++ b/cmd/pinniped/cmd/login_oidc.go @@ -328,6 +328,9 @@ func flowOptions( flowSource, requestedIDPType, requestedFlow, strings.Join([]string{idpdiscoveryv1alpha1.IDPFlowCLIPassword.String(), idpdiscoveryv1alpha1.IDPFlowBrowserAuthcode.String()}, ", ")) } + // TODO: implement this + case idpdiscoveryv1alpha1.IDPTypeGitHub: + panic("GitHub has not been implemented yet.") default: // Surprisingly cobra does not support this kind of flag validation. See https://github.com/spf13/pflag/issues/236 return nil, fmt.Errorf( diff --git a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go index e6897e323..bfcbba5bb 100644 --- a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go +++ b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go @@ -238,7 +238,7 @@ func validateOrganizationsPolicy(organizationsSpec *v1alpha1.GitHubOrganizations } func (c *gitHubWatcherController) validateUpstreamAndUpdateConditions(ctx controllerlib.Context, upstream *v1alpha1.GitHubIdentityProvider) ( - *upstreamgithub.ProviderConfig, // If validated, returns the config + *upstreamgithub.Provider, // If validated, returns the config error, // This error will only refer to programmatic errors such as inability to perform a Dial or dereference a pointer, not configuration errors ) { conditions := make([]*metav1.Condition, 0) @@ -291,22 +291,24 @@ func (c *gitHubWatcherController) validateUpstreamAndUpdateConditions(ctx contro return nil, k8sutilerrors.NewAggregate(applicationErrors) } - providerConfig := &upstreamgithub.ProviderConfig{ - Name: upstream.Name, - ResourceUID: upstream.UID, - Host: hostURL, - GroupNameAttribute: groupNameAttribute, - UsernameAttribute: usernameAttribute, - OAuth2Config: &oauth2.Config{ - ClientID: clientID, - ClientSecret: clientSecret, + provider := upstreamgithub.New( + upstreamgithub.ProviderConfig{ + Name: upstream.Name, + ResourceUID: upstream.UID, + Host: hostURL, + GroupNameAttribute: groupNameAttribute, + UsernameAttribute: usernameAttribute, + OAuth2Config: &oauth2.Config{ + ClientID: clientID, + ClientSecret: clientSecret, + }, + AllowedOrganizations: upstream.Spec.AllowAuthentication.Organizations.Allowed, + OrganizationLoginPolicy: policy, + AuthorizationURL: fmt.Sprintf("%s/login/oauth/authorize", hostURL), + HttpClient: httpClient, }, - AllowedOrganizations: upstream.Spec.AllowAuthentication.Organizations.Allowed, - OrganizationLoginPolicy: policy, - AuthorizationURL: fmt.Sprintf("%s/login/oauth/authorize", hostURL), - HttpClient: httpClient, - } - return providerConfig, k8sutilerrors.NewAggregate(applicationErrors) + ) + return provider, k8sutilerrors.NewAggregate(applicationErrors) } func validateHost(gitHubAPIConfig v1alpha1.GitHubAPIConfig) (*metav1.Condition, *endpointaddr.HostPort) { diff --git a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go index d8b72110a..92562b641 100644 --- a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go @@ -371,7 +371,8 @@ func TestController(t *testing.T) { wantErr string wantLogs []string wantResultingCache []*upstreamgithub.ProviderConfig - wantResultingUpstreams []v1alpha1.GitHubIdentityProvider + // wantResultingCache []*oidctestutil.TestUpstreamGitHubIdentityProvider + wantResultingUpstreams []v1alpha1.GitHubIdentityProvider }{ { name: "no GitHubIdentityProviders", @@ -1717,7 +1718,9 @@ func TestController(t *testing.T) { cache := dynamicupstreamprovider.NewDynamicUpstreamIDPProvider() cache.SetGitHubIdentityProviders([]upstreamprovider.UpstreamGithubIdentityProviderI{ - &upstreamgithub.ProviderConfig{Name: "initial-entry-to-remove"}, + upstreamgithub.New( + upstreamgithub.ProviderConfig{Name: "initial-entry-to-remove"}, + ), }) var log bytes.Buffer @@ -1757,12 +1760,12 @@ func TestController(t *testing.T) { require.Equal(t, len(tt.wantResultingCache), len(actualIDPList)) for i := 0; i < len(tt.wantResultingCache); i++ { // Do not expect any particular order in the cache - var actualIDP *upstreamgithub.ProviderConfig + var actualIDP *upstreamgithub.Provider for _, possibleIDP := range actualIDPList { if possibleIDP.GetName() == tt.wantResultingCache[i].Name { // For this check, we know that the actual IDPs are going to have type upstreamgithub.ProviderConfig var ok bool - actualIDP, ok = possibleIDP.(*upstreamgithub.ProviderConfig) + actualIDP, ok = possibleIDP.(*upstreamgithub.Provider) require.True(t, ok) break } @@ -1785,7 +1788,8 @@ func TestController(t *testing.T) { require.NoError(t, err) compareTLSClientConfigWithinHttpClients(t, phttp.Default(certPool), actualIDP.GetHttpClient()) - require.Equal(t, tt.wantResultingCache[i].OAuth2Config, actualIDP.OAuth2Config) + require.Equal(t, tt.wantResultingCache[i].OAuth2Config, actualIDP.GetOAuth2Config()) + } // Verify the status conditions as reported in Kubernetes diff --git a/internal/federationdomain/federationdomainproviders/federation_domain_identity_providers_lister_finder.go b/internal/federationdomain/federationdomainproviders/federation_domain_identity_providers_lister_finder.go index 1c0e398aa..2e4aa887b 100644 --- a/internal/federationdomain/federationdomainproviders/federation_domain_identity_providers_lister_finder.go +++ b/internal/federationdomain/federationdomainproviders/federation_domain_identity_providers_lister_finder.go @@ -189,7 +189,10 @@ func (u *FederationDomainIdentityProvidersListerFinder) GetIdentityProviders() [ for _, p := range cachedGitHubProviders { if idp.UID == p.GetResourceUID() { providers = append(providers, &resolvedgithub.FederationDomainResolvedGitHubIdentityProvider{ - // TODO: fill this out. + DisplayName: idp.DisplayName, + Provider: p, + SessionProviderType: psession.ProviderTypeGitHub, + Transforms: idp.Transforms, }) } } diff --git a/internal/federationdomain/federationdomainproviders/federation_domain_identity_providers_lister_finder_test.go b/internal/federationdomain/federationdomainproviders/federation_domain_identity_providers_lister_finder_test.go index 7e4d7a5b9..53ae52045 100644 --- a/internal/federationdomain/federationdomainproviders/federation_domain_identity_providers_lister_finder_test.go +++ b/internal/federationdomain/federationdomainproviders/federation_domain_identity_providers_lister_finder_test.go @@ -10,6 +10,7 @@ import ( "go.pinniped.dev/internal/federationdomain/idplister" "go.pinniped.dev/internal/federationdomain/resolvedprovider" + "go.pinniped.dev/internal/federationdomain/resolvedprovider/resolvedgithub" "go.pinniped.dev/internal/federationdomain/resolvedprovider/resolvedldap" "go.pinniped.dev/internal/federationdomain/resolvedprovider/resolvedoidc" "go.pinniped.dev/internal/testutil/oidctestutil" @@ -52,6 +53,14 @@ func TestFederationDomainIdentityProvidersListerFinder(t *testing.T) { WithName("my-ad-idp2"). WithResourceUID("my-ad-uid-idp2"). Build() + myDefaultGitHubIDP := oidctestutil.NewTestUpstreamGitHubIdentityProviderBuilder(). + WithName("my-default-github-idp"). + WithResourceUID("my-default-github-uid-idp"). + Build() + myGitHubIDP1 := oidctestutil.NewTestUpstreamGitHubIdentityProviderBuilder(). + WithName("my-github-idp1"). + WithResourceUID("my-github-uid-idp1"). + Build() // FederationDomainIssuers fakeIssuerURL := "https://www.fakeissuerurl.com" @@ -77,13 +86,20 @@ func TestFederationDomainIdentityProvidersListerFinder(t *testing.T) { }) require.NoError(t, err) - fdIssuerWithOIDCAndLDAPAndADIDPs, err := NewFederationDomainIssuer(fakeIssuerURL, []*FederationDomainIdentityProvider{ + fdIssuerWithDefaultGitHubIDP, err := NewFederationDomainIssuerWithDefaultIDP(fakeIssuerURL, &FederationDomainIdentityProvider{ + DisplayName: "my-default-github-idp", + UID: "my-default-github-uid-idp", + }) + require.NoError(t, err) + + fdIssuerWithOIDCAndLDAPAndADAndGitHubIDPs, err := NewFederationDomainIssuer(fakeIssuerURL, []*FederationDomainIdentityProvider{ {DisplayName: "my-oidc-idp1", UID: "my-oidc-uid-idp1"}, {DisplayName: "my-oidc-idp2", UID: "my-oidc-uid-idp2"}, {DisplayName: "my-ldap-idp1", UID: "my-ldap-uid-idp1"}, {DisplayName: "my-ldap-idp2", UID: "my-ldap-uid-idp2"}, {DisplayName: "my-ad-idp1", UID: "my-ad-uid-idp1"}, {DisplayName: "my-ad-idp2", UID: "my-ad-uid-idp2"}, + {DisplayName: "my-github-idp1", UID: "my-github-uid-idp1"}, }) require.NoError(t, err) @@ -99,6 +115,7 @@ func TestFederationDomainIdentityProvidersListerFinder(t *testing.T) { {DisplayName: "my-ldap-idp4", UID: "my-ldap-uid-idp4"}, {DisplayName: "my-ad-idp2", UID: "my-ad-uid-idp2"}, {DisplayName: "my-ad-idp3", UID: "my-ad-uid-idp3"}, + {DisplayName: "my-github-idp1", UID: "my-github-uid-idp1"}, }) require.NoError(t, err) @@ -133,6 +150,11 @@ func TestFederationDomainIdentityProvidersListerFinder(t *testing.T) { Provider: myADIDP1, SessionProviderType: "activedirectory", } + myGitHub1Resolved := &resolvedgithub.FederationDomainResolvedGitHubIdentityProvider{ + DisplayName: "my-github-idp1", + Provider: myGitHubIDP1, + SessionProviderType: "github", + } myDefaultOIDCIDPResolved := &resolvedoidc.FederationDomainResolvedOIDCIdentityProvider{ DisplayName: "my-default-oidc-idp", @@ -144,15 +166,21 @@ func TestFederationDomainIdentityProvidersListerFinder(t *testing.T) { Provider: myDefaultLDAPIDP, SessionProviderType: "ldap", } + myDefaultGitHubIDPResolved := &resolvedgithub.FederationDomainResolvedGitHubIdentityProvider{ + DisplayName: "my-default-github-idp", + Provider: myDefaultGitHubIDP, + SessionProviderType: "github", + } testFindUpstreamIDPByDisplayName := []struct { - name string - wrappedLister idplister.UpstreamIdentityProvidersLister - federationDomainIssuer *FederationDomainIssuer - findIDPByDisplayName string - wantOIDCIDPByDisplayName *resolvedoidc.FederationDomainResolvedOIDCIdentityProvider - wantLDAPIDPByDisplayName *resolvedldap.FederationDomainResolvedLDAPIdentityProvider - wantError string + name string + wrappedLister idplister.UpstreamIdentityProvidersLister + federationDomainIssuer *FederationDomainIssuer + findIDPByDisplayName string + wantOIDCIDPByDisplayName *resolvedoidc.FederationDomainResolvedOIDCIdentityProvider + wantLDAPIDPByDisplayName *resolvedldap.FederationDomainResolvedLDAPIdentityProvider + wantGitHubIDPByDisplayName *resolvedgithub.FederationDomainResolvedGitHubIdentityProvider + wantError string }{ { name: "FindUpstreamIDPByDisplayName will find an upstream IdP by display name with one IDP configured", @@ -182,8 +210,9 @@ func TestFederationDomainIdentityProvidersListerFinder(t *testing.T) { WithOIDC(myOIDCIDP2). WithLDAP(myLDAPIDP1). WithLDAP(myLDAPIDP2). + WithGitHub(myGitHubIDP1). BuildDynamicUpstreamIDPProvider(), - federationDomainIssuer: fdIssuerWithOIDCAndLDAPAndADIDPs, + federationDomainIssuer: fdIssuerWithOIDCAndLDAPAndADAndGitHubIDPs, wantOIDCIDPByDisplayName: myOIDCIDP1Resolved, }, { @@ -195,6 +224,7 @@ func TestFederationDomainIdentityProvidersListerFinder(t *testing.T) { WithLDAP(myLDAPIDP1). WithLDAP(myLDAPIDP2). WithActiveDirectory(myADIDP1). + WithGitHub(myGitHubIDP1). BuildDynamicUpstreamIDPProvider(), federationDomainIssuer: fdIssuerWithOIDCIDP1, wantOIDCIDPByDisplayName: myOIDCIDP1Resolved, @@ -208,11 +238,13 @@ func TestFederationDomainIdentityProvidersListerFinder(t *testing.T) { WithLDAP(myLDAPIDP1). WithLDAP(myLDAPIDP2). WithActiveDirectory(myADIDP1). + WithGitHub(myGitHubIDP1). BuildDynamicUpstreamIDPProvider(), - federationDomainIssuer: fdIssuerWithOIDCAndLDAPAndADIDPs, + federationDomainIssuer: fdIssuerWithOIDCAndLDAPAndADAndGitHubIDPs, + wantLDAPIDPByDisplayName: myLDAPIDP1Resolved, }, { - name: "FindUpstreamIDPByDisplayName will find an upstream IDP of type AD (LDAP) by display name", + name: "FindUpstreamIDPByDisplayName will find an upstream IDP of type AD (LDAP) by display name", findIDPByDisplayName: "my-ad-idp1", wrappedLister: testidplister.NewUpstreamIDPListerBuilder(). WithOIDC(myOIDCIDP1). @@ -220,10 +252,25 @@ func TestFederationDomainIdentityProvidersListerFinder(t *testing.T) { WithLDAP(myLDAPIDP1). WithLDAP(myLDAPIDP2). WithActiveDirectory(myADIDP1). + WithGitHub(myGitHubIDP1). BuildDynamicUpstreamIDPProvider(), - federationDomainIssuer: fdIssuerWithOIDCAndLDAPAndADIDPs, + federationDomainIssuer: fdIssuerWithOIDCAndLDAPAndADAndGitHubIDPs, wantLDAPIDPByDisplayName: myADIDP1Resolved, }, + { + name: "FindUpstreamIDPByDisplayName will find an upstream IDP of type GitHub by display name", + findIDPByDisplayName: "my-github-idp1", + wrappedLister: testidplister.NewUpstreamIDPListerBuilder(). + WithOIDC(myOIDCIDP1). + WithOIDC(myOIDCIDP2). + WithLDAP(myLDAPIDP1). + WithLDAP(myLDAPIDP2). + WithActiveDirectory(myADIDP1). + WithGitHub(myGitHubIDP1). + BuildDynamicUpstreamIDPProvider(), + federationDomainIssuer: fdIssuerWithOIDCAndLDAPAndADAndGitHubIDPs, + wantGitHubIDPByDisplayName: myGitHub1Resolved, + }, { name: "FindUpstreamIDPByDisplayName will error if IDP by display name is not found - no such display name", findIDPByDisplayName: "i-cant-find-my-idp", @@ -233,8 +280,9 @@ func TestFederationDomainIdentityProvidersListerFinder(t *testing.T) { WithLDAP(myLDAPIDP1). WithLDAP(myLDAPIDP2). WithActiveDirectory(myADIDP1). + WithGitHub(myGitHubIDP1). BuildDynamicUpstreamIDPProvider(), - federationDomainIssuer: fdIssuerWithOIDCAndLDAPAndADIDPs, + federationDomainIssuer: fdIssuerWithOIDCAndLDAPAndADAndGitHubIDPs, wantError: `identity provider not found: "i-cant-find-my-idp"`, }, { @@ -266,6 +314,9 @@ func TestFederationDomainIdentityProvidersListerFinder(t *testing.T) { if tt.wantLDAPIDPByDisplayName != nil { require.Equal(t, tt.wantLDAPIDPByDisplayName, foundIDP) } + if tt.wantGitHubIDPByDisplayName != nil { + require.Equal(t, tt.wantGitHubIDPByDisplayName, foundIDP) + } }) } @@ -275,6 +326,7 @@ func TestFederationDomainIdentityProvidersListerFinder(t *testing.T) { federationDomainIssuer *FederationDomainIssuer wantDefaultOIDCIDP *resolvedoidc.FederationDomainResolvedOIDCIdentityProvider wantDefaultLDAPIDP *resolvedldap.FederationDomainResolvedLDAPIdentityProvider + wantDefaultGitHubIDP *resolvedgithub.FederationDomainResolvedGitHubIdentityProvider wantError string }{ { @@ -293,6 +345,14 @@ func TestFederationDomainIdentityProvidersListerFinder(t *testing.T) { federationDomainIssuer: fdIssuerWithDefaultLDAPIDP, wantDefaultLDAPIDP: myDefaultLDAPIDPResolved, }, + { + name: "FindDefaultIDP resturns a GitHubIdentityProvider if there is a GitHubIdentityProvider defined as the default IDP", + wrappedLister: testidplister.NewUpstreamIDPListerBuilder(). + WithGitHub(myDefaultGitHubIDP). + BuildDynamicUpstreamIDPProvider(), + federationDomainIssuer: fdIssuerWithDefaultGitHubIDP, + wantDefaultGitHubIDP: myDefaultGitHubIDPResolved, + }, { name: "FindDefaultIDP returns an error if there is no default IDP to return", wrappedLister: testidplister.NewUpstreamIDPListerBuilder(). @@ -342,6 +402,9 @@ func TestFederationDomainIdentityProvidersListerFinder(t *testing.T) { if tt.wantDefaultLDAPIDP != nil { require.Equal(t, tt.wantDefaultLDAPIDP, foundIDP) } + if tt.wantDefaultGitHubIDP != nil { + require.Equal(t, tt.wantDefaultGitHubIDP, foundIDP) + } }) } @@ -359,14 +422,16 @@ func TestFederationDomainIdentityProvidersListerFinder(t *testing.T) { WithLDAP(myLDAPIDP1). WithLDAP(myLDAPIDP2). WithActiveDirectory(myADIDP1). + WithGitHub(myGitHubIDP1). BuildDynamicUpstreamIDPProvider(), - federationDomainIssuer: fdIssuerWithOIDCAndLDAPAndADIDPs, + federationDomainIssuer: fdIssuerWithOIDCAndLDAPAndADAndGitHubIDPs, wantIDPs: []resolvedprovider.FederationDomainResolvedIdentityProvider{ myOIDCIDP1Resolved, myOIDCIDP2Resolved, myLDAPIDP1Resolved, myLDAPIDP2Resolved, myADIDP1Resolved, + myGitHub1Resolved, }, }, { @@ -380,6 +445,7 @@ func TestFederationDomainIdentityProvidersListerFinder(t *testing.T) { Build()). WithLDAP(myLDAPIDP1). WithActiveDirectory(myADIDP1). + WithGitHub(myGitHubIDP1). BuildDynamicUpstreamIDPProvider(), federationDomainIssuer: fdIssuerWithLotsOfIDPs, wantIDPs: []resolvedprovider.FederationDomainResolvedIdentityProvider{ @@ -387,13 +453,14 @@ func TestFederationDomainIdentityProvidersListerFinder(t *testing.T) { myOIDCIDP2Resolved, myLDAPIDP1Resolved, myADIDP1Resolved, + myGitHub1Resolved, }, }, { name: "GetIdentityProviders will return empty list if no IDPs are found", wrappedLister: testidplister.NewUpstreamIDPListerBuilder(). BuildDynamicUpstreamIDPProvider(), - federationDomainIssuer: fdIssuerWithOIDCAndLDAPAndADIDPs, + federationDomainIssuer: fdIssuerWithOIDCAndLDAPAndADAndGitHubIDPs, wantIDPs: []resolvedprovider.FederationDomainResolvedIdentityProvider{}, }, } @@ -420,7 +487,7 @@ func TestFederationDomainIdentityProvidersListerFinder(t *testing.T) { name: "IDPCount when there are none to be found", wrappedLister: testidplister.NewUpstreamIDPListerBuilder(). BuildDynamicUpstreamIDPProvider(), - federationDomainIssuer: fdIssuerWithOIDCAndLDAPAndADIDPs, + federationDomainIssuer: fdIssuerWithOIDCAndLDAPAndADAndGitHubIDPs, wantCount: 0, }, { @@ -443,9 +510,14 @@ func TestFederationDomainIdentityProvidersListerFinder(t *testing.T) { WithName("my-ad-idp-that-isnt-in-fd-issuer"). WithResourceUID("my-ad-idp-that-isnt-in-fd-issuer"). Build()). + WithGitHub(myGitHubIDP1). + WithGitHub(oidctestutil.NewTestUpstreamGitHubIdentityProviderBuilder(). + WithName("my-github-idp-that-isnt-in-fd-issuer"). + WithResourceUID("my-github-idp-that-isnt-in-fd-issuer"). + Build()). BuildDynamicUpstreamIDPProvider(), - federationDomainIssuer: fdIssuerWithOIDCAndLDAPAndADIDPs, - wantCount: 5, + federationDomainIssuer: fdIssuerWithOIDCAndLDAPAndADAndGitHubIDPs, + wantCount: 6, }, } @@ -482,6 +554,14 @@ func TestFederationDomainIdentityProvidersListerFinder(t *testing.T) { federationDomainIssuer: fdIssuerWithDefaultLDAPIDP, wantHasDefaultIDP: true, }, + { + name: "HasDefaultIDP when there is a GitHub provider set as default", + wrappedLister: testidplister.NewUpstreamIDPListerBuilder(). + WithGitHub(myDefaultGitHubIDP). + BuildDynamicUpstreamIDPProvider(), + federationDomainIssuer: fdIssuerWithDefaultGitHubIDP, + wantHasDefaultIDP: true, + }, { name: "HasDefaultIDP when there is one set even if it cannot be found", wrappedLister: testidplister.NewUpstreamIDPListerBuilder(). @@ -497,7 +577,7 @@ func TestFederationDomainIdentityProvidersListerFinder(t *testing.T) { name: "HasDefaultIDP when there is none set", wrappedLister: testidplister.NewUpstreamIDPListerBuilder(). BuildDynamicUpstreamIDPProvider(), - federationDomainIssuer: fdIssuerWithOIDCAndLDAPAndADIDPs, + federationDomainIssuer: fdIssuerWithOIDCAndLDAPAndADAndGitHubIDPs, wantHasDefaultIDP: false, }, } diff --git a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go index 673af6578..e84e92d9d 100644 --- a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go +++ b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go @@ -1,3 +1,6 @@ +// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + package resolvedgithub import ( @@ -60,34 +63,37 @@ func (p *FederationDomainResolvedGitHubIdentityProvider) ApplyIDPSpecificSession session.GitHub = idpSpecificSessionData.(*psession.GitHubSessionData) } -func (p *FederationDomainResolvedGitHubIdentityProvider) UpstreamAuthorizeRedirectURL(state *resolvedprovider.UpstreamAuthorizeRequestState, downstreamIssuerURL string) (string, error) { +func (p *FederationDomainResolvedGitHubIdentityProvider) UpstreamAuthorizeRedirectURL( + state *resolvedprovider.UpstreamAuthorizeRequestState, //nolint:all + downstreamIssuerURL string, //nolint:all +) (string, error) { // TODO: implement return "", nil } func (p *FederationDomainResolvedGitHubIdentityProvider) Login( - ctx context.Context, - submittedUsername string, - submittedPassword string, + ctx context.Context, //nolint:all + submittedUsername string, //nolint:all + submittedPassword string, //nolint:all ) (*resolvedprovider.Identity, *resolvedprovider.IdentityLoginExtras, error) { // TODO: implement return nil, nil, nil } func (p *FederationDomainResolvedGitHubIdentityProvider) LoginFromCallback( - ctx context.Context, - authCode string, - pkce pkce.Code, - nonce nonce.Nonce, - redirectURI string, + ctx context.Context, //nolint:all + authCode string, //nolint:all + pkce pkce.Code, //nolint:all + nonce nonce.Nonce, //nolint:all + redirectURI string, //nolint:all ) (*resolvedprovider.Identity, *resolvedprovider.IdentityLoginExtras, error) { // TODO: implement return nil, nil, nil } func (p *FederationDomainResolvedGitHubIdentityProvider) UpstreamRefresh( - ctx context.Context, - identity *resolvedprovider.Identity, + ctx context.Context, //nolint:all + identity *resolvedprovider.Identity, //nolint:all ) (refreshedIdentity *resolvedprovider.RefreshedIdentity, err error) { // TODO: implement return nil, nil diff --git a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go index c36c1bea7..274144d56 100644 --- a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go +++ b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go @@ -1 +1,4 @@ +// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + package resolvedgithub diff --git a/internal/federationdomain/resolvedprovider/resolvedldap/resolved_ldap_provider.go b/internal/federationdomain/resolvedprovider/resolvedldap/resolved_ldap_provider.go index c8fea641c..d993ec12f 100644 --- a/internal/federationdomain/resolvedprovider/resolvedldap/resolved_ldap_provider.go +++ b/internal/federationdomain/resolvedprovider/resolvedldap/resolved_ldap_provider.go @@ -78,6 +78,8 @@ func (p *FederationDomainResolvedLDAPIdentityProvider) CloneIDPSpecificSessionDa return session.ActiveDirectory.Clone() case psession.ProviderTypeOIDC: // this is just here to avoid a lint error about not handling all cases fallthrough + case psession.ProviderTypeGitHub: // this is just here to avoid a lint error about not handling all cases + fallthrough default: return nil } @@ -153,6 +155,8 @@ func (p *FederationDomainResolvedLDAPIdentityProvider) Login( } case psession.ProviderTypeOIDC: // this is just here to avoid a lint error about not handling all cases fallthrough + case psession.ProviderTypeGitHub: // this is just here to avoid a lint error about not handling all cases + fallthrough default: return nil, nil, ErrUnexpectedUpstreamLDAPError.WithWrap(fmt.Errorf("unexpected provider type %q", p.GetSessionProviderType())) } @@ -207,6 +211,8 @@ func (p *FederationDomainResolvedLDAPIdentityProvider) UpstreamRefresh( additionalAttributes = sessionData.ExtraRefreshAttributes case psession.ProviderTypeOIDC: // this is just here to avoid a lint error about not handling all cases fallthrough + case psession.ProviderTypeGitHub: // this is just here to avoid a lint error about not handling all cases + fallthrough default: // This shouldn't really happen. return nil, resolvedprovider.ErrUpstreamRefreshError().WithHintf( diff --git a/internal/federationdomain/upstreamprovider/upsteam_provider.go b/internal/federationdomain/upstreamprovider/upsteam_provider.go index ab5920bdf..4717fb01c 100644 --- a/internal/federationdomain/upstreamprovider/upsteam_provider.go +++ b/internal/federationdomain/upstreamprovider/upsteam_provider.go @@ -128,6 +128,7 @@ type UpstreamLDAPIdentityProviderI interface { PerformRefresh(ctx context.Context, storedRefreshAttributes RefreshAttributes, idpDisplayName string) (groups []string, err error) } +// TODO: Impl this interface thoroughly to support GitHub login. type UpstreamGithubIdentityProviderI interface { UpstreamIdentityProviderI diff --git a/internal/fositestorage/authorizationcode/authorizationcode.go b/internal/fositestorage/authorizationcode/authorizationcode.go index 41635a7ad..dad061800 100644 --- a/internal/fositestorage/authorizationcode/authorizationcode.go +++ b/internal/fositestorage/authorizationcode/authorizationcode.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 authorizationcode @@ -190,6 +190,7 @@ func (e *errSerializationFailureWithCause) Error() string { return fmt.Sprintf("%s: %s", fosite.ErrSerializationFailure, e.cause) } +// TODO: need to revisit this, there is a unit test now failing. // ExpectedAuthorizeCodeSessionJSONFromFuzzing is used for round tripping tests. // It is exported to allow integration tests to use it. const ExpectedAuthorizeCodeSessionJSONFromFuzzing = `{ diff --git a/internal/psession/pinniped_session.go b/internal/psession/pinniped_session.go index e27b489b0..76fccc3a0 100644 --- a/internal/psession/pinniped_session.go +++ b/internal/psession/pinniped_session.go @@ -85,6 +85,7 @@ const ( ProviderTypeOIDC ProviderType = "oidc" ProviderTypeLDAP ProviderType = "ldap" ProviderTypeActiveDirectory ProviderType = "activedirectory" + ProviderTypeGitHub ProviderType = "github" ) // OIDCSessionData is the additional data needed by Pinniped when the upstream IDP is an OIDC provider. @@ -143,8 +144,9 @@ func (s *ActiveDirectorySessionData) Clone() *ActiveDirectorySessionData { } } +// TODO: flesh this out, GitHub will need additional data. type GitHubSessionData struct { - // TODO: flesh this out + // TODO: flesh this out. } func (s *GitHubSessionData) Clone() *GitHubSessionData { diff --git a/internal/testutil/githubtestutil/testgithubprovider.go b/internal/testutil/githubtestutil/testgithubprovider.go deleted file mode 100644 index 532b9b8a0..000000000 --- a/internal/testutil/githubtestutil/testgithubprovider.go +++ /dev/null @@ -1,8 +0,0 @@ -// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved. -// SPDX-License-Identifier: Apache-2.0 - -package githubtestutil - -type TestUpstreamGithubIdentityProvider struct { - Name string -} diff --git a/internal/testutil/oidctestutil/testgithubprovider.go b/internal/testutil/oidctestutil/testgithubprovider.go new file mode 100644 index 000000000..361ae30df --- /dev/null +++ b/internal/testutil/oidctestutil/testgithubprovider.go @@ -0,0 +1,77 @@ +// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package oidctestutil + +import ( + "k8s.io/apimachinery/pkg/types" + + "go.pinniped.dev/internal/federationdomain/upstreamprovider" + "go.pinniped.dev/internal/idtransform" +) + +// TODO: flesh this out. +type TestUpstreamGitHubIdentityProviderBuilder struct { + name string + clientID string + resourceUID types.UID + displayNameForFederationDomain string + transformsForFederationDomain *idtransform.TransformationPipeline +} + +func (u *TestUpstreamGitHubIdentityProviderBuilder) WithName(value string) *TestUpstreamGitHubIdentityProviderBuilder { + u.name = value + return u +} + +func (u *TestUpstreamGitHubIdentityProviderBuilder) WithResourceUID(value types.UID) *TestUpstreamGitHubIdentityProviderBuilder { + u.resourceUID = value + return u +} + +func (u *TestUpstreamGitHubIdentityProviderBuilder) WithClientID(value string) *TestUpstreamGitHubIdentityProviderBuilder { + u.clientID = value + return u +} + +func (u *TestUpstreamGitHubIdentityProviderBuilder) Build() *TestUpstreamGitHubIdentityProvider { + if u.displayNameForFederationDomain == "" { + // default it to the CR name + u.displayNameForFederationDomain = u.name + } + if u.transformsForFederationDomain == nil { + // default to an empty pipeline + u.transformsForFederationDomain = idtransform.NewTransformationPipeline() + } + // TODO: flesh this out. + return &TestUpstreamGitHubIdentityProvider{ + Name: u.name, + ResourceUID: u.resourceUID, + ClientID: u.clientID, + DisplayNameForFederationDomain: u.displayNameForFederationDomain, + TransformsForFederationDomain: u.transformsForFederationDomain, + } +} + +func NewTestUpstreamGitHubIdentityProviderBuilder() *TestUpstreamGitHubIdentityProviderBuilder { + return &TestUpstreamGitHubIdentityProviderBuilder{} +} + +// TODO: flesh this out. +type TestUpstreamGitHubIdentityProvider struct { + Name string + ClientID string + ResourceUID types.UID + DisplayNameForFederationDomain string + TransformsForFederationDomain *idtransform.TransformationPipeline +} + +var _ upstreamprovider.UpstreamGithubIdentityProviderI = &TestUpstreamGitHubIdentityProvider{} + +func (u *TestUpstreamGitHubIdentityProvider) GetResourceUID() types.UID { + return u.ResourceUID +} + +func (u *TestUpstreamGitHubIdentityProvider) GetName() string { + return u.Name +} diff --git a/internal/testutil/testidplister/testidplister.go b/internal/testutil/testidplister/testidplister.go index ee0f9ae82..c753d9af4 100644 --- a/internal/testutil/testidplister/testidplister.go +++ b/internal/testutil/testidplister/testidplister.go @@ -11,6 +11,7 @@ import ( "go.pinniped.dev/internal/federationdomain/dynamicupstreamprovider" "go.pinniped.dev/internal/federationdomain/resolvedprovider" + "go.pinniped.dev/internal/federationdomain/resolvedprovider/resolvedgithub" "go.pinniped.dev/internal/federationdomain/resolvedprovider/resolvedldap" "go.pinniped.dev/internal/federationdomain/resolvedprovider/resolvedoidc" "go.pinniped.dev/internal/federationdomain/upstreamprovider" @@ -24,6 +25,7 @@ type TestFederationDomainIdentityProvidersListerFinder struct { upstreamOIDCIdentityProviders []*oidctestutil.TestUpstreamOIDCIdentityProvider upstreamLDAPIdentityProviders []*oidctestutil.TestUpstreamLDAPIdentityProvider upstreamActiveDirectoryIdentityProviders []*oidctestutil.TestUpstreamLDAPIdentityProvider + upstreamGitHubIdentityProviders []*oidctestutil.TestUpstreamGitHubIdentityProvider defaultIDPDisplayName string } @@ -69,6 +71,16 @@ func (t *TestFederationDomainIdentityProvidersListerFinder) GetIdentityProviders fdIDPs[i] = fdIDP i++ } + for _, testIDP := range t.upstreamGitHubIdentityProviders { + fdIDP := &resolvedgithub.FederationDomainResolvedGitHubIdentityProvider{ + DisplayName: testIDP.DisplayNameForFederationDomain, + Provider: testIDP, + SessionProviderType: psession.ProviderTypeGitHub, + Transforms: testIDP.TransformsForFederationDomain, + } + fdIDPs[i] = fdIDP + i++ + } return fdIDPs } @@ -110,6 +122,16 @@ func (t *TestFederationDomainIdentityProvidersListerFinder) FindUpstreamIDPByDis }, nil } } + for _, testIDP := range t.upstreamGitHubIdentityProviders { + if upstreamIDPDisplayName == testIDP.DisplayNameForFederationDomain { + return &resolvedgithub.FederationDomainResolvedGitHubIdentityProvider{ + DisplayName: testIDP.DisplayNameForFederationDomain, + Provider: testIDP, + SessionProviderType: psession.ProviderTypeGitHub, + Transforms: testIDP.TransformsForFederationDomain, + }, nil + } + } return nil, fmt.Errorf("did not find IDP with name %q", upstreamIDPDisplayName) } @@ -125,12 +147,17 @@ func (t *TestFederationDomainIdentityProvidersListerFinder) SetActiveDirectoryId t.upstreamActiveDirectoryIdentityProviders = providers } +func (t *TestFederationDomainIdentityProvidersListerFinder) SetGitHubIdentityProviders(providers []*oidctestutil.TestUpstreamGitHubIdentityProvider) { + t.upstreamGitHubIdentityProviders = providers +} + // UpstreamIDPListerBuilder can be used to build either a dynamicupstreamprovider.DynamicUpstreamIDPProvider // or a FederationDomainIdentityProvidersListerFinderI for testing. type UpstreamIDPListerBuilder struct { upstreamOIDCIdentityProviders []*oidctestutil.TestUpstreamOIDCIdentityProvider upstreamLDAPIdentityProviders []*oidctestutil.TestUpstreamLDAPIdentityProvider upstreamActiveDirectoryIdentityProviders []*oidctestutil.TestUpstreamLDAPIdentityProvider + upstreamGitHubIdentityProviders []*oidctestutil.TestUpstreamGitHubIdentityProvider defaultIDPDisplayName string } @@ -149,6 +176,11 @@ func (b *UpstreamIDPListerBuilder) WithActiveDirectory(upstreamActiveDirectoryId return b } +func (b *UpstreamIDPListerBuilder) WithGitHub(upstreamGithubIdentityProviders ...*oidctestutil.TestUpstreamGitHubIdentityProvider) *UpstreamIDPListerBuilder { + b.upstreamGitHubIdentityProviders = append(b.upstreamGitHubIdentityProviders, upstreamGithubIdentityProviders...) + return b +} + func (b *UpstreamIDPListerBuilder) WithDefaultIDPDisplayName(defaultIDPDisplayName string) *UpstreamIDPListerBuilder { b.defaultIDPDisplayName = defaultIDPDisplayName return b @@ -159,6 +191,7 @@ func (b *UpstreamIDPListerBuilder) BuildFederationDomainIdentityProvidersListerF upstreamOIDCIdentityProviders: b.upstreamOIDCIdentityProviders, upstreamLDAPIdentityProviders: b.upstreamLDAPIdentityProviders, upstreamActiveDirectoryIdentityProviders: b.upstreamActiveDirectoryIdentityProviders, + upstreamGitHubIdentityProviders: b.upstreamGitHubIdentityProviders, defaultIDPDisplayName: b.defaultIDPDisplayName, } } @@ -184,6 +217,12 @@ func (b *UpstreamIDPListerBuilder) BuildDynamicUpstreamIDPProvider() dynamicupst } idpProvider.SetActiveDirectoryIdentityProviders(adUpstreams) + githubUpstreams := make([]upstreamprovider.UpstreamGithubIdentityProviderI, len(b.upstreamGitHubIdentityProviders)) + for i := range b.upstreamGitHubIdentityProviders { + githubUpstreams[i] = upstreamprovider.UpstreamGithubIdentityProviderI(b.upstreamGitHubIdentityProviders[i]) + } + idpProvider.SetGitHubIdentityProviders(githubUpstreams) + return idpProvider } @@ -204,6 +243,8 @@ func (b *UpstreamIDPListerBuilder) RequireExactlyOneCallToPasswordCredentialsGra actualArgs = upstreamOIDC.PasswordCredentialsGrantAndValidateTokensArgs(0) } } + // TODO: probably add GitHub loop once we flesh out the structs + require.Equal(t, 1, actualCallCountAcrossAllOIDCUpstreams, "should have been exactly one call to PasswordCredentialsGrantAndValidateTokens() by all OIDC upstreams", ) @@ -219,6 +260,7 @@ func (b *UpstreamIDPListerBuilder) RequireExactlyZeroCallsToPasswordCredentialsG for _, upstreamOIDC := range b.upstreamOIDCIdentityProviders { actualCallCountAcrossAllOIDCUpstreams += upstreamOIDC.PasswordCredentialsGrantAndValidateTokensCallCount() } + // TODO: probably add GitHub loop once we flesh out the structs require.Equal(t, 0, actualCallCountAcrossAllOIDCUpstreams, "expected exactly zero calls to PasswordCredentialsGrantAndValidateTokens()", ) @@ -241,6 +283,7 @@ func (b *UpstreamIDPListerBuilder) RequireExactlyOneCallToExchangeAuthcodeAndVal actualArgs = upstreamOIDC.ExchangeAuthcodeAndValidateTokensArgs(0) } } + // TODO: probably add GitHub loop once we flesh out the structs require.Equal(t, 1, actualCallCountAcrossAllOIDCUpstreams, "should have been exactly one call to ExchangeAuthcodeAndValidateTokens() by all OIDC upstreams", ) @@ -256,6 +299,7 @@ func (b *UpstreamIDPListerBuilder) RequireExactlyZeroCallsToExchangeAuthcodeAndV for _, upstreamOIDC := range b.upstreamOIDCIdentityProviders { actualCallCountAcrossAllOIDCUpstreams += upstreamOIDC.ExchangeAuthcodeAndValidateTokensCallCount() } + // TODO: probably add GitHub loop once we flesh out the structs require.Equal(t, 0, actualCallCountAcrossAllOIDCUpstreams, "expected exactly zero calls to ExchangeAuthcodeAndValidateTokens()", ) @@ -294,6 +338,7 @@ func (b *UpstreamIDPListerBuilder) RequireExactlyOneCallToPerformRefresh( actualArgs = upstreamAD.PerformRefreshArgs(0) } } + // TODO: probably add GitHub loop once we flesh out the structs require.Equal(t, 1, actualCallCountAcrossAllUpstreams, "should have been exactly one call to PerformRefresh() by all upstreams", ) @@ -315,6 +360,7 @@ func (b *UpstreamIDPListerBuilder) RequireExactlyZeroCallsToPerformRefresh(t *te for _, upstreamActiveDirectory := range b.upstreamActiveDirectoryIdentityProviders { actualCallCountAcrossAllUpstreams += upstreamActiveDirectory.PerformRefreshCallCount() } + // TODO: probably add GitHub loop once we flesh out the structs require.Equal(t, 0, actualCallCountAcrossAllUpstreams, "expected exactly zero calls to PerformRefresh()", @@ -338,6 +384,7 @@ func (b *UpstreamIDPListerBuilder) RequireExactlyOneCallToValidateToken( actualArgs = upstreamOIDC.ValidateTokenAndMergeWithUserInfoArgs(0) } } + // TODO: probably add GitHub loop once we flesh out the structs require.Equal(t, 1, actualCallCountAcrossAllOIDCUpstreams, "should have been exactly one call to ValidateTokenAndMergeWithUserInfo() by all OIDC upstreams", ) @@ -353,6 +400,7 @@ func (b *UpstreamIDPListerBuilder) RequireExactlyZeroCallsToValidateToken(t *tes for _, upstreamOIDC := range b.upstreamOIDCIdentityProviders { actualCallCountAcrossAllOIDCUpstreams += upstreamOIDC.ValidateTokenAndMergeWithUserInfoCallCount() } + // TODO: probably add GitHub loop once we flesh out structs require.Equal(t, 0, actualCallCountAcrossAllOIDCUpstreams, "expected exactly zero calls to ValidateTokenAndMergeWithUserInfo()", ) @@ -375,6 +423,7 @@ func (b *UpstreamIDPListerBuilder) RequireExactlyOneCallToRevokeToken( actualArgs = upstreamOIDC.RevokeTokenArgs(0) } } + // TODO: probably add GitHub loop once we flesh out structs require.Equal(t, 1, actualCallCountAcrossAllOIDCUpstreams, "should have been exactly one call to RevokeToken() by all OIDC upstreams", ) @@ -390,6 +439,7 @@ func (b *UpstreamIDPListerBuilder) RequireExactlyZeroCallsToRevokeToken(t *testi for _, upstreamOIDC := range b.upstreamOIDCIdentityProviders { actualCallCountAcrossAllOIDCUpstreams += upstreamOIDC.RevokeTokenCallCount() } + // TODO: probably add GitHub loop once we flesh out structs require.Equal(t, 0, actualCallCountAcrossAllOIDCUpstreams, "expected exactly zero calls to RevokeToken()", ) diff --git a/internal/upstreamgithub/upstreamgithub.go b/internal/upstreamgithub/upstreamgithub.go index fa9bf1f39..cd2c15875 100644 --- a/internal/upstreamgithub/upstreamgithub.go +++ b/internal/upstreamgithub/upstreamgithub.go @@ -5,12 +5,15 @@ package upstreamgithub import ( + "context" "net/http" "golang.org/x/oauth2" + "k8s.io/apimachinery/pkg/types" "go.pinniped.dev/generated/latest/apis/supervisor/idp/v1alpha1" + "go.pinniped.dev/internal/authenticators" "go.pinniped.dev/internal/federationdomain/upstreamprovider" ) @@ -28,44 +31,74 @@ type ProviderConfig struct { HttpClient *http.Client } -var _ upstreamprovider.UpstreamGithubIdentityProviderI = (*ProviderConfig)(nil) - -func (p *ProviderConfig) GetResourceUID() types.UID { - return p.ResourceUID +type Provider struct { + c ProviderConfig } -func (p *ProviderConfig) GetName() string { - return p.Name +var _ upstreamprovider.UpstreamGithubIdentityProviderI = &Provider{} +var _ authenticators.UserAuthenticator = &Provider{} + +// New creates a Provider. The config is not a pointer to ensure that a copy of the config is created, +// making the resulting Provider use an effectively read-only configuration. +func New(config ProviderConfig) *Provider { + return &Provider{c: config} } -func (p *ProviderConfig) GetClientID() string { - return p.OAuth2Config.ClientID +// GetConfig is a reader for the config. Returns a copy of the config to keep the underlying config read-only. +func (p *Provider) GetConfig() ProviderConfig { + return p.c } -func (p *ProviderConfig) GetHost() string { - return p.Host +// GetName returns a name for this upstream provider. +func (p *Provider) GetName() string { + return p.c.Name } -func (p *ProviderConfig) GetUsernameAttribute() v1alpha1.GitHubUsernameAttribute { - return p.UsernameAttribute +func (p *Provider) GetResourceUID() types.UID { + return p.c.ResourceUID } -func (p *ProviderConfig) GetGroupNameAttribute() v1alpha1.GitHubGroupNameAttribute { - return p.GroupNameAttribute +func (p *Provider) GetClientID() string { + return p.c.OAuth2Config.ClientID } -func (p *ProviderConfig) GetAllowedOrganizations() []string { - return p.AllowedOrganizations +func (p *Provider) GetOAuth2Config() *oauth2.Config { + return p.c.OAuth2Config } -func (p *ProviderConfig) GetOrganizationLoginPolicy() v1alpha1.GitHubAllowedAuthOrganizationsPolicy { - return p.OrganizationLoginPolicy +func (p *Provider) GetHost() string { + return p.c.Host } -func (p *ProviderConfig) GetAuthorizationURL() string { - return p.AuthorizationURL +func (p *Provider) GetUsernameAttribute() v1alpha1.GitHubUsernameAttribute { + return p.c.UsernameAttribute } -func (p *ProviderConfig) GetHttpClient() *http.Client { - return p.HttpClient +func (p *Provider) GetGroupNameAttribute() v1alpha1.GitHubGroupNameAttribute { + return p.c.GroupNameAttribute +} + +func (p *Provider) GetAllowedOrganizations() []string { + return p.c.AllowedOrganizations +} + +func (p *Provider) GetOrganizationLoginPolicy() v1alpha1.GitHubAllowedAuthOrganizationsPolicy { + return p.c.OrganizationLoginPolicy +} + +func (p *Provider) GetAuthorizationURL() string { + return p.c.AuthorizationURL +} + +func (p *Provider) GetHttpClient() *http.Client { + return p.c.HttpClient +} + +// AuthenticateUser authenticates an end user and returns their mapped username, groups, and UID. Implements authenticators.UserAuthenticator. +func (p *Provider) AuthenticateUser( + ctx context.Context, //nolint:all + username, password string, //nolint:all +) (*authenticators.Response, bool, error) { + // TODO: implement this, currently just placeholder to satisfy UserAuthenticator interface above + return nil, false, nil } diff --git a/internal/upstreamgithub/upstreamgithub_test.go b/internal/upstreamgithub/upstreamgithub_test.go index 37e13266f..c5a1cda99 100644 --- a/internal/upstreamgithub/upstreamgithub_test.go +++ b/internal/upstreamgithub/upstreamgithub_test.go @@ -2,3 +2,5 @@ // SPDX-License-Identifier: Apache-2.0 package upstreamgithub + +// TODO: as we flesh out the Provider & ProviderConfig add tests From 7968ed6d69f2590046372a3b9161553089a05693 Mon Sep 17 00:00:00 2001 From: "Benjamin A. Petersen" Date: Thu, 4 Apr 2024 16:13:14 -0400 Subject: [PATCH 03/16] Allow GitHubIdentityProvider IDP type by FederationDomainWatcher --- .../federation_domain_watcher.go | 31 +++++-- .../federation_domain_watcher_test.go | 92 ++++++++++++++++++- internal/supervisor/server/server.go | 1 + 3 files changed, 115 insertions(+), 9 deletions(-) diff --git a/internal/controller/supervisorconfig/federation_domain_watcher.go b/internal/controller/supervisorconfig/federation_domain_watcher.go index f571209ae..d40f7f85f 100644 --- a/internal/controller/supervisorconfig/federation_domain_watcher.go +++ b/internal/controller/supervisorconfig/federation_domain_watcher.go @@ -67,6 +67,7 @@ const ( kindLDAPIdentityProvider = "LDAPIdentityProvider" kindOIDCIdentityProvider = "OIDCIdentityProvider" kindActiveDirectoryIdentityProvider = "ActiveDirectoryIdentityProvider" + kindGitHubIdentityProvider = "GitHubIdentityProvider" celTransformerMaxExpressionRuntime = 5 * time.Second ) @@ -88,9 +89,9 @@ type federationDomainWatcherController struct { oidcIdentityProviderInformer idpinformers.OIDCIdentityProviderInformer ldapIdentityProviderInformer idpinformers.LDAPIdentityProviderInformer activeDirectoryIdentityProviderInformer idpinformers.ActiveDirectoryIdentityProviderInformer - - celTransformer *celtransformer.CELTransformer - allowedKinds sets.Set[string] + githubIdentityProviderInformer idpinformers.GitHubIdentityProviderInformer + celTransformer *celtransformer.CELTransformer + allowedKinds sets.Set[string] } // NewFederationDomainWatcherController creates a controllerlib.Controller that watches @@ -104,9 +105,10 @@ func NewFederationDomainWatcherController( oidcIdentityProviderInformer idpinformers.OIDCIdentityProviderInformer, ldapIdentityProviderInformer idpinformers.LDAPIdentityProviderInformer, activeDirectoryIdentityProviderInformer idpinformers.ActiveDirectoryIdentityProviderInformer, + githubProviderInformer idpinformers.GitHubIdentityProviderInformer, withInformer pinnipedcontroller.WithInformerOptionFunc, ) controllerlib.Controller { - allowedKinds := sets.New(kindActiveDirectoryIdentityProvider, kindLDAPIdentityProvider, kindOIDCIdentityProvider) + allowedKinds := sets.New(kindActiveDirectoryIdentityProvider, kindLDAPIdentityProvider, kindOIDCIdentityProvider, kindGitHubIdentityProvider) return controllerlib.New( controllerlib.Config{ Name: controllerName, @@ -119,6 +121,7 @@ func NewFederationDomainWatcherController( oidcIdentityProviderInformer: oidcIdentityProviderInformer, ldapIdentityProviderInformer: ldapIdentityProviderInformer, activeDirectoryIdentityProviderInformer: activeDirectoryIdentityProviderInformer, + githubIdentityProviderInformer: githubProviderInformer, allowedKinds: allowedKinds, }, }, @@ -148,6 +151,13 @@ func NewFederationDomainWatcherController( pinnipedcontroller.MatchAnythingIgnoringUpdatesFilter(pinnipedcontroller.SingletonQueue()), controllerlib.InformerOption{}, ), + withInformer( + githubProviderInformer, + // Since this controller only cares about IDP metadata names and UIDs (immutable fields), + // we only need to trigger Sync on creates and deletes. + pinnipedcontroller.MatchAnythingIgnoringUpdatesFilter(pinnipedcontroller.SingletonQueue()), + controllerlib.InformerOption{}, + ), ) } @@ -264,9 +274,12 @@ func (c *federationDomainWatcherController) makeLegacyFederationDomainIssuer( if err != nil { return nil, nil, err } - + githubIdentityProviders, err := c.githubIdentityProviderInformer.Lister().List(labels.Everything()) + if err != nil { + return nil, nil, err + } // Check if that there is exactly one IDP defined in the Supervisor namespace of any IDP CRD type. - idpCRsCount := len(oidcIdentityProviders) + len(ldapIdentityProviders) + len(activeDirectoryIdentityProviders) + idpCRsCount := len(oidcIdentityProviders) + len(ldapIdentityProviders) + len(activeDirectoryIdentityProviders) + len(githubIdentityProviders) switch { case idpCRsCount == 1: @@ -286,6 +299,10 @@ func (c *federationDomainWatcherController) makeLegacyFederationDomainIssuer( defaultFederationDomainIdentityProvider.DisplayName = activeDirectoryIdentityProviders[0].Name defaultFederationDomainIdentityProvider.UID = activeDirectoryIdentityProviders[0].UID foundIDPName = activeDirectoryIdentityProviders[0].Name + case len(githubIdentityProviders) == 1: + defaultFederationDomainIdentityProvider.DisplayName = githubIdentityProviders[0].Name + defaultFederationDomainIdentityProvider.UID = githubIdentityProviders[0].UID + foundIDPName = githubIdentityProviders[0].Name } // Backwards compatibility mode always uses an empty identity transformation pipeline since no // transformations are defined on the FederationDomain. @@ -446,6 +463,8 @@ func (c *federationDomainWatcherController) findIDPsUIDByObjectRef(objectRef cor foundIDP, err = c.activeDirectoryIdentityProviderInformer.Lister().ActiveDirectoryIdentityProviders(namespace).Get(objectRef.Name) case kindOIDCIdentityProvider: foundIDP, err = c.oidcIdentityProviderInformer.Lister().OIDCIdentityProviders(namespace).Get(objectRef.Name) + case kindGitHubIdentityProvider: + foundIDP, err = c.githubIdentityProviderInformer.Lister().GitHubIdentityProviders(namespace).Get(objectRef.Name) default: // This shouldn't happen because this helper function is not called when the kind is invalid. return "", false, fmt.Errorf("unexpected kind: %s", objectRef.Kind) diff --git a/internal/controller/supervisorconfig/federation_domain_watcher_test.go b/internal/controller/supervisorconfig/federation_domain_watcher_test.go index 1f8876a6f..d4c6c800a 100644 --- a/internal/controller/supervisorconfig/federation_domain_watcher_test.go +++ b/internal/controller/supervisorconfig/federation_domain_watcher_test.go @@ -42,6 +42,7 @@ func TestFederationDomainWatcherControllerInformerFilters(t *testing.T) { oidcIdentityProviderInformer := pinnipedinformers.NewSharedInformerFactoryWithOptions(nil, 0).IDP().V1alpha1().OIDCIdentityProviders() ldapIdentityProviderInformer := pinnipedinformers.NewSharedInformerFactoryWithOptions(nil, 0).IDP().V1alpha1().LDAPIdentityProviders() adIdentityProviderInformer := pinnipedinformers.NewSharedInformerFactoryWithOptions(nil, 0).IDP().V1alpha1().ActiveDirectoryIdentityProviders() + githubIdentityProviderInformer := pinnipedinformers.NewSharedInformerFactoryWithOptions(nil, 0).IDP().V1alpha1().GitHubIdentityProviders() tests := []struct { name string @@ -82,6 +83,13 @@ func TestFederationDomainWatcherControllerInformerFilters(t *testing.T) { wantAdd: true, wantUpdate: false, wantDelete: true, + }, { + name: "any GitHubIdentityProvider adds or deletes, but updates are ignored", + obj: &idpv1alpha1.GitHubIdentityProvider{}, + informer: githubIdentityProviderInformer, + wantAdd: true, + wantUpdate: false, + wantDelete: true, }, } for _, test := range tests { @@ -100,6 +108,7 @@ func TestFederationDomainWatcherControllerInformerFilters(t *testing.T) { oidcIdentityProviderInformer, ldapIdentityProviderInformer, adIdentityProviderInformer, + githubIdentityProviderInformer, withInformer.WithInformer, // make it possible to observe the behavior of the Filters ) @@ -163,6 +172,14 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { }, } + gitHubIdentityProvider := &idpv1alpha1.GitHubIdentityProvider{ + ObjectMeta: metav1.ObjectMeta{ + Name: "some-github-idp", + Namespace: namespace, + UID: "some-github-idp", + }, + } + federationDomain1 := &configv1alpha1.FederationDomain{ ObjectMeta: metav1.ObjectMeta{Name: "config1", Namespace: namespace, Generation: 123}, Spec: configv1alpha1.FederationDomainSpec{Issuer: "https://issuer1.com"}, @@ -487,7 +504,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { LastTransitionTime: time, Reason: "KindUnrecognized", Message: fmt.Sprintf(`some kinds specified by .spec.identityProviders[].objectRef.kind are `+ - `not recognized (should be one of "ActiveDirectoryIdentityProvider", "LDAPIdentityProvider", "OIDCIdentityProvider"): %s`, badKinds), + `not recognized (should be one of "ActiveDirectoryIdentityProvider", "GitHubIdentityProvider", "LDAPIdentityProvider", "OIDCIdentityProvider"): %s`, badKinds), } } @@ -604,6 +621,30 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { ), }, }, + { + name: "legacy config: when no identity provider is specified on federation domains, but exactly one GitHub identity " + + "provider resource exists on cluster, the controller will set a default IDP on each federation domain " + + "matching the only identity provider found", + inputObjects: []runtime.Object{ + federationDomain1, + federationDomain2, + gitHubIdentityProvider, + }, + wantFDIssuers: []*federationdomainproviders.FederationDomainIssuer{ + federationDomainIssuerWithDefaultIDP(t, federationDomain1.Spec.Issuer, gitHubIdentityProvider.ObjectMeta), + federationDomainIssuerWithDefaultIDP(t, federationDomain2.Spec.Issuer, gitHubIdentityProvider.ObjectMeta), + }, + wantStatusUpdates: []*configv1alpha1.FederationDomain{ + expectedFederationDomainStatusUpdate(federationDomain1, + configv1alpha1.FederationDomainPhaseReady, + allHappyConditionsLegacyConfigurationSuccess(federationDomain1.Spec.Issuer, gitHubIdentityProvider.Name, frozenMetav1Now, 123), + ), + expectedFederationDomainStatusUpdate(federationDomain2, + configv1alpha1.FederationDomainPhaseReady, + allHappyConditionsLegacyConfigurationSuccess(federationDomain2.Spec.Issuer, gitHubIdentityProvider.Name, frozenMetav1Now, 123), + ), + }, + }, { name: "when there are two valid FederationDomains, but one is already up to date, the sync loop only updates " + "the out-of-date FederationDomain", @@ -948,6 +989,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { oidcIdentityProvider, ldapIdentityProvider, adIdentityProvider, + gitHubIdentityProvider, }, wantFDIssuers: []*federationdomainproviders.FederationDomainIssuer{}, wantStatusUpdates: []*configv1alpha1.FederationDomain{ @@ -956,7 +998,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { conditionstestutil.Replace( allHappyConditionsLegacyConfigurationSuccess(federationDomain1.Spec.Issuer, "", frozenMetav1Now, 123), []metav1.Condition{ - sadIdentityProvidersFoundConditionIdentityProviderNotSpecified(3, frozenMetav1Now, 123), + sadIdentityProvidersFoundConditionIdentityProviderNotSpecified(4, frozenMetav1Now, 123), sadReadyCondition(frozenMetav1Now, 123), }), ), @@ -994,6 +1036,14 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { Name: "cant-find-me-still-name", }, }, + { + DisplayName: "cant-find-me-again", + ObjectRef: corev1.TypedLocalObjectReference{ + APIGroup: ptr.To(apiGroupSupervisor), + Kind: "GitHubIdentityProvider", + Name: "cant-find-me-again-name", + }, + }, }, }, }, @@ -1013,7 +1063,9 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { cannot find resource specified by .spec.identityProviders[1].objectRef (with name "cant-find-me-either-name") - cannot find resource specified by .spec.identityProviders[2].objectRef (with name "cant-find-me-still-name")`, + cannot find resource specified by .spec.identityProviders[2].objectRef (with name "cant-find-me-still-name") + + cannot find resource specified by .spec.identityProviders[3].objectRef (with name "cant-find-me-again-name")`, ), frozenMetav1Now, 123), sadReadyCondition(frozenMetav1Now, 123), }), @@ -1026,6 +1078,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { oidcIdentityProvider, ldapIdentityProvider, adIdentityProvider, + gitHubIdentityProvider, &configv1alpha1.FederationDomain{ ObjectMeta: metav1.ObjectMeta{Name: "config1", Namespace: namespace, Generation: 123}, Spec: configv1alpha1.FederationDomainSpec{ @@ -1055,6 +1108,14 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { Name: adIdentityProvider.Name, }, }, + { + DisplayName: "can-find-me-four", + ObjectRef: corev1.TypedLocalObjectReference{ + APIGroup: ptr.To(apiGroupSupervisor), + Kind: "GitHubIdentityProvider", + Name: gitHubIdentityProvider.Name, + }, + }, }, }, }, @@ -1077,6 +1138,11 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { UID: adIdentityProvider.UID, Transforms: idtransform.NewTransformationPipeline(), }, + { + DisplayName: "can-find-me-four", + UID: gitHubIdentityProvider.UID, + Transforms: idtransform.NewTransformationPipeline(), + }, }), }, wantStatusUpdates: []*configv1alpha1.FederationDomain{ @@ -1095,6 +1161,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { oidcIdentityProvider, ldapIdentityProvider, adIdentityProvider, + gitHubIdentityProvider, &configv1alpha1.FederationDomain{ ObjectMeta: metav1.ObjectMeta{Name: "config1", Namespace: namespace, Generation: 123}, Spec: configv1alpha1.FederationDomainSpec{ @@ -1148,6 +1215,14 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { Name: adIdentityProvider.Name, }, }, + { + DisplayName: "duplicate2", + ObjectRef: corev1.TypedLocalObjectReference{ + APIGroup: ptr.To(apiGroupSupervisor), + Kind: "GitHubIdentityProvider", + Name: gitHubIdentityProvider.Name, + }, + }, }, }, }, @@ -1174,6 +1249,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { oidcIdentityProvider, ldapIdentityProvider, adIdentityProvider, + gitHubIdentityProvider, &configv1alpha1.FederationDomain{ ObjectMeta: metav1.ObjectMeta{Name: "config1", Namespace: namespace, Generation: 123}, Spec: configv1alpha1.FederationDomainSpec{ @@ -1211,6 +1287,14 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { Name: adIdentityProvider.Name, }, }, + { + DisplayName: "name5", + ObjectRef: corev1.TypedLocalObjectReference{ + APIGroup: ptr.To(apiGroupSupervisor), // correct + Kind: "GitHubIdentityProvider", + Name: gitHubIdentityProvider.Name, + }, + }, }, }, }, @@ -1244,6 +1328,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { oidcIdentityProvider, ldapIdentityProvider, adIdentityProvider, + gitHubIdentityProvider, &configv1alpha1.FederationDomain{ ObjectMeta: metav1.ObjectMeta{Name: "config1", Namespace: namespace, Generation: 123}, Spec: configv1alpha1.FederationDomainSpec{ @@ -2024,6 +2109,7 @@ func TestTestFederationDomainWatcherControllerSync(t *testing.T) { pinnipedInformers.IDP().V1alpha1().OIDCIdentityProviders(), pinnipedInformers.IDP().V1alpha1().LDAPIdentityProviders(), pinnipedInformers.IDP().V1alpha1().ActiveDirectoryIdentityProviders(), + pinnipedInformers.IDP().V1alpha1().GitHubIdentityProviders(), controllerlib.WithInformer, ) diff --git a/internal/supervisor/server/server.go b/internal/supervisor/server/server.go index 1afe994cf..d66694c07 100644 --- a/internal/supervisor/server/server.go +++ b/internal/supervisor/server/server.go @@ -177,6 +177,7 @@ func prepareControllers( pinnipedInformers.IDP().V1alpha1().OIDCIdentityProviders(), pinnipedInformers.IDP().V1alpha1().LDAPIdentityProviders(), pinnipedInformers.IDP().V1alpha1().ActiveDirectoryIdentityProviders(), + pinnipedInformers.IDP().V1alpha1().GitHubIdentityProviders(), controllerlib.WithInformer, ), singletonWorker, From 0edee374986ee011dd08675ee7f40ca6fe1d6a40 Mon Sep 17 00:00:00 2001 From: "Benjamin A. Petersen" Date: Wed, 10 Apr 2024 15:05:18 -0400 Subject: [PATCH 04/16] Update idp discovery types --- .../idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go.tmpl | 1 - .../idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go | 1 - .../idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go | 1 - .../idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go | 1 - .../idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go | 1 - .../idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go | 1 - .../idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go | 1 - .../idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go | 1 - .../idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go | 1 - .../idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go | 1 - .../idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go | 1 - 11 files changed, 11 deletions(-) diff --git a/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go.tmpl b/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go.tmpl index ed1836768..303acdbfb 100644 --- a/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go.tmpl +++ b/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go.tmpl @@ -15,7 +15,6 @@ const ( IDPTypeOIDC IDPType = "oidc" IDPTypeLDAP IDPType = "ldap" IDPTypeActiveDirectory IDPType = "activedirectory" - IDPTypeGitHub IDPType = "github" IDPFlowCLIPassword IDPFlow = "cli_password" IDPFlowBrowserAuthcode IDPFlow = "browser_authcode" diff --git a/generated/1.21/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go b/generated/1.21/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go index ed1836768..303acdbfb 100644 --- a/generated/1.21/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go +++ b/generated/1.21/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go @@ -15,7 +15,6 @@ const ( IDPTypeOIDC IDPType = "oidc" IDPTypeLDAP IDPType = "ldap" IDPTypeActiveDirectory IDPType = "activedirectory" - IDPTypeGitHub IDPType = "github" IDPFlowCLIPassword IDPFlow = "cli_password" IDPFlowBrowserAuthcode IDPFlow = "browser_authcode" diff --git a/generated/1.22/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go b/generated/1.22/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go index ed1836768..303acdbfb 100644 --- a/generated/1.22/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go +++ b/generated/1.22/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go @@ -15,7 +15,6 @@ const ( IDPTypeOIDC IDPType = "oidc" IDPTypeLDAP IDPType = "ldap" IDPTypeActiveDirectory IDPType = "activedirectory" - IDPTypeGitHub IDPType = "github" IDPFlowCLIPassword IDPFlow = "cli_password" IDPFlowBrowserAuthcode IDPFlow = "browser_authcode" diff --git a/generated/1.23/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go b/generated/1.23/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go index ed1836768..303acdbfb 100644 --- a/generated/1.23/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go +++ b/generated/1.23/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go @@ -15,7 +15,6 @@ const ( IDPTypeOIDC IDPType = "oidc" IDPTypeLDAP IDPType = "ldap" IDPTypeActiveDirectory IDPType = "activedirectory" - IDPTypeGitHub IDPType = "github" IDPFlowCLIPassword IDPFlow = "cli_password" IDPFlowBrowserAuthcode IDPFlow = "browser_authcode" diff --git a/generated/1.24/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go b/generated/1.24/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go index ed1836768..303acdbfb 100644 --- a/generated/1.24/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go +++ b/generated/1.24/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go @@ -15,7 +15,6 @@ const ( IDPTypeOIDC IDPType = "oidc" IDPTypeLDAP IDPType = "ldap" IDPTypeActiveDirectory IDPType = "activedirectory" - IDPTypeGitHub IDPType = "github" IDPFlowCLIPassword IDPFlow = "cli_password" IDPFlowBrowserAuthcode IDPFlow = "browser_authcode" diff --git a/generated/1.25/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go b/generated/1.25/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go index ed1836768..303acdbfb 100644 --- a/generated/1.25/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go +++ b/generated/1.25/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go @@ -15,7 +15,6 @@ const ( IDPTypeOIDC IDPType = "oidc" IDPTypeLDAP IDPType = "ldap" IDPTypeActiveDirectory IDPType = "activedirectory" - IDPTypeGitHub IDPType = "github" IDPFlowCLIPassword IDPFlow = "cli_password" IDPFlowBrowserAuthcode IDPFlow = "browser_authcode" diff --git a/generated/1.26/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go b/generated/1.26/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go index ed1836768..303acdbfb 100644 --- a/generated/1.26/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go +++ b/generated/1.26/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go @@ -15,7 +15,6 @@ const ( IDPTypeOIDC IDPType = "oidc" IDPTypeLDAP IDPType = "ldap" IDPTypeActiveDirectory IDPType = "activedirectory" - IDPTypeGitHub IDPType = "github" IDPFlowCLIPassword IDPFlow = "cli_password" IDPFlowBrowserAuthcode IDPFlow = "browser_authcode" diff --git a/generated/1.27/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go b/generated/1.27/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go index ed1836768..303acdbfb 100644 --- a/generated/1.27/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go +++ b/generated/1.27/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go @@ -15,7 +15,6 @@ const ( IDPTypeOIDC IDPType = "oidc" IDPTypeLDAP IDPType = "ldap" IDPTypeActiveDirectory IDPType = "activedirectory" - IDPTypeGitHub IDPType = "github" IDPFlowCLIPassword IDPFlow = "cli_password" IDPFlowBrowserAuthcode IDPFlow = "browser_authcode" diff --git a/generated/1.28/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go b/generated/1.28/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go index ed1836768..303acdbfb 100644 --- a/generated/1.28/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go +++ b/generated/1.28/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go @@ -15,7 +15,6 @@ const ( IDPTypeOIDC IDPType = "oidc" IDPTypeLDAP IDPType = "ldap" IDPTypeActiveDirectory IDPType = "activedirectory" - IDPTypeGitHub IDPType = "github" IDPFlowCLIPassword IDPFlow = "cli_password" IDPFlowBrowserAuthcode IDPFlow = "browser_authcode" diff --git a/generated/1.29/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go b/generated/1.29/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go index ed1836768..303acdbfb 100644 --- a/generated/1.29/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go +++ b/generated/1.29/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go @@ -15,7 +15,6 @@ const ( IDPTypeOIDC IDPType = "oidc" IDPTypeLDAP IDPType = "ldap" IDPTypeActiveDirectory IDPType = "activedirectory" - IDPTypeGitHub IDPType = "github" IDPFlowCLIPassword IDPFlow = "cli_password" IDPFlowBrowserAuthcode IDPFlow = "browser_authcode" diff --git a/generated/latest/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go b/generated/latest/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go index ed1836768..303acdbfb 100644 --- a/generated/latest/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go +++ b/generated/latest/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go @@ -15,7 +15,6 @@ const ( IDPTypeOIDC IDPType = "oidc" IDPTypeLDAP IDPType = "ldap" IDPTypeActiveDirectory IDPType = "activedirectory" - IDPTypeGitHub IDPType = "github" IDPFlowCLIPassword IDPFlow = "cli_password" IDPFlowBrowserAuthcode IDPFlow = "browser_authcode" From e3aa495e0bc7f83a0bfd1db4831107ed61107f44 Mon Sep 17 00:00:00 2001 From: "Benjamin A. Petersen" Date: Wed, 10 Apr 2024 15:09:23 -0400 Subject: [PATCH 05/16] Update idp discovery handler test --- .../endpoints/idpdiscovery/idp_discovery_handler_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/internal/federationdomain/endpoints/idpdiscovery/idp_discovery_handler_test.go b/internal/federationdomain/endpoints/idpdiscovery/idp_discovery_handler_test.go index 892a1dc84..241cf1dcd 100644 --- a/internal/federationdomain/endpoints/idpdiscovery/idp_discovery_handler_test.go +++ b/internal/federationdomain/endpoints/idpdiscovery/idp_discovery_handler_test.go @@ -39,6 +39,7 @@ func TestIDPDiscovery(t *testing.T) { "pinniped_identity_providers": [ {"name": "a-some-ldap-idp", "type": "ldap", "flows": ["cli_password", "browser_authcode"]}, {"name": "a-some-oidc-idp", "type": "oidc", "flows": ["browser_authcode"]}, + {"name": "g-some-github-idp", "type": "github", "flows": ["browser_authcode"]}, {"name": "x-some-ldap-idp", "type": "ldap", "flows": ["cli_password", "browser_authcode"]}, {"name": "x-some-oidc-idp", "type": "oidc", "flows": ["browser_authcode"]}, {"name": "y-some-ad-idp", "type": "activedirectory", "flows": ["cli_password", "browser_authcode"]}, @@ -49,6 +50,7 @@ func TestIDPDiscovery(t *testing.T) { }`), wantSecondResponseBodyJSON: here.Doc(`{ "pinniped_identity_providers": [ + {"name": "g-some-github-idp", "type": "github", "flows": ["browser_authcode"]}, {"name": "some-other-ad-idp-1", "type": "activedirectory", "flows": ["cli_password", "browser_authcode"]}, {"name": "some-other-ad-idp-2", "type": "activedirectory", "flows": ["cli_password", "browser_authcode"]}, {"name": "some-other-ldap-idp-1", "type": "ldap", "flows": ["cli_password", "browser_authcode"]}, @@ -77,6 +79,7 @@ func TestIDPDiscovery(t *testing.T) { WithOIDC(oidctestutil.NewTestUpstreamOIDCIdentityProviderBuilder().WithName("a-some-oidc-idp").Build()). WithLDAP(oidctestutil.NewTestUpstreamLDAPIdentityProviderBuilder().WithName("z-some-ldap-idp").Build()). WithLDAP(oidctestutil.NewTestUpstreamLDAPIdentityProviderBuilder().WithName("x-some-ldap-idp").Build()). + WithGitHub(oidctestutil.NewTestUpstreamGitHubIdentityProviderBuilder().WithName("g-some-github-idp").Build()). WithActiveDirectory(oidctestutil.NewTestUpstreamLDAPIdentityProviderBuilder().WithName("z-some-ad-idp").Build()). WithActiveDirectory(oidctestutil.NewTestUpstreamLDAPIdentityProviderBuilder().WithName("y-some-ad-idp").Build()). BuildFederationDomainIdentityProvidersListerFinder() From 0e3641bba2776c9d187257c03e773933f16ae5c0 Mon Sep 17 00:00:00 2001 From: "Benjamin A. Petersen" Date: Wed, 10 Apr 2024 15:11:17 -0400 Subject: [PATCH 06/16] Fix test idp lister --- internal/testutil/testidplister/testidplister.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/testutil/testidplister/testidplister.go b/internal/testutil/testidplister/testidplister.go index c753d9af4..8195949e0 100644 --- a/internal/testutil/testidplister/testidplister.go +++ b/internal/testutil/testidplister/testidplister.go @@ -39,7 +39,7 @@ func (t *TestFederationDomainIdentityProvidersListerFinder) IDPCount() int { func (t *TestFederationDomainIdentityProvidersListerFinder) GetIdentityProviders() []resolvedprovider.FederationDomainResolvedIdentityProvider { fdIDPs := make([]resolvedprovider.FederationDomainResolvedIdentityProvider, - len(t.upstreamOIDCIdentityProviders)+len(t.upstreamLDAPIdentityProviders)+len(t.upstreamActiveDirectoryIdentityProviders)) + len(t.upstreamOIDCIdentityProviders)+len(t.upstreamLDAPIdentityProviders)+len(t.upstreamActiveDirectoryIdentityProviders)+len(t.upstreamGitHubIdentityProviders)) i := 0 for _, testIDP := range t.upstreamOIDCIdentityProviders { fdIDP := &resolvedoidc.FederationDomainResolvedOIDCIdentityProvider{ From 8f71f965b99f9a45b7613dbd33b38d49afd1ed0d Mon Sep 17 00:00:00 2001 From: "Benjamin A. Petersen" Date: Wed, 10 Apr 2024 15:15:11 -0400 Subject: [PATCH 07/16] Add github to login_oidc.go --- cmd/pinniped/cmd/login_oidc.go | 7 +++---- cmd/pinniped/cmd/login_oidc_test.go | 4 ++-- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/cmd/pinniped/cmd/login_oidc.go b/cmd/pinniped/cmd/login_oidc.go index 34f51706e..edda536a5 100644 --- a/cmd/pinniped/cmd/login_oidc.go +++ b/cmd/pinniped/cmd/login_oidc.go @@ -304,7 +304,8 @@ func flowOptions( } switch requestedIDPType { - case idpdiscoveryv1alpha1.IDPTypeOIDC: + // TODO(BEN): previously i had put a panic("GitHub NOPE") here. Now its time to work out the details of doing an actual GitHub Login.... + case idpdiscoveryv1alpha1.IDPTypeOIDC, idpdiscoveryv1alpha1.IDPTypeGitHub: switch requestedFlow { case idpdiscoveryv1alpha1.IDPFlowCLIPassword: return useCLIFlow, nil @@ -328,9 +329,6 @@ func flowOptions( flowSource, requestedIDPType, requestedFlow, strings.Join([]string{idpdiscoveryv1alpha1.IDPFlowCLIPassword.String(), idpdiscoveryv1alpha1.IDPFlowBrowserAuthcode.String()}, ", ")) } - // TODO: implement this - case idpdiscoveryv1alpha1.IDPTypeGitHub: - panic("GitHub has not been implemented yet.") default: // Surprisingly cobra does not support this kind of flag validation. See https://github.com/spf13/pflag/issues/236 return nil, fmt.Errorf( @@ -340,6 +338,7 @@ func flowOptions( idpdiscoveryv1alpha1.IDPTypeOIDC.String(), idpdiscoveryv1alpha1.IDPTypeLDAP.String(), idpdiscoveryv1alpha1.IDPTypeActiveDirectory.String(), + idpdiscoveryv1alpha1.IDPTypeGitHub.String(), }, ", "), ) } diff --git a/cmd/pinniped/cmd/login_oidc_test.go b/cmd/pinniped/cmd/login_oidc_test.go index 42b84f9fa..1bdccb595 100644 --- a/cmd/pinniped/cmd/login_oidc_test.go +++ b/cmd/pinniped/cmd/login_oidc_test.go @@ -161,7 +161,7 @@ func TestLoginOIDCCommand(t *testing.T) { }, wantError: true, wantStderr: here.Doc(` - Error: --upstream-identity-provider-type value not recognized: invalid (supported values: oidc, ldap, activedirectory) + Error: --upstream-identity-provider-type value not recognized: invalid (supported values: oidc, ldap, activedirectory, github) `), }, { @@ -173,7 +173,7 @@ func TestLoginOIDCCommand(t *testing.T) { env: map[string]string{"PINNIPED_UPSTREAM_IDENTITY_PROVIDER_FLOW": "browser_authcode"}, wantError: true, wantStderr: here.Doc(` - Error: --upstream-identity-provider-type value not recognized: invalid (supported values: oidc, ldap, activedirectory) + Error: --upstream-identity-provider-type value not recognized: invalid (supported values: oidc, ldap, activedirectory, github) `), }, { From 8de45244289e370927a61c8ff5c15c16cc3a56be Mon Sep 17 00:00:00 2001 From: "Benjamin A. Petersen" Date: Wed, 10 Apr 2024 15:43:43 -0400 Subject: [PATCH 08/16] Add github to kubeconfig.go --- cmd/pinniped/cmd/kubeconfig.go | 15 +++++++++++++-- cmd/pinniped/cmd/kubeconfig_test.go | 7 ++++--- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/cmd/pinniped/cmd/kubeconfig.go b/cmd/pinniped/cmd/kubeconfig.go index 1674c1bd8..ae5e3c2ca 100644 --- a/cmd/pinniped/cmd/kubeconfig.go +++ b/cmd/pinniped/cmd/kubeconfig.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 cmd @@ -143,7 +143,18 @@ func kubeconfigCommand(deps kubeconfigDeps) *cobra.Command { f.BoolVar(&flags.oidc.debugSessionCache, "oidc-debug-session-cache", false, "Print debug logs related to the OpenID Connect session cache") f.StringVar(&flags.oidc.requestAudience, "oidc-request-audience", "", "Request a token with an alternate audience using RFC8693 token exchange") f.StringVar(&flags.oidc.upstreamIDPName, "upstream-identity-provider-name", "", "The name of the upstream identity provider used during login with a Supervisor") - f.StringVar(&flags.oidc.upstreamIDPType, "upstream-identity-provider-type", "", fmt.Sprintf("The type of the upstream identity provider used during login with a Supervisor (e.g. '%s', '%s', '%s')", idpdiscoveryv1alpha1.IDPTypeOIDC, idpdiscoveryv1alpha1.IDPTypeLDAP, idpdiscoveryv1alpha1.IDPTypeActiveDirectory)) + f.StringVar( + &flags.oidc.upstreamIDPType, + "upstream-identity-provider-type", + "", + fmt.Sprintf( + "The type of the upstream identity provider used during login with a Supervisor (e.g. '%s', '%s', '%s', '%s')", + idpdiscoveryv1alpha1.IDPTypeOIDC, + idpdiscoveryv1alpha1.IDPTypeLDAP, + idpdiscoveryv1alpha1.IDPTypeActiveDirectory, + idpdiscoveryv1alpha1.IDPTypeGitHub, + ), + ) f.StringVar(&flags.oidc.upstreamIDPFlow, "upstream-identity-provider-flow", "", fmt.Sprintf("The type of client flow to use with the upstream identity provider during login with a Supervisor (e.g. '%s', '%s')", idpdiscoveryv1alpha1.IDPFlowCLIPassword, idpdiscoveryv1alpha1.IDPFlowBrowserAuthcode)) f.StringVar(&flags.kubeconfigPath, "kubeconfig", os.Getenv("KUBECONFIG"), "Path to kubeconfig file") f.StringVar(&flags.kubeconfigContextOverride, "kubeconfig-context", "", "Kubeconfig context name (default: current active context)") diff --git a/cmd/pinniped/cmd/kubeconfig_test.go b/cmd/pinniped/cmd/kubeconfig_test.go index 1af712dcf..4a2770a06 100644 --- a/cmd/pinniped/cmd/kubeconfig_test.go +++ b/cmd/pinniped/cmd/kubeconfig_test.go @@ -156,7 +156,7 @@ func TestGetKubeconfig(t *testing.T) { --timeout duration Timeout for autodiscovery and validation (default 10m0s) --upstream-identity-provider-flow string The type of client flow to use with the upstream identity provider during login with a Supervisor (e.g. 'cli_password', 'browser_authcode') --upstream-identity-provider-name string The name of the upstream identity provider used during login with a Supervisor - --upstream-identity-provider-type string The type of the upstream identity provider used during login with a Supervisor (e.g. 'oidc', 'ldap', 'activedirectory') + --upstream-identity-provider-type string The type of the upstream identity provider used during login with a Supervisor (e.g. 'oidc', 'ldap', 'activedirectory', 'github') `) }, }, @@ -908,7 +908,8 @@ func TestGetKubeconfig(t *testing.T) { idpsDiscoveryResponse: here.Docf(`{ "pinniped_identity_providers": [ {"name": "some-ldap-idp", "type": "ldap"}, - {"name": "some-oidc-idp", "type": "oidc"} + {"name": "some-oidc-idp", "type": "oidc"}, + {"name": "some-github-idp", "type": "github"} ] }`), wantLogs: func(issuerCABundle string, issuerURL string) []string { @@ -927,7 +928,7 @@ func TestGetKubeconfig(t *testing.T) { wantStderr: func(issuerCABundle string, issuerURL string) testutil.RequireErrorStringFunc { return testutil.WantExactErrorString(`Error: multiple Supervisor upstream identity providers were found, ` + `so the --upstream-identity-provider-name/--upstream-identity-provider-type flags must be specified. ` + - `Found these upstreams: [{"name":"some-ldap-idp","type":"ldap"},{"name":"some-oidc-idp","type":"oidc"}]` + "\n") + `Found these upstreams: [{"name":"some-ldap-idp","type":"ldap"},{"name":"some-oidc-idp","type":"oidc"},{"name":"some-github-idp","type":"github"}]` + "\n") }, }, { From 79d0e7405677ff808bd699a0345c6505de2fef20 Mon Sep 17 00:00:00 2001 From: "Benjamin A. Petersen" Date: Thu, 11 Apr 2024 13:19:24 -0400 Subject: [PATCH 09/16] Fix github_upstream_watcher so GitHub is listed in Supervisor idp discovery doc --- cmd/pinniped/cmd/login_oidc.go | 2 +- .../supervisorconfig/federation_domain_watcher.go | 5 +++-- .../resolvedgithub/resolved_github_provider.go | 14 ++++++++++++-- internal/psession/pinniped_session.go | 1 - internal/upstreamgithub/upstreamgithub.go | 2 +- 5 files changed, 17 insertions(+), 7 deletions(-) diff --git a/cmd/pinniped/cmd/login_oidc.go b/cmd/pinniped/cmd/login_oidc.go index edda536a5..7d73ab9ee 100644 --- a/cmd/pinniped/cmd/login_oidc.go +++ b/cmd/pinniped/cmd/login_oidc.go @@ -304,7 +304,7 @@ func flowOptions( } switch requestedIDPType { - // TODO(BEN): previously i had put a panic("GitHub NOPE") here. Now its time to work out the details of doing an actual GitHub Login.... + // TODO: Decide if we can bundle GitHub here long term or if it needs its own case case idpdiscoveryv1alpha1.IDPTypeOIDC, idpdiscoveryv1alpha1.IDPTypeGitHub: switch requestedFlow { case idpdiscoveryv1alpha1.IDPFlowCLIPassword: diff --git a/internal/controller/supervisorconfig/federation_domain_watcher.go b/internal/controller/supervisorconfig/federation_domain_watcher.go index d40f7f85f..1edcc474f 100644 --- a/internal/controller/supervisorconfig/federation_domain_watcher.go +++ b/internal/controller/supervisorconfig/federation_domain_watcher.go @@ -90,8 +90,9 @@ type federationDomainWatcherController struct { ldapIdentityProviderInformer idpinformers.LDAPIdentityProviderInformer activeDirectoryIdentityProviderInformer idpinformers.ActiveDirectoryIdentityProviderInformer githubIdentityProviderInformer idpinformers.GitHubIdentityProviderInformer - celTransformer *celtransformer.CELTransformer - allowedKinds sets.Set[string] + + celTransformer *celtransformer.CELTransformer + allowedKinds sets.Set[string] } // NewFederationDomainWatcherController creates a controllerlib.Controller that watches diff --git a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go index e84e92d9d..3574d4e06 100644 --- a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go +++ b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go @@ -5,6 +5,7 @@ package resolvedgithub import ( "context" + "fmt" "go.pinniped.dev/generated/latest/apis/supervisor/idpdiscovery/v1alpha1" "go.pinniped.dev/internal/federationdomain/resolvedprovider" @@ -44,8 +45,13 @@ func (p *FederationDomainResolvedGitHubIdentityProvider) GetIDPDiscoveryType() v } func (p *FederationDomainResolvedGitHubIdentityProvider) GetIDPDiscoveryFlows() []v1alpha1.IDPFlow { - // TODO: implement - return []v1alpha1.IDPFlow{} + // TODO: review and see if this is actually true to follow the OIDC model + flows := []v1alpha1.IDPFlow{v1alpha1.IDPFlowBrowserAuthcode} + // TODO: coming as a later feature? The UpstreamGithubIdentityProviderI does not currently impl this func + // if p.Provider.AllowsPasswordGrant() { + // flows = append(flows, v1alpha1.IDPFlowCLIPassword) + // } + return flows } func (p *FederationDomainResolvedGitHubIdentityProvider) GetTransforms() *idtransform.TransformationPipeline { @@ -68,6 +74,7 @@ func (p *FederationDomainResolvedGitHubIdentityProvider) UpstreamAuthorizeRedire downstreamIssuerURL string, //nolint:all ) (string, error) { // TODO: implement + fmt.Printf("GithubResolvedIdentityProvider ~ UpstreamAuthorizeRedirectURL() called with state: %#v, downstreamIssuerURL %s", state, downstreamIssuerURL) return "", nil } @@ -77,6 +84,7 @@ func (p *FederationDomainResolvedGitHubIdentityProvider) Login( submittedPassword string, //nolint:all ) (*resolvedprovider.Identity, *resolvedprovider.IdentityLoginExtras, error) { // TODO: implement + fmt.Printf("GithubResolvedIdentityProvider ~ Login() called with submittedUserName %s, submittedPassword %s", submittedUsername, submittedPassword) return nil, nil, nil } @@ -88,6 +96,7 @@ func (p *FederationDomainResolvedGitHubIdentityProvider) LoginFromCallback( redirectURI string, //nolint:all ) (*resolvedprovider.Identity, *resolvedprovider.IdentityLoginExtras, error) { // TODO: implement + fmt.Printf("GithubResolvedIdentityProvider ~ LoginFromCallback() called wtih authCode: %s, pkce: %#v, nonce: %#v, redirectURI: %s", authCode, pkce, nonce, redirectURI) return nil, nil, nil } @@ -96,5 +105,6 @@ func (p *FederationDomainResolvedGitHubIdentityProvider) UpstreamRefresh( identity *resolvedprovider.Identity, //nolint:all ) (refreshedIdentity *resolvedprovider.RefreshedIdentity, err error) { // TODO: implement + fmt.Printf("GithubResolvedIdentityProvider ~ UpstreamRefresh() called with identity %#v", identity) return nil, nil } diff --git a/internal/psession/pinniped_session.go b/internal/psession/pinniped_session.go index 76fccc3a0..a1f34a11b 100644 --- a/internal/psession/pinniped_session.go +++ b/internal/psession/pinniped_session.go @@ -146,7 +146,6 @@ func (s *ActiveDirectorySessionData) Clone() *ActiveDirectorySessionData { // TODO: flesh this out, GitHub will need additional data. type GitHubSessionData struct { - // TODO: flesh this out. } func (s *GitHubSessionData) Clone() *GitHubSessionData { diff --git a/internal/upstreamgithub/upstreamgithub.go b/internal/upstreamgithub/upstreamgithub.go index cd2c15875..678ab0ae1 100644 --- a/internal/upstreamgithub/upstreamgithub.go +++ b/internal/upstreamgithub/upstreamgithub.go @@ -96,7 +96,7 @@ func (p *Provider) GetHttpClient() *http.Client { // AuthenticateUser authenticates an end user and returns their mapped username, groups, and UID. Implements authenticators.UserAuthenticator. func (p *Provider) AuthenticateUser( - ctx context.Context, //nolint:all + ctx context.Context, //nolint:all username, password string, //nolint:all ) (*authenticators.Response, bool, error) { // TODO: implement this, currently just placeholder to satisfy UserAuthenticator interface above From 42ef46b74e0294c3dd5cf783fc41e3bcff411382 Mon Sep 17 00:00:00 2001 From: "Benjamin A. Petersen" Date: Mon, 22 Apr 2024 16:36:14 -0400 Subject: [PATCH 10/16] expand TestUpstreamGitHubIdentityProvider --- .../resolved_github_provider.go | 20 ++-- .../oidctestutil/testgithubprovider.go | 91 ++++++++++++++++++- 2 files changed, 99 insertions(+), 12 deletions(-) diff --git a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go index 3574d4e06..faca23e6b 100644 --- a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go +++ b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go @@ -70,8 +70,8 @@ func (p *FederationDomainResolvedGitHubIdentityProvider) ApplyIDPSpecificSession } func (p *FederationDomainResolvedGitHubIdentityProvider) UpstreamAuthorizeRedirectURL( - state *resolvedprovider.UpstreamAuthorizeRequestState, //nolint:all - downstreamIssuerURL string, //nolint:all + state *resolvedprovider.UpstreamAuthorizeRequestState, + downstreamIssuerURL string, ) (string, error) { // TODO: implement fmt.Printf("GithubResolvedIdentityProvider ~ UpstreamAuthorizeRedirectURL() called with state: %#v, downstreamIssuerURL %s", state, downstreamIssuerURL) @@ -80,8 +80,8 @@ func (p *FederationDomainResolvedGitHubIdentityProvider) UpstreamAuthorizeRedire func (p *FederationDomainResolvedGitHubIdentityProvider) Login( ctx context.Context, //nolint:all - submittedUsername string, //nolint:all - submittedPassword string, //nolint:all + submittedUsername string, + submittedPassword string, ) (*resolvedprovider.Identity, *resolvedprovider.IdentityLoginExtras, error) { // TODO: implement fmt.Printf("GithubResolvedIdentityProvider ~ Login() called with submittedUserName %s, submittedPassword %s", submittedUsername, submittedPassword) @@ -90,19 +90,19 @@ func (p *FederationDomainResolvedGitHubIdentityProvider) Login( func (p *FederationDomainResolvedGitHubIdentityProvider) LoginFromCallback( ctx context.Context, //nolint:all - authCode string, //nolint:all - pkce pkce.Code, //nolint:all - nonce nonce.Nonce, //nolint:all - redirectURI string, //nolint:all + authCode string, + pkce pkce.Code, + nonce nonce.Nonce, + redirectURI string, ) (*resolvedprovider.Identity, *resolvedprovider.IdentityLoginExtras, error) { // TODO: implement - fmt.Printf("GithubResolvedIdentityProvider ~ LoginFromCallback() called wtih authCode: %s, pkce: %#v, nonce: %#v, redirectURI: %s", authCode, pkce, nonce, redirectURI) + fmt.Printf("GithubResolvedIdentityProvider ~ LoginFromCallback() called with authCode: %s, pkce: %#v, nonce: %#v, redirectURI: %s", authCode, pkce, nonce, redirectURI) return nil, nil, nil } func (p *FederationDomainResolvedGitHubIdentityProvider) UpstreamRefresh( ctx context.Context, //nolint:all - identity *resolvedprovider.Identity, //nolint:all + identity *resolvedprovider.Identity, ) (refreshedIdentity *resolvedprovider.RefreshedIdentity, err error) { // TODO: implement fmt.Printf("GithubResolvedIdentityProvider ~ UpstreamRefresh() called with identity %#v", identity) diff --git a/internal/testutil/oidctestutil/testgithubprovider.go b/internal/testutil/oidctestutil/testgithubprovider.go index 361ae30df..5b985815b 100644 --- a/internal/testutil/oidctestutil/testgithubprovider.go +++ b/internal/testutil/oidctestutil/testgithubprovider.go @@ -4,19 +4,27 @@ package oidctestutil import ( + "net/http" + "k8s.io/apimachinery/pkg/types" + "go.pinniped.dev/generated/latest/apis/supervisor/idp/v1alpha1" "go.pinniped.dev/internal/federationdomain/upstreamprovider" "go.pinniped.dev/internal/idtransform" ) -// TODO: flesh this out. type TestUpstreamGitHubIdentityProviderBuilder struct { name string clientID string resourceUID types.UID displayNameForFederationDomain string transformsForFederationDomain *idtransform.TransformationPipeline + usernameAttribute v1alpha1.GitHubUsernameAttribute + groupNameAttribute v1alpha1.GitHubGroupNameAttribute + allowedOrganizations []string + organizationLoginPolicy v1alpha1.GitHubAllowedAuthOrganizationsPolicy + authorizationURL string + httpClient *http.Client } func (u *TestUpstreamGitHubIdentityProviderBuilder) WithName(value string) *TestUpstreamGitHubIdentityProviderBuilder { @@ -34,6 +42,41 @@ func (u *TestUpstreamGitHubIdentityProviderBuilder) WithClientID(value string) * return u } +func (u *TestUpstreamGitHubIdentityProviderBuilder) WithDisplayNameForFederationDomain(value string) *TestUpstreamGitHubIdentityProviderBuilder { + u.displayNameForFederationDomain = value + return u +} + +func (u *TestUpstreamGitHubIdentityProviderBuilder) WithUsernameAttribute(value v1alpha1.GitHubUsernameAttribute) *TestUpstreamGitHubIdentityProviderBuilder { + u.usernameAttribute = value + return u +} + +func (u *TestUpstreamGitHubIdentityProviderBuilder) WithGroupNameAttribute(value v1alpha1.GitHubGroupNameAttribute) *TestUpstreamGitHubIdentityProviderBuilder { + u.groupNameAttribute = value + return u +} + +func (u *TestUpstreamGitHubIdentityProviderBuilder) WithAllowedOrganizations(value []string) *TestUpstreamGitHubIdentityProviderBuilder { + u.allowedOrganizations = value + return u +} + +func (u *TestUpstreamGitHubIdentityProviderBuilder) WithOrganizationLoginPolicy(value v1alpha1.GitHubAllowedAuthOrganizationsPolicy) *TestUpstreamGitHubIdentityProviderBuilder { + u.organizationLoginPolicy = value + return u +} + +func (u *TestUpstreamGitHubIdentityProviderBuilder) WithAuthorizationURL(value string) *TestUpstreamGitHubIdentityProviderBuilder { + u.authorizationURL = value + return u +} + +func (u *TestUpstreamGitHubIdentityProviderBuilder) WithHttpClient(value *http.Client) *TestUpstreamGitHubIdentityProviderBuilder { + u.httpClient = value + return u +} + func (u *TestUpstreamGitHubIdentityProviderBuilder) Build() *TestUpstreamGitHubIdentityProvider { if u.displayNameForFederationDomain == "" { // default it to the CR name @@ -43,13 +86,18 @@ func (u *TestUpstreamGitHubIdentityProviderBuilder) Build() *TestUpstreamGitHubI // default to an empty pipeline u.transformsForFederationDomain = idtransform.NewTransformationPipeline() } - // TODO: flesh this out. return &TestUpstreamGitHubIdentityProvider{ Name: u.name, ResourceUID: u.resourceUID, ClientID: u.clientID, DisplayNameForFederationDomain: u.displayNameForFederationDomain, TransformsForFederationDomain: u.transformsForFederationDomain, + UsernameAttribute: u.usernameAttribute, + GroupNameAttribute: u.groupNameAttribute, + AllowedOrganizations: u.allowedOrganizations, + OrganizationLoginPolicy: u.organizationLoginPolicy, + AuthorizationURL: u.authorizationURL, + HttpClient: u.httpClient, } } @@ -62,8 +110,15 @@ type TestUpstreamGitHubIdentityProvider struct { Name string ClientID string ResourceUID types.UID + Host string DisplayNameForFederationDomain string TransformsForFederationDomain *idtransform.TransformationPipeline + UsernameAttribute v1alpha1.GitHubUsernameAttribute + GroupNameAttribute v1alpha1.GitHubGroupNameAttribute + AllowedOrganizations []string + OrganizationLoginPolicy v1alpha1.GitHubAllowedAuthOrganizationsPolicy + AuthorizationURL string + HttpClient *http.Client } var _ upstreamprovider.UpstreamGithubIdentityProviderI = &TestUpstreamGitHubIdentityProvider{} @@ -75,3 +130,35 @@ func (u *TestUpstreamGitHubIdentityProvider) GetResourceUID() types.UID { func (u *TestUpstreamGitHubIdentityProvider) GetName() string { return u.Name } + +func (u *TestUpstreamGitHubIdentityProvider) GetHost() string { + return u.Host +} + +func (u *TestUpstreamGitHubIdentityProvider) GetClientID() string { + return u.ClientID +} + +func (u *TestUpstreamGitHubIdentityProvider) GetUsernameAttribute() v1alpha1.GitHubUsernameAttribute { + return u.UsernameAttribute +} + +func (u *TestUpstreamGitHubIdentityProvider) GetGroupNameAttribute() v1alpha1.GitHubGroupNameAttribute { + return u.GroupNameAttribute +} + +func (u *TestUpstreamGitHubIdentityProvider) GetAllowedOrganizations() []string { + return u.AllowedOrganizations +} + +func (u *TestUpstreamGitHubIdentityProvider) GetOrganizationLoginPolicy() v1alpha1.GitHubAllowedAuthOrganizationsPolicy { + return u.OrganizationLoginPolicy +} + +func (u *TestUpstreamGitHubIdentityProvider) GetAuthorizationURL() string { + return u.AuthorizationURL +} + +func (u *TestUpstreamGitHubIdentityProvider) GetHttpClient() *http.Client { + return u.HttpClient +} From be1915d2d7e91cca2783651f5739748d95c632d0 Mon Sep 17 00:00:00 2001 From: Joshua Casey Date: Wed, 17 Apr 2024 17:39:59 -0500 Subject: [PATCH 11/16] fixed fuzzing --- .../fositestorage/authorizationcode/authorizationcode.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/internal/fositestorage/authorizationcode/authorizationcode.go b/internal/fositestorage/authorizationcode/authorizationcode.go index dad061800..f43437081 100644 --- a/internal/fositestorage/authorizationcode/authorizationcode.go +++ b/internal/fositestorage/authorizationcode/authorizationcode.go @@ -371,15 +371,18 @@ const ExpectedAuthorizeCodeSessionJSONFromFuzzing = `{ "ĩŦʀ宍D挟": "q萮左/篣AÚƄŕ~čfVLPC諡}", "姧骦:駝重EȫʆɵʮGɃ": "囤1+,Ȳ齠@ɍB鳛Nč乿ƔǴę鏶" } + }, + "github": { } } }, "requestedAudience": [ - "ň" + "ȝâ融貵捠ʼn0" ], "grantedAudience": [ - "â融貵捠ʼn", - "d鞕ȸ腿tʏƲ%}ſ¯Ɣ 籌Tǘ乚Ȥ2" + "ȸ腿tʏƲ%", + "泡hUɨĪű鹠NƤ鷒絓", + "ěå=瑅ƍ逤ŔfȀ箬+橇肅aā鲴" ] }, "version": "6" From 2753b468fd684c999eb2419ddbebc566eaad7399 Mon Sep 17 00:00:00 2001 From: "Benjamin A. Petersen" Date: Tue, 23 Apr 2024 11:30:10 -0400 Subject: [PATCH 12/16] Update TestSupervisorFederationDomainStatus test --- test/integration/supervisor_federationdomain_status_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/integration/supervisor_federationdomain_status_test.go b/test/integration/supervisor_federationdomain_status_test.go index 454d19de2..a9e40dc0f 100644 --- a/test/integration/supervisor_federationdomain_status_test.go +++ b/test/integration/supervisor_federationdomain_status_test.go @@ -1,4 +1,4 @@ -// Copyright 2023 the Pinniped contributors. All Rights Reserved. +// Copyright 2023-2024 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package integration @@ -362,7 +362,7 @@ func TestSupervisorFederationDomainStatus_Disruptive(t *testing.T) { { Type: "IdentityProvidersObjectRefKindValid", Status: "False", Reason: "KindUnrecognized", Message: `some kinds specified by .spec.identityProviders[].objectRef.kind are not recognized ` + - `(should be one of "ActiveDirectoryIdentityProvider", "LDAPIdentityProvider", "OIDCIdentityProvider"): "this is the wrong kind"`, + `(should be one of "ActiveDirectoryIdentityProvider", "GitHubIdentityProvider", "LDAPIdentityProvider", "OIDCIdentityProvider"): "this is the wrong kind"`, }, { Type: "Ready", Status: "False", Reason: "NotReady", @@ -493,7 +493,7 @@ func TestSupervisorFederationDomainStatus_Disruptive(t *testing.T) { { Type: "IdentityProvidersObjectRefKindValid", Status: "False", Reason: "KindUnrecognized", Message: `some kinds specified by .spec.identityProviders[].objectRef.kind are not recognized ` + - `(should be one of "ActiveDirectoryIdentityProvider", "LDAPIdentityProvider", "OIDCIdentityProvider"): "this is the wrong kind"`, + `(should be one of "ActiveDirectoryIdentityProvider", "GitHubIdentityProvider", "LDAPIdentityProvider", "OIDCIdentityProvider"): "this is the wrong kind"`, }, { Type: "Ready", Status: "False", Reason: "NotReady", From cd86d57763459e253ccb88b9296e2e41b2619e8f Mon Sep 17 00:00:00 2001 From: "Benjamin A. Petersen" Date: Thu, 25 Apr 2024 11:07:55 -0400 Subject: [PATCH 13/16] review cleanup, remove TODOs --- .../types_supervisor_idp_discovery.go.tmpl | 1 + cmd/pinniped/cmd/login_oidc.go | 3 +-- .../github_upstream_watcher_test.go | 4 +-- .../resolved_github_provider.go | 27 +++++++------------ .../resolved_github_provider_test.go | 2 ++ .../resolvedldap/resolved_ldap_provider.go | 12 +++------ .../upstreamprovider/upsteam_provider.go | 1 - .../authorizationcode/authorizationcode.go | 1 - internal/psession/pinniped_session.go | 1 - .../oidctestutil/testgithubprovider.go | 1 - .../testutil/testidplister/testidplister.go | 10 +------ internal/upstreamgithub/upstreamgithub.go | 13 --------- 12 files changed, 18 insertions(+), 58 deletions(-) diff --git a/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go.tmpl b/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go.tmpl index 303acdbfb..1b16bfe90 100644 --- a/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go.tmpl +++ b/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go.tmpl @@ -16,6 +16,7 @@ const ( IDPTypeLDAP IDPType = "ldap" IDPTypeActiveDirectory IDPType = "activedirectory" IDPTypeGitHub IDPType = "github" + IDPFlowCLIPassword IDPFlow = "cli_password" IDPFlowBrowserAuthcode IDPFlow = "browser_authcode" ) diff --git a/cmd/pinniped/cmd/login_oidc.go b/cmd/pinniped/cmd/login_oidc.go index 7d73ab9ee..4369d12ab 100644 --- a/cmd/pinniped/cmd/login_oidc.go +++ b/cmd/pinniped/cmd/login_oidc.go @@ -304,8 +304,7 @@ func flowOptions( } switch requestedIDPType { - // TODO: Decide if we can bundle GitHub here long term or if it needs its own case - case idpdiscoveryv1alpha1.IDPTypeOIDC, idpdiscoveryv1alpha1.IDPTypeGitHub: + case idpdiscoveryv1alpha1.IDPTypeOIDC: 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 92562b641..fa8401714 100644 --- a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go @@ -371,8 +371,7 @@ func TestController(t *testing.T) { wantErr string wantLogs []string wantResultingCache []*upstreamgithub.ProviderConfig - // wantResultingCache []*oidctestutil.TestUpstreamGitHubIdentityProvider - wantResultingUpstreams []v1alpha1.GitHubIdentityProvider + wantResultingUpstreams []v1alpha1.GitHubIdentityProvider }{ { name: "no GitHubIdentityProviders", @@ -1763,7 +1762,6 @@ func TestController(t *testing.T) { var actualIDP *upstreamgithub.Provider for _, possibleIDP := range actualIDPList { if possibleIDP.GetName() == tt.wantResultingCache[i].Name { - // For this check, we know that the actual IDPs are going to have type upstreamgithub.ProviderConfig var ok bool actualIDP, ok = possibleIDP.(*upstreamgithub.Provider) require.True(t, ok) diff --git a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go index faca23e6b..4b17120ef 100644 --- a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go +++ b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider.go @@ -5,6 +5,7 @@ package resolvedgithub import ( "context" + "errors" "fmt" "go.pinniped.dev/generated/latest/apis/supervisor/idpdiscovery/v1alpha1" @@ -45,13 +46,7 @@ func (p *FederationDomainResolvedGitHubIdentityProvider) GetIDPDiscoveryType() v } func (p *FederationDomainResolvedGitHubIdentityProvider) GetIDPDiscoveryFlows() []v1alpha1.IDPFlow { - // TODO: review and see if this is actually true to follow the OIDC model - flows := []v1alpha1.IDPFlow{v1alpha1.IDPFlowBrowserAuthcode} - // TODO: coming as a later feature? The UpstreamGithubIdentityProviderI does not currently impl this func - // if p.Provider.AllowsPasswordGrant() { - // flows = append(flows, v1alpha1.IDPFlowCLIPassword) - // } - return flows + return []v1alpha1.IDPFlow{v1alpha1.IDPFlowBrowserAuthcode} } func (p *FederationDomainResolvedGitHubIdentityProvider) GetTransforms() *idtransform.TransformationPipeline { @@ -73,38 +68,34 @@ func (p *FederationDomainResolvedGitHubIdentityProvider) UpstreamAuthorizeRedire state *resolvedprovider.UpstreamAuthorizeRequestState, downstreamIssuerURL string, ) (string, error) { - // TODO: implement fmt.Printf("GithubResolvedIdentityProvider ~ UpstreamAuthorizeRedirectURL() called with state: %#v, downstreamIssuerURL %s", state, downstreamIssuerURL) - return "", nil + return "", errors.New("function UpstreamAuthorizeRedirectURL not yet implemented for GitHub IDP") } func (p *FederationDomainResolvedGitHubIdentityProvider) Login( - ctx context.Context, //nolint:all + _ context.Context, submittedUsername string, submittedPassword string, ) (*resolvedprovider.Identity, *resolvedprovider.IdentityLoginExtras, error) { - // TODO: implement fmt.Printf("GithubResolvedIdentityProvider ~ Login() called with submittedUserName %s, submittedPassword %s", submittedUsername, submittedPassword) - return nil, nil, nil + return nil, nil, errors.New("function Login not yet implemented for GitHub IDP") } func (p *FederationDomainResolvedGitHubIdentityProvider) LoginFromCallback( - ctx context.Context, //nolint:all + _ context.Context, authCode string, pkce pkce.Code, nonce nonce.Nonce, redirectURI string, ) (*resolvedprovider.Identity, *resolvedprovider.IdentityLoginExtras, error) { - // TODO: implement fmt.Printf("GithubResolvedIdentityProvider ~ LoginFromCallback() called with authCode: %s, pkce: %#v, nonce: %#v, redirectURI: %s", authCode, pkce, nonce, redirectURI) - return nil, nil, nil + return nil, nil, errors.New("function LoginFromCallback not yet implemented for GitHub IDP") } func (p *FederationDomainResolvedGitHubIdentityProvider) UpstreamRefresh( - ctx context.Context, //nolint:all + _ context.Context, identity *resolvedprovider.Identity, ) (refreshedIdentity *resolvedprovider.RefreshedIdentity, err error) { - // TODO: implement fmt.Printf("GithubResolvedIdentityProvider ~ UpstreamRefresh() called with identity %#v", identity) - return nil, nil + return nil, errors.New("function UpstreamRefresh not yet implemented for GitHub IDP") } diff --git a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go index 274144d56..be58da09f 100644 --- a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go +++ b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go @@ -2,3 +2,5 @@ // SPDX-License-Identifier: Apache-2.0 package resolvedgithub + +// TODO: write some tests. diff --git a/internal/federationdomain/resolvedprovider/resolvedldap/resolved_ldap_provider.go b/internal/federationdomain/resolvedprovider/resolvedldap/resolved_ldap_provider.go index d993ec12f..a700ae4fc 100644 --- a/internal/federationdomain/resolvedprovider/resolvedldap/resolved_ldap_provider.go +++ b/internal/federationdomain/resolvedprovider/resolvedldap/resolved_ldap_provider.go @@ -76,9 +76,7 @@ func (p *FederationDomainResolvedLDAPIdentityProvider) CloneIDPSpecificSessionDa return nil } return session.ActiveDirectory.Clone() - case psession.ProviderTypeOIDC: // this is just here to avoid a lint error about not handling all cases - fallthrough - case psession.ProviderTypeGitHub: // this is just here to avoid a lint error about not handling all cases + case psession.ProviderTypeOIDC, psession.ProviderTypeGitHub: // this is just here to avoid a lint error about not handling all cases fallthrough default: return nil @@ -153,9 +151,7 @@ func (p *FederationDomainResolvedLDAPIdentityProvider) Login( UserDN: authenticateResponse.DN, ExtraRefreshAttributes: authenticateResponse.ExtraRefreshAttributes, } - case psession.ProviderTypeOIDC: // this is just here to avoid a lint error about not handling all cases - fallthrough - case psession.ProviderTypeGitHub: // this is just here to avoid a lint error about not handling all cases + case psession.ProviderTypeOIDC, psession.ProviderTypeGitHub: // this is just here to avoid a lint error about not handling all cases fallthrough default: return nil, nil, ErrUnexpectedUpstreamLDAPError.WithWrap(fmt.Errorf("unexpected provider type %q", p.GetSessionProviderType())) @@ -209,9 +205,7 @@ func (p *FederationDomainResolvedLDAPIdentityProvider) UpstreamRefresh( } dn = sessionData.UserDN additionalAttributes = sessionData.ExtraRefreshAttributes - case psession.ProviderTypeOIDC: // this is just here to avoid a lint error about not handling all cases - fallthrough - case psession.ProviderTypeGitHub: // this is just here to avoid a lint error about not handling all cases + case psession.ProviderTypeOIDC, psession.ProviderTypeGitHub: // this is just here to avoid a lint error about not handling all cases fallthrough default: // This shouldn't really happen. diff --git a/internal/federationdomain/upstreamprovider/upsteam_provider.go b/internal/federationdomain/upstreamprovider/upsteam_provider.go index 4717fb01c..ab5920bdf 100644 --- a/internal/federationdomain/upstreamprovider/upsteam_provider.go +++ b/internal/federationdomain/upstreamprovider/upsteam_provider.go @@ -128,7 +128,6 @@ type UpstreamLDAPIdentityProviderI interface { PerformRefresh(ctx context.Context, storedRefreshAttributes RefreshAttributes, idpDisplayName string) (groups []string, err error) } -// TODO: Impl this interface thoroughly to support GitHub login. type UpstreamGithubIdentityProviderI interface { UpstreamIdentityProviderI diff --git a/internal/fositestorage/authorizationcode/authorizationcode.go b/internal/fositestorage/authorizationcode/authorizationcode.go index f43437081..cf3be23ef 100644 --- a/internal/fositestorage/authorizationcode/authorizationcode.go +++ b/internal/fositestorage/authorizationcode/authorizationcode.go @@ -190,7 +190,6 @@ func (e *errSerializationFailureWithCause) Error() string { return fmt.Sprintf("%s: %s", fosite.ErrSerializationFailure, e.cause) } -// TODO: need to revisit this, there is a unit test now failing. // ExpectedAuthorizeCodeSessionJSONFromFuzzing is used for round tripping tests. // It is exported to allow integration tests to use it. const ExpectedAuthorizeCodeSessionJSONFromFuzzing = `{ diff --git a/internal/psession/pinniped_session.go b/internal/psession/pinniped_session.go index a1f34a11b..d825ce037 100644 --- a/internal/psession/pinniped_session.go +++ b/internal/psession/pinniped_session.go @@ -144,7 +144,6 @@ func (s *ActiveDirectorySessionData) Clone() *ActiveDirectorySessionData { } } -// TODO: flesh this out, GitHub will need additional data. type GitHubSessionData struct { } diff --git a/internal/testutil/oidctestutil/testgithubprovider.go b/internal/testutil/oidctestutil/testgithubprovider.go index 5b985815b..83c1825cd 100644 --- a/internal/testutil/oidctestutil/testgithubprovider.go +++ b/internal/testutil/oidctestutil/testgithubprovider.go @@ -105,7 +105,6 @@ func NewTestUpstreamGitHubIdentityProviderBuilder() *TestUpstreamGitHubIdentityP return &TestUpstreamGitHubIdentityProviderBuilder{} } -// TODO: flesh this out. type TestUpstreamGitHubIdentityProvider struct { Name string ClientID string diff --git a/internal/testutil/testidplister/testidplister.go b/internal/testutil/testidplister/testidplister.go index 8195949e0..13b94b29f 100644 --- a/internal/testutil/testidplister/testidplister.go +++ b/internal/testutil/testidplister/testidplister.go @@ -243,8 +243,6 @@ func (b *UpstreamIDPListerBuilder) RequireExactlyOneCallToPasswordCredentialsGra actualArgs = upstreamOIDC.PasswordCredentialsGrantAndValidateTokensArgs(0) } } - // TODO: probably add GitHub loop once we flesh out the structs - require.Equal(t, 1, actualCallCountAcrossAllOIDCUpstreams, "should have been exactly one call to PasswordCredentialsGrantAndValidateTokens() by all OIDC upstreams", ) @@ -260,7 +258,6 @@ func (b *UpstreamIDPListerBuilder) RequireExactlyZeroCallsToPasswordCredentialsG for _, upstreamOIDC := range b.upstreamOIDCIdentityProviders { actualCallCountAcrossAllOIDCUpstreams += upstreamOIDC.PasswordCredentialsGrantAndValidateTokensCallCount() } - // TODO: probably add GitHub loop once we flesh out the structs require.Equal(t, 0, actualCallCountAcrossAllOIDCUpstreams, "expected exactly zero calls to PasswordCredentialsGrantAndValidateTokens()", ) @@ -283,7 +280,6 @@ func (b *UpstreamIDPListerBuilder) RequireExactlyOneCallToExchangeAuthcodeAndVal actualArgs = upstreamOIDC.ExchangeAuthcodeAndValidateTokensArgs(0) } } - // TODO: probably add GitHub loop once we flesh out the structs require.Equal(t, 1, actualCallCountAcrossAllOIDCUpstreams, "should have been exactly one call to ExchangeAuthcodeAndValidateTokens() by all OIDC upstreams", ) @@ -299,7 +295,7 @@ func (b *UpstreamIDPListerBuilder) RequireExactlyZeroCallsToExchangeAuthcodeAndV for _, upstreamOIDC := range b.upstreamOIDCIdentityProviders { actualCallCountAcrossAllOIDCUpstreams += upstreamOIDC.ExchangeAuthcodeAndValidateTokensCallCount() } - // TODO: probably add GitHub loop once we flesh out the structs + require.Equal(t, 0, actualCallCountAcrossAllOIDCUpstreams, "expected exactly zero calls to ExchangeAuthcodeAndValidateTokens()", ) @@ -384,7 +380,6 @@ func (b *UpstreamIDPListerBuilder) RequireExactlyOneCallToValidateToken( actualArgs = upstreamOIDC.ValidateTokenAndMergeWithUserInfoArgs(0) } } - // TODO: probably add GitHub loop once we flesh out the structs require.Equal(t, 1, actualCallCountAcrossAllOIDCUpstreams, "should have been exactly one call to ValidateTokenAndMergeWithUserInfo() by all OIDC upstreams", ) @@ -400,7 +395,6 @@ func (b *UpstreamIDPListerBuilder) RequireExactlyZeroCallsToValidateToken(t *tes for _, upstreamOIDC := range b.upstreamOIDCIdentityProviders { actualCallCountAcrossAllOIDCUpstreams += upstreamOIDC.ValidateTokenAndMergeWithUserInfoCallCount() } - // TODO: probably add GitHub loop once we flesh out structs require.Equal(t, 0, actualCallCountAcrossAllOIDCUpstreams, "expected exactly zero calls to ValidateTokenAndMergeWithUserInfo()", ) @@ -423,7 +417,6 @@ func (b *UpstreamIDPListerBuilder) RequireExactlyOneCallToRevokeToken( actualArgs = upstreamOIDC.RevokeTokenArgs(0) } } - // TODO: probably add GitHub loop once we flesh out structs require.Equal(t, 1, actualCallCountAcrossAllOIDCUpstreams, "should have been exactly one call to RevokeToken() by all OIDC upstreams", ) @@ -439,7 +432,6 @@ func (b *UpstreamIDPListerBuilder) RequireExactlyZeroCallsToRevokeToken(t *testi for _, upstreamOIDC := range b.upstreamOIDCIdentityProviders { actualCallCountAcrossAllOIDCUpstreams += upstreamOIDC.RevokeTokenCallCount() } - // TODO: probably add GitHub loop once we flesh out structs require.Equal(t, 0, actualCallCountAcrossAllOIDCUpstreams, "expected exactly zero calls to RevokeToken()", ) diff --git a/internal/upstreamgithub/upstreamgithub.go b/internal/upstreamgithub/upstreamgithub.go index 678ab0ae1..88fa1c79a 100644 --- a/internal/upstreamgithub/upstreamgithub.go +++ b/internal/upstreamgithub/upstreamgithub.go @@ -5,15 +5,12 @@ package upstreamgithub import ( - "context" "net/http" "golang.org/x/oauth2" - "k8s.io/apimachinery/pkg/types" "go.pinniped.dev/generated/latest/apis/supervisor/idp/v1alpha1" - "go.pinniped.dev/internal/authenticators" "go.pinniped.dev/internal/federationdomain/upstreamprovider" ) @@ -36,7 +33,6 @@ type Provider struct { } var _ upstreamprovider.UpstreamGithubIdentityProviderI = &Provider{} -var _ authenticators.UserAuthenticator = &Provider{} // New creates a Provider. The config is not a pointer to ensure that a copy of the config is created, // making the resulting Provider use an effectively read-only configuration. @@ -93,12 +89,3 @@ func (p *Provider) GetAuthorizationURL() string { func (p *Provider) GetHttpClient() *http.Client { return p.c.HttpClient } - -// AuthenticateUser authenticates an end user and returns their mapped username, groups, and UID. Implements authenticators.UserAuthenticator. -func (p *Provider) AuthenticateUser( - ctx context.Context, //nolint:all - username, password string, //nolint:all -) (*authenticators.Response, bool, error) { - // TODO: implement this, currently just placeholder to satisfy UserAuthenticator interface above - return nil, false, nil -} From 8a961bfa2133901e6db2d3c16d99bc332a6ff1c9 Mon Sep 17 00:00:00 2001 From: "Benjamin A. Petersen" Date: Thu, 25 Apr 2024 16:41:14 -0400 Subject: [PATCH 14/16] Add upstreamgithub unit tests --- .../resolved_github_provider_test.go | 49 +++++++++++++- .../upstreamgithub/upstreamgithub_test.go | 66 ++++++++++++++++++- 2 files changed, 113 insertions(+), 2 deletions(-) diff --git a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go index be58da09f..cc017cc08 100644 --- a/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go +++ b/internal/federationdomain/resolvedprovider/resolvedgithub/resolved_github_provider_test.go @@ -3,4 +3,51 @@ package resolvedgithub -// TODO: write some tests. +import ( + "context" + "testing" + + "github.com/stretchr/testify/require" + + "go.pinniped.dev/generated/latest/apis/supervisor/idpdiscovery/v1alpha1" + "go.pinniped.dev/internal/idtransform" + "go.pinniped.dev/internal/psession" + "go.pinniped.dev/internal/upstreamgithub" +) + +type fakeTransformer struct{} + +func (a fakeTransformer) Evaluate(_ context.Context, _ string, _ []string) (*idtransform.TransformationResult, error) { + return &idtransform.TransformationResult{}, nil +} +func (a fakeTransformer) Source() interface{} { return nil } + +func TestFederationDomainResolvedGitHubIdentityProvider(t *testing.T) { + fake := fakeTransformer{} + transforms := idtransform.NewTransformationPipeline() + transforms.AppendTransformation(fake) + subject := FederationDomainResolvedGitHubIdentityProvider{ + DisplayName: "fake-display-name", + Provider: upstreamgithub.New(upstreamgithub.ProviderConfig{ + Name: "fake-provider-config", + ResourceUID: "fake-resource-uid", + }), + SessionProviderType: psession.ProviderTypeGitHub, + Transforms: transforms, + } + + require.Equal(t, "fake-display-name", subject.GetDisplayName()) + require.Equal(t, upstreamgithub.New(upstreamgithub.ProviderConfig{ + Name: "fake-provider-config", + ResourceUID: "fake-resource-uid", + }), subject.GetProvider()) + require.Equal(t, psession.ProviderTypeGitHub, subject.GetSessionProviderType()) + require.Equal(t, v1alpha1.IDPTypeGitHub, subject.GetIDPDiscoveryType()) + require.Equal(t, []v1alpha1.IDPFlow{v1alpha1.IDPFlowBrowserAuthcode}, subject.GetIDPDiscoveryFlows()) + require.Equal(t, transforms, subject.GetTransforms()) + require.Equal(t, &psession.GitHubSessionData{}, subject.CloneIDPSpecificSessionDataFromSession(&psession.CustomSessionData{ + Username: "fake-username", + UpstreamUsername: "fake-upstream-username", + GitHub: &psession.GitHubSessionData{}, + })) +} diff --git a/internal/upstreamgithub/upstreamgithub_test.go b/internal/upstreamgithub/upstreamgithub_test.go index c5a1cda99..d0160ea0b 100644 --- a/internal/upstreamgithub/upstreamgithub_test.go +++ b/internal/upstreamgithub/upstreamgithub_test.go @@ -3,4 +3,68 @@ package upstreamgithub -// TODO: as we flesh out the Provider & ProviderConfig add tests +import ( + "net/http" + "testing" + + "github.com/stretchr/testify/require" + "golang.org/x/oauth2" + "k8s.io/apimachinery/pkg/types" + + "go.pinniped.dev/generated/latest/apis/supervisor/idp/v1alpha1" +) + +func TestGitHubProvider(t *testing.T) { + subject := New(ProviderConfig{ + Name: "foo", + ResourceUID: "resource-uid-12345", + Host: "fake-host", + UsernameAttribute: "fake-username-attribute", + GroupNameAttribute: "fake-group-name-attribute", + OAuth2Config: &oauth2.Config{ + ClientID: "fake-client-id", + ClientSecret: "fake-client-secret", + }, + AllowedOrganizations: []string{"fake-org", "fake-org2"}, + OrganizationLoginPolicy: v1alpha1.GitHubAllowedAuthOrganizationsPolicyAllGitHubUsers, + AuthorizationURL: "https://fake-authorization-url", + HttpClient: &http.Client{ + Timeout: 1234509, + }, + }) + + require.Equal(t, ProviderConfig{ + Name: "foo", + ResourceUID: "resource-uid-12345", + Host: "fake-host", + UsernameAttribute: "fake-username-attribute", + GroupNameAttribute: "fake-group-name-attribute", + OAuth2Config: &oauth2.Config{ + ClientID: "fake-client-id", + ClientSecret: "fake-client-secret", + }, + AllowedOrganizations: []string{"fake-org", "fake-org2"}, + OrganizationLoginPolicy: v1alpha1.GitHubAllowedAuthOrganizationsPolicyAllGitHubUsers, + AuthorizationURL: "https://fake-authorization-url", + HttpClient: &http.Client{ + Timeout: 1234509, + }, + }, subject.GetConfig()) + + require.Equal(t, "foo", subject.GetName()) + require.Equal(t, types.UID("resource-uid-12345"), subject.GetResourceUID()) + require.Equal(t, "fake-client-id", subject.GetClientID()) + require.Equal(t, &oauth2.Config{ + ClientID: "fake-client-id", + ClientSecret: "fake-client-secret", + }, subject.GetOAuth2Config()) + require.Equal(t, "fake-host", subject.GetHost()) + require.Equal(t, v1alpha1.GitHubUsernameAttribute("fake-username-attribute"), subject.GetUsernameAttribute()) + require.Equal(t, v1alpha1.GitHubGroupNameAttribute("fake-group-name-attribute"), subject.GetGroupNameAttribute()) + require.Equal(t, []string{"fake-org", "fake-org2"}, subject.GetAllowedOrganizations()) + require.Equal(t, v1alpha1.GitHubAllowedAuthOrganizationsPolicyAllGitHubUsers, subject.GetOrganizationLoginPolicy()) + require.Equal(t, "https://fake-authorization-url", subject.GetAuthorizationURL()) + require.Equal(t, &http.Client{ + Timeout: 1234509, + }, subject.GetHttpClient()) +} From 6424f45c194aad71af96479a59f0335c4977fb0e Mon Sep 17 00:00:00 2001 From: "Benjamin A. Petersen" Date: Mon, 29 Apr 2024 14:57:14 -0400 Subject: [PATCH 15/16] 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": ""}, From 9a21cb9cc798cc9d7ff010e3230c3c194bd2e679 Mon Sep 17 00:00:00 2001 From: "Benjamin A. Petersen" Date: Mon, 29 Apr 2024 15:21:35 -0400 Subject: [PATCH 16/16] update code generation --- .../idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go | 1 + .../idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go | 1 + .../idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go | 1 + .../idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go | 1 + .../idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go | 1 + .../idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go | 1 + .../idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go | 1 + .../idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go | 1 + .../idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go | 1 + .../idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go | 1 + 10 files changed, 10 insertions(+) diff --git a/generated/1.21/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go b/generated/1.21/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go index 303acdbfb..1b16bfe90 100644 --- a/generated/1.21/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go +++ b/generated/1.21/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go @@ -16,6 +16,7 @@ const ( IDPTypeLDAP IDPType = "ldap" IDPTypeActiveDirectory IDPType = "activedirectory" IDPTypeGitHub IDPType = "github" + IDPFlowCLIPassword IDPFlow = "cli_password" IDPFlowBrowserAuthcode IDPFlow = "browser_authcode" ) diff --git a/generated/1.22/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go b/generated/1.22/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go index 303acdbfb..1b16bfe90 100644 --- a/generated/1.22/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go +++ b/generated/1.22/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go @@ -16,6 +16,7 @@ const ( IDPTypeLDAP IDPType = "ldap" IDPTypeActiveDirectory IDPType = "activedirectory" IDPTypeGitHub IDPType = "github" + IDPFlowCLIPassword IDPFlow = "cli_password" IDPFlowBrowserAuthcode IDPFlow = "browser_authcode" ) diff --git a/generated/1.23/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go b/generated/1.23/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go index 303acdbfb..1b16bfe90 100644 --- a/generated/1.23/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go +++ b/generated/1.23/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go @@ -16,6 +16,7 @@ const ( IDPTypeLDAP IDPType = "ldap" IDPTypeActiveDirectory IDPType = "activedirectory" IDPTypeGitHub IDPType = "github" + IDPFlowCLIPassword IDPFlow = "cli_password" IDPFlowBrowserAuthcode IDPFlow = "browser_authcode" ) diff --git a/generated/1.24/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go b/generated/1.24/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go index 303acdbfb..1b16bfe90 100644 --- a/generated/1.24/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go +++ b/generated/1.24/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go @@ -16,6 +16,7 @@ const ( IDPTypeLDAP IDPType = "ldap" IDPTypeActiveDirectory IDPType = "activedirectory" IDPTypeGitHub IDPType = "github" + IDPFlowCLIPassword IDPFlow = "cli_password" IDPFlowBrowserAuthcode IDPFlow = "browser_authcode" ) diff --git a/generated/1.25/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go b/generated/1.25/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go index 303acdbfb..1b16bfe90 100644 --- a/generated/1.25/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go +++ b/generated/1.25/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go @@ -16,6 +16,7 @@ const ( IDPTypeLDAP IDPType = "ldap" IDPTypeActiveDirectory IDPType = "activedirectory" IDPTypeGitHub IDPType = "github" + IDPFlowCLIPassword IDPFlow = "cli_password" IDPFlowBrowserAuthcode IDPFlow = "browser_authcode" ) diff --git a/generated/1.26/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go b/generated/1.26/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go index 303acdbfb..1b16bfe90 100644 --- a/generated/1.26/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go +++ b/generated/1.26/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go @@ -16,6 +16,7 @@ const ( IDPTypeLDAP IDPType = "ldap" IDPTypeActiveDirectory IDPType = "activedirectory" IDPTypeGitHub IDPType = "github" + IDPFlowCLIPassword IDPFlow = "cli_password" IDPFlowBrowserAuthcode IDPFlow = "browser_authcode" ) diff --git a/generated/1.27/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go b/generated/1.27/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go index 303acdbfb..1b16bfe90 100644 --- a/generated/1.27/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go +++ b/generated/1.27/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go @@ -16,6 +16,7 @@ const ( IDPTypeLDAP IDPType = "ldap" IDPTypeActiveDirectory IDPType = "activedirectory" IDPTypeGitHub IDPType = "github" + IDPFlowCLIPassword IDPFlow = "cli_password" IDPFlowBrowserAuthcode IDPFlow = "browser_authcode" ) diff --git a/generated/1.28/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go b/generated/1.28/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go index 303acdbfb..1b16bfe90 100644 --- a/generated/1.28/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go +++ b/generated/1.28/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go @@ -16,6 +16,7 @@ const ( IDPTypeLDAP IDPType = "ldap" IDPTypeActiveDirectory IDPType = "activedirectory" IDPTypeGitHub IDPType = "github" + IDPFlowCLIPassword IDPFlow = "cli_password" IDPFlowBrowserAuthcode IDPFlow = "browser_authcode" ) diff --git a/generated/1.29/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go b/generated/1.29/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go index 303acdbfb..1b16bfe90 100644 --- a/generated/1.29/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go +++ b/generated/1.29/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go @@ -16,6 +16,7 @@ const ( IDPTypeLDAP IDPType = "ldap" IDPTypeActiveDirectory IDPType = "activedirectory" IDPTypeGitHub IDPType = "github" + IDPFlowCLIPassword IDPFlow = "cli_password" IDPFlowBrowserAuthcode IDPFlow = "browser_authcode" ) diff --git a/generated/latest/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go b/generated/latest/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go index 303acdbfb..1b16bfe90 100644 --- a/generated/latest/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go +++ b/generated/latest/apis/supervisor/idpdiscovery/v1alpha1/types_supervisor_idp_discovery.go @@ -16,6 +16,7 @@ const ( IDPTypeLDAP IDPType = "ldap" IDPTypeActiveDirectory IDPType = "activedirectory" IDPTypeGitHub IDPType = "github" + IDPFlowCLIPassword IDPFlow = "cli_password" IDPFlowBrowserAuthcode IDPFlow = "browser_authcode" )