werkator build succeeded after 01:23
Co-authored-by: mhoennig <michael@hoennig.de> Reviewed-on: #55
193 lines
13 KiB
Markdown
193 lines
13 KiB
Markdown
> **WARNING:** This document describes only the change applied in this PR.
|
|
> It may already be outdated once the next PR is merged.
|
|
> Historic PR-documentation is not maintained along with new PRs — treat it as a snapshot, not as current documentation.
|
|
|
|
## The Problem
|
|
|
|
A security audit found six ways a build could reach past the boundaries Werkator sets for it.
|
|
Five of them are holes in defences the code already claims to have.
|
|
|
|
Werkator states its trust model in several places.
|
|
`stripPinned` exists so a branch cannot switch off its own sandbox.
|
|
`requirePullRequest` is a gate for building what is not fully trusted.
|
|
The Docker and werkdock runners mask `.git/werkator/` with an empty tmpfs so a build never sees the git token.
|
|
ADR 0008 exists because builds are expected to run isolated.
|
|
Each finding below is a place where one of these statements did not hold.
|
|
|
|
The objection that whoever may push to a built branch has the build host anyway does not carry.
|
|
It is contradicted by the code's own defences, and it breaks down in three concrete ways:
|
|
one instance serves several repositories (ADR 0009), so a pusher to one is not authorised on another;
|
|
artifacts of a public repository are downloadable by anyone, so the audience of a leak is the internet, not the pusher;
|
|
and the victim of the artifact-origin finding is the operator, whose browser holds the control token.
|
|
|
|
## Non-Goals
|
|
|
|
- No separate origin or host for artifact serving; the sandbox header solves the same problem without new DNS, certificates and broken permalinks.
|
|
- No disposable per-build Docker daemon. The socket becomes opt-in here; isolating the builds that genuinely need a daemon is deferred.
|
|
- No change to the artifact-collection order in `BuildExecutor`. The terminal status is still recorded before artifacts are collected, and the escape case takes the status back afterwards instead of reordering the lifecycle.
|
|
- No review of the trust model as such: this PR restores what the code promises, it does not redefine it.
|
|
|
|
## The Scenarios
|
|
|
|
### Feature: a build stays inside the boundaries set for it
|
|
|
|
#### Background
|
|
|
|
- The *branch layer* is the `.werkator.yml` committed on a branch; it describes that branch's build and loses its *pinned* keys (`ConfigLoader.stripPinned`).
|
|
- *Staging* is the per-build directory the captured output is written to, later copied into the artifact store.
|
|
- *Collection* is the copying of a build's declared artifact directories into the store; it runs in the server process on the host, not in the build's sandbox.
|
|
|
|
#### Scenario#55.01: An artifact page cannot act as the instance
|
|
|
|
So that a report a build wrote cannot use the operator's session against the instance that serves it.
|
|
|
|
- **Given** a build stored an HTML page as an artifact
|
|
- **When** an operator opens that page
|
|
- **Then** the response carries `Content-Security-Policy: sandbox allow-scripts`
|
|
- **and** the document runs on an opaque origin, reaching neither the control token in `localStorage` nor the JSON API
|
|
- **and** its own scripts still run, so an interactive report keeps working
|
|
- **and** the response carries `X-Content-Type-Options: nosniff`
|
|
- **and** the same holds for every path to the same bytes, the permanent `/branches/…` URL included
|
|
|
|
##### Verified by
|
|
|
|
- [ArtifactFileControllerTest: an html artifact is sandboxed onto its own origin, away from the control token](../../src/test/kotlin/de/hoennig/werkator/server/ArtifactFileControllerTest.kt)
|
|
- [ArtifactFileControllerTest: every artifact file is sandboxed, not only the pages a build calls index.html](../../src/test/kotlin/de/hoennig/werkator/server/ArtifactFileControllerTest.kt)
|
|
- [ArtifactFileControllerTest: the permanent URL is sandboxed like every other way to the same bytes](../../src/test/kotlin/de/hoennig/werkator/server/ArtifactFileControllerTest.kt)
|
|
|
|
#### Scenario#55.02: A build gets a Docker daemon only when it says it needs one
|
|
|
|
So that the way around the container is taken deliberately, by the repository, and not handed to every build by default.
|
|
|
|
- **Given** a repository configures `docker.enabled: true`
|
|
- **When** a build runs without `docker.socket`
|
|
- **Then** no Docker socket is mounted into the build container
|
|
- **When** a build runs with `docker.socket: true`
|
|
- **Then** the socket is mounted as before
|
|
- **And** a branch that sets `docker.socket` in its own committed configuration is ignored, because the key is pinned
|
|
- **And** whether the daemon is rootless still decides the ownership repair, independently of the socket
|
|
|
|
##### Verified by
|
|
|
|
- [DockerBuildRunnerTest: without docker.socket the daemon stays outside the build container](../../src/test/kotlin/de/hoennig/werkator/build/DockerBuildRunnerTest.kt)
|
|
- [DockerBuildRunnerTest: a build without the socket still follows the rootless daemon's ownership rules](../../src/test/kotlin/de/hoennig/werkator/build/DockerBuildRunnerTest.kt)
|
|
- [ConfigLoaderTest: loadForWorktree pins secrets and the docker sandbox policy to .git, but allows docker.image](../../src/test/kotlin/de/hoennig/werkator/config/ConfigLoaderTest.kt)
|
|
|
|
#### Scenario#55.03: A werkdock build cannot write the primary checkout
|
|
|
|
So that a build cannot set the configuration the next build is trusted to read.
|
|
|
|
- **Given** a build runs in a werkdock sandbox
|
|
- **When** the sandbox is assembled
|
|
- **Then** the repository directory is mounted read-only
|
|
- **and** the branch's worktree is mounted read-write
|
|
- **and** that worktree's git admin directory is mounted read-write
|
|
- **and** the committed `.werkator.yml` at the repository root is therefore not writable, which is the layer the pinned settings are read from
|
|
|
|
##### Verified by
|
|
|
|
- [WerkdockBuildRunnerTest: mounts the primary checkout read-only, the workspace read-write](../../src/test/kotlin/de/hoennig/werkator/build/WerkdockBuildRunnerTest.kt)
|
|
- [WerkdockBuildRunnerTest: assembles the exact werkdock run command for a loaded image](../../src/test/kotlin/de/hoennig/werkator/build/WerkdockBuildRunnerTest.kt)
|
|
|
|
#### Scenario#55.04: Artifact collection does not read for the build what the build cannot read
|
|
|
|
So that a build cannot use the server as its reader, and a public repository cannot be turned into a publisher of host files.
|
|
|
|
- **Given** a build's artifact directory holds a symlink
|
|
- **When** the link resolves inside the collected directory
|
|
- **Then** it is copied as before
|
|
- **When** the link leaves the collected directory, or dangles
|
|
- **Then** the collection is aborted and nothing of that build is stored
|
|
- **and** the build is recorded as FAILED even though its commands succeeded
|
|
- **and** the link's target is untouched
|
|
|
|
##### Verified by
|
|
|
|
- [FileArtifactStoreTest: a symlink out of the artifact directory is refused and nothing is stored](../../src/test/kotlin/de/hoennig/werkator/artifacts/FileArtifactStoreTest.kt)
|
|
- [FileArtifactStoreTest: a symlink inside the artifact directory is stored like the file it names](../../src/test/kotlin/de/hoennig/werkator/artifacts/FileArtifactStoreTest.kt)
|
|
- [FileArtifactStoreTest: a dangling symlink is refused rather than guessed at](../../src/test/kotlin/de/hoennig/werkator/artifacts/FileArtifactStoreTest.kt)
|
|
- [BuildExecutorArtifactIntegrationTest: a build whose artifacts point out of the workspace ends up FAILED with nothing stored](../../src/test/kotlin/de/hoennig/werkator/artifacts/BuildExecutorArtifactIntegrationTest.kt)
|
|
|
|
#### Scenario#55.05: The pinned settings hold whatever the branch layer looks like
|
|
|
|
So that a branch cannot switch off a sandbox by writing a value that removes it instead of setting it.
|
|
|
|
- **Given** a host requires containers (`docker.enabled: true`)
|
|
- **When** a branch writes `docker: null`, or a definition as a scalar
|
|
- **Then** the load is refused, naming the offending key and the shape it expects
|
|
- **When** a branch writes `builds.<name>: null`
|
|
- **Then** that is accepted, because it says this branch runs no such build
|
|
- **When** a branch empties `builds.default` and defines a build of its own
|
|
- **Then** that build still carries the host's sandbox policy, taken from the host's `builds.default`
|
|
|
|
##### Verified by
|
|
|
|
- [ConfigLoaderTest: a branch that writes a pinned block as null is refused, not silently obeyed](../../src/test/kotlin/de/hoennig/werkator/config/ConfigLoaderTest.kt)
|
|
- [ConfigLoaderTest: a branch that writes a definition as a scalar is refused](../../src/test/kotlin/de/hoennig/werkator/config/ConfigLoaderTest.kt)
|
|
- [ConfigLoaderTest: emptying builds.default does not drop the sandbox policy of the branch's own builds](../../src/test/kotlin/de/hoennig/werkator/config/ConfigLoaderTest.kt)
|
|
- [ConfigLoaderTest: a branch whose layer lacks the predecessor loses only its follow-up](../../src/test/kotlin/de/hoennig/werkator/config/ConfigLoaderTest.kt)
|
|
|
|
#### Scenario#55.06: A build's log file is a name, not a path
|
|
|
|
So that a build cannot have the server truncate and overwrite a file on the host with its output.
|
|
|
|
- **Given** a build definition sets `stdoutLog` or `stderrLog`
|
|
- **When** the value is a plain file name
|
|
- **Then** the log is written inside the build's staging directory as before
|
|
- **When** the value is absolute, climbs out with `..`, names a subdirectory, or is blank
|
|
- **Then** the build fails before any file is opened, and the message names the key
|
|
|
|
##### Verified by
|
|
|
|
- [BuildLogNamesTest](../../src/test/kotlin/de/hoennig/werkator/build/BuildLogNamesTest.kt)
|
|
- [BuildExecutorArtifactIntegrationTest: a build naming a host path as its log file fails without writing that file](../../src/test/kotlin/de/hoennig/werkator/artifacts/BuildExecutorArtifactIntegrationTest.kt)
|
|
|
|
## The Solution
|
|
|
|
**Artifacts on their own origin.**
|
|
`ArtifactFileController.respond` — the single place every artifact byte passes — adds `Content-Security-Policy: sandbox allow-scripts` and `X-Content-Type-Options: nosniff`.
|
|
`allow-scripts` keeps interactive reports working; `allow-same-origin` must never join it, as the two together dissolve the sandbox.
|
|
A separate origin would be stronger still, and is a non-goal here: it costs DNS, certificates and every existing permalink, including the `target_url` already recorded in Gitea.
|
|
|
|
**The Docker socket is opt-in.**
|
|
New `docker.socket`, default `false`, pinned alongside `docker.enabled` and `docker.network`.
|
|
The socket is still located either way, because whether the daemon is rootless decides the in-container ownership repair — that is a property of the host, not a permission of the build.
|
|
|
|
**The primary checkout is read-only under werkdock.**
|
|
Only the branch's worktree and its git admin directory stay writable, which is what a build needs.
|
|
The mount order is unchanged; the base mount merely lost its write permission.
|
|
|
|
**Collection stops at the edge of what it collects.**
|
|
`Files.copy` follows symlinks, and collection runs on the host outside the build's sandbox, so a link was enough to have Werkator read a file the build could not open.
|
|
Links resolving inside the collected directory are copied as before; one that leaves it raises `ArtifactEscapeException`, the incoming directory is discarded, and `BuildExecutor` takes the terminal status back to FAILED.
|
|
That late correction is deliberate: collection runs after the status is recorded, and reordering the lifecycle would touch the worktree pruning and the "still finishing" view that rely on the current order.
|
|
|
|
**Pinning is enforced, not only subtracted.**
|
|
`checkLayerShape` refuses a branch layer that writes a non-map where a block belongs, naming the key — ignoring it would leave a line doing something other than what it says, the same reasoning as `checkTriggerBlocks`.
|
|
`enforcePinned` then writes the pinned values back from the host's layers after merging, falling back to the host's `builds.default`, so the pinning holds by what the merged configuration says rather than by what was removed from the branch layer.
|
|
`builds.<name>: null` keeps its established meaning.
|
|
|
|
**Log names are names.**
|
|
`BuildLogNames.requireSimple` rejects absolute, climbing, nested and blank names before any stream is opened.
|
|
|
|
## Open Questions
|
|
|
|
- The artifact sandbox forbids a report from calling Werkator's own API. No report is known to do this; if one ever should, it needs its own decision rather than a wider header.
|
|
- `docker.socket` makes the privilege visible but does not reduce it: a build that sets it is still as privileged as the daemon. The disposable daemon is the actual fix (see Follow-up PRs).
|
|
- A build failing at collection reports `success` to Gitea first and `failure` a moment later. Correct in substance, briefly confusing in the UI.
|
|
|
|
## Additional Changes
|
|
|
|
- `AGENTS.md`, `docs/configuration.md` and the architecture skill describe the new pinned key and the two-part pinning.
|
|
- The `init` templates document `docker.socket` and the log-name rule.
|
|
- Version bumped to 1.10.0 with its release-notes entry.
|
|
|
|
## Prerequisite PRs
|
|
|
|
- [PR#54](2026-09-11-PR%2354-a-home-page-for-the-instance.md) — the branch starts from it; RFC 0003 D8/D9 define what a private repository gives out, which Scenario#55.01 and #55.04 build on.
|
|
|
|
## Follow-up PRs
|
|
|
|
- A disposable Docker daemon per build, so that builds needing a daemon stop needing the host's (audit finding 3, the part left open here).
|
|
- vm4006 needs `docker.socket: true` for Werkator's own Testcontainers build before it is updated to this version.
|