From 8930a3c1066e47c743e8f8bf2e6bd1ba03487aa7 Mon Sep 17 00:00:00 2001 From: Mark Goddard Date: Fri, 8 Dec 2023 09:35:52 +0000 Subject: [PATCH] Prevent running from a different Kayobe configuration repository There are various ways in which it is possible to operate Kayobe incorrectly. One example is executing Kayobe from a different Kayobe configuration repository than the one referred to by the KAYOBE_CONFIG_PATH environment variable. While this shouldn't necessarily cause any errors, it may lead to unexpected results if the operator assumes they are operating against the configuration in the current directory. This change adds a validation step that checks for this case and fails the command early if found. Change-Id: I709884bbd7edebf1d409f39c11f293560e987506 --- kayobe/cli/commands.py | 8 ++ kayobe/tests/unit/test_utils.py | 128 +++++++++++++++++- kayobe/utils.py | 62 ++++++++- ...validate-config-path-f26550903c1eb82a.yaml | 7 + 4 files changed, 195 insertions(+), 10 deletions(-) create mode 100644 releasenotes/notes/validate-config-path-f26550903c1eb82a.yaml diff --git a/kayobe/cli/commands.py b/kayobe/cli/commands.py index 170a5698f..5231a0b04 100644 --- a/kayobe/cli/commands.py +++ b/kayobe/cli/commands.py @@ -237,7 +237,15 @@ class HookDispatcher(CommandHook): self.logger.debug("Running hooks: %s" % hooks) self.command.run_kayobe_playbooks(parsed_args, hooks) + def _preflight_checks(self, parsed_args): + # NOTE(mgoddard): Currently all commands use KayobeAnsibleMixin, so + # should have a config_path attribute, but better to be defensive. + config_path = getattr(parsed_args, "config_path", None) + if config_path: + utils.validate_config_path(config_path) + def before(self, parsed_args): + self._preflight_checks(parsed_args) self.run_hooks(parsed_args, "pre") return parsed_args diff --git a/kayobe/tests/unit/test_utils.py b/kayobe/tests/unit/test_utils.py index 88fb96da3..0d31f39dc 100644 --- a/kayobe/tests/unit/test_utils.py +++ b/kayobe/tests/unit/test_utils.py @@ -12,6 +12,7 @@ # License for the specific language governing permissions and limitations # under the License. +import logging import os import subprocess import unittest @@ -189,14 +190,12 @@ key3: mock_call.assert_called_once_with(["command", "to", "run"]) self.assertIsNone(output) - @mock.patch("kayobe.utils.open") @mock.patch.object(subprocess, "check_call") - def test_run_command_quiet(self, mock_call, mock_open): - mock_devnull = mock_open.return_value.__enter__.return_value + def test_run_command_quiet(self, mock_call): output = utils.run_command(["command", "to", "run"], quiet=True) mock_call.assert_called_once_with(["command", "to", "run"], - stdout=mock_devnull, - stderr=mock_devnull) + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL) self.assertIsNone(output) @mock.patch.object(subprocess, "check_output") @@ -332,3 +331,122 @@ key3: finder = utils.EnvironmentFinder('/etc/kayobe', None) self.assertEqual([], finder.ordered()) self.assertEqual([], finder.ordered_paths()) + + @mock.patch.object(utils, "run_command") + @mock.patch.object(utils, "is_readable_file") + def test_validate_config_path_kayobe(self, mock_readable, mock_run): + mock_run.return_value = b"/path/to/config" + utils.validate_config_path("/path/to/config/etc/kayobe") + mock_run.assert_called_once_with( + ["git", "rev-parse", "--show-toplevel"], + check_output=True, quiet=True) + self.assertFalse(mock_readable.called) + + @mock.patch.object(utils, "run_command") + @mock.patch.object(utils, "is_readable_file") + def test_validate_config_path_not_a_repo(self, mock_readable, mock_run): + mock_run.side_effect = subprocess.CalledProcessError( + "not a repo", "command") + utils.validate_config_path("/path/to/config/etc/kayobe") + mock_run.assert_called_once_with( + ["git", "rev-parse", "--show-toplevel"], + check_output=True, quiet=True) + self.assertFalse(mock_readable.called) + + @mock.patch.object(utils, "run_command") + @mock.patch.object(utils, "is_readable_file") + def test_validate_config_path_no_git(self, mock_readable, mock_run): + mock_run.side_effect = FileNotFoundError("No such file") + utils.validate_config_path("/path/to/config/etc/kayobe") + mock_run.assert_called_once_with( + ["git", "rev-parse", "--show-toplevel"], + check_output=True, quiet=True) + self.assertFalse(mock_readable.called) + + @mock.patch.object(utils, "run_command") + @mock.patch.object(utils, "is_readable_file") + @mock.patch.object(utils, "read_file") + def test_validate_config_path_gitreview(self, mock_read, mock_readable, + mock_run): + mock_run.return_value = b"/path/to/repo" + mock_readable.return_value = {"result": True} + mock_read.return_value = """ +[gerrit] +project=openstack/kayobe-config.git +""" + with self.assertLogs(level=logging.ERROR) as ctx: + self.assertRaises(SystemExit, + utils.validate_config_path, + "/path/to/config/etc/kayobe") + exp = ("Executing from within a different Kayobe configuration " + "repository is not allowed") + assert any(exp in t for t in ctx.output) + mock_run.assert_called_once_with( + ["git", "rev-parse", "--show-toplevel"], + check_output=True, quiet=True) + mock_readable.assert_called_once_with("/path/to/repo/.gitreview") + mock_read.assert_called_once_with("/path/to/repo/.gitreview") + + @mock.patch.object(utils, "run_command") + @mock.patch.object(utils, "is_readable_file") + def test_validate_config_path_no_gitreview(self, mock_readable, mock_run): + mock_run.return_value = b"/path/to/repo" + mock_readable.return_value = {"result": False} + utils.validate_config_path("/path/to/config/etc/kayobe") + mock_run.assert_called_once_with( + ["git", "rev-parse", "--show-toplevel"], + check_output=True, quiet=True) + mock_readable.assert_called_once_with("/path/to/repo/.gitreview") + + @mock.patch.object(utils, "run_command") + @mock.patch.object(utils, "is_readable_file") + @mock.patch.object(utils, "read_file") + def test_validate_config_path_gitreview_no_gerrit(self, mock_read, + mock_readable, mock_run): + mock_run.return_value = b"/path/to/repo" + mock_readable.return_value = {"result": False} + mock_read.return_value = """ +[foo] +bar=baz +""" + utils.validate_config_path("/path/to/config/etc/kayobe") + mock_run.assert_called_once_with( + ["git", "rev-parse", "--show-toplevel"], + check_output=True, quiet=True) + mock_readable.assert_called_once_with("/path/to/repo/.gitreview") + + @mock.patch.object(utils, "run_command") + @mock.patch.object(utils, "is_readable_file") + @mock.patch.object(utils, "read_file") + def test_validate_config_path_gitreview_no_project(self, mock_read, + mock_readable, + mock_run): + mock_run.return_value = b"/path/to/repo" + mock_readable.return_value = {"result": False} + mock_read.return_value = """ +[gerrit] +bar=baz +""" + utils.validate_config_path("/path/to/config/etc/kayobe") + mock_run.assert_called_once_with( + ["git", "rev-parse", "--show-toplevel"], + check_output=True, quiet=True) + mock_readable.assert_called_once_with("/path/to/repo/.gitreview") + + @mock.patch.object(utils, "run_command") + @mock.patch.object(utils, "is_readable_file") + @mock.patch.object(utils, "read_file") + def test_validate_config_path_gitreview_other_project(self, mock_read, + mock_readable, + mock_run): + mock_run.return_value = b"/path/to/repo" + mock_readable.return_value = {"result": False} + mock_read.return_value = """ +[gerrit] +project=baz +""" + utils.validate_config_path("/path/to/config/etc/kayobe") + mock_run.assert_called_once_with( + ["git", "rev-parse", "--show-toplevel"], + check_output=True, quiet=True) + mock_readable.assert_called_once_with("/path/to/repo/.gitreview") diff --git a/kayobe/utils.py b/kayobe/utils.py index 4d10e924c..f53d8e2b6 100644 --- a/kayobe/utils.py +++ b/kayobe/utils.py @@ -14,6 +14,7 @@ import base64 from collections import defaultdict +import configparser import glob import graphlib from importlib.metadata import Distribution @@ -223,11 +224,10 @@ def run_command(cmd, quiet=False, check_output=False, **kwargs): cmd_string = " ".join(cmd) LOG.debug("Running command: %s", cmd_string) if quiet: - with open("/dev/null", "w") as devnull: - kwargs["stdout"] = devnull - kwargs["stderr"] = devnull - subprocess.check_call(cmd, **kwargs) - elif check_output: + kwargs["stderr"] = subprocess.DEVNULL + if not check_output: + kwargs["stdout"] = subprocess.DEVNULL + if check_output: return subprocess.check_output(cmd, **kwargs) else: subprocess.check_call(cmd, **kwargs) @@ -409,3 +409,55 @@ class EnvironmentFinder(object): ) result.append(full_path) return result + + +def _gitreview_is_kayobe_config(gitreview_path): + """Return whether a .gitreview file is for kayobe-config.""" + config = configparser.ConfigParser() + config_string = read_file(gitreview_path) + config.read_string(config_string) + gerrit_project = config.get('gerrit', 'project') + if not gerrit_project: + return False + gerrit_project = os.path.basename(gerrit_project) + gerrit_project = os.path.splitext(gerrit_project)[0] + if gerrit_project == 'kayobe-config': + return True + + +def validate_config_path(config_path): + """Validate the Kayobe configuration path. + + Check whether we are executing from inside a Kayobe configuration + repository, and if so, assert that matches the Kayobe configuration path + defined in CLI args or environment variables. + + Exit 1 if validation fails. + + :param config_path: Kayobe configuration path or None. + """ + assert config_path + + try: + cmd = ["git", "rev-parse", "--show-toplevel"] + repo_root = run_command(cmd, quiet=True, check_output=True) + except (FileNotFoundError, subprocess.CalledProcessError): + # FileNotFoundError: git probably not installed. + # CalledProcessError: probably not in a git repository. + return + + repo_root = repo_root.decode().strip() + if config_path: + repo_config_path = os.path.join(repo_root, "etc", "kayobe") + if repo_config_path == os.path.realpath(config_path): + return + + # Paths did not match. Check that repo_root does not look like a Kayobe + # configuration repo. + gitreview_path = os.path.join(repo_root, ".gitreview") + result = is_readable_file(gitreview_path) + if result["result"]: + if _gitreview_is_kayobe_config(gitreview_path): + LOG.error("Executing from within a different Kayobe configuration " + "repository is not allowed") + sys.exit(1) diff --git a/releasenotes/notes/validate-config-path-f26550903c1eb82a.yaml b/releasenotes/notes/validate-config-path-f26550903c1eb82a.yaml new file mode 100644 index 000000000..ed99e10b5 --- /dev/null +++ b/releasenotes/notes/validate-config-path-f26550903c1eb82a.yaml @@ -0,0 +1,7 @@ +--- +features: + - | + Adds validation to protect against executing Kayobe from within a different + Kayobe configuration repository than the one referred to by environment + variables (e.g. ``KAYOBE_CONFIG_PATH``) or CLI arguments (e.g. + ``--config-path``).