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

Make sure vault.unseal runner is unauthenticated #86

Merged
merged 2 commits into from
Oct 2, 2024
Merged

Make sure vault.unseal runner is unauthenticated #86

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8cf21c8
fixing #85
voyvodov Sep 4, 2024
29532b4
fixup! fixing #85
voyvodov Sep 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog/85.fixed.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Change unseal query to be always unauthenticated.
4 changes: 3 additions & 1 deletion src/saltext/vault/runners/vault.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -548,7 +548,9 @@ def unseal():
salt-run vault.unseal
"""
for key in __opts__["vault"]["keys"]:
ret = vault.query("POST", "sys/unseal", __opts__, __context__, payload={"key": key})
ret = vault.query(
"POST", "sys/unseal", __opts__, __context__, is_unauthd=True, payload={"key": key}
)
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for submitting bug + fix and sorry for taking a while!

Have you verified this actually fixes the behavior? Afair is_unauthd only influences whether a token use is deducted (internal statistics).

I think in the code's current state you might need to create the unauthenticated client yourself (since it's a runner function we can just rely on the opts directly):

from saltext.vault.utils import factory
from saltext.vault.utils.client import VaultClient
# ...

def unseal():
    config = factory.parse_config(__opts__.get("vault", {}))
    client = VaultClient(**config["server"], **config["client"])
    for key in __opts__["vault"]["keys"]:
        ret = client.post("sys/unseal", payload={"key": key})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right :( .
I was expecting that unauth is really unauth. Probably that will be good to be fixed.

Using your solution for now (will prepare another one where unauth is really unauth)

Copy link
Member

Choose a reason for hiding this comment

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

I don't think making vault.query with is_unauthd=True do an unauthenticated request is the correct layer to fix this behavior. The parameter is passed through to the authenticated client and is an ugly workaround intended to avoid unnecessary token reissuance when calling arbitrary endpoints - afair the token is still optionally used for auditing purposes.*

I would prefer introducing a separate vault.query_noauth helper function essentially doing the same as above (for locally sourced configuration).

* A more theoretical concern that's specific to the sealed state is that it would only work as expected when sourcing the parameters from local configuration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wait...what? I was thinking that I pushed the change (using the client directly).
Will do it in a few minutes 🤦🏻‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. Now it uses directly the client (so no tokens, authentication are used). Pretty much raw call (as it should be)

if ret["sealed"] is False:
return True
return False
Expand Down
Loading