[WIP] Encryption Config #516

enj · 2019-07-03T01:27:29Z

Observer for a single secret with the complete config
Wiring of secret to revision control and syncing from managed namespace
Empty shell of encryption controller - I deleted the logic I was considering

Signed-off-by: Monis Khan mkhan@redhat.com

sttts · 2019-07-03T08:00:43Z

pkg/operator/configobservation/configobservercontroller/observe_config_controller.go

@@ -61,10 +64,11 @@ func NewConfigObserver(
 				SchedulerLister:       configInformer.Config().V1().Schedulers().Lister(),

 				ConfigmapLister:              kubeInformersForNamespaces.ConfigMapLister(),
+				SecretLister_:                kubeInformersForNamespaces.SecretLister(),


underscore?

Matches others in the same file.

mfojtik · 2019-07-03T12:00:13Z

pkg/operator/configobservation/encryption/controller_encryption.go

+		return err
+	}
+
+	switch originalSpec.ManagementState {


we have library-go helper to deal with this

mfojtik · 2019-07-03T12:01:13Z

pkg/operator/configobservation/encryption/observe_encryption.go

+}
+
+func NewEncryptionObserver(targetNamespace string, encryptionConfigPath []string) configobserver.ObserveConfigFunc {
+	return func(genericListers configobserver.Listers, recorder events.Recorder, existingConfig map[string]interface{}) (map[string]interface{}, []error) {


strange to return a func this big

yeah, I think this is because we try to be generic. Also what I personally don't like about the definition is that we lose type safety and it's really hard to read what the function does. We accept map[string]interface{} and return map[string]interface{} :(

strange to return a func this big

I did not want to create a new struct just to close over the inputs.

mfojtik · 2019-07-03T12:02:12Z

pkg/operator/configobservation/encryption/observe_encryption.go

+		encryptionConfigSecret, err := listers.SecretLister().Secrets(targetNamespace).Get(encryptionConfSecret)
+		if errors.IsNotFound(err) {
+			recorder.Warningf("ObserveEncryptionConfigNotFound", "encryption config secret %s/%s not found", targetNamespace, encryptionConfSecret)
+			// TODO what is the best thing to do here?


what produce this secret?

mfojtik · 2019-07-03T12:03:02Z

pkg/operator/configobservation/encryption/sync_encryption.go

+		resourcesynccontroller.ResourceLocation{Namespace: targetNamespace, Name: encryptionConfSecret},
+		resourcesynccontroller.ResourceLocation{Namespace: operatorclient.GlobalMachineSpecifiedConfigNamespace, Name: sourceName},
+	); err != nil {
+		panic(err) // coding error


don't panic, propagate the error up

p0lyn0mial · 2019-07-17T10:00:44Z

pkg/operator/configobservation/encryption/observe_encryption.go

+			return previouslyObservedConfig, append(errs, err)
+		}
+
+		if !equality.Semantic.DeepEqual(existingEncryptionConfig, []string{encryptionConfFilePath}) {


nit: we could compare existingEncryptionConfig field before SetNestedStringSlice (line 58).

what if existingEncryptionConfig hasn't changed ? Should we then simply return previouslyObservedConfig ? For example:

if equality.Semantic.DeepEqual(existingEncryptionConfig, []string{encryptionConfFilePath}) { return previouslyObservedConfig, errs } recorder.Eventf("ObserveEncryptionConfigChanged", "encryption config file changed from %s to %s", existingEncryptionConfig, encryptionConfFilePath) err := unstructured.SetNestedStringSlice(observedConfig, []string{encryptionConfFilePath} ...

p0lyn0mial · 2019-07-17T10:02:40Z

pkg/operator/configobservation/encryption/observe_encryption.go

+				errs = append(errs, err)
+			}
+		}
+


it is okay to continue even if errs != nil ? Does it mean that previouslyObservedConfig could be incomplete? (some fields could be missing?)

p0lyn0mial · 2019-07-17T10:05:00Z

pkg/operator/configobservation/encryption/observe_encryption.go

+			// TODO what is the best thing to do here?
+			// for now we do not unset the config as we are checking a synced version of the secret that could be deleted
+			// return observedConfig, errs
+			return previouslyObservedConfig, errs // do not append the not found error


this can fail because errs could actually have some errors (line 35)

p0lyn0mial · 2019-07-17T10:08:51Z

pkg/operator/configobservation/encryption/observe_encryption.go

+		}
+
+		if !equality.Semantic.DeepEqual(existingEncryptionConfig, []string{encryptionConfFilePath}) {
+			recorder.Eventf("ObserveEncryptionConfigChanged", "encryption config file changed from %s to %s", existingEncryptionConfig, encryptionConfFilePath)


nit: could we make the msg a bit more accurate and assume that existingEncryptionConfig holds only one element? Then the msg would look like changed from x to y instead of changed from [x] to y

p0lyn0mial · 2019-07-17T10:13:04Z

pkg/operator/configobservation/encryption/observe_encryption.go

+}
+
+func NewEncryptionObserver(targetNamespace string, encryptionConfigPath []string) configobserver.ObserveConfigFunc {
+	return func(genericListers configobserver.Listers, recorder events.Recorder, existingConfig map[string]interface{}) (map[string]interface{}, []error) {


yeah, I think this is because we try to be generic. Also what I personally don't like about the definition is that we lose type safety and it's really hard to read what the function does. We accept map[string]interface{} and return map[string]interface{} :(

p0lyn0mial · 2019-07-17T11:26:47Z

pkg/operator/configobservation/encryption/key_controller_encryption.go

+			Name:      fmt.Sprintf("encryption-%s-%s-%s-%d", c.componentName, gr.Group, gr.Resource, keyID),
+			Namespace: operatorclient.GlobalMachineSpecifiedConfigNamespace,
+			Labels: map[string]string{
+				encryptionSecretComponent: c.componentName,


shouldn't encryptionSecretComponent label point to targetNamespace?

p0lyn0mial · 2019-07-17T11:36:45Z

pkg/operator/configobservation/encryption/key_controller_encryption.go

+
+func newAES256Key() []byte {
+	b := make([]byte, 32) // AES-256 == 32 byte key
+	if _, err := rand.Read(b); err != nil {


I'd handle the error, perhaps there are actually cases when this function might fail.

It will never fail on linux, and the OAuth server has matched this behavior since the beginning of OpenShift.

p0lyn0mial · 2019-07-17T11:45:01Z

pkg/operator/configobservation/encryption/key_controller_encryption.go

+	c.componentSelector = labelSelectorOrDie(encryptionSecretComponent + "=" + targetNamespace)
+
+	operatorClient.Informer().AddEventHandler(c.eventHandler())
+	kubeInformersForNamespaces.InformersFor(operatorclient.GlobalMachineSpecifiedConfigNamespace).Core().V1().Secrets().Informer().AddEventHandler(c.eventHandler())


will the sync func be triggered if there is no secrets in GlobalMachineSpecifiedConfigNamespace namespace?

Operator client informer will always trigger this.

p0lyn0mial

although I truly enjoyed reading your code it would be nice to add documentation and some unit tests at some point.

I'm not sure if I fully understand how the code works but hopefully, the following will help the next reviewers.

NewEncryptionKeyController essentially generates encryption keys for validGRs based on needsNewKey condition and places them in GlobalMachineSpecifiedConfigNamespace ns.

NewEncryptionStateController takes all those keys and generates a secret (encryption-config-kube-apiserver) with an EncryptionConfiguration in GlobalMachineSpecifiedConfigNamespace

SyncEncryptionConfig propagates encryption-config-kube-apiserver secret to a secret in targetNamespace ns.

NewEncryptionPodStateController waits for encryption-config-kube-apiserver in targetNamespace ns and propagates state back to the secrets created by NewEncryptionKeyController by adding some labels.

NewEncryptionMigrationController waits for encryption-config-kube-apiserver in targetNamespace ns and takes all the keys/secrets in GlobalMachineSpecifiedConfigNamespace finds the writeKey and performs encryption by modifing desired resources.

NewEncryptionPruneController removes the last 10 secrets with encryptionSecretMigratedTimestamp label attached.

p0lyn0mial · 2019-07-17T17:27:18Z

pkg/operator/configobservation/encryption/key_controller_encryption.go

+}
+
+func (c *EncryptionKeyController) sync() error {
+	if ready, err := shouldRunEncryptionController(c.operatorClient); err != nil || !ready {


this controller and the others are not guarded by a TechPreviewNoUpgrade feature flag?

p0lyn0mial · 2019-07-17T18:46:04Z

pkg/operator/configobservation/encryption/key_controller_encryption.go

+func (c *EncryptionKeyController) generateKeySecret(gr schema.GroupResource, keyID uint64) *corev1.Secret {
+	return &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      fmt.Sprintf("encryption-%s-%s-%s-%d", c.componentName, gr.Group, gr.Resource, keyID),


from what I have seen the keyID is monotonic and I'm not sure if I understand how does it relate to
https://github.com/openshift/cluster-kube-apiserver-operator/pull/516/files#diff-9b3719f78d8f89fa6b78d0f3040a3862R159

sttts · 2019-07-19T15:56:13Z

pkg/operator/configobservation/encryption/key_controller_encryption.go

+
+		preRunCachesSynced: []cache.InformerSynced{
+			operatorClient.Informer().HasSynced,
+			kubeInformersForNamespaces.InformersFor(operatorclient.GlobalMachineSpecifiedConfigNamespace).Core().V1().Secrets().Informer().HasSynced,


factor out kubeInformersForNamespaces.InformersFor(operatorclient.GlobalMachineSpecifiedConfigNamespace) into var and reuse below. Too easy to lose track of these, leading to ugly bugs.

sttts · 2019-07-19T16:04:59Z

pkg/operator/configobservation/encryption/key_controller_encryption.go

+	return configError
+}
+
+func (c *EncryptionKeyController) handleEncryptionKey() error {


"handle" here means that new keys are created if needed, right? Call the function checkAndCreateKeys.

sttts · 2019-07-19T16:05:23Z

pkg/operator/configobservation/encryption/key_controller_encryption.go

+
+const encWorkKey = "key"
+
+type EncryptionKeyController struct {


this needs a ten liner comment what this controller does.

sttts · 2019-07-19T16:05:39Z

pkg/operator/configobservation/encryption/key_controller_encryption.go

+
+	preRunCachesSynced []cache.InformerSynced
+
+	validGRs map[schema.GroupResource]bool


what does valid mean? encrypted CRs?

sttts · 2019-07-19T16:06:45Z

pkg/operator/configobservation/encryption/key_controller_encryption.go

+
+	validGRs map[schema.GroupResource]bool
+
+	componentName     string


what is a component? Be more precise. Below I see that you get secrets with the selector.

sttts · 2019-07-19T16:18:41Z

pkg/operator/configobservation/encryption/key_controller_encryption.go

+		return keyID, true // eh?
+	}
+
+	return keyID, time.Now().After(migrationTimestamp.Add(30 * time.Minute)) // TODO how often?


make it a constant

rewriting all secrets every 30 min? Is that a good idea?

@smarterclayton sounds like this interval came from you. We have to migrate all resources, i.e. update/patch all of them. Are we sure something in this dimension makes sense? Was expecting something around days to a month, like for certs.

sttts · 2019-07-22T12:07:32Z

pkg/operator/configobservation/encryption/state_controller_encryption.go

+
+const stateWorkKey = "key"
+
+type EncryptionStateController struct {


ten-liner godoc

sttts · 2019-07-22T12:08:15Z

pkg/operator/configobservation/encryption/state_controller_encryption.go

+	return configError
+}
+
+func (c *EncryptionStateController) handleEncryptionStateConfig() error {


better name

sttts · 2019-07-22T12:09:50Z

pkg/operator/configobservation/encryption/sync_encryption.go

+		resourcesynccontroller.ResourceLocation{Namespace: targetNamespace, Name: encryptionConfSecret},
+		resourcesynccontroller.ResourceLocation{Namespace: operatorclient.GlobalMachineSpecifiedConfigNamespace, Name: sourceName},
+	); err != nil {
+		panic(err) // coding error


sttts · 2019-07-22T12:10:22Z

pkg/operator/starter.go

@@ -82,6 +84,61 @@ func RunOperator(ctx *controllercmd.ControllerContext) error {
 		ctx.EventRecorder,
 	)

+	validGRs := map[schema.GroupResource]bool{


encryptedGRs

sttts · 2019-07-22T12:12:37Z

pkg/operator/starter.go

@@ -190,6 +252,9 @@ var RevisionSecrets = []revision.RevisionResource{
 	// this is needed so that the cert syncer itself can request certs.  It uses localhost
 	{Name: "kube-apiserver-cert-syncer-client-cert-key"},
 	{Name: "kubelet-client"},
+
+	// etcd encryption
+	{Name: "encryption-config", Optional: true},


Does optional mean that a Secret read error in the kubelet, some race or some admin deleting the secret by accident will lead to an API server to start with invalid encryption config? This will stop it from reading data, but even worse: it might make it store data unencrypted. CVE ahead.

Need to think on this some.

p0lyn0mial · 2019-08-21T15:24:42Z

pkg/operator/encryption/shared_encryption.go

+	operatorInformer := operatorClient.Informer()
+	operatorInformer.AddEventHandler(eventHandler)
+
+	secretsInformer := kubeInformersForNamespaces.InformersFor(operatorclient.GlobalMachineSpecifiedConfigNamespace).Core().V1().Secrets().Informer()


I asked myself whether it is safe to assume that an informer will be created for that namespace. I think it is because we do secretClient.Secrets(operatorclient.GlobalMachineSpecifiedConfigNamespace) which uses CachedSecretGetter.

p0lyn0mial · 2019-08-22T07:48:42Z

pkg/operator/encryption/shared_encryption.go

+	}
+
+	if len(revisions) != 1 {
+		return "", nil // api servers have not converged onto a single revision


we could at least log that we are waiting for pods to converge, otherwise, it could be hard to tell why the controller isn't running.

openshift-ci-robot · 2019-08-22T20:21:50Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: enj
To complete the pull request process, please assign sttts
You can assign the PR to them by writing /assign @sttts in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

enj · 2019-09-04T00:00:32Z

/retest
/refresh

enj · 2019-09-10T02:35:48Z

/retest

enj · 2019-09-10T16:47:18Z

/retest

enj · 2019-09-11T02:16:58Z

/retest

Signed-off-by: Monis Khan <mkhan@redhat.com>

…aded Signed-off-by: Monis Khan <mkhan@redhat.com>

Signed-off-by: Monis Khan <mkhan@redhat.com>

openshift-ci-robot · 2019-09-25T17:20:47Z

@enj: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/verify-deps	`8752b4e`	link	`/test verify-deps`
ci/prow/verify	`8752b4e`	link	`/test verify`
ci/prow/e2e-aws	`8752b4e`	link	`/test e2e-aws`
ci/prow/e2e-aws-upgrade	`8752b4e`	link	`/test e2e-aws-upgrade`
ci/prow/e2e-aws-operator-encryption	`8752b4e`	link	`/test e2e-aws-operator-encryption`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

enj · 2019-10-09T17:54:32Z

@mfojtik @sttts @p0lyn0mial you probably care about this sooner rather than later.

/close

openshift-ci-robot · 2019-10-09T17:54:33Z

@enj: Closed this PR.

In response to this:

@mfojtik @sttts @p0lyn0mial you probably care about this sooner rather than later.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 3, 2019

openshift-ci-robot requested review from mfojtik and soltysh July 3, 2019 01:27

sttts reviewed Jul 3, 2019

View reviewed changes

mfojtik reviewed Jul 3, 2019

View reviewed changes

p0lyn0mial reviewed Jul 17, 2019

View reviewed changes

sttts reviewed Jul 19, 2019

View reviewed changes

sttts reviewed Jul 22, 2019

View reviewed changes

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 30, 2019

enj force-pushed the enj/f/enc_config branch from 36b2100 to b37b3b2 Compare August 16, 2019 23:52

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 16, 2019

enj changed the base branch from master to feature-etcd-encryption August 21, 2019 05:05

enj changed the base branch from feature-etcd-encryption to master August 21, 2019 11:36

p0lyn0mial reviewed Aug 21, 2019

View reviewed changes

p0lyn0mial reviewed Aug 22, 2019

View reviewed changes

enj mentioned this pull request Sep 16, 2019

Add stub test-e2e-encryption suite #566

Merged

enj and others added 26 commits September 24, 2019 09:47

split unsupport config overrides in two

cc02c86

Signed-off-by: Monis Khan <mkhan@redhat.com>

TODO: one-way force rotation

e14b4a6

Signed-off-by: Monis Khan <mkhan@redhat.com>

simple tests

5da8dca

Signed-off-by: Monis Khan <mkhan@redhat.com>

fix WaitForKubeAPIServerClusterOperatorAvailableNotProgressingNotDegr…

9030413

…aded Signed-off-by: Monis Khan <mkhan@redhat.com>

advnaced tests

820ab78

Signed-off-by: Monis Khan <mkhan@redhat.com>

test unset

0eee735

Signed-off-by: Monis Khan <mkhan@redhat.com>

HACK: mergo bug

040d495

Signed-off-by: Monis Khan <mkhan@redhat.com>

increase all timeouts

96e7134

Signed-off-by: Monis Khan <mkhan@redhat.com>

DROP: temp increase base timeout since encrpytion is always enabled

20691df

Signed-off-by: Monis Khan <mkhan@redhat.com>

rebuild etcd client every time

81e602b

Signed-off-by: Monis Khan <mkhan@redhat.com>

make rotation a subtest for nicer output

7d3149a

Signed-off-by: Monis Khan <mkhan@redhat.com>

address comments

673685d

Signed-off-by: Monis Khan <mkhan@redhat.com>

filter component

e82b574

Signed-off-by: Monis Khan <mkhan@redhat.com>

retry update on kas-o resource

432f232

Signed-off-by: Monis Khan <mkhan@redhat.com>

faster rotation test

00413ce

Signed-off-by: Monis Khan <mkhan@redhat.com>

better error

00b60b1

Signed-off-by: Monis Khan <mkhan@redhat.com>

limit rollouts due to read key removal

9f2236f

Signed-off-by: Monis Khan <mkhan@redhat.com>

TODO: test against actual keys

2362af5

Signed-off-by: Monis Khan <mkhan@redhat.com>

try waitForEncryptionTypeLastSecretMigrated

cb03c0c

Signed-off-by: Monis Khan <mkhan@redhat.com>

tolerate migrations

1356522

Signed-off-by: Monis Khan <mkhan@redhat.com>

better logs

63ac9fb

Signed-off-by: Monis Khan <mkhan@redhat.com>

annotation

25ffd34

Signed-off-by: Monis Khan <mkhan@redhat.com>

TODO add stress

d185112

Signed-off-by: Monis Khan <mkhan@redhat.com>

DROP: date logs

9925b58

Signed-off-by: Monis Khan <mkhan@redhat.com>

adds unit tests for EncryptionPruneController

c982172

review changes

8752b4e

openshift-ci-robot closed this Oct 9, 2019


		const encWorkKey = "key"

		type EncryptionKeyController struct {


		preRunCachesSynced []cache.InformerSynced

		validGRs map[schema.GroupResource]bool


		const stateWorkKey = "key"

		type EncryptionStateController struct {

[WIP] Encryption Config #516

[WIP] Encryption Config #516

Conversation

enj commented Jul 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

p0lyn0mial left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts Jul 23, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci-robot commented Aug 22, 2019

enj commented Sep 4, 2019

enj commented Sep 10, 2019

enj commented Sep 10, 2019

enj commented Sep 11, 2019

openshift-ci-robot commented Sep 25, 2019

enj commented Oct 9, 2019

openshift-ci-robot commented Oct 9, 2019

p0lyn0mial left a comment •

edited

sttts Jul 23, 2019 •

edited