From 8da1ffbcd70b5c763848a2832460bc23d6a3afc4 Mon Sep 17 00:00:00 2001 From: Pedro Algarvio Date: Tue, 28 Sep 2021 16:39:37 +0100 Subject: [PATCH 01/66] Redirect imports of ``salt.ext.six`` to ``six`` Fixes #60966 --- changelog/60966.fixed | 1 + salt/__init__.py | 14 +++- tests/pytests/unit/test_ext_importers.py | 89 ++++++++++++++++++++++++ tests/unit/test_ext.py | 61 ---------------- 4 files changed, 103 insertions(+), 62 deletions(-) create mode 100644 changelog/60966.fixed create mode 100644 tests/pytests/unit/test_ext_importers.py delete mode 100644 tests/unit/test_ext.py diff --git a/changelog/60966.fixed b/changelog/60966.fixed new file mode 100644 index 000000000000..99f011a25287 --- /dev/null +++ b/changelog/60966.fixed @@ -0,0 +1 @@ +Redirect imports of ``salt.ext.six`` to ``six`` diff --git a/salt/__init__.py b/salt/__init__.py index 01ed8446b372..0a18a9bf87c4 100644 --- a/salt/__init__.py +++ b/salt/__init__.py @@ -36,8 +36,20 @@ def load_module(self, name): return mod +class SixRedirectImporter: + def find_module(self, module_name, package_path=None): + if module_name.startswith("salt.ext.six"): + return self + return None + + def load_module(self, name): + mod = importlib.import_module(name[9:]) + sys.modules[name] = mod + return mod + + # Try our importer first -sys.meta_path = [TornadoImporter()] + sys.meta_path +sys.meta_path = [TornadoImporter(), SixRedirectImporter()] + sys.meta_path # All salt related deprecation warnings should be shown once each! diff --git a/tests/pytests/unit/test_ext_importers.py b/tests/pytests/unit/test_ext_importers.py new file mode 100644 index 000000000000..bedf0fe8c02e --- /dev/null +++ b/tests/pytests/unit/test_ext_importers.py @@ -0,0 +1,89 @@ +import logging +import os +import subprocess +import sys + +import pytest +import salt +import six # pylint: disable=blacklisted-external-import,3rd-party-module-not-gated + +log = logging.getLogger(__name__) + + +def test_tornado_import_override(tmp_path): + """ + Ensure we are not using any non vendor'ed tornado + """ + test_source = """ + from __future__ import absolute_import, print_function + import salt + import tornado + print(tornado.__name__) + """ + tornado_source = """ + foo = 'bar' + """ + with pytest.helpers.temp_file( + "test.py", directory=tmp_path, contents=test_source + ) as test_source_path, pytest.helpers.temp_file( + "tornado.py", directory=tmp_path, contents=tornado_source + ): + env = os.environ.copy() + env["PYTHONPATH"] = os.pathsep.join(sys.path) + ret = subprocess.run( + [sys.executable, str(test_source_path)], + stderr=subprocess.PIPE, + stdout=subprocess.PIPE, + env=env, + shell=False, + check=False, + universal_newlines=True, + ) + assert ret.returncode == 0 + assert ret.stdout.strip() == "salt.ext.tornado" + + +@pytest.mark.parametrize( + "six_import_line,six_print_line", + ( + ("import salt.ext.six", "print(salt.ext.six.__name__, salt.ext.six.__file__)"), + ("import salt.ext.six as six", "print(six.__name__, six.__file__)"), + ("from salt.ext import six", "print(six.__name__, six.__file__)"), + ("import six", "print(six.__name__, six.__file__)"), + ), +) +def test_salt_ext_six_import_override(tmp_path, six_import_line, six_print_line): + """ + Ensure we are not using, the now non existent, vendor'ed six + """ + test_source = """ + import salt + {} + {} + """.format( + six_import_line, six_print_line + ) + with pytest.helpers.temp_file( + "test.py", directory=tmp_path, contents=test_source + ) as test_source_path: + env = os.environ.copy() + env["PYTHONPATH"] = os.pathsep.join(sys.path) + ret = subprocess.run( + [sys.executable, str(test_source_path)], + stderr=subprocess.PIPE, + stdout=subprocess.PIPE, + env=env, + shell=False, + check=False, + universal_newlines=True, + ) + assert ret.returncode == 0 + assert ret.stdout.strip() == "six {}".format(six.__file__) + + +def test_regression_56063(): + importer = salt.TornadoImporter() + try: + importer.find_module("tornado") + except TypeError: + assert False, "TornadoImporter raised type error when one argument passed" diff --git a/tests/unit/test_ext.py b/tests/unit/test_ext.py deleted file mode 100644 index eefedf8a08b7..000000000000 --- a/tests/unit/test_ext.py +++ /dev/null @@ -1,61 +0,0 @@ -import logging -import os -import subprocess -import sys -import tempfile - -import salt -import salt.modules.cmdmod -import salt.utils.files -import salt.utils.platform -import tests.support.helpers -from tests.support.unit import TestCase - -log = logging.getLogger(__name__) - - -class VendorTornadoTest(TestCase): - """ - Ensure we are not using any non vendor'ed tornado - """ - - def test_import_override(self): - tmp = tempfile.mkdtemp() - test_source = tests.support.helpers.dedent( - """ - from __future__ import absolute_import, print_function - import salt - import tornado - print(tornado.__name__) - """ - ) - test_source_path = os.path.join(tmp, "test.py") - tornado_source = tests.support.helpers.dedent( - """ - foo = 'bar' - """ - ) - tornado_source_path = os.path.join(tmp, "tornado.py") - with salt.utils.files.fopen(test_source_path, "w") as fp: - fp.write(test_source) - with salt.utils.files.fopen(tornado_source_path, "w") as fp: - fp.write(tornado_source) - # Preserve the virtual environment - env = os.environ.copy() - env["PYTHONPATH"] = os.pathsep.join(sys.path) - p = subprocess.Popen( - [sys.executable, test_source_path], - stderr=subprocess.PIPE, - stdout=subprocess.PIPE, - env=env, - ) - p.wait() - pout = p.stdout.read().strip().decode() - assert pout == "salt.ext.tornado", pout - - def test_regression_56063(self): - importer = salt.TornadoImporter() - try: - importer.find_module("tornado") - except TypeError: - assert False, "TornadoImporter raised type error when one argument passed" From 7138386e7ba64247f3b95b6f20e0a7f33df5c05e Mon Sep 17 00:00:00 2001 From: ScriptAutomate Date: Tue, 28 Sep 2021 15:16:23 -0500 Subject: [PATCH 02/66] Latest changelog update for 3004 --- CHANGELOG.md | 4 +++- changelog/60966.fixed | 1 - doc/topics/releases/3004.rst | 1 + 3 files changed, 4 insertions(+), 2 deletions(-) delete mode 100644 changelog/60966.fixed diff --git a/CHANGELOG.md b/CHANGELOG.md index adc79e2c3201..8a591b6f0549 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,9 +7,10 @@ Versions are `MAJOR.PATCH`. # Changelog -Salt 3004 (2021-09-27) +Salt 3004 (2021-09-28) ====================== + Removed ------- @@ -41,6 +42,7 @@ Changed Fixed ----- +- Redirect imports of ``salt.ext.six`` to ``six`` (#60966) - Surface strerror to user state instead of returning false (#20789) - Fixing _get_envs() to preserve the order of pillar_roots. _get_envs() returned pillar_roots in a non-deterministic order. (#24501) - Fixes salt-cloud `KeyError` that occurs when there exists any subnets with no tags when profiles use `subnetname` (#44330) diff --git a/changelog/60966.fixed b/changelog/60966.fixed deleted file mode 100644 index 99f011a25287..000000000000 --- a/changelog/60966.fixed +++ /dev/null @@ -1 +0,0 @@ -Redirect imports of ``salt.ext.six`` to ``six`` diff --git a/doc/topics/releases/3004.rst b/doc/topics/releases/3004.rst index 23a940eeec69..f311a35f307d 100644 --- a/doc/topics/releases/3004.rst +++ b/doc/topics/releases/3004.rst @@ -73,6 +73,7 @@ Changed Fixed ===== +- Redirect imports of ``salt.ext.six`` to ``six`` (#60966) - Surface strerror to user state instead of returning false (#20789) - Fixing _get_envs() to preserve the order of pillar_roots. _get_envs() returned pillar_roots in a non-deterministic order. (#24501) - Fixes salt-cloud ``KeyError`` that occurs when there exists any subnets with no tags when profiles use ``subnetname`` (#44330) From 663dd8a696067f538a8a1feb19930a012a983474 Mon Sep 17 00:00:00 2001 From: Pedro Algarvio Date: Wed, 29 Sep 2021 15:05:12 +0100 Subject: [PATCH 03/66] Handle signals and properly exit, instead of raising exceptions. This was introduced in 26fcda50740c92fe3e4a5ca50a0d83227009942a Fixes #60391 Fixes #60963 --- changelog/60391.fixed | 1 + changelog/60963.fixed | 1 + salt/scripts.py | 56 ++++++-------- tests/pytests/integration/cli/test_salt.py | 85 +++++++++++++++++++++- 4 files changed, 107 insertions(+), 36 deletions(-) create mode 100644 changelog/60391.fixed create mode 100644 changelog/60963.fixed diff --git a/changelog/60391.fixed b/changelog/60391.fixed new file mode 100644 index 000000000000..97a11dd9241b --- /dev/null +++ b/changelog/60391.fixed @@ -0,0 +1 @@ +Handle signals and properly exit, instead of raising exceptions. diff --git a/changelog/60963.fixed b/changelog/60963.fixed new file mode 100644 index 000000000000..97a11dd9241b --- /dev/null +++ b/changelog/60963.fixed @@ -0,0 +1 @@ +Handle signals and properly exit, instead of raising exceptions. diff --git a/salt/scripts.py b/salt/scripts.py index 38adbdf20e14..93eab0f702e0 100644 --- a/salt/scripts.py +++ b/salt/scripts.py @@ -22,31 +22,7 @@ raise SystemExit(salt.defaults.exitcodes.EX_GENERIC) -def _handle_interrupt(exc, original_exc, hardfail=False, trace=""): - """ - if hardfailing: - If we got the original stacktrace, log it - If all cases, raise the original exception - but this is logically part the initial - stack. - else just let salt exit gracefully - - """ - if hardfail: - if trace: - log.error(trace) - raise original_exc - else: - raise exc - - def _handle_signals(client, signum, sigframe): - try: - # This raises AttributeError on Python 3.4 and 3.5 if there is no current exception. - # Ref: https://bugs.python.org/issue23003 - trace = traceback.format_exc() - except AttributeError: - trace = "" try: hardcrash = client.options.hard_crash except (AttributeError, KeyError): @@ -69,17 +45,25 @@ def _handle_signals(client, signum, sigframe): else: exit_msg = None - _handle_interrupt( - SystemExit(exit_msg), - Exception("\nExiting with hard crash on Ctrl-c"), - hardcrash, - trace=trace, - ) + if exit_msg is None and hardcrash: + exit_msg = "\nExiting with hard crash on Ctrl-c" + if exit_msg: + print(exit_msg, file=sys.stderr, flush=True) + if hardcrash: + try: + # This raises AttributeError on Python 3.4 and 3.5 if there is no current exception. + # Ref: https://bugs.python.org/issue23003 + trace = traceback.format_exc() + log.error(trace) + except AttributeError: + pass + sys.exit(salt.defaults.exitcodes.EX_GENERIC) + sys.exit(salt.defaults.exitcodes.EX_OK) def _install_signal_handlers(client): # Install the SIGINT/SIGTERM handlers if not done so far - if signal.getsignal(signal.SIGINT) is signal.SIG_DFL: + if signal.getsignal(signal.SIGINT) in (signal.SIG_DFL, signal.default_int_handler): # No custom signal handling was added, install our own signal.signal(signal.SIGINT, functools.partial(_handle_signals, client)) @@ -474,12 +458,14 @@ def salt_ssh(): _install_signal_handlers(client) client.run() except SaltClientError as err: - trace = traceback.format_exc() + print(str(err), file=sys.stderr, flush=True) try: - hardcrash = client.options.hard_crash + if client.options.hard_crash: + trace = traceback.format_exc() + log.error(trace) except (AttributeError, KeyError): - hardcrash = False - _handle_interrupt(SystemExit(err), err, hardcrash, trace=trace) + pass + sys.exit(salt.defaults.exitcodes.EX_GENERIC) def salt_cloud(): diff --git a/tests/pytests/integration/cli/test_salt.py b/tests/pytests/integration/cli/test_salt.py index c02b177b0d30..e5de16d6d423 100644 --- a/tests/pytests/integration/cli/test_salt.py +++ b/tests/pytests/integration/cli/test_salt.py @@ -1,14 +1,19 @@ """ :codeauthor: Thayne Harbaugh (tharbaug@adobe.com) """ - import logging import os import shutil +import signal +import subprocess +import sys +import tempfile +import time import pytest import salt.defaults.exitcodes import salt.utils.path +from saltfactories.utils.processes import ProcessResult log = logging.getLogger(__name__) @@ -124,3 +129,81 @@ def test_exit_status_correct_usage(salt_cli, salt_minion): """ ret = salt_cli.run("test.ping", minion_tgt=salt_minion.id) assert ret.exitcode == salt.defaults.exitcodes.EX_OK, ret + + +@pytest.mark.slow_test +@pytest.mark.skip_on_windows(reason="Windows does not support SIGINT") +def test_interrupt_on_long_running_job(salt_cli, salt_master, salt_minion): + """ + Ensure that a call to ``salt`` that is taking too long, when a user + hits CTRL-C, that the JID is printed to the console. + + Refer to https://github.com/saltstack/salt/issues/60963 for more details + """ + ret = salt_cli.run("test.sleep", "1", minion_tgt=salt_minion.id) + assert ret.exitcode == 0 + assert ret.json is True + terminal_stdout = tempfile.SpooledTemporaryFile(512000, buffering=0) + terminal_stderr = tempfile.SpooledTemporaryFile(512000, buffering=0) + cmdline = [ + sys.executable, + salt_cli.get_script_path(), + "--config-dir={}".format(salt_master.config_dir), + salt_minion.id, + "test.sleep", + "30", + ] + proc = subprocess.Popen( + cmdline, + shell=False, + stdout=terminal_stdout, + stderr=terminal_stderr, + universal_newlines=True, + ) + try: + # Make sure it actually starts + proc.wait(1) + except subprocess.TimeoutExpired: + pass + else: + pytest.fail("The test process failed to start") + + time.sleep(2) + # Send CTRL-C to the process + proc.send_signal(signal.SIGINT) + with proc: + # Wait for the process to terminate, to avoid zombies. + # Shouldn't really take the 30 seconds + proc.wait(30) + # poll the terminal so the right returncode is set on the popen object + proc.poll() + # This call shouldn't really be necessary + proc.communicate() + + terminal_stdout.flush() + terminal_stdout.seek(0) + if sys.version_info < (3, 6): # pragma: no cover + stdout = proc._translate_newlines( + terminal_stdout.read(), __salt_system_encoding__ + ) + else: + stdout = proc._translate_newlines( + terminal_stdout.read(), __salt_system_encoding__, sys.stdout.errors + ) + terminal_stdout.close() + + terminal_stderr.flush() + terminal_stderr.seek(0) + if sys.version_info < (3, 6): # pragma: no cover + stderr = proc._translate_newlines( + terminal_stderr.read(), __salt_system_encoding__ + ) + else: + stderr = proc._translate_newlines( + terminal_stderr.read(), __salt_system_encoding__, sys.stderr.errors + ) + terminal_stderr.close() + ret = ProcessResult(proc.returncode, stdout, stderr, cmdline=proc.args) + log.debug(ret) + assert "Exiting gracefully on Ctrl-c" in ret.stderr + assert "This job's jid is" in ret.stderr From 227c2dc249ebe25700303a97670d855aa6249346 Mon Sep 17 00:00:00 2001 From: Wayne Werner Date: Tue, 5 Oct 2021 15:22:31 -0500 Subject: [PATCH 04/66] Add test for #61003 --- tests/pytests/unit/modules/test_yumpkg.py | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/tests/pytests/unit/modules/test_yumpkg.py b/tests/pytests/unit/modules/test_yumpkg.py index 7bcb04f447f4..ea8135bcef63 100644 --- a/tests/pytests/unit/modules/test_yumpkg.py +++ b/tests/pytests/unit/modules/test_yumpkg.py @@ -1988,3 +1988,26 @@ def test_services_need_restart_requires_dnf(): """Test that yumpkg.services_need_restart raises an error if DNF is unavailable.""" with patch("salt.modules.yumpkg._yum", Mock(return_value="yum")): pytest.raises(CommandExecutionError, yumpkg.services_need_restart) + + +def test_61003_pkg_should_not_fail_when_target_not_in_old_pkgs(): + patch_list_pkgs = patch( + "salt.modules.yumpkg.list_pkgs", return_value={}, autospec=True + ) + patch_salt = patch.dict( + yumpkg.__salt__, + { + "pkg_resource.parse_targets": Mock( + return_value=[ + { + "fnord-this-is-not-actually-a-package": "fnord-this-is-not-actually-a-package-1.2.3" + } + ] + ) + }, + ) + with patch_list_pkgs, patch_salt: + # During the 3004rc1 we discoverd that if list_pkgs was missing + # packages that were returned by parse_targets that yumpkg.remove would + # catch on fire. This ensures that won't go undetected again. + yumpkg.remove() From b925111f284617cd4eae177b4254bcdae5c569d2 Mon Sep 17 00:00:00 2001 From: Wayne Werner Date: Tue, 5 Oct 2021 15:32:35 -0500 Subject: [PATCH 05/66] Fix #61003 Restored the previously shifted check for version_to_remove in old[target]. This had been extracted along with the correctly extracted double pkg_params[target] lookup, but that lost the `target in old` guard. Putting the check back here prevents KeyError when looking for a non-existent target in `old`. --- salt/modules/yumpkg.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/salt/modules/yumpkg.py b/salt/modules/yumpkg.py index 4c799501decd..cf684e20f733 100644 --- a/salt/modules/yumpkg.py +++ b/salt/modules/yumpkg.py @@ -2122,12 +2122,11 @@ def remove(name=None, pkgs=None, **kwargs): # pylint: disable=W0613 for target in pkg_params: version_to_remove = pkg_params[target] - installed_versions = old[target].split(",") # Check if package version set to be removed is actually installed: if target in old and not version_to_remove: targets.append(target) - elif target in old and version_to_remove in installed_versions: + elif target in old and version_to_remove in old[target].split(","): arch = "" pkgname = target try: From 0a6a14f6a51e729862b0efff5601b581c964d0d3 Mon Sep 17 00:00:00 2001 From: Megan Wilhite Date: Fri, 1 Oct 2021 10:53:52 -0400 Subject: [PATCH 06/66] Handle various architecture formats in aptpkg module --- changelog/60971.fixed | 1 + salt/modules/aptpkg.py | 5 +-- tests/pytests/unit/modules/test_aptpkg.py | 49 ++++++++++++++++++++++- 3 files changed, 51 insertions(+), 4 deletions(-) create mode 100644 changelog/60971.fixed diff --git a/changelog/60971.fixed b/changelog/60971.fixed new file mode 100644 index 000000000000..4bac7bdf3e48 --- /dev/null +++ b/changelog/60971.fixed @@ -0,0 +1 @@ +Handle all repo formats in the aptpkg module. diff --git a/salt/modules/aptpkg.py b/salt/modules/aptpkg.py index ea173c33f066..dfa86480a4c5 100644 --- a/salt/modules/aptpkg.py +++ b/salt/modules/aptpkg.py @@ -175,13 +175,12 @@ def _parse_sources(self, line): if repo_line[0] not in ["deb", "deb-src", "rpm", "rpm-src"]: self.invalid = True return False - self.architectures = [] if repo_line[1].startswith("["): opts = re.search(r"\[.*\]", self.line).group(0).strip("[]") - repo_line = [x for x in repo_line if x.strip("[]")] + repo_line = [x.strip("[]") for x in repo_line if x.strip("[]")] for opt in opts.split(): if opt.startswith("arch"): - self.architectures = opt.split("=", 1)[1] + self.architectures.extend(opt.split("=", 1)[1].split(",")) try: repo_line.pop(repo_line.index(opt)) except ValueError: diff --git a/tests/pytests/unit/modules/test_aptpkg.py b/tests/pytests/unit/modules/test_aptpkg.py index 9260fda004e7..a753f05b50f1 100644 --- a/tests/pytests/unit/modules/test_aptpkg.py +++ b/tests/pytests/unit/modules/test_aptpkg.py @@ -20,7 +20,7 @@ CommandNotFoundError, SaltInvocationError, ) -from tests.support.mock import MagicMock, Mock, call, patch +from tests.support.mock import MagicMock, Mock, call, mock_open, patch try: from aptsources import sourceslist # pylint: disable=unused-import @@ -1108,3 +1108,50 @@ def test_services_need_restart(): "cups-daemon", "rsyslog", ] + + +def test_sourceslist_multiple_comps(): + """ + Test SourcesList when repo has multiple comps + """ + repo_line = "deb http://archive.ubuntu.com/ubuntu/ focal-updates main restricted" + with patch.object(aptpkg, "HAS_APT", return_value=True): + with patch("salt.utils.files.fopen", mock_open(read_data=repo_line)): + with patch("pathlib.Path.is_file", side_effect=[True, False]): + sources = aptpkg.SourcesList() + for source in sources: + assert source.type == "deb" + assert source.uri == "http://archive.ubuntu.com/ubuntu/" + assert source.comps == ["main", "restricted"] + assert source.dist == "focal-updates" + + +@pytest.mark.parametrize( + "repo_line", + [ + "deb [ arch=amd64 ] http://archive.ubuntu.com/ubuntu/ focal-updates main restricted", + "deb [arch=amd64 ] http://archive.ubuntu.com/ubuntu/ focal-updates main restricted", + "deb [arch=amd64 test=one ] http://archive.ubuntu.com/ubuntu/ focal-updates main restricted", + "deb [arch=amd64,armel test=one ] http://archive.ubuntu.com/ubuntu/ focal-updates main restricted", + "deb [ arch=amd64,armel test=one ] http://archive.ubuntu.com/ubuntu/ focal-updates main restricted", + "deb [ arch=amd64,armel test=one] http://archive.ubuntu.com/ubuntu/ focal-updates main restricted", + "deb [arch=amd64] http://archive.ubuntu.com/ubuntu/ focal-updates main restricted", + ], +) +def test_sourceslist_architectures(repo_line): + """ + Test SourcesList when architectures is in repo + """ + with patch.object(aptpkg, "HAS_APT", return_value=True): + with patch("salt.utils.files.fopen", mock_open(read_data=repo_line)): + with patch("pathlib.Path.is_file", side_effect=[True, False]): + sources = aptpkg.SourcesList() + for source in sources: + assert source.type == "deb" + assert source.uri == "http://archive.ubuntu.com/ubuntu/" + assert source.comps == ["main", "restricted"] + assert source.dist == "focal-updates" + if "," in repo_line: + assert source.architectures == ["amd64", "armel"] + else: + assert source.architectures == ["amd64"] From d1dd0067ac924a360d314a332626e15b0bc1be80 Mon Sep 17 00:00:00 2001 From: Megan Wilhite Date: Tue, 5 Oct 2021 08:11:59 -0400 Subject: [PATCH 07/66] Write file even if does not exist --- salt/modules/aptpkg.py | 59 +++++++++++-------- .../pytests/functional/modules/test_aptpkg.py | 1 - .../pytests/functional/states/test_pkgrepo.py | 10 ++++ 3 files changed, 46 insertions(+), 24 deletions(-) create mode 100644 tests/pytests/functional/states/test_pkgrepo.py diff --git a/salt/modules/aptpkg.py b/salt/modules/aptpkg.py index dfa86480a4c5..3bf0c5b3660a 100644 --- a/salt/modules/aptpkg.py +++ b/salt/modules/aptpkg.py @@ -137,7 +137,6 @@ def __init__(self, line, file=None): self.file = file if not self.file: self.file = str(pathlib.Path(os.sep, "etc", "apt", "sources.list")) - self.edit = False self._parse_sources(line) def repo_line(self): @@ -148,17 +147,16 @@ def repo_line(self): if self.invalid: return self.line if self.disabled: - repo_line = repo_line.append("# ") + repo_line.append("#") repo_line.append(self.type) if self.architectures: - repo_line.append(" [arch={}] ".format(" ".join(self.architectures))) + repo_line.append("[arch={}] ".format(" ".join(self.architectures))) repo_line = repo_line + [self.uri, self.dist, " ".join(self.comps)] if self.comment: repo_line.append("#{}".format(self.comment)) - repo_line.append("\n") - return " ".join(repo_line) + return " ".join(repo_line) + "\n" def _parse_sources(self, line): """ @@ -200,15 +198,24 @@ def __init__(self): for file in self.files: if file.is_dir(): for fp in file.glob("**/*.list"): - file = fp - if file.is_file(): - with salt.utils.files.fopen(file) as source: - for line in source: - self.list.append(SourceEntry(line, file=str(file))) + self.add_file(file=fp) + else: + self.add_file(file) def __iter__(self): yield from self.list + def add_file(self, file): + """ + Add the lines of a file to self.list + """ + if file.is_file(): + with salt.utils.files.fopen(file) as source: + for line in source: + self.list.append(SourceEntry(line, file=str(file))) + else: + log.debug("The apt sources file %s does not exist", file) + def add(self, type, uri, dist, orig_comps, architectures): repo_line = [ type, @@ -220,22 +227,29 @@ def add(self, type, uri, dist, orig_comps, architectures): return SourceEntry(" ".join(repo_line)) def remove(self, source): - apt_cmd = salt.utils.path.which("apt-add-repository") - if not apt_cmd: - raise CommandNotFoundError("apt-add-repository is required") - __salt__["cmd.run"]([apt_cmd, "--remove", source.line]) + """ + remove a source from the list of sources + """ self.list.remove(source) def save(self): - for source in self.list: - if source.edit: - if not pathlib.Path(source.file).is_file(): - with salt.utils.files.fopen(source.file, "w") as fp: - fp.write(source.repo_line()) - else: - __salt__["file.replace"]( - source.file, pattern=source.line, repl=source.repo_line() + """ + write all of the sources from the list of sources + to the file. + """ + open_files = {} + try: + for source in self.list: + if source.file not in open_files: + open_files[ + source.file + ] = salt.utils.files.fopen( # pylint: disable=resource-leakage + source.file, "w" ) + open_files[source.file].write(source.repo_line()) + finally: + for fp in open_files: + open_files[fp].close() def _get_ppa_info_from_launchpad(owner_name, ppa_name): @@ -2569,7 +2583,6 @@ def mod_repo(repo, saltenv="base", **kwargs): for key in kwargs: if key in _MODIFY_OK and hasattr(mod_source, key): setattr(mod_source, key, kwargs[key]) - mod_source.edit = True sources.save() # on changes, explicitly refresh if refresh: diff --git a/tests/pytests/functional/modules/test_aptpkg.py b/tests/pytests/functional/modules/test_aptpkg.py index 02155d9f15cd..65b8064f4557 100644 --- a/tests/pytests/functional/modules/test_aptpkg.py +++ b/tests/pytests/functional/modules/test_aptpkg.py @@ -147,7 +147,6 @@ def test_get_repos_doesnot_exist(): assert not ret -@pytest.mark.skip_if_binaries_missing("apt-add-repository") @pytest.mark.destructive_test def test_del_repo(revert_repo_file): """ diff --git a/tests/pytests/functional/states/test_pkgrepo.py b/tests/pytests/functional/states/test_pkgrepo.py new file mode 100644 index 000000000000..fe25f7f8001a --- /dev/null +++ b/tests/pytests/functional/states/test_pkgrepo.py @@ -0,0 +1,10 @@ +import salt.utils.files + + +def test_removed_installed_cycle(states, tmp_path): + repo_file = tmp_path / "stable-binary.list" + repo_content = "deb http://www.deb-multimedia.org stable main" + ret = states.pkgrepo.managed(name=repo_content, file=repo_file, clean_file=True) + with salt.utils.files.fopen(repo_file, "r") as fp: + file_content = fp.read() + assert file_content.strip() == repo_content From a5d32c4c0b44d4ea33ba6e5495e5e68c66c056f0 Mon Sep 17 00:00:00 2001 From: Megan Wilhite Date: Tue, 5 Oct 2021 09:00:12 -0400 Subject: [PATCH 08/66] only run test on debian based platforms --- .../pytests/functional/states/test_pkgrepo.py | 12 ++++++- tests/pytests/unit/modules/test_aptpkg.py | 31 +++++++++++-------- 2 files changed, 29 insertions(+), 14 deletions(-) diff --git a/tests/pytests/functional/states/test_pkgrepo.py b/tests/pytests/functional/states/test_pkgrepo.py index fe25f7f8001a..013348e005f8 100644 --- a/tests/pytests/functional/states/test_pkgrepo.py +++ b/tests/pytests/functional/states/test_pkgrepo.py @@ -1,7 +1,17 @@ +import platform + +import pytest import salt.utils.files -def test_removed_installed_cycle(states, tmp_path): +@pytest.mark.skipif( + not any([x for x in ["ubuntu", "debian"] if x in platform.platform()]), + reason="Test only for debian based platforms", +) +def test_adding_repo_file(states, tmp_path): + """ + test adding a repo file using pkgrepo.managed + """ repo_file = tmp_path / "stable-binary.list" repo_content = "deb http://www.deb-multimedia.org stable main" ret = states.pkgrepo.managed(name=repo_content, file=repo_file, clean_file=True) diff --git a/tests/pytests/unit/modules/test_aptpkg.py b/tests/pytests/unit/modules/test_aptpkg.py index a753f05b50f1..6c5ed29848a7 100644 --- a/tests/pytests/unit/modules/test_aptpkg.py +++ b/tests/pytests/unit/modules/test_aptpkg.py @@ -1110,6 +1110,9 @@ def test_services_need_restart(): ] +@pytest.mark.skipif( + HAS_APTSOURCES is True, reason="Only run test with python3-apt library is missing." +) def test_sourceslist_multiple_comps(): """ Test SourcesList when repo has multiple comps @@ -1126,6 +1129,9 @@ def test_sourceslist_multiple_comps(): assert source.dist == "focal-updates" +@pytest.mark.skipif( + HAS_APTSOURCES is True, reason="Only run test with python3-apt library is missing." +) @pytest.mark.parametrize( "repo_line", [ @@ -1142,16 +1148,15 @@ def test_sourceslist_architectures(repo_line): """ Test SourcesList when architectures is in repo """ - with patch.object(aptpkg, "HAS_APT", return_value=True): - with patch("salt.utils.files.fopen", mock_open(read_data=repo_line)): - with patch("pathlib.Path.is_file", side_effect=[True, False]): - sources = aptpkg.SourcesList() - for source in sources: - assert source.type == "deb" - assert source.uri == "http://archive.ubuntu.com/ubuntu/" - assert source.comps == ["main", "restricted"] - assert source.dist == "focal-updates" - if "," in repo_line: - assert source.architectures == ["amd64", "armel"] - else: - assert source.architectures == ["amd64"] + with patch("salt.utils.files.fopen", mock_open(read_data=repo_line)): + with patch("pathlib.Path.is_file", side_effect=[True, False]): + sources = aptpkg.SourcesList() + for source in sources: + assert source.type == "deb" + assert source.uri == "http://archive.ubuntu.com/ubuntu/" + assert source.comps == ["main", "restricted"] + assert source.dist == "focal-updates" + if "," in repo_line: + assert source.architectures == ["amd64", "armel"] + else: + assert source.architectures == ["amd64"] From ab122332c8865e691d40740d0c3a1beef5355a16 Mon Sep 17 00:00:00 2001 From: Megan Wilhite Date: Wed, 6 Oct 2021 07:42:47 -0400 Subject: [PATCH 09/66] remove extra space for arch --- salt/modules/aptpkg.py | 2 +- .../pytests/functional/states/test_pkgrepo.py | 20 +++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/salt/modules/aptpkg.py b/salt/modules/aptpkg.py index 3bf0c5b3660a..2b30ca7419b1 100644 --- a/salt/modules/aptpkg.py +++ b/salt/modules/aptpkg.py @@ -151,7 +151,7 @@ def repo_line(self): repo_line.append(self.type) if self.architectures: - repo_line.append("[arch={}] ".format(" ".join(self.architectures))) + repo_line.append("[arch={}]".format(" ".join(self.architectures))) repo_line = repo_line + [self.uri, self.dist, " ".join(self.comps)] if self.comment: diff --git a/tests/pytests/functional/states/test_pkgrepo.py b/tests/pytests/functional/states/test_pkgrepo.py index 013348e005f8..9decfe68720c 100644 --- a/tests/pytests/functional/states/test_pkgrepo.py +++ b/tests/pytests/functional/states/test_pkgrepo.py @@ -18,3 +18,23 @@ def test_adding_repo_file(states, tmp_path): with salt.utils.files.fopen(repo_file, "r") as fp: file_content = fp.read() assert file_content.strip() == repo_content + + +@pytest.mark.skipif( + not any([x for x in ["ubuntu", "debian"] if x in platform.platform()]), + reason="Test only for debian based platforms", +) +def test_adding_repo_file_arch(states, tmp_path): + """ + test adding a repo file using pkgrepo.managed + and setting architecture + """ + repo_file = tmp_path / "stable-binary.list" + repo_content = "deb [arch=amd64 ] http://www.deb-multimedia.org stable main" + ret = states.pkgrepo.managed(name=repo_content, file=repo_file, clean_file=True) + with salt.utils.files.fopen(repo_file, "r") as fp: + file_content = fp.read() + assert ( + file_content.strip() + == "deb [arch=amd64] http://www.deb-multimedia.org stable main" + ) From 6017b0c5021de063e234df23c8823460ba78f389 Mon Sep 17 00:00:00 2001 From: Megan Wilhite Date: Wed, 6 Oct 2021 08:53:40 -0400 Subject: [PATCH 10/66] convert pathlib to string for pkgrepo test --- tests/pytests/functional/states/test_pkgrepo.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/pytests/functional/states/test_pkgrepo.py b/tests/pytests/functional/states/test_pkgrepo.py index 9decfe68720c..82a6f093983f 100644 --- a/tests/pytests/functional/states/test_pkgrepo.py +++ b/tests/pytests/functional/states/test_pkgrepo.py @@ -12,7 +12,7 @@ def test_adding_repo_file(states, tmp_path): """ test adding a repo file using pkgrepo.managed """ - repo_file = tmp_path / "stable-binary.list" + repo_file = str(tmp_path / "stable-binary.list") repo_content = "deb http://www.deb-multimedia.org stable main" ret = states.pkgrepo.managed(name=repo_content, file=repo_file, clean_file=True) with salt.utils.files.fopen(repo_file, "r") as fp: @@ -29,7 +29,7 @@ def test_adding_repo_file_arch(states, tmp_path): test adding a repo file using pkgrepo.managed and setting architecture """ - repo_file = tmp_path / "stable-binary.list" + repo_file = str(tmp_path / "stable-binary.list") repo_content = "deb [arch=amd64 ] http://www.deb-multimedia.org stable main" ret = states.pkgrepo.managed(name=repo_content, file=repo_file, clean_file=True) with salt.utils.files.fopen(repo_file, "r") as fp: From 832764bcc3ae432bcdd92a81d42b0230b06ee952 Mon Sep 17 00:00:00 2001 From: Megan Wilhite Date: Thu, 7 Oct 2021 12:23:44 -0400 Subject: [PATCH 11/66] Use temporary files first then copy to sources files --- salt/modules/aptpkg.py | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/salt/modules/aptpkg.py b/salt/modules/aptpkg.py index 2b30ca7419b1..7ded2944c66b 100644 --- a/salt/modules/aptpkg.py +++ b/salt/modules/aptpkg.py @@ -17,6 +17,8 @@ import os import pathlib import re +import shutil +import tempfile import time from urllib.error import HTTPError from urllib.request import Request as _Request @@ -175,7 +177,7 @@ def _parse_sources(self, line): return False if repo_line[1].startswith("["): opts = re.search(r"\[.*\]", self.line).group(0).strip("[]") - repo_line = [x.strip("[]") for x in repo_line if x.strip("[]")] + repo_line = [x for x in (line.strip("[]") for line in repo_line) if x] for opt in opts.split(): if opt.startswith("arch"): self.architectures.extend(opt.split("=", 1)[1].split(",")) @@ -237,19 +239,17 @@ def save(self): write all of the sources from the list of sources to the file. """ - open_files = {} - try: + filemap = {} + with tempfile.TemporaryDirectory() as tmpdir: for source in self.list: - if source.file not in open_files: - open_files[ - source.file - ] = salt.utils.files.fopen( # pylint: disable=resource-leakage - source.file, "w" - ) - open_files[source.file].write(source.repo_line()) - finally: - for fp in open_files: - open_files[fp].close() + fname = pathlib.Path(tmpdir, pathlib.Path(source.file).name) + with salt.utils.files.fopen(fname, "a") as fp: + fp.write(source.repo_line()) + if source.file not in filemap: + filemap[source.file] = {"tmp": fname} + + for fp in filemap: + shutil.move(filemap[fp]["tmp"], fp) def _get_ppa_info_from_launchpad(owner_name, ppa_name): From 9815cbeed97ddd5c14e65a5a319827a678167e64 Mon Sep 17 00:00:00 2001 From: nicholasmhughes Date: Sat, 2 Oct 2021 11:06:08 -0400 Subject: [PATCH 12/66] fixes saltstack/salt#59182 fix handling of duplicate keys in rest_cherrypy data --- salt/netapi/rest_cherrypy/app.py | 12 +++++++++++- tests/unit/netapi/test_rest_cherrypy.py | 13 +++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/salt/netapi/rest_cherrypy/app.py b/salt/netapi/rest_cherrypy/app.py index 852efb97dac3..e5fd515f9313 100644 --- a/salt/netapi/rest_cherrypy/app.py +++ b/salt/netapi/rest_cherrypy/app.py @@ -966,8 +966,18 @@ def urlencoded_processor(entity): urlencoded = urlencoded.decode("utf-8") except (UnicodeDecodeError, AttributeError): pass + logger.debug("Raw request body: %s", urlencoded) cherrypy.serving.request.raw_body = urlencoded - cherrypy.serving.request.unserialized_data = dict(parse_qsl(urlencoded)) + unserialized_data = {} + for key, val in parse_qsl(urlencoded): + unserialized_data.setdefault(key, []).append(val) + for key, val in unserialized_data.items(): + if len(val) == 1: + unserialized_data[key] = val[0] + if len(val) == 0: + unserialized_data[key] = "" + cherrypy.serving.request.unserialized_data = unserialized_data + logger.debug("Unserialized request data: %s", unserialized_data) @process_request_body diff --git a/tests/unit/netapi/test_rest_cherrypy.py b/tests/unit/netapi/test_rest_cherrypy.py index 79e8914949d4..dc4a0f0e8753 100644 --- a/tests/unit/netapi/test_rest_cherrypy.py +++ b/tests/unit/netapi/test_rest_cherrypy.py @@ -51,6 +51,19 @@ def test_urlencoded_ctype(self): self.assertEqual(request.raw_body, raw) self.assertDictEqual(request.unserialized_data, data) + def test_urlencoded_multi_args(self): + multi_args = "arg=arg1&arg=arg2" + expected = {"arg": ["arg1", "arg2"]} + request, response = self.request( + "/", + method="POST", + body=multi_args, + headers=(("Content-type", "application/x-www-form-urlencoded"),), + ) + self.assertEqual(response.status, "200 OK") + self.assertEqual(request.raw_body, multi_args) + self.assertDictEqual(request.unserialized_data, expected) + def test_json_ctype(self): data = {"valid": "stuff"} request, response = self.request( From 64158e4e588295a605edced81de42e03273a71f5 Mon Sep 17 00:00:00 2001 From: nicholasmhughes Date: Sat, 2 Oct 2021 11:15:38 -0400 Subject: [PATCH 13/66] added changelog --- changelog/59182.fixed | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog/59182.fixed diff --git a/changelog/59182.fixed b/changelog/59182.fixed new file mode 100644 index 000000000000..8803c50189cd --- /dev/null +++ b/changelog/59182.fixed @@ -0,0 +1 @@ +fixed issue where multiple args to netapi were not preserved From d2ded59d3f672c319ee70354e169dfaf239fc85e Mon Sep 17 00:00:00 2001 From: nicholasmhughes Date: Thu, 7 Oct 2021 22:09:52 -0400 Subject: [PATCH 14/66] remove log messages to prevent leaks of sensitive info --- salt/netapi/rest_cherrypy/app.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/salt/netapi/rest_cherrypy/app.py b/salt/netapi/rest_cherrypy/app.py index e5fd515f9313..97b92714995d 100644 --- a/salt/netapi/rest_cherrypy/app.py +++ b/salt/netapi/rest_cherrypy/app.py @@ -966,7 +966,6 @@ def urlencoded_processor(entity): urlencoded = urlencoded.decode("utf-8") except (UnicodeDecodeError, AttributeError): pass - logger.debug("Raw request body: %s", urlencoded) cherrypy.serving.request.raw_body = urlencoded unserialized_data = {} for key, val in parse_qsl(urlencoded): @@ -977,7 +976,6 @@ def urlencoded_processor(entity): if len(val) == 0: unserialized_data[key] = "" cherrypy.serving.request.unserialized_data = unserialized_data - logger.debug("Unserialized request data: %s", unserialized_data) @process_request_body From d1587a3f4eed3980e8bf8b43e202a0d52d2e7262 Mon Sep 17 00:00:00 2001 From: "Gareth J. Greenaway" Date: Fri, 8 Oct 2021 15:46:39 -0700 Subject: [PATCH 15/66] Reverting changes in PR #60150. Updating installed and removed functions to return changes when test=True. --- salt/states/pkg.py | 25 ++++++++-- salt/states/service.py | 13 ----- tests/pytests/unit/states/test_pkg.py | 60 +++++++++++++++++++++++ tests/pytests/unit/states/test_service.py | 2 +- 4 files changed, 81 insertions(+), 19 deletions(-) diff --git a/salt/states/pkg.py b/salt/states/pkg.py index 8cf229bae8f4..f71f61e720b4 100644 --- a/salt/states/pkg.py +++ b/salt/states/pkg.py @@ -1822,12 +1822,17 @@ def installed( ) comment = [] + changes = {"installed": {}} if __opts__["test"]: if targets: if sources: - summary = ", ".join(targets) + _targets = targets else: - summary = ", ".join([_get_desired_pkg(x, targets) for x in targets]) + _targets = [_get_desired_pkg(x, targets) for x in targets] + summary = ", ".join(targets) + changes["installed"].update( + {x: {"new": "installed", "old": ""} for x in targets} + ) comment.append( "The following packages would be installed/updated: {}".format(summary) ) @@ -1836,6 +1841,9 @@ def installed( "The following packages would have their selection status " "changed from 'purge' to 'install': {}".format(", ".join(to_unpurge)) ) + changes["installed"].update( + {x: {"new": "installed", "old": ""} for x in to_unpurge} + ) if to_reinstall: # Add a comment for each package in to_reinstall with its # pkg.verify output @@ -1848,6 +1856,9 @@ def installed( reinstall_targets.append( _get_desired_pkg(reinstall_pkg, to_reinstall) ) + changes["installed"].update( + {x: {"new": "installed", "old": ""} for x in reinstall_targets} + ) msg = "The following packages would be reinstalled: " msg += ", ".join(reinstall_targets) comment.append(msg) @@ -1861,10 +1872,11 @@ def installed( "Package '{}' would be reinstalled because the " "following files have been altered:".format(pkgstr) ) + changes["installed"].update({reinstall_pkg: {}}) comment.append(_nested_output(altered_files[reinstall_pkg])) ret = { "name": name, - "changes": {}, + "changes": changes, "result": None, "comment": "\n".join(comment), } @@ -1872,7 +1884,6 @@ def installed( ret.setdefault("warnings", []).extend(warnings) return ret - changes = {"installed": {}} modified_hold = None not_modified_hold = None failed_hold = None @@ -2948,9 +2959,13 @@ def _uninstall( } if __opts__["test"]: + + _changes = {} + _changes.update({x: {"new": "{}d".format(action), "old": ""} for x in targets}) + return { "name": name, - "changes": {}, + "changes": _changes, "result": None, "comment": "The following packages will be {}d: {}.".format( action, ", ".join(targets) diff --git a/salt/states/service.py b/salt/states/service.py index e903032744ec..f9664e1f1fcc 100644 --- a/salt/states/service.py +++ b/salt/states/service.py @@ -508,19 +508,6 @@ def running(name, enable=None, sig=None, init_delay=None, **kwargs): ret.update(_enable(name, None, **kwargs)) elif enable is False and before_toggle_enable_status: ret.update(_disable(name, None, **kwargs)) - else: - if __opts__["test"]: - ret["result"] = None - ret["comment"] = "\n".join( - [ - _f - for _f in [ - "The service {} is set to restart".format(name), - unmask_ret["comment"], - ] - if _f - ] - ) return ret # Run the tests diff --git a/tests/pytests/unit/states/test_pkg.py b/tests/pytests/unit/states/test_pkg.py index 438dd9b121ae..7e667d36fd66 100644 --- a/tests/pytests/unit/states/test_pkg.py +++ b/tests/pytests/unit/states/test_pkg.py @@ -37,6 +37,15 @@ def pkgs(): } +@pytest.fixture(scope="module") +def list_pkgs(): + return { + "pkga": "1.0.1", + "pkgb": "1.0.2", + "pkgc": "1.0.3", + } + + def test_uptodate_with_changes(pkgs): """ Test pkg.uptodate with simulated changes @@ -518,3 +527,54 @@ def test_mod_aggregate(): } res = pkg.mod_aggregate(low, chunks, running) assert res == expected + + +def test_installed_with_changes_test_true(list_pkgs): + """ + Test pkg.installed with simulated changes + """ + + list_pkgs = MagicMock(return_value=list_pkgs) + + with patch.dict( + pkg.__salt__, + { + "pkg.list_pkgs": list_pkgs, + }, + ): + + expected = {"installed": {"dummy": {"new": "installed", "old": ""}}} + # Run state with test=true + with patch.dict(pkg.__opts__, {"test": True}): + ret = pkg.installed("dummy", test=True) + assert ret["result"] is None + assert ret["changes"] == expected + + +@pytest.mark.parametrize("action", ["removed", "purged"]) +def test_removed_purged_with_changes_test_true(list_pkgs, action): + """ + Test pkg.removed with simulated changes + """ + + list_pkgs = MagicMock(return_value=list_pkgs) + + mock_parse_targets = MagicMock(return_value=[{"pkga": None}, "repository"]) + + with patch.dict( + pkg.__salt__, + { + "pkg.list_pkgs": list_pkgs, + "pkg_resource.parse_targets": mock_parse_targets, + "pkg_resource.version_clean": MagicMock(return_value=None), + }, + ): + + expected = {"pkga": {"new": "{}".format(action), "old": ""}} + pkg_actions = {"removed": pkg.removed, "purged": pkg.purged} + + # Run state with test=true + with patch.dict(pkg.__opts__, {"test": True}): + ret = pkg_actions[action]("pkga", test=True) + assert ret["result"] is None + assert ret["changes"] == expected diff --git a/tests/pytests/unit/states/test_service.py b/tests/pytests/unit/states/test_service.py index 960eea5d6ce7..b1e184b8354b 100644 --- a/tests/pytests/unit/states/test_service.py +++ b/tests/pytests/unit/states/test_service.py @@ -244,7 +244,7 @@ def test_running(): with patch.dict(service.__opts__, {"test": True}): with patch.dict(service.__salt__, {"service.status": tmock}): - assert service.running("salt") == ret[10] + assert service.running("salt") == ret[5] with patch.dict(service.__salt__, {"service.status": fmock}): assert service.running("salt") == ret[3] From 3f5a639943d519d45c5e74692b3b877cb0c378e2 Mon Sep 17 00:00:00 2001 From: "Gareth J. Greenaway" Date: Fri, 8 Oct 2021 16:02:46 -0700 Subject: [PATCH 16/66] Adding changelog. --- changelog/60995.fixed | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog/60995.fixed diff --git a/changelog/60995.fixed b/changelog/60995.fixed new file mode 100644 index 000000000000..258127e49e23 --- /dev/null +++ b/changelog/60995.fixed @@ -0,0 +1 @@ +Reverting changes in PR #60150. Updating installed and removed functions to return changes when test=True. From 3525300cb72f01ba91b6951244b4756362ea799f Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Thu, 7 Oct 2021 17:22:37 -0700 Subject: [PATCH 17/66] Add a test and fix for extra-filerefs --- salt/client/ssh/__init__.py | 1 + tests/pytests/integration/ssh/test_state.py | 47 +++++++++++++++++++++ 2 files changed, 48 insertions(+) diff --git a/salt/client/ssh/__init__.py b/salt/client/ssh/__init__.py index d554f31b63fc..9c0d289a4b13 100644 --- a/salt/client/ssh/__init__.py +++ b/salt/client/ssh/__init__.py @@ -1133,6 +1133,7 @@ def run_wfunc(self): opts_pkg["_ssh_version"] = self.opts["_ssh_version"] opts_pkg["thin_dir"] = self.opts["thin_dir"] opts_pkg["master_tops"] = self.opts["master_tops"] + opts_pkg["extra_filerefs"] = self.opts.get("extra_filerefs", "") opts_pkg["__master_opts__"] = self.context["master_opts"] if "known_hosts_file" in self.opts: opts_pkg["known_hosts_file"] = self.opts["known_hosts_file"] diff --git a/tests/pytests/integration/ssh/test_state.py b/tests/pytests/integration/ssh/test_state.py index 88dc43a1ddaa..58330a5dd882 100644 --- a/tests/pytests/integration/ssh/test_state.py +++ b/tests/pytests/integration/ssh/test_state.py @@ -45,3 +45,50 @@ def test_state_with_import(salt_ssh_cli, state_tree): ret = salt_ssh_cli.run("state.sls", "test") assert ret.exitcode == 0 assert ret.json + + +@pytest.fixture +def nested_state_tree(base_env_state_tree_root_dir, tmpdir): + top_file = """ + base: + 'localhost': + - basic + '127.0.0.1': + - basic + """ + state_file = """ + /{}/file.txt: + file.managed: + - source: salt://foo/file.jinja + - template: jinja + """.format( + tmpdir + ) + file_jinja = """ + {% from 'foo/map.jinja' import comment %}{{ comment }} + """ + map_file = """ + {% set comment = "blah blah" %} + """ + statedir = base_env_state_tree_root_dir / "foo" + top_tempfile = pytest.helpers.temp_file( + "top.sls", top_file, base_env_state_tree_root_dir + ) + map_tempfile = pytest.helpers.temp_file("map.jinja", map_file, statedir) + file_tempfile = pytest.helpers.temp_file("file.jinja", file_jinja, statedir) + state_tempfile = pytest.helpers.temp_file("init.sls", state_file, statedir) + + with top_tempfile, map_tempfile, state_tempfile, file_tempfile: + yield + + +@pytest.mark.slow_test +def test_state_with_import_from_dir(salt_ssh_cli, nested_state_tree): + """ + verify salt-ssh can use imported map files in states + """ + ret = salt_ssh_cli.run( + "--extra-filerefs=salt://foo/map.jinja", "state.apply", "foo" + ) + assert ret.exitcode == 0 + assert ret.json From 711d24d3b43614153d55a75aae71e7079b8decd6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Su=C3=A1rez=20Hern=C3=A1ndez?= Date: Thu, 30 Sep 2021 11:06:09 +0100 Subject: [PATCH 18/66] Do not break master_tops for minion with version lower to 3003 --- salt/master.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/salt/master.py b/salt/master.py index 00c51f957ecc..d3e2a8373e4c 100644 --- a/salt/master.py +++ b/salt/master.py @@ -1170,6 +1170,7 @@ class AESFuncs(TransportMethods): "_dir_list", "_symlink_list", "_file_envs", + "_ext_nodes", ) def __init__(self, opts): @@ -1368,6 +1369,9 @@ def _master_tops(self, load): return {} return self.masterapi._master_tops(load, skip_verify=True) + # Needed so older minions can request master_tops + _ext_nodes = _master_tops + def _master_opts(self, load): """ Return the master options to the minion From bf0b71fdd33e9064995f69596d8c2359b9360b40 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Su=C3=A1rez=20Hern=C3=A1ndez?= Date: Thu, 30 Sep 2021 11:47:11 +0100 Subject: [PATCH 19/66] Add changelog file --- changelog/60980.fixed | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog/60980.fixed diff --git a/changelog/60980.fixed b/changelog/60980.fixed new file mode 100644 index 000000000000..ef346eb32f8e --- /dev/null +++ b/changelog/60980.fixed @@ -0,0 +1 @@ +Do not break master_tops for minion with version lower to 3003 From a572c6b5b782166636f5287fbd7fc5c1589c4c38 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Su=C3=A1rez=20Hern=C3=A1ndez?= Date: Fri, 8 Oct 2021 12:20:37 +0100 Subject: [PATCH 20/66] Add extra comment to clarify discussion --- salt/master.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/salt/master.py b/salt/master.py index d3e2a8373e4c..8a94c4403d03 100644 --- a/salt/master.py +++ b/salt/master.py @@ -1170,7 +1170,7 @@ class AESFuncs(TransportMethods): "_dir_list", "_symlink_list", "_file_envs", - "_ext_nodes", + "_ext_nodes", # To be removed in 3006 (Sulfur) #60980 ) def __init__(self, opts): @@ -1370,6 +1370,7 @@ def _master_tops(self, load): return self.masterapi._master_tops(load, skip_verify=True) # Needed so older minions can request master_tops + # To be removed in 3006 (Sulfur) #60980 _ext_nodes = _master_tops def _master_opts(self, load): From 839730f2bee843b5a5abeae608bad781af65a05e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pablo=20Su=C3=A1rez=20Hern=C3=A1ndez?= Date: Fri, 8 Oct 2021 12:22:25 +0100 Subject: [PATCH 21/66] Update changelog file --- changelog/60980.fixed | 1 + 1 file changed, 1 insertion(+) diff --git a/changelog/60980.fixed b/changelog/60980.fixed index ef346eb32f8e..ba9be239b175 100644 --- a/changelog/60980.fixed +++ b/changelog/60980.fixed @@ -1 +1,2 @@ Do not break master_tops for minion with version lower to 3003 +This is going to be deprecated in Salt 3006 (Sulfur) From 14eeae6250385d6ba79640add55e85ee49e02414 Mon Sep 17 00:00:00 2001 From: Megan Wilhite Date: Mon, 11 Oct 2021 10:46:27 -0400 Subject: [PATCH 22/66] Add deprecated changelog --- changelog/60980.deprecated | 1 + changelog/60980.fixed | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) create mode 100644 changelog/60980.deprecated diff --git a/changelog/60980.deprecated b/changelog/60980.deprecated new file mode 100644 index 000000000000..6b9aed55285f --- /dev/null +++ b/changelog/60980.deprecated @@ -0,0 +1 @@ +The _ext_nodes alias to the master_tops function was added back in 3004 to maintain backwards compatibility with older supported versions. This alias will now be removed in 3006. This change will break Master and Minion communication compatibility with Salt minions running versions 3003 and lower. diff --git a/changelog/60980.fixed b/changelog/60980.fixed index ba9be239b175..7227e04988d1 100644 --- a/changelog/60980.fixed +++ b/changelog/60980.fixed @@ -1,2 +1,2 @@ Do not break master_tops for minion with version lower to 3003 -This is going to be deprecated in Salt 3006 (Sulfur) +This is going to be removed in Salt 3006 (Sulfur) From 124efb7c150474b96f382133be4dc814d5aecc53 Mon Sep 17 00:00:00 2001 From: Pedro Algarvio Date: Thu, 7 Oct 2021 18:07:53 +0100 Subject: [PATCH 23/66] Assert that the command didn't finish Refs https://github.com/saltstack/salt/pull/60972 --- tests/pytests/integration/cli/test_salt.py | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/tests/pytests/integration/cli/test_salt.py b/tests/pytests/integration/cli/test_salt.py index e5de16d6d423..733c2376181c 100644 --- a/tests/pytests/integration/cli/test_salt.py +++ b/tests/pytests/integration/cli/test_salt.py @@ -13,7 +13,7 @@ import pytest import salt.defaults.exitcodes import salt.utils.path -from saltfactories.utils.processes import ProcessResult +from saltfactories.utils.processes import ProcessResult, terminate_process log = logging.getLogger(__name__) @@ -140,9 +140,15 @@ def test_interrupt_on_long_running_job(salt_cli, salt_master, salt_minion): Refer to https://github.com/saltstack/salt/issues/60963 for more details """ + # Ensure test.sleep is working as supposed + start = time.time() ret = salt_cli.run("test.sleep", "1", minion_tgt=salt_minion.id) + stop = time.time() assert ret.exitcode == 0 assert ret.json is True + assert stop - start > 1, "The command should have taken more than 1 second" + + # Now the real test terminal_stdout = tempfile.SpooledTemporaryFile(512000, buffering=0) terminal_stderr = tempfile.SpooledTemporaryFile(512000, buffering=0) cmdline = [ @@ -160,17 +166,19 @@ def test_interrupt_on_long_running_job(salt_cli, salt_master, salt_minion): stderr=terminal_stderr, universal_newlines=True, ) + start = time.time() try: # Make sure it actually starts proc.wait(1) except subprocess.TimeoutExpired: pass else: + terminate_process(proc.pid, kill_children=True) pytest.fail("The test process failed to start") time.sleep(2) # Send CTRL-C to the process - proc.send_signal(signal.SIGINT) + os.kill(proc.pid, signal.SIGINT) with proc: # Wait for the process to terminate, to avoid zombies. # Shouldn't really take the 30 seconds @@ -179,6 +187,7 @@ def test_interrupt_on_long_running_job(salt_cli, salt_master, salt_minion): proc.poll() # This call shouldn't really be necessary proc.communicate() + stop = time.time() terminal_stdout.flush() terminal_stdout.seek(0) @@ -205,5 +214,11 @@ def test_interrupt_on_long_running_job(salt_cli, salt_master, salt_minion): terminal_stderr.close() ret = ProcessResult(proc.returncode, stdout, stderr, cmdline=proc.args) log.debug(ret) + # If the minion ID is on stdout it means that the command finished and wasn't terminated + assert ( + salt_minion.id not in ret.stdout + ), "The command wasn't actually terminated. Took {} seconds.".format( + round(stop - start, 2) + ) assert "Exiting gracefully on Ctrl-c" in ret.stderr assert "This job's jid is" in ret.stderr From 5032f4cd852abe351c457b2d2c1b909101b9cb6b Mon Sep 17 00:00:00 2001 From: Pedro Algarvio Date: Sat, 9 Oct 2021 06:27:55 +0100 Subject: [PATCH 24/66] Always restore signals, even when exceptions occur --- salt/utils/process.py | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/salt/utils/process.py b/salt/utils/process.py index 2f085e004b83..8e003b19bc25 100644 --- a/salt/utils/process.py +++ b/salt/utils/process.py @@ -1182,14 +1182,15 @@ def default_signals(*signals): else: old_signals[signum] = saved_signal - # Do whatever is needed with the reset signals - yield - - # Restore signals - for signum in old_signals: - signal.signal(signum, old_signals[signum]) - - del old_signals + try: + # Do whatever is needed with the reset signals + yield + finally: + # Restore signals + for signum in old_signals: + signal.signal(signum, old_signals[signum]) + + del old_signals class SubprocessList: From 5a97c37b11d4e0b576b0cbce69ee7820269b3a50 Mon Sep 17 00:00:00 2001 From: Pedro Algarvio Date: Sat, 9 Oct 2021 06:28:50 +0100 Subject: [PATCH 25/66] Reset signal handlers before starting the process --- tests/pytests/integration/cli/test_salt.py | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/tests/pytests/integration/cli/test_salt.py b/tests/pytests/integration/cli/test_salt.py index 733c2376181c..9ecc6c7f5295 100644 --- a/tests/pytests/integration/cli/test_salt.py +++ b/tests/pytests/integration/cli/test_salt.py @@ -13,6 +13,7 @@ import pytest import salt.defaults.exitcodes import salt.utils.path +from salt.utils.process import default_signals from saltfactories.utils.processes import ProcessResult, terminate_process log = logging.getLogger(__name__) @@ -159,13 +160,19 @@ def test_interrupt_on_long_running_job(salt_cli, salt_master, salt_minion): "test.sleep", "30", ] - proc = subprocess.Popen( - cmdline, - shell=False, - stdout=terminal_stdout, - stderr=terminal_stderr, - universal_newlines=True, - ) + + # When the whole test suite runs, the default SIGTERM and SIGINT signals + # are set to SIG_IGN, ignore. The `with` line below ensures that the default + # handlers are set when starting the process. + # Failure to do so, will make the test fail. + with default_signals(signal.SIGINT, signal.SIGTERM): + proc = subprocess.Popen( + cmdline, + shell=False, + stdout=terminal_stdout, + stderr=terminal_stderr, + universal_newlines=True, + ) start = time.time() try: # Make sure it actually starts From ed7c74ac41288e0eb26135cb75b00e4b06e706e3 Mon Sep 17 00:00:00 2001 From: Pedro Algarvio Date: Sat, 9 Oct 2021 17:55:17 +0100 Subject: [PATCH 26/66] Make sure that the `ProcessManager` doesn't always ignore signals --- salt/cli/api.py | 7 ++-- salt/cli/daemons.py | 7 ++-- salt/client/netapi.py | 7 ++-- salt/master.py | 7 ++-- salt/minion.py | 7 ++-- salt/utils/process.py | 38 +++++++++++++--------- tests/pytests/integration/cli/test_salt.py | 35 ++++++++++++-------- 7 files changed, 55 insertions(+), 53 deletions(-) diff --git a/salt/cli/api.py b/salt/cli/api.py index 62a39ee6ab96..1669e47492bc 100644 --- a/salt/cli/api.py +++ b/salt/cli/api.py @@ -77,10 +77,7 @@ def shutdown(self, exitcode=0, exitmsg=None): exitmsg = msg.strip() super().shutdown(exitcode, exitmsg) - def _handle_signals(self, signum, sigframe): # pylint: disable=unused-argument + def _handle_signals(self, signum, sigframe): # escalate signal to the process manager processes - self.api.process_manager.stop_restarting() - self.api.process_manager.send_signal_to_processes(signum) - # kill any remaining processes - self.api.process_manager.kill_children() + self.api.process_manager._handle_signals(signum, sigframe) super()._handle_signals(signum, sigframe) diff --git a/salt/cli/daemons.py b/salt/cli/daemons.py index eacdc34e5ad0..f967b0d1f11d 100644 --- a/salt/cli/daemons.py +++ b/salt/cli/daemons.py @@ -126,13 +126,10 @@ class Master( Creates a master server """ - def _handle_signals(self, signum, sigframe): # pylint: disable=unused-argument + def _handle_signals(self, signum, sigframe): if hasattr(self.master, "process_manager"): # escalate signal to the process manager processes - self.master.process_manager.stop_restarting() - self.master.process_manager.send_signal_to_processes(signum) - # kill any remaining processes - self.master.process_manager.kill_children() + self.master.process_manager._handle_signals(signum, sigframe) super()._handle_signals(signum, sigframe) def prepare(self): diff --git a/salt/client/netapi.py b/salt/client/netapi.py index 57e8fc69b1ab..15a5910fa36f 100644 --- a/salt/client/netapi.py +++ b/salt/client/netapi.py @@ -71,9 +71,6 @@ def run(self): self.process_manager.run() - def _handle_signals(self, signum, sigframe): # pylint: disable=unused-argument + def _handle_signals(self, signum, sigframe): # escalate the signals to the process manager - self.process_manager.stop_restarting() - self.process_manager.send_signal_to_processes(signum) - # kill any remaining processes - self.process_manager.kill_children() + self.process_manager._handle_signals(signum, sigframe) diff --git a/salt/master.py b/salt/master.py index 8a94c4403d03..64940119ee6a 100644 --- a/salt/master.py +++ b/salt/master.py @@ -785,12 +785,9 @@ def start(self): self.process_manager.run() - def _handle_signals(self, signum, sigframe): # pylint: disable=unused-argument + def _handle_signals(self, signum, sigframe): # escalate the signals to the process manager - self.process_manager.stop_restarting() - self.process_manager.send_signal_to_processes(signum) - # kill any remaining processes - self.process_manager.kill_children() + self.process_manager._handle_signals(signum, sigframe) time.sleep(1) sys.exit(0) diff --git a/salt/minion.py b/salt/minion.py index 9aaec23b1c99..f4e80557d051 100644 --- a/salt/minion.py +++ b/salt/minion.py @@ -1319,13 +1319,10 @@ def __init__( # No custom signal handling was added, install our own signal.signal(signal.SIGTERM, self._handle_signals) - def _handle_signals(self, signum, sigframe): # pylint: disable=unused-argument + def _handle_signals(self, signum, sigframe): self._running = False # escalate the signals to the process manager - self.process_manager.stop_restarting() - self.process_manager.send_signal_to_processes(signum) - # kill any remaining processes - self.process_manager.kill_children() + self.process_manager._handle_signals(signum, sigframe) time.sleep(1) sys.exit(0) diff --git a/salt/utils/process.py b/salt/utils/process.py index 8e003b19bc25..7bf320adc737 100644 --- a/salt/utils/process.py +++ b/salt/utils/process.py @@ -648,10 +648,10 @@ def run(self, asynchronous=False): # make sure to kill the subprocesses if the parent is killed if signal.getsignal(signal.SIGTERM) is signal.SIG_DFL: # There are no SIGTERM handlers installed, install ours - signal.signal(signal.SIGTERM, self.kill_children) + signal.signal(signal.SIGTERM, self._handle_signals) if signal.getsignal(signal.SIGINT) is signal.SIG_DFL: # There are no SIGINT handlers installed, install ours - signal.signal(signal.SIGINT, self.kill_children) + signal.signal(signal.SIGINT, self._handle_signals) while True: log.trace("Process manager iteration") @@ -691,19 +691,6 @@ def kill_children(self, *args, **kwargs): """ Kill all of the children """ - # first lets reset signal handlers to default one to prevent running this twice - signal.signal(signal.SIGTERM, signal.SIG_IGN) - signal.signal(signal.SIGINT, signal.SIG_IGN) - - # check that this is the correct process, children inherit this - # handler, if we are in a child lets just run the original handler - if os.getpid() != self._pid: - if callable(self._sigterm_handler): - return self._sigterm_handler(*args) - elif self._sigterm_handler is not None: - return signal.default_int_handler(signal.SIGTERM)(*args) - else: - return if salt.utils.platform.is_windows(): if multiprocessing.current_process().name != "MainProcess": # Since the main process will kill subprocesses by tree, @@ -827,6 +814,27 @@ def terminate(self): self.send_signal_to_processes(signal.SIGTERM) self.kill_children() + def _handle_signals(self, *args, **kwargs): + # first lets reset signal handlers to default one to prevent running this twice + signal.signal(signal.SIGTERM, signal.SIG_IGN) + signal.signal(signal.SIGINT, signal.SIG_IGN) + + self.stop_restarting() + self.send_signal_to_processes(signal.SIGTERM) + + # check that this is the correct process, children inherit this + # handler, if we are in a child lets just run the original handler + if os.getpid() != self._pid: + if callable(self._sigterm_handler): + return self._sigterm_handler(*args) + elif self._sigterm_handler is not None: + return signal.default_int_handler(signal.SIGTERM)(*args) + else: + return + + # Terminate child processes + self.kill_children(*args, **kwargs) + class Process(multiprocessing.Process): """ diff --git a/tests/pytests/integration/cli/test_salt.py b/tests/pytests/integration/cli/test_salt.py index 9ecc6c7f5295..edb1436e3928 100644 --- a/tests/pytests/integration/cli/test_salt.py +++ b/tests/pytests/integration/cli/test_salt.py @@ -13,7 +13,6 @@ import pytest import salt.defaults.exitcodes import salt.utils.path -from salt.utils.process import default_signals from saltfactories.utils.processes import ProcessResult, terminate_process log = logging.getLogger(__name__) @@ -161,18 +160,28 @@ def test_interrupt_on_long_running_job(salt_cli, salt_master, salt_minion): "30", ] - # When the whole test suite runs, the default SIGTERM and SIGINT signals - # are set to SIG_IGN, ignore. The `with` line below ensures that the default - # handlers are set when starting the process. - # Failure to do so, will make the test fail. - with default_signals(signal.SIGINT, signal.SIGTERM): - proc = subprocess.Popen( - cmdline, - shell=False, - stdout=terminal_stdout, - stderr=terminal_stderr, - universal_newlines=True, - ) + # If this test starts failing, commend the following block of code + proc = subprocess.Popen( + cmdline, + shell=False, + stdout=terminal_stdout, + stderr=terminal_stderr, + universal_newlines=True, + ) + # and uncomment the following block of code + + # with default_signals(signal.SIGINT, signal.SIGTERM): + # proc = subprocess.Popen( + # cmdline, + # shell=False, + # stdout=terminal_stdout, + # stderr=terminal_stderr, + # universal_newlines=True, + # ) + + # What this means is that something in salt or the test suite is setting + # the SIGTERM and SIGINT signals to SIG_IGN, ignore. + # Check which line of code is doing that and fix it start = time.time() try: # Make sure it actually starts From 98f679ff2660411cefa24dd4f38f11b2b5ec4e5b Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Sat, 9 Oct 2021 14:16:05 -0700 Subject: [PATCH 27/66] Provide valid default value for bootstrap_delay --- changelog/61005.fixed | 1 + salt/config/__init__.py | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) create mode 100644 changelog/61005.fixed diff --git a/changelog/61005.fixed b/changelog/61005.fixed new file mode 100644 index 000000000000..3f108ab0fbb1 --- /dev/null +++ b/changelog/61005.fixed @@ -0,0 +1 @@ +Set default 'bootstrap_delay' to 0 diff --git a/salt/config/__init__.py b/salt/config/__init__.py index 9cd8bbaeb5fd..97a7fce2f006 100644 --- a/salt/config/__init__.py +++ b/salt/config/__init__.py @@ -1662,7 +1662,7 @@ def _gather_buffer_space(): "log_granular_levels": {}, "log_rotate_max_bytes": 0, "log_rotate_backup_count": 0, - "bootstrap_delay": None, + "bootstrap_delay": 0, "cache": "localfs", } ) From dddde00be62cad81a4817d510c23eaeba58774d7 Mon Sep 17 00:00:00 2001 From: Alyssa Rock Date: Thu, 7 Oct 2021 12:33:53 -0600 Subject: [PATCH 28/66] Update changelog for 3004 --- CHANGELOG.md | 9 +++++---- changelog/60391.fixed | 1 - changelog/60963.fixed | 1 - 3 files changed, 5 insertions(+), 6 deletions(-) delete mode 100644 changelog/60391.fixed delete mode 100644 changelog/60963.fixed diff --git a/CHANGELOG.md b/CHANGELOG.md index 8a591b6f0549..a4df5edbb943 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,7 @@ Versions are `MAJOR.PATCH`. # Changelog -Salt 3004 (2021-09-28) +Salt 3004 (2021-10-07) ====================== @@ -42,6 +42,7 @@ Changed Fixed ----- +- Handle signals and properly exit, instead of raising exceptions. (#60391, #60963) - Redirect imports of ``salt.ext.six`` to ``six`` (#60966) - Surface strerror to user state instead of returning false (#20789) - Fixing _get_envs() to preserve the order of pillar_roots. _get_envs() returned pillar_roots in a non-deterministic order. (#24501) @@ -305,7 +306,7 @@ Fixed valid if Powershell 7 is installed on the system. (#58598) - Fixed the zabbix.host_create call on zabbix_host.present to include the optional parameter visible_name. Now working as documented. (#58602) -- Fixed some bugs to allow zabbix_host.present to update a host already +- Fixed some bugs to allow zabbix_host.present to update a host already existent on Zabbix server: - Added checks before "pop" the elements "bulk" and "details" from @@ -313,8 +314,8 @@ Fixed didn't works with Zabbix >= 5.0 - Fixed the "inventory" comparison. It failed when both current and new inventory were missing. - - Rewrite of the update_interfaces routine to really "update" the - interfaces and not trying to delete and recreate all interfaces, + - Rewrite of the update_interfaces routine to really "update" the + interfaces and not trying to delete and recreate all interfaces, which almost always gives errors as interfaces with linked items can't be deleted. (#58603) - Added the "details" mandatory object with the properly default values diff --git a/changelog/60391.fixed b/changelog/60391.fixed deleted file mode 100644 index 97a11dd9241b..000000000000 --- a/changelog/60391.fixed +++ /dev/null @@ -1 +0,0 @@ -Handle signals and properly exit, instead of raising exceptions. diff --git a/changelog/60963.fixed b/changelog/60963.fixed deleted file mode 100644 index 97a11dd9241b..000000000000 --- a/changelog/60963.fixed +++ /dev/null @@ -1 +0,0 @@ -Handle signals and properly exit, instead of raising exceptions. From 3345c1c657061e3194b22b96d47a22e145b419d0 Mon Sep 17 00:00:00 2001 From: Alyssa Rock Date: Mon, 11 Oct 2021 16:28:09 -0600 Subject: [PATCH 29/66] Update changelog and release notes for 3004 --- CHANGELOG.md | 9 +++++++-- changelog/59182.fixed | 1 - changelog/60971.fixed | 1 - changelog/60980.deprecated | 1 - changelog/60980.fixed | 2 -- changelog/60995.fixed | 1 - doc/topics/releases/3004.rst | 7 +++++++ 7 files changed, 14 insertions(+), 8 deletions(-) delete mode 100644 changelog/59182.fixed delete mode 100644 changelog/60971.fixed delete mode 100644 changelog/60980.deprecated delete mode 100644 changelog/60980.fixed delete mode 100644 changelog/60995.fixed diff --git a/CHANGELOG.md b/CHANGELOG.md index a4df5edbb943..9ab90a98a266 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,10 +7,9 @@ Versions are `MAJOR.PATCH`. # Changelog -Salt 3004 (2021-10-07) +Salt 3004 (2021-10-11) ====================== - Removed ------- @@ -24,6 +23,7 @@ Removed Deprecated ---------- +- The _ext_nodes alias to the master_tops function was added back in 3004 to maintain backwards compatibility with older supported versions. This alias will now be removed in 3006. This change will break Master and Minion communication compatibility with Salt minions running versions 3003 and lower. (#60980) - utils/boto3_elasticsearch is no longer needed (#59882) - Changed "manufacture" grain to "manufacturer" for Solaris on SPARC to unify the name across all platforms. The old "manufacture" grain is now deprecated and will be removed in Sulfur (#60511) - Deprecate `salt.payload.Serial` (#60953) @@ -42,6 +42,11 @@ Changed Fixed ----- +- Fixed issue where multiple args to netapi were not preserved (#59182) +- Handle all repo formats in the aptpkg module. (#60971) +- Do not break master_tops for minion with version lower to 3003 + This is going to be removed in Salt 3006 (Sulfur) (#60980) +- Reverting changes in PR #60150. Updating installed and removed functions to return changes when test=True. (#60995) - Handle signals and properly exit, instead of raising exceptions. (#60391, #60963) - Redirect imports of ``salt.ext.six`` to ``six`` (#60966) - Surface strerror to user state instead of returning false (#20789) diff --git a/changelog/59182.fixed b/changelog/59182.fixed deleted file mode 100644 index 8803c50189cd..000000000000 --- a/changelog/59182.fixed +++ /dev/null @@ -1 +0,0 @@ -fixed issue where multiple args to netapi were not preserved diff --git a/changelog/60971.fixed b/changelog/60971.fixed deleted file mode 100644 index 4bac7bdf3e48..000000000000 --- a/changelog/60971.fixed +++ /dev/null @@ -1 +0,0 @@ -Handle all repo formats in the aptpkg module. diff --git a/changelog/60980.deprecated b/changelog/60980.deprecated deleted file mode 100644 index 6b9aed55285f..000000000000 --- a/changelog/60980.deprecated +++ /dev/null @@ -1 +0,0 @@ -The _ext_nodes alias to the master_tops function was added back in 3004 to maintain backwards compatibility with older supported versions. This alias will now be removed in 3006. This change will break Master and Minion communication compatibility with Salt minions running versions 3003 and lower. diff --git a/changelog/60980.fixed b/changelog/60980.fixed deleted file mode 100644 index 7227e04988d1..000000000000 --- a/changelog/60980.fixed +++ /dev/null @@ -1,2 +0,0 @@ -Do not break master_tops for minion with version lower to 3003 -This is going to be removed in Salt 3006 (Sulfur) diff --git a/changelog/60995.fixed b/changelog/60995.fixed deleted file mode 100644 index 258127e49e23..000000000000 --- a/changelog/60995.fixed +++ /dev/null @@ -1 +0,0 @@ -Reverting changes in PR #60150. Updating installed and removed functions to return changes when test=True. diff --git a/doc/topics/releases/3004.rst b/doc/topics/releases/3004.rst index f311a35f307d..6aabb35179ef 100644 --- a/doc/topics/releases/3004.rst +++ b/doc/topics/releases/3004.rst @@ -55,6 +55,7 @@ Removed Deprecated ========== +- The _ext_nodes alias to the master_tops function was added back in 3004 to maintain backwards compatibility with older supported versions. This alias will now be removed in 3006. This change will break Master and Minion communication compatibility with Salt minions running versions 3003 and lower. (#60980) - utils/boto3_elasticsearch is no longer needed (#59882) - Changed "manufacture" grain to "manufacturer" for Solaris on SPARC to unify the name across all platforms. The old "manufacture" grain is now deprecated and will be removed in Sulfur (#60511) - Deprecate ``salt.payload.Serial`` (#60953) @@ -73,6 +74,12 @@ Changed Fixed ===== +- Fixed issue where multiple args to netapi were not preserved (#59182) +- Handle all repo formats in the aptpkg module. (#60971) +- Do not break master_tops for minion with version lower to 3003 + This is going to be removed in Salt 3006 (Sulfur) (#60980) +- Reverting changes in PR #60150. Updating installed and removed functions to return changes when test=True. (#60995) +- Handle signals and properly exit, instead of raising exceptions. (#60391, #60963) - Redirect imports of ``salt.ext.six`` to ``six`` (#60966) - Surface strerror to user state instead of returning false (#20789) - Fixing _get_envs() to preserve the order of pillar_roots. _get_envs() returned pillar_roots in a non-deterministic order. (#24501) From bac2d1e6aeea8f12dd1712207b87ea2695f35b7c Mon Sep 17 00:00:00 2001 From: Megan Wilhite Date: Tue, 12 Oct 2021 07:00:29 -0400 Subject: [PATCH 30/66] Add PR 61020 to changelog --- CHANGELOG.md | 1 + changelog/61005.fixed | 1 - doc/topics/releases/3004.rst | 1 + 3 files changed, 2 insertions(+), 1 deletion(-) delete mode 100644 changelog/61005.fixed diff --git a/CHANGELOG.md b/CHANGELOG.md index 9ab90a98a266..9aab14560448 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,6 +42,7 @@ Changed Fixed ----- +- Set default 'bootstrap_delay' to 0 (#61005) - Fixed issue where multiple args to netapi were not preserved (#59182) - Handle all repo formats in the aptpkg module. (#60971) - Do not break master_tops for minion with version lower to 3003 diff --git a/changelog/61005.fixed b/changelog/61005.fixed deleted file mode 100644 index 3f108ab0fbb1..000000000000 --- a/changelog/61005.fixed +++ /dev/null @@ -1 +0,0 @@ -Set default 'bootstrap_delay' to 0 diff --git a/doc/topics/releases/3004.rst b/doc/topics/releases/3004.rst index 6aabb35179ef..9bb62d67607e 100644 --- a/doc/topics/releases/3004.rst +++ b/doc/topics/releases/3004.rst @@ -74,6 +74,7 @@ Changed Fixed ===== +- Set default 'bootstrap_delay' to 0 (#61005) - Fixed issue where multiple args to netapi were not preserved (#59182) - Handle all repo formats in the aptpkg module. (#60971) - Do not break master_tops for minion with version lower to 3003 From 9a7310811decefac075118af4c4cc587404a61ce Mon Sep 17 00:00:00 2001 From: krionbsd Date: Fri, 19 Nov 2021 10:55:46 +0100 Subject: [PATCH 31/66] Change MD5 to SHA256 fingerprint for new github.com fingerprint --- tests/integration/files/ssh/known_hosts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/integration/files/ssh/known_hosts b/tests/integration/files/ssh/known_hosts index b46ae35a6b70..aa02480ca8a3 100644 --- a/tests/integration/files/ssh/known_hosts +++ b/tests/integration/files/ssh/known_hosts @@ -1 +1,3 @@ |1|muzcBqgq7+ByUY7aLICytOff8UI=|rZ1JBNlIOqRnwwsJl9yP+xMxgf8= ssh-rsa AAAAB3NzaC1yc2EAAAABIwAAAQEAq2A7hRGmdnm9tUDbO9IDSwBK6TbQa+PXYPCPy6rbTrTtw7PHkccKrpp0yVhp5HdEIcKr6pLlVDBfOLX9QUsyCOV0wzfjIJNlGEYsdlLJizHhbn2mUjvSAHQqZETYP81eFzLQNnPHt4EVVUh7VfDESU84KezmD5QlWpXLmvU31/yMf+Se8xhHTvKSCZIFImWwoG6mbUoWf9nzpIoaSjB+weqqUUmpaaasXVal72J+UX2B+2RPW3RcT0eOzQgqlJL3RKrTJvdsjE3JEAvGq3lGHSZXy28G3skua2SmVi/w4yCE6gbODqnTWlg7+wC604ydGXA8VJiS5ap43JXiUFFAaQ== +github.com ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBEmKSENjQEezOmxkZMy7opKgwFB9nkt5YRrYMjNuG5N87uRgg6CLrbo5wAdT/y6v0mKV0U2w0WZ2YB/++Tpockg= +github.com ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIOMqqnkVzrm0SdG6UOoqKLsabgH5C9okWi0dh2l9GKJl From 1d3e2841ddaf54a6ad226c8a6adbd602bdff0113 Mon Sep 17 00:00:00 2001 From: krionbsd Date: Fri, 19 Nov 2021 18:36:29 +0100 Subject: [PATCH 32/66] Check only ssh-rsa encyption for set_known_host --- tests/integration/modules/test_ssh.py | 9 +++++++-- tests/integration/states/test_ssh_known_hosts.py | 3 ++- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/tests/integration/modules/test_ssh.py b/tests/integration/modules/test_ssh.py index 4bae9c101924..ffa052402e57 100644 --- a/tests/integration/modules/test_ssh.py +++ b/tests/integration/modules/test_ssh.py @@ -132,7 +132,9 @@ def test_recv_known_host_entries(self): """ Check that known host information is returned from remote host """ - ret = self.run_function("ssh.recv_known_host_entries", ["github.com"]) + ret = self.run_function( + "ssh.recv_known_host_entries", ["github.com"], enc="ssh-rsa" + ) try: self.assertNotEqual(ret, None) self.assertEqual(ret[0]["enc"], "ssh-rsa") @@ -219,7 +221,10 @@ def test_set_known_host(self): """ # add item ret = self.run_function( - "ssh.set_known_host", ["root", "github.com"], config=self.known_hosts + "ssh.set_known_host", + ["root", "github.com"], + enc="ssh-rsa", + config=self.known_hosts, ) try: self.assertEqual(ret["status"], "updated") diff --git a/tests/integration/states/test_ssh_known_hosts.py b/tests/integration/states/test_ssh_known_hosts.py index beeb0342bd5e..cb4b40d3a063 100644 --- a/tests/integration/states/test_ssh_known_hosts.py +++ b/tests/integration/states/test_ssh_known_hosts.py @@ -11,7 +11,7 @@ from tests.support.runtests import RUNTIME_VARS GITHUB_FINGERPRINT = "9d:38:5b:83:a9:17:52:92:56:1a:5e:c4:d4:81:8e:0a:ca:51:a2:64:f1:74:20:11:2e:f8:8a:c3:a1:39:49:8f" -GITHUB_IP = "192.30.253.113" +GITHUB_IP = "140.82.121.4" @pytest.mark.skip_if_binaries_missing("ssh", "ssh-keygen", check_all=True) @@ -37,6 +37,7 @@ def test_present(self): kwargs = { "name": "github.com", "user": "root", + "enc": "ssh-rsa", "fingerprint": GITHUB_FINGERPRINT, "config": self.known_hosts, } From b7cea89897026ca1260b40f4a42727fea31542f5 Mon Sep 17 00:00:00 2001 From: Megan Wilhite Date: Mon, 31 Jan 2022 08:46:02 -0700 Subject: [PATCH 33/66] Use main branch for kitchen-docker project --- Gemfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile b/Gemfile index 2786c6d8c3b5..cfd12f8cf358 100644 --- a/Gemfile +++ b/Gemfile @@ -8,7 +8,7 @@ gem 'kitchen-sync' gem 'git' group :docker do - gem 'kitchen-docker', :git => 'https://github.com/test-kitchen/kitchen-docker.git' + gem 'kitchen-docker', :git => 'https://github.com/test-kitchen/kitchen-docker.git', :branch => 'main' end group :windows do From 82d4d68eb4c7fbb5262e945d121e77ab63496306 Mon Sep 17 00:00:00 2001 From: Wayne Werner Date: Tue, 7 Dec 2021 12:19:57 -0600 Subject: [PATCH 34/66] Add tests for validate_tgt This function evolved over the years, but never had any tests. We're adding tests now to cover the various cases: - there are no valid minions (currently fails, should return False) - there are target minions that aren't in valid minions (correctly fails) - target minions are a subset of valid minions (i.e. all of the target minions are found in the valid minions -- there are no extras) (correctly passes) --- tests/pytests/unit/utils/test_minions.py | 51 +++++++++++++++++++++--- 1 file changed, 46 insertions(+), 5 deletions(-) diff --git a/tests/pytests/unit/utils/test_minions.py b/tests/pytests/unit/utils/test_minions.py index a9eee20ea184..e673ae145e09 100644 --- a/tests/pytests/unit/utils/test_minions.py +++ b/tests/pytests/unit/utils/test_minions.py @@ -1,3 +1,4 @@ +import pytest import salt.utils.minions import salt.utils.network from tests.support.mock import patch @@ -31,6 +32,7 @@ def test_connected_ids_remote_minions(): "publish_port": 4505, "detect_remote_minions": True, "remote_minions_port": 22, + "minion_data_cache": True, } minion = "minion" minion2 = "minion2" @@ -38,14 +40,53 @@ def test_connected_ids_remote_minions(): ip = salt.utils.network.ip_addrs() mdata = {"grains": {"ipv4": ip, "ipv6": []}} mdata2 = {"grains": {"ipv4": [minion2_ip], "ipv6": []}} - ckminions = salt.utils.minions.CkMinions({"minion_data_cache": True}) patch_net = patch("salt.utils.network.local_port_tcp", return_value={"127.0.0.1"}) patch_remote_net = patch( "salt.utils.network.remote_port_tcp", return_value={minion2_ip} ) patch_list = patch("salt.cache.Cache.list", return_value=[minion, minion2]) patch_fetch = patch("salt.cache.Cache.fetch", side_effect=[mdata, mdata2]) - with patch.dict(ckminions.opts, opts): - with patch_net, patch_list, patch_fetch, patch_remote_net: - ret = ckminions.connected_ids() - assert ret == {minion2, minion} + ckminions = salt.utils.minions.CkMinions(opts) + with patch_net, patch_list, patch_fetch, patch_remote_net: + ret = ckminions.connected_ids() + assert ret == {minion2, minion} + + + +# These validate_tgt tests make the assumption that CkMinions.check_minions is +# correct. In other words, these tests are only worthwhile if check_minions is +# also correct. +@pytest.mark.xfail +def test_validate_tgt_should_return_false_when_no_valid_minions_have_been_found(): + ckminions = salt.utils.minions.CkMinions(opts={}) + with patch('salt.utils.minions.CkMinions.check_minions', autospec=True, return_value={}): + result = ckminions.validate_tgt('fnord', 'fnord', 'fnord', minions=[]) + assert result is False + + +@pytest.mark.parametrize( + 'valid_minions, target_minions', [ + (['one', 'two', 'three'], ['one', 'two', 'five']), + (['one'], ['one', 'two']), + (['one', 'two', 'three', 'four'], ['five']), + ] +) +def test_validate_tgt_should_return_false_when_minions_have_minions_not_in_valid_minions(valid_minions, target_minions): + ckminions = salt.utils.minions.CkMinions(opts={}) + with patch('salt.utils.minions.CkMinions.check_minions', autospec=True, return_value={'minions': valid_minions}): + result = ckminions.validate_tgt('fnord', 'fnord', 'fnord', minions=target_minions) + assert result is False + + +@pytest.mark.parametrize( + 'valid_minions, target_minions', [ + (['one', 'two', 'three', 'five'], ['one', 'two', 'five']), + (['one'], ['one']), + (['one', 'two', 'three', 'four', 'five'], ['five']), + ] +) +def test_validate_tgt_should_return_true_when_all_minions_are_found_in_valid_minions(valid_minions, target_minions): + ckminions = salt.utils.minions.CkMinions(opts={}) + with patch('salt.utils.minions.CkMinions.check_minions', autospec=True, return_value={'minions': valid_minions}): + result = ckminions.validate_tgt('fnord', 'fnord', 'fnord', minions=target_minions) + assert result is True From a13458df56b75c56d174d7341c555c9010dcc02d Mon Sep 17 00:00:00 2001 From: Wayne Werner Date: Tue, 7 Dec 2021 12:25:56 -0600 Subject: [PATCH 35/66] Refactor minions should be a subset of v_minions - the extra code was just getting in the way. Also, this function evolved over time but the docstring never kept up. Updated the docstring to more accurately describe the function's behavior. --- salt/utils/minions.py | 15 ++++--- tests/pytests/unit/utils/test_minions.py | 57 ++++++++++++++++-------- 2 files changed, 47 insertions(+), 25 deletions(-) diff --git a/salt/utils/minions.py b/salt/utils/minions.py index a639bbb51341..f137de2c0fa8 100644 --- a/salt/utils/minions.py +++ b/salt/utils/minions.py @@ -736,8 +736,14 @@ def check_minions( def validate_tgt(self, valid, expr, tgt_type, minions=None, expr_form=None): """ - Return a Bool. This function returns if the expression sent in is - within the scope of the valid expression + Validate the target minions against the possible valid minions. + + If ``minions`` is provided, they will be compared against the valid + minions. Otherwise, ``expr`` and ``tgt_type`` will be used to expand + to a list of target minions. + + Return True if all of the requested minions are valid minions, + otherwise return False. """ v_minions = set(self.check_minions(valid, "compound").get("minions", [])) @@ -746,10 +752,7 @@ def validate_tgt(self, valid, expr, tgt_type, minions=None, expr_form=None): minions = set(_res["minions"]) else: minions = set(minions) - d_bool = not bool(minions.difference(v_minions)) - if len(v_minions) == len(minions) and d_bool: - return True - return d_bool + return minions.issubset(v_minions) def match_check(self, regex, fun): """ diff --git a/tests/pytests/unit/utils/test_minions.py b/tests/pytests/unit/utils/test_minions.py index e673ae145e09..a9cb312fa7a7 100644 --- a/tests/pytests/unit/utils/test_minions.py +++ b/tests/pytests/unit/utils/test_minions.py @@ -52,41 +52,60 @@ def test_connected_ids_remote_minions(): assert ret == {minion2, minion} - # These validate_tgt tests make the assumption that CkMinions.check_minions is # correct. In other words, these tests are only worthwhile if check_minions is # also correct. @pytest.mark.xfail def test_validate_tgt_should_return_false_when_no_valid_minions_have_been_found(): ckminions = salt.utils.minions.CkMinions(opts={}) - with patch('salt.utils.minions.CkMinions.check_minions', autospec=True, return_value={}): - result = ckminions.validate_tgt('fnord', 'fnord', 'fnord', minions=[]) + with patch( + "salt.utils.minions.CkMinions.check_minions", autospec=True, return_value={} + ): + result = ckminions.validate_tgt("fnord", "fnord", "fnord", minions=[]) assert result is False @pytest.mark.parametrize( - 'valid_minions, target_minions', [ - (['one', 'two', 'three'], ['one', 'two', 'five']), - (['one'], ['one', 'two']), - (['one', 'two', 'three', 'four'], ['five']), - ] + "valid_minions, target_minions", + [ + (["one", "two", "three"], ["one", "two", "five"]), + (["one"], ["one", "two"]), + (["one", "two", "three", "four"], ["five"]), + ], ) -def test_validate_tgt_should_return_false_when_minions_have_minions_not_in_valid_minions(valid_minions, target_minions): +def test_validate_tgt_should_return_false_when_minions_have_minions_not_in_valid_minions( + valid_minions, target_minions +): ckminions = salt.utils.minions.CkMinions(opts={}) - with patch('salt.utils.minions.CkMinions.check_minions', autospec=True, return_value={'minions': valid_minions}): - result = ckminions.validate_tgt('fnord', 'fnord', 'fnord', minions=target_minions) + with patch( + "salt.utils.minions.CkMinions.check_minions", + autospec=True, + return_value={"minions": valid_minions}, + ): + result = ckminions.validate_tgt( + "fnord", "fnord", "fnord", minions=target_minions + ) assert result is False @pytest.mark.parametrize( - 'valid_minions, target_minions', [ - (['one', 'two', 'three', 'five'], ['one', 'two', 'five']), - (['one'], ['one']), - (['one', 'two', 'three', 'four', 'five'], ['five']), - ] + "valid_minions, target_minions", + [ + (["one", "two", "three", "five"], ["one", "two", "five"]), + (["one"], ["one"]), + (["one", "two", "three", "four", "five"], ["five"]), + ], ) -def test_validate_tgt_should_return_true_when_all_minions_are_found_in_valid_minions(valid_minions, target_minions): +def test_validate_tgt_should_return_true_when_all_minions_are_found_in_valid_minions( + valid_minions, target_minions +): ckminions = salt.utils.minions.CkMinions(opts={}) - with patch('salt.utils.minions.CkMinions.check_minions', autospec=True, return_value={'minions': valid_minions}): - result = ckminions.validate_tgt('fnord', 'fnord', 'fnord', minions=target_minions) + with patch( + "salt.utils.minions.CkMinions.check_minions", + autospec=True, + return_value={"minions": valid_minions}, + ): + result = ckminions.validate_tgt( + "fnord", "fnord", "fnord", minions=target_minions + ) assert result is True From 4fc36aa00a8b6a8eadff24dc63884b9bfe562671 Mon Sep 17 00:00:00 2001 From: Wayne Werner Date: Tue, 7 Dec 2021 12:31:00 -0600 Subject: [PATCH 36/66] Fix #60413 When using a syndic and user auth, it was possible for v_minions and minions to be two empty sets, which returned True. This allowed the user to still publish the function. The Syndic would get the published event and apply it, even though it should have been rejected. However, if there are no valid minions, then it doesn't matter what the targets are -- there are not valid targets, so there's no reason to do any further checks. --- changelog/60413.fixed | 1 + salt/utils/minions.py | 4 ++++ tests/pytests/unit/utils/test_minions.py | 1 - 3 files changed, 5 insertions(+), 1 deletion(-) create mode 100644 changelog/60413.fixed diff --git a/changelog/60413.fixed b/changelog/60413.fixed new file mode 100644 index 000000000000..0e1e0784ecce --- /dev/null +++ b/changelog/60413.fixed @@ -0,0 +1 @@ +Fixed targeting bug, especially visible when using syndic and user auth. diff --git a/salt/utils/minions.py b/salt/utils/minions.py index f137de2c0fa8..3e2f448db6c0 100644 --- a/salt/utils/minions.py +++ b/salt/utils/minions.py @@ -747,6 +747,10 @@ def validate_tgt(self, valid, expr, tgt_type, minions=None, expr_form=None): """ v_minions = set(self.check_minions(valid, "compound").get("minions", [])) + if not v_minions: + # There are no valid minions, so it doesn't matter what we are + # targeting - this is a fail. + return False if minions is None: _res = self.check_minions(expr, tgt_type) minions = set(_res["minions"]) diff --git a/tests/pytests/unit/utils/test_minions.py b/tests/pytests/unit/utils/test_minions.py index a9cb312fa7a7..7875a959ce3c 100644 --- a/tests/pytests/unit/utils/test_minions.py +++ b/tests/pytests/unit/utils/test_minions.py @@ -55,7 +55,6 @@ def test_connected_ids_remote_minions(): # These validate_tgt tests make the assumption that CkMinions.check_minions is # correct. In other words, these tests are only worthwhile if check_minions is # also correct. -@pytest.mark.xfail def test_validate_tgt_should_return_false_when_no_valid_minions_have_been_found(): ckminions = salt.utils.minions.CkMinions(opts={}) with patch( From 67536977a6cf3d01cdc14495b63c1650771ae819 Mon Sep 17 00:00:00 2001 From: Wayne Werner Date: Wed, 2 Feb 2022 11:57:19 -0600 Subject: [PATCH 37/66] Rename changelog to security --- changelog/{60413.fixed => 60413.security} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename changelog/{60413.fixed => 60413.security} (100%) diff --git a/changelog/60413.fixed b/changelog/60413.security similarity index 100% rename from changelog/60413.fixed rename to changelog/60413.security From 51eaa5d9fd75cf8776a7197003d996c7beb92f70 Mon Sep 17 00:00:00 2001 From: Wayne Werner Date: Wed, 2 Feb 2022 12:00:29 -0600 Subject: [PATCH 38/66] add cve# to changelog --- changelog/60413.security | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog/60413.security b/changelog/60413.security index 0e1e0784ecce..14eecd9b0d2f 100644 --- a/changelog/60413.security +++ b/changelog/60413.security @@ -1 +1 @@ -Fixed targeting bug, especially visible when using syndic and user auth. +Fixed targeting bug, especially visible when using syndic and user auth. (CVE-2022-22941) From fcda76891d65ce4425b6753ca7c1a9ce90fcdc12 Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Tue, 25 Jan 2022 15:53:56 -0700 Subject: [PATCH 39/66] Sign pillar data --- salt/pillar/__init__.py | 1 + salt/transport/mixins/auth.py | 20 ++++++++++++--- salt/transport/tcp.py | 40 ++++++++++++++++++++++++++--- salt/transport/zeromq.py | 48 +++++++++++++++++++++++++++++++---- 4 files changed, 98 insertions(+), 11 deletions(-) diff --git a/salt/pillar/__init__.py b/salt/pillar/__init__.py index 22f5c3a0a9a8..569108afd481 100644 --- a/salt/pillar/__init__.py +++ b/salt/pillar/__init__.py @@ -9,6 +9,7 @@ import os import sys import traceback +import uuid import salt.ext.tornado.gen import salt.fileclient diff --git a/salt/transport/mixins/auth.py b/salt/transport/mixins/auth.py index 90197fb5067f..2eac6b3b2edd 100644 --- a/salt/transport/mixins/auth.py +++ b/salt/transport/mixins/auth.py @@ -112,7 +112,7 @@ def post_fork(self, _, __): self.master_key = salt.crypt.MasterKeys(self.opts) - def _encrypt_private(self, ret, dictkey, target): + def _encrypt_private(self, ret, dictkey, target, nonce=None, sign_messages=True): """ The server equivalent of ReqChannel.crypted_transfer_decode_dictentry """ @@ -127,7 +127,6 @@ def _encrypt_private(self, ret, dictkey, target): except OSError: log.error("AES key not found") return {"error": "AES key not found"} - pret = {} key = salt.utils.stringutils.to_bytes(key) if HAS_M2: @@ -135,7 +134,22 @@ def _encrypt_private(self, ret, dictkey, target): else: cipher = PKCS1_OAEP.new(pub) pret["key"] = cipher.encrypt(key) - pret[dictkey] = pcrypt.dumps(ret if ret is not False else {}) + if ret is False: + ret = {} + if sign_messages: + if nonce is None: + return {"error": "Nonce not included in request"} + tosign = salt.payload.dumps( + {"key": pret["key"], "pillar": ret, "nonce": nonce} + ) + master_pem_path = os.path.join(self.opts["pki_dir"], "master.pem") + signed_msg = { + "data": tosign, + "sig": salt.crypt.sign_message(master_pem_path, tosign), + } + pret[dictkey] = pcrypt.dumps(signed_msg) + else: + pret[dictkey] = pcrypt.dumps(ret) return pret def _update_aes(self): diff --git a/salt/transport/tcp.py b/salt/transport/tcp.py index f8f51eab6695..171aedec4ff9 100644 --- a/salt/transport/tcp.py +++ b/salt/transport/tcp.py @@ -13,6 +13,7 @@ import time import traceback import urllib.parse +import uuid import salt.crypt import salt.exceptions @@ -266,12 +267,15 @@ def _package_load(self, load): return { "enc": self.crypt, "load": load, + "version": 2, } @salt.ext.tornado.gen.coroutine def crypted_transfer_decode_dictentry( self, load, dictkey=None, tries=3, timeout=60 ): + nonce = uuid.uuid4().hex + load["nonce"] = nonce if not self.auth.authenticated: yield self.auth.authenticate() ret = yield self.message_client.send( @@ -285,10 +289,29 @@ def crypted_transfer_decode_dictentry( else: cipher = PKCS1_OAEP.new(key) aes = cipher.decrypt(ret["key"]) + + # Decrypt using the public key. pcrypt = salt.crypt.Crypticle(self.opts, aes) - data = pcrypt.loads(ret[dictkey]) - data = salt.transport.frame.decode_embedded_strs(data) - raise salt.ext.tornado.gen.Return(data) + signed_msg = pcrypt.loads(ret[dictkey]) + + # Validate the master's signature. + master_pubkey_path = os.path.join(self.opts["pki_dir"], "minion_master.pub") + if not salt.crypt.verify_signature( + master_pubkey_path, signed_msg["data"], signed_msg["sig"] + ): + raise salt.crypt.AuthenticationError( + "Pillar payload signature failed to validate." + ) + + # Make sure the signed key matches the key we used to decrypt the data. + data = salt.payload.loads(signed_msg["data"]) + if data["key"] != ret["key"]: + raise salt.crypt.AuthenticationError("Key verification failed.") + + # Validate the nonce. + if data["nonce"] != nonce: + raise salt.crypt.AuthenticationError("Pillar nonce verification failed.") + raise salt.ext.tornado.gen.Return(data["pillar"]) @salt.ext.tornado.gen.coroutine def _crypted_transfer(self, load, tries=3, timeout=60): @@ -696,6 +719,10 @@ def handle_message(self, stream, header, payload): ) raise salt.ext.tornado.gen.Return() + version = 0 + if "version" in payload: + version = payload["version"] + # intercept the "_auth" commands, since the main daemon shouldn't know # anything about our key auth if ( @@ -731,12 +758,19 @@ def handle_message(self, stream, header, payload): ) ) elif req_fun == "send_private": + sign_messages = False + nonce = None + if version > 1: + sign_messages = True + nonce = payload["load"].get("nonce") stream.write( salt.transport.frame.frame_msg( self._encrypt_private( ret, req_opts["key"], req_opts["tgt"], + nonce, + sign_messages, ), header=header, ) diff --git a/salt/transport/zeromq.py b/salt/transport/zeromq.py index 357fb08553d5..97c4cc80d9f9 100644 --- a/salt/transport/zeromq.py +++ b/salt/transport/zeromq.py @@ -8,6 +8,7 @@ import signal import sys import threading +import uuid from random import randint import salt.auth @@ -211,22 +212,27 @@ def _package_load(self, load): return { "enc": self.crypt, "load": load, + "version": 2, } @salt.ext.tornado.gen.coroutine def crypted_transfer_decode_dictentry( self, load, dictkey=None, tries=3, timeout=60 ): + nonce = uuid.uuid4().hex + load["nonce"] = nonce if not self.auth.authenticated: # Return control back to the caller, continue when authentication succeeds yield self.auth.authenticate() - # Return control to the caller. When send() completes, resume by populating ret with the Future.result + + # Return control to the caller. When send() completes, resume by + # populating ret with the Future.result ret = yield self.message_client.send( self._package_load(self.auth.crypticle.dumps(load)), timeout=timeout, tries=tries, ) - key = self.auth.get_keys() + if "key" not in ret: # Reauth in the case our key is deleted on the master side. yield self.auth.authenticate() @@ -235,15 +241,36 @@ def crypted_transfer_decode_dictentry( timeout=timeout, tries=tries, ) + + key = self.auth.get_keys() if HAS_M2: aes = key.private_decrypt(ret["key"], RSA.pkcs1_oaep_padding) else: cipher = PKCS1_OAEP.new(key) aes = cipher.decrypt(ret["key"]) + + # Decrypt using the public key. pcrypt = salt.crypt.Crypticle(self.opts, aes) - data = pcrypt.loads(ret[dictkey]) - data = salt.transport.frame.decode_embedded_strs(data) - raise salt.ext.tornado.gen.Return(data) + signed_msg = pcrypt.loads(ret[dictkey]) + + # Validate the master's signature. + master_pubkey_path = os.path.join(self.opts["pki_dir"], "minion_master.pub") + if not salt.crypt.verify_signature( + master_pubkey_path, signed_msg["data"], signed_msg["sig"] + ): + raise salt.crypt.AuthenticationError( + "Pillar payload signature failed to validate." + ) + + # Make sure the signed key matches the key we used to decrypt the data. + data = salt.payload.loads(signed_msg["data"]) + if data["key"] != ret["key"]: + raise salt.crypt.AuthenticationError("Key verification failed.") + + # Validate the nonce. + if data["nonce"] != nonce: + raise salt.crypt.AuthenticationError("Pillar nonce verification failed.") + raise salt.ext.tornado.gen.Return(data["pillar"]) @salt.ext.tornado.gen.coroutine def _crypted_transfer(self, load, tries=3, timeout=60, raw=False): @@ -735,6 +762,10 @@ def handle_message(self, stream, payload): ) raise salt.ext.tornado.gen.Return() + version = 0 + if "version" in payload: + version = payload["version"] + # intercept the "_auth" commands, since the main daemon shouldn't know # anything about our key auth if payload["enc"] == "clear" and payload.get("load", {}).get("cmd") == "_auth": @@ -758,12 +789,19 @@ def handle_message(self, stream, payload): elif req_fun == "send": stream.send(salt.payload.dumps(self.crypticle.dumps(ret))) elif req_fun == "send_private": + sign_messages = False + nonce = None + if version > 1: + sign_messages = True + nonce = payload["load"]["nonce"] stream.send( salt.payload.dumps( self._encrypt_private( ret, req_opts["key"], req_opts["tgt"], + nonce, + sign_messages, ) ) ) From 66b43e30d6f26851bff75096920005943529f641 Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Tue, 25 Jan 2022 15:56:18 -0700 Subject: [PATCH 40/66] Add regression tests for CVE-2022-22934 --- tests/pytests/unit/transport/test_zeromq.py | 530 ++++++++++++++++++++ 1 file changed, 530 insertions(+) diff --git a/tests/pytests/unit/transport/test_zeromq.py b/tests/pytests/unit/transport/test_zeromq.py index 44f38ee99812..84107db7d379 100644 --- a/tests/pytests/unit/transport/test_zeromq.py +++ b/tests/pytests/unit/transport/test_zeromq.py @@ -3,8 +3,12 @@ """ import hashlib +import logging +import os +import pytest import salt.config +import salt.crypt import salt.exceptions import salt.ext.tornado.gen import salt.ext.tornado.ioloop @@ -17,6 +21,130 @@ from salt.transport.zeromq import AsyncReqMessageClientPool from tests.support.mock import MagicMock, patch +try: + from M2Crypto import RSA + + HAS_M2 = True +except ImportError: + HAS_M2 = False + try: + from Cryptodome.Cipher import PKCS1_OAEP + except ImportError: + from Crypto.Cipher import PKCS1_OAEP # nosec + +log = logging.getLogger(__name__) + +MASTER_PRIV_KEY = """ +-----BEGIN RSA PRIVATE KEY----- +MIIEogIBAAKCAQEAoAsMPt+4kuIG6vKyw9r3+OuZrVBee/2vDdVetW+Js5dTlgrJ +aghWWn3doGmKlEjqh7E4UTa+t2Jd6w8RSLnyHNJ/HpVhMG0M07MF6FMfILtDrrt8 +ZX7eDVt8sx5gCEpYI+XG8Y07Ga9i3Hiczt+fu6HYwu96HggmG2pqkOrn3iGfqBvV +YVFJzSZYe7e4c1PeEs0xYcrA4k+apyGsMtpef8vRUrNicRLc7dAcvfhtgt2DXEZ2 +d72t/CR4ygtUvPXzisaTPW0G7OWAheCloqvTIIPQIjR8htFxGTz02STVXfnhnJ0Z +k8KhqKF2v1SQvIYxsZU7jaDgl5i3zpeh58cYOwIDAQABAoIBABZUJEO7Y91+UnfC +H6XKrZEZkcnH7j6/UIaOD9YhdyVKxhsnax1zh1S9vceNIgv5NltzIsfV6vrb6v2K +Dx/F7Z0O0zR5o+MlO8ZncjoNKskex10gBEWG00Uqz/WPlddiQ/TSMJTv3uCBAzp+ +S2Zjdb4wYPUlgzSgb2ygxrhsRahMcSMG9PoX6klxMXFKMD1JxiY8QfAHahPzQXy9 +F7COZ0fCVo6BE+MqNuQ8tZeIxu8mOULQCCkLFwXmkz1FpfK/kNRmhIyhxwvCS+z4 +JuErW3uXfE64RLERiLp1bSxlDdpvRO2R41HAoNELTsKXJOEt4JANRHm/CeyA5wsh +NpscufUCgYEAxhgPfcMDy2v3nL6KtkgYjdcOyRvsAF50QRbEa8ldO+87IoMDD/Oe +osFERJ5hhyyEO78QnaLVegnykiw5DWEF02RKMhD/4XU+1UYVhY0wJjKQIBadsufB +2dnaKjvwzUhPh5BrBqNHl/FXwNCRDiYqXa79eWCPC9OFbZcUWWq70s8CgYEAztOI +61zRfmXJ7f70GgYbHg+GA7IrsAcsGRITsFR82Ho0lqdFFCxz7oK8QfL6bwMCGKyk +nzk+twh6hhj5UNp18KN8wktlo02zTgzgemHwaLa2cd6xKgmAyuPiTgcgnzt5LVNG +FOjIWkLwSlpkDTl7ZzY2QSy7t+mq5d750fpIrtUCgYBWXZUbcpPL88WgDB7z/Bjg +dlvW6JqLSqMK4b8/cyp4AARbNp12LfQC55o5BIhm48y/M70tzRmfvIiKnEc/gwaE +NJx4mZrGFFURrR2i/Xx5mt/lbZbRsmN89JM+iKWjCpzJ8PgIi9Wh9DIbOZOUhKVB +9RJEAgo70LvCnPTdS0CaVwKBgDJW3BllAvw/rBFIH4OB/vGnF5gosmdqp3oGo1Ik +jipmPAx6895AH4tquIVYrUl9svHsezjhxvjnkGK5C115foEuWXw0u60uiTiy+6Pt +2IS0C93VNMulenpnUrppE7CN2iWFAiaura0CY9fE/lsVpYpucHAWgi32Kok+ZxGL +WEttAoGAN9Ehsz4LeQxEj3x8wVeEMHF6OsznpwYsI2oVh6VxpS4AjgKYqeLVcnNi +TlZFsuQcqgod8OgzA91tdB+Rp86NygmWD5WzeKXpCOg9uA+y/YL+0sgZZHsuvbK6 +PllUgXdYxqClk/hdBFB7v9AQoaj7K9Ga22v32msftYDQRJ94xOI= +-----END RSA PRIVATE KEY----- +""" + + +MASTER_PUB_KEY = """ +-----BEGIN PUBLIC KEY----- +MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAoAsMPt+4kuIG6vKyw9r3 ++OuZrVBee/2vDdVetW+Js5dTlgrJaghWWn3doGmKlEjqh7E4UTa+t2Jd6w8RSLny +HNJ/HpVhMG0M07MF6FMfILtDrrt8ZX7eDVt8sx5gCEpYI+XG8Y07Ga9i3Hiczt+f +u6HYwu96HggmG2pqkOrn3iGfqBvVYVFJzSZYe7e4c1PeEs0xYcrA4k+apyGsMtpe +f8vRUrNicRLc7dAcvfhtgt2DXEZ2d72t/CR4ygtUvPXzisaTPW0G7OWAheCloqvT +IIPQIjR8htFxGTz02STVXfnhnJ0Zk8KhqKF2v1SQvIYxsZU7jaDgl5i3zpeh58cY +OwIDAQAB +-----END PUBLIC KEY----- +""" + + +MINION_PRIV_KEY = """ +-----BEGIN RSA PRIVATE KEY----- +MIIEowIBAAKCAQEAsT6TwnlI0L7urjXu6D5E11tFJ/NglQ45jW/WN9tAUNvphq6Q +cjJCd/aWmdqlqe7ix8y9M/8rgwghRQsnPXblVBvPwFcUEXhMRnOGzqbq/0zyQX01 +KecT0plBhlDt2lTyCLU6E4XCqyLbPfOxgXzsVqM0/TnzRtpVvGNy+5N4eFGylrjb +cJhPxKt2G9TDOCM/hYacDs5RVIYQQmcYb8LJq7G3++FfWpYRDaxdKoHNFDspEynd +jzr67hgThnwzc388OKNJx/7B2atwPTunPb3YBjgwDyRO/01OKK4gUHdw5KoctFgp +kDCDjwjemlyXV+MYODRTIdtOlAP83ZkntEuLoQIDAQABAoIBAAJOKNtvFGfF2l9H +S4CXZSUGU0a+JaCkR+wmnjsPwPn/dXDpAe8nGpidpNicPWqRm6WABjeQHaxda+fB +lpSrRtEdo3zoi2957xQJ5wddDtI1pmXJQrdbm0H/K39oIg/Xtv/IZT769TM6OtVg +paUxG/aftmeGXDtGfIL8w1jkuPABRBLOakWQA9uVdeG19KTU0Ag8ilpJdEX64uFJ +W75bpVjT+KO/6aV1inuCntQSP097aYvUWajRwuiYVJOxoBZHme3IObcE6mdnYXeQ +wblyWBpJUHrOS4MP4HCODV2pHKZ2rr7Nwhh8lMNw/eY9OP0ifz2AcAqe3sUMQOKP +T0qRC6ECgYEAyeU5JvUPOpxXvvChYh6gJ8pYTIh1ueDP0O5e4t3vhz6lfy9DKtRN +ROJLUorHvw/yVXMR72nT07a0z2VswcrUSw8ov3sI53F0NkLGEafQ35lVhTGs4vTl +CFoQCuAKPsxeUl4AIbfbpkDsLGQqzW1diFArK7YeQkpGuGaGodXl480CgYEA4L40 +x5cUXnAhTPsybo7sbcpiwFHoGblmdkvpYvHA2QxtNSi2iHHdqGo8qP1YsZjKQn58 +371NhtqidrJ6i/8EBFP1dy+y/jr9qYlZNNGcQeBi+lshrEOIf1ct56KePG79s8lm +DmD1OY8tO2R37+Py46Nq1n6viT/ST4NjLQI3GyUCgYEAiOswSDA3ZLs0cqRD/gPg +/zsliLmehTFmHj4aEWcLkz+0Ar3tojUaNdX12QOPFQ7efH6uMhwl8NVeZ6xUBlTk +hgbAzqLE1hjGBCpiowSZDZqyOcMHiV8ll/VkHcv0hsQYT2m6UyOaDXTH9g70TB6Y +KOKddGZsvO4cad/1+/jQkB0CgYAzDEEkzLY9tS57M9uCrUgasAu6L2CO50PUvu1m +Ig9xvZbYqkS7vVFhva/FmrYYsOHQNLbcgz0m0mZwm52mSuh4qzFoPxdjE7cmWSJA +ExRxCiyxPR3q6PQKKJ0urgtPIs7RlX9u6KsKxfC6OtnbTWWQO0A7NE9e13ZHxUoz +oPsvWQKBgCa0+Fb2lzUeiQz9bV1CBkWneDZUXuZHmabAZomokX+h/bq+GcJFzZjW +3kAHwYkIy9IAy3SyO/6CP0V3vAye1p+XbotiwsQ/XZnr0pflSQL3J1l1CyN3aopg +Niv7k/zBn15B72aK73R/CpUSk9W/eJGqk1NcNwf8hJHsboRYx6BR +-----END RSA PRIVATE KEY----- +""" + + +MINION_PUB_KEY = """ +-----BEGIN PUBLIC KEY----- +MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAsT6TwnlI0L7urjXu6D5E +11tFJ/NglQ45jW/WN9tAUNvphq6QcjJCd/aWmdqlqe7ix8y9M/8rgwghRQsnPXbl +VBvPwFcUEXhMRnOGzqbq/0zyQX01KecT0plBhlDt2lTyCLU6E4XCqyLbPfOxgXzs +VqM0/TnzRtpVvGNy+5N4eFGylrjbcJhPxKt2G9TDOCM/hYacDs5RVIYQQmcYb8LJ +q7G3++FfWpYRDaxdKoHNFDspEyndjzr67hgThnwzc388OKNJx/7B2atwPTunPb3Y +BjgwDyRO/01OKK4gUHdw5KoctFgpkDCDjwjemlyXV+MYODRTIdtOlAP83ZkntEuL +oQIDAQAB +-----END PUBLIC KEY----- +""" + +AES_KEY = "8wxWlOaMMQ4d3yT74LL4+hGrGTf65w8VgrcNjLJeLRQ2Q6zMa8ItY2EQUgMKKDb7JY+RnPUxbB0=" + + +@pytest.fixture +def pki_dir(tmpdir): + madir = tmpdir.mkdir("master") + mapriv = madir.join("master.pem") + mapriv.write(MASTER_PRIV_KEY.strip()) + mapub = madir.join("master.pub") + mapub.write(MASTER_PUB_KEY.strip()) + mipub = madir.mkdir("minions").join("minion") + mipub.write(MINION_PUB_KEY.strip()) + midir = tmpdir.mkdir("minion") + mipub = midir.join("minion.pub") + mipub.write(MINION_PUB_KEY.strip()) + mipriv = midir.join("minion.pem") + mipriv.write(MINION_PRIV_KEY.strip()) + mimapriv = midir.join("minion_master.pub") + mimapriv.write(MASTER_PUB_KEY.strip()) + try: + yield tmpdir + finally: + tmpdir.remove() + def test_master_uri(): """ @@ -236,3 +364,405 @@ def test_zeromq_async_pub_channel_filtering_decode_message( res = channel._decode_messages(message) assert res.result()["enc"] == "aes" + + +def test_req_server_chan_encrypt_v2(pki_dir): + loop = salt.ext.tornado.ioloop.IOLoop.current() + opts = { + "worker_threads": 1, + "master_uri": "tcp://127.0.0.1:4506", + "interface": "127.0.0.1", + "ret_port": 4506, + "ipv6": False, + "zmq_monitor": False, + "mworker_queue_niceness": False, + "sock_dir": ".", + "pki_dir": str(pki_dir.join("master")), + "id": "minion", + "__role": "minion", + "keysize": 4096, + } + server = salt.transport.zeromq.ZeroMQReqServerChannel(opts) + dictkey = "pillar" + nonce = "abcdefg" + pillar_data = {"pillar1": "meh"} + ret = server._encrypt_private(pillar_data, dictkey, "minion", nonce) + assert "key" in ret + assert dictkey in ret + + key = salt.crypt.get_rsa_key(pki_dir.join("minion", "minion.pem"), None) + if HAS_M2: + aes = key.private_decrypt(ret["key"], RSA.pkcs1_oaep_padding) + else: + cipher = PKCS1_OAEP.new(key) + aes = cipher.decrypt(ret["key"]) + pcrypt = salt.crypt.Crypticle(opts, aes) + signed_msg = pcrypt.loads(ret[dictkey]) + + assert "sig" in signed_msg + assert "data" in signed_msg + data = salt.payload.loads(signed_msg["data"]) + assert "key" in data + assert data["key"] == ret["key"] + assert "key" in data + assert data["nonce"] == nonce + assert "pillar" in data + assert data["pillar"] == pillar_data + + +def test_req_server_chan_encrypt_v1(pki_dir): + loop = salt.ext.tornado.ioloop.IOLoop.current() + opts = { + "worker_threads": 1, + "master_uri": "tcp://127.0.0.1:4506", + "interface": "127.0.0.1", + "ret_port": 4506, + "ipv6": False, + "zmq_monitor": False, + "mworker_queue_niceness": False, + "sock_dir": ".", + "pki_dir": str(pki_dir.join("master")), + "id": "minion", + "__role": "minion", + "keysize": 4096, + } + server = salt.transport.zeromq.ZeroMQReqServerChannel(opts) + dictkey = "pillar" + nonce = "abcdefg" + pillar_data = {"pillar1": "meh"} + ret = server._encrypt_private(pillar_data, dictkey, "minion", sign_messages=False) + + assert "key" in ret + assert dictkey in ret + + key = salt.crypt.get_rsa_key(pki_dir.join("minion", "minion.pem"), None) + if HAS_M2: + aes = key.private_decrypt(ret["key"], RSA.pkcs1_oaep_padding) + else: + cipher = PKCS1_OAEP.new(key) + aes = cipher.decrypt(ret["key"]) + pcrypt = salt.crypt.Crypticle(opts, aes) + data = pcrypt.loads(ret[dictkey]) + assert data == pillar_data + + +def test_req_chan_decode_data_dict_entry_v1(pki_dir): + mockloop = MagicMock() + opts = { + "master_uri": "tcp://127.0.0.1:4506", + "interface": "127.0.0.1", + "ret_port": 4506, + "ipv6": False, + "sock_dir": ".", + "pki_dir": str(pki_dir.join("minion")), + "id": "minion", + "__role": "minion", + "keysize": 4096, + } + master_opts = dict(opts, pki_dir=str(pki_dir.join("master"))) + server = salt.transport.zeromq.ZeroMQReqServerChannel(master_opts) + client = salt.transport.zeromq.AsyncZeroMQReqChannel(opts, io_loop=mockloop) + dictkey = "pillar" + target = "minion" + pillar_data = {"pillar1": "meh"} + ret = server._encrypt_private(pillar_data, dictkey, target, sign_messages=False) + key = client.auth.get_keys() + if HAS_M2: + aes = key.private_decrypt(ret["key"], RSA.pkcs1_oaep_padding) + else: + cipher = PKCS1_OAEP.new(key) + aes = cipher.decrypt(ret["key"]) + pcrypt = salt.crypt.Crypticle(client.opts, aes) + ret_pillar_data = pcrypt.loads(ret[dictkey]) + assert ret_pillar_data == pillar_data + + +async def test_req_chan_decode_data_dict_entry_v2(pki_dir): + mockloop = MagicMock() + opts = { + "master_uri": "tcp://127.0.0.1:4506", + "interface": "127.0.0.1", + "ret_port": 4506, + "ipv6": False, + "sock_dir": ".", + "pki_dir": str(pki_dir.join("minion")), + "id": "minion", + "__role": "minion", + "keysize": 4096, + } + master_opts = dict(opts, pki_dir=str(pki_dir.join("master"))) + server = salt.transport.zeromq.ZeroMQReqServerChannel(master_opts) + client = salt.transport.zeromq.AsyncZeroMQReqChannel(opts, io_loop=mockloop) + + dictkey = "pillar" + target = "minion" + pillar_data = {"pillar1": "meh"} + + # Mock auth and message client. + auth = client.auth + auth._crypticle = salt.crypt.Crypticle(opts, AES_KEY) + client.auth = MagicMock() + client.auth.authenticated = True + client.auth.get_keys = auth.get_keys + client.auth.crypticle.dumps = auth.crypticle.dumps + client.auth.crypticle.loads = auth.crypticle.loads + client.message_client = MagicMock() + + @salt.ext.tornado.gen.coroutine + def mocksend(msg, timeout=60, tries=3): + client.message_client.msg = msg + load = client.auth.crypticle.loads(msg["load"]) + ret = server._encrypt_private( + pillar_data, dictkey, target, nonce=load["nonce"], sign_messages=True + ) + raise salt.ext.tornado.gen.Return(ret) + + client.message_client.send = mocksend + + # Note the 'ver' value in 'load' does not represent the the 'version' sent + # in the top level of the transport's message. + load = { + "id": target, + "grains": {}, + "saltenv": "base", + "pillarenv": "base", + "pillar_override": True, + "extra_minion_data": {}, + "ver": "2", + "cmd": "_pillar", + } + ret = await client.crypted_transfer_decode_dictentry( + load, + dictkey="pillar", + ) + assert "version" in client.message_client.msg + assert client.message_client.msg["version"] == 2 + assert ret == {"pillar1": "meh"} + + +async def test_req_chan_decode_data_dict_entry_v2_bad_nonce(pki_dir): + mockloop = MagicMock() + opts = { + "master_uri": "tcp://127.0.0.1:4506", + "interface": "127.0.0.1", + "ret_port": 4506, + "ipv6": False, + "sock_dir": ".", + "pki_dir": str(pki_dir.join("minion")), + "id": "minion", + "__role": "minion", + "keysize": 4096, + } + master_opts = dict(opts, pki_dir=str(pki_dir.join("master"))) + server = salt.transport.zeromq.ZeroMQReqServerChannel(master_opts) + client = salt.transport.zeromq.AsyncZeroMQReqChannel(opts, io_loop=mockloop) + + dictkey = "pillar" + badnonce = "abcdefg" + target = "minion" + pillar_data = {"pillar1": "meh"} + + # Mock auth and message client. + auth = client.auth + auth._crypticle = salt.crypt.Crypticle(opts, AES_KEY) + client.auth = MagicMock() + client.auth.authenticated = True + client.auth.get_keys = auth.get_keys + client.auth.crypticle.dumps = auth.crypticle.dumps + client.auth.crypticle.loads = auth.crypticle.loads + client.message_client = MagicMock() + ret = server._encrypt_private( + pillar_data, dictkey, target, nonce=badnonce, sign_messages=True + ) + + @salt.ext.tornado.gen.coroutine + def mocksend(msg, timeout=60, tries=3): + client.message_client.msg = msg + raise salt.ext.tornado.gen.Return(ret) + + client.message_client.send = mocksend + + # Note the 'ver' value in 'load' does not represent the the 'version' sent + # in the top level of the transport's message. + load = { + "id": target, + "grains": {}, + "saltenv": "base", + "pillarenv": "base", + "pillar_override": True, + "extra_minion_data": {}, + "ver": "2", + "cmd": "_pillar", + } + + with pytest.raises(salt.crypt.AuthenticationError) as excinfo: + ret = await client.crypted_transfer_decode_dictentry( + load, + dictkey="pillar", + ) + assert "Pillar nonce verification failed." == excinfo.value.message + + +async def test_req_chan_decode_data_dict_entry_v2_bad_signature(pki_dir): + mockloop = MagicMock() + opts = { + "master_uri": "tcp://127.0.0.1:4506", + "interface": "127.0.0.1", + "ret_port": 4506, + "ipv6": False, + "sock_dir": ".", + "pki_dir": str(pki_dir.join("minion")), + "id": "minion", + "__role": "minion", + "keysize": 4096, + } + master_opts = dict(opts, pki_dir=str(pki_dir.join("master"))) + server = salt.transport.zeromq.ZeroMQReqServerChannel(master_opts) + client = salt.transport.zeromq.AsyncZeroMQReqChannel(opts, io_loop=mockloop) + + dictkey = "pillar" + badnonce = "abcdefg" + target = "minion" + pillar_data = {"pillar1": "meh"} + + # Mock auth and message client. + auth = client.auth + auth._crypticle = salt.crypt.Crypticle(opts, AES_KEY) + client.auth = MagicMock() + client.auth.authenticated = True + client.auth.get_keys = auth.get_keys + client.auth.crypticle.dumps = auth.crypticle.dumps + client.auth.crypticle.loads = auth.crypticle.loads + client.message_client = MagicMock() + + @salt.ext.tornado.gen.coroutine + def mocksend(msg, timeout=60, tries=3): + client.message_client.msg = msg + load = client.auth.crypticle.loads(msg["load"]) + ret = server._encrypt_private( + pillar_data, dictkey, target, nonce=load["nonce"], sign_messages=True + ) + + key = client.auth.get_keys() + if HAS_M2: + aes = key.private_decrypt(ret["key"], RSA.pkcs1_oaep_padding) + else: + cipher = PKCS1_OAEP.new(key) + aes = cipher.decrypt(ret["key"]) + pcrypt = salt.crypt.Crypticle(client.opts, aes) + signed_msg = pcrypt.loads(ret[dictkey]) + # Changing the pillar data will cause the signature verification to + # fail. + data = salt.payload.loads(signed_msg["data"]) + data["pillar"] = {"pillar1": "bar"} + signed_msg["data"] = salt.payload.dumps(data) + ret[dictkey] = pcrypt.dumps(signed_msg) + raise salt.ext.tornado.gen.Return(ret) + + client.message_client.send = mocksend + + # Note the 'ver' value in 'load' does not represent the the 'version' sent + # in the top level of the transport's message. + load = { + "id": target, + "grains": {}, + "saltenv": "base", + "pillarenv": "base", + "pillar_override": True, + "extra_minion_data": {}, + "ver": "2", + "cmd": "_pillar", + } + + with pytest.raises(salt.crypt.AuthenticationError) as excinfo: + ret = await client.crypted_transfer_decode_dictentry( + load, + dictkey="pillar", + ) + assert "Pillar payload signature failed to validate." == excinfo.value.message + + +async def test_req_chan_decode_data_dict_entry_v2_bad_key(pki_dir): + mockloop = MagicMock() + opts = { + "master_uri": "tcp://127.0.0.1:4506", + "interface": "127.0.0.1", + "ret_port": 4506, + "ipv6": False, + "sock_dir": ".", + "pki_dir": str(pki_dir.join("minion")), + "id": "minion", + "__role": "minion", + "keysize": 4096, + } + master_opts = dict(opts, pki_dir=str(pki_dir.join("master"))) + server = salt.transport.zeromq.ZeroMQReqServerChannel(master_opts) + client = salt.transport.zeromq.AsyncZeroMQReqChannel(opts, io_loop=mockloop) + + dictkey = "pillar" + badnonce = "abcdefg" + target = "minion" + pillar_data = {"pillar1": "meh"} + + # Mock auth and message client. + auth = client.auth + auth._crypticle = salt.crypt.Crypticle(opts, AES_KEY) + client.auth = MagicMock() + client.auth.authenticated = True + client.auth.get_keys = auth.get_keys + client.auth.crypticle.dumps = auth.crypticle.dumps + client.auth.crypticle.loads = auth.crypticle.loads + client.message_client = MagicMock() + + @salt.ext.tornado.gen.coroutine + def mocksend(msg, timeout=60, tries=3): + client.message_client.msg = msg + load = client.auth.crypticle.loads(msg["load"]) + ret = server._encrypt_private( + pillar_data, dictkey, target, nonce=load["nonce"], sign_messages=True + ) + + key = client.auth.get_keys() + if HAS_M2: + aes = key.private_decrypt(ret["key"], RSA.pkcs1_oaep_padding) + else: + cipher = PKCS1_OAEP.new(key) + aes = cipher.decrypt(ret["key"]) + pcrypt = salt.crypt.Crypticle(client.opts, aes) + signed_msg = pcrypt.loads(ret[dictkey]) + + # Now encrypt with a different key + key = salt.crypt.Crypticle.generate_key_string() + pcrypt = salt.crypt.Crypticle(opts, key) + pubfn = os.path.join(master_opts["pki_dir"], "minions", "minion") + pub = salt.crypt.get_rsa_pub_key(pubfn) + ret[dictkey] = pcrypt.dumps(signed_msg) + key = salt.utils.stringutils.to_bytes(key) + if HAS_M2: + ret["key"] = pub.public_encrypt(key, RSA.pkcs1_oaep_padding) + else: + cipher = PKCS1_OAEP.new(pub) + ret["key"] = cipher.encrypt(key) + raise salt.ext.tornado.gen.Return(ret) + + client.message_client.send = mocksend + + # Note the 'ver' value in 'load' does not represent the the 'version' sent + # in the top level of the transport's message. + load = { + "id": target, + "grains": {}, + "saltenv": "base", + "pillarenv": "base", + "pillar_override": True, + "extra_minion_data": {}, + "ver": "2", + "cmd": "_pillar", + } + + with pytest.raises(salt.crypt.AuthenticationError) as excinfo: + ret = await client.crypted_transfer_decode_dictentry( + load, + dictkey="pillar", + ) + assert "Key verification failed." == excinfo.value.message From e2faa58202ffb867b03e3618328c4026e01809f1 Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Tue, 25 Jan 2022 15:57:45 -0700 Subject: [PATCH 41/66] Add changelog for cve-2022-22934 --- changelog/cve-2202-22934.fixed | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog/cve-2202-22934.fixed diff --git a/changelog/cve-2202-22934.fixed b/changelog/cve-2202-22934.fixed new file mode 100644 index 000000000000..7e6c0ceccaf0 --- /dev/null +++ b/changelog/cve-2202-22934.fixed @@ -0,0 +1 @@ +Sign pillar data to prevent MiTM attacks. From c5d52746ab9c497e3b0eb12341a43ba94cd5f5b4 Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Tue, 25 Jan 2022 16:07:02 -0700 Subject: [PATCH 42/66] Provide users with a nice warning when something goes wrong --- salt/pillar/__init__.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/salt/pillar/__init__.py b/salt/pillar/__init__.py index 569108afd481..e595b3fb1b06 100644 --- a/salt/pillar/__init__.py +++ b/salt/pillar/__init__.py @@ -241,6 +241,9 @@ def compile_pillar(self): load, dictkey="pillar", ) + except salt.crypt.AuthenticationError as exc: + log.error(exc.message) + raise SaltClientError("Exception getting pillar.") except Exception: # pylint: disable=broad-except log.exception("Exception getting pillar:") raise SaltClientError("Exception getting pillar.") From 84efc9fa332aada824f95ffa0189bb79897f6914 Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Wed, 26 Jan 2022 15:06:53 -0700 Subject: [PATCH 43/66] Rename changelog file --- changelog/{cve-2202-22934.fixed => cve-2202-22934.security} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename changelog/{cve-2202-22934.fixed => cve-2202-22934.security} (100%) diff --git a/changelog/cve-2202-22934.fixed b/changelog/cve-2202-22934.security similarity index 100% rename from changelog/cve-2202-22934.fixed rename to changelog/cve-2202-22934.security From 91348f3362e4ece17f9b80d4425e447e7677285c Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Tue, 1 Feb 2022 20:13:49 -0700 Subject: [PATCH 44/66] Fix wart in tests --- tests/pytests/unit/transport/test_zeromq.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/pytests/unit/transport/test_zeromq.py b/tests/pytests/unit/transport/test_zeromq.py index 84107db7d379..d9c6e925b076 100644 --- a/tests/pytests/unit/transport/test_zeromq.py +++ b/tests/pytests/unit/transport/test_zeromq.py @@ -390,7 +390,7 @@ def test_req_server_chan_encrypt_v2(pki_dir): assert "key" in ret assert dictkey in ret - key = salt.crypt.get_rsa_key(pki_dir.join("minion", "minion.pem"), None) + key = salt.crypt.get_rsa_key(str(pki_dir.join("minion", "minion.pem")), None) if HAS_M2: aes = key.private_decrypt(ret["key"], RSA.pkcs1_oaep_padding) else: @@ -435,7 +435,7 @@ def test_req_server_chan_encrypt_v1(pki_dir): assert "key" in ret assert dictkey in ret - key = salt.crypt.get_rsa_key(pki_dir.join("minion", "minion.pem"), None) + key = salt.crypt.get_rsa_key(str(pki_dir.join("minion", "minion.pem")), None) if HAS_M2: aes = key.private_decrypt(ret["key"], RSA.pkcs1_oaep_padding) else: From 7842b6f9b779b14a8bace422695814f987389d2c Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Wed, 2 Feb 2022 15:32:42 -0700 Subject: [PATCH 45/66] Return bool when using m2crypo --- salt/crypt.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/salt/crypt.py b/salt/crypt.py index 776ffaba5817..08ecb213710c 100644 --- a/salt/crypt.py +++ b/salt/crypt.py @@ -262,7 +262,12 @@ def verify_signature(pubkey_path, message, signature): md = EVP.MessageDigest("sha1") md.update(salt.utils.stringutils.to_bytes(message)) digest = md.final() - return pubkey.verify(digest, signature) + try: + return pubkey.verify(digest, signature) + except RSA.RSAError as exc: + if exc.args[0] == "bad signature": + return False + raise else: verifier = PKCS1_v1_5.new(pubkey) return verifier.verify( From 76215a6242834ecdcc9e498dd61e9d1702facb49 Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Fri, 28 Jan 2022 23:36:00 -0700 Subject: [PATCH 46/66] Limit the amount of empty space while searching ifconfig output --- changelog/cve-2020-22937.security | 1 + salt/utils/network.py | 4 ++-- tests/pytests/unit/utils/test_network.py | 8 ++++++++ 3 files changed, 11 insertions(+), 2 deletions(-) create mode 100644 changelog/cve-2020-22937.security create mode 100644 tests/pytests/unit/utils/test_network.py diff --git a/changelog/cve-2020-22937.security b/changelog/cve-2020-22937.security new file mode 100644 index 000000000000..f6bd0b8daa75 --- /dev/null +++ b/changelog/cve-2020-22937.security @@ -0,0 +1 @@ +Fix denail of service in junos ifconfig output parsing. diff --git a/salt/utils/network.py b/salt/utils/network.py index 22075066fd1d..356d1e5ae721 100644 --- a/salt/utils/network.py +++ b/salt/utils/network.py @@ -1003,10 +1003,10 @@ def _junos_interfaces_ifconfig(out): pip = re.compile( r".*?inet\s*(primary)*\s+mtu" - r" (\d+)\s+local=[^\d]*(.*?)\s+dest=[^\d]*(.*?)\/([\d]*)\s+bcast=((?:[0-9]{1,3}\.){3}[0-9]{1,3})" + r" (\d+)\s+local=[^\d]*(.*?)\s{0,40}dest=[^\d]*(.*?)\/([\d]*)\s{0,40}bcast=((?:[0-9]{1,3}\.){3}[0-9]{1,3})" ) pip6 = re.compile( - r".*?inet6 mtu [^\d]+\s+local=([0-9a-f:]+)%([a-zA-Z0-9]*)/([\d]*)\s" + r".*?inet6 mtu [^\d]+\s{0,40}local=([0-9a-f:]+)%([a-zA-Z0-9]*)/([\d]*)\s" ) pupdown = re.compile("UP") diff --git a/tests/pytests/unit/utils/test_network.py b/tests/pytests/unit/utils/test_network.py new file mode 100644 index 000000000000..c5f976f6749d --- /dev/null +++ b/tests/pytests/unit/utils/test_network.py @@ -0,0 +1,8 @@ +import salt.utils.network + + +def test_junos_ifconfig_output_parsing(): + ret = salt.utils.network._junos_interfaces_ifconfig( + "inet mtu 0 local=" + " " * 3456 + ) + assert ret == {"inet": {"up": False}} From aec513d5f9d423f3b09ec799333c49fad4c85409 Mon Sep 17 00:00:00 2001 From: Daniel Wozniak Date: Wed, 2 Feb 2022 15:33:38 -0700 Subject: [PATCH 47/66] Update changelog/cve-2020-22937.security Co-authored-by: Megan Wilhite --- changelog/cve-2020-22937.security | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog/cve-2020-22937.security b/changelog/cve-2020-22937.security index f6bd0b8daa75..ce0bc99125cc 100644 --- a/changelog/cve-2020-22937.security +++ b/changelog/cve-2020-22937.security @@ -1 +1 @@ -Fix denail of service in junos ifconfig output parsing. +Fix denial of service in junos ifconfig output parsing. From a44bb6b39b4145198407c4b8ee0f4869690a112c Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Thu, 27 Jan 2022 15:21:27 -0700 Subject: [PATCH 48/66] Prevent auth replays and sign replies --- salt/crypt.py | 181 +++++++++++++--------------------- salt/transport/mixins/auth.py | 94 ++++++++++++++---- salt/transport/tcp.py | 7 +- salt/transport/zeromq.py | 6 +- 4 files changed, 154 insertions(+), 134 deletions(-) diff --git a/salt/crypt.py b/salt/crypt.py index 08ecb213710c..e6e5b0d7553c 100644 --- a/salt/crypt.py +++ b/salt/crypt.py @@ -17,6 +17,7 @@ import sys import time import traceback +import uuid import weakref import salt.defaults.exitcodes @@ -734,7 +735,6 @@ def sign_in(self, timeout=60, safe=True, tries=1, channel=None): with the publication port and the shared AES key. """ - auth = {} auth_timeout = self.opts.get("auth_timeout", None) if auth_timeout is not None: @@ -746,10 +746,6 @@ def sign_in(self, timeout=60, safe=True, tries=1, channel=None): if auth_tries is not None: tries = auth_tries - m_pub_fn = os.path.join(self.opts["pki_dir"], self.mpub) - - auth["master_uri"] = self.opts["master_uri"] - close_channel = False if not channel: close_channel = True @@ -774,45 +770,21 @@ def sign_in(self, timeout=60, safe=True, tries=1, channel=None): finally: if close_channel: channel.close() + ret = self.handle_signin_response(sign_in_payload, payload) + raise salt.ext.tornado.gen.Return(ret) - if not isinstance(payload, dict): + def handle_signin_response(self, sign_in_payload, payload): + auth = {} + m_pub_fn = os.path.join(self.opts["pki_dir"], self.mpub) + auth["master_uri"] = self.opts["master_uri"] + if not isinstance(payload, dict) or "load" not in payload: log.error("Sign-in attempt failed: %s", payload) - raise salt.ext.tornado.gen.Return(False) - if "load" in payload: - if "ret" in payload["load"]: - if not payload["load"]["ret"]: - if self.opts["rejected_retry"]: - log.error( - "The Salt Master has rejected this minion's public " - "key.\nTo repair this issue, delete the public key " - "for this minion on the Salt Master.\nThe Salt " - "Minion will attempt to to re-authenicate." - ) - raise salt.ext.tornado.gen.Return("retry") - else: - log.critical( - "The Salt Master has rejected this minion's public " - "key!\nTo repair this issue, delete the public key " - "for this minion on the Salt Master and restart this " - "minion.\nOr restart the Salt Master in open mode to " - "clean out the keys. The Salt Minion will now exit." - ) - # Add a random sleep here for systems that are using a - # a service manager to immediately restart the service - # to avoid overloading the system - time.sleep(random.randint(10, 20)) - sys.exit(salt.defaults.exitcodes.EX_NOPERM) - # has the master returned that its maxed out with minions? - elif payload["load"]["ret"] == "full": - raise salt.ext.tornado.gen.Return("full") - else: - log.error( - "The Salt Master has cached the public key for this " - "node, this salt minion will wait for %s seconds " - "before attempting to re-authenticate", - self.opts["acceptance_wait_time"], - ) - raise salt.ext.tornado.gen.Return("retry") + return False + + clear_signed_data = payload["load"] + clear_signature = payload["sig"] + payload = salt.payload.loads(clear_signed_data) + auth["aes"] = self.verify_master(payload, master_pub="token" in sign_in_payload) if not auth["aes"]: log.critical( @@ -827,6 +799,54 @@ def sign_in(self, timeout=60, safe=True, tries=1, channel=None): m_pub_fn, ) raise SaltClientError("Invalid master key") + + master_pubkey_path = os.path.join(self.opts["pki_dir"], self.mpub) + if os.path.exists(master_pubkey_path) and not verify_signature( + master_pubkey_path, clear_signed_data, clear_signature + ): + log.critical("The payload signature did not validate.") + raise SaltClientError("Invalid signature") + + if payload["nonce"] != sign_in_payload["nonce"]: + log.critical("The payload nonce did not validate.") + raise SaltClientError("Invalid nonce") + + if "ret" in payload: + if not payload["ret"]: + if self.opts["rejected_retry"]: + log.error( + "The Salt Master has rejected this minion's public " + "key.\nTo repair this issue, delete the public key " + "for this minion on the Salt Master.\nThe Salt " + "Minion will attempt to to re-authenicate." + ) + return "retry" + else: + log.critical( + "The Salt Master has rejected this minion's public " + "key!\nTo repair this issue, delete the public key " + "for this minion on the Salt Master and restart this " + "minion.\nOr restart the Salt Master in open mode to " + "clean out the keys. The Salt Minion will now exit." + ) + # Add a random sleep here for systems that are using a + # a service manager to immediately restart the service + # to avoid overloading the system + time.sleep(random.randint(10, 20)) + sys.exit(salt.defaults.exitcodes.EX_NOPERM) + # has the master returned that its maxed out with minions? + elif payload["ret"] == "full": + return "full" + else: + log.error( + "The Salt Master has cached the public key for this " + "node, this salt minion will wait for %s seconds " + "before attempting to re-authenticate", + self.opts["acceptance_wait_time"], + ) + return "retry" + + log.error("WTF 2") if self.opts.get("syndic_master", False): # Is syndic syndic_finger = self.opts.get( "syndic_finger", self.opts.get("master_finger", False) @@ -848,8 +868,9 @@ def sign_in(self, timeout=60, safe=True, tries=1, channel=None): != self.opts["master_finger"] ): self._finger_fail(self.opts["master_finger"], m_pub_fn) + auth["publish_port"] = payload["publish_port"] - raise salt.ext.tornado.gen.Return(auth) + return auth def get_keys(self): """ @@ -897,6 +918,7 @@ def minion_sign_in_payload(self): payload = {} payload["cmd"] = "_auth" payload["id"] = self.opts["id"] + payload["nonce"] = uuid.uuid4().hex if "autosign_grains" in self.opts: autosign_grains = {} for grain in self.opts["autosign_grains"]: @@ -1382,78 +1404,7 @@ def sign_in(self, timeout=60, safe=True, tries=1, channel=None): if close_channel: channel.close() - if "load" in payload: - if "ret" in payload["load"]: - if not payload["load"]["ret"]: - if self.opts["rejected_retry"]: - log.error( - "The Salt Master has rejected this minion's public " - "key.\nTo repair this issue, delete the public key " - "for this minion on the Salt Master.\nThe Salt " - "Minion will attempt to to re-authenicate." - ) - return "retry" - else: - log.critical( - "The Salt Master has rejected this minion's public " - "key!\nTo repair this issue, delete the public key " - "for this minion on the Salt Master and restart this " - "minion.\nOr restart the Salt Master in open mode to " - "clean out the keys. The Salt Minion will now exit." - ) - sys.exit(salt.defaults.exitcodes.EX_NOPERM) - # has the master returned that its maxed out with minions? - elif payload["load"]["ret"] == "full": - return "full" - else: - log.error( - "The Salt Master has cached the public key for this " - "node. If this is the first time connecting to this " - "master then this key may need to be accepted using " - "'salt-key -a %s' on the salt master. This salt " - "minion will wait for %s seconds before attempting " - "to re-authenticate.", - self.opts["id"], - self.opts["acceptance_wait_time"], - ) - return "retry" - auth["aes"] = self.verify_master(payload, master_pub="token" in sign_in_payload) - if not auth["aes"]: - log.critical( - "The Salt Master server's public key did not authenticate!\n" - "The master may need to be updated if it is a version of Salt " - "lower than %s, or\n" - "If you are confident that you are connecting to a valid Salt " - "Master, then remove the master public key and restart the " - "Salt Minion.\nThe master public key can be found " - "at:\n%s", - salt.version.__version__, - m_pub_fn, - ) - sys.exit(42) - if self.opts.get("syndic_master", False): # Is syndic - syndic_finger = self.opts.get( - "syndic_finger", self.opts.get("master_finger", False) - ) - if syndic_finger: - if ( - salt.utils.crypt.pem_finger( - m_pub_fn, sum_type=self.opts["hash_type"] - ) - != syndic_finger - ): - self._finger_fail(syndic_finger, m_pub_fn) - else: - if self.opts.get("master_finger", False): - if ( - salt.utils.crypt.pem_finger( - m_pub_fn, sum_type=self.opts["hash_type"] - ) - != self.opts["master_finger"] - ): - self._finger_fail(self.opts["master_finger"], m_pub_fn) - auth["publish_port"] = payload["publish_port"] - return auth + return self.handle_signin_response(sign_in_payload, payload) class Crypticle: diff --git a/salt/transport/mixins/auth.py b/salt/transport/mixins/auth.py index 2eac6b3b2edd..8381ff01c3d2 100644 --- a/salt/transport/mixins/auth.py +++ b/salt/transport/mixins/auth.py @@ -152,6 +152,15 @@ def _encrypt_private(self, ret, dictkey, target, nonce=None, sign_messages=True) pret[dictkey] = pcrypt.dumps(ret) return pret + def _clear_signed(self, load): + master_pem_path = os.path.join(self.opts["pki_dir"], "master.pem") + tosign = salt.payload.dumps(load) + return { + "enc": "clear", + "load": tosign, + "sig": salt.crypt.sign_message(master_pem_path, tosign), + } + def _update_aes(self): """ Check to see if a fresh AES key is available and update the components @@ -178,7 +187,7 @@ def _decode_payload(self, payload): payload["load"] = self.crypticle.loads(payload["load"]) return payload - def _auth(self, load): + def _auth(self, load, sign_messages=False): """ Authenticate the client, use the sent public key to encrypt the AES key which was generated at start up. @@ -196,7 +205,10 @@ def _auth(self, load): if not salt.utils.verify.valid_id(self.opts, load["id"]): log.info("Authentication request from invalid id %s", load["id"]) - return {"enc": "clear", "load": {"ret": False}} + if sign_messages: + return self._clear_signed({"ret": False, "nonce": load["nonce"]}) + else: + return {"enc": "clear", "load": {"ret": False}} log.info("Authentication request from %s", load["id"]) # 0 is default which should be 'unlimited' @@ -234,7 +246,12 @@ def _auth(self, load): self.event.fire_event( eload, salt.utils.event.tagify(prefix="auth") ) - return {"enc": "clear", "load": {"ret": "full"}} + if sign_messages: + return self._clear_signed( + {"ret": "full", "nonce": load["nonce"]} + ) + else: + return {"enc": "clear", "load": {"ret": "full"}} # Check if key is configured to be auto-rejected/signed auto_reject = self.auto_key.check_autoreject(load["id"]) @@ -261,8 +278,10 @@ def _auth(self, load): eload = {"result": False, "id": load["id"], "pub": load["pub"]} if self.opts.get("auth_events") is True: self.event.fire_event(eload, salt.utils.event.tagify(prefix="auth")) - return {"enc": "clear", "load": {"ret": False}} - + if sign_messages: + return self._clear_signed({"ret": False, "nonce": load["nonce"]}) + else: + return {"enc": "clear", "load": {"ret": False}} elif os.path.isfile(pubfn): # The key has been accepted, check it with salt.utils.files.fopen(pubfn, "r") as pubfn_handle: @@ -286,7 +305,12 @@ def _auth(self, load): self.event.fire_event( eload, salt.utils.event.tagify(prefix="auth") ) - return {"enc": "clear", "load": {"ret": False}} + if sign_messages: + return self._clear_signed( + {"ret": False, "nonce": load["nonce"]} + ) + else: + return {"enc": "clear", "load": {"ret": False}} elif not os.path.isfile(pubfn_pend): # The key has not been accepted, this is a new minion @@ -296,7 +320,10 @@ def _auth(self, load): eload = {"result": False, "id": load["id"], "pub": load["pub"]} if self.opts.get("auth_events") is True: self.event.fire_event(eload, salt.utils.event.tagify(prefix="auth")) - return {"enc": "clear", "load": {"ret": False}} + if sign_messages: + return self._clear_signed({"ret": False, "nonce": load["nonce"]}) + else: + return {"enc": "clear", "load": {"ret": False}} if auto_reject: key_path = pubfn_rejected @@ -319,7 +346,6 @@ def _auth(self, load): # Write the key to the appropriate location with salt.utils.files.fopen(key_path, "w+") as fp_: fp_.write(load["pub"]) - ret = {"enc": "clear", "load": {"ret": key_result}} eload = { "result": key_result, "act": key_act, @@ -328,7 +354,12 @@ def _auth(self, load): } if self.opts.get("auth_events") is True: self.event.fire_event(eload, salt.utils.event.tagify(prefix="auth")) - return ret + if sign_messages: + return self._clear_signed( + {"ret": key_result, "nonce": load["nonce"]} + ) + else: + return {"enc": "clear", "load": {"ret": key_result}} elif os.path.isfile(pubfn_pend): # This key is in the pending dir and is awaiting acceptance @@ -344,7 +375,6 @@ def _auth(self, load): "Pending public key for %s rejected via autoreject_file", load["id"], ) - ret = {"enc": "clear", "load": {"ret": False}} eload = { "result": False, "act": "reject", @@ -353,7 +383,10 @@ def _auth(self, load): } if self.opts.get("auth_events") is True: self.event.fire_event(eload, salt.utils.event.tagify(prefix="auth")) - return ret + if sign_messages: + return self._clear_signed({"ret": False, "nonce": load["nonce"]}) + else: + return {"enc": "clear", "load": {"ret": False}} elif not auto_sign: # This key is in the pending dir and is not being auto-signed. @@ -381,7 +414,12 @@ def _auth(self, load): self.event.fire_event( eload, salt.utils.event.tagify(prefix="auth") ) - return {"enc": "clear", "load": {"ret": False}} + if sign_messages: + return self._clear_signed( + {"ret": False, "nonce": load["nonce"]} + ) + else: + return {"enc": "clear", "load": {"ret": False}} else: log.info( "Authentication failed from host %s, the key is in " @@ -400,7 +438,12 @@ def _auth(self, load): self.event.fire_event( eload, salt.utils.event.tagify(prefix="auth") ) - return {"enc": "clear", "load": {"ret": True}} + if sign_messages: + return self._clear_signed( + {"ret": True, "nonce": load["nonce"]} + ) + else: + return {"enc": "clear", "load": {"ret": True}} else: # This key is in pending and has been configured to be # auto-signed. Check to see if it is the same key, and if @@ -422,7 +465,12 @@ def _auth(self, load): self.event.fire_event( eload, salt.utils.event.tagify(prefix="auth") ) - return {"enc": "clear", "load": {"ret": False}} + if sign_messages: + return self._clear_signed( + {"ret": False, "nonce": load["nonce"]} + ) + else: + return {"enc": "clear", "load": {"ret": False}} else: os.remove(pubfn_pend) @@ -432,7 +480,10 @@ def _auth(self, load): eload = {"result": False, "id": load["id"], "pub": load["pub"]} if self.opts.get("auth_events") is True: self.event.fire_event(eload, salt.utils.event.tagify(prefix="auth")) - return {"enc": "clear", "load": {"ret": False}} + if sign_messages: + return self._clear_signed({"ret": False, "nonce": load["nonce"]}) + else: + return {"enc": "clear", "load": {"ret": False}} log.info("Authentication accepted from %s", load["id"]) # only write to disk if you are adding the file, and in open mode, @@ -451,7 +502,10 @@ def _auth(self, load): fp_.write(load["pub"]) elif not load["pub"]: log.error("Public key is empty: %s", load["id"]) - return {"enc": "clear", "load": {"ret": False}} + if sign_messages: + return self._clear_signed({"ret": False, "nonce": load["nonce"]}) + else: + return {"enc": "clear", "load": {"ret": False}} pub = None @@ -465,7 +519,10 @@ def _auth(self, load): pub = salt.crypt.get_rsa_pub_key(pubfn) except salt.crypt.InvalidKeyError as err: log.error('Corrupt public key "%s": %s', pubfn, err) - return {"enc": "clear", "load": {"ret": False}} + if sign_messages: + return self._clear_signed({"ret": False, "nonce": load["nonce"]}) + else: + return {"enc": "clear", "load": {"ret": False}} if not HAS_M2: cipher = PKCS1_OAEP.new(pub) @@ -552,4 +609,7 @@ def _auth(self, load): eload = {"result": True, "act": "accept", "id": load["id"], "pub": load["pub"]} if self.opts.get("auth_events") is True: self.event.fire_event(eload, salt.utils.event.tagify(prefix="auth")) + if sign_messages: + ret["nonce"] = load["nonce"] + return self._clear_signed(ret) return ret diff --git a/salt/transport/tcp.py b/salt/transport/tcp.py index 171aedec4ff9..68fc4b250a89 100644 --- a/salt/transport/tcp.py +++ b/salt/transport/tcp.py @@ -418,6 +418,7 @@ def _package_load(self, load): return { "enc": self.crypt, "load": load, + "version": 2, } @salt.ext.tornado.gen.coroutine @@ -723,6 +724,10 @@ def handle_message(self, stream, header, payload): if "version" in payload: version = payload["version"] + sign_messages = False + if version > 1: + sign_messages = True + # intercept the "_auth" commands, since the main daemon shouldn't know # anything about our key auth if ( @@ -731,7 +736,7 @@ def handle_message(self, stream, header, payload): ): yield stream.write( salt.transport.frame.frame_msg( - self._auth(payload["load"]), header=header + self._auth(payload["load"], sign_messages), header=header ) ) raise salt.ext.tornado.gen.Return() diff --git a/salt/transport/zeromq.py b/salt/transport/zeromq.py index 97c4cc80d9f9..5231591790f7 100644 --- a/salt/transport/zeromq.py +++ b/salt/transport/zeromq.py @@ -766,10 +766,14 @@ def handle_message(self, stream, payload): if "version" in payload: version = payload["version"] + sign_messages = False + if version > 1: + sign_messages = True + # intercept the "_auth" commands, since the main daemon shouldn't know # anything about our key auth if payload["enc"] == "clear" and payload.get("load", {}).get("cmd") == "_auth": - stream.send(salt.payload.dumps(self._auth(payload["load"]))) + stream.send(salt.payload.dumps(self._auth(payload["load"], sign_messages))) raise salt.ext.tornado.gen.Return() # TODO: test From f6cfe97dc33f326d52f36333a7da210d8f4199a2 Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Thu, 27 Jan 2022 15:21:50 -0700 Subject: [PATCH 49/66] Add tests for cve-2022-22935 --- tests/pytests/unit/transport/test_zeromq.py | 338 ++++++++++++++++++++ 1 file changed, 338 insertions(+) diff --git a/tests/pytests/unit/transport/test_zeromq.py b/tests/pytests/unit/transport/test_zeromq.py index d9c6e925b076..643606304986 100644 --- a/tests/pytests/unit/transport/test_zeromq.py +++ b/tests/pytests/unit/transport/test_zeromq.py @@ -2,9 +2,12 @@ :codeauthor: Thomas Jackson """ +import ctypes import hashlib import logging +import multiprocessing import os +import uuid import pytest import salt.config @@ -18,6 +21,7 @@ import salt.utils.platform import salt.utils.process import salt.utils.stringutils +from salt.master import SMaster from salt.transport.zeromq import AsyncReqMessageClientPool from tests.support.mock import MagicMock, patch @@ -77,6 +81,91 @@ -----END PUBLIC KEY----- """ +MASTER2_PRIV_KEY = """ +-----BEGIN RSA PRIVATE KEY----- +MIIEogIBAAKCAQEAp+8cTxguO6Vg+YO92VfHgNld3Zy8aM3JbZvpJcjTnis+YFJ7 +Zlkcc647yPRRwY9nYBNywahnt5kIeuT1rTvTsMBZWvmUoEVUj1Xg8XXQkBvb9Ozy +Gqy/G/p8KDDpzMP/U+XCnUeHiXTZrgnqgBIc2cKeCVvWFqDi0GRFGzyaXLaX3PPm +M7DJ0MIPL1qgmcDq6+7Ze0gJ9SrDYFAeLmbuT1OqDfufXWQl/82JXeiwU2cOpqWq +7n5fvPOWim7l1tzQ+dSiMRRm0xa6uNexCJww3oJSwvMbAmgzvOhqqhlqv+K7u0u7 +FrFFojESsL36Gq4GBrISnvu2tk7u4GGNTYYQbQIDAQABAoIBAADrqWDQnd5DVZEA +lR+WINiWuHJAy/KaIC7K4kAMBgbxrz2ZbiY9Ok/zBk5fcnxIZDVtXd1sZicmPlro +GuWodIxdPZAnWpZ3UtOXUayZK/vCP1YsH1agmEqXuKsCu6Fc+K8VzReOHxLUkmXn +FYM+tixGahXcjEOi/aNNTWitEB6OemRM1UeLJFzRcfyXiqzHpHCIZwBpTUAsmzcG +QiVDkMTKubwo/m+PVXburX2CGibUydctgbrYIc7EJvyx/cpRiPZXo1PhHQWdu4Y1 +SOaC66WLsP/wqvtHo58JQ6EN/gjSsbAgGGVkZ1xMo66nR+pLpR27coS7o03xCks6 +DY/0mukCgYEAuLIGgBnqoh7YsOBLd/Bc1UTfDMxJhNseo+hZemtkSXz2Jn51322F +Zw/FVN4ArXgluH+XsOhvG/MFFpojwZSrb0Qq5b1MRdo9qycq8lGqNtlN1WHqosDQ +zW29kpL0tlRrSDpww3wRESsN9rH5XIrJ1b3ZXuO7asR+KBVQMy/+NcUCgYEA6MSC +c+fywltKPgmPl5j0DPoDe5SXE/6JQy7w/vVGrGfWGf/zEJmhzS2R+CcfTTEqaT0T +Yw8+XbFgKAqsxwtE9MUXLTVLI3sSUyE4g7blCYscOqhZ8ItCUKDXWkSpt++rG0Um +1+cEJP/0oCazG6MWqvBC4NpQ1nzh46QpjWqMwokCgYAKDLXJ1p8rvx3vUeUJW6zR +dfPlEGCXuAyMwqHLxXgpf4EtSwhC5gSyPOtx2LqUtcrnpRmt6JfTH4ARYMW9TMef +QEhNQ+WYj213mKP/l235mg1gJPnNbUxvQR9lkFV8bk+AGJ32JRQQqRUTbU+yN2MQ +HEptnVqfTp3GtJIultfwOQKBgG+RyYmu8wBP650izg33BXu21raEeYne5oIqXN+I +R5DZ0JjzwtkBGroTDrVoYyuH1nFNEh7YLqeQHqvyufBKKYo9cid8NQDTu+vWr5UK +tGvHnwdKrJmM1oN5JOAiq0r7+QMAOWchVy449VNSWWV03aeftB685iR5BXkstbIQ +EVopAoGAfcGBTAhmceK/4Q83H/FXBWy0PAa1kZGg/q8+Z0KY76AqyxOVl0/CU/rB +3tO3sKhaMTHPME/MiQjQQGoaK1JgPY6JHYvly2KomrJ8QTugqNGyMzdVJkXAK2AM +GAwC8ivAkHf8CHrHa1W7l8t2IqBjW1aRt7mOW92nfG88Hck0Mbo= +-----END RSA PRIVATE KEY----- +""" + + +MASTER2_PUB_KEY = """ +-----BEGIN PUBLIC KEY----- +MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAp+8cTxguO6Vg+YO92VfH +gNld3Zy8aM3JbZvpJcjTnis+YFJ7Zlkcc647yPRRwY9nYBNywahnt5kIeuT1rTvT +sMBZWvmUoEVUj1Xg8XXQkBvb9OzyGqy/G/p8KDDpzMP/U+XCnUeHiXTZrgnqgBIc +2cKeCVvWFqDi0GRFGzyaXLaX3PPmM7DJ0MIPL1qgmcDq6+7Ze0gJ9SrDYFAeLmbu +T1OqDfufXWQl/82JXeiwU2cOpqWq7n5fvPOWim7l1tzQ+dSiMRRm0xa6uNexCJww +3oJSwvMbAmgzvOhqqhlqv+K7u0u7FrFFojESsL36Gq4GBrISnvu2tk7u4GGNTYYQ +bQIDAQAB +-----END PUBLIC KEY----- +""" + + +MASTER_SIGNING_PRIV = """ +-----BEGIN RSA PRIVATE KEY----- +MIIEpAIBAAKCAQEAtieqrBMTM0MSIbhPKkDcozHqyXKyL/+bXYYw+iVPsns7c7bJ +zBqenLQlWoRVyrVyBFrrwQSrKu/0Mqn3l639iOGPlUoR3I7aZKIpyEdDkqd3xGIC +e+BtNNDqhUai67L63hEdG+iYAchi8UZw3LZGtcGpJ3FkBH4cYFX9EOam2QjbD7WY +EO7m1+j6XEYIOTCmAP9dGAvBbU0Jblc+wYxG3qNr+2dBWsK76QXWEqib2VSOGP+z +gjJa8tqY7PXXdOJpalQXNphmD/4o4pHKR4Euy0yL/1oMkpacmrV61LWB8Trnx9nS +9gdVrUteQF/cL1KAGwOsdVmiLpHfvqLLRqSAAQIDAQABAoIBABjB+HEN4Kixf4fk +wKHKEhL+SF6b/7sFX00NXZ/KLXRhSnnWSMQ8g/1hgMg2P2DfW4FbCDsCUu9xkLvI +HTZY+CJAIh9U42uaYPWXkt09TmJi76TZ+2Nx4/XvRUjbCm7Fs1I2ekHeUbbAUS5g ++BsPjTnL+h05zLHNoDa5yT0gVGIgFsQcX/w38arZCe8Rjp9le7PXUB5IIqASsDiw +t8zJvdyWToeXd0WswCHTQu5coHvKo5MCjIZZ1Ink1yJcCCc3rKDc+q3jB2z9T9oW +cUsKzJ4VuleiYj1eRxFITBmXbjKrb/GPRRUkeqCQbs68Hyj2d3UtOFDPeF4vng/3 +jGsHPq8CgYEA0AHAbwykVC6NMa37BTvEqcKoxbjTtErxR+yczlmVDfma9vkwtZvx +FJdbS/+WGA/ucDby5x5b2T5k1J9ueMR86xukb+HnyS0WKsZ94Ie8WnJAcbp+38M6 +7LD0u74Cgk93oagDAzUHqdLq9cXxv/ppBpxVB1Uvu8DfVMHj+wt6ie8CgYEA4C7u +u+6b8EmbGqEdtlPpScKG0WFstJEDGXRARDCRiVP2w6wm25v8UssCPvWcwf8U1Hoq +lhMY+H6a5dnRRiNYql1MGQAsqMi7VeJNYb0B1uxi7X8MPM+SvXoAglX7wm1z0cVy +O4CE5sEKbBg6aQabx1x9tzdrm80SKuSsLc5HRQ8CgYEAp/mCKSuQWNru8ruJBwTp +IB4upN1JOUN77ZVKW+lD0XFMjz1U9JPl77b65ziTQQM8jioRpkqB6cHVM088qxIh +vssn06Iex/s893YrmPKETJYPLMhqRNEn+JQ+To53ADykY0uGg0SD18SYMbmULHBP ++CKvF6jXT0vGDnA1ZzoxzskCgYEA2nQhYrRS9EVlhP93KpJ+A8gxA5tCCHo+YPFt +JoWFbCKLlYUNoHZR3IPCPoOsK0Zbj+kz0mXtsUf9vPkR+py669haLQqEejyQgFIz +QYiiYEKc6/0feapzvXtDP751w7JQaBtVAzJrT0jQ1SCO2oT8C7rPLlgs3fdpOq72 +MPSPcnUCgYBWHm6bn4HvaoUSr0v2hyD9fHZS/wDTnlXVe5c1XXgyKlJemo5dvycf +HUCmN/xIuO6AsiMdqIzv+arNJdboz+O+bNtS43LkTJfEH3xj2/DdUogdvOgG/iPM +u9KBT1h+euws7PqC5qt4vqLwCTTCZXmUS8Riv+62RCC3kZ5AbpT3ZA== +-----END RSA PRIVATE KEY----- +""" + +MASTER_SIGNING_PUB = """ +-----BEGIN PUBLIC KEY----- +MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAtieqrBMTM0MSIbhPKkDc +ozHqyXKyL/+bXYYw+iVPsns7c7bJzBqenLQlWoRVyrVyBFrrwQSrKu/0Mqn3l639 +iOGPlUoR3I7aZKIpyEdDkqd3xGICe+BtNNDqhUai67L63hEdG+iYAchi8UZw3LZG +tcGpJ3FkBH4cYFX9EOam2QjbD7WYEO7m1+j6XEYIOTCmAP9dGAvBbU0Jblc+wYxG +3qNr+2dBWsK76QXWEqib2VSOGP+zgjJa8tqY7PXXdOJpalQXNphmD/4o4pHKR4Eu +y0yL/1oMkpacmrV61LWB8Trnx9nS9gdVrUteQF/cL1KAGwOsdVmiLpHfvqLLRqSA +AQIDAQAB +-----END PUBLIC KEY----- +""" MINION_PRIV_KEY = """ -----BEGIN RSA PRIVATE KEY----- @@ -127,12 +216,27 @@ @pytest.fixture def pki_dir(tmpdir): madir = tmpdir.mkdir("master") + mapriv = madir.join("master.pem") mapriv.write(MASTER_PRIV_KEY.strip()) mapub = madir.join("master.pub") mapub.write(MASTER_PUB_KEY.strip()) + + maspriv = madir.join("master_sign.pem") + maspriv.write(MASTER_SIGNING_PRIV.strip()) + maspub = madir.join("master_sign.pub") + maspub.write(MASTER_SIGNING_PUB.strip()) + mipub = madir.mkdir("minions").join("minion") mipub.write(MINION_PUB_KEY.strip()) + for sdir in [ + "minions_autosign", + "minions_denied", + "minions_pre", + "minions_rejected", + ]: + madir.mkdir(sdir) + midir = tmpdir.mkdir("minion") mipub = midir.join("minion.pub") mipub.write(MINION_PUB_KEY.strip()) @@ -140,6 +244,8 @@ def pki_dir(tmpdir): mipriv.write(MINION_PRIV_KEY.strip()) mimapriv = midir.join("minion_master.pub") mimapriv.write(MASTER_PUB_KEY.strip()) + mimaspriv = midir.join("master_sign.pub") + mimaspriv.write(MASTER_SIGNING_PUB.strip()) try: yield tmpdir finally: @@ -766,3 +872,235 @@ def mocksend(msg, timeout=60, tries=3): dictkey="pillar", ) assert "Key verification failed." == excinfo.value.message + + +async def test_req_serv_auth_v1(pki_dir): + mockloop = MagicMock() + opts = { + "master_uri": "tcp://127.0.0.1:4506", + "interface": "127.0.0.1", + "ret_port": 4506, + "ipv6": False, + "sock_dir": ".", + "pki_dir": str(pki_dir.join("minion")), + "id": "minion", + "__role": "minion", + "keysize": 4096, + "max_minions": 0, + "auto_accept": False, + "open_mode": False, + "key_pass": None, + "master_sign_pubkey": False, + "publish_port": 4505, + "auth_mode": 1, + } + SMaster.secrets["aes"] = { + "secret": multiprocessing.Array( + ctypes.c_char, + salt.utils.stringutils.to_bytes(salt.crypt.Crypticle.generate_key_string()), + ), + "reload": salt.crypt.Crypticle.generate_key_string, + } + master_opts = dict(opts, pki_dir=str(pki_dir.join("master"))) + server = salt.transport.zeromq.ZeroMQReqServerChannel(master_opts) + server.auto_key = salt.daemons.masterapi.AutoKey(server.opts) + server.cache_cli = False + server.master_key = salt.crypt.MasterKeys(server.opts) + + pub = salt.crypt.get_rsa_pub_key(str(pki_dir.join("minion", "minion.pub"))) + token = salt.utils.stringutils.to_bytes(salt.crypt.Crypticle.generate_key_string()) + nonce = uuid.uuid4().hex + load = { + "cmd": "_auth", + "id": "minion", + "token": token, + "pub": MINION_PUB_KEY.strip(), + } + ret = server._auth(load, sign_messages=False) + assert "load" not in ret + + +async def test_req_serv_auth_v2(pki_dir): + mockloop = MagicMock() + opts = { + "master_uri": "tcp://127.0.0.1:4506", + "interface": "127.0.0.1", + "ret_port": 4506, + "ipv6": False, + "sock_dir": ".", + "pki_dir": str(pki_dir.join("minion")), + "id": "minion", + "__role": "minion", + "keysize": 4096, + "max_minions": 0, + "auto_accept": False, + "open_mode": False, + "key_pass": None, + "master_sign_pubkey": False, + "publish_port": 4505, + "auth_mode": 1, + } + SMaster.secrets["aes"] = { + "secret": multiprocessing.Array( + ctypes.c_char, + salt.utils.stringutils.to_bytes(salt.crypt.Crypticle.generate_key_string()), + ), + "reload": salt.crypt.Crypticle.generate_key_string, + } + master_opts = dict(opts, pki_dir=str(pki_dir.join("master"))) + server = salt.transport.zeromq.ZeroMQReqServerChannel(master_opts) + server.auto_key = salt.daemons.masterapi.AutoKey(server.opts) + server.cache_cli = False + server.master_key = salt.crypt.MasterKeys(server.opts) + + pub = salt.crypt.get_rsa_pub_key(str(pki_dir.join("minion", "minion.pub"))) + token = salt.utils.stringutils.to_bytes(salt.crypt.Crypticle.generate_key_string()) + nonce = uuid.uuid4().hex + load = { + "cmd": "_auth", + "id": "minion", + "nonce": nonce, + "token": token, + "pub": MINION_PUB_KEY.strip(), + } + ret = server._auth(load, sign_messages=True) + assert "sig" in ret + assert "load" in ret + + +async def test_req_chan_auth_v2(pki_dir, io_loop): + mockloop = MagicMock() + opts = { + "master_uri": "tcp://127.0.0.1:4506", + "interface": "127.0.0.1", + "ret_port": 4506, + "ipv6": False, + "sock_dir": ".", + "pki_dir": str(pki_dir.join("minion")), + "id": "minion", + "__role": "minion", + "keysize": 4096, + "max_minions": 0, + "auto_accept": False, + "open_mode": False, + "key_pass": None, + "publish_port": 4505, + "auth_mode": 1, + } + SMaster.secrets["aes"] = { + "secret": multiprocessing.Array( + ctypes.c_char, + salt.utils.stringutils.to_bytes(salt.crypt.Crypticle.generate_key_string()), + ), + "reload": salt.crypt.Crypticle.generate_key_string, + } + master_opts = dict(opts, pki_dir=str(pki_dir.join("master"))) + master_opts["master_sign_pubkey"] = False + server = salt.transport.zeromq.ZeroMQReqServerChannel(master_opts) + server.auto_key = salt.daemons.masterapi.AutoKey(server.opts) + server.cache_cli = False + server.master_key = salt.crypt.MasterKeys(server.opts) + opts["verify_master_pubkey_sign"] = False + opts["always_verify_signature"] = False + client = salt.transport.zeromq.AsyncZeroMQReqChannel(opts, io_loop=io_loop) + signin_payload = client.auth.minion_sign_in_payload() + pload = client._package_load(signin_payload) + assert "version" in pload + assert pload["version"] == 2 + + ret = server._auth(pload["load"], sign_messages=True) + assert "sig" in ret + ret = client.auth.handle_signin_response(signin_payload, ret) + assert "aes" in ret + assert "master_uri" in ret + assert "publish_port" in ret + + +async def test_req_chan_auth_v2_with_master_signing(pki_dir, io_loop): + mockloop = MagicMock() + opts = { + "master_uri": "tcp://127.0.0.1:4506", + "interface": "127.0.0.1", + "ret_port": 4506, + "ipv6": False, + "sock_dir": ".", + "pki_dir": str(pki_dir.join("minion")), + "id": "minion", + "__role": "minion", + "keysize": 4096, + "max_minions": 0, + "auto_accept": False, + "open_mode": False, + "key_pass": None, + "publish_port": 4505, + "auth_mode": 1, + } + SMaster.secrets["aes"] = { + "secret": multiprocessing.Array( + ctypes.c_char, + salt.utils.stringutils.to_bytes(salt.crypt.Crypticle.generate_key_string()), + ), + "reload": salt.crypt.Crypticle.generate_key_string, + } + master_opts = dict(opts, pki_dir=str(pki_dir.join("master"))) + master_opts["master_sign_pubkey"] = True + master_opts["master_use_pubkey_signature"] = False + master_opts["signing_key_pass"] = True + master_opts["master_sign_key_name"] = "master_sign" + server = salt.transport.zeromq.ZeroMQReqServerChannel(master_opts) + server.auto_key = salt.daemons.masterapi.AutoKey(server.opts) + server.cache_cli = False + server.master_key = salt.crypt.MasterKeys(server.opts) + opts["verify_master_pubkey_sign"] = True + opts["always_verify_signature"] = True + opts["master_sign_key_name"] = "master_sign" + opts["master"] = "master" + + assert ( + pki_dir.join("minion", "minion_master.pub").read() + == pki_dir.join("master", "master.pub").read() + ) + + client = salt.transport.zeromq.AsyncZeroMQReqChannel(opts, io_loop=io_loop) + signin_payload = client.auth.minion_sign_in_payload() + pload = client._package_load(signin_payload) + assert "version" in pload + assert pload["version"] == 2 + + server_reply = server._auth(pload["load"], sign_messages=True) + # With version 2 we always get a clear signed response + assert "enc" in server_reply + assert server_reply["enc"] == "clear" + assert "sig" in server_reply + assert "load" in server_reply + ret = client.auth.handle_signin_response(signin_payload, server_reply) + assert "aes" in ret + assert "master_uri" in ret + assert "publish_port" in ret + + # Now create a new master key pair and try auth with it. + mapriv = pki_dir.join("master", "master.pem") + mapriv.remove() + mapriv.write(MASTER2_PRIV_KEY.strip()) + mapub = pki_dir.join("master", "master.pub") + mapub.remove() + mapub.write(MASTER2_PUB_KEY.strip()) + + server = salt.transport.zeromq.ZeroMQReqServerChannel(master_opts) + server.auto_key = salt.daemons.masterapi.AutoKey(server.opts) + server.cache_cli = False + server.master_key = salt.crypt.MasterKeys(server.opts) + + signin_payload = client.auth.minion_sign_in_payload() + pload = client._package_load(signin_payload) + server_reply = server._auth(pload["load"], sign_messages=True) + ret = client.auth.handle_signin_response(signin_payload, server_reply) + + assert "aes" in ret + assert "master_uri" in ret + assert "publish_port" in ret + + assert ( + pki_dir.join("minion", "minion_master.pub").read() + == pki_dir.join("master", "master.pub").read() + ) From 6070ff34074bbcb760950a1c6945fa7535e50531 Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Thu, 27 Jan 2022 15:23:55 -0700 Subject: [PATCH 50/66] Add changelog for cve-2020-22935 --- changelog/cve-2020-22935.security | 1 + salt/crypt.py | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) create mode 100644 changelog/cve-2020-22935.security diff --git a/changelog/cve-2020-22935.security b/changelog/cve-2020-22935.security new file mode 100644 index 000000000000..fd26c907b643 --- /dev/null +++ b/changelog/cve-2020-22935.security @@ -0,0 +1 @@ +Sign authentication replies to prevent MiTM diff --git a/salt/crypt.py b/salt/crypt.py index e6e5b0d7553c..a4406cf3b2f3 100644 --- a/salt/crypt.py +++ b/salt/crypt.py @@ -846,7 +846,6 @@ def handle_signin_response(self, sign_in_payload, payload): ) return "retry" - log.error("WTF 2") if self.opts.get("syndic_master", False): # Is syndic syndic_finger = self.opts.get( "syndic_finger", self.opts.get("master_finger", False) From b191a5757e46379bbf8fb5d6a25e3d37b5d5cb0d Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Fri, 4 Feb 2022 15:02:33 -0700 Subject: [PATCH 51/66] Fix typo --- salt/crypt.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/salt/crypt.py b/salt/crypt.py index a4406cf3b2f3..d4294b28ffd4 100644 --- a/salt/crypt.py +++ b/salt/crypt.py @@ -818,7 +818,7 @@ def handle_signin_response(self, sign_in_payload, payload): "The Salt Master has rejected this minion's public " "key.\nTo repair this issue, delete the public key " "for this minion on the Salt Master.\nThe Salt " - "Minion will attempt to to re-authenicate." + "Minion will attempt to re-authenicate." ) return "retry" else: From d2f79c82aaed7154249ab3a47b7fadaa03ed2233 Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Tue, 25 Jan 2022 21:38:43 -0700 Subject: [PATCH 52/66] Prevent replays of file server requests --- salt/crypt.py | 19 ++++++++++++++----- salt/transport/tcp.py | 8 ++++++-- salt/transport/zeromq.py | 10 +++++++--- 3 files changed, 27 insertions(+), 10 deletions(-) diff --git a/salt/crypt.py b/salt/crypt.py index d4294b28ffd4..de9c748feac7 100644 --- a/salt/crypt.py +++ b/salt/crypt.py @@ -1491,13 +1491,17 @@ def decrypt(self, data): data = cypher.decrypt(data) return data[: -data[-1]] - def dumps(self, obj): + def dumps(self, obj, nonce=None): """ Serialize and encrypt a python object """ - return self.encrypt(self.PICKLE_PAD + salt.payload.dumps(obj)) + if nonce: + toencrypt = self.PICKLE_PAD + nonce.encode() + salt.payload.dumps(obj) + else: + toencrypt = self.PICKLE_PAD + salt.payload.dumps(obj) + return self.encrypt(toencrypt) - def loads(self, data, raw=False): + def loads(self, data, raw=False, nonce=None): """ Decrypt and un-serialize a python object """ @@ -1505,5 +1509,10 @@ def loads(self, data, raw=False): # simple integrity check to verify that we got meaningful data if not data.startswith(self.PICKLE_PAD): return {} - load = salt.payload.loads(data[len(self.PICKLE_PAD) :], raw=raw) - return load + data = data[len(self.PICKLE_PAD) :] + if nonce: + ret_nonce = data[:32].decode() + data = data[32:] + if ret_nonce != nonce: + raise SaltClientError("Nonce verification error") + return salt.payload.loads(data, raw=raw) diff --git a/salt/transport/tcp.py b/salt/transport/tcp.py index 68fc4b250a89..452e8e4d4421 100644 --- a/salt/transport/tcp.py +++ b/salt/transport/tcp.py @@ -321,6 +321,7 @@ def _crypted_transfer(self, load, tries=3, timeout=60): Indeed, we can fail too early in case of a master restart during a minion state execution call """ + load["nonce"] = uuid.uuid4().hex @salt.ext.tornado.gen.coroutine def _do_transfer(): @@ -334,7 +335,7 @@ def _do_transfer(): # communication, we do not subscribe to return events, we just # upload the results to the master if data: - data = self.auth.crypticle.loads(data) + data = self.auth.crypticle.loads(data, nonce=load.get("nonce")) data = salt.transport.frame.decode_embedded_strs(data) raise salt.ext.tornado.gen.Return(data) @@ -757,9 +758,12 @@ def handle_message(self, stream, header, payload): if req_fun == "send_clear": stream.write(salt.transport.frame.frame_msg(ret, header=header)) elif req_fun == "send": + nonce = None + if version > 1: + nonce = payload["load"]["nonce"] stream.write( salt.transport.frame.frame_msg( - self.crypticle.dumps(ret), header=header + self.crypticle.dumps(ret, nonce), header=header ) ) elif req_fun == "send_private": diff --git a/salt/transport/zeromq.py b/salt/transport/zeromq.py index 5231591790f7..ab67f9ccfe29 100644 --- a/salt/transport/zeromq.py +++ b/salt/transport/zeromq.py @@ -56,6 +56,7 @@ except ImportError: from Crypto.Cipher import PKCS1_OAEP # nosec + log = logging.getLogger(__name__) @@ -72,7 +73,6 @@ def _get_master_uri(master_ip, master_port, source_ip=None, source_port=None): master_uri = "tcp://{master_ip}:{master_port}".format( master_ip=ip_bracket(master_ip), master_port=master_port ) - if source_ip or source_port: if LIBZMQ_VERSION_INFO >= (4, 1, 6) and ZMQ_VERSION_INFO >= (16, 0, 1): # The source:port syntax for ZeroMQ has been added in libzmq 4.1.6 @@ -287,6 +287,7 @@ def _crypted_transfer(self, load, tries=3, timeout=60, raw=False): :param int tries: The number of times to make before failure :param int timeout: The number of seconds on a response before failing """ + load["nonce"] = uuid.uuid4().hex @salt.ext.tornado.gen.coroutine def _do_transfer(): @@ -301,7 +302,7 @@ def _do_transfer(): # communication, we do not subscribe to return events, we just # upload the results to the master if data: - data = self.auth.crypticle.loads(data, raw) + data = self.auth.crypticle.loads(data, raw, load.get("nonce")) if not raw: data = salt.transport.frame.decode_embedded_strs(data) raise salt.ext.tornado.gen.Return(data) @@ -791,7 +792,10 @@ def handle_message(self, stream, payload): if req_fun == "send_clear": stream.send(salt.payload.dumps(ret)) elif req_fun == "send": - stream.send(salt.payload.dumps(self.crypticle.dumps(ret))) + nonce = None + if version > 1: + nonce = payload["load"]["nonce"] + stream.send(salt.payload.dumps(self.crypticle.dumps(ret, nonce))) elif req_fun == "send_private": sign_messages = False nonce = None From ca0890bbbed9cc401213017d4031d8a116a53a92 Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Thu, 27 Jan 2022 18:26:27 -0700 Subject: [PATCH 53/66] Add regresion tests for fileserver nonce --- salt/minion.py | 14 ++++++++++ salt/utils/jid.py | 7 +++++ tests/pytests/unit/test_crypt.py | 45 +++++++++++++++++++++++++++++++ tests/pytests/unit/test_minion.py | 22 +++++++++++++++ 4 files changed, 88 insertions(+) diff --git a/salt/minion.py b/salt/minion.py index f4e80557d051..25fa0c76cd19 100644 --- a/salt/minion.py +++ b/salt/minion.py @@ -1687,6 +1687,20 @@ def _handle_decoded_payload(self, data): Override this method if you wish to handle the decoded data differently. """ + # Do not re-play old jobs + last_jid_path = os.path.join(self.opts["cachedir"], ".last_jid") + if os.path.exists(last_jid_path): + with salt.utils.files.fopen(last_jid_path, "r") as fp: + last_jid = fp.read() + last_dt = salt.utils.jid.jid_to_datetime(last_jid) + this_dt = salt.utils.jid.jid_to_datetime(data["jid"]) + if last_dt >= this_dt: + log.error("Received old JID, doing nothing: %r %r", last_jid, data["jid"]) + return + + with salt.utils.files.fopen(last_jid_path, "w") as fp: + fp.write(data["jid"]) + # Ensure payload is unicode. Disregard failure to decode binary blobs. if "user" in data: log.info( diff --git a/salt/utils/jid.py b/salt/utils/jid.py index c3dbf8a078af..35914a1043d1 100644 --- a/salt/utils/jid.py +++ b/salt/utils/jid.py @@ -34,6 +34,13 @@ def gen_jid(opts): return "{:%Y%m%d%H%M%S%f}_{}".format(jid_dt, os.getpid()) +def jid_to_datetime(jid): + """ + Return a datetime object from jid + """ + return datetime.datetime.strptime(jid.split("_")[0], "%Y%m%d%H%M%S%f") + + def is_jid(jid): """ Returns True if the passed in value is a job id diff --git a/tests/pytests/unit/test_crypt.py b/tests/pytests/unit/test_crypt.py index aa8f439b8c77..3939aac06414 100644 --- a/tests/pytests/unit/test_crypt.py +++ b/tests/pytests/unit/test_crypt.py @@ -4,8 +4,12 @@ Unit tests for salt's crypt module """ + +import uuid + import pytest import salt.crypt +import salt.master import salt.utils.files @@ -18,3 +22,44 @@ def test_get_rsa_pub_key_bad_key(tmp_path): fp.write("") with pytest.raises(salt.crypt.InvalidKeyError): salt.crypt.get_rsa_pub_key(key_path) + + +def test_cryptical_dumps_no_nonce(): + master_crypt = salt.crypt.Crypticle({}, salt.crypt.Crypticle.generate_key_string()) + data = {"foo": "bar"} + ret = master_crypt.dumps(data) + + # Validate message structure + assert isinstance(ret, bytes) + une = master_crypt.decrypt(ret) + une.startswith(master_crypt.PICKLE_PAD) + assert salt.payload.loads(une[len(master_crypt.PICKLE_PAD) :]) == data + + # Validate load back to orig data + assert master_crypt.loads(ret) == data + + +def test_cryptical_dumps_valid_nonce(): + nonce = uuid.uuid4().hex + master_crypt = salt.crypt.Crypticle({}, salt.crypt.Crypticle.generate_key_string()) + data = {"foo": "bar"} + ret = master_crypt.dumps(data, nonce=nonce) + + assert isinstance(ret, bytes) + une = master_crypt.decrypt(ret) + une.startswith(master_crypt.PICKLE_PAD) + nonce_and_data = une[len(master_crypt.PICKLE_PAD) :] + assert nonce_and_data.startswith(nonce.encode()) + assert salt.payload.loads(nonce_and_data[len(nonce) :]) == data + + assert master_crypt.loads(ret, nonce=nonce) == data + + +def test_cryptical_dumps_invalid_nonce(): + nonce = uuid.uuid4().hex + master_crypt = salt.crypt.Crypticle({}, salt.crypt.Crypticle.generate_key_string()) + data = {"foo": "bar"} + ret = master_crypt.dumps(data, nonce=nonce) + assert isinstance(ret, bytes) + with pytest.raises(salt.crypt.SaltClientError, match="Nonce verification error"): + assert master_crypt.loads(ret, nonce="abcde") diff --git a/tests/pytests/unit/test_minion.py b/tests/pytests/unit/test_minion.py index 7de60c49e35d..e2030efac176 100644 --- a/tests/pytests/unit/test_minion.py +++ b/tests/pytests/unit/test_minion.py @@ -10,6 +10,7 @@ import salt.syspaths import salt.utils.crypt import salt.utils.event as event +import salt.utils.jid import salt.utils.platform import salt.utils.process from salt._compat import ipaddress @@ -945,3 +946,24 @@ def test_config_cache_path_overrides(): mminion = salt.minion.MasterMinion(opts) assert mminion.opts["cachedir"] == cachedir + + +async def test_minion_old_jid(tmpdir, caplog): + """ + Minion does not generate grains when load_grains is False + """ + opts = { + "random_startup_delay": 0, + "grains": {"foo": "bar"}, + "cachedir": str(tmpdir), + } + minion = salt.minion.Minion(opts, load_grains=False) + jid1 = salt.utils.jid.gen_jid(opts) + jid2 = salt.utils.jid.gen_jid(opts) + last_jid_path = os.path.join(opts["cachedir"], ".last_jid") + with salt.utils.files.fopen(last_jid_path, "w") as fp: + fp.write(jid2) + with caplog.at_level(logging.INFO): + ret = await minion._handle_decoded_payload({"jid": jid1}) + assert ret is None + assert "Received old JID, doing nothing" in caplog.text From 71cb857b28b9636f6f4ff2312b00f0b9aa702e02 Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Thu, 27 Jan 2022 18:28:10 -0700 Subject: [PATCH 54/66] Add changelog for cve-2022-22936 --- changelog/cve-2022-22936.security | 1 + salt/minion.py | 13 ------------- tests/pytests/unit/test_minion.py | 21 --------------------- 3 files changed, 1 insertion(+), 34 deletions(-) create mode 100644 changelog/cve-2022-22936.security diff --git a/changelog/cve-2022-22936.security b/changelog/cve-2022-22936.security new file mode 100644 index 000000000000..f33fdc83d4ad --- /dev/null +++ b/changelog/cve-2022-22936.security @@ -0,0 +1 @@ +Prevent job and fileserver replays diff --git a/salt/minion.py b/salt/minion.py index 25fa0c76cd19..63fb6e91eae8 100644 --- a/salt/minion.py +++ b/salt/minion.py @@ -1687,19 +1687,6 @@ def _handle_decoded_payload(self, data): Override this method if you wish to handle the decoded data differently. """ - # Do not re-play old jobs - last_jid_path = os.path.join(self.opts["cachedir"], ".last_jid") - if os.path.exists(last_jid_path): - with salt.utils.files.fopen(last_jid_path, "r") as fp: - last_jid = fp.read() - last_dt = salt.utils.jid.jid_to_datetime(last_jid) - this_dt = salt.utils.jid.jid_to_datetime(data["jid"]) - if last_dt >= this_dt: - log.error("Received old JID, doing nothing: %r %r", last_jid, data["jid"]) - return - - with salt.utils.files.fopen(last_jid_path, "w") as fp: - fp.write(data["jid"]) # Ensure payload is unicode. Disregard failure to decode binary blobs. if "user" in data: diff --git a/tests/pytests/unit/test_minion.py b/tests/pytests/unit/test_minion.py index e2030efac176..985ec99276f4 100644 --- a/tests/pytests/unit/test_minion.py +++ b/tests/pytests/unit/test_minion.py @@ -946,24 +946,3 @@ def test_config_cache_path_overrides(): mminion = salt.minion.MasterMinion(opts) assert mminion.opts["cachedir"] == cachedir - - -async def test_minion_old_jid(tmpdir, caplog): - """ - Minion does not generate grains when load_grains is False - """ - opts = { - "random_startup_delay": 0, - "grains": {"foo": "bar"}, - "cachedir": str(tmpdir), - } - minion = salt.minion.Minion(opts, load_grains=False) - jid1 = salt.utils.jid.gen_jid(opts) - jid2 = salt.utils.jid.gen_jid(opts) - last_jid_path = os.path.join(opts["cachedir"], ".last_jid") - with salt.utils.files.fopen(last_jid_path, "w") as fp: - fp.write(jid2) - with caplog.at_level(logging.INFO): - ret = await minion._handle_decoded_payload({"jid": jid1}) - assert ret is None - assert "Received old JID, doing nothing" in caplog.text From 3be9a1bb187b0f4c7f006ba849566b31da507cf7 Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Mon, 7 Feb 2022 15:58:30 -0700 Subject: [PATCH 55/66] Job replay mitigation --- salt/crypt.py | 47 ++++++++++++--- salt/master.py | 57 ++++++++++++++----- salt/transport/mixins/auth.py | 2 + salt/transport/tcp.py | 10 ++-- salt/transport/zeromq.py | 15 +++-- .../transport/server/test_req_channel.py | 16 +++++- .../zeromq/test_pub_server_channel.py | 40 ++++++++++--- 7 files changed, 146 insertions(+), 41 deletions(-) diff --git a/salt/crypt.py b/salt/crypt.py index de9c748feac7..f0535dcc169f 100644 --- a/salt/crypt.py +++ b/salt/crypt.py @@ -531,7 +531,7 @@ def __key(cls, opts, io_loop=None): def __init__(self, opts, io_loop=None): pass - # an init for the singleton instance to call + ## an init for the singleton instance to call def __singleton_init__(self, opts, io_loop=None): """ Init an Auth instance @@ -702,9 +702,17 @@ def _authenticate(self): self._authenticate_future.set_exception(error) else: key = self.__key(self.opts) - AsyncAuth.creds_map[key] = creds - self._creds = creds - self._crypticle = Crypticle(self.opts, creds["aes"]) + if key not in AsyncAuth.creds_map: + log.error("%s Got new master aes key.", self) + AsyncAuth.creds_map[key] = creds + self._creds = creds + self._crypticle = Crypticle(self.opts, creds["aes"]) + elif self._creds["aes"] != creds["aes"]: + log.error("%s The master's aes key has changed.", self) + AsyncAuth.creds_map[key] = creds + self._creds = creds + self._crypticle = Crypticle(self.opts, creds["aes"]) + self._authenticate_future.set_result( True ) # mark the sign-in as complete @@ -1280,6 +1288,7 @@ def __singleton_init__(self, opts, io_loop=None): self.token = salt.utils.stringutils.to_bytes(Crypticle.generate_key_string()) self.pub_path = os.path.join(self.opts["pki_dir"], "minion.pub") self.rsa_path = os.path.join(self.opts["pki_dir"], "minion.pem") + self._creds = None if "syndic_master" in self.opts: self.mpub = "syndic_master.pub" elif "alert_master" in self.opts: @@ -1349,8 +1358,14 @@ def authenticate(self, _=None): # TODO: remove unused var ) continue break - self._creds = creds - self._crypticle = Crypticle(self.opts, creds["aes"]) + if self._creds is None: + log.error("%s Got new master aes key.", self) + self._creds = creds + self._crypticle = Crypticle(self.opts, creds["aes"]) + elif self._creds["aes"] != creds["aes"]: + log.error("%s The master's aes key has changed.", self) + self._creds = creds + self._crypticle = Crypticle(self.opts, creds["aes"]) def sign_in(self, timeout=60, safe=True, tries=1, channel=None): """ @@ -1418,10 +1433,11 @@ class Crypticle: AES_BLOCK_SIZE = 16 SIG_SIZE = hashlib.sha256().digest_size - def __init__(self, opts, key_string, key_size=192): + def __init__(self, opts, key_string, key_size=192, serial=0): self.key_string = key_string self.keys = self.extract_keys(self.key_string, key_size) self.key_size = key_size + self.serial = serial @classmethod def generate_key_string(cls, key_size=192): @@ -1515,4 +1531,19 @@ def loads(self, data, raw=False, nonce=None): data = data[32:] if ret_nonce != nonce: raise SaltClientError("Nonce verification error") - return salt.payload.loads(data, raw=raw) + payload = salt.payload.loads(data, raw=raw) + if isinstance(payload, dict): + if "serial" in payload: + serial = payload.pop("serial") + if serial <= self.serial: + log.critical( + "A message with an invalid serial was received.\n" + "this serial: %d\n" + "last serial: %d\n" + "The minion will not honor this request.", + serial, + self.serial, + ) + return {} + self.serial = serial + return payload diff --git a/salt/master.py b/salt/master.py index 64940119ee6a..adc735329a2d 100644 --- a/salt/master.py +++ b/salt/master.py @@ -128,6 +128,44 @@ def __prep_key(self): """ return salt.daemons.masterapi.access_keys(self.opts) + @classmethod + def get_serial(cls, opts=None, event=None): + with cls.secrets["aes"]["secret"].get_lock(): + if cls.secrets["aes"]["serial"].value == sys.maxsize: + cls.rotate_secrets(opts, event, use_lock=False) + else: + cls.secrets["aes"]["serial"].value += 1 + return cls.secrets["aes"]["serial"].value + + @classmethod + def rotate_secrets(cls, opts=None, event=None, use_lock=True): + log.info("Rotating master AES key") + if opts is None: + opts = {} + + for secret_key, secret_map in cls.secrets.items(): + # should be unnecessary-- since no one else should be modifying + if use_lock: + with secret_map["secret"].get_lock(): + secret_map["secret"].value = salt.utils.stringutils.to_bytes( + secret_map["reload"]() + ) + if "serial" in secret_map: + secret_map["serial"].value = 0 + else: + secret_map["secret"].value = salt.utils.stringutils.to_bytes( + secret_map["reload"]() + ) + if "serial" in secret_map: + secret_map["serial"].value = 0 + if event: + event.fire_event({"rotate_{}_key".format(secret_key): True}, tag="key") + + if opts.get("ping_on_rotate"): + # Ping all minions to get them to pick up the new key + log.debug("Pinging all connected minions due to key rotation") + salt.utils.master.ping_all_connected_minions(opts) + class Maintenance(salt.utils.process.SignalHandlingProcess): """ @@ -280,21 +318,8 @@ def handle_key_rotate(self, now): to_rotate = True if to_rotate: - log.info("Rotating master AES key") - for secret_key, secret_map in SMaster.secrets.items(): - # should be unnecessary-- since no one else should be modifying - with secret_map["secret"].get_lock(): - secret_map["secret"].value = salt.utils.stringutils.to_bytes( - secret_map["reload"]() - ) - self.event.fire_event( - {"rotate_{}_key".format(secret_key): True}, tag="key" - ) + SMaster.rotate_secrets(self.opts, self.event) self.rotate = now - if self.opts.get("ping_on_rotate"): - # Ping all minions to get them to pick up the new key - log.debug("Pinging all connected minions due to key rotation") - salt.utils.master.ping_all_connected_minions(self.opts) def handle_git_pillar(self): """ @@ -670,8 +695,12 @@ def start(self): salt.crypt.Crypticle.generate_key_string() ), ), + "serial": multiprocessing.Value( + ctypes.c_longlong, lock=False # We'll use the lock from 'secret' + ), "reload": salt.crypt.Crypticle.generate_key_string, } + log.info("Creating master process manager") # Since there are children having their own ProcessManager we should wait for kill more time. self.process_manager = salt.utils.process.ProcessManager(wait_for_kill=5) diff --git a/salt/transport/mixins/auth.py b/salt/transport/mixins/auth.py index 8381ff01c3d2..5f5e9b34a5a5 100644 --- a/salt/transport/mixins/auth.py +++ b/salt/transport/mixins/auth.py @@ -603,6 +603,8 @@ def _auth(self, load, sign_messages=False): ret["aes"] = pub.public_encrypt(aes, RSA.pkcs1_oaep_padding) else: ret["aes"] = cipher.encrypt(aes) + # ret["serial"] = salt.master.SMaster.get_serial() + ret["serial"] = 0 # Be aggressive about the signature digest = salt.utils.stringutils.to_bytes(hashlib.sha256(aes).hexdigest()) ret["sig"] = salt.crypt.private_encrypt(self.master_key.key, digest) diff --git a/salt/transport/tcp.py b/salt/transport/tcp.py index 452e8e4d4421..cf2f0413f161 100644 --- a/salt/transport/tcp.py +++ b/salt/transport/tcp.py @@ -321,7 +321,9 @@ def _crypted_transfer(self, load, tries=3, timeout=60): Indeed, we can fail too early in case of a master restart during a minion state execution call """ - load["nonce"] = uuid.uuid4().hex + nonce = uuid.uuid4().hex + if load and isinstance(load, dict): + load["nonce"] = nonce @salt.ext.tornado.gen.coroutine def _do_transfer(): @@ -335,7 +337,7 @@ def _do_transfer(): # communication, we do not subscribe to return events, we just # upload the results to the master if data: - data = self.auth.crypticle.loads(data, nonce=load.get("nonce")) + data = self.auth.crypticle.loads(data, nonce=nonce) data = salt.transport.frame.decode_embedded_strs(data) raise salt.ext.tornado.gen.Return(data) @@ -760,7 +762,7 @@ def handle_message(self, stream, header, payload): elif req_fun == "send": nonce = None if version > 1: - nonce = payload["load"]["nonce"] + nonce = payload["load"].pop("nonce") stream.write( salt.transport.frame.frame_msg( self.crypticle.dumps(ret, nonce), header=header @@ -1682,7 +1684,7 @@ def publish(self, load): Publish "load" to minions """ payload = {"enc": "aes"} - + load["serial"] = salt.master.SMaster.get_serial() crypticle = salt.crypt.Crypticle( self.opts, salt.master.SMaster.secrets["aes"]["secret"].value ) diff --git a/salt/transport/zeromq.py b/salt/transport/zeromq.py index ab67f9ccfe29..a722a4b17eaa 100644 --- a/salt/transport/zeromq.py +++ b/salt/transport/zeromq.py @@ -68,6 +68,7 @@ def _get_master_uri(master_ip, master_port, source_ip=None, source_port=None): rc = zmq_connect(socket, "tcp://192.168.1.17:5555;192.168.1.1:5555"); assert (rc == 0); Source: http://api.zeromq.org/4-1:zmq-tcp """ + from salt.utils.zeromq import ip_bracket master_uri = "tcp://{master_ip}:{master_port}".format( @@ -287,7 +288,9 @@ def _crypted_transfer(self, load, tries=3, timeout=60, raw=False): :param int tries: The number of times to make before failure :param int timeout: The number of seconds on a response before failing """ - load["nonce"] = uuid.uuid4().hex + nonce = uuid.uuid4().hex + if load and isinstance(load, dict): + load["nonce"] = nonce @salt.ext.tornado.gen.coroutine def _do_transfer(): @@ -302,7 +305,7 @@ def _do_transfer(): # communication, we do not subscribe to return events, we just # upload the results to the master if data: - data = self.auth.crypticle.loads(data, raw, load.get("nonce")) + data = self.auth.crypticle.loads(data, raw, nonce) if not raw: data = salt.transport.frame.decode_embedded_strs(data) raise salt.ext.tornado.gen.Return(data) @@ -777,6 +780,10 @@ def handle_message(self, stream, payload): stream.send(salt.payload.dumps(self._auth(payload["load"], sign_messages))) raise salt.ext.tornado.gen.Return() + nonce = None + if version > 1: + nonce = payload["load"].pop("nonce", None) + # TODO: test try: # Take the payload_handler function that was registered when we created the channel @@ -792,9 +799,6 @@ def handle_message(self, stream, payload): if req_fun == "send_clear": stream.send(salt.payload.dumps(ret)) elif req_fun == "send": - nonce = None - if version > 1: - nonce = payload["load"]["nonce"] stream.send(salt.payload.dumps(self.crypticle.dumps(ret, nonce))) elif req_fun == "send_private": sign_messages = False @@ -1064,6 +1068,7 @@ def publish(self, load): :param dict load: A load to be sent across the wire to minions """ payload = {"enc": "aes"} + load["serial"] = salt.master.SMaster.get_serial() crypticle = salt.crypt.Crypticle( self.opts, salt.master.SMaster.secrets["aes"]["secret"].value ) diff --git a/tests/pytests/functional/transport/server/test_req_channel.py b/tests/pytests/functional/transport/server/test_req_channel.py index 7a392cd75879..17d8861ccfd1 100644 --- a/tests/pytests/functional/transport/server/test_req_channel.py +++ b/tests/pytests/functional/transport/server/test_req_channel.py @@ -1,3 +1,4 @@ +import ctypes import logging import multiprocessing @@ -6,6 +7,7 @@ import salt.exceptions import salt.ext.tornado.gen import salt.log.setup +import salt.master import salt.transport.client import salt.transport.server import salt.utils.platform @@ -33,6 +35,18 @@ def __init__(self, config, req_channel_crypt): self.running = multiprocessing.Event() def run(self): + salt.master.SMaster.secrets["aes"] = { + "secret": multiprocessing.Array( + ctypes.c_char, + salt.utils.stringutils.to_bytes( + salt.crypt.Crypticle.generate_key_string() + ), + ), + "serial": multiprocessing.Value( + ctypes.c_longlong, lock=False # We'll use the lock from 'secret' + ), + } + self.io_loop = salt.ext.tornado.ioloop.IOLoop() self.io_loop.make_current() self.req_server_channel.post_fork(self._handle_payload, io_loop=self.io_loop) @@ -121,7 +135,7 @@ def test_basic(req_channel): {"baz": "qux", "list": [1, 2, 3]}, ] for msg in msgs: - ret = req_channel.send(msg, timeout=5, tries=1) + ret = req_channel.send(dict(msg), timeout=5, tries=1) assert ret["load"] == msg diff --git a/tests/pytests/functional/transport/zeromq/test_pub_server_channel.py b/tests/pytests/functional/transport/zeromq/test_pub_server_channel.py index 9e183c11e037..81240f5264dc 100644 --- a/tests/pytests/functional/transport/zeromq/test_pub_server_channel.py +++ b/tests/pytests/functional/transport/zeromq/test_pub_server_channel.py @@ -10,6 +10,7 @@ import salt.ext.tornado.gen import salt.ext.tornado.ioloop import salt.log.setup +import salt.master import salt.transport.client import salt.transport.server import salt.transport.zeromq @@ -40,6 +41,21 @@ def __init__( self.started = multiprocessing.Event() self.running = multiprocessing.Event() + def _rotate_secrets(self, now=None): + salt.master.SMaster.secrets["aes"] = { + "secret": multiprocessing.Array( + ctypes.c_char, + salt.utils.stringutils.to_bytes( + salt.crypt.Crypticle.generate_key_string() + ), + ), + "serial": multiprocessing.Value( + ctypes.c_longlong, lock=False # We'll use the lock from 'secret' + ), + "reload": salt.crypt.Crypticle.generate_key_string, + "rotate_master_key": self._rotate_secrets, + } + def run(self): """ Gather results until then number of seconds specified by timeout passes @@ -67,12 +83,15 @@ def run(self): try: serial_payload = salt.payload.loads(payload) payload = crypticle.loads(serial_payload["load"]) + if not payload: + continue if "start" in payload: self.running.set() continue if "stop" in payload: break last_msg = time.time() + log.error("WTFSON %r", payload) self.results.append(payload["jid"]) except salt.exceptions.SaltDeserializationError: if not self.zmq_filtering: @@ -108,10 +127,7 @@ def __init__(self, master_config, minion_config, **collector_kwargs): self.master_config = master_config self.minion_config = minion_config self.collector_kwargs = collector_kwargs - self.aes_key = multiprocessing.Array( - ctypes.c_char, - salt.utils.stringutils.to_bytes(salt.crypt.Crypticle.generate_key_string()), - ) + self.aes_key = salt.crypt.Crypticle.generate_key_string() self.process_manager = salt.utils.process.ProcessManager( name="ZMQ-PubServer-ProcessManager" ) @@ -126,14 +142,20 @@ def __init__(self, master_config, minion_config, **collector_kwargs): self.queue = multiprocessing.Queue() self.stopped = multiprocessing.Event() self.collector = Collector( - self.minion_config, - self.pub_uri, - self.aes_key.value, - **self.collector_kwargs + self.minion_config, self.pub_uri, self.aes_key, **self.collector_kwargs ) def run(self): - salt.master.SMaster.secrets["aes"] = {"secret": self.aes_key} + if "aes" not in salt.master.SMaster.secrets: + salt.master.SMaster.secrets["aes"] = { + "secret": multiprocessing.Array( + ctypes.c_char, + salt.utils.stringutils.to_bytes(self.aes_key), + ), + "serial": multiprocessing.Value( + ctypes.c_longlong, lock=False # We'll use the lock from 'secret' + ), + } try: while True: payload = self.queue.get() From 6f26378ef30246f67b56bbed2de3f7549179fe42 Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Tue, 8 Feb 2022 14:40:52 -0700 Subject: [PATCH 56/66] Fix merge warts --- salt/transport/tcp.py | 12 ++++-------- salt/transport/zeromq.py | 5 ----- 2 files changed, 4 insertions(+), 13 deletions(-) diff --git a/salt/transport/tcp.py b/salt/transport/tcp.py index cf2f0413f161..9665b98f4f81 100644 --- a/salt/transport/tcp.py +++ b/salt/transport/tcp.py @@ -744,6 +744,10 @@ def handle_message(self, stream, header, payload): ) raise salt.ext.tornado.gen.Return() + nonce = None + if version > 1: + nonce = payload["load"].pop("nonce", None) + # TODO: test try: ret, req_opts = yield self.payload_handler(payload) @@ -760,20 +764,12 @@ def handle_message(self, stream, header, payload): if req_fun == "send_clear": stream.write(salt.transport.frame.frame_msg(ret, header=header)) elif req_fun == "send": - nonce = None - if version > 1: - nonce = payload["load"].pop("nonce") stream.write( salt.transport.frame.frame_msg( self.crypticle.dumps(ret, nonce), header=header ) ) elif req_fun == "send_private": - sign_messages = False - nonce = None - if version > 1: - sign_messages = True - nonce = payload["load"].get("nonce") stream.write( salt.transport.frame.frame_msg( self._encrypt_private( diff --git a/salt/transport/zeromq.py b/salt/transport/zeromq.py index a722a4b17eaa..6e65a44ea2fc 100644 --- a/salt/transport/zeromq.py +++ b/salt/transport/zeromq.py @@ -801,11 +801,6 @@ def handle_message(self, stream, payload): elif req_fun == "send": stream.send(salt.payload.dumps(self.crypticle.dumps(ret, nonce))) elif req_fun == "send_private": - sign_messages = False - nonce = None - if version > 1: - sign_messages = True - nonce = payload["load"]["nonce"] stream.send( salt.payload.dumps( self._encrypt_private( From 44f1bdd2ed253160aa0352e9d352df196a7228d6 Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Thu, 10 Feb 2022 22:05:06 -0700 Subject: [PATCH 57/66] more test fixes --- requirements/static/ci/linux.in | 2 ++ .../zeromq/test_pub_server_channel.py | 19 +++++++++---------- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/requirements/static/ci/linux.in b/requirements/static/ci/linux.in index 9ea8d7405d0f..638c07af867a 100644 --- a/requirements/static/ci/linux.in +++ b/requirements/static/ci/linux.in @@ -8,3 +8,5 @@ ansible twilio python-telegram-bot==12.8; python_version <= '3.5' python-telegram-bot>=13.7; python_version > '3.5' +mercurial +ghlab diff --git a/tests/pytests/functional/transport/zeromq/test_pub_server_channel.py b/tests/pytests/functional/transport/zeromq/test_pub_server_channel.py index 81240f5264dc..6bbb38ad3630 100644 --- a/tests/pytests/functional/transport/zeromq/test_pub_server_channel.py +++ b/tests/pytests/functional/transport/zeromq/test_pub_server_channel.py @@ -146,16 +146,15 @@ def __init__(self, master_config, minion_config, **collector_kwargs): ) def run(self): - if "aes" not in salt.master.SMaster.secrets: - salt.master.SMaster.secrets["aes"] = { - "secret": multiprocessing.Array( - ctypes.c_char, - salt.utils.stringutils.to_bytes(self.aes_key), - ), - "serial": multiprocessing.Value( - ctypes.c_longlong, lock=False # We'll use the lock from 'secret' - ), - } + salt.master.SMaster.secrets["aes"] = { + "secret": multiprocessing.Array( + ctypes.c_char, + salt.utils.stringutils.to_bytes(self.aes_key), + ), + "serial": multiprocessing.Value( + ctypes.c_longlong, lock=False # We'll use the lock from 'secret' + ), + } try: while True: payload = self.queue.get() From 13a0e69aecaa09740f2d4f4f9ca15c91c537e094 Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Sat, 12 Feb 2022 21:08:28 +0000 Subject: [PATCH 58/66] Fix auth tests on windows --- tests/pytests/unit/transport/test_zeromq.py | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/tests/pytests/unit/transport/test_zeromq.py b/tests/pytests/unit/transport/test_zeromq.py index 643606304986..fccf0073ee8d 100644 --- a/tests/pytests/unit/transport/test_zeromq.py +++ b/tests/pytests/unit/transport/test_zeromq.py @@ -910,11 +910,17 @@ async def test_req_serv_auth_v1(pki_dir): pub = salt.crypt.get_rsa_pub_key(str(pki_dir.join("minion", "minion.pub"))) token = salt.utils.stringutils.to_bytes(salt.crypt.Crypticle.generate_key_string()) nonce = uuid.uuid4().hex + + # We need to read the public key with fopen otherwise the newlines might + # not match on windows. + with salt.utils.files.fopen(str(pki_dir.join("minion", "minion.pub")), "r") as fp: + pub_key = fp.read() + load = { "cmd": "_auth", "id": "minion", "token": token, - "pub": MINION_PUB_KEY.strip(), + "pub": pub_key, } ret = server._auth(load, sign_messages=False) assert "load" not in ret @@ -956,12 +962,18 @@ async def test_req_serv_auth_v2(pki_dir): pub = salt.crypt.get_rsa_pub_key(str(pki_dir.join("minion", "minion.pub"))) token = salt.utils.stringutils.to_bytes(salt.crypt.Crypticle.generate_key_string()) nonce = uuid.uuid4().hex + + # We need to read the public key with fopen otherwise the newlines might + # not match on windows. + with salt.utils.files.fopen(str(pki_dir.join("minion", "minion.pub")), "r") as fp: + pub_key = fp.read() + load = { "cmd": "_auth", "id": "minion", "nonce": nonce, "token": token, - "pub": MINION_PUB_KEY.strip(), + "pub": pub_key, } ret = server._auth(load, sign_messages=True) assert "sig" in ret From 2a050b89709d0338700c6fbdeeeb9e4e3713d850 Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Mon, 14 Feb 2022 12:07:32 -0700 Subject: [PATCH 59/66] Remove unwanted requirements change --- requirements/static/ci/linux.in | 2 -- 1 file changed, 2 deletions(-) diff --git a/requirements/static/ci/linux.in b/requirements/static/ci/linux.in index 638c07af867a..9ea8d7405d0f 100644 --- a/requirements/static/ci/linux.in +++ b/requirements/static/ci/linux.in @@ -8,5 +8,3 @@ ansible twilio python-telegram-bot==12.8; python_version <= '3.5' python-telegram-bot>=13.7; python_version > '3.5' -mercurial -ghlab From 04d888c34666320dcfa3bdc8736d9573a61a006e Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Mon, 14 Feb 2022 14:25:09 -0700 Subject: [PATCH 60/66] Clean up cruft --- .../{cve-2020-22935.security => cve-2022-22935.security} | 0 .../{cve-2020-22937.security => cve-2022-22937.security} | 0 salt/crypt.py | 6 +++--- salt/transport/mixins/auth.py | 3 +-- salt/utils/jid.py | 7 ------- .../functional/transport/zeromq/test_pub_server_channel.py | 1 - 6 files changed, 4 insertions(+), 13 deletions(-) rename changelog/{cve-2020-22935.security => cve-2022-22935.security} (100%) rename changelog/{cve-2020-22937.security => cve-2022-22937.security} (100%) diff --git a/changelog/cve-2020-22935.security b/changelog/cve-2022-22935.security similarity index 100% rename from changelog/cve-2020-22935.security rename to changelog/cve-2022-22935.security diff --git a/changelog/cve-2020-22937.security b/changelog/cve-2022-22937.security similarity index 100% rename from changelog/cve-2020-22937.security rename to changelog/cve-2022-22937.security diff --git a/salt/crypt.py b/salt/crypt.py index f0535dcc169f..b8a96ff29011 100644 --- a/salt/crypt.py +++ b/salt/crypt.py @@ -531,7 +531,7 @@ def __key(cls, opts, io_loop=None): def __init__(self, opts, io_loop=None): pass - ## an init for the singleton instance to call + # an init for the singleton instance to call def __singleton_init__(self, opts, io_loop=None): """ Init an Auth instance @@ -703,12 +703,12 @@ def _authenticate(self): else: key = self.__key(self.opts) if key not in AsyncAuth.creds_map: - log.error("%s Got new master aes key.", self) + log.debug("%s Got new master aes key.", self) AsyncAuth.creds_map[key] = creds self._creds = creds self._crypticle = Crypticle(self.opts, creds["aes"]) elif self._creds["aes"] != creds["aes"]: - log.error("%s The master's aes key has changed.", self) + log.debug("%s The master's aes key has changed.", self) AsyncAuth.creds_map[key] = creds self._creds = creds self._crypticle = Crypticle(self.opts, creds["aes"]) diff --git a/salt/transport/mixins/auth.py b/salt/transport/mixins/auth.py index 5f5e9b34a5a5..1e2e8e6b7bfa 100644 --- a/salt/transport/mixins/auth.py +++ b/salt/transport/mixins/auth.py @@ -603,8 +603,7 @@ def _auth(self, load, sign_messages=False): ret["aes"] = pub.public_encrypt(aes, RSA.pkcs1_oaep_padding) else: ret["aes"] = cipher.encrypt(aes) - # ret["serial"] = salt.master.SMaster.get_serial() - ret["serial"] = 0 + # Be aggressive about the signature digest = salt.utils.stringutils.to_bytes(hashlib.sha256(aes).hexdigest()) ret["sig"] = salt.crypt.private_encrypt(self.master_key.key, digest) diff --git a/salt/utils/jid.py b/salt/utils/jid.py index 35914a1043d1..c3dbf8a078af 100644 --- a/salt/utils/jid.py +++ b/salt/utils/jid.py @@ -34,13 +34,6 @@ def gen_jid(opts): return "{:%Y%m%d%H%M%S%f}_{}".format(jid_dt, os.getpid()) -def jid_to_datetime(jid): - """ - Return a datetime object from jid - """ - return datetime.datetime.strptime(jid.split("_")[0], "%Y%m%d%H%M%S%f") - - def is_jid(jid): """ Returns True if the passed in value is a job id diff --git a/tests/pytests/functional/transport/zeromq/test_pub_server_channel.py b/tests/pytests/functional/transport/zeromq/test_pub_server_channel.py index 6bbb38ad3630..5633bceafec3 100644 --- a/tests/pytests/functional/transport/zeromq/test_pub_server_channel.py +++ b/tests/pytests/functional/transport/zeromq/test_pub_server_channel.py @@ -91,7 +91,6 @@ def run(self): if "stop" in payload: break last_msg = time.time() - log.error("WTFSON %r", payload) self.results.append(payload["jid"]) except salt.exceptions.SaltDeserializationError: if not self.zmq_filtering: From 697a3f96cff58ac7894468c0ae60d2b605d1fcc2 Mon Sep 17 00:00:00 2001 From: Frode Gundersen Date: Wed, 16 Feb 2022 09:34:44 -0700 Subject: [PATCH 61/66] update docs for 3004.1 release --- CHANGELOG.md | 13 +++++++++++++ changelog/60413.security | 1 - changelog/cve-2022-22935.security | 1 - changelog/cve-2022-22936.security | 1 - changelog/cve-2022-22937.security | 1 - changelog/cve-2202-22934.security | 1 - doc/man/salt-api.1 | 2 +- doc/man/salt-call.1 | 2 +- doc/man/salt-cloud.1 | 2 +- doc/man/salt-cp.1 | 2 +- doc/man/salt-key.1 | 2 +- doc/man/salt-master.1 | 2 +- doc/man/salt-minion.1 | 2 +- doc/man/salt-proxy.1 | 2 +- doc/man/salt-run.1 | 2 +- doc/man/salt-ssh.1 | 2 +- doc/man/salt-syndic.1 | 2 +- doc/man/salt-unity.1 | 2 +- doc/man/salt.1 | 2 +- doc/man/salt.7 | 4 ++-- doc/man/spm.1 | 2 +- doc/topics/releases/3004.1.rst | 16 ++++++++++++++++ 22 files changed, 45 insertions(+), 21 deletions(-) delete mode 100644 changelog/60413.security delete mode 100644 changelog/cve-2022-22935.security delete mode 100644 changelog/cve-2022-22936.security delete mode 100644 changelog/cve-2022-22937.security delete mode 100644 changelog/cve-2202-22934.security create mode 100644 doc/topics/releases/3004.1.rst diff --git a/CHANGELOG.md b/CHANGELOG.md index 9aab14560448..c241974ccf68 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,19 @@ Versions are `MAJOR.PATCH`. # Changelog +Salt 3004.1 (2022-02-16) +======================== + +Security +-------- + +- Sign authentication replies to prevent MiTM (cve-2022-22935) +- Prevent job and fileserver replays (cve-2022-22936) +- Fix denial of service in junos ifconfig output parsing. (cve-2022-22937) +- Sign pillar data to prevent MiTM attacks. (cve-2202-22934) +- Fixed targeting bug, especially visible when using syndic and user auth. (CVE-2022-22941) (#60413) + + Salt 3004 (2021-10-11) ====================== diff --git a/changelog/60413.security b/changelog/60413.security deleted file mode 100644 index 14eecd9b0d2f..000000000000 --- a/changelog/60413.security +++ /dev/null @@ -1 +0,0 @@ -Fixed targeting bug, especially visible when using syndic and user auth. (CVE-2022-22941) diff --git a/changelog/cve-2022-22935.security b/changelog/cve-2022-22935.security deleted file mode 100644 index fd26c907b643..000000000000 --- a/changelog/cve-2022-22935.security +++ /dev/null @@ -1 +0,0 @@ -Sign authentication replies to prevent MiTM diff --git a/changelog/cve-2022-22936.security b/changelog/cve-2022-22936.security deleted file mode 100644 index f33fdc83d4ad..000000000000 --- a/changelog/cve-2022-22936.security +++ /dev/null @@ -1 +0,0 @@ -Prevent job and fileserver replays diff --git a/changelog/cve-2022-22937.security b/changelog/cve-2022-22937.security deleted file mode 100644 index ce0bc99125cc..000000000000 --- a/changelog/cve-2022-22937.security +++ /dev/null @@ -1 +0,0 @@ -Fix denial of service in junos ifconfig output parsing. diff --git a/changelog/cve-2202-22934.security b/changelog/cve-2202-22934.security deleted file mode 100644 index 7e6c0ceccaf0..000000000000 --- a/changelog/cve-2202-22934.security +++ /dev/null @@ -1 +0,0 @@ -Sign pillar data to prevent MiTM attacks. diff --git a/doc/man/salt-api.1 b/doc/man/salt-api.1 index dbcc5b18056f..fa9c708c7837 100644 --- a/doc/man/salt-api.1 +++ b/doc/man/salt-api.1 @@ -1,6 +1,6 @@ .\" Man page generated from reStructuredText. . -.TH "SALT-API" "1" "Sep 27, 2021" "3004" "Salt" +.TH "SALT-API" "1" "Feb 16, 2022" "3004.1" "Salt" .SH NAME salt-api \- salt-api Command . diff --git a/doc/man/salt-call.1 b/doc/man/salt-call.1 index 0111afe9789e..a0aadbdb8ba3 100644 --- a/doc/man/salt-call.1 +++ b/doc/man/salt-call.1 @@ -1,6 +1,6 @@ .\" Man page generated from reStructuredText. . -.TH "SALT-CALL" "1" "Sep 27, 2021" "3004" "Salt" +.TH "SALT-CALL" "1" "Feb 16, 2022" "3004.1" "Salt" .SH NAME salt-call \- salt-call Documentation . diff --git a/doc/man/salt-cloud.1 b/doc/man/salt-cloud.1 index e39b7671d16c..b12faa75d186 100644 --- a/doc/man/salt-cloud.1 +++ b/doc/man/salt-cloud.1 @@ -1,6 +1,6 @@ .\" Man page generated from reStructuredText. . -.TH "SALT-CLOUD" "1" "Sep 27, 2021" "3004" "Salt" +.TH "SALT-CLOUD" "1" "Feb 16, 2022" "3004.1" "Salt" .SH NAME salt-cloud \- Salt Cloud Command . diff --git a/doc/man/salt-cp.1 b/doc/man/salt-cp.1 index 084b57dd2ae2..debd293f2be6 100644 --- a/doc/man/salt-cp.1 +++ b/doc/man/salt-cp.1 @@ -1,6 +1,6 @@ .\" Man page generated from reStructuredText. . -.TH "SALT-CP" "1" "Sep 27, 2021" "3004" "Salt" +.TH "SALT-CP" "1" "Feb 16, 2022" "3004.1" "Salt" .SH NAME salt-cp \- salt-cp Documentation . diff --git a/doc/man/salt-key.1 b/doc/man/salt-key.1 index cd966e521d11..25f364a3c159 100644 --- a/doc/man/salt-key.1 +++ b/doc/man/salt-key.1 @@ -1,6 +1,6 @@ .\" Man page generated from reStructuredText. . -.TH "SALT-KEY" "1" "Sep 27, 2021" "3004" "Salt" +.TH "SALT-KEY" "1" "Feb 16, 2022" "3004.1" "Salt" .SH NAME salt-key \- salt-key Documentation . diff --git a/doc/man/salt-master.1 b/doc/man/salt-master.1 index 8a0ffeb49993..e3251582714f 100644 --- a/doc/man/salt-master.1 +++ b/doc/man/salt-master.1 @@ -1,6 +1,6 @@ .\" Man page generated from reStructuredText. . -.TH "SALT-MASTER" "1" "Sep 27, 2021" "3004" "Salt" +.TH "SALT-MASTER" "1" "Feb 16, 2022" "3004.1" "Salt" .SH NAME salt-master \- salt-master Documentation . diff --git a/doc/man/salt-minion.1 b/doc/man/salt-minion.1 index b4e8e00285e3..b25bf8d86762 100644 --- a/doc/man/salt-minion.1 +++ b/doc/man/salt-minion.1 @@ -1,6 +1,6 @@ .\" Man page generated from reStructuredText. . -.TH "SALT-MINION" "1" "Sep 27, 2021" "3004" "Salt" +.TH "SALT-MINION" "1" "Feb 16, 2022" "3004.1" "Salt" .SH NAME salt-minion \- salt-minion Documentation . diff --git a/doc/man/salt-proxy.1 b/doc/man/salt-proxy.1 index a0f4f7e0703d..740f441ecfe4 100644 --- a/doc/man/salt-proxy.1 +++ b/doc/man/salt-proxy.1 @@ -1,6 +1,6 @@ .\" Man page generated from reStructuredText. . -.TH "SALT-PROXY" "1" "Sep 27, 2021" "3004" "Salt" +.TH "SALT-PROXY" "1" "Feb 16, 2022" "3004.1" "Salt" .SH NAME salt-proxy \- salt-proxy Documentation . diff --git a/doc/man/salt-run.1 b/doc/man/salt-run.1 index 8f6f6279c182..da5985e2ad26 100644 --- a/doc/man/salt-run.1 +++ b/doc/man/salt-run.1 @@ -1,6 +1,6 @@ .\" Man page generated from reStructuredText. . -.TH "SALT-RUN" "1" "Sep 27, 2021" "3004" "Salt" +.TH "SALT-RUN" "1" "Feb 16, 2022" "3004.1" "Salt" .SH NAME salt-run \- salt-run Documentation . diff --git a/doc/man/salt-ssh.1 b/doc/man/salt-ssh.1 index 4aef4093b682..ae34c6ea7129 100644 --- a/doc/man/salt-ssh.1 +++ b/doc/man/salt-ssh.1 @@ -1,6 +1,6 @@ .\" Man page generated from reStructuredText. . -.TH "SALT-SSH" "1" "Sep 27, 2021" "3004" "Salt" +.TH "SALT-SSH" "1" "Feb 16, 2022" "3004.1" "Salt" .SH NAME salt-ssh \- salt-ssh Documentation . diff --git a/doc/man/salt-syndic.1 b/doc/man/salt-syndic.1 index f6d08faf56af..5ccb6fb3a603 100644 --- a/doc/man/salt-syndic.1 +++ b/doc/man/salt-syndic.1 @@ -1,6 +1,6 @@ .\" Man page generated from reStructuredText. . -.TH "SALT-SYNDIC" "1" "Sep 27, 2021" "3004" "Salt" +.TH "SALT-SYNDIC" "1" "Feb 16, 2022" "3004.1" "Salt" .SH NAME salt-syndic \- salt-syndic Documentation . diff --git a/doc/man/salt-unity.1 b/doc/man/salt-unity.1 index e8262593dc63..80fa599916c2 100644 --- a/doc/man/salt-unity.1 +++ b/doc/man/salt-unity.1 @@ -1,6 +1,6 @@ .\" Man page generated from reStructuredText. . -.TH "SALT-UNITY" "1" "Sep 27, 2021" "3004" "Salt" +.TH "SALT-UNITY" "1" "Feb 16, 2022" "3004.1" "Salt" .SH NAME salt-unity \- salt-unity Command . diff --git a/doc/man/salt.1 b/doc/man/salt.1 index 1c35eef820a8..108d38ac0943 100644 --- a/doc/man/salt.1 +++ b/doc/man/salt.1 @@ -1,6 +1,6 @@ .\" Man page generated from reStructuredText. . -.TH "SALT" "1" "Sep 27, 2021" "3004" "Salt" +.TH "SALT" "1" "Feb 16, 2022" "3004.1" "Salt" .SH NAME salt \- salt . diff --git a/doc/man/salt.7 b/doc/man/salt.7 index dfc1fd110318..618c63080eda 100644 --- a/doc/man/salt.7 +++ b/doc/man/salt.7 @@ -1,6 +1,6 @@ .\" Man page generated from reStructuredText. . -.TH "SALT" "7" "Sep 27, 2021" "3004" "Salt" +.TH "SALT" "7" "Feb 16, 2022" "3004.1" "Salt" .SH NAME salt \- Salt Documentation . @@ -193862,7 +193862,7 @@ Passes through all the parameters described in the \fI\%utils.http.query function\fP: .INDENT 7.0 .TP -.B salt.utils.http.query(url, method=\(aqGET\(aq, params=None, data=None, data_file=None, header_dict=None, header_list=None, header_file=None, username=None, password=None, auth=None, decode=False, decode_type=\(aqauto\(aq, status=False, headers=False, text=False, cookies=None, cookie_jar=None, cookie_format=\(aqlwp\(aq, persist_session=False, session_cookie_jar=None, data_render=False, data_renderer=None, header_render=False, header_renderer=None, template_dict=None, test=False, test_url=None, node=\(aqminion\(aq, port=80, opts=None, backend=None, ca_bundle=None, verify_ssl=None, cert=None, text_out=None, headers_out=None, decode_out=None, stream=False, streaming_callback=None, header_callback=None, handle=False, agent=\(aqSalt/3004rc0+315.g248e4e042c\(aq, hide_fields=None, raise_error=True, formdata=False, formdata_fieldname=None, formdata_filename=None, decode_body=True, **kwargs) +.B salt.utils.http.query(url, method=\(aqGET\(aq, params=None, data=None, data_file=None, header_dict=None, header_list=None, header_file=None, username=None, password=None, auth=None, decode=False, decode_type=\(aqauto\(aq, status=False, headers=False, text=False, cookies=None, cookie_jar=None, cookie_format=\(aqlwp\(aq, persist_session=False, session_cookie_jar=None, data_render=False, data_renderer=None, header_render=False, header_renderer=None, template_dict=None, test=False, test_url=None, node=\(aqminion\(aq, port=80, opts=None, backend=None, ca_bundle=None, verify_ssl=None, cert=None, text_out=None, headers_out=None, decode_out=None, stream=False, streaming_callback=None, header_callback=None, handle=False, agent=\(aqSalt/3004.1(aq, hide_fields=None, raise_error=True, formdata=False, formdata_fieldname=None, formdata_filename=None, decode_body=True, **kwargs) Query a resource, and decode the return data .UNINDENT .INDENT 7.0 diff --git a/doc/man/spm.1 b/doc/man/spm.1 index d4ac9df26189..d0f9e78929bf 100644 --- a/doc/man/spm.1 +++ b/doc/man/spm.1 @@ -1,6 +1,6 @@ .\" Man page generated from reStructuredText. . -.TH "SPM" "1" "Sep 27, 2021" "3004" "Salt" +.TH "SPM" "1" "Feb 16, 2022" "3004.1" "Salt" .SH NAME spm \- Salt Package Manager Command . diff --git a/doc/topics/releases/3004.1.rst b/doc/topics/releases/3004.1.rst new file mode 100644 index 000000000000..3f6df5e76f00 --- /dev/null +++ b/doc/topics/releases/3004.1.rst @@ -0,0 +1,16 @@ +.. _release-3004-1: + +========================= +Salt 3004.1 Release Notes +========================= + +Version 3004.1 is a CVE security fix release for :ref:`3004 `. + +Security +-------- + +- Sign authentication replies to prevent MiTM (cve-2022-22935) +- Prevent job and fileserver replays (cve-2022-22936) +- Fix denial of service in junos ifconfig output parsing. (cve-2022-22937) +- Sign pillar data to prevent MiTM attacks. (cve-2202-22934) +- Fixed targeting bug, especially visible when using syndic and user auth. (CVE-2022-22941) (#60413) From 935a5807caa17342da804f8c82b1d2f902cc4915 Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Wed, 16 Feb 2022 17:36:05 -0700 Subject: [PATCH 62/66] Fix warts in new minion auth --- salt/crypt.py | 34 +++-- tests/pytests/unit/test_crypt.py | 106 +++++++++++++ tests/pytests/unit/transport/test_zeromq.py | 157 ++++++++++++++++++++ 3 files changed, 281 insertions(+), 16 deletions(-) diff --git a/salt/crypt.py b/salt/crypt.py index b8a96ff29011..76870216fd67 100644 --- a/salt/crypt.py +++ b/salt/crypt.py @@ -266,9 +266,8 @@ def verify_signature(pubkey_path, message, signature): try: return pubkey.verify(digest, signature) except RSA.RSAError as exc: - if exc.args[0] == "bad signature": - return False - raise + log.debug("Signature verification failed: %s", exc.args[0]) + return False else: verifier = PKCS1_v1_5.new(pubkey) return verifier.verify( @@ -793,20 +792,23 @@ def handle_signin_response(self, sign_in_payload, payload): clear_signature = payload["sig"] payload = salt.payload.loads(clear_signed_data) - auth["aes"] = self.verify_master(payload, master_pub="token" in sign_in_payload) - if not auth["aes"]: - log.critical( - "The Salt Master server's public key did not authenticate!\n" - "The master may need to be updated if it is a version of Salt " - "lower than %s, or\n" - "If you are confident that you are connecting to a valid Salt " - "Master, then remove the master public key and restart the " - "Salt Minion.\nThe master public key can be found " - "at:\n%s", - salt.version.__version__, - m_pub_fn, + if "pub_key" in payload: + auth["aes"] = self.verify_master( + payload, master_pub="token" in sign_in_payload ) - raise SaltClientError("Invalid master key") + if not auth["aes"]: + log.critical( + "The Salt Master server's public key did not authenticate!\n" + "The master may need to be updated if it is a version of Salt " + "lower than %s, or\n" + "If you are confident that you are connecting to a valid Salt " + "Master, then remove the master public key and restart the " + "Salt Minion.\nThe master public key can be found " + "at:\n%s", + salt.version.__version__, + m_pub_fn, + ) + raise SaltClientError("Invalid master key") master_pubkey_path = os.path.join(self.opts["pki_dir"], self.mpub) if os.path.exists(master_pubkey_path) and not verify_signature( diff --git a/tests/pytests/unit/test_crypt.py b/tests/pytests/unit/test_crypt.py index 3939aac06414..a40c34b9d52f 100644 --- a/tests/pytests/unit/test_crypt.py +++ b/tests/pytests/unit/test_crypt.py @@ -12,6 +12,92 @@ import salt.master import salt.utils.files +PRIV_KEY = """ +-----BEGIN RSA PRIVATE KEY----- +MIIEogIBAAKCAQEAoAsMPt+4kuIG6vKyw9r3+OuZrVBee/2vDdVetW+Js5dTlgrJ +aghWWn3doGmKlEjqh7E4UTa+t2Jd6w8RSLnyHNJ/HpVhMG0M07MF6FMfILtDrrt8 +ZX7eDVt8sx5gCEpYI+XG8Y07Ga9i3Hiczt+fu6HYwu96HggmG2pqkOrn3iGfqBvV +YVFJzSZYe7e4c1PeEs0xYcrA4k+apyGsMtpef8vRUrNicRLc7dAcvfhtgt2DXEZ2 +d72t/CR4ygtUvPXzisaTPW0G7OWAheCloqvTIIPQIjR8htFxGTz02STVXfnhnJ0Z +k8KhqKF2v1SQvIYxsZU7jaDgl5i3zpeh58cYOwIDAQABAoIBABZUJEO7Y91+UnfC +H6XKrZEZkcnH7j6/UIaOD9YhdyVKxhsnax1zh1S9vceNIgv5NltzIsfV6vrb6v2K +Dx/F7Z0O0zR5o+MlO8ZncjoNKskex10gBEWG00Uqz/WPlddiQ/TSMJTv3uCBAzp+ +S2Zjdb4wYPUlgzSgb2ygxrhsRahMcSMG9PoX6klxMXFKMD1JxiY8QfAHahPzQXy9 +F7COZ0fCVo6BE+MqNuQ8tZeIxu8mOULQCCkLFwXmkz1FpfK/kNRmhIyhxwvCS+z4 +JuErW3uXfE64RLERiLp1bSxlDdpvRO2R41HAoNELTsKXJOEt4JANRHm/CeyA5wsh +NpscufUCgYEAxhgPfcMDy2v3nL6KtkgYjdcOyRvsAF50QRbEa8ldO+87IoMDD/Oe +osFERJ5hhyyEO78QnaLVegnykiw5DWEF02RKMhD/4XU+1UYVhY0wJjKQIBadsufB +2dnaKjvwzUhPh5BrBqNHl/FXwNCRDiYqXa79eWCPC9OFbZcUWWq70s8CgYEAztOI +61zRfmXJ7f70GgYbHg+GA7IrsAcsGRITsFR82Ho0lqdFFCxz7oK8QfL6bwMCGKyk +nzk+twh6hhj5UNp18KN8wktlo02zTgzgemHwaLa2cd6xKgmAyuPiTgcgnzt5LVNG +FOjIWkLwSlpkDTl7ZzY2QSy7t+mq5d750fpIrtUCgYBWXZUbcpPL88WgDB7z/Bjg +dlvW6JqLSqMK4b8/cyp4AARbNp12LfQC55o5BIhm48y/M70tzRmfvIiKnEc/gwaE +NJx4mZrGFFURrR2i/Xx5mt/lbZbRsmN89JM+iKWjCpzJ8PgIi9Wh9DIbOZOUhKVB +9RJEAgo70LvCnPTdS0CaVwKBgDJW3BllAvw/rBFIH4OB/vGnF5gosmdqp3oGo1Ik +jipmPAx6895AH4tquIVYrUl9svHsezjhxvjnkGK5C115foEuWXw0u60uiTiy+6Pt +2IS0C93VNMulenpnUrppE7CN2iWFAiaura0CY9fE/lsVpYpucHAWgi32Kok+ZxGL +WEttAoGAN9Ehsz4LeQxEj3x8wVeEMHF6OsznpwYsI2oVh6VxpS4AjgKYqeLVcnNi +TlZFsuQcqgod8OgzA91tdB+Rp86NygmWD5WzeKXpCOg9uA+y/YL+0sgZZHsuvbK6 +PllUgXdYxqClk/hdBFB7v9AQoaj7K9Ga22v32msftYDQRJ94xOI= +-----END RSA PRIVATE KEY----- +""" + + +PUB_KEY = """ +-----BEGIN PUBLIC KEY----- +MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAoAsMPt+4kuIG6vKyw9r3 ++OuZrVBee/2vDdVetW+Js5dTlgrJaghWWn3doGmKlEjqh7E4UTa+t2Jd6w8RSLny +HNJ/HpVhMG0M07MF6FMfILtDrrt8ZX7eDVt8sx5gCEpYI+XG8Y07Ga9i3Hiczt+f +u6HYwu96HggmG2pqkOrn3iGfqBvVYVFJzSZYe7e4c1PeEs0xYcrA4k+apyGsMtpe +f8vRUrNicRLc7dAcvfhtgt2DXEZ2d72t/CR4ygtUvPXzisaTPW0G7OWAheCloqvT +IIPQIjR8htFxGTz02STVXfnhnJ0Zk8KhqKF2v1SQvIYxsZU7jaDgl5i3zpeh58cY +OwIDAQAB +-----END PUBLIC KEY----- +""" + +PRIV_KEY2 = """ +-----BEGIN RSA PRIVATE KEY----- +MIIEogIBAAKCAQEAp+8cTxguO6Vg+YO92VfHgNld3Zy8aM3JbZvpJcjTnis+YFJ7 +Zlkcc647yPRRwY9nYBNywahnt5kIeuT1rTvTsMBZWvmUoEVUj1Xg8XXQkBvb9Ozy +Gqy/G/p8KDDpzMP/U+XCnUeHiXTZrgnqgBIc2cKeCVvWFqDi0GRFGzyaXLaX3PPm +M7DJ0MIPL1qgmcDq6+7Ze0gJ9SrDYFAeLmbuT1OqDfufXWQl/82JXeiwU2cOpqWq +7n5fvPOWim7l1tzQ+dSiMRRm0xa6uNexCJww3oJSwvMbAmgzvOhqqhlqv+K7u0u7 +FrFFojESsL36Gq4GBrISnvu2tk7u4GGNTYYQbQIDAQABAoIBAADrqWDQnd5DVZEA +lR+WINiWuHJAy/KaIC7K4kAMBgbxrz2ZbiY9Ok/zBk5fcnxIZDVtXd1sZicmPlro +GuWodIxdPZAnWpZ3UtOXUayZK/vCP1YsH1agmEqXuKsCu6Fc+K8VzReOHxLUkmXn +FYM+tixGahXcjEOi/aNNTWitEB6OemRM1UeLJFzRcfyXiqzHpHCIZwBpTUAsmzcG +QiVDkMTKubwo/m+PVXburX2CGibUydctgbrYIc7EJvyx/cpRiPZXo1PhHQWdu4Y1 +SOaC66WLsP/wqvtHo58JQ6EN/gjSsbAgGGVkZ1xMo66nR+pLpR27coS7o03xCks6 +DY/0mukCgYEAuLIGgBnqoh7YsOBLd/Bc1UTfDMxJhNseo+hZemtkSXz2Jn51322F +Zw/FVN4ArXgluH+XsOhvG/MFFpojwZSrb0Qq5b1MRdo9qycq8lGqNtlN1WHqosDQ +zW29kpL0tlRrSDpww3wRESsN9rH5XIrJ1b3ZXuO7asR+KBVQMy/+NcUCgYEA6MSC +c+fywltKPgmPl5j0DPoDe5SXE/6JQy7w/vVGrGfWGf/zEJmhzS2R+CcfTTEqaT0T +Yw8+XbFgKAqsxwtE9MUXLTVLI3sSUyE4g7blCYscOqhZ8ItCUKDXWkSpt++rG0Um +1+cEJP/0oCazG6MWqvBC4NpQ1nzh46QpjWqMwokCgYAKDLXJ1p8rvx3vUeUJW6zR +dfPlEGCXuAyMwqHLxXgpf4EtSwhC5gSyPOtx2LqUtcrnpRmt6JfTH4ARYMW9TMef +QEhNQ+WYj213mKP/l235mg1gJPnNbUxvQR9lkFV8bk+AGJ32JRQQqRUTbU+yN2MQ +HEptnVqfTp3GtJIultfwOQKBgG+RyYmu8wBP650izg33BXu21raEeYne5oIqXN+I +R5DZ0JjzwtkBGroTDrVoYyuH1nFNEh7YLqeQHqvyufBKKYo9cid8NQDTu+vWr5UK +tGvHnwdKrJmM1oN5JOAiq0r7+QMAOWchVy449VNSWWV03aeftB685iR5BXkstbIQ +EVopAoGAfcGBTAhmceK/4Q83H/FXBWy0PAa1kZGg/q8+Z0KY76AqyxOVl0/CU/rB +3tO3sKhaMTHPME/MiQjQQGoaK1JgPY6JHYvly2KomrJ8QTugqNGyMzdVJkXAK2AM +GAwC8ivAkHf8CHrHa1W7l8t2IqBjW1aRt7mOW92nfG88Hck0Mbo= +-----END RSA PRIVATE KEY----- +""" + + +PUB_KEY2 = """ +-----BEGIN PUBLIC KEY----- +MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAp+8cTxguO6Vg+YO92VfH +gNld3Zy8aM3JbZvpJcjTnis+YFJ7Zlkcc647yPRRwY9nYBNywahnt5kIeuT1rTvT +sMBZWvmUoEVUj1Xg8XXQkBvb9OzyGqy/G/p8KDDpzMP/U+XCnUeHiXTZrgnqgBIc +2cKeCVvWFqDi0GRFGzyaXLaX3PPmM7DJ0MIPL1qgmcDq6+7Ze0gJ9SrDYFAeLmbu +T1OqDfufXWQl/82JXeiwU2cOpqWq7n5fvPOWim7l1tzQ+dSiMRRm0xa6uNexCJww +3oJSwvMbAmgzvOhqqhlqv+K7u0u7FrFFojESsL36Gq4GBrISnvu2tk7u4GGNTYYQ +bQIDAQAB +-----END PUBLIC KEY----- +""" + def test_get_rsa_pub_key_bad_key(tmp_path): """ @@ -63,3 +149,23 @@ def test_cryptical_dumps_invalid_nonce(): assert isinstance(ret, bytes) with pytest.raises(salt.crypt.SaltClientError, match="Nonce verification error"): assert master_crypt.loads(ret, nonce="abcde") + + +def test_verify_signature(tmpdir): + tmpdir.join("foo.pem").write(PRIV_KEY.strip()) + tmpdir.join("foo.pub").write(PUB_KEY.strip()) + tmpdir.join("bar.pem").write(PRIV_KEY2.strip()) + tmpdir.join("bar.pub").write(PUB_KEY2.strip()) + msg = b"foo bar" + sig = salt.crypt.sign_message(str(tmpdir.join("foo.pem")), msg) + assert salt.crypt.verify_signature(str(tmpdir.join("foo.pub")), msg, sig) + + +def test_verify_signature_bad_sig(tmpdir): + tmpdir.join("foo.pem").write(PRIV_KEY.strip()) + tmpdir.join("foo.pub").write(PUB_KEY.strip()) + tmpdir.join("bar.pem").write(PRIV_KEY2.strip()) + tmpdir.join("bar.pub").write(PUB_KEY2.strip()) + msg = b"foo bar" + sig = salt.crypt.sign_message(str(tmpdir.join("foo.pem")), msg) + assert not salt.crypt.verify_signature(str(tmpdir.join("bar.pub")), msg, sig) diff --git a/tests/pytests/unit/transport/test_zeromq.py b/tests/pytests/unit/transport/test_zeromq.py index fccf0073ee8d..1f0515c91a9a 100644 --- a/tests/pytests/unit/transport/test_zeromq.py +++ b/tests/pytests/unit/transport/test_zeromq.py @@ -1116,3 +1116,160 @@ async def test_req_chan_auth_v2_with_master_signing(pki_dir, io_loop): pki_dir.join("minion", "minion_master.pub").read() == pki_dir.join("master", "master.pub").read() ) + + +async def test_req_chan_auth_v2_new_minion_with_master_pub(pki_dir, io_loop): + + pki_dir.join("master", "minions", "minion").remove() + mockloop = MagicMock() + opts = { + "master_uri": "tcp://127.0.0.1:4506", + "interface": "127.0.0.1", + "ret_port": 4506, + "ipv6": False, + "sock_dir": ".", + "pki_dir": str(pki_dir.join("minion")), + "id": "minion", + "__role": "minion", + "keysize": 4096, + "max_minions": 0, + "auto_accept": False, + "open_mode": False, + "key_pass": None, + "publish_port": 4505, + "auth_mode": 1, + "acceptance_wait_time": 3, + } + SMaster.secrets["aes"] = { + "secret": multiprocessing.Array( + ctypes.c_char, + salt.utils.stringutils.to_bytes(salt.crypt.Crypticle.generate_key_string()), + ), + "reload": salt.crypt.Crypticle.generate_key_string, + } + master_opts = dict(opts, pki_dir=str(pki_dir.join("master"))) + master_opts["master_sign_pubkey"] = False + server = salt.transport.zeromq.ZeroMQReqServerChannel(master_opts) + server.auto_key = salt.daemons.masterapi.AutoKey(server.opts) + server.cache_cli = False + server.master_key = salt.crypt.MasterKeys(server.opts) + opts["verify_master_pubkey_sign"] = False + opts["always_verify_signature"] = False + client = salt.transport.zeromq.AsyncZeroMQReqChannel(opts, io_loop=io_loop) + signin_payload = client.auth.minion_sign_in_payload() + pload = client._package_load(signin_payload) + assert "version" in pload + assert pload["version"] == 2 + + ret = server._auth(pload["load"], sign_messages=True) + assert "sig" in ret + ret = client.auth.handle_signin_response(signin_payload, ret) + assert ret == "retry" + + +async def test_req_chan_auth_v2_new_minion_with_master_pub_bad_sig(pki_dir, io_loop): + + pki_dir.join("master", "minions", "minion").remove() + + # Give the master a different key than the minion has. + mapriv = pki_dir.join("master", "master.pem") + mapriv.remove() + mapriv.write(MASTER2_PRIV_KEY.strip()) + mapub = pki_dir.join("master", "master.pub") + mapub.remove() + mapub.write(MASTER2_PUB_KEY.strip()) + + mockloop = MagicMock() + opts = { + "master_uri": "tcp://127.0.0.1:4506", + "interface": "127.0.0.1", + "ret_port": 4506, + "ipv6": False, + "sock_dir": ".", + "pki_dir": str(pki_dir.join("minion")), + "id": "minion", + "__role": "minion", + "keysize": 4096, + "max_minions": 0, + "auto_accept": False, + "open_mode": False, + "key_pass": None, + "publish_port": 4505, + "auth_mode": 1, + "acceptance_wait_time": 3, + } + SMaster.secrets["aes"] = { + "secret": multiprocessing.Array( + ctypes.c_char, + salt.utils.stringutils.to_bytes(salt.crypt.Crypticle.generate_key_string()), + ), + "reload": salt.crypt.Crypticle.generate_key_string, + } + master_opts = dict(opts, pki_dir=str(pki_dir.join("master"))) + master_opts["master_sign_pubkey"] = False + server = salt.transport.zeromq.ZeroMQReqServerChannel(master_opts) + server.auto_key = salt.daemons.masterapi.AutoKey(server.opts) + server.cache_cli = False + server.master_key = salt.crypt.MasterKeys(server.opts) + opts["verify_master_pubkey_sign"] = False + opts["always_verify_signature"] = False + client = salt.transport.zeromq.AsyncZeroMQReqChannel(opts, io_loop=io_loop) + signin_payload = client.auth.minion_sign_in_payload() + pload = client._package_load(signin_payload) + assert "version" in pload + assert pload["version"] == 2 + + ret = server._auth(pload["load"], sign_messages=True) + assert "sig" in ret + with pytest.raises(salt.crypt.SaltClientError, match="Invalid signature"): + ret = client.auth.handle_signin_response(signin_payload, ret) + + +async def test_req_chan_auth_v2_new_minion_without_master_pub(pki_dir, io_loop): + + pki_dir.join("master", "minions", "minion").remove() + pki_dir.join("minion", "minion_master.pub").remove() + mockloop = MagicMock() + opts = { + "master_uri": "tcp://127.0.0.1:4506", + "interface": "127.0.0.1", + "ret_port": 4506, + "ipv6": False, + "sock_dir": ".", + "pki_dir": str(pki_dir.join("minion")), + "id": "minion", + "__role": "minion", + "keysize": 4096, + "max_minions": 0, + "auto_accept": False, + "open_mode": False, + "key_pass": None, + "publish_port": 4505, + "auth_mode": 1, + "acceptance_wait_time": 3, + } + SMaster.secrets["aes"] = { + "secret": multiprocessing.Array( + ctypes.c_char, + salt.utils.stringutils.to_bytes(salt.crypt.Crypticle.generate_key_string()), + ), + "reload": salt.crypt.Crypticle.generate_key_string, + } + master_opts = dict(opts, pki_dir=str(pki_dir.join("master"))) + master_opts["master_sign_pubkey"] = False + server = salt.transport.zeromq.ZeroMQReqServerChannel(master_opts) + server.auto_key = salt.daemons.masterapi.AutoKey(server.opts) + server.cache_cli = False + server.master_key = salt.crypt.MasterKeys(server.opts) + opts["verify_master_pubkey_sign"] = False + opts["always_verify_signature"] = False + client = salt.transport.zeromq.AsyncZeroMQReqChannel(opts, io_loop=io_loop) + signin_payload = client.auth.minion_sign_in_payload() + pload = client._package_load(signin_payload) + assert "version" in pload + assert pload["version"] == 2 + + ret = server._auth(pload["load"], sign_messages=True) + assert "sig" in ret + ret = client.auth.handle_signin_response(signin_payload, ret) + assert ret == "retry" From 8890a6c76904897aecf9fabfb21d22ccbb2edf4f Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Thu, 24 Feb 2022 21:56:03 -0700 Subject: [PATCH 63/66] Test fix --- salt/transport/tcp.py | 48 +++++++++++-------- salt/transport/zeromq.py | 27 +++++++---- .../zeromq/test_pub_server_channel.py | 37 ++++++++------ tests/pytests/unit/transport/test_tcp.py | 20 ++++---- tests/unit/transport/test_ipc.py | 2 + 5 files changed, 80 insertions(+), 54 deletions(-) diff --git a/salt/transport/tcp.py b/salt/transport/tcp.py index 9665b98f4f81..f00b3c40eb6a 100644 --- a/salt/transport/tcp.py +++ b/salt/transport/tcp.py @@ -1422,7 +1422,7 @@ class PubServer(salt.ext.tornado.tcpserver.TCPServer): TCP publisher """ - def __init__(self, opts, io_loop=None): + def __init__(self, opts, io_loop=None, pack_publish=lambda _: _): super().__init__(ssl_options=opts.get("ssl")) self.io_loop = io_loop self.opts = opts @@ -1449,6 +1449,10 @@ def __init__(self, opts, io_loop=None): ) else: self.event = None + self._pack_publish = pack_publish + + def pack_publish(self, load): + return self._pack_publish(load) def close(self): if self._closing: @@ -1557,6 +1561,7 @@ def handle_stream(self, stream, address): @salt.ext.tornado.gen.coroutine def publish_payload(self, package, _): log.debug("TCP PubServer sending payload: %s", package) + payload = self.pack_publish(package) payload = salt.transport.frame.frame_msg(package["payload"]) to_remove = [] @@ -1632,7 +1637,9 @@ def _publish_daemon(self, **kwargs): self.io_loop = salt.ext.tornado.ioloop.IOLoop.current() # Spin up the publisher - pub_server = PubServer(self.opts, io_loop=self.io_loop) + pub_server = PubServer( + self.opts, io_loop=self.io_loop, pack_publish=self.pack_publish + ) sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) sock.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1) _set_tcp_keepalive(sock, self.opts) @@ -1675,10 +1682,7 @@ def pre_fork(self, process_manager, kwargs=None): """ process_manager.add_process(self._publish_daemon, kwargs=kwargs) - def publish(self, load): - """ - Publish "load" to minions - """ + def pack_publish(self, load): payload = {"enc": "aes"} load["serial"] = salt.master.SMaster.get_serial() crypticle = salt.crypt.Crypticle( @@ -1689,20 +1693,6 @@ def publish(self, load): master_pem_path = os.path.join(self.opts["pki_dir"], "master.pem") log.debug("Signing data packet") payload["sig"] = salt.crypt.sign_message(master_pem_path, payload["load"]) - # Use the Salt IPC server - if self.opts.get("ipc_mode", "") == "tcp": - pull_uri = int(self.opts.get("tcp_master_publish_pull", 4514)) - else: - pull_uri = os.path.join(self.opts["sock_dir"], "publish_pull.ipc") - # TODO: switch to the actual asynchronous interface - # pub_sock = salt.transport.ipc.IPCMessageClient(self.opts, io_loop=self.io_loop) - pub_sock = salt.utils.asynchronous.SyncWrapper( - salt.transport.ipc.IPCMessageClient, - (pull_uri,), - loop_kwarg="io_loop", - ) - pub_sock.connect() - int_payload = {"payload": salt.payload.dumps(payload)} # add some targeting stuff for lists only (for now) @@ -1719,5 +1709,21 @@ def publish(self, load): int_payload["topic_lst"] = match_ids else: int_payload["topic_lst"] = load["tgt"] + return int_payload + + def publish(self, load): + """ + Publish "load" to minions + """ # Send it over IPC! - pub_sock.send(int_payload) + if self.opts.get("ipc_mode", "") == "tcp": + pull_uri = int(self.opts.get("tcp_master_publish_pull", 4514)) + else: + pull_uri = os.path.join(self.opts["sock_dir"], "publish_pull.ipc") + pub_sock = salt.utils.asynchronous.SyncWrapper( + salt.transport.ipc.IPCMessageClient, + (pull_uri,), + loop_kwarg="io_loop", + ) + pub_sock.connect() + pub_sock.send(load) diff --git a/salt/transport/zeromq.py b/salt/transport/zeromq.py index 6e65a44ea2fc..9e61b2325545 100644 --- a/salt/transport/zeromq.py +++ b/salt/transport/zeromq.py @@ -939,6 +939,8 @@ def _publish_daemon(self, log_queue=None): try: log.debug("Publish daemon getting data from puller %s", pull_uri) package = pull_sock.recv() + package = salt.payload.loads(package) + package = self.pack_publish(package) log.debug("Publish daemon received payload. size=%d", len(package)) unpacked_package = salt.payload.unpackage(package) @@ -1031,8 +1033,8 @@ def pub_connect(self): """ if self.pub_sock: self.pub_close() - ctx = zmq.Context.instance() - self._sock_data.sock = ctx.socket(zmq.PUSH) + self._sock_data._ctx = zmq.Context() + self._sock_data.sock = self._sock_data._ctx.socket(zmq.PUSH) self.pub_sock.setsockopt(zmq.LINGER, -1) if self.opts.get("ipc_mode", "") == "tcp": pull_uri = "tcp://127.0.0.1:{}".format( @@ -1054,14 +1056,10 @@ def pub_close(self): if hasattr(self._sock_data, "sock"): self._sock_data.sock.close() delattr(self._sock_data, "sock") + if hasattr(self._sock_data, "_ctx"): + self._sock_data._ctx.destroy() - def publish(self, load): - """ - Publish "load" to minions. This send the load to the publisher daemon - process with does the actual sending to minions. - - :param dict load: A load to be sent across the wire to minions - """ + def pack_publish(self, load): payload = {"enc": "aes"} load["serial"] = salt.master.SMaster.get_serial() crypticle = salt.crypt.Crypticle( @@ -1094,9 +1092,18 @@ def publish(self, load): load.get("jid", None), len(payload), ) + return payload + + def publish(self, load): + """ + Publish "load" to minions. This send the load to the publisher daemon + process with does the actual sending to minions. + + :param dict load: A load to be sent across the wire to minions + """ if not self.pub_sock: self.pub_connect() - self.pub_sock.send(payload) + self.pub_sock.send(salt.payload.dumps(load)) log.debug("Sent payload to publish daemon.") diff --git a/tests/pytests/functional/transport/zeromq/test_pub_server_channel.py b/tests/pytests/functional/transport/zeromq/test_pub_server_channel.py index 5633bceafec3..e7033f810a20 100644 --- a/tests/pytests/functional/transport/zeromq/test_pub_server_channel.py +++ b/tests/pytests/functional/transport/zeromq/test_pub_server_channel.py @@ -127,6 +127,15 @@ def __init__(self, master_config, minion_config, **collector_kwargs): self.minion_config = minion_config self.collector_kwargs = collector_kwargs self.aes_key = salt.crypt.Crypticle.generate_key_string() + salt.master.SMaster.secrets["aes"] = { + "secret": multiprocessing.Array( + ctypes.c_char, + salt.utils.stringutils.to_bytes(self.aes_key), + ), + "serial": multiprocessing.Value( + ctypes.c_longlong, lock=False # We'll use the lock from 'secret' + ), + } self.process_manager = salt.utils.process.ProcessManager( name="ZMQ-PubServer-ProcessManager" ) @@ -145,15 +154,6 @@ def __init__(self, master_config, minion_config, **collector_kwargs): ) def run(self): - salt.master.SMaster.secrets["aes"] = { - "secret": multiprocessing.Array( - ctypes.c_char, - salt.utils.stringutils.to_bytes(self.aes_key), - ), - "serial": multiprocessing.Value( - ctypes.c_longlong, lock=False # We'll use the lock from 'secret' - ), - } try: while True: payload = self.queue.get() @@ -247,12 +247,16 @@ def test_issue_36469_tcp(salt_master, salt_minion): https://github.com/saltstack/salt/issues/36469 """ - def _send_small(server_channel, sid, num=10): + def _send_small(opts, sid, num=10): + server_channel = salt.transport.zeromq.ZeroMQPubServerChannel(opts) for idx in range(num): load = {"tgt_type": "glob", "tgt": "*", "jid": "{}-s{}".format(sid, idx)} server_channel.publish(load) + time.sleep(0.3) + server_channel.close_pub() - def _send_large(server_channel, sid, num=10, size=250000 * 3): + def _send_large(opts, sid, num=10, size=250000 * 3): + server_channel = salt.transport.zeromq.ZeroMQPubServerChannel(opts) for idx in range(num): load = { "tgt_type": "glob", @@ -261,16 +265,19 @@ def _send_large(server_channel, sid, num=10, size=250000 * 3): "xdata": "0" * size, } server_channel.publish(load) + time.sleep(0.3) + server_channel.close_pub() opts = dict(salt_master.config.copy(), ipc_mode="tcp", pub_hwm=0) send_num = 10 * 4 expect = [] with PubServerChannelProcess(opts, salt_minion.config.copy()) as server_channel: + assert "aes" in salt.master.SMaster.secrets with ThreadPoolExecutor(max_workers=4) as executor: - executor.submit(_send_small, server_channel, 1) - executor.submit(_send_large, server_channel, 2) - executor.submit(_send_small, server_channel, 3) - executor.submit(_send_large, server_channel, 4) + executor.submit(_send_small, opts, 1) + executor.submit(_send_large, opts, 2) + executor.submit(_send_small, opts, 3) + executor.submit(_send_large, opts, 4) expect.extend(["{}-s{}".format(a, b) for a in range(10) for b in (1, 3)]) expect.extend(["{}-l{}".format(a, b) for a in range(10) for b in (2, 4)]) results = server_channel.collector.results diff --git a/tests/pytests/unit/transport/test_tcp.py b/tests/pytests/unit/transport/test_tcp.py index d003797d29c9..3b6e17547256 100644 --- a/tests/pytests/unit/transport/test_tcp.py +++ b/tests/pytests/unit/transport/test_tcp.py @@ -210,15 +210,17 @@ def test_tcp_pub_server_channel_publish_filtering(temp_salt_master): SyncWrapper.return_value = wrap # try simple publish with glob tgt_type - channel.publish({"test": "value", "tgt_type": "glob", "tgt": "*"}) - payload = wrap.send.call_args[0][0] + payload = channel.pack_publish( + {"test": "value", "tgt_type": "glob", "tgt": "*"} + ) # verify we send it without any specific topic assert "topic_lst" not in payload # try simple publish with list tgt_type - channel.publish({"test": "value", "tgt_type": "list", "tgt": ["minion01"]}) - payload = wrap.send.call_args[0][0] + payload = channel.pack_publish( + {"test": "value", "tgt_type": "list", "tgt": ["minion01"]} + ) # verify we send it with correct topic assert "topic_lst" in payload @@ -226,8 +228,9 @@ def test_tcp_pub_server_channel_publish_filtering(temp_salt_master): # try with syndic settings opts["order_masters"] = True - channel.publish({"test": "value", "tgt_type": "list", "tgt": ["minion01"]}) - payload = wrap.send.call_args[0][0] + payload = channel.pack_publish( + {"test": "value", "tgt_type": "list", "tgt": ["minion01"]} + ) # verify we send it without topic for syndics assert "topic_lst" not in payload @@ -257,8 +260,9 @@ def test_tcp_pub_server_channel_publish_filtering_str_list(temp_salt_master): check_minions.return_value = {"minions": ["minion02"]} # try simple publish with list tgt_type - channel.publish({"test": "value", "tgt_type": "list", "tgt": "minion02"}) - payload = wrap.send.call_args[0][0] + payload = channel.pack_publish( + {"test": "value", "tgt_type": "list", "tgt": "minion02"} + ) # verify we send it with correct topic assert "topic_lst" in payload diff --git a/tests/unit/transport/test_ipc.py b/tests/unit/transport/test_ipc.py index f663d941b592..79b49f940602 100644 --- a/tests/unit/transport/test_ipc.py +++ b/tests/unit/transport/test_ipc.py @@ -40,6 +40,8 @@ class IPCMessagePubSubCase(salt.ext.tornado.testing.AsyncTestCase): def setUp(self): super().setUp() self.opts = {"ipc_write_buffer": 0} + if not os.path.exists(RUNTIME_VARS.TMP): + os.mkdir(RUNTIME_VARS.TMP) self.socket_path = os.path.join(RUNTIME_VARS.TMP, "ipc_test.ipc") self.pub_channel = self._get_pub_channel() self.sub_channel = self._get_sub_channel() From 4a1175b5ff4ea19cd86f3448f4176a452c88ddf4 Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Thu, 24 Feb 2022 22:08:05 -0700 Subject: [PATCH 64/66] Update release notes --- doc/topics/releases/3004.1.rst | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/doc/topics/releases/3004.1.rst b/doc/topics/releases/3004.1.rst index 3f6df5e76f00..5c1b07a48dfb 100644 --- a/doc/topics/releases/3004.1.rst +++ b/doc/topics/releases/3004.1.rst @@ -6,6 +6,23 @@ Salt 3004.1 Release Notes Version 3004.1 is a CVE security fix release for :ref:`3004 `. +Important notice about upgrading +-------------------------------- + +Version 3004.1 is a security release. 3004.1 minions are not able to +communicate with masters older than 3004.1. You must upgrade your masters +before upgrading minions. + +Minion authentication security +------------------------------ + +Authentication between masters and minions rely on public/private key +encryption and message signing. To secure minion authentication before you must +pre-seed the master's public key on minions. To pre-seed the minions' master +key, place a copy of the master's public key in the minion's pki directory as +``minion_master.pub``. + + Security -------- From 466bc9ab0cdd7909fc4d852256935cedbcdb700f Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Mon, 28 Feb 2022 13:47:49 -0700 Subject: [PATCH 65/66] Remove cve from non cve worty issue --- CHANGELOG.md | 2 +- doc/topics/releases/3004.1.rst | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c241974ccf68..b92e95997fb8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,9 +15,9 @@ Security - Sign authentication replies to prevent MiTM (cve-2022-22935) - Prevent job and fileserver replays (cve-2022-22936) -- Fix denial of service in junos ifconfig output parsing. (cve-2022-22937) - Sign pillar data to prevent MiTM attacks. (cve-2202-22934) - Fixed targeting bug, especially visible when using syndic and user auth. (CVE-2022-22941) (#60413) +- Fix denial of service in junos ifconfig output parsing. Salt 3004 (2021-10-11) diff --git a/doc/topics/releases/3004.1.rst b/doc/topics/releases/3004.1.rst index 5c1b07a48dfb..442265da6e76 100644 --- a/doc/topics/releases/3004.1.rst +++ b/doc/topics/releases/3004.1.rst @@ -28,6 +28,6 @@ Security - Sign authentication replies to prevent MiTM (cve-2022-22935) - Prevent job and fileserver replays (cve-2022-22936) -- Fix denial of service in junos ifconfig output parsing. (cve-2022-22937) - Sign pillar data to prevent MiTM attacks. (cve-2202-22934) - Fixed targeting bug, especially visible when using syndic and user auth. (CVE-2022-22941) (#60413) +- Fix denial of service in junos ifconfig output parsing. From a172b0afb343e6f543724308b972f3687d5c00fb Mon Sep 17 00:00:00 2001 From: Rick van de Loo Date: Wed, 6 Apr 2022 14:57:14 +0200 Subject: [PATCH 66/66] hotfix Caught exception in wait_for_fun utils hotfix like https://github.com/saltstack/salt/pull/61918 but based on the https://github.com/saltstack/salt/releases/tag/v3004.1 tag --- salt/cloud/clouds/openstack.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/salt/cloud/clouds/openstack.py b/salt/cloud/clouds/openstack.py index 6a98081c46d0..ef65805a42c3 100644 --- a/salt/cloud/clouds/openstack.py +++ b/salt/cloud/clouds/openstack.py @@ -444,11 +444,9 @@ def _get_ips(node, addr_type="public"): "OS-EXT-IPS:type" ): ret.append(addr["addr"]) - elif addr_type == "public" and __utils__["cloud.is_public_ip"]( - addr["addr"] - ): + elif addr_type == "public" and salt.utils.cloud.is_public_ip(addr["addr"]): ret.append(addr["addr"]) - elif addr_type == "private" and not __utils__["cloud.is_public_ip"]( + elif addr_type == "private" and not salt.utils.cloud.is_public_ip( addr["addr"] ): ret.append(addr["addr"])