Review findings from the 7a6d744 internal sync: Alloy credential path, env-file quoting, teardown/path guards

[thiras]

Review findings surfaced while syncing decdn/internal-devops from this template at 7a6d744
(decdn/internal-devops#15). All line references are against 7a6d744. Two were fixed downstream
in inventory/docs; the rest live in template-owned files and must be fixed here, since anything
patched in the internal instance is clobbered by the next sync.

Ordered by severity.

1. grafana_alloy_secret_file defaults into a directory the node user owns

ansible/roles/grafana_alloy/defaults/main.yml:37 defaults the Grafana Cloud credential file to
/etc/decdn/grafana-alloy.env, but ansible/roles/decdn_node/tasks/main.yml:1109 creates
/etc/decdn as decdn:decdn 0750. The file itself is root:root 0600, yet write permission on
the parent directory is enough to unlink and replace it — so a compromised (or merely buggy)
decdn process can substitute the EnvironmentFile that systemd expands as root into the
Alloy unit (templates/alloy.service.j2, EnvironmentFile=). That is a privilege boundary the
role explicitly claims to hold: the unit comment says "the agent never needs (and must not get)
direct read access", and preflight enforces root:root 0600 on the file.

Suggested fix: default to a path outside any service-owned directory, e.g.
/etc/grafana-alloy.env (it already satisfies the validate-paths.yml /etc/... regex and sits
outside grafana_alloy_config_dir, which the disable path removes wholesale). Optionally have
preflight also assert the parent directory is root-owned and not group/other writable.

Downstream workaround applied: overridden in inventory.

2. Inventory-rendered DECDN_RPC_URL is not safe under the role's own bash sourcing

ansible/roles/decdn_node/tasks/main.yml:1299 renders the value with to_json, i.e. double
quotes. ansible/roles/decdn_node/tasks/main.yml:1444-1456 then sources that file with
/bin/bash (set -a; . /etc/decdn/decdn.env) before decdn config validate, and the task's own
comment notes bash "expands $VAR and executes $(...) and backticks, which systemd would pass
through literally".

So the role's documented contract and its rendering disagree: roles/decdn_node/README.md:305-309
tells operators to single-quote any value containing $ or a backtick, and states that the
escaping applied to decdn_extra_env is not applied to decdn_rpc_url, "which is rendered
raw". An RPC URL containing a literal $ (legal in a query string / API key) is silently mangled
at validate time; a $(...) substring is executed as the decdn user.

Suggested fix: render DECDN_RPC_URL single-quoted with ' -> '\'' escaping (as for
decdn_extra_env), or source the env file in a way that does not perform expansion. Either way,
reconcile README lines 305-309 with the actual behaviour.

3. Teardown marker is matched anywhere in the unit file, not as the ownership stamp

ansible/roles/grafana_alloy/tasks/main.yml:51 gates removal on
_ga_managed_marker in (… | b64decode). vars/main.yml:4 documents the marker as "the ownership
stamp emitted as the first line" of the template. A hand-written unit that merely mentions
MANAGED BY the grafana_alloy role in a comment (e.g. an operator noting the role used to own the
service) is then treated as role-managed and deleted on the disable path.

Suggested fix: compare against the first line, e.g.
… | b64decode).split('\n')[0] is search(...) / startswith(_ga_managed_marker), matching what
the template actually emits and what vars/main.yml claims.

4. validate-paths.yml rejects .. segments but not .

ansible/roles/grafana_alloy/tasks/validate-paths.yml:16-18 refuses .. and empty segments, but a
bare . passes both the regexes (. is in the character class) and the segment checks. So
grafana_alloy_config_dir: /etc/alloy/. normalises to /etc/alloy — harmless — while something
like /var/lib/alloy/. on the destructive disable path (state: absent) resolves to a different
directory than the literal string suggests, which is exactly the class of surprise this guard
exists to prevent.

Suggested fix: add - "'.' not in item.value.split('/')" alongside the existing .. check.

5. No preflight rejects grafana_alloy_user: root

templates/alloy.service.j2 renders User={{ grafana_alloy_user }} and the surrounding comments
build the whole credential model on Alloy running unprivileged ("read by systemd as ROOT before
privileges drop, so the agent never needs … direct read access"). Nothing stops an override from
setting grafana_alloy_user: root, which silently dissolves that boundary.

Suggested fix: assert grafana_alloy_user not in ['root', '0'] (same for the group) in
tasks/preflight.yml.

6. (nit) Uncovered node.toml.j2 OTLP branch

templates/node.toml.j2:356-364 has three outcomes. Molecule covers the explicit endpoint
(molecule/default/converge.yml:52) and the Grafana-enabled fallback
(molecule/grafana-cloud/verify.yml:150-155), but nothing asserts the third: Grafana disabled
with an empty decdn_otlp_endpoint must omit observability.otlp_endpoint entirely. A one-line
assertion in an existing verify would close it.

---

Happy to send PRs for any subset — say which you'd like and I'll open them against main.

[thiras]

7. --check is broken for decdn_node_install_method: manual in dir-mode

Found while dry-running the internal fleet against 7a6d744.

ansible/roles/decdn_node/tasks/main.yml:159-174 ("Identify the on-disk format of both
resolved binaries") is an ansible.builtin.command with no check_mode: false, so under
--check it is skipped and registers stdout: "". The very next task
(main.yml:176, "Require both binaries to be ELF …") loops over
_decdn_bin_file.results and asserts 'ELF' in item.stdout, which is now vacuously false:

failed: [bcl] (item=…/decdn/target/release/decdn-node) =>
  "…/decdn-node is \"\" — not an ELF x86-64 binary for x86_64-unknown-linux-gnu. …"

So make check can never succeed for a dir-mode manual install, even when the binaries on
the control machine are perfectly good ELF x86-64 (they are — the same tree deploys fine).
The whole point of that gate is to catch a Mach-O build before a deploy, but it instead
blocks the non-mutating rehearsal.

Suggested fix: add check_mode: false to the file -b probe (it is a read-only
command on localhost with changed_when: false, so it is safe to run in check mode).
Alternatively gate the assert on not ansible_check_mode, though that loses the check-mode
signal.

Workaround: pass both explicit paths (-e decdn_node_manual_bin_src=… -e decdn_cli_manual_bin_src=…), which takes the trusted-verbatim branch and skips the probe.

[altanoruc]

Opened #60 (d64071b) for these findings.