Skip to content

feat(runner): mechanical push fences + a 407 challenge that lets git out at all - #118

Merged
rubenvdlinde merged 6 commits into
developmentfrom
feat/builder-push-controls
Aug 2, 2026
Merged

feat(runner): mechanical push fences + a 407 challenge that lets git out at all#118
rubenvdlinde merged 6 commits into
developmentfrom
feat/builder-push-controls

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

Tasks 1–3 of hydra's builder-write-access-hardeningthe controls, before the
capability
. Nothing here gives anything write access; task 4 does that, later,
on purpose.

What this is

hydra's builder is to get a writable tree and a forge credential that can push.
Repository content is hostile input — the model reads code and issue text it did
not author and then acts with write access — so no security property here may
depend on its compliance.

The load-bearing line is a withholding, not a rule

When a stage declares push, the command child runs without
GIT_FORGE_TOKEN and GIT_ASKPASS. The runner clones with the credential and
pushes with the credential; the one process that reads untrusted content never
holds it.

Without that, a shell-capable agent could run git push itself and no
runner-side check could observe it — the fences would be decoration around a
hole, and they would pass every unit test, because a unit test drives them
directly.

On top of that (src/pushGuard.js)

  • the destination must be the one repository the stage was scoped to, host
    included (evil.example/ConductionNL/hydra has the same owner and name);
  • the branch must be feature/<issue>/<slug> for this issue, anchored at
    both ends, protected names refused by name, refs/... refused rather than
    normalised;
  • the diff may not touch .github/workflows, .forgejo/workflows,
    .github/actions or .git/, may not touch a dependency manifest at any depth,
    and may not leave the issue's declared scope.

Every unreadable input fails closed — an empty change set passes every rule,
so "could not read the diff" must never be spelled the same way as "nothing
changed".

The change set comes from git status --porcelain --untracked-files=all, not
git diff. A file the builder creates is untracked, so a diff-only read
reports nothing and a brand-new .github/workflows/pwn.yml goes straight
through.

The bug that would have made all of it moot

The governed CONNECT proxy could not carry git traffic at all, and nothing
could see it.

HTTPS_PROXY=http://run:<token>@egress-proxy:3128 does not make every client
present the credential. curl's CLI sends Proxy-Authorization preemptively; git
sets libcurl's proxy auth to CURLAUTH_ANY, which waits for a 407 challenge
first. Answering 403 told git there was nothing to offer, so it never offered the
token already in its proxy URL.

Measured inside the jailed container, same proxy, same URL:

curl --proxy http://run:tok@egress-proxy:3128 https://github.com/
    => Proxy-Authorization sent  => 200 Connection Established
git  HTTPS_PROXY=http://run:tok@egress-proxy:3128 ls-remote https://github.com/…
    => no Proxy-Authorization    => 403 no_run_token, every time

The 14 existing proxy tests could not have caught it: their client sets the
header itself, so every case exercised the authenticated path exclusively. An
unauthenticated CONNECT is now 407 with Proxy-Authenticate: Basic — it opens
no tunnel, so default-deny is intact.

Two more gaps that deny every stage behind the proxy

  1. A stage had no run token. /run has minted one per turn since governed
    egress shipped; /stage never did. The PDP refuses a token-less CONNECT
    before evaluating policy, and the symptom is a clone failure that points at
    the forge.
  2. The TTL was wrong by two orders of magnitude. 150 s (a turn) against a
    30-minute stage ceiling: the clone at the start succeeds, the push at the end
    is refused invalid_token. mint() now takes a TTL and the stage passes its
    own ceiling.

How it was proved

test/stage.push.test.js runs against a real git remote that demands HTTP
Basic auth
— with a file:// remote every push succeeds regardless of
credentials, so the central claim would pass either way — and asserts at the
destination: a refused push is proved by the bare repository still pointing
at the same commit.

Each control was mutation-checked:

mutation result
remove the assertPushAllowed() call the four fence tests go red, and the push to main succeeds
leave the credential in the command child the two credential tests go red, and the injected git push origin HEAD:refs/heads/main succeeds
read the change set with git diff the new .github/workflows/pwn.yml goes through

Live, on the hardened posture (cap_drop: ALL, read-only root, internal
network, no default route):

  • direct egress from the jailed container: refused in 118 ms — the same probe
    from an unjailed container connects in 277 ms;
  • non-forge hosts through the proxy: not_allowlisted in 26–48 ms;
  • six consecutive clones of a public repo through the proxy: 6/6 OK,
    855–1123 ms
    . The iptables jail this replaces gave 2-in-3 failures at ~135 s.

Suites: test/test.sh 25/25 · phpunit-unit.xml 1427 tests, 4421 assertions.

