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

[BUG] Vault session storage does not allow unlimited use tokens #62380

Closed
bluesliverx opened this issue Jul 25, 2022 · 2 comments · Fixed by #62684
Closed

[BUG] Vault session storage does not allow unlimited use tokens #62380

bluesliverx opened this issue Jul 25, 2022 · 2 comments · Fixed by #62684
Labels
Bug broken, incorrect, or confusing behavior needs-triage Vault

Comments

@bluesliverx
Copy link
Contributor

Description
When using the Vault module default session token_backend and creating tokens with unlimited uses (0), the token is never used more than once. This occurs due to the fact that the unlimited_use_token flag can never set on the connection unless the token_backend is set to disk. I would like to use the disk backend, but cannot due to issues using Vault in extpillar modules.

Setup
Our configuration quite complex, but using a boiled down config of:

peer_run:
  .*:
  - vault.generate_token
vault:
  auth:
    method: approle
    role_id: <role-id>
    secret_id: <secret-id>
    # Default value left in to make it clear what the value is
    token_backend: session
    ttl: 1800
    uses: 0
  # We use a namespace, but this isn't required at all to exhibit the bug
  #namespace: <namespace>
  # Limiting policies also would no effect here
  #policies:
  #- policy_common
  #- policy_{grains[role]}
  url: <vault-url>

should end up triggering the bug. Anytime a Vault token is generated, it will immediately discard it and generate a new token on the next use since the unlimited_use_token flag was never set on it (see the code here that is only reached if the backend is disk).

This occurs on VMs and bare metal machines. Anything that is integrated with vault with that token storage backend stored in the session.

Steps to Reproduce the behavior
Reproducing it is easy, but knowing that it is reproduced is harder since the logging is not always in a place where you can see it. Configure the salt master with Vault as shown above and then start salt master with DEBUG logging. Run a state that retrieves any vault secret twice (different secrets are fine too) using vault.read_secret. As long as that method is called twice in the same Salt run, the behavior will show up since two tokens will be generated. The minion or the master logs will show the behavior. I verified the behavior by editing the code and writing to a file every time a token was generated just to make sure, but that shouldn't be necessary.

Expected behavior
Session storage should only create a token once during any salt call no matter the number of secrets retrieved if configured with uses: 0.

Screenshots
If applicable, add screenshots to help explain your problem.
n/a

Versions Report

salt --versions-report (Provided by running salt --versions-report. Please also mention any differences in master/minion versions.)
Salt Version:
          Salt: 3004.1

Dependency Versions:
          cffi: Not Installed
      cherrypy: Not Installed
      dateutil: Not Installed
     docker-py: Not Installed
         gitdb: Not Installed
     gitpython: Not Installed
        Jinja2: 3.1.2
       libgit2: Not Installed
      M2Crypto: 0.35.2
          Mako: Not Installed
       msgpack: 1.0.4.rc1
  msgpack-pure: Not Installed
  mysql-python: Not Installed
     pycparser: Not Installed
      pycrypto: Not Installed
  pycryptodome: 3.14.1
        pygit2: Not Installed
        Python: 3.6.8 (default, Nov 16 2020, 16:55:22)
  python-gnupg: Not Installed
        PyYAML: 6.0
         PyZMQ: 21.0.2
         smmap: Not Installed
       timelib: Not Installed
       Tornado: 4.5.3
           ZMQ: 4.3.3

System Versions:
          dist: centos 7 Core
        locale: UTF-8
       machine: x86_64
       release: 3.10.0-1160.36.2.el7.x86_64
        system: Linux
       version: CentOS Linux 7 Core

Additional context
Recent changes to the vault utils module in master do not fix this issue as well. I already have a fix that I'll be submitting shortly.

@bluesliverx bluesliverx added Bug broken, incorrect, or confusing behavior needs-triage labels Jul 25, 2022
@welcome
Copy link

welcome bot commented Jul 25, 2022

Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here’s some information that may help as you continue your Salt journey.
Please be sure to review our Code of Conduct. Also, check out some of our community resources including:

There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar.
If you have additional questions, email us at saltproject@vmware.com. We’re glad you’ve joined our community and look forward to doing awesome things with you!

lkubb added a commit to lkubb/salt-vault-formula that referenced this issue Oct 5, 2022
This commit represents a fundamental rewrite in how Salt interacts with
Vault. The master should still be compatible with minions running the
old code. There should be no breaking changes to public interfaces and
the old configuration format should still apply.

Core:
- Issue AppRoles to minions
- Manage entities with templatable metadata for minions
- Use inbuilt Salt cache
- Separate config cache from token cache
- Cache: introduce connection-scope vs global scope

Utility module:
- Support being imported (__utils__ deprecation)
- Raise exceptions on queries to simplify response handling
- Add classes to wrap complexity, especially regarding KV v2
- Lay some groundwork for renewing tokens

Execution module:
- Add patch_secret
- Add version support to delete_secret
- Allow returning listed keys only in list_secret
- Add policy_[fetch/write/delete] and policies_list
- Add query for arbitrary API queries

State module:
- Make use of execution module
- Change output format

Docs:
- Update for new configuration format
- Correct examples
- Add configuration examples
- Add required policies

Fixes:
saltstack/salt#62552
saltstack/salt#59827
saltstack/salt#62380
saltstack/salt#58174

Probably fixes:
saltstack/salt#60779
saltstack/salt#57561

Might fix:
saltstack/salt#59846
@bluesliverx
Copy link
Contributor Author

#62684 will fix this hopefully, but even more, I think it might have already been addressed by commits in master around the vault functionality. Just an FYI. I'll leave this open until I can confirm either one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior needs-triage Vault
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants