From 288e092d2ed7f4ee4c459562da600cad36cc9bc7 Mon Sep 17 00:00:00 2001 From: Joshua Casey Date: Mon, 22 Jul 2024 23:47:45 -0500 Subject: [PATCH] GitHub IDP watcher should not dial an address that has already been validated --- .../github_upstream_watcher.go | 131 ++++++++---- .../github_upstream_watcher_test.go | 187 +++++++++++++++--- internal/supervisor/server/server.go | 2 + 3 files changed, 257 insertions(+), 63 deletions(-) diff --git a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go index 9a58f7e80..7003a4cbf 100644 --- a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go +++ b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher.go @@ -6,6 +6,7 @@ package githubupstreamwatcher import ( "context" + "crypto/sha256" "crypto/tls" "crypto/x509" "errors" @@ -14,6 +15,7 @@ import ( "net/http" "slices" "strings" + "time" "golang.org/x/oauth2" corev1 "k8s.io/api/core/v1" @@ -21,6 +23,7 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/util/cache" utilerrors "k8s.io/apimachinery/pkg/util/errors" corev1informers "k8s.io/client-go/informers/core/v1" "k8s.io/utils/clock" @@ -67,6 +70,42 @@ type UpstreamGitHubIdentityProviderICache interface { SetGitHubIdentityProviders([]upstreamprovider.UpstreamGithubIdentityProviderI) } +type GitHubValidatedAPICacheI interface { + MarkAsValidated(address string, caBundlePEM []byte) + IsValid(address string, caBundlePEM []byte) bool +} + +type GitHubValidatedAPICache struct { + cache *cache.Expiring +} + +type GitHubValidatedAPICacheKey struct { + address string + caBundlePEMSHA256 [32]byte +} + +func (g *GitHubValidatedAPICache) MarkAsValidated(address string, caBundlePEM []byte) { + key := GitHubValidatedAPICacheKey{ + address: address, + caBundlePEMSHA256: sha256.Sum256(caBundlePEM), + } + // Existence in the cache means it has been validated + g.cache.Set(key, nil, 24*365*time.Hour) +} + +func (g *GitHubValidatedAPICache) IsValid(address string, caBundlePEM []byte) bool { + key := GitHubValidatedAPICacheKey{ + address: address, + caBundlePEMSHA256: sha256.Sum256(caBundlePEM), + } + _, ok := g.cache.Get(key) + return ok +} + +func NewGitHubValidatedAPICache(cache *cache.Expiring) GitHubValidatedAPICacheI { + return &GitHubValidatedAPICache{cache: cache} +} + type gitHubWatcherController struct { namespace string cache UpstreamGitHubIdentityProviderICache @@ -77,6 +116,7 @@ type gitHubWatcherController struct { configMapInformer corev1informers.ConfigMapInformer clock clock.Clock dialFunc func(network, addr string, config *tls.Config) (*tls.Conn, error) + validatedCache GitHubValidatedAPICacheI } // New instantiates a new controllerlib.Controller which will populate the provided UpstreamGitHubIdentityProviderICache. @@ -91,6 +131,7 @@ func New( withInformer pinnipedcontroller.WithInformerOptionFunc, clock clock.Clock, dialFunc func(network, addr string, config *tls.Config) (*tls.Conn, error), + validatedCache *cache.Expiring, ) controllerlib.Controller { c := gitHubWatcherController{ namespace: namespace, @@ -102,6 +143,7 @@ func New( configMapInformer: configMapInformer, clock: clock, dialFunc: dialFunc, + validatedCache: NewGitHubValidatedAPICache(validatedCache), } return controllerlib.New( @@ -279,19 +321,38 @@ func (c *gitHubWatcherController) validateUpstreamAndUpdateConditions(ctx contro hostCondition, hostPort := validateHost(upstream.Spec.GitHubAPI) conditions = append(conditions, hostCondition) - tlsConfigCondition, certPool := c.validateTLSConfiguration(upstream.Spec.GitHubAPI.TLS) + tlsConfigCondition, caBundlePEM, certPool := tlsconfigutil.ValidateTLSConfig( + tlsconfigutil.TLSSpecForSupervisor(upstream.Spec.GitHubAPI.TLS), + "spec.githubAPI.tls", + c.namespace, + c.secretInformer, + c.configMapInformer) conditions = append(conditions, tlsConfigCondition) - // TODO: skip this if it is already validated for this same spec and CA bundle (or perhaps hash of CA bundle) - githubConnectionCondition, hostURL, httpClient, githubConnectionErr := c.validateGitHubConnection( - hostPort, - certPool, - hostCondition.Status == metav1.ConditionTrue && tlsConfigCondition.Status == metav1.ConditionTrue, - ) - if githubConnectionErr != nil { - applicationErrors = append(applicationErrors, githubConnectionErr) + var hostURL string + var httpClient *http.Client + if hostCondition.Status != metav1.ConditionTrue || tlsConfigCondition.Status != metav1.ConditionTrue { + conditions = append(conditions, &metav1.Condition{ + Type: GitHubConnectionValid, + Status: metav1.ConditionUnknown, + Reason: "UnableToValidate", + Message: "unable to validate; see other conditions for details", + }) + } else { + address := hostPort.Endpoint() + var githubConnectionCondition *metav1.Condition + var githubConnectionErr error + + githubConnectionCondition, hostURL, httpClient, githubConnectionErr = c.validateGitHubConnection( + address, + caBundlePEM, + certPool, + ) + if githubConnectionErr != nil { + applicationErrors = append(applicationErrors, githubConnectionErr) + } + conditions = append(conditions, githubConnectionCondition) } - conditions = append(conditions, githubConnectionCondition) // The critical pattern to maintain is that every run of the sync loop will populate the exact number of the exact // same set of conditions. Conditions depending on other conditions should get Status: metav1.ConditionUnknown, or @@ -373,47 +434,35 @@ func validateHost(gitHubAPIConfig idpv1alpha1.GitHubAPIConfig) (*metav1.Conditio }, &hostPort } -func (c *gitHubWatcherController) validateTLSConfiguration(tlsSpec *idpv1alpha1.TLSSpec) (*metav1.Condition, *x509.CertPool) { - tlsCondition, _, certPool := tlsconfigutil.ValidateTLSConfig( - tlsconfigutil.TLSSpecForSupervisor(tlsSpec), - "spec.githubAPI.tls", - c.namespace, - c.secretInformer, - c.configMapInformer) - - return tlsCondition, certPool -} - func (c *gitHubWatcherController) validateGitHubConnection( - hostPort *endpointaddr.HostPort, + address string, + caBundlePEM []byte, certPool *x509.CertPool, - validSoFar bool, ) (*metav1.Condition, string, *http.Client, error) { - if !validSoFar { - return &metav1.Condition{ - Type: GitHubConnectionValid, - Status: metav1.ConditionUnknown, - Reason: "UnableToValidate", - Message: "unable to validate; see other conditions for details", - }, "", nil, nil + var conn *tls.Conn + var tlsDialErr error + + if !c.validatedCache.IsValid(address, caBundlePEM) { + conn, tlsDialErr = c.dialFunc("tcp", address, ptls.Default(certPool)) + if tlsDialErr != nil { + return &metav1.Condition{ + Type: GitHubConnectionValid, + Status: metav1.ConditionFalse, + Reason: "UnableToDialServer", + Message: fmt.Sprintf("cannot dial server spec.githubAPI.host (%q): %s", address, buildDialErrorMessage(tlsDialErr)), + }, "", nil, tlsDialErr + } + tlsDialErr = conn.Close() } - conn, tlsDialErr := c.dialFunc("tcp", hostPort.Endpoint(), ptls.Default(certPool)) - if tlsDialErr != nil { - return &metav1.Condition{ - Type: GitHubConnectionValid, - Status: metav1.ConditionFalse, - Reason: "UnableToDialServer", - Message: fmt.Sprintf("cannot dial server spec.githubAPI.host (%q): %s", hostPort.Endpoint(), buildDialErrorMessage(tlsDialErr)), - }, "", nil, tlsDialErr - } + c.validatedCache.MarkAsValidated(address, caBundlePEM) return &metav1.Condition{ Type: GitHubConnectionValid, Status: metav1.ConditionTrue, Reason: conditionsutil.ReasonSuccess, - Message: fmt.Sprintf("spec.githubAPI.host (%q) is reachable and TLS verification succeeds", hostPort.Endpoint()), - }, fmt.Sprintf("https://%s", hostPort.Endpoint()), phttp.Default(certPool), conn.Close() + Message: fmt.Sprintf("spec.githubAPI.host (%q) is reachable and TLS verification succeeds", address), + }, fmt.Sprintf("https://%s", address), phttp.Default(certPool), tlsDialErr } // buildDialErrorMessage standardizes DNS error messages that appear differently on different platforms, so that tests and log grepping is uniform. diff --git a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go index 6a48663e1..15ca58aaf 100644 --- a/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go +++ b/internal/controller/supervisorconfig/githubupstreamwatcher/github_upstream_watcher_test.go @@ -6,12 +6,14 @@ package githubupstreamwatcher import ( "bytes" "context" + "crypto/sha256" "crypto/tls" "crypto/x509" "encoding/base64" "fmt" "net" "net/http" + "slices" "strings" "testing" "time" @@ -23,6 +25,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/cache" utilnet "k8s.io/apimachinery/pkg/util/net" k8sinformers "k8s.io/client-go/informers" kubernetesfake "k8s.io/client-go/kubernetes/fake" @@ -400,14 +403,16 @@ func TestController(t *testing.T) { } tests := []struct { - name string - githubIdentityProviders []runtime.Object - secretsAndConfigMaps []runtime.Object - mockDialer func(network, addr string, config *tls.Config) (*tls.Conn, error) - wantErr string - wantLogs []string - wantResultingCache []*upstreamgithub.ProviderConfig - wantResultingUpstreams []idpv1alpha1.GitHubIdentityProvider + name string + githubIdentityProviders []runtime.Object + secretsAndConfigMaps []runtime.Object + mockDialer func(t *testing.T) func(network, addr string, config *tls.Config) (*tls.Conn, error) + preexistingValidatedCache []GitHubValidatedAPICacheKey + wantErr string + wantLogs []string + wantResultingCache []*upstreamgithub.ProviderConfig + wantResultingUpstreams []idpv1alpha1.GitHubIdentityProvider + wantValidatedCache []GitHubValidatedAPICacheKey }{ { name: "no GitHubIdentityProviders", @@ -444,6 +449,12 @@ func TestController(t *testing.T) { HttpClient: phttp.Default(goodServerCertPool), }, }, + wantValidatedCache: []GitHubValidatedAPICacheKey{ + { + address: goodServerDomain, + caBundlePEMSHA256: sha256.Sum256(goodServerCA), + }, + }, wantResultingUpstreams: []idpv1alpha1.GitHubIdentityProvider{ { ObjectMeta: validFilledOutIDP.ObjectMeta, @@ -500,6 +511,12 @@ func TestController(t *testing.T) { HttpClient: phttp.Default(goodServerCertPool), }, }, + wantValidatedCache: []GitHubValidatedAPICacheKey{ + { + address: goodServerDomain, + caBundlePEMSHA256: sha256.Sum256(goodServerCA), + }, + }, wantResultingUpstreams: []idpv1alpha1.GitHubIdentityProvider{ { ObjectMeta: validMinimalIDP.ObjectMeta, @@ -538,12 +555,16 @@ func TestController(t *testing.T) { return githubIDP }(), }, - mockDialer: func(network, addr string, config *tls.Config) (*tls.Conn, error) { - require.Equal(t, "github.com:443", addr) - // don't actually dial github.com to avoid making external network calls in unit test - configClone := config.Clone() - configClone.RootCAs = goodServerCertPool - return tls.Dial(network, goodServerDomain, configClone) + mockDialer: func(t *testing.T) func(network, addr string, config *tls.Config) (*tls.Conn, error) { + t.Helper() + + return func(network, addr string, config *tls.Config) (*tls.Conn, error) { + require.Equal(t, "github.com:443", addr) + // don't actually dial github.com to avoid making external network calls in unit test + configClone := config.Clone() + configClone.RootCAs = goodServerCertPool + return tls.Dial(network, goodServerDomain, configClone) + } }, wantResultingCache: []*upstreamgithub.ProviderConfig{ { @@ -568,6 +589,12 @@ func TestController(t *testing.T) { HttpClient: phttp.Default(goodServerCertPool), }, }, + wantValidatedCache: []GitHubValidatedAPICacheKey{ + { + address: "github.com:443", + caBundlePEMSHA256: sha256.Sum256(goodServerCA), + }, + }, wantResultingUpstreams: []idpv1alpha1.GitHubIdentityProvider{ { ObjectMeta: validMinimalIDP.ObjectMeta, @@ -636,6 +663,12 @@ func TestController(t *testing.T) { HttpClient: phttp.Default(goodServerIPv6CertPool), }, }, + wantValidatedCache: []GitHubValidatedAPICacheKey{ + { + address: goodServerIPv6Domain, + caBundlePEMSHA256: sha256.Sum256(goodServerIPv6CA), + }, + }, wantResultingUpstreams: []idpv1alpha1.GitHubIdentityProvider{ { ObjectMeta: validMinimalIDP.ObjectMeta, @@ -745,6 +778,12 @@ func TestController(t *testing.T) { HttpClient: phttp.Default(goodServerCertPool), }, }, + wantValidatedCache: []GitHubValidatedAPICacheKey{ + { + address: goodServerDomain, + caBundlePEMSHA256: sha256.Sum256(goodServerCA), + }, + }, wantResultingUpstreams: []idpv1alpha1.GitHubIdentityProvider{ { ObjectMeta: func() metav1.ObjectMeta { @@ -920,6 +959,12 @@ func TestController(t *testing.T) { HttpClient: phttp.Default(goodServerCertPool), }, }, + wantValidatedCache: []GitHubValidatedAPICacheKey{ + { + address: goodServerDomain, + caBundlePEMSHA256: sha256.Sum256(goodServerCA), + }, + }, wantResultingUpstreams: []idpv1alpha1.GitHubIdentityProvider{ { ObjectMeta: func() metav1.ObjectMeta { @@ -998,6 +1043,81 @@ func TestController(t *testing.T) { buildLogForUpdatingPhase("idp-with-tls-in-secret", "Ready"), }, }, + { + name: "happy path with previously validated address/CA Bundle does not validate again", + secretsAndConfigMaps: []runtime.Object{goodClientCredentialsSecret}, + githubIdentityProviders: []runtime.Object{validFilledOutIDP}, + mockDialer: func(t *testing.T) func(network, addr string, config *tls.Config) (*tls.Conn, error) { + t.Helper() + + return func(network, addr string, config *tls.Config) (*tls.Conn, error) { + t.Errorf("this test should not perform dial") + t.FailNow() + return nil, nil + } + }, + preexistingValidatedCache: []GitHubValidatedAPICacheKey{ + { + address: goodServerDomain, + caBundlePEMSHA256: sha256.Sum256(goodServerCA), + }, + }, + wantResultingCache: []*upstreamgithub.ProviderConfig{ + { + Name: "some-idp-name", + ResourceUID: "some-resource-uid", + APIBaseURL: fmt.Sprintf("https://%s/api/v3", *validFilledOutIDP.Spec.GitHubAPI.Host), + UsernameAttribute: "id", + GroupNameAttribute: "name", + OAuth2Config: &oauth2.Config{ + ClientID: "some-client-id", + ClientSecret: "some-client-secret", + Endpoint: oauth2.Endpoint{ + AuthURL: fmt.Sprintf("https://%s/login/oauth/authorize", *validFilledOutIDP.Spec.GitHubAPI.Host), + DeviceAuthURL: "", // not used + TokenURL: fmt.Sprintf("https://%s/login/oauth/access_token", *validFilledOutIDP.Spec.GitHubAPI.Host), + AuthStyle: oauth2.AuthStyleInParams, + }, + RedirectURL: "", // not used + Scopes: []string{"read:user", "read:org"}, + }, + AllowedOrganizations: setutil.NewCaseInsensitiveSet("organization1", "org2"), + HttpClient: phttp.Default(goodServerCertPool), + }, + }, + wantValidatedCache: []GitHubValidatedAPICacheKey{ + { + address: goodServerDomain, + caBundlePEMSHA256: sha256.Sum256(goodServerCA), + }, + }, + wantResultingUpstreams: []idpv1alpha1.GitHubIdentityProvider{ + { + ObjectMeta: validFilledOutIDP.ObjectMeta, + Spec: validFilledOutIDP.Spec, + Status: idpv1alpha1.GitHubIdentityProviderStatus{ + Phase: idpv1alpha1.GitHubPhaseReady, + Conditions: []metav1.Condition{ + buildClaimsValidatedTrue(t), + buildClientCredentialsSecretValidTrue(t, validFilledOutIDP.Spec.Client.SecretName), + buildGitHubConnectionValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), + buildHostValidTrue(t, *validFilledOutIDP.Spec.GitHubAPI.Host), + buildOrganizationsPolicyValidTrue(t, *validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy), + buildTLSConfigurationValidTrue(t), + }, + }, + }, + }, + wantLogs: []string{ + buildLogForUpdatingClientCredentialsSecretValid("some-idp-name", "True", "Success", fmt.Sprintf(`clientID and clientSecret have been read from spec.client.SecretName (\"%s\")`, validFilledOutIDP.Spec.Client.SecretName)), + buildLogForUpdatingClaimsValidTrue("some-idp-name"), + buildLogForUpdatingOrganizationPolicyValid("some-idp-name", "True", "Success", fmt.Sprintf(`spec.allowAuthentication.organizations.policy (\"%s\") is valid`, string(*validFilledOutIDP.Spec.AllowAuthentication.Organizations.Policy))), + buildLogForUpdatingHostValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is valid`, *validFilledOutIDP.Spec.GitHubAPI.Host), + buildLogForUpdatingTLSConfigurationValid("some-idp-name", "True", "Success", "spec.githubAPI.tls is valid: loaded TLS configuration"), + buildLogForUpdatingGitHubConnectionValid("some-idp-name", "True", "Success", `spec.githubAPI.host (\"%s\") is reachable and TLS verification succeeds`, *validFilledOutIDP.Spec.GitHubAPI.Host), + buildLogForUpdatingPhase("some-idp-name", "Ready"), + }, + }, { name: "Host error - missing spec.githubAPI.host", secretsAndConfigMaps: []runtime.Object{goodClientCredentialsSecret}, @@ -2015,8 +2135,8 @@ func TestController(t *testing.T) { fakeKubeClient := kubernetesfake.NewSimpleClientset(tt.secretsAndConfigMaps...) kubeInformers := k8sinformers.NewSharedInformerFactoryWithOptions(fakeKubeClient, 0) - cache := dynamicupstreamprovider.NewDynamicUpstreamIDPProvider() - cache.SetGitHubIdentityProviders([]upstreamprovider.UpstreamGithubIdentityProviderI{ + idpCache := dynamicupstreamprovider.NewDynamicUpstreamIDPProvider() + idpCache.SetGitHubIdentityProviders([]upstreamprovider.UpstreamGithubIdentityProviderI{ upstreamgithub.New( upstreamgithub.ProviderConfig{Name: "initial-entry-to-remove"}, ), @@ -2027,14 +2147,19 @@ func TestController(t *testing.T) { gitHubIdentityProviderInformer := supervisorInformers.IDP().V1alpha1().GitHubIdentityProviders() - dialer := tt.mockDialer - if dialer == nil { - dialer = tls.Dial + dialer := tls.Dial + if tt.mockDialer != nil { + dialer = tt.mockDialer(t) + } + + validatedCache := cache.NewExpiring() + for _, item := range tt.preexistingValidatedCache { + validatedCache.Set(item, nil, 1*time.Hour) } controller := New( namespace, - cache, + idpCache, fakeSupervisorClient, gitHubIdentityProviderInformer, kubeInformers.Core().V1().Secrets(), @@ -2043,6 +2168,7 @@ func TestController(t *testing.T) { controllerlib.WithInformer, frozenClockForLastTransitionTime, dialer, + validatedCache, ) ctx, cancel := context.WithCancel(context.Background()) @@ -2061,8 +2187,8 @@ func TestController(t *testing.T) { require.NoError(t, err) } - // Verify what's in the cache - actualIDPList := cache.GetGitHubIdentityProviders() + // Verify what's in the IDP cache + actualIDPList := idpCache.GetGitHubIdentityProviders() require.Equal(t, len(tt.wantResultingCache), len(actualIDPList)) for i := range len(tt.wantResultingCache) { // Do not expect any particular order in the cache @@ -2088,6 +2214,19 @@ func TestController(t *testing.T) { require.Contains(t, tt.wantResultingCache[i].APIBaseURL, actualProvider.GetConfig().APIBaseURL) } + // Verify what's in the validated cache + var uniqueAddresses []string + for _, cachedIDP := range tt.wantResultingCache { + if !slices.Contains(uniqueAddresses, cachedIDP.APIBaseURL) { + uniqueAddresses = append(uniqueAddresses, cachedIDP.APIBaseURL) + } + } + require.Equal(t, len(uniqueAddresses), len(tt.wantValidatedCache), "every unique IDP address should have an entry in the validated cache") + for _, item := range tt.wantValidatedCache { + _, ok := validatedCache.Get(item) + require.True(t, ok, "item with address %q must be found in the validated cache", item.address) + } + // Verify the status conditions as reported in Kubernetes allGitHubIDPs, err := fakeSupervisorClient.IDPV1alpha1().GitHubIdentityProviders(namespace).List(ctx, metav1.ListOptions{}) require.NoError(t, err) @@ -2412,6 +2551,7 @@ func TestController_OnlyWantActions(t *testing.T) { controllerlib.WithInformer, frozenClockForLastTransitionTime, tls.Dial, + cache.NewExpiring(), ) ctx, cancel := context.WithCancel(context.Background()) @@ -2535,6 +2675,7 @@ func TestGitHubUpstreamWatcherControllerFilterSecret(t *testing.T) { observableInformers.WithInformer, clock.RealClock{}, tls.Dial, + cache.NewExpiring(), ) unrelated := &corev1.Secret{} @@ -2591,6 +2732,7 @@ func TestGitHubUpstreamWatcherControllerFilterConfigMaps(t *testing.T) { observableInformers.WithInformer, clock.RealClock{}, tls.Dial, + cache.NewExpiring(), ) unrelated := &corev1.ConfigMap{} @@ -2647,6 +2789,7 @@ func TestGitHubUpstreamWatcherControllerFilterGitHubIDP(t *testing.T) { observableInformers.WithInformer, clock.RealClock{}, tls.Dial, + cache.NewExpiring(), ) unrelated := &idpv1alpha1.GitHubIdentityProvider{} diff --git a/internal/supervisor/server/server.go b/internal/supervisor/server/server.go index 41fa312ae..15c75e378 100644 --- a/internal/supervisor/server/server.go +++ b/internal/supervisor/server/server.go @@ -25,6 +25,7 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/runtime/serializer" + "k8s.io/apimachinery/pkg/util/cache" apimachineryversion "k8s.io/apimachinery/pkg/version" genericapifilters "k8s.io/apiserver/pkg/endpoints/filters" openapinamer "k8s.io/apiserver/pkg/endpoints/openapi" @@ -341,6 +342,7 @@ func prepareControllers( controllerlib.WithInformer, clock.RealClock{}, tls.Dial, + cache.NewExpiring(), ), singletonWorker). WithController(