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.

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

Execute ScriptVaultSecret on demand instead of on load #82418

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

Conversation

Urth
Copy link
Contributor

@Urth Urth commented Dec 13, 2023

SUMMARY

A vault secret script can do whatever is needed to return a vault password. This can mean loading something from a local password store or accessing a remote password storage solution. When combining the password script with multiple vault_identity_list entries this adds a lot of unneeded loading of vault indentity passwords. Most of which may not be required to complete the ansible run.

This commit delays executing for Script and ClientScriptVaultSecret classes until the bytes password is accessed.

We currently have a vault_identity_list with 35 different entries. Each entry requires authorization and loading them takes a while making the startup time for ansible significant. Executing the password scripts on demand would solve 2 problems:

  • users don't have to change their configuration to exclude entries they cannot access.
  • the time to load unused passwords is removed from the execution time.
ISSUE TYPE
  • Feature Pull Request
ADDITIONAL INFORMATION
$ grep vault_id_match ansible.cfg 
vault_id_match = true
# Before
$ time ansible-vault decrypt < tmp.yaml
Decryption successful
...
real    0m13,144s
user    0m3,159s
sys     0m0,729s
# After
$ time ansible-vault decrypt < tmp.yaml
Decryption successful
...
real    0m0,494s
user    0m0,229s
sys     0m0,016s

@ansibot ansibot added feature This issue/PR relates to a feature request. needs_triage Needs a first human triage before being processed. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Dec 13, 2023
@Urth Urth force-pushed the vault-lazy-loaded-password-script branch from c960e54 to c9dc0d3 Compare December 13, 2023 15:18
A vault secret script can do whatever is needed to return a vault
password. This can mean loading something from a local password store
or accessing a remote password storage solution. When combining the
password script with multiple vault_identity_list entries this adds a
lot of unneeded loading of vault indentity passwords. Most of which may
not be required to complete the ansible run.

This commit delays executing for Script and ClientScriptVaultSecret
classes until the bytes password is accessed.
@Urth Urth force-pushed the vault-lazy-loaded-password-script branch from c9dc0d3 to 4ef61dc Compare December 13, 2023 15:46
@ansibot ansibot removed the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. label Dec 13, 2023
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Jan 2, 2024
@nitzmahone nitzmahone self-requested a review January 4, 2024 19:14
@nitzmahone nitzmahone removed the needs_triage Needs a first human triage before being processed. label Jan 4, 2024
@mstud
Copy link

mstud commented Feb 19, 2024

This is relevant for us, too. When can we expect this to be merged?

@nitzmahone
Copy link
Member

This could have some unintended side-effects that work against the desired behavior.

The gnarliest one off the top of my head: deferring the script execution until first secret access optimizes for "nothing runs", but when the first encounter with an encrypted file/value occurs post-fork (eg, an include_vars or any other DataLoader activity that occurs in a host-specific context), without a bunch of locking and extra bookkeeping to marshal the vault secret state back to the controller from workers, the secret scripts will be run repeatedly on every fork.

This also likely clashes with some upcoming changes around how undecryptable vaulted values are tracked, where we need to know if a value is decryptable before the first fork occurs (for the same reasons as above). That change would basically defeat this one anyway, at least the first time a vaulted value is encountered pre-fork.

It seems like you have some use cases for vault access that aren't handled well by its original design assumptions. Are you using multiple vault_id values concurrently, where some users have > 1 vault secrets in play, or do you maintain multiple encrypted copies of the data in question (using different secrets) and have your secret script map a run to 0-or-1 vault secrets and only load the content that you know will work for that secret? If it's the "multiple" case, would it be helpful if a single vault script could supply N vault-id/secret pairs instead of just one? That way, you could apply whatever error-handling/fallback logic you like (including parallelization, if necessary), and with a full knowledge of what else had already occurred.

@MJJoker
Copy link

MJJoker commented Feb 20, 2024

Relevant for me too.
I have one vault-script that can access keepassxc, the vault-id is used to get he correct secret.
The same script (and keepassxc-DB) is used for a lot of plays, I do not know which secrets to return without the proper ids. So I would need multiple vault-ids.

As we are talking about secrets, I do not like the idea of returning "all the secrets" (or just multiple) or having them in memory just-in-case they are needed.

That sounds like a big change, but ansible could handle all the secrets/credentials in a separate instance/process and accessing them is communicating with this separate instance anyway.

@Urth
Copy link
Contributor Author

Urth commented Feb 20, 2024

Are you using multiple vault_id values concurrently, where some users have > 1 vault secrets in play. ... If it's the "multiple" case, would it be helpful if a single vault script could supply N vault-id/secret pairs instead of just one? That way, you could apply whatever error-handling/fallback logic you like (including parallelization, if necessary), and with a full knowledge of what else had already occurred.

Yes, we have multiple vault id's and they all map to the same script. Giving the script access to N wouldn't help much since we don't know which will be used and have to check all of them. What would help is if ansible could preprocess the secrets and collect the vault-id's that will be used and only request those.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature This issue/PR relates to a feature request. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants