diff --git a/controllers/configurationpolicy_controller.go b/controllers/configurationpolicy_controller.go index 883fea69..d690c823 100644 --- a/controllers/configurationpolicy_controller.go +++ b/controllers/configurationpolicy_controller.go @@ -92,11 +92,6 @@ func (r *ConfigurationPolicyReconciler) SetupWithManager(mgr ctrl.Manager) error // blank assignment to verify that ConfigurationPolicyReconciler implements reconcile.Reconciler var _ reconcile.Reconciler = &ConfigurationPolicyReconciler{} -type cachedEncryptionKey struct { - key []byte - previousKey []byte -} - type discoveryInfo struct { apiResourceList []*metav1.APIResourceList apiGroups []*restmapper.APIGroupResources @@ -106,7 +101,6 @@ type discoveryInfo struct { // ConfigurationPolicyReconciler reconciles a ConfigurationPolicy object type ConfigurationPolicyReconciler struct { - cachedEncryptionKey *cachedEncryptionKey // This client, initialized using mgr.Client() above, is a split client // that reads objects from the cache and writes to the apiserver client.Client @@ -922,13 +916,11 @@ func (r *ConfigurationPolicyReconciler) handleObjectTemplates(plc policyv1.Confi tmplResolverCfg := templates.Config{MissingAPIResourceCacheTTL: 10 * time.Second} resolveOptions := templates.ResolveOptions{} - usedKeyCache := false - if usesEncryption(plc) { var encryptionConfig templates.EncryptionConfig var err error - encryptionConfig, usedKeyCache, err = r.getEncryptionConfig(plc, false) + encryptionConfig, err = r.getEncryptionConfig(context.TODO(), &plc) if err != nil { addTemplateErrorViolation("", err.Error()) @@ -992,31 +984,6 @@ func (r *ConfigurationPolicyReconciler) handleObjectTemplates(plc policyv1.Confi r.processedPolicyCache.Delete(plc.GetUID()) resolvedTemplate, tplErr := tmplResolver.ResolveTemplate(rawData, nil, &resolveOptions) - - // If the error is because the padding is invalid, this either means the encrypted value was not - // generated by the "protect" template function or the AES key is incorrect. Control for a stale - // cached key. - if usedKeyCache && (errors.Is(tplErr, templates.ErrInvalidPKCS7Padding) || - errors.Is(tplErr, templates.ErrInvalidAESKey) || - errors.Is(tplErr, templates.ErrAESKeyNotSet)) { - log.V(2).Info( - "The template decryption failed likely due to an invalid encryption key, will refresh " + - "the encryption key cache and try the decryption again", - ) - var encryptionConfig templates.EncryptionConfig - - encryptionConfig, usedKeyCache, err = r.getEncryptionConfig(plc, true) - if err != nil { - addTemplateErrorViolation("", err.Error()) - - return - } - - resolveOptions.EncryptionConfig = encryptionConfig - - resolvedTemplate, tplErr = tmplResolver.ResolveTemplate(rawData, nil, &resolveOptions) - } - if tplErr != nil { var msg string diff --git a/controllers/encryption.go b/controllers/encryption.go index 54e1f03c..44a97203 100644 --- a/controllers/encryption.go +++ b/controllers/encryption.go @@ -17,51 +17,13 @@ import ( const IVAnnotation = "policy.open-cluster-management.io/encryption-iv" -// getEncryptionKey will get the encryption key in the managed cluster namespace used for policy template encryption. -func (r *ConfigurationPolicyReconciler) getEncryptionKey(namespace string) (*cachedEncryptionKey, error) { - // #nosec G101 - const secretName = "policy-encryption-key" - - ctx := context.TODO() - objectKey := types.NamespacedName{ - Name: secretName, - Namespace: namespace, - } - encryptionSecret := &corev1.Secret{} - - err := r.Get(ctx, objectKey, encryptionSecret) - if err != nil { - return nil, fmt.Errorf("failed to get the encryption key from Secret %s/%s: %w", namespace, secretName, err) - } - - var key []byte - if len(encryptionSecret.Data["key"]) > 0 { - key = encryptionSecret.Data["key"] - } - - var previousKey []byte - if len(encryptionSecret.Data["previousKey"]) > 0 { - previousKey = encryptionSecret.Data["previousKey"] - } - - cachedKey := &cachedEncryptionKey{ - key: key, - previousKey: previousKey, - } - - return cachedKey, nil -} - // getEncryptionConfig returns the encryption config for decrypting values that were encrypted using Hub templates. // The returned bool determines if the cache was used. This value can be helpful to know if the cache should be // refreshed if the decryption failed. The forceRefresh argument will skip the cache and update it with what the // API returns. -func (r *ConfigurationPolicyReconciler) getEncryptionConfig(policy policyv1.ConfigurationPolicy, forceRefresh bool) ( - templates.EncryptionConfig, bool, error, +func (r *ConfigurationPolicyReconciler) getEncryptionConfig(ctx context.Context, policy *policyv1.ConfigurationPolicy) ( + templates.EncryptionConfig, error, ) { - log := log.WithValues("policy", policy.GetName()) - usedCache := false - annotations := policy.GetAnnotations() ivBase64 := annotations[IVAnnotation] @@ -69,40 +31,32 @@ func (r *ConfigurationPolicyReconciler) getEncryptionConfig(policy policyv1.Conf if err != nil { err = fmt.Errorf(`the policy annotation of "%s" is not Base64: %w`, IVAnnotation, err) - return templates.EncryptionConfig{}, usedCache, err + return templates.EncryptionConfig{}, err } - if r.cachedEncryptionKey == nil || forceRefresh { - r.cachedEncryptionKey = &cachedEncryptionKey{} + objectKey := types.NamespacedName{ + Name: "policy-encryption-key", + Namespace: policy.GetNamespace(), } - if r.cachedEncryptionKey.key == nil { - log.V(2).Info( - "The encryption key is not cached, getting the encryption key from the server for the EncryptionConfig " + - "object", - ) - - var err error + encryptionSecret := &corev1.Secret{} - // The managed cluster namespace will be the same namespace as the ConfigurationPolicy - r.cachedEncryptionKey, err = r.getEncryptionKey(policy.GetNamespace()) - if err != nil { - return templates.EncryptionConfig{}, usedCache, err - } - } else { - log.V(2).Info("Using the cached encryption key for the EncryptionConfig object") - usedCache = true + err = r.Get(ctx, objectKey, encryptionSecret) + if err != nil { + return templates.EncryptionConfig{}, fmt.Errorf( + "failed to get the encryption key from Secret %s/policy-encryption-key: %w", policy.GetNamespace(), err, + ) } encryptionConfig := templates.EncryptionConfig{ - AESKey: r.cachedEncryptionKey.key, - AESKeyFallback: r.cachedEncryptionKey.previousKey, + AESKey: encryptionSecret.Data["key"], + AESKeyFallback: encryptionSecret.Data["previousKey"], DecryptionConcurrency: r.DecryptionConcurrency, DecryptionEnabled: true, InitializationVector: iv, } - return encryptionConfig, usedCache, nil + return encryptionConfig, nil } // usesEncryption detects if the initialization vector is set on the policy. If it is, then there diff --git a/controllers/encryption_test.go b/controllers/encryption_test.go index d3b47e8d..efa2114b 100644 --- a/controllers/encryption_test.go +++ b/controllers/encryption_test.go @@ -4,7 +4,7 @@ package controllers import ( - "bytes" + "context" "crypto/rand" "testing" @@ -67,41 +67,16 @@ func getEmptyPolicy() policyv1.ConfigurationPolicy { } } -func TestGetEncryptionKey(t *testing.T) { - t.Parallel() - RegisterFailHandler(Fail) - - r := getReconciler(true) - cachedEncryptionKey, err := r.getEncryptionKey(clusterName) - - Expect(err).ToNot(HaveOccurred()) - Expect(cachedEncryptionKey.key).ToNot(BeNil()) - Expect(cachedEncryptionKey.previousKey).ToNot(BeNil()) -} - -func TestGetEncryptionKeyFail(t *testing.T) { - t.Parallel() - RegisterFailHandler(Fail) - - r := getReconciler(false) - cachedEncryptionKey, err := r.getEncryptionKey(clusterName) - - Expect(err).To(HaveOccurred()) - Expect(cachedEncryptionKey).To(BeNil()) -} - func TestGetEncryptionConfig(t *testing.T) { t.Parallel() RegisterFailHandler(Fail) r := getReconciler(true) - Expect(r.cachedEncryptionKey).To(BeNil()) policy := getEmptyPolicy() - config, usedCache, err := r.getEncryptionConfig(policy, false) + config, err := r.getEncryptionConfig(context.TODO(), &policy) Expect(err).ToNot(HaveOccurred()) - Expect(usedCache).To(BeFalse()) Expect(config).ToNot(BeNil()) Expect(config.AESKey).ToNot(BeNil()) Expect(config.AESKeyFallback).ToNot(BeNil()) @@ -109,31 +84,6 @@ func TestGetEncryptionConfig(t *testing.T) { Expect(config.DecryptionConcurrency).To(Equal(uint8(5))) Expect(config.EncryptionEnabled).To(BeFalse()) Expect(config.InitializationVector).ToNot(BeNil()) - - Expect(r.cachedEncryptionKey).ToNot(BeNil()) - Expect(r.cachedEncryptionKey.key).ToNot(BeNil()) - Expect(r.cachedEncryptionKey.previousKey).ToNot(BeNil()) -} - -func TestGetEncryptionConfigCached(t *testing.T) { - t.Parallel() - RegisterFailHandler(Fail) - - // Ensure there is no secret so that if the cache isn't used, getEncryptionConfig will fail. - r := getReconciler(false) - - key := make([]byte, keySize/8) - _, err := rand.Read(key) - Expect(err).ToNot(HaveOccurred()) - - r.cachedEncryptionKey = &cachedEncryptionKey{key: key} - policy := getEmptyPolicy() - - config, usedCache, err := r.getEncryptionConfig(policy, false) - Expect(err).ToNot(HaveOccurred()) - Expect(usedCache).To(BeTrue()) - Expect(config).ToNot(BeNil()) - Expect(config.AESKey).ToNot(BeNil()) } func TestGetEncryptionConfigInvalidIV(t *testing.T) { @@ -141,7 +91,6 @@ func TestGetEncryptionConfigInvalidIV(t *testing.T) { RegisterFailHandler(Fail) r := getReconciler(true) - Expect(r.cachedEncryptionKey).To(BeNil()) policy := policyv1.ConfigurationPolicy{ ObjectMeta: metav1.ObjectMeta{ @@ -151,7 +100,7 @@ func TestGetEncryptionConfigInvalidIV(t *testing.T) { }, } - _, _, err := r.getEncryptionConfig(policy, false) + _, err := r.getEncryptionConfig(context.TODO(), &policy) Expect(err.Error()).To( Equal( "the policy annotation of \"policy.open-cluster-management.io/encryption-iv\" is not Base64: illegal " + @@ -165,11 +114,10 @@ func TestGetEncryptionConfigNoSecret(t *testing.T) { RegisterFailHandler(Fail) r := getReconciler(false) - Expect(r.cachedEncryptionKey).To(BeNil()) policy := getEmptyPolicy() - _, _, err := r.getEncryptionConfig(policy, false) + _, err := r.getEncryptionConfig(context.TODO(), &policy) Expect(err.Error()).To( Equal( `failed to get the encryption key from Secret local-cluster/policy-encryption-key: secrets ` + @@ -178,47 +126,6 @@ func TestGetEncryptionConfigNoSecret(t *testing.T) { ) } -func TestGetEncryptionConfigForceRefresh(t *testing.T) { - t.Parallel() - RegisterFailHandler(Fail) - - r := getReconciler(true) - key := bytes.Repeat([]byte{byte('A')}, keySize/8) - r.cachedEncryptionKey = &cachedEncryptionKey{key: key} - - policy := getEmptyPolicy() - - config, usedCache, err := r.getEncryptionConfig(policy, true) - Expect(err).ToNot(HaveOccurred()) - Expect(usedCache).To(BeFalse()) - Expect(config).ToNot(BeNil()) - Expect(config.AESKey).ToNot(Equal(key)) -} - -func TestGetEncryptionKeyEmptySecret(t *testing.T) { - t.Parallel() - RegisterFailHandler(Fail) - - encryptionSecret := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: secretName, - Namespace: clusterName, - }, - Data: map[string][]byte{ - "key": {}, - "previousKey": {}, - }, - } - client := fake.NewClientBuilder().WithObjects(encryptionSecret).Build() - - r := ConfigurationPolicyReconciler{Client: client, DecryptionConcurrency: 5} - cachedEncryptionKey, err := r.getEncryptionKey(clusterName) - - Expect(err).ToNot(HaveOccurred()) - Expect(cachedEncryptionKey.key).To(BeNil()) - Expect(cachedEncryptionKey.previousKey).To(BeNil()) -} - func TestUsesEncryption(t *testing.T) { t.Parallel() RegisterFailHandler(Fail) diff --git a/main.go b/main.go index 299ca492..0234e122 100644 --- a/main.go +++ b/main.go @@ -229,6 +229,13 @@ func main() { }), } + cacheByObject[&corev1.Secret{}] = cache.ByObject{ + Field: fields.SelectorFromSet(fields.Set{ + "metadata.namespace": watchNamespace, + "metadata.name": "policy-encryption-key", + }), + } + if opts.enableOperatorPolicy { cacheByObject[&policyv1beta1.OperatorPolicy{}] = cache.ByObject{ Field: fields.SelectorFromSet(fields.Set{ @@ -237,7 +244,13 @@ func main() { } } } else { - log.Info("Skipping restrictions on the ConfigurationPolicy cache because watchNamespace is empty") + log.Info("Skipping namespace restrictions on the cache because watchNamespace is empty") + + cacheByObject[&corev1.Secret{}] = cache.ByObject{ + Field: fields.SelectorFromSet(fields.Set{ + "metadata.name": "policy-encryption-key", + }), + } } // Set default manager options @@ -257,13 +270,6 @@ func main() { Cache: cache.Options{ ByObject: cacheByObject, }, - // Disable the cache for Secrets to avoid a watch getting created when the `policy-encryption-key` - // Secret is retrieved. Special cache handling is done by the controller. - Client: client.Options{ - Cache: &client.CacheOptions{ - DisableFor: []client.Object{&corev1.Secret{}}, - }, - }, // Override the EventBroadcaster so that the spam filter will not ignore events for the policy but with // different messages if a large amount of events for that policy are sent in a short time. EventBroadcaster: record.NewBroadcasterWithCorrelatorOptions(