feat(eif): sign, repack, and resign EIFs via dedicated [eif] keyset - #688
feat(eif): sign, repack, and resign EIFs via dedicated [eif] keyset#688jmt-lab wants to merge 4 commits into
Conversation
aca378d to
8e54b42
Compare
ginglis13
left a comment
There was a problem hiding this comment.
just an initial pass, mostly nitpicking overly verbose comments
| // Placeholder variant name; the upstream repo carries `aws-nitro-eks-2` | ||
| // and similar EIF variants under `variants/`. Any variant whose | ||
| // `image-format = "eif"` will exercise the same path. | ||
| const EIF_VARIANT: &str = "aws-nitro-eks-2"; |
There was a problem hiding this comment.
We should change this and remove mention of upstream repo which isn't factual and doesn't mean anything here
Extend eif-builder with EifSectionSignature (0x04) covering PCR0 via
COSE_Sign1 ECDSA (ES256/ES384). rpm2eif reads a dedicated `[eif]`
section in Infra.toml and picks local (aws-lc-rs) or KMS signing from
its shape; absent section builds unsigned. Replaces the earlier sbkeys
autodetect so the Nitro-hypervisor trust root is decoupled from the
UEFI Secure Boot code-signing key (distinct algorithms, rotation
cadence, and trust domain). Signing is orthogonal to UEFI Secure Boot.
Wire format matches aws-nitro-enclaves-image-format: CBOR Array(1) of
Map { signing_certificate: PEM, signature: COSE_Sign1 } with a PcrInfo
payload for PCR0 = SHA-384(48 zero bytes || SHA-384(kernel || cmdline
|| ramdisks)), so nitro-cli describe-eif populates PCR8.
Adds two repack-variant capabilities:
* In-tree EIF repack (eif2eif): rewrites CA bundle / root.json in the
EIF rootfs, rebuilds erofs + dm-verity, and produces a freshly signed
.eif with a cmdline pinned to the new verity root hash.
* Guest EIF resign during host repack: img2img walks guest-images install
paths and resigns every *.eif in place via a new `eif-builder resign`
subcommand (byte-preserves kernel/cmdline/ramdisk, rewrites only the
signature section + header CRC).
Highlights:
* new eif-builder/src/signer.rs (Signer trait, LocalSigner, KmsSigner)
* CLI: --signing-cert + --signing-key (local) or --kms-key-id (KMS),
enforced via clap ArgGroup; new `resign` subcommand
* typed KMS errors (KmsAccessDenied, KmsKeyDisabled, KmsInvalidKeyState)
* ECDSA DER->raw validates SEQUENCE length and rejects trailing bytes
* shared eif-sign-helper sourced by rpm2eif, eif2eif, and img2img;
hard-fails on mounted cert without a signing backend
(opt-out: EIF_ALLOW_UNSIGNED_WITH_CERT=1)
New deps: aws-lc-rs, coset, ciborium, pem, x509-cert.
Tests: PCR0 spec conformance, COSE payload shape, end-to-end signature
verification, DER parser edge cases, resign round-trips, and the
eif-sign-helper discovery matrix.
Mirror the os-release append that rpm2img performs, so EIF-built rootfs
images expose the same identifying fields (VERSION, PRETTY_NAME,
VARIANT_ID, VERSION_ID, BUILD_ID, VENDOR_NAME, and the canonical URLs)
to userspace. Unlike rpm2img, INSTALL_ROOT already points at the merged
sysroot, so the append writes directly to ${INSTALL_ROOT}/usr/lib/os-release
without the ${ARCH}-bottlerocket-linux-gnu/sys-root prefix.
Add `[package.metadata.build-variant] eif-pcie-flags` so `image-format = "eif"` variants can author the PCIE flag word written into the EIF header. The value flows buildsys -> Docker ARG (`EIF_PCIE_FLAGS`) -> rpm2eif / eif2eif -> `eif-builder --pcie-flags <hex>`; when unset, `eif-builder`'s built-in `DEFAULT_PCIE_FLAGS` (0x240) is applied, so existing variants are unaffected. The header value must match the PCIE flags the sidecar shim passes to the hypervisor at launch (`header == launch-flags`, hypervisor-enforced); today the shim's launch flags are authored independently, so a per-variant knob is what lets a build keep the two sides in sync. Once the shim derives its launch flags from the header directly (host echoes, hypervisor enforces), the coupling goes away — but this knob is still the authoring surface for the header value itself. Accepted forms: TOML integer literal (decimal, per TOML spec, e.g. `832`) or a `0x`-prefixed hex string (e.g. `"0x340"`). Unprefixed string forms like `"340"` are rejected at parse time: eif-builder's --pcie-flags CLI treats `340` as hex, so silently reading the unprefixed string as decimal would produce exactly the header/launch-flags mismatch this knob exists to prevent. The header==launch-flags contract is documented on `DEFAULT_PCIE_FLAGS` in eif-builder, on the new `BuildVariant` field, and briefly in the eif-builder `--pcie-flags` CLI help. Tests: 6 new unit tests in tools/buildsys covering omitted field, integer literal, hex string (`0x` and `0X`), unprefixed-string rejection, and u16 overflow.
b3766e6 to
355bc86
Compare
The sidecar Nitro Enclaves in-memory image loader validates `BZIMAGE_HEADER_MAGIC` in the EIF kernel section and boots via the bzImage protocol (same as every `nitro-cli`-built EIF). rpm2eif was embedding an uncompressed ELF `vmlinux` "for PVH boot" -- correct only for a bare-metal Firecracker PVH loader, wrong for the sidecar path. The enclave started, the guest never booted, zero console output, and the failed boot wedged the host. Fix by making the format selectable and defaulting to bzImage: * Add `[package.metadata.build-variant] eif-kernel-format` accepting `"bzimage"` (default) or `"vmlinux"`. Ignored on aarch64 (only PE-wrapped `Image` is valid) and by non-EIF variants. * Plumb via buildsys -> Docker ARG (`EIF_KERNEL_FORMAT`) -> rpm2eif `--eif-kernel-format=<fmt>`. Empty ARG means "apply rpm2eif's default" (bzImage), so existing variants pick up the fix automatically. * rpm2eif x86_64 branch switches on the flag: `bzimage` uses the RPM-shipped `vmlinuz` verbatim (no ELF extraction); `vmlinux` retains the existing prefer-bare-then-carve-ELF-out-of-vmlinuz path for bare-metal PVH consumers. * eif-builder x86_64 `prepare_kernel` is already a byte pass-through; both formats round-trip cleanly. Only doc comments needed updating. Kept selectable rather than hardcoded because Firecracker upstream has shipped conflicting guidance on compressed vs. uncompressed kernels more than once, and forking rpm2eif to swap formats is worse than a one-line variant knob. The `extract_vmlinux` machinery is reachable only via `--eif-kernel-format=vmlinux` but is not dead code. Tests: 4 new buildsys unit tests (omitted/bzimage/vmlinux/unknown), 3 new rpm2eif bash tests (bzimage/vmlinux accepted, unknown rejected).
| /// Fail loudly if `Infra.lock` shadows an `[eif]` section that exists in | ||
| /// `Infra.toml`. | ||
| /// | ||
| /// Called from `eif_signing_args` on the "no eif config" branch. The | ||
| /// hazard: `InfraConfig::from_path_or_lock` prefers `Infra.lock` when it | ||
| /// exists and never consults `Infra.toml`, so a lock generated before the | ||
| /// `[eif]` schema landed will happily deserialize to `eif = None`. Without | ||
| /// this check that silently produces an unsigned EIF — same class of | ||
| /// silent-downgrade failure that motivated the "reject broken Infra.toml" | ||
| /// stance elsewhere. | ||
| /// | ||
| /// Detection is intentionally coarse: any parseable Infra.toml with an | ||
| /// `[eif]` section, next to an `Infra.lock` that was actually loaded, is | ||
| /// treated as stale. We don't try to diff the two — if the lock is | ||
| /// current, the caller already unwrapped `Some(eif)` and never reached | ||
| /// this function. | ||
| /// | ||
| /// If Infra.toml is malformed, missing, or the paths don't line up, we | ||
| /// stay quiet: `from_path_or_lock` already validated whichever file it | ||
| /// actually loaded, and the "no eif" outcome is the correct one for the | ||
| /// no-Infra.toml case. |
There was a problem hiding this comment.
| /// Fail loudly if `Infra.lock` shadows an `[eif]` section that exists in | |
| /// `Infra.toml`. | |
| /// | |
| /// Called from `eif_signing_args` on the "no eif config" branch. The | |
| /// hazard: `InfraConfig::from_path_or_lock` prefers `Infra.lock` when it | |
| /// exists and never consults `Infra.toml`, so a lock generated before the | |
| /// `[eif]` schema landed will happily deserialize to `eif = None`. Without | |
| /// this check that silently produces an unsigned EIF — same class of | |
| /// silent-downgrade failure that motivated the "reject broken Infra.toml" | |
| /// stance elsewhere. | |
| /// | |
| /// Detection is intentionally coarse: any parseable Infra.toml with an | |
| /// `[eif]` section, next to an `Infra.lock` that was actually loaded, is | |
| /// treated as stale. We don't try to diff the two — if the lock is | |
| /// current, the caller already unwrapped `Some(eif)` and never reached | |
| /// this function. | |
| /// | |
| /// If Infra.toml is malformed, missing, or the paths don't line up, we | |
| /// stay quiet: `from_path_or_lock` already validated whichever file it | |
| /// actually loaded, and the "no eif" outcome is the correct one for the | |
| /// no-Infra.toml case. | |
| /// Ensure Infra.lock file is accurate and up-to-date |
| /// silently interpreted. The rationale: PCIE flag values are conventionally | ||
| /// written in hex on the `eif-builder --pcie-flags` CLI (whose parser is | ||
| /// hex-only, prefix optional) and in the aws-nitro-enclaves-image-format | ||
| /// header definitions. A user copy-pasting a hex value like `340` from those | ||
| /// contexts into `eif-pcie-flags = "340"` almost certainly means `0x340` | ||
| /// (bits 6, 8, 9), not `340` decimal (`0x154`, bits 2, 4, 6, 8). Treating | ||
| /// the unprefixed string as decimal would produce a header/launch-flags | ||
| /// mismatch that only surfaces at enclave launch, which is the exact kind | ||
| /// of silent misconfiguration this knob exists to prevent. Forcing an | ||
| /// explicit base makes the author's intent unambiguous at parse time: | ||
| /// integer form is unambiguously decimal, string form is unambiguously hex. |
There was a problem hiding this comment.
This is already covered by the crate level docs
| /// silently interpreted. The rationale: PCIE flag values are conventionally | |
| /// written in hex on the `eif-builder --pcie-flags` CLI (whose parser is | |
| /// hex-only, prefix optional) and in the aws-nitro-enclaves-image-format | |
| /// header definitions. A user copy-pasting a hex value like `340` from those | |
| /// contexts into `eif-pcie-flags = "340"` almost certainly means `0x340` | |
| /// (bits 6, 8, 9), not `340` decimal (`0x154`, bits 2, 4, 6, 8). Treating | |
| /// the unprefixed string as decimal would produce a header/launch-flags | |
| /// mismatch that only surfaces at enclave launch, which is the exact kind | |
| /// of silent misconfiguration this knob exists to prevent. Forcing an | |
| /// explicit base makes the author's intent unambiguous at parse time: | |
| /// integer form is unambiguously decimal, string form is unambiguously hex. | |
| /// silently interpreted. |
| /// | ||
| /// Encoded in the variant Cargo.toml as either a TOML integer literal | ||
| /// (decimal, e.g. `832`) or a `0x`-prefixed hex string (e.g. `"0x340"`). | ||
| /// Unprefixed string forms like `"340"` are rejected: the `eif-builder | ||
| /// --pcie-flags` CLI treats `340` as hex, so silently reading the | ||
| /// unprefixed string as decimal here would produce a header/launch-flags | ||
| /// mismatch that only surfaces at enclave launch. Forwarded to | ||
| /// `eif-builder --pcie-flags <hex>` at rpm2eif / eif2eif time. When | ||
| /// absent, `eif-builder` uses its built-in `DEFAULT_PCIE_FLAGS` | ||
| /// (`EIF_HDR_FLAG_PCIE | EIF_HDR_FLAG_PCIE_VIRTIO`, `0x240`). Non-EIF | ||
| /// variants ignore this field. | ||
| /// | ||
| /// Contract: the flags written into the EIF header here **must** match the | ||
| /// PCIE flags the sidecar shim passes to the hypervisor at launch. The | ||
| /// hypervisor enforces `header == launch-flags`; a mismatch fails | ||
| /// attestation / launch. Today the shim's launch flags are hard-coded on | ||
| /// the shim side, so this knob is what lets a variant author the value | ||
| /// that keeps the two sides in sync. Once the shim derives its launch | ||
| /// flags from the header directly (host echoes, hypervisor enforces), the | ||
| /// coupling goes away — but the knob is still how the header value gets | ||
| /// authored per-variant. | ||
| /// | ||
| /// Example: | ||
| /// ```ignore | ||
| /// [package.metadata.build-variant] | ||
| /// image-format = "eif" | ||
| /// eif-pcie-flags = 0x340 | ||
| /// ``` |
There was a problem hiding this comment.
Already covered in crate documentation
| /// | |
| /// Encoded in the variant Cargo.toml as either a TOML integer literal | |
| /// (decimal, e.g. `832`) or a `0x`-prefixed hex string (e.g. `"0x340"`). | |
| /// Unprefixed string forms like `"340"` are rejected: the `eif-builder | |
| /// --pcie-flags` CLI treats `340` as hex, so silently reading the | |
| /// unprefixed string as decimal here would produce a header/launch-flags | |
| /// mismatch that only surfaces at enclave launch. Forwarded to | |
| /// `eif-builder --pcie-flags <hex>` at rpm2eif / eif2eif time. When | |
| /// absent, `eif-builder` uses its built-in `DEFAULT_PCIE_FLAGS` | |
| /// (`EIF_HDR_FLAG_PCIE | EIF_HDR_FLAG_PCIE_VIRTIO`, `0x240`). Non-EIF | |
| /// variants ignore this field. | |
| /// | |
| /// Contract: the flags written into the EIF header here **must** match the | |
| /// PCIE flags the sidecar shim passes to the hypervisor at launch. The | |
| /// hypervisor enforces `header == launch-flags`; a mismatch fails | |
| /// attestation / launch. Today the shim's launch flags are hard-coded on | |
| /// the shim side, so this knob is what lets a variant author the value | |
| /// that keeps the two sides in sync. Once the shim derives its launch | |
| /// flags from the header directly (host echoes, hypervisor enforces), the | |
| /// coupling goes away — but the knob is still how the header value gets | |
| /// authored per-variant. | |
| /// | |
| /// Example: | |
| /// ```ignore | |
| /// [package.metadata.build-variant] | |
| /// image-format = "eif" | |
| /// eif-pcie-flags = 0x340 | |
| /// ``` |
| /// | ||
| /// Defaults to [`EifKernelFormat::Bzimage`] when unset: the sidecar | ||
| /// enclave loader validates `BZIMAGE_HEADER_MAGIC` and boots via the | ||
| /// bzImage protocol, matching every `nitro-cli`-built EIF. Kept | ||
| /// selectable (rather than a hardcoded constant) because Firecracker | ||
| /// upstream has flipped between compressed and uncompressed kernels | ||
| /// more than once, and a bare-metal PVH loader still needs the ELF | ||
| /// `vmlinux` form. | ||
| /// | ||
| /// Ignored on aarch64 (only PE-wrapped `Image` is valid there) and on | ||
| /// non-EIF variants. Forwarded to `rpm2eif` as the | ||
| /// `EIF_KERNEL_FORMAT` build-arg (empty string when unset; `rpm2eif` | ||
| /// applies its own default). |
There was a problem hiding this comment.
| /// | |
| /// Defaults to [`EifKernelFormat::Bzimage`] when unset: the sidecar | |
| /// enclave loader validates `BZIMAGE_HEADER_MAGIC` and boots via the | |
| /// bzImage protocol, matching every `nitro-cli`-built EIF. Kept | |
| /// selectable (rather than a hardcoded constant) because Firecracker | |
| /// upstream has flipped between compressed and uncompressed kernels | |
| /// more than once, and a bare-metal PVH loader still needs the ELF | |
| /// `vmlinux` form. | |
| /// | |
| /// Ignored on aarch64 (only PE-wrapped `Image` is valid there) and on | |
| /// non-EIF variants. Forwarded to `rpm2eif` as the | |
| /// `EIF_KERNEL_FORMAT` build-arg (empty string when unset; `rpm2eif` | |
| /// applies its own default). |
| /// x86_64 kernel image format that `rpm2eif` embeds in the EIF kernel | ||
| /// section. See [`BuildVariant::eif_kernel_format`] for the rationale on | ||
| /// keeping this selectable rather than pinning one value. | ||
| /// | ||
| /// The TOML values `"bzimage"` and `"vmlinux"` are accepted; both are | ||
| /// case-insensitive (via the `lowercase` rename). Ignored on aarch64. |
There was a problem hiding this comment.
| /// x86_64 kernel image format that `rpm2eif` embeds in the EIF kernel | |
| /// section. See [`BuildVariant::eif_kernel_format`] for the rationale on | |
| /// keeping this selectable rather than pinning one value. | |
| /// | |
| /// The TOML values `"bzimage"` and `"vmlinux"` are accepted; both are | |
| /// case-insensitive (via the `lowercase` rename). Ignored on aarch64. | |
| /// x86_64 kernel image format that `rpm2eif` embeds in the EIF kernel | |
| /// section. |
| /// | ||
| /// The outer envelope of the `EifSectionSignature` is a `Vec<PcrSignature>` | ||
| /// serialized with `serde_cbor::to_vec`. The upstream `describe-eif` path | ||
| /// decodes it with `serde_cbor::from_slice::<Vec<PcrSignature>>`, so we | ||
| /// must serialize with the *same* serde derive on a struct with the *same* | ||
| /// field shape (`Vec<u8>` — not `serde_bytes::ByteBuf`), or the decode will | ||
| /// fail with `invalid type: byte array, expected a sequence`. |
There was a problem hiding this comment.
| /// | |
| /// The outer envelope of the `EifSectionSignature` is a `Vec<PcrSignature>` | |
| /// serialized with `serde_cbor::to_vec`. The upstream `describe-eif` path | |
| /// decodes it with `serde_cbor::from_slice::<Vec<PcrSignature>>`, so we | |
| /// must serialize with the *same* serde derive on a struct with the *same* | |
| /// field shape (`Vec<u8>` — not `serde_bytes::ByteBuf`), or the decode will | |
| /// fail with `invalid type: byte array, expected a sequence`. |
| /// Note: the byte fields decode as CBOR **arrays of integers** (major | ||
| /// type 4), not byte strings (major type 2). This is what `serde_cbor` | ||
| /// emits for a `Vec<u8>` under a plain `#[derive(Serialize)]` — the | ||
| /// standard serde `Vec<T>` path — and it is what upstream produces and | ||
| /// upstream consumers expect. Emitting byte strings here (via | ||
| /// `ciborium::Value::Bytes` or `serde_bytes`) would make the CBOR envelope | ||
| /// unparseable by `serde_cbor::from_slice::<Vec<PcrSignature>>` and would | ||
| /// desync the COSE payload from the verifier's `measured_payload` | ||
| /// recomputation, silently failing signature verification. |
There was a problem hiding this comment.
| /// Note: the byte fields decode as CBOR **arrays of integers** (major | |
| /// type 4), not byte strings (major type 2). This is what `serde_cbor` | |
| /// emits for a `Vec<u8>` under a plain `#[derive(Serialize)]` — the | |
| /// standard serde `Vec<T>` path — and it is what upstream produces and | |
| /// upstream consumers expect. Emitting byte strings here (via | |
| /// `ciborium::Value::Bytes` or `serde_bytes`) would make the CBOR envelope | |
| /// unparseable by `serde_cbor::from_slice::<Vec<PcrSignature>>` and would | |
| /// desync the COSE payload from the verifier's `measured_payload` | |
| /// recomputation, silently failing signature verification. |
| /// crate — the family of "hand-rolled DER" was flagged in code review as a | ||
| /// maintenance risk when duplicated. | ||
| #[cfg(test)] | ||
| pub(crate) mod der_helpers { |
There was a problem hiding this comment.
why not just define these directly in mod tests ?
| /// Kept in one place so the length-encoding logic can't drift across the | ||
| /// crate — the family of "hand-rolled DER" was flagged in code review as a | ||
| /// maintenance risk when duplicated. |
There was a problem hiding this comment.
| /// Kept in one place so the length-encoding logic can't drift across the | |
| /// crate — the family of "hand-rolled DER" was flagged in code review as a | |
| /// maintenance risk when duplicated. |
| // Dispatch: three states — unsigned, local-signed, KMS-signed. Clap has | ||
| // already enforced the mutual-exclusion constraints, so we only need to | ||
| // build the right signer here. |
There was a problem hiding this comment.
| // Dispatch: three states — unsigned, local-signed, KMS-signed. Clap has | |
| // already enforced the mutual-exclusion constraints, so we only need to | |
| // build the right signer here. |
| // ...recompute PCR0 the same way `build_signed_eif` did, then | ||
| // reproduce the upstream `measured_payload` construction exactly. | ||
| let pcr0 = compute_pcr0(b"FAKE_KERNEL", b"cmd", &[&[][..]]); |
There was a problem hiding this comment.
Can we use the upstream pcr measurement logic? https://github.com/aws/aws-nitro-enclaves-image-format/blob/main/src/utils/mod.rs
Extend eif-builder with EifSectionSignature (0x04) covering PCR0 via COSE_Sign1 ECDSA (ES256/ES384). rpm2eif reads a dedicated
[eif]section in Infra.toml and picks local (aws-lc-rs) or KMS signing from its shape; absent section builds unsigned. Replaces the earlier sbkeys autodetect so the Nitro-hypervisor trust root is decoupled from the UEFI Secure Boot code-signing key (distinct algorithms, rotation cadence, and trust domain). Signing is orthogonal to UEFI Secure Boot.Wire format matches aws-nitro-enclaves-image-format: CBOR Array(1) of Map { signing_certificate: PEM, signature: COSE_Sign1 } with a PcrInfo payload for PCR0 = SHA-384(48 zero bytes || SHA-384(kernel || cmdline || ramdisks)), so nitro-cli describe-eif populates PCR8.
Adds two repack-variant capabilities:
eif-builder resignsubcommand (byte-preserves kernel/cmdline/ramdisk, rewrites only the signature section + header CRC).Highlights:
resignsubcommandNew deps: aws-lc-rs, coset, ciborium, pem, x509-cert.
Tests: PCR0 spec conformance, COSE payload shape, end-to-end signature verification, DER parser edge cases, resign round-trips, and the eif-sign-helper discovery matrix.
Terms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.