fleet/fleetd
3
0
Fork 0
Code Issues 73 Pull Requests Actions Packages Projects Releases 1 Wiki Activity

CB-596: config-driven deny-by-default member credential policy #109

Closed
agent wants to merge 0 commits from worker/cb596-4e49ef-3 into main
pull from: worker/cb596-4e49ef-3
merge into: fleet:main
Conversation 3 Commits - Files Changed -
agent commented 2026-08-17 09:02:40 +02:00
Member
Copy Link
Copy Source

Closes gitea issue #82.

Replaces the CB-592 hardcoded single-name GITEA_ACCESS_TOKEN shadow in HerdrPeerLauncher with a config-driven deny-by-default policy: BridgedConfig.MemberCredentials (memberCredentials: policy/allow/known). Every name in known that is not also in allow gets an explicit non-secret sentinel overlay; an allowed name gets no overlay entry at all so the pane's own inherited value passes through. An unrecognized policy value refuses at config load (following the rejectUnknownAuthMode/rejectUnknownPlacement pattern). A credential-shaped host env var name that is on neither list is logged as a gap at WARN (name only, count only — never a value).

bridged.yaml is gitignored, so the 31 measured credential names and the 4-name allowlist (AI_GATEWAY_TOKEN, WORKER_GITEA_TOKEN, CONTEXT7_TOKEN, GITEA_HOST) ship as a commented memberCredentials: block in bridged.example.yaml for the operator to apply to the live config.

Tests: mvn clean install — 867 tests, 0 failures, BUILD SUCCESS.

Closes gitea issue #82. Replaces the CB-592 hardcoded single-name GITEA_ACCESS_TOKEN shadow in HerdrPeerLauncher with a config-driven deny-by-default policy: BridgedConfig.MemberCredentials (memberCredentials: policy/allow/known). Every name in known that is not also in allow gets an explicit non-secret sentinel overlay; an allowed name gets no overlay entry at all so the pane's own inherited value passes through. An unrecognized policy value refuses at config load (following the rejectUnknownAuthMode/rejectUnknownPlacement pattern). A credential-shaped host env var name that is on neither list is logged as a gap at WARN (name only, count only — never a value). bridged.yaml is gitignored, so the 31 measured credential names and the 4-name allowlist (AI_GATEWAY_TOKEN, WORKER_GITEA_TOKEN, CONTEXT7_TOKEN, GITEA_HOST) ship as a commented memberCredentials: block in bridged.example.yaml for the operator to apply to the live config. Tests: mvn clean install — 867 tests, 0 failures, BUILD SUCCESS.
agent added 1 commit 2026-08-17 09:02:40 +02:00
CB-596: member credential blocking is config-driven deny-by-default, not one hardcoded name
CI / build (pull_request) Successful in 1m1s
Details
CI / contract (pull_request) Successful in 1m26s
Details
850fb12807 
Replaces CB-592's single hardcoded GITEA_ACCESS_TOKEN shadow in HerdrPeerLauncher
with BridgedConfig.MemberCredentials (memberCredentials: policy/allow/known).
Every known name not also allowed is overlaid with a sentinel; an unrecognized
policy value refuses at load; a credential-shaped host env var on neither list
is logged as a gap (name only, never a value). bridged.yaml is gitignored, so
the 31 measured names + 4-name allowlist ship as a commented block in
bridged.example.yaml for the operator to apply live.
ltms commented 2026-08-17 09:06:08 +02:00
Owner
Copy Link
Copy Source

Reviewed. The structure is right and the config work is good — but this does not block anything on this host, and the reason is partly my brief. Not merging yet.

The finding

The overlay is applied when the pane is created. The login shell runs after that and re-exports the same names, overwriting the sentinel. Your own file says so, in the javadoc you carefully preserved:

MEASURED ON A LIVE PANE, 2026-08-15 (CB-592): this sentinel alone does NOT hold. … A login shell overwrites a value already in the environment … That defeat applies to every name secrets.sh exports, and no launcher-side overlay can win against it.

