diff --git a/cmd/pinniped-supervisor/main.go b/cmd/pinniped-supervisor/main.go index d6fa97acd..6e0866338 100644 --- a/cmd/pinniped-supervisor/main.go +++ b/cmd/pinniped-supervisor/main.go @@ -6,22 +6,20 @@ package main import ( "context" "fmt" - "io/ioutil" "net" "net/http" "os" "os/signal" "time" - kubeinformers "k8s.io/client-go/informers" - "k8s.io/client-go/kubernetes" "k8s.io/client-go/pkg/version" "k8s.io/client-go/rest" restclient "k8s.io/client-go/rest" "k8s.io/component-base/logs" "k8s.io/klog/v2" - "sigs.k8s.io/yaml" + pinnipedclientset "go.pinniped.dev/generated/1.19/client/clientset/versioned" + pinnipedinformers "go.pinniped.dev/generated/1.19/client/informers/externalversions" "go.pinniped.dev/internal/controller/supervisorconfig" "go.pinniped.dev/internal/controllerlib" "go.pinniped.dev/internal/downward" @@ -69,63 +67,57 @@ func waitForSignal() os.Signal { func startControllers( ctx context.Context, issuerProvider *issuerprovider.Provider, - kubeClient kubernetes.Interface, - kubeInformers kubeinformers.SharedInformerFactory, - serverInstallationNamespace string, - staticConfig StaticConfig, + pinnipedInformers pinnipedinformers.SharedInformerFactory, ) { // Create controller manager. controllerManager := controllerlib. NewManager(). WithController( supervisorconfig.NewDynamicConfigWatcherController( - serverInstallationNamespace, - staticConfig.NamesConfig.DynamicConfigMap, issuerProvider, - kubeClient, - kubeInformers.Core().V1().ConfigMaps(), + pinnipedInformers.Config().V1alpha1().OIDCProviderConfigs(), controllerlib.WithInformer, ), singletonWorker, ) - kubeInformers.Start(ctx.Done()) + pinnipedInformers.Start(ctx.Done()) go controllerManager.Start(ctx) } -func newK8sClient() (kubernetes.Interface, error) { +func newPinnipedClient() (pinnipedclientset.Interface, error) { kubeConfig, err := restclient.InClusterConfig() if err != nil { return nil, fmt.Errorf("could not load in-cluster configuration: %w", err) } // Connect to the core Kubernetes API. - kubeClient, err := kubernetes.NewForConfig(kubeConfig) + pinnipedClient, err := pinnipedclientset.NewForConfig(kubeConfig) if err != nil { return nil, fmt.Errorf("could not load in-cluster configuration: %w", err) } - return kubeClient, nil + return pinnipedClient, nil } -func run(serverInstallationNamespace string, staticConfig StaticConfig) error { +func run(serverInstallationNamespace string) error { ctx, cancel := context.WithCancel(context.Background()) defer cancel() - kubeClient, err := newK8sClient() + pinnipedClient, err := newPinnipedClient() if err != nil { return fmt.Errorf("cannot create k8s client: %w", err) } - kubeInformers := kubeinformers.NewSharedInformerFactoryWithOptions( - kubeClient, + pinnipedInformers := pinnipedinformers.NewSharedInformerFactoryWithOptions( + pinnipedClient, defaultResyncInterval, - kubeinformers.WithNamespace(serverInstallationNamespace), + pinnipedinformers.WithNamespace(serverInstallationNamespace), ) issuerProvider := issuerprovider.New() - startControllers(ctx, issuerProvider, kubeClient, kubeInformers, serverInstallationNamespace, staticConfig) + startControllers(ctx, issuerProvider, pinnipedInformers) //nolint: gosec // Intentionally binding to all network interfaces. l, err := net.Listen("tcp", ":80") @@ -143,14 +135,6 @@ func run(serverInstallationNamespace string, staticConfig StaticConfig) error { return nil } -type StaticConfig struct { - NamesConfig NamesConfigSpec `json:"names"` -} - -type NamesConfigSpec struct { - DynamicConfigMap string `json:"dynamicConfigMap"` -} - func main() { logs.InitLogs() defer logs.FlushLogs() @@ -164,17 +148,7 @@ func main() { klog.Fatal(fmt.Errorf("could not read pod metadata: %w", err)) } - // Read static config. - data, err := ioutil.ReadFile(os.Args[2]) - if err != nil { - klog.Fatal(fmt.Errorf("read file: %w", err)) - } - var staticConfig StaticConfig - if err := yaml.Unmarshal(data, &staticConfig); err != nil { - klog.Fatal(fmt.Errorf("decode yaml: %w", err)) - } - - if err := run(podInfo.Namespace, staticConfig); err != nil { + if err := run(podInfo.Namespace); err != nil { klog.Fatal(err) } } diff --git a/deploy-supervisor/rbac.yaml b/deploy-supervisor/rbac.yaml index ecba850b2..a4b34e9a6 100644 --- a/deploy-supervisor/rbac.yaml +++ b/deploy-supervisor/rbac.yaml @@ -13,8 +13,8 @@ metadata: labels: app: #@ data.values.app_name rules: - - apiGroups: [""] - resources: [configmaps] + - apiGroups: [config.pinniped.dev] + resources: [oidcproviderconfigs] verbs: [get, list, watch] --- kind: RoleBinding diff --git a/internal/controller/supervisorconfig/dynamic_config_watcher.go b/internal/controller/supervisorconfig/dynamic_config_watcher.go index d1aa8e710..a060e4e99 100644 --- a/internal/controller/supervisorconfig/dynamic_config_watcher.go +++ b/internal/controller/supervisorconfig/dynamic_config_watcher.go @@ -5,12 +5,12 @@ package supervisorconfig import ( "fmt" + "net/url" k8serrors "k8s.io/apimachinery/pkg/api/errors" - corev1informers "k8s.io/client-go/informers/core/v1" - "k8s.io/client-go/kubernetes" "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" ) @@ -22,41 +22,36 @@ const ( // 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 *string) + SetIssuer(issuer *url.URL) error } type dynamicConfigWatcherController struct { - configMapName string - configMapNamespace string - issuerSetter IssuerSetter - k8sClient kubernetes.Interface - configMapInformer corev1informers.ConfigMapInformer + 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( - serverInstallationNamespace string, - configMapName string, issuerObserver IssuerSetter, - k8sClient kubernetes.Interface, - configMapInformer corev1informers.ConfigMapInformer, + opcInformer configinformers.OIDCProviderConfigInformer, withInformer pinnipedcontroller.WithInformerOptionFunc, ) controllerlib.Controller { return controllerlib.New( controllerlib.Config{ Name: "DynamicConfigWatcherController", Syncer: &dynamicConfigWatcherController{ - configMapNamespace: serverInstallationNamespace, - configMapName: configMapName, - issuerSetter: issuerObserver, - k8sClient: k8sClient, - configMapInformer: configMapInformer, + issuerSetter: issuerObserver, + opcInformer: opcInformer, }, }, withInformer( - configMapInformer, - pinnipedcontroller.NameAndNamespaceExactMatchFilterFactory(configMapName, serverInstallationNamespace), + opcInformer, + pinnipedcontroller.NoOpFilter(), controllerlib.InformerOption{}, ), ) @@ -69,44 +64,49 @@ func (c *dynamicConfigWatcherController) Sync(ctx controllerlib.Context) error { // TODO The discovery endpoint would return an error until all missing configuration options are // filled in. - configMap, err := c.configMapInformer. + opc, err := c.opcInformer. Lister(). - ConfigMaps(c.configMapNamespace). - Get(c.configMapName) + OIDCProviderConfigs(ctx.Key.Namespace). + Get(ctx.Key.Name) notFound := k8serrors.IsNotFound(err) if err != nil && !notFound { - return fmt.Errorf("failed to get %s/%s secret: %w", c.configMapNamespace, c.configMapName, err) + 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 configmap", - "configmap", - klog.KRef(c.configMapNamespace, c.configMapName), + "dynamicConfigWatcherController Sync found no oidcproviderconfig", + "oidcproviderconfig", + klog.KRef(ctx.Key.Namespace, ctx.Key.Name), ) c.issuerSetter.SetIssuer(nil) return nil } - issuer, ok := configMap.Data[issuerConfigMapKey] - if !ok { + url, err := url.Parse(opc.Spec.Issuer) + if err != nil { klog.InfoS( - "dynamicConfigWatcherController Sync found no issuer", - "configmap", - klog.KObj(configMap), + "dynamicConfigWatcherController Sync failed to parse issuer", + "err", + err, ) - c.issuerSetter.SetIssuer(nil) return nil } klog.InfoS( "dynamicConfigWatcherController Sync issuer", - "configmap", - klog.KObj(configMap), + "oidcproviderconfig", + klog.KObj(opc), "issuer", - issuer, + url, ) - c.issuerSetter.SetIssuer(&issuer) + if err := c.issuerSetter.SetIssuer(url); err != nil { + klog.InfoS( + "dynamicConfigWatcherController Sync failed to set issuer", + "err", + err, + ) + } return nil } diff --git a/internal/oidc/discovery/discovery.go b/internal/oidc/discovery/discovery.go index a65f8c8db..96f858f51 100644 --- a/internal/oidc/discovery/discovery.go +++ b/internal/oidc/discovery/discovery.go @@ -8,6 +8,7 @@ import ( "encoding/json" "fmt" "net/http" + "net/url" ) // Metadata holds all fields (that we care about) from the OpenID Provider Metadata section in the @@ -30,7 +31,7 @@ type Metadata struct { // // Implementations of this type should be thread-safe to support calls from multiple goroutines. type IssuerGetter interface { - GetIssuer() *string + GetIssuer() *url.URL } // New returns an http.Handler that will use information from the provided IssuerGetter to serve an @@ -39,22 +40,23 @@ func New(ig IssuerGetter) 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 { + 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 } - issuer := ig.GetIssuer() - if issuer == nil { - http.Error(w, `{"error": "OIDC discovery not available (unknown issuer)"}`, http.StatusServiceUnavailable) - return - } - + issuerURL := issuer.String() 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), + Issuer: issuerURL, + AuthorizationEndpoint: fmt.Sprintf("%s/oauth2/v0/auth", issuerURL), + TokenEndpoint: fmt.Sprintf("%s/oauth2/v0/token", issuerURL), + JWKSURL: fmt.Sprintf("%s/oauth2/v0/keys", issuerURL), ResponseTypesSupported: []string{}, SubjectTypesSupported: []string{}, IDTokenSigningAlgValuesSupported: []string{}, diff --git a/internal/oidc/discovery/discovery_test.go b/internal/oidc/discovery/discovery_test.go index b89aab977..47fab0701 100644 --- a/internal/oidc/discovery/discovery_test.go +++ b/internal/oidc/discovery/discovery_test.go @@ -7,6 +7,7 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "net/url" "testing" "github.com/stretchr/testify/require" @@ -18,7 +19,7 @@ func TestDiscovery(t *testing.T) { tests := []struct { name string - issuer string + issuer *url.URL method string wantStatus int @@ -26,16 +27,16 @@ func TestDiscovery(t *testing.T) { wantBody interface{} }{ { - name: "issuer returns nil issuer", + name: "nil issuer", method: http.MethodGet, - wantStatus: http.StatusServiceUnavailable, + wantStatus: http.StatusNotFound, wantBody: map[string]string{ "error": "OIDC discovery not available (unknown issuer)", }, }, { - name: "issuer returns non-nil issuer", - issuer: "https://some-issuer.com", + name: "issuer without path", + issuer: must(url.Parse("https://some-issuer.com")), method: http.MethodGet, wantStatus: http.StatusOK, wantContentType: "application/json", @@ -49,9 +50,25 @@ func TestDiscovery(t *testing.T) { IDTokenSigningAlgValuesSupported: []string{}, }, }, + { + name: "issuer with path", + issuer: must(url.Parse("https://some-issuer.com/some/path")), + method: http.MethodGet, + wantStatus: http.StatusOK, + wantContentType: "application/json", + wantBody: &Metadata{ + Issuer: "https://some-issuer.com/some/path", + AuthorizationEndpoint: "https://some-issuer.com/some/path/oauth2/v0/auth", + TokenEndpoint: "https://some-issuer.com/some/path/oauth2/v0/token", + JWKSURL: "https://some-issuer.com/some/path/oauth2/v0/keys", + ResponseTypesSupported: []string{}, + SubjectTypesSupported: []string{}, + IDTokenSigningAlgValuesSupported: []string{}, + }, + }, { name: "bad method", - issuer: "https://some-issuer.com", + issuer: must(url.Parse("https://some-issuer.com")), method: http.MethodPost, wantStatus: http.StatusMethodNotAllowed, wantBody: map[string]string{ @@ -63,11 +80,7 @@ func TestDiscovery(t *testing.T) { test := test t.Run(test.name, func(t *testing.T) { p := issuerprovider.New() - if test.issuer != "" { - p.SetIssuer(&test.issuer) - } else { - p.SetIssuer(nil) - } + p.SetIssuer(test.issuer) handler := New(p) req := httptest.NewRequest(test.method, "/this/path/shouldnt/matter", nil) @@ -88,3 +101,10 @@ 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 index 248258601..36ec8c59d 100644 --- a/internal/oidc/issuerprovider/issuerprovider.go +++ b/internal/oidc/issuerprovider/issuerprovider.go @@ -4,14 +4,20 @@ // Package issuerprovider provides a thread-safe type that can hold on to an OIDC issuer name. package issuerprovider -import "sync" +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 *string + issuer *url.URL } // New returns an empty Provider, i.e., one that holds a nil issuer. @@ -19,14 +25,56 @@ func New() *Provider { return &Provider{} } -func (p *Provider) SetIssuer(issuer *string) { +// 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() *string { +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/issuerprovider/issuerprovider_test.go b/internal/oidc/issuerprovider/issuerprovider_test.go new file mode 100644 index 000000000..356b338b5 --- /dev/null +++ b/internal/oidc/issuerprovider/issuerprovider_test.go @@ -0,0 +1,84 @@ +package issuerprovider + +import ( + "net/url" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestProvider(t *testing.T) { + tests := []struct { + name string + issuer *url.URL + wantError string + }{ + { + name: "nil issuer", + issuer: nil, + }, + { + name: "no scheme", + issuer: must(url.Parse("tuna.com")), + wantError: `issuer must have "https" scheme`, + }, + { + name: "bad scheme", + issuer: must(url.Parse("ftp://tuna.com")), + wantError: `issuer must have "https" scheme`, + }, + { + name: "fragment", + issuer: must(url.Parse("https://tuna.com/fish#some-frag")), + wantError: `issuer must not have fragment`, + }, + { + name: "query", + issuer: must(url.Parse("https://tuna.com?some=query")), + wantError: `issuer must not have query`, + }, + { + name: "username", + issuer: must(url.Parse("https://username@tuna.com")), + wantError: `issuer must not have username or password`, + }, + { + name: "password", + issuer: must(url.Parse("https://username:password@tuna.com")), + wantError: `issuer must not have username or password`, + }, + { + name: "without path", + issuer: must(url.Parse("https://tuna.com")), + }, + { + name: "with path", + issuer: must(url.Parse("https://tuna.com/fish/marlin")), + }, + { + name: "trailing slash in path", + issuer: must(url.Parse("https://tuna.com/")), + wantError: `issuer must not have trailing slash in path`, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + p := New() + err := p.SetIssuer(tt.issuer) + 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()) + } + }) + } +} + +func must(u *url.URL, err error) *url.URL { + if err != nil { + panic(err) + } + return u +} diff --git a/test/integration/supervisor_discovery_test.go b/test/integration/supervisor_discovery_test.go index 6c5cce780..d0456ce07 100644 --- a/test/integration/supervisor_discovery_test.go +++ b/test/integration/supervisor_discovery_test.go @@ -40,9 +40,13 @@ func TestSupervisorOIDCDiscovery(t *testing.T) { // When this test has finished, recreate any OIDCProviderConfigs that had existed on the cluster before this test. t.Cleanup(func() { + cleanupCtx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) + defer cancel() + for _, config := range originalConfigList.Items { thisConfig := config - _, err := client.ConfigV1alpha1().OIDCProviderConfigs(ns).Create(ctx, &thisConfig, metav1.CreateOptions{}) + thisConfig.ResourceVersion = "" // Get rid of resource version since we can't create an object with one. + _, err := client.ConfigV1alpha1().OIDCProviderConfigs(ns).Create(cleanupCtx, &thisConfig, metav1.CreateOptions{}) require.NoError(t, err) } }) @@ -64,6 +68,10 @@ func TestSupervisorOIDCDiscovery(t *testing.T) { // 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, @@ -77,7 +85,10 @@ func TestSupervisorOIDCDiscovery(t *testing.T) { // When this test has finished, clean up the new OIDCProviderConfig. t.Cleanup(func() { - err = client.ConfigV1alpha1().OIDCProviderConfigs(ns).Delete(ctx, newOIDCProviderConfig.Name, metav1.DeleteOptions{}) + cleanupCtx, 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) }) @@ -94,9 +105,11 @@ func TestSupervisorOIDCDiscovery(t *testing.T) { 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 - return err == nil + return err == nil && response.StatusCode == http.StatusOK }, 10*time.Second, 200*time.Millisecond) require.NoError(t, err) + require.Equal(t, http.StatusOK, response.StatusCode) + responseBody, err := ioutil.ReadAll(response.Body) require.NoError(t, err) err = response.Body.Close() @@ -116,7 +129,6 @@ func TestSupervisorOIDCDiscovery(t *testing.T) { }`) expectedJSON := fmt.Sprintf(expectedResultTemplate, issuer, issuer, issuer, issuer) - require.Equal(t, 200, response.StatusCode) require.Equal(t, "application/json", response.Header.Get("content-type")) require.JSONEq(t, expectedJSON, string(responseBody)) }