Repository navigation
test(enclave): make the PCR and nil-attester tests check what they claim; drop curl from the EIF builder setup - #109
Merged
Conversation
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.
There was a problem hiding this comment.
🟢 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.
5 of 6 tasks
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
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
enclave/proofs_test.go:AttestationDoc.PCRsis a struct value, and testy'scheck.NotNilreports nil only for channels, funcs, interfaces, maps, pointers, slices and unsafe pointers, socheck.NotNil(t, attestationDoc.PCRs)could never fail.TestGenerateAttestationWithMockandTestGenerateAttestationWithEncryptedBidscheck each PCR field right after that call, so the call is removed there. InTestGenerateAttestationWithMixedBidTypesit was the only PCR check, so it becomescheck.NotEqual(t, enclaveapi.PCRs{}, attestationDoc.PCRs).TestGenerateAttestationandTestGenerateKeyAttestationcheck only the "enclave attester is nil" error; the success paths are in the...WithMocktests. They are renamedTestGenerateAttestation_NilAttesterandTestGenerateKeyAttestation_NilAttester, the patternTestNewKeyManager_NilAttesteralready uses.enclave/scripts/setup-nitro-instance.shno longer checks for or installs curl. Nothing on the builder instance runs curl:eif-build.ymlruns this script andbuild-eif-ci.shthere 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.Pre-merge checklist
go test -count=1 ./...is green. With the mock enclave returning no PCRs,TestGenerateAttestationWithMixedBidTypesfails on the new check (expected want != gotwith zero PCRs on both sides), while the previouscheck.NotNilpassed under the same mock.mise run //:lintreports 0 issues andgo vet ./...is clean.shellcheckoutput for the script is unchanged (only the existing SC1091 note for the runtime-sourced/etc/os-release), andshfmt -i 4 -sr -dreports nothing on the changed line.install_dependenciesrun with stub commands under bash 3.2 and 5.3 logs✓ aws already installedand installs nothing whenawsis onPATH, runsdnf install -y awscliwhen it is missing, and logs no curl line in either case..mise/tasks/testrunsgo test ./... "$@", no workflow passes-run, andgit grep -wfinds no other reference.go list -deps ./enclave/...is identical before and after, and the enclave binary built with thedocker.ymlflags plus-buildvcs=false -trimpathis byte-identical before and after for amd64 and arm64.Post-deploy/apply verification
mainafter merge passes, includingupdate-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.)update-pcrsentry for the merge commit keeps PCR1 equal to the previous entry. PCR0 and PCR2 change on everymaincommit, including CI-only ones, becausego buildstamps the commit into the enclave binary's build info. Check withjq -r '.pcr_sets[-2:][] | "\(.commit_sha[:7]) \(.pcr1[:16])"' validation/pcrs.jsononmainafter the PCR commit lands. (pcrs.json at 4aed97e: e2d4a95 pcr1=3b4a7e1b5f13c5a1, 18f0326 pcr1=3b4a7e1b5f13c5a1, unchanged; PCR0 and PCR2 changed as expected.)