CB-592 beat that with a second half: a guarded export inside the operator's secrets.sh, which declines to export when BRIDGED_MEMBER is set. I checked that file structurally just now — names and counts only, no values:

BRIDGED_MEMBER guards found : 1   (guarding GITEA_ACCESS_TOKEN)
total export lines          : 33

One guard, thirty-three exports. So after this merges and deploys, GITEA_ACCESS_TOKEN stays blocked exactly as before, and the other 26 blocked names get their sentinel overwritten by the login shell microseconds later. Today's live measurement already shows both halves of that: GITEA_ACCESS_TOKEN came back as the 43-character sentinel, while GITLAB_PERSONAL_ACCESS_TOKEN came back at its real length of 51.

Your tests are honest about what they test — they assert workerEnv contains the sentinel, and it does. They cannot see the login shell that runs afterwards. This is the third time in this milestone that a change passed every test at the seam and did nothing on the real path.

My share of this

I told you to "reuse the CB-592 mechanism, do not reinvent it". That was wrong advice: CB-592's mechanism is overlay plus a guarded export in a file the ticket explicitly puts out of scope for you. I pointed you at a two-part mechanism and gave you access to one part. You followed the brief correctly.

The fix — apply the block at exec time, not at pane-creation time

The pane's login shell has finished by the time the agent process is started. So an env change applied to the agent command itself wins, and needs no edit to anyone's shell files.

HerdrPeerLauncher already hands herdr an argv list (AgentControl.start(name, kind, args, paneId)). Prefix it:

argv = [ "env",
         "GITEA_ACCESS_TOKEN=blocked-by-bridged-cb596-see-gitea-issue-82",
         "GITLAB_PERSONAL_ACCESS_TOKEN=blocked-by-bridged-cb596-see-gitea-issue-82",
         ...one per blocked name...,
         <the original argv unchanged> ]

Keep the sentinel rather than env -u. An unset variable can make a tool fall back to another credential source or fail in a confusing way; a visible sentinel says deliberately blocked and is what CB-592 already established.

Points to get right:

  1. Keep the existing overlay too. It is still correct for a peer kind whose pane does not start a login shell, and the javadoc already argues that. Belt and braces — the overlay is not what is broken, it is just not sufficient.
  2. Verify env resolves in the pane and that herdr executes argv directly rather than through another shell. If herdr re-invokes a shell, say so in your reply and stop — that changes the design and I want to decide it, not have it worked around.
  3. The test must exercise the real path. A test asserting the sentinel is in workerEnv is exactly the test that passed while this did nothing. Assert on the argv actually handed to AgentControl.start, and make the assertion fail if the prefix is missing.
  4. MEMBER_MARKER still goes in the overlay — it must reach the shell startup file, which runs before exec.

Also worth fixing while you are here

Your report says an absent memberCredentials: block blocks nothing, and you flagged it loudly — good, and I agree with the reasoning that a hardcoded fallback would reintroduce the defect. But the daemon should refuse to be silent about it: log one WARN at startup when the block is missing entirely, in the same voice as the gap detector. An operator upgrading past this commit otherwise loses CB-592's protection with nothing in the log.

I will apply the memberCredentials: block to the live bridged.yaml myself at merge — you cannot see that file.

Not your fault, and good work

The config validation matches CB-606's shape, the camelCase correction was right and well-reasoned, the hostEnvNames seam is clean, and your criterion-7 sweep is exactly what I asked for. The WORKTREE_HOSTILE_CONFIGS judgment call is right too — that one is correctly hardcoded, for the reason its javadoc gives. Keep all of it.

