Skip to content

API-1844: KMS Encryption Provider #1876

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ require (
github.com/opencontainers/go-digest v1.0.0
github.com/opencontainers/runc v1.1.13
github.com/opencontainers/selinux v1.11.0
github.com/openshift/api v0.0.0-20250320170726-75d64d71980b
github.com/openshift/api v0.0.0-20250405052455-aa882942241d
github.com/openshift/build-machinery-go v0.0.0-20250102153059-e85a1a7ecb5c
github.com/openshift/client-go v0.0.0-20250125113824-8e1f0b8fa9a7
github.com/pkg/errors v0.9.1
Expand Down
4 changes: 2 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -203,8 +203,8 @@ github.com/opencontainers/runc v1.1.13 h1:98S2srgG9vw0zWcDpFMn5TRrh8kLxa/5OFUstu
github.com/opencontainers/runc v1.1.13/go.mod h1:R016aXacfp/gwQBYw2FDGa9m+n6atbLWrYY8hNMT/sA=
github.com/opencontainers/selinux v1.11.0 h1:+5Zbo97w3Lbmb3PeqQtpmTkMwsW5nRI3YaLpt7tQ7oU=
github.com/opencontainers/selinux v1.11.0/go.mod h1:E5dMC3VPuVvVHDYmi78qvhJp8+M586T4DlDRYpFkyec=
github.com/openshift/api v0.0.0-20250320170726-75d64d71980b h1:GGuFSHESP0BSOu70AqV4u9IVrjYdaeu4Id+HXRIOvkw=
github.com/openshift/api v0.0.0-20250320170726-75d64d71980b/go.mod h1:yk60tHAmHhtVpJQo3TwVYq2zpuP70iJIFDCmeKMIzPw=
github.com/openshift/api v0.0.0-20250405052455-aa882942241d h1:OWknR960jKxORjMEEeMc30m/wHit6w/Wxqb3XtToVdE=
github.com/openshift/api v0.0.0-20250405052455-aa882942241d/go.mod h1:yk60tHAmHhtVpJQo3TwVYq2zpuP70iJIFDCmeKMIzPw=
github.com/openshift/build-machinery-go v0.0.0-20250102153059-e85a1a7ecb5c h1:6XcszPFZpan4qll5XbdLll7n1So3IsPn28aw2j1obMo=
github.com/openshift/build-machinery-go v0.0.0-20250102153059-e85a1a7ecb5c/go.mod h1:8jcm8UPtg2mCAsxfqKil1xrmRMI3a+XU2TZ9fF8A7TE=
github.com/openshift/client-go v0.0.0-20250125113824-8e1f0b8fa9a7 h1:4iliLcvr1P9EUMZgIaSNEKNQQzBn+L6PSequlFOuB6Q=
Expand Down
148 changes: 113 additions & 35 deletions pkg/operator/encryption/controllers/key_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,15 @@ import (
"k8s.io/klog/v2"
"k8s.io/utils/ptr"

configv1 "github.com/openshift/api/config/v1"
operatorv1 "github.com/openshift/api/operator/v1"
configv1client "github.com/openshift/client-go/config/clientset/versioned/typed/config/v1"
configv1informers "github.com/openshift/client-go/config/informers/externalversions/config/v1"
applyoperatorv1 "github.com/openshift/client-go/operator/applyconfigurations/operator/v1"

"github.com/openshift/library-go/pkg/controller/factory"
"github.com/openshift/library-go/pkg/operator/encryption/crypto"
"github.com/openshift/library-go/pkg/operator/encryption/encryptionconfig"
"github.com/openshift/library-go/pkg/operator/encryption/secrets"
"github.com/openshift/library-go/pkg/operator/encryption/state"
"github.com/openshift/library-go/pkg/operator/encryption/statemachine"
Expand Down Expand Up @@ -159,7 +161,7 @@ func (c *keyController) sync(ctx context.Context, syncCtx factory.SyncContext) (
}

func (c *keyController) checkAndCreateKeys(ctx context.Context, syncContext factory.SyncContext, encryptedGRs []schema.GroupResource) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was looking at your change to the key controller and the more I am thinking about it the more I think we should handle local keys and kms keys differently. A lot of the existing key controller logic is about generating the key and backing it. Whereas for KMS we want to generating a plugin config and backing it up.

Because of that, I think it would be more appropriate to create a new controller for KMS. wdyt?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, there is one particular logic for which we will need the keyController and a new key state which is when the key is rotated on the external KMS.

When that happens we the KMS plugin will returned a new key_id in the responses: https://github.com/kubernetes/enhancements/tree/master/keps/sig-auth/3299-kms-v2-improvements#key_id-and-rotation.
In our case to handle that scenario we want the keyController to probe the Status procedure call and create a new key secret when the key_id changes to trigger a storage migration.

I recall discussing that with you in the past and we agreed that this could be done at a later point, but looking at your PR, was the intent behind your key controller changes to act as a placeholder to include that logic later?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was looking at your change to the key controller and the more I am thinking about it the more I think we should handle local keys and kms keys differently. A lot of the existing key controller logic is about generating the key and backing it. Whereas for KMS we want to generating a plugin config and backing it up.

Reasonably, we can follow-up with this when we add a new controller to manage the lifecycle of kms-plugin itself. Because then we'd need to update the status somewhere to list active KMS config hashes for the plugin controller to spin up unix sockets.

currentMode, externalReason, err := c.getCurrentModeAndExternalReason(ctx)
currentKeyState, err := c.getCurrentEncryptionModeWithExternalReason(ctx)
if err != nil {
return err
}
Expand All @@ -175,13 +177,14 @@ func (c *keyController) checkAndCreateKeys(ctx context.Context, syncContext fact

// avoid intended start of encryption
hasBeenOnBefore := currentConfig != nil || len(secrets) > 0
if currentMode == state.Identity && !hasBeenOnBefore {
if currentKeyState.Mode == state.Identity && !hasBeenOnBefore {
return nil
}

var (
newKeyRequired bool
newKeyID uint64
latestKeyID uint64
reasons []string
)

Expand All @@ -191,23 +194,28 @@ func (c *keyController) checkAndCreateKeys(ctx context.Context, syncContext fact

var commonReason *string
for gr, grKeys := range desiredEncryptionState {
latestKeyID, internalReason, needed := needsNewKey(grKeys, currentMode, externalReason, encryptedGRs)
// if KMSPluginHash in GR ReadKey is not the same as current KMSPluginHash, needed is true.
ks, needed := needsNewKeyWithInternalReason(grKeys, currentKeyState.Mode, currentKeyState.KMSPluginHash, currentKeyState.ExternalReason, encryptedGRs)
if !needed {
continue
}

latestKeyID = ks.Generation

if commonReason == nil {
commonReason = &internalReason
} else if *commonReason != internalReason {
commonReason = &ks.InternalReason
} else if *commonReason != ks.InternalReason {
commonReason = ptr.To("") // this means we have no common reason
}

newKeyRequired = true

nextKeyID := latestKeyID + 1
if newKeyID < nextKeyID {
newKeyID = nextKeyID
}
reasons = append(reasons, fmt.Sprintf("%s-%s", gr.Resource, internalReason))

reasons = append(reasons, fmt.Sprintf("%s-%s", gr.Resource, ks.InternalReason))
}
if !newKeyRequired {
return nil
Expand All @@ -216,12 +224,20 @@ func (c *keyController) checkAndCreateKeys(ctx context.Context, syncContext fact
reasons = []string{*commonReason} // don't repeat reasons
}

sort.Sort(sort.StringSlice(reasons))
internalReason := strings.Join(reasons, ", ")
keySecret, err := c.generateKeySecret(newKeyID, currentMode, internalReason, externalReason)
sort.Strings(reasons)
currentKeyState.InternalReason = strings.Join(reasons, ", ")

var keySecret *corev1.Secret
if currentKeyState.Mode == state.KMS {
keySecret, err = c.generateKMSKeySecret(newKeyID, currentKeyState.KMSConfig, currentKeyState.InternalReason, currentKeyState.ExternalReason)
} else {
keySecret, err = c.generateLocalKeySecret(newKeyID, currentKeyState.Mode, currentKeyState.InternalReason, currentKeyState.ExternalReason)
}

if err != nil {
return fmt.Errorf("failed to create key: %v", err)
}

_, createErr := c.secretClient.Secrets("openshift-config-managed").Create(ctx, keySecret, metav1.CreateOptions{})
if errors.IsAlreadyExists(createErr) {
return c.validateExistingSecret(ctx, keySecret, newKeyID)
Expand All @@ -242,22 +258,37 @@ func (c *keyController) validateExistingSecret(ctx context.Context, keySecret *c
return err
}

ks, err := secrets.ToKeyState(actualKeySecret)
if err != nil {
return fmt.Errorf("secret %s/%s is invalid, new keys cannot be created for encryption target", keySecret.Namespace, keySecret.Name)
}

if ks.Generation == 0 {
return fmt.Errorf("secret %s/%s is invalid, key generation id cannot be zero", keySecret.Namespace, keySecret.Name)
}

if ks.Mode == state.KMS && ks.KMSPluginHash != "" {
return nil
}

if ks.Mode == state.KMS && ks.KMSPluginHash == "" {
// kmsPluginHash is mandatory in case of KMS
return fmt.Errorf("secret %s/%s is invalid, new KMS config keys cannot be created for encryption target", keySecret.Namespace, keySecret.Name)
}

actualKeyID, ok := state.NameToKeyID(actualKeySecret.Name)
if !ok || actualKeyID != keyID {
// TODO we can just get stuck in degraded here ...
return fmt.Errorf("secret %s has an invalid name, new keys cannot be created for encryption target", keySecret.Name)
}

if _, err := secrets.ToKeyState(actualKeySecret); err != nil {
return fmt.Errorf("secret %s is invalid, new keys cannot be created for encryption target", keySecret.Name)
return fmt.Errorf("secret %s/%s has an invalid name, new keys cannot be created for encryption target", keySecret.Namespace, keySecret.Name)
}

return nil // we made this key earlier
}

func (c *keyController) generateKeySecret(keyID uint64, currentMode state.Mode, internalReason, externalReason string) (*corev1.Secret, error) {
func (c *keyController) generateLocalKeySecret(keyID uint64, currentMode state.Mode, internalReason, externalReason string) (*corev1.Secret, error) {
bs := crypto.ModeToNewKeyFunc[currentMode]()
ks := state.KeyState{
Generation: keyID,
Key: apiserverv1.Key{
Name: fmt.Sprintf("%d", keyID),
Secret: base64.StdEncoding.EncodeToString(bs),
Expand All @@ -269,50 +300,88 @@ func (c *keyController) generateKeySecret(keyID uint64, currentMode state.Mode,
return secrets.FromKeyState(c.instanceName, ks)
}

func (c *keyController) getCurrentModeAndExternalReason(ctx context.Context) (state.Mode, string, error) {
func (c *keyController) generateKMSKeySecret(keyID uint64, kmsConfig *configv1.KMSConfig, internalReason, externalReason string) (*corev1.Secret, error) {
kmsConfig = kmsConfig.DeepCopy()

kmsPluginHash, err := encryptionconfig.HashKMSConfig(*kmsConfig)
if err != nil {
return nil, err
}

ks := state.KeyState{
Generation: keyID,
Mode: state.KMS,
InternalReason: internalReason,
ExternalReason: externalReason,
KMSPluginHash: kmsPluginHash,
KMSConfig: kmsConfig,
}
return secrets.FromKeyState(c.instanceName, ks)
}

func (c *keyController) getCurrentEncryptionModeWithExternalReason(ctx context.Context) (state.KeyState, error) {
apiServer, err := c.apiServerClient.Get(ctx, "cluster", metav1.GetOptions{})
if err != nil {
return "", "", err
return state.KeyState{}, err
}

operatorSpec, _, _, err := c.operatorClient.GetOperatorState()
if err != nil {
return "", "", err
return state.KeyState{}, err
}

encryptionConfig, err := structuredUnsupportedConfigFrom(operatorSpec.UnsupportedConfigOverrides.Raw, c.unsupportedConfigPrefix)
if err != nil {
return "", "", err
return state.KeyState{}, err
}

reason := encryptionConfig.Encryption.Reason
switch currentMode := state.Mode(apiServer.Spec.Encryption.Type); currentMode {
case state.AESCBC, state.AESGCM, state.Identity: // secretbox is disabled for now
return currentMode, reason, nil
return state.KeyState{Mode: currentMode, ExternalReason: reason}, nil
case state.KMS:
kmsConfig := apiServer.Spec.Encryption.KMS.DeepCopy()

kmsPluginHash, err := encryptionconfig.HashKMSConfig(*kmsConfig)
if err != nil {
return state.KeyState{}, fmt.Errorf("encryption mode configured: %s, but provided kms config could not generate required kms plugin hash %v", currentMode, err)
}

ks := state.KeyState{
Mode: state.KMS,
ExternalReason: reason,

KMSPluginHash: kmsPluginHash,
KMSConfig: kmsConfig,
}
return ks, nil
case "": // unspecified means use the default (which can change over time)
return state.DefaultMode, reason, nil
return state.KeyState{Mode: state.DefaultMode, ExternalReason: reason}, nil
default:
return "", "", fmt.Errorf("unknown encryption mode configured: %s", currentMode)
return state.KeyState{}, fmt.Errorf("unknown encryption mode configured: %s", currentMode)
}
}

// needsNewKey checks whether a new key must be created for the given resource. If true, it also returns the latest
// needsNewKeyWithInternalReason checks whether a new key must be created for the given resource. If true, it also returns the latest
// used key ID and a reason string.
func needsNewKey(grKeys state.GroupResourceState, currentMode state.Mode, externalReason string, encryptedGRs []schema.GroupResource) (uint64, string, bool) {
func needsNewKeyWithInternalReason(grKeys state.GroupResourceState, currentMode state.Mode, optionalCurrentKMSHash string, externalReason string, encryptedGRs []schema.GroupResource) (state.KeyState, bool) {
// we always need to have some encryption keys unless we are turned off
if len(grKeys.ReadKeys) == 0 {
return 0, "key-does-not-exist", currentMode != state.Identity
return state.KeyState{InternalReason: "key-does-not-exist"}, currentMode != state.Identity
}

latestKey := grKeys.ReadKeys[0]
latestKeyID, ok := state.NameToKeyID(latestKey.Key.Name)
if !ok {
return latestKeyID, fmt.Sprintf("key-secret-%d-is-invalid", latestKeyID), true
latestKeyID := latestKey.Generation

if latestKeyID == 0 {
latestKey.InternalReason = fmt.Sprintf("key-secret-%d-is-invalid", latestKeyID)
return latestKey, true
}

// if latest secret has been deleted, we will never be able to migrate to that key.
if !latestKey.Backed {
return latestKeyID, fmt.Sprintf("encryption-config-key-%d-not-backed-by-secret", latestKeyID), true
latestKey.InternalReason = fmt.Sprintf("encryption-config-key-%d-not-backed-by-secret", latestKeyID)
return latestKey, true
}

// check that we have pruned read-keys: the write-keys, plus at most one more backed read-key (potentially some unbacked once before)
Expand All @@ -323,32 +392,41 @@ func needsNewKey(grKeys state.GroupResourceState, currentMode state.Mode, extern
}
}
if backedKeys > 2 {
return 0, "", false
return state.KeyState{}, false
}

// we have not migrated the latest key, do nothing until that is complete
if allMigrated, _, _ := state.MigratedFor(encryptedGRs, latestKey); !allMigrated {
return 0, "", false
return state.KeyState{}, false
}

// if the most recent secret was encrypted in a mode different than the current mode, we need to generate a new key
if latestKey.Mode != currentMode {
return latestKeyID, "encryption-mode-changed", true
latestKey.InternalReason = "encryption-mode-changed"
return latestKey, true
}

// if the most recent secret turned off encryption and we want to keep it that way, do nothing
if latestKey.Mode == state.Identity && currentMode == state.Identity {
return 0, "", false
return state.KeyState{}, false
}

// if the hash of the kms config has updated, we need a new KMS backing secret
if currentMode == state.KMS && latestKey.KMSPluginHash != optionalCurrentKMSHash {
latestKey.InternalReason = "kms-config-changed"
return latestKey, true
}

// if the most recent secret has a different external reason than the current reason, we need to generate a new key
if latestKey.ExternalReason != externalReason && len(externalReason) != 0 {
return latestKeyID, "external-reason-changed", true
latestKey.InternalReason = "external-reason-changed"
return latestKey, true
}

// we check for encryptionSecretMigratedTimestamp set by migration controller to determine when migration completed
// this also generates back pressure for key rotation when migration takes a long time or was recently completed
return latestKeyID, "rotation-interval-has-passed", time.Since(latestKey.Migrated.Timestamp) > encryptionSecretMigrationInterval
latestKey.InternalReason = "rotation-interval-has-passed"
return latestKey, time.Since(latestKey.Migrated.Timestamp) > encryptionSecretMigrationInterval
}

// TODO make this un-settable once set
Expand Down
10 changes: 6 additions & 4 deletions pkg/operator/encryption/controllers/key_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,11 @@ import (
"encoding/base64"
"errors"
"fmt"
clocktesting "k8s.io/utils/clock/testing"
"testing"
"time"

clocktesting "k8s.io/utils/clock/testing"

corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/equality"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -281,11 +282,11 @@ func TestKeyController(t *testing.T) {
Type: "EncryptionKeyControllerDegraded",
Status: "True",
Reason: "Error",
Message: "secret encryption-key-kms-1 is invalid, new keys cannot be created for encryption target",
Message: "secret openshift-config-managed/encryption-key-kms-1 is invalid, new keys cannot be created for encryption target",
}
encryptiontesting.ValidateOperatorClientConditions(ts, operatorClient, []operatorv1.OperatorCondition{expectedCondition})
},
expectedError: errors.New("secret encryption-key-kms-1 is invalid, new keys cannot be created for encryption target"),
expectedError: errors.New("secret openshift-config-managed/encryption-key-kms-1 is invalid, new keys cannot be created for encryption target"),
},

{
Expand Down Expand Up @@ -494,7 +495,8 @@ func TestGetCurrentModeAndExternalReason(t *testing.T) {

// act
target := keyController{unsupportedConfigPrefix: scenario.prefix, operatorClient: fakeOperatorClient, apiServerClient: fakeApiServerClient}
_, externalReason, err := target.getCurrentModeAndExternalReason(context.TODO())
ks, err := target.getCurrentEncryptionModeWithExternalReason(context.TODO())
externalReason := ks.ExternalReason

// validate
if err != nil {
Expand Down
3 changes: 2 additions & 1 deletion pkg/operator/encryption/controllers/state_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,11 @@ import (
"encoding/base64"
"errors"
"fmt"
clocktesting "k8s.io/utils/clock/testing"
"testing"
"time"

clocktesting "k8s.io/utils/clock/testing"

corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/equality"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down
Loading