Support for Hashicorp Vault #198

Pluies · 2019-11-05T17:25:16Z

Hello all!

I've been building on @bboerst 's work from his fork to add support for Hashicorp Vault as a backend for external-secrets. 🎉

I've currently got this working through the bundled Helm Chart, which I've had to tweak a little to add the necessary bits and pieces.

I've also added a first draft of documentation, please let me know what you think / what could be improved!

Fixes #62

Pluies · 2019-11-05T17:37:00Z

cc @silasbw @JacopoDaeli @jeffpearce from the linked issue 👍

silasbw

Awesome 🔥 🌶️ ! I'm excited to get this merged in.

Can you add some basic unit testing?

silasbw · 2019-11-05T21:18:51Z

README.md

+  # This role will need to be bound to kubernetes-external-secret's ServiceAccount; see Vault's documentation:
+  # https://www.vaultproject.io/docs/auth/kubernetes.html
+  vaultRole: my-vault-role
+  properties:


rename to data. we renamed this to data a while ago (but are still backwards compatible with properties).

Sounds good! Fixing...

silasbw · 2019-11-05T21:19:47Z

examples/hello-service-external-secret-vault.yml

+  backendType: vault
+  vaultMountPoint: my-kubernetes-vault-mount-point
+  vaultRole: my-vault-role
+  properties:


rename to data

Fixing here too 👍

silasbw · 2019-11-05T21:22:49Z

lib/backends/vault-backend.js

+   */
+  async getSecretManifestData ({ secretDescriptor }) {
+    const data = {}
+    // Use secretDescriptor.properties to be backwards compatible.


misplaced comment

Oh yes, punted it a few lines down

lib/backends/vault-backend.js

Flydiverny · 2019-11-05T20:47:18Z

lib/backends/vault-backend.js

+  async getSecretManifestData ({ secretDescriptor }) {
+    const data = {}
+    // Use secretDescriptor.properties to be backwards compatible.
+    const vaultMountPoint = secretDescriptor.vaultMountPoint
+    const vaultRole = secretDescriptor.vaultRole
+
+    const externalData = secretDescriptor.data || secretDescriptor.properties
+    const secretPropertyValues = await this._fetchSecretPropertyValues({
+      vaultMountPoint,
+      vaultRole,
+      externalData
+    })
+    externalData.forEach((secret, index) => {
+      data[secret.name] = (Buffer.from(secretPropertyValues[index], 'utf8')).toString('base64')
+    })
+    return data
+  }


Think it would be good if we didn't have to maintain duplicates of getSecretManifestData and _fetchSecretPropertyValues now theres an additional dataFrom support in kv-backend as well :)

Perhaps we could pass along extras from secretDescriptor

ie changing
https://github.com/godaddy/kubernetes-external-secrets/blob/25e2f7426ee9bedeffe02e7d8e8befd07fedad30/lib/backends/kv-backend.js#L87-L95

to something like

async getSecretManifestData ({ secretDescriptor: { // Use secretDescriptor.properties to be backwards compatible. properties = [], data = properties, dataFrom = [], ...secretDescriptorOptions } }) {

and pass along secretDescriptorConfig to the fetch functions which can send it down to abstract get ie

const [dataFromValues, dataValues] = await Promise.all([ this._fetchDataFromValues({ dataFrom, secretDescriptorOptions}), this._fetchDataValues({ data, secretDescriptorOptions}) ])

then signature of _get could become

_get ({ secretKey, secretDescriptorOptions }) {

@silasbw forgot to press post 😅

keweilu · 2019-11-06T00:51:08Z

charts/kubernetes-external-secrets/templates/rbac.yaml

@@ -46,4 +46,22 @@ subjects:
  - name: {{ template "kubernetes-external-secrets.serviceAccountName" . }}
    namespace: {{ .Release.Namespace | quote }}
    kind: ServiceAccount
+---
+apiVersion: rbac.authorization.k8s.io/v1beta1
+kind: ClusterRoleBinding


can you add this to https://github.com/godaddy/kubernetes-external-secrets/blob/master/external-secrets.yml?

keweilu · 2019-11-06T00:52:23Z

config/environment.js

@@ -16,6 +16,7 @@ if (environment === 'development') {
  require('dotenv').config()
 }

+const vaultEndpoint = process.env.VAULT_ADDR || 'http://127.0.0.1:8200'


add the VAULT_ADDR env variable to the chart values file https://github.com/godaddy/kubernetes-external-secrets/blob/master/charts/kubernetes-external-secrets/values.yaml#L6 and update README?

Good point, adding this!

Pluies · 2019-11-06T11:34:20Z

Alright, I've addressed all code comments; now adding unit tests as well as per @silasbw comment 👍

silasbw

i think this is awesome. i think kubernetes-external-secrets has an issue with our kv-backend.js/KVBackend implementation being too specific to AWS, but i think that's outside the scope of this PR (i'll let @Flydiverny confirm :))

silasbw · 2019-11-06T20:50:49Z

README.md

@@ -217,7 +218,7 @@ data:

 ## Backends

-kubernetes-external-secrets supports both AWS Secrets Manager and AWS System Manager.
+kubernetes-external-secrets supports AWS Secrets Manager, AWS System Manager, and Hashicorp Vault.


Flydiverny · 2019-11-06T23:02:51Z

Think we can merge this as is once tests are in, and do a separate PR to improve kv-backend! :)

doh, forgot about the tests :)

Pluies · 2019-11-07T10:32:28Z

The tests are here! 🎉

Pluies · 2019-11-07T17:35:13Z

Uh oh, wait a bit before merging please – I just realised that this version (post-reviews) now creates undefined keys in the Secret:

apiVersion: v1
kind: Secret
metadata:
  name: hello-service
  namespace: default
type: Opaque
data:
  undefined: b3Blbiwgc2VzYW1l

I have to leave for the day but will review this and fix it asap tomorrow.

Pluies · 2019-11-08T10:59:50Z

Found & fixed the issue & rebased on top of master. This is ready to merge if approved :)

