1
0
mirror of synced 2026-10-23 07:08:55 +03:00

Address the pre-merge review

Ten fixes from a review pass with fresh eyes. The ones that mattered:

rebuild-yum-repo.sh signed repomd.xml with no -u, so it used whatever
the keyring's default key resolved to. That is the packaging key today
only because there is one secret key in it. A repository whose metadata
is signed by the wrong key is one every client with gpgcheck=1 rejects,
produced by a run that exits 0.

The snapshot publish gate required only deb and rpm, whilst its comment
justified exactly one exception. A failed docs, tarball or wheel job
published a snapshot missing those files, and a failed repo-rpm job was
worse: install-repo-readme.py then fails after the packages are uploaded
and signed, leaving half a snapshot on the download server.

The location input was interpolated into shell on the runner holding both
publishing keys. It now travels through the environment, and is validated
against the shape of a datestamp before anything downstream sees it, so a
bad one fails in five seconds rather than after an hour of building.

Make.bat tested %ERRORLEVEL% inside a parenthesised block, where it is
expanded at parse time, so a failed Authenticode signature on
pgAdmin4.exe was undetectable; the handler was PAUSE, which on a headless
signing host either returns at once or blocks until the job times out.
Both sites now fail the build.

A called workflow inherits the caller's ref, so once this is on master an
ordinary push would have shared a concurrency group with the nightly
snapshot's builds and cancelled them.

And PULL_KEY named a file that does not exist on the download server,
which would have failed the first release promotion.

The rest: if-no-files-found on every build artefact, so a build that
produces nothing fails rather than silently shipping a snapshot without
it; pinned syft and helm installers, which were piping a default branch
into a shell as root in the container that builds signed packages; the
Helm chart no longer built and discarded; and the comments that had
stopped describing the code.

The self-test grows from 56 cases to 70, covering the verbs that delete
things, arguments that look like options, and the SSH_ORIGINAL_COMMAND
entry point rather than only the dry-run path.
This commit is contained in:
Dave Page
2026-09-21 09:30:46 +01:00
parent b8ac3485ad
commit 6f322ea160
17 changed files with 676 additions and 525 deletions

View File

@@ -1,9 +1,9 @@
name: Check container build
on:
# Callable from snapshot.yml, so that what gets published is the
# same build that runs on every push rather than a second
# definition of it that can drift.
# Callable, so that a workflow which needs a container build gets this one
# rather than a second definition of it that can drift. Snapshot does not
# call it today, the image not being part of the download tree.
workflow_call:
push:
@@ -36,7 +36,12 @@ concurrency:
# github.workflow, because in a reusable workflow that expression is
# the CALLER's name. Every workflow Snapshot calls would otherwise
# share one group and, with cancel-in-progress, cancel each other.
group: 'check-container-build @ ${{ github.event.pull_request.head.label || github.head_ref || github.ref }}'
# github.event_name is in the key because a called workflow inherits the
# caller's ref: once this is on master, the nightly Snapshot's deb build
# and an ordinary push's deb build would otherwise share a group and,
# with cancel-in-progress, kill each other. A push landing mid-nightly
# would cancel the snapshot's build and skip the publish entirely.
group: 'check-container-build @ ${{ github.event.pull_request.head.label || github.head_ref || github.ref }} @ ${{ github.event_name }}'
cancel-in-progress: true
jobs:

View File

