Skip to content
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

Present Consul server cert chain when using Vault secrets backend #1251

Merged

Conversation

brucec5
Copy link
Contributor

@brucec5 brucec5 commented Jun 1, 2022

Changes proposed in this PR:

  • Present full cert chain (minus root cert) when using Vault as the secrets backend
  • Update some bats tests to get them to pass

I was told by some HC support/CSM folks to open this as a PR. I don't know if this is the best way to accomplish this, or if the testing is sufficient, but hopefully this is useful. This was initially just meant to be a POC to show the engineering team what I did to address the issue on my end.

Currently, only the leaf cert gets presented by the Consul server (both UI and API). This works fine if you have a self-signed cert, but if you're using Vault as a secrets backend with an intermediate CA (and perhaps an offline root CA), then you can't currently verify the full chain.

The fix is to loop over the CA chain and append the intermediate cert(s) to the end of the server cert. This lets Consul present the full chain, which allows for proper validation.

This is related to #879, but only addresses the Vault secrets backend. Different work will need to be done to support the Kubernetes secrets backend.

How I've tested this PR:

I set up an offline root CA cert and used that to sign an L1 ICA in Vault, and used the L1 ICA to sign an L2 ICA. I then used that L2 ICA to issue certs for Consul. I added the root CA cert to my system's trust store and was able to access the UI in the browser without running into a TLS verification error.

It's been a while, but I'm pretty sure I also tested setting up a root CA directly in Vault and using that to issue certs for Consul.

Please feel free to suggest additional testing.

How I expect reviewers to test this PR:

  • Deploy Vault to a local K8s cluster
  • Set up a root CA in Vault
  • Set up an L1 ICA in Vault signed by the root CA
  • Optionally set up an L2 ICA (and so on)
  • Configure a values.yaml for Consul that uses the Vault secrets backend for TLS certs with TLS enabled
  • Either add the root CA cert to your system trust store and load the UI via a web browser, or use openssl to verify the chain via openssl s_client -connect localhost:8501 -CAfile path/to/root-ca.pem, which should return successfully
  • Uninstall Consul and reinstall it to get certs issued directly from the Vault root CA, which shouldn't result in any extra certs being presented in the chain

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...)

brucec5 and others added 3 commits April 27, 2022 16:53
Previously, the helm chart only rendered the server leaf cert when using
the vault PKI backend. If your PKI was set up to use one or more
intermediate certs, this meant that Consul wasn't presenting the full
intermediate CA chain.

This change includes all intermediate CA certs in alongside the leaf
cert. It skips the root cert, since that's presumably already going to
be in your system's trust store.

If there's a better way to do this, I'm all ears! This is the first time
I've dealt with helm and vault templating.
This doesn't actually test that the template is doing the _right_ thing,
but at least we can verify that the rendered template makes some sense.
@hashicorp-cla
Copy link

hashicorp-cla commented Jun 1, 2022

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@jmurret jmurret left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brucec5 Thank you so much for raising this issue, giving us detailed information, and submitting this solution. I have validated through a manual script i created in this repo: https://github.com/jmurret/consul-vault-offline-ca-root.
As mentioned in that README, the browser on a mac gives some fits related to the leaf cert not being standards compliant, but the trust chain is properly displayed in the browser and it passes openssl verification. I think the mac browser issues are more related to the strictness of this development, self-signed cert, so I'm not worried about it.

Also, I copied these changes and ran them through our acceptance tests and nothing failed.

I think for the moment manual verification plus the bats unit tests are sufficient. I'll talk to the team if we want to create an acceptance for it, but since this is an area of code that won't change much and would not really be affected too much other Consul configurations or any timing conditions or anything similar that we see in Kubernetes environments, we may just leave the BATS tests as sufficient, but we'll see.

Once again, thank you so much. I'll get another team to review. Cheers. -John

@t-eckert t-eckert self-requested a review June 2, 2022 15:57
Copy link
Contributor

@t-eckert t-eckert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brucec5, this looks fantastic! Thank you for putting the work in to solve this issue. And thank you to @jmurret for the validation script.

@jmurret
Copy link
Member

jmurret commented Jun 2, 2022

@brucec5 can you please sign the CLA? It's the last hurdle to getting this merged and ready for release. 🙏

@brucec5
Copy link
Contributor Author

brucec5 commented Jun 10, 2022

Sorry for the delay on getting back to this one! I got distracted by some other stuff and kinda forgot about this PR. The CLA is signed now.

@ishustava ishustava merged commit 9968b41 into hashicorp:main Jun 10, 2022
ishustava added a commit that referenced this pull request Jun 10, 2022
ishustava added a commit that referenced this pull request Jun 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants