From 6b653fc663f811aa2823512b2202750928dde9e4 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Wed, 7 Oct 2020 19:18:34 -0700 Subject: [PATCH] Creation and deletion of OIDC Provider discovery endpoints from config - The OIDCProviderConfigWatcherController synchronizes the OIDCProviderConfig settings to dynamically mount and unmount the OIDC discovery endpoints for each provider - Integration test passes but unit tests need to be added still --- cmd/pinniped-supervisor/main.go | 19 +-- .../dynamic_config_watcher.go | 114 -------------- .../oidcproviderconfig_watcher.go | 91 +++++++++++ internal/oidc/discovery/discovery.go | 31 +--- internal/oidc/discovery/discovery_test.go | 52 +------ .../oidc/issuerprovider/issuerprovider.go | 80 ---------- internal/oidc/provider/manager.go | 115 ++++++++++++++ internal/oidc/provider/oidcprovider.go | 50 +++++++ .../oidcprovider_test.go} | 15 +- test/integration/supervisor_discovery_test.go | 141 +++++++++++++----- 10 files changed, 388 insertions(+), 320 deletions(-) delete mode 100644 internal/controller/supervisorconfig/dynamic_config_watcher.go create mode 100644 internal/controller/supervisorconfig/oidcproviderconfig_watcher.go delete mode 100644 internal/oidc/issuerprovider/issuerprovider.go create mode 100644 internal/oidc/provider/manager.go create mode 100644 internal/oidc/provider/oidcprovider.go rename internal/oidc/{issuerprovider/issuerprovider_test.go => provider/oidcprovider_test.go} (88%) diff --git a/cmd/pinniped-supervisor/main.go b/cmd/pinniped-supervisor/main.go index 96758ba2..9b111f50 100644 --- a/cmd/pinniped-supervisor/main.go +++ b/cmd/pinniped-supervisor/main.go @@ -23,8 +23,7 @@ import ( "go.pinniped.dev/internal/controller/supervisorconfig" "go.pinniped.dev/internal/controllerlib" "go.pinniped.dev/internal/downward" - "go.pinniped.dev/internal/oidc/discovery" - "go.pinniped.dev/internal/oidc/issuerprovider" + "go.pinniped.dev/internal/oidc/provider" ) const ( @@ -32,10 +31,8 @@ const ( defaultResyncInterval = 3 * time.Minute ) -func start(ctx context.Context, l net.Listener, discoveryHandler http.Handler) { - server := http.Server{ - Handler: discoveryHandler, - } +func start(ctx context.Context, l net.Listener, handler http.Handler) { + server := http.Server{Handler: handler} errCh := make(chan error) go func() { @@ -63,14 +60,14 @@ func waitForSignal() os.Signal { func startControllers( ctx context.Context, - issuerProvider *issuerprovider.Provider, + issuerProvider *provider.Manager, pinnipedInformers pinnipedinformers.SharedInformerFactory, ) { // Create controller manager. controllerManager := controllerlib. NewManager(). WithController( - supervisorconfig.NewDynamicConfigWatcherController( + supervisorconfig.NewOIDCProviderConfigWatcherController( issuerProvider, pinnipedInformers.Config().V1alpha1().OIDCProviderConfigs(), controllerlib.WithInformer, @@ -113,8 +110,8 @@ func run(serverInstallationNamespace string) error { pinnipedinformers.WithNamespace(serverInstallationNamespace), ) - issuerProvider := issuerprovider.New() - startControllers(ctx, issuerProvider, pinnipedInformers) + oidProvidersManager := provider.NewManager(http.NotFoundHandler()) + startControllers(ctx, oidProvidersManager, pinnipedInformers) //nolint: gosec // Intentionally binding to all network interfaces. l, err := net.Listen("tcp", ":80") @@ -123,7 +120,7 @@ func run(serverInstallationNamespace string) error { } defer l.Close() - start(ctx, l, discovery.New(issuerProvider)) + start(ctx, l, oidProvidersManager) klog.InfoS("supervisor is ready", "address", l.Addr().String()) gotSignal := waitForSignal() diff --git a/internal/controller/supervisorconfig/dynamic_config_watcher.go b/internal/controller/supervisorconfig/dynamic_config_watcher.go deleted file mode 100644 index 085475b1..00000000 --- a/internal/controller/supervisorconfig/dynamic_config_watcher.go +++ /dev/null @@ -1,114 +0,0 @@ -// Copyright 2020 the Pinniped contributors. All Rights Reserved. -// SPDX-License-Identifier: Apache-2.0 - -package supervisorconfig - -import ( - "fmt" - "net/url" - - k8serrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/klog/v2" - - configinformers "go.pinniped.dev/generated/1.19/client/informers/externalversions/config/v1alpha1" - pinnipedcontroller "go.pinniped.dev/internal/controller" - "go.pinniped.dev/internal/controllerlib" -) - -// IssuerSetter can be notified of a valid issuer with its SetIssuer function. If there is no -// longer any valid issuer, then nil can be passed to this interface. -// -// If the IssuerSetter doesn't like the provided issuer, it can return an error. -// -// Implementations of this type should be thread-safe to support calls from multiple goroutines. -type IssuerSetter interface { - SetIssuer(issuer *url.URL) error -} - -type dynamicConfigWatcherController struct { - issuerSetter IssuerSetter - opcInformer configinformers.OIDCProviderConfigInformer -} - -// NewDynamicConfigWatcherController creates a controllerlib.Controller that watches -// OIDCProviderConfig objects and notifies a callback object of their creation or deletion. -func NewDynamicConfigWatcherController( - issuerObserver IssuerSetter, - opcInformer configinformers.OIDCProviderConfigInformer, - withInformer pinnipedcontroller.WithInformerOptionFunc, -) controllerlib.Controller { - return controllerlib.New( - controllerlib.Config{ - Name: "DynamicConfigWatcherController", - Syncer: &dynamicConfigWatcherController{ - issuerSetter: issuerObserver, - opcInformer: opcInformer, - }, - }, - withInformer( - opcInformer, - pinnipedcontroller.NoOpFilter(), - controllerlib.InformerOption{}, - ), - ) -} - -// Sync implements controllerlib.Syncer. -func (c *dynamicConfigWatcherController) Sync(ctx controllerlib.Context) error { - // TODO Watch the configmap to find the issuer name, ingress url, etc. - // TODO Update some kind of in-memory representation of the configuration so the discovery endpoint can use it. - // TODO The discovery endpoint would return an error until all missing configuration options are - // filled in. - - opc, err := c.opcInformer. - Lister(). - OIDCProviderConfigs(ctx.Key.Namespace). - Get(ctx.Key.Name) - notFound := k8serrors.IsNotFound(err) - if err != nil && !notFound { - return fmt.Errorf("failed to get %s/%s oidcproviderconfig: %w", ctx.Key.Namespace, ctx.Key.Name, err) - } - - if notFound { - klog.InfoS( - "dynamicConfigWatcherController Sync found no oidcproviderconfig", - "oidcproviderconfig", - klog.KRef(ctx.Key.Namespace, ctx.Key.Name), - ) - if err := c.issuerSetter.SetIssuer(nil); err != nil { - klog.InfoS( - "dynamicConfigWatcherController Sync failed to set issuer", - "err", - err, - ) - } - return nil - } - - url, err := url.Parse(opc.Spec.Issuer) - if err != nil { - klog.InfoS( - "dynamicConfigWatcherController Sync failed to parse issuer", - "err", - err, - ) - return nil - } - - klog.InfoS( - "dynamicConfigWatcherController Sync issuer", - "oidcproviderconfig", - klog.KObj(opc), - "issuer", - url, - ) - if err := c.issuerSetter.SetIssuer(url); err != nil { - klog.InfoS( - "dynamicConfigWatcherController Sync failed to set issuer", - "err", - err, - ) - } - - return nil -} diff --git a/internal/controller/supervisorconfig/oidcproviderconfig_watcher.go b/internal/controller/supervisorconfig/oidcproviderconfig_watcher.go new file mode 100644 index 00000000..204600ab --- /dev/null +++ b/internal/controller/supervisorconfig/oidcproviderconfig_watcher.go @@ -0,0 +1,91 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package supervisorconfig + +import ( + "net/url" + + "k8s.io/apimachinery/pkg/labels" + "k8s.io/klog/v2" + + configinformers "go.pinniped.dev/generated/1.19/client/informers/externalversions/config/v1alpha1" + pinnipedcontroller "go.pinniped.dev/internal/controller" + "go.pinniped.dev/internal/controllerlib" + "go.pinniped.dev/internal/oidc/provider" +) + +// ProvidersSetter can be notified of all known valid providers with its SetIssuer function. +// 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 ProvidersSetter interface { + SetProviders(oidcProviders ...*provider.OIDCProvider) +} + +type oidcProviderConfigWatcherController struct { + providerSetter ProvidersSetter + opcInformer configinformers.OIDCProviderConfigInformer +} + +// NewOIDCProviderConfigWatcherController creates a controllerlib.Controller that watches +// OIDCProviderConfig objects and notifies a callback object of the collection of provider configs. +func NewOIDCProviderConfigWatcherController( + issuerObserver ProvidersSetter, + opcInformer configinformers.OIDCProviderConfigInformer, + withInformer pinnipedcontroller.WithInformerOptionFunc, +) controllerlib.Controller { + return controllerlib.New( + controllerlib.Config{ + Name: "OIDCProviderConfigWatcherController", + Syncer: &oidcProviderConfigWatcherController{ + providerSetter: issuerObserver, + opcInformer: opcInformer, + }, + }, + withInformer( + opcInformer, + pinnipedcontroller.NoOpFilter(), + controllerlib.InformerOption{}, + ), + ) +} + +// Sync implements controllerlib.Syncer. +func (c *oidcProviderConfigWatcherController) Sync(ctx controllerlib.Context) error { + all, err := c.opcInformer.Lister().List(labels.Everything()) + if err != nil { + return err + } + + oidcProviders := make([]*provider.OIDCProvider, 0) + for _, opc := range all { + issuerURL, err := url.Parse(opc.Spec.Issuer) + if err != nil { + klog.InfoS( + "OIDCProviderConfigWatcherController Sync failed to parse issuer", + "err", + err, + ) + continue + } + oidcProvider := &provider.OIDCProvider{Issuer: issuerURL} + err = oidcProvider.Validate() + if err != nil { + klog.InfoS( + "OIDCProviderConfigWatcherController Sync could failed to validate OIDCProviderConfig", + "err", + err, + ) + continue + } + oidcProviders = append(oidcProviders, oidcProvider) + klog.InfoS( + "OIDCProviderConfigWatcherController Sync accepted OIDCProviderConfig", + "issuer", + issuerURL, + ) + } + + c.providerSetter.SetProviders(oidcProviders...) + return nil +} diff --git a/internal/oidc/discovery/discovery.go b/internal/oidc/discovery/discovery.go index 2293e963..84ca1c77 100644 --- a/internal/oidc/discovery/discovery.go +++ b/internal/oidc/discovery/discovery.go @@ -8,16 +8,13 @@ import ( "encoding/json" "fmt" "net/http" - "net/url" - - "go.pinniped.dev/internal/oidc" ) // Metadata holds all fields (that we care about) from the OpenID Provider Metadata section in the // OpenID Connect Discovery specification: // https://openid.net/specs/openid-connect-discovery-1_0.html#rfc.section.3. type Metadata struct { - // vvvRequiredvvv + // vvv Required vvv Issuer string `json:"issuer"` @@ -29,44 +26,28 @@ type Metadata struct { SubjectTypesSupported []string `json:"subject_types_supported"` IDTokenSigningAlgValuesSupported []string `json:"id_token_signing_alg_values_supported"` - // ^^^Required^^^ + // ^^^ Required ^^^ - // vvvOptionalvvv + // vvv Optional vvv TokenEndpointAuthMethodsSupported []string `json:"token_endpoint_auth_methods_supported"` TokenEndpointAuthSigningAlgoValuesSupported []string `json:"token_endpoint_auth_signing_alg_values_supported"` ScopesSupported []string `json:"scopes_supported"` ClaimsSupported []string `json:"claims_supported"` - // ^^^Optional^^^ + // ^^^ Optional ^^^ } -// IssuerGetter holds onto an issuer which can be retrieved via its GetIssuer function. If there is -// no valid issuer, then nil will be returned. -// -// Implementations of this type should be thread-safe to support calls from multiple goroutines. -type IssuerGetter interface { - GetIssuer() *url.URL -} - -// New returns an http.Handler that will use information from the provided IssuerGetter to serve an -// OIDC discovery endpoint. -func New(ig IssuerGetter) http.Handler { +// New returns an http.Handler that serves an OIDC discovery endpoint. +func New(issuerURL string) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") - issuer := ig.GetIssuer() - if issuer == nil || r.URL.Path != issuer.Path+oidc.WellKnownURLPath { - http.Error(w, `{"error": "OIDC discovery not available (unknown issuer)"}`, http.StatusNotFound) - return - } - if r.Method != http.MethodGet { http.Error(w, `{"error": "Method not allowed (try GET)"}`, http.StatusMethodNotAllowed) return } - issuerURL := issuer.String() oidcConfig := Metadata{ Issuer: issuerURL, AuthorizationEndpoint: fmt.Sprintf("%s/oauth2/v0/auth", issuerURL), diff --git a/internal/oidc/discovery/discovery_test.go b/internal/oidc/discovery/discovery_test.go index 49f46beb..14f2d9b6 100644 --- a/internal/oidc/discovery/discovery_test.go +++ b/internal/oidc/discovery/discovery_test.go @@ -7,20 +7,18 @@ import ( "encoding/json" "net/http" "net/http/httptest" - "net/url" "testing" "github.com/stretchr/testify/require" "go.pinniped.dev/internal/oidc" - "go.pinniped.dev/internal/oidc/issuerprovider" ) func TestDiscovery(t *testing.T) { tests := []struct { name string - issuer *url.URL + issuer string method string path string @@ -29,37 +27,8 @@ func TestDiscovery(t *testing.T) { wantBody interface{} }{ { - name: "nil issuer", - method: http.MethodGet, - path: oidc.WellKnownURLPath, - wantStatus: http.StatusNotFound, - wantBody: map[string]string{ - "error": "OIDC discovery not available (unknown issuer)", - }, - }, - { - name: "root path mismatch", - issuer: must(url.Parse("https://some-issuer.com/some/path")), - method: http.MethodGet, - path: "/some/other/path" + oidc.WellKnownURLPath, - wantStatus: http.StatusNotFound, - wantBody: map[string]string{ - "error": "OIDC discovery not available (unknown issuer)", - }, - }, - { - name: "well-known path mismatch", - issuer: must(url.Parse("https://some-issuer.com/some/path")), - method: http.MethodGet, - path: "/some/path/that/is/not/the/well-known/path", - wantStatus: http.StatusNotFound, - wantBody: map[string]string{ - "error": "OIDC discovery not available (unknown issuer)", - }, - }, - { - name: "issuer path matches", - issuer: must(url.Parse("https://some-issuer.com/some/path")), + name: "happy path", + issuer: "https://some-issuer.com/some/path", method: http.MethodGet, path: "/some/path" + oidc.WellKnownURLPath, wantStatus: http.StatusOK, @@ -80,7 +49,7 @@ func TestDiscovery(t *testing.T) { }, { name: "bad method", - issuer: must(url.Parse("https://some-issuer.com")), + issuer: "https://some-issuer.com", method: http.MethodPost, path: oidc.WellKnownURLPath, wantStatus: http.StatusMethodNotAllowed, @@ -92,11 +61,7 @@ func TestDiscovery(t *testing.T) { for _, test := range tests { test := test t.Run(test.name, func(t *testing.T) { - p := issuerprovider.New() - err := p.SetIssuer(test.issuer) - require.NoError(t, err) - - handler := New(p) + handler := New(test.issuer) req := httptest.NewRequest(test.method, test.path, nil) rsp := httptest.NewRecorder() handler.ServeHTTP(rsp, req) @@ -115,10 +80,3 @@ func TestDiscovery(t *testing.T) { }) } } - -func must(u *url.URL, err error) *url.URL { - if err != nil { - panic(err) - } - return u -} diff --git a/internal/oidc/issuerprovider/issuerprovider.go b/internal/oidc/issuerprovider/issuerprovider.go deleted file mode 100644 index 36ec8c59..00000000 --- a/internal/oidc/issuerprovider/issuerprovider.go +++ /dev/null @@ -1,80 +0,0 @@ -// Copyright 2020 the Pinniped contributors. All Rights Reserved. -// SPDX-License-Identifier: Apache-2.0 - -// Package issuerprovider provides a thread-safe type that can hold on to an OIDC issuer name. -package issuerprovider - -import ( - "net/url" - "strings" - "sync" - - "go.pinniped.dev/internal/constable" -) - -// Provider is a type that can hold onto an issuer value, which may be nil. -// -// It is thread-safe. -type Provider struct { - mu sync.RWMutex - issuer *url.URL -} - -// New returns an empty Provider, i.e., one that holds a nil issuer. -func New() *Provider { - return &Provider{} -} - -// SetIssuer validates and sets the provided issuer. If validation fails, SetIssuer will return -// an error. -func (p *Provider) SetIssuer(issuer *url.URL) error { - if err := p.validateIssuer(issuer); err != nil { - return err - } - p.setIssuer(issuer) - return nil -} - -func (p *Provider) validateIssuer(issuer *url.URL) error { - if issuer == nil { - return nil - } - - if issuer.Scheme != "https" && removeMeAfterWeNoLongerNeedHTTPIssuerSupport(issuer.Scheme) { - return constable.Error(`issuer must have "https" scheme`) - } - - if issuer.User != nil { - return constable.Error(`issuer must not have username or password`) - } - - if strings.HasSuffix(issuer.Path, "/") { - return constable.Error(`issuer must not have trailing slash in path`) - } - - if issuer.RawQuery != "" { - return constable.Error(`issuer must not have query`) - } - - if issuer.Fragment != "" { - return constable.Error(`issuer must not have fragment`) - } - - return nil -} - -func (p *Provider) setIssuer(issuer *url.URL) { - p.mu.Lock() - defer p.mu.Unlock() - p.issuer = issuer -} - -func (p *Provider) GetIssuer() *url.URL { - p.mu.RLock() - defer p.mu.RUnlock() - return p.issuer -} - -func removeMeAfterWeNoLongerNeedHTTPIssuerSupport(scheme string) bool { - return scheme != "http" -} diff --git a/internal/oidc/provider/manager.go b/internal/oidc/provider/manager.go new file mode 100644 index 00000000..a50a7095 --- /dev/null +++ b/internal/oidc/provider/manager.go @@ -0,0 +1,115 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package provider + +import ( + "net/http" + "net/url" + "strings" + "sync" + + "k8s.io/klog/v2" + + "go.pinniped.dev/internal/oidc" + "go.pinniped.dev/internal/oidc/discovery" +) + +// Manager can manage multiple active OIDC providers. It acts as a request router for them. +// +// It is thread-safe. +type Manager struct { + mu sync.RWMutex + providerHandlers map[string]*providerHandler // map of issuer name to providerHandler + nextHandler http.Handler // the next handler in a chain, called when this manager didn't know how to handle a request +} + +// New returns an empty Manager. +// nextHandler will be invoked for any requests that could not be handled by this manager's providers. +func NewManager(nextHandler http.Handler) *Manager { + return &Manager{providerHandlers: make(map[string]*providerHandler), nextHandler: nextHandler} +} + +type providerHandler struct { + provider *OIDCProvider + discoveryHandler http.Handler +} + +func (h *providerHandler) Issuer() *url.URL { + return h.provider.Issuer +} + +// SetProviders adds or updates all the given providerHandlers using each provider's issuer string +// as the name of the provider to decide if it is an add or update operation. +// +// It also removes any providerHandlers that were previously added but were not passed in to +// the current invocation. +// +// This method assumes that all of the OIDCProvider arguments have already been validated +// by someone else before they are passed to this method. +func (c *Manager) SetProviders(oidcProviders ...*OIDCProvider) { + c.mu.Lock() + defer c.mu.Unlock() + // Add all of the incoming providers. + for _, incomingProvider := range oidcProviders { + issuerString := incomingProvider.Issuer.String() + c.providerHandlers[issuerString] = &providerHandler{ + provider: incomingProvider, + discoveryHandler: discovery.New(issuerString), + } + klog.InfoS("oidc provider manager added or updated issuer", "issuer", issuerString) + } + // Remove any providers that we previously handled but no longer exist. + for issuerKey := range c.providerHandlers { + if !findIssuerInListOfProviders(issuerKey, oidcProviders) { + delete(c.providerHandlers, issuerKey) + klog.InfoS("oidc provider manager removed issuer", "issuer", issuerKey) + } + } +} + +// ServeHTTP implements the http.Handler interface. +func (c *Manager) ServeHTTP(resp http.ResponseWriter, req *http.Request) { + providerHandler := c.findProviderHandlerByIssuerURL(req.Host, req.URL.Path) + if providerHandler != nil { + if req.URL.Path == providerHandler.Issuer().Path+oidc.WellKnownURLPath { + providerHandler.discoveryHandler.ServeHTTP(resp, req) + return // handled! + } + klog.InfoS( + "oidc provider manager found issuer but could not handle request", + "method", req.Method, + "host", req.Host, + "path", req.URL.Path, + ) + } else { + klog.InfoS( + "oidc provider manager could not find issuer to handle request", + "method", req.Method, + "host", req.Host, + "path", req.URL.Path, + ) + } + // Didn't know how to handle this request, so send it along the chain for further processing. + c.nextHandler.ServeHTTP(resp, req) +} + +func (c *Manager) findProviderHandlerByIssuerURL(host, path string) *providerHandler { + for _, providerHandler := range c.providerHandlers { + pi := providerHandler.Issuer() + // TODO do we need to compare scheme? not sure how to get it from the http.Request object + if host == pi.Host && strings.HasPrefix(path, pi.Path) { // TODO probably need better logic here? also maybe needs some of the logic from inside ServeMux + return providerHandler + } + } + return nil +} + +func findIssuerInListOfProviders(issuer string, oidcProviders []*OIDCProvider) bool { + for _, provider := range oidcProviders { + if provider.Issuer.String() == issuer { + return true + } + } + return false +} diff --git a/internal/oidc/provider/oidcprovider.go b/internal/oidc/provider/oidcprovider.go new file mode 100644 index 00000000..c7c24de4 --- /dev/null +++ b/internal/oidc/provider/oidcprovider.go @@ -0,0 +1,50 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package provider + +import ( + "net/url" + "strings" + + "go.pinniped.dev/internal/constable" +) + +// OIDCProvider represents all of the settings and state for an OIDC provider. +type OIDCProvider struct { + Issuer *url.URL +} + +// Validate returns an error if there is anything wrong with the provider settings, or +// returns nil if there is nothing wrong with the settings. +func (p *OIDCProvider) Validate() error { + if p.Issuer == nil { + return constable.Error(`provider must have an issuer`) + } + + if p.Issuer.Scheme != "https" && p.removeMeAfterWeNoLongerNeedHTTPIssuerSupport(p.Issuer.Scheme) { + return constable.Error(`issuer must have "https" scheme`) + } + + if p.Issuer.User != nil { + return constable.Error(`issuer must not have username or password`) + } + + if strings.HasSuffix(p.Issuer.Path, "/") { + return constable.Error(`issuer must not have trailing slash in path`) + } + + if p.Issuer.RawQuery != "" { + return constable.Error(`issuer must not have query`) + } + + if p.Issuer.Fragment != "" { + return constable.Error(`issuer must not have fragment`) + } + + return nil +} + +func (p *OIDCProvider) removeMeAfterWeNoLongerNeedHTTPIssuerSupport(scheme string) bool { + return scheme != "http" +} diff --git a/internal/oidc/issuerprovider/issuerprovider_test.go b/internal/oidc/provider/oidcprovider_test.go similarity index 88% rename from internal/oidc/issuerprovider/issuerprovider_test.go rename to internal/oidc/provider/oidcprovider_test.go index 128502f4..2da0e3a8 100644 --- a/internal/oidc/issuerprovider/issuerprovider_test.go +++ b/internal/oidc/provider/oidcprovider_test.go @@ -1,7 +1,7 @@ // Copyright 2020 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 -package issuerprovider +package provider import ( "net/url" @@ -10,15 +10,16 @@ import ( "github.com/stretchr/testify/require" ) -func TestProvider(t *testing.T) { +func TestOIDCProviderValidations(t *testing.T) { tests := []struct { name string issuer *url.URL wantError string }{ { - name: "nil issuer", - issuer: nil, + name: "provider must have an issuer", + issuer: nil, + wantError: "provider must have an issuer", }, { name: "no scheme", @@ -67,14 +68,12 @@ func TestProvider(t *testing.T) { for _, tt := range tests { tt := tt t.Run(tt.name, func(t *testing.T) { - p := New() - err := p.SetIssuer(tt.issuer) + p := OIDCProvider{Issuer: tt.issuer} + err := p.Validate() if tt.wantError != "" { require.EqualError(t, err, tt.wantError) - require.Nil(t, p.GetIssuer()) } else { require.NoError(t, err) - require.Equal(t, tt.issuer, p.GetIssuer()) } }) } diff --git a/test/integration/supervisor_discovery_test.go b/test/integration/supervisor_discovery_test.go index d2e0bca2..148e1878 100644 --- a/test/integration/supervisor_discovery_test.go +++ b/test/integration/supervisor_discovery_test.go @@ -13,9 +13,11 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "go.pinniped.dev/generated/1.19/apis/config/v1alpha1" + pinnipedclientset "go.pinniped.dev/generated/1.19/client/clientset/versioned" "go.pinniped.dev/internal/here" "go.pinniped.dev/test/library" ) @@ -24,7 +26,6 @@ func TestSupervisorOIDCDiscovery(t *testing.T) { env := library.IntegrationEnv(t) client := library.NewPinnipedClientset(t) - httpClient := &http.Client{} ns := env.SupervisorNamespace ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) defer cancel() @@ -32,7 +33,6 @@ func TestSupervisorOIDCDiscovery(t *testing.T) { // Temporarily remove any existing OIDCProviderConfigs from the cluster so we can test from a clean slate. originalConfigList, err := client.ConfigV1alpha1().OIDCProviderConfigs(ns).List(ctx, metav1.ListOptions{}) require.NoError(t, err) - for _, config := range originalConfigList.Items { err := client.ConfigV1alpha1().OIDCProviderConfigs(ns).Delete(ctx, config.Name, metav1.DeleteOptions{}) require.NoError(t, err) @@ -52,51 +52,85 @@ func TestSupervisorOIDCDiscovery(t *testing.T) { }) // Test that there is no default discovery endpoint available when there are no OIDCProviderConfigs. + requireDiscoveryEndpointIsNotFound(t, fmt.Sprintf("http://%s", env.SupervisorAddress)) + + // Define several unique issuer strings. + issuer1 := fmt.Sprintf("http://%s/nested/issuer1", env.SupervisorAddress) + issuer2 := fmt.Sprintf("http://%s/nested/issuer2", env.SupervisorAddress) + issuer3 := fmt.Sprintf("http://%s/issuer3", env.SupervisorAddress) + issuer4 := fmt.Sprintf("http://%s/issuer4", env.SupervisorAddress) + + // When OIDCProviderConfig are created in sequence they each cause a discovery endpoint to appear only for as long as the OIDCProviderConfig exists. + createdOIDCProviderConfig1 := requireCreatingOIDCProviderConfigCausesWellKnownEndpointToAppear(t, client, ns, issuer1, "from-integration-test1") + requireDeletingOIDCProviderConfigCausesWellKnownEndpointToDisappear(t, createdOIDCProviderConfig1, client, ns, issuer1) + createdOIDCProviderConfig2 := requireCreatingOIDCProviderConfigCausesWellKnownEndpointToAppear(t, client, ns, issuer2, "from-integration-test2") + requireDeletingOIDCProviderConfigCausesWellKnownEndpointToDisappear(t, createdOIDCProviderConfig2, client, ns, issuer2) + + // When multiple OIDCProviderConfigs exist at the same time they each serve a unique discovery endpoint. + createdOIDCProviderConfig3 := requireCreatingOIDCProviderConfigCausesWellKnownEndpointToAppear(t, client, ns, issuer3, "from-integration-test3") + createdOIDCProviderConfig4 := requireCreatingOIDCProviderConfigCausesWellKnownEndpointToAppear(t, client, ns, issuer4, "from-integration-test4") + requireWellKnownEndpointIsWorking(t, issuer3) // discovery for issuer3 is still working after issuer4 started working + + // When they are deleted they stop serving discovery endpoints. + requireDeletingOIDCProviderConfigCausesWellKnownEndpointToDisappear(t, createdOIDCProviderConfig3, client, ns, issuer2) + requireDeletingOIDCProviderConfigCausesWellKnownEndpointToDisappear(t, createdOIDCProviderConfig4, client, ns, issuer2) +} + +func requireDiscoveryEndpointIsNotFound(t *testing.T, issuerName string) { + t.Helper() + httpClient := &http.Client{} + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) + defer cancel() + requestNonExistentPath, err := http.NewRequestWithContext( ctx, http.MethodGet, - fmt.Sprintf("http://%s/.well-known/openid-configuration", env.SupervisorAddress), + fmt.Sprintf("%s/.well-known/openid-configuration", issuerName), nil, ) + + var response *http.Response + assert.Eventually(t, func() bool { + response, err = httpClient.Do(requestNonExistentPath) //nolint:bodyclose + return err == nil && response.StatusCode == http.StatusNotFound + }, 10*time.Second, 200*time.Millisecond) require.NoError(t, err) - notFoundResponse, err := httpClient.Do(requestNonExistentPath) + require.Equal(t, http.StatusNotFound, response.StatusCode) + err = response.Body.Close() require.NoError(t, err) - require.Equal(t, 404, notFoundResponse.StatusCode) - err = notFoundResponse.Body.Close() +} + +func requireCreatingOIDCProviderConfigCausesWellKnownEndpointToAppear(t *testing.T, client pinnipedclientset.Interface, ns string, issuerName string, oidcProviderConfigName string) *v1alpha1.OIDCProviderConfig { + t.Helper() + newOIDCProviderConfig := createOIDCProviderConfig(t, oidcProviderConfigName, client, ns, issuerName) + requireWellKnownEndpointIsWorking(t, issuerName) + return newOIDCProviderConfig +} + +func requireDeletingOIDCProviderConfigCausesWellKnownEndpointToDisappear(t *testing.T, existingOIDCProviderConfig *v1alpha1.OIDCProviderConfig, client pinnipedclientset.Interface, ns string, issuerName string) { + t.Helper() + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) + defer cancel() + + // Delete the OIDCProviderConfig. + err := client.ConfigV1alpha1().OIDCProviderConfigs(ns).Delete(ctx, existingOIDCProviderConfig.Name, metav1.DeleteOptions{}) require.NoError(t, err) - // Create a new OIDCProviderConfig with a known issuer. - issuer := fmt.Sprintf("http://%s/nested/issuer", env.SupervisorAddress) - newOIDCProviderConfig := v1alpha1.OIDCProviderConfig{ - TypeMeta: metav1.TypeMeta{ - Kind: "OIDCProviderConfig", - APIVersion: v1alpha1.SchemeGroupVersion.String(), - }, - ObjectMeta: metav1.ObjectMeta{ - Name: "nested-issuser-config-from-integration-test", - Namespace: ns, - }, - Spec: v1alpha1.OIDCProviderConfigSpec{ - Issuer: issuer, - }, - } - _, err = client.ConfigV1alpha1().OIDCProviderConfigs(ns).Create(ctx, &newOIDCProviderConfig, metav1.CreateOptions{}) - require.NoError(t, err) + // Fetch that same discovery endpoint as before, but now it should not exist anymore. Give it some time for the endpoint to go away. + requireDiscoveryEndpointIsNotFound(t, issuerName) +} - // When this test has finished, clean up the new OIDCProviderConfig. - t.Cleanup(func() { - cleanupCtx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) - defer cancel() +func requireWellKnownEndpointIsWorking(t *testing.T, issuerName string) { + t.Helper() + httpClient := &http.Client{} + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) + defer cancel() - err = client.ConfigV1alpha1().OIDCProviderConfigs(ns).Delete(cleanupCtx, newOIDCProviderConfig.Name, metav1.DeleteOptions{}) - require.NoError(t, err) - }) - - // Define a request to the new discovery endpoint which should have been created for the above OIDCProviderConfig. + // Define a request to the new discovery endpoint which should have been created by an OIDCProviderConfig. requestDiscoveryEndpoint, err := http.NewRequestWithContext( ctx, http.MethodGet, - fmt.Sprintf("http://%s/nested/issuer/.well-known/openid-configuration", env.SupervisorAddress), + fmt.Sprintf("%s/.well-known/openid-configuration", issuerName), nil, ) require.NoError(t, err) @@ -104,7 +138,7 @@ func TestSupervisorOIDCDiscovery(t *testing.T) { // Fetch that discovery endpoint. Give it some time for the endpoint to come into existence. var response *http.Response assert.Eventually(t, func() bool { - response, err = httpClient.Do(requestDiscoveryEndpoint) //nolint:bodyclose // the body is closed below after it is read + response, err = httpClient.Do(requestDiscoveryEndpoint) //nolint:bodyclose return err == nil && response.StatusCode == http.StatusOK }, 10*time.Second, 200*time.Millisecond) require.NoError(t, err) @@ -129,8 +163,45 @@ func TestSupervisorOIDCDiscovery(t *testing.T) { "subject_types_supported": ["public"], "id_token_signing_alg_values_supported": ["RS256"] }`) - expectedJSON := fmt.Sprintf(expectedResultTemplate, issuer, issuer, issuer, issuer) + expectedJSON := fmt.Sprintf(expectedResultTemplate, issuerName, issuerName, issuerName, issuerName) require.Equal(t, "application/json", response.Header.Get("content-type")) require.JSONEq(t, expectedJSON, string(responseBody)) } + +func createOIDCProviderConfig(t *testing.T, oidcProviderConfigName string, client pinnipedclientset.Interface, ns string, issuerName string) *v1alpha1.OIDCProviderConfig { + t.Helper() + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) + defer cancel() + + newOIDCProviderConfig := v1alpha1.OIDCProviderConfig{ + TypeMeta: metav1.TypeMeta{ + Kind: "OIDCProviderConfig", + APIVersion: v1alpha1.SchemeGroupVersion.String(), + }, + ObjectMeta: metav1.ObjectMeta{ + Name: oidcProviderConfigName, + Namespace: ns, + }, + Spec: v1alpha1.OIDCProviderConfigSpec{ + Issuer: issuerName, + }, + } + createdOIDCProviderConfig, err := client.ConfigV1alpha1().OIDCProviderConfigs(ns).Create(ctx, &newOIDCProviderConfig, metav1.CreateOptions{}) + require.NoError(t, err) + + // When this test has finished, be sure to clean up the new OIDCProviderConfig. + t.Cleanup(func() { + cleanupCtx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) + defer cancel() + + err = client.ConfigV1alpha1().OIDCProviderConfigs(ns).Delete(cleanupCtx, newOIDCProviderConfig.Name, metav1.DeleteOptions{}) + notFound := k8serrors.IsNotFound(err) + // It's okay if it is not found, because it might have been deleted by another part of this test. + if !notFound { + require.NoError(t, err) + } + }) + + return createdOIDCProviderConfig +}