Skip to content

encryption: fix freezing key controller #615

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-robot merged 4 commits into from
Nov 28, 2019
Merged

encryption: fix freezing key controller #615

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9e1ce74
encryption: add key controller test to progress with write and read key
sttts Nov 27, 2019
654fd05
encryption: unfreeze key controller after two keys
sttts Nov 27, 2019
181c0e9
encryption: adapt e2e integration test
sttts Nov 27, 2019
2977f17
encryption: fix recovery case while preserving last read key
sttts Nov 28, 2019
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
11 changes: 8 additions & 3 deletions pkg/operator/encryption/controllers/key_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,9 +309,14 @@ func needsNewKey(grKeys state.GroupResourceState, currentMode state.Mode, extern
return latestKeyID, fmt.Sprintf("encryption-config-key-%d-not-backed-by-secret", latestKeyID), true
}

// if the length of read secrets is more than one (i.e. we have more than just the write key),
// then we haven't successfully migrated and removed old keys so you should wait before generating more keys.
if len(grKeys.ReadKeys) > 1 {
// check that we have pruned read-keys: the write-keys, plus at most one more backed read-key (potentially some unbacked once before)
backedKeys := 0
for _, rk := range grKeys.ReadKeys {
if rk.Backed {
backedKeys++
}
}
if backedKeys > 2 {
return 0, "", false
}

Expand Down
44 changes: 44 additions & 0 deletions pkg/operator/encryption/controllers/key_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package controllers

import (
"encoding/base64"
"errors"
"fmt"
"testing"
Expand All @@ -12,6 +13,7 @@ import (
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/apimachinery/pkg/util/diff"
apiserverconfigv1 "k8s.io/apiserver/pkg/apis/config/v1"
"k8s.io/client-go/kubernetes/fake"
clientgotesting "k8s.io/client-go/testing"

Expand Down Expand Up @@ -210,6 +212,48 @@ func TestKeyController(t *testing.T) {
},
},

{
name: "create a new write key when the previous key expired and another read key exists",
Copy link
Copy Markdown
Contributor

@p0lyn0mial p0lyn0mial Nov 28, 2019

Choose a reason for hiding this comment

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

shouldn't we validate if a new key was created? (validateFunc function)

targetGRs: []schema.GroupResource{
{Group: "", Resource: "secrets"},
},
initialObjects: []runtime.Object{
encryptiontesting.CreateDummyKubeAPIPod("kube-apiserver-1", "kms", "node-1"),
encryptiontesting.CreateExpiredMigratedEncryptionKeySecretWithRawKey("kms", []schema.GroupResource{{Group: "", Resource: "secrets"}}, 6, []byte("61def964fb967f5d7c44a2af8dab6865")),
encryptiontesting.CreateEncryptionKeySecretWithRawKey("kms", nil, 5, []byte("51def964fb967f5d7c44a2af8dab6865")),
func() *corev1.Secret {
keysResForSecrets := encryptiontesting.EncryptionKeysResourceTuple{
Resource: "secrets",
Keys: []apiserverconfigv1.Key{
{
Name: "6",
Secret: base64.StdEncoding.EncodeToString([]byte("61def964fb967f5d7c44a2af8dab6865")),
},
{
Name: "5",
Secret: base64.StdEncoding.EncodeToString([]byte("51def964fb967f5d7c44a2af8dab6865")),
},
},
}

ec := encryptiontesting.CreateEncryptionCfgWithWriteKey([]encryptiontesting.EncryptionKeysResourceTuple{keysResForSecrets})
ecs := createEncryptionCfgSecret(t, "kms", "1", ec)
ecs.APIVersion = corev1.SchemeGroupVersion.String()

return ecs
}(),
},
apiServerObjects: apiServerAesCBC,
targetNamespace: "kms",
expectedActions: []string{
"list:pods:kms",
"get:secrets:kms",
"list:secrets:openshift-config-managed",
"create:secrets:openshift-config-managed",
Copy link
Copy Markdown
Contributor

@p0lyn0mial p0lyn0mial Nov 28, 2019

Choose a reason for hiding this comment

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

this only says that something was created ;)

Copy link
Copy Markdown
Contributor Author

@sttts sttts Nov 28, 2019

Choose a reason for hiding this comment

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

that's a general issue in this test, isn't it? But what else do we create in the key controller than keys? :)

Copy link
Copy Markdown
Contributor

@p0lyn0mial p0lyn0mial Nov 28, 2019

Choose a reason for hiding this comment

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

yeah, hard to beat that argument :)

"create:events:kms",
},
},

