Skip to content

Commit

Permalink
Use the controller-runtime cache to get the decryption key
Browse files Browse the repository at this point in the history 
The stale cached decryption key detection logic was not 100% accurate, so
just use the controller-runtime cache every time.

Relates:
https://issues.redhat.com/browse/ACM-11497

Signed-off-by: mprahl <[email protected]>
  • Loading branch information
@mprahl @openshift-merge-bot
mprahl authored and openshift-merge-bot[bot] committed Aug 5, 2024
1 parent e150fdc commit ec8a702
Show file tree
Hide file tree
Showing 4 changed files with 34 additions and 200 deletions.
35 changes: 1 addition & 34 deletions controllers/configurationpolicy_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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())

Expand Down Expand Up @@ -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`)

Expand Down
76 changes: 15 additions & 61 deletions controllers/encryption.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,92 +17,46 @@ 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]

iv, err := base64.StdEncoding.DecodeString(ivBase64)
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
Expand Down
101 changes: 4 additions & 97 deletions controllers/encryption_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
package controllers

import (
"bytes"
"context"
"crypto/rand"
"testing"

Expand Down Expand Up @@ -67,81 +67,30 @@ 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())
Expect(config.DecryptionEnabled).To(BeTrue())
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) {
t.Parallel()
RegisterFailHandler(Fail)

r := getReconciler(true)
Expect(r.cachedEncryptionKey).To(BeNil())

policy := policyv1.ConfigurationPolicy{
ObjectMeta: metav1.ObjectMeta{
Expand All @@ -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 " +
Expand All @@ -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 ` +
Expand All @@ -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)
Expand Down
22 changes: 14 additions & 8 deletions main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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.
Expand Down Expand Up @@ -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(
Expand Down

0 comments on commit ec8a702

Please sign in to comment.