diff --git a/README.md b/README.md index 7f1a6ad..5fdb55a 100644 --- a/README.md +++ b/README.md @@ -285,8 +285,6 @@ 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 a9df1e3..97fc7f3 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -49,7 +49,6 @@ 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. diff --git a/enroll/puppet.py b/enroll/puppet.py index e034cea..1b78789 100644 --- a/enroll/puppet.py +++ b/enroll/puppet.py @@ -291,54 +291,8 @@ 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 c54f6c7..e4c4792 100644 --- a/tests/test_manifest_puppet.py +++ b/tests/test_manifest_puppet.py @@ -1326,85 +1326,3 @@ 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