diff --git a/Makefile b/Makefile index 56635863..fc5431f4 100644 --- a/Makefile +++ b/Makefile @@ -20,8 +20,8 @@ install: check: venv format lint uv run pytest -export COVERAGE_PROCESS_START = $(PWD)/.coveragerc -export COVERAGE_FILE = $(PWD)/.coverage +coverage: export COVERAGE_PROCESS_START = $(PWD)/.coveragerc +coverage: export COVERAGE_FILE = $(PWD)/.coverage coverage: uv run coverage erase uv run coverage run --parallel-mode -m pytest diff --git a/cfbs/build.py b/cfbs/build.py index 1df1bc41..d47e333e 100644 --- a/cfbs/build.py +++ b/cfbs/build.py @@ -287,16 +287,25 @@ def _localize_file_inputs(name, input_data, destination, build_modules): If a response is already shipped by another module's own "directory" build step (e.g. the project author set one up manually), that step's destination is used instead of making a redundant copy. + + Returns the masterfiles-relative destination path of every file that was + localized, so callers can make sure those exact paths get synced by the + policy update mechanism even if their extension isn't one of the ones + normally recognized. """ + module_dir_name = name[2:] if name.startswith("./") else name module_dir_name = os.path.basename(module_dir_name.rstrip("/")) + localized_paths = [] + def _localize(rel_path): if not rel_path or not os.path.isfile(rel_path): return rel_path already_shipped = _path_if_already_shipped(rel_path, build_modules, destination) if already_shipped is not None: + localized_paths.append(strip_left(already_shipped, "$(sys.inputdir)/")) return already_shipped rel_path = os.path.normpath(rel_path) @@ -309,10 +318,66 @@ def _localize(rel_path): "modules" if in_module_dir else "", rel_path, ) + abs_destination = os.path.abspath(destination) + if ( + os.path.commonpath([os.path.abspath(dest), abs_destination]) + != abs_destination + ): + # rel_path contained a ".." segment, or was absolute (which + # discards the destination prefix in os.path.join above) - + # either way it would land outside the built masterfiles. + raise CFBSExitError( + "Input file response '%s' would be placed outside the " + "built masterfiles - refusing to copy it" % rel_path + ) cp(rel_path, dest) - return "$(sys.inputdir)/" + os.path.relpath(dest, destination) + dest_rel = os.path.relpath(dest, destination) + localized_paths.append(dest_rel) + return "$(sys.inputdir)/" + dest_rel map_file_responses(input_data, _localize) + return localized_paths + + +def _warn_if_input_paths_extra_unsupported(destination, build_modules): + """input_paths_extra (CFE-4708) is only understood by masterfiles + 3.29.0+ - on an older target it's just an unused variable in def.json, + so the file(s) it names won't actually get synced to clients. + """ + MIN_MASTERFILES_VERSION_FOR_INPUT_PATHS_EXTRA = (3, 29) + + def_json = read_json(os.path.join(destination, "def.json")) + if not def_json: + return + if not def_json.get("vars", {}).get("default:update_def.input_paths_extra"): + return + + masterfiles = next( + (m for m in build_modules if m.get("name") == "masterfiles"), None + ) + version = masterfiles.get("version") if masterfiles else None + if not version: + # Not an index-added "masterfiles" module (local copy) + # nothing to check the version of. + # Assume the user has the latest version of masterfiles + return + + parts = version.split(".") + try: + found = (int(parts[0]), int(parts[1])) + except (IndexError, ValueError): + return + if found >= MIN_MASTERFILES_VERSION_FOR_INPUT_PATHS_EXTRA: + return + + log.warning( + "'input_paths_extra' in def.json requires masterfiles %s or later, but version is %s." + " Files with extensions not listed in 'input_name_patterns' may silently not sync to clients." + % ( + ".".join(str(n) for n in MIN_MASTERFILES_VERSION_FOR_INPUT_PATHS_EXTRA), + version, + ) + ) def _perform_input_step(args, name, destination, prefix, build_modules): @@ -336,7 +401,7 @@ def _perform_input_step(args, name, destination, prefix, build_modules): ) return extras, original = read_json(src), read_json(dst) - _localize_file_inputs(name, extras, destination, build_modules) + localized_paths = _localize_file_inputs(name, extras, destination, build_modules) extras = generate_augment(name, extras) log.debug("Generated augment: %s", pretty(extras)) if not extras: @@ -344,6 +409,16 @@ def _perform_input_step(args, name, destination, prefix, build_modules): "Input data '%s' is incomplete: Skipping build step." % os.path.basename(src) ) + if localized_paths: + # Files brought in through "file" type inputs aren't necessarily + # matched by the policy update's default `input_name_patterns`. + # Rather than widening that extension-based matching for the whole + # policy set, point at exactly these files, by their literal + # relative path. + relative_paths = [path.replace(os.sep, "/") for path in localized_paths] + extras = merge_json( + extras, {"vars": {"default:update_def.input_paths_extra": relative_paths}} + ) if original: log.debug("Original def.json: %s", pretty(original)) merged = merge_json(original, extras) @@ -532,6 +607,7 @@ def perform_build(config: CFBSConfig, diffs_filename=None) -> int: raise CFBSExitError( "Error parsing JSON in 'out/masterfiles/def.json': %s" % e ) + _warn_if_input_paths_extra_unsupported("out/masterfiles", config["build"]) print("") print("Generating tarball...") sh("( cd out/ && tar -czf masterfiles.tgz masterfiles )") diff --git a/tests/shell/064_input_file_check_mpf.sh b/tests/shell/064_input_file_check_mpf.sh new file mode 100644 index 00000000..17f285df --- /dev/null +++ b/tests/shell/064_input_file_check_mpf.sh @@ -0,0 +1,23 @@ +set -e +set -x +cd tests/ +mkdir -p ./tmp/ +cd ./tmp/ +touch cfbs.json && rm cfbs.json +rm -rf .git +rm -rf copy-a-file out + +# A project pinned to a masterfiles version older than 3.29 +# must warn that files may silently not get synced to clients. +cp ../shell/064_input_file_check_mpf/example-cfbs.json cfbs.json +echo "some content" > source.txt + +mkdir -p copy-a-file +printf 'bundle agent copy_a_file {\n reports:\n "Copying $(cfbs.source)";\n}\n' > copy-a-file/copy_a_file.cf + +echo '[{"type": "file", "variable": "source", "namespace": "cfbs", "bundle": "copy_a_file", "label": "Source file", "question": "Which file should be copied?", "filetype": [".txt", ".log"], "response": "./source.txt"}]' > copy-a-file/input.json + +# Building against masterfiles 3.27.1 must warn that it predates 3.29. +cfbs build 2>&1 | grep -q "requires masterfiles 3.29 or later, but version is 3.27.1" + +rm -rf copy-a-file out source.txt diff --git a/tests/shell/064_input_file_check_mpf/example-cfbs.json b/tests/shell/064_input_file_check_mpf/example-cfbs.json new file mode 100644 index 00000000..924f7f2e --- /dev/null +++ b/tests/shell/064_input_file_check_mpf/example-cfbs.json @@ -0,0 +1,46 @@ +{ + "name": "Example project", + "description": "Example description", + "type": "policy-set", + "git": false, + "build": [ + { + "name": "masterfiles", + "description": "Official CFEngine Masterfiles Policy Framework (MPF).", + "tags": ["supported", "base"], + "repo": "https://github.com/cfengine/masterfiles", + "by": "https://github.com/cfengine", + "version": "3.27.1", + "commit": "15d8ca2ca951db3ddf197b90c9dea6ef75f5fa89", + "added_by": "cfbs add", + "steps": [ + "run EXPLICIT_VERSION=3.27.1 EXPLICIT_RELEASE=1 ./prepare.sh -y", + "copy ./ ./" + ] + }, + { + "name": "./copy-a-file/", + "description": "Copy a file into masterfiles via a file input.", + "tags": ["local"], + "added_by": "cfbs add", + "steps": [ + "directory ./ services/cfbs/copy-a-file/", + "policy_files services/cfbs/copy-a-file/", + "bundles copy_a_file", + "input ./input.json def.json" + ], + "input": [ + { + "type": "file", + "variable": "source", + "namespace": "cfbs", + "bundle": "copy_a_file", + "label": "Source file", + "question": "Which file should be copied?", + "filetype": [".txt", ".log"], + "default": "./source.txt" + } + ] + } + ] +} diff --git a/tests/shell/all.sh b/tests/shell/all.sh index 7abe3e27..293dc030 100644 --- a/tests/shell/all.sh +++ b/tests/shell/all.sh @@ -107,6 +107,7 @@ run_test tests/shell/060_input_file.sh run_test tests/shell/061_set_input_file.sh run_test tests/shell/062_input_file_in_list_with_keys.sh run_test tests/shell/063_input_string_multiline_in_list.sh +run_test tests/shell/064_input_file_check_mpf.sh # Summary _suite_end=$(date +%s) diff --git a/tests/test_build.py b/tests/test_build.py index bcefbc6a..d1be2426 100644 --- a/tests/test_build.py +++ b/tests/test_build.py @@ -1,7 +1,12 @@ import os import copy +import json +import tempfile -from cfbs.build import _localize_file_inputs +import pytest + +from cfbs.build import _localize_file_inputs, _perform_input_step +from cfbs.utils import CFBSExitError def test_localize_file_inputs_copies_single_file(tmp_path, monkeypatch): @@ -18,11 +23,14 @@ def test_localize_file_inputs_copies_single_file(tmp_path, monkeypatch): } ] - _localize_file_inputs("run-a-script", input_data, "out/masterfiles", []) + localized_paths = _localize_file_inputs( + "run-a-script", input_data, "out/masterfiles", [] + ) expected_dest = "out/masterfiles/services/cfbs/deploy.sh" assert os.path.isfile(expected_dest) assert input_data[0]["response"] == "$(sys.inputdir)/services/cfbs/deploy.sh" + assert localized_paths == ["services/cfbs/deploy.sh"] def test_localize_file_inputs_copies_list_of_files(tmp_path, monkeypatch): @@ -48,7 +56,9 @@ def test_localize_file_inputs_copies_list_of_files(tmp_path, monkeypatch): } ] - _localize_file_inputs("run-scripts-module", input_data, "out/masterfiles", []) + localized_paths = _localize_file_inputs( + "run-scripts-module", input_data, "out/masterfiles", [] + ) assert input_data[0]["response"] == [ "$(sys.inputdir)/services/cfbs/one.sh", @@ -58,6 +68,10 @@ def test_localize_file_inputs_copies_list_of_files(tmp_path, monkeypatch): assert os.path.isfile( "out/masterfiles/services/cfbs/modules/run-scripts-module/two.sh" ) + assert localized_paths == [ + "services/cfbs/one.sh", + "services/cfbs/modules/run-scripts-module/two.sh", + ] def test_localize_file_inputs_strips_local_module_prefix_with_dot_slash( @@ -119,6 +133,33 @@ def test_localize_file_inputs_ignores_missing_file(tmp_path, monkeypatch): assert input_data == before +def test_localize_file_inputs_rejects_path_traversal(tmp_path, monkeypatch): + """An absolute (or '..'-laden) response would otherwise let os.path.join + discard the destination prefix, placing the file outside the built + masterfiles entirely - refuse it instead of writing there.""" + monkeypatch.chdir(tmp_path) + os.makedirs("out/masterfiles") + + with tempfile.NamedTemporaryFile(suffix=".txt", delete=False) as f: + f.write(b"pwned\n") + outside_path = f.name + + try: + input_data = [{"type": "file", "variable": "script", "response": outside_path}] + + with pytest.raises(CFBSExitError): + _localize_file_inputs("some-module", input_data, "out/masterfiles", []) + + assert not os.path.exists( + "out/masterfiles/services/cfbs/" + os.path.basename(outside_path) + ) + finally: + try: + os.unlink(outside_path) + except OSError: + pass + + def test_localize_file_inputs_ignores_non_file_types(tmp_path, monkeypatch): monkeypatch.chdir(tmp_path) os.makedirs("out/masterfiles") @@ -197,3 +238,52 @@ def test_localize_file_inputs_ignores_non_directory_steps(tmp_path, monkeypatch) "$(sys.inputdir)/services/cfbs/modules/run-scripts/deploy.sh" ) assert os.path.isfile("out/masterfiles/services/cfbs/modules/run-scripts/deploy.sh") + + +def test_perform_input_step_adds_input_paths_extra_for_localized_files( + tmp_path, monkeypatch +): + """A "file" type input should make the build add its exact destination + path to `default:update_def.input_paths_extra`, so the policy update + mechanism syncs it even if its extension isn't in the default + `input_name_patterns` list. + """ + monkeypatch.chdir(tmp_path) + os.makedirs("out/masterfiles") + os.makedirs("run-a-script") + with open("deploy.sh", "w") as f: + f.write("echo hi\n") + with open("run-a-script/input.json", "w") as f: + json.dump([{"type": "file", "variable": "script", "response": "deploy.sh"}], f) + + _perform_input_step( + ["./input.json", "def.json"], "run-a-script", "out/masterfiles", "+", [] + ) + + with open("out/masterfiles/def.json") as f: + result = json.load(f) + + expected_path = "services/cfbs/deploy.sh" + assert result["vars"]["default:update_def.input_paths_extra"] == [expected_path] + + +def test_perform_input_step_skips_input_paths_extra_for_non_file_inputs( + tmp_path, monkeypatch +): + """A build with only "string"-type inputs has nothing to localize, so no + `input_paths_extra` augment should be generated at all. + """ + monkeypatch.chdir(tmp_path) + os.makedirs("out/masterfiles") + os.makedirs("some-module") + with open("some-module/input.json", "w") as f: + json.dump([{"type": "string", "variable": "greeting", "response": "hello"}], f) + + _perform_input_step( + ["./input.json", "def.json"], "some-module", "out/masterfiles", "+", [] + ) + + with open("out/masterfiles/def.json") as f: + result = json.load(f) + + assert "vars" not in result