Add configuration for encryption providers #46460

sakshamsharma · 2017-05-25T20:31:13Z

Additions

Allows providing a configuration file (using flag --experimental-encryption-provider-config) to use the existing AEAD transformer (with multiple keys) by composing mutable transformer, prefix transformer (for parsing providerId), another prefix transformer (for parsing keyId), and AES-GCM transformers (one for each key). Multiple providers can be configured using the configuration file.

Example configuration:

kind: EncryptionConfig
apiVersion: v1
resources:
  - resources:
    - namespaces
    providers:
    - aes:
        keys:
        - name: key1
          secret: c2vjcmv0iglzihnly3vyzq==
        - name: key2
          secret: dghpcybpcybwyxnzd29yza==
    - identity: {}

Need for configuration discussed in:
#41939
Encryption

Pathway of a read/write request:

MutableTransformer
PrefixTransformer reads the provider-id, and passes the request further if that matches.
PrefixTransformer reads the key-id, and passes the request further if that matches.
GCMTransformer tries decrypting and authenticating the cipher text in case of reads. Similarly for writes.

Caveats

To keep the command line parameter parsing independent of the individual transformer's configuration, we need to convert the configuration to an interface{} and manually parse it in the transformer. Suggestions on better ways to do this are welcome.
Flags --encryption-provider and --encrypt-resource (both mentioned in this document ) are not supported in this because they do not allow more than one provider, and the current format for the configuration file possibly supersedes their functionality.
Currently, it can be tested by adding --experimental-encryption-provider-config=config.yml to hack/local-up-cluster.sh on line 511, and placing the above configuration in config.yml in the root project directory.

Previous discussion on these changes:
sakshamsharma#1

@jcbsmpsn @destijl @smarterclayton

TODO

Investigate if we need to store keys on disk (per encryption.md)
Look at alpha flag conventions
Need to reserve k8s:enc prefix formally for encrypted data. Else find a better way to detect transformed data.

k8s-ci-robot · 2017-05-25T20:31:21Z

Hi @sakshamsharma. 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 @k8s-bot 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.

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.

jcbsmpsn · 2017-05-25T20:40:28Z

@k8s-bot ok to test

jcbsmpsn · 2017-05-25T20:43:05Z

staging/src/k8s.io/apiserver/pkg/server/options/encryption_provider_config.go

