Skip to content

test(enclave): make the PCR and nil-attester tests check what they claim; drop curl from the EIF builder setup - #109

Merged
nickpell merged 3 commits into
mainfrom
nick/proofs-tests-and-builder-deps
Oct 16, 2026
Merged

nickpell merged 3 commits into
mainfrom
nick/proofs-tests-and-builder-deps

Conversation

@nickpell

@nickpell nickpell commented Oct 16, 2026 •

Copy link
Copy Markdown
Member

Summary

  • enclave/proofs_test.go: AttestationDoc.PCRs is a struct value, and testy's check.NotNil reports nil only for channels, funcs, interfaces, maps, pointers, slices and unsafe pointers, so check.NotNil(t, attestationDoc.PCRs) could never fail. TestGenerateAttestationWithMock and TestGenerateAttestationWithEncryptedBids check each PCR field right after that call, so the call is removed there. In TestGenerateAttestationWithMixedBidTypes it was the only PCR check, so it becomes check.NotEqual(t, enclaveapi.PCRs{}, attestationDoc.PCRs).
  • TestGenerateAttestation and TestGenerateKeyAttestation check only the "enclave attester is nil" error; the success paths are in the ...WithMock tests. They are renamed TestGenerateAttestation_NilAttester and TestGenerateKeyAttestation_NilAttester, the pattern TestNewKeyManager_NilAttester already uses.
  • enclave/scripts/setup-nitro-instance.sh no longer checks for or installs curl. Nothing on the builder instance runs curl: eif-build.yml runs this script and build-eif-ci.sh there over ssh, and neither calls it. The only curl call in the workflow, the ORAS download in "Install ORAS", runs on the GitHub-hosted runner. curl comes preinstalled on the AL2023 builder AMI (Build EIF run 38768298414 logs ✓ curl already installed), so no install step goes away. jq was dropped from the same list for the same reason in ci: drop the unused jq dependency from the EIF builder setup script #107.
  • Each change is its own commit. None of them is an input to the enclave binary or the EIF.

Pre-merge checklist

  • Tests pass: go test -count=1 ./... is green. With the mock enclave returning no PCRs, TestGenerateAttestationWithMixedBidTypes fails on the new check (expected want != got with zero PCRs on both sides), while the previous check.NotNil passed under the same mock.
  • Lint passes: mise run //:lint reports 0 issues and go vet ./... is clean. shellcheck output for the script is unchanged (only the existing SC1091 note for the runtime-sourced /etc/os-release), and shfmt -i 4 -sr -d reports nothing on the changed line.
  • Behaviour: install_dependencies run with stub commands under bash 3.2 and 5.3 logs ✓ aws already installed and installs nothing when aws is on PATH, runs dnf install -y awscli when it is missing, and logs no curl line in either case.
  • No test filter names the old test names: .mise/tasks/test runs go test ./... "$@", no workflow passes -run, and git grep -w finds no other reference.
  • Enclave binary unchanged: go list -deps ./enclave/... is identical before and after, and the enclave binary built with the docker.yml flags plus -buildvcs=false -trimpath is byte-identical before and after for amd64 and arm64.
  • GitHub Actions checks pass: Go test, lint and docker-build (run 38907322819) and Ratchet Lint (run 38907322122).
  • Diff contains no unintended changes: two files, 4 insertions and 6 deletions.

Post-deploy/apply verification

  • The first Build EIF run on main after merge passes, including update-pcrs, and its "Setup Nitro instance" step logs no curl line and ends with ✓ Setup complete!. (Build EIF run 38911967996 on 18f0326: Build EIF on Nitro EC2 and Update PCR validation file both succeeded. Setup ended ✓ Setup complete! at 14:01:28 UTC with no curl line; the only later curl is the runner-side download step. PCR commit 4aed97e followed, and its own Build EIF run 38913265785 was skipped.)
  • The update-pcrs entry for the merge commit keeps PCR1 equal to the previous entry. PCR0 and PCR2 change on every main commit, including CI-only ones, because go build stamps the commit into the enclave binary's build info. Check with jq -r '.pcr_sets[-2:][] | "\(.commit_sha[:7]) \(.pcr1[:16])"' validation/pcrs.json on main after the PCR commit lands. (pcrs.json at 4aed97e: e2d4a95 pcr1=3b4a7e1b5f13c5a1, 18f0326 pcr1=3b4a7e1b5f13c5a1, unchanged; PCR0 and PCR2 changed as expected.)

AttestationDoc.PCRs is a struct value, which testy's NotNil never
reports as nil. Two tests already check each PCR field, so drop the
call there. TestGenerateAttestationWithMixedBidTypes had no other PCR
check, so compare against the zero PCRs value instead.
…ttester

TestGenerateAttestation and TestGenerateKeyAttestation check only the
"enclave attester is nil" error; the success paths are in the WithMock
tests. Add the _NilAttester suffix that TestNewKeyManager_NilAttester
already uses.
Nothing on the builder instance runs curl. The workflow runs this
script and build-eif-ci.sh there over ssh, and neither calls curl. The
only curl call in eif-build.yml, the ORAS download, runs on the GitHub
runner. jq was dropped from the same list for the same reason in #107.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The limited test and setup changes have no identified blocking issues.

0 open findings

What changed in this PR

This PR strengthens enclave test assertions and removes an unused builder dependency, continuing the cleanup from #107 without changing production enclave code.

Changes:

  • Remove ineffective PCR nil checks and add a nonzero-PCR assertion.
  • Rename nil-attester tests to clarify their scope.
  • Stop checking for or installing curl on the EIF builder.
File Description
enclave/​scripts/​setup-nitro-instance.sh Remove curl from builder dependencies.
enclave/​proofs_test.go Improve PCR assertions and clarify nil-attester test names.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@nickpell
nickpell merged commit 18f0326 into main Oct 16, 2026
5 checks passed
@nickpell
nickpell deleted the nick/proofs-tests-and-builder-deps branch October 16, 2026 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants