-
Notifications
You must be signed in to change notification settings - Fork 321
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add configuration for the vault Connect CA provider #872
Conversation
8537635
to
9085c09
Compare
f1ace61
to
010fd45
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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. |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, great call-out! I've added more info to this comment. Let me know if that makes sense!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
"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", | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that makes sense, thank you!!
@@ -88,17 +103,23 @@ path "consul/data/secret/gossip" { | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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 }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so beautifully readable!!
Changes proposed in this PR:
How I've tested this PR:
acceptance tests
How I expect reviewers to test this PR:
👀
Checklist: