From fd6a7f589290dbd77ef2651aca7fa028a2ac0461 Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Tue, 6 Oct 2020 10:11:57 -0400 Subject: [PATCH] supervisor-oidc: hoist OIDC discovery handler for testing Signed-off-by: Andrew Keesler --- cmd/pinniped-supervisor/main.go | 30 +++---- .../dynamic_config_watcher.go | 72 +++++++++++++-- internal/oidc/discovery/discovery.go | 66 ++++++++++++++ internal/oidc/discovery/discovery_test.go | 90 +++++++++++++++++++ .../oidc/issuerprovider/issuerprovider.go | 32 +++++++ internal/oidc/oidc.go | 9 ++ 6 files changed, 275 insertions(+), 24 deletions(-) create mode 100644 internal/oidc/discovery/discovery.go create mode 100644 internal/oidc/discovery/discovery_test.go create mode 100644 internal/oidc/issuerprovider/issuerprovider.go create mode 100644 internal/oidc/oidc.go diff --git a/cmd/pinniped-supervisor/main.go b/cmd/pinniped-supervisor/main.go index c15ef7a1..d6fa97ac 100644 --- a/cmd/pinniped-supervisor/main.go +++ b/cmd/pinniped-supervisor/main.go @@ -25,6 +25,9 @@ import ( "go.pinniped.dev/internal/controller/supervisorconfig" "go.pinniped.dev/internal/controllerlib" "go.pinniped.dev/internal/downward" + "go.pinniped.dev/internal/oidc" + "go.pinniped.dev/internal/oidc/discovery" + "go.pinniped.dev/internal/oidc/issuerprovider" ) const ( @@ -32,11 +35,11 @@ const ( defaultResyncInterval = 3 * time.Minute ) -type helloWorld struct{} - -func (w *helloWorld) start(ctx context.Context, l net.Listener) error { +func start(ctx context.Context, l net.Listener, discoveryHandler http.Handler) { + mux := http.NewServeMux() + mux.Handle(oidc.WellKnownURLPath, discoveryHandler) server := http.Server{ - Handler: w, + Handler: mux, } errCh := make(chan error) @@ -55,14 +58,6 @@ func (w *helloWorld) start(ctx context.Context, l net.Listener) error { } } }() - - return nil -} - -func (w *helloWorld) ServeHTTP(rsp http.ResponseWriter, req *http.Request) { - // TODO this is just a placeholder to allow manually testing that this is reachable; we don't want a hello world endpoint - defer req.Body.Close() - _, _ = fmt.Fprintf(rsp, "Hello, world!") } func waitForSignal() os.Signal { @@ -73,6 +68,7 @@ func waitForSignal() os.Signal { func startControllers( ctx context.Context, + issuerProvider *issuerprovider.Provider, kubeClient kubernetes.Interface, kubeInformers kubeinformers.SharedInformerFactory, serverInstallationNamespace string, @@ -85,6 +81,7 @@ func startControllers( supervisorconfig.NewDynamicConfigWatcherController( serverInstallationNamespace, staticConfig.NamesConfig.DynamicConfigMap, + issuerProvider, kubeClient, kubeInformers.Core().V1().ConfigMaps(), controllerlib.WithInformer, @@ -127,7 +124,8 @@ func run(serverInstallationNamespace string, staticConfig StaticConfig) error { kubeinformers.WithNamespace(serverInstallationNamespace), ) - startControllers(ctx, kubeClient, kubeInformers, serverInstallationNamespace, staticConfig) + issuerProvider := issuerprovider.New() + startControllers(ctx, issuerProvider, kubeClient, kubeInformers, serverInstallationNamespace, staticConfig) //nolint: gosec // Intentionally binding to all network interfaces. l, err := net.Listen("tcp", ":80") @@ -136,11 +134,7 @@ func run(serverInstallationNamespace string, staticConfig StaticConfig) error { } defer l.Close() - helloHandler := &helloWorld{} - err = helloHandler.start(ctx, l) - if err != nil { - return fmt.Errorf("cannot start webhook: %w", err) - } + start(ctx, l, discovery.New(issuerProvider)) 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 index 10319a17..d1aa8e71 100644 --- a/internal/controller/supervisorconfig/dynamic_config_watcher.go +++ b/internal/controller/supervisorconfig/dynamic_config_watcher.go @@ -4,6 +4,9 @@ package supervisorconfig import ( + "fmt" + + k8serrors "k8s.io/apimachinery/pkg/api/errors" corev1informers "k8s.io/client-go/informers/core/v1" "k8s.io/client-go/kubernetes" "k8s.io/klog/v2" @@ -12,14 +15,30 @@ import ( "go.pinniped.dev/internal/controllerlib" ) +const ( + issuerConfigMapKey = "issuer" +) + +// 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. +// +// Implementations of this type should be thread-safe to support calls from multiple goroutines. +type IssuerSetter interface { + SetIssuer(issuer *string) +} + type dynamicConfigWatcherController struct { - k8sClient kubernetes.Interface - configMapInformer corev1informers.ConfigMapInformer + configMapName string + configMapNamespace string + issuerSetter IssuerSetter + k8sClient kubernetes.Interface + configMapInformer corev1informers.ConfigMapInformer } func NewDynamicConfigWatcherController( serverInstallationNamespace string, configMapName string, + issuerObserver IssuerSetter, k8sClient kubernetes.Interface, configMapInformer corev1informers.ConfigMapInformer, withInformer pinnipedcontroller.WithInformerOptionFunc, @@ -28,8 +47,11 @@ func NewDynamicConfigWatcherController( controllerlib.Config{ Name: "DynamicConfigWatcherController", Syncer: &dynamicConfigWatcherController{ - k8sClient: k8sClient, - configMapInformer: configMapInformer, + configMapNamespace: serverInstallationNamespace, + configMapName: configMapName, + issuerSetter: issuerObserver, + k8sClient: k8sClient, + configMapInformer: configMapInformer, }, }, withInformer( @@ -44,9 +66,47 @@ func NewDynamicConfigWatcherController( 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. + // TODO The discovery endpoint would return an error until all missing configuration options are + // filled in. - klog.InfoS("DynamicConfigWatcherController sync finished") + configMap, err := c.configMapInformer. + Lister(). + ConfigMaps(c.configMapNamespace). + Get(c.configMapName) + notFound := k8serrors.IsNotFound(err) + if err != nil && !notFound { + return fmt.Errorf("failed to get %s/%s secret: %w", c.configMapNamespace, c.configMapName, err) + } + + if notFound { + klog.InfoS( + "dynamicConfigWatcherController Sync found no configmap", + "configmap", + klog.KRef(c.configMapNamespace, c.configMapName), + ) + c.issuerSetter.SetIssuer(nil) + return nil + } + + issuer, ok := configMap.Data[issuerConfigMapKey] + if !ok { + klog.InfoS( + "dynamicConfigWatcherController Sync found no issuer", + "configmap", + klog.KObj(configMap), + ) + c.issuerSetter.SetIssuer(nil) + return nil + } + + klog.InfoS( + "dynamicConfigWatcherController Sync issuer", + "configmap", + klog.KObj(configMap), + "issuer", + issuer, + ) + c.issuerSetter.SetIssuer(&issuer) return nil } diff --git a/internal/oidc/discovery/discovery.go b/internal/oidc/discovery/discovery.go new file mode 100644 index 00000000..a65f8c8d --- /dev/null +++ b/internal/oidc/discovery/discovery.go @@ -0,0 +1,66 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +// Package discovery provides a handler for the OIDC discovery endpoint. +package discovery + +import ( + "encoding/json" + "fmt" + "net/http" +) + +// 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 { + Issuer string `json:"issuer"` + + AuthorizationEndpoint string `json:"authorization_endpoint"` + TokenEndpoint string `json:"token_endpoint"` + JWKSURL string `json:"jwks_url"` + + ResponseTypesSupported []string `json:"response_types_supported"` + SubjectTypesSupported []string `json:"subject_types_supported"` + IDTokenSigningAlgValuesSupported []string `json:"id_token_signing_alg_values_supported"` +} + +// 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() *string +} + +// 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 { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + + if r.Method != http.MethodGet { + http.Error(w, `{"error": "Method not allowed (try GET)"}`, http.StatusMethodNotAllowed) + return + } + + issuer := ig.GetIssuer() + if issuer == nil { + http.Error(w, `{"error": "OIDC discovery not available (unknown issuer)"}`, http.StatusServiceUnavailable) + return + } + + oidcConfig := Metadata{ + Issuer: *issuer, + AuthorizationEndpoint: fmt.Sprintf("%s/oauth2/v0/auth", *issuer), + TokenEndpoint: fmt.Sprintf("%s/oauth2/v0/token", *issuer), + JWKSURL: fmt.Sprintf("%s/oauth2/v0/keys", *issuer), + ResponseTypesSupported: []string{}, + SubjectTypesSupported: []string{}, + IDTokenSigningAlgValuesSupported: []string{}, + } + if err := json.NewEncoder(w).Encode(&oidcConfig); err != nil { + http.Error(w, err.Error(), http.StatusInternalServerError) + } + }) +} diff --git a/internal/oidc/discovery/discovery_test.go b/internal/oidc/discovery/discovery_test.go new file mode 100644 index 00000000..b89aab97 --- /dev/null +++ b/internal/oidc/discovery/discovery_test.go @@ -0,0 +1,90 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package discovery + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/require" + + "go.pinniped.dev/internal/oidc/issuerprovider" +) + +func TestDiscovery(t *testing.T) { + tests := []struct { + name string + + issuer string + method string + + wantStatus int + wantContentType string + wantBody interface{} + }{ + { + name: "issuer returns nil issuer", + method: http.MethodGet, + wantStatus: http.StatusServiceUnavailable, + wantBody: map[string]string{ + "error": "OIDC discovery not available (unknown issuer)", + }, + }, + { + name: "issuer returns non-nil issuer", + issuer: "https://some-issuer.com", + method: http.MethodGet, + wantStatus: http.StatusOK, + wantContentType: "application/json", + wantBody: &Metadata{ + Issuer: "https://some-issuer.com", + AuthorizationEndpoint: "https://some-issuer.com/oauth2/v0/auth", + TokenEndpoint: "https://some-issuer.com/oauth2/v0/token", + JWKSURL: "https://some-issuer.com/oauth2/v0/keys", + ResponseTypesSupported: []string{}, + SubjectTypesSupported: []string{}, + IDTokenSigningAlgValuesSupported: []string{}, + }, + }, + { + name: "bad method", + issuer: "https://some-issuer.com", + method: http.MethodPost, + wantStatus: http.StatusMethodNotAllowed, + wantBody: map[string]string{ + "error": "Method not allowed (try GET)", + }, + }, + } + for _, test := range tests { + test := test + t.Run(test.name, func(t *testing.T) { + p := issuerprovider.New() + if test.issuer != "" { + p.SetIssuer(&test.issuer) + } else { + p.SetIssuer(nil) + } + + handler := New(p) + req := httptest.NewRequest(test.method, "/this/path/shouldnt/matter", nil) + rsp := httptest.NewRecorder() + handler.ServeHTTP(rsp, req) + + require.Equal(t, test.wantStatus, rsp.Code) + + if test.wantContentType != "" { + require.Equal(t, test.wantContentType, rsp.Header().Get("Content-Type")) + } + + if test.wantBody != nil { + wantJSON, err := json.Marshal(test.wantBody) + require.NoError(t, err) + require.JSONEq(t, string(wantJSON), rsp.Body.String()) + } + }) + } +} diff --git a/internal/oidc/issuerprovider/issuerprovider.go b/internal/oidc/issuerprovider/issuerprovider.go new file mode 100644 index 00000000..24825860 --- /dev/null +++ b/internal/oidc/issuerprovider/issuerprovider.go @@ -0,0 +1,32 @@ +// 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 "sync" + +// 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 *string +} + +// New returns an empty Provider, i.e., one that holds a nil issuer. +func New() *Provider { + return &Provider{} +} + +func (p *Provider) SetIssuer(issuer *string) { + p.mu.Lock() + defer p.mu.Unlock() + p.issuer = issuer +} + +func (p *Provider) GetIssuer() *string { + p.mu.RLock() + defer p.mu.RUnlock() + return p.issuer +} diff --git a/internal/oidc/oidc.go b/internal/oidc/oidc.go new file mode 100644 index 00000000..795c3d38 --- /dev/null +++ b/internal/oidc/oidc.go @@ -0,0 +1,9 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +// Package oidc contains common OIDC functionality needed by Pinniped. +package oidc + +const ( + WellKnownURLPath = "/.well-known/openid-configuration" +)