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.

Sign up for GitHub

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

Jump to bottom

Allow verifying recovery keys #1095

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Allow verifying recovery keys #1095

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 19 additions & 4 deletions hvac/api/system_backend/key.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
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.

Copy link
Author

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.

Copy link
Contributor

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.

"""Cancel any in-progress rekey verification.
This clears any progress made and resets the nonce. Unlike cancel_rekey, this only resets
the current verification operation, not the entire rekey atttempt.
Expand All @@ -329,15 +329,19 @@ def cancel_rekey_verify(self):
Supported methods:
DELETE: /sys/rekey/verify. Produces: 204 (empty body)

:param recovery_key: If true, send requests to "rekey-recovery-key" instead of "rekey" api path.
:type recovery_key: bool
:return: The response of the request.
:rtype: requests.Response
"""
api_path = "/v1/sys/rekey/verify"
if recovery_key:
api_path = "/v1/sys/rekey-recovery-key/verify"
return self._adapter.delete(
url=api_path,
)

def rekey_verify(self, key, nonce):
def rekey_verify(self, key, nonce, recovery_key=False):
"""Enter a single new recovery key share to progress the rekey verification of the Vault.
If the threshold number of new recovery key shares is reached, Vault will complete the
rekey. Otherwise, this API must be called multiple times until that threshold is met.
Expand All @@ -346,6 +350,8 @@ def rekey_verify(self, key, nonce):
Supported methods:
PUT: /sys/rekey/verify. Produces: 200 application/json

:param recovery_key: If true, send requests to "rekey-recovery-key" instead of "rekey" api path.
:type recovery_key: bool
:param key: Specifies multiple recovery share keys.
:type key: str | unicode
:param nonce: Specifies the nonce of the rekey verify operation.
Expand All @@ -359,12 +365,14 @@ def rekey_verify(self, key, nonce):
}

api_path = "/v1/sys/rekey/verify"
if recovery_key:
api_path = "/v1/sys/rekey-recovery-key/verify"
return self._adapter.put(
url=api_path,
json=params,
)

def rekey_verify_multi(self, keys, nonce):
def rekey_verify_multi(self, keys, nonce, recovery_key=False):
"""Enter multiple new recovery key shares to progress the rekey verification of the Vault.
If the threshold number of new recovery key shares is reached, Vault will complete the
rekey. Otherwise, this API must be called multiple times until that threshold is met.
Expand All @@ -373,6 +381,8 @@ def rekey_verify_multi(self, keys, nonce):
Supported methods:
PUT: /sys/rekey/verify. Produces: 200 application/json

:param recovery_key: If true, send requests to "rekey-recovery-key" instead of "rekey" api path.
:type recovery_key: bool
:param keys: Specifies multiple recovery share keys.
:type keys: list
:param nonce: Specifies the nonce of the rekey verify operation.
Expand All @@ -386,22 +396,27 @@ def rekey_verify_multi(self, keys, nonce):
result = self.rekey_verify(
key=key,
nonce=nonce,
recovery_key=recovery_key,
)
if result.get("complete"):
break

return result

def read_rekey_verify_progress(self):
def read_rekey_verify_progress(self, recovery_key=False):
"""Read the configuration and progress of the current rekey verify attempt.

Supported methods:
GET: /sys/rekey/verify. Produces: 200 application/json

:param recovery_key: If true, send requests to "rekey-recovery-key" instead of "rekey" api path.
:type recovery_key: bool
:return: The JSON response of the request.
:rtype: requests.Response
"""
api_path = "/v1/sys/rekey/verify"
if recovery_key:
api_path = "/v1/sys/rekey-recovery-key/verify"
return self._adapter.get(
url=api_path,
)