9f5f99aa86
PR#55 added docker.socket to DockerConfig and to both pinning lists, but not to DockerOverrides. The key was parsed, pinned and enforced, and then dropped when the definition was applied — leaving the default false. Since ADR 0007 the sandbox lives under builds.<name>.docker, so the key was unreachable from the only section a current configuration uses; only the deprecated branches section could set it, which is where the existing test put it. On vm4006 this cost every Testcontainers test: the socket was never mounted, and the build found no Docker environment at all. Beside the three-line fix, a test now compares DockerConfig against DockerOverrides and WerkdockConfig against WerkdockOverrides, so a key added to a sandbox and forgotten in its overrides fails the build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: mhoennig <michael@hoennig.de> Reviewed-on: #72
122 lines
6.1 KiB
Markdown
122 lines
6.1 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.
|
|
|
|
## Related Links
|
|
|
|
- [PR#55](2026-09-11-PR%2355-security-hardening.md) — introduced `docker.socket` as an opt-in, pinned key.
|
|
- ADR 0007 — build definitions; the reason the key's only usable spelling is `builds.<name>.docker.socket`.
|
|
|
|
## The Problem
|
|
|
|
vm4006 was updated from 1.2.0 to 1.18.1 and every test using Testcontainers failed at once:
|
|
|
|
```
|
|
Could not find a valid Docker environment. Attempted configurations were:
|
|
UnixSocketClientProviderStrategy: failed with exception InvalidConfigurationException
|
|
(Could not find unix domain socket). Root cause NoSuchFileException (/var/run/docker.sock)
|
|
```
|
|
|
|
The host's configuration said `builds.default.docker.socket: true`, and `werkator config:print` confirmed it.
|
|
The socket was nevertheless not mounted into the build container.
|
|
|
|
`DockerOverrides` — the shape a build definition carries its docker settings in — had no `socket` field.
|
|
PR#55 added the key to `DockerConfig`, to the strip list and to the enforcement list, and stopped there.
|
|
So the key was parsed, pinned and enforced correctly, and then `BuildDefinition.applyTo` built the effective settings field by field and dropped it, leaving the data-class default `false`.
|
|
|
|
The key was therefore unreachable from the only section a current configuration uses.
|
|
Since ADR 0007 the sandbox policy lives under `builds.<name>.docker`; only the deprecated `branches` section could still set it — which is exactly where the existing test for the socket's pinning put it, which is why the gap survived a security PR with tests of its own.
|
|
|
|
This is the failure mode this repository names in `AGENTS.md`: a key that is silently ignored rather than refused means a build that quietly stops working.
|
|
Here it meant a CI host that could no longer run a single integration test, with a configuration that read correctly on the page and in `config:print`.
|
|
|
|
## Non-Goals
|
|
|
|
- No change to what is pinned or how: the pinning was never the defect.
|
|
A branch still cannot hand itself the socket.
|
|
- No new configuration key, and no change to the default (`false`).
|
|
|
|
## The Scenarios
|
|
|
|
### Feature: a build definition can say what its build needs
|
|
|
|
#### Background
|
|
|
|
- A *build definition* (`builds.<name>`) carries its settings as nullable overrides; `applyTo` lays them over the branch settings, and `null` inherits.
|
|
- `docker.socket` mounts the host's Docker socket into the build container. It is pinned: the host decides, a branch never.
|
|
|
|
#### Scenario#72.01: A definition's socket setting reaches the build
|
|
|
|
So that a build that talks to the Docker daemon itself can be configured the way every other setting is.
|
|
|
|
- **Given** a build definition with `docker.socket: true`
|
|
- **When** its settings are applied to the branch config
|
|
- **Then** the resulting settings have the socket enabled
|
|
|
|
##### Verified by
|
|
|
|
- [BuildDefinitionTest — a definition's docker.socket reaches the settings its build runs with](../../src/test/kotlin/de/hoennig/werkator/config/BuildDefinitionTest.kt)
|
|
|
|
#### Scenario#72.02: A definition that says nothing inherits
|
|
|
|
So that the override stays an override.
|
|
|
|
- **Given** a branch config with the socket enabled
|
|
- **and** a definition that does not mention it
|
|
- **When** the definition is applied
|
|
- **Then** the socket stays enabled
|
|
|
|
##### Verified by
|
|
|
|
- [BuildDefinitionTest — a definition that says nothing about the socket keeps the branch's setting](../../src/test/kotlin/de/hoennig/werkator/config/BuildDefinitionTest.kt)
|
|
|
|
#### Scenario#72.03: Every sandbox setting can be said in a definition
|
|
|
|
So that the next key added to a sandbox cannot be forgotten in the overrides the same way.
|
|
|
|
- **Given** the `DockerConfig`/`DockerOverrides` and `WerkdockConfig`/`WerkdockOverrides` class pairs
|
|
- **When** their properties are compared
|
|
- **Then** no setting exists that a build definition cannot express
|
|
|
|
##### Verified by
|
|
|
|
- [BuildDefinitionTest — every docker and werkdock setting can be said in a build definition](../../src/test/kotlin/de/hoennig/werkator/config/BuildDefinitionTest.kt)
|
|
|
|
#### Scenario#72.04: The host's socket survives a branch layer
|
|
|
|
So that the shape vm4006 actually runs is covered end to end, not only the class pair.
|
|
|
|
- **Given** a host that configures the sandbox under `builds.default`, socket included
|
|
- **and** a branch whose committed configuration says nothing about docker
|
|
- **When** the branch's build settings are resolved
|
|
- **Then** the socket is still enabled
|
|
|
|
##### Verified by
|
|
|
|
- [ConfigLoaderTest — a build definition keeps the host's docker.socket across a branch layer](../../src/test/kotlin/de/hoennig/werkator/config/ConfigLoaderTest.kt)
|
|
|
|
## The Solution
|
|
|
|
`DockerOverrides` gets a `socket: Boolean?`, and `applyTo` carries it over like every other docker setting.
|
|
Three lines, and the bug is gone.
|
|
|
|
The fourth change is the one worth having: `Scenario#72.03` compares the properties of `DockerConfig` against `DockerOverrides` and `WerkdockConfig` against `WerkdockOverrides`.
|
|
Two data classes that have to be kept in step by hand will drift, and this one drifted in a security PR without anything noticing for four days.
|
|
The test does not check a behaviour; it checks that the pair is complete, which is the property that was actually violated.
|
|
|
|
It is worth saying what this test cannot do: a key added to `DockerConfig` and forgotten in the overrides is caught, because nothing else in the codebase would name it.
|
|
The defect found here would in fact have been caught twice over, since the scenarios above also fail to compile without the field.
|
|
|
|
## Additional Changes
|
|
|
|
- None.
|
|
|
|
## Prerequisite PRs
|
|
|
|
- None.
|
|
|
|
## Follow-up PRs
|
|
|
|
- Artifacts are served with Spring's reflected-file-download default `Content-Disposition: inline;filename=f.txt`, so a downloaded `hsadmin-ng-0.0.1-SNAPSHOT.jar` would be saved as `f.txt`. Found while verifying vm4006.
|
|
- The systemd unit `init --systemd` generates declares no `SuccessExitStatus=143`, so every clean stop is journalled as `Failed with result 'exit-code'`.
|