From 3160b5bad1b41ab93ae6b2e46164a759d51ced2a Mon Sep 17 00:00:00 2001 From: "Benjamin A. Petersen" Date: Thu, 22 Jun 2023 16:12:50 -0400 Subject: [PATCH] Reorganized FederationDomain packages to avoid circular dependency Co-authored-by: Ryan Richard --- .../dynamiccertauthority.go | 4 +- .../dynamiccertauthority_test.go | 4 +- .../{issuer => clientcertissuer}/issuer.go | 2 +- .../issuer_test.go | 2 +- internal/concierge/apiserver/apiserver.go | 4 +- internal/concierge/server/server.go | 6 +- .../federation_domain_watcher.go | 20 ++--- .../federation_domain_watcher_test.go | 38 ++++---- internal/oidc/auth/auth_handler.go | 11 +-- internal/oidc/callback/callback_handler.go | 4 +- .../idpdiscovery/idp_discovery_handler.go | 6 +- internal/oidc/login/post_login_handler.go | 4 +- ...omain_identity_providers_lister_finder.go} | 88 +++++++------------ .../federation_domain_issuer.go | 6 +- .../federation_domain_issuer_test.go | 2 +- internal/oidc/provider/manager/manager.go | 8 +- .../oidc/provider/manager/manager_test.go | 14 +-- .../resolvedprovider/resolved_provider.go | 27 ++++++ internal/oidc/token/token_handler.go | 21 ++--- internal/oidc/token/token_handler_test.go | 4 +- internal/registry/credentialrequest/rest.go | 6 +- .../registry/credentialrequest/rest_test.go | 4 +- .../testutil/oidctestutil/oidctestutil.go | 29 +++--- 23 files changed, 162 insertions(+), 152 deletions(-) rename internal/{issuer => clientcertissuer}/issuer.go (98%) rename internal/{issuer => clientcertissuer}/issuer_test.go (99%) rename internal/oidc/provider/{federation_domain_identity_providers_lister.go => federationdomainproviders/federation_domain_identity_providers_lister_finder.go} (70%) rename internal/oidc/provider/{ => federationdomainproviders}/federation_domain_issuer.go (93%) rename internal/oidc/provider/{ => federationdomainproviders}/federation_domain_issuer_test.go (98%) create mode 100644 internal/oidc/provider/resolvedprovider/resolved_provider.go diff --git a/internal/certauthority/dynamiccertauthority/dynamiccertauthority.go b/internal/certauthority/dynamiccertauthority/dynamiccertauthority.go index 42dbf7d5..e75c7a25 100644 --- a/internal/certauthority/dynamiccertauthority/dynamiccertauthority.go +++ b/internal/certauthority/dynamiccertauthority/dynamiccertauthority.go @@ -11,7 +11,7 @@ import ( "k8s.io/apiserver/pkg/server/dynamiccertificates" "go.pinniped.dev/internal/certauthority" - "go.pinniped.dev/internal/issuer" + "go.pinniped.dev/internal/clientcertissuer" ) // ca is a type capable of issuing certificates. @@ -21,7 +21,7 @@ type ca struct { // New creates a ClientCertIssuer, ready to issue certs whenever // the given CertKeyContentProvider has a keypair to provide. -func New(provider dynamiccertificates.CertKeyContentProvider) issuer.ClientCertIssuer { +func New(provider dynamiccertificates.CertKeyContentProvider) clientcertissuer.ClientCertIssuer { return &ca{ provider: provider, } diff --git a/internal/certauthority/dynamiccertauthority/dynamiccertauthority_test.go b/internal/certauthority/dynamiccertauthority/dynamiccertauthority_test.go index b33cb9dd..cd07d173 100644 --- a/internal/certauthority/dynamiccertauthority/dynamiccertauthority_test.go +++ b/internal/certauthority/dynamiccertauthority/dynamiccertauthority_test.go @@ -9,8 +9,8 @@ import ( "github.com/stretchr/testify/require" + "go.pinniped.dev/internal/clientcertissuer" "go.pinniped.dev/internal/dynamiccert" - "go.pinniped.dev/internal/issuer" "go.pinniped.dev/internal/testutil" ) @@ -116,7 +116,7 @@ func TestCAIssuePEM(t *testing.T) { } } -func issuePEM(provider dynamiccert.Provider, ca issuer.ClientCertIssuer, caCrt, caKey []byte) ([]byte, []byte, error) { +func issuePEM(provider dynamiccert.Provider, ca clientcertissuer.ClientCertIssuer, caCrt, caKey []byte) ([]byte, []byte, error) { // if setting fails, look at that error if caCrt != nil || caKey != nil { if err := provider.SetCertKeyContent(caCrt, caKey); err != nil { diff --git a/internal/issuer/issuer.go b/internal/clientcertissuer/issuer.go similarity index 98% rename from internal/issuer/issuer.go rename to internal/clientcertissuer/issuer.go index 8023b7e7..1efbc3e7 100644 --- a/internal/issuer/issuer.go +++ b/internal/clientcertissuer/issuer.go @@ -1,7 +1,7 @@ // Copyright 2021-2023 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 -package issuer +package clientcertissuer import ( "fmt" diff --git a/internal/issuer/issuer_test.go b/internal/clientcertissuer/issuer_test.go similarity index 99% rename from internal/issuer/issuer_test.go rename to internal/clientcertissuer/issuer_test.go index 114e5f15..82583f20 100644 --- a/internal/issuer/issuer_test.go +++ b/internal/clientcertissuer/issuer_test.go @@ -1,7 +1,7 @@ // Copyright 2023 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 -package issuer +package clientcertissuer import ( "errors" diff --git a/internal/concierge/apiserver/apiserver.go b/internal/concierge/apiserver/apiserver.go index 5fa743b0..e1e7d074 100644 --- a/internal/concierge/apiserver/apiserver.go +++ b/internal/concierge/apiserver/apiserver.go @@ -15,8 +15,8 @@ import ( "k8s.io/apiserver/pkg/registry/rest" genericapiserver "k8s.io/apiserver/pkg/server" + "go.pinniped.dev/internal/clientcertissuer" "go.pinniped.dev/internal/controllerinit" - "go.pinniped.dev/internal/issuer" "go.pinniped.dev/internal/plog" "go.pinniped.dev/internal/pversion" "go.pinniped.dev/internal/registry/credentialrequest" @@ -30,7 +30,7 @@ type Config struct { type ExtraConfig struct { Authenticator credentialrequest.TokenCredentialRequestAuthenticator - Issuer issuer.ClientCertIssuer + Issuer clientcertissuer.ClientCertIssuer BuildControllersPostStartHook controllerinit.RunnerBuilder Scheme *runtime.Scheme NegotiatedSerializer runtime.NegotiatedSerializer diff --git a/internal/concierge/server/server.go b/internal/concierge/server/server.go index ee446c42..95fa26c5 100644 --- a/internal/concierge/server/server.go +++ b/internal/concierge/server/server.go @@ -23,6 +23,7 @@ import ( conciergeopenapi "go.pinniped.dev/generated/latest/client/concierge/openapi" "go.pinniped.dev/internal/certauthority/dynamiccertauthority" + "go.pinniped.dev/internal/clientcertissuer" "go.pinniped.dev/internal/concierge/apiserver" conciergescheme "go.pinniped.dev/internal/concierge/scheme" "go.pinniped.dev/internal/config/concierge" @@ -33,7 +34,6 @@ import ( "go.pinniped.dev/internal/downward" "go.pinniped.dev/internal/dynamiccert" "go.pinniped.dev/internal/here" - "go.pinniped.dev/internal/issuer" "go.pinniped.dev/internal/kubeclient" "go.pinniped.dev/internal/plog" "go.pinniped.dev/internal/pversion" @@ -159,7 +159,7 @@ func (a *App) runServer(ctx context.Context) error { return fmt.Errorf("could not prepare controllers: %w", err) } - certIssuer := issuer.ClientCertIssuers{ + certIssuer := clientcertissuer.ClientCertIssuers{ dynamiccertauthority.New(dynamicSigningCertProvider), // attempt to use the real Kube CA if possible dynamiccertauthority.New(impersonationProxySigningCertProvider), // fallback to our internal CA if we need to } @@ -194,7 +194,7 @@ func (a *App) runServer(ctx context.Context) error { func getAggregatedAPIServerConfig( dynamicCertProvider dynamiccert.Private, authenticator credentialrequest.TokenCredentialRequestAuthenticator, - issuer issuer.ClientCertIssuer, + issuer clientcertissuer.ClientCertIssuer, buildControllers controllerinit.RunnerBuilder, apiGroupSuffix string, aggregatedAPIServerPort int64, diff --git a/internal/controller/supervisorconfig/federation_domain_watcher.go b/internal/controller/supervisorconfig/federation_domain_watcher.go index 0663469f..0d68339e 100644 --- a/internal/controller/supervisorconfig/federation_domain_watcher.go +++ b/internal/controller/supervisorconfig/federation_domain_watcher.go @@ -26,7 +26,7 @@ import ( pinnipedcontroller "go.pinniped.dev/internal/controller" "go.pinniped.dev/internal/controllerlib" "go.pinniped.dev/internal/idtransform" - "go.pinniped.dev/internal/oidc/provider" + "go.pinniped.dev/internal/oidc/provider/federationdomainproviders" "go.pinniped.dev/internal/plog" ) @@ -34,7 +34,7 @@ import ( // If there are no longer any valid issuers, then it can be called with no arguments. // Implementations of this type should be thread-safe to support calls from multiple goroutines. type FederationDomainsSetter interface { - SetFederationDomains(federationDomains ...*provider.FederationDomainIssuer) + SetFederationDomains(federationDomains ...*federationdomainproviders.FederationDomainIssuer) } type federationDomainWatcherController struct { @@ -145,7 +145,7 @@ func (c *federationDomainWatcherController) Sync(ctx controllerlib.Context) erro var errs []error - federationDomainIssuers := make([]*provider.FederationDomainIssuer, 0) + federationDomainIssuers := make([]*federationdomainproviders.FederationDomainIssuer, 0) for _, federationDomain := range federationDomains { issuerURL, urlParseErr := url.Parse(federationDomain.Spec.Issuer) @@ -187,8 +187,8 @@ func (c *federationDomainWatcherController) Sync(ctx controllerlib.Context) erro // Create the list of IDPs for this FederationDomain. // Don't worry if the IDP CRs themselves is phase=Ready because those which are not ready will not be loaded // into the provider cache, so they cannot actually be used to authenticate. - federationDomainIdentityProviders := []*provider.FederationDomainIdentityProvider{} - var defaultFederationDomainIdentityProvider *provider.FederationDomainIdentityProvider + federationDomainIdentityProviders := []*federationdomainproviders.FederationDomainIdentityProvider{} + var defaultFederationDomainIdentityProvider *federationdomainproviders.FederationDomainIdentityProvider if len(federationDomain.Spec.IdentityProviders) == 0 { // When the FederationDomain does not list any IDPs, then we might be in backwards compatibility mode. oidcIdentityProviders, _ := c.oidcIdentityProviderInformer.Lister().List(labels.Everything()) @@ -200,7 +200,7 @@ func (c *federationDomainWatcherController) Sync(ctx controllerlib.Context) erro idpCRsCount := len(oidcIdentityProviders) + len(ldapIdentityProviders) + len(activeDirectoryIdentityProviders) if idpCRsCount == 1 { // If so, default that IDP's DisplayName to be the same as its resource Name. - defaultFederationDomainIdentityProvider = &provider.FederationDomainIdentityProvider{} + defaultFederationDomainIdentityProvider = &federationdomainproviders.FederationDomainIdentityProvider{} switch { case len(oidcIdentityProviders) == 1: defaultFederationDomainIdentityProvider.DisplayName = oidcIdentityProviders[0].Name @@ -388,7 +388,7 @@ func (c *federationDomainWatcherController) Sync(ctx controllerlib.Context) erro } } // For each valid IDP (unique displayName, valid objectRef + valid transforms), add it to the list. - federationDomainIdentityProviders = append(federationDomainIdentityProviders, &provider.FederationDomainIdentityProvider{ + federationDomainIdentityProviders = append(federationDomainIdentityProviders, &federationdomainproviders.FederationDomainIdentityProvider{ DisplayName: idp.DisplayName, UID: idpResourceUID, Transforms: pipeline, @@ -401,14 +401,14 @@ func (c *federationDomainWatcherController) Sync(ctx controllerlib.Context) erro } // Now that we have the list of IDPs for this FederationDomain, create the issuer. - var federationDomainIssuer *provider.FederationDomainIssuer + var federationDomainIssuer *federationdomainproviders.FederationDomainIssuer err = nil if defaultFederationDomainIdentityProvider != nil { // This is the constructor for the backwards compatibility mode. - federationDomainIssuer, err = provider.NewFederationDomainIssuerWithDefaultIDP(federationDomain.Spec.Issuer, defaultFederationDomainIdentityProvider) + federationDomainIssuer, err = federationdomainproviders.NewFederationDomainIssuerWithDefaultIDP(federationDomain.Spec.Issuer, defaultFederationDomainIdentityProvider) } else { // This is the constructor for any other case, including when there is an empty list of IDPs. - federationDomainIssuer, err = provider.NewFederationDomainIssuer(federationDomain.Spec.Issuer, federationDomainIdentityProviders) + federationDomainIssuer, err = federationdomainproviders.NewFederationDomainIssuer(federationDomain.Spec.Issuer, federationDomainIdentityProviders) } if err != nil { // Note that the FederationDomainIssuer constructors validate the Issuer URL. diff --git a/internal/controller/supervisorconfig/federation_domain_watcher_test.go b/internal/controller/supervisorconfig/federation_domain_watcher_test.go index 66eb6e00..67b89394 100644 --- a/internal/controller/supervisorconfig/federation_domain_watcher_test.go +++ b/internal/controller/supervisorconfig/federation_domain_watcher_test.go @@ -28,7 +28,7 @@ import ( pinnipedinformers "go.pinniped.dev/generated/latest/client/supervisor/informers/externalversions" "go.pinniped.dev/internal/controllerlib" "go.pinniped.dev/internal/here" - "go.pinniped.dev/internal/oidc/provider" + "go.pinniped.dev/internal/oidc/provider/federationdomainproviders" "go.pinniped.dev/internal/testutil" ) @@ -91,10 +91,10 @@ func TestInformerFilters(t *testing.T) { type fakeFederationDomainsSetter struct { SetFederationDomainsWasCalled bool - FederationDomainsReceived []*provider.FederationDomainIssuer + FederationDomainsReceived []*federationdomainproviders.FederationDomainIssuer } -func (f *fakeFederationDomainsSetter) SetFederationDomains(federationDomains ...*provider.FederationDomainIssuer) { +func (f *fakeFederationDomainsSetter) SetFederationDomains(federationDomains ...*federationdomainproviders.FederationDomainIssuer) { f.SetFederationDomainsWasCalled = true f.FederationDomainsReceived = federationDomains } @@ -196,15 +196,15 @@ func TestSync(t *testing.T) { err := controllerlib.TestSync(t, subject, *syncContext) r.NoError(err) - fd1, err := provider.NewFederationDomainIssuer(federationDomain1.Spec.Issuer, []*provider.FederationDomainIdentityProvider{}) + fd1, err := federationdomainproviders.NewFederationDomainIssuer(federationDomain1.Spec.Issuer, []*federationdomainproviders.FederationDomainIdentityProvider{}) r.NoError(err) - fd2, err := provider.NewFederationDomainIssuer(federationDomain2.Spec.Issuer, []*provider.FederationDomainIdentityProvider{}) + fd2, err := federationdomainproviders.NewFederationDomainIssuer(federationDomain2.Spec.Issuer, []*federationdomainproviders.FederationDomainIdentityProvider{}) r.NoError(err) r.True(federationDomainsSetter.SetFederationDomainsWasCalled) r.ElementsMatch( - []*provider.FederationDomainIssuer{ + []*federationdomainproviders.FederationDomainIssuer{ fd1, fd2, }, @@ -297,15 +297,15 @@ func TestSync(t *testing.T) { err := controllerlib.TestSync(t, subject, *syncContext) r.NoError(err) - fd1, err := provider.NewFederationDomainIssuer(federationDomain1.Spec.Issuer, []*provider.FederationDomainIdentityProvider{}) + fd1, err := federationdomainproviders.NewFederationDomainIssuer(federationDomain1.Spec.Issuer, []*federationdomainproviders.FederationDomainIdentityProvider{}) r.NoError(err) - fd2, err := provider.NewFederationDomainIssuer(federationDomain2.Spec.Issuer, []*provider.FederationDomainIdentityProvider{}) + fd2, err := federationdomainproviders.NewFederationDomainIssuer(federationDomain2.Spec.Issuer, []*federationdomainproviders.FederationDomainIdentityProvider{}) r.NoError(err) r.True(federationDomainsSetter.SetFederationDomainsWasCalled) r.ElementsMatch( - []*provider.FederationDomainIssuer{ + []*federationdomainproviders.FederationDomainIssuer{ fd1, fd2, }, @@ -335,10 +335,10 @@ func TestSync(t *testing.T) { err := controllerlib.TestSync(t, subject, *syncContext) r.EqualError(err, "could not update status: some update error") - fd1, err := provider.NewFederationDomainIssuer(federationDomain1.Spec.Issuer, []*provider.FederationDomainIdentityProvider{}) + fd1, err := federationdomainproviders.NewFederationDomainIssuer(federationDomain1.Spec.Issuer, []*federationdomainproviders.FederationDomainIdentityProvider{}) r.NoError(err) - fd2, err := provider.NewFederationDomainIssuer(federationDomain2.Spec.Issuer, []*provider.FederationDomainIdentityProvider{}) + fd2, err := federationdomainproviders.NewFederationDomainIssuer(federationDomain2.Spec.Issuer, []*federationdomainproviders.FederationDomainIdentityProvider{}) r.NoError(err) r.True(federationDomainsSetter.SetFederationDomainsWasCalled) @@ -554,12 +554,12 @@ func TestSync(t *testing.T) { err := controllerlib.TestSync(t, subject, *syncContext) r.NoError(err) - validFederationDomain, err := provider.NewFederationDomainIssuer(validFederationDomain.Spec.Issuer, []*provider.FederationDomainIdentityProvider{}) + validFederationDomain, err := federationdomainproviders.NewFederationDomainIssuer(validFederationDomain.Spec.Issuer, []*federationdomainproviders.FederationDomainIdentityProvider{}) r.NoError(err) r.True(federationDomainsSetter.SetFederationDomainsWasCalled) r.Equal( - []*provider.FederationDomainIssuer{ + []*federationdomainproviders.FederationDomainIssuer{ validFederationDomain, }, federationDomainsSetter.FederationDomainsReceived, @@ -628,12 +628,12 @@ func TestSync(t *testing.T) { err := controllerlib.TestSync(t, subject, *syncContext) r.EqualError(err, "could not update status: some update error") - validFederationDomain, err := provider.NewFederationDomainIssuer(validFederationDomain.Spec.Issuer, []*provider.FederationDomainIdentityProvider{}) + validFederationDomain, err := federationdomainproviders.NewFederationDomainIssuer(validFederationDomain.Spec.Issuer, []*federationdomainproviders.FederationDomainIdentityProvider{}) r.NoError(err) r.True(federationDomainsSetter.SetFederationDomainsWasCalled) r.Equal( - []*provider.FederationDomainIssuer{ + []*federationdomainproviders.FederationDomainIssuer{ validFederationDomain, }, federationDomainsSetter.FederationDomainsReceived, @@ -718,12 +718,12 @@ func TestSync(t *testing.T) { err := controllerlib.TestSync(t, subject, *syncContext) r.NoError(err) - nonDuplicateFederationDomain, err := provider.NewFederationDomainIssuer(federationDomain.Spec.Issuer, []*provider.FederationDomainIdentityProvider{}) + nonDuplicateFederationDomain, err := federationdomainproviders.NewFederationDomainIssuer(federationDomain.Spec.Issuer, []*federationdomainproviders.FederationDomainIdentityProvider{}) r.NoError(err) r.True(federationDomainsSetter.SetFederationDomainsWasCalled) r.Equal( - []*provider.FederationDomainIssuer{ + []*federationdomainproviders.FederationDomainIssuer{ nonDuplicateFederationDomain, }, federationDomainsSetter.FederationDomainsReceived, @@ -891,12 +891,12 @@ func TestSync(t *testing.T) { err := controllerlib.TestSync(t, subject, *syncContext) r.NoError(err) - nonDuplicateFederationDomain, err := provider.NewFederationDomainIssuer(federationDomainDifferentIssuerAddress.Spec.Issuer, []*provider.FederationDomainIdentityProvider{}) + nonDuplicateFederationDomain, err := federationdomainproviders.NewFederationDomainIssuer(federationDomainDifferentIssuerAddress.Spec.Issuer, []*federationdomainproviders.FederationDomainIdentityProvider{}) r.NoError(err) r.True(federationDomainsSetter.SetFederationDomainsWasCalled) r.Equal( - []*provider.FederationDomainIssuer{ + []*federationdomainproviders.FederationDomainIssuer{ nonDuplicateFederationDomain, }, federationDomainsSetter.FederationDomainsReceived, diff --git a/internal/oidc/auth/auth_handler.go b/internal/oidc/auth/auth_handler.go index 6bb7ea09..3ea3e7ca 100644 --- a/internal/oidc/auth/auth_handler.go +++ b/internal/oidc/auth/auth_handler.go @@ -23,8 +23,9 @@ import ( "go.pinniped.dev/internal/oidc/csrftoken" "go.pinniped.dev/internal/oidc/downstreamsession" "go.pinniped.dev/internal/oidc/login" - "go.pinniped.dev/internal/oidc/provider" + "go.pinniped.dev/internal/oidc/provider/federationdomainproviders" "go.pinniped.dev/internal/oidc/provider/formposthtml" + "go.pinniped.dev/internal/oidc/provider/resolvedprovider" "go.pinniped.dev/internal/oidc/provider/upstreamprovider" "go.pinniped.dev/internal/plog" "go.pinniped.dev/internal/psession" @@ -39,7 +40,7 @@ const ( func NewHandler( downstreamIssuer string, - idpFinder provider.FederationDomainIdentityProvidersFinderI, + idpFinder federationdomainproviders.FederationDomainIdentityProvidersFinderI, oauthHelperWithoutStorage fosite.OAuth2Provider, oauthHelperWithStorage fosite.OAuth2Provider, generateCSRF func() (csrftoken.CSRFToken, error), @@ -213,7 +214,7 @@ func handleAuthRequestForLDAPUpstreamBrowserFlow( generateCSRF func() (csrftoken.CSRFToken, error), generateNonce func() (nonce.Nonce, error), generatePKCE func() (pkce.Code, error), - ldapUpstream *provider.FederationDomainResolvedLDAPIdentityProvider, + ldapUpstream *resolvedprovider.FederationDomainResolvedLDAPIdentityProvider, idpType psession.ProviderType, downstreamIssuer string, upstreamStateEncoder oidc.Encoder, @@ -332,7 +333,7 @@ func handleAuthRequestForOIDCUpstreamBrowserFlow( generateCSRF func() (csrftoken.CSRFToken, error), generateNonce func() (nonce.Nonce, error), generatePKCE func() (pkce.Code, error), - oidcUpstream *provider.FederationDomainResolvedOIDCIdentityProvider, + oidcUpstream *resolvedprovider.FederationDomainResolvedOIDCIdentityProvider, downstreamIssuer string, upstreamStateEncoder oidc.Encoder, cookieCodec oidc.Codec, @@ -448,7 +449,7 @@ func readCSRFCookie(r *http.Request, codec oidc.Decoder) csrftoken.CSRFToken { // chooseUpstreamIDP selects either an OIDC, an LDAP, or an AD IDP, or returns an error. // Note that AD and LDAP IDPs both return the same interface type, but different ProviderTypes values. -func chooseUpstreamIDP(idpDisplayName string, idpLister provider.FederationDomainIdentityProvidersFinderI) (*provider.FederationDomainResolvedOIDCIdentityProvider, *provider.FederationDomainResolvedLDAPIdentityProvider, error) { +func chooseUpstreamIDP(idpDisplayName string, idpLister federationdomainproviders.FederationDomainIdentityProvidersFinderI) (*resolvedprovider.FederationDomainResolvedOIDCIdentityProvider, *resolvedprovider.FederationDomainResolvedLDAPIdentityProvider, error) { // When a request is made to the authorization endpoint which does not specify the IDP name, then it might // be an old dynamic client (OIDCClient). We need to make this work, but only in the backwards compatibility case // where there is exactly one IDP defined in the namespace and no IDPs listed on the FederationDomain. diff --git a/internal/oidc/callback/callback_handler.go b/internal/oidc/callback/callback_handler.go index e260d201..5613108c 100644 --- a/internal/oidc/callback/callback_handler.go +++ b/internal/oidc/callback/callback_handler.go @@ -14,13 +14,13 @@ import ( "go.pinniped.dev/internal/httputil/securityheader" "go.pinniped.dev/internal/oidc" "go.pinniped.dev/internal/oidc/downstreamsession" - "go.pinniped.dev/internal/oidc/provider" + "go.pinniped.dev/internal/oidc/provider/federationdomainproviders" "go.pinniped.dev/internal/oidc/provider/formposthtml" "go.pinniped.dev/internal/plog" ) func NewHandler( - upstreamIDPs provider.FederationDomainIdentityProvidersFinderI, + upstreamIDPs federationdomainproviders.FederationDomainIdentityProvidersFinderI, oauthHelper fosite.OAuth2Provider, stateDecoder, cookieDecoder oidc.Decoder, redirectURI string, diff --git a/internal/oidc/idpdiscovery/idp_discovery_handler.go b/internal/oidc/idpdiscovery/idp_discovery_handler.go index 0b93b2b7..d9034828 100644 --- a/internal/oidc/idpdiscovery/idp_discovery_handler.go +++ b/internal/oidc/idpdiscovery/idp_discovery_handler.go @@ -11,11 +11,11 @@ import ( "sort" "go.pinniped.dev/generated/latest/apis/supervisor/idpdiscovery/v1alpha1" - "go.pinniped.dev/internal/oidc/provider" + "go.pinniped.dev/internal/oidc/provider/federationdomainproviders" ) // NewHandler returns an http.Handler that serves the upstream IDP discovery endpoint. -func NewHandler(upstreamIDPs provider.FederationDomainIdentityProvidersListerI) http.Handler { +func NewHandler(upstreamIDPs federationdomainproviders.FederationDomainIdentityProvidersListerI) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.Method != http.MethodGet { http.Error(w, `Method not allowed (try GET)`, http.StatusMethodNotAllowed) @@ -36,7 +36,7 @@ func NewHandler(upstreamIDPs provider.FederationDomainIdentityProvidersListerI) }) } -func responseAsJSON(upstreamIDPs provider.FederationDomainIdentityProvidersListerI) ([]byte, error) { +func responseAsJSON(upstreamIDPs federationdomainproviders.FederationDomainIdentityProvidersListerI) ([]byte, error) { r := v1alpha1.IDPDiscoveryResponse{PinnipedIDPs: []v1alpha1.PinnipedIDP{}} // The cache of IDPs could change at any time, so always recalculate the list. diff --git a/internal/oidc/login/post_login_handler.go b/internal/oidc/login/post_login_handler.go index 8c9a5970..a9b0c7fc 100644 --- a/internal/oidc/login/post_login_handler.go +++ b/internal/oidc/login/post_login_handler.go @@ -12,11 +12,11 @@ import ( "go.pinniped.dev/internal/httputil/httperr" "go.pinniped.dev/internal/oidc" "go.pinniped.dev/internal/oidc/downstreamsession" - "go.pinniped.dev/internal/oidc/provider" + "go.pinniped.dev/internal/oidc/provider/federationdomainproviders" "go.pinniped.dev/internal/plog" ) -func NewPostHandler(issuerURL string, upstreamIDPs provider.FederationDomainIdentityProvidersFinderI, oauthHelper fosite.OAuth2Provider) HandlerFunc { +func NewPostHandler(issuerURL string, upstreamIDPs federationdomainproviders.FederationDomainIdentityProvidersFinderI, oauthHelper fosite.OAuth2Provider) HandlerFunc { return func(w http.ResponseWriter, r *http.Request, encodedState string, decodedState *oidc.UpstreamStateParamData) error { // Note that the login handler prevents this handler from being called with OIDC upstreams. _, ldapUpstream, err := upstreamIDPs.FindUpstreamIDPByDisplayName(decodedState.UpstreamName) diff --git a/internal/oidc/provider/federation_domain_identity_providers_lister.go b/internal/oidc/provider/federationdomainproviders/federation_domain_identity_providers_lister_finder.go similarity index 70% rename from internal/oidc/provider/federation_domain_identity_providers_lister.go rename to internal/oidc/provider/federationdomainproviders/federation_domain_identity_providers_lister_finder.go index 42d47ed3..27d76a1d 100644 --- a/internal/oidc/provider/federation_domain_identity_providers_lister.go +++ b/internal/oidc/provider/federationdomainproviders/federation_domain_identity_providers_lister_finder.go @@ -1,7 +1,7 @@ // Copyright 2020-2023 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 -package provider +package federationdomainproviders import ( "fmt" @@ -11,7 +11,7 @@ import ( "go.pinniped.dev/internal/idtransform" "go.pinniped.dev/internal/oidc/idplister" - "go.pinniped.dev/internal/oidc/provider/upstreamprovider" + "go.pinniped.dev/internal/oidc/provider/resolvedprovider" "go.pinniped.dev/internal/psession" ) @@ -24,44 +24,24 @@ type FederationDomainIdentityProvider struct { Transforms *idtransform.TransformationPipeline } -// FederationDomainResolvedOIDCIdentityProvider represents a FederationDomainIdentityProvider which has -// been resolved dynamically based on the currently loaded IDP CRs to include the provider.UpstreamOIDCIdentityProviderI -// and other metadata about the provider. -type FederationDomainResolvedOIDCIdentityProvider struct { - DisplayName string - Provider upstreamprovider.UpstreamOIDCIdentityProviderI - SessionProviderType psession.ProviderType - Transforms *idtransform.TransformationPipeline -} - -// FederationDomainResolvedLDAPIdentityProvider represents a FederationDomainIdentityProvider which has -// been resolved dynamically based on the currently loaded IDP CRs to include the provider.UpstreamLDAPIdentityProviderI -// and other metadata about the provider. -type FederationDomainResolvedLDAPIdentityProvider struct { - DisplayName string - Provider upstreamprovider.UpstreamLDAPIdentityProviderI - SessionProviderType psession.ProviderType - Transforms *idtransform.TransformationPipeline -} - type FederationDomainIdentityProvidersFinderI interface { FindDefaultIDP() ( - *FederationDomainResolvedOIDCIdentityProvider, - *FederationDomainResolvedLDAPIdentityProvider, + *resolvedprovider.FederationDomainResolvedOIDCIdentityProvider, + *resolvedprovider.FederationDomainResolvedLDAPIdentityProvider, error, ) FindUpstreamIDPByDisplayName(upstreamIDPDisplayName string) ( - *FederationDomainResolvedOIDCIdentityProvider, - *FederationDomainResolvedLDAPIdentityProvider, + *resolvedprovider.FederationDomainResolvedOIDCIdentityProvider, + *resolvedprovider.FederationDomainResolvedLDAPIdentityProvider, error, ) } type FederationDomainIdentityProvidersListerI interface { - GetOIDCIdentityProviders() []*FederationDomainResolvedOIDCIdentityProvider - GetLDAPIdentityProviders() []*FederationDomainResolvedLDAPIdentityProvider - GetActiveDirectoryIdentityProviders() []*FederationDomainResolvedLDAPIdentityProvider + GetOIDCIdentityProviders() []*resolvedprovider.FederationDomainResolvedOIDCIdentityProvider + GetLDAPIdentityProviders() []*resolvedprovider.FederationDomainResolvedLDAPIdentityProvider + GetActiveDirectoryIdentityProviders() []*resolvedprovider.FederationDomainResolvedLDAPIdentityProvider } type FederationDomainIdentityProvidersListerFinderI interface { @@ -69,16 +49,16 @@ type FederationDomainIdentityProvidersListerFinderI interface { FederationDomainIdentityProvidersFinderI } -// FederationDomainIdentityProvidersLister implements FederationDomainIdentityProvidersListerFinderI. -var _ FederationDomainIdentityProvidersListerFinderI = (*FederationDomainIdentityProvidersLister)(nil) +// FederationDomainIdentityProvidersListerFinder implements FederationDomainIdentityProvidersListerFinderI. +var _ FederationDomainIdentityProvidersListerFinderI = (*FederationDomainIdentityProvidersListerFinder)(nil) -// FederationDomainIdentityProvidersLister wraps an UpstreamIdentityProvidersLister. The lister which is being +// FederationDomainIdentityProvidersListerFinder wraps an UpstreamIdentityProvidersLister. The lister which is being // wrapped should contain all valid upstream providers that are currently defined in the Supervisor. -// FederationDomainIdentityProvidersLister provides a lookup method which only looks up IDPs within those which +// FederationDomainIdentityProvidersListerFinder provides a lookup method which only looks up IDPs within those which // have allowed resource IDs, and also uses display names (name aliases) instead of the actual resource names to do the // lookups. It also provides list methods which only list the allowed identity providers (to be used by the IDP // discovery endpoint, for example). -type FederationDomainIdentityProvidersLister struct { +type FederationDomainIdentityProvidersListerFinder struct { wrappedLister idplister.UpstreamIdentityProvidersLister configuredIdentityProviders []*FederationDomainIdentityProvider defaultIdentityProvider *FederationDomainIdentityProvider @@ -86,17 +66,17 @@ type FederationDomainIdentityProvidersLister struct { allowedIDPResourceUIDs sets.Set[types.UID] } -// NewFederationDomainUpstreamIdentityProvidersLister returns a new FederationDomainIdentityProvidersLister +// NewFederationDomainIdentityProvidersListerFinder returns a new FederationDomainIdentityProvidersListerFinder // which only lists those IDPs allowed by its parameter. Every FederationDomainIdentityProvider in the // federationDomainIssuer parameter's IdentityProviders() list must have a unique DisplayName. // Note that a single underlying IDP UID may be used by multiple FederationDomainIdentityProvider in the parameter. // The wrapped lister should contain all valid upstream providers that are defined in the Supervisor, and is expected to -// be thread-safe and to change its contents over time. The FederationDomainIdentityProvidersLister will filter out the +// be thread-safe and to change its contents over time. The FederationDomainIdentityProvidersListerFinder will filter out the // ones that don't apply to this federation domain. -func NewFederationDomainUpstreamIdentityProvidersLister( +func NewFederationDomainIdentityProvidersListerFinder( federationDomainIssuer *FederationDomainIssuer, wrappedLister idplister.UpstreamIdentityProvidersLister, -) *FederationDomainIdentityProvidersLister { +) *FederationDomainIdentityProvidersListerFinder { // Create a copy of the input slice so we won't need to worry about the caller accidentally changing it. copyOfFederationDomainIdentityProviders := []*FederationDomainIdentityProvider{} // Create a map and a set for quick lookups of the same data that was passed in via the @@ -110,7 +90,7 @@ func NewFederationDomainUpstreamIdentityProvidersLister( copyOfFederationDomainIdentityProviders = append(copyOfFederationDomainIdentityProviders, &shallowCopyOfIDP) } - return &FederationDomainIdentityProvidersLister{ + return &FederationDomainIdentityProvidersListerFinder{ wrappedLister: wrappedLister, configuredIdentityProviders: copyOfFederationDomainIdentityProviders, defaultIdentityProvider: federationDomainIssuer.DefaultIdentityProvider(), @@ -122,9 +102,9 @@ func NewFederationDomainUpstreamIdentityProvidersLister( // FindUpstreamIDPByDisplayName selects either an OIDC, LDAP, or ActiveDirectory IDP, or returns an error. // It only considers the allowed IDPs while doing the lookup by display name. // Note that ActiveDirectory and LDAP IDPs both return the same type, but with different SessionProviderType values. -func (u *FederationDomainIdentityProvidersLister) FindUpstreamIDPByDisplayName(upstreamIDPDisplayName string) ( - *FederationDomainResolvedOIDCIdentityProvider, - *FederationDomainResolvedLDAPIdentityProvider, +func (u *FederationDomainIdentityProvidersListerFinder) FindUpstreamIDPByDisplayName(upstreamIDPDisplayName string) ( + *resolvedprovider.FederationDomainResolvedOIDCIdentityProvider, + *resolvedprovider.FederationDomainResolvedLDAPIdentityProvider, error, ) { // Given a display name, look up the identity provider's UID for that display name. @@ -156,9 +136,9 @@ func (u *FederationDomainIdentityProvidersLister) FindUpstreamIDPByDisplayName(u // This can be used to handle the backwards compatibility mode where an authorization request could be made // without specifying an IDP name, and there are no IDPs explicitly specified on the FederationDomain, and there // is exactly one IDP CR defined in the Supervisor namespace. -func (u *FederationDomainIdentityProvidersLister) FindDefaultIDP() ( - *FederationDomainResolvedOIDCIdentityProvider, - *FederationDomainResolvedLDAPIdentityProvider, +func (u *FederationDomainIdentityProvidersListerFinder) FindDefaultIDP() ( + *resolvedprovider.FederationDomainResolvedOIDCIdentityProvider, + *resolvedprovider.FederationDomainResolvedLDAPIdentityProvider, error, ) { if u.defaultIdentityProvider == nil { @@ -168,10 +148,10 @@ func (u *FederationDomainIdentityProvidersLister) FindDefaultIDP() ( } // GetOIDCIdentityProviders lists only the OIDC providers for this FederationDomain. -func (u *FederationDomainIdentityProvidersLister) GetOIDCIdentityProviders() []*FederationDomainResolvedOIDCIdentityProvider { +func (u *FederationDomainIdentityProvidersListerFinder) GetOIDCIdentityProviders() []*resolvedprovider.FederationDomainResolvedOIDCIdentityProvider { // Get the cached providers once at the start in case they change during the rest of this function. cachedProviders := u.wrappedLister.GetOIDCIdentityProviders() - providers := []*FederationDomainResolvedOIDCIdentityProvider{} + providers := []*resolvedprovider.FederationDomainResolvedOIDCIdentityProvider{} // 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... for _, idp := range u.configuredIdentityProviders { @@ -179,7 +159,7 @@ func (u *FederationDomainIdentityProvidersLister) GetOIDCIdentityProviders() []* for _, p := range cachedProviders { if idp.UID == p.GetResourceUID() { // Found it, so append it to the result. - providers = append(providers, &FederationDomainResolvedOIDCIdentityProvider{ + providers = append(providers, &resolvedprovider.FederationDomainResolvedOIDCIdentityProvider{ DisplayName: idp.DisplayName, Provider: p, SessionProviderType: psession.ProviderTypeOIDC, @@ -192,10 +172,10 @@ func (u *FederationDomainIdentityProvidersLister) GetOIDCIdentityProviders() []* } // GetLDAPIdentityProviders lists only the LDAP providers for this FederationDomain. -func (u *FederationDomainIdentityProvidersLister) GetLDAPIdentityProviders() []*FederationDomainResolvedLDAPIdentityProvider { +func (u *FederationDomainIdentityProvidersListerFinder) GetLDAPIdentityProviders() []*resolvedprovider.FederationDomainResolvedLDAPIdentityProvider { // Get the cached providers once at the start in case they change during the rest of this function. cachedProviders := u.wrappedLister.GetLDAPIdentityProviders() - providers := []*FederationDomainResolvedLDAPIdentityProvider{} + providers := []*resolvedprovider.FederationDomainResolvedLDAPIdentityProvider{} // 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... for _, idp := range u.configuredIdentityProviders { @@ -203,7 +183,7 @@ func (u *FederationDomainIdentityProvidersLister) GetLDAPIdentityProviders() []* for _, p := range cachedProviders { if idp.UID == p.GetResourceUID() { // Found it, so append it to the result. - providers = append(providers, &FederationDomainResolvedLDAPIdentityProvider{ + providers = append(providers, &resolvedprovider.FederationDomainResolvedLDAPIdentityProvider{ DisplayName: idp.DisplayName, Provider: p, SessionProviderType: psession.ProviderTypeLDAP, @@ -216,10 +196,10 @@ func (u *FederationDomainIdentityProvidersLister) GetLDAPIdentityProviders() []* } // GetActiveDirectoryIdentityProviders lists only the ActiveDirectory providers for this FederationDomain. -func (u *FederationDomainIdentityProvidersLister) GetActiveDirectoryIdentityProviders() []*FederationDomainResolvedLDAPIdentityProvider { +func (u *FederationDomainIdentityProvidersListerFinder) GetActiveDirectoryIdentityProviders() []*resolvedprovider.FederationDomainResolvedLDAPIdentityProvider { // Get the cached providers once at the start in case they change during the rest of this function. cachedProviders := u.wrappedLister.GetActiveDirectoryIdentityProviders() - providers := []*FederationDomainResolvedLDAPIdentityProvider{} + providers := []*resolvedprovider.FederationDomainResolvedLDAPIdentityProvider{} // 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... for _, idp := range u.configuredIdentityProviders { @@ -227,7 +207,7 @@ func (u *FederationDomainIdentityProvidersLister) GetActiveDirectoryIdentityProv for _, p := range cachedProviders { if idp.UID == p.GetResourceUID() { // Found it, so append it to the result. - providers = append(providers, &FederationDomainResolvedLDAPIdentityProvider{ + providers = append(providers, &resolvedprovider.FederationDomainResolvedLDAPIdentityProvider{ DisplayName: idp.DisplayName, Provider: p, SessionProviderType: psession.ProviderTypeActiveDirectory, diff --git a/internal/oidc/provider/federation_domain_issuer.go b/internal/oidc/provider/federationdomainproviders/federation_domain_issuer.go similarity index 93% rename from internal/oidc/provider/federation_domain_issuer.go rename to internal/oidc/provider/federationdomainproviders/federation_domain_issuer.go index bdb27f14..7d937c49 100644 --- a/internal/oidc/provider/federation_domain_issuer.go +++ b/internal/oidc/provider/federationdomainproviders/federation_domain_issuer.go @@ -1,7 +1,7 @@ // Copyright 2020-2023 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 -package provider +package federationdomainproviders import ( "fmt" @@ -11,8 +11,8 @@ import ( "go.pinniped.dev/internal/constable" ) -// FederationDomainIssuer represents all the settings and state for a downstream OIDC provider -// as defined by a FederationDomain. +// FederationDomainIssuer is a parsed FederationDomain representing all the settings for a downstream OIDC provider +// and contains configuration representing a set of upstream identity providers. type FederationDomainIssuer struct { issuer string issuerHost string diff --git a/internal/oidc/provider/federation_domain_issuer_test.go b/internal/oidc/provider/federationdomainproviders/federation_domain_issuer_test.go similarity index 98% rename from internal/oidc/provider/federation_domain_issuer_test.go rename to internal/oidc/provider/federationdomainproviders/federation_domain_issuer_test.go index 08a3d851..cd4c97fb 100644 --- a/internal/oidc/provider/federation_domain_issuer_test.go +++ b/internal/oidc/provider/federationdomainproviders/federation_domain_issuer_test.go @@ -1,7 +1,7 @@ // Copyright 2020-2023 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 -package provider +package federationdomainproviders import ( "testing" diff --git a/internal/oidc/provider/manager/manager.go b/internal/oidc/provider/manager/manager.go index 2c8df7a0..2d0e760a 100644 --- a/internal/oidc/provider/manager/manager.go +++ b/internal/oidc/provider/manager/manager.go @@ -23,7 +23,7 @@ import ( "go.pinniped.dev/internal/oidc/jwks" "go.pinniped.dev/internal/oidc/login" "go.pinniped.dev/internal/oidc/oidcclientvalidator" - "go.pinniped.dev/internal/oidc/provider" + "go.pinniped.dev/internal/oidc/provider/federationdomainproviders" "go.pinniped.dev/internal/oidc/token" "go.pinniped.dev/internal/plog" "go.pinniped.dev/internal/secret" @@ -36,7 +36,7 @@ import ( // It is thread-safe. type Manager struct { mu sync.RWMutex - providers []*provider.FederationDomainIssuer + providers []*federationdomainproviders.FederationDomainIssuer providerHandlers map[string]http.Handler // map of all routes for all providers nextHandler http.Handler // the next handler in a chain, called when this manager didn't know how to handle a request dynamicJWKSProvider jwks.DynamicJWKSProvider // in-memory cache of per-issuer JWKS data @@ -77,7 +77,7 @@ func NewManager( // // This method assumes that all of the FederationDomainIssuer arguments have already been validated // by someone else before they are passed to this method. -func (m *Manager) SetFederationDomains(federationDomains ...*provider.FederationDomainIssuer) { +func (m *Manager) SetFederationDomains(federationDomains ...*federationdomainproviders.FederationDomainIssuer) { m.mu.Lock() defer m.mu.Unlock() @@ -123,7 +123,7 @@ func (m *Manager) SetFederationDomains(federationDomains ...*provider.Federation wrapGetter(incomingFederationDomain.Issuer(), m.secretCache.GetStateEncoderBlockKey), ) - idpLister := provider.NewFederationDomainUpstreamIdentityProvidersLister(incomingFederationDomain, m.upstreamIDPs) + idpLister := federationdomainproviders.NewFederationDomainIdentityProvidersListerFinder(incomingFederationDomain, m.upstreamIDPs) m.providerHandlers[(issuerHostWithPath + oidc.WellKnownEndpointPath)] = discovery.NewHandler(issuerURL) diff --git a/internal/oidc/provider/manager/manager_test.go b/internal/oidc/provider/manager/manager_test.go index 9f02d9cb..8cf764c6 100644 --- a/internal/oidc/provider/manager/manager_test.go +++ b/internal/oidc/provider/manager/manager_test.go @@ -25,7 +25,7 @@ import ( "go.pinniped.dev/internal/oidc" "go.pinniped.dev/internal/oidc/discovery" "go.pinniped.dev/internal/oidc/jwks" - "go.pinniped.dev/internal/oidc/provider" + "go.pinniped.dev/internal/oidc/provider/federationdomainproviders" "go.pinniped.dev/internal/secret" "go.pinniped.dev/internal/testutil" "go.pinniped.dev/internal/testutil/oidctestutil" @@ -39,7 +39,7 @@ func TestManager(t *testing.T) { nextHandler http.HandlerFunc fallbackHandlerWasCalled bool dynamicJWKSProvider jwks.DynamicJWKSProvider - federationDomainIDPs []*provider.FederationDomainIdentityProvider + federationDomainIDPs []*federationdomainproviders.FederationDomainIdentityProvider kubeClient *fake.Clientset ) @@ -249,7 +249,7 @@ func TestManager(t *testing.T) { parsedUpstreamIDPAuthorizationURL, err := url.Parse(upstreamIDPAuthorizationURL) r.NoError(err) - federationDomainIDPs = []*provider.FederationDomainIdentityProvider{ + federationDomainIDPs = []*federationdomainproviders.FederationDomainIdentityProvider{ { DisplayName: upstreamIDPName, UID: upstreamResourceUID, @@ -391,9 +391,9 @@ func TestManager(t *testing.T) { when("given some valid providers via SetFederationDomains()", func() { it.Before(func() { - fd1, err := provider.NewFederationDomainIssuer(issuer1, federationDomainIDPs) + fd1, err := federationdomainproviders.NewFederationDomainIssuer(issuer1, federationDomainIDPs) r.NoError(err) - fd2, err := provider.NewFederationDomainIssuer(issuer2, federationDomainIDPs) + fd2, err := federationdomainproviders.NewFederationDomainIssuer(issuer2, federationDomainIDPs) r.NoError(err) subject.SetFederationDomains(fd1, fd2) @@ -434,9 +434,9 @@ func TestManager(t *testing.T) { when("given the same valid providers as arguments to SetFederationDomains() in reverse order", func() { it.Before(func() { - fd1, err := provider.NewFederationDomainIssuer(issuer1, federationDomainIDPs) + fd1, err := federationdomainproviders.NewFederationDomainIssuer(issuer1, federationDomainIDPs) r.NoError(err) - fd2, err := provider.NewFederationDomainIssuer(issuer2, federationDomainIDPs) + fd2, err := federationdomainproviders.NewFederationDomainIssuer(issuer2, federationDomainIDPs) r.NoError(err) subject.SetFederationDomains(fd2, fd1) diff --git a/internal/oidc/provider/resolvedprovider/resolved_provider.go b/internal/oidc/provider/resolvedprovider/resolved_provider.go new file mode 100644 index 00000000..f37f0a16 --- /dev/null +++ b/internal/oidc/provider/resolvedprovider/resolved_provider.go @@ -0,0 +1,27 @@ +package resolvedprovider + +import ( + "go.pinniped.dev/internal/idtransform" + "go.pinniped.dev/internal/oidc/provider/upstreamprovider" + "go.pinniped.dev/internal/psession" +) + +// FederationDomainResolvedOIDCIdentityProvider represents a FederationDomainIdentityProvider which has +// been resolved dynamically based on the currently loaded IDP CRs to include the provider.UpstreamOIDCIdentityProviderI +// and other metadata about the provider. +type FederationDomainResolvedOIDCIdentityProvider struct { + DisplayName string + Provider upstreamprovider.UpstreamOIDCIdentityProviderI + SessionProviderType psession.ProviderType + Transforms *idtransform.TransformationPipeline +} + +// FederationDomainResolvedLDAPIdentityProvider represents a FederationDomainIdentityProvider which has +// been resolved dynamically based on the currently loaded IDP CRs to include the provider.UpstreamLDAPIdentityProviderI +// and other metadata about the provider. +type FederationDomainResolvedLDAPIdentityProvider struct { + DisplayName string + Provider upstreamprovider.UpstreamLDAPIdentityProviderI + SessionProviderType psession.ProviderType + Transforms *idtransform.TransformationPipeline +} diff --git a/internal/oidc/token/token_handler.go b/internal/oidc/token/token_handler.go index 09cb1596..cddca455 100644 --- a/internal/oidc/token/token_handler.go +++ b/internal/oidc/token/token_handler.go @@ -22,14 +22,15 @@ import ( "go.pinniped.dev/internal/idtransform" "go.pinniped.dev/internal/oidc" "go.pinniped.dev/internal/oidc/downstreamsession" - "go.pinniped.dev/internal/oidc/provider" + "go.pinniped.dev/internal/oidc/provider/federationdomainproviders" + "go.pinniped.dev/internal/oidc/provider/resolvedprovider" "go.pinniped.dev/internal/oidc/provider/upstreamprovider" "go.pinniped.dev/internal/plog" "go.pinniped.dev/internal/psession" ) func NewHandler( - idpLister provider.FederationDomainIdentityProvidersListerI, + idpLister federationdomainproviders.FederationDomainIdentityProvidersListerI, oauthHelper fosite.OAuth2Provider, ) http.Handler { return httperr.HandlerFunc(func(w http.ResponseWriter, r *http.Request) error { @@ -97,7 +98,7 @@ func errUpstreamRefreshError() *fosite.RFC6749Error { } } -func upstreamRefresh(ctx context.Context, accessRequest fosite.AccessRequester, idpLister provider.FederationDomainIdentityProvidersListerI) error { +func upstreamRefresh(ctx context.Context, accessRequest fosite.AccessRequester, idpLister federationdomainproviders.FederationDomainIdentityProvidersListerI) error { session := accessRequest.GetSession().(*psession.PinnipedSession) customSessionData := session.Custom @@ -129,7 +130,7 @@ func upstreamRefresh(ctx context.Context, accessRequest fosite.AccessRequester, func upstreamOIDCRefresh( ctx context.Context, session *psession.PinnipedSession, - idpLister provider.FederationDomainIdentityProvidersListerI, + idpLister federationdomainproviders.FederationDomainIdentityProvidersListerI, grantedScopes []string, clientID string, ) error { @@ -310,8 +311,8 @@ func getString(m map[string]interface{}, key string) (string, bool) { func findOIDCProviderByNameAndValidateUID( s *psession.CustomSessionData, - idpLister provider.FederationDomainIdentityProvidersListerI, -) (*provider.FederationDomainResolvedOIDCIdentityProvider, error) { + idpLister federationdomainproviders.FederationDomainIdentityProvidersListerI, +) (*resolvedprovider.FederationDomainResolvedOIDCIdentityProvider, error) { for _, p := range idpLister.GetOIDCIdentityProviders() { if p.Provider.GetName() == s.ProviderName { if p.Provider.GetResourceUID() != s.ProviderUID { @@ -328,7 +329,7 @@ func findOIDCProviderByNameAndValidateUID( func upstreamLDAPRefresh( ctx context.Context, - idpLister provider.FederationDomainIdentityProvidersListerI, + idpLister federationdomainproviders.FederationDomainIdentityProvidersListerI, session *psession.PinnipedSession, grantedScopes []string, clientID string, @@ -439,9 +440,9 @@ func transformRefreshedIdentity( func findLDAPProviderByNameAndValidateUID( s *psession.CustomSessionData, - idpLister provider.FederationDomainIdentityProvidersListerI, -) (*provider.FederationDomainResolvedLDAPIdentityProvider, string, error) { - var providers []*provider.FederationDomainResolvedLDAPIdentityProvider + idpLister federationdomainproviders.FederationDomainIdentityProvidersListerI, +) (*resolvedprovider.FederationDomainResolvedLDAPIdentityProvider, string, error) { + var providers []*resolvedprovider.FederationDomainResolvedLDAPIdentityProvider var dn string if s.ProviderType == psession.ProviderTypeLDAP { providers = idpLister.GetLDAPIdentityProviders() diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index 494346d2..aaeef0f6 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -54,7 +54,7 @@ import ( "go.pinniped.dev/internal/oidc/clientregistry" "go.pinniped.dev/internal/oidc/jwks" "go.pinniped.dev/internal/oidc/oidcclientvalidator" - "go.pinniped.dev/internal/oidc/provider" + "go.pinniped.dev/internal/oidc/provider/federationdomainproviders" "go.pinniped.dev/internal/oidcclientsecretstorage" "go.pinniped.dev/internal/psession" "go.pinniped.dev/internal/testutil" @@ -4170,7 +4170,7 @@ func requireClaimsAreEqual(t *testing.T, claimName string, claimsOfTokenA map[st func exchangeAuthcodeForTokens( t *testing.T, test authcodeExchangeInputs, - idps provider.FederationDomainIdentityProvidersListerFinderI, + idps federationdomainproviders.FederationDomainIdentityProvidersListerFinderI, kubeResources func(t *testing.T, supervisorClient *supervisorfake.Clientset, kubeClient *fake.Clientset), ) ( subject http.Handler, diff --git a/internal/registry/credentialrequest/rest.go b/internal/registry/credentialrequest/rest.go index 4e272266..4a5631d0 100644 --- a/internal/registry/credentialrequest/rest.go +++ b/internal/registry/credentialrequest/rest.go @@ -21,7 +21,7 @@ import ( "k8s.io/utils/trace" loginapi "go.pinniped.dev/generated/latest/apis/concierge/login" - "go.pinniped.dev/internal/issuer" + "go.pinniped.dev/internal/clientcertissuer" ) // clientCertificateTTL is the TTL for short-lived client certificates returned by this API. @@ -31,7 +31,7 @@ type TokenCredentialRequestAuthenticator interface { AuthenticateTokenCredentialRequest(ctx context.Context, req *loginapi.TokenCredentialRequest) (user.Info, error) } -func NewREST(authenticator TokenCredentialRequestAuthenticator, issuer issuer.ClientCertIssuer, resource schema.GroupResource) *REST { +func NewREST(authenticator TokenCredentialRequestAuthenticator, issuer clientcertissuer.ClientCertIssuer, resource schema.GroupResource) *REST { return &REST{ authenticator: authenticator, issuer: issuer, @@ -41,7 +41,7 @@ func NewREST(authenticator TokenCredentialRequestAuthenticator, issuer issuer.Cl type REST struct { authenticator TokenCredentialRequestAuthenticator - issuer issuer.ClientCertIssuer + issuer clientcertissuer.ClientCertIssuer tableConvertor rest.TableConvertor } diff --git a/internal/registry/credentialrequest/rest_test.go b/internal/registry/credentialrequest/rest_test.go index 3ad72844..a9bff18f 100644 --- a/internal/registry/credentialrequest/rest_test.go +++ b/internal/registry/credentialrequest/rest_test.go @@ -25,7 +25,7 @@ import ( "k8s.io/utils/ptr" loginapi "go.pinniped.dev/generated/latest/apis/concierge/login" - "go.pinniped.dev/internal/issuer" + "go.pinniped.dev/internal/clientcertissuer" "go.pinniped.dev/internal/mocks/credentialrequestmocks" "go.pinniped.dev/internal/mocks/issuermocks" "go.pinniped.dev/internal/testutil" @@ -392,7 +392,7 @@ func requireSuccessfulResponseWithAuthenticationFailureMessage(t *testing.T, err }) } -func successfulIssuer(ctrl *gomock.Controller) issuer.ClientCertIssuer { +func successfulIssuer(ctrl *gomock.Controller) clientcertissuer.ClientCertIssuer { clientCertIssuer := issuermocks.NewMockClientCertIssuer(ctrl) clientCertIssuer.EXPECT(). IssueClientCertPEM(gomock.Any(), gomock.Any(), gomock.Any()). diff --git a/internal/testutil/oidctestutil/oidctestutil.go b/internal/testutil/oidctestutil/oidctestutil.go index a574c781..e879ffcf 100644 --- a/internal/testutil/oidctestutil/oidctestutil.go +++ b/internal/testutil/oidctestutil/oidctestutil.go @@ -36,6 +36,7 @@ import ( "go.pinniped.dev/internal/fositestoragei" "go.pinniped.dev/internal/idtransform" "go.pinniped.dev/internal/oidc/provider" + "go.pinniped.dev/internal/oidc/provider/resolvedprovider" "go.pinniped.dev/internal/oidc/provider/upstreamprovider" "go.pinniped.dev/internal/psession" "go.pinniped.dev/internal/testutil" @@ -470,10 +471,10 @@ func (t *TestFederationDomainIdentityProvidersListerFinder) SetDefaultIDPDisplay t.defaultIDPDisplayName = displayName } -func (t *TestFederationDomainIdentityProvidersListerFinder) GetOIDCIdentityProviders() []*provider.FederationDomainResolvedOIDCIdentityProvider { - fdIDPs := make([]*provider.FederationDomainResolvedOIDCIdentityProvider, len(t.upstreamOIDCIdentityProviders)) +func (t *TestFederationDomainIdentityProvidersListerFinder) GetOIDCIdentityProviders() []*resolvedprovider.FederationDomainResolvedOIDCIdentityProvider { + fdIDPs := make([]*resolvedprovider.FederationDomainResolvedOIDCIdentityProvider, len(t.upstreamOIDCIdentityProviders)) for i, testIDP := range t.upstreamOIDCIdentityProviders { - fdIDP := &provider.FederationDomainResolvedOIDCIdentityProvider{ + fdIDP := &resolvedprovider.FederationDomainResolvedOIDCIdentityProvider{ DisplayName: testIDP.DisplayNameForFederationDomain, Provider: testIDP, SessionProviderType: psession.ProviderTypeOIDC, @@ -484,10 +485,10 @@ func (t *TestFederationDomainIdentityProvidersListerFinder) GetOIDCIdentityProvi return fdIDPs } -func (t *TestFederationDomainIdentityProvidersListerFinder) GetLDAPIdentityProviders() []*provider.FederationDomainResolvedLDAPIdentityProvider { - fdIDPs := make([]*provider.FederationDomainResolvedLDAPIdentityProvider, len(t.upstreamLDAPIdentityProviders)) +func (t *TestFederationDomainIdentityProvidersListerFinder) GetLDAPIdentityProviders() []*resolvedprovider.FederationDomainResolvedLDAPIdentityProvider { + fdIDPs := make([]*resolvedprovider.FederationDomainResolvedLDAPIdentityProvider, len(t.upstreamLDAPIdentityProviders)) for i, testIDP := range t.upstreamLDAPIdentityProviders { - fdIDP := &provider.FederationDomainResolvedLDAPIdentityProvider{ + fdIDP := &resolvedprovider.FederationDomainResolvedLDAPIdentityProvider{ DisplayName: testIDP.DisplayNameForFederationDomain, Provider: testIDP, SessionProviderType: psession.ProviderTypeLDAP, @@ -498,10 +499,10 @@ func (t *TestFederationDomainIdentityProvidersListerFinder) GetLDAPIdentityProvi return fdIDPs } -func (t *TestFederationDomainIdentityProvidersListerFinder) GetActiveDirectoryIdentityProviders() []*provider.FederationDomainResolvedLDAPIdentityProvider { - fdIDPs := make([]*provider.FederationDomainResolvedLDAPIdentityProvider, len(t.upstreamActiveDirectoryIdentityProviders)) +func (t *TestFederationDomainIdentityProvidersListerFinder) GetActiveDirectoryIdentityProviders() []*resolvedprovider.FederationDomainResolvedLDAPIdentityProvider { + fdIDPs := make([]*resolvedprovider.FederationDomainResolvedLDAPIdentityProvider, len(t.upstreamActiveDirectoryIdentityProviders)) for i, testIDP := range t.upstreamActiveDirectoryIdentityProviders { - fdIDP := &provider.FederationDomainResolvedLDAPIdentityProvider{ + fdIDP := &resolvedprovider.FederationDomainResolvedLDAPIdentityProvider{ DisplayName: testIDP.DisplayNameForFederationDomain, Provider: testIDP, SessionProviderType: psession.ProviderTypeActiveDirectory, @@ -512,17 +513,17 @@ func (t *TestFederationDomainIdentityProvidersListerFinder) GetActiveDirectoryId return fdIDPs } -func (t *TestFederationDomainIdentityProvidersListerFinder) FindDefaultIDP() (*provider.FederationDomainResolvedOIDCIdentityProvider, *provider.FederationDomainResolvedLDAPIdentityProvider, error) { +func (t *TestFederationDomainIdentityProvidersListerFinder) FindDefaultIDP() (*resolvedprovider.FederationDomainResolvedOIDCIdentityProvider, *resolvedprovider.FederationDomainResolvedLDAPIdentityProvider, error) { if t.defaultIDPDisplayName == "" { return nil, nil, fmt.Errorf("identity provider not found: this federation domain does not have a default identity provider") } return t.FindUpstreamIDPByDisplayName(t.defaultIDPDisplayName) } -func (t *TestFederationDomainIdentityProvidersListerFinder) FindUpstreamIDPByDisplayName(upstreamIDPDisplayName string) (*provider.FederationDomainResolvedOIDCIdentityProvider, *provider.FederationDomainResolvedLDAPIdentityProvider, error) { +func (t *TestFederationDomainIdentityProvidersListerFinder) FindUpstreamIDPByDisplayName(upstreamIDPDisplayName string) (*resolvedprovider.FederationDomainResolvedOIDCIdentityProvider, *resolvedprovider.FederationDomainResolvedLDAPIdentityProvider, error) { for _, testIDP := range t.upstreamOIDCIdentityProviders { if upstreamIDPDisplayName == testIDP.DisplayNameForFederationDomain { - return &provider.FederationDomainResolvedOIDCIdentityProvider{ + return &resolvedprovider.FederationDomainResolvedOIDCIdentityProvider{ DisplayName: testIDP.DisplayNameForFederationDomain, Provider: testIDP, SessionProviderType: psession.ProviderTypeOIDC, @@ -532,7 +533,7 @@ func (t *TestFederationDomainIdentityProvidersListerFinder) FindUpstreamIDPByDis } for _, testIDP := range t.upstreamLDAPIdentityProviders { if upstreamIDPDisplayName == testIDP.DisplayNameForFederationDomain { - return nil, &provider.FederationDomainResolvedLDAPIdentityProvider{ + return nil, &resolvedprovider.FederationDomainResolvedLDAPIdentityProvider{ DisplayName: testIDP.DisplayNameForFederationDomain, Provider: testIDP, SessionProviderType: psession.ProviderTypeLDAP, @@ -542,7 +543,7 @@ func (t *TestFederationDomainIdentityProvidersListerFinder) FindUpstreamIDPByDis } for _, testIDP := range t.upstreamActiveDirectoryIdentityProviders { if upstreamIDPDisplayName == testIDP.DisplayNameForFederationDomain { - return nil, &provider.FederationDomainResolvedLDAPIdentityProvider{ + return nil, &resolvedprovider.FederationDomainResolvedLDAPIdentityProvider{ DisplayName: testIDP.DisplayNameForFederationDomain, Provider: testIDP, SessionProviderType: psession.ProviderTypeActiveDirectory,