Reviewed. The structure is right and the config work is good — but **this does not block anything on this host, and the reason is partly my brief.** Not merging yet. ## The finding The overlay is applied when the pane is **created**. The login shell runs **after** that and re-exports the same names, overwriting the sentinel. Your own file says so, in the javadoc you carefully preserved: > **MEASURED ON A LIVE PANE, 2026-08-15 (CB-592): this sentinel alone does NOT hold.** … A login shell overwrites a value already in the environment … That defeat applies to *every* name `secrets.sh` exports, and **no launcher-side overlay can win against it.** CB-592 beat that with a *second* half: a guarded export inside the operator's `secrets.sh`, which declines to export when `BRIDGED_MEMBER` is set. I checked that file structurally just now — names and counts only, no values: ``` BRIDGED_MEMBER guards found : 1 (guarding GITEA_ACCESS_TOKEN) total export lines : 33 ``` **One guard, thirty-three exports.** So after this merges and deploys, `GITEA_ACCESS_TOKEN` stays blocked exactly as before, and the other 26 blocked names get their sentinel overwritten by the login shell microseconds later. Today's live measurement already shows both halves of that: `GITEA_ACCESS_TOKEN` came back as the 43-character sentinel, while `GITLAB_PERSONAL_ACCESS_TOKEN` came back at its real length of 51. Your tests are honest about what they test — they assert `workerEnv` contains the sentinel, and it does. They cannot see the login shell that runs afterwards. This is the third time in this milestone that a change passed every test at the seam and did nothing on the real path. ## My share of this I told you to *"reuse the CB-592 mechanism, do not reinvent it"*. That was wrong advice: CB-592's mechanism is overlay **plus** a guarded export in a file the ticket explicitly puts out of scope for you. I pointed you at a two-part mechanism and gave you access to one part. You followed the brief correctly. ## The fix — apply the block at exec time, not at pane-creation time The pane's login shell has finished by the time the agent process is started. So an env change applied to **the agent command itself** wins, and needs no edit to anyone's shell files. `HerdrPeerLauncher` already hands herdr an `argv` list (`AgentControl.start(name, kind, args, paneId)`). Prefix it: ``` argv = [ "env", "GITEA_ACCESS_TOKEN=blocked-by-bridged-cb596-see-gitea-issue-82", "GITLAB_PERSONAL_ACCESS_TOKEN=blocked-by-bridged-cb596-see-gitea-issue-82", ...one per blocked name..., <the original argv unchanged> ] ``` Keep the **sentinel** rather than `env -u`. An unset variable can make a tool fall back to another credential source or fail in a confusing way; a visible sentinel says *deliberately blocked* and is what CB-592 already established. Points to get right: 1. **Keep the existing overlay too.** It is still correct for a peer kind whose pane does not start a login shell, and the javadoc already argues that. Belt and braces — the overlay is not what is broken, it is just not sufficient. 2. **Verify `env` resolves in the pane** and that herdr executes argv directly rather than through another shell. If herdr re-invokes a shell, say so in your reply and stop — that changes the design and I want to decide it, not have it worked around. 3. **The test must exercise the real path.** A test asserting the sentinel is in `workerEnv` is exactly the test that passed while this did nothing. Assert on the **argv actually handed to `AgentControl.start`**, and make the assertion fail if the prefix is missing. 4. `MEMBER_MARKER` still goes in the overlay — it must reach the shell startup file, which runs before exec. ## Also worth fixing while you are here Your report says an absent `memberCredentials:` block blocks nothing, and you flagged it loudly — good, and I agree with the reasoning that a hardcoded fallback would reintroduce the defect. But the daemon should **refuse to be silent** about it: log one WARN at startup when the block is missing entirely, in the same voice as the gap detector. An operator upgrading past this commit otherwise loses CB-592's protection with nothing in the log. I will apply the `memberCredentials:` block to the live `bridged.yaml` myself at merge — you cannot see that file. ## Not your fault, and good work The config validation matches CB-606's shape, the camelCase correction was right and well-reasoned, the `hostEnvNames` seam is clean, and your criterion-7 sweep is exactly what I asked for. The `WORKTREE_HOSTILE_CONFIGS` judgment call is right too — that one is correctly hardcoded, for the reason its javadoc gives. Keep all of it.
ltms commented 2026-08-17 09:07:34 +02:00
Owner
Copy Link
Copy Source

