Bootstrap gossip encryption with Vault #811

kschoche · 2021-10-28T18:38:57Z

Changes proposed in this PR:

remove the extra logic in vault_cluster for waiting for pods as this is no longer needed now that we're using vault dev-mode (thx @ndhanushkodi)
Add a new Stanza in values.yaml : global.secretsBackend which holds the Roles and has some light documentation on its usage (to be improved yet)
Add logic to bootstrap gossip encryption key to the server-statefulset+client-daemonset with Vault annotations.
Add unit and acceptance tests to validate above logic.

How I've tested this PR:

Acceptance tests are added which bootstrap consul using a gossip encryption key
bats tests

How I expect reviewers to test this PR:
Code Review + run the acceptance tests/bats tests.

Checklist:

Tests added
CHANGELOG entry added

HashiCorp engineers only, community PRs should not add a changelog entry.
Entries should use present tense (e.g. Add support for...)

acceptance/framework/vault/vault_cluster.go

charts/consul/test/unit/gossip-encryption-autogenerate-job.bats

t-eckert

Thank you Kyle, very cool.

acceptance/framework/vault/vault_cluster.go

acceptance/tests/vault/vault_test.go

charts/consul/templates/client-daemonset.yaml

charts/consul/templates/server-statefulset.yaml

charts/consul/values.yaml

acceptance/tests/vault/vault_test.go

acceptance/framework/vault/vault_cluster.go

ishustava

Great work on this, Kyle! I'm still reviewing and finishing up some testing but wanted to leave the comments I have so far. Most of them are small edits, but there are a couple of questions inline as well, plus two comments below.

When I ran TestVault_BootstrapConsulGossipEncryptionKey test locally, it did not pass with vault-agent-init for the server erroring out with 2021-11-04T22:33:49.427Z [ERROR] auth.handler: error authenticating: error="context deadline exceeded" backoff=9.8s and because of it the server-acl-init job could never finish.
I also have a larger UX question. I find it a bit counter-intuitive the way you need to set secretKey for a secret in vault that makes it inconsistent with how we do it in k8s.

For example, in Kubernetes when I create a secret with kubectl create secret generic foo --from-literal=key=bar, I'd then set secretName to foo and secretKey to key. The same thing is quite different in Vault. If I create a similar secret in vault with vault kv put internal/foo key="bar", then I would need to set secretKey to .Data.data.key which is kind of confusing. I know this is how vault injector works under the hood, but I wonder if we should make the UX consistent since the fact that we're using vault injector is an implementation detail and the user should not care about it.

acceptance/tests/vault/vault_test.go

charts/consul/templates/client-daemonset.yaml

ishustava · 2021-11-04T22:16:36Z

charts/consul/values.yaml

+      # Enabling the Vault secrets backend will cause Kubernetes secrets to no longer be used for the following Consul secrets:
+      # - gossip encryption key defined by `global.gossipEncryption.secretName/Key`


Could you make this description more generic? I think this will a bit be hard to maintain going forward if for every secret we're enabling for vault, we'd also need to update this description

charts/consul/values.yaml

acceptance/framework/vault/vault_cluster.go

Co-authored-by: Iryna Shustava <ishustava@users.noreply.github.com>

kschoche · 2021-11-11T15:57:28Z

Great work on this, Kyle! I'm still reviewing and finishing up some testing but wanted to leave the comments I have so far. Most of them are small edits, but there are a couple of questions inline as well, plus two comments below.

When I ran TestVault_BootstrapConsulGossipEncryptionKey test locally, it did not pass with vault-agent-init for the server erroring out with 2021-11-04T22:33:49.427Z [ERROR] auth.handler: error authenticating: error="context deadline exceeded" backoff=9.8s and because of it the server-acl-init job could never finish.

I also have a larger UX question. I find it a bit counter-intuitive the way you need to set secretKey for a secret in vault that makes it inconsistent with how we do it in k8s.
For example, in Kubernetes when I create a secret with kubectl create secret generic foo --from-literal=key=bar, I'd then set secretName to foo and secretKey to key. The same thing is quite different in Vault. If I create a similar secret in vault with vault kv put internal/foo key="bar", then I would need to set secretKey to .Data.data.key which is kind of confusing. I know this is how vault injector works under the hood, but I wonder if we should make the UX consistent since the fact that we're using vault injector is an implementation detail and the user should not care about it.

