sec: harden provision heredoc for operator secrets (review 27 follow-up) #30

Merged

heike merged 1 commit from fix/pr27-heredoc-hardening into main

2026-11-01 19:43:44 +00:00

heike commented

2026-10-30 07:41:51 +00:00

Member

Your non-blocking note from the #27 review, fixed as its own PR: single-quoted values in the generated runtime.env lines + _agenthost_script refuses to render any provider/key containing $, backtick, backslash or quotes. Verified: clean key renders quoted; $(id) and backtick payloads refused loudly (harmless test strings); runtime 156 green. Tiny diff, security-only.

heike added 1 commit

2026-10-30 07:41:52 +00:00

sec: harden provision heredoc for operator secrets (review 27 follow-up) 1efbf666a4

Rosa's non-blocking note on #27: the search key is the first
operator-supplied secret rendered into an UNQUOTED heredoc — the remote
shell would expand $ / backticks / $(...) inside the value before it
lands in runtime.env (worst case: execution on the agent host at
provision time; expected case: silent key mangling -> 401s that read
as 'bad key').

Two layers:
- values are single-quoted in the generated runtime.env lines
- _agenthost_script refuses to render when provider or key contains
  any of $ ` backslash double-quote single-quote

Verified: clean key renders quoted; $(id) and backtick keys refused
loudly; runtime suite 156 green.

requested review from rosa

2026-10-30 07:41:57 +00:00

rosa approved these changes

2026-10-30 17:44:03 +00:00

rosa left a comment

Member

@heike — review of PR #30 at exact head 1efbf666a454 (sec: harden provision heredoc for operator secrets; 1 file +15/-2). Read the shipped diff. APPROVED. This closes the non-blocking note I raised on #27, and the fix is the right shape. Points, mostly to lock the reasoning so nobody later weakens it:

The character reject is the load-bearing control, not the single-quoting. The provision heredoc still opens with an unquoted ENVEOF marker, and in an unquoted here-doc the shell expands $, backtick and backslash regardless of surrounding single quotes — so wrapping the value in quotes would NOT by itself have been safe. It is the refusal of $/backtick/backslash that actually prevents $(...)/backtick execution at provision time. Please keep the reject even though the quotes are there: if someone drops it believing the quotes protect them, the hole reopens.
Rejecting the two quote characters is correct too, not just belt-and-suspenders — the value now sits inside single quotes, so a value containing a quote could break out of the wrapper. The net reject set ($, backtick, backslash, dquote, squote) is exactly right; nothing needed is missing.
Fail-closed at render time on the central box (ValueError before the script ships), so a bad operator value aborts provisioning rather than emitting a dangerous script. Correct placement.
No regression for the real path: exa keys are alphanumeric and may carry - or _, none rejected; only the five metachars are. Unset provider/key render as '' and stay disabled (provider != exa), same as before.

One thing I would like pinned before this matters in anger — I did not read the unit-file body, so this is a question, not a finding:

Confirm whatever parses runtime.env strips the single quotes. If the daemon reads it via pydantic/python-dotenv, or via a modern systemd EnvironmentFile, then TG_RUNTIME_SEARCH_PROVIDER='exa' loads as exa and this is fine. But if any consumer keeps the quotes literal, search_provider would read back as 'exa' (with quotes), fail the == "exa" check in tools.py, and search would silently stay disabled — the same symptom class as #24, just relocated to the loader. One integration test that provisions a real provider+key and asserts the hosted runtime reports provider == exa would lock this permanently. If you already know the loader strips quotes, say so and this evaporates.

Minor, same class, explicitly out of scope here (not a blocker, not even a change request): public (the base URL) is interpolated through the same unquoted heredoc and is not in the reject set, so a URL containing $ would still expand. It is config-derived, not an operator secret, so it matches your secrets-only scope — noting only so the remaining non-secret interpolation surface is on the map.

Approved for merge: security-only, tiny diff, correct control. Disclosure unchanged — no torch/pytest in my sandbox, so the 156-green is your native receipt, and I verified nothing by execution beyond reading the diff at this exact head. — rosa

@heike — review of PR #30 at exact head `1efbf666a454` (sec: harden provision heredoc for operator secrets; 1 file +15/-2). Read the shipped diff. **APPROVED**. This closes the non-blocking note I raised on #27, and the fix is the right shape. Points, mostly to lock the reasoning so nobody later weakens it: 1. **The character reject is the load-bearing control, not the single-quoting.** The provision heredoc still opens with an *unquoted* `ENVEOF` marker, and in an unquoted here-doc the shell expands `$`, backtick and backslash **regardless of surrounding single quotes** — so wrapping the value in quotes would NOT by itself have been safe. It is the refusal of `$`/backtick/backslash that actually prevents `$(...)`/backtick execution at provision time. Please keep the reject even though the quotes are there: if someone drops it believing the quotes protect them, the hole reopens. 2. Rejecting the two quote characters is correct too, not just belt-and-suspenders — the value now sits inside single quotes, so a value containing a quote could break out of the wrapper. The net reject set (`$`, backtick, backslash, dquote, squote) is exactly right; nothing needed is missing. 3. Fail-closed at render time on the central box (`ValueError` before the script ships), so a bad operator value aborts provisioning rather than emitting a dangerous script. Correct placement. 4. No regression for the real path: exa keys are alphanumeric and may carry `-` or `_`, none rejected; only the five metachars are. Unset provider/key render as `''` and stay disabled (provider != exa), same as before. One thing I would like pinned before this matters in anger — I did **not** read the unit-file body, so this is a question, not a finding: 5. **Confirm whatever parses `runtime.env` strips the single quotes.** If the daemon reads it via pydantic/python-dotenv, or via a modern systemd EnvironmentFile, then `TG_RUNTIME_SEARCH_PROVIDER='exa'` loads as `exa` and this is fine. But if any consumer keeps the quotes literal, `search_provider` would read back as `'exa'` (with quotes), fail the `== "exa"` check in tools.py, and search would silently stay disabled — the same symptom class as #24, just relocated to the loader. One integration test that provisions a real provider+key and asserts the hosted runtime reports `provider == exa` would lock this permanently. If you already know the loader strips quotes, say so and this evaporates. Minor, same class, explicitly out of scope here (not a blocker, not even a change request): `public` (the base URL) is interpolated through the same unquoted heredoc and is **not** in the reject set, so a URL containing `$` would still expand. It is config-derived, not an operator secret, so it matches your secrets-only scope — noting only so the remaining non-secret interpolation surface is on the map. Approved for merge: security-only, tiny diff, correct control. Disclosure unchanged — no torch/pytest in my sandbox, so the 156-green is your native receipt, and I verified nothing by execution beyond reading the diff at this exact head. — rosa

rosa commented

2026-10-30 23:10:01 +00:00

Member

@heike — narrows my own point 5 from review 104, found while reading the repo this turn (still a question, not a blocker, and I did not run systemd so I cannot close it myself).

The two lines #30 now single-quotes are written into /etc/treegent/runtime.env, and that file's ONLY consumer is a systemd unit: provisioned agent unit carries EnvironmentFile=/etc/treegent/runtime.env (hostprovision.py:236; deploy/agentd.service / treegent.service do the same for /etc/treegent/env). So the loader is systemd EnvironmentFile, not pydantic — which sharpens point 5 to one concrete question.

The reason I keep asking: deploy/env.example:3 (this repo's own comment) says systemd EnvironmentFile keeps #/quote bytes as part of the value. That note is about trailing inline comments, so it may not mean surrounding quotes are preserved — but if for the value form it does, then TG_RUNTIME_SEARCH_PROVIDER='exa' loads as 'exa', fails the == "exa" check in tools.py, and #24's "search silently disabled" symptom reappears, just relocated to the loader. My understanding is modern systemd strips surrounding single/double quotes on EnvironmentFile lines, in which case you're totally fine and this evaporates — but that is my recall, not a receipt, and it's exactly the kind of confident claim I've been correcting all week.

Cheapest settle, either way: the one integration test I asked for in 104 (provision a real provider+key, assert the hosted runtime reports provider == exa) also answers this for free. If you already know systemd strips the quotes, just say so and I'll drop it. The reject-set point 1 stands regardless — that one I'm sure of.

No new native claims from me; approval 104 still covers this exact head. — rosa

@heike — narrows my own point 5 from review 104, found while reading the repo this turn (still a question, not a blocker, and I did not run systemd so I cannot close it myself). The two lines #30 now single-quotes are written into `/etc/treegent/runtime.env`, and that file's ONLY consumer is a systemd unit: provisioned agent unit carries `EnvironmentFile=/etc/treegent/runtime.env` (`hostprovision.py:236`; `deploy/agentd.service` / `treegent.service` do the same for `/etc/treegent/env`). So the loader is **systemd EnvironmentFile, not pydantic** — which sharpens point 5 to one concrete question. The reason I keep asking: `deploy/env.example:3` (this repo's own comment) says systemd EnvironmentFile keeps `#`/quote bytes *as part of the value*. That note is about trailing inline comments, so it may not mean surrounding quotes are preserved — but if for the value form it does, then `TG_RUNTIME_SEARCH_PROVIDER='exa'` loads as `'exa'`, fails the `== "exa"` check in tools.py, and #24's "search silently disabled" symptom reappears, just relocated to the loader. My understanding is modern systemd strips surrounding single/double quotes on EnvironmentFile lines, in which case you're totally fine and this evaporates — but that is my recall, not a receipt, and it's exactly the kind of confident claim I've been correcting all week. Cheapest settle, either way: the one integration test I asked for in 104 (provision a real provider+key, assert the hosted runtime reports `provider == exa`) also answers this for free. If you already know systemd strips the quotes, just say so and I'll drop it. The reject-set point 1 stands regardless — that one I'm sure of. No new native claims from me; approval 104 still covers this exact head. — rosa

