Skip to content

CNTRLPLANE-3237: Error out when encryption-config Secret contains mismatching KMS providers #2202

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.

Sign up for GitHub

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

Merged
openshift-merge-bot merged 1 commit into from
May 7, 2026
Merged

CNTRLPLANE-3237: Error out when encryption-config Secret contains mismatching KMS providers #2202

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 4 additions & 1 deletion pkg/operator/encryption/controllers/migration_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,10 @@ func (c *migrationController) migrateKeysIfNeededAndRevisionStable(ctx context.C
return nil, err
}
currentState, _ := encryptiondata.ToEncryptionState(currentEncryptionConfig, encryptionSecrets)
desiredEncryptedSecretData := encryptiondata.FromEncryptionState(desiredEncryptionState)
desiredEncryptedSecretData, err := encryptiondata.FromEncryptionState(desiredEncryptionState)
if err != nil {
return nil, err
}

// no storage migration until config is stable
if !reflect.DeepEqual(currentEncryptionConfig.Encryption.Resources, desiredEncryptedSecretData.Encryption.Resources) {
Expand Down
5 changes: 4 additions & 1 deletion pkg/operator/encryption/controllers/state_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,10 @@ func (c *stateController) generateAndApplyCurrentEncryptionConfigSecret(ctx cont
return nil
}

desiredSecretData := encryptiondata.FromEncryptionState(desiredEncryptionState)
desiredSecretData, err := encryptiondata.FromEncryptionState(desiredEncryptionState)
if err != nil {
return err
}
changed, err := c.applyEncryptionConfigSecret(ctx, desiredSecretData, recorder)
if err != nil {
return err
Expand Down
13 changes: 10 additions & 3 deletions pkg/operator/encryption/encryptiondata/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"strings"

corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/equality"
"k8s.io/apimachinery/pkg/runtime/schema"
apiserverconfigv1 "k8s.io/apiserver/pkg/apis/apiserver/v1"
"k8s.io/klog/v2"
Expand Down Expand Up @@ -36,7 +37,7 @@ func (c *Config) HasEncryptionConfiguration() bool {
}

// FromEncryptionState converts encryption state to Config.
func FromEncryptionState(encryptionState map[schema.GroupResource]state.GroupResourceState) *Config {
func FromEncryptionState(encryptionState map[schema.GroupResource]state.GroupResourceState) (*Config, error) {
resourceConfigs := make([]apiserverconfigv1.ResourceConfiguration, 0, len(encryptionState))
var kmsProviders map[string]*configv1.KMSConfig

Expand All @@ -56,7 +57,13 @@ func FromEncryptionState(encryptionState map[schema.GroupResource]state.GroupRes
if kmsProviders == nil {
kmsProviders = map[string]*configv1.KMSConfig{}
}
if _, exists := kmsProviders[key.Key.Name]; !exists {
if provider, exists := kmsProviders[key.Key.Name]; exists {
// Sanity check: the same keyID seen from a different resource must carry
// an identical provider config, since they originate from the same Key Secret.
if !equality.Semantic.DeepEqual(provider, key.KMSConfig.Provider) {
return nil, fmt.Errorf("KMS provider config mismatch for keyID %s: configs from different resources must be identical", key.Key.Name)

@bertinatto bertinatto May 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

in what scenarios can this happen? When the user manually change the secrets? Or when there's a bug in our code?

@ardaguclu ardaguclu May 6, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

When the user manually change the secrets?

I think mostly this can cause

@p0lyn0mial p0lyn0mial May 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Or when there's a bug in our code?

that too, that's why we want to add an extra check.

}
} else {
kmsProviders[key.Key.Name] = key.KMSConfig.Provider
}
}
Expand All @@ -71,7 +78,7 @@ func FromEncryptionState(encryptionState map[schema.GroupResource]state.GroupRes
return &Config{
Encryption: &apiserverconfigv1.EncryptionConfiguration{Resources: resourceConfigs},
KMSProviders: kmsProviders,
}
}, nil
}

// ToEncryptionState converts config to state.
Expand Down
95 changes: 94 additions & 1 deletion pkg/operator/encryption/encryptiondata/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -657,7 +657,10 @@ func TestFromEncryptionState(t *testing.T) {
}
grState[gr] = ks
}
actualOutput := encryptiondata.FromEncryptionState(grState)
actualOutput, err := encryptiondata.FromEncryptionState(grState)
if err != nil {
t.Fatalf("unexpected error from FromEncryptionState: %v", err)
}
expectedOutput := scenario.makeOutput(scenario.writeKeyIn, scenario.readKeysIn)

if !cmp.Equal(expectedOutput, actualOutput.Encryption.Resources) {
Expand Down Expand Up @@ -705,6 +708,96 @@ func newFakeIdentityKeyForTest() []byte {
return make([]byte, 16)
}

func TestFromEncryptionStateKMSProviderConfigValidation(t *testing.T) {
tests := []struct {
name string
encryptionState map[schema.GroupResource]state.GroupResourceState
expectedErr string
}{
{
name: "matching provider configs across resources",
encryptionState: map[schema.GroupResource]state.GroupResourceState{
{Resource: "secrets"}: {
ReadKeys: []state.KeyState{{
Key: apiserverconfigv1.Key{Name: "1", Secret: "AAAAAAAAAAAAAAAAAAAAAA=="},
Mode: state.KMS,
KMSConfig: &state.KMSConfig{
Encryption: &apiserverconfigv1.KMSConfiguration{APIVersion: "v2", Name: "1", Endpoint: "unix:///var/run/kmsplugin/kms-1.sock"},
Provider: encryptiontesting.DefaultKMSProviderConfig,
},
}},
},
{Resource: "configmaps"}: {
ReadKeys: []state.KeyState{{
Key: apiserverconfigv1.Key{Name: "1", Secret: "AAAAAAAAAAAAAAAAAAAAAA=="},
Mode: state.KMS,
KMSConfig: &state.KMSConfig{
Encryption: &apiserverconfigv1.KMSConfiguration{APIVersion: "v2", Name: "1", Endpoint: "unix:///var/run/kmsplugin/kms-1.sock"},
Provider: encryptiontesting.DefaultKMSProviderConfig,
},
Comment on lines +718 to +737

@coderabbitai coderabbitai Bot May 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Use distinct-but-equal provider configs in the “matching” test case.

Line 726 and Line 736 currently reuse the same *configv1.KMSConfig pointer. That can let a pointer-identity bug pass. Please construct two separate but equivalent provider config objects for the two resources.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/operator/encryption/encryptiondata/config_test.go` around lines 718 -
737, The test "matching provider configs across resources" currently reuses the
same *configv1.KMSConfig pointer via encryptiontesting.DefaultKMSProviderConfig
for both resources; create two separate but equal provider config objects
instead (e.g., construct a new apiserverconfigv1.KMSConfiguration/Provider
object for the secrets entry and a distinct one with the same fields for the
configmaps entry) so that state.KMSConfig.KMSConfig pointers are different while
values match; update the encryptionState entries' KMSConfig.Provider fields to
reference these two distinct instances rather than the single shared pointer.

}},
},
},
},
{
name: "mismatched provider configs across resources",
encryptionState: map[schema.GroupResource]state.GroupResourceState{
{Resource: "secrets"}: {
ReadKeys: []state.KeyState{{
Key: apiserverconfigv1.Key{Name: "1", Secret: "AAAAAAAAAAAAAAAAAAAAAA=="},
Mode: state.KMS,
KMSConfig: &state.KMSConfig{
Encryption: &apiserverconfigv1.KMSConfiguration{APIVersion: "v2", Name: "1", Endpoint: "unix:///var/run/kmsplugin/kms-1.sock"},
Provider: &configv1.KMSConfig{
Type: configv1.VaultKMSProvider,
Vault: configv1.VaultKMSConfig{
VaultAddress: "https://vault-a.example.com",
TransitKey: "key-a",
},
},
},
}},
},
{Resource: "configmaps"}: {
ReadKeys: []state.KeyState{{
Key: apiserverconfigv1.Key{Name: "1", Secret: "AAAAAAAAAAAAAAAAAAAAAA=="},
Mode: state.KMS,
KMSConfig: &state.KMSConfig{
Encryption: &apiserverconfigv1.KMSConfiguration{APIVersion: "v2", Name: "1", Endpoint: "unix:///var/run/kmsplugin/kms-1.sock"},
Provider: &configv1.KMSConfig{
Type: configv1.VaultKMSProvider,
Vault: configv1.VaultKMSConfig{
VaultAddress: "https://vault-b.example.com",
TransitKey: "key-b",
},
},
},
}},
},
},
expectedErr: `KMS provider config mismatch for keyID 1: configs from different resources must be identical`,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
_, err := encryptiondata.FromEncryptionState(tt.encryptionState)
if tt.expectedErr != "" {
if err == nil {
t.Fatal("expected error, got nil")
}
if err.Error() != tt.expectedErr {
t.Fatalf("unexpected error:\n got: %v\n expected: %v", err, tt.expectedErr)
}
return
}
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
})
}
}

func TestSecretRoundtrip(t *testing.T) {
tests := []struct {
name string
Expand Down
5 changes: 4 additions & 1 deletion pkg/operator/encryption/statemachine/transition_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,10 @@ func TestGetDesiredEncryptionState(t *testing.T) {
}
expected := expected.DeepCopy()
expected.TypeMeta = metav1.TypeMeta{}
secretData := encryptiondata.FromEncryptionState(state)
secretData, err := encryptiondata.FromEncryptionState(state)
if err != nil {
ts.Fatalf("unexpected error from FromEncryptionState: %v", err)
}
if !reflect.DeepEqual(expected, secretData.Encryption) {
ts.Errorf("unexpected encryption config (A: expected, B: got):\n%s", diff.ObjectDiff(expected, secretData.Encryption))
}
Expand Down