-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Update Vault CA provider to apply full cert chain #11451
Conversation
Thanks for putting this out @krarey -- it overall LGTM! Would you mind adding some specific tests that tests for a cert chain presence? Also, to err on the side of over caution, do we think such a change could break any practitioner workflows? As in, say folks are looking for just one cert and now receive more than one? |
Thanks @FFMMM!
I think I need more detail on what you have in mind. Are you thinking a test that actually parses the returned certificate chain and makes sure each issuer cert is provided in the expected order? If so, that may make more sense at a higher level, when the respective Sign() call is made against any of the Built-in/Vault/ACM engines. If something else, let me know, I'm happy to flesh out the validation logic a bit more.
I don't think this should have any impact – the existing code was already attaching the issuer CA, and in most cases (root + consul-managed intermediate), this patch will result in an identical chain. In this case we are just ensuring any additional subordinate CAs are also added, should they be available and properly configured on the issuing Vault intermediate (if the subordinate CAs haven't been attached to the intermediate ca_chain in Vault, the result would again be the same as before, |
I noticed CI has not run ( |
1db6549
to
885112b
Compare
Currently held while we work through changes to the secondary DC intermediate signing flow. |
This pull request has been automatically flagged for inactivity because it has not been acted upon in the last 60 days. It will be closed if no new activity occurs in the next 30 days. Please feel free to re-open to resurrect the change if you feel this has happened by mistake. Thank you for your contributions. |
This ended up requiring a few bug fixes, and a few more changes to the vault provider. Existing releases of vault have this bug hashicorp/vault#13489, which meant using Thanks for getting us started and pointed in the right direction @krarey! I'm going to close this PR since it is replaced by #11910. |
Modifies the Vault CA provider for Consul Connect to apply the complete CA chain to generated certificates. More information in issue #11448.