diff --git a/.github/workflows/image-build-push.yml b/.github/workflows/image-build-push.yml index 000f1e3b727..a8cb5f358e1 100644 --- a/.github/workflows/image-build-push.yml +++ b/.github/workflows/image-build-push.yml @@ -6,8 +6,13 @@ defaults: # In a forked PR, this workflow may run a fork's own Dockerfile/build scripts, # so the `build` job never references a secret - the trusted side (loading # the artifact `build` produces and actually pushing it) lives in -# image-push.yml, triggered via workflow_run once this workflow completes, -# gated behind its own approval. +# image-push.yml, triggered via workflow_run once this workflow completes. +# +# The approval gate for a fork PR lives in `approve`, right after `detect` +# knows a build is needed, so a maintainer sees the request before the fork +# build (often several minutes) even starts. It's skipped when the PR author +# already has push access to this repo (`detect`'s `is_authorized` output), +# since there's nothing to approve for a maintainer's own fork. # # For anything else (a push, or a same-repo PR - `detect`'s `is_fork` output, # using the same fork-check idiom as push-tagged-image.yml), there's nothing @@ -46,6 +51,7 @@ jobs: manifest_matrix: ${{ steps.detect.outputs.manifest_matrix }} needs_build: ${{ steps.detect.outputs.needs_build }} is_fork: ${{ steps.fork.outputs.is_fork }} + is_authorized: ${{ steps.authorized.outputs.is_authorized }} steps: - uses: actions/checkout@v7 - name: Compute per-image build status @@ -118,11 +124,36 @@ jobs: run: | set -eu -o pipefail if [ "$EVENT_NAME" = "pull_request" ] && [ "$HEAD_OWNER" != "$BASE_OWNER" ]; then + echo "fork: head repo owner '$HEAD_OWNER' != base repo owner '$BASE_OWNER'; treating as a fork PR" echo "is_fork=true" >> "$GITHUB_OUTPUT" else + echo "fork: not a fork PR (head owner '${HEAD_OWNER:-n/a}', base owner '$BASE_OWNER')" echo "is_fork=false" >> "$GITHUB_OUTPUT" fi + # A maintainer's own fork PR needs no human approval to build/push - + # they already have that access directly. author_association reflects + # the PR author's relationship to this repo, not the fork. + - name: Determine whether the PR author is already authorized to push + id: authorized + env: + AUTHOR_ASSOCIATION: ${{ github.event.pull_request.author_association }} + run: | + set -eu -o pipefail + case "$AUTHOR_ASSOCIATION" in + OWNER | MEMBER | COLLABORATOR) + echo "authorized: author_association is '$AUTHOR_ASSOCIATION'; skipping the approval gate" + echo "is_authorized=true" >> "$GITHUB_OUTPUT" + ;; + *) + # GitHub computes this without visibility into an org membership + # that isn't publicized, so a real member/owner with private + # membership on this repo's org still lands here as NONE. + echo "authorized: author_association is '$AUTHOR_ASSOCIATION'; requiring approval" >&2 + echo "is_authorized=false" >> "$GITHUB_OUTPUT" + ;; + esac + # create-manifests never runs for a fork, so the release names would # silently be left off. Fails ahead of the build and the approval. - name: Refuse a release marker on a fork @@ -139,15 +170,32 @@ jobs: exit 1 done + # One gate for the whole run rather than `environment:` on the build matrix + # below, where every (image, arch) would be a pending deployment of its own + # to approve. Skipped entirely when the PR author is already authorized - + # see "Determine whether the PR author is already authorized to push" above. + approve: + name: "Approve: build and push image(s) from this fork PR" + needs: detect + if: needs.detect.outputs.needs_build == 'true' && needs.detect.outputs.is_fork == 'true' && needs.detect.outputs.is_authorized == 'false' + runs-on: ubuntu-24.04 + environment: image-push + steps: + - name: Record approval + run: echo "Approved; proceeding to build and push from this fork PR" + # --- Fork PRs: build with no secrets (this job never has registry # credentials, so there's nothing to gain by gating it - see #8609 - # discussion), then hand off to image-push.yml for the trusted, - # approval-gated push. --- + # discussion), then hand off to image-push.yml for the trusted push. --- build: name: Build ${{ matrix.build.repo }} (${{ matrix.build.arch }}) - needs: detect - if: needs.detect.outputs.needs_build == 'true' && needs.detect.outputs.is_fork == 'true' + needs: [detect, approve] + # `approve` is skipped (not failed) when the author is already + # authorized, so its result is checked explicitly. `always()` is required + # for that check to run at all: without it, GitHub prefixes this + # condition with an implicit success(), which a skipped `approve` fails. + if: always() && needs.detect.outputs.needs_build == 'true' && needs.detect.outputs.is_fork == 'true' && (needs.detect.outputs.is_authorized == 'true' || needs.approve.result == 'success') strategy: fail-fast: false matrix: @@ -193,8 +241,8 @@ jobs: repos.txt tag.txt arch.txt - # image-push.yml can't download an expired artifact, and the approval - # it waits on is a human one that may not come the same day. + # image-push.yml can't download an expired artifact, so this + # outlives any delay in its workflow_run trigger firing. retention-days: 7 # --- Pushes and same-repo PRs: no fork content ever runs here, so build diff --git a/.github/workflows/image-push.yml b/.github/workflows/image-push.yml index 36f78c68f4e..853c05b29a0 100644 --- a/.github/workflows/image-push.yml +++ b/.github/workflows/image-push.yml @@ -9,6 +9,10 @@ defaults: # and this workflow never checks out or executes the triggering PR's code. # It only loads the artifact "Image build" produced (an inert tarball plus # metadata) and pushes it, after re-validating the tag. +# +# By the time an artifact exists to push, image-build-push.yml's `approve` +# job has already required maintainer sign-off (or PR-author authorization) +# before its `build` job ran. on: workflow_run: workflows: ["Image build"] @@ -21,8 +25,6 @@ permissions: contents: read jobs: - # Ungated on purpose: asking a maintainer to approve a push only to discover - # the run built nothing trains people to click Approve without looking. check-artifacts: name: Check for built images # Only fork completions of "Image build" ever produce artifacts here - @@ -55,23 +57,10 @@ jobs: core.setOutput("has_artifacts", keys.length > 0 ? "true" : "false"); core.setOutput("images", JSON.stringify(keys)); - # One gate for the whole run rather than `environment:` on the fan-out below, - # where every image would be a pending deployment of its own to approve. - approve: - name: "Approve: push the built image(s) to DockerHub" - needs: check-artifacts - if: needs.check-artifacts.outputs.has_artifacts == 'true' - runs-on: ubuntu-24.04 - environment: image-push - steps: - - name: Record what was approved - env: - IMAGES: ${{ needs.check-artifacts.outputs.images }} - run: echo "Approved; pushing $(jq -r 'join(", ")' <<< "$IMAGES")" - push: name: Push ${{ matrix.image }} - needs: [check-artifacts, approve] + needs: check-artifacts + if: needs.check-artifacts.outputs.has_artifacts == 'true' strategy: fail-fast: false matrix: diff --git a/CLAUDE.md b/CLAUDE.md index a349bb39f1a..800711e03c4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -168,6 +168,9 @@ rationale that belongs in the commit message or PR description. a single file. If two functions share a reason, put it in one place (a doc comment on the shared helper or constant) and let the other site point to it - Never re-describe in comments what a linked issue or commit message already covers +- Write for a reader who has no memory of this change. Describe what is true + now, not what changed — "no gate here" or "moved from X" reads as a diff + note and goes stale the moment the history isn't front of mind - Test doc comments: the test name plus its assertions already say what is being tested. Comment only what is not obvious from those — a non-obvious setup step, or why the test skips under some condition diff --git a/containers/ddev-traefik-router/Dockerfile b/containers/ddev-traefik-router/Dockerfile index cb8e397111c..bb5c4dd2da8 100644 --- a/containers/ddev-traefik-router/Dockerfile +++ b/containers/ddev-traefik-router/Dockerfile @@ -6,6 +6,7 @@ ENV TRAEFIK_MONITOR_PORT=10999 RUN apk add --no-cache bash curl file htop jq openssl socat vim yq WORKDIR /mnt/ddev-global-cache/traefik COPY files / +RUN ls -ld RUN chmod ugo+rx /usr/local/bin/monitor-traefik-stderr.sh /usr/local/bin/docker-entrypoint.sh /usr/local/bin/ddev-router-fallback-responder.sh /healthcheck.sh # Make Traefik commands work without --configFile by using default location # https://doc.traefik.io/traefik/getting-started/configuration-overview/#configuration-file diff --git a/docs/content/developers/building-contributing.md b/docs/content/developers/building-contributing.md index a7276e89fb0..21dc3d173d1 100644 --- a/docs/content/developers/building-contributing.md +++ b/docs/content/developers/building-contributing.md @@ -324,10 +324,11 @@ Changing `containers/ddev-dbserver` is the one case where `make` alone isn't eno What happens next depends on whether the PR is from a fork: -* **Fork PRs** (security boundary — the PR could contain an arbitrary Dockerfile/build script): a `build` job builds the image(s) per architecture with no registry credentials at all — nothing in that job can reach `docker.io`, so there's nothing to gain by gating it before it runs. Once it finishes, a separate, trusted `image-push.yml` workflow — which never checks out or runs the pull request's code — loads what it produced and pushes it, gated behind a maintainer's approval on the `image-push` environment. Both the tag and every repository name in that artifact are re-validated first (`containers/validate-image-tag.sh`, `containers/validate-image-repo.sh`), so an approval can only ever publish a hash-shaped tag under a known DDEV repository. A comment is posted on the PR once the push completes. If the build produced nothing to push, no approval is requested at all. +* **Fork PRs** (security boundary — the PR could contain an arbitrary Dockerfile/build script) **from someone without push access**: as soon as `detect` finds an image to build, an `approve` job asks a maintainer to sign off on the `image-push` environment, before anything is built. Once approved, a `build` job builds the image(s) per architecture with no registry credentials at all — nothing in that job can reach `docker.io`, so there's nothing to gain by gating it after the fact instead of before. A separate, trusted `image-push.yml` workflow — which never checks out or runs the pull request's code — then loads what `build` produced and pushes it, re-validating both the tag and every repository name first (`containers/validate-image-tag.sh`, `containers/validate-image-repo.sh`) so the earlier approval can only ever publish a hash-shaped tag under a known DDEV repository. A comment is posted on the PR once the push completes. If `detect` found nothing to build, no approval is requested at all. +* **Fork PRs from a maintainer's own fork** (`author_association` is `OWNER`, `MEMBER`, or `COLLABORATOR`): the same fork-safe `build` / `image-push.yml` split runs, but the `approve` job is skipped — they already have push access, so there's nothing to ask permission for. * **Everything else** (a push to `main`, or a pull request from a branch in the same repository — no fork content is ever involved): `build-and-push` builds and pushes directly in one step, with no approval gate at all — the same trust level `main-build.yml` already runs at unguarded. A `create-manifests` job then assembles the multi-arch manifest and comments on the PR, if there is one. -So a maintainer only ever needs to click **Approve** once — for a fork PR's push step — and only when the PR actually changed a container image; everything else is fully automatic. +So a maintainer only ever needs to click **Approve** once — as soon as a fork PR without push access changes a container image — and only that once; everything else is fully automatic. ## Pull Requests diff --git a/docs/content/developers/release-management.md b/docs/content/developers/release-management.md index 8cb7c60af84..7566f19bb8c 100644 --- a/docs/content/developers/release-management.md +++ b/docs/content/developers/release-management.md @@ -105,13 +105,13 @@ A `containers/ddev-dbserver` change builds and pushes all 20 database variants ( ### One-time setup: the `image-push` GitHub Environment -Fork PRs build with no registry credentials at all (nothing to gain by gating that step), then go through a single approval before the built image is actually pushed, gated by the `image-push` GitHub Environment (Settings → Environments): +Fork PRs from a contributor without push access go through a single approval, gated by the `image-push` GitHub Environment (Settings → Environments), *before* anything is built — as soon as `detect` finds an image that needs building: 1. Create the environment `image-push`. 2. Add required reviewers (the maintainers/dev team). -3. Leave `PUSH_SERVICE_ACCOUNT_TOKEN` as a repository secret. The environment gates the approval, not the secret: one job holds the approval and the per-image push jobs run after it, so a run costs one approval rather than one per image, and those jobs read the repository secret — the same one the non-fork path has always used. +3. Leave `PUSH_SERVICE_ACCOUNT_TOKEN` as a repository secret. The environment gates the approval, not the secret: one job holds the approval and the build/push jobs run after it, so a run costs one approval rather than one per image, and those jobs read the repository secret — the same one the non-fork path has always used. -This approval only applies to fork PRs. A push to `main` or a same-repo PR builds and pushes without any approval at all, using the repository-level `PUSH_SERVICE_ACCOUNT_TOKEN` secret directly (that path never declares `environment:` on its jobs, so this environment's protection rules don't apply to it). +This approval only applies to fork PRs from a contributor without push access — checked via `author_association`. A fork PR from a maintainer's own fork (`OWNER`/`MEMBER`/`COLLABORATOR`), a push to `main`, or a same-repo PR all build and push without any approval at all, using the repository-level `PUSH_SERVICE_ACCOUNT_TOKEN` secret directly (those paths never gate on `environment:`, so this environment's protection rules don't apply to them). When testing this on `ddev-test/ddev`, do the same steps there first, and confirm `vars.DOCKER_ORG` on that repository points at the DockerHub org used for testing. diff --git a/pkg/versionconstants/versionconstants.go b/pkg/versionconstants/versionconstants.go index b08ff2907ef..2b6b0d6d3c2 100644 --- a/pkg/versionconstants/versionconstants.go +++ b/pkg/versionconstants/versionconstants.go @@ -46,10 +46,10 @@ var BaseDBTagBranch = "20260814_rfay_docker_update_phase_2" var TraefikRouterImage = "ddev/ddev-traefik-router" // TraefikRouterTag is traefik router tag -var TraefikRouterTag = "bffcda31c5" // 20260814_rfay_docker_update_phase_2-bffcda31c5 +var TraefikRouterTag = "49c52cbcb6" // 20260821_test_fork_pr_image-49c52cbcb6 // TraefikRouterTagBranch is the branch TraefikRouterTag's content was built from. -var TraefikRouterTagBranch = "20260814_rfay_docker_update_phase_2" +var TraefikRouterTagBranch = "20260821_test_fork_pr_image" // SSHAuthImage is image for agent var SSHAuthImage = "ddev/ddev-ssh-agent"