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] sdb.get_or_set_hash does not work with Vault #60779

Closed
baturaysoysal opened this issue Aug 23, 2021 · 5 comments · Fixed by #62684
Closed

[BUG] sdb.get_or_set_hash does not work with Vault #60779

baturaysoysal opened this issue Aug 23, 2021 · 5 comments · Fixed by #62684
Assignees
Labels
Bug broken, incorrect, or confusing behavior severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around Vault
Milestone

Comments

@baturaysoysal
Copy link

Description
I tried using sdb.get_or_set_hash function with Vault as SDB backend. While sdb.get and sdb.set works flawlessly, I could not get sdb.get_or_set_hash working, when creating a new secret.

Here are the scenarios I tested:

  • salt 'myminion' sdb.set 'sdb://myvault/salt/test/password' 1234
    • WORKS 100%
  • salt 'myminion' sdb.get 'sdb://myvault/salt/test/password'
    • WORKS 100%
  • salt 'myminion' sdb.get_or_set_hash 'sdb://myvault/salt/test/password' 10
    • Works if password key was set in test secret before. Returns the value for password
    • Works if password key was set in test secret, and I am setting a new key named password1. Generates a new 10 digit string and replaces the {"password": 1234} with {"password1": "some_random_string"}
    • Does not work if test secret was not created previously (under kv secret engine named salt). Please note that the same thing (non-existing secret creation) works perfectly with sdb.set, which this function calls internally.

The error I see when using sdb.get_or_set_hash, creating a new secret is as follows:

myminion:
    ERROR: 403 Client Error: Forbidden for url: https://<VAULT_ADDRESS>:8200/v1/salt/test

I added some print statements inside sdb.py and vault.py to check if some argument is different between calling sdb.set and sdb.get_or_set_hash, and I could not see any difference.

I tested with both v1 and v2 KV secrets. I also tried the old and new notation for separating the key ('?' vs '/').

Setup
Our set up is an on-prem VM infrastructure on VMware.

Steps to Reproduce the behavior

# Check if "test" exists
# salt 'myminion' sdb.get 'sdb://myvault/salt/test/password'
myminion:
    None

# Try setting "test" using "sdb.get_or_set_hash"
# salt 'myminion' sdb.get_or_set_hash 'sdb://myvault/salt/test/password' 10
myminion:
    ERROR: 403 Client Error: Forbidden for url: https://<VAULT_ADDRESS>:8200/v1/salt/test
ERROR: Minions returned with non-zero exit code

# Verify it was not set
# salt 'myminion' sdb.get 'sdb://myvault/salt/test/password'
myminion:
    None

# Try setting "test" using "sdb.set"
# salt 'myminion' sdb.set 'sdb://myvault/salt/test/password' 1234
myminion:
    True

# Verify it was set using "sdb.get_or_set_hash"
# salt 'myminion' sdb.get_or_set_hash 'sdb://myvault/salt/test/password' 10
myminion:
    1234

# Try setting a different key in secret "test" using "sdb.get_or_set_hash"
# salt 'myminion' sdb.get_or_set_hash 'sdb://myvault/salt/test/password1' 10
myminion:
    bm^8t%-shs

# Verify it was set
# salt 'myminion' sdb.get_or_set_hash 'sdb://myvault/salt/test/password1' 10
myminion:
    bm^8t%-shs
    
# salt 'myminion' sdb.get 'sdb://myvault/salt/test/password1'
myminion:
    bm^8t%-shs

Expected behavior
sdb.get_or_set_hash can create a secret as sdb.set does.

Versions Report

(Master) salt --versions-report
Salt Version:
          Salt: 3002.6

Dependency Versions:
          cffi: 1.14.5
      cherrypy: Not Installed
      dateutil: Not Installed
     docker-py: Not Installed
         gitdb: Not Installed
     gitpython: Not Installed
        Jinja2: 2.11.1
       libgit2: 1.1.0
      M2Crypto: 0.35.2
          Mako: Not Installed
       msgpack: 0.6.2
  msgpack-pure: Not Installed
  mysql-python: Not Installed
     pycparser: 2.20
      pycrypto: Not Installed
  pycryptodome: Not Installed
        pygit2: 1.5.0
        Python: 3.6.8 (default, Nov 16 2020, 16:55:22)
  python-gnupg: Not Installed
        PyYAML: 3.13
         PyZMQ: 17.0.0
         smmap: Not Installed
       timelib: Not Installed
       Tornado: 4.5.3
           ZMQ: 4.1.4

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

(Minion) salt-minion --versions-report
Salt Version:
          Salt: 3003

Dependency Versions:
          cffi: Not Installed
      cherrypy: Not Installed
      dateutil: Not Installed
     docker-py: Not Installed
         gitdb: Not Installed
     gitpython: Not Installed
        Jinja2: 2.11.1
       libgit2: Not Installed
      M2Crypto: 0.35.2
          Mako: Not Installed
       msgpack: 0.6.2
  msgpack-pure: Not Installed
  mysql-python: Not Installed
     pycparser: Not Installed
      pycrypto: Not Installed
  pycryptodome: Not Installed
        pygit2: Not Installed
        Python: 3.6.8 (default, Nov 16 2020, 16:55:22)
  python-gnupg: Not Installed
        PyYAML: 3.13
         PyZMQ: 17.0.0
         smmap: Not Installed
       timelib: Not Installed
       Tornado: 4.5.3
           ZMQ: 4.1.4

System Versions:
          dist: centos 7 Core
        locale: UTF-8
       machine: x86_64
       release: 3.10.0-1160.21.1.el7.x86_64
        system: Linux
       version: CentOS Linux 7 Core
@baturaysoysal baturaysoysal added Bug broken, incorrect, or confusing behavior needs-triage labels Aug 23, 2021
@OrangeDog
Copy link
Contributor

OrangeDog commented Aug 23, 2021

I assume it's because it's doing a get on a non-existent secret.
Is there a setting or permission in Vault to make it return 404 in that case instead?

Either way, the code should probably handle it.

@OrangeDog OrangeDog added severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around Vault and removed needs-triage labels Aug 23, 2021
@OrangeDog OrangeDog added this to the Approved milestone Aug 23, 2021
@baturaysoysal
Copy link
Author

I think the issue is more likely to be on the set portion of sdb.get_or_set_hash.

Running these commands on the minion with salt-call shows more verbose output, plus I added some print statements to the code. As you can see, the get part succeeds, a random hash is generated, but it cannot POST the new value.

