From 1f5978aa1a6c77d77e15fa34b8c92e6909c53c8a Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Wed, 7 Apr 2021 16:12:13 -0700 Subject: [PATCH] Supervisor pre-factor to make room for upstream LDAP identity providers --- .../upstreamwatcher/upstreamwatcher.go | 4 +- .../upstreamwatcher/upstreamwatcher_test.go | 6 +- internal/ldap/ldap.go | 20 ++++++ internal/oidc/auth/auth_handler.go | 10 +-- internal/oidc/auth/auth_handler_test.go | 70 +++++++++---------- internal/oidc/callback/callback_handler.go | 8 +-- .../oidc/callback/callback_handler_test.go | 4 +- internal/oidc/oidc.go | 15 +++- internal/oidc/oidctestutil/oidc.go | 25 +++++-- .../provider/dynamic_upstream_idp_provider.go | 45 +++++++++--- internal/oidc/provider/manager/manager.go | 28 ++++---- .../oidc/provider/manager/manager_test.go | 8 +-- test/integration/supervisor_login_test.go | 4 +- 13 files changed, 157 insertions(+), 90 deletions(-) create mode 100644 internal/ldap/ldap.go diff --git a/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher.go b/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher.go index 345d2b5f..a4cc821e 100644 --- a/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher.go +++ b/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher.go @@ -66,7 +66,7 @@ const ( // IDPCache is a thread safe cache that holds a list of validated upstream OIDC IDP configurations. type IDPCache interface { - SetIDPList([]provider.UpstreamOIDCIdentityProviderI) + SetOIDCIdentityProviders([]provider.UpstreamOIDCIdentityProviderI) } // lruValidatorCache caches the *oidc.Provider associated with a particular issuer/TLS configuration. @@ -159,7 +159,7 @@ func (c *controller) Sync(ctx controllerlib.Context) error { validatedUpstreams = append(validatedUpstreams, provider.UpstreamOIDCIdentityProviderI(valid)) } } - c.cache.SetIDPList(validatedUpstreams) + c.cache.SetOIDCIdentityProviders(validatedUpstreams) if requeue { return controllerlib.ErrSyntheticRequeue } diff --git a/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher_test.go b/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher_test.go index f7397352..0b199a11 100644 --- a/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher_test.go +++ b/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher_test.go @@ -76,7 +76,7 @@ func TestControllerFilterSecret(t *testing.T) { kubeInformers := informers.NewSharedInformerFactory(fakeKubeClient, 0) testLog := testlogger.New(t) cache := provider.NewDynamicUpstreamIDPProvider() - cache.SetIDPList([]provider.UpstreamOIDCIdentityProviderI{ + cache.SetOIDCIdentityProviders([]provider.UpstreamOIDCIdentityProviderI{ &upstreamoidc.ProviderConfig{Name: "initial-entry"}, }) secretInformer := kubeInformers.Core().V1().Secrets() @@ -611,7 +611,7 @@ func TestController(t *testing.T) { kubeInformers := informers.NewSharedInformerFactory(fakeKubeClient, 0) testLog := testlogger.New(t) cache := provider.NewDynamicUpstreamIDPProvider() - cache.SetIDPList([]provider.UpstreamOIDCIdentityProviderI{ + cache.SetOIDCIdentityProviders([]provider.UpstreamOIDCIdentityProviderI{ &upstreamoidc.ProviderConfig{Name: "initial-entry"}, }) @@ -640,7 +640,7 @@ func TestController(t *testing.T) { } require.Equal(t, strings.Join(tt.wantLogs, "\n"), strings.Join(testLog.Lines(), "\n")) - actualIDPList := cache.GetIDPList() + actualIDPList := cache.GetOIDCIdentityProviders() require.Equal(t, len(tt.wantResultingCache), len(actualIDPList)) for i := range actualIDPList { actualIDP := actualIDPList[i].(*upstreamoidc.ProviderConfig) diff --git a/internal/ldap/ldap.go b/internal/ldap/ldap.go new file mode 100644 index 00000000..182971d4 --- /dev/null +++ b/internal/ldap/ldap.go @@ -0,0 +1,20 @@ +// Copyright 2021 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +// Package ldap contains common LDAP functionality needed by Pinniped. +package ldap + +import ( + "context" + + "k8s.io/apiserver/pkg/authentication/authenticator" +) + +// This interface is similar to the k8s token authenticator, but works with username/passwords instead +// of a single token string. +// +// See k8s.io/apiserver/pkg/authentication/authenticator/interfaces.go for the token authenticator +// interface, as well as the Response type. +type UserAuthenticator interface { + AuthenticateUser(ctx context.Context, username, password string) (*authenticator.Response, bool, error) +} diff --git a/internal/oidc/auth/auth_handler.go b/internal/oidc/auth/auth_handler.go index e5d359a8..394d8432 100644 --- a/internal/oidc/auth/auth_handler.go +++ b/internal/oidc/auth/auth_handler.go @@ -1,4 +1,4 @@ -// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // Package auth provides a handler for the OIDC authorization endpoint. @@ -27,7 +27,7 @@ import ( func NewHandler( downstreamIssuer string, - idpListGetter oidc.IDPListGetter, + idpLister oidc.UpstreamIdentityProvidersLister, oauthHelper fosite.OAuth2Provider, generateCSRF func() (csrftoken.CSRFToken, error), generatePKCE func() (pkce.Code, error), @@ -52,7 +52,7 @@ func NewHandler( return nil } - upstreamIDP, err := chooseUpstreamIDP(idpListGetter) + upstreamIDP, err := chooseUpstreamIDP(idpLister) if err != nil { plog.WarningErr("authorize upstream config", err) return err @@ -165,8 +165,8 @@ func readCSRFCookie(r *http.Request, codec oidc.Decoder) csrftoken.CSRFToken { return csrfFromCookie } -func chooseUpstreamIDP(idpListGetter oidc.IDPListGetter) (provider.UpstreamOIDCIdentityProviderI, error) { - allUpstreamIDPs := idpListGetter.GetIDPList() +func chooseUpstreamIDP(idpLister oidc.UpstreamOIDCIdentityProvidersLister) (provider.UpstreamOIDCIdentityProviderI, error) { + allUpstreamIDPs := idpLister.GetOIDCIdentityProviders() if len(allUpstreamIDPs) == 0 { return nil, httperr.New( http.StatusUnprocessableEntity, diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index a0f715d1..a69fa3c4 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -1,4 +1,4 @@ -// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package auth @@ -244,7 +244,7 @@ func TestAuthorizationEndpoint(t *testing.T) { name string issuer string - idpListGetter provider.DynamicUpstreamIDPProvider + idpLister provider.DynamicUpstreamIDPProvider generateCSRF func() (csrftoken.CSRFToken, error) generatePKCE func() (pkce.Code, error) generateNonce func() (nonce.Nonce, error) @@ -270,7 +270,7 @@ func TestAuthorizationEndpoint(t *testing.T) { { name: "happy path using GET without a CSRF cookie", issuer: downstreamIssuer, - idpListGetter: oidctestutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), + idpLister: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(&upstreamOIDCIdentityProvider).Build(), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, @@ -288,7 +288,7 @@ func TestAuthorizationEndpoint(t *testing.T) { { name: "happy path using GET with a CSRF cookie", issuer: downstreamIssuer, - idpListGetter: oidctestutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), + idpLister: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(&upstreamOIDCIdentityProvider).Build(), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, @@ -306,7 +306,7 @@ func TestAuthorizationEndpoint(t *testing.T) { { name: "happy path using POST", issuer: downstreamIssuer, - idpListGetter: oidctestutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), + idpLister: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(&upstreamOIDCIdentityProvider).Build(), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, @@ -326,7 +326,7 @@ func TestAuthorizationEndpoint(t *testing.T) { { name: "happy path with prompt param login passed through to redirect uri", issuer: downstreamIssuer, - idpListGetter: oidctestutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), + idpLister: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(&upstreamOIDCIdentityProvider).Build(), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, @@ -346,7 +346,7 @@ func TestAuthorizationEndpoint(t *testing.T) { { name: "error while decoding CSRF cookie just generates a new cookie and succeeds as usual", issuer: downstreamIssuer, - idpListGetter: oidctestutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), + idpLister: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(&upstreamOIDCIdentityProvider).Build(), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, @@ -366,7 +366,7 @@ func TestAuthorizationEndpoint(t *testing.T) { { name: "happy path when downstream redirect uri matches what is configured for client except for the port number", issuer: downstreamIssuer, - idpListGetter: oidctestutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), + idpLister: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(&upstreamOIDCIdentityProvider).Build(), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, @@ -388,7 +388,7 @@ func TestAuthorizationEndpoint(t *testing.T) { { name: "happy path when downstream requested scopes include offline_access", issuer: downstreamIssuer, - idpListGetter: oidctestutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), + idpLister: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(&upstreamOIDCIdentityProvider).Build(), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, @@ -408,7 +408,7 @@ func TestAuthorizationEndpoint(t *testing.T) { { name: "downstream redirect uri does not match what is configured for client", issuer: downstreamIssuer, - idpListGetter: oidctestutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), + idpLister: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(&upstreamOIDCIdentityProvider).Build(), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, @@ -425,7 +425,7 @@ func TestAuthorizationEndpoint(t *testing.T) { { name: "downstream client does not exist", issuer: downstreamIssuer, - idpListGetter: oidctestutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), + idpLister: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(&upstreamOIDCIdentityProvider).Build(), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, @@ -440,7 +440,7 @@ func TestAuthorizationEndpoint(t *testing.T) { { name: "response type is unsupported", issuer: downstreamIssuer, - idpListGetter: oidctestutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), + idpLister: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(&upstreamOIDCIdentityProvider).Build(), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, @@ -456,7 +456,7 @@ func TestAuthorizationEndpoint(t *testing.T) { { name: "downstream scopes do not match what is configured for client", issuer: downstreamIssuer, - idpListGetter: oidctestutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), + idpLister: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(&upstreamOIDCIdentityProvider).Build(), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, @@ -472,7 +472,7 @@ func TestAuthorizationEndpoint(t *testing.T) { { name: "missing response type in request", issuer: downstreamIssuer, - idpListGetter: oidctestutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), + idpLister: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(&upstreamOIDCIdentityProvider).Build(), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, @@ -488,7 +488,7 @@ func TestAuthorizationEndpoint(t *testing.T) { { name: "missing client id in request", issuer: downstreamIssuer, - idpListGetter: oidctestutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), + idpLister: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(&upstreamOIDCIdentityProvider).Build(), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, @@ -503,7 +503,7 @@ func TestAuthorizationEndpoint(t *testing.T) { { name: "missing PKCE code_challenge in request", // See https://tools.ietf.org/html/rfc7636#section-4.4.1 issuer: downstreamIssuer, - idpListGetter: oidctestutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), + idpLister: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(&upstreamOIDCIdentityProvider).Build(), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, @@ -519,7 +519,7 @@ func TestAuthorizationEndpoint(t *testing.T) { { name: "invalid value for PKCE code_challenge_method in request", // https://tools.ietf.org/html/rfc7636#section-4.3 issuer: downstreamIssuer, - idpListGetter: oidctestutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), + idpLister: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(&upstreamOIDCIdentityProvider).Build(), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, @@ -535,7 +535,7 @@ func TestAuthorizationEndpoint(t *testing.T) { { name: "when PKCE code_challenge_method in request is `plain`", // https://tools.ietf.org/html/rfc7636#section-4.3 issuer: downstreamIssuer, - idpListGetter: oidctestutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), + idpLister: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(&upstreamOIDCIdentityProvider).Build(), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, @@ -551,7 +551,7 @@ func TestAuthorizationEndpoint(t *testing.T) { { name: "missing PKCE code_challenge_method in request", // See https://tools.ietf.org/html/rfc7636#section-4.4.1 issuer: downstreamIssuer, - idpListGetter: oidctestutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), + idpLister: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(&upstreamOIDCIdentityProvider).Build(), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, @@ -569,7 +569,7 @@ func TestAuthorizationEndpoint(t *testing.T) { // through that part of the fosite library. name: "prompt param is not allowed to have none and another legal value at the same time", issuer: downstreamIssuer, - idpListGetter: oidctestutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), + idpLister: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(&upstreamOIDCIdentityProvider).Build(), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, @@ -585,7 +585,7 @@ func TestAuthorizationEndpoint(t *testing.T) { { name: "OIDC validations are skipped when the openid scope was not requested", issuer: downstreamIssuer, - idpListGetter: oidctestutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), + idpLister: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(&upstreamOIDCIdentityProvider).Build(), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, @@ -606,7 +606,7 @@ func TestAuthorizationEndpoint(t *testing.T) { { name: "state does not have enough entropy", issuer: downstreamIssuer, - idpListGetter: oidctestutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), + idpLister: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(&upstreamOIDCIdentityProvider).Build(), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, @@ -622,7 +622,7 @@ func TestAuthorizationEndpoint(t *testing.T) { { name: "error while encoding upstream state param", issuer: downstreamIssuer, - idpListGetter: oidctestutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), + idpLister: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(&upstreamOIDCIdentityProvider).Build(), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, @@ -637,7 +637,7 @@ func TestAuthorizationEndpoint(t *testing.T) { { name: "error while encoding CSRF cookie value for new cookie", issuer: downstreamIssuer, - idpListGetter: oidctestutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), + idpLister: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(&upstreamOIDCIdentityProvider).Build(), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, @@ -652,7 +652,7 @@ func TestAuthorizationEndpoint(t *testing.T) { { name: "error while generating CSRF token", issuer: downstreamIssuer, - idpListGetter: oidctestutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), + idpLister: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(&upstreamOIDCIdentityProvider).Build(), generateCSRF: func() (csrftoken.CSRFToken, error) { return "", fmt.Errorf("some csrf generator error") }, generatePKCE: happyPKCEGenerator, generateNonce: happyNonceGenerator, @@ -667,7 +667,7 @@ func TestAuthorizationEndpoint(t *testing.T) { { name: "error while generating nonce", issuer: downstreamIssuer, - idpListGetter: oidctestutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), + idpLister: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(&upstreamOIDCIdentityProvider).Build(), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, generateNonce: func() (nonce.Nonce, error) { return "", fmt.Errorf("some nonce generator error") }, @@ -682,7 +682,7 @@ func TestAuthorizationEndpoint(t *testing.T) { { name: "error while generating PKCE", issuer: downstreamIssuer, - idpListGetter: oidctestutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), + idpLister: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(&upstreamOIDCIdentityProvider).Build(), generateCSRF: happyCSRFGenerator, generatePKCE: func() (pkce.Code, error) { return "", fmt.Errorf("some PKCE generator error") }, generateNonce: happyNonceGenerator, @@ -697,7 +697,7 @@ func TestAuthorizationEndpoint(t *testing.T) { { name: "no upstream providers are configured", issuer: downstreamIssuer, - idpListGetter: oidctestutil.NewIDPListGetter(), // empty + idpLister: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC().Build(), // empty method: http.MethodGet, path: happyGetRequestPath, wantStatus: http.StatusUnprocessableEntity, @@ -707,7 +707,7 @@ func TestAuthorizationEndpoint(t *testing.T) { { name: "too many upstream providers are configured", issuer: downstreamIssuer, - idpListGetter: oidctestutil.NewIDPListGetter(&upstreamOIDCIdentityProvider, &upstreamOIDCIdentityProvider), // more than one not allowed + idpLister: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(&upstreamOIDCIdentityProvider, &upstreamOIDCIdentityProvider).Build(), // more than one not allowed method: http.MethodGet, path: happyGetRequestPath, wantStatus: http.StatusUnprocessableEntity, @@ -717,7 +717,7 @@ func TestAuthorizationEndpoint(t *testing.T) { { name: "PUT is a bad method", issuer: downstreamIssuer, - idpListGetter: oidctestutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), + idpLister: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(&upstreamOIDCIdentityProvider).Build(), method: http.MethodPut, path: "/some/path", wantStatus: http.StatusMethodNotAllowed, @@ -727,7 +727,7 @@ func TestAuthorizationEndpoint(t *testing.T) { { name: "PATCH is a bad method", issuer: downstreamIssuer, - idpListGetter: oidctestutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), + idpLister: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(&upstreamOIDCIdentityProvider).Build(), method: http.MethodPatch, path: "/some/path", wantStatus: http.StatusMethodNotAllowed, @@ -737,7 +737,7 @@ func TestAuthorizationEndpoint(t *testing.T) { { name: "DELETE is a bad method", issuer: downstreamIssuer, - idpListGetter: oidctestutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), + idpLister: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(&upstreamOIDCIdentityProvider).Build(), method: http.MethodDelete, path: "/some/path", wantStatus: http.StatusMethodNotAllowed, @@ -803,7 +803,7 @@ func TestAuthorizationEndpoint(t *testing.T) { for _, test := range tests { test := test t.Run(test.name, func(t *testing.T) { - subject := NewHandler(test.issuer, test.idpListGetter, oauthHelper, test.generateCSRF, test.generatePKCE, test.generateNonce, test.stateEncoder, test.cookieEncoder) + subject := NewHandler(test.issuer, test.idpLister, oauthHelper, test.generateCSRF, test.generatePKCE, test.generateNonce, test.stateEncoder, test.cookieEncoder) runOneTestCase(t, test, subject) }) } @@ -812,7 +812,7 @@ func TestAuthorizationEndpoint(t *testing.T) { test := tests[0] require.Equal(t, "happy path using GET without a CSRF cookie", test.name) // re-use the happy path test case - subject := NewHandler(test.issuer, test.idpListGetter, oauthHelper, test.generateCSRF, test.generatePKCE, test.generateNonce, test.stateEncoder, test.cookieEncoder) + subject := NewHandler(test.issuer, test.idpLister, oauthHelper, test.generateCSRF, test.generatePKCE, test.generateNonce, test.stateEncoder, test.cookieEncoder) runOneTestCase(t, test, subject) @@ -823,7 +823,7 @@ func TestAuthorizationEndpoint(t *testing.T) { AuthorizationURL: *upstreamAuthURL, Scopes: []string{"other-scope1", "other-scope2"}, } - test.idpListGetter.SetIDPList([]provider.UpstreamOIDCIdentityProviderI{provider.UpstreamOIDCIdentityProviderI(&newProviderSettings)}) + test.idpLister.SetOIDCIdentityProviders([]provider.UpstreamOIDCIdentityProviderI{provider.UpstreamOIDCIdentityProviderI(&newProviderSettings)}) // Update the expectations of the test case to match the new upstream IDP settings. test.wantLocationHeader = urlWithQuery(upstreamAuthURL.String(), diff --git a/internal/oidc/callback/callback_handler.go b/internal/oidc/callback/callback_handler.go index a103824e..5bece1d9 100644 --- a/internal/oidc/callback/callback_handler.go +++ b/internal/oidc/callback/callback_handler.go @@ -33,7 +33,7 @@ const ( ) func NewHandler( - idpListGetter oidc.IDPListGetter, + upstreamIDPs oidc.UpstreamOIDCIdentityProvidersLister, oauthHelper fosite.OAuth2Provider, stateDecoder, cookieDecoder oidc.Decoder, redirectURI string, @@ -44,7 +44,7 @@ func NewHandler( return err } - upstreamIDPConfig := findUpstreamIDPConfig(state.UpstreamName, idpListGetter) + upstreamIDPConfig := findUpstreamIDPConfig(state.UpstreamName, upstreamIDPs) if upstreamIDPConfig == nil { plog.Warning("upstream provider not found") return httperr.New(http.StatusUnprocessableEntity, "upstream provider not found") @@ -143,8 +143,8 @@ func validateRequest(r *http.Request, stateDecoder, cookieDecoder oidc.Decoder) return state, nil } -func findUpstreamIDPConfig(upstreamName string, idpListGetter oidc.IDPListGetter) provider.UpstreamOIDCIdentityProviderI { - for _, p := range idpListGetter.GetIDPList() { +func findUpstreamIDPConfig(upstreamName string, upstreamIDPs oidc.UpstreamOIDCIdentityProvidersLister) provider.UpstreamOIDCIdentityProviderI { + for _, p := range upstreamIDPs.GetOIDCIdentityProviders() { if p.GetName() == upstreamName { return p } diff --git a/internal/oidc/callback/callback_handler_test.go b/internal/oidc/callback/callback_handler_test.go index 218b6753..ad82928b 100644 --- a/internal/oidc/callback/callback_handler_test.go +++ b/internal/oidc/callback/callback_handler_test.go @@ -626,8 +626,8 @@ func TestCallbackEndpoint(t *testing.T) { jwksProviderIsUnused := jwks.NewDynamicJWKSProvider() oauthHelper := oidc.FositeOauth2Helper(oauthStore, downstreamIssuer, hmacSecretFunc, jwksProviderIsUnused, timeoutsConfiguration) - idpListGetter := oidctestutil.NewIDPListGetter(&test.idp) - subject := NewHandler(idpListGetter, oauthHelper, happyStateCodec, happyCookieCodec, happyUpstreamRedirectURI) + idpLister := oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(&test.idp).Build() + subject := NewHandler(idpLister, oauthHelper, happyStateCodec, happyCookieCodec, happyUpstreamRedirectURI) req := httptest.NewRequest(test.method, test.path, nil) if test.csrfCookie != "" { req.Header.Set("Cookie", test.csrfCookie) diff --git a/internal/oidc/oidc.go b/internal/oidc/oidc.go index da511515..0297b43c 100644 --- a/internal/oidc/oidc.go +++ b/internal/oidc/oidc.go @@ -1,4 +1,4 @@ -// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // Package oidc contains common OIDC functionality needed by Pinniped. @@ -274,8 +274,17 @@ func FositeErrorForLog(err error) []interface{} { return keysAndValues } -type IDPListGetter interface { - GetIDPList() []provider.UpstreamOIDCIdentityProviderI +type UpstreamOIDCIdentityProvidersLister interface { + GetOIDCIdentityProviders() []provider.UpstreamOIDCIdentityProviderI +} + +type UpstreamLDAPIdentityProvidersLister interface { + GetLDAPIdentityProviders() []provider.UpstreamLDAPIdentityProviderI +} + +type UpstreamIdentityProvidersLister interface { + UpstreamOIDCIdentityProvidersLister + UpstreamLDAPIdentityProvidersLister } func GrantScopeIfRequested(authorizeRequester fosite.AuthorizeRequester, scopeName string) { diff --git a/internal/oidc/oidctestutil/oidc.go b/internal/oidc/oidctestutil/oidc.go index 07a1c890..34938139 100644 --- a/internal/oidc/oidctestutil/oidc.go +++ b/internal/oidc/oidctestutil/oidc.go @@ -1,4 +1,4 @@ -// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package oidctestutil @@ -112,16 +112,29 @@ func (u *TestUpstreamOIDCIdentityProvider) ValidateToken(_ context.Context, _ *o panic("implement me") } -func NewIDPListGetter(upstreamOIDCIdentityProviders ...*TestUpstreamOIDCIdentityProvider) provider.DynamicUpstreamIDPProvider { +type UpstreamIDPListerBuilder struct { + upstreamOIDCIdentityProviders []*TestUpstreamOIDCIdentityProvider +} + +func (b *UpstreamIDPListerBuilder) WithOIDC(upstreamOIDCIdentityProviders ...*TestUpstreamOIDCIdentityProvider) *UpstreamIDPListerBuilder { + b.upstreamOIDCIdentityProviders = append(b.upstreamOIDCIdentityProviders, upstreamOIDCIdentityProviders...) + return b +} + +func (b *UpstreamIDPListerBuilder) Build() provider.DynamicUpstreamIDPProvider { idpProvider := provider.NewDynamicUpstreamIDPProvider() - upstreams := make([]provider.UpstreamOIDCIdentityProviderI, len(upstreamOIDCIdentityProviders)) - for i := range upstreamOIDCIdentityProviders { - upstreams[i] = provider.UpstreamOIDCIdentityProviderI(upstreamOIDCIdentityProviders[i]) + upstreams := make([]provider.UpstreamOIDCIdentityProviderI, len(b.upstreamOIDCIdentityProviders)) + for i := range b.upstreamOIDCIdentityProviders { + upstreams[i] = provider.UpstreamOIDCIdentityProviderI(b.upstreamOIDCIdentityProviders[i]) } - idpProvider.SetIDPList(upstreams) + idpProvider.SetOIDCIdentityProviders(upstreams) return idpProvider } +func NewUpstreamIDPListerBuilder() *UpstreamIDPListerBuilder { + return &UpstreamIDPListerBuilder{} +} + // Declare a separate type from the production code to ensure that the state param's contents was serialized // in the format that we expect, with the json keys that we expect, etc. This also ensure that the order of // the serialized fields is the same, which doesn't really matter expect that we can make simpler equality diff --git a/internal/oidc/provider/dynamic_upstream_idp_provider.go b/internal/oidc/provider/dynamic_upstream_idp_provider.go index fd367d88..1893352b 100644 --- a/internal/oidc/provider/dynamic_upstream_idp_provider.go +++ b/internal/oidc/provider/dynamic_upstream_idp_provider.go @@ -1,4 +1,4 @@ -// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package provider @@ -10,6 +10,7 @@ import ( "golang.org/x/oauth2" + "go.pinniped.dev/internal/ldap" "go.pinniped.dev/pkg/oidcclient/nonce" "go.pinniped.dev/pkg/oidcclient/oidctypes" "go.pinniped.dev/pkg/oidcclient/pkce" @@ -48,30 +49,54 @@ type UpstreamOIDCIdentityProviderI interface { ValidateToken(ctx context.Context, tok *oauth2.Token, expectedIDTokenNonce nonce.Nonce) (*oidctypes.Token, error) } +type UpstreamLDAPIdentityProviderI interface { + // A name for this upstream provider. + GetName() string + + // A method for performing user authentication against the upstream LDAP provider. + ldap.UserAuthenticator +} + type DynamicUpstreamIDPProvider interface { - SetIDPList(oidcIDPs []UpstreamOIDCIdentityProviderI) - GetIDPList() []UpstreamOIDCIdentityProviderI + SetOIDCIdentityProviders(oidcIDPs []UpstreamOIDCIdentityProviderI) + GetOIDCIdentityProviders() []UpstreamOIDCIdentityProviderI + SetLDAPIdentityProviders(ldapIDPs []UpstreamLDAPIdentityProviderI) + GetLDAPIdentityProviders() []UpstreamLDAPIdentityProviderI } type dynamicUpstreamIDPProvider struct { - federationDomains []UpstreamOIDCIdentityProviderI - mutex sync.RWMutex + oidcUpstreams []UpstreamOIDCIdentityProviderI + ldapUpstreams []UpstreamLDAPIdentityProviderI + mutex sync.RWMutex } func NewDynamicUpstreamIDPProvider() DynamicUpstreamIDPProvider { return &dynamicUpstreamIDPProvider{ - federationDomains: []UpstreamOIDCIdentityProviderI{}, + oidcUpstreams: []UpstreamOIDCIdentityProviderI{}, + ldapUpstreams: []UpstreamLDAPIdentityProviderI{}, } } -func (p *dynamicUpstreamIDPProvider) SetIDPList(oidcIDPs []UpstreamOIDCIdentityProviderI) { +func (p *dynamicUpstreamIDPProvider) SetOIDCIdentityProviders(oidcIDPs []UpstreamOIDCIdentityProviderI) { p.mutex.Lock() // acquire a write lock defer p.mutex.Unlock() - p.federationDomains = oidcIDPs + p.oidcUpstreams = oidcIDPs } -func (p *dynamicUpstreamIDPProvider) GetIDPList() []UpstreamOIDCIdentityProviderI { +func (p *dynamicUpstreamIDPProvider) GetOIDCIdentityProviders() []UpstreamOIDCIdentityProviderI { p.mutex.RLock() // acquire a read lock defer p.mutex.RUnlock() - return p.federationDomains + return p.oidcUpstreams +} + +func (p *dynamicUpstreamIDPProvider) SetLDAPIdentityProviders(ldapIDPs []UpstreamLDAPIdentityProviderI) { + p.mutex.Lock() // acquire a write lock + defer p.mutex.Unlock() + p.ldapUpstreams = ldapIDPs +} + +func (p *dynamicUpstreamIDPProvider) GetLDAPIdentityProviders() []UpstreamLDAPIdentityProviderI { + p.mutex.RLock() // acquire a read lock + defer p.mutex.RUnlock() + return p.ldapUpstreams } diff --git a/internal/oidc/provider/manager/manager.go b/internal/oidc/provider/manager/manager.go index 52227ce5..669e5152 100644 --- a/internal/oidc/provider/manager/manager.go +++ b/internal/oidc/provider/manager/manager.go @@ -1,4 +1,4 @@ -// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package manager @@ -8,10 +8,6 @@ import ( "strings" "sync" - "go.pinniped.dev/internal/secret" - - "go.pinniped.dev/internal/oidc/dynamiccodec" - corev1client "k8s.io/client-go/kubernetes/typed/core/v1" "go.pinniped.dev/internal/oidc" @@ -19,10 +15,12 @@ import ( "go.pinniped.dev/internal/oidc/callback" "go.pinniped.dev/internal/oidc/csrftoken" "go.pinniped.dev/internal/oidc/discovery" + "go.pinniped.dev/internal/oidc/dynamiccodec" "go.pinniped.dev/internal/oidc/jwks" "go.pinniped.dev/internal/oidc/provider" "go.pinniped.dev/internal/oidc/token" "go.pinniped.dev/internal/plog" + "go.pinniped.dev/internal/secret" "go.pinniped.dev/pkg/oidcclient/nonce" "go.pinniped.dev/pkg/oidcclient/pkce" ) @@ -33,22 +31,22 @@ import ( type Manager struct { mu sync.RWMutex providers []*provider.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 - idpListGetter oidc.IDPListGetter // in-memory cache of upstream IDPs - secretCache *secret.Cache // in-memory cache of cryptographic material + 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 + upstreamIDPs oidc.UpstreamIdentityProvidersLister // in-memory cache of upstream IDPs + secretCache *secret.Cache // in-memory cache of cryptographic material secretsClient corev1client.SecretInterface } // NewManager returns an empty Manager. // nextHandler will be invoked for any requests that could not be handled by this manager's providers. // dynamicJWKSProvider will be used as an in-memory cache for per-issuer JWKS data. -// idpListGetter will be used as an in-memory cache of currently configured upstream IDPs. +// upstreamIDPs will be used as an in-memory cache of currently configured upstream IDPs. func NewManager( nextHandler http.Handler, dynamicJWKSProvider jwks.DynamicJWKSProvider, - idpListGetter oidc.IDPListGetter, + upstreamIDPs oidc.UpstreamIdentityProvidersLister, secretCache *secret.Cache, secretsClient corev1client.SecretInterface, ) *Manager { @@ -56,7 +54,7 @@ func NewManager( providerHandlers: make(map[string]http.Handler), nextHandler: nextHandler, dynamicJWKSProvider: dynamicJWKSProvider, - idpListGetter: idpListGetter, + upstreamIDPs: upstreamIDPs, secretCache: secretCache, secretsClient: secretsClient, } @@ -110,7 +108,7 @@ func (m *Manager) SetProviders(federationDomains ...*provider.FederationDomainIs m.providerHandlers[(issuerHostWithPath + oidc.AuthorizationEndpointPath)] = auth.NewHandler( issuer, - m.idpListGetter, + m.upstreamIDPs, oauthHelperWithNullStorage, csrftoken.Generate, pkce.Generate, @@ -120,7 +118,7 @@ func (m *Manager) SetProviders(federationDomains ...*provider.FederationDomainIs ) m.providerHandlers[(issuerHostWithPath + oidc.CallbackEndpointPath)] = callback.NewHandler( - m.idpListGetter, + m.upstreamIDPs, oauthHelperWithKubeStorage, upstreamStateEncoder, csrfCookieEncoder, diff --git a/internal/oidc/provider/manager/manager_test.go b/internal/oidc/provider/manager/manager_test.go index 64b7ab4a..a5d79df9 100644 --- a/internal/oidc/provider/manager/manager_test.go +++ b/internal/oidc/provider/manager/manager_test.go @@ -1,4 +1,4 @@ -// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2021 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package manager @@ -221,7 +221,7 @@ func TestManager(t *testing.T) { parsedUpstreamIDPAuthorizationURL, err := url.Parse(upstreamIDPAuthorizationURL) r.NoError(err) - idpListGetter := oidctestutil.NewIDPListGetter(&oidctestutil.TestUpstreamOIDCIdentityProvider{ + idpLister := oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(&oidctestutil.TestUpstreamOIDCIdentityProvider{ Name: "test-idp", ClientID: "test-client-id", AuthorizationURL: *parsedUpstreamIDPAuthorizationURL, @@ -238,7 +238,7 @@ func TestManager(t *testing.T) { }, }, nil }, - }) + }).Build() kubeClient = fake.NewSimpleClientset() secretsClient := kubeClient.CoreV1().Secrets("some-namespace") @@ -254,7 +254,7 @@ func TestManager(t *testing.T) { cache.SetStateEncoderHashKey(issuer2, []byte("some-state-encoder-hash-key-2")) cache.SetStateEncoderBlockKey(issuer2, []byte("16-bytes-STATE02")) - subject = NewManager(nextHandler, dynamicJWKSProvider, idpListGetter, &cache, secretsClient) + subject = NewManager(nextHandler, dynamicJWKSProvider, idpLister, &cache, secretsClient) }) when("given no providers via SetProviders()", func() { diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index 909d6b1b..93441949 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -388,7 +388,9 @@ func requestAuthorizationUsingLDAPIdentityProvider(t *testing.T, downstreamAutho defer authResponse.Body.Close() require.NoError(t, err) - t.Skip("The rest of this test will not work until we implement the corresponding production code.") // TODO remove this skip + // TODO remove this skip + _ = responseBody // suppress linter until we remove the below skip + t.Skip("The rest of this test will not work until we implement the corresponding production code.") require.Equalf(t, http.StatusOK, authResponse.StatusCode, "response body was: %s", string(responseBody)) }