File make-pass-renderer-configurable-other-fixes-532... of Package venv-salt-minion

Overview Repositories Revisions Requests Users Attributes Meta

File make-pass-renderer-configurable-other-fixes-532.patch of Package venv-salt-minion (Revision 142b3b9df4b275c9d704c4c26b883512)

Currently displaying revision 142b3b9df4b275c9d704c4c26b883512 , Show latest

From 7b4f5007b7e6a35386d197afe53d02c8d7b41d53 Mon Sep 17 00:00:00 2001
From: Daniel Mach <daniel.mach@gmail.com>
Date: Thu, 6 Oct 2022 11:58:23 +0200
Subject: [PATCH] Make pass renderer configurable & other fixes (#532)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* pass: Use a secure way of handling pass arguments

The original code would fail on pass paths with spaces,
because they would be split into multiple arguments.

* pass: Strip only trailing newline characters from the secret

* pass: Do not modify $HOME env globally

Just set $HOME for calling the pass binary
to avoid affecting anything outside the pass renderer.

* pass: Use pass executable path from _get_pass_exec()

* Make the pass renderer more configurable

1. Allow us to make the pass renderer fail during pillar rendering
   when a secret corresponding with a pass path cannot be fetched.
   For this we add a master config variable pass_strict_fetch.

2. Allow to have prefix for variables that should be processed
   with the pass renderer.
   For this we add a master config variable pass_variable_prefix.

3. Allow us to configure pass' GNUPGHOME and PASSWORD_STORE_DIR
   environmental variables.
   For this we add master config variables pass_gnupghome and pass_dir.

* Add tests for the pass renderer

* pass: Handle FileNotFoundError when pass binary is not available

Co-authored-by: Marcus Rückert <darix@nordisch.org>
---
 changelog/62120.added                     |   4 +
 changelog/62120.fixed                     |   4 +
 salt/config/__init__.py                   |  12 ++
 salt/renderers/pass.py                    | 104 ++++++++++++--
 tests/pytests/unit/renderers/test_pass.py | 164 ++++++++++++++++++++++
 5 files changed, 274 insertions(+), 14 deletions(-)
 create mode 100644 changelog/62120.added
 create mode 100644 changelog/62120.fixed
 create mode 100644 tests/pytests/unit/renderers/test_pass.py

diff --git a/changelog/62120.added b/changelog/62120.added
new file mode 100644
index 0000000000..4303d124f0
--- /dev/null
+++ b/changelog/62120.added
@@ -0,0 +1,4 @@
+Config option pass_variable_prefix allows to distinguish variables that contain paths to pass secrets.
+Config option pass_strict_fetch allows to error out when a secret cannot be fetched from pass.
+Config option pass_dir allows setting the PASSWORD_STORE_DIR env for pass.
+Config option pass_gnupghome allows setting the $GNUPGHOME env for pass.
diff --git a/changelog/62120.fixed b/changelog/62120.fixed
new file mode 100644
index 0000000000..22a9711383
--- /dev/null
+++ b/changelog/62120.fixed
@@ -0,0 +1,4 @@
+Pass executable path from _get_path_exec() is used when calling the program.
+The $HOME env is no longer modified globally.
+Only trailing newlines are stripped from the fetched secret.
+Pass process arguments are handled in a secure way.
diff --git a/salt/config/__init__.py b/salt/config/__init__.py
index 2c42290598..9e72a5b4b7 100644
--- a/salt/config/__init__.py
+++ b/salt/config/__init__.py
@@ -960,6 +960,14 @@ VALID_OPTS = immutabletypes.freeze(
         # Use Adler32 hashing algorithm for server_id (default False until Sodium, "adler32" after)
         # Possible values are: False, adler32, crc32
         "server_id_use_crc": (bool, str),
+        # pass renderer: Fetch secrets only for the template variables matching the prefix
+        "pass_variable_prefix": str,
+        # pass renderer: Whether to error out when unable to fetch a secret
+        "pass_strict_fetch": bool,
+        # pass renderer: Set GNUPGHOME env for Pass
+        "pass_gnupghome": str,
+        # pass renderer: Set PASSWORD_STORE_DIR env for Pass
+        "pass_dir": str,
     }
 )
 
