-
Notifications
You must be signed in to change notification settings - Fork 373
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
Allow verifying recovery keys #1095
base: main
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1095 +/- ##
==========================================
- Coverage 87.09% 87.02% -0.08%
==========================================
Files 64 64
Lines 3146 3152 +6
==========================================
+ Hits 2740 2743 +3
- Misses 406 409 +3
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @wilsonge welcome! Thanks for submitting this. I haven't looked too closely yet and it will take some time before I'm able to. In the meantime, I would also like to see tests and documentation/examples for the change included in the PR.
Hopefully there's existing tests and documentation you could extend (I haven't looked yet); if not I understand that adding it from scratch could be a burden.
@@ -320,7 +320,7 @@ def read_backup_keys(self, recovery_key=False): | |||
url=api_path, | |||
) | |||
|
|||
def cancel_rekey_verify(self): | |||
def cancel_rekey_verify(self, recovery_key=False): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def cancel_rekey_verify(self, recovery_key=False): | |
def cancel_rekey_verify(self, *, recovery_key=False): |
Let's make all of these keyword-only. The ability to pass a naked boolean value could be unclear, and future-looking, I'd like to try to avoid adding new positional parameters where possible, because it makes adding new parameters or changing ones in the future more strict since re-ordering is a breaking change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you willing to trade off the consistency of how we're implementing this in the other methods further up which already have the same parameter. If you're happy for the inconsistency happy to roll it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing that out, I didn't see the other methods. I guess we'll leave them positional for consistency.
If I had a comprehensive plan/guidance written out about this I might've gone the other way, but I haven't been able to write that yet.
There aren't :) I did check before I submitted. None of the recovery key based methods are documented or tested sadly. |
It looks like we have some docs here: https://github.com/hvac/hvac/blob/main/docs/usage/system_backend/key.rst (published: https://hvac.readthedocs.io/en/stable/usage/system_backend/key.html) And integration tests here: https://github.com/hvac/hvac/blob/main/tests/integration_tests/api/system_backend/test_key.py |
Currently the recovery mechanism only works for rekey's but not for recovery keys. This uses the same parameter we use elsewhere in the file to facilitate that
Documentation for the new path being used here: https://developer.hashicorp.com/vault/api-docs/system/rekey-recovery-key#submit-verification-key
Resolves #1097