Skip to content

fix(release): run the release image as nonroot - #66

Merged
andybons merged 3 commits into
mainfrom
fix/nonroot-release-image
Sep 3, 2026
Merged

fix(release): run the release image as nonroot#66
andybons merged 3 commits into
mainfrom
fix/nonroot-release-image

Conversation

@andybons

@andybons andybons commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

The final stage used gcr.io/distroless/static-debian12, which defaults to root. Gatekeeper needs no root privilege — it binds a high port, reads config and key material from mounts, and writes nothing to the image filesystem.

The cost shows up under orchestrators that enforce runAsNonRoot: the pod policy rejects the image at container start (container has runAsNonRoot and image will run as root), and every consumer has to pin an explicit runAsUser in their deployment — which the boxes chart just had to do (meetneptune/boxes#637). An image-level default is the right home for an invariant every consumer wants.

The :nonroot variant declares UID/GID 65532 in the image. Deployments already pinning 65532 keep working unchanged, since that is the identity the image now declares.

Changelog entry for v0.22.1 rides in this PR, so the release tag lands on a tree that carries it.

The final stage used gcr.io/distroless/static-debian12, which defaults
to root. Gatekeeper needs no root privilege -- it binds a high port,
reads config and keys from mounts, and writes nothing to the image
filesystem -- and an orchestrator enforcing runAsNonRoot rejects the
image at container start unless every deployment pins an explicit
runAsUser. The :nonroot variant declares UID/GID 65532 in the image,
where the invariant belongs.
@claude

claude Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor
📦 Previous Review (superseded by newer review)

Claude finished @andybons's task in 1m 10s —— View job


Review complete

  • Gather context (PR diff, changed files)
  • Understand request (code review via /code-review skill)
  • Review cmd/gatekeeper/Dockerfile change
  • Review CHANGELOG.md change
  • Post final review summary

Summary

The change itself — switching the final stage from gcr.io/distroless/static-debian12 to gcr.io/distroless/static-debian12:nonroot — is correct and minimal. Gatekeeper genuinely needs no root: it binds 9080 (non-privileged), and per docs/content/guides/12-docker-deployment.md, config/CA material is only ever read from mounts, nothing is written to the image filesystem. The changelog entry is accurate and well-scoped to v0.22.1.

Finding: this PR breaks the repo's own documented CA setup

examples/gen-ca.sh:48 generates the interception CA key with:

chmod 0600 ca.key

That key is owned by whatever host user ran the script. The docker-deployment guide's own Compose example (docs/content/guides/12-docker-deployment.md:176-179) then bind-mounts it straight into the container:

volumes:
  - ./ca.key:/etc/gatekeeper/ca.key:ro

Docker bind mounts preserve host UID/ownership and permission bits — they aren't remapped to the container user. Before this PR the container ran as root, so it could always read the 0600 file regardless of owner. After this PR the container runs as UID/GID 65532 (distroless nonroot), which is very unlikely to match the host UID that ran gen-ca.sh, and 0600 has no "other" read bit. Result: a fresh deployment that follows this repo's own documented steps (gen-ca.sh → docker-deployment guide) will fail to start with a permission-denied error reading tls.ca_key, and the changelog's claim that "deployments that already pin UID 65532 keep working unchanged" doesn't cover the far more common case of a mounted key owned by an arbitrary host user.

This doesn't need to block a minimal Dockerfile fix, but it should be tracked — either:

  • relax examples/gen-ca.sh's chmod to 0644 (the CA key for local dev/example use, where the accompanying docs already say "for TLS interception," not a production secret), or
  • add a note to the docker-deployment guide that the CA key/cert must be readable by UID 65532 (e.g. chmod 0644 or chown 65532 before mounting).

Given this PR's stated scope is just the Dockerfile fix + changelog, I'd suggest filing this as a fast follow-up rather than expanding this PR — happy to open that PR if wanted.

Fix this →

No other issues found — no security, race-condition, or logic problems in the two changed files.

The :nonroot base cannot read the 0600 ca.key gen-ca.sh generates under
the invoking host user, because a bind mount preserves host ownership.
Running the container as that user keeps the key private and the
container non-root; loosening the key's mode would leave the
interception CA readable to every local user.

Reported by the automated review on the pull request.

andybons commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Good catch — the documented flow really would have failed at startup, and the changelog claim did not cover it. Taken in this PR rather than as a follow-up, in 1145057, but with the other remedy than the two suggested.

Relaxing gen-ca.sh to 0644 keeps the flow working by making the interception CA key readable to every local user, which is the wrong lesson for an example people copy. Instead the guide's two docker run examples now pass --user "$(id -u):$(id -g)" and the Compose example sets user: — the container runs as the key's owner, the key stays 0600, and the container stays non-root. One explanatory note in the guide, and the v0.22.1 changelog entry now names this exact flow as the one thing the base-image change affects, replacing the "keep working unchanged" claim that overclaimed.

@claude

claude Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor
📦 Previous Review (superseded by newer review)

Claude finished @andybons's task in 1m 40s —— View job


Reviewing PR #66

  • Gather context (PR diff, changed files)
  • Review Dockerfile change (nonroot base image)
  • Review docs change
  • Review CHANGELOG entry
  • Post final review

Summary

This is a small, well-scoped fix: switching the release image's final stage to gcr.io/distroless/static-debian12:nonroot is the correct way to satisfy runAsNonRoot policies at the image level, and the accompanying docker run/Compose --user/user: additions correctly address the resulting ca.key (0600, host-owned) readability gap rather than papering over it by loosening the key's file mode. Verified that gatekeeper's runtime code path never writes to the image filesystem (proxy.NewCA's os.WriteFile/os.MkdirAll calls are unused by cmd/gatekeeper — only reachable from library/test callers), so the PR body's "writes nothing to the image filesystem" claim holds for the actual running binary. No security, race, or logic issues found in the Dockerfile or CHANGELOG changes.