@@ -34,7 +34,12 @@ concurrency:
# github.workflow, because in a reusable workflow that expression is
# the CALLER's name. Every workflow Snapshot calls would otherwise
# share one group and, with cancel-in-progress, cancel each other.
group: 'check-deb-build @ ${{ github.ref }}'
# github.event_name is in the key because a called workflow inherits the
# caller's ref: once this is on master, the nightly Snapshot's deb build
# and an ordinary push's deb build would otherwise share a group and,
# with cancel-in-progress, kill each other. A push landing mid-nightly
# would cancel the snapshot's build and skip the publish entirely.
group: 'check-deb-build @ ${{ github.ref }} @ ${{ github.event_name }}'
cancel-in-progress: true
permissions:
@@ -98,7 +103,7 @@ jobs:
|| python3 -m pip install sphinxcontrib-youtube
- name: Install syft
run: curl -sSfL https://raw.githubusercontent.com/anchore/syft/main/install.sh | sh -s -- -b /usr/local/bin
run: curl -sSfL https://raw.githubusercontent.com/anchore/syft/v1.52.0/install.sh | sh -s -- -b /usr/local/bin v1.52.0
- uses: actions/setup-node@v7
with:
@@ -127,4 +132,5 @@ jobs:
uses: actions/upload-artifact@v7
with:
name: pgadmin4-deb-build-output-${{ matrix.name }}
if-no-files-found: error
path: dist/*.deb

View File

@@ -36,7 +36,12 @@ concurrency:
# github.workflow, because in a reusable workflow that expression is
# the CALLER's name. Every workflow Snapshot calls would otherwise
# share one group and, with cancel-in-progress, cancel each other.
group: 'check-doc-builds @ ${{ github.event.pull_request.head.label || github.head_ref || github.ref }}'
# github.event_name is in the key because a called workflow inherits the
# caller's ref: once this is on master, the nightly Snapshot's deb build
# and an ordinary push's deb build would otherwise share a group and,
# with cancel-in-progress, kill each other. A push landing mid-nightly
# would cancel the snapshot's build and skip the publish entirely.
group: 'check-doc-builds @ ${{ github.event.pull_request.head.label || github.head_ref || github.ref }} @ ${{ github.event_name }}'
cancel-in-progress: true
jobs:
@@ -63,6 +68,7 @@ jobs:
uses: actions/upload-artifact@v7
with:
name: pgadmin4-html-build-output
if-no-files-found: error
path: docs/en_US/_build/html/*
- name: Check the ePub build
@@ -74,6 +80,7 @@ jobs:
uses: actions/upload-artifact@v7
with:
name: pgadmin4-epub-build-output
if-no-files-found: error
path: docs/en_US/_build/epub/pgAdmin4.epub
- name: Check the PDF build
@@ -85,4 +92,5 @@ jobs:
uses: actions/upload-artifact@v7
with:
name: pgadmin4-pdf-build-output
if-no-files-found: error
path: docs/en_US/_build/latex/pgadmin4.pdf

View File

@@ -24,7 +24,12 @@ concurrency:
# github.workflow, because in a reusable workflow that expression is
# the CALLER's name. Every workflow Snapshot calls would otherwise
# share one group and, with cancel-in-progress, cancel each other.
group: 'check-helm-build @ ${{ github.ref }}'
# github.event_name is in the key because a called workflow inherits the
# caller's ref: once this is on master, the nightly Snapshot's deb build
# and an ordinary push's deb build would otherwise share a group and,
# with cancel-in-progress, kill each other. A push landing mid-nightly
# would cancel the snapshot's build and skip the publish entirely.
group: 'check-helm-build @ ${{ github.ref }} @ ${{ github.event_name }}'
cancel-in-progress: true
permissions:
@@ -39,7 +44,7 @@ jobs:
- name: Install Helm
run: |
curl -fsSL https://raw.githubusercontent.com/helm/helm/main/scripts/get-helm-3 -o get-helm-3
curl -fsSL https://raw.githubusercontent.com/helm/helm/v4.3.0/scripts/get-helm-3 -o get-helm-3
chmod 700 get-helm-3
./get-helm-3
@@ -50,4 +55,5 @@ jobs:
uses: actions/upload-artifact@v7
with:
name: pgadmin4-helm-build-output
if-no-files-found: error
path: pgadmin4-helm-*.tgz

View File

@@ -115,4 +115,5 @@ jobs:
uses: actions/upload-artifact@v7
with:
name: pgadmin4-macos-build-output-${{ matrix.name }}
if-no-files-found: error
path: dist/*.dmg

View File

@@ -38,7 +38,12 @@ concurrency:
# github.workflow, because in a reusable workflow that expression is
# the CALLER's name. Every workflow Snapshot calls would otherwise
# share one group and, with cancel-in-progress, cancel each other.
group: 'check-python-build @ ${{ github.event.pull_request.head.label || github.head_ref || github.ref }}'
# github.event_name is in the key because a called workflow inherits the
# caller's ref: once this is on master, the nightly Snapshot's deb build
# and an ordinary push's deb build would otherwise share a group and,
# with cancel-in-progress, kill each other. A push landing mid-nightly
# would cancel the snapshot's build and skip the publish entirely.
group: 'check-python-build @ ${{ github.event.pull_request.head.label || github.head_ref || github.ref }} @ ${{ github.event_name }}'
cancel-in-progress: true
jobs:
@@ -65,4 +70,5 @@ jobs:
uses: actions/upload-artifact@v7
with:
name: pgadmin4-python-build-output
if-no-files-found: error
path: dist/*

View File

@@ -37,7 +37,12 @@ concurrency:
# github.workflow, because in a reusable workflow that expression is
# the CALLER's name. Every workflow Snapshot calls would otherwise
# share one group and, with cancel-in-progress, cancel each other.
group: 'check-rpm-build @ ${{ github.ref }}'
# github.event_name is in the key because a called workflow inherits the
# caller's ref: once this is on master, the nightly Snapshot's deb build
# and an ordinary push's deb build would otherwise share a group and,
# with cancel-in-progress, kill each other. A push landing mid-nightly
# would cancel the snapshot's build and skip the publish entirely.
group: 'check-rpm-build @ ${{ github.ref }} @ ${{ github.event_name }}'
cancel-in-progress: true
permissions:
@@ -135,7 +140,7 @@ jobs:
run: python3 -m pip install --break-system-packages sphinxcontrib-youtube || python3 -m pip install sphinxcontrib-youtube
- name: Install syft
run: curl -sSfL https://raw.githubusercontent.com/anchore/syft/main/install.sh | sh -s -- -b /usr/local/bin
run: curl -sSfL https://raw.githubusercontent.com/anchore/syft/v1.52.0/install.sh | sh -s -- -b /usr/local/bin v1.52.0
- uses: actions/setup-node@v7
with:
@@ -160,4 +165,5 @@ jobs:
uses: actions/upload-artifact@v7
with:
name: pgadmin4-rpm-build-output-${{ matrix.name }}
if-no-files-found: error
path: dist/*.rpm

View File

@@ -38,7 +38,12 @@ concurrency:
# github.workflow, because in a reusable workflow that expression is
# the CALLER's name. Every workflow Snapshot calls would otherwise
# share one group and, with cancel-in-progress, cancel each other.
group: 'check-tarball-build @ ${{ github.event.pull_request.head.label || github.head_ref || github.ref }}'
# github.event_name is in the key because a called workflow inherits the
# caller's ref: once this is on master, the nightly Snapshot's deb build
# and an ordinary push's deb build would otherwise share a group and,
# with cancel-in-progress, kill each other. A push landing mid-nightly
# would cancel the snapshot's build and skip the publish entirely.
group: 'check-tarball-build @ ${{ github.event.pull_request.head.label || github.head_ref || github.ref }} @ ${{ github.event_name }}'
cancel-in-progress: true
jobs:
@@ -65,4 +70,5 @@ jobs:
uses: actions/upload-artifact@v7
with:
name: pgadmin4-tarball-build-output
if-no-files-found: error
path: dist/*

View File

@@ -137,4 +137,5 @@ jobs:
uses: actions/upload-artifact@v7
with:
name: pgadmin4-windows-build-output
if-no-files-found: error
path: dist/*.exe

View File

@@ -151,6 +151,7 @@ jobs:
uses: actions/upload-artifact@v7
with:
name: pgadmin4-macos-snapshot-${{ matrix.name }}
if-no-files-found: error
path: |
dist/*.dmg
dist/*.zip

View File

@@ -158,9 +158,10 @@ jobs:
CALL "%VS_INSTALL_DIR%\VC\Auxiliary\Build\vcvars64.bat" || EXIT /b 1
CALL Make.bat || EXIT /b 1
# Make.bat verifies the installer's own signature, but says nothing about
# the executable inside it, which is signed in a separate step and is the
# thing users actually run.
# Make.bat now fails the build if either signature fails, the executable
# inside the installer included, so this is a second opinion rather than
# the only check. It verifies what is in dist/, which is the installer;
# the executable within it is covered by Make.bat refusing to continue.
- name: Verify the signatures
shell: powershell
run: |
@@ -175,4 +176,5 @@ jobs:
uses: actions/upload-artifact@v7
with:
name: pgadmin4-windows-snapshot
if-no-files-found: error
path: dist/*.exe

View File

@@ -60,9 +60,25 @@ jobs:
name: ${{ steps.pick.outputs.name }}
steps:
- id: pick
env:
REQUESTED: ${{ inputs.location }}
run: |
NAME="${{ inputs.location || '' }}"
set -euo pipefail
NAME="${REQUESTED:-}"
[ -n "${NAME}" ] || NAME=$(date +%Y-%m-%d)
# Validated here so that everything downstream, including a shell on
# the machine holding the publishing keys, is working with a name
# that cannot be anything but a date and an optional suffix. The
# wrapper on the server applies its own rule to the same value; this
# one exists so a bad input fails in five seconds rather than after
# an hour of building.
if ! printf '%s' "${NAME}" | grep -Eq '^[0-9]{4}-[0-9]{2}-[0-9]{2}(-[1-9][0-9]?)?$'; then
echo "Not a valid snapshot location: ${NAME}" >&2
echo "Expected YYYY-MM-DD, optionally with a -N suffix." >&2
exit 1
fi
echo "Snapshot location: ${NAME}"
echo "name=${NAME}" >> "${GITHUB_OUTPUT}"
@@ -76,8 +92,11 @@ jobs:
uses: ./.github/workflows/check-python-build.yml
docs:
uses: ./.github/workflows/check-doc-builds.yml
helm:
uses: ./.github/workflows/check-helm-build.yml
# No helm job here. The chart is not part of the download tree: the
# buildfarm pushed it to an OCI registry, which nothing in this workflow
# does yet. Calling the build and then discarding its artefact, which is what
# happened before, cost a runner and proved nothing that check-helm-build
# does not already prove on every push.
# The repository configuration RPMs. These are not built by the QA
# workflows, because they are not a build of pgAdmin: they are a one-file
@@ -98,8 +117,12 @@ jobs:
- name: Build the repository RPMs
env:
PGADMIN_REPO_DIR: https://ftp.postgresql.org/pub/pgadmin/pgadmin4/snapshots/${{ needs.location.outputs.name }}/yum
LOCATION: ${{ needs.location.outputs.name }}
run: |
# Through the environment rather than interpolated into the script.
# The location is a dispatch input, so it is text somebody chose, and
# ${{ }} substitution happens before the shell ever sees the line.
export PGADMIN_REPO_DIR="https://ftp.postgresql.org/pub/pgadmin/pgadmin4/snapshots/${LOCATION}/yum"
echo "Repositories will point at ${PGADMIN_REPO_DIR}"
pkg/redhat/repo-rpm.sh
@@ -107,6 +130,7 @@ jobs:
uses: actions/upload-artifact@v7
with:
name: pgadmin4-repo-rpm-build-output
if-no-files-found: error
path: dist/*.noarch.rpm
macos:
@@ -132,9 +156,22 @@ jobs:
# is not gated by it.
environment: publishing
# The Windows signing host is the one runner that can be offline, and a
# snapshot without an installer is still worth having.
if: always() && needs.deb.result == 'success' && needs.rpm.result == 'success'
# Everything must have built except Windows. That is the one runner which
# can legitimately be offline, and a snapshot without the installer is
# still worth having; a snapshot missing its documentation, source tarball,
# wheel or repository RPMs is not, and the repository RPMs are worse than
# missing, since install-repo-readme.py then fails after the packages have
# been uploaded and signed, leaving half a snapshot on the download server.
if: >-
always()
&& needs.location.result == 'success'
&& needs.deb.result == 'success'
&& needs.rpm.result == 'success'
&& needs.tarball.result == 'success'
&& needs.wheel.result == 'success'
&& needs.docs.result == 'success'
&& needs.repo-rpm.result == 'success'
&& needs.macos.result == 'success'
steps:
- uses: actions/checkout@v7
@@ -150,6 +187,8 @@ jobs:
# table, since dpkg already encodes it there; the codename does not, so
# that mapping is explicit.
- name: Assemble the staging tree
env:
LOCATION: ${{ needs.location.outputs.name }}
run: |
set -euo pipefail
# Flat, beside apt/ and yum/, which is how both the staging server
@@ -163,7 +202,7 @@ jobs:
# them.
rm -rf stage
STAGE="stage/${{ needs.location.outputs.name }}"
STAGE="stage/${LOCATION}"
mkdir -p "${STAGE}/yum"
# An artefact can be legitimately absent: the Windows signing host is

1005
Make.bat

File diff suppressed because it is too large Load Diff

View File

@@ -75,7 +75,10 @@ needs the publishing account to be able to write it.
Three key pairs. `roadie-publish` and `roadie-upload` belong to the runner;
`paxsor-pull` belongs to the download server, which uses it to fetch staged
content from the staging server during publication.
content from the staging server during publication. Its private half must be at
`~pgaupload/.ssh/id_ed25519_paxsor_pull` on the download server, which is
where `pga-publish` looks for it; the name in the key management system is
only a label, and the path is what matters.
`authorized_keys.procyon` and `authorized_keys.paxsor` are annotated templates:
substitute the addresses and the public keys, and read the annotations before

View File

@@ -169,7 +169,7 @@ CLOUDFRONT_CONF = "/etc/pga-publish.cloudfront"
# SITE: use the internal name or address that paxsor actually resolves.
PULL_HOST = "procyon"
PULL_USER = "pgaupload"
PULL_KEY = os.path.expanduser("~/.ssh/id_staging_pull")
PULL_KEY = os.path.expanduser("~/.ssh/id_ed25519_paxsor_pull")
# One publishing operation at a time. Two concurrent workflow runs indexing the
# same repository would produce a Release file describing packages that are half

View File

@@ -55,7 +55,13 @@ VERSION=$3
TREE=${ROOT}/yum/${FAMILY}/${NAME}-${VERSION}-${ARCH}
/usr/bin/createrepo_c "${TREE}"
/usr/bin/gpg --yes --detach-sign --armor "${TREE}/repodata/repomd.xml"
# -u, because without it gpg signs with whatever the keyring's default
# key resolves to. That is the packaging key today only because there is
# one secret key in it, and it stops being true the moment a second one
# lands. A repository whose metadata is signed by the wrong key is one
# every client with gpgcheck=1 rejects, produced by a run that exits 0.
/usr/bin/gpg --yes -u packages@pgadmin.org --detach-sign --armor \
"${TREE}/repodata/repomd.xml"
# Some EL variants report $releasever with a variant suffix, so the repository
# has to answer to each of the resulting directory names.

View File

@@ -39,6 +39,32 @@ check() {
fi
}
# check_ssh <expected-exit> <description> <raw SSH_ORIGINAL_COMMAND>
#
# The other helper drives --dry-run, which takes its request from argv and so
# joins it with single spaces. Production takes the raw string sshd puts in
# SSH_ORIGINAL_COMMAND, which keeps runs of whitespace, tabs, and whatever else
# the client sent. Only rejections are tested this way: an accepted request
# would run for real, and the point of these cases is that none of them gets
# that far.
check_ssh() {
local expected="$1"; shift
local description="$1"; shift
local output
output="$(SSH_ORIGINAL_COMMAND="$1" "$WRAPPER" 2>&1)"
local rc=$?
if [ "$rc" = "$expected" ]; then
pass=$((pass + 1))
printf 'ok %-3s %s\n' "$rc" "$description"
else
fail=$((fail + 1))
printf 'FAIL %-3s (wanted %s) %s\n' "$rc" "$expected" "$description"
printf ' %s\n' "$output"
fi
}
echo "== accepted requests: staging role (procyon) =="
check 0 staging "hello" hello
check 0 staging "stage-create" stage-create 2026-09-17
@@ -50,6 +76,11 @@ check 0 staging "stage-index-yum" stage-index-yum 2026-09-17
echo
echo "== accepted requests: download role (paxsor) =="
check 0 download "snapshot-create" snapshot-create 2026-09-17
check 0 download "snapshot-sign" snapshot-sign 2026-09-17
check 0 download "snapshot-index-apt" snapshot-index-apt 2026-09-17
check 0 download "snapshot-index-yum" snapshot-index-yum 2026-09-17
check 0 download "snapshot-purge" snapshot-purge
check 0 download "release-exists" release-exists 9.18
check 0 download "release-create" release-create 9.18
check 0 download "release-create with patch" release-create 9.18.1
@@ -111,6 +142,21 @@ check 64 download "sync-s3 takes no argument" "sync-s3 release"
check 77 download "unpublished yum arch" "rebuild-yum redhat rhel 9 aarch64"
check 77 download "unpublished apt codename" "rebuild-apt plucky"
echo
echo "== through the real entry point, SSH_ORIGINAL_COMMAND =="
check_ssh 64 "tab instead of a space" "$(printf 'stage-create\t2026-09-17')"
check_ssh 64 "embedded newline" "$(printf 'hello\nstage-create 2026-09-17')"
check_ssh 64 "trailing carriage return" "$(printf 'hello\r')"
check_ssh 64 "Cyrillic homoglyph in the verb" "ѕtage-create 2026-09-17"
check_ssh 64 "non-ASCII in an argument" "rebuild-apt bookwоrm"
check_ssh 64 "NUL-ish escape in the request" "$(printf 'hello\\x00')"
echo
echo "== an argument may not look like an option =="
check 64 download "leading hyphen as a codename" "rebuild-apt -r"
check 64 download "leading hyphen as a datestamp" "snapshot-sign -rf"
check 64 staging "lone hyphen" "stage-exists -"
echo
echo "== the dry-run flag is not reachable from a client =="
# With SSH_ORIGINAL_COMMAND set, our own argv is ignored entirely, so a client