Hi @ishustava thanks for the awesome/helpful review!
Your question number 1 was a bug in Vault that is fixed by modifying the issuer field of the kube auth config for vault. Once we start running against Vault 1.9 this wont be needed.
For your second question, I've updated the values.yaml file to include language which describes how the user will interact with consul and vault, in doing so I also removed the need for any vault api-specific wording in the key fields so now the user just specifies "key" and not ".Data.data.key" as this is implicit. I was not able to figure out a way to remove the implicit "data" field of the secret path, but I think this is less of an issue as the vault reference guides and docs mention that you need to add "data/" after your kv2 mount point when reading. Thoughts?

ishustava

Good work, Kyle! I think everything looks good, I've added some minor edits.

My only other concern is about the issuer field for the vault auth method (see comment inline). I don't think it's blocking since for now it's passing on kind, but I think we should fix it soon because it'll cause problems when we merge the feature branch into main and run the nightly tests.

acceptance/framework/helpers/helpers.go

acceptance/framework/vault/vault_cluster.go

ishustava · 2021-11-12T02:45:56Z

acceptance/framework/vault/vault_cluster.go

@@ -158,7 +154,7 @@ func (v *VaultCluster) bootstrap(t *testing.T, ctx environment.TestContext) {
 	}
 	// We need to kubectl exec this one on the vault server:
 	// This is taken from https://learn.hashicorp.com/tutorials/vault/kubernetes-google-cloud-gke?in=vault/kubernetes#configure-kubernetes-authentication
-	cmdString := fmt.Sprintf("VAULT_TOKEN=%s vault write auth/kubernetes/config token_reviewer_jwt=\"$(cat /var/run/secrets/kubernetes.io/serviceaccount/token)\" kubernetes_host=\"https://${KUBERNETES_PORT_443_TCP_ADDR}:443\" kubernetes_ca_cert=@/var/run/secrets/kubernetes.io/serviceaccount/ca.crt", vaultRootToken)
+	cmdString := fmt.Sprintf("VAULT_TOKEN=%s vault write auth/kubernetes/config issuer=\"https://kubernetes.default.svc.cluster.local\" token_reviewer_jwt=\"$(cat /var/run/secrets/kubernetes.io/serviceaccount/token)\" kubernetes_host=\"https://${KUBERNETES_PORT_443_TCP_ADDR}:443\" kubernetes_ca_cert=@/var/run/secrets/kubernetes.io/serviceaccount/ca.crt", vaultRootToken)


