Make pass renderer configurable & other fixes

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.

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.

salt/config/__init__.py

@@ -964,6 +964,14 @@ def _gather_buffer_space():
         # The port to be used when checking if a master is connected to a
         # minion
         "remote_minions_port": int,
+        # 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,
     }
 )
 
@@ -1604,6 +1612,10 @@ def _gather_buffer_space():
         "fips_mode": False,
         "detect_remote_minions": False,
         "remote_minions_port": 22,
+        "pass_variable_prefix": "",
+        "pass_strict_fetch": False,
+        "pass_gnupghome": "",
+        "pass_dir": "",
     }
 )
 

salt/renderers/pass.py

@@ -45,6 +45,34 @@
 
         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 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)

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"]