@@ -1601,6 +1609,10 @@ DEFAULT_MASTER_OPTS = immutabletypes.freeze(
         "fips_mode": False,
         "detect_remote_minions": False,
         "remote_minions_port": 22,
+        "pass_variable_prefix": "",
+        "pass_strict_fetch": False,
+        "pass_gnupghome": "",
+        "pass_dir": "",
     }
 )
 
diff --git a/salt/renderers/pass.py b/salt/renderers/pass.py
index 71b1021b96..ba0f152c23 100644
--- a/salt/renderers/pass.py
+++ b/salt/renderers/pass.py
@@ -45,6 +45,34 @@ Install pass binary
 
         pass:
           pkg.installed
+
+Salt master configuration options
+
+.. code-block:: yaml
+
+        # If the prefix is *not* set (default behavior), all template variables are
+        # considered for fetching secrets from Pass. Those that cannot be resolved
+        # to a secret are passed through.
+        #
+        # If the prefix is set, only the template variables with matching prefix are
+        # considered for fetching the secrets, other variables are passed through.
+        #
+        # For ease of use it is recommended to set the following options as well:
+        #       renderer: 'jinja|yaml|pass'
+        #       pass_strict_fetch: true
+        #
+        pass_variable_prefix: 'pass:'
+
+        # If set to 'true', error out when unable to fetch a secret for a template variable.
+        pass_strict_fetch: true
+
+        # Set GNUPGHOME env for Pass.
+        # Defaults to: ~/.gnupg
+        pass_gnupghome: <path>
+
+        # Set PASSWORD_STORE_DIR env for Pass.
+        # Defaults to: ~/.password-store
+        pass_dir: <path>
 """
 
 
@@ -54,7 +82,7 @@ from os.path import expanduser
 from subprocess import PIPE, Popen
 
 import salt.utils.path
-from salt.exceptions import SaltRenderError
+from salt.exceptions import SaltConfigurationError, SaltRenderError
 
 log = logging.getLogger(__name__)
 
@@ -75,18 +103,71 @@ def _fetch_secret(pass_path):
     Fetch secret from pass based on pass_path. If there is
     any error, return back the original pass_path value
     """
-    cmd = "pass show {}".format(pass_path.strip())
-    log.debug("Fetching secret: %s", cmd)
+    pass_exec = _get_pass_exec()
+
+    # Make a backup in case we want to return the original value without stripped whitespaces
+    original_pass_path = pass_path
+
+    # Remove the optional prefix from pass path
+    pass_prefix = __opts__["pass_variable_prefix"]
+    if pass_prefix:
+        # If we do not see our prefix we do not want to process this variable
+        # and we return the unmodified pass path
+        if not pass_path.startswith(pass_prefix):
+            return pass_path
+
+        # strip the prefix from the start of the string
+        pass_path = pass_path[len(pass_prefix) :]
+
+    # The pass_strict_fetch option must be used with pass_variable_prefix
+    pass_strict_fetch = __opts__["pass_strict_fetch"]
+    if pass_strict_fetch and not pass_prefix:
+        msg = "The 'pass_strict_fetch' option requires 'pass_variable_prefix' option enabled"
+        raise SaltConfigurationError(msg)
+
+    # Remove whitespaces from the pass_path
+    pass_path = pass_path.strip()
 
