VAULT-18307 Vault incorrectly requeues credentials when the rotation period is updated #23528
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
github-actions
bot
added
the
hashicorp-contributed-pr
kpcraig
changed the title
|
Build Results: |
|
CI Results: |
schavis
requested changes
Oct 9, 2023
schavis
added
the
content-lgtm
|
Content LGTM. Feel free to merge once ENG signs off on the other changes |
schavis
approved these changes
Oct 9, 2023
is this worth it? who can say
fairclothjm
approved these changes
Oct 11, 2023
fairclothjm
reviewed
Oct 11, 2023
marcboudreau
pushed a commit
that referenced
this pull request
Oct 11, 2023
…guration (#23547) * CI: Pre-emptively delete logs dir after cache restore in test-collect-reports (#23600) * Fix OktaNumberChallenge (#23565) * remove arg * changelog * exclude changelog in verifying doc/ui PRs (#23601) * Audit: eventlogger sink node reopen on SIGHUP (#23598) * ensure nodes are asked to reload audit files on SIGHUP * added changelog * Capture errors emitted from all nodes during proccessing of audit pipelines (#23582) * Update security-scan.yml * Listeners: Redaction only for TCP (#23592) * redaction should only work for TCP listeners, also fix bug that allowed custom response headers for unix listeners * fix failing test * updates from PR feedback * fix panic when unlocking unlocked user (#23611) * VAULT-18307: update rotation period for aws static roles on update (#23528) * add disable_replication_status_endpoints tcp listener config parameter * add wrapping handler for disabled replication status endpoints setting * adapt disable_replication_status_endpoints configuration parsing code to refactored parsing code * refactor configuration parsing code to facilitate testing * fix a panic when parsing configuration * update refactored configuration parsing code * fix merge corruption * add changelog file * document new TCP listener configuration parameter * make sure disable_replication_status_endpoints only has effect on TCP listeners * use active voice for explanation of disable_replication_status_endpoints * fix minor merge issue --------- Co-authored-by: Kuba Wieczorek <kuba.wieczorek@hashicorp.com> Co-authored-by: Angel Garbarino <Monkeychip@users.noreply.github.com> Co-authored-by: Hamid Ghaf <83242695+hghaf099@users.noreply.github.com> Co-authored-by: Peter Wilson <peter.wilson@hashicorp.com> Co-authored-by: Mark Collao <106274486+mcollao-hc@users.noreply.github.com> Co-authored-by: davidadeleon <56207066+davidadeleon@users.noreply.github.com> Co-authored-by: kpcraig <3031348+kpcraig@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR implements a hopefully simple fix where Vault wouldn't reprioritize credentials when the rotation period was changed. Tests are incoming, but I wanted to get some thoughts on a question below:
The question that invites some bikeshedding is how to update the 'next rotation time' period -
Right now, I am doing the simplest case - take the current time, add the rotation period. This is easy to understand, but if, say someone updates a 30 day rotation to a 45 day rotation, could result in a rotation period (just this once) that is much longer than expected.
Another way would be to update the next rotation time to be "as though" it was with the new rotation period, e.g., if you update a 30d period to 45d, we would add only 15 days. If you lowered the rotation period to 20d, we would subtract 10 days. This could result in a priority in the past, which might be unexpected, but would be programmatically fine - the credential would simply rotate the next time the queue is checked.
There are also hybrid approaches possible, of course, trading off complexity with trying to do "what people would want".