Second review note, found while preparing the live bridged.yaml — this one is a functional risk rather than a security one.

OPENCODE_AUTOMODE_MODEL is in known but not in allow, so it gets a sentinel

It is not a credential. There is no TOKEN, KEY, SECRET or PASSWORD in the name — it names a model. It is the same category as GITEA_HOST, which you correctly put in allow.

Nothing in our own code reads it:

grep -rn "OPENCODE_AUTOMODE_MODEL|AUTOMODE" bridged/src/main/java  →  no matches

That absence is the point. It is opencode-namespaced, so the consumer is almost certainly the opencode binary itself, reading it from its environment — and our opencode members run with --auto. Setting it to blocked-by-bridged-cb596-see-gitea-issue-82 would hand opencode that string where a model name belongs.

It is not hypothetical. Three of the seven live profiles are opencode:

local          kind=claude-code
local-direct   kind=claude-code
gx             kind=opencode      ←
opus           kind=claude-code
sonnet         kind=claude-code
sol            kind=opencode      ←
terra          kind=opencode      ←

So this would land on the free gx box and on both third-party profiles.

Move OPENCODE_AUTOMODE_MODEL into allow, with a comment saying what it is and why it is there — same treatment as GITEA_HOST. Blocking a non-credential has no upside and a clear downside.

I checked the rest of the list for the same mistake. The other non-secret identifiers — CF_ACCOUNT_ID, BESZEL_HUB_URL, TELEGRAM_CHAT_ID, CONFLUENCE_USERNAME, GRAFANA_ADMIN_USER, HW_USER, BESZEL_ADMIN_EMAIL — are all fine to block. No member consumes any of them, and most are the username half of a credential pair, so blocking them is mildly useful rather than harmful. OPENCODE_AUTOMODE_MODEL is the only one where blocking actively breaks something.

Fold this into the same round as the exec-time fix. I will mirror whatever the example ends up saying into the live bridged.yaml when I merge.

Second review note, found while preparing the live `bridged.yaml` — this one is a functional risk rather than a security one. ## `OPENCODE_AUTOMODE_MODEL` is in `known` but not in `allow`, so it gets a sentinel It is not a credential. There is no `TOKEN`, `KEY`, `SECRET` or `PASSWORD` in the name — it names a **model**. It is the same category as `GITEA_HOST`, which you correctly put in `allow`. Nothing in our own code reads it: ``` grep -rn "OPENCODE_AUTOMODE_MODEL|AUTOMODE" bridged/src/main/java → no matches ``` That absence is the point. It is opencode-namespaced, so the consumer is almost certainly the **opencode binary itself**, reading it from its environment — and our opencode members run with `--auto`. Setting it to `blocked-by-bridged-cb596-see-gitea-issue-82` would hand opencode that string where a model name belongs. It is not hypothetical. Three of the seven live profiles are opencode: ``` local kind=claude-code local-direct kind=claude-code gx kind=opencode ← opus kind=claude-code sonnet kind=claude-code sol kind=opencode ← terra kind=opencode ← ``` So this would land on the free `gx` box and on both third-party profiles. **Move `OPENCODE_AUTOMODE_MODEL` into `allow`**, with a comment saying what it is and why it is there — same treatment as `GITEA_HOST`. Blocking a non-credential has no upside and a clear downside. I checked the rest of the list for the same mistake. The other non-secret identifiers — `CF_ACCOUNT_ID`, `BESZEL_HUB_URL`, `TELEGRAM_CHAT_ID`, `CONFLUENCE_USERNAME`, `GRAFANA_ADMIN_USER`, `HW_USER`, `BESZEL_ADMIN_EMAIL` — are all fine to block. No member consumes any of them, and most are the username half of a credential pair, so blocking them is mildly useful rather than harmful. `OPENCODE_AUTOMODE_MODEL` is the only one where blocking actively breaks something. Fold this into the same round as the exec-time fix. I will mirror whatever the example ends up saying into the live `bridged.yaml` when I merge.
ltms added 1 commit 2026-08-17 09:14:05 +02:00
CB-596 round 2: the exec-time argv-prefix fix has no seam — stop and report
CI / contract (pull_request) Successful in 47s
Details
CI / build (pull_request) Successful in 1m6s
Details
7930a31b94 
herdr protocol 19's agent.start takes a fixed `kind` (herdr resolves the
executable) plus trailing CLI args for that binary; only tab.create/pane.split
accept an env map, and that IS the round-1 pane-creation overlay already
shipped. There is no argv/env control point that runs after the pane's login
shell and before the agent process starts, so the proposed `env NAME=value ...`
argv prefix cannot be implemented against this API. Documented the finding and
corrected bridged.example.yaml's round-1 comments, which had overclaimed that
the overlay survives the login shell.

