diff --git a/DEVELOPMENT.md b/DEVELOPMENT.md index b5ba7cb..df88b8f 100644 --- a/DEVELOPMENT.md +++ b/DEVELOPMENT.md @@ -376,23 +376,14 @@ Puppet and Salt also run `cm.resolve_catalog_conflicts()` after renderer role co capture_file(abs_path, role_name, reason, policy, path_filter, ...) -> skip if already seen globally or in this role -> skip if --exclude-path matches - -> ask IgnorePolicy.inspect_file(abs_path) - -> open the source through fsutil.open_no_follow_path() - -> reject symlinks in any path component, not only the leaf - -> fstat() the opened descriptor - -> reject non-regular or over-size files - -> read the exact bytes from that descriptor - -> scan those bytes for binary/secret content unless dangerous - -> use the inspected fstat() for owner/group/mode metadata - -> write the inspected bytes to artifacts// - with no-follow destination creation + -> ask IgnorePolicy.deny_reason(abs_path) + -> stat owner/group/mode with fsutil.stat_triplet() + -> copy to artifacts// -> append ManagedFile -> mark seen in role/global ``` -This ordering is intentional. Enroll should not scan one file and later copy a different file after a race. When `IgnorePolicy.inspect_file()` succeeds, `capture_file()` writes the exact bytes that were inspected and uses the same descriptor's stat metadata. - -`fsutil.stat_triplet()` and `stat_triplet_from_stat()` return owner, group, and a zero-padded octal mode string. They fall back to numeric uid/gid strings if user/group names cannot be resolved. +`fsutil.stat_triplet()` returns owner, group, and a zero-padded octal mode string. It falls back to numeric uid/gid strings if user/group names cannot be resolved. ### 7.2 `capture_link()` @@ -459,20 +450,6 @@ regex:^/path/...$ regex `expand_includes()` is conservative: it ignores symlinks, respects excludes, caps file counts, and returns notes for unmatched patterns or caps. -### 7.6 Output, artifact, and cache safety helpers - -Several safety helpers protect privileged runs from following attacker-controlled paths: - -- `fsutil.open_no_follow_path()` opens source and artifact paths component-by-component, rejecting symlinked parent directories as well as symlinked leaf files. -- `harvest_safety.prepare_new_private_dir()` is used for user-facing plaintext output directories such as `harvest --out` and default manifest output; it refuses existing final paths and creates `0700` directories. -- `harvest_safety.ensure_safe_output_parent()` is used when writing output files such as reports or encrypted SOPS bundles. It validates parents before staging a temporary file and atomically replacing the final path. -- `harvest_safety.ensure_private_dir()` is used for persistent internal directories such as Enroll's cache root. Existing directories are allowed, but symlink components and unsafe root-run parents are refused. -- `cache.new_harvest_cache_dir()` creates unpredictable per-harvest cache directories beneath the hardened cache root with `mkdtemp()` and private permissions. -- `manifest_safety.safe_artifact_file()` validates referenced harvested artifacts before renderers copy them. It rejects absolute or `..` paths, symlinks, non-regular files, hardlinks, and paths that resolve outside the artifact root. -- `manifest_safety.prepare_manifest_output_dir()` refuses unsafe manifest output paths. In `--fqdn` site mode, where an existing tree is intentionally reused, it walks the existing output tree and refuses symlinks before merging generated files. - -When adding a new code path that writes plaintext host state, prefer these helpers over raw `mkdir(parents=True)`, `open()`, `shutil.copy*()`, or `tar.extract*()`. - --- ## 8. Platform and package backends @@ -811,7 +788,8 @@ SOPS mode: The renderers do not know about SOPS. -Before dispatching to a renderer, `manifest.manifest()` calls `validate.validate_harvest()` with normal schema validation enabled. That means generated configuration-management code is only rendered after Enroll has checked the bundle schema, referenced artifact existence, and artifact safety. If validation fails, manifest generation stops rather than trying to produce best-effort output from a malformed or tampered bundle. +Note: Manifest deliberately hooks into validate() to make sure the harvest meets the schema and +doesn't contain dangerous tamperings before turning it into config management code. --- @@ -1402,24 +1380,17 @@ This is intended to answer “what did Enroll collect and why?” 1. `state.json` exists, 2. it parses as JSON, 3. it validates against the vendored schema unless `--no-schema` is set, -4. every `managed_file.src_rel` is relative and points to a safe artifact file, -5. firewall runtime generated artifacts exist and are safe, -6. the top-level `artifacts/` path is a real directory rather than a symlink or file, -7. the whole artifact tree contains no symlink directories, symlink files, hardlinks, special files, or paths that escape the artifact root, -8. unreferenced artifact files are reported as warnings. - -`validate_harvest()` is used in three important contexts: - -- `enroll validate` exposes the checks directly to users. -- `manifest.manifest()` validates before rendering Ansible/Puppet/Salt output. -- `diff.compare_harvests()` validates both input bundles before comparing them, using `no_schema=True` so older harvests can still be inspected while artifact safety checks remain active. - -`diff --enforce` renders the old harvest through `manifest.manifest()`, so enforcement also passes through manifest-time validation before a local apply tool is invoked. +4. every `managed_file.src_rel` points to an artifact file, +5. firewall runtime generated artifacts exist, +6. there are no unreferenced artifact files, reported as warnings. +7. there are no malicious or unsafe bits such as symlinks/hardlinks etc traversing out of the artifact tree It returns a `ValidationResult` with `errors`, `warnings`, `ok()`, `to_dict()`, and `to_text()`. The CLI supports local schema override with `--schema`, warning failure with `--fail-on-warnings`, JSON/text output, and `--out`. +Note that manifest() hooks into validate() to make sure the harvest is safe before rendering it into config management code. + --- ## 19. Remote harvesting @@ -1486,7 +1457,7 @@ Unknown host keys are rejected by default through Paramiko's reject policy. User - device nodes, - anything resolving outside the destination. -This helper is reused by remote harvest, manifest SOPS extraction, validate/diff bundle resolution, and any code path that needs to unpack a harvest tarball. Do not use raw `tar.extractall()` for user- or remote-provided bundles. +This helper is reused by remote harvest, manifest SOPS extraction, and diff bundle resolution. --- diff --git a/enroll/cache.py b/enroll/cache.py index 3a53b87..1dc656b 100644 --- a/enroll/cache.py +++ b/enroll/cache.py @@ -8,8 +8,6 @@ from datetime import datetime from pathlib import Path from typing import Optional -from .harvest_safety import OutputSafetyError, ensure_private_dir - def _safe_component(s: str) -> str: s = s.strip() @@ -46,17 +44,16 @@ class HarvestCache: def _ensure_dir_secure(path: Path) -> None: - """Create a private cache directory with output-path safety checks. - - Cache roots are persistent, so existing directories are allowed, but they - still need the same symlink-component and root-parent trust checks as - plaintext harvest/manifest output paths. - """ - + """Create a directory with restrictive permissions; refuse symlinks.""" + # Refuse a symlink at the leaf. + if path.exists() and path.is_symlink(): + raise RuntimeError(f"Refusing to use symlink path: {path}") + path.mkdir(parents=True, exist_ok=True, mode=0o700) try: - ensure_private_dir(path, label="cache directory") - except OutputSafetyError as e: - raise RuntimeError(str(e)) from e + os.chmod(path, 0o700) + except OSError: + # Best-effort; on some FS types chmod may fail. + pass def new_harvest_cache_dir(*, hint: Optional[str] = None) -> HarvestCache: diff --git a/enroll/harvest_safety.py b/enroll/harvest_safety.py index d6d738c..b6e4bc4 100644 --- a/enroll/harvest_safety.py +++ b/enroll/harvest_safety.py @@ -69,8 +69,6 @@ def _assert_existing_output_dir_component(path: Path, *, label: str) -> None: raise OutputSafetyError( f"{label} parent path contains a symlink; refusing: {path}" ) - if not stat.S_ISDIR(st.st_mode): - raise OutputSafetyError(f"{label} parent is not a directory: {path}") _assert_trusted_root_parent(path, st, label=label) @@ -216,24 +214,6 @@ def write_text_output_file( return out -def ensure_private_dir(path: str | Path, *, label: str = "output") -> Path: - """Create or validate a private directory without requiring it to be empty. - - This is for persistent internal directories such as Enroll's cache root, - where existing contents are expected across runs. It uses the same - component-by-component symlink and root-parent trust checks as user-facing - plaintext output directories, but permits an existing final directory. - """ - - out = Path(path).expanduser() - sentinel = out / ".enroll-private-dir-check" - _assert_no_existing_symlink_components(sentinel, label=label) - out = _mkdir_private_dir_tree(out, label=label, final_must_be_new=False) - _assert_no_existing_symlink_components(sentinel, label=label) - _chmod_private(out) - return out - - def prepare_new_private_dir(path: str | Path, *, label: str = "output") -> Path: """Create a brand-new private output directory. diff --git a/enroll/validate.py b/enroll/validate.py index e8c65ea..60d7f7c 100644 --- a/enroll/validate.py +++ b/enroll/validate.py @@ -232,79 +232,65 @@ def validate_harvest( # Validate the whole artifact tree too, so unreferenced symlinks, # hardlinks, special files, and path-shaping tricks do not survive # validation simply because no managed_file currently references them. - if artifacts_dir.exists(): - try: - artifacts_st = artifacts_dir.lstat() - except OSError as e: - errors.append(f"unable to inspect artifacts directory: {e}") - else: - if stat.S_ISLNK(artifacts_st.st_mode): - errors.append(f"artifacts directory is a symlink: {artifacts_dir}") - elif not stat.S_ISDIR(artifacts_st.st_mode): - errors.append(f"artifacts path is not a directory: {artifacts_dir}") - else: - for root, dirs, files in os.walk(artifacts_dir, followlinks=False): - root_p = Path(root) - for name in list(dirs): - fp = root_p / name - try: - st = fp.lstat() - except FileNotFoundError: - continue - if stat.S_ISLNK(st.st_mode): - errors.append(f"artifact directory is a symlink: {fp}") - elif not stat.S_ISDIR(st.st_mode): - errors.append( - f"artifact directory is not a directory: {fp}" - ) + if artifacts_dir.exists() and artifacts_dir.is_dir(): + for root, dirs, files in os.walk(artifacts_dir, followlinks=False): + root_p = Path(root) + for name in list(dirs): + fp = root_p / name + try: + st = fp.lstat() + except FileNotFoundError: + continue + if stat.S_ISLNK(st.st_mode): + errors.append(f"artifact directory is a symlink: {fp}") + elif not stat.S_ISDIR(st.st_mode): + errors.append(f"artifact directory is not a directory: {fp}") - for name in files: - fp = root_p / name - try: - st = fp.lstat() - except FileNotFoundError: - continue - try: - rel = fp.relative_to(artifacts_dir) - except ValueError: - errors.append(f"artifact escapes artifact root: {fp}") - continue - parts = rel.parts - if len(parts) < 2: - errors.append( - f"artifact is not under a role directory: {fp}" - ) - continue - role_name = parts[0] - src_rel = "/".join(parts[1:]) + for name in files: + fp = root_p / name + try: + st = fp.lstat() + except FileNotFoundError: + continue + try: + rel = fp.relative_to(artifacts_dir) + except ValueError: + errors.append(f"artifact escapes artifact root: {fp}") + continue + parts = rel.parts + if len(parts) < 2: + errors.append(f"artifact is not under a role directory: {fp}") + continue + role_name = parts[0] + src_rel = "/".join(parts[1:]) - if stat.S_ISLNK(st.st_mode): - errors.append( - f"artifact is a symlink: artifacts/{role_name}/{src_rel}" - ) - continue - if not stat.S_ISREG(st.st_mode): - errors.append( - f"artifact is not a regular file: artifacts/{role_name}/{src_rel}" - ) - continue - if st.st_nlink > 1: - errors.append( - f"artifact is hardlinked: artifacts/{role_name}/{src_rel}" - ) - continue - try: - safe_artifact_file(bundle.dir, role_name, src_rel) - except (FileNotFoundError, ArtifactSafetyError) as e: - errors.append( - f"unsafe artifact: artifacts/{role_name}/{src_rel}: {e}" - ) - continue + if stat.S_ISLNK(st.st_mode): + errors.append( + f"artifact is a symlink: artifacts/{role_name}/{src_rel}" + ) + continue + if not stat.S_ISREG(st.st_mode): + errors.append( + f"artifact is not a regular file: artifacts/{role_name}/{src_rel}" + ) + continue + if st.st_nlink > 1: + errors.append( + f"artifact is hardlinked: artifacts/{role_name}/{src_rel}" + ) + continue + try: + safe_artifact_file(bundle.dir, role_name, src_rel) + except (FileNotFoundError, ArtifactSafetyError) as e: + errors.append( + f"unsafe artifact: artifacts/{role_name}/{src_rel}: {e}" + ) + continue - if (role_name, src_rel) not in referenced: - warnings.append( - f"unreferenced artifact present: artifacts/{role_name}/{src_rel}" - ) + if (role_name, src_rel) not in referenced: + warnings.append( + f"unreferenced artifact present: artifacts/{role_name}/{src_rel}" + ) return ValidationResult(errors=errors, warnings=warnings) finally: diff --git a/tests/test_cache_security.py b/tests/test_cache_security.py index a1b7622..4fda1e1 100644 --- a/tests/test_cache_security.py +++ b/tests/test_cache_security.py @@ -95,44 +95,3 @@ def test_enroll_cache_dir_uses_default_when_xdg_not_set(monkeypatch): monkeypatch.delenv("XDG_CACHE_HOME", raising=False) result = enroll_cache_dir() assert str(result).endswith("/.local/cache/enroll") - - -def test_ensure_dir_secure_refuses_symlink_parent(tmp_path: Path): - from enroll.cache import _ensure_dir_secure - - target = tmp_path / "target" - target.mkdir() - link = tmp_path / "link" - link.symlink_to(target, target_is_directory=True) - - with pytest.raises(RuntimeError, match="symlink"): - _ensure_dir_secure(link / "enroll" / "harvest") - - assert not (target / "enroll" / "harvest").exists() - - -def test_ensure_dir_secure_rejects_unsafe_root_parent(tmp_path: Path, monkeypatch): - from enroll.cache import _ensure_dir_secure - import enroll.harvest_safety as hs - - untrusted = tmp_path / "untrusted" - untrusted.mkdir() - untrusted.chmod(0o777) - - monkeypatch.setattr(hs, "_effective_uid", lambda: 0) - with pytest.raises(RuntimeError, match="not owned by root|writable by group/other"): - _ensure_dir_secure(untrusted / "cache") - - -def test_ensure_dir_secure_rejects_existing_file_when_not_root( - tmp_path: Path, monkeypatch -): - from enroll.cache import _ensure_dir_secure - import enroll.harvest_safety as hs - - path = tmp_path / "cache" - path.write_text("not a dir", encoding="utf-8") - - monkeypatch.setattr(hs, "_effective_uid", lambda: 1000) - with pytest.raises(RuntimeError, match="not a directory"): - _ensure_dir_secure(path) diff --git a/tests/test_validate.py b/tests/test_validate.py index 002d2cf..5ac33c9 100644 --- a/tests/test_validate.py +++ b/tests/test_validate.py @@ -453,35 +453,3 @@ def test_validate_harvest_rejects_unreferenced_artifact_symlink(tmp_path: Path): assert result.ok is False assert any("symlink" in e for e in result.errors) - - -def test_validate_harvest_rejects_top_level_artifacts_symlink(tmp_path: Path): - bundle_dir = tmp_path / "bundle" - bundle_dir.mkdir() - target = tmp_path / "artifact-target" - target.mkdir() - (bundle_dir / "artifacts").symlink_to(target, target_is_directory=True) - (bundle_dir / "state.json").write_text( - json.dumps({"roles": {"users": {"managed_files": []}}}), - encoding="utf-8", - ) - - result = validate_harvest(str(bundle_dir), no_schema=True) - - assert result.ok is False - assert any("artifacts directory is a symlink" in e for e in result.errors) - - -def test_validate_harvest_rejects_top_level_artifacts_file(tmp_path: Path): - bundle_dir = tmp_path / "bundle" - bundle_dir.mkdir() - (bundle_dir / "artifacts").write_text("not a directory", encoding="utf-8") - (bundle_dir / "state.json").write_text( - json.dumps({"roles": {"users": {"managed_files": []}}}), - encoding="utf-8", - ) - - result = validate_harvest(str(bundle_dir), no_schema=True) - - assert result.ok is False - assert any("artifacts path is not a directory" in e for e in result.errors)