Add fail-closed ARM64 self-update artifact selection - #91
Conversation
CherylSnowVeil
left a comment
There was a problem hiding this comment.
Review — ARM64 self-update artifact selection (9c1ebaf)
Initial review of the current head. I traced the implementation against the release workflow contract and the real published releases rather than relying on the PR description.
What I verified
- Release contract alignment.
.github/workflows/release.ymlproducescb.exe(amd64),container-bin-<tag>-windows-{amd64,arm64}.zip, and a three-entrySHA256SUMS(cb.exeline is the amd64 raw binary), withattest-build-provenancecoveringcb.exeand*.zip.validateRelease,checksumManifestNames, and the attestation subject-digest check match this contract exactly. - Real releases. Published v1.0.0/v1.1.0 have only
{cb.exe, amd64 zip, SHA256SUMS}— thechecksumLayoutLegacyAMD64path covers them (including the env-gated Windows integration test, which still pins v1.1.0, an immutable legacy release). An arm64 build against those releases fails closed with "missing required asset …windows-arm64.zip". - Selection (
selfupdate.go). Arch gate happens before any network access; companion arm64 asset metadata (name/size/canonical URL) is validated even on amd64 so a malformed companion asset can't slip through — covered byTestPlanRejectsUnsafeCompanionARM64MetadataOnAMD64. - Staging (
stage.go). Downloads the selected artifact (arm64 zip or amd64cb.exe) + manifest with the existing canonical-URL/redirect/size guards;Staged.Cleanuplayout guard extended correctly (base-name check, still confined to the staging-prefixed dir). - Verification (
verify.go). Plan re-validation is consistent with staging's; checksum manifest verified before attestation;gh attestation verifybound to repo/workflow/ref/predicate with subject digest match; pre/post attestation re-hashes close the mutation window; extraction re-validatesos.SameFile, enforces the exact{cb.exe, LICENSE, README.md}entry set, rejects duplicates/encryption/unsafe methods/non-regular entries, bounds total uncompressed size (overflow-safe ordering), enforcesn == UncompressedSize64on top of the zip CRC, createscb.exeO_EXCL|0600with the private DACL, then re-hashes both the extracted exe and the archive. ThechecksumLayoutunexported field also means externally constructedPlanvalues fail closed inStage/Verify. - CI is green on all jobs, including native
Test and build (Windows ARM64),Format, vet, test (Linux),govulncheck, and reproducibility. - Docs (README, security-model, release-matrix, roadmap files) accurately describe the new behavior and correctly keep the ARM64 support claim gated on real Docker Desktop E2E.
Finding
- 🟢 [nit]
internal/selfupdate/verify_test.go—arm64TestArchivesilently dropsextraskeys outside its fixed write list. The helper mergesextrasintoentriesbut only writes names present in{"cb.exe", "LICENSE", "README.md", "../escape"}. Any future test passing a different extra name (e.g. to exercise the duplicate-entry or unsafe-method rejection) would produce an archive that doesn't contain it, leading to a confusing failure or an accidentally empty fixture. Consider iterating theextrasmap after the canonical entries (ort.Fatalon keys outside the write list).
Summary
The implementation is consistently fail-closed, keeps the amd64 path byte-compatible with legacy two-entry manifests, threads architecture selection through GOARCH without expanding the trust surface, and the new verification path (attest-before-extract, exact archive layout, bounded decompression, re-hash chain) is solid. Docs are accurate. One test-helper nit above; nothing blocking.
|
Addressed CherylSnowVeil's latest test-helper nit in signed commit ca9a6fd. arm64TestArchive now writes every arbitrary extra entry after the canonical entries in deterministic sorted order, while still allowing canonical-entry overrides. The unexpected-entry regression now uses unexpected.bin, proving non-hardcoded extras are present. Full race tests, vet, and release-style version checks pass locally. |
|
Conflict resolved in signed merge commit 8278e26 after PRs #88 and #89 advanced main. The resolution keeps their merged roadmap updates, PR #91's ARM64 status, and the ca9a6fd test-helper fix. Full race tests, vet, version checks, both Python suites, and a full Windows ARM64 cross-build pass on the merged tree. |
Summary
GOARCH, while preserving the rawcb.exeselection and asset names for amd64cb.exefrom the canonicalcb.exe/LICENSE/README.mdlayoutThis unit starts from current
mainand is independent of the unmerged replacement transaction in #90.Validation
go test -race ./...go vet ./...go test -exec=/bin/true ./...go test -exec=/bin/true ./...internal/selfupdatetest binary, including ARM64 selection, staging ACLs, three-entry manifest validation, provenance boundary, exact archive extraction, and unsafe-entry rejection