Support encryption-at-rest for the kube-apiserver #3368

georgebuckerfield · 2017-09-11T21:38:54Z

This PR adds support for enabling encryption-at-rest for data in etcd, via the kube-apiserver (as per https://kubernetes.io/docs/tasks/administer-cluster/encrypt-data).

I've put the functionality behind a feature flag, +EnableDataEncryption. It can then be enabled per-cluster by using --enable-encryption-config on the command line, or by adding a kubeEncryptionConfig section to the cluster spec. This is passed through to the kube-apiserver by the nodeup process. I'm not sure if this is the best way of doing it right now, but it is working.

Fixes #3356.

k8s-ci-robot · 2017-09-11T21:38:55Z

@georgebuckerfield: Your pull request title starts with "WIP", so the do-not-merge/work-in-progress label will be added.

This label will ensure that your pull request will not be merged. Remove the prefix from your pull request title to trigger the removal of the label and allow for your pull request to be merged.

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.

k8s-ci-robot · 2017-09-11T21:39:02Z

Hi @georgebuckerfield. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

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.

gambol99 · 2017-09-11T21:50:23Z

cmd/kops/create_cluster.go

@@ -299,6 +300,9 @@ func NewCmdCreateCluster(f *util.Factory, out io.Writer) *cobra.Command {
 		cmd.Flags().StringVar(&options.VSphereCoreDNSServer, "vsphere-coredns-server", options.VSphereCoreDNSServer, "vsphere-coredns-server is required for vSphere.")
 		cmd.Flags().StringVar(&options.VSphereDatastore, "vsphere-datastore", options.VSphereDatastore, "vsphere-datastore is required for vSphere.  Set a valid datastore in which to store dynamic provision volumes.")
 	}
+	if featureflag.EnableEtcdEncryption.Enabled() {


Not sure the feature flag or cli option is really required; i'm tempted to say we shouldn't encourage creating clusters via cli options, but push for creation via config instead; @justinsb @chrislovecnm .. I'm also guessing the feature requires more config than bool to be useful, i.e. keys ... I think we should drop the reference to etcd as well

I agree with @gambol99, let's not have a cli option, and remove the etcd verbadge. The users can use manifests or do an edit on the cluster after a create.

I am on the side of not having a featureflag as well.

gambol99 · 2017-09-11T21:52:51Z

nodeup/pkg/model/encryption.go

+)
+
+// EncryptionBuilder builds the encryption-at-rest config
+type EncryptionBuilder struct {


can we not wrap all this logic into the kube_apisever builder itself?

gambol99 · 2017-09-11T21:57:16Z

nodeup/pkg/model/encryption.go

+var _ fi.ModelBuilder = &EncryptionBuilder{}
+
+// Build is responsible for building the config file
+func (b *EncryptionBuilder) Build(c *fi.ModelBuilderContext) error {


I noticed your add the build here .. there doesn't appear to be a conditional on the role though? .. does the file need to be on masters and nodes?

gambol99 · 2017-09-11T22:00:29Z

nodeup/pkg/model/encryption.go

+import (
+	"fmt"
+
+	"k8s.io/apiserver/pkg/server/options/encryptionconfig"


I think a general convention ... std lib, local imports and then external .. Could we drop the encryptionconfig import down

I can change this, but it doesn't seem to be consistent with the rest of the repo. The convention seems to be: std lib, k8s.io libraries, external libraries e.g. in nodeup/pkg/model/kubeapiserver.go, nodeup/pkg/model/kubecontrollermanager.go etc. Happy to modify if I'm wrong though!

The imports formatting is not consistent through the project ;( But yes @gambol99 has it correct

gambol99 · 2017-09-11T22:02:36Z

nodeup/pkg/model/encryption.go

+
+func (b *EncryptionBuilder) buildEncryptionConfig() (*encryptionconfig.EncryptionConfig, error) {
+	resources := b.Cluster.Spec.KubeEtcdEncryptionConfig.Resources
+	config := &encryptionconfig.EncryptionConfig{


this could probably be a oneliner

return &encryptionconfig.EncryptionConfig{ Kind: "EncryptionConfig", APIVersion: "v1", Resources: b.Cluster.Spec.KubeEtcdEncryptionConfig.Resources, }, nil

gambol99 · 2017-09-11T22:09:15Z

pkg/apis/kops/encryption.go

+)
+
+// KubeEtcdEncryptionConfig holds the etcd encryption config
+type KubeEtcdEncryptionConfig struct {


i think we can keep this in the componentconfig.go

gambol99 · 2017-09-11T22:12:38Z

pkg/apis/kops/v1alpha1/encryption.go

+)
+
+// KubeEtcdEncryptionConfig holds the etcd encryption config
+type KubeEtcdEncryptionConfig struct {


same as above

gambol99 · 2017-09-11T22:15:13Z

pkg/model/components/apiserver.go

@@ -204,6 +204,11 @@ func (b *KubeAPIServerOptionsBuilder) BuildOptions(o interface{}) error {
 		c.InsecurePort = 8080
 	}

+	// Set the path for the encryption-at-rest config
+	if *clusterSpec.KubeEtcdEncryptionConfig.Enabled {


does this not cause a panic if not set? .. should we check for a nil

gambol99 · 2017-09-11T22:17:31Z

pkg/featureflag/featureflag.go

@@ -55,6 +55,9 @@ var EnableExternalDNS = New("EnableExternalDNS", Bool(false))
 // SpecOverrideFlag allows setting spec values on create
 var SpecOverrideFlag = New("SpecOverrideFlag", Bool(false))

+// EnableEtcdEncryption if will enable encryption-at-rest for etcd.
+var EnableEtcdEncryption = New("EnableDataEncryption", Bool(false))


dependent on @justinsb and @chrislovecnm if a feature flag is required

We can pull it

gambol99 · 2017-09-11T22:28:24Z

upup/pkg/fi/nodeup/command.go

@@ -220,6 +220,9 @@ func (c *NodeUpCommand) Run(out io.Writer) error {
 	} else {
 		loader.Builders = append(loader.Builders, &model.KubeRouterBuilder{NodeupModelContext: modelContext})
 	}
+	if *c.cluster.Spec.KubeEtcdEncryptionConfig.Enabled {
+		loader.Builders = append(loader.Builders, &model.EncryptionBuilder{NodeupModelContext: modelContext})


tempted to say we wrap this into the KubeAPIServerBuilder

gambol99 · 2017-09-11T22:29:20Z

/assign @gambol99
/assign @justinsb

georgebuckerfield · 2017-09-12T11:33:36Z

Thanks for the review @gambol99, I'll start addressing these tonight. Going to try and get some tests added as well.

chrislovecnm · 2017-09-15T00:39:38Z

/ok-to-test

georgebuckerfield · 2017-09-15T10:28:34Z

/retest

chrislovecnm

You have taken in a great, but complex piece of work. Thanks for the help! Appreciate your patience with the PR review process.

Question for yah

chrislovecnm · 2017-09-15T13:59:59Z

nodeup/pkg/model/tests/encryptionbuilder/aescbc/cluster.yaml

+      - aescbc:
+          keys:
+          - name: key1
+            secret: c2VjcmV0IGlzIHNlY3VyQ==


What is this secret used for? Should we have this in a kops secret?

That secret is used by the apiserver to encrypt/decrypt the data, so ideally I think it should be a secret/obscured somehow. What makes it tricky is users can specify an arbitrary number of keys for each provider (the apiserver will try them each in order when decrypting), so I'm not quite sure how that will work with kops secrets? I suppose users could create multiple secrets with kops secret create aescbckey key1 c2Vj..., kops secret create aescbckey key1 nf9D..., or something similar. We could then swap in the actual secrets when nodeup runs?

Or actually, just store the entire encryptionconfig in a secret? That might simplify things a bit.

I would store the encryptionconfig in a secret. We just had a PR for docker login perms. Let me find it.

georgebuckerfield · 2017-09-18T19:13:33Z

Ok, so I've refactored this. The process for enabling encryption for a cluster would now be:

kops create cluster ... as usual
create a secret containing your encryption config kops create secret encryptionconfig -f config.yaml
edit the cluster, adding encryptionConfig: true to the cluster spec
kops update cluster --yes as usual

I'm completely open to renaming the EncryptionConfig field on the cluster spec to something better/more descriptive. Maybe just EncryptionEnabled?

chrislovecnm · 2017-09-20T22:01:58Z

@gambol99 can you give a review, please? Many changes have been made.

chrislovecnm · 2017-09-20T22:07:42Z

@georgebuckerfield looks good, if @gambol99 does not get back to you, please reach out. But @gambol99 if available will give it a look.

Again appreciate your patience with this, as this PR went through a few cycles

georgebuckerfield · 2017-09-21T09:33:33Z

Cheers @chrislovecnm, appreciate your help getting this through!

gambol99 · 2017-09-22T08:05:31Z

@georgebuckerfield @chrislovecnm ... lgtm

chrislovecnm · 2017-09-22T16:08:00Z

/lgtm

chrislovecnm · 2017-09-22T16:09:24Z

Sorry, @georgebuckerfield ... can we squash the git commits into reasonable chunks of work or just one commit? One last thing.

- add a new kops secret command to store encryption config - add the experimential-encryption-provider-config flag to the kube-apiserver - add functionality to nodeup to create the stored encryption config in the correct path

georgebuckerfield · 2017-09-22T16:22:08Z

Yep, @chrislovecnm, I'm working on that now. I've made it slightly complicated for myself by merging in upstream updates halfway through (to resolve a conflict), just sorting that out now.

georgebuckerfield · 2017-09-22T16:55:11Z

/retest

chrislovecnm · 2017-09-22T16:57:26Z

@georgebuckerfield restarted the travis build for you

chrislovecnm · 2017-09-22T23:46:16Z

/lgtm

k8s-github-robot · 2017-09-22T23:47:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrislovecnm

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

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [chrislovecnm]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-09-23T00:19:47Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-09-23T00:56:44Z

Automatic merge from submit-queue. .

elisiano · 2017-12-26T18:12:25Z

Unless I'm miserably failing at searching, I could not find any documentation on how the config file should look like when executing kops create secret encryptionconfig -f config.yaml.

Is there any example available somewhere?

iMartyn · 2018-03-22T10:39:09Z

Because people will find @elisiano's question and not an answer : https://kubernetes.io/docs/tasks/administer-cluster/encrypt-data/ covers creating that config.yaml file.

elisiano · 2018-03-22T11:52:26Z

Thank you @iMartyn !

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 11, 2017

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 11, 2017

gambol99 reviewed Sep 11, 2017

View reviewed changes

k8s-ci-robot assigned gambol99 and justinsb Sep 11, 2017

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 15, 2017

chrislovecnm reviewed Sep 15, 2017

View reviewed changes

georgebuckerfield changed the title ~~WIP: Support encryption-at-rest for the kube-apiserver~~ Support encryption-at-rest for the kube-apiserver Sep 18, 2017

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 18, 2017

k8s-ci-robot assigned chrislovecnm Sep 22, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 22, 2017

chrislovecnm removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 22, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 22, 2017

Enable support for encryption-at-rest

da36425

- add a new kops secret command to store encryption config - add the experimential-encryption-provider-config flag to the kube-apiserver - add functionality to nodeup to create the stored encryption config in the correct path

georgebuckerfield force-pushed the enable-data-encryption branch from e472e4d to da36425 Compare September 22, 2017 16:34

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 22, 2017

k8s-github-robot merged commit 6a23853 into kubernetes:master Sep 23, 2017

prateeknayak mentioned this pull request Sep 28, 2017

Secrets not encrypting as part of Kubernetes 1.7 upgrade with kops 1.7 #3468

Closed

Support encryption-at-rest for the kube-apiserver #3368

Support encryption-at-rest for the kube-apiserver #3368

Conversation

georgebuckerfield commented Sep 11, 2017 • edited

k8s-ci-robot commented Sep 11, 2017

k8s-ci-robot commented Sep 11, 2017

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

georgebuckerfield Sep 15, 2017 • 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

gambol99 commented Sep 11, 2017

georgebuckerfield commented Sep 12, 2017

chrislovecnm commented Sep 15, 2017

georgebuckerfield commented Sep 15, 2017

chrislovecnm left a 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

georgebuckerfield commented Sep 18, 2017

chrislovecnm commented Sep 20, 2017

chrislovecnm commented Sep 20, 2017

georgebuckerfield commented Sep 21, 2017

gambol99 commented Sep 22, 2017 • edited

chrislovecnm commented Sep 22, 2017

chrislovecnm commented Sep 22, 2017

georgebuckerfield commented Sep 22, 2017

georgebuckerfield commented Sep 22, 2017

chrislovecnm commented Sep 22, 2017

chrislovecnm commented Sep 22, 2017

k8s-github-robot commented Sep 22, 2017

k8s-github-robot commented Sep 23, 2017

k8s-github-robot commented Sep 23, 2017

elisiano commented Dec 26, 2017

iMartyn commented Mar 22, 2018

elisiano commented Mar 22, 2018

georgebuckerfield commented Sep 11, 2017 •

edited

georgebuckerfield Sep 15, 2017 •

edited

gambol99 commented Sep 22, 2017 •

edited