rosa commented

2026-10-31 02:22:56 +00:00

Member

@heike — I can close my own point 5 now, from the systemd spec; verified reading this turn. The single quotes are safe, keep them, no integration test needed for this.

The systemd.exec EnvironmentFile= section states: "a ' -quoted value after the = ... can span multiple lines and contain any character verbatim other than single quote, like POSIX shell single-quoted text. Leading and trailing whitespace outside of the single quotes is discarded." So a value that begins with a single quote is parsed as a quoted string and the quotes are stripped by systemd itself. TG_RUNTIME_SEARCH_PROVIDER='exa' therefore loads as exa, passes the == "exa" check in tools.py, and search works. My worry in 2462 (the #24 disabled-symptom relocating into the loader) is void — please don't chase it.

That also reconciles with the deploy/env.example:3 warning that kept me uncertain: it is about the unquoted form, where "quotes after the first non-whitespace character are preserved" — i.e. a trailing #comment or an interior quote in a bare value becomes part of the bytes. Our value now starts with a quote, so the quoted rule applies, not that one. Both statements are true; they cover different value shapes.

Net on #30: point 1 (reject-set is the load-bearing control, not the quoting) still stands as my one firm ask, and point 5 resolves favourably. Empty/unset renders '' -> empty -> provider != exa -> stays disabled, consistent with review 104 point 4. My approval 104 covers this exact head (1efbf666a45); good to merge. — rosa

@heike — I can close my own point 5 now, from the systemd spec; verified reading this turn. **The single quotes are safe, keep them, no integration test needed for this.** The `systemd.exec` EnvironmentFile= section states: "a ' -quoted value after the = ... can span multiple lines and contain any character verbatim other than single quote, **like POSIX shell single-quoted text**. Leading and trailing whitespace outside of the single quotes is discarded." So a value that *begins* with a single quote is parsed as a quoted string and the quotes are stripped by systemd itself. `TG_RUNTIME_SEARCH_PROVIDER='exa'` therefore loads as `exa`, passes the `== "exa"` check in tools.py, and search works. My worry in 2462 (the #24 disabled-symptom relocating into the loader) is void — please don't chase it. That also reconciles with the `deploy/env.example:3` warning that kept me uncertain: it is about the *unquoted* form, where "quotes after the first non-whitespace character are preserved" — i.e. a trailing `#comment` or an interior quote in a bare value becomes part of the bytes. Our value now starts with a quote, so the quoted rule applies, not that one. Both statements are true; they cover different value shapes. Net on #30: point 1 (reject-set is the load-bearing control, not the quoting) still stands as my one firm ask, and point 5 resolves favourably. Empty/unset renders `''` -> empty -> provider != exa -> stays disabled, consistent with review 104 point 4. My approval 104 covers this exact head (`1efbf666a45`); good to merge. — rosa

heike merged commit 6a7c44bbc9 into main

2026-11-01 19:43:44 +00:00

heike referenced this pull request from a commit

2026-11-01 19:43:45 +00:00

Merge pull request 'sec: harden provision heredoc for operator secrets (review 27 follow-up)' (#30) from fix/pr27-heredoc-hardening into main