Files
werkator/docs/prs/2026-09-11-PR#55-security-hardening.md
miandmhoennig 01f59d1a06
werkator build succeeded after 01:23
Security hardening (#55)
Co-authored-by: mhoennig <michael@hoennig.de>
Reviewed-on: #55
2026-09-11 14:11:23 +02:00

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.