⚠️ Operator prerequisite, measured

Run tokens live in ICacheFactory::createDistributed(). With no
memcache.distributed configured that falls back to APCu — per process pool.
A token minted in a CLI process and POSTed to the live PDP within the same second
came back 401 invalid_token. On an instance running background jobs in
cron mode (Nextcloud's own recommendation) every flow-dispatched stage would be
denied egress. Recorded in deploy/egress-allowlist.md along with the other
measured trap: the allowlist is exact hostnames, so with github.com allowed,
api.github.com is denied.

Not in scope, deliberately

HermiqWorkloadNode is untouched — a flow cannot declare a push, so nothing gains
write access from this PR.

…he child that reads hostile input

A builder stage clones a repository, lets a model change it, and pushes the
result. The model reads code and issue text it did not author, so everything it
touches is hostile input and no security property may depend on its compliance.

The load-bearing line is not a rule, it is a withholding: when a stage declares
`push`, the command child runs WITHOUT `GIT_FORGE_TOKEN` and `GIT_ASKPASS`. The
runner clones with the credential and pushes with the credential; the one
process that reads untrusted content never holds it. Without that, a
shell-capable agent could simply run `git push` and no runner-side rule could
observe it — the fences would be decoration around a hole, and they would pass
every unit test, because a unit test drives them directly.

On top of that, `pushGuard.js`:
  - the push target must be the ONE repository the stage was scoped to
    (host included — `evil.example/ConductionNL/hydra` has the same owner and
    name and is somebody else's server);
  - the branch must be `feature/<issue>/<slug>` for THIS issue, anchored at both
    ends, with protected names refused by name and `refs/...` refused rather
    than normalised;
  - the diff may not touch `.github/workflows`, `.forgejo/workflows`,
    `.github/actions` or `.git/`, may not touch a dependency manifest at any
    depth, and may not leave the scope the issue declared.

Every unreadable input fails CLOSED, because an empty change set passes every
rule and "could not read the diff" must never be spelled the same way as
"nothing changed".

The change set comes from `git status --porcelain --untracked-files=all`, not
`git diff`. A file the builder CREATES is untracked, so a diff-only read reports
nothing and a brand-new `.github/workflows/pwn.yml` goes straight through.

Also closes an asymmetry the governed-egress change left behind: `/run` has
built a per-run proxy URL since it shipped, but `/stage` only passed through the
container's static `HTTPS_PROXY`. Behind the governed proxy that is not a
smaller fence, it is no route at all — the PDP denies a token-less CONNECT with
`no_run_token` before it evaluates policy. `/stage` now takes `runToken` and
builds the same per-run URL, assigned after the passthrough so a static value
can never shadow it.

Proved, not asserted. `stage.push.test.js` runs against a real git remote that
demands HTTP Basic auth (a `file://` remote needs no credential, so the central
claim would pass either way) and asserts at the DESTINATION — a refused push is
proved by the bare repository still pointing at the same commit. Each control
was mutation-checked:

  - remove the `assertPushAllowed()` call  -> the four fence tests go red, and
    the push to `main` SUCCEEDS;
  - leave the credential in the command child -> the two credential tests go
    red, and the injected `git push origin HEAD:refs/heads/main` succeeds;
  - read the change set with `git diff`   -> the new workflow file goes through.

25 assertions in `test/test.sh`, all green.
…nt the token it held

The governed CONNECT proxy could not carry `git` traffic at all, and nothing
could see it.

`HTTPS_PROXY=http://run:<token>@egress-proxy:3128` does not make every client
present the credential. curl's CLI defaults to Basic and sends
`Proxy-Authorization` preemptively; git sets libcurl's proxy auth to
`CURLAUTH_ANY`, which waits for a 407 challenge first. Answering an
unauthenticated CONNECT with 403 told git there was nothing to offer, so it never
offered the token already sitting in its proxy URL.

Measured inside the jailed container, same proxy, same URL:

  curl --proxy http://run:tok@egress-proxy:3128 https://github.com/
      => Proxy-Authorization sent  => 200 Connection Established
  git  HTTPS_PROXY=http://run:tok@egress-proxy:3128 ls-remote https://github.com/…
      => no Proxy-Authorization    => 403 no_run_token, every time

The 14 existing proxy tests could not have caught it: `connectThrough()` sets the
header itself, so every case exercised the authenticated path exclusively. A
control that blocks the one workload it exists to govern is the iptables jail's
failure repeated one layer up.

An unauthenticated CONNECT is now answered `407 Proxy Authentication Required`
with `Proxy-Authenticate: Basic realm="hermiq-egress"`. It opens no tunnel, so
default-deny is intact — it is a refusal that names how to proceed. A token that
IS presented and refused by policy stays a 403: retrying cannot help, and
challenging again would loop.

Two tests, not one. Asserting the 407 alone would certify a proxy that challenges
and then refuses the answer — indistinguishable from the bug. The second drives
the actual two-step exchange: CONNECT, take the challenge, CONNECT again with the
credential, tunnel opens, PDP consulted exactly once.

Live, on the hardened stack (cap_drop ALL, read-only root, internal network, no
default route): six consecutive clones of a public repo through the proxy, 6/6 OK
at 855–1123 ms. The jail this replaces gave 2-in-3 failures at ~135 s.

egress-allowlist.md also records two things measured while proving this, either
of which denies every stage:

  - the allowlist is EXACT hostnames. With `github.com` allowed, `api.github.com`
    is denied `not_allowlisted`.
  - run tokens live in `createDistributed()`, which falls back to APCu — PER
    PROCESS POOL. A token minted in a CLI process and POSTed to the live PDP a
    second later came back 401 `invalid_token`. On an instance running background
    jobs in cron mode (Nextcloud's own recommendation) every flow-dispatched
    stage would be denied egress, and the symptom is a clone failure rather than
    a policy error. `memcache.distributed` is a prerequisite.
…at it may write

Two gaps between the workload plane and the governed egress posture, both of
which deny every stage the moment the sidecar is put behind the proxy.

1. A STAGE HAD NO RUN TOKEN. `ProviderFactory` has minted one for every LLM turn
   since governed egress shipped; `StageDispatchService` never did. Behind the
   CONNECT proxy that is not a smaller fence but no route at all — the PDP
   refuses a token-less CONNECT with `no_run_token` before it evaluates any
   policy — and the symptom is a `git clone` failure that points at the forge.

2. THE TTL WAS THE WRONG ONE BY TWO ORDERS OF MAGNITUDE. `RunTokenService`'s
   default is the LLM turn's timeout plus slack: 150 seconds. A stage's ceiling
   is thirty minutes, so a stage token minted at the default expires mid-workload
   — the clone at the start succeeds and the push at the end is refused
   `invalid_token`, which reads like a credential problem and is not. `mint()`
   takes an optional TTL and the stage passes its own ceiling, so the token still
   dies with the work it belongs to.

`dispatch()` also takes a `push` declaration and forwards it verbatim. Its
presence is what makes the runner withhold the forge credential from the command
child and perform the push itself behind the branch/repository/diff fences; its
absence leaves a stage exactly as read-only as it has always been.

⚠️ DELIBERATELY NOT WIRED INTO `HermiqWorkloadNode`. A flow cannot declare a
push yet, so nothing gains write access from this commit. The fences and the
transport come first; connecting the switch is the capability, and the capability
is a later task on purpose — turning on write access before the controls exist
inverts the risk for the whole window in between.

Tests: `buildParams()` becomes `protected` for the same reason `mapResult()` and
`reasonFrom()` already are — a shape check nothing exercises is one that silently
stops holding — and three cases assert what only the payload builder can show:
every stage carries a token, a stage that declares no push sends no `push` KEY
(an empty object would read as "may write"), and a declared push arrives intact.
`RunTokenService` is constructed for real over a stub cache rather than mocked:
a mock returning a fixed string asserts that the call happens and nothing about
what is made, which is precisely where the TTL bug lived.

1427 tests, 4421 assertions, green.

⚠️ Operator prerequisite, measured on a live instance and recorded in
egress-allowlist.md: run tokens live in `ICacheFactory::createDistributed()`,
which with no `memcache.distributed` configured falls back to APCu — per process
pool. A token minted in a CLI process and POSTed to the live PDP within the same
second came back 401 `invalid_token`. On an instance running background jobs in
cron mode, which is Nextcloud's own recommendation, every flow-dispatched stage
would be denied egress.
@github-actions

github-actions Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/hermiq @ 545a02e

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
composer ✅ 117/117
npm ✅ 657/657
PHPUnit ⏭️
Newman ⏭️
Playwright ⏭️

Quality workflow — 2026-08-02 15:44 UTC

Download the full PDF report from the workflow artifacts.

buildParams() reached 128 lines and phpcs read the prose paragraph after the
@param list as a misaligned continuation of the last one.

Two extractions rather than shorter comments: mintEgressIdentity() (why a stage
needs a token at all, why the TTL is the stage's own ceiling, and the APCu
process-pool trap) and withToolTree() (which tree the command comes from). Both
are self-contained decisions that were only ever in buildParams() because it grew
around them — the length gate was reporting something real.

phpcs 0 errors, phpmd clean, 1427 tests green.
@github-actions

github-actions Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/hermiq @ 896cbba

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
composer ✅ 117/117
npm ✅ 657/657
PHPUnit
Newman
Playwright ⏭️

Coverage: 85.3% (93/109 statements)


Quality workflow — 2026-08-02 15:56 UTC

Download the full PDF report from the workflow artifacts.

… fails otherwise

CI runs PHPUnit with beStrictAboutCoverageMetadata="true", which fails any test
that executes a class the file has not declared. The dispatcher tests construct
a REAL RunTokenService over a stub cache, so six of them errored with 'executed
code that is not listed as code to be covered or used'.

Not reproducible locally: the container has no coverage driver, so the strictness
never engages and the suite reads OK. Worth stating — a green local run says
nothing about this particular gate.

The real service stays. A mock returning a fixed string would assert that the
call happens and nothing about what is made, and what is made is exactly where
the TTL bug lived.
@github-actions

github-actions Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/hermiq @ 49df2b2

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
composer ✅ 117/117
npm ✅ 657/657
PHPUnit
Newman
Playwright ⏭️

Coverage: 85.3% (93/109 statements)


Quality workflow — 2026-08-02 16:01 UTC

Download the full PDF report from the workflow artifacts.

…ses it there too

My own explanation of the declaration broke it. PHPUnit scans the whole docblock
for annotations, so the backticked mention in the paragraph above was read as a
second declaration and the trailing backtick made it invalid — failing all twelve
tests in the file with a message that names none of them.

Still not reproducible locally: the container has no coverage driver, so the
strictness never engages and the suite reads OK either way.
@github-actions

github-actions Bot commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/hermiq @ edde55d

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
composer ✅ 117/117
npm ✅ 657/657
PHPUnit
Newman
Playwright ⏭️

Coverage: 85.3% (93/109 statements)


Quality workflow — 2026-08-02 16:05 UTC

Download the full PDF report from the workflow artifacts.

@rubenvdlinde
rubenvdlinde merged commit 19fb97b into development Aug 2, 2026
33 checks passed
rubenvdlinde added a commit that referenced this pull request Aug 2, 2026
… credential (#122)

The runner has accepted a `push` declaration since #118 and
`StageDispatchService::dispatch()` forwarded it, but `HermiqWorkloadNode` had no
`push` config key, so no flow could declare one and nothing had gained write
access. This is that switch.

TWO KEYS, NOT ONE, and the reason is not tidiness. `credentialId` is spent on
the broker's SERVER-SIDE calls — the tool tree arrives as
`GET /repos/*/tarball/*` performed inside OpenRegister — which only a
host-locked PROXY credential can serve, and `resolveInjectable()` refuses that
exact shape by design. A push needs the opposite: git speaks the smart-HTTP pack
protocol, so there is no single call to proxy and the token has to BE in the
container, i.e. an `inject_only` credential. The two postures are mutually
exclusive by construction, so one id could never have expressed a stage that
fetches a private tool tree AND pushes. Hence `pushCredentialId`, falling back
to `credentialId` so every read-only stage that shipped before stays on the path
it was already on.

Mutation-checked: collapse the two back into one and
`testTheBrokerCredentialAndThePushCredentialAreNotTheSameCall` goes red. Both
ids reach the broker either way, so nothing about the payload distinguishes the
fixed code from the collapsed code — only the id each CALL was made with does,
which is why `resolveForgeToken()` and `fetchToolArchive()` became protected.

`mapResult()` NOW CARRIES `push`. It is an allowlist, so a key the runner
returns and it does not name is a key no flow can ever see: without this a flow
could declare a push, the runner could perform it, and the record would say only
`exitCode: 0` — leaving "it pushed" and "it found nothing to push"
indistinguishable, which is the conflation every other seam in that class exists
to prevent. Absent rather than `false` for a read-only stage.

The push declaration is RENDERED per item, like every other configured value.
`branch` and `issue` together ARE the allowlist `pushGuard` enforces and both
are derived per item; an unrendered `feature/{{issueNumber}}/x` would be refused
by the runner as "outside the allowlist" — a refusal that reads like a scope
violation rather than a templating bug, which is exactly how the un-rendered
`credentialId` hid for a release.

Refused at SAVE time when a push names no branch or no issue. `pushGuard` builds
its pattern out of the issue number and fails closed without one, so such a flow
is not one with a wider fence — it is one that can never push, and the author
should hear that while they can still fix it. Held on `execute()` too, because a
seeded or imported flow never passes `validateConfig()`.

The load-bearing control is unchanged and was re-verified by mutation: when a
stage declares `push` the command child runs WITHOUT `GIT_FORGE_TOKEN` and
`GIT_ASKPASS`. Leave the credential in the child and both "the command child
does NOT hold the credential" and "an injected instruction cannot push" go red —
the second being the one that shows the property does not depend on the model.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant