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

enable auto-tidy expired issuers in vault (as CA) #17138

Merged
merged 12 commits into from
May 3, 2023

Conversation

eikenb
Copy link
Contributor

@eikenb eikenb commented Apr 25, 2023

When using vault as a CA and generating the local signing cert, try to enable the pki endpoint's auto-tidy feature with it set to tidy expired issuers.

@eikenb eikenb added type/enhancement Proposed improvement or new feature theme/connect Anything related to Consul Connect, Service Mesh, Side Car Proxies theme/consul-vault Relating to Consul & Vault interactions theme/certificates Related to creating, distributing, and rotating certificates in Consul pr/no-docs PR does not include docs and should not trigger reminder for cherrypicking them. pr/no-backport labels Apr 25, 2023
@eikenb eikenb force-pushed the eikenb/auto-tidy-expired-issuers branch from a372770 to 33ed9e8 Compare April 25, 2023 22:20
@eikenb eikenb marked this pull request as ready for review April 27, 2023 19:51
Copy link
Contributor

@jkirschner-hashicorp jkirschner-hashicorp left a comment

Choose a reason for hiding this comment

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

@eikenb : Left some thoughts on log message copy and docs updates. Thanks for the finishing touches here! I also like that we now have a pattern to reference for testing against different Vault versions (when we expect the behavior to differ).

@jkirschner-hashicorp
Copy link
Contributor

@eikenb : I'm good with this PR, but I can't approve on behalf of eng. Can you request a review from a ZTS eng team member?

@jkirschner-hashicorp jkirschner-hashicorp requested review from a team and cthain and removed request for a team April 28, 2023 21:55
Copy link
Contributor

@jkirschner-hashicorp jkirschner-hashicorp left a comment

Choose a reason for hiding this comment

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

Submitting to clear my previous "request changes" review. I'm happy with this, but someone from eng needs to give an eng review.

@jkirschner-hashicorp jkirschner-hashicorp dismissed their stale review April 28, 2023 21:56

My comments have been satisfied, but I'm not an eng reviewer. Waiting on eng reviewer.

Copy link
Contributor

@kisunji kisunji left a comment

Choose a reason for hiding this comment

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

Correct me if I'm missing some context behind the decision, but I think this should be called once during setupIntermediatePKIPath, since it's a one-time operation.

As-is, we're adding a network call and possibly unactionable INFO log to a hotpath (leaf cert generation) which might get noisy if there are many service registrations.

@eikenb
Copy link
Contributor Author

eikenb commented May 1, 2023

Correct me if I'm missing some context behind the decision, but I think this should be called once during setupIntermediatePKIPath, since it's a one-time operation.

As-is, we're adding a network call and possibly unactionable INFO log to a hotpath (leaf cert generation) which might get noisy if there are many service registrations.

AFAIK setupIntermediatePKIPath only gets called once when the PKI path is created/mounted. This doesn't handle cases of the PKI path already being created and the auto-tidy needing to be set (E.g. by version upgrades or permissions settings). Jared and I discussed this and wanted to be sure it was being set.

We could change those log messages from INFO to DEBUG as that'd raise it above the default settings.

@kisunji
Copy link
Contributor

kisunji commented May 1, 2023

Correct me if I'm missing some context behind the decision, but I think this should be called once during setupIntermediatePKIPath, since it's a one-time operation.
As-is, we're adding a network call and possibly unactionable INFO log to a hotpath (leaf cert generation) which might get noisy if there are many service registrations.

AFAIK setupIntermediatePKIPath only gets called once when the PKI path is created/mounted. This doesn't handle cases of the PKI path already being created and the auto-tidy needing to be set (E.g. by version upgrades or permissions settings). Jared and I discussed this and wanted to be sure it was being set.

We could change those log messages from INFO to DEBUG as that'd raise it above the default settings.

I think setupIntermediatePKIPath gets called on Configure which happens on every leader election so auto-tidy should eventually get set without user intervention.

@eikenb
Copy link
Contributor Author

eikenb commented May 2, 2023

@kisunji .. I'll take a look at those code paths tomorrow and move the call if it looks like that will work. Thanks!

@eikenb
Copy link
Contributor Author

eikenb commented May 2, 2023

@kisunji .. I verified your idea and am going to update my PR for that. I thought I had checked that path but I must have got off track as some point. Thanks again.

@eikenb
Copy link
Contributor Author

eikenb commented May 2, 2023

@kisunji .. code and test updated. Ready for another look.

eikenb and others added 11 commits May 2, 2023 13:32
When using vault as a CA and generating the local signing cert, try to
enable the pki endpoint's auto-tidy feature with it set to tidy expired
issuers.
Co-authored-by: Jared Kirschner <85913323+jkirschner-hashicorp@users.noreply.github.com>
Co-authored-by: Jared Kirschner <85913323+jkirschner-hashicorp@users.noreply.github.com>
Co-authored-by: Jared Kirschner <85913323+jkirschner-hashicorp@users.noreply.github.com>
Co-authored-by: Jared Kirschner <85913323+jkirschner-hashicorp@users.noreply.github.com>
@eikenb eikenb force-pushed the eikenb/auto-tidy-expired-issuers branch from 3b94649 to ff5a593 Compare May 2, 2023 20:32
Copy link
Contributor

@kisunji kisunji left a comment

Choose a reason for hiding this comment

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

small comment on the return vals but will preapprove!

agent/connect/ca/provider_vault.go Outdated Show resolved Hide resolved
@eikenb eikenb force-pushed the eikenb/auto-tidy-expired-issuers branch from ad0a150 to f32687d Compare May 3, 2023 18:41
@eikenb eikenb merged commit bd76fde into main May 3, 2023
@eikenb eikenb deleted the eikenb/auto-tidy-expired-issuers branch May 3, 2023 20:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/no-backport pr/no-docs PR does not include docs and should not trigger reminder for cherrypicking them. theme/certificates Related to creating, distributing, and rotating certificates in Consul theme/connect Anything related to Consul Connect, Service Mesh, Side Car Proxies theme/consul-vault Relating to Consul & Vault interactions type/enhancement Proposed improvement or new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants