From ad019f6b090a508403039d5158ed405146a851e2 Mon Sep 17 00:00:00 2001 From: Miguel Jacq Date: Mon, 22 Jun 2026 14:45:12 +1000 Subject: [PATCH 1/2] normalise control characters in generated manifest scalars --- enroll/puppet.py | 46 ++++++++++++++++++++ tests/test_manifest_puppet.py | 82 +++++++++++++++++++++++++++++++++++ 2 files changed, 128 insertions(+) diff --git a/enroll/puppet.py b/enroll/puppet.py index 1b78789..e034cea 100644 --- a/enroll/puppet.py +++ b/enroll/puppet.py @@ -291,8 +291,54 @@ def _puppet_name(raw: str, *, fallback: str = "role") -> str: return s +# Control characters (C0 range plus DEL) that should never appear raw inside a +# generated Puppet manifest scalar. They cannot occur in values harvested from a +# live host (e.g. /etc/passwd GECOS is newline-delimited), so their presence +# indicates a hand-edited or tampered harvest. Emitting them verbatim is valid +# Puppet but produces multi-line / control-laden manifests; normalise them into +# explicit escapes instead. +_PP_CONTROL_CHARS = frozenset(chr(c) for c in range(0x20)) | {"\x7f"} + +# Puppet double-quoted recognised single-character escapes. +_PP_DQ_ESCAPES = { + "\n": "\\n", + "\t": "\\t", + "\r": "\\r", + "\\": "\\\\", + '"': '\\"', + "$": "\\$", +} + + +def _pp_quote_double(s: str) -> str: + """Render a Puppet double-quoted string with control characters escaped. + + Only used as a fallback when a value contains raw control characters, so the + common case stays single-quoted and byte-identical to historical output. + """ + + out = [] + for ch in s: + esc = _PP_DQ_ESCAPES.get(ch) + if esc is not None: + out.append(esc) + elif ch in _PP_CONTROL_CHARS: + # Puppet supports \uXXXX style escapes inside double-quoted strings. + out.append(f"\\u{{{ord(ch):04x}}}") + else: + out.append(ch) + return '"' + "".join(out) + '"' + + def _pp_quote(value: Any) -> str: s = str(value) + # Puppet single-quoted strings only honour \\ and \' escapes; everything + # else (including a literal newline) is taken verbatim. That is safe but lets + # a tampered harvest splatter raw control characters across the manifest. + # When any are present, fall back to a double-quoted string where they can be + # neutralised into explicit escapes. + if any(ch in _PP_CONTROL_CHARS for ch in s): + return _pp_quote_double(s) s = s.replace("\\", "\\\\").replace("'", "\\'") return f"'{s}'" diff --git a/tests/test_manifest_puppet.py b/tests/test_manifest_puppet.py index e4c4792..c54f6c7 100644 --- a/tests/test_manifest_puppet.py +++ b/tests/test_manifest_puppet.py @@ -1326,3 +1326,85 @@ def test_manifest_puppet_fqdn_writes_erb_template_values_to_node_hiera( ) assert "Any $main_key = undef," in init_pp assert "content => template($attrs['template'])" in init_pp + + +def test_pp_quote_common_case_is_single_quoted_and_stable(): + """Values without control characters keep the historical single-quoted form.""" + from enroll.puppet import _pp_quote + + assert _pp_quote("Alice Example") == "'Alice Example'" + assert _pp_quote("0644") == "'0644'" + assert _pp_quote("/etc/nginx/nginx.conf") == "'/etc/nginx/nginx.conf'" + # Single quote and backslash keep their single-quoted escaping. + assert _pp_quote("a'b") == "'a\\'b'" + assert _pp_quote("back\\slash") == "'back\\\\slash'" + + +def test_pp_quote_neutralises_raw_control_characters(): + """A tampered harvest cannot splatter raw control characters into a manifest. + + GECOS and similar scalars are newline-delimited on a live host, so control + characters only appear via a hand-edited/tampered state.json. When present, + _pp_quote switches to a double-quoted Puppet string and escapes them rather + than emitting them verbatim. + """ + from enroll.puppet import _pp_quote + + rendered = _pp_quote("a\ntouch /tmp/pwned") + assert rendered == '"a\\ntouch /tmp/pwned"' + # No raw C0/DEL byte survives into the rendered scalar. + for value in ("a\nb", "x\r\ny", "a\tb", "a\x00b", "a\x7fb"): + out = _pp_quote(value) + assert not any(ch in out for ch in [chr(c) for c in range(0x20)] + ["\x7f"]) + + +def test_pp_quote_double_fallback_cannot_introduce_interpolation(): + """The double-quoted fallback must not enable Puppet interpolation/breakout.""" + from enroll.puppet import _pp_quote + + # $ would interpolate in a double-quoted Puppet string; it must be escaped. + out = _pp_quote("a\n${::osfamily}") + assert "\\${::osfamily}" in out + assert "${::osfamily}" not in out.replace("\\${::osfamily}", "") + # A double quote cannot terminate the string early. + out2 = _pp_quote('a\n"; notify{x:} ') + assert out2.startswith('"') and out2.endswith('"') + assert '\\"' in out2 + + +def test_manifest_puppet_user_gecos_with_newline_is_single_line(tmp_path: Path): + """End-to-end: a newline in a user's gecos yields a single-line comment.""" + bundle = tmp_path / "bundle" + out = tmp_path / "puppet" + state = { + "roles": { + "users": { + "role_name": "users", + "users": [ + { + "name": "eviluser", + "uid": 1001, + "primary_group": "evil", + "supplementary_groups": [], + "home": "/home/eviluser", + "shell": "/bin/bash", + "gecos": "Real Name\ntouch /tmp/pwned", + } + ], + "managed_files": [], + "managed_dirs": [], + "excluded": [], + "notes": [], + } + } + } + _write_state(bundle, state) + manifest.manifest(str(bundle), str(out), target="puppet") + + init_pp = (out / "modules" / "users" / "manifests" / "init.pp").read_text( + encoding="utf-8" + ) + # The comment attribute must be on one line with the newline escaped. + assert 'comment => "Real Name\\ntouch /tmp/pwned"' in init_pp + # And there must be no line that is just the injected command. + assert "\ntouch /tmp/pwned\n" not in init_pp From 70525e52d8b90d21fd8ba255117dd757f8a2d810 Mon Sep 17 00:00:00 2001 From: Miguel Jacq Date: Mon, 22 Jun 2026 14:49:56 +1000 Subject: [PATCH 2/2] Doc updates --- README.md | 2 ++ SECURITY.md | 1 + 2 files changed, 3 insertions(+) diff --git a/README.md b/README.md index 5fdb55a..7f1a6ad 100644 --- a/README.md +++ b/README.md @@ -285,6 +285,8 @@ By default, `enroll` does **not** assume how you handle secrets in Ansible. It w Safe-mode content scanning is intentionally conservative. It treats common assignment-style credential keys as sensitive, including names such as `password`, `client_secret`, `secret_key`, `auth_token`, `api_key`, `aws_access_key_id`, `aws_secret_access_key`, `azure_client_secret`, `GOOGLE_APPLICATION_CREDENTIALS`, and service-account key names. +**IMPORTANT**: Enroll ignores comments in files! If you have commented out *real secrets*, there's still a risk that Enroll could capture that data even without `--dangerous`. If you are in doubt, play it safe: use `--sops` and/or encrypt the output at rest in a way that makes sense to you. + Automatic harvesting of per-user shell dotfiles is also disabled by default, even when those files differ from `/etc/skel`, because `.bashrc`, `.profile`, `.bash_aliases`, and similar files commonly contain exported tokens, credentials, or aliases/functions with embedded secrets. Use `--dangerous` for automatic shell-dotfile capture, or use targeted `--include-path` patterns for narrower safe-mode review. If you wish to opt in to collecting everything, use `--dangerous` mode, but be aware of what it means: diff --git a/SECURITY.md b/SECURITY.md index 97fc7f3..a9df1e3 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -49,6 +49,7 @@ The following are generally out of scope and should not be reported as Enroll vu * A user configuring a webhook, email target, SSH proxy command, SOPS binary, package manager, or configuration-management tool that they do not trust. * A compromised system where an attacker already controls root-owned files, root’s shell, root’s configuration, or the privileged tools Enroll invokes. * Reports that amount to “if root runs this tool with malicious options, root can make the system do dangerous things.” +* Enroll harvesting a file that has a *commented out* secret even with `--dangerous` disabled (it ignores comments so as to not be totally useless when it comes to harvesting config files). It is still the responsibility of the user to use `--sops` or appropriate at-rest encryption if in the slightest doubt about what might get harvested. Enroll is a tool for administrators, not a sandbox for hostile local users. It cannot make unsafe local trust decisions safe if the operator’s own execution environment is already attacker-controlled.