Support vault cacert bytes env #507

tomhjp · 2023-08-02T12:58:52Z

This builds on #446 to support setting CA certificates for injected pods, but uses VAULT_CACERT_BYTES instead of writing files. It turned out Vault Agent's auto-auth already supported that env var but templating did not because consul-template hadn't implemented it yet. This was fixed in Vault 1.14.2 and 1.15.0, via hashicorp/vault#22322. Vault 1.15.0 also added the -dev-tls-san flag via hashicorp/vault#22657 which lets us easily test with self-signed certificates.

To test and teardown locally, use make image deploy exercise teardown in a local kind cluster. I added that workflow to CI as well to make sure any breakages are found quickly.

kschoche

Everything looks great, I had just a question around whether or not we should require b64 encoding of the field?

kschoche · 2023-08-02T16:10:09Z

agent-inject/agent/agent.go

 	CACert string

+	// CACertBytes is the contents of the CA certificate to trust
+	// for TLS with Vault as a PEM-encoded certificate or bundle.


Is this expected to be b64 encoded? Should we specify one way or another?

It can be PEM-encoded or base64. I added some comments in d50e787 to document the optional base64 encoding.

kschoche · 2023-08-02T16:43:08Z

agent-inject/agent/container_env.go

+	if a.Vault.CACertBytes != "" {
+		envs = append(envs, corev1.EnvVar{
+			Name:  "VAULT_CACERT_BYTES",
+			Value: decodeIfBase64(a.Vault.CACertBytes),


I guess going back to my previous comment, is there any reason not to make b64 encoding a requirement? The formatting of certs with all their carriage returns is to me frustrating at best trying to get into string values.

I don't see the benefit of making b64 required. Supporting the raw PEM is nice because it's how the environment variable is eventually consumed anyway, and supporting b64 is nice because as you say it can be frustrating when there are multiple levels of interpretation in the automation that gets the value into place.

tvoran · 2023-08-05T01:24:14Z

I think vault has a dev-tls mode now that generates its own certs, might able to use that instead of cert-manager, if we can copy the generated cert out of the vault container.

helm install vault hashicorp/vault \
  --set='server.dev.enabled=true' \
  --set='server.extraArgs=-dev-tls -dev-tls-cert-dir=/tmp' \
  ...

tomhjp · 2023-08-15T12:07:32Z

I think vault has a dev-tls mode now that generates its own certs, might able to use that instead of cert-manager

I considered this before but didn't give it a fair shot because of the fact it would require a multi-stage Vault helm chart rollout. One part to get Vault and its certificate, and then another part to give those certificate details to the injector. I had a more serious go anyway in c8fc7ff, and it's actually a really nice simplification overall. Unfortunately though, -dev-tls doesn't support custom DNS names, so we end up with this error:

Error checking seal status: Get "https://vault.default.svc:8200/v1/sys/seal-status": tls: failed to verify certificate: x509: certificate is valid for localhost, localhost4, localhost6, localhost.localdomain, vault-0, not vault.default.svc

We can get around it by setting VAULT_SKIP_VERIFY to true, but then that doesn't test the custom CA either. I might raise a separate Vault PR to support custom DNS names for -dev-tls, as that would be a nice option in the future.

tomhjp · 2023-08-15T12:27:32Z

Vault v1.14.2 should have full support for VAULT_CACERT_BYTES as long as hashicorp/vault#22338 lands in time. I'm planning to hold off merging this until that releases, because otherwise we'll be stuck with the hack in this PR for a long time for backwards compatibility reasons.

M0NsTeRRR · 2023-10-22T20:30:52Z

Hello,
Thanks for your works :), any news to get this merged @tomhjp ?
Regards,

tomhjp · 2023-10-23T15:03:31Z

Thanks for the reminder @M0NsTeRRR - I just pushed a few updates that take advantage of Vault updates in 1.14.2 and 1.15.0. There should be a release of both Vault and vault-helm pretty soon, at which point I can remove the version pinning from dev.values.yaml and this should be ready to land.

@tvoran @kschoche, the updates are non-trivial so you may want to have another scan, but nothing we haven't already discussed in the PR feedback. The only remaining change I'm planning to make before merging is to get rid of the version pinning in dev.values.yaml.

tomhjp · 2023-10-23T15:04:59Z

Actually, we'll also be releasing vault-k8s soon, and it would be nice to get this change into that release, so perhaps I won't wait for the vault-helm release, but instead just give @tvoran and @kschoche a chance to have another pass at reviewing. I'll back out the version pinning in a separate PR after all the releases are done.

tvoran

Worked great for me testing locally. Would you mind updating the PR description since I think it's changed a bit since its original incarnation? And a changelog entry as well.

kschoche · 2023-10-23T18:35:23Z

(still) Looks good to me, I like the approach! 👍🏽

tomhjp · 2023-10-24T09:48:31Z

Thanks for the reviews!

Would you mind updating the PR description since I think it's changed a bit since its original incarnation? And a changelog entry as well.

Thanks for the reminder, done 👍

tomhjp added 2 commits August 1, 2023 22:07

Support setting VAULT_CACERT_BYTES for sidecars

854ae71

Workaround for consul-template not supporting VAULT_CACERT_BYTES

0584c89

tomhjp requested review from tvoran and kschoche August 2, 2023 12:58

This was referenced Aug 2, 2023

Adding support for the webhook injector to be able to inject a cert into the mutated Pods using an environment var. #446

Closed

Support for setting Vault CA from VAULT_CACERT_BYTES env hashicorp/consul-template#1782

Merged

kschoche approved these changes Aug 2, 2023

View reviewed changes

tomhjp added 2 commits August 15, 2023 13:23

Add comments about optional base64 encoding

d50e787

One more comment

c66100d

tomhjp added 6 commits October 23, 2023 13:42

Use Vault's -dev-tls flag instead

b28d518

Merge branch 'main' into support-vault-cacert-bytes-env

ef1efc5

Backout hacks and use -dev-tls-san option from Vault 1.15.0

89a252b

backout debug Makefile changes

8287054

Simplify CA sharing and pin to 1.15.0 until next release

45c63a0

Delete unused variable

da6722b

tvoran approved these changes Oct 23, 2023

View reviewed changes

tomhjp added 2 commits October 24, 2023 10:37

Use 'make deploy exercise teardown' in CI to ensure it continues to work

f2b80fa

Add changelog

ca52e6f

Fix relative directories in CI

93ed4d9

tomhjp merged commit 599c50f into main Oct 24, 2023
18 checks passed

tomhjp deleted the support-vault-cacert-bytes-env branch October 24, 2023 11:52

tomhjp mentioned this pull request Oct 24, 2023

Mechanism for injecting vault-ca.crt #19

Closed

tomhjp mentioned this pull request Oct 24, 2023

custom CA for external vault server #83

Closed

LS80 mentioned this pull request Nov 8, 2023

Add support for AGENT_INJECT_VAULT_CACERT_BYTES hashicorp/vault-helm#974

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support vault cacert bytes env #507

Support vault cacert bytes env #507

tomhjp commented Aug 2, 2023 •

edited

Loading

kschoche left a comment

kschoche Aug 2, 2023

tomhjp Aug 15, 2023

kschoche Aug 2, 2023

tomhjp Aug 15, 2023

tvoran commented Aug 5, 2023 •

edited

Loading

tomhjp commented Aug 15, 2023

tomhjp commented Aug 15, 2023

M0NsTeRRR commented Oct 22, 2023

tomhjp commented Oct 23, 2023

tomhjp commented Oct 23, 2023

tvoran left a comment

kschoche commented Oct 23, 2023

tomhjp commented Oct 24, 2023

Support vault cacert bytes env #507

Support vault cacert bytes env #507

Conversation

tomhjp commented Aug 2, 2023 • edited Loading

kschoche left a comment

Choose a reason for hiding this comment

kschoche Aug 2, 2023

Choose a reason for hiding this comment

tomhjp Aug 15, 2023

Choose a reason for hiding this comment

kschoche Aug 2, 2023

Choose a reason for hiding this comment

tomhjp Aug 15, 2023

Choose a reason for hiding this comment

tvoran commented Aug 5, 2023 • edited Loading

tomhjp commented Aug 15, 2023

tomhjp commented Aug 15, 2023

M0NsTeRRR commented Oct 22, 2023

tomhjp commented Oct 23, 2023

tomhjp commented Oct 23, 2023

tvoran left a comment

Choose a reason for hiding this comment

kschoche commented Oct 23, 2023

tomhjp commented Oct 24, 2023

tomhjp commented Aug 2, 2023 •

edited

Loading

tvoran commented Aug 5, 2023 •

edited

Loading