fix(ansible): close grafana_alloy privilege and env-file review findings

[altanoruc]

Bug Description

Review findings from the internal sync at 7a6d744 (decdn/internal-devops#15), filed as #59 — plus the --check ELF-probe miss in comment 7.

Fixes #59

Root Cause

1. Alloy credentials defaulted into /etc/decdn, which the node user owns, so a compromised decdn process can replace the root-read EnvironmentFile.
2. decdn config validate bash-sourced a to_json (double-quoted) env file, so $VAR / $(...) in an RPC URL expanded at validate time even though systemd would pass them through literally.
3. Teardown treated any unit that merely mentioned the managed-by stamp as owned.
4. Path guards refused .. but not ..
5. Nothing stopped a root/0 Alloy identity, including named UID/GID-0 aliases.
6. Disabled Grafana + empty otlp_endpoint was untested.
7. Dir-mode file -b was skipped under --check, so the ELF assert saw empty stdout.

Fix

• Default the secret file to /etc/grafana-alloy.env; require it to be a direct child of root-controlled /etc. Existing /etc/decdn/grafana-alloy.env files must be moved — nested overrides are intentionally rejected because a writable ancestor can replace the file.
• Resolve existing Alloy account/group IDs and reject root, numeric 0, and named UID/GID-0 aliases before either startup or teardown.
• Keep systemd-safe to_json rendering; replace bash source with a non-expanding host parser restricted to one-line EnvironmentFile forms whose semantics it implements exactly.
• Match teardown ownership to the unit's exact first line; reject . and .. path segments.
• Add check_mode: false to the read-only ELF probe.
• Add regression coverage for every gate, including literal $HOME$(touch …), UID/GID-0 aliases, nested secret paths, disabled-path dot segments, and the OTLP omission branch.

How to Verify

1. make molecule (needs Docker) — default / grafana-cloud / validation / slow-readiness.
2. make check against a dir-mode manual install should no longer fail the ELF assert.
3. Confirm /etc/grafana-alloy.env is provisioned as root:root 0600 directly under /etc.

Test Plan

• Added regression coverage in molecule default / grafana-cloud / validation / slow-readiness
• make lint-ansible — production profile, 0 failures / 0 warnings
• make lint-alloy — Grafana Alloy 1.19.2 accepts all rendered variants
• Direct Ansible syntax checks for all touched Molecule playbooks
• Galaxy collection build
• Full make molecule locally — Docker daemon unavailable on this runner; CI owns the containerized run

Risk Assessment

Medium — the env-file load path for decdn config validate changed. Host-provisioned files remain supported in the documented one-line unquoted, complete single-quoted, and systemd double-quoted forms. The Alloy secret-path default is intentionally breaking for existing opt-in deployments: move /etc/decdn/grafana-alloy.env to /etc/grafana-alloy.env; overriding back to a nested path is rejected.

[Copilot]

🟡 Changes recommended

Environment parsing, root-alias identity validation, and nested secret-path handling remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Hardens Grafana Alloy credential handling and teardown, updates environment-file validation, and expands Molecule coverage.

Changes:

• Moves credentials to /etc/grafana-alloy.env with stronger path and ownership checks.
• Replaces shell sourcing and fixes --check ELF probing.
• Updates documentation, templates, changelog, and regression tests.

File summaries

File	Description
ansible/roles/grafana_alloy/vars/main.yml	Updates the ownership marker.
ansible/roles/grafana_alloy/tasks/validate-paths.yml	Rejects dot path segments.
ansible/roles/grafana_alloy/tasks/preflight.yml	Validates credential-parent security.
ansible/roles/grafana_alloy/tasks/main.yml	Adds identity validation and safer teardown.
ansible/roles/grafana_alloy/README.md	Documents the new credential path.
ansible/roles/grafana_alloy/files/grafana-alloy.env.example	Updates credential provisioning guidance.
ansible/roles/grafana_alloy/defaults/main.yml	Changes the default secret location.
ansible/roles/decdn_node/tasks/main.yml	Adds check-mode probing and Python environment parsing.
ansible/roles/decdn_node/README.md	Documents updated environment parsing.
ansible/roles/decdn_node/files/decdn.env.example	Updates environment-file guidance.
ansible/README.md	Updates credential provisioning instructions.
ansible/molecule/validation/converge.yml	Adds validation regression cases.
ansible/molecule/slow-readiness/verify.yml	Verifies disabled OTLP omission.
ansible/molecule/grafana-cloud/verify.yml	Updates path and teardown tests.
ansible/molecule/grafana-cloud/prepare.yml	Stages credentials at the new path.
ansible/molecule/default/verify.yml	Verifies literal RPC URL handling.
ansible/molecule/default/converge.yml	Adds shell-metacharacter test input.
ansible/galaxy/CHANGELOG.md	Records the fixes and behavior changes.

Review details

• Files reviewed: 18/18 changed files
• Comments generated: 5
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[altanoruc]

Addressed the valid Copilot findings: UID/GID-0 aliases are now resolved and rejected; the EnvironmentFile loader implements only an exact, tested systemd-compatible subset; nested Alloy secret paths are explicitly rejected/documented; stale source/diagnostic wording is fixed. The json.loads and “could not be sourced” comments targeted the obsolete d64071b revision.

[altanoruc]

Review (post-merge, head 227a0ab)

Verdict: sound. No correctness or security defects found. Four low-severity notes below; none warrant a revert.

Scope checked: full diff against 7a6d744, the five Copilot threads (all resolved by the final head), CI (11/11 green incl. molecule), and the parser against systemd's src/basic/env-file.c state machine.

Confirmed correct

• EnvironmentFile parser ≡ systemd on the accepted subset. Double-quote escape set "\$matchesSHELL_NEED_ESCAPEexactly; any other\xkeeps the backslash, as systemd does. Single-quoted values take no escape processing in systemd either, sovalue[1:-1]is exact. Unquoteda#banda=bare literal in both. Every divergence is a *rejection* (exit 3), never a silently different value: leading whitespace before the key,KEY =, 'a''b'concatenation, unquoted`, continuation lines, tab (systemd's env_value_is_valid permits \t; the role gates it upstream anyway). validate_value mirrors unichar_is_valid (FDD0–FDEF, xFFFE/xFFFF); BOM rejection is stricter than systemd but harmless.
• Root cause 1 is real. decdn_etc is created owner: decdn_user, mode: 0750 (decdn_node/tasks/main.yml ~L1122), so a root-owned 0600 file inside it is replaceable by the node process via rename(2). Moving the default to a direct /etc child and pinning the regex to ^/etc/[A-Za-z0-9._-]+$ closes it; the /etc stat gate (root:root, no g/o write bit, mode | length == 4) is correctly indexed ([-2] group, [-1] other).
• UID/GID-0 alias gate. getent_passwd[user][1] / getent_group[group][1] are the uid/gid fields (module stores record[1:]); fail_key: false yields None for a missing key, so the is none or != 0 disjunction is right. Placed before both paths, as the disabled path can userdel the account.
• Teardown marker. Template's first line is byte-identical to _ga_managed_marker (em-dash included); the grafana-cloud scenario exercises both the managed-removal and the foreign-survives cases, so a template edit that breaks the coupling fails CI rather than silently no-op'ing teardown.
• Regression fixture is discriminating. $HOME$(touch /tmp/decdn-rpc-expanded) would have executed under the prior set -a; . file (runs as decdn_user, /tmp writable), and the stub compares the received env literally rather than just checking the marker file.
• Exit-3 reservation is safe: decdn main.rs returns only ExitCode::SUCCESS/FAILURE, so a CLI failure cannot be misreported as an env-file parse failure.

Notes (non-blocking)

1. ; comment lines rejected. systemd's COMMENTS is "#;"; the parser only recognises #. A host-provisioned file with ; … lines fails the gate with a generic "could not be parsed". Either accept ; or name it in the exit-3 fail_msg.
2. binary-check-mode case is not self-proving. Its pass condition is "failed at Require an RPC endpoint", which is also what happens if check_mode: true on the enclosing block did not propagate into include_role. It does propagate (role tasks inherit block attributes via the parent chain), but the case cannot distinguish "probe ran under --check" from "probe ran for real". Registering _decdn_bin_file and asserting ansible_check_mode inside the rescue would make it a true guard.
3. Migration ergonomics. An upgraded opt-in host now fails at Require the Grafana Cloud secret environment file with a message that does not mention the old path. Adding one line ("if /etc/decdn/grafana-alloy.env exists, mv it") to that fail_msg would save a README round-trip. The stale file is not a leak (root:root 0600), but it is deletable by decdn and never cleaned up.
4. .molecule-checkbin is not git-ignored. The always: block removes it, but an interrupted run leaves a copy of /bin/true under ansible/. One .gitignore line.

Minor: decdn_node/README.md and decdn.env.example still describe behaviour by contrast ("rather than bash source", "does not use bash source"); the CHANGELOG is the right home for that framing.

---

Generated by Claude Code