Skip to content

Commit d82c48b

Browse files
ardagucluardaguclu
committed
Introduce kms to kms migration
1 parent c877160 commit d82c48b

3 files changed

Lines changed: 337 additions & 16 deletions

File tree

‎pkg/operator/encryption/controllers/key_controller.go‎

Lines changed: 29 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -198,7 +198,7 @@ func (c *keyController) checkAndCreateKeys(ctx context.Context, syncContext fact
198198

199199
var commonReason *string
200200
for gr, grKeys := range desiredEncryptionState {
201-
latestKeyID, internalReason, needed := needsNewKey(grKeys, currentMode, externalReason, encryptedGRs)
201+
latestKeyID, internalReason, needed := needsNewKey(grKeys, currentMode, externalReason, encryptedGRs, apiEncryptionConfiguration)
202202
if !needed {
203203
continue
204204
}
@@ -262,6 +262,26 @@ func (c *keyController) validateExistingSecret(ctx context.Context, keySecret *c
262262
return nil // we made this key earlier
263263
}
264264

265+
// kmsMigrationRequired reports whether the KMS config change between latest
266+
// (stored in the key secret) and current (from the APIServer CR) involves
267+
// migration-triggering fields that require a new encryption key.
268+
// Returns false when only in-place-safe fields differ (image, TLS, authentication)
269+
// or when configs are identical.
270+
func kmsMigrationRequired(latest, current configv1.KMSPluginConfig) bool {
271+
if latest.Type != current.Type {
272+
return true
273+
}
274+
if latest.Type == configv1.VaultKMSProvider {
275+
if latest.Vault.VaultAddress != current.Vault.VaultAddress ||
276+
latest.Vault.VaultNamespace != current.Vault.VaultNamespace ||
277+
latest.Vault.TransitMount != current.Vault.TransitMount ||
278+
latest.Vault.TransitKey != current.Vault.TransitKey {
279+
return true
280+
}
281+
}
282+
return false
283+
}
284+
265285
func (c *keyController) generateKeySecret(ctx context.Context, keyID uint64, currentMode state.Mode, apiServerEncryption configv1.APIServerEncryption, internalReason, externalReason string) (*corev1.Secret, error) {
266286
bs := crypto.ModeToNewKeyFunc[currentMode]()
267287
ks := state.KeyState{
@@ -337,7 +357,7 @@ func (c *keyController) getCurrentModeReasonAndEncryptionConfig(ctx context.Cont
337357

338358
// needsNewKey checks whether a new key must be created for the given resource. If true, it also returns the latest
339359
// used key ID and a reason string.
340-
func needsNewKey(grKeys state.GroupResourceState, currentMode state.Mode, externalReason string, encryptedGRs []schema.GroupResource) (uint64, string, bool) {
360+
func needsNewKey(grKeys state.GroupResourceState, currentMode state.Mode, externalReason string, encryptedGRs []schema.GroupResource, currentApiServerEncryption configv1.APIServerEncryption) (uint64, string, bool) {
341361
// we always need to have some encryption keys unless we are turned off
342362
if len(grKeys.ReadKeys) == 0 {
343363
return 0, "key-does-not-exist", currentMode != state.Identity
@@ -382,13 +402,14 @@ func needsNewKey(grKeys state.GroupResourceState, currentMode state.Mode, extern
382402

383403
if currentMode == state.KMS {
384404
// We are here because Encryption Mode is not changed
405+
// However, we need to create a new key if migration-triggering fields
406+
// in the KMS provider configuration have changed.
407+
if kmsMigrationRequired(latestKey.KMS.Plugin, currentApiServerEncryption.KMS) {
408+
return latestKeyID, "kms-provider-changed", true
409+
}
385410

386-
// For now in Tech Preview v1, we don't support configurational changes. Therefore,
387-
// it is pointless comparing the secrets.
388-
389-
// For KMS mode, we don't do time-based rotation. Therefore, we shortcut here
390-
// KMS keys are rotated externally by the KMS system.
391-
// Moreover, we don't trigger new key when external reason is changed.
411+
// For KMS mode, we don't do time-based rotation. KMS keys are rotated
412+
// externally by the KMS provider. Moreover, we don't trigger new key when external reason is changed.
392413
// Because it would lead to duplicate providers which is not allowed.
393414
return 0, "", false
394415
}

‎pkg/operator/encryption/controllers/key_controller_test.go‎

Lines changed: 269 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import (
99
"testing"
1010
"time"
1111

12+
"github.com/stretchr/testify/require"
1213
clocktesting "k8s.io/utils/clock/testing"
1314

1415
corev1 "k8s.io/api/core/v1"
@@ -702,6 +703,162 @@ func TestKeyController(t *testing.T) {
702703
}
703704
},
704705
},
706+
707+
{
708+
name: "creates a new KMS key when VaultAddress changes",
709+
targetGRs: []schema.GroupResource{
710+
{Group: "", Resource: "secrets"},
711+
},
712+
initialObjects: []runtime.Object{
713+
encryptiontesting.CreateDummyKubeAPIPod("kube-apiserver-1", "kms", "node-1"),
714+
encryptiontesting.CreateMigratedEncryptionKeySecretWithKMSPluginConfig("kms", []schema.GroupResource{{Group: "", Resource: "secrets"}}, 5, time.Now()),
715+
encryptiontesting.CreateVaultAppRoleSecret("vault-approle-secret", "test-role-id", "test-secret-id"),
716+
},
717+
apiServerObjects: []runtime.Object{func() runtime.Object {
718+
s := simpleAPIServer.DeepCopy()
719+
changedConfig := encryptiontesting.DefaultKMSPluginConfig.DeepCopy()
720+
changedConfig.Vault.VaultAddress = "https://vault-new.example.com"
721+
s.Spec.Encryption = configv1.APIServerEncryption{Type: "KMS", KMS: *changedConfig}
722+
return s
723+
}()},
724+
targetNamespace: "kms",
725+
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"},
726+
validateFunc: func(ts *testing.T, actions []clientgotesting.Action, targetNamespace string, targetGRs []schema.GroupResource) {
727+
for _, action := range actions {
728+
if action.Matches("create", "secrets") {
729+
createAction := action.(clientgotesting.CreateAction)
730+
actualSecret := createAction.GetObject().(*corev1.Secret)
731+
732+
if actualSecret.Annotations["encryption.apiserver.operator.openshift.io/mode"] != "KMS" {
733+
ts.Errorf("expected mode KMS, got %s", actualSecret.Annotations["encryption.apiserver.operator.openshift.io/mode"])
734+
}
735+
if actualSecret.Annotations["encryption.apiserver.operator.openshift.io/internal-reason"] != "secrets-kms-provider-changed" {
736+
ts.Errorf("unexpected internal reason: %s", actualSecret.Annotations["encryption.apiserver.operator.openshift.io/internal-reason"])
737+
}
738+
if actualSecret.Name != "encryption-key-kms-6" {
739+
ts.Errorf("expected key ID 6, got %s", actualSecret.Name)
740+
}
741+
742+
kmsProviderConfigData := actualSecret.Data["encryption.apiserver.operator.openshift.io-kms-plugin-config"]
743+
providerConfig, err := encoding.DecodeKMSPluginConfig(kmsProviderConfigData)
744+
if err != nil {
745+
ts.Fatalf("failed to encode KMS config: %v", err)
746+
}
747+
if providerConfig.Vault.VaultAddress != "https://vault-new.example.com" {
748+
ts.Errorf("expected new VaultAddress, got %s", providerConfig.Vault.VaultAddress)
749+
}
750+
return
751+
}
752+
}
753+
ts.Errorf("the secret wasn't created")
754+
},
755+
},
756+
757+
{
758+
name: "creates a new KMS key when TransitKey changes",
759+
targetGRs: []schema.GroupResource{
760+
{Group: "", Resource: "secrets"},
761+
},
762+
initialObjects: []runtime.Object{
763+
encryptiontesting.CreateDummyKubeAPIPod("kube-apiserver-1", "kms", "node-1"),
764+
encryptiontesting.CreateMigratedEncryptionKeySecretWithKMSPluginConfig("kms", []schema.GroupResource{{Group: "", Resource: "secrets"}}, 5, time.Now()),
765+
encryptiontesting.CreateVaultAppRoleSecret("vault-approle-secret", "test-role-id", "test-secret-id"),
766+
},
767+
apiServerObjects: []runtime.Object{func() runtime.Object {
768+
s := simpleAPIServer.DeepCopy()
769+
changedConfig := encryptiontesting.DefaultKMSPluginConfig.DeepCopy()
770+
changedConfig.Vault.TransitKey = "new-transit-key"
771+
s.Spec.Encryption = configv1.APIServerEncryption{Type: "KMS", KMS: *changedConfig}
772+
return s
773+
}()},
774+
targetNamespace: "kms",
775+
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"},
776+
validateFunc: func(ts *testing.T, actions []clientgotesting.Action, targetNamespace string, targetGRs []schema.GroupResource) {
777+
for _, action := range actions {
778+
if action.Matches("create", "secrets") {
779+
createAction := action.(clientgotesting.CreateAction)
780+
actualSecret := createAction.GetObject().(*corev1.Secret)
781+
782+
if actualSecret.Annotations["encryption.apiserver.operator.openshift.io/internal-reason"] != "secrets-kms-provider-changed" {
783+
ts.Errorf("unexpected internal reason: %s", actualSecret.Annotations["encryption.apiserver.operator.openshift.io/internal-reason"])
784+
}
785+
786+
kmsProviderConfigData := actualSecret.Data["encryption.apiserver.operator.openshift.io-kms-plugin-config"]
787+
providerConfig, err := encoding.DecodeKMSPluginConfig(kmsProviderConfigData)
788+
if err != nil {
789+
ts.Fatalf("failed to encode KMS config: %v", err)
790+
}
791+
if providerConfig.Vault.TransitKey != "new-transit-key" {
792+
ts.Errorf("expected new TransitKey, got %s", providerConfig.Vault.TransitKey)
793+
}
794+
return
795+
}
796+
}
797+
ts.Errorf("the secret wasn't created")
798+
},
799+
},
800+
801+
{
802+
name: "no-op when only KMSPluginImage changes (non-migration field)",
803+
targetGRs: []schema.GroupResource{
804+
{Group: "", Resource: "secrets"},
805+
},
806+
initialObjects: []runtime.Object{
807+
encryptiontesting.CreateDummyKubeAPIPod("kube-apiserver-1", "kms", "node-1"),
808+
encryptiontesting.CreateMigratedEncryptionKeySecretWithKMSPluginConfig("kms", []schema.GroupResource{{Group: "", Resource: "secrets"}}, 5, time.Now()),
809+
},
810+
apiServerObjects: []runtime.Object{func() runtime.Object {
811+
s := simpleAPIServer.DeepCopy()
812+
changedConfig := encryptiontesting.DefaultKMSPluginConfig.DeepCopy()
813+
changedConfig.Vault.KMSPluginImage = "registry.example.com/kms-plugin@sha256:0000000000000000000000000000000000000000000000000000000000000000"
814+
s.Spec.Encryption = configv1.APIServerEncryption{Type: "KMS", KMS: *changedConfig}
815+
return s
816+
}()},
817+
targetNamespace: "kms",
818+
expectedActions: []string{"list:pods:kms", "get:secrets:kms", "list:secrets:openshift-config-managed"},
819+
},
820+
821+
{
822+
name: "no-op when only Authentication changes (non-migration field)",
823+
targetGRs: []schema.GroupResource{
824+
{Group: "", Resource: "secrets"},
825+
},
826+
initialObjects: []runtime.Object{
827+
encryptiontesting.CreateDummyKubeAPIPod("kube-apiserver-1", "kms", "node-1"),
828+
encryptiontesting.CreateMigratedEncryptionKeySecretWithKMSPluginConfig("kms", []schema.GroupResource{{Group: "", Resource: "secrets"}}, 5, time.Now()),
829+
},
830+
apiServerObjects: []runtime.Object{func() runtime.Object {
831+
s := simpleAPIServer.DeepCopy()
832+
changedConfig := encryptiontesting.DefaultKMSPluginConfig.DeepCopy()
833+
changedConfig.Vault.Authentication.AppRole.Secret.Name = "new-approle-secret"
834+
s.Spec.Encryption = configv1.APIServerEncryption{Type: "KMS", KMS: *changedConfig}
835+
return s
836+
}()},
837+
targetNamespace: "kms",
838+
expectedActions: []string{"list:pods:kms", "get:secrets:kms", "list:secrets:openshift-config-managed"},
839+
},
840+
841+
{
842+
name: "no-op when only TLS changes (non-migration field)",
843+
targetGRs: []schema.GroupResource{
844+
{Group: "", Resource: "secrets"},
845+
},
846+
initialObjects: []runtime.Object{
847+
encryptiontesting.CreateDummyKubeAPIPod("kube-apiserver-1", "kms", "node-1"),
848+
encryptiontesting.CreateMigratedEncryptionKeySecretWithKMSPluginConfig("kms", []schema.GroupResource{{Group: "", Resource: "secrets"}}, 5, time.Now()),
849+
},
850+
apiServerObjects: []runtime.Object{func() runtime.Object {
851+
s := simpleAPIServer.DeepCopy()
852+
changedConfig := encryptiontesting.DefaultKMSPluginConfig.DeepCopy()
853+
changedConfig.Vault.TLS = configv1.VaultTLSConfig{
854+
CABundle: configv1.VaultConfigMapReference{Name: "my-ca"},
855+
}
856+
s.Spec.Encryption = configv1.APIServerEncryption{Type: "KMS", KMS: *changedConfig}
857+
return s
858+
}()},
859+
targetNamespace: "kms",
860+
expectedActions: []string{"list:pods:kms", "get:secrets:kms", "list:secrets:openshift-config-managed"},
861+
},
705862
}
706863

707864
for _, scenario := range scenarios {
@@ -987,3 +1144,115 @@ func TestGetCurrentModeReasonAndEncryptionConfig(t *testing.T) {
9871144
})
9881145
}
9891146
}
1147+
1148+
func TestNeedsNewKey(t *testing.T) {
1149+
baseConfig := &configv1.KMSPluginConfig{
1150+
Type: configv1.VaultKMSProvider,
1151+
Vault: configv1.VaultKMSPluginConfig{
1152+
KMSPluginImage: "registry.example.com/kms-plugin@sha256:abcdef1234567890abcdef1234567890abcdef1234567890abcdef1234567890",
1153+
VaultAddress: "https://vault.example.com",
1154+
VaultNamespace: "ns1",
1155+
TransitMount: "transit",
1156+
TransitKey: "my-key",
1157+
Authentication: configv1.VaultAuthentication{
1158+
Type: configv1.VaultAuthenticationTypeAppRole,
1159+
AppRole: configv1.VaultAppRoleAuthentication{
1160+
Secret: configv1.VaultSecretReference{Name: "vault-approle-secret"},
1161+
},
1162+
},
1163+
},
1164+
}
1165+
1166+
tests := []struct {
1167+
name string
1168+
latest *configv1.KMSPluginConfig
1169+
current *configv1.KMSPluginConfig
1170+
expected bool
1171+
}{
1172+
{
1173+
name: "identical configs",
1174+
latest: baseConfig.DeepCopy(),
1175+
current: baseConfig.DeepCopy(),
1176+
expected: false,
1177+
},
1178+
{
1179+
name: "different VaultAddress",
1180+
latest: baseConfig.DeepCopy(),
1181+
current: func() *configv1.KMSPluginConfig {
1182+
c := baseConfig.DeepCopy()
1183+
c.Vault.VaultAddress = "https://vault-new.example.com"
1184+
return c
1185+
}(),
1186+
expected: true,
1187+
},
1188+
{
1189+
name: "different VaultNamespace",
1190+
latest: baseConfig.DeepCopy(),
1191+
current: func() *configv1.KMSPluginConfig {
1192+
c := baseConfig.DeepCopy()
1193+
c.Vault.VaultNamespace = "ns2"
1194+
return c
1195+
}(),
1196+
expected: true,
1197+
},
1198+
{
1199+
name: "different TransitKey",
1200+
latest: baseConfig.DeepCopy(),
1201+
current: func() *configv1.KMSPluginConfig {
1202+
c := baseConfig.DeepCopy()
1203+
c.Vault.TransitKey = "new-key"
1204+
return c
1205+
}(),
1206+
expected: true,
1207+
},
1208+
{
1209+
name: "different TransitMount",
1210+
latest: baseConfig.DeepCopy(),
1211+
current: func() *configv1.KMSPluginConfig {
1212+
c := baseConfig.DeepCopy()
1213+
c.Vault.TransitMount = "custom-transit"
1214+
return c
1215+
}(),
1216+
expected: true,
1217+
},
1218+
{
1219+
name: "different KMSPluginImage only",
1220+
latest: baseConfig.DeepCopy(),
1221+
current: func() *configv1.KMSPluginConfig {
1222+
c := baseConfig.DeepCopy()
1223+
c.Vault.KMSPluginImage = "registry.example.com/kms-plugin@sha256:0000000000000000000000000000000000000000000000000000000000000000"
1224+
return c
1225+
}(),
1226+
expected: false,
1227+
},
1228+
{
1229+
name: "different TLS only",
1230+
latest: baseConfig.DeepCopy(),
1231+
current: func() *configv1.KMSPluginConfig {
1232+
c := baseConfig.DeepCopy()
1233+
c.Vault.TLS = configv1.VaultTLSConfig{
1234+
CABundle: configv1.VaultConfigMapReference{Name: "my-ca"},
1235+
}
1236+
return c
1237+
}(),
1238+
expected: false,
1239+
},
1240+
{
1241+
name: "different Authentication only",
1242+
latest: baseConfig.DeepCopy(),
1243+
current: func() *configv1.KMSPluginConfig {
1244+
c := baseConfig.DeepCopy()
1245+
c.Vault.Authentication.AppRole.Secret.Name = "new-secret"
1246+
return c
1247+
}(),
1248+
expected: false,
1249+
},
1250+
}
1251+
1252+
for _, tt := range tests {
1253+
t.Run(tt.name, func(t *testing.T) {
1254+
got := kmsMigrationRequired(*tt.latest, *tt.current)
1255+
require.Equal(t, tt.expected, got)
1256+
})
1257+
}
1258+
}

0 commit comments

Comments
 (0)