Add configuration for the vault Connect CA provider #872

ishustava · 2021-11-22T19:14:52Z

Changes proposed in this PR:

Adds helm values to configure vault connect ca provider
Uses k8s auth method to configure connect CA provider in consul (Support auth methods for the vault connect CA provider consul#11573)
This PR does not work if vault is configured with TLS (this change will come as a separate PR)

How I've tested this PR:
acceptance tests

How I expect reviewers to test this PR:
👀

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/tests/vault/vault_test.go

charts/consul/templates/server-config-configmap.yaml

acceptance/framework/helpers/helpers.go

kschoche

Fantastic work @ishustava!!

I just had one question which is not a blocker...

I see that you've added policies against sys/mounts and a bunch of others based on the consul.io page you linked (thx!), and it looks like this actually allows Consul to create/manage the vault pieces (which explains all the policy paths).

There is a second method where you enable the PKI paths from vault instead of consul like this, I'll be using it for Server-TLS and we could extend the approach to connect-ca as well:

      // Enable the PKI Secrets engine.
       err = v.vaultClient.Sys().Mount("pki", &vapi.MountInput{
               Type:   "pki",
               Config: vapi.MountConfigInput{},
       })
       if err != nil {
               t.Fatal("unable to mount pki secrets engine to pki", "err", err)
       }

What I'm wondering is if this is a reasonable/realistic approach to expect that users would allow Consul to manage the vault paths, or would users more likely be expecting to do this from Vault instead of Consul. Does it really matter since it's just acceptance tests? (probably not). Thoughts?

charts/consul/values.yaml

ishustava · 2021-11-23T22:04:00Z

There is a second method where you enable the PKI paths from vault instead of consul like this, I'll be using it for Server-TLS and we could extend the approach to connect-ca as well:

Ah that makes sense! yeah feel free to change this policy if we don't need those paths anymore once we have the PKI mounted.

For users, I think it depends if they are using TLS or not and also if they want to allow consul to do the mounting. consul docs also have another policy that is more restrictive but I chose this one because it's simpler for acceptance tests.

ndhanushkodi · 2021-11-23T22:11:18Z

acceptance/tests/vault/vault_test.go

 	require.NoError(t, err)

-	// Create the Auth Roles for consul-server + consul-client.
-	logger.Log(t, "Creating the consul-server and consul-client-roles")
+	// Create the Auth Roles for consul-server and consul-client.


Could we say "Create the Auth Roles for consul-server and consul-client which enables ___"? I wasn't sure what auth roles do until I read this (https://www.vaultproject.io/docs/auth/kubernetes#configuration) so maybe it'd be nice to add a tiny summary in the comment?

Yeah, great call-out! I've added more info to this comment. Let me know if that makes sense!

ndhanushkodi

Really awesome work!! This is the first vault PR I'm reviewing so I just had some comments around where adding some clarifications could be nice based on where I wasn't sure of something. No worries if a more appropriate place to address some of this would be in official documentation though. I really love how the acceptance test kind of self documents for even someone like me coming back to looking at this project after a long time!

ndhanushkodi · 2021-11-23T22:26:02Z

acceptance/tests/vault/vault_test.go

+		"global.secretsBackend.vault.connectCA.address":             fmt.Sprintf("http://%s-vault:8200", vaultReleaseName),
+		"global.secretsBackend.vault.connectCA.rootPKIPath":         "connect_root",
+		"global.secretsBackend.vault.connectCA.intermediatePKIPath": "connect_inter",
+


why are the connectCA keys under global.secretsBackend.vault but the gossip stuff is under global.gossipEncryption? Were we just trying to respect the previously used values for the gossipEncryption configuration? I guess I found it slightly unintuitive that you configure different vault things under different keys but this is not blocking the PR, just wanted to clarify if this is the case. cc @kschoche

Good question. I actually suggested that we use existing configuration for gossip secret to make a consistent regardless of whether you use k8s secrets or vault secrets. I think this allows us to re-use existing configuration that is for the same purpose rather than duplicate it for vault specifically. So in this case, gossip key configuration is just a reference to some secret which could be in k8s or in vault.

Connect CA config is a bit different. First, it doesn't require any secrets since Consul can create all those secrets in Vault as long as the policy allows. Second, this configuration, unlike gossip key config, is specific to vault connect CA provider and so these options only make sense under secretsBackend.vault

that makes sense, thank you!!

ndhanushkodi · 2021-11-23T22:31:09Z

acceptance/tests/vault/vault_test.go

@@ -88,17 +103,23 @@ path "consul/data/secret/gossip" {
 	}


why don't we need to generate anything for the connect ca like we generated the gossip secret? is the only info vault needs in this case just the address and two paths and it will generate the certs itself? maybe its worth a comment here but also if this is just something that would be documented in user facing docs that's fine too.

is the only info vault needs in this case just the address and two paths and it will generate the certs itself?

Yep, except it's Consul that will generate it for us (as long as the policy allows it to do so).

From https://www.consul.io/docs/connect/ca/vault#root-and-intermediate-pki-paths:

If either path does not exist, then Consul will attempt to mount and initialize it. This requires additional privileges by the Vault token in use. If the paths already exist, Consul will use them as configured.

ndhanushkodi · 2021-11-23T22:35:01Z

charts/consul/templates/server-config-configmap.yaml

@@ -11,6 +11,36 @@ metadata:
    heritage: {{ .Release.Service }}
    release: {{ .Release.Name }}
 data:
+  {{- $vaultConnectCAEnabled := and .Values.global.secretsBackend.vault.connectCA.address .Values.global.secretsBackend.vault.connectCA.rootPKIPath .Values.global.secretsBackend.vault.connectCA.intermediatePKIPath -}}
+  {{- if and .Values.global.secretsBackend.vault.enabled $vaultConnectCAEnabled }}


so beautifully readable!!

charts/consul/values.yaml

ishustava added 3 commits November 19, 2021 13:11

wip: support connect CA without TLS

9e1c4d0

add unit tests

bcf121d

include additional ca example

274b5f6

ishustava commented Nov 22, 2021

View reviewed changes

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

remove todo; update consul image

9085c09

ishustava force-pushed the connect-ca branch from 8537635 to 9085c09 Compare November 22, 2021 19:16

ishustava requested review from kschoche, a team and ndhanushkodi and removed request for a team November 22, 2021 19:18

fix linter error

814a650

kschoche reviewed Nov 22, 2021

View reviewed changes

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

kschoche reviewed Nov 22, 2021

View reviewed changes

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

kschoche reviewed Nov 22, 2021

View reviewed changes

charts/consul/templates/server-config-configmap.yaml Show resolved Hide resolved

ishustava added 3 commits November 22, 2021 15:35

Make sure there are pods in WaitForAllPodsToBeReady

6333008

Add clarifying comment about connect CA policy

f957474

rename rules->policy

010fd45

ishustava force-pushed the connect-ca branch from f1ace61 to 010fd45 Compare November 22, 2021 23:57

kschoche reviewed Nov 23, 2021

View reviewed changes

acceptance/framework/helpers/helpers.go Show resolved Hide resolved

kschoche approved these changes Nov 23, 2021

View reviewed changes

ishustava commented Nov 23, 2021

View reviewed changes

charts/consul/values.yaml Outdated Show resolved Hide resolved

Update charts/consul/values.yaml

3eb912a

ndhanushkodi reviewed Nov 23, 2021

View reviewed changes

Add a clarifying comment about vault roles

cc8f3a2

ndhanushkodi approved these changes Nov 23, 2021

View reviewed changes

Update based on Nitya's review

2d0013b

ishustava merged commit 9c515aa into consul-vault-base Nov 24, 2021

ishustava deleted the connect-ca branch November 24, 2021 20:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add configuration for the vault Connect CA provider #872

Add configuration for the vault Connect CA provider #872

ishustava commented Nov 22, 2021 •

edited

Loading

kschoche left a comment

ishustava commented Nov 23, 2021 •

edited

Loading

ndhanushkodi Nov 23, 2021 •

edited

Loading

ishustava Nov 23, 2021

ndhanushkodi left a comment

ndhanushkodi Nov 23, 2021

ishustava Nov 23, 2021

ndhanushkodi Nov 24, 2021

ndhanushkodi Nov 23, 2021

ishustava Nov 23, 2021 •

edited

Loading

ndhanushkodi Nov 23, 2021

Add configuration for the vault Connect CA provider #872

Add configuration for the vault Connect CA provider #872

Conversation

ishustava commented Nov 22, 2021 • edited Loading

kschoche left a comment

Choose a reason for hiding this comment

ishustava commented Nov 23, 2021 • edited Loading

ndhanushkodi Nov 23, 2021 • edited Loading

Choose a reason for hiding this comment

ishustava Nov 23, 2021

Choose a reason for hiding this comment

ndhanushkodi left a comment

Choose a reason for hiding this comment

ndhanushkodi Nov 23, 2021

Choose a reason for hiding this comment

ishustava Nov 23, 2021

Choose a reason for hiding this comment

ndhanushkodi Nov 24, 2021

Choose a reason for hiding this comment

ndhanushkodi Nov 23, 2021

Choose a reason for hiding this comment

ishustava Nov 23, 2021 • edited Loading

Choose a reason for hiding this comment

ndhanushkodi Nov 23, 2021

Choose a reason for hiding this comment

ishustava commented Nov 22, 2021 •

edited

Loading

ishustava commented Nov 23, 2021 •

edited

Loading

ndhanushkodi Nov 23, 2021 •

edited

Loading

ishustava Nov 23, 2021 •

edited

Loading