Skip to content

Add multi-use cache to vault module #57066

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

Merged
merged 18 commits into from
May 8, 2020
Merged

Conversation

mchugh19
Copy link
Contributor

@mchugh19 mchugh19 commented May 4, 2020

What does this PR do?

Rewrite of @thenoid's vault cache attempt in #54094 + pretty good tests!

Allows the generation of longer lived vault tokens, and caches this information on the minion. Also caches the vault secret backend version to save a network trip on each secret lookup.

What issues does this PR fix or reference?

Closes: #54094

Previous Behavior

Salt's vault handling would generate a single use token on behalf of the minion.

New Behavior

The single use token is still the default, but configuration options allow generating longer lived tokens(by uses or ttl).

If master config includes:

vault:
  url: http://127.0.0.1:8200
  auth:
    method: token
    token: <some token>
    uses: 10
    ttl: 300
  policies:
    - salt-policy

A token will be generated, which has an Explicit Max TTL of 5 minutes (not valid after this time, and cannot be renewed) and allows for 10 uses, whichever occurs first. This token will then be written to the minon's cache. When a request is made to connect to vault like in vault.read_secret the cache file will be read and evaluated to see if it still has a valid ttl, if so it will be used to contact vault. If the token is invalid, the cache will be cleared, and a new token generated for the minion.

The cache file is intended to be written with 600 permissions, so leaking of these credentials should only be done to the root user, who already has the ability to run salt-call vault.read_secret.

# ls -l /var/cache/salt/minion/salt_vault_token
-rw------- 1 root root 560 May  4 09:33 /var/cache/salt/minion/salt_vault_token

This behavior was tested with multiple umasks, but only on linux. But as this could be a security concern, please doublecheck the with salt.utils.files.fpopen(cache_file, "w", mode=0o600) as fp_: logic

This PR also updates the previous Vault secret KV v2 support to save the results of the metadata lookups into the same cache file. These lookups are done when accessing a secret to determine if the classic secret API should be used, or the newer KV v2 one. So when using a multi-use token on a minion that has accessed the secret before and has cached metadata, the only http request issued from the minion should be a single authenticated lookup for the secret data. Previously, this would have been 3: for token generation, metadata lookup, and secret request.

The uses, and ttl are global settings on the master and would apply to all minions. For environments which would like to allow some minions to have longer lived tokens, while others should be single-use, this information can be delegated to the minion config.

Master:

vault:
  url: http://127.0.0.1:8200
  auth:
    method: token
    token: <sometoken>
    allow_minion_override: True
  policies:
    - salt-policy

Minion:

vault:
  auth:
   uses: 15
   ttl: 86400

Should anyone desire to clear the minion's token/metadata cache file, this PR also added a vault.clear_token_cache function to the vault execution module.

Supported scenarios:

  • Current single use token (default ttl 2764800 (32 days))
  • multi-use token: uses: int
  • unlimited-use token: uses: 0
  • token regeneration when ttl expired token found
  • clearing minion cache

Merge requirements satisfied?

  • Docs
  • Changelog
  • Tests written/updated

Commits signed with GPG?

No

@mchugh19 mchugh19 requested a review from a team as a code owner May 4, 2020 10:33
@ghost ghost requested review from cmcmarrow and removed request for a team May 4, 2020 10:33
@thenoid
Copy link

thenoid commented May 4, 2020

Haha, thanks for updating another one of my PRs! I should send you a beer.

I'm not sure I'm a huge fan of the metadata cach'ing since that information could drift over some amount of time. maybe provide a way to disable it? and also provide a cache invalidation TTL? or maybe simply if the token has expired clear the cache?

@mchugh19
Copy link
Contributor Author

mchugh19 commented May 4, 2020

:) The vault binary itself saves the vault login token on disk in a dot file, so based on that I figured the ability to do the same and save it into a file wasn't any worse. The cache file itself is written as the same user as the minion (with no other permissions), and assuming the filesystem doesn't leak credentials I figured the the risk was fine, and grants no additional access worse than running salt-call vault.read_secret. The minion's public key is kept on disk with similar permissions, so I'm not sure there's a better way to add controls here.
The default behavior is still to use single use tokens, and these are never saved to disk.

As for caching the metadata, there could be a maximum ttl on this data, but it's only used for determining if the secret path is secret kv v2 or v1. So the only time that is likely to change is when the vault server is upgraded. If desired a salt scheduled task could be run to run the flush_token_cache function, or there could just be hardcoded ttl to flush the cache every 7 days or something. But at that point, I'm not sure if there's anything to gain, as you wouldn't want broken lookups for up to 7 days, and would need to run the flush_token_cache function manually anyway. If the multi-use token is in use, when the token expires, so does the metadata cache. So if you used a 1-24 hour token ttl, you don't run much risk of metadata staleness.

I'm not at all opposed to adding an option to store info for the current session only and not touch disk. But we do need to be careful not to generate too many configurables. Figured the best way to have the chat was over a PR. At this point I'm all ears!

@mchugh19
Copy link
Contributor Author

mchugh19 commented May 4, 2020

Just uploaded a commit which supports toggling between __context__ or disk storage (context was already used for single use tokens, so it wasn't too rough to use that path everywhere)

The default is to use __context__ and the file on disk can be specified with token_backend: disk in the master config

vault:
  url: http://127.0.0.1:8200
  auth:
    method: token
    token: <some token>
    uses: 20
    token_backend: disk

@thenoid
Copy link

thenoid commented May 4, 2020

Just uploaded a commit which supports toggling between __context__ or disk storage (context was already used for single use tokens, so it wasn't too rough to use that path everywhere)

The default is to use __context__ and the file on disk can be specified with token_backend: disk in the master config

vault:
  url: http://127.0.0.1:8200
  auth:
    method: token
    token: <some token>
    uses: 20
    token_backend: disk

Love the solution here! Thanks.

Copy link
Contributor

@cmcmarrow cmcmarrow left a comment

Choose a reason for hiding this comment

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

@mchugh19 as far as I can tell this does not break backwards compatible. Which is great. I just want to confirm with you that backwards compatible is good. Thanks for adding test!

@mchugh19
Copy link
Contributor Author

mchugh19 commented May 6, 2020

@cmcmarrow Yep! Everything should be backwards compatible. If no config options are changed, everything still defaults to single use vault tokens, and no files touch disk. The only update to this path is caching the vault metadata lookups in __context__.

All new functionality needs to be enabled by adding the uses (optional ttl) or token_backend configs.

Copy link
Contributor

@cmcmarrow cmcmarrow left a comment

Choose a reason for hiding this comment

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

@mchugh19 thanks for confirming!

@dwoz dwoz merged commit d4bb81d into saltstack:master May 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ZRelease-Sodium retired label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants