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

The 'verify' key in Vault module configuration has to be specified in the minion config, too #58174

Closed
gasmick opened this issue Aug 12, 2020 · 3 comments · Fixed by #62684
Closed
Assignees
Labels
Bug broken, incorrect, or confusing behavior Documentation Relates to Salt documentation severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around time-estimate-sprint
Milestone

Comments

@gasmick
Copy link

gasmick commented Aug 12, 2020

Description
The vault module configuration documentation does not mention the necessity of vault:verify key in the minion config. Consequently, the __opts__ dictionary on minion is missing the vault:verify key, which is required in the code in utils/vault.py.

Setup

vault:
  url: https://vault.service.consul:8200
  verify: /etc/ssl/certs/vault-ca.pem
  role_name: minion
  auth:
      method: approle
      role_id: xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
      secret_id: yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy
      uses: 0
  policies:
      - saltstack/minions

Steps to Reproduce the behavior

  1. Use as the value of verify any file which is not system default like /etc/ssl/certs/ca-certificates.crt or /etc/ssl/certs/ca-bundle.crt
  2. Relocate for test's sake default CA Bundle, either /etc/ssl/certs/ca-certificates.crt or /etc/ssl/certs/ca-bundle.crt depending on the OS.
  3. run salt-call vault.read_secret 'secret/your-secret' and get a Certificate Validation Error

Explanation
If minion config doesn't include the vault:verify key, AND master config doesn't include vault:auth:allow_minion_override: True, we end up at the "Don't worry" line inside the make_request function in salt/utils/vault.py

    if "verify" in args:
        args["verify"] = args["verify"]
    else:
        try:
            args["verify"] = __opts__.get("vault").get("verify", None)
        except (TypeError, AttributeError):
            # Don't worry about setting verify if it doesn't exist
            pass

This new behavior is not documented.

Expected behavior
With this code in utils/vault.py the documentation has to include the explicit instruction to place the vault:verify key into the minion config and set the allow_minion_override to True on salt-master.

Minion config:

vault:
  verify: /etc/ssl/certs/vault-ca.pem

Master config:

vault:
  url: https://vault.service.consul:8200
  verify: /etc/ssl/certs/vault-ca.pem
  role_name: minion
  auth:
      method: approle
      role_id: xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
      secret_id: yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy
      allow_minion_override: True
      uses: 0
  policies:
      - saltstack/minions

Versions Report

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

Dependency Versions:
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: 2.6.1
      docker-py: Not Installed
          gitdb: Not Installed
      gitpython: Not Installed
         Jinja2: 2.10
        libgit2: Not Installed
       M2Crypto: 0.36.0
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.5.6
   mysql-python: 1.3.10
      pycparser: Not Installed
       pycrypto: 2.6.1
   pycryptodome: 3.4.7
         pygit2: Not Installed
         Python: 3.6.9 (default, Jul 17 2020, 12:50:27)
   python-gnupg: 0.4.1
         PyYAML: 3.12
          PyZMQ: 17.1.2
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 4.5.3
            ZMQ: 4.2.5

System Versions:
           dist: ubuntu 18.04 Bionic Beaver
         locale: UTF-8
        machine: x86_64
        release: 5.3.0-1032-aws
         system: Linux
        version: Ubuntu 18.04 Bionic Beaver
@gasmick gasmick added the Bug broken, incorrect, or confusing behavior label Aug 12, 2020
@gasmick gasmick changed the title [BUG] The 'verify' key in Vault module configuration has to be specified in the minion config The 'verify' key in Vault module configuration has to be specified in the minion config Aug 12, 2020
@gasmick gasmick changed the title The 'verify' key in Vault module configuration has to be specified in the minion config The 'verify' key in Vault module configuration has to be specified in the minion config, too Aug 12, 2020
@alan-cugler
Copy link
Contributor

In case anyone looking at this issue is wondering you can set on both the minion and master:

vault:
  verify: False

To get disable the verify if you're not needing the cert check for whatever reason.

@dwoz dwoz removed the needs-triage label Sep 4, 2020
@dwoz dwoz added this to the Magnesium milestone Sep 4, 2020
@dwoz
Copy link
Contributor

dwoz commented Sep 4, 2020

At the very least this should have been properly documented as a breaking change. We'll add some documentation for it.

@sagetherage sagetherage added the Magnesium Mg release after Na prior to Al label Sep 10, 2020
@sagetherage sagetherage removed the Magnesium Mg release after Na prior to Al label Oct 8, 2020
@sagetherage sagetherage modified the milestones: Magnesium, Approved Oct 8, 2020
@dmulyalin
Copy link

Hit this problem and changed original code to fix it before found this GitHub issue page:

    if "verify" in args:
        args["verify"] = args["verify"]
    else:
        try:
            # replaced this line with following line   
            # args["verify"] = __opts__.get("vault").get("verify", None)
            args["verify"] = connection.get("verify", None)
        except (TypeError, AttributeError):
            # Don't worry about setting verify if it doesn't exist
            pass

where connection is a dictionary of content {'url': 'https://vaul_url', 'token': 'token', 'verify': False, 'uses': 1, 'lease_duration': 123, 'issued': 12345} - and it contains verify: False option set on master config.

It might be that using connection dictionary is a better way to get "verify" configuration as it populated with config from master, instead of defining "verify" option in multiple places, in that case current documentation will stay correct.

@sagetherage sagetherage added Documentation Relates to Salt documentation severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around labels Mar 22, 2021
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 Documentation Relates to Salt documentation severity-medium 3rd level, incorrect or bad functionality, confusing and lacks a work around time-estimate-sprint
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants