Add KMS plugin registry #49742

sakshamsharma · 2017-07-27T19:40:40Z

Allows supporting KMS services as encryption providers using a plugin mechanism similar to admission plugins.

Simplifies #48574

Progresses #48522

deads2k · 2017-07-27T19:51:11Z

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

+	// +optional
+	CacheSize int `json:"cachesize,omitempty"`
+	// configfile is the path to the configuration file for the named KMS provider.
+	ConfigFile string `json:"configfile"`


I'd like to see this structure follow the admission plugin configuration structure and allow either a path or a rawextension directly here.

I'd like to see this structure follow the admission plugin configuration structure and allow either a path or a rawextension directly here.

This is ok as a followup

sakshamsharma · 2017-07-27T19:51:15Z

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

+	if err != nil {
+		return nil, true, err
+	}
+	if !PluginEnabledFn(name, config1) {


I am not entirely sure whether this part is relevant to this process, and I think it can be removed without introducing any immediate issues. Have copied this from admissions plugins code.

I am not entirely sure whether this part is relevant to this process, and I think it can be removed without introducing any immediate issues. Have copied this from admissions plugins code.

You should probably leave it for zero config options.

You should probably leave it for zero config options.

Does that mean I should delete this? I'm sorry, I didn't understand zero config options
Update: So I'll leave this code here.

deads2k · 2017-07-27T19:54:47Z

I'd like to see this moved to be a peer of AdmissionConfiguration and gain proper validation functions.

There is code duplication that should be resolved, but as a building block, I think this keeps our types closer to where they need to be.

The actual handling of the EncryptionProviderConfigFilepath needs to move into the storage layer apply methods too.

liggitt · 2017-07-27T20:00:48Z

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

+	Name string `json:"name"`
+	// cacheSize is the maximum number of secrets which are cached in memory. The default value is 1000.
+	// +optional
+	CacheSize int `json:"cachesize,omitempty"`


~~shouldn't cachesize be part of the config?~~

nm, misread where this struct was

It is not specific to the plugin's configuration. It is a property of the envelope transformer. It will also be used by cloudprovidedkms based plugins (example #48574, see configuration in description)

liggitt · 2017-07-27T20:03:00Z

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

@@ -38,6 +39,7 @@ const (
 	aesCBCTransformerPrefixV1    = "k8s:enc:aescbc:v1:"
 	aesGCMTransformerPrefixV1    = "k8s:enc:aesgcm:v1:"
 	secretboxTransformerPrefixV1 = "k8s:enc:secretbox:v1:"
+	envelopeTransformerPrefixV1  = "k8s:enc:envelope:v1:"


seems like this should match the config field name, both envelope or both kms

deads2k · 2017-07-27T20:07:25Z

I think this moves us in the right direction in spite of the debt. Fix the prefix and I can get behind this one.

sakshamsharma · 2017-07-27T22:25:44Z

Fixed the prefix, and added unit tests. Accidentally closed the PR.

@lavalamp @cheftako Can you PTAL at this? The idea is to mirror the admission plugins code
So no new parameter would be passed from kube-apiserver to the generic apiserver encryption configuration parsing (thus, not affecting other apiservers).
We would register the cloud from the kube-apiserver. Other KMS providers like vault will register themselves to this plugin factory as well. A different apiserver can choose which plugins to register.

sakshamsharma · 2017-07-27T22:39:06Z

/release-note-none

cheftako

Minor tweaks

cheftako · 2017-07-27T22:37:33Z

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

+			}
+			defer f.Close()
+			envelopeService, pluginFound, err := KMSPluginRegistry.getPlugin(provider.KMS.Name, f)
+			if pluginFound == false {


Please reorder found vs err handling. Assuming that there is an actual error, reporting that should take precedence. Especially as the failure to find a plugin provider could be the result of the error.

cheftako · 2017-07-27T22:46:17Z

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

+
+// All registered KMS options.
+type KMSPlugins struct {
+	lock     sync.Mutex


I would expect that registration is a rare event but that getting a plugin might not be. If so then this might benefit from a RWLock.

cheftako · 2017-07-27T22:47:41Z

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

+	if found {
+		glog.Fatalf("KMS plugin %q was registered twice", name)
+	}
+	if ps.registry == nil {


This works but it feels wrong to me to access a map just prior to initializing. May be less work on later devs if you put the nil check first.

cheftako · 2017-07-27T23:00:02Z

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

+// known but failed to initialize.  The config parameter specifies the io.Reader
+// handler of the configuration file for the cloud provider, or nil for no configuration.
+func (ps *KMSPlugins) getPlugin(name string, config io.Reader) (envelope.Service, bool, error) {
+	ps.lock.Lock()


Not sure it is a good idea to hold the lock for this entire method. As it stands there are multiple pieces of code for which you are holding the lock that you do not control and could even be RPC calls. Assuming that the lock is only securing access to the registry object, I would suggest you should create a smaller get method which just access registry with its lock and then call that from here.

sakshamsharma · 2017-07-27T23:13:26Z

@cheftako Addressed those issues, thanks! I also added support for cloudprovidedkms configuration with tests. It looks almost the same as the KMS one. Another quick pass would be greatly appreciated, since the cloudprovidedkms was something you had recommended.

Example code after this PR in kube-apiserver:

cloudKMSGetter := func (name string) (envelope.Service, error) {
    cloud, err := cloudprovider.InitCloudProvider(s.CloudProvider.CloudProvider, s.CloudProvider.CloudConfigFile)
    // handle err
    return cloud.KeyManagementService(name)
}
encryptionconfig.KMSPluginRegistry.RegisterCloudProvidedKMSPlugin(cloudKMSGetter)

transformerOverrides, err := encryptionconfig.GetTransformerOverrides(s.Etcd.EncryptionProviderConfigFilepath)

No dependency to cloudproviders introduced, no changes in function signature, other apiservers don't break.

If you want to see how #48574 looks after this PR, here's a version based on this PR: https://github.com/sakshamsharma/kubernetes/tree/gkms-with-plugin (commit of note: see latest commit)

Example of how to register vault:

vaultFactory := func (config io.Reader) (envelope.Service, error) {
     // make vault
}
encryptionconfig.KMSPluginRegistry.Register("vault", vaultFactory)

@deads2k @liggitt if this is still acceptable for you, can we merge this in? The other PR can then get in too (basically the changes on the above mentioned branch on my fork) because the controversial part is covered in this one.

deads2k · 2017-07-28T12:47:52Z

@deads2k @liggitt if this is still acceptable for you, can we merge this in? The other PR can then get in too (basically the changes on the above mentioned branch on my fork) because the controversial part is covered in this one.

I'll merge this bit this it is directly related to the API machinery and gives a good (if slightly debt-y) plugin mechanism. The other implementation pull seems to have slipped past sig-auth until a couple days ago. There probably ought to be more time to comment there.

/lgtm

k8s-github-robot · 2017-07-28T12:48:16Z

[APPROVALNOTIFIER] This PR is APPROVED

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

Associated issue: 48574

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

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

~~staging/src/k8s.io/apiserver/OWNERS~~ [deads2k]

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

cheftako · 2017-07-28T17:53:55Z

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

+			if found == true {
+				return nil, fmt.Errorf("more than one provider specified in a single element, should split into different list elements")
+			}
+			f, err := os.Open(provider.KMS.ConfigFile)


It feels wrong that we are trying to open/read the file here. It would seem better to just pass this location down to the factory and let it do the read. Especially as in something like a hybrid case this could be a key into a config server or be a file+coordinate into a larger config file. We should need to know or care about such things. However if we insist on opening+reading the file then we end up having to care about these things.

The admissions factory used this format, which is why I stuck with this. We can change this in a follow-up.
Once this PR is merged, I'll open a new one to change this and remove the reflect mentioned in your 2nd comment, along with some other cleanup.

It feels wrong that we are trying to open/read the file here. It would seem better to just pass this location down to the factory and let it do the read. Especially as in something like a

To make this API congruent to the admission config, there will be a choice of separate file or embedded content depending the sensitivity (which we don't know since its pluggable) of the config in question.

An approach which I've proposed earlier, and I've seen used in the cloudproviders package:
Have a field config map[string]interface{} on the KMSConfig struct. Send it to the plugin factory. The plugin factory would use the mapstructure library to parse it into a struct safely. This library is used in certain cloudproviders like openstack.
So this way you can support both config: and configfile: attributes on the kms configuration.

May not be acceptable though, due to a point in the flow where the config is unstructured (it will still be read in a structured form later on). Just putting this out here.

The reader interface lets you load a structured config from an external file or an inlined config. All our config files should be structured and versioned (see scheduler config, ABAC config, admission config, etc)

cheftako · 2017-07-28T17:59:14Z

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

+
+// splitStream reads the stream bytes and constructs two copies of it.
+func splitStream(config io.Reader) (io.Reader, io.Reader, error) {
+	if config == nil || reflect.ValueOf(config).IsNil() {


What are we trying to protect ourselves against with this use of reflect? None of our other nil checks use it (other than the place we cut&pasted this code from). When we opened the config reader a nil check was sufficient. This object has not been copied/valueOf so I believe we are pulling in something we are better of without for no good reason.

What are we trying to protect ourselves against with this use of reflect? None of our other nil checks use it (other than the place we cut&pasted this code from). When we opened the config reader a nil check was sufficient. This object has not been copied/valueOf so I believe we are pulling in something we are better of without for no good reason

Nil versus nil. Is it a nil interface or a non-nil interface with a nil value. It may drop out when these plugin paths are collapsed.

cheftako

Would like to see us fix config handling in a later release. Might be good to file an issue on it.

sakshamsharma · 2017-07-28T22:21:28Z

One of the comments was not in the proper format, and this package was removed from hack/.golint_failures by another PR recently.
@deads2k @cheftako @liggitt Sorry about that, can you please lgtm again?

Update: Something broke pull-kubernetes-bazel it appears. Will track submit queue and update this message when it is fixed.

deads2k · 2017-07-31T11:42:05Z

/retest

cjwagner · 2017-07-31T23:39:19Z

bump

cjwagner · 2017-08-01T00:44:45Z

/test pull-kubernetes-unit

k8s-github-robot · 2017-08-01T01:27:59Z

Automatic merge from submit-queue

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

k8s-github-robot assigned soltysh Jul 27, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Jul 27, 2017

sakshamsharma force-pushed the kms-plugin-registry branch 4 times, most recently from 1a51e8f to bfe736c Compare July 27, 2017 19:47

deads2k reviewed Jul 27, 2017

View reviewed changes

sakshamsharma commented Jul 27, 2017

View reviewed changes

liggitt reviewed Jul 27, 2017

View reviewed changes

sakshamsharma force-pushed the kms-plugin-registry branch 3 times, most recently from 3613e7b to df65bee Compare July 27, 2017 20:56

sakshamsharma closed this Jul 27, 2017

sakshamsharma reopened this Jul 27, 2017

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

cheftako reviewed Jul 27, 2017

View reviewed changes

sakshamsharma force-pushed the kms-plugin-registry branch from df65bee to adea7c3 Compare July 27, 2017 23:12

sakshamsharma force-pushed the kms-plugin-registry branch from adea7c3 to 38266b7 Compare July 27, 2017 23:16

sakshamsharma mentioned this pull request Jul 28, 2017

Add Google cloud KMS service for envelope encryption transformer #48574

Merged

sakshamsharma force-pushed the kms-plugin-registry branch from 38266b7 to e73d9d6 Compare July 28, 2017 00:55

liggitt assigned deads2k Jul 28, 2017

sakshamsharma force-pushed the kms-plugin-registry branch from e73d9d6 to ead42cc Compare July 28, 2017 03:36

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

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

cheftako reviewed Jul 28, 2017

View reviewed changes

cheftako approved these changes Jul 28, 2017

View reviewed changes

sakshamsharma mentioned this pull request Jul 28, 2017

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

Closed

sakshamsharma force-pushed the kms-plugin-registry branch from ead42cc to f354c62 Compare July 28, 2017 22:16

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 28, 2017

sakshamsharma added 3 commits July 28, 2017 15:32

Add KMS plugin registry

4998943

Add unit tests for KMS transformer initialization

b76c63a

Add cloudprovidedkms provider support

68a32c0

sakshamsharma force-pushed the kms-plugin-registry branch from f354c62 to 68a32c0 Compare July 28, 2017 22:39

deads2k added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 31, 2017

soltysh removed their assignment Jul 31, 2017

k8s-github-robot merged commit 1bc5682 into kubernetes:master Aug 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add KMS plugin registry #49742

Add KMS plugin registry #49742

sakshamsharma commented Jul 27, 2017

deads2k Jul 27, 2017

deads2k Jul 27, 2017

sakshamsharma Jul 27, 2017 •

edited

deads2k Jul 27, 2017

sakshamsharma Jul 27, 2017 •

edited

deads2k commented Jul 27, 2017

liggitt Jul 27, 2017 •

edited

sakshamsharma Jul 27, 2017 •

edited

liggitt Jul 27, 2017

deads2k commented Jul 27, 2017

sakshamsharma commented Jul 27, 2017 •

edited

sakshamsharma commented Jul 27, 2017

cheftako left a comment

cheftako Jul 27, 2017

cheftako Jul 27, 2017

cheftako Jul 27, 2017

cheftako Jul 27, 2017

sakshamsharma commented Jul 27, 2017 •

edited

deads2k commented Jul 28, 2017

k8s-github-robot commented Jul 28, 2017

cheftako Jul 28, 2017

sakshamsharma Jul 28, 2017 •

edited

deads2k Jul 28, 2017

sakshamsharma Jul 28, 2017 •

edited

liggitt Jul 28, 2017

cheftako Jul 28, 2017

deads2k Jul 28, 2017

cheftako left a comment

sakshamsharma commented Jul 28, 2017 •

edited

deads2k commented Jul 31, 2017

cjwagner commented Jul 31, 2017

cjwagner commented Aug 1, 2017

k8s-github-robot commented Aug 1, 2017

Add KMS plugin registry #49742

Add KMS plugin registry #49742

Conversation

sakshamsharma commented Jul 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sakshamsharma Jul 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sakshamsharma Jul 27, 2017 • edited

Choose a reason for hiding this comment

deads2k commented Jul 27, 2017

liggitt Jul 27, 2017 • edited

Choose a reason for hiding this comment

sakshamsharma Jul 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Jul 27, 2017

sakshamsharma commented Jul 27, 2017 • edited

sakshamsharma commented Jul 27, 2017

cheftako 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

sakshamsharma commented Jul 27, 2017 • edited

deads2k commented Jul 28, 2017

k8s-github-robot commented Jul 28, 2017

Choose a reason for hiding this comment

sakshamsharma Jul 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sakshamsharma Jul 28, 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

cheftako left a comment

Choose a reason for hiding this comment

sakshamsharma commented Jul 28, 2017 • edited

deads2k commented Jul 31, 2017

cjwagner commented Jul 31, 2017

cjwagner commented Aug 1, 2017

k8s-github-robot commented Aug 1, 2017

sakshamsharma Jul 27, 2017 •

edited

sakshamsharma Jul 27, 2017 •

edited

liggitt Jul 27, 2017 •

edited

sakshamsharma Jul 27, 2017 •

edited

sakshamsharma commented Jul 27, 2017 •

edited

sakshamsharma commented Jul 27, 2017 •

edited

sakshamsharma Jul 28, 2017 •

edited

sakshamsharma Jul 28, 2017 •

edited

sakshamsharma commented Jul 28, 2017 •

edited