I just ran into something today. It seems like the issuer on gke is different (it's kubernetes/serviceaccount). After looking into it for a bit, it seems that the issue is with the version of k8s: as of 1.21+ the tokens mounted to pods have a different format (this new format would require the issuer to be specified in the vault auth method config). The more interesting part is that the jwt token mounted into your pod is now a temporary token that expires after some time and is renewed by kube periodically, while the token that's in the k8s secret still has the old format.

I think we should change this to instead use the ca cert and the jwt token from the service account token secret instead of exec'ing into the pod. Using this method might not require us to provide an issuer and should hopefully work the same across k8s versions too.

Once we do that, I'd also suggest running nightlies with this test to confirm that it works for the versions of k8s we're testing with. This can be done after this PR is merged, but since we'll run into it eventually (i.e. when the feature branch is merged), it's probably better to deal with this now rather than later.

ishustava · 2021-11-12T02:51:49Z

acceptance/tests/vault/vault_test.go

+		"global.acls.manageSystemACLs":       "true",
+		"global.tls.enabled":                 "true",
+		"global.gossipEncryption.secretName": "consul/data/secret/gossip",
+		"global.gossipEncryption.secretKey":  "gossip",


acceptance/tests/vault/vault_test.go

ishustava · 2021-11-12T02:54:25Z

charts/consul/templates/_helpers.tpl

+            {{ "{{" }}- with secret "{{ .secretName }}" -{{ "}}" }}
+            {{ "{{" }}- {{ printf ".Data.data.%s" .secretKey }} -{{ "}}" }}
+            {{ "{{" }}- end -{{ "}}" }}
+{{- end -}}


❤️ this refactor! makes it look much cleaner

charts/consul/values.yaml

Co-authored-by: Iryna Shustava <ishustava@users.noreply.github.com>

Add base bootstrapping logic and acceptance tests

dad54ce

kschoche added area/chart-only Related to changes that simply require yaml Helm chart changes, e.g. exposing a new field vault labels Oct 28, 2021

kschoche self-assigned this Oct 28, 2021

kschoche added 3 commits October 28, 2021 13:42

a little bit of cleanup

60ea8ea

add bats tests

209be6d

add fail if secrename/key arent specified properly

547888e

kschoche commented Nov 1, 2021

View reviewed changes

acceptance/framework/vault/vault_cluster.go Outdated Show resolved Hide resolved

kschoche commented Nov 1, 2021

View reviewed changes

charts/consul/test/unit/gossip-encryption-autogenerate-job.bats Show resolved Hide resolved

kschoche requested review from a team, ishustava, sadjamz and t-eckert and removed request for a team November 1, 2021 14:32

kschoche marked this pull request as ready for review November 1, 2021 14:32

kschoche added 2 commits November 1, 2021 10:57

test

51d012a

test

52d4af6

t-eckert approved these changes Nov 1, 2021

View reviewed changes

kschoche added 3 commits November 1, 2021 14:06

PR comments

e80cd63

Merge branch 'main' into gossip-boostrapping

7db948d

Merge branch 'consul-vault-base' into gossip-boostrapping

98f2fa3

kschoche commented Nov 1, 2021

View reviewed changes

acceptance/tests/vault/vault_test.go Outdated Show resolved Hide resolved

kschoche commented Nov 1, 2021

View reviewed changes

acceptance/framework/vault/vault_cluster.go Outdated Show resolved Hide resolved

update gossip stanza

2d214c0

ishustava reviewed Nov 4, 2021

View reviewed changes

kschoche and others added 5 commits November 5, 2021 12:59

some cleanup

4bbe42b

update templating and fix some tests

2c8960e

Apply suggestions from code review

843be93

Co-authored-by: Iryna Shustava <ishustava@users.noreply.github.com>

changes

39570a3

update bats tests

2e24a03

kschoche added 3 commits November 9, 2021 13:25

remove .data.data

9081bc3

bug fixes in test

2cb9e65

update test framework to use k8s svc as issuer to work around vault bug

b3a4d9f

update autogen test since secretkey and name together are required now

f7f3705

ishustava approved these changes Nov 12, 2021

View reviewed changes

kschoche and others added 3 commits November 12, 2021 08:59

Apply suggestions from code review

bb266b3

Co-authored-by: Iryna Shustava <ishustava@users.noreply.github.com>

review comments

1f06598

update retries

d99e845

kschoche merged commit 80102a5 into consul-vault-base Nov 12, 2021

kschoche deleted the gossip-boostrapping branch November 12, 2021 19:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bootstrap gossip encryption with Vault #811

Bootstrap gossip encryption with Vault #811

kschoche commented Oct 28, 2021 •

edited

Loading

t-eckert left a comment

ishustava left a comment

ishustava Nov 4, 2021

kschoche commented Nov 11, 2021

ishustava left a comment

ishustava Nov 12, 2021

ishustava Nov 12, 2021

ishustava Nov 12, 2021

		# Enabling the Vault secrets backend will cause Kubernetes secrets to no longer be used for the following Consul secrets:
		# - gossip encryption key defined by `global.gossipEncryption.secretName/Key`

Bootstrap gossip encryption with Vault #811

Bootstrap gossip encryption with Vault #811

Conversation

kschoche commented Oct 28, 2021 • edited Loading

t-eckert left a comment

Choose a reason for hiding this comment

ishustava left a comment

Choose a reason for hiding this comment

ishustava Nov 4, 2021

Choose a reason for hiding this comment

kschoche commented Nov 11, 2021

ishustava left a comment

Choose a reason for hiding this comment

ishustava Nov 12, 2021

Choose a reason for hiding this comment

ishustava Nov 12, 2021

Choose a reason for hiding this comment

ishustava Nov 12, 2021

Choose a reason for hiding this comment

kschoche commented Oct 28, 2021 •

edited

Loading