diff --git a/lib/clusterproxy/export_test.go b/lib/clusterproxy/export_test.go index 984311a..0d83f92 100644 --- a/lib/clusterproxy/export_test.go +++ b/lib/clusterproxy/export_test.go @@ -20,6 +20,8 @@ import ( "time" "k8s.io/client-go/rest" + + libsveltosv1beta1 "github.com/projectsveltos/libsveltos/api/v1beta1" ) const ( @@ -32,12 +34,20 @@ var ( IsSveltosClusterInPullMode = isSveltosClusterInPullMode ) -// StoreTestWiCache inserts a pre-built entry into the workload identity cache. +// StoreTestWiCache inserts a pre-built entry into the workload identity cache, hashed from wi +// so it is only treated as valid for that exact WorkloadIdentityConfig. // For use in tests only. -func StoreTestWiCache(namespace, name string, cfg *rest.Config, expiresAt time.Time) { - wiCache.Store(wiCacheKey(namespace, name), cachedRestConfig{config: cfg, expiresAt: expiresAt}) +func StoreTestWiCache(namespace, name string, cfg *rest.Config, expiresAt time.Time, + wi *libsveltosv1beta1.WorkloadIdentityConfig) { + + specHash, _ := workloadIdentityConfigHash(wi) + wiCache.Store(wiCacheKey(namespace, name), cachedRestConfig{config: cfg, expiresAt: expiresAt, specHash: specHash}) } +// WorkloadIdentityConfigHashForTest calls the internal workloadIdentityConfigHash function. +// For use in tests only. +var WorkloadIdentityConfigHashForTest = workloadIdentityConfigHash + // LoadTestWiCache returns the cached rest.Config and expiry for the given cluster. // For use in tests only. func LoadTestWiCache(namespace, name string) (*rest.Config, time.Time, bool) { diff --git a/lib/clusterproxy/workload_identity.go b/lib/clusterproxy/workload_identity.go index 67dba1b..6318d5a 100644 --- a/lib/clusterproxy/workload_identity.go +++ b/lib/clusterproxy/workload_identity.go @@ -18,7 +18,9 @@ package clusterproxy import ( "context" + "crypto/sha256" "encoding/base64" + "encoding/json" "fmt" "net/http" "os" @@ -66,6 +68,10 @@ const ( type cachedRestConfig struct { config *rest.Config expiresAt time.Time + // specHash is the hash of the WorkloadIdentityConfig this entry was built from. + // A mismatch means spec.workloadIdentity changed since this entry was cached, so + // it must not be reused even though it has not expired yet. + specHash string } var ( @@ -84,6 +90,20 @@ func wiCacheKey(namespace, name string) string { return namespace + "/" + name } +// workloadIdentityConfigHash returns a stable hash of a WorkloadIdentityConfig, used to +// detect when spec.workloadIdentity changes (provider, endpoint, cloud-specific fields, +// or which CA Secret is referenced) so a stale cached rest.Config is not reused. +// It does not detect the referenced CA Secret's content changing while keeping the same +// name; that is a narrower, separate problem left for a follow-up. +func workloadIdentityConfigHash(wi *libsveltosv1beta1.WorkloadIdentityConfig) (string, error) { + data, err := json.Marshal(wi) + if err != nil { + return "", err + } + hash := sha256.Sum256(data) + return fmt.Sprintf("%x", hash), nil +} + // getWorkloadIdentityRestConfig returns a rest.Config for a SveltosCluster that // uses cloud provider workload identity. Results are cached and proactively // refreshed when they approach expiry; concurrent calls for the same cluster are @@ -98,10 +118,15 @@ func getWorkloadIdentityRestConfig( key := wiCacheKey(clusterNamespace, clusterName) - // Fast path: valid cached entry. + specHash, err := workloadIdentityConfigHash(wi) + if err != nil { + return nil, errors.Wrap(err, "failed to hash workload identity config") + } + + // Fast path: valid cached entry for the current spec. if v, ok := wiCache.Load(key); ok { entry := v.(cachedRestConfig) - if time.Until(entry.expiresAt) > wiRefreshThreshold { + if entry.specHash == specHash && time.Until(entry.expiresAt) > wiRefreshThreshold { return entry.config, nil } } @@ -133,7 +158,7 @@ func getWorkloadIdentityRestConfig( return nil, err } - wiCache.Store(key, cachedRestConfig{config: cfg, expiresAt: expiresAt}) + wiCache.Store(key, cachedRestConfig{config: cfg, expiresAt: expiresAt, specHash: specHash}) return result{cfg: cfg}, nil }) if err != nil { diff --git a/lib/clusterproxy/workload_identity_test.go b/lib/clusterproxy/workload_identity_test.go index 3e5d4a9..7864a8a 100644 --- a/lib/clusterproxy/workload_identity_test.go +++ b/lib/clusterproxy/workload_identity_test.go @@ -35,6 +35,10 @@ import ( const ( wiTestCachedEndpoint = "https://cached.example.com" wiTestCASecretName = "my-ca" + wiTestEndpoint = "https://example.com" + wiTestGCPProjectID = "proj" + wiTestGCPCluster = "cluster" + wiTestGCPLocation = "us-central1" ) var _ = Describe("WorkloadIdentity cache", func() { @@ -48,8 +52,12 @@ var _ = Describe("WorkloadIdentity cache", func() { }) It("EvictWorkloadIdentityCache removes the entry", func() { - cfg := &rest.Config{Host: "https://example.com"} - clusterproxy.StoreTestWiCache(ns, name, cfg, time.Now().Add(time.Hour)) + cfg := &rest.Config{Host: wiTestEndpoint} + wi := &libsveltosv1beta1.WorkloadIdentityConfig{ + Provider: libsveltosv1beta1.WorkloadIdentityProviderGCP, + Endpoint: wiTestEndpoint, + } + clusterproxy.StoreTestWiCache(ns, name, cfg, time.Now().Add(time.Hour), wi) _, _, ok := clusterproxy.LoadTestWiCache(ns, name) Expect(ok).To(BeTrue()) @@ -61,18 +69,19 @@ var _ = Describe("WorkloadIdentity cache", func() { }) It("GetSveltosKubernetesRestConfig returns cached config when not near expiry", func() { - cached := &rest.Config{Host: wiTestCachedEndpoint} - clusterproxy.StoreTestWiCache(ns, name, cached, time.Now().Add(time.Hour)) - wi := &libsveltosv1beta1.WorkloadIdentityConfig{ Provider: libsveltosv1beta1.WorkloadIdentityProviderGCP, Endpoint: wiTestCachedEndpoint, GCP: &libsveltosv1beta1.GCPWorkloadIdentityConfig{ - ProjectID: "proj", - ClusterName: "cluster", - Location: "us-central1", + ProjectID: wiTestGCPProjectID, + ClusterName: wiTestGCPCluster, + Location: wiTestGCPLocation, }, } + + cached := &rest.Config{Host: wiTestCachedEndpoint} + clusterproxy.StoreTestWiCache(ns, name, cached, time.Now().Add(time.Hour), wi) + sveltosCluster := &libsveltosv1beta1.SveltosCluster{ ObjectMeta: metav1.ObjectMeta{ Namespace: ns, @@ -90,6 +99,72 @@ var _ = Describe("WorkloadIdentity cache", func() { Expect(err).To(BeNil()) Expect(got).To(Equal(cached)) }) + + It("GetSveltosKubernetesRestConfig does not reuse a cached config once workloadIdentity spec changes", func() { + oldWi := &libsveltosv1beta1.WorkloadIdentityConfig{ + Provider: libsveltosv1beta1.WorkloadIdentityProviderGCP, + Endpoint: wiTestCachedEndpoint, + GCP: &libsveltosv1beta1.GCPWorkloadIdentityConfig{ + ProjectID: wiTestGCPProjectID, + ClusterName: wiTestGCPCluster, + Location: wiTestGCPLocation, + }, + } + cached := &rest.Config{Host: wiTestCachedEndpoint} + clusterproxy.StoreTestWiCache(ns, name, cached, time.Now().Add(time.Hour), oldWi) + + // Same cluster, but the CA secret reference was added/changed: this must be treated + // as a different spec, not the one the cached entry was built from. + newWi := oldWi.DeepCopy() + newWi.CASecretRef = &corev1.LocalObjectReference{Name: wiTestCASecretName} + + sveltosCluster := &libsveltosv1beta1.SveltosCluster{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: ns, + Name: name, + }, + Spec: libsveltosv1beta1.SveltosClusterSpec{ + WorkloadIdentity: newWi, + }, + } + + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(sveltosCluster).Build() + logger := logr.Discard() + + // The stale cached config must not be returned. Without real cloud credentials in this + // test environment the slow path is expected to fail obtaining a token, which is itself + // proof the fast path was correctly bypassed. + got, err := clusterproxy.GetSveltosKubernetesRestConfig(ctx, logger, c, ns, name) + Expect(err).ToNot(BeNil()) + Expect(got).ToNot(Equal(cached)) + }) +}) + +var _ = Describe("workloadIdentityConfigHash", func() { + It("returns the same hash for equal configs and a different hash when the spec changes", func() { + wi := &libsveltosv1beta1.WorkloadIdentityConfig{ + Provider: libsveltosv1beta1.WorkloadIdentityProviderGCP, + Endpoint: wiTestCachedEndpoint, + GCP: &libsveltosv1beta1.GCPWorkloadIdentityConfig{ + ProjectID: wiTestGCPProjectID, + ClusterName: wiTestGCPCluster, + Location: wiTestGCPLocation, + }, + } + + h1, err := clusterproxy.WorkloadIdentityConfigHashForTest(wi) + Expect(err).To(BeNil()) + + h2, err := clusterproxy.WorkloadIdentityConfigHashForTest(wi.DeepCopy()) + Expect(err).To(BeNil()) + Expect(h1).To(Equal(h2)) + + withCA := wi.DeepCopy() + withCA.CASecretRef = &corev1.LocalObjectReference{Name: wiTestCASecretName} + h3, err := clusterproxy.WorkloadIdentityConfigHashForTest(withCA) + Expect(err).To(BeNil()) + Expect(h3).ToNot(Equal(h1)) + }) }) var _ = Describe("WorkloadIdentity CA secret", func() {