Allow verifying recovery keys #1095
Conversation
briantist
added
enhancement
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.
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.
| 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.
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.
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 |
briantist
added
minor
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