One small doc inaccuracy, left as an inline comment: the Compose example's see the note below (docs/content/guides/12-docker-deployment.md:189) points at the explanatory note in the "Running the container" section, which is above the Compose example, not below it.

Comment thread docs/content/guides/12-docker-deployment.md Outdated
Reported by the automated review on the pull request.
@claude

claude Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Claude finished @andybons's task in 1m 4s —— View job


Review complete

  • Gather context (PR diff, changed files)
  • Review cmd/gatekeeper/Dockerfile change
  • Review CHANGELOG.md change
  • Review docs/content/guides/12-docker-deployment.md change
  • Post final review summary

Summary

Re-reviewed the current state of the branch (3 commits: the Dockerfile fix, the ca.key-ownership follow-up, and the "note above" wording fix). The nit from the previous review round — "see the note below" pointing the wrong direction — is fixed in 750eb14; it now correctly reads "see the note above."

The overall approach in 1145057 is sound and is the better of the two remedies the earlier review suggested: keeping ca.key at 0600 and running the container as its owner (--user "$(id -u):$(id -g)" / Compose user:) avoids loosening the interception CA key's permissions for every local user, while still fixing the documented flow that this PR's base-image change broke. The changelog entry for v0.22.1 accurately reflects this — it no longer claims deployments "keep working unchanged" and instead names the one flow that changes.

One minor note (non-blocking)

docs/content/guides/12-docker-deployment.md:190 hardcodes the Compose user: value:

    # Your host uid:gid, so the container can read the 0600 ca.key it
    # bind-mounts -- see the note above.
    user: "1000:1000"

Unlike the two docker run examples, which compute the real value with --user "$(id -u):$(id -g)", this is a literal 1000:1000 — correct only for hosts where the invoking user's UID/GID happens to be 1000 (common but not universal, e.g. non-first Linux users, or ID-mapped/rootless setups). The comment says "your host uid:gid" but a reader who copies the block verbatim without running id -u/id -g first will get a container that can't read ca.key whenever their UID isn't 1000. Since Compose doesn't support inline shell command substitution the way docker run does, an alternative would be user: "${UID:-1000}:${GID:-1000}" with a one-line instruction to export UID GID before docker compose up (bash's $UID/$GID aren't exported by default) — but that trades one gotcha for another, so a comment is probably fine either way. Flagging only because it's easy to miss when copy-pasting.

No security, race-condition, or logic issues in the three changed files — the base-image switch is correct and minimal, and the permission fix correctly targets the actual failure mode (host-owned bind mount vs. UID 65532) rather than the previous review's suggested alternative of relaxing the key's mode.

@andybons
andybons merged commit 9f8445f into main Sep 3, 2026
2 checks passed
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