-    proc = Popen(cmd.split(" "), stdout=PIPE, stderr=PIPE)
-    pass_data, pass_error = proc.communicate()
+    cmd = [pass_exec, "show", pass_path]
+    log.debug("Fetching secret: %s", " ".join(cmd))
+
+    # Make sure environment variable HOME is set, since Pass looks for the
+    # password-store under ~/.password-store.
+    env = os.environ.copy()
+    env["HOME"] = expanduser("~")
+
+    pass_dir = __opts__["pass_dir"]
+    if pass_dir:
+        env["PASSWORD_STORE_DIR"] = pass_dir
+
+    pass_gnupghome = __opts__["pass_gnupghome"]
+    if pass_gnupghome:
+        env["GNUPGHOME"] = pass_gnupghome
+
+    try:
+        proc = Popen(cmd, stdout=PIPE, stderr=PIPE, env=env)
+        pass_data, pass_error = proc.communicate()
+        pass_returncode = proc.returncode
+    except OSError as e:
+        pass_data, pass_error = "", str(e)
+        pass_returncode = 1
 
     # The version of pass used during development sent output to
     # stdout instead of stderr even though its returncode was non zero.
-    if proc.returncode or not pass_data:
-        log.warning("Could not fetch secret: %s %s", pass_data, pass_error)
-        pass_data = pass_path
-    return pass_data.strip()
+    if pass_returncode or not pass_data:
+        try:
+            pass_error = pass_error.decode("utf-8")
+        except (AttributeError, ValueError):
+            pass
+        msg = "Could not fetch secret '{}' from the password store: {}".format(
+            pass_path, pass_error
+        )
+        if pass_strict_fetch:
+            raise SaltRenderError(msg)
+        else:
+            log.warning(msg)
+            return original_pass_path
+    return pass_data.rstrip("\r\n")
 
 
 def _decrypt_object(obj):
@@ -108,9 +189,4 @@ def render(pass_info, saltenv="base", sls="", argline="", **kwargs):
     """
     Fetch secret from pass based on pass_path
     """
-    _get_pass_exec()
-
-    # Make sure environment variable HOME is set, since Pass looks for the
-    # password-store under ~/.password-store.
-    os.environ["HOME"] = expanduser("~")
     return _decrypt_object(pass_info)