Flydiverny

name is currently disregarded when generating the secret, ie name field should control what name is set in the generated secret.

README.md

examples/hello-service-external-secret-vault.yml

Flydiverny · 2019-11-08T11:24:03Z

lib/backends/vault-backend.js

+      externalData
+    })
+    externalData.forEach((secret, index) => {
+      data[secret.property] = (Buffer.from(secretPropertyValues[index], 'utf8')).toString('base64')


this should be set by secret.name instead of property

Flydiverny · 2019-11-08T11:26:38Z

I was also perhaps a bit fast on merging #204 (meanwhile you rebased) but I can fix that part after this is merged

VAULT_ENDPOINT is now VAULT_ADDR to keep with the vault defaults. The RBAC is necessary for the Vault Kubernetes authentication method. Fix property lookup by stringifying only if we're sending back the whole secret data rather than just one property.

…ample yml

Pluies · 2019-11-08T12:32:24Z

@Flydiverny thanks for the feedback! I've amended this PR by adding name where needed, and changing secretDescriptor to spec to align with #204.

Flydiverny

Whoop think this is good to go! 🎉
@silasbw wanna take a last look?

silasbw · 2019-11-08T18:55:14Z

We just released 2.1.0 which includes vault support. thank you for contributing this 🥂 !

silasbw reviewed Nov 5, 2019

View reviewed changes

lib/backends/vault-backend.js Show resolved Hide resolved

silasbw requested review from Flydiverny and keweilu November 5, 2019 21:26

Flydiverny reviewed Nov 6, 2019

View reviewed changes

keweilu reviewed Nov 6, 2019

View reviewed changes

Pluies force-pushed the master branch from f301667 to 9eeeb0b Compare November 6, 2019 11:32

silasbw previously approved these changes Nov 6, 2019

View reviewed changes

Flydiverny mentioned this pull request Nov 8, 2019

refactor: make kv backend more generic #206

Merged

Pluies force-pushed the master branch from 1c46a34 to 82f7388 Compare November 8, 2019 10:49

Flydiverny suggested changes Nov 8, 2019

View reviewed changes

Brian Boerst and others added 11 commits November 8, 2019 12:27

Inital adding of backend components for vault.

2545ba6

Update package-lock.json to reflect new vault dependency

5d053ff

Support securityContext in chart

42af483

Simplify the logic by requiring .property

e4baf8d

Hashicorp Vault documentation

18d8714

Linting

e233fdb

"properties" should be "data"

0494bb5

Use consts instead of vars

948b5d5

Add VAULT_ADDR defaults & documentation, add ClusterRoleBinding in ex…

88d39f3

…ample yml

Vault: add backend test

078e39f

Florent Delannoy added 4 commits November 8, 2019 12:27

Remove comment

47fd9c7

Remove unused var

e44d38b

Fix getting property

2048dea

secretDescriptor -> spec; add name

b56c367

Pluies force-pushed the master branch from 82f7388 to b56c367 Compare November 8, 2019 12:31

Flydiverny approved these changes Nov 8, 2019

View reviewed changes

silasbw approved these changes Nov 8, 2019

View reviewed changes

silasbw merged commit d61312c into external-secrets:master Nov 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for Hashicorp Vault #198

Support for Hashicorp Vault #198

Pluies commented Nov 5, 2019

Pluies commented Nov 5, 2019

silasbw left a comment

silasbw Nov 5, 2019

Pluies Nov 6, 2019

silasbw Nov 5, 2019

Pluies Nov 6, 2019

silasbw Nov 5, 2019

Pluies Nov 6, 2019

Flydiverny Nov 5, 2019 •

edited

Loading

keweilu Nov 6, 2019

Pluies Nov 6, 2019

keweilu Nov 6, 2019

Pluies Nov 6, 2019

Pluies commented Nov 6, 2019

silasbw left a comment

silasbw Nov 6, 2019

Flydiverny commented Nov 6, 2019

Pluies commented Nov 7, 2019

Pluies commented Nov 7, 2019

Pluies commented Nov 8, 2019

Flydiverny left a comment

Flydiverny Nov 8, 2019

Flydiverny commented Nov 8, 2019

Pluies commented Nov 8, 2019

Flydiverny left a comment

silasbw commented Nov 8, 2019

Support for Hashicorp Vault #198

Support for Hashicorp Vault #198

Conversation

Pluies commented Nov 5, 2019

Pluies commented Nov 5, 2019

silasbw 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

Choose a reason for hiding this comment

Flydiverny Nov 5, 2019 • 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

Pluies commented Nov 6, 2019

silasbw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Flydiverny commented Nov 6, 2019

Pluies commented Nov 7, 2019

Pluies commented Nov 7, 2019

Pluies commented Nov 8, 2019

Flydiverny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Flydiverny commented Nov 8, 2019

Pluies commented Nov 8, 2019

Flydiverny left a comment

Choose a reason for hiding this comment

silasbw commented Nov 8, 2019

Flydiverny Nov 5, 2019 •

edited

Loading