fix(ci): harden SecureBoot key validation#6
Merged
Conversation
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Looks good. This directly follows up on my prior review (74d6523, review 4676641982) and resolves both points raised there.
Suggestions (1)
- [native-codex]
.github/workflows/compose-signed-installer.yml— The "live smoke evidence" was run by extracting and executing the compose shell on an x86 devbox, not via an actualworkflow_dispatchrun of this workflow on this branch/head. That's a meaningfully faithful test (same imager/base/extension digests, same script), and it's exactly the run that caught the real PKCS#8-vs-PKCS#1 bug this PR fixes, so I'm treating the prior "unverified in CI" concern as resolved. Still, aworkflow_dispatchrun oncodex/secureboot-smoke-hardeningitself (not just the extracted shell) would additionally confirm thechromium-buildrunner env (docker socket, GHCR cross-repo pull,/dev/shmavailability) behaves the same as the devbox — worth doing opportunistically before this becomes the production signing path, not a blocker.
Strengths
- The PKCS#8→PKCS#1 normalization (
openssl rsa -traditional) is scoped correctly: it runs before the key is used for-pubout/-check, and underset -euo pipefaila non-RSA or unparseable key aborts the job immediately (matches the intent in the added comment — fail loud here, not with an opaque imager error downstream). - The new UKI key/cert match check is a correct construction: derive the pubkey from the private key (
openssl pkey -pubout) and compare it against the pubkey extracted from the cert (openssl x509 -pubkey) viacmp -s. Mismatch aborts viaset -ebefore the imager ever runs, and the temp.pubfiles are cleaned up (either by the explicitrm -fon the match path, or by the pre-existingEXITtrap on$secureboot_dirifcmpfails first). talos_version/TALOSremoval is clean — confirmed no remaining references anywhere else in the file, so this isn't dead-code-adjacent breakage.- Verified secret hygiene carries over unchanged from the prior review (tmpfs-backed
mktemp -d,umask 077,:romount,EXITtrap set before secrets are written).
Recommended Action
- Optional: exercise this exact branch via
workflow_dispatchin CI (not just the devbox-extracted shell) before it becomes the default production signing path.
reviewed head: 4a32797
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
Follow-up to #5 and Ally review 4676641982.
Live smoke evidence
Ran the exact ct6 SecureBoot compose on the x86 devbox with disposable keys and no publish:
The first smoke run also proved Talos rejects PKCS#8 PCR keys. The updated workflow normalizes PKCS#8 RSA inputs to PKCS#1, and a second full compose passed through that exact path.
Validation