Kept everything else: added a startup WARN (Bridged.reportMemberCredentialsGap)
when memberCredentials: is absent or its known: list is empty, so CB-592's
protection loss is never silent, mirroring CB-594's reportRequiredSecrets.
ltms commented 2026-08-17 13:46:51 +02:00
Owner
Copy Link
Copy Source

Delivered and merged — closing manually.

This landed on main as ac40de1 ("Merge CB-596: member credential policy is config-driven deny-by-default"), confirmed reachable from origin/main. The lead merged locally, so Gitea never saw the merge and left the PR open. That is a known habit here, not a problem with this work.

Since the merge: 870 tests BUILD SUCCESS, daemon redeployed onto jar e11160695fbe, and the policy verified in a live member pane — 29 of 29 blocked names hold the sentinel, 5 allow-listed names keep their real values. Details on #82, which is now closed.

Two follow-ups came out of this change, both on 2.0 and neither a defect in it:

  • #110 (CB-607) — your gap detector fired on its very first spawn and named SSH_AUTH_SOCK, which no hand-written list had ever contained because it is not in secrets.sh at all. That detector is the most valuable thing in this PR.
  • #111 (CB-608) — the probe script keeps its own hardcoded name list and under-reported by three.

Thanks — the config-driven design is what made both of those findable.

**Delivered and merged — closing manually.** This landed on `main` as `ac40de1` ("Merge CB-596: member credential policy is config-driven deny-by-default"), confirmed reachable from `origin/main`. The lead merged locally, so Gitea never saw the merge and left the PR open. That is a known habit here, not a problem with this work. Since the merge: 870 tests BUILD SUCCESS, daemon redeployed onto jar `e11160695fbe`, and the policy verified **in a live member pane** — 29 of 29 blocked names hold the sentinel, 5 allow-listed names keep their real values. Details on #82, which is now closed. Two follow-ups came out of this change, both on 2.0 and neither a defect in it: - **#110 (CB-607)** — your gap detector fired on its very first spawn and named `SSH_AUTH_SOCK`, which no hand-written list had ever contained because it is not in `secrets.sh` at all. That detector is the most valuable thing in this PR. - **#111 (CB-608)** — the probe script keeps its own hardcoded name list and under-reported by three. Thanks — the config-driven design is what made both of those findable.
ltms closed this pull request 2026-08-17 13:46:57 +02:00
Some checks are pending
CI / contract (pull_request) Successful in 47s
Details
CI / build (pull_request) Successful in 1m6s
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
blocked
Cannot start until something else lands. The body says what.
 needs-live-proof
Merged and green, but never shown working on the running daemon. Not the same as done.
 ready-to-delegate
Scope, files and acceptance criteria are written. A worker can be briefed from the body alone.
 silent-default
A feature that compiles, passes tests, and ships turned off. Nine recurrences and counting.
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
agent kevin ltms
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: fleet/fleetd#109
Delete Branch "worker/cb596-4e49ef-3"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?