diff --git a/tests/pytests/unit/renderers/test_pass.py b/tests/pytests/unit/renderers/test_pass.py
new file mode 100644
index 0000000000..74e822c7ec
--- /dev/null
+++ b/tests/pytests/unit/renderers/test_pass.py
@@ -0,0 +1,164 @@
+import importlib
+
+import pytest
+
+import salt.config
+import salt.exceptions
+from tests.support.mock import MagicMock, patch
+
+# "pass" is a reserved keyword, we need to import it differently
+pass_ = importlib.import_module("salt.renderers.pass")
+
+
+@pytest.fixture
+def configure_loader_modules():
+    return {
+        pass_: {
+            "__opts__": salt.config.DEFAULT_MASTER_OPTS.copy(),
+            "_get_pass_exec": MagicMock(return_value="/usr/bin/pass"),
+        }
+    }
+
+
+# The default behavior is that if fetching a secret from pass fails,
+# the value is passed through. Even the trailing newlines are preserved.
+def test_passthrough():
+    pass_path = "secret\n"
+    expected = pass_path
+    result = pass_.render(pass_path)
+
+    assert result == expected
+
+
+# Fetch a secret in the strict mode.
+def test_strict_fetch():
+    config = {
+        "pass_variable_prefix": "pass:",
+        "pass_strict_fetch": True,
+    }
+
+    popen_mock = MagicMock(spec=pass_.Popen)
+    popen_mock.return_value.communicate.return_value = ("password123456\n", "")
+    popen_mock.return_value.returncode = 0
+
+    mocks = {
+        "Popen": popen_mock,
+    }
+
+    pass_path = "pass:secret"
+    expected = "password123456"
+    with patch.dict(pass_.__opts__, config), patch.dict(pass_.__dict__, mocks):
+        result = pass_.render(pass_path)
+
+    assert result == expected
+
+
+# Fail to fetch a secret in the strict mode.
+def test_strict_fetch_fail():
+    config = {
+        "pass_variable_prefix": "pass:",
+        "pass_strict_fetch": True,
+    }
+
+    popen_mock = MagicMock(spec=pass_.Popen)
+    popen_mock.return_value.communicate.return_value = ("", "Secret not found")
+    popen_mock.return_value.returncode = 1
+
+    mocks = {
+        "Popen": popen_mock,
+    }
+
+    pass_path = "pass:secret"
+    with patch.dict(pass_.__opts__, config), patch.dict(pass_.__dict__, mocks):
+        with pytest.raises(salt.exceptions.SaltRenderError):
+            pass_.render(pass_path)
+
+
+# Passthrough a value that doesn't have a pass prefix.
+def test_strict_fetch_passthrough():
+    config = {
+        "pass_variable_prefix": "pass:",
+        "pass_strict_fetch": True,
+    }
+
+    pass_path = "variable-without-pass-prefix\n"
+    expected = pass_path
+    with patch.dict(pass_.__opts__, config):
+        result = pass_.render(pass_path)
+
+    assert result == expected
+
+
+# Fetch a secret in the strict mode. The pass path contains spaces.
+def test_strict_fetch_pass_path_with_spaces():
+    config = {
+        "pass_variable_prefix": "pass:",
+        "pass_strict_fetch": True,
+    }
+
+    popen_mock = MagicMock(spec=pass_.Popen)
+    popen_mock.return_value.communicate.return_value = ("password123456\n", "")
+    popen_mock.return_value.returncode = 0
+
+    mocks = {
+        "Popen": popen_mock,
+    }
+
+    pass_path = "pass:se cr et"
+    with patch.dict(pass_.__opts__, config), patch.dict(pass_.__dict__, mocks):
+        pass_.render(pass_path)
+
+    call_args, call_kwargs = popen_mock.call_args_list[0]
+    assert call_args[0] == ["/usr/bin/pass", "show", "se cr et"]
+
+
+# Fetch a secret in the strict mode. The secret contains leading and trailing whitespaces.
+def test_strict_fetch_secret_with_whitespaces():
+    config = {
+        "pass_variable_prefix": "pass:",
+        "pass_strict_fetch": True,
+    }
+
+    popen_mock = MagicMock(spec=pass_.Popen)
+    popen_mock.return_value.communicate.return_value = (" \tpassword123456\t \r\n", "")
+    popen_mock.return_value.returncode = 0
+
+    mocks = {
+        "Popen": popen_mock,
+    }
+
+    pass_path = "pass:secret"
+    expected = " \tpassword123456\t "  # only the trailing newlines get striped
+    with patch.dict(pass_.__opts__, config), patch.dict(pass_.__dict__, mocks):
+        result = pass_.render(pass_path)
+
+    assert result == expected
+
+
+# Test setting env variables based on config values:
+# - pass_gnupghome -> GNUPGHOME
+# - pass_dir -> PASSWORD_STORE_DIR
+def test_env():
+    config = {
+        "pass_variable_prefix": "pass:",
+        "pass_strict_fetch": True,
+        "pass_gnupghome": "/path/to/gnupghome",
+        "pass_dir": "/path/to/secretstore",
+    }
+
+    popen_mock = MagicMock(spec=pass_.Popen)
+    popen_mock.return_value.communicate.return_value = ("password123456\n", "")
+    popen_mock.return_value.returncode = 0
+
+    mocks = {
+        "Popen": popen_mock,
+    }
+
+    pass_path = "pass:secret"
+    expected = " \tpassword123456\t "  # only the trailing newlines get striped
+    with patch.dict(pass_.__opts__, config), patch.dict(pass_.__dict__, mocks):
+        result = pass_.render(pass_path)
+
+    call_args, call_kwargs = popen_mock.call_args_list[0]
+    assert call_kwargs["env"]["GNUPGHOME"] == config["pass_gnupghome"]
+    assert call_kwargs["env"]["PASSWORD_STORE_DIR"] == config["pass_dir"]
-- 
2.37.3

Places

File make-pass-renderer-configurable-other-fixes-532.patch of Package venv-salt-minion (Revision 142b3b9df4b275c9d704c4c26b883512)

Places