feat(tests): KMS bundle-signing e2e against MiniStack over TLS#1298
Open
lockwobr wants to merge 1 commit into
Open
feat(tests): KMS bundle-signing e2e against MiniStack over TLS#1298lockwobr wants to merge 1 commit into
lockwobr wants to merge 1 commit into
Conversation
lockwobr
added
the
theme/ci-dx
lockwobr
force-pushed
the
feat/kms-ministack-e2e
branch
from
78526cc to
65d44fa
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces end-to-end testing infrastructure for KMS-backed bundle signing using MiniStack (AWS emulator). It pins awscli, ministack_image, and mkcert (with checksums) in .settings.yaml, extends the load-versions action to export those values, adds mkcert detection and installation to tooling scripts, updates cli-e2e to include signing tests with selectors, adds a kms-ministack-e2e CI workflow that provisions MiniStack over TLS, creates an ECDSA P-256 KMS key, builds/selects aicr, exports the KMS public key, and runs Chainsaw tests. It also adds a run.sh harness, a Chainsaw test spec for signing/verification/tamper/wrong-key cases, and a README documenting local/CI usage and trust handling. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/kms-ministack-e2e.yaml:
- Around line 41-43: Add an inline explanatory comment next to the permissions
block explaining why id-token: write is required: state that id-token: write
enables GitHub Actions OIDC tokens used for Sigstore keyless OIDC signing which
the workflow uses to produce SLSA provenance, and reference the
provenance-generating step or job by name so reviewers can easily correlate the
permission to the Sigstore signing step.
In `@tools/setup-tools`:
- Around line 499-519: Add SHA256 checksum verification for the mkcert binary
download similar to helmfile/chainsaw: add entries under
testing_tools.mkcert_checksums in .settings.yaml (keys like linux_amd64 and
linux_arm64), compute expected checksum lookup using MKCERT_ARCH mapping,
download the MKCERT_URL to a temp file (instead of streaming to /usr/local/bin),
verify the file's sha256sum against the expected checksum, only then move it to
/usr/local/bin and chmod +x; update the install block that constructs
MKCERT_URL, the download step and the final install step to perform these
verification checks and fail with a clear log message if the checksum does not
match.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: b87e50ea-b099-44c0-8c79-ea71c7de526b
📒 Files selected for processing (8)
.github/actions/load-versions/action.yml.github/workflows/kms-ministack-e2e.yaml.settings.yamltests/chainsaw/cli/bundle-attestation-kms-ministack/README.mdtests/chainsaw/cli/bundle-attestation-kms-ministack/chainsaw-test.yamltests/chainsaw/cli/bundle-attestation-kms-ministack/run.shtools/check-toolstools/setup-tools
lockwobr
force-pushed
the
feat/kms-ministack-e2e
branch
from
65d44fa to
4c522f8
Compare
Contributor
Coverage Report ✅
Coverage BadgeNo Go source files changed in this PR. |
lockwobr
force-pushed
the
feat/kms-ministack-e2e
branch
from
4c522f8 to
8f1d1ab
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/kms-ministack-e2e.yaml:
- Around line 126-281: The workflow embeds heavy implementation (Install AWS
CLI, Install mkcert and provision TLS cert, Start MiniStack, Provision KMS key
in MiniStack, Export public key to PEM, Run KMS MiniStack chainsaw tests)
directly in inline steps instead of a reusable composite action; extract these
steps into a Layer-2 composite action that encapsulates AWS
CLI/mkcert/MiniStack/KMS setup and teardown and replace the inline blocks with
calls to that composite action, exposing inputs for MINISTACK_IMAGE,
MINISTACK_CERT_DIR, MINISTACK_ENDPOINT and outputs like
KMS_KEY_ARN/KMS_URI/KMS_PEM_FILE and an option to run the chainsaw test so the
workflow only orchestrates the composite action.
In @.settings.yaml:
- Around line 86-88: The pinned awscli entry in .settings.yaml is not being
wired into the local tool installers and validators; update tools/setup-tools to
install testing_tools.awscli (use the pinned version from .settings.yaml) and
update tools/check-tools to verify the aws binary and its version matches
testing_tools.awscli, so
tests/chainsaw/cli/bundle-attestation-kms-ministack/run.sh can rely on the
installed tool; reference the awscli key in .settings.yaml, the setup logic in
tools/setup-tools, and the validation logic in tools/check-tools when making the
changes.
In `@tests/chainsaw/cli/bundle-attestation-kms-ministack/chainsaw-test.yaml`:
- Around line 24-30: Update the default MiniStack endpoint scheme from http to
https by replacing the literal "http://localhost:4566" with
"https://localhost:4566" wherever it appears in this YAML (notably in the
MINISTACK_ENDPOINT default in the description and the other occurrences at lines
referenced); search for the string "http://localhost:4566" and change it to
"https://localhost:4566" so the harness, CI, and run.sh TLS usage matches the
documented default and any CLI fallbacks.
- Around line 140-144: The test currently only asserts that the keys
"checksumsPassed" and "bundleAttested" appear in stdout, which lets false values
pass; update the assertions to verify the actual boolean values emitted by the
verifier (ChecksumsPassed and BundleAttested) are true. Locate the CLI test
checks around the blocks referencing 'checksumsPassed' and 'bundleAttested'
(also the similar block at 185-188) and change them to assert the JSON value is
true (e.g., check for '"checksumsPassed": true' and '"bundleAttested": true' or
parse stdout as JSON and assert .ChecksumsPassed == true and .BundleAttested ==
true) so failed verification no longer falsely passes. Ensure you reference the
verifier fields ChecksumsPassed and BundleAttested from
pkg/bundler/verifier/verifier.go when updating the expectations.
In `@tests/chainsaw/cli/bundle-attestation-kms-ministack/README.md`:
- Around line 68-70: The fenced code block containing the example URI
awskms://<host:port>/<arn> is unlabeled and triggers MD040; add a language tag
(e.g., text) to the opening fence so it becomes ```text to satisfy linting and
ensure consistent rendering in docs (update the fenced block around the
awskms://<host:port>/<arn> example in README.md).
- Around line 139-149: Update the "Triggers on push to `main` and
`workflow_dispatch`" paragraph to also list the `pull_request` trigger (and
which branch patterns it matches) so the README accurately reflects the
workflow's current triggers; edit the sentence that begins "Triggers on push to
`main` and `workflow_dispatch`." to read the full set (e.g., push to main,
pull_request matching the same branches, and workflow_dispatch), and add a brief
clarifying note that this describes current behavior (not future intent) so
contributors know when CI will run.
In `@tests/chainsaw/cli/bundle-attestation-kms-ministack/run.sh`:
- Around line 237-278: In build_binary(), when using a pre-built executable (the
branch that checks [ -n "${AICR_BIN}" ] && [ -x "${AICR_BIN}" ]), export
AICR_ATTESTED=true so downstream tests (e.g., verify-min-trust-verified) run for
CI-attested artifacts; conversely, in the snapshot build path (the goreleaser
branch that builds an unattested binary) explicitly set/export
AICR_ATTESTED=false to ensure attestation state is clear. Reference: function
build_binary, variables AICR_BIN and AICR_ATTESTED.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: f526079a-d596-4026-a922-559887357f32
📒 Files selected for processing (8)
.github/actions/load-versions/action.yml.github/workflows/kms-ministack-e2e.yaml.settings.yamltests/chainsaw/cli/bundle-attestation-kms-ministack/README.mdtests/chainsaw/cli/bundle-attestation-kms-ministack/chainsaw-test.yamltests/chainsaw/cli/bundle-attestation-kms-ministack/run.shtools/check-toolstools/setup-tools
Comment on lines
+126
to
+281
| - name: Install AWS CLI | ||
| run: | | ||
| set -euo pipefail | ||
| python3 -m pip install --quiet "awscli==${{ steps.versions.outputs.awscli }}" | ||
|
|
||
| - name: Install mkcert and provision TLS cert | ||
| env: | ||
| MKCERT_VERSION: ${{ steps.versions.outputs.mkcert }} | ||
| MKCERT_SHA256: ${{ steps.versions.outputs.mkcert_sha256_linux_amd64 }} | ||
| run: | | ||
| set -euo pipefail | ||
| # Pinned mkcert binary (GitHub release), checksum-verified before install. | ||
| tmp="$(mktemp)" | ||
| curl -fsSL -o "$tmp" \ | ||
| "https://github.com/FiloSottile/mkcert/releases/download/${MKCERT_VERSION}/mkcert-${MKCERT_VERSION}-linux-amd64" | ||
| echo "${MKCERT_SHA256} ${tmp}" | sha256sum -c - | ||
| sudo install -m 0755 "$tmp" /usr/local/bin/mkcert | ||
| rm -f "$tmp" | ||
|
|
||
| # Install the mkcert CA into the runner trust store so the Go AWS SDK | ||
| # trusts MiniStack's TLS. Ephemeral runner, so this is safe. | ||
| mkcert -install | ||
|
|
||
| mkdir -p "${MINISTACK_CERT_DIR}" | ||
| mkcert -cert-file "${MINISTACK_CERT_DIR}/cert.pem" \ | ||
| -key-file "${MINISTACK_CERT_DIR}/key.pem" \ | ||
| localhost 127.0.0.1 ::1 | ||
| chmod 644 "${MINISTACK_CERT_DIR}"/*.pem | ||
|
|
||
| # The Python aws CLI uses its own bundle; point it at the mkcert root CA. | ||
| echo "AWS_CA_BUNDLE=$(mkcert -CAROOT)/rootCA.pem" >> "$GITHUB_ENV" | ||
|
|
||
| - name: Start MiniStack | ||
| env: | ||
| MINISTACK_IMAGE: ${{ steps.versions.outputs.ministack_image }} | ||
| run: | | ||
| set -euo pipefail | ||
|
|
||
| # Token-free AWS emulator; image is pinned in .settings.yaml. USE_SSL=1 | ||
| # serves TLS with the mounted mkcert cert (sigstore awskms needs https). | ||
| docker run -d --name ministack -p 4566:4566 \ | ||
| -e USE_SSL=1 \ | ||
| -e MINISTACK_SSL_CERT=/certs/cert.pem \ | ||
| -e MINISTACK_SSL_KEY=/certs/key.pem \ | ||
| -v "${MINISTACK_CERT_DIR}:/certs:ro" \ | ||
| "${MINISTACK_IMAGE}" >/dev/null | ||
|
|
||
| # Wait for KMS to serve. AWS_MAX_ATTEMPTS=1 + short CLI timeouts keep each | ||
| # probe fast while the endpoint is still coming up. | ||
| for _ in $(seq 1 45); do | ||
| if [ "$(docker inspect -f '{{.State.Running}}' ministack 2>/dev/null)" != "true" ]; then | ||
| docker logs ministack 2>&1 | tail -20 | ||
| echo "::error::MiniStack container exited unexpectedly" | ||
| exit 1 | ||
| fi | ||
| if AWS_MAX_ATTEMPTS=1 aws kms list-keys \ | ||
| --endpoint-url "${MINISTACK_ENDPOINT}" \ | ||
| --cli-connect-timeout 3 --cli-read-timeout 5 \ | ||
| --output text >/dev/null 2>&1; then | ||
| echo "MiniStack KMS is ready" | ||
| exit 0 | ||
| fi | ||
| sleep 2 | ||
| done | ||
| docker logs ministack 2>&1 | tail -20 | ||
| echo "::error::MiniStack did not become ready in time" | ||
| exit 1 | ||
|
|
||
| - name: Build aicr binary | ||
| env: | ||
| GOFLAGS: -mod=vendor | ||
| run: | | ||
| set -euo pipefail | ||
| goreleaser build --clean --single-target --snapshot --timeout 10m | ||
|
|
||
| - name: Locate binary and detect attestation | ||
| run: | | ||
| set -euo pipefail | ||
| AICR_BIN="" | ||
| for pattern in dist/aicr_linux_amd64_v1/aicr dist/aicr_linux_amd64/aicr; do | ||
| if [ -x "$pattern" ]; then | ||
| AICR_BIN="$(pwd)/$pattern" | ||
| break | ||
| fi | ||
| done | ||
| if [ -z "$AICR_BIN" ]; then | ||
| echo "::error::Built binary not found in dist/" | ||
| exit 1 | ||
| fi | ||
| echo "AICR_BIN=${AICR_BIN}" >> "$GITHUB_ENV" | ||
| echo "Binary: ${AICR_BIN}" | ||
|
|
||
| ATTEST_FILE="$(dirname "$AICR_BIN")/aicr-attestation.sigstore.json" | ||
| if [ -f "$ATTEST_FILE" ]; then | ||
| echo "AICR_ATTESTED=true" >> "$GITHUB_ENV" | ||
| echo "AICR_IDENTITY_REGEXP=https://github.com/${{ github.repository }}/.github/workflows/kms-ministack-e2e\\.yaml@.*" >> "$GITHUB_ENV" | ||
| echo "Binary attestation found: $ATTEST_FILE" | ||
| else | ||
| echo "AICR_ATTESTED=false" >> "$GITHUB_ENV" | ||
| echo "::warning::No binary attestation found; verify-min-trust-verified step will be skipped" | ||
| fi | ||
|
|
||
| - name: Provision KMS key in MiniStack | ||
| run: | | ||
| set -euo pipefail | ||
|
|
||
| # Create an ECDSA P-256 asymmetric signing key | ||
| KMS_KEY_ARN=$(aws kms create-key \ | ||
| --endpoint-url "${MINISTACK_ENDPOINT}" \ | ||
| --key-spec ECC_NIST_P256 \ | ||
| --key-usage SIGN_VERIFY \ | ||
| --description "aicr-e2e-signing-key" \ | ||
| --query 'KeyMetadata.Arn' \ | ||
| --output text) | ||
|
|
||
| echo "Provisioned KMS key ARN: ${KMS_KEY_ARN}" | ||
| echo "KMS_KEY_ARN=${KMS_KEY_ARN}" >> "$GITHUB_ENV" | ||
|
|
||
| # Derive the URI with the MiniStack endpoint embedded in the host portion. | ||
| # Format: awskms://<host:port>/<arn> | ||
| HOST="${MINISTACK_ENDPOINT#http://}" | ||
| HOST="${HOST#https://}" | ||
| HOST="${HOST%/}" | ||
| KMS_URI="awskms://${HOST}/${KMS_KEY_ARN}" | ||
| echo "KMS_URI=${KMS_URI}" >> "$GITHUB_ENV" | ||
| echo "KMS URI: ${KMS_URI}" | ||
|
|
||
| - name: Export public key to PEM | ||
| run: | | ||
| set -euo pipefail | ||
| mkdir -p /tmp/kms-e2e | ||
| aws kms get-public-key \ | ||
| --endpoint-url "${MINISTACK_ENDPOINT}" \ | ||
| --key-id "${KMS_KEY_ARN}" \ | ||
| --query 'PublicKey' \ | ||
| --output text | base64 -d | \ | ||
| openssl pkey -pubin -inform DER -outform PEM \ | ||
| -out /tmp/kms-e2e/signing-key.pem | ||
| echo "KMS_PEM_FILE=/tmp/kms-e2e/signing-key.pem" >> "$GITHUB_ENV" | ||
| echo "Public key exported to /tmp/kms-e2e/signing-key.pem" | ||
|
|
||
| - name: Run KMS MiniStack chainsaw tests | ||
| env: | ||
| AICR_ATTESTED: ${{ env.AICR_ATTESTED }} | ||
| AICR_IDENTITY_REGEXP: ${{ env.AICR_IDENTITY_REGEXP }} | ||
| run: | | ||
| set -euo pipefail | ||
| chainsaw test \ | ||
| --no-cluster \ | ||
| --config tests/chainsaw/chainsaw-config.yaml \ | ||
| --test-dir tests/chainsaw/cli/bundle-attestation-kms-ministack/ \ | ||
| --selector 'requires=ministack' | ||
|
|
||
| - name: Stop MiniStack | ||
| if: always() | ||
| run: docker rm -f ministack >/dev/null 2>&1 || true |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Move the MiniStack harness into a composite action.
This block is doing most of the implementation work directly in the workflow, not just orchestration. That is well beyond the repo’s inline-step exception and will be harder to reuse and maintain than a Layer-2 composite action for the AWS CLI/mkcert/MiniStack/KMS setup flow.
Based on learnings, the inline-shell exception is only for narrowly scoped single-use steps with an inline rationale; as per coding guidelines, GitHub workflow files must implement the “three-layer composite actions architecture: primitives → composed actions → workflows.”
🧰 Tools
🪛 zizmor (1.25.2)
[info] 129-129: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
[warning] 221-221: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/kms-ministack-e2e.yaml around lines 126 - 281, The
workflow embeds heavy implementation (Install AWS CLI, Install mkcert and
provision TLS cert, Start MiniStack, Provision KMS key in MiniStack, Export
public key to PEM, Run KMS MiniStack chainsaw tests) directly in inline steps
instead of a reusable composite action; extract these steps into a Layer-2
composite action that encapsulates AWS CLI/mkcert/MiniStack/KMS setup and
teardown and replace the inline blocks with calls to that composite action,
exposing inputs for MINISTACK_IMAGE, MINISTACK_CERT_DIR, MINISTACK_ENDPOINT and
outputs like KMS_KEY_ARN/KMS_URI/KMS_PEM_FILE and an option to run the chainsaw
test so the workflow only orchestrates the composite action.
Sources: Coding guidelines, Learnings
Comment on lines
+68
to
+70
| ``` | ||
| awskms://<host:port>/<arn> | ||
| ``` |
There was a problem hiding this comment.
Add a language tag to this fenced block.
This unlabeled block trips MD040 and renders less consistently in documentation tooling. text would be sufficient here.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 68-68: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/chainsaw/cli/bundle-attestation-kms-ministack/README.md` around lines
68 - 70, The fenced code block containing the example URI
awskms://<host:port>/<arn> is unlabeled and triggers MD040; add a language tag
(e.g., text) to the opening fence so it becomes ```text to satisfy linting and
ensure consistent rendering in docs (update the fenced block around the
awskms://<host:port>/<arn> example in README.md).
Source: Linters/SAST tools
Comment on lines
+139
to
+149
| Triggers on push to `main` and `workflow_dispatch`. MiniStack runs as a plain | ||
| `docker run` step (not a `services:` container, which would start before the | ||
| `load-versions` step that resolves the pinned image). The workflow: | ||
|
|
||
| - Installs the pinned `mkcert`, runs `mkcert -install`, issues a `localhost` | ||
| cert, and exports `AWS_CA_BUNDLE`, then starts MiniStack with `USE_SSL=1`. | ||
| - Generates an SLSA predicate and builds an attested binary (public Sigstore), | ||
| so `verify-min-trust-verified` can exercise the full `verified` trust level. | ||
| - Sets `AICR_ATTESTED=true` when the attestation file is present; the chainsaw | ||
| step self-skips it when absent (e.g. forked PRs without OIDC). | ||
|
|
There was a problem hiding this comment.
Document the pull_request trigger here too.
This section describes only push to main and workflow_dispatch, but the workflow also runs for matching PR changes. Please document the current trigger set so contributor expectations match what CI actually does.
As per coding guidelines, documentation should “clearly distinguish current behavior from future intent” and prefer “what happens” over “what should happen.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/chainsaw/cli/bundle-attestation-kms-ministack/README.md` around lines
139 - 149, Update the "Triggers on push to `main` and `workflow_dispatch`"
paragraph to also list the `pull_request` trigger (and which branch patterns it
matches) so the README accurately reflects the workflow's current triggers; edit
the sentence that begins "Triggers on push to `main` and `workflow_dispatch`."
to read the full set (e.g., push to main, pull_request matching the same
branches, and workflow_dispatch), and add a brief clarifying note that this
describes current behavior (not future intent) so contributors know when CI will
run.
Source: Coding guidelines
lockwobr
force-pushed
the
feat/kms-ministack-e2e
branch
from
8f1d1ab to
aab418d
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/e2e (1)
10-10: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueUpdate comments to reflect all test directories.
The script now runs tests from three directories (
cli/,signing/,bundle-templates/) but the comments on lines 10 and 99 still reference only two.📝 Suggested fix
# WHAT THIS SCRIPT DOES: # 1. Builds the aicr binary via make build # 2. Locates the platform-specific binary -# 3. Runs chainsaw test --no-cluster on tests/chainsaw/cli/ and tests/chainsaw/bundle-templates/ +# 3. Runs chainsaw test --no-cluster on tests/chainsaw/cli/, signing/, and bundle-templates/-msg "Running chainsaw tests (CLI + bundle template tests)" +msg "Running chainsaw tests (CLI, signing, and bundle template tests)"Also applies to: 99-99
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/e2e` at line 10, Update the top-of-file comment that currently lists "cli/ and bundle-templates/" so it accurately reflects all three test directories by including "signing/" as well; similarly update the other matching comment later in the file (the comment around line 99) to list "cli/, signing/, and bundle-templates/" so both comments match the actual test directories the script runs.
♻️ Duplicate comments (1)
tests/chainsaw/signing/bundle-attestation-kms-ministack/chainsaw-test.yaml (1)
190-193:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStrengthen PEM-verify assertions to validate successful outcomes.
Line 192 currently asserts only field presence, so the step can pass even when verification reports failure values. Assert
truevalues (and includebundleAttested) to avoid false-green E2E results.Suggested patch
- name: verify-with-pem-public-key @@ - "${AICR_BIN}" verify "${WORK}/bundle" \ + OUTPUT=$("${AICR_BIN}" verify "${WORK}/bundle" \ --key "${PEM_FILE}" \ --certificate-identity-regexp "${AICR_IDENTITY_REGEXP:-}" \ - --format json + --format json) + echo "${OUTPUT}" + echo "${OUTPUT}" | grep -q '"checksumsPassed": true' + echo "${OUTPUT}" | grep -q '"bundleAttested": true' check: ($error == null): true - (contains($stdout, '"checksumsPassed"')): true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/chainsaw/signing/bundle-attestation-kms-ministack/chainsaw-test.yaml` around lines 190 - 193, The test's PEM-verify step currently only checks presence of fields; update the check block to assert successful boolean results and include bundleAttested. Replace or augment the existing checks (($error == null) and contains($stdout, '"checksumsPassed"')) with explicit assertions that checksumsPassed is true and verificationSucceeded is true (e.g., check contains('"checksumsPassed": true') and contains('"verificationSucceeded": true')) and also assert bundleAttested is present/true (e.g., contains('"bundleAttested": true') or contains('"bundleAttested"')). Ensure these checks are in the same check mapping as the original to prevent false-positive passes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/kms-ministack-e2e.yaml:
- Around line 46-55: The pull_request.paths array in the workflow is missing the
local action path '.github/actions/setup-build-tools/**' which this workflow
uses (the local action referenced as setup-build-tools); add
'.github/actions/setup-build-tools/**' to the pull_request.paths list so changes
to that action will trigger the workflow, ensuring the workflow's
pull_request.paths entry and the referenced local action (setup-build-tools)
stay in sync.
- Around line 56-64: Move the id-token: write permission out of the
workflow-level permissions block and add it under the specific job's permissions
(the kms-ministack-e2e job) so OIDC issuance is scoped to that job; update the
permissions block named permissions (remove id-token: write from the top-level
permissions) and add id-token: write under jobs.kms-ministack-e2e.permissions to
ensure only the signing job can mint OIDC tokens.
In `@tests/chainsaw/signing/bundle-attestation-kms-ministack/README.md`:
- Around line 1-4: The README uses the acronym "E2E" without defining it; update
the first mention of "End-to-end test suite" to "End-to-end (E2E) test suite" so
the acronym is expanded on first use (e.g., in the line containing "End-to-end
test suite for KMS-backed bundle signing and verification (`aicr bundle --attest
--signing-key awskms://...`)"). Ensure any subsequent uses of "E2E" remain
unchanged.
In `@tests/chainsaw/signing/bundle-attestation-kms-ministack/run.sh`:
- Around line 67-76: The MINISTACK_IMAGE default can drift from .settings.yaml;
update the run.sh logic for setting MINISTACK_IMAGE so that if MINISTACK_IMAGE
is empty it parses .settings.yaml to read testing_tools.ministack_image and uses
that value (falling back to the current hardcoded
"ministackorg/ministack:1.3.61" only if the YAML key is missing or parsing
fails); implement this in the same script near the existing MINISTACK_IMAGE
assignment, using a lightweight YAML extraction (e.g., grep/sed or yq) and
ensure MINISTACK_IMAGE remains exported and compatible with existing
MINISTACK_ENDPOINT and setup_tls usage.
In `@tools/setup-tools`:
- Around line 528-543: Update the comment to state that aws will be reinstalled
when UPGRADE=true (so it’s not strictly "only installed when absent"), and after
the pip install in the aws installation block (which uses python3 -m pip install
--user "awscli==${AWSCLI_VERSION}"), add a PATH verification step: use
command_exists aws (or check for ${HOME}/.local/bin/aws and $(python3 -m site
--user-base)/bin) to confirm the installed aws is on PATH and if not, call
log_warning with instructions to add the user's local bin to PATH (or advise
manual install), and keep using existing helpers like command_exists,
prompt_continue, and variables AWSCLI_VERSION and UPGRADE to locate the change.
---
Outside diff comments:
In `@tools/e2e`:
- Line 10: Update the top-of-file comment that currently lists "cli/ and
bundle-templates/" so it accurately reflects all three test directories by
including "signing/" as well; similarly update the other matching comment later
in the file (the comment around line 99) to list "cli/, signing/, and
bundle-templates/" so both comments match the actual test directories the script
runs.
---
Duplicate comments:
In `@tests/chainsaw/signing/bundle-attestation-kms-ministack/chainsaw-test.yaml`:
- Around line 190-193: The test's PEM-verify step currently only checks presence
of fields; update the check block to assert successful boolean results and
include bundleAttested. Replace or augment the existing checks (($error == null)
and contains($stdout, '"checksumsPassed"')) with explicit assertions that
checksumsPassed is true and verificationSucceeded is true (e.g., check
contains('"checksumsPassed": true') and contains('"verificationSucceeded":
true')) and also assert bundleAttested is present/true (e.g.,
contains('"bundleAttested": true') or contains('"bundleAttested"')). Ensure
these checks are in the same check mapping as the original to prevent
false-positive passes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: bb2ff1e2-0ac9-4882-bc8d-8054765beaa6
📒 Files selected for processing (13)
.github/actions/cli-e2e/action.yml.github/actions/load-versions/action.yml.github/workflows/kms-ministack-e2e.yaml.settings.yamltests/chainsaw/signing/bundle-attestation-ci/chainsaw-test.yamltests/chainsaw/signing/bundle-attestation-kms-ministack/README.mdtests/chainsaw/signing/bundle-attestation-kms-ministack/chainsaw-test.yamltests/chainsaw/signing/bundle-attestation-kms-ministack/run.shtests/chainsaw/signing/bundle-attestation/chainsaw-test.yamltests/chainsaw/signing/bundle-headless-oidc-ci/chainsaw-test.yamltools/check-toolstools/e2etools/setup-tools
Adds an end-to-end test that exercises the real awskms:// provider path for `aicr bundle --attest --signing-key` and `aicr verify` against an emulated AWS KMS, closing the automated-test gap for PR #1205's KMS signing feature. Key points: - MiniStack (https://ministack.org), an MIT-licensed token-free AWS emulator, replaces LocalStack, whose latest/stable images now refuse to boot without a paid license token. The image is pinned in .settings.yaml (never :latest). - sigstore's awskms client hardcodes https://, so MiniStack runs with USE_SSL=1 behind a mkcert localhost cert. Trust is handled across three planes: Go/aicr via the system store (mkcert CA), the aws CLI via AWS_CA_BUNDLE, and binary attestation against the untouched public Sigstore root. - The bundle and verify steps pass --certificate-identity-regexp so a binary attested by an e2e/build-attested workflow (not on-tag) is accepted. - mkcert is added as a pinned tool (.settings.yaml, load-versions, setup-tools, check-tools). - run.sh reproduces the CI flow locally; the chainsaw suite is gated by --selector 'requires=ministack'. Closes #1214
lockwobr
force-pushed
the
feat/kms-ministack-e2e
branch
from
aab418d to
cdec678
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/chainsaw/signing/bundle-attestation-kms-ministack/chainsaw-test.yaml`:
- Around line 185-192: The PEM verification assertion is only checking for the
existence of "checksumsPassed" rather than its value and omits "bundleAttested";
update the check block for the "${AICR_BIN} verify \"${WORK}/bundle\" --key
\"${PEM_FILE}\"" invocation so it asserts (contains($stdout, '"checksumsPassed":
true')): true and (contains($stdout, '"bundleAttested": true')): true (matching
the KMS verification pattern), ensuring both values are actually true.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 38790a21-b656-4988-89d0-1d5456d135a3
📒 Files selected for processing (13)
.github/actions/cli-e2e/action.yml.github/actions/load-versions/action.yml.github/workflows/kms-ministack-e2e.yaml.settings.yamltests/chainsaw/signing/bundle-attestation-ci/chainsaw-test.yamltests/chainsaw/signing/bundle-attestation-kms-ministack/README.mdtests/chainsaw/signing/bundle-attestation-kms-ministack/chainsaw-test.yamltests/chainsaw/signing/bundle-attestation-kms-ministack/run.shtests/chainsaw/signing/bundle-attestation/chainsaw-test.yamltests/chainsaw/signing/bundle-headless-oidc-ci/chainsaw-test.yamltools/check-toolstools/e2etools/setup-tools
Comment on lines
+185
to
+192
| # Verify using the PEM file path instead of KMS URI | ||
| "${AICR_BIN}" verify "${WORK}/bundle" \ | ||
| --key "${PEM_FILE}" \ | ||
| --certificate-identity-regexp "${AICR_IDENTITY_REGEXP:-}" \ | ||
| --format json | ||
| check: | ||
| ($error == null): true | ||
| (contains($stdout, '"checksumsPassed"')): true |
There was a problem hiding this comment.
Strengthen PEM verification assertion to check actual values.
Line 192 only asserts the checksumsPassed key exists, not that its value is true, and does not check bundleAttested at all. This allows failed verification ("checksumsPassed": false) to pass the test.
Compare to the KMS verification step at lines 145-146, which greps for '"checksumsPassed": true' and '"bundleAttested": true' to ensure the values are actually true. The PEM verification should match that pattern.
🔒 Proposed fix
# Verify using the PEM file path instead of KMS URI
- "${AICR_BIN}" verify "${WORK}/bundle" \
+ OUTPUT=$("${AICR_BIN}" verify "${WORK}/bundle" \
--key "${PEM_FILE}" \
--certificate-identity-regexp "${AICR_IDENTITY_REGEXP:-}" \
- --format json
+ --format json)
+ echo "${OUTPUT}"
+ # Assert the actual values are true, not just that the fields exist
+ echo "${OUTPUT}" | grep -q '"checksumsPassed": true'
+ echo "${OUTPUT}" | grep -q '"bundleAttested": true'
check:
($error == null): true
- (contains($stdout, '"checksumsPassed"')): true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/chainsaw/signing/bundle-attestation-kms-ministack/chainsaw-test.yaml`
around lines 185 - 192, The PEM verification assertion is only checking for the
existence of "checksumsPassed" rather than its value and omits "bundleAttested";
update the check block for the "${AICR_BIN} verify \"${WORK}/bundle\" --key
\"${PEM_FILE}\"" invocation so it asserts (contains($stdout, '"checksumsPassed":
true')): true and (contains($stdout, '"bundleAttested": true')): true (matching
the KMS verification pattern), ensuring both values are actually true.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds an end-to-end test that exercises the real
awskms://provider path foraicr bundle --attest --signing-keyandaicr verify, against an emulated AWSKMS (MiniStack) so the full provider-resolution, sign, and verify round-trip
runs in CI with no real AWS credentials and no paid license.
Motivation / Context
PR #1205 added
--signing-key <kms-uri>toaicr bundle --attest, but its unittests stub the KMS at the
kmsSignerseam, so the real provider path(
kms.Get("awskms://...")->SignerVerifier.PublicKey->SignMessage) wasnever exercised automatically. This closes that gap.
Fixes: N/A
Related: #1205
Closes: #1214
Type of Change
Component(s) Affected
tests/chainsaw/cli/bundle-attestation-kms-ministack), CI workflow, toolingImplementation Notes
latest/stableimages nowrefuse to boot without a paid license token (exit 55). MiniStack
(https://ministack.org) is MIT-licensed and token-free; its KMS supports the
asymmetric ECDSA P-256
SIGN_VERIFYkeys cosign'sawskms://signer needs.Pinned in
.settings.yaml(never:latest).awskmsclient hardcodeshttps://, soMiniStack runs with
USE_SSL=1behind a mkcertlocalhostcert. Trust spansthree planes: Go/
aicrvia the system store (mkcert CA), theawsCLI viaAWS_CA_BUNDLE, and binary-attestation verification against the untouchedpublic Sigstore root. mkcert is added as a pinned tool.
--certificate-identity-regexpso a binary attested by an e2e/build-attestedworkflow (not
on-tag) is accepted.run.shreproduces the same flow locally.Testing
Passed 1, Failed 0(KMS MiniStack E2E: PASSED),including tamper-detection and wrong-key-rejection negative steps.
yamllint -c .yamllint.yamlclean;bash -nclean onrun.sh,tools/setup-tools,tools/check-tools.Risk Assessment
Rollout notes: N/A. The new workflow triggers on push to
mainandworkflow_dispatch.Checklist
yamllint)git commit -S)