Vaultproviderspec (#1) #888

kksriram · 2017-08-09T05:11:42Z

Initial version of Vault KMS Provider
Renamed image file to be consistent with spec name
Brought spec inline with EnvelopeTransformer interfaces in PR 49350
Wrap at 80

k8s-ci-robot · 2017-08-09T05:11:44Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

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.

kksriram · 2017-08-10T16:40:31Z

check CLA again

kksriram · 2017-08-10T16:53:45Z

Proposal for Issue kubernetes/kubernetes#49817

deads2k · 2017-08-10T17:01:46Z

I'm very interested in having a second example of a KMS provider.

@kubernetes/sig-api-machinery-feature-requests @kubernetes/sig-auth-feature-requests

kksriram · 2017-08-10T17:05:55Z

This is the same content that was socialized via the google doc at sig-auth 7/27, with a few updates to bring it inline with @sakshamsharma 's changes.

sakshamsharma · 2017-08-10T17:20:04Z

contributors/design-proposals/vault-based-kms-provider.md

+	    - secrets
+	    providers:
+	    - kms:
+	      name: vault


Indent the properties of KMS (name, cache size, config file) one more level.

dims · 2017-08-10T18:10:36Z

contributors/design-proposals/vault-based-kms-provider.md

+#### Authentication Configuration
+##### Vault Server Authentication
+
+For the Kubernetes cluster to authenticate the vault server, if TLS is enabled :


"if TLS is enabled" why optional? let's mandate it!

Makes sense. Fixed.

dims · 2017-08-10T18:12:15Z

contributors/design-proposals/vault-based-kms-provider.md

+1. ``role-id`` : RoleID of the AppRole
+2. ``secret-id`` : secret Id only if associated with the appRole.
+
+Here's a sample configuration file with Vault AppRole


This sample configuration did not render well when i clicked on "view"

dims · 2017-08-10T18:16:45Z

contributors/design-proposals/vault-based-kms-provider.md

+### Pseudocode
+#### Prefix Metadata
+Every encrypted secret will have the following metadata prefixed.
+``k8s:enc:kms:<api-version>:vault:len(<KEK-key-name>:<KEK-key-version>:<DEK


Should all this just be json or yaml?

kksriram · 2017-08-10T21:36:29Z

contributors/design-proposals/vault-based-kms-provider.md

+performance impact. Hence, depending on the cache size, there will be a
+performance impact.
+3. Response time.
+    4. will depend on choice of encryption algorithm and strength.


Still not fixed. If you look at the file preview, the bullet points are inline and not nested as you wanted.

I see it. Thanks. Will fix it on the next go around with other comments.

kksriram · 2017-08-10T21:37:03Z

contributors/design-proposals/vault-based-kms-provider.md

+to implement specific providers for each (in K8S).
+* Reduced risk of encryption key compromise.
+    * encryption key is stored and managed in Vault.
+    * encryption key does not need to leave the Vault.


kksriram · 2017-08-23T19:06:30Z

@destijl You had a question at sig-auth today whether supporting additional Vault Auth Backends will need code changes.

Right now, of the supported Auth Backends, typically its just the three in the current spec that would be used by system processes. The rest would typically be used by humans (which I think doesn't matter in this particular case).

At first glance, I don't see how we can get away with not changing code when adding other auth backends.

Talking over with @vineet-garg we will need code changes if supporting new auth back-ends beyond what's in the proposal. I would prefer to focus on a small set of auth backends (the ones in the proposal) and get those in first. Based on interest in the community, we can consider supporting others.

hongchaodeng · 2017-08-24T22:34:37Z

contributors/design-proposals/vault-based-kms-provider.md

+key(s) locally in a server configuration file. The provider encrypts and
+decrypts secrets in-process. Building upon these, a KMS provider framework with
+an option to support different KMS providers like google cloud KMS is being
+added via PRs [48574](https://github.com/kubernetes/kubernetes/pull/48575) and


48574

Shouldn't it be kubernetes/kubernetes#48522 ?

the proposal refers to pull request#(48574), what you posted is corresponding issue#(48522).

hongchaodeng · 2017-08-24T22:46:33Z

contributors/design-proposals/vault-based-kms-provider.md

+  role-id: db02de05-fa39-4855-059b-67221c5c2f63
+```
+
+## Key Generation and rotation


How is DEK generated?

That is in envelope.generateKey which is part of existing envelope transformer.

Worth adding that note to the proposal?

jcbsmpsn · 2017-08-28T23:24:40Z

This comment on the Google KMS review will affect your implementation. Basically, the expected direction is to implement these integrations out of the Kubernetes repo. The mechanism to accomplish that does not exist yet.

liggitt · 2017-08-29T00:25:30Z

I still think gaining experience with a small number of encryption providers in-tree is valuable in designing an out of process plugin mechanism.

kksriram · 2017-08-29T01:32:48Z

@jcbsmpsn @liggitt I haven't caught up on the in-tree v out-of-process discussion over on @sakshamsharma PR 48574. FWIW, my colleague @vineet-garg has an implementation for this spec, that should be coming upstream later this week. We could use that to review what the impact is with a second provider.

More after I catch up on the discussion on 48574.

deads2k · 2017-09-19T18:30:00Z

@destijl You had a question at sig-auth today whether supporting additional Vault Auth Backends will need code changes.

@destijl the response #888 (comment) looked pretty reasonable as a starting point. Do you have further questions?

kksriram · 2017-09-20T17:43:13Z

I meant to update earlier but slipped my mind. I did catch up on the in-tree vs out-of-tree discussion and the proposal .

I agree with @liggitt earlier comment that gaining experience with a second encryption provider in-tree would help guide the design of the proposal for out-of-tree.

As such, my preference would be to get the Vault code merged after review, as an in-tree provider and then refactor based on what we learn with the out-of-tree proposal.

deads2k · 2017-09-26T16:51:51Z

This lgtm. I'm going to see where we'll put it in API machinery, but the KMS plugin structure is unopinionated about whether it is or isn't a cloud provider, so that won't cause any trouble.

lavalamp · 2017-09-26T20:43:06Z

What new (compile-time) dependencies would be required?

This is not really a sustainable way of developing addons/plugins. I would much rather have a process boundary here. I am slightly persuaded by the argument that we should have two concrete uses before generalizing, but only slightly. I would definitely want an agreement that we would force the next person to want to add one of these to build a cross-process API. We can talk about it at tomorrow's sig meeting.

deads2k · 2017-09-26T21:04:03Z

I would definitely want an agreement that we would force the next person to want to add one of these to build a cross-process API.

Assuming this one goes well and doesn't expose fundamental concerns, I'm ok with that. Though I may be inclined to force the existing impls out of tree a release or two later to maintain equal footing.

ericchiang · 2017-09-27T00:15:59Z

contributors/design-proposals/vault-based-kms-provider.md

+The KEK is generated in Vault and rotated using direct API call or CLI to Vault
+itself. The Key never leaves the vault.
+
+Note that when a key is rotated, Vault does not allow to choose a different


s/Vault does not allow to choose/Vault does not allow choosing/g

ericchiang · 2017-09-27T00:19:23Z

contributors/design-proposals/vault-based-kms-provider.md

+2. ``key-names`` list of names of the keys in Vault to be used. eg: key-name:
+kube-secret-enc-key.
+
+Note :  key name does not need to be changed if key is rotated in Vault, the


s/if key is/if the key is/

ericchiang · 2017-09-27T00:22:37Z

contributors/design-proposals/vault-based-kms-provider.md

+minimum:
+
+1. ``addr`` Vault service base endpoint eg.  https://example.com:8200
+2. ``key-names`` list of names of the keys in Vault to be used. eg: key-name:


Do you need an API version field for the config here to inform the <api-version> of the metadata listed above?

Probably do. Missed that. Thanks for catching it.

It is the existing EnvelopeTransformer that is reading the property from configuration file and adding it to metadata, Not the vaultEnvelopeService as mentioned in above : "Of the above metadata..."

vineet-garg · 2017-09-28T17:42:38Z

@lavalamp Compile time dependencies:
github.com/ghodss/yaml
github.com/hashicorp/vault/api

* Initial version of Vault KMS Provider * Renamed image file to be consistent with spec name * Brought spec inline with EnvelopeTransformer interfaces in PR 49350 * Wrap at 80

deads2k · 2017-10-05T18:49:19Z

/lgtm

Discussed in sig-auth and sig-apimachinery. This will be the second impl.

k8s-github-robot · 2017-10-05T18:50:31Z

Automatic merge from submit-queue.

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Aug 9, 2017

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Aug 10, 2017

kksriram mentioned this pull request Aug 10, 2017

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

Closed

deads2k added the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label Aug 10, 2017

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. kind/feature Categorizes issue or PR as related to a new feature. labels Aug 10, 2017

deads2k assigned liggitt, ericchiang and deads2k Aug 10, 2017

sakshamsharma reviewed Aug 10, 2017

View reviewed changes

dims reviewed Aug 10, 2017

View reviewed changes

kksriram commented Aug 10, 2017

View reviewed changes

k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 15, 2017

vineet-garg mentioned this pull request Aug 16, 2017

Vault provider oracle/kubernetes#1

Closed

hongchaodeng reviewed Aug 24, 2017

View reviewed changes

vineet-garg mentioned this pull request Aug 30, 2017

KMS vault provider oracle/kubernetes#3

Closed

vineet-garg mentioned this pull request Aug 31, 2017

Vault KMS provider for envelope encryption of secrets in a cluster oracle/kubernetes#4

Merged

kksriram mentioned this pull request Sep 1, 2017

Vault KMS provider for envelope encryption of secrets in a cluster kubernetes/kubernetes#51760

Closed

ericchiang reviewed Sep 27, 2017

View reviewed changes

deads2k mentioned this pull request Oct 2, 2017

Encryption at rest KMS integration kubernetes/enhancements#460

Closed

kksriram added 3 commits October 5, 2017 08:44

Vaultproviderspec (#1)

40fe163

* Initial version of Vault KMS Provider * Renamed image file to be consistent with spec name * Brought spec inline with EnvelopeTransformer interfaces in PR 49350 * Wrap at 80

resolved conflicts

65648f2

Typos addressing @ericchiang comments

526cd08

kksriram force-pushed the for/upstream/master/vault_provider_spec branch from 2841b7b to 526cd08 Compare October 5, 2017 15:56

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

k8s-github-robot merged commit badb60e into kubernetes:master Oct 5, 2017

kksriram deleted the for/upstream/master/vault_provider_spec branch October 5, 2017 19:37

ericchiang mentioned this pull request Oct 24, 2017

azure - Keyvault as KMS kubernetes/kubernetes#54439

Closed

danehans pushed a commit to danehans/community that referenced this pull request Jul 18, 2023

Nominating myself back as a maintainer (kubernetes#888)

3d0a0dc

Vaultproviderspec (#1) #888

Vaultproviderspec (#1) #888

Uh oh!

Conversation

kksriram commented Aug 9, 2017

Uh oh!

k8s-ci-robot commented Aug 9, 2017

Uh oh!

kksriram commented Aug 10, 2017

Uh oh!

kksriram commented Aug 10, 2017

Uh oh!

deads2k commented Aug 10, 2017

Uh oh!

kksriram commented Aug 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kksriram commented Aug 23, 2017

Uh oh!

hongchaodeng Aug 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcbsmpsn commented Aug 28, 2017

Uh oh!

liggitt commented Aug 29, 2017

Uh oh!

kksriram commented Aug 29, 2017

Uh oh!

deads2k commented Sep 19, 2017

Uh oh!

kksriram commented Sep 20, 2017

Uh oh!

deads2k commented Sep 26, 2017

Uh oh!

lavalamp commented Sep 26, 2017

Uh oh!

deads2k commented Sep 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hongchaodeng Aug 24, 2017 •

edited

Loading

deads2k commented Oct 5, 2017 •

edited

Loading