Only get (I don't think the ERROR line is important, it is handled and result becomes None, which is correct):

# salt-call sdb.get 'sdb://myvault/salt/test1?password'
[WARNING ] /usr/lib/python3.6/site-packages/requests/packages/urllib3/connectionpool.py:1004: InsecureRequestWarning: Unverified HTTPS request is being made. Adding certificate verification is strongly advised. See: https://urllib3.readthedocs.io/en/latest/advanced-usage.html#ssl-warnings
  InsecureRequestWarning,

[WARNING ] /usr/lib/python3.6/site-packages/requests/packages/urllib3/connectionpool.py:1004: InsecureRequestWarning: Unverified HTTPS request is being made. Adding certificate verification is strongly advised. See: https://urllib3.readthedocs.io/en/latest/advanced-usage.html#ssl-warnings
  InsecureRequestWarning,

[ERROR   ] Error from vault: {"errors":[]}

local:
    None

get_or_set_hash:

# salt-call sdb.get_or_set_hash 'sdb://myvault/salt/test1?password'
== Getting existing records
[WARNING ] /usr/lib/python3.6/site-packages/requests/packages/urllib3/connectionpool.py:1004: InsecureRequestWarning: Unverified HTTPS request is being made. Adding certificate verification is strongly advised. See: https://urllib3.readthedocs.io/en/latest/advanced-usage.html#ssl-warnings
  InsecureRequestWarning,

[WARNING ] /usr/lib/python3.6/site-packages/requests/packages/urllib3/connectionpool.py:1004: InsecureRequestWarning: Unverified HTTPS request is being made. Adding certificate verification is strongly advised. See: https://urllib3.readthedocs.io/en/latest/advanced-usage.html#ssl-warnings
  InsecureRequestWarning,

[ERROR   ] Error from vault: {"errors":[]}

Return value: None
ret is None, setting new value
New value: _7d_s#&-
uri: sdb://myvault/salt/test1?password, val: _7d_s#&-

[WARNING ] /usr/lib/python3.6/site-packages/requests/packages/urllib3/connectionpool.py:1004: InsecureRequestWarning: Unverified HTTPS request is being made. Adding certificate verification is strongly advised. See: https://urllib3.readthedocs.io/en/latest/advanced-usage.html#ssl-warnings
  InsecureRequestWarning,

[WARNING ] /usr/lib/python3.6/site-packages/requests/packages/urllib3/connectionpool.py:1004: InsecureRequestWarning: Unverified HTTPS request is being made. Adding certificate verification is strongly advised. See: https://urllib3.readthedocs.io/en/latest/advanced-usage.html#ssl-warnings
  InsecureRequestWarning,

[ERROR   ] Unable to connect to vault server: {"errors":["permission denied"]}

[ERROR   ] Failed to write secret! HTTPError: 403 Client Error: Forbidden for url: https://<VAULT_ADDR>:8200/v1/salt/test1
Error running 'sdb.get_or_set_hash': 403 Client Error: Forbidden for url: https://<VAULT_ADDR>:8200/v1/salt/test1

Here are the print commands I added, in case you wonder:

def sdb_get_or_set_hash(
    uri,
    opts,
    length=8,
    chars="abcdefghijklmnopqrstuvwxyz0123456789!@#$%^&*(-_=+)",
    utils=None,
):
    # ...
    print("== Getting existing records")
    ret = sdb_get(uri, opts, utils=utils)
    print(f"Return value: {ret}")

    if ret is None:
        print("ret is None, setting new value")
        val = "".join([random.SystemRandom().choice(chars) for _ in range(length)])
        print(f"New value: {val}")
        print(f"uri: {uri}, val: {val}")
        sdb_set(uri, val, opts, utils)

    return ret or val

@baturaysoysal
Copy link
Author

Meanwhile, I had another finding.

I tried to implement the "get, if empty result, then set" logic in the state file I am trying to write, in order to develop a workaround (since this issue blocks our development process). The get is, again, successful. However, when I try to run set from inside a salt formula, it also fails with 403.

By adding a state:

tmp_create_secret_vault:
  module.run:
    - name: sdb.set
    - uri: {{ 'sdb://myvault/salt/rabbitmq_team_passwords/' ~ cluster_name ~ '/' ~ team_username ~ '/password' }}
    - value: {{ team_password }}

From inside Jinja:

{% set create_vault_secret_result = salt['sdb.set']('sdb://myvault/salt/rabbitmq_team_passwords/' ~ cluster_name ~ '/' ~ team_username ~ '/password', team_password) %}

Both these methods fail with the following message:

403 Client Error: Forbidden for url: https://<VAULT_IP>:8200/v1/salt/data/rabbitmq_team_passwords/<cluster_name>/<team_username>

Running the command from CLI still works:

salt 'myminion' sdb.set 'sdb://myvault/salt/rabbitmq_team_passwords/<cluster_name>/<team_name>/password' '1234'
myminion:
    True

I think this might be related to the original issue, so I thought it would be worth to mention it.

@baturaysoysal
Copy link
Author

I think the issue is the same as #57561

The workaround to set uses: 0 works for me as well. This explains both of the above issues:

  • sdb.get and sdb.set work separately, but sdb.get_or_set_hash does not work, since it tries to use the same token twice.
  • I cannot get and set a secret from Vault from inside a single Salt formula, because it probably tries to use the same key more than once, too.

@jonsulman
Copy link

Even sdb.get does not work if the secret does not exist. In sdb/vault.py:92:

    try:
        url = "v1/{}".format(path)
        response = __utils__["vault.make_request"]("GET", url)
        if response.status_code == 404:
            if version2["v2"]:
                path = version2["data"] + "/" + key
                url = "v1/{}".format(path)
                response = __utils__["vault.make_request"]("GET", url)
                if response.status_code == 404:
                    return None
            else:
                return None

With the default of uses: 1 if the secret does not exist, sdb will always try the path/key URL. This second request will always get a 403 status because the key is no longer valid. This code should check the key's remaining use counter and get a new one if necessary.

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
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 severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around Vault
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants