Repository navigation
Scaffold hardened Axum application Docker recipes - #401
ChristianPavilonis wants to merge 17 commits into
Conversation
Retain full vulnerability evidence in framework packaging checks without treating the fixture as an approved release. Keep strict defaults and reject report-only GHCR publication. Update inherited Bookworm packages and cover the policy boundary with regression tests.
Reject empty and non-string CVE/package identities in both scan policies. Record the successful hosted native builds and artifact/index handoff.
Replace Python helpers and tests with dependency-free Node.js tooling, update workflows and documentation, and preserve OCI artifact verification contracts. Accept valid repeated separators in image repository names and share Compose startup polling with regression coverage.
prk-Jr
left a comment
There was a problem hiding this comment.
🔧 Please resolve the integration conflicts against current main and validate the resulting revision before merging.
The reviewed PR head is 493644242f5839afbe5d37a032e08c13a381dc04; current main was checked at bc4aeb850c74ba982bdb4ad45309520b2ebb07b5. The merge check identifies conflicts in crates/edgezero-cli/src/generator.rs, .github/workflows/deploy-action.yml, and the location of docs/specs/aws-reference-deployment.md.
When resolving the generator conflict, keep the generated Rust version and the Docker builder digest aligned with main's toolchain upgrade. This PR introduces GENERATED_RUST_VERSION = "1.95.0" and a pinned 1.95 builder, while current main uses 1.98.1. Update the toolchain declaration and builder digest together; changing the image tag alone leaves the old digest selected. Rerun the affected checks on the integrated revision, including container preparation and native image execution.
No additional confirmed code defects were found in this review pass across the 29 changed files and relevant consumers.
Validation at the reviewed PR head: workspace tests passed (1,424 passed, one ignored), formatting and strict all-feature Clippy passed, native feature compilation and Spin WASM compilation passed, container helper tests passed including the prepared-fixture checks, and docs lint/format/build passed. Workflow and shell static checks passed, with a different local actionlint version from CI. These results do not validate a resolved merge with current main. Real Docker execution could not be repeated because the local daemon was unavailable; no current-head hosted checks were listed.
😃 The publisher independently recomputes committed source identities before registry login, verifies both architecture bundles, and prevents report-only scans from authorizing GHCR publication.
aram356
left a comment
There was a problem hiding this comment.
PR Review
Summary
Adds a hardened Axum Dockerfile scaffold, native amd64/arm64 container CI with an OCI artifact handoff, an app-owned GHCR publishing recipe, and Docker/Kubernetes/AWS deployment docs. The verification tooling is careful, but the generated image ignores SIGTERM, its runtime base means the shipped strict release gate rejects every build, and several integration hazards with current main merge without a textual conflict. This review adds to the existing conflict request rather than repeating it.
I built the fixture image from this head locally (native arm64) and measured its stop behavior and scan results; the numbers are in the inline comments.
Findings
Blocking
- 🔧 The container ignores SIGTERM, so every stop ends in SIGKILL (
crates/edgezero-cli/src/templates/root/Dockerfile.hbs:64, inline) - 🔧 The runtime base makes the shipped strict release gate unpassable (
crates/edgezero-cli/src/templates/root/Dockerfile.hbs:52, inline) - 🔧 New
stackpoplinks after the repository transfer; the generated README's docs link returns 404 (crates/edgezero-cli/src/templates/root/README.md.hbs:57, inline) - 🔧 The toolchain test breaks on the required Rust 1.98.1 bump (
crates/edgezero-cli/src/generator.rs:1141, inline) - 🔧 Merging would close #398, #399 and #400: GitHub lists all three as closing references for this PR, but the description says #399 and #400 must stay open and that #398's production acceptance is still outstanding. The description also calls the PR a draft, but it is not marked as one. Remove the #399/#400 links from the Development sidebar, decide explicitly whether this merge should close #398, and convert the PR to draft if it is not ready to merge.
- 🔧 The spec relocation breaks guide links (in addition to the existing conflict request): on main,
docs/specsmoved underdocs/superpowers/, whichdocs/.vitepress/config.mtsexcludes withsrcExclude: ['superpowers/**']. Onceaws-reference-deployment.mdmoves there, the relative links atdocs/guide/deploying-axum-containers.md:150anddocs/guide/deploying-axum-kubernetes.md:160,240point at pages the site no longer builds, and VitePress fails the build on dead links by default. Link to the file on GitHub instead, or drop those links. Also note that no hosted CI has run on49364424: GitHub does not runpull_requestworkflows while a PR has merge conflicts, so the integrated revision will be the first to exercise the new workflows on this head.
Non-blocking
- ♻️ Split this PR: 29 files and +4,325 lines combine about 340 lines of scaffold change (#398), about 3,000 lines of CI, verification and publishing tooling with tests (#399), and about 1,000 lines of deployment and AWS design docs (#400/#410). Each part has different reviewers, risk and acceptance criteria. Suggested split: (1) the Dockerfile,
.dockerignoreand README templates, generator tests and the Axum guide section; (2) native CI, the handoff helpers and the publishing recipe; (3) the design docs underdocs/superpowers/specs/. - ♻️ Keep point-in-time status out of committed docs: several pages record review-time state that will be wrong soon after merge. Examples: dated scan counts and a specific CI run link (
docs/guide/publishing-axum-containers.md:208-226); this PR's own heads and CI status (docs/specs/aws-reference-deployment.md:24-36); a developer's local, uncommitted worktree (issue-410-native-store-providers,docs/specs/aws-reference-deployment.md:32and:215); and issue-ownership notes in the Axum guide's "Packaging verification and limits" section. That belongs in the PR description and the issues. The docs should state the contract and its current limits. - ♻️ Keep the Kubernetes draft out of the published navigation (
docs/.vitepress/config.mts:61, inline) - ♻️ Replace the hand-written TOML parser (
scripts/axum_container_toml.mjs:3, inline) - ♻️ One implementation of the context identity, not three (
examples/container-publishing/release.yml:46, inline) - ♻️ Every source edit reinstalls cmake and rebuilds all dependencies (
crates/edgezero-cli/src/templates/root/Dockerfile.hbs:40, inline) - 🤔 The vulnerability gate can't see the Rust dependencies (
crates/edgezero-cli/src/templates/root/Dockerfile.hbs:46, inline) - 🤔 The container CI resolves a fresh dependency graph on every run (
scripts/prepare_generated_axum_container.sh:22, inline) - 🤔 Downstream-specific build steps in the framework guide (
docs/guide/adapters/axum.md:305, inline) - ⛏ Non-hermetic unit test fails on hosts without
readelf(scripts/tests/test_axum_container_artifacts.test.mjs:325, inline) - ⛏ Stale Python references after the Node migration (
scripts/axum_container_helpers.mjs:87, inline) - ⛏ Prerequisite lists disagree (
scripts/test_generated_axum_container.sh:7, inline) - ⛏ Absolute symlink targets never resolve (
scripts/axum_container_helpers.mjs:171, inline) - ⛏ Duplicate BuildKit pin for an unused builder (
.github/workflows/axum-container.yml:104, inline) - ⛏ Docs-only edits run both native container builds (
.github/workflows/axum-container.yml:18, inline) - ⛏ Drop bzip2/xz decompression of artifact input (
examples/container-publishing/verify_artifacts.mjs:189, inline)
CI Status
Local runs at 49364424:
- fmt: PASS
- clippy (
--workspace --all-targets --all-features -- -D warnings): PASS - tests (
cargo test --workspace --all-targets): PASS - Node helper tests: 50 pass, 1 fail on macOS (non-hermetic; see inline), 3 skipped by default. The 3 prepared-fixture tests pass with
AXUM_PREPARED_CONTEXTset. - shellcheck, actionlint and zizmor on the new workflows and scripts: clean
- Fixture
docker build(native arm64): PASS - Hosted CI on this head: none has run (blocked by merge conflicts)
dhruv8sh
left a comment
There was a problem hiding this comment.
PR Review
Summary
Solid, security-conscious Axum container packaging, but the branch is stale against main (missing #366 and #419), conflicts, and reintroduces stackpop references. Rebase and the items below are needed before merge.
😃 Praise
- Strong hardening: non-root UID 10001,
--read-only,cap-drop=ALL,no-new-privileges; digest-pinned base images and actions withpersist-credentials: false; toolchain verified beforerustup; strict scan policy and post-push digest verification for GHCR; Dockerfile/.dockerignore generated only for the Axum adapter, with good unit tests (mock-rustuptoolchain check, digit-leading app names).
Findings
Blocking
- 🔧 Stale base / merge conflict: branch lacks #366 (Rust 1.98.1, Node 24.20.0) and #419 (IABTechLab transfer);
git merge-treereports a conflict.GENERATED_RUST_VERSION = "1.95.0"ingenerator.rswhile main emitsrust 1.98.1. Rebase, bump to 1.98.1, re-resolve therust:1.98.1-bookworm@sha256:…digest inDockerfile.hbs, and update the hard-coded"1.95.0"in the new tests. - 🔧
stackpopreferences reintroduced (inline on the README template). Also indocs/guide/publishing-axum-containers.md(git dependency examples), issue/PR links in the Kubernetes guide anddocs/specs/aws-reference-deployment.md, and theorg.opencontainers.image.sourcefallbackstackpop/edgezeroinexamples/container-publishing/release.yml. Switch toIABTechLab/edgezero/iabtechlab.github.io/edgezero. - 🔧 Hard-coded Node 24.12.0 in workflows (inline).
- ❓ Draft docs in main nav (inline).
Non-blocking
- 🤔 Bespoke tooling scope:
scripts/axum_container_toml.mjsis a 182-line hand-rolled TOML parser andexamples/container-publishing/verify_artifacts.mjsis 524 lines of OCI/tar verification; withrelease.ymlandpublish.shthat is a lot of security-sensitive code for the framework to own as an "example". Consider a small TOML dep (e.g.smol-toml) for tests, or splitting the publishing recipe into its own PR. - 🤔 Missing
# syntax=directive for heredocRUN(inline). - ⛏ No dependency layer caching in the Dockerfile (inline).
- ⛏
rmSyncwithoutforce(inline). - 📝 Gate toolchain: clippy on 1.98.1 fails in untouched
edgezero-macros(inline_trait_bounds) because the branch still pins 1.95.0; resolved by the rebase.
CI Status
- fmt: PASS
- clippy: PASS on branch pin 1.95.0; FAIL on 1.98.1 (pre-existing lint in untouched crate, fixed by rebase)
- tests: PASS
- Docker/OCI workflows and Node helper tests: not run locally (require Docker/buildx)
Resolve merge conflicts, preserve combined CI coverage, align container scaffolding with Rust 1.98.1, and repair relocated documentation links.
…ps. Changes remain uncommitted; generated checkout artifacts were removed. Validation remains incomplete for native arm64 and hosted release flows.
…, paginated artifact selection, registry guards, simplified Node setup, focused tests and documentation cleanup. No new dependencies.
Review reconciliation at d7c4ee9I checked all 39 review threads and all five nonempty submitted review summaries against the current branch. No pending review was exposed by the API. This pass changes GitHub discussion state only, not source code or release policy. Replied and resolved
There are now 29 resolved threads and 10 open threads. Previously resolved threads already had explanatory replies, so I did not duplicate them or dismiss any reviewer's change request. The review-summary integration request is also addressed. Main was merged in 66c73e9, production Rust is 1.98.1, Node is aligned to 24.20.0, and GitHub reports MERGEABLE. The cited point-in-time documentation status was removed, internal drafts were taken out of published navigation, relocated documentation links were repaired, and the Kubernetes draft now uses GitHub-compatible warning syntax. These do not clear the separate future builder-digest maintenance or merge-acceptance requests below. Remaining inline requests
Remaining summary-only requests
Evidence and limits
|
prk-Jr
left a comment
There was a problem hiding this comment.
PR Review
Summary
Follow-up review at d7c4ee9e4a9db1d547272b3f13a6177f10098dba (base 87456c708ffe9edf86b3a0f8b06dfd15d60b07b5), covering the changes since my previous review at 1f244a7c. 4ea53d66 and d7c4ee9e do what they set out to do, and I found no new code defects in this delta. One earlier blocker is still open: it concerns PR metadata, not code.
Findings
Blocking
- 🔧 Merging would still close unfinished issues (existing finding, still unresolved): GitHub currently lists #398, #399 and #400 as closing references. The PR description says #399 and #400 must stay open and that #398 still has production acceptance outstanding (required-input startup failure, durable restart, readiness and bounded SIGTERM). Once the code threads are settled, merging would close all three automatically. Fix: remove #399 and #400 from the Development sidebar. Either change the #398 link to a non-closing reference or have an owner explicitly approve closing it on the packaging slice. The description also calls the PR a draft, but it is still marked ready for review. This is a metadata change only.
😃 Praise
- The Compose smoke test now checks
compose downbefore reporting PASS. It keeps the primary failure status if cleanup also fails and preserves diagnostics. I made the success-path teardown ignore failures, and both new teardown tests failed, so the regression is covered. - Artifact selection now reads every page of results, and a duplicate accepted artifact that only appears on a later page is rejected rather than silently selected.
Checked in this delta, no findings
node-version-file: .tool-versionswith an exact# v6.5.0pin comment. The standalone recipe's 24.20.0 pin is now kept in sync with.tool-versionsby a test.gh api --paginate --slurp | jq: the pinned publishergh(2.102.0) supports--slurp, and the default Actions bash shell usespipefail, so an API failure fails the step.- Registry readiness now fails the job if the registry never starts. Buildx setup uses
driver: docker, and only the handoff helper creates the pinned BuildKit builder. - The runtime-tar check now rejects
.gitat the root and in nested paths while still allowing.gitignore/.gitkeep. The GHCR preflight fails with a clear message, before any network call, when inline credentials are missing.
The other reviewers' open threads are not repeated here.
CI Status
- Hosted CI at
d7c4ee9e: all 33 checks passed, including native amd64/arm64 container execution and the handoff (run 41463920215, Node 24.20.0). - Local helper suite: 88 passed, 0 failed, 3 prepared-context tests skipped. Local Node was 24.12.0, not the declared 24.20.0.
- Not rerun locally: Rust fmt/clippy/tests (no Rust changes since my last review; hosted CI covers them) and Docker execution. GHCR publication/attestation, Kubernetes and AWS acceptance remain unexercised, as the PR states.
Review follow-through completedPushed 91cb3e6 and 827317a. Every one of the 39 review threads is now resolved, with an explanatory reply. The 26 threads already resolved before this audit retained their existing fix explanations; this audit resolved three previously fixed threads, then replied to and resolved the ten remaining threads after implementing or recording the agreed response. The owner's decisions also apply to the summary-only PR split/custom-verifier request and later scope request: retain this PR's current scope and verifier. The combined OIDC publisher is retained with its whole-job signing-authority risk explicitly accepted in the guide and thread reply. Review-summary text has no separate thread-resolution control; these decisions are recorded here and in the updated PR description. The remaining fixes include:
I removed the closing associations for #398, #399 and #400 and verified that all remain open. This preserves their outstanding acceptance criteria rather than closing unfinished work. The PR body now agrees with its ready-for-review metadata and explicitly distinguishes delivered preparation from production acceptance. Concrete new commit subjects and the proposed final squash subject replace progress prose as the description of this delivery; old tested revision hashes were preserved. Local validation at 827317a passed:
Current-head hosted checks, including native arm64 and the two-platform handoff, are still running. These are not claimed as passed. Protected GHCR/OIDC publication and production Kubernetes/AWS/runtime acceptance were not exercised or waived. Resolving review discussions does not dismiss reviewer verdicts, grant release approval or complete those acceptance criteria. |
prk-Jr
left a comment
There was a problem hiding this comment.
PR Review
Summary
Re-review at 827317aa6eb0797c62d8f1469d089b7a9df0b3c1 (base 87456c708ffe9edf86b3a0f8b06dfd15d60b07b5), covering 91cb3e65 and 827317aa. My earlier blocker is resolved: GitHub no longer lists any closing issues, and the description now says #398, #399, #400 and #410 remain open. I found no new defects. The two inline comments are non-blocking. Approving, which replaces my previous change request.
😃 Praise
- The remaining review threads were closed with real mechanisms, not just replies: the paired Rust version/builder digest constant, the derived-lock registry subset check, the platform-bound runtime material in the signed release record, and the checksum-verified cargo-audit source install. I disabled the subset check as a test, and both of its regression tests failed, so the check is covered.
- The cleared scan comes from the Debian 13 runtime and the patch-level lockfile updates, not from a waiver. The hosted native logs at this head show 0 High and 0 Critical OS findings and 0 Rust advisories on both architectures.
Verified in this delta
- The Dockerfile renders the builder digest from the same constant as
RUST_VERSION. The frontend is pinned, and the cache mounts still copy the binary out within the sameRUN..dockerignoreexcludes**/target. - The
cargo-audit0.22.2 checksum matches the checksum crates.io publishes for that version. - The moved trigger paths cover everything removed from
deploy-action.yml. The release recipe keeps actionlint, the pin gate, Zizmor and ShellCheck. - The derived-lock check applies only to the framework fixture. The app-owned recipe still uses
prepare_inputs.mjsand its committed lock. - An optional image
variantis carried from the config into the receipt and the index, with a check that they match.
Findings
Non-blocking
- 🤔 Guide links are pinned to pre-merge commits (inline,
docs/guide/adapters/axum.md:342; alsodocs/guide/deploying-axum-containers.md:132,150) - ⛏ Duplicated validation tool versions; the static checks gate the native builds (inline,
.github/workflows/axum-container.yml:51)
CI Status
- Hosted CI at
827317aa: all 33 checks passed, including native amd64/arm64 and the handoff (run 41481693509). - Local: fmt passed.
cargo test -p edgezero-clipassed. Clippy (-p edgezero-cli --all-targets --all-features -D warnings) passed on the pinned Rust 1.98.1. The container helper suite had 111 passed, 0 failed and 3 prepared-context tests skipped, on local Node 24.12.0. - Not exercised: local Docker, GHCR/OIDC publication, and Kubernetes/AWS acceptance, which the PR explicitly leaves outstanding.
aram356
left a comment
There was a problem hiding this comment.
PR Review
Summary
Re-review at 827317aa. I checked all 22 findings from my previous review against this head: 21 are fixed in code, and the PR split was declined as an owner decision, which I'm not raising again. The runtime now passes the strict image gate and stops cleanly, and the integration problems with main are resolved. Two new blocking problems remain: the smoke scripts' security assertions can't fail under macOS's default bash, and two tests assert workflow files whose changes never trigger them. prk-Jr's two open threads from yesterday (commit permalinks, duplicated tool pins on the critical path) are not repeated here.
Previous findings, verified
- Stop signal: on a native arm64 build of the fixture,
docker stopnow takes 1 s and exits 0 (previously 20 s, exit 137); the Compose example'sstoptakes 1 s and exits 0 (previously 31 s, exit 137). - Runtime base: the arm64 image is 64 MB (previously 177 MB) and has no shell. The hosted bundles from run 41481693509 report 0 High/Critical OS findings on both architectures.
- Floating runtime provenance: both hosted bundles record the resolved
cc-debian13:nonrootdigest in their BuildKit materials, and the handoff verified them. - Confirmed in code: IABTechLab links, the toolchain test's synthetic pin, no closing-issue associations, internal designs out of the published nav,
smol-tomlinstead of the hand-written parser, the shared context hash, cmake beforeCOPYplus cache mounts, the seeded fixture lockfile, the generic asset-stage example, the stubbedreadelftest, the Python references, absolute symlink resolution, the single BuildKit pin, docs paths out of the container triggers, and the removal of bzip2/xz.
Findings
Blocking
- 🔧 On macOS's default bash, the smoke scripts' security checks can't fail (
scripts/test_axum_container_deployment.sh:39-40, plus four lines inscripts/test_generated_axum_container.sh; inline) - 🔧 Tests assert workflow files whose changes never trigger them (
scripts/tests/test_axum_container_publishing.test.mjs:252and:232; inline)
Non-blocking
- 🤔 Strict release policy doesn't cover the Rust advisory inventory (
examples/container-publishing/verify_artifacts.mjs:307; inline) - ⛏ Leftover changes in
deploy-action.yml(.github/workflows/deploy-action.yml:107; inline) - ⛏ GNU-only checksum check (
scripts/test_axum_container_handoff.sh:103; inline)
📌 Out of Scope
- Main's
Cargo.lockalso pins tokio 1.53.1, which RUSTSEC-2026-0386 (AsyncFd::newand related constructors violate I/O safety) marks unsound; it is patched in 1.53.3. The fixture lock inherits it from there. Bump it in a separate change.
CI Status
Local runs at 827317aa:
- fmt: PASS
- clippy (
--workspace --all-targets --all-features -- -D warnings): PASS - tests (
cargo test --workspace --all-targets): PASS (1,554 passed, 1 ignored) - feature check (
--features "fastly cloudflare spin") and Spinwasm32-wasip2check: PASS - Node helper tests on macOS (Node 24.20.0,
/bin/bash3.2): 103 pass, 8 fail (explained inline), 3 skipped. The 3 prepared-context tests pass withAXUM_PREPARED_CONTEXTset. - shellcheck, actionlint and zizmor on the new and changed workflows and scripts: clean
- Native arm64 fixture build, HTTP,
docker stopand Composestop: PASS - Hosted CI at this head: all checks pass, including native amd64/arm64 and the handoff
Review follow-through at 6ef7f99Implemented the approved responses in 6ef7f99, replied to all seven newly open threads, and resolved them. Earlier resolved threads and their explanations were preserved.
The review summary's Tokio warning is real: the retained arm64 bundle now fails strict verification on that warning and still passes report-only. Zero vulnerabilities is not zero advisory warnings. The Tokio update remains separate, and the PR description now makes that distinction. Local validation passed: all 130 helper tests with fresh prepared context and no skips; Bash 3.2 focused regressions; native amd64 HTTP/TLS/Compose; CLI tests and strict all-feature Clippy; Rust formatting; docs formatting/lint/build; Bash syntax, ShellCheck, actionlint, recipe action pins and Zizmor; all 660 deployment contracts and the existing Rust build-cache action contracts. Two read-only workers reviewed the changes; their one policy-filter finding was fixed and covered before committing. Current-head hosted checks are running at https://github.com/IABTechLab/edgezero/actions/runs/42162617461. Preparation passed and both native jobs have started independently of static validation, confirming the intended scheduling change. No hosted result is claimed before completion. This does not dismiss reviewer verdicts, close outstanding acceptance issues, approve registry publication, change repository protection, or establish native macOS/GHCR/OIDC/Kubernetes/AWS production acceptance. |
prk-Jr
left a comment
There was a problem hiding this comment.
PR Review
Summary
Full re-review at 89d50968f4c958b1229230e63f8856e90ec83aba (base cf2c57b1), covering all 37 changed files rather than only the delta since my approval at 827317aa. The Rust generator, the Dockerfile template, the workflows and the verifier's OCI/scan handling hold up. Two small defects contradict claims this PR makes, so I'm moving from approve to request changes. Both fixes are a line or two.
😃 Praise
- The verifier is careful: digest regexes are anchored, tar members are checked against an allowlist of names, blob reads use
O_NOFOLLOW, exceptions must match exactly, and strict mode can't be bypassed through Trivy severity casing or a filtered cargo-audit report. - Native build jobs get no credentials or OIDC, every action is pinned to a full SHA, and no
${{ }}expression is interpolated into arun:block. The builder digest and Rust version come from one paired constant.
Findings
Blocking
- 🔧 Bash 3.2 cleanup aborts when the smoke fails before any container exists (
scripts/test_generated_axum_container.sh:52; inline) - 🔧 Lowercase or indented
FROMlines escape the pinned-base check (examples/container-publishing/prepare_inputs.mjs:42,verify_artifacts.mjs:450; inline)
Non-blocking
- 🤔 Compose-check diagnostics are lost in CI (
scripts/test_axum_container_deployment.sh:28; inline) - 📝 The docs could say SIGTERM is ignored outright. The app runs as PID 1 and installs only
tokio::signal::ctrl_c()(dev_server.rs:314), so SIGTERM does nothing until the runtime's kill timeout.STOPSIGNAL SIGINTis right for Docker and Compose, butdocker kill -s TERMor an orchestrator stop-signal override gets no reaction, then a SIGKILL. One sentence inaxum.mdanddeploying-axum-containers.mdwould make that explicit. - 🌱 Hardening (low; producer-controlled or fails closed):
hashContexthashesname\0bytes\0with no length prefix, so two different contexts can produce the same hash, and the native build never re-hashes the extracted context beforedocker buildx build. The separate git-archive hash still binds the source.readTaraccumulates PAX metadata without a total limit and re-spreads the object on every header, so time and memory grow without bound.extract/runtime-tardon't checksum first.- The derived-lock subset check (
axum_container_helpers.mjs:104) compares onlyregistry+sources, so an addedgit+/sparse+package passes. - Every "unsafe archive members" test case also sets
link, so removing the traversal checks would still pass. - A CRLF
.tool-versionsfails the toolchain check with a misleading "exact X.Y.Z" message.
CI Status
- Hosted at
89d50968: all 33 checks pass, including native amd64/arm64 and the handoff (run 42185282051). - Local on Rust 1.98.1: fmt passed,
cargo test -p edgezero-clipassed (227), clippy (-p edgezero-cli --all-targets --all-features -D warnings) passed. Helper suite: 127 passed, 0 failed, 3 prepared-context tests skipped, on local Node 24.12.0. actionlint, ShellCheck and the action-pin check were clean, and the docs build succeeded. - Not exercised: local Docker build, GHCR/OIDC publication, Kubernetes/AWS.
Review follow-through at 1a6ae86Pushed 1a6ae86. I reread all 49 threads, all nine nonempty review summaries and all conversation comments, with pagination checked. No pending review was exposed by the API; other reviewers' private drafts are not visible. The 46 previously resolved threads retain their existing explanations. The three newly open threads have individual replies for cleanup, base extraction and Compose CI diagnostics. Remaining points from the latest review
Earlier summary-only points remain handled by the previous reconciliation and 6ef7f999 follow-through. The owner retains this PR's scope, custom verifier and explicitly accepted whole-job OIDC authority. The Tokio upgrade remains separate, with strict Rust-advisory rejection and no automatic Rust waiver. I rechecked that #398, #399, #400 and #410 are open and that this PR has no closing-issue associations. Review-time design links, dated internal files, tool pins/checksums and the proposed concrete final squash subject retain their documented dispositions. No new follow-up issue was created for the optional hardening items. Validation and limitsThe committed diff byte-matches the tested working changes.
Thread resolution does not dismiss reviewer verdicts, grant release approval, close outstanding issue acceptance or authorize registry/cloud operations. Re-review is requested from prk-Jr, aram356 and dhruv8sh. |
prk-Jr
left a comment
There was a problem hiding this comment.
PR Review
Summary
Follow-up review at 1a6ae8610e12c9b6631e1b8f9ae2fcff889d52e8 (base cf2c57b1, unchanged since my full review at 89d50968). This pass covers the one new commit and rechecks my earlier findings. Both blockers are fixed, and each fix has a regression test that fails when the fix is reverted. The one new comment below is non-blocking. Approving, which replaces my previous change request.
😃 Praise
- Every fix comes with a test that catches its removal. I reverted each one locally and the matching test failed:
- Removing the handoff
--verify: 1 test fails. - Restoring the NUL-separated context hash: 2 tests fail.
- Dropping the root-receipt exclusion: 2 tests fail.
- Restoring the old
FROMregex: 4 tests fail. - Restoring the old
CONTAINERSloop under/bin/bash3.2: 4 cleanup tests fail. - Removing either
\rstrip inDockerfile.hbs: the Rust generator test panics.
- Removing the handoff
- The Compose evidence directory is kept tight: it must not already exist, only
*.logfiles andexit-statusare copied (not the rendered config or response bodies), and the original exit status is preserved.
My previous findings, verified
- 🔧 Bash 3.2 cleanup with an empty
CONTAINERS: fixed with the${CONTAINERS[@]+"${CONTAINERS[@]}"}expansion and covered by tests under Bash 3.2. - 🔧 Lowercase, indented or
--platformFROMlines: fixed. Preparation, fixture recording and the release record now share onebaseImages(). - 🤔 Compose diagnostics in CI: fixed. The handoff passes
--evidence-dir "$OUTPUT/compose"and the failure upload collects it. - 📝 SIGTERM wording: fixed in both guides.
- 🌱 Hardening: done: the length-prefixed context hash, the pre-build context re-check, separate unsafe-archive cases and CRLF handling. The cumulative PAX limit and the non-registry derived-lock entries are deferred with reasons, which is fine for producer-controlled inputs.
Findings
Non-blocking
- 🤔 The
FROMregex also matches heredoc lines (inline,examples/container-publishing/prepare_inputs.mjs:24)
CI Status
- Hosted at
1a6ae861: all 33 checks pass, including native amd64/arm64 and the handoff (run 42701012913). - Local on Rust 1.98.1: fmt passed,
cargo test -p edgezero-clipassed (227), and clippy (-p edgezero-cli --all-targets --all-features -D warnings) passed. - Helper suite: 156 passed, 0 failed, 3 prepared-context tests skipped, under both macOS
/bin/bash3.2 and Bash 5.2, on local Node 24.12.0. - Not exercised locally: Docker builds (hosted native jobs cover them), the CRLF awk under Debian
mawk, GHCR/OIDC publication, and Kubernetes/AWS.
| } | ||
|
|
||
| export function baseImages(dockerfile) { | ||
| return [...dockerfile.matchAll(/^\s*FROM\s+(?:--\S+\s+)*(\S+)/gim)].map(match => match[1]); |
There was a problem hiding this comment.
🤔 The new pattern also matches from lines inside heredoc bodies
This is a side effect of the regex I suggested last round. Because it ignores case and indentation, lines inside RUN <<EOF … EOF bodies are now read as base images, even though Docker doesn't treat them as instructions:
| Heredoc line | base_images |
|---|---|
from os import path (Python) |
["gcr.io/distroless/cc-debian13:nonroot","os"] |
FROM users (SQL) |
["a@sha256:1","users"] |
check_inputs then rejects the build with "Pinned base trace missing", which is a wrong error for a valid application Dockerfile. It only fails closed: the new pattern accepts a superset of the old one's inputs, so no unpinned base gets through. The scaffold template has no such lines, so CI is unaffected.
Fix (optional): skip heredoc bodies before matching, i.e. lines from a <<[-]?['"]?WORD marker through the line that is exactly WORD. Alternatively, note in the publishing guide that heredoc lines starting with from are treated as FROM instructions.
aram356
left a comment
There was a problem hiding this comment.
PR Review
Summary
Re-review at 1a6ae861, covering the three commits since my review at 827317aa. All five of my previous findings are fixed. The new strict Rust advisory gate exposes two problems at this head: the guide's local reproduction commands fail, and no scaffolded app can currently pass the gate, because of a dependency that is not in the image and that consumers can't upgrade themselves. prk-Jr's open heredoc note on prepare_inputs.mjs:24 is not repeated here; my FROM finding below is a separate case with the same root cause.
Previous findings, verified
- Bare
[[ … ]]assertions under Bash 3.2: fixed. Every one now has an explicit failure exit, andtest_axum_container_shell.test.mjsruns the shipping assertions under the bash onPATH. On macOS (/bin/bash3.2.57, Node 24.20.0) the helper suite now passes 156 tests with 0 failures; 8 failed at827317aa. - Tests asserting workflows outside their triggers: fixed. The suite now reads only
axum-container.ymlandrelease.yml, both inside that workflow's paths. - Strict policy and Rust advisories: implemented. Strict
verify,indexandrelease-recordblock Rust vulnerabilities andunsoundwarnings; inventory collection and framework packaging stay report-only. deploy-action.ymlleftovers: fixed. Relative to the merge base, only the comment pairing the validator pins remains.- GNU-only
sha256sum --strict: fixed with a direct hash comparison.
Findings
Blocking
- 🔧 The guide's local reproduction commands now fail (
docs/guide/publishing-axum-containers.md:51and:85; inline) - ❓ The strict Rust gate currently blocks every scaffolded app, over a crate the image doesn't contain (
examples/container-publishing/verify_artifacts.mjs:325-327; inline)
Non-blocking
- 🤔 Valid Dockerfiles are rejected as unpinned (
examples/container-publishing/verify_artifacts.mjs:338; inline)
📌 Out of Scope
- On main, upgrade rusqlite to a release that allows libsqlite3-sys 0.39 or later (RUSTSEC-2026-0459), and tokio to 1.53.3 or later (RUSTSEC-2026-0386). Main's
Cargo.lockstill has libsqlite3-sys 0.38.2 and tokio 1.53.1, the fixture lock inherits both, and both now block the strict gate.
CI Status
Local runs at 1a6ae861:
- fmt: PASS
- clippy (
--workspace --all-targets --all-features -- -D warnings): PASS - tests (
cargo test --workspace --all-targets): PASS (1,554 passed, 1 ignored) - feature check (
--features "fastly cloudflare spin") and Spinwasm32-wasip2check: PASS - Node helper tests on macOS (
/bin/bash3.2.57, Node 24.20.0): 156 passed, 0 failed, 3 skipped. The 3 prepared-context tests pass withAXUM_PREPARED_CONTEXTset. - shellcheck, actionlint and zizmor on the changed workflows and scripts: clean
- Native arm64 fixture build: PASS. The image is 64 MB, runs as
10001:10001withSTOPSIGNAL SIGINT, serves HTTP, anddocker stopexits 0 immediately. - Hosted CI at this head: all 33 checks pass, including native amd64/arm64 and the handoff (run 42701012913).
| ```sh | ||
| npm ci --prefix scripts --ignore-scripts | ||
| ./scripts/prepare_generated_axum_container.sh /tmp/axum-context | ||
| ./scripts/test_axum_container_handoff.sh build \ |
There was a problem hiding this comment.
🔧 The guide's local reproduction commands now fail
This command, and the import example at line 85, run the handoff helper with its default SCAN_POLICY=strict (scripts/test_axum_container_handoff.sh:15). Strict verification now also gates Rust advisories, and the fixture this guide prepares can't pass that gate: its lockfile is seeded from main's Cargo.lock, which currently resolves libsqlite3-sys 0.38.2 (RUSTSEC-2026-0459) and tokio 1.53.1 (RUSTSEC-2026-0386, unsound).
I downloaded the accepted arm64 bundle from this head's run 42701012913 and ran the same verification the helper performs:
verify --scan-policy strict -> exit 1: Artifact verification failed: Blocked Rust vulnerabilities; see rust-advisories.json
verify --scan-policy report-only -> exit 0
In build mode that strict check is the final step, so a local run fails only after the full build, smokes and scans. In import mode it runs first and fails immediately.
Fix: run both documented commands under the policy framework CI already uses:
SCAN_POLICY=report-only ./scripts/test_axum_container_handoff.sh build \
/tmp/axum-context/container-probe /tmp/axum-amd64 \
linux/amd64 "$(git rev-parse HEAD)" local| if (policy === 'strict') { | ||
| require(!findings.found, 'Blocked Rust vulnerabilities; see rust-advisories.json'); | ||
| require(!warnings.unsound?.length, 'Blocked Rust unsound advisories; see rust-advisories.json'); |
There was a problem hiding this comment.
❓ The strict Rust gate currently blocks every scaffolded app, over a crate the image doesn't contain
The vulnerability that fails strict verification at this head is RUSTSEC-2026-0459 in libsqlite3-sys 0.38.2 (patched in 0.39.0). In a prepared fixture it comes in only through rusqlite 0.40.2, which the Spin adapter's cli feature uses:
libsqlite3-sys v0.38.2
└── rusqlite v0.40.2
└── edgezero-adapter-spin v0.1.0
├── container-probe-adapter-spin v0.1.0
└── edgezero-cli v0.1.0
└── container-probe-cli v0.1.0
cargo tree -p container-probe-adapter-axum -i libsqlite3-sys reports that no package matches, so the binary in the image doesn't link it.
A consumer can't clear the finding either. rusqlite 0.40.2 requires libsqlite3-sys ^0.38.1, which excludes the fixed 0.39.0, so a fresh cargo generate-lockfile in a generated app still resolves 0.38.x. With no Rust waiver path, no app generated from this scaffold can release through the recipe until EdgeZero moves to a newer rusqlite. That applies even though its image doesn't contain the affected code.
The publishing guide states the whole-lockfile scope deliberately. Is this outcome intended? If not, two options gate on what the image actually ships:
- limit strict findings to packages reachable from the image's binary package through normal and build dependencies, using the
resolvegraph from thecargo metadataoutput the preparation already produces; or - build with
cargo auditableand audit the exported binary withcargo audit bin.
Either way, upgrading rusqlite on main clears today's instance; I've noted that as out of scope in the review summary.
| require(typeof inputs.toolchain === 'string' && /^rust [0-9]+\.[0-9]+\.[0-9]+$/m.test(inputs.toolchain), 'Toolchain trace missing'); | ||
| // The scaffold deliberately follows this runtime tag; all other bases remain pinned. | ||
| require(Array.isArray(inputs.base_images) && inputs.base_images.length && inputs.base_images.every(base => typeof base === 'string' && | ||
| (TAGGED_RUNTIMES.has(base) || /^[^\s@]+@sha256:[0-9a-f]{64}(?![\s\S])/.test(base))), 'Pinned base trace missing'); |
There was a problem hiding this comment.
🤔 Valid Dockerfiles are rejected as unpinned
baseImages() records every FROM operand as a base image, including operands that pull nothing. I ran check_inputs on base images taken from two Dockerfiles that use only pinned or allowlisted images:
| Added stage | base_images |
Result |
|---|---|---|
FROM runtime AS debug (an earlier stage) |
[…builder…, "gcr.io/distroless/cc-debian13:nonroot", "runtime"] |
Pinned base trace missing |
FROM scratch |
[…builder…, "scratch"] |
Pinned base trace missing |
Both fail closed, like prk-Jr's heredoc case on prepare_inputs.mjs:24, but the error is wrong, and a stage-reference or scratch final stage is a common pattern.
Suggestion: one change covers all three cases. Decide pinning from the BuildKit materials that resolved_base_images already loads and verifies. Require every pkg:docker/ material to carry digest=sha256: in its URI, except the allowlisted runtime tag; the builder and frontend materials already do, as in pkg:docker/rust@1.98.1-bookworm?digest=sha256:…. Materials list only the images BuildKit actually pulled, so stage names, scratch and heredoc text never appear. base_images can stay as the recorded trace.
Summary
.dockerignorefiles using the application's committed lockfile and declared Rust toolchain, a paired Rust builder version/digest, a digest-pinned Dockerfile frontend, BuildKit dependency caches, CA trust, numeric non-root identity and explicit container binding.Scope and issue relationships
Part of #391. Related to #398, #399, #400 and #410, with provider implementation tracked separately in #414. All four issues remain open for their outstanding acceptance. This PR delivers packaging preparation and the related CI/documentation preparation; it does not redefine the issues' completion criteria.
The current combined PR scope and custom OCI verifier are deliberately retained after review. The publishing recipe also retains its combined publication/attestation job. Its whole-job OIDC signing-authority risk is explicitly accepted and documented; credentials and OIDC are not available to the native build jobs.
Applications own their release artifacts, registry approval, credentials, configuration, secrets, storage policy and deployment. Trusted Server's images and releases remain downstream. Resolved review threads do not establish production acceptance or authorize merging unfinished issue criteria.
Main changes
gcr.io/distroless/cc-debian13:nonrootruntime. Runtime updates are pulled and tested; publication and rollback use the exact tested output digests.container-probepreparation stages framework sources deliberately. It labels its derived fixture lock, records both seed and prepared lock hashes, rejects changed registry name/version/source tuples, and keeps the separate application-owned committed consumer-lock contract intact.STOPSIGNAL SIGINTuses the existing graceful path, not bounded SIGTERM acceptance.SCAN_EXCEPTIONSdoes not enable an automatic waiver path in this sample workflow.cargo-audit0.22.2 is installed from a checksum-pinned source archive with its packaged lockfile. Rust advisory collection and framework verification remain report-only. Strict application verification rejects Rust vulnerabilities and unsound warnings as well as High/Critical image findings; no automatic Rust advisory waiver path is provided.docs/superpowers/specs/2026-10-14-deploying-axum-kubernetes.mdanddocs/superpowers/plans/2026-10-14-aws-reference-deployment.md.Review follow-through at
1a6ae861Committed and pushed the approved remaining fixes in 1a6ae861. The resolution comment accounts for the latest summary-only points and approved deferrals.
Validation of the tested working changes committed here:
All 49 review threads have explanatory responses and are resolved. Re-review is requested from prk-Jr, aram356 and dhruv8sh. This does not dismiss their verdicts or close any issue acceptance.
Review follow-through at
6ef7f999blob/mainguide links. Strict application verification now rejects Rust vulnerabilities andunsoundwarnings. Both policies require unfiltered warning-category selection. Framework collection/verification remains report-only; OS exceptions cannot waive Rust findings, and the Tokio update remains separate.Validation of these changes:
6ef7f999, run 42162617461. This is historical evidence, not current-head verification.Native macOS userland, protected GHCR/OIDC publication and cloud/runtime production acceptance were not exercised. Two read-only workers checked shell portability and CI/policy behavior. The policy review found an informational-warning filter gap; it was fixed and covered before committing. No repository protection settings, dependency upgrade or Rust waiver path was added.
Earlier validation
Local checks at implementation commit
827317aa6eb0797c62d8f1469d089b7a9df0b3c1, including unchanged relevant inputs tested immediately before committing:LC_ALL=C, matching the test's deterministic sorting contract.SCAN_POLICY=strict.sha256:cfc14c62b697225036d242674494d4b10e80c5a9525aa8d42c81489c1234aac5. This is point-in-time evidence, not a permanent vulnerability-free claim. The retained hosted inventories also include an unsound Tokio warning, distinct from the zero-vulnerability count. Current strict verification rejects that warning.827317aa, run 41481693509. These earlier results do not substitute for current-head verification above.The local handoff used the pinned BuildKit image and local Buildx 0.38.1. Hosted workflows pin Buildx 0.37.0. Both must retain the observed resolved-material/config-digest evidence and exact tested child identities. No QEMU execution substitutes for native arm64 acceptance.
Remaining acceptance outside this preparation
Packaging preparation is not production certification. No end-client-delivery guarantee, multi-writer embedded-store support, Kubernetes/EKS acceptance, Fargate acceptance, real AWS provider acceptance or identity/CAS certification is claimed.
Use a concrete final squash subject such as
Scaffold and verify hardened Axum application containers; earlier implementation-progress commit subjects are not release-status evidence.