diff --git a/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher.go b/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher.go index 144d2944..bc3db3bf 100644 --- a/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher.go +++ b/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher.go @@ -6,7 +6,11 @@ package upstreamwatcher import ( "context" + "crypto/tls" + "crypto/x509" + "encoding/base64" "fmt" + "net/http" "net/url" "sort" "time" @@ -48,10 +52,12 @@ const ( reasonMissingKeys = "SecretMissingKeys" reasonSuccess = "Success" reasonUnreachable = "Unreachable" + reasonInvalidTLSConfig = "InvalidTLSConfig" reasonInvalidResponse = "InvalidResponse" // Errors that are generated by our reconcile process. - errFailureStatus = constable.Error("UpstreamOIDCProvider has a failing condition") + errFailureStatus = constable.Error("UpstreamOIDCProvider has a failing condition") + errNoCertificates = constable.Error("no certificates found") ) // IDPCache is a thread safe cache that holds a list of validated upstream OIDC IDP configurations. @@ -59,13 +65,39 @@ type IDPCache interface { SetIDPList([]provider.UpstreamOIDCIdentityProvider) } +// lruValidatorCache caches the *oidc.Provider associated with a particular issuer/TLS configuration. +type lruValidatorCache struct{ cache *cache.Expiring } + +func (c *lruValidatorCache) getProvider(spec *v1alpha1.UpstreamOIDCProviderSpec) *oidc.Provider { + if result, ok := c.cache.Get(c.cacheKey(spec)); ok { + return result.(*oidc.Provider) + } + return nil +} + +func (c *lruValidatorCache) putProvider(spec *v1alpha1.UpstreamOIDCProviderSpec, provider *oidc.Provider) { + c.cache.Set(c.cacheKey(spec), provider, validatorCacheTTL) +} + +func (c *lruValidatorCache) cacheKey(spec *v1alpha1.UpstreamOIDCProviderSpec) interface{} { + var key struct{ issuer, caBundle string } + key.issuer = spec.Issuer + if spec.TLS != nil { + key.caBundle = spec.TLS.CertificateAuthorityData + } + return key +} + type controller struct { cache IDPCache log logr.Logger client pinnipedclientset.Interface providers idpinformers.UpstreamOIDCProviderInformer secrets corev1informers.SecretInformer - validatorCache *cache.Expiring + validatorCache interface { + getProvider(spec *v1alpha1.UpstreamOIDCProviderSpec) *oidc.Provider + putProvider(spec *v1alpha1.UpstreamOIDCProviderSpec, provider *oidc.Provider) + } } // New instantiates a new controllerlib.Controller which will populate the provided IDPCache. @@ -82,7 +114,7 @@ func New( client: client, providers: providers, secrets: secrets, - validatorCache: cache.NewExpiring(), + validatorCache: &lruValidatorCache{cache: cache.NewExpiring()}, } filter := pinnipedcontroller.MatchAnythingFilter(pinnipedcontroller.SingletonQueue()) return controllerlib.New( @@ -197,15 +229,22 @@ func (c *controller) validateSecret(upstream *v1alpha1.UpstreamOIDCProvider, res // validateIssuer validates the .spec.issuer field, performs OIDC discovery, and returns the appropriate OIDCDiscoverySucceeded condition. func (c *controller) validateIssuer(ctx context.Context, upstream *v1alpha1.UpstreamOIDCProvider, result *provider.UpstreamOIDCIdentityProvider) *v1alpha1.Condition { // Get the provider (from cache if possible). - var discoveredProvider *oidc.Provider - if cached, ok := c.validatorCache.Get(upstream.Spec.Issuer); ok { - discoveredProvider = cached.(*oidc.Provider) - } + discoveredProvider := c.validatorCache.getProvider(&upstream.Spec) // If the provider does not exist in the cache, do a fresh discovery lookup and save to the cache. if discoveredProvider == nil { - var err error - discoveredProvider, err = oidc.NewProvider(ctx, upstream.Spec.Issuer) + tlsConfig, err := getTLSConfig(upstream) + if err != nil { + return &v1alpha1.Condition{ + Type: typeOIDCDiscoverySucceeded, + Status: v1alpha1.ConditionFalse, + Reason: reasonInvalidTLSConfig, + Message: err.Error(), + } + } + httpClient := &http.Client{Transport: &http.Transport{TLSClientConfig: tlsConfig}} + + discoveredProvider, err = oidc.NewProvider(oidc.ClientContext(ctx, httpClient), upstream.Spec.Issuer) if err != nil { return &v1alpha1.Condition{ Type: typeOIDCDiscoverySucceeded, @@ -216,7 +255,7 @@ func (c *controller) validateIssuer(ctx context.Context, upstream *v1alpha1.Upst } // Update the cache with the newly discovered value. - c.validatorCache.Set(upstream.Spec.Issuer, discoveredProvider, validatorCacheTTL) + c.validatorCache.putProvider(&upstream.Spec, discoveredProvider) } // Parse out and validate the discovered authorize endpoint. @@ -248,6 +287,28 @@ func (c *controller) validateIssuer(ctx context.Context, upstream *v1alpha1.Upst } } +func getTLSConfig(upstream *v1alpha1.UpstreamOIDCProvider) (*tls.Config, error) { + result := tls.Config{ + MinVersion: tls.VersionTLS12, + } + + if upstream.Spec.TLS == nil || upstream.Spec.TLS.CertificateAuthorityData == "" { + return &result, nil + } + + bundle, err := base64.StdEncoding.DecodeString(upstream.Spec.TLS.CertificateAuthorityData) + if err != nil { + return nil, fmt.Errorf("spec.certificateAuthorityData is invalid: %w", err) + } + + result.RootCAs = x509.NewCertPool() + if !result.RootCAs.AppendCertsFromPEM(bundle) { + return nil, fmt.Errorf("spec.certificateAuthorityData is invalid: %w", errNoCertificates) + } + + return &result, nil +} + func (c *controller) updateStatus(ctx context.Context, upstream *v1alpha1.UpstreamOIDCProvider, conditions []*v1alpha1.Condition) { log := c.log.WithValues("namespace", upstream.Namespace, "name", upstream.Name) updated := upstream.DeepCopy() diff --git a/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher_test.go b/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher_test.go index 2e91a523..949effaf 100644 --- a/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher_test.go +++ b/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher_test.go @@ -5,9 +5,9 @@ package upstreamwatcher import ( "context" + "encoding/base64" "encoding/json" "net/http" - "net/http/httptest" "net/url" "strings" "testing" @@ -25,6 +25,7 @@ import ( pinnipedinformers "go.pinniped.dev/generated/1.19/client/supervisor/informers/externalversions" "go.pinniped.dev/internal/controllerlib" "go.pinniped.dev/internal/oidc/provider" + "go.pinniped.dev/internal/testutil" "go.pinniped.dev/internal/testutil/testlogger" ) @@ -34,7 +35,8 @@ func TestController(t *testing.T) { earlier := metav1.NewTime(now.Add(-1 * time.Hour).UTC()) // Start another test server that answers discovery successfully. - testIssuer := newTestIssuer(t) + testIssuerCA, testIssuerURL := newTestIssuer(t) + testIssuerCABase64 := base64.StdEncoding.EncodeToString([]byte(testIssuerCA)) testIssuerAuthorizeURL, err := url.Parse("https://example.com/authorize") require.NoError(t, err) @@ -65,7 +67,8 @@ func TestController(t *testing.T) { inputUpstreams: []runtime.Object{&v1alpha1.UpstreamOIDCProvider{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, Spec: v1alpha1.UpstreamOIDCProviderSpec{ - Issuer: testIssuer.URL, + Issuer: testIssuerURL, + TLS: &v1alpha1.TLSSpec{CertificateAuthorityData: testIssuerCABase64}, Client: v1alpha1.OIDCClient{SecretName: testSecretName}, AuthorizationConfig: v1alpha1.OIDCAuthorizationConfig{AdditionalScopes: testAdditionalScopes}, }, @@ -106,7 +109,8 @@ func TestController(t *testing.T) { inputUpstreams: []runtime.Object{&v1alpha1.UpstreamOIDCProvider{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, Spec: v1alpha1.UpstreamOIDCProviderSpec{ - Issuer: testIssuer.URL, + Issuer: testIssuerURL, + TLS: &v1alpha1.TLSSpec{CertificateAuthorityData: testIssuerCABase64}, Client: v1alpha1.OIDCClient{SecretName: testSecretName}, AuthorizationConfig: v1alpha1.OIDCAuthorizationConfig{AdditionalScopes: testAdditionalScopes}, }, @@ -151,7 +155,8 @@ func TestController(t *testing.T) { inputUpstreams: []runtime.Object{&v1alpha1.UpstreamOIDCProvider{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, Spec: v1alpha1.UpstreamOIDCProviderSpec{ - Issuer: testIssuer.URL, + Issuer: testIssuerURL, + TLS: &v1alpha1.TLSSpec{CertificateAuthorityData: testIssuerCABase64}, Client: v1alpha1.OIDCClient{SecretName: testSecretName}, AuthorizationConfig: v1alpha1.OIDCAuthorizationConfig{AdditionalScopes: testAdditionalScopes}, }, @@ -190,6 +195,102 @@ func TestController(t *testing.T) { }, }}, }, + { + name: "TLS CA bundle is invalid base64", + inputUpstreams: []runtime.Object{&v1alpha1.UpstreamOIDCProvider{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: "test-name"}, + Spec: v1alpha1.UpstreamOIDCProviderSpec{ + Issuer: testIssuerURL, + TLS: &v1alpha1.TLSSpec{ + CertificateAuthorityData: "invalid-base64", + }, + Client: v1alpha1.OIDCClient{SecretName: testSecretName}, + AuthorizationConfig: v1alpha1.OIDCAuthorizationConfig{AdditionalScopes: append(testAdditionalScopes, "xyz", "openid")}, + }, + }}, + inputSecrets: []runtime.Object{&corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testSecretName}, + Type: "secrets.pinniped.dev/oidc-client", + Data: testValidSecretData, + }}, + wantErr: controllerlib.ErrSyntheticRequeue.Error(), + wantLogs: []string{ + `upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, + `upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="spec.certificateAuthorityData is invalid: illegal base64 data at input byte 7" "reason"="InvalidTLSConfig" "status"="False" "type"="OIDCDiscoverySucceeded"`, + `upstream-observer "error"="UpstreamOIDCProvider has a failing condition" "msg"="found failing condition" "message"="spec.certificateAuthorityData is invalid: illegal base64 data at input byte 7" "name"="test-name" "namespace"="test-namespace" "reason"="InvalidTLSConfig" "type"="OIDCDiscoverySucceeded"`, + }, + wantResultingCache: []provider.UpstreamOIDCIdentityProvider{}, + wantResultingUpstreams: []v1alpha1.UpstreamOIDCProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, + Status: v1alpha1.UpstreamOIDCProviderStatus{ + Phase: "Error", + Conditions: []v1alpha1.Condition{ + { + Type: "ClientCredentialsValid", + Status: "True", + LastTransitionTime: now, + Reason: "Success", + Message: "loaded client credentials", + }, + { + Type: "OIDCDiscoverySucceeded", + Status: "False", + LastTransitionTime: now, + Reason: "InvalidTLSConfig", + Message: `spec.certificateAuthorityData is invalid: illegal base64 data at input byte 7`, + }, + }, + }, + }}, + }, + { + name: "TLS CA bundle does not have any certificates", + inputUpstreams: []runtime.Object{&v1alpha1.UpstreamOIDCProvider{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: "test-name"}, + Spec: v1alpha1.UpstreamOIDCProviderSpec{ + Issuer: testIssuerURL, + TLS: &v1alpha1.TLSSpec{ + CertificateAuthorityData: base64.StdEncoding.EncodeToString([]byte("not-a-pem-ca-bundle")), + }, + Client: v1alpha1.OIDCClient{SecretName: testSecretName}, + AuthorizationConfig: v1alpha1.OIDCAuthorizationConfig{AdditionalScopes: append(testAdditionalScopes, "xyz", "openid")}, + }, + }}, + inputSecrets: []runtime.Object{&corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testSecretName}, + Type: "secrets.pinniped.dev/oidc-client", + Data: testValidSecretData, + }}, + wantErr: controllerlib.ErrSyntheticRequeue.Error(), + wantLogs: []string{ + `upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="loaded client credentials" "reason"="Success" "status"="True" "type"="ClientCredentialsValid"`, + `upstream-observer "level"=0 "msg"="updated condition" "name"="test-name" "namespace"="test-namespace" "message"="spec.certificateAuthorityData is invalid: no certificates found" "reason"="InvalidTLSConfig" "status"="False" "type"="OIDCDiscoverySucceeded"`, + `upstream-observer "error"="UpstreamOIDCProvider has a failing condition" "msg"="found failing condition" "message"="spec.certificateAuthorityData is invalid: no certificates found" "name"="test-name" "namespace"="test-namespace" "reason"="InvalidTLSConfig" "type"="OIDCDiscoverySucceeded"`, + }, + wantResultingCache: []provider.UpstreamOIDCIdentityProvider{}, + wantResultingUpstreams: []v1alpha1.UpstreamOIDCProvider{{ + ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, + Status: v1alpha1.UpstreamOIDCProviderStatus{ + Phase: "Error", + Conditions: []v1alpha1.Condition{ + { + Type: "ClientCredentialsValid", + Status: "True", + LastTransitionTime: now, + Reason: "Success", + Message: "loaded client credentials", + }, + { + Type: "OIDCDiscoverySucceeded", + Status: "False", + LastTransitionTime: now, + Reason: "InvalidTLSConfig", + Message: `spec.certificateAuthorityData is invalid: no certificates found`, + }, + }, + }, + }}, + }, { name: "issuer is invalid URL", inputUpstreams: []runtime.Object{&v1alpha1.UpstreamOIDCProvider{ @@ -240,7 +341,8 @@ func TestController(t *testing.T) { inputUpstreams: []runtime.Object{&v1alpha1.UpstreamOIDCProvider{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, Spec: v1alpha1.UpstreamOIDCProviderSpec{ - Issuer: testIssuer.URL + "/invalid", + Issuer: testIssuerURL + "/invalid", + TLS: &v1alpha1.TLSSpec{CertificateAuthorityData: testIssuerCABase64}, Client: v1alpha1.OIDCClient{SecretName: testSecretName}, AuthorizationConfig: v1alpha1.OIDCAuthorizationConfig{AdditionalScopes: testAdditionalScopes}, }, @@ -285,7 +387,8 @@ func TestController(t *testing.T) { inputUpstreams: []runtime.Object{&v1alpha1.UpstreamOIDCProvider{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName}, Spec: v1alpha1.UpstreamOIDCProviderSpec{ - Issuer: testIssuer.URL + "/insecure", + Issuer: testIssuerURL + "/insecure", + TLS: &v1alpha1.TLSSpec{CertificateAuthorityData: testIssuerCABase64}, Client: v1alpha1.OIDCClient{SecretName: testSecretName}, AuthorizationConfig: v1alpha1.OIDCAuthorizationConfig{AdditionalScopes: testAdditionalScopes}, }, @@ -330,7 +433,8 @@ func TestController(t *testing.T) { inputUpstreams: []runtime.Object{&v1alpha1.UpstreamOIDCProvider{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: "test-name"}, Spec: v1alpha1.UpstreamOIDCProviderSpec{ - Issuer: testIssuer.URL, + Issuer: testIssuerURL, + TLS: &v1alpha1.TLSSpec{CertificateAuthorityData: testIssuerCABase64}, Client: v1alpha1.OIDCClient{SecretName: testSecretName}, AuthorizationConfig: v1alpha1.OIDCAuthorizationConfig{AdditionalScopes: append(testAdditionalScopes, "xyz", "openid")}, }, @@ -373,7 +477,8 @@ func TestController(t *testing.T) { inputUpstreams: []runtime.Object{&v1alpha1.UpstreamOIDCProvider{ ObjectMeta: metav1.ObjectMeta{Namespace: testNamespace, Name: testName, Generation: 1234}, Spec: v1alpha1.UpstreamOIDCProviderSpec{ - Issuer: testIssuer.URL, + Issuer: testIssuerURL, + TLS: &v1alpha1.TLSSpec{CertificateAuthorityData: testIssuerCABase64}, Client: v1alpha1.OIDCClient{SecretName: testSecretName}, AuthorizationConfig: v1alpha1.OIDCAuthorizationConfig{AdditionalScopes: testAdditionalScopes}, }, @@ -486,10 +591,9 @@ func normalizeUpstreams(upstreams []v1alpha1.UpstreamOIDCProvider, now metav1.Ti return result } -func newTestIssuer(t *testing.T) *httptest.Server { +func newTestIssuer(t *testing.T) (string, string) { mux := http.NewServeMux() - testServer := httptest.NewServer(mux) - t.Cleanup(testServer.Close) + caBundlePEM, testURL := testutil.TLSTestServer(t, mux.ServeHTTP) type providerJSON struct { Issuer string `json:"issuer"` @@ -502,7 +606,7 @@ func newTestIssuer(t *testing.T) *httptest.Server { mux.HandleFunc("/.well-known/openid-configuration", func(w http.ResponseWriter, r *http.Request) { w.Header().Set("content-type", "application/json") _ = json.NewEncoder(w).Encode(&providerJSON{ - Issuer: testServer.URL, + Issuer: testURL, AuthURL: "https://example.com/authorize", }) }) @@ -511,7 +615,7 @@ func newTestIssuer(t *testing.T) *httptest.Server { mux.HandleFunc("/invalid/.well-known/openid-configuration", func(w http.ResponseWriter, r *http.Request) { w.Header().Set("content-type", "application/json") _ = json.NewEncoder(w).Encode(&providerJSON{ - Issuer: testServer.URL + "/invalid", + Issuer: testURL + "/invalid", AuthURL: "%", }) }) @@ -520,10 +624,10 @@ func newTestIssuer(t *testing.T) *httptest.Server { mux.HandleFunc("/insecure/.well-known/openid-configuration", func(w http.ResponseWriter, r *http.Request) { w.Header().Set("content-type", "application/json") _ = json.NewEncoder(w).Encode(&providerJSON{ - Issuer: testServer.URL + "/insecure", + Issuer: testURL + "/insecure", AuthURL: "http://example.com/authorize", }) }) - return testServer + return caBundlePEM, testURL }