{
name: "no-op when the previous key was migrated and the current one is valid but hasn't been observed",
targetGRs: []schema.GroupResource{
Expand Down
6 changes: 3 additions & 3 deletions pkg/operator/encryption/controllers/prune_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,12 @@ func TestPruneController(t *testing.T) {
},

{
name: "14 keys were migrated, 1 of them is used, 10 are kept, the 3 most recent are pruned",
name: "15 keys were migrated, 2 of them are used, 10 are kept, the 3 most oldest are pruned",
targetNamespace: "kms",
targetGRs: []schema.GroupResource{
{Group: "", Resource: "secrets"},
},
initialSecrets: createMigratedEncryptionKeySecretsWithRndKey(t, 14, "kms", "secrets"),
initialSecrets: createMigratedEncryptionKeySecretsWithRndKey(t, 15, "kms", "secrets"),
expectedActions: []string{
"list:pods:kms",
"get:secrets:kms",
Expand Down Expand Up @@ -144,7 +144,7 @@ func TestPruneController(t *testing.T) {
}

encryptionConfig := func() *corev1.Secret {
additionalReadKeys := state.KeysWithPotentiallyPersistedData(scenario.targetGRs, state.SortRecentFirst(initialKeys))
additionalReadKeys := state.KeysWithPotentiallyPersistedDataAndNextReadKey(scenario.targetGRs, state.SortRecentFirst(initialKeys))
var additionaConfigReadKeys []apiserverconfigv1.Key
for _, rk := range additionalReadKeys {
additionaConfigReadKeys = append(additionaConfigReadKeys, apiserverconfigv1.Key{
Expand Down
10 changes: 7 additions & 3 deletions pkg/operator/encryption/state/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,15 @@ func MigratedFor(grs []schema.GroupResource, km KeyState) (ok bool, missing []sc
return true, nil, ""
}

// KeysWithPotentiallyPersistedData returns the minimal, recent secrets which have migrated all given GRs.
func KeysWithPotentiallyPersistedData(grs []schema.GroupResource, recentFirstSortedKeys []KeyState) []KeyState {
// KeysWithPotentiallyPersistedDataAndNextReadKey returns the minimal, recent secrets which have migrated all given GRs.
func KeysWithPotentiallyPersistedDataAndNextReadKey(grs []schema.GroupResource, recentFirstSortedKeys []KeyState) []KeyState {
for i, k := range recentFirstSortedKeys {
if allMigrated, missing, _ := MigratedFor(grs, k); allMigrated {
return recentFirstSortedKeys[:i+1]
if i+1 < len(recentFirstSortedKeys) {
Copy link
Copy Markdown
Contributor

@p0lyn0mial p0lyn0mial Nov 28, 2019

Choose a reason for hiding this comment

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

this looks to be safe, we could have a unit test for it.

Copy link
Copy Markdown
Contributor Author

@sttts sttts Nov 28, 2019

Choose a reason for hiding this comment

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

what do you want to test?

Copy link
Copy Markdown
Contributor Author

@sttts sttts Nov 28, 2019

Choose a reason for hiding this comment

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

we have tests that "one more read-key" is fine. So this is covered. The rest here is just Golang standard behaviour.

Copy link
Copy Markdown
Contributor

@p0lyn0mial p0lyn0mial Nov 28, 2019

Choose a reason for hiding this comment

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

sgtm.

return recentFirstSortedKeys[:i+2]
} else {
return recentFirstSortedKeys[:i+1]
}
} else {
// continue with keys we haven't found a migration key for yet
grs = missing
Expand Down
23 changes: 16 additions & 7 deletions pkg/operator/encryption/statemachine/transition.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ func getDesiredEncryptionState(oldEncryptionConfig *apiserverconfigv1.Encryption
// potentially persisted data keys.
currentlyEncryptedGRs = toBeEncryptedGRs
}
expectedReadSecrets := state.KeysWithPotentiallyPersistedData(currentlyEncryptedGRs, backedKeys)
expectedReadSecrets := state.KeysWithPotentiallyPersistedDataAndNextReadKey(currentlyEncryptedGRs, backedKeys)
for gr, grState := range desiredEncryptionState {
changed := false
for _, expected := range expectedReadSecrets {
Expand All @@ -126,7 +126,7 @@ func getDesiredEncryptionState(oldEncryptionConfig *apiserverconfigv1.Encryption
}
if !found {
// Just adding raw key without trusting any metadata on it
grState.ReadKeys = append(grState.ReadKeys, expected)
grState.ReadKeys = state.SortRecentFirst(append(grState.ReadKeys, expected)) // sort into right position
changed = true
allReadSecretsAsExpected = false
klog.V(4).Infof("encrypted resource %s misses read key %s", gr, expected.Key.Name)
Expand Down Expand Up @@ -178,13 +178,22 @@ func getDesiredEncryptionState(oldEncryptionConfig *apiserverconfigv1.Encryption
klog.V(4).Infof(reason)
return desiredEncryptionState
}
writeAndLastReadKey := []state.KeyState{writeKey}
if len(backedKeys) >= 2 {
writeAndLastReadKey = append(writeAndLastReadKey, backedKeys[1])
}
for gr := range desiredEncryptionState {
grState := desiredEncryptionState[gr]
grState.ReadKeys = writeAndLastReadKey

// cut down read keys to all expected read keys, and everything in between
if len(expectedReadSecrets) == 0 {
grState.ReadKeys = []state.KeyState{}
} else {
lastExpected := expectedReadSecrets[len(expectedReadSecrets)-1]
for i, rk := range grState.ReadKeys {
if state.EqualKeyAndEqualID(&rk, &lastExpected) {
grState.ReadKeys = grState.ReadKeys[:i+1]
break
}
}
}

desiredEncryptionState[gr] = grState
}
klog.V(4).Infof("write key %s set as sole write key", writeKey.Key.Name)
Expand Down
32 changes: 32 additions & 0 deletions pkg/operator/encryption/statemachine/transition_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,6 +356,14 @@ func TestGetDesiredEncryptionState(t *testing.T) {
Secret: base64.StdEncoding.EncodeToString([]byte("3cbfbe7d76876e076b076c659cd895ff")),
}},
},
}, {
// one more read key for backup/recovery
AESCBC: &apiserverconfigv1.AESConfiguration{
Keys: []apiserverconfigv1.Key{{
Name: "2",
Secret: base64.StdEncoding.EncodeToString([]byte("2b234b23cb23c4b2cb24cb24bcbffbca")),
}},
},
}},
},
{
Expand Down Expand Up @@ -383,6 +391,14 @@ func TestGetDesiredEncryptionState(t *testing.T) {
Secret: base64.StdEncoding.EncodeToString([]byte("3cbfbe7d76876e076b076c659cd895ff")),
}},
},
}, {
// one more read key for backup/recovery
AESCBC: &apiserverconfigv1.AESConfiguration{
Keys: []apiserverconfigv1.Key{{
Name: "2",
Secret: base64.StdEncoding.EncodeToString([]byte("2b234b23cb23c4b2cb24cb24bcbffbca")),
}},
},
}},
},
}}),
Expand Down Expand Up @@ -483,6 +499,14 @@ func TestGetDesiredEncryptionState(t *testing.T) {
Secret: base64.StdEncoding.EncodeToString([]byte("3cbfbe7d76876e076b076c659cd895ff")),
}},
},
}, {
// one more read key for backup/recovery
AESCBC: &apiserverconfigv1.AESConfiguration{
Keys: []apiserverconfigv1.Key{{
Name: "2",
Secret: base64.StdEncoding.EncodeToString([]byte("2b234b23cb23c4b2cb24cb24bcbffbca")),
}},
},
}, {
Identity: &apiserverconfigv1.IdentityConfiguration{},
}},
Expand Down Expand Up @@ -510,6 +534,14 @@ func TestGetDesiredEncryptionState(t *testing.T) {
Secret: base64.StdEncoding.EncodeToString([]byte("3cbfbe7d76876e076b076c659cd895ff")),
}},
},
}, {
// one more read key for backup/recovery
AESCBC: &apiserverconfigv1.AESConfiguration{
Keys: []apiserverconfigv1.Key{{
Name: "2",
Secret: base64.StdEncoding.EncodeToString([]byte("2b234b23cb23c4b2cb24cb24bcbffbca")),
}},
},
}, {
Identity: &apiserverconfigv1.IdentityConfiguration{},
}},
Expand Down
31 changes: 17 additions & 14 deletions test/e2e-encryption/encryption_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,6 @@ func TestEncryptionIntegration(tt *testing.T) {
waitForConfigs(
"kubeapiservers.operator.openshift.io=aescbc:1,identity,aesgcm:2;kubeschedulers.operator.openshift.io=aescbc:1,identity,aesgcm:2",
"kubeapiservers.operator.openshift.io=identity,aescbc:1,aesgcm:2;kubeschedulers.operator.openshift.io=identity,aescbc:1,aesgcm:2",
"kubeapiservers.operator.openshift.io=identity,aesgcm:2;kubeschedulers.operator.openshift.io=identity,aesgcm:2",
)

t.Logf("Switch to empty mode")
Expand All @@ -220,9 +219,9 @@ func TestEncryptionIntegration(tt *testing.T) {
require.NoError(t, err)
waitForKeys(3)
waitForConfigs(
"kubeapiservers.operator.openshift.io=identity,aescbc:3,aesgcm:2;kubeschedulers.operator.openshift.io=identity,aescbc:3,aesgcm:2",
"kubeapiservers.operator.openshift.io=identity,aescbc:3,aescbc:1,aesgcm:2;kubeschedulers.operator.openshift.io=identity,aescbc:3,aescbc:1,aesgcm:2",
"kubeapiservers.operator.openshift.io=aescbc:3,aescbc:1,identity,aesgcm:2;kubeschedulers.operator.openshift.io=aescbc:3,aescbc:1,identity,aesgcm:2",
"kubeapiservers.operator.openshift.io=aescbc:3,identity,aesgcm:2;kubeschedulers.operator.openshift.io=aescbc:3,identity,aesgcm:2",
"kubeapiservers.operator.openshift.io=aescbc:3,identity;kubeschedulers.operator.openshift.io=aescbc:3,identity",
)

t.Logf("Setting external reason")
Expand All @@ -242,28 +241,28 @@ func TestEncryptionIntegration(tt *testing.T) {
setExternalReason("a")
waitForKeys(4)
waitForConfigs(
"kubeapiservers.operator.openshift.io=aescbc:3,aescbc:4,identity;kubeschedulers.operator.openshift.io=aescbc:3,aescbc:4,identity",
"kubeapiservers.operator.openshift.io=aescbc:3,aescbc:4,identity,aesgcm:2;kubeschedulers.operator.openshift.io=aescbc:3,aescbc:4,identity,aesgcm:2",
"kubeapiservers.operator.openshift.io=aescbc:4,aescbc:3,identity,aesgcm:2;kubeschedulers.operator.openshift.io=aescbc:4,aescbc:3,identity,aesgcm:2",
"kubeapiservers.operator.openshift.io=aescbc:4,aescbc:3,identity;kubeschedulers.operator.openshift.io=aescbc:4,aescbc:3,identity",
"kubeapiservers.operator.openshift.io=aescbc:4,identity;kubeschedulers.operator.openshift.io=aescbc:4,identity",
)

t.Logf("Setting another external reason")
setExternalReason("b")
waitForKeys(5)
waitForConfigs(
"kubeapiservers.operator.openshift.io=aescbc:4,aescbc:5,identity;kubeschedulers.operator.openshift.io=aescbc:4,aescbc:5,identity",
"kubeapiservers.operator.openshift.io=aescbc:4,aescbc:5,aescbc:3,identity;kubeschedulers.operator.openshift.io=aescbc:4,aescbc:5,aescbc:3,identity",
"kubeapiservers.operator.openshift.io=aescbc:5,aescbc:4,aescbc:3,identity;kubeschedulers.operator.openshift.io=aescbc:5,aescbc:4,aescbc:3,identity",
"kubeapiservers.operator.openshift.io=aescbc:5,aescbc:4,identity;kubeschedulers.operator.openshift.io=aescbc:5,aescbc:4,identity",
"kubeapiservers.operator.openshift.io=aescbc:5,identity;kubeschedulers.operator.openshift.io=aescbc:5,identity",
)

t.Logf("Expire the last key")
_, err = kubeClient.CoreV1().Secrets("openshift-config-managed").Patch(fmt.Sprintf("encryption-key-%s-5", component), types.MergePatchType, []byte(`{"metadata":{"annotations":{"encryption.apiserver.operator.openshift.io/migrated-timestamp":"2010-10-17T14:14:52+02:00"}}}`))
require.NoError(t, err)
waitForKeys(6)
waitForConfigs(
"kubeapiservers.operator.openshift.io=aescbc:5,aescbc:6,identity;kubeschedulers.operator.openshift.io=aescbc:5,aescbc:6,identity",
"kubeapiservers.operator.openshift.io=aescbc:5,aescbc:6,aescbc:4,identity;kubeschedulers.operator.openshift.io=aescbc:5,aescbc:6,aescbc:4,identity",
"kubeapiservers.operator.openshift.io=aescbc:6,aescbc:5,aescbc:4,identity;kubeschedulers.operator.openshift.io=aescbc:6,aescbc:5,aescbc:4,identity",
"kubeapiservers.operator.openshift.io=aescbc:6,aescbc:5,identity;kubeschedulers.operator.openshift.io=aescbc:6,aescbc:5,identity",
"kubeapiservers.operator.openshift.io=aescbc:6,identity;kubeschedulers.operator.openshift.io=aescbc:6,identity",
)

t.Logf("Delete the last key")
Expand All @@ -283,12 +282,14 @@ func TestEncryptionIntegration(tt *testing.T) {
// kubeapiservers.operator.openshift.io=aescbc:6,aescbc:5,identity;kubeschedulers.operator.openshift.io=aescbc:6,aescbc:5,identity
// but eventually we get the following:
waitForConfigEventually(
// 6 as preserved config key, 7 as newly created key, and 5 as fully migrated key
"kubeapiservers.operator.openshift.io=aescbc:6,aescbc:7,aescbc:5,identity;kubeschedulers.operator.openshift.io=aescbc:6,aescbc:7,aescbc:5,identity",
// 6 as preserved, unbacked config key, 7 as newly created key, and 5 as fully migrated key
"kubeapiservers.operator.openshift.io=aescbc:6,aescbc:7,aescbc:5,aescbc:4,identity;kubeschedulers.operator.openshift.io=aescbc:6,aescbc:7,aescbc:5,aescbc:4,identity",
)
waitForConfigs(
// 7 is promoted
"kubeapiservers.operator.openshift.io=aescbc:7,aescbc:6,aescbc:5,aescbc:4,identity;kubeschedulers.operator.openshift.io=aescbc:7,aescbc:6,aescbc:5,aescbc:4,identity",
// 7 is migrated, plus one more backed key, which is 5 (6 is deleted)
"kubeapiservers.operator.openshift.io=aescbc:7,aescbc:6,aescbc:5,identity;kubeschedulers.operator.openshift.io=aescbc:7,aescbc:6,aescbc:5,identity",
"kubeapiservers.operator.openshift.io=aescbc:7,identity;kubeschedulers.operator.openshift.io=aescbc:7,identity",
)

t.Logf("Delete the openshift-config-managed config")
Expand All @@ -297,15 +298,17 @@ func TestEncryptionIntegration(tt *testing.T) {
err = kubeClient.CoreV1().Secrets("openshift-config-managed").Delete(fmt.Sprintf("encryption-config-%s", component), nil)
require.NoError(t, err)
waitForConfigs(
"kubeapiservers.operator.openshift.io=aescbc:7,identity;kubeschedulers.operator.openshift.io=aescbc:7,identity",
// one migrated read-key (7) and one more backed key (5), and everything in between (6)
"kubeapiservers.operator.openshift.io=aescbc:7,aescbc:6,aescbc:5,identity;kubeschedulers.operator.openshift.io=aescbc:7,aescbc:6,aescbc:5,identity",
)

t.Logf("Delete the openshift-config-managed config")
deployer.DeleteOperandConfig()
waitForConfigs(
// 7 is migrated and hence only one needed, but we rotate through identity
"kubeapiservers.operator.openshift.io=identity,aescbc:7;kubeschedulers.operator.openshift.io=identity,aescbc:7",
"kubeapiservers.operator.openshift.io=aescbc:7,identity;kubeschedulers.operator.openshift.io=aescbc:7,identity",
// 7 is migrated, plus one backed key (5). 6 is deleted, and therefore is not preserved (would be if the operand config was not deleted)
"kubeapiservers.operator.openshift.io=aescbc:7,aescbc:5,identity;kubeschedulers.operator.openshift.io=aescbc:7,aescbc:5,identity",
)
}

Expand Down