|
|
|
@@ -42,8 +42,8 @@ Each item is a TODO with the background that justifies it.
|
|
|
|
|
|
|
|
|
|
#### TODO 1 — Restrict the Gitea-token config file to the owner at creation
|
|
|
|
|
|
|
|
|
|
- [x] In [`InitCommand.kt:86-106`](../../src/main/kotlin/de/hoennig/gittally/commands/InitCommand.kt), create `.git/gittally/.gittally.yml` and its parent `.git/gittally/` with `0600`/`0700`, atomically at creation (as [`GitAskPass`](../../src/main/kotlin/de/hoennig/gittally/git/GitAskPass.kt) does), not via plain `writeText` at the umask default.
|
|
|
|
|
**Done:** both `init` paths now go through [`SecretFiles`](../../src/main/kotlin/de/hoennig/gittally/SecretFiles.kt), which sets the mode as a file attribute at creation.
|
|
|
|
|
- [x] In [`InitCommand.kt:86-106`](../../src/main/kotlin/de/hoennig/werkator/commands/InitCommand.kt), create `.git/gittally/.gittally.yml` and its parent `.git/gittally/` with `0600`/`0700`, atomically at creation (as [`GitAskPass`](../../src/main/kotlin/de/hoennig/werkator/git/GitAskPass.kt) does), not via plain `writeText` at the umask default.
|
|
|
|
|
**Done:** both `init` paths now go through [`SecretFiles`](../../src/main/kotlin/de/hoennig/werkator/SecretFiles.kt), which sets the mode as a file attribute at creation.
|
|
|
|
|
|
|
|
|
|
**Background.**
|
|
|
|
|
`init` creates the file it labels "secrets" — where the operator pastes the Gitea API token — with `writeText` and no permission restriction, so it inherits the umask (typically `0644`, world-readable).
|
|
|
|
@@ -52,7 +52,7 @@ On a shared host (Hostsharing is a multi-tenant deployment target, per ADR 0005)
|
|
|
|
|
|
|
|
|
|
#### TODO 2 — Stop handing the control token to every unauthenticated reader
|
|
|
|
|
|
|
|
|
|
- [x] Reconsider the token distribution in [`UiController.kt:186`](../../src/main/kotlin/de/hoennig/gittally/server/UiController.kt) and [`fragments.html:9`](../../src/main/resources/templates/fragments.html): do not embed the live token in public HTML, or gate the pages behind the same check as the mutations.
|
|
|
|
|
- [x] Reconsider the token distribution in [`UiController.kt:186`](../../src/main/kotlin/de/hoennig/werkator/server/UiController.kt) and [`fragments.html:9`](../../src/main/resources/templates/fragments.html): do not embed the live token in public HTML, or gate the pages behind the same check as the mutations.
|
|
|
|
|
**Done in v0.9.10, without gating the pages.** Public read access is a requirement, not an oversight: build states, logs and artifacts must stay linkable without a login.
|
|
|
|
|
So the `<meta>` tag is gone and `gittally.js` keeps the token in `localStorage`, asking for it once per browser (the operator reads it from `.git/gittally/control-token`, which needs shell access to the host).
|
|
|
|
|
The token is a real secret again, and as a request header it stays inherently CSRF-safe — a foreign origin cannot set it without a CORS grant.
|
|
|
|
@@ -61,7 +61,7 @@ On a shared host (Hostsharing is a multi-tenant deployment target, per ADR 0005)
|
|
|
|
|
Superseded: read access no longer implies write access. `docs/deployment.md` gained a "Control Token" section describing the split.
|
|
|
|
|
|
|
|
|
|
**Background.**
|
|
|
|
|
Every server-rendered page embeds the live control token in `<meta name="gittally-control-token">` so [`gittally.js`](../../src/main/resources/static/gittally.js) can read it, but no GET is authenticated.
|
|
|
|
|
Every server-rendered page embeds the live control token in `<meta name="gittally-control-token">` so [`gittally.js`](../../src/main/resources/static/werkator.js) can read it, but no GET is authenticated.
|
|
|
|
|
So `curl -s http://host:18080/ | grep gittally-control-token` yields the token, which unlocks restart/cancel/delete.
|
|
|
|
|
Read access therefore equals write access, and the token provides no real second trust tier.
|
|
|
|
|
Blast radius is limited to build-lifecycle operations (a DoS/integrity concern, not secret disclosure or RCE), which is why this is a design flaw rather than Critical — but the token gives a false sense of protection.
|
|
|
|
@@ -70,7 +70,7 @@ Blast radius is limited to build-lifecycle operations (a DoS/integrity concern,
|
|
|
|
|
|
|
|
|
|
#### TODO 3 — Accept the control token via header only
|
|
|
|
|
|
|
|
|
|
- [x] Remove the `token` query-parameter variant from the three mutating endpoints in [`BuildsApiController.kt:90,115,129`](../../src/main/kotlin/de/hoennig/gittally/server/BuildsApiController.kt); keep only the `X-GitTally-Token` header (which the bundled UI already uses).
|
|
|
|
|
- [x] Remove the `token` query-parameter variant from the three mutating endpoints in [`BuildsApiController.kt:90,115,129`](../../src/main/kotlin/de/hoennig/werkator/server/BuildsApiController.kt); keep only the `X-GitTally-Token` header (which the bundled UI already uses).
|
|
|
|
|
|
|
|
|
|
**Background.**
|
|
|
|
|
Tokens in URLs are routinely written to access logs, reverse-proxy logs, browser history, and the `Referer` header on outbound navigation.
|
|
|
|
@@ -79,7 +79,7 @@ The query-param path exists only for legacy convenience.
|
|
|
|
|
|
|
|
|
|
#### TODO 4 — Redact the Gitea token in `config:print`
|
|
|
|
|
|
|
|
|
|
- [x] Mask `git.token` (and any future secret) by default in [`ConfigPrintCommand.kt:20-31`](../../src/main/kotlin/de/hoennig/gittally/commands/ConfigPrintCommand.kt); gate the plaintext value behind an explicit `--show-secrets` flag.
|
|
|
|
|
- [x] Mask `git.token` (and any future secret) by default in [`ConfigPrintCommand.kt:20-31`](../../src/main/kotlin/de/hoennig/werkator/commands/ConfigPrintCommand.kt); gate the plaintext value behind an explicit `--show-secrets` flag.
|
|
|
|
|
Masked as `***` on both the `--full` and the raw path, with a leading YAML comment naming the flag, so the output stays parseable when piped.
|
|
|
|
|
- [x] Update `tools/setup-gittally-instance:272`, which currently steers the operator to run `config:print --full` to view the token.
|
|
|
|
|
|
|
|
|
@@ -89,7 +89,7 @@ There is no redaction and no masked default.
|
|
|
|
|
|
|
|
|
|
#### TODO 5 — Reduce information disclosure on the unauthenticated read API
|
|
|
|
|
|
|
|
|
|
- [x] Decide whether build-log streaming ([`BuildsApiController`](../../src/main/kotlin/de/hoennig/gittally/server/BuildsApiController.kt) `/api/builds/current/{key}/log`), [`SystemApiController`](../../src/main/kotlin/de/hoennig/gittally/server/SystemApiController.kt), and [`WatcherApiController`](../../src/main/kotlin/de/hoennig/gittally/server/WatcherApiController.kt) should stay fully public, or be gated / scrubbed.
|
|
|
|
|
- [x] Decide whether build-log streaming ([`BuildsApiController`](../../src/main/kotlin/de/hoennig/werkator/server/BuildsApiController.kt) `/api/builds/current/{key}/log`), [`SystemApiController`](../../src/main/kotlin/de/hoennig/werkator/server/SystemApiController.kt), and [`WatcherApiController`](../../src/main/kotlin/de/hoennig/werkator/server/WatcherApiController.kt) should stay fully public, or be gated / scrubbed.
|
|
|
|
|
**Decided: they stay fully public, no scrubbing.** The watched projects (GitTally itself and hs.hsadmin.ng) are open source, the repositories hold no secrets, and the builds run tests against test data — credentials appearing in a log are fixtures, not real ones. Builds neither deploy nor sign; the only planned artifact is a jar.
|
|
|
|
|
Public logs are also the point of the tool: a red build must be diagnosable from the link in the Gitea status without a login.
|
|
|
|
|
This is a property of the watched project, not of GitTally: an installation whose builds touch real credentials must keep its instance off the public internet (reverse proxy or `bindAddress: 127.0.0.1`), because GitTally offers no per-endpoint gating.
|
|
|
|
@@ -104,14 +104,14 @@ Raw build output may contain secrets echoed by build scripts; `/api/system` expo
|
|
|
|
|
|
|
|
|
|
A branch is built with its own build settings: `.gittally.yml` from the **build worktree** (the commit being built) overrides the `.git`/primary config — except for a pinned set that a branch must never control. **Implemented in this PR.**
|
|
|
|
|
|
|
|
|
|
- [x] Worktree config layer for builds via [`ConfigLoader.loadForWorktree`](../../src/main/kotlin/de/hoennig/gittally/config/ConfigLoader.kt), wired into the two build-time config consumers ([`BuildExecutor.branchConfig`](../../src/main/kotlin/de/hoennig/gittally/build/BuildExecutor.kt), [`FileArtifactStore.branchConfig`](../../src/main/kotlin/de/hoennig/gittally/artifacts/FileArtifactStore.kt)) — both already hold the prepared worktree path. Precedence is worktree > `.git` > project.
|
|
|
|
|
- [x] Worktree config layer for builds via [`ConfigLoader.loadForWorktree`](../../src/main/kotlin/de/hoennig/werkator/config/ConfigLoader.kt), wired into the two build-time config consumers ([`BuildExecutor.branchConfig`](../../src/main/kotlin/de/hoennig/werkator/build/BuildExecutor.kt), [`FileArtifactStore.branchConfig`](../../src/main/kotlin/de/hoennig/werkator/artifacts/FileArtifactStore.kt)) — both already hold the prepared worktree path. Precedence is worktree > `.git` > project.
|
|
|
|
|
- [x] **Pinned to `.git`/primary — stripped from the worktree layer:** `git`, `gitea`, `server` (secrets + server-side), and the sandbox policy `docker.enabled`/`docker.network`. A branch cannot disable its container or change its network mode.
|
|
|
|
|
- [x] Worktree-overridable: `buildCommand`, `cleanCommand`, `artifactDirs`, `stdoutLog`/`stderrLog`, `autoBuild`, and `docker.image`/`dockerfile`/`context`/`env`.
|
|
|
|
|
- [x] Pinned set enforced in code (`ConfigLoader.stripPinned`), documented in `docs/configuration.md`, and asserted as an invariant in `AGENTS.md`.
|
|
|
|
|
- [ ] **Deferred:** `autoBuild` scheduling and `requirePullRequest` are still read from the primary config, not the worktree — the watcher evaluates them *before* a build (and thus a worktree) exists. Sourcing them per-branch would need the watcher to read the branch's committed config directly (e.g. via `git show <branch>:.gittally.yml`); out of scope here.
|
|
|
|
|
|
|
|
|
|
**Background.**
|
|
|
|
|
The build command is executed via `bash -c "$3"` inside the build container ([`DockerBuildRunner.kt:285`](../../src/main/kotlin/de/hoennig/gittally/build/DockerBuildRunner.kt)).
|
|
|
|
|
The build command is executed via `bash -c "$3"` inside the build container ([`DockerBuildRunner.kt:285`](../../src/main/kotlin/de/hoennig/werkator/build/DockerBuildRunner.kt)).
|
|
|
|
|
Letting a branch define its own `buildCommand` is not a new risk — a CI already runs arbitrary code from that commit; the container is the sandbox.
|
|
|
|
|
The real escalation is a branch turning the sandbox **off**: if the worktree could set `docker.enabled: false` (or host `docker.network`), the build would run natively on the host — which is why those two keys are pinned.
|
|
|
|
|
Secrets are also safe from the build process (the Gitea token is used only by the server/watcher and is never placed in the build environment — `runCommand` passes only `mapOf("branch" to ...)`), and the whole `git`/`gitea`/`server` sections are stripped from the worktree layer as defense in depth.
|
|
|
|
@@ -121,7 +121,7 @@ Before this PR all build config was loaded from the primary checkout via `config
|
|
|
|
|
|
|
|
|
|
#### TODO 7 — Default `bindAddress` to `127.0.0.1`
|
|
|
|
|
|
|
|
|
|
- [x] Change the default in [`GitTallyConfig.kt:21`](../../src/main/kotlin/de/hoennig/gittally/config/GitTallyConfig.kt) and the `init` template ([`InitCommand.kt:126`](../../src/main/kotlin/de/hoennig/gittally/commands/InitCommand.kt)) from `0.0.0.0` to `127.0.0.1`; require operators to opt into all-interfaces.
|
|
|
|
|
- [x] Change the default in [`GitTallyConfig.kt:21`](../../src/main/kotlin/de/hoennig/werkator/config/WerkatorConfig.kt) and the `init` template ([`InitCommand.kt:126`](../../src/main/kotlin/de/hoennig/werkator/commands/InitCommand.kt)) from `0.0.0.0` to `127.0.0.1`; require operators to opt into all-interfaces.
|
|
|
|
|
Shipped as v0.9.9 with the migration note in the release notes, `docs/configuration.md` ("Notes on `server.bindAddress`") and `docs/deployment.md`: existing configs keep their explicit value, and the managed nginx now needs `0.0.0.0` set deliberately.
|
|
|
|
|
|
|
|
|
|
**Background.**
|
|
|
|
@@ -130,7 +130,7 @@ The deployment doc already recommends `127.0.0.1`; the default and template shou
|
|
|
|
|
|
|
|
|
|
#### TODO 8 — Compare fixed-length hashes in the token check
|
|
|
|
|
|
|
|
|
|
- [x] In [`ControlTokenService.kt:35-37`](../../src/main/kotlin/de/hoennig/gittally/server/ControlTokenService.kt), compare `SHA-256(submitted)` against `SHA-256(secret)` with `MessageDigest.isEqual`, so the comparison is always over equal-length buffers.
|
|
|
|
|
- [x] In [`ControlTokenService.kt:35-37`](../../src/main/kotlin/de/hoennig/werkator/server/ControlTokenService.kt), compare `SHA-256(submitted)` against `SHA-256(secret)` with `MessageDigest.isEqual`, so the comparison is always over equal-length buffers.
|
|
|
|
|
|
|
|
|
|
**Background.**
|
|
|
|
|
`MessageDigest.isEqual` returns early on a length mismatch, leaking the token length via timing.
|
|
|
|
@@ -138,7 +138,7 @@ Largely theoretical given the 192-bit CSPRNG token, but a cheap deviation from c
|
|
|
|
|
|
|
|
|
|
#### TODO 9 — Add `--` before positional refnames in git calls
|
|
|
|
|
|
|
|
|
|
- [x] Insert `--` before the branch argument in `checkout`, `fetchBranch`, and `resetHardToOrigin` in [`GitService.kt:145,40,155`](../../src/main/kotlin/de/hoennig/gittally/git/GitService.kt) (e.g. `git switch -- <branch>`).
|
|
|
|
|
- [x] Insert `--` before the branch argument in `checkout`, `fetchBranch`, and `resetHardToOrigin` in [`GitService.kt:145,40,155`](../../src/main/kotlin/de/hoennig/werkator/git/GitService.kt) (e.g. `git switch -- <branch>`).
|
|
|
|
|
**Done for `checkout` and `fetchBranch`.** `resetHardToOrigin` keeps its plain form: `git reset --hard -- <commit>` is rejected (`fatal: Cannot do hard reset with paths`), and its argument is already prefixed with `origin/`, so it can never start with `-`.
|
|
|
|
|
|
|
|
|
|
**Background.**
|
|
|
|
@@ -147,8 +147,8 @@ These three methods currently have no production callers and the actively-used p
|
|
|
|
|
|
|
|
|
|
#### TODO 10 — Create secret files restricted atomically
|
|
|
|
|
|
|
|
|
|
- [x] Set the mode at creation for the control-token file ([`ControlTokenService.kt:29-30`](../../src/main/kotlin/de/hoennig/gittally/server/ControlTokenService.kt)) and the setup-script YAML (`tools/setup-gittally-instance:253`), instead of `chmod 0600` after the write.
|
|
|
|
|
**Done:** the control-token file via [`SecretFiles`](../../src/main/kotlin/de/hoennig/gittally/SecretFiles.kt), the setup script by writing the YAML in a `umask 077` subshell instead of `chmod`-ing afterwards.
|
|
|
|
|
- [x] Set the mode at creation for the control-token file ([`ControlTokenService.kt:29-30`](../../src/main/kotlin/de/hoennig/werkator/server/ControlTokenService.kt)) and the setup-script YAML (`tools/setup-gittally-instance:253`), instead of `chmod 0600` after the write.
|
|
|
|
|
**Done:** the control-token file via [`SecretFiles`](../../src/main/kotlin/de/hoennig/werkator/SecretFiles.kt), the setup script by writing the YAML in a `umask 077` subshell instead of `chmod`-ing afterwards.
|
|
|
|
|
|
|
|
|
|
**Background.**
|
|
|
|
|
Both currently write the file at the umask default and tighten it afterward, leaving a brief window where the secret exists world-readable.
|
|
|
|
@@ -175,11 +175,11 @@ A small TOCTOU gap; `GitAskPass`'s atomic-at-creation approach is the pattern to
|
|
|
|
|
|
|
|
|
|
Recorded so future changes do not silently regress these.
|
|
|
|
|
|
|
|
|
|
- **Path traversal in artifact serving** — [`ArtifactFileController`](../../src/main/kotlin/de/hoennig/gittally/server/ArtifactFileController.kt) normalizes then enforces `startsWith(artifactDir)` and `isRegularFile(..., NOFOLLOW_LINKS)` (rejects symlink escape); [`FileArtifactStore`](../../src/main/kotlin/de/hoennig/gittally/artifacts/FileArtifactStore.kt) whitelists the key to `[A-Za-z0-9._-]+` and requires `dir.parent == branchesDir`.
|
|
|
|
|
- **Command injection** — every git/docker/certbot/nginx call uses `ProcessBuilder(List<String>)` ([`GitCommandRunner.kt`](../../src/main/kotlin/de/hoennig/gittally/git/GitCommandRunner.kt)); no `Runtime.exec(String)`, no shell-string interpolation; the only `sh -c`/`bash -c` uses pass data as positional args.
|
|
|
|
|
- **Branch-name → filesystem** — always routed through [`ArtifactKeys.sanitize`](../../src/main/kotlin/de/hoennig/gittally/build/ArtifactKeys.kt) (`/` → `_`) plus a SHA suffix, so worktree/container/volume names cannot traverse.
|
|
|
|
|
- **Path traversal in artifact serving** — [`ArtifactFileController`](../../src/main/kotlin/de/hoennig/werkator/server/ArtifactFileController.kt) normalizes then enforces `startsWith(artifactDir)` and `isRegularFile(..., NOFOLLOW_LINKS)` (rejects symlink escape); [`FileArtifactStore`](../../src/main/kotlin/de/hoennig/werkator/artifacts/FileArtifactStore.kt) whitelists the key to `[A-Za-z0-9._-]+` and requires `dir.parent == branchesDir`.
|
|
|
|
|
- **Command injection** — every git/docker/certbot/nginx call uses `ProcessBuilder(List<String>)` ([`GitCommandRunner.kt`](../../src/main/kotlin/de/hoennig/werkator/git/GitCommandRunner.kt)); no `Runtime.exec(String)`, no shell-string interpolation; the only `sh -c`/`bash -c` uses pass data as positional args.
|
|
|
|
|
- **Branch-name → filesystem** — always routed through [`ArtifactKeys.sanitize`](../../src/main/kotlin/de/hoennig/werkator/build/ArtifactKeys.kt) (`/` → `_`) plus a SHA suffix, so worktree/container/volume names cannot traverse.
|
|
|
|
|
- **XSS** — no `th:utext` in any template; `gittally.js` builds all DOM via `createElement` + `textContent`/`dataset`, no `innerHTML`.
|
|
|
|
|
- **SSRF** — the Gitea status proxy validates the commit against `[0-9a-fA-F]{7,40}` ([`StatusApiController.kt`](../../src/main/kotlin/de/hoennig/gittally/server/StatusApiController.kt)); the Gitea base URL/owner/repo/token come from config, not the request.
|
|
|
|
|
- **SSRF** — the Gitea status proxy validates the commit against `[0-9a-fA-F]{7,40}` ([`StatusApiController.kt`](../../src/main/kotlin/de/hoennig/werkator/server/StatusApiController.kt)); the Gitea base URL/owner/repo/token come from config, not the request.
|
|
|
|
|
- **Deserialization** — Jackson JSON/YAML without polymorphic/default typing (no gadget-chain RCE); unreadable state files degrade to empty.
|
|
|
|
|
- **Credential transport** — Gitea token sent as an `Authorization` header, never in a URL ([`GiteaClient.kt`](../../src/main/kotlin/de/hoennig/gittally/gitea/GiteaClient.kt)); git auth via env-based `GIT_ASKPASS` with a `0700` script containing no secrets, deleted in `finally`; TLS verification never disabled.
|
|
|
|
|
- **nginx/certbot** — `serverName`/`upstreamHost` validated against `[A-Za-z0-9][A-Za-z0-9.-]*` before substitution ([`NginxProxyManager.kt`](../../src/main/kotlin/de/hoennig/gittally/server/NginxProxyManager.kt)); ports range-checked.
|
|
|
|
|
- **Credential transport** — Gitea token sent as an `Authorization` header, never in a URL ([`GiteaClient.kt`](../../src/main/kotlin/de/hoennig/werkator/gitea/GiteaClient.kt)); git auth via env-based `GIT_ASKPASS` with a `0700` script containing no secrets, deleted in `finally`; TLS verification never disabled.
|
|
|
|
|
- **nginx/certbot** — `serverName`/`upstreamHost` validated against `[A-Za-z0-9][A-Za-z0-9.-]*` before substitution ([`NginxProxyManager.kt`](../../src/main/kotlin/de/hoennig/werkator/server/NginxProxyManager.kt)); ports range-checked.
|
|
|
|
|
mhoennig