diff --git a/pkg/operator/encryption/controllers/key_controller.go b/pkg/operator/encryption/controllers/key_controller.go index b0e1403993..2d39542150 100644 --- a/pkg/operator/encryption/controllers/key_controller.go +++ b/pkg/operator/encryption/controllers/key_controller.go @@ -198,7 +198,7 @@ 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) + latestKeyID, internalReason, needed := needsNewKey(grKeys, currentMode, externalReason, encryptedGRs, apiEncryptionConfiguration) if !needed { continue } @@ -262,6 +262,26 @@ func (c *keyController) validateExistingSecret(ctx context.Context, keySecret *c return nil // we made this key earlier } +// kmsMigrationRequired reports whether the KMS config change between latest +// (stored in the key secret) and current (from the APIServer CR) involves +// migration-triggering fields that require a new encryption key. +// Returns false when only in-place-safe fields differ (image, TLS, authentication) +// or when configs are identical. +func kmsMigrationRequired(latest, current configv1.KMSPluginConfig) bool { + if latest.Type != current.Type { + return true + } + if latest.Type == configv1.VaultKMSProvider { + if latest.Vault.VaultAddress != current.Vault.VaultAddress || + latest.Vault.VaultNamespace != current.Vault.VaultNamespace || + latest.Vault.TransitMount != current.Vault.TransitMount || + latest.Vault.TransitKey != current.Vault.TransitKey { + return true + } + } + return false +} + func (c *keyController) generateKeySecret(ctx context.Context, keyID uint64, currentMode state.Mode, apiServerEncryption configv1.APIServerEncryption, internalReason, externalReason string) (*corev1.Secret, error) { bs := crypto.ModeToNewKeyFunc[currentMode]() ks := state.KeyState{ @@ -337,7 +357,7 @@ func (c *keyController) getCurrentModeReasonAndEncryptionConfig(ctx context.Cont // needsNewKey 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 needsNewKey(grKeys state.GroupResourceState, currentMode state.Mode, externalReason string, encryptedGRs []schema.GroupResource, currentApiServerEncryption configv1.APIServerEncryption) (uint64, string, 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 @@ -382,13 +402,14 @@ func needsNewKey(grKeys state.GroupResourceState, currentMode state.Mode, extern if currentMode == state.KMS { // We are here because Encryption Mode is not changed + // However, we need to create a new key if migration-triggering fields + // in the KMS provider configuration have changed. + if kmsMigrationRequired(latestKey.KMS.Plugin, currentApiServerEncryption.KMS) { + return latestKeyID, "kms-provider-changed", true + } - // For now in Tech Preview v1, we don't support configurational changes. Therefore, - // it is pointless comparing the secrets. - - // For KMS mode, we don't do time-based rotation. Therefore, we shortcut here - // KMS keys are rotated externally by the KMS system. - // Moreover, we don't trigger new key when external reason is changed. + // For KMS mode, we don't do time-based rotation. KMS keys are rotated + // externally by the KMS provider. Moreover, we don't trigger new key when external reason is changed. // Because it would lead to duplicate providers which is not allowed. return 0, "", false } diff --git a/pkg/operator/encryption/controllers/key_controller_test.go b/pkg/operator/encryption/controllers/key_controller_test.go index 9fdb0cbd0e..c6cb8552c5 100644 --- a/pkg/operator/encryption/controllers/key_controller_test.go +++ b/pkg/operator/encryption/controllers/key_controller_test.go @@ -9,6 +9,7 @@ import ( "testing" "time" + "github.com/stretchr/testify/require" clocktesting "k8s.io/utils/clock/testing" corev1 "k8s.io/api/core/v1" @@ -702,6 +703,162 @@ func TestKeyController(t *testing.T) { } }, }, + + { + name: "creates a new KMS key when VaultAddress changes", + targetGRs: []schema.GroupResource{ + {Group: "", Resource: "secrets"}, + }, + initialObjects: []runtime.Object{ + encryptiontesting.CreateDummyKubeAPIPod("kube-apiserver-1", "kms", "node-1"), + encryptiontesting.CreateMigratedEncryptionKeySecretWithKMSPluginConfig("kms", []schema.GroupResource{{Group: "", Resource: "secrets"}}, 5, time.Now()), + encryptiontesting.CreateVaultAppRoleSecret("vault-approle-secret", "test-role-id", "test-secret-id"), + }, + apiServerObjects: []runtime.Object{func() runtime.Object { + s := simpleAPIServer.DeepCopy() + changedConfig := encryptiontesting.DefaultKMSPluginConfig.DeepCopy() + changedConfig.Vault.VaultAddress = "https://vault-new.example.com" + s.Spec.Encryption = configv1.APIServerEncryption{Type: "KMS", KMS: *changedConfig} + return s + }()}, + targetNamespace: "kms", + expectedActions: []string{"list:pods:kms", "get:secrets:kms", "list:secrets:openshift-config-managed", "get:secrets:openshift-config", "create:secrets:openshift-config-managed", "create:events:kms"}, + validateFunc: func(ts *testing.T, actions []clientgotesting.Action, targetNamespace string, targetGRs []schema.GroupResource) { + for _, action := range actions { + if action.Matches("create", "secrets") { + createAction := action.(clientgotesting.CreateAction) + actualSecret := createAction.GetObject().(*corev1.Secret) + + if actualSecret.Annotations["encryption.apiserver.operator.openshift.io/mode"] != "KMS" { + ts.Errorf("expected mode KMS, got %s", actualSecret.Annotations["encryption.apiserver.operator.openshift.io/mode"]) + } + if actualSecret.Annotations["encryption.apiserver.operator.openshift.io/internal-reason"] != "secrets-kms-provider-changed" { + ts.Errorf("unexpected internal reason: %s", actualSecret.Annotations["encryption.apiserver.operator.openshift.io/internal-reason"]) + } + if actualSecret.Name != "encryption-key-kms-6" { + ts.Errorf("expected key ID 6, got %s", actualSecret.Name) + } + + kmsProviderConfigData := actualSecret.Data["encryption.apiserver.operator.openshift.io-kms-plugin-config"] + providerConfig, err := encoding.DecodeKMSPluginConfig(kmsProviderConfigData) + if err != nil { + ts.Fatalf("failed to encode KMS config: %v", err) + } + if providerConfig.Vault.VaultAddress != "https://vault-new.example.com" { + ts.Errorf("expected new VaultAddress, got %s", providerConfig.Vault.VaultAddress) + } + return + } + } + ts.Errorf("the secret wasn't created") + }, + }, + + { + name: "creates a new KMS key when TransitKey changes", + targetGRs: []schema.GroupResource{ + {Group: "", Resource: "secrets"}, + }, + initialObjects: []runtime.Object{ + encryptiontesting.CreateDummyKubeAPIPod("kube-apiserver-1", "kms", "node-1"), + encryptiontesting.CreateMigratedEncryptionKeySecretWithKMSPluginConfig("kms", []schema.GroupResource{{Group: "", Resource: "secrets"}}, 5, time.Now()), + encryptiontesting.CreateVaultAppRoleSecret("vault-approle-secret", "test-role-id", "test-secret-id"), + }, + apiServerObjects: []runtime.Object{func() runtime.Object { + s := simpleAPIServer.DeepCopy() + changedConfig := encryptiontesting.DefaultKMSPluginConfig.DeepCopy() + changedConfig.Vault.TransitKey = "new-transit-key" + s.Spec.Encryption = configv1.APIServerEncryption{Type: "KMS", KMS: *changedConfig} + return s + }()}, + targetNamespace: "kms", + expectedActions: []string{"list:pods:kms", "get:secrets:kms", "list:secrets:openshift-config-managed", "get:secrets:openshift-config", "create:secrets:openshift-config-managed", "create:events:kms"}, + validateFunc: func(ts *testing.T, actions []clientgotesting.Action, targetNamespace string, targetGRs []schema.GroupResource) { + for _, action := range actions { + if action.Matches("create", "secrets") { + createAction := action.(clientgotesting.CreateAction) + actualSecret := createAction.GetObject().(*corev1.Secret) + + if actualSecret.Annotations["encryption.apiserver.operator.openshift.io/internal-reason"] != "secrets-kms-provider-changed" { + ts.Errorf("unexpected internal reason: %s", actualSecret.Annotations["encryption.apiserver.operator.openshift.io/internal-reason"]) + } + + kmsProviderConfigData := actualSecret.Data["encryption.apiserver.operator.openshift.io-kms-plugin-config"] + providerConfig, err := encoding.DecodeKMSPluginConfig(kmsProviderConfigData) + if err != nil { + ts.Fatalf("failed to encode KMS config: %v", err) + } + if providerConfig.Vault.TransitKey != "new-transit-key" { + ts.Errorf("expected new TransitKey, got %s", providerConfig.Vault.TransitKey) + } + return + } + } + ts.Errorf("the secret wasn't created") + }, + }, + + { + name: "no-op when only KMSPluginImage changes (non-migration field)", + targetGRs: []schema.GroupResource{ + {Group: "", Resource: "secrets"}, + }, + initialObjects: []runtime.Object{ + encryptiontesting.CreateDummyKubeAPIPod("kube-apiserver-1", "kms", "node-1"), + encryptiontesting.CreateMigratedEncryptionKeySecretWithKMSPluginConfig("kms", []schema.GroupResource{{Group: "", Resource: "secrets"}}, 5, time.Now()), + }, + apiServerObjects: []runtime.Object{func() runtime.Object { + s := simpleAPIServer.DeepCopy() + changedConfig := encryptiontesting.DefaultKMSPluginConfig.DeepCopy() + changedConfig.Vault.KMSPluginImage = "registry.example.com/kms-plugin@sha256:0000000000000000000000000000000000000000000000000000000000000000" + s.Spec.Encryption = configv1.APIServerEncryption{Type: "KMS", KMS: *changedConfig} + return s + }()}, + targetNamespace: "kms", + expectedActions: []string{"list:pods:kms", "get:secrets:kms", "list:secrets:openshift-config-managed"}, + }, + + { + name: "no-op when only Authentication changes (non-migration field)", + targetGRs: []schema.GroupResource{ + {Group: "", Resource: "secrets"}, + }, + initialObjects: []runtime.Object{ + encryptiontesting.CreateDummyKubeAPIPod("kube-apiserver-1", "kms", "node-1"), + encryptiontesting.CreateMigratedEncryptionKeySecretWithKMSPluginConfig("kms", []schema.GroupResource{{Group: "", Resource: "secrets"}}, 5, time.Now()), + }, + apiServerObjects: []runtime.Object{func() runtime.Object { + s := simpleAPIServer.DeepCopy() + changedConfig := encryptiontesting.DefaultKMSPluginConfig.DeepCopy() + changedConfig.Vault.Authentication.AppRole.Secret.Name = "new-approle-secret" + s.Spec.Encryption = configv1.APIServerEncryption{Type: "KMS", KMS: *changedConfig} + return s + }()}, + targetNamespace: "kms", + expectedActions: []string{"list:pods:kms", "get:secrets:kms", "list:secrets:openshift-config-managed"}, + }, + + { + name: "no-op when only TLS changes (non-migration field)", + targetGRs: []schema.GroupResource{ + {Group: "", Resource: "secrets"}, + }, + initialObjects: []runtime.Object{ + encryptiontesting.CreateDummyKubeAPIPod("kube-apiserver-1", "kms", "node-1"), + encryptiontesting.CreateMigratedEncryptionKeySecretWithKMSPluginConfig("kms", []schema.GroupResource{{Group: "", Resource: "secrets"}}, 5, time.Now()), + }, + apiServerObjects: []runtime.Object{func() runtime.Object { + s := simpleAPIServer.DeepCopy() + changedConfig := encryptiontesting.DefaultKMSPluginConfig.DeepCopy() + changedConfig.Vault.TLS = configv1.VaultTLSConfig{ + CABundle: configv1.VaultConfigMapReference{Name: "my-ca"}, + } + s.Spec.Encryption = configv1.APIServerEncryption{Type: "KMS", KMS: *changedConfig} + return s + }()}, + targetNamespace: "kms", + expectedActions: []string{"list:pods:kms", "get:secrets:kms", "list:secrets:openshift-config-managed"}, + }, } for _, scenario := range scenarios { @@ -987,3 +1144,115 @@ func TestGetCurrentModeReasonAndEncryptionConfig(t *testing.T) { }) } } + +func TestNeedsNewKey(t *testing.T) { + baseConfig := &configv1.KMSPluginConfig{ + Type: configv1.VaultKMSProvider, + Vault: configv1.VaultKMSPluginConfig{ + KMSPluginImage: "registry.example.com/kms-plugin@sha256:abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890", + VaultAddress: "https://vault.example.com", + VaultNamespace: "ns1", + TransitMount: "transit", + TransitKey: "my-key", + Authentication: configv1.VaultAuthentication{ + Type: configv1.VaultAuthenticationTypeAppRole, + AppRole: configv1.VaultAppRoleAuthentication{ + Secret: configv1.VaultSecretReference{Name: "vault-approle-secret"}, + }, + }, + }, + } + + tests := []struct { + name string + latest *configv1.KMSPluginConfig + current *configv1.KMSPluginConfig + expected bool + }{ + { + name: "identical configs", + latest: baseConfig.DeepCopy(), + current: baseConfig.DeepCopy(), + expected: false, + }, + { + name: "different VaultAddress", + latest: baseConfig.DeepCopy(), + current: func() *configv1.KMSPluginConfig { + c := baseConfig.DeepCopy() + c.Vault.VaultAddress = "https://vault-new.example.com" + return c + }(), + expected: true, + }, + { + name: "different VaultNamespace", + latest: baseConfig.DeepCopy(), + current: func() *configv1.KMSPluginConfig { + c := baseConfig.DeepCopy() + c.Vault.VaultNamespace = "ns2" + return c + }(), + expected: true, + }, + { + name: "different TransitKey", + latest: baseConfig.DeepCopy(), + current: func() *configv1.KMSPluginConfig { + c := baseConfig.DeepCopy() + c.Vault.TransitKey = "new-key" + return c + }(), + expected: true, + }, + { + name: "different TransitMount", + latest: baseConfig.DeepCopy(), + current: func() *configv1.KMSPluginConfig { + c := baseConfig.DeepCopy() + c.Vault.TransitMount = "custom-transit" + return c + }(), + expected: true, + }, + { + name: "different KMSPluginImage only", + latest: baseConfig.DeepCopy(), + current: func() *configv1.KMSPluginConfig { + c := baseConfig.DeepCopy() + c.Vault.KMSPluginImage = "registry.example.com/kms-plugin@sha256:0000000000000000000000000000000000000000000000000000000000000000" + return c + }(), + expected: false, + }, + { + name: "different TLS only", + latest: baseConfig.DeepCopy(), + current: func() *configv1.KMSPluginConfig { + c := baseConfig.DeepCopy() + c.Vault.TLS = configv1.VaultTLSConfig{ + CABundle: configv1.VaultConfigMapReference{Name: "my-ca"}, + } + return c + }(), + expected: false, + }, + { + name: "different Authentication only", + latest: baseConfig.DeepCopy(), + current: func() *configv1.KMSPluginConfig { + c := baseConfig.DeepCopy() + c.Vault.Authentication.AppRole.Secret.Name = "new-secret" + return c + }(), + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := kmsMigrationRequired(*tt.latest, *tt.current) + require.Equal(t, tt.expected, got) + }) + } +} diff --git a/test/e2e-encryption/encryption_test.go b/test/e2e-encryption/encryption_test.go index 37f558e7bf..4d73007526 100644 --- a/test/e2e-encryption/encryption_test.go +++ b/test/e2e-encryption/encryption_test.go @@ -586,33 +586,64 @@ func TestEncryptionIntegration(tt *testing.T) { verifyKMSPlugins() verifyKMSSecretData() + t.Logf("KMS-to-KMS migration: change VaultAddress") + _, err = fakeApiServerClient.Patch(ctx, "cluster", types.MergePatchType, []byte(`{"spec":{"encryption":{"kms":{"vault":{"vaultAddress":"https://vault-new.example.com"}}}}}`), metav1.PatchOptions{}) + require.NoError(t, err) + waitForKeys(12) + kms13 := kmsPluginName("kubeapiservers", "13") + kms13Sched := kmsPluginName("kubeschedulers", "13") + waitForConfigs( + fmt.Sprintf("kubeapiservers.operator.openshift.io=kms:%s,kms:%s,aescbc:11,identity;kubeschedulers.operator.openshift.io=kms:%s,kms:%s,aescbc:11,identity", kms12, kms13, kms12Sched, kms13Sched), + fmt.Sprintf("kubeapiservers.operator.openshift.io=kms:%s,kms:%s,aescbc:11,identity;kubeschedulers.operator.openshift.io=kms:%s,kms:%s,aescbc:11,identity", kms13, kms12, kms13Sched, kms12Sched), + fmt.Sprintf("kubeapiservers.operator.openshift.io=kms:%s,kms:%s,identity;kubeschedulers.operator.openshift.io=kms:%s,kms:%s,identity", kms13, kms12, kms13Sched, kms12Sched), + ) + waitForMigration("13") + waitForConditionStatus("Encrypted", operatorv1.ConditionTrue) + + t.Logf("Verify KMS-to-KMS migration key secret contains updated plugin config") + kmsKeySecret13, err := kubeClient.CoreV1().Secrets("openshift-config-managed").Get(ctx, fmt.Sprintf("encryption-key-%s-13", component), metav1.GetOptions{}) + require.NoError(t, err) + kmsPluginConfigData13 := kmsKeySecret13.Data["encryption.apiserver.operator.openshift.io-kms-plugin-config"] + require.NotEmpty(t, kmsPluginConfigData13) + pluginConfig13, err := encoding.DecodeKMSPluginConfig(kmsPluginConfigData13) + require.NoError(t, err) + require.Equal(t, "https://vault-new.example.com", pluginConfig13.Vault.VaultAddress) + require.Equal(t, "test-transit-key", pluginConfig13.Vault.TransitKey) + + t.Logf("KMS non-migration change: only KMSPluginImage changes (no new key expected)") + _, err = fakeApiServerClient.Patch(ctx, "cluster", types.MergePatchType, []byte(`{"spec":{"encryption":{"type":"KMS","kms":{"type":"Vault","vault":{"kmsPluginImage":"registry.example.com/kms-plugin@sha256:0000000000000000000000000000000000000000000000000000000000000000","vaultAddress":"https://vault-new.example.com","authentication":{"type":"AppRole","appRole":{"secret":{"name":"vault-approle-secret"}}},"transitKey":"test-transit-key"}}}}}`), metav1.PatchOptions{}) + require.NoError(t, err) + time.Sleep(5 * time.Second) + waitForKeys(12) + waitForConditionStatus("Encrypted", operatorv1.ConditionTrue) + t.Logf("Delete the encryption-config while in KMS mode") _, err = kubeClient.CoreV1().Secrets("openshift-config-managed").Patch(ctx, fmt.Sprintf("encryption-config-%s", component), types.JSONPatchType, []byte(`[{"op":"remove","path":"/metadata/finalizers"}]`), metav1.PatchOptions{}) require.NoError(t, err) err = kubeClient.CoreV1().Secrets("openshift-config-managed").Delete(ctx, fmt.Sprintf("encryption-config-%s", component), metav1.DeleteOptions{}) require.NoError(t, err) waitForConfigs( - fmt.Sprintf("kubeapiservers.operator.openshift.io=kms:%s,aescbc:11,identity;kubeschedulers.operator.openshift.io=kms:%s,aescbc:11,identity", kms12, kms12Sched), + fmt.Sprintf("kubeapiservers.operator.openshift.io=kms:%s,kms:%s,identity;kubeschedulers.operator.openshift.io=kms:%s,kms:%s,identity", kms13, kms12, kms13Sched, kms12Sched), ) waitForConditionStatus("Encrypted", operatorv1.ConditionTrue) t.Logf("Delete the operand config while in KMS mode") deployer.DeleteOperandConfig() waitForConfigs( - // kms12 is migrated and hence only one needed, but we rotate through identity - fmt.Sprintf("kubeapiservers.operator.openshift.io=identity,kms:%s,aescbc:11;kubeschedulers.operator.openshift.io=identity,kms:%s,aescbc:11", kms12, kms12Sched), - // kms12 is migrated, plus one backed key (11) - fmt.Sprintf("kubeapiservers.operator.openshift.io=kms:%s,aescbc:11,identity;kubeschedulers.operator.openshift.io=kms:%s,aescbc:11,identity", kms12, kms12Sched), + // kms13 is migrated and hence only one needed, but we rotate through identity + fmt.Sprintf("kubeapiservers.operator.openshift.io=identity,kms:%s,kms:%s;kubeschedulers.operator.openshift.io=identity,kms:%s,kms:%s", kms13, kms12, kms13Sched, kms12Sched), + // kms13 is migrated, plus one backed key (12) + fmt.Sprintf("kubeapiservers.operator.openshift.io=kms:%s,kms:%s,identity;kubeschedulers.operator.openshift.io=kms:%s,kms:%s,identity", kms13, kms12, kms13Sched, kms12Sched), ) waitForConditionStatus("Encrypted", operatorv1.ConditionTrue) t.Logf("Switch to identity from KMS") _, err = fakeApiServerClient.Patch(ctx, "cluster", types.MergePatchType, []byte(`{"spec":{"encryption":{"type":"identity","kms":null}}}`), metav1.PatchOptions{}) require.NoError(t, err) - waitForKeys(12) + waitForKeys(13) waitForConfigs( - fmt.Sprintf("kubeapiservers.operator.openshift.io=kms:%s,aescbc:11,identity,aesgcm:13;kubeschedulers.operator.openshift.io=kms:%s,aescbc:11,identity,aesgcm:13", kms12, kms12Sched), - fmt.Sprintf("kubeapiservers.operator.openshift.io=identity,kms:%s,aescbc:11,aesgcm:13;kubeschedulers.operator.openshift.io=identity,kms:%s,aescbc:11,aesgcm:13", kms12, kms12Sched), + fmt.Sprintf("kubeapiservers.operator.openshift.io=kms:%s,kms:%s,identity,aesgcm:14;kubeschedulers.operator.openshift.io=kms:%s,kms:%s,identity,aesgcm:14", kms13, kms12, kms13Sched, kms12Sched), + fmt.Sprintf("kubeapiservers.operator.openshift.io=identity,kms:%s,kms:%s,aesgcm:14;kubeschedulers.operator.openshift.io=identity,kms:%s,kms:%s,aesgcm:14", kms13, kms12, kms13Sched, kms12Sched), ) waitForConditionStatus("Encrypted", operatorv1.ConditionFalse) }