fix(ci): build SecureBoot signed AMT installers#5
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.
Important Issues (1)
- [native-codex]
.github/workflows/compose-signed-installer.yml:139-148— The newsecureboot-installerpath (subcommand name,/secureboot/{uki-signing-key,uki-signing-cert,pcr-signing-key}.pemmount convention, and thesecureboot: true/signing systemd-bootlog markers the script now hard-greps for) has never actually executed.compose-signed-installer.ymlisworkflow_dispatch-only, so opening this PR did not run it, and none of the workflow's last 5 runs are at this PR's head SHA (74d6523) — the only run cited in the PR body (29136475511) is the old, buggy code path. The PR's "Validation" section covers YAML parse /bash -n/git diff --checkonly, not a live imager invocation.- I checked the mount path and filenames against upstream Talos docs and they do match the
imagerSecureBoot profile convention (/secureboot/uki-signing-key.pem,/secureboot/uki-signing-cert.pem,/secureboot/pcr-signing-key.pem), so the design looks right. But the two literalgrep -qchecks (secureboot: true,signing systemd-boot) are exact-string bets on this imager build's log wording, and this is exactly the kind of signing-pipeline change where a wrong guess fails either open (falls through to publish) or closed (blocks all future signed builds) with no prior evidence either way. - Recommend a
workflow_dispatchdry run against this branch (with the three new secrets populated) before merge, to confirm the tool acceptssecureboot-installer, the log lines actually appear verbatim, and_out/installer-amd64-secureboot.taris produced — otherwise this ships unverified into the exact signing path that caused the original SecureBoot canary failure.
- I checked the mount path and filenames against upstream Talos docs and they do match the
Suggestions (1)
- [pr-review-toolkit]
.github/workflows/compose-signed-installer.yml:35-37,70—talos_version/TALOSis no longer used anywhere in the script body now thatghcr.io/siderolabs/imager:${TALOS}was replaced by the derived$IMAGER. Either drop the now-dead input/env, or use it to sanity-check that the derived imager tag actually matches the requested Talos version.
Strengths
- Solid secret hygiene on the new SecureBoot key handling: PEM files are written to a
mktemp -d'd directory (tmpfs via/dev/shmwhen available) underumask 077(mode 600), mounted:rointo the imager container, and cleaned up via anEXITtrap set before any secret material is written. - The added pre-flight checks (all three new secrets present,
openssl pkey/openssl x509sanity-parsing the key/cert material before use) fail closed early rather than surfacing an opaquedocker runerror. - The fail-closed
grep -Eq 'secureboot: false|unsigned\.uki'guard directly encodes the root-cause signature from the cited failing run (29136475511), which is the right instinct even though it's unverified against the new code path (see Important above).
Recommended Action
- Run a
workflow_dispatchsmoke test on this branch to confirm the new imager invocation and log-marker assertions actually match reality before relying on this for production signed installers. - Optionally clean up the now-unused
talos_versioninput.
reviewed head: 74d6523
Author
|
Addressed in #6. Live smoke results against the real ct6 private inputs on the x86 devbox, using disposable keys and no push:
The first smoke run found that Talos v1.13 rejects PKCS#8 PCR keys. #6 normalizes both RSA inputs to PKCS#1, verifies the UKI key/cert pair, removes the dead talos_version/TALOS fields, and passed a second full compose starting from PKCS#8 inputs. |
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
Fixes the deployable AMT installer compose workflow so an image tagged
*-signedis actually a Talos SecureBoot installer.Changes:
ghcr.io/blockcast/imager:<base tag>instead of the stock Sidero imagersecureboot-installerinstead ofinstaller/secureboot_out/installer-amd64-secureboot.tarsecureboot: falseorunsigned.ukiRoot Cause
The previous compose workflow overlaid the signed AMT extension with
ghcr.io/siderolabs/imager:${TALOS} installer. The run log for29136475511showedsecureboot: falseandcopying .../unsigned.uki, which matches the data-4 SecureBoot canary failure.Required Secrets
This workflow now requires these repo secrets, populated from the Vault SecureBoot PKI path:
TALOS_SECUREBOOT_UKI_SIGNING_KEY_PEMTALOS_SECUREBOOT_UKI_SIGNING_CERT_PEMTALOS_SECUREBOOT_PCR_SIGNING_KEY_PEMValidation
ruby -e 'require "yaml"; YAML.load_file(...)'yqand ranbash -ngit diff --check