File make-pass-renderer-configurable-other-fixes-532.patch of Package salt
From 030e2cb20af09673d5f38d68bcb257c6c839a2f3 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 7cdee12c4d..0cc0deb874 100644
--- a/salt/config/__init__.py
+++ b/salt/config/__init__.py
@@ -967,6 +967,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,
}
)
@@ -1608,6 +1616,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
