diff --git a/controllers/configurationpolicy_controller.go b/controllers/configurationpolicy_controller.go index 13ec29ec..6a6a3b0b 100644 --- a/controllers/configurationpolicy_controller.go +++ b/controllers/configurationpolicy_controller.go @@ -155,14 +155,8 @@ func (r *ConfigurationPolicyReconciler) SetupWithManager( // blank assignment to verify that ConfigurationPolicyReconciler implements reconcile.Reconciler var _ reconcile.Reconciler = &ConfigurationPolicyReconciler{} -type cachedEncryptionKey struct { - key []byte - previousKey []byte -} - // 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 @@ -1090,13 +1084,12 @@ func (r *ConfigurationPolicyReconciler) handleTemplatization( } 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()) @@ -1170,32 +1163,6 @@ func (r *ConfigurationPolicyReconciler) handleTemplatization( resolvedTemplate, tplErr := tmplResolver.ResolveTemplate(rawData, nil, &resolveOptions) if tplErr != nil { - // 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. - isRefreshableError := errors.Is(tplErr, templates.ErrInvalidPKCS7Padding) || - errors.Is(tplErr, templates.ErrInvalidAESKey) || errors.Is(tplErr, templates.ErrAESKeyNotSet) - - if usedKeyCache && isRefreshableError { - 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", - ) - - encryptionConfig, usedNewCache, err := r.getEncryptionConfig(plc, true) - if err != nil { - addTemplateErrorViolation("", err.Error()) - - return parentStatusUpdateNeeded, err - } - - usedKeyCache = usedNewCache - resolveOptions.EncryptionConfig = encryptionConfig - resolvedTemplate, tplErr = tmplResolver.ResolveTemplate(rawData, nil, &resolveOptions) - } - } - - if tplErr != nil { // this could be a *new* tplErr if errors.Is(tplErr, templates.ErrInvalidAESKey) || errors.Is(tplErr, templates.ErrAESKeyNotSet) { addTemplateErrorViolation("", `The "policy-encryption-key" Secret contains an invalid AES key`) diff --git a/controllers/encryption.go b/controllers/encryption.go index d788d8b3..d1cc3bed 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.Con 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 e5152d7c..6b9bd039 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 d93574af..5d9ea6b5 100644 --- a/main.go +++ b/main.go @@ -233,6 +233,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{ @@ -241,7 +248,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", + }), + } } // No need to resync every 10 hours. @@ -274,13 +287,6 @@ func main() { ByObject: cacheByObject, SyncPeriod: &disableResync, }, - // 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(