ci: sign RPM packages before pushing to GCP Artifact Registry#4392
ci: sign RPM packages before pushing to GCP Artifact Registry#4392PrzemekZglinicki wants to merge 3 commits intomainfrom
Conversation
|
Przemek Zglinicki seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
1 similar comment
|
Przemek Zglinicki seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
PrzemekZglinicki
force-pushed
the
devprod-4134-sign-rpm-packages-connect
branch
4 times, most recently
from
e9d288b to
59ae745
Compare
|
@claude review once |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Commits
Review
|
PrzemekZglinicki
force-pushed
the
devprod-4134-sign-rpm-packages-connect
branch
from
59ae745 to
6b00c86
Compare
|
Commits Review LGTM |
PrzemekZglinicki
force-pushed
the
devprod-4134-sign-rpm-packages-connect
branch
from
6b00c86 to
75ddf01
Compare
|
Commits Review LGTM |
PrzemekZglinicki
force-pushed
the
devprod-4134-sign-rpm-packages-connect
branch
from
75ddf01 to
05fc4b6
Compare
|
Commits Review LGTM |
PrzemekZglinicki
force-pushed
the
devprod-4134-sign-rpm-packages-connect
branch
from
05fc4b6 to
c55b774
Compare
|
Commits Review LGTM |
PrzemekZglinicki
force-pushed
the
devprod-4134-sign-rpm-packages-connect
branch
2 times, most recently
from
afa557c to
c178a89
Compare
|
Commits LGTM Review Targeted change adds RPM signing before GCP AR upload. The script uses a subshell-scoped LGTM |
PrzemekZglinicki
force-pushed
the
devprod-4134-sign-rpm-packages-connect
branch
from
c178a89 to
c340f6e
Compare
|
Commits Review LGTM |
|
Commits
Review The production-intent changes in commit LGTM for the code in commit |
|
Test purpose packages was uploaded to gcp ar: 
|
|
Commits
Review LGTM |
In this repo we use the squash and merge concept, so the names of the particular commit doesn't matter, but also I can update them. What is more I think it is worth to keep test examples in the PR for future. |
ivotron
left a comment
ivotron
left a comment
There was a problem hiding this comment.
goreleaser has support for signing rpm packages using a custom key. that should be done instead. see https://goreleaser.com/customization/sign/sign/ for more
PrzemekZglinicki
force-pushed
the
devprod-4134-sign-rpm-packages-connect
branch
from
67ed751 to
44ea702
Compare
| schedule: | ||
| - cron: '0 2 * * *' # run at 2 AM UTC | ||
| workflow_dispatch: | ||
| pull_request: |
There was a problem hiding this comment.
The release workflow trigger has been replaced with pull_request: and the original push: tags, schedule, and workflow_dispatch triggers commented out. This converts the production release workflow into a PR-only workflow — production tag releases, nightly edge builds, and manual dispatch will all stop working if merged. This appears to be test scaffolding from the dropped "ci: Test purpose changes. Drop before merge!" commit (a2d3e5d) that the third commit claimed to remove but did not. The original triggers must be restored before merge.
| goos: [linux] | ||
| # goos: [windows, darwin, linux] |
There was a problem hiding this comment.
Test-purpose change must be reverted before merge. goos was reduced to [linux] only, with the original [windows, darwin, linux] commented out. If merged, Windows and macOS binaries would no longer be built or released. Restore the original goos line.
| # hooks: | ||
| # post: | ||
| # # The binary is signed and notarized when running a production release, but for snapshot builds notarization is | ||
| # # skipped and only ad-hoc signing is performed (not cryptographic material is needed). | ||
| # # | ||
| # # note: environment variables required for signing and notarization (set in CI) but are not needed for snapshot builds | ||
| # # QUILL_SIGN_P12, QUILL_SIGN_PASSWORD, QUILL_NOTARY_KEY, QUILL_NOTARY_KEY_ID, QUILL_NOTARY_ISSUER | ||
| # - cmd: ./resources/scripts/sign_for_darwin.sh "{{ .Os }}" "{{ .Path }}" "{{ .IsSnapshot }}" | ||
| # env: | ||
| # - QUILL_LOG_FILE=target/dist/quill-{{ .Target }}.log |
There was a problem hiding this comment.
Test-purpose change must be reverted before merge. The Darwin signing/notarization post-build hook is commented out. With Darwin builds also disabled (line 10), this is consistent for testing — but both must be restored before merge so macOS binaries continue to be signed and notarized.
| #archives: | ||
| # - id: connect | ||
| # ids: [connect] | ||
| # formats: tar.gz | ||
| # files: | ||
| # - README.md | ||
| # - CHANGELOG.md | ||
| # - licenses |
There was a problem hiding this comment.
Test-purpose change must be reverted before merge. The entire archives: section is commented out. If merged, no tar.gz archives would be produced for any release.
| #dockers_v2: | ||
| # - id: connect | ||
| # dockerfile: resources/docker/Dockerfile | ||
| # ids: | ||
| # - connect | ||
| # images: | ||
| # - redpandadata/connect | ||
| # - public.ecr.aws/l9j0i2e0/connect | ||
| # tags: | ||
| # - "{{ if not .IsSnapshot }}{{ .Version }}{{ end }}" | ||
| # - "{{ if and (not .IsSnapshot) (eq .Prerelease ``) }}{{ .Major }}.{{.Minor}}{{ end }}" | ||
| # - "{{ if and (not .IsSnapshot) (eq .Prerelease ``) }}{{ .Major }}{{ end }}" | ||
| # - "{{ if and (not .IsSnapshot) (eq .Prerelease ``) }}latest{{ end }}" | ||
| # - "{{ if or .IsSnapshot (ne .Prerelease ``) }}edge{{ end }}" | ||
| # platforms: | ||
| # - linux/amd64 | ||
| # - linux/arm64 | ||
| # extra_files: | ||
| # - config/docker.yaml |
There was a problem hiding this comment.
Test-purpose change must be reverted before merge. The entire dockers_v2: section is commented out. If merged, no Docker images for redpandadata/connect or public.ecr.aws/l9j0i2e0/connect would be built or pushed.
| # # Gets run once per artifact (deb or rpm) | ||
| # - name: Publish Linux packages to Cloudsmith | ||
| # ids: | ||
| # - connect-linux-pkgs | ||
| # cmd: ./resources/scripts/push_pkg_to_cloudsmith.sh {{ .ArtifactPath }} {{ .Version }} | ||
| # env: | ||
| # - CLOUDSMITH_API_KEY={{ .Env.CLOUDSMITH_API_KEY }} |
There was a problem hiding this comment.
Test-purpose change must be reverted before merge. The Cloudsmith publisher is commented out. The PR description states "DEBs are unchanged — GCP AR provides repository-level signing for APT", but this commit also disables Cloudsmith publishing entirely, which is unrelated to the RPM signing change. Restore the Cloudsmith publisher.
| release: | ||
| github: | ||
| owner: redpanda-data | ||
| name: connect | ||
| prerelease: auto | ||
| replace_existing_artifacts: true | ||
| mode: replace | ||
| disable: true | ||
| # github: | ||
| # owner: redpanda-data | ||
| # name: connect | ||
| # prerelease: auto | ||
| # replace_existing_artifacts: true | ||
| # mode: replace | ||
|
|
||
| checksum: | ||
| split: true | ||
| #checksum: | ||
| # split: true |
There was a problem hiding this comment.
Test-purpose change must be reverted before merge. The release: block has been replaced with disable: true (with the original GitHub release config commented out), and the checksum: block is commented out. If merged, no GitHub release would be published for the connect distribution and no checksum file would be produced. Restore both blocks.
| #archives: | ||
| # - id: connect-fips | ||
| # ids: [connect-fips] | ||
| # formats: tar.gz | ||
| # name_template: 'redpanda-connect-fips_{{ .Version }}_{{ .Os }}_{{ .Arch }}{{ with .Arm }}v{{ . }}{{ end }}{{ with .Mips }}_{{ . }}{{ end }}{{ if not (eq .Amd64 "v1") }}{{ .Amd64 }}{{ end }}' | ||
| # files: | ||
| # - README-FIPS.md | ||
| # - CHANGELOG.md | ||
| # - licenses |
There was a problem hiding this comment.
Test-purpose change must be reverted before merge. The entire archives: section is commented out. If merged, no tar.gz archive of the FIPS distribution would be produced.
| # # Gets run once per artifact (deb or rpm) | ||
| # - name: Publish Linux packages to Cloudsmith | ||
| # ids: | ||
| # - connect-fips-pkgs | ||
| # cmd: ./resources/scripts/push_pkg_to_cloudsmith.sh {{ .ArtifactPath }} {{ .Version }} | ||
| # env: | ||
| # - CLOUDSMITH_API_KEY={{ .Env.CLOUDSMITH_API_KEY }} |
There was a problem hiding this comment.
Test-purpose change must be reverted before merge. The Cloudsmith publisher for connect-fips-pkgs is commented out. This is unrelated to the RPM signing change and would stop FIPS packages from being pushed to Cloudsmith.
| release: | ||
| github: | ||
| owner: redpanda-data | ||
| name: connect | ||
| prerelease: auto | ||
| replace_existing_artifacts: true | ||
| mode: keep-existing | ||
| disable: true | ||
| # github: | ||
| # owner: redpanda-data | ||
| # name: connect | ||
| # prerelease: auto | ||
| # replace_existing_artifacts: true | ||
| # mode: keep-existing | ||
|
|
||
| checksum: | ||
| split: true | ||
| #checksum: | ||
| # split: true |
There was a problem hiding this comment.
Test-purpose change must be reverted before merge. Same pattern as in connect.yaml — the release: block is replaced with disable: true (original commented out) and checksum: is commented out. Restore both blocks for the FIPS distribution.
| runs-on: ubuntu-latest | ||
| steps: | ||
|
|
||
| - name: Checkout code | ||
| uses: actions/checkout@v6 | ||
|
|
||
| - name: Set up test packages | ||
| run: | | ||
| touch /tmp/test.rpm | ||
| touch /tmp/test.deb | ||
|
|
||
| - name: Mock gcloud CLI | ||
| run: | | ||
| printf '#!/bin/bash\necho "gcloud $*"\n' \ | ||
| | sudo tee /usr/local/bin/gcloud > /dev/null | ||
| sudo chmod +x /usr/local/bin/gcloud | ||
|
|
||
| - name: Test RPM GA — routed to redpanda-yum | ||
| run: | | ||
| out=$(./resources/scripts/push_pkg_to_gcp_ar.sh /tmp/test.rpm 1.0.0) | ||
| echo "$out" | ||
| echo "$out" | grep -q "artifacts yum upload redpanda-yum" | ||
|
|
||
| - name: Test RPM RC — routed to redpanda-unstable-yum | ||
| run: | | ||
| out=$(./resources/scripts/push_pkg_to_gcp_ar.sh /tmp/test.rpm 1.0.0-rc1) | ||
| echo "$out" | ||
| echo "$out" | grep -q "artifacts yum upload redpanda-unstable-yum" | ||
|
|
||
| - name: Test DEB — routed to redpanda-apt | ||
| run: | | ||
| out=$(./resources/scripts/push_pkg_to_gcp_ar.sh /tmp/test.deb 1.0.0) | ||
| echo "$out" |
There was a problem hiding this comment.
This test-push-to-gcp-ar job uses touch /tmp/test.rpm / touch /tmp/test.deb to create empty files and only verifies the gcloud routing (which command/repository the script invokes). Despite the PR description claiming the test "asserts signing + routing", signing is not exercised here — it happens in goreleaser via nfpms.rpm.signature.key_file. The "missing-region error case" mentioned in the PR description is also not tested. Either align the description with what the test actually does, or expand the test to actually validate signing and the missing-region path.
|
Commits
Review This PR mixes two distinct categories of change: (a) a legitimate RPM-signing implementation in the goreleaser configs ( The PR description is also misleading: it claims
|
PrzemekZglinicki
force-pushed
the
devprod-4134-sign-rpm-packages-connect
branch
3 times, most recently
from
aa64b47 to
31a1524
Compare
|
Commits
Review The actual code changes (fetch RPM signing key from AWS Secrets Manager, configure goreleaser to sign RPMs for the LGTM |
PrzemekZglinicki
force-pushed
the
devprod-4134-sign-rpm-packages-connect
branch
from
31a1524 to
9e54ece
Compare
|
Commits
Review CI-only change: moves RPM signing into goreleaser's nfpm support, adds a post-publish signature verification step in LGTM |
| uses: aws-actions/aws-secretsmanager-get-secrets@v3 | ||
| with: | ||
| secret-ids: | | ||
| RPM_SIGNING_KEY_PRIVATE,sdlc/prod/github/rpm_signing_key_private |
There was a problem hiding this comment.
this secret should be moved so that it is stored as JSON, to keep it consistent with the rest of secrets. that'd also allow for this to be moved to the previous task, with the rest of commits that are fetched as part of this workflow
There was a problem hiding this comment.
@PrzemekZglinicki you have to sign the CLA |
| - name: Verify RPM signature in GCP AR | ||
| if: ${{ github.event_name == 'push' && (matrix.variant == 'connect' || matrix.variant == 'connect-fips') }} | ||
| env: | ||
| RPM_SIGNING_KEY_PRIVATE: ${{ env.RPM_SIGNING_KEY_PRIVATE }} | ||
| run: | | ||
| VERSION="${{ github.ref_name }}" | ||
| VERSION="${VERSION#v}" | ||
| SEARCH_TERM=$(printf '%s' "$VERSION" | tr '-' '~') | ||
| GA_PATTERN='^[0-9]+\.[0-9]+\.[0-9]+$' | ||
| if [[ "$VERSION" =~ $GA_PATTERN ]]; then | ||
| REPO="redpanda-yum" | ||
| else | ||
| REPO="redpanda-unstable-yum" | ||
| fi | ||
| if [[ "${{ matrix.variant }}" == "connect-fips" ]]; then | ||
| PKG_NAME="redpanda-connect-fips" | ||
| else | ||
| PKG_NAME="redpanda-connect" | ||
| fi | ||
| RPM_FILE=$(gcloud artifacts files list \ | ||
| --repository="$REPO" --location=us-central1 \ | ||
| --project=production-devprod \ | ||
| --filter="name~${SEARCH_TERM} AND name~x86_64" \ | ||
| --format="value(name)" | grep "${PKG_NAME}-" | grep -v "${PKG_NAME}-fips" | head -1) | ||
| [[ -n "$RPM_FILE" ]] || { echo "ERROR: no RPM found for ${PKG_NAME} ${SEARCH_TERM} in ${REPO}"; exit 1; } | ||
| mkdir -p /tmp/rpms | ||
| gcloud artifacts files download \ | ||
| --repository="$REPO" --location=us-central1 \ | ||
| --project=production-devprod --destination=/tmp/rpms/ "$RPM_FILE" | ||
| RPM=$(find /tmp/rpms -name "*.rpm" | head -1) | ||
| [[ -n "$RPM" ]] || { echo "ERROR: no .rpm file found in /tmp/rpms"; exit 1; } | ||
| tmpgnupg=$(mktemp -d) | ||
| tmpdb=$(mktemp -d) | ||
| pubkey=$(mktemp) | ||
| trap 'rm -rf "$tmpgnupg" "$tmpdb" "$pubkey"' EXIT | ||
| printf '%s\n' "$RPM_SIGNING_KEY_PRIVATE" | GNUPGHOME="$tmpgnupg" gpg --batch --import | ||
| KEY_ID=$(GNUPGHOME="$tmpgnupg" gpg --list-secret-keys --with-colons | awk -F: '/^sec/{print $5}' | head -1) | ||
| GNUPGHOME="$tmpgnupg" gpg --armor --export "$KEY_ID" > "$pubkey" | ||
| rpm --dbpath "$tmpdb" --import "$pubkey" | ||
| rpm --dbpath "$tmpdb" --checksig "$RPM" | ||
| echo "RPM signature verification passed for: $(basename "$RPM")" | ||
|
|
There was a problem hiding this comment.
instead of this being here, it should be in a separate bash script and referenced in the goreleaser config, as done for cloudsmith here https://github.com/redpanda-data/connect/blob/main/.goreleaser/connect.yaml#L97-L100
There was a problem hiding this comment.
It is verification, we can move it to the script. It is similar approach as in the smokechecks in the vtools repo, we sign the package, push it to the gcp ar and at the end verify that this package is available and correctly signed, it isn't a big effort and we are sure that package is correct.
There was a problem hiding this comment.
Whole logic of the push to gcp ar is in the script.
PrzemekZglinicki
force-pushed
the
devprod-4134-sign-rpm-packages-connect
branch
from
9e54ece to
efe275b
Compare
|
Commits
Review LGTM |
- Add rpm.signature.key_file to nfpms in connect.yaml and connect-fips.yaml
so nfpm signs RPMs at build time via goreleaser's native signing support
- Add "Prepare RPM signing key" step: writes RPM_SIGNING_KEY_PRIVATE to a
temp .asc file and exports RPM_KEY_FILE_PRIVATE via GITHUB_ENV
- Add "Verify RPM signature in GCP AR" step: after publish, downloads the
RPM from GCP AR and verifies its GPG signature using rpm --dbpath <tmpdir>
to avoid the silent-pass bug where rpm --checksig uses the system keyring
- Fix SEARCH_TERM to use tr instead of bash ${var//-/~} substitution — bash
expands ~ to $HOME in parameter replacement even inside double quotes
- Simplify test-push-to-gcp-ar: routing-only tests with mocked gcloud CLI
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
PrzemekZglinicki
force-pushed
the
devprod-4134-sign-rpm-packages-connect
branch
from
efe275b to
2d8c9f2
Compare
|
Commits
Review LGTM |
3 participants
Summary
'` instead of `${VERSION//-/}` — bash expands `~` to `$HOME` in parameter substitution replacements even inside double quotes🤖 Generated with Claude Code