+		if err != nil {
+			return err
+		}
+		if providerConfig.Kind == "AEAD" {


s/AEAD/k8s-aes-gcm/

Is there a reason for skipping the -v1 suffix here?
Update: Shall read version from the version: parameter in config file.

jcbsmpsn · 2017-05-25T20:44:46Z

staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/aes/aes.go

+		}), nil
+
+	} else {
+		return nil, fmt.Errorf("no valid keys found in configuration for AEAD transformer")


s/AEAD/k8s-aes-gcm-v1/

jcbsmpsn · 2017-05-25T20:48:14Z

/release-note-none

jcbsmpsn · 2017-05-25T22:54:54Z

/assign @smarterclayton

destijl · 2017-05-25T23:51:11Z

staging/src/k8s.io/apiserver/pkg/server/options/encryption_provider_config_test.go

+  resource: /registry/
+`
+
+var incorrectConfig1 string = `


give these descriptive names. "badConfigMissingSecret"

destijl · 2017-05-26T00:06:14Z

staging/src/k8s.io/apiserver/pkg/storage/value/location_transformer.go

+				maxMatch = length
+				ts = []int{i}
+			} else if length == maxMatch {
+				ts = append(ts, i)


is there any point in using a slice? Even if there are two the same you just arbitrarily pick the first one. Seems like you could remove this line and it would work exactly the same.

So what you suggest will be correct if you need to do writes. But if you're doing a read, you may need to try out all the transformers which could have written to that location in the past. So for instance, someone configures a transformer1 to write to location A. Then, some days later, the person wants to migrate to transformer2 for location A. There would still be some resources which were written with transformer1, and have not been migrated over yet. Thus we need to return all the transformers which could have written to that location, to be able to decrypt the information.

Now there is an issue with my implementation as well, something I had not noticed earlier:
If you initially use transformer1 for writing to location /a/, and then add transformer2 for writing to location /a/b, when you try to read /a/b/c, this function would not return transformer1 in the list. So we shall not be able to decrypt the resource because the TransformFromStorage shall now not use transformer1. I had not noticed this issue earlier.

An approach may be to try all (there won't be many, in most cases) transformers while reading, and only one transformer (what you suggested) while writing. Might have a performance impact, possibly.

Another approach would be to design a good migration scheme which shall detect a change in configuration, and will auto-migrate data to the new configuration. That way we know exactly which transformer was used to write to each location. I suppose this will require persisting the last configuration on disk in plaintext probably, and checking the current configuration against that at cluster bootup (we could, of course, omit the key secrets from this backup).

destijl · 2017-05-26T00:12:49Z

staging/src/k8s.io/apiserver/pkg/storage/value/location_transformer.go

+}
+
+// TransformFromStorage collects all possible transformers (based on their location parameter) and
+// attempts to decrypt using each of them


guessing seems dangerous here. Do we need to add the resource type into the ciphertext spec with the key id?

I've put my thoughts on this in the comment on longestMatchingTransformers

destijl · 2017-05-26T00:32:23Z

staging/src/k8s.io/apiserver/pkg/server/options/encryption_provider_config_test.go

+func TestEncryptionProviderConfigCorrectParsing(t *testing.T) {
+	testConfigFile := ".test-enc-provider-correct"
+
+	err := ioutil.WriteFile(testConfigFile, []byte(correctConfig), 0600)


You could put the parsing code in another function that accepts io.Reader and pass buffers here, avoiding needing to actually write these to disk. Then the top level function just opens the file and passes it to the function that does the actual parsing.

Will do that.

smarterclayton · 2017-05-26T03:17:34Z

staging/src/k8s.io/apiserver/pkg/server/options/encryption_provider_config_test.go

+      secret: dGhpcyBpcyBwYXNzd29yZA==
+    - name: key3
+      secret: azhzIHNlY3JldCBzdG9yZQ==
+  resource: /registry/


We shouldn't be exposing etcd keys in this config. This config should be able kubernetes resource types. pkg/storage/storage_factory.go should accept a group resource override that includes a transformer - that transformer should be global.

I don't see a strong reason to do anything more complicated than identifying specific resources to encrypt (by opaquely passing in an encryption provider). We deliberately do not expose details of storage location because they are mutable.

smarterclayton · 2017-05-26T03:18:29Z

staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/aes/aes.go

+func NewGCMTransformerFromConfig(config map[string]interface{}) (value.Transformer, error) {
+
+	// Obtain list of keys as []interface{}
+	if keysInterface, ok := config["keys"].([]interface{}); ok {


We generally don't do this - we define API types and include the config that way. Anything that reads from an API config should be outside this package (consuming it) and result in a properly instantiated prefix transformer.

smarterclayton · 2017-05-26T03:23:27Z

staging/src/k8s.io/apiserver/pkg/storage/value/helpers.go

+}
+
+// GetKeyDataFromConfig parses a valid interface to a KeyConfig object
+func GetKeyDataFromConfig(config interface{}) (KeyConfig, error) {


This is too generic - it's better to have a package that takes a concrete config api (i.e. Kube API structs) type and then knows how to turn that into a concrete AES PrefixTransformer, and then have a fairly top level package in apiserver be consumable (pkg/storage/encryption/options.go) to pass into the StorageConfig.

You can define your config api types in a place where config apis would generally belong - however, I know we are trying not to add more things to kubernetes/pkg/apis/componentconfig. @deads2k where would api server specific config types go today?

I suppose my previous comment (alternative configuration style) can help address this. Let me know if it is not so.

smarterclayton · 2017-05-26T03:24:23Z

staging/src/k8s.io/apiserver/pkg/storage/value/location_transformer.go

+)
+
+// LocationTransformer holds a transformer interface and the location of the resources it has to be used for.
+type LocationTransformer struct {


As mentioned above I don't think we should be doing any sort of transformation by key. A storage is configured with a value transformer - we have 1 storage per api resource, and the level of encryption should be at one or more api resources, not at key level.

wojtek-t · 2017-05-29T09:26:11Z

I had some email discussion about this with @sakshamsharma and I agree with @smarterclayton comments. I hopefully clarified a bunch of his questions offline.

sakshamsharma · 2017-05-30T11:00:39Z

I have updated the code to allow specifying the resource names instead of the path now, as suggested by @smarterclayton . Thanks @wojtek-t !
Let me know if that portion looks fine now. I've also acted on @destijl 's recommendations.

As for the configuration comments, I am waiting on the request to @deads2k in the review above. I agree that the current way of parsing the configuration in storage/value/encrypt/aes is sub-optimal, but some more explanation on where to add the config-parsing would be a great help.

Update: One way I can think of possibly improving the situation is to move the parsing of configuration to staging/apiserver/pkg/server/options/encryption_provider_config.go.
Although, anything other than the current setup seems awkward because we have to keep logic pertaining to the AES transformer (the fact that it needs to read keys, for instance) outside the code for the transformer itself. This was deemed desirable in some conversation, which I cannot find now. Can we somehow achieve this (transformer stays coupled with its configuration) as well as the recommended changes (configuration parsing stays in API files)?

sakshamsharma · 2017-06-02T03:38:39Z

@smarterclayton Done now. Sorry for the delay, was away from keyboard.

smarterclayton · 2017-06-02T04:29:54Z

/lgtm

Although the queue is fairly stopped up

k8s-github-robot · 2017-06-02T04:29:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sakshamsharma, smarterclayton

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

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

~~cmd/kube-apiserver/OWNERS~~ [smarterclayton]
~~hack/OWNERS~~ [smarterclayton]
~~staging/src/k8s.io/apiserver/OWNERS~~ [smarterclayton]

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

sakshamsharma · 2017-06-02T05:09:50Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this

sakshamsharma · 2017-06-02T05:33:40Z

@k8s-bot pull-kubernetes-e2e-kops-aws test this

smarterclayton · 2017-06-04T20:49:53Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this

sakshamsharma · 2017-06-05T02:32:14Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this

sakshamsharma · 2017-06-05T03:44:45Z

/retest

sakshamsharma · 2017-06-05T04:07:56Z

Looking at the history here.

Apparently some commit broke the e2e test. All the recent PRs seem to have failed e2e tests.

k8s-ci-robot · 2017-06-05T04:08:09Z

@sakshamsharma: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-federation-e2e-gce	`9760d00`	link	`@k8s-bot pull-kubernetes-federation-e2e-gce test this`

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 · 2017-06-05T12:06:01Z

staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/config.go

+)
+
+const (
+	aesTransformerPrefixV1 = "k8s:enc:aes:v1:"


So we use this prefix to determine if and how we encrypted an object on disk? Wouldn't it be easier to use something like go-jose / JWE to store this information in a tamper proof way? We may still need k8s:enc: to prevent any collisions, but the algorithm would not need to be part of the prefix.

smarterclayton · 2017-06-05T13:25:22Z

No it would not be easier :) It be a long term approach, but we still have to prefix and version. JWT is much more complex at our existing storage layer today with backwards compatibility taken into account.

…

On Mon, Jun 5, 2017 at 8:06 AM, Mo Khan ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In staging/src/k8s.io/apiserver/pkg/server/options/ encryptionconfig/config.go <#46460 (comment)> : > + "encoding/base64" + "fmt" + "io" + "io/ioutil" + "os" + + yaml "github.com/ghodss/yaml" + + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apiserver/pkg/storage/value" + aestransformer "k8s.io/apiserver/pkg/storage/value/encrypt/aes" + "k8s.io/apiserver/pkg/storage/value/encrypt/identity" +) + +const ( + aesTransformerPrefixV1 = "k8s:enc:aes:v1:" So we use this prefix to determine if and how we encrypted an object on disk? Wouldn't it be easier to use something like go-jose / JWE to store this information in a tamper proof way? We may still need k8s:enc: to prevent any collisions, but the algorithm would not need to be part of the prefix. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#46460 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p85Iblb5DmWSWaGIYdXKFojsyiAoks5sA-8xgaJpZM4Nm2qw> .

smarterclayton · 2017-06-05T13:25:32Z

/retest

…

On Mon, Jun 5, 2017 at 9:25 AM, Clayton Coleman ***@***.***> wrote: No it would not be easier :) It be a long term approach, but we still have to prefix and version. JWT is much more complex at our existing storage layer today with backwards compatibility taken into account. On Mon, Jun 5, 2017 at 8:06 AM, Mo Khan ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In staging/src/k8s.io/apiserver/pkg/server/options/encryptionco > nfig/config.go > <#46460 (comment)> > : > > > + "encoding/base64" > + "fmt" > + "io" > + "io/ioutil" > + "os" > + > + yaml "github.com/ghodss/yaml" > + > + "k8s.io/apimachinery/pkg/runtime/schema" > + "k8s.io/apiserver/pkg/storage/value" > + aestransformer "k8s.io/apiserver/pkg/storage/value/encrypt/aes" > + "k8s.io/apiserver/pkg/storage/value/encrypt/identity" > +) > + > +const ( > + aesTransformerPrefixV1 = "k8s:enc:aes:v1:" > > So we use this prefix to determine if and how we encrypted an object on > disk? Wouldn't it be easier to use something like go-jose / JWE to store > this information in a tamper proof way? We may still need k8s:enc: to > prevent any collisions, but the algorithm would not need to be part of the > prefix. > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#46460 (review)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/ABG_p85Iblb5DmWSWaGIYdXKFojsyiAoks5sA-8xgaJpZM4Nm2qw> > . >

k8s-github-robot · 2017-06-05T23:43:46Z

Automatic merge from submit-queue (batch tested with PRs 46550, 46663, 46816, 46820, 46460)

smarterclayton · 2017-06-05T23:45:38Z

We'll need to update this based on #46916

…

On Mon, Jun 5, 2017 at 7:44 PM, Kubernetes Submit Queue < ***@***.***> wrote: Merged #46460 <#46460>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#46460 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p3oqEyJSOoGgIDB11t27dCfiqfDbks5sBJLFgaJpZM4Nm2qw> .

@destijl

Automatic merge from submit-queue Add encryption provider support via environment variables These changes are needed to allow cloud providers to use the encryption providers as an alpha feature. The version checks can be done in the respective cloud providers'. Context: #46460 and #46916 @destijl @jcbsmpsn @smarterclayton

php-coder · 2017-06-20T14:08:07Z

staging/src/k8s.io/apiserver/pkg/storage/etcd3/store.go

@@ -388,7 +388,7 @@ func (s *store) List(ctx context.Context, key, resourceVersion string, pred stor

 	elems := make([]*elemForDecode, 0, len(getResp.Kvs))
 	for _, kv := range getResp.Kvs {
-		data, _, err := s.transformer.TransformFromStorage(kv.Value, authenticatedDataString(key))
+		data, _, err := s.transformer.TransformFromStorage(kv.Value, authenticatedDataString(kv.Key))


@sakshamsharma Could you explain what this change does, please?

The intention is to have the location of the resource as the key. Since the key was written using its own location, it must be authenticated (while decrypting) using its own location as well. kv.Key is its location. key was a top level location (example: /api/Secrets/). When I was attempting to write integration tests (which couldn't get done at that time though), I discovered this issue.

Let me know if I went wrong somewhere.

php-coder · 2017-06-20T14:40:14Z

staging/src/k8s.io/apiserver/pkg/storage/value/encrypt/identity/identity.go

+
+// encryptIdentityTransformer performs no transformation on provided data, but validates
+// that the data is not encrypted data during TransformFromStorage
+type identityTransformer struct{}


@sakshamsharma @smarterclayton This is the 3rd identityTransformer in our codebase (first, second).

Do we really need to have 3 of them? What is the reason?

I see that this implementation has check for k8s:enc: prefix. Why 2 others don't have such check? Should we copy it to them?

This new identity transformer needs to return an error if it finds something that is encrypted. Without that check, the PrefixTransformer cannot try to use other transformers, because it will never see this one failing.

I had originally just modified the one in transformer.go to do this, but @smarterclayton reasoned that the identity transformer (which is used for all the cases where there is no encryption) should only be returning the data. Which is why, we now have a separate one for cases where we want to add more logic.

php-coder · 2017-06-21T12:55:04Z

staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/encryptionconfig_test.go

@@ -0,0 +1,172 @@
+/*


@sakshamsharma @smarterclayton I've just noticed that test named encryptionconfig_test.go while source file has name config.go. I can prepare a patch and rename it, if needed.

The package name is encryptionconfig. It may be violating some convention, but I'm not entirely sure.

The file will be renamed in #53249

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 25, 2017

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

k8s-github-robot assigned ericchiang and timothysc May 25, 2017

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. release-note-label-needed labels May 25, 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 May 25, 2017

jcbsmpsn reviewed May 25, 2017

View reviewed changes

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels May 25, 2017

sakshamsharma force-pushed the location_transformer branch 3 times, most recently from c4ab7f0 to a8ba07c Compare May 25, 2017 22:52

k8s-ci-robot assigned smarterclayton May 25, 2017

sakshamsharma force-pushed the location_transformer branch from a8ba07c to 076c961 Compare May 26, 2017 00:31

destijl reviewed May 26, 2017

View reviewed changes

smarterclayton reviewed May 26, 2017

View reviewed changes

wojtek-t self-requested a review May 29, 2017 09:26

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 30, 2017

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

enj reviewed Jun 5, 2017

View reviewed changes

k8s-github-robot merged commit f893cdd into kubernetes:master Jun 5, 2017

sakshamsharma mentioned this pull request Jun 5, 2017

Add a secretbox and AES-CBC path for encrypt at rest #46916

Merged

sakshamsharma mentioned this pull request Jun 14, 2017

Add encryption provider support via environment variables #47481

Merged

php-coder reviewed Jun 20, 2017

View reviewed changes

sakshamsharma mentioned this pull request Jun 20, 2017

Storage configuration for serviceaccounts being used to access secrets in APIserver #47815

Closed

php-coder reviewed Jun 21, 2017

View reviewed changes

php-coder mentioned this pull request Jun 21, 2017

UPSTREAM: 46460: Add configuration for encryption providers openshift/origin#14798

Merged

This was referenced Jul 20, 2017

Encryption configuration file: show a warning when the file is readable to all #49287

Closed

Encryption configuration file: don't try to read the file fully #49290

Closed

This was referenced Jul 21, 2017

Suggestion: Is it possible to get Kubernetes keep its Secrets in HashiCorp Vault #10439

Closed

encrypt secrets when in etcd. #12742

Closed

kksriram mentioned this pull request Jul 28, 2017

Support a Vault based KMS provider for envelope encryption of resources in a cluster #49817

Closed

Add configuration for encryption providers #46460

Add configuration for encryption providers #46460

Conversation

sakshamsharma commented May 25, 2017 • edited Loading

Additions

Caveats

TODO

k8s-ci-robot commented May 25, 2017

jcbsmpsn commented May 25, 2017

Choose a reason for hiding this comment

sakshamsharma May 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcbsmpsn commented May 25, 2017

jcbsmpsn commented May 25, 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

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

wojtek-t commented May 29, 2017

sakshamsharma commented May 30, 2017 • edited Loading

sakshamsharma commented Jun 2, 2017

smarterclayton commented Jun 2, 2017

k8s-github-robot commented Jun 2, 2017

sakshamsharma commented Jun 2, 2017

sakshamsharma commented Jun 2, 2017

smarterclayton commented Jun 4, 2017

sakshamsharma commented Jun 5, 2017

sakshamsharma commented Jun 5, 2017

sakshamsharma commented Jun 5, 2017 • edited Loading

k8s-ci-robot commented Jun 5, 2017

Choose a reason for hiding this comment

smarterclayton commented Jun 5, 2017 via email

smarterclayton commented Jun 5, 2017 via email

k8s-github-robot commented Jun 5, 2017

smarterclayton commented Jun 5, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

php-coder Jun 20, 2017 • edited Loading

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

sakshamsharma commented May 25, 2017 •

edited

Loading

sakshamsharma May 25, 2017 •

edited

Loading

sakshamsharma commented May 30, 2017 •

edited

Loading

sakshamsharma commented Jun 5, 2017 •

edited

Loading

php-coder Jun 20, 2017 •

edited

Loading