Conversation
|
OK I think the chain here should be:
|
3b90847 to
f92013b
Compare
f92013b to
69757fc
Compare
There was a problem hiding this comment.
lgtm
just needs a rebase I think.
The only nit is that
config.erofs_formats = composefs_ctl::composefs::erofs::format::FormatConfig {
default: composefs_ctl::composefs::erofs::format::FormatVersion::V1,
extra: [composefs_ctl::composefs::erofs::format::FormatVersion::V2].into(),
};
Is duplicated 3 times here. So if we add a new version in the future we need to care about 3 places. Maybe it should be a helper function?
Johan-Liebert1
left a comment
There was a problem hiding this comment.
Lots of .clone() that I believe shouldn't be needed, I might be wrong though. Supporting both V1 and V2 are great, but it is a bit messy, not sure if there's a better way to do this. Also, some inconsistencies here and there (esp in comments) regarding whether V1 is the default or V2
| seal_state=$1 | ||
| shift | ||
| # EROFS format version to pass to bootc container ukify (optional, default: v2) | ||
| erofs_version=${1:-v2} |
There was a problem hiding this comment.
Kind of conflicts with composefs/composefs-rs#330. We'd probably want to have the same defaults everywhere
| os_id: Option<String>, | ||
| boot_digest: String, | ||
| /// The composefs image digest parsed from (and validated against) the UKI's | ||
| /// own cmdline. This is the authoritative deployment key for UKI boots: |
There was a problem hiding this comment.
This is for every boot right, not just UKIs?
| let composefs_info = BootComposefsCmdline::<Sha512HashValue>::from_cmdline(&cmdline) | ||
| .context("Parsing composefs=")? | ||
| .ok_or_else(|| anyhow::anyhow!("No composefs= or composefs.digest.v1= karg found in UKI cmdline"))?; | ||
| let composefs_cmdline = composefs_info.digest().clone(); |
There was a problem hiding this comment.
This name is a bit confusing. afaiu this is only the digest and not the entire cmdline?
|
|
||
| if test "${boot_type}" = "uki"; then | ||
| /run/packaging/seal-uki /run/target /out /run/secrets "${allow_missing_verity}" "${seal_state}" | ||
| /run/packaging/seal-uki /run/target /out /run/secrets "${allow_missing_verity}" "${seal_state}" "${erofs_version}" |
There was a problem hiding this comment.
We also need this in tmt/tests/booted/test-install-to-filesystem-var-mount.sh
| // (see setup_composefs_boot for the full rationale). Provisional value for | ||
| // BLS (where bootc writes the karg from this same id); overridden for UKI by | ||
| // the digest the UKI cmdline actually carries. | ||
| let provisional_deploy_id = boot_id_v2.clone().unwrap_or_else(|| id.clone()); |
There was a problem hiding this comment.
id here is confusing especially with both boot_id_v1/v2 defined. I believe it's the erofs digest corresponding to the erofs version that the repo is currently using?
| // Authoritative collision check against the final deploy key. For UKI this | ||
| // may differ from the provisional checked above (the UKI may carry a | ||
| // non-default digest), so this is the load-bearing guarantee. | ||
| ensure_no_deploy_collision(host, &deploy_id)?; |
There was a problem hiding this comment.
why do we need to do this again?
| // setup-root opens `state/deploy/<this>` using that same karg, so we must | ||
| // key the deployment off exactly this value -- whether the UKI was sealed | ||
| // with the V2 (default) or V1 EROFS digest. | ||
| let deploy_id = uki_info.composefs_cmdline.clone(); |
There was a problem hiding this comment.
we shouldn't need to clone this?
6ec1bf4 to
cbff4b5
Compare
|
In the general case we may need to add support for "older bootc version" which includes not just the v2 digest but the xattr filtering logic too? See composefs/composefs-rs#337 |
cbff4b5 to
8e7417f
Compare
4bf145c to
45e92e4
Compare
|
OK, this one wants #2290 to land first which fixes our composefs mounts on c9s. |
CentOS 9 cannot consume sealed host-built UKI upgrades because shared storage is unavailable and its guest-local builder produces unsigned images. Record that limitation while retaining installation, readonly, other upgrade variants, and newer-system sealed coverage. With V1 EROFS as the default, sealed UKIs are viable on CentOS 9; exclude only the BLS and unsealed modes that still require newer dracut/systemd features. Resolve both the runtime base and buildroot from each matrix OS. Otherwise CentOS 9 jobs silently build EL10 RPMs and binaries that cannot run against its older glibc. Generated-by: AI Signed-off-by: Colin Walters <walters@verbum.org>
The `--bind-storage-ro` host container-storage passthrough relies on a libvirt-managed virtiofsd, which cannot run in some environments such as nested user namespaces or cloud/non-qemu setups. Plans that normally request bind-storage previously had no way to opt out short of editing plan metadata. Add a `--skip-bind-storage` flag (and matching `BOOTC_skip_bind_storage` env var) that forces those plans to run without the host container-storage mount. Default behavior is unchanged: bind-storage is still used wherever it is requested and supported. Plans that depend on a locally built upgrade image reaching the VM via bind-storage will be unable to perform the upgrade/switch step when this is set. Assisted-by: AI Signed-off-by: Colin Walters <walters@verbum.org>
composefs-rs landed support for V1 EROFS, which we need to enable composefs on RHEL9. Make new installs produce both V1 and V2 EROFS images for committed composefs images, and make V1 the default wherever a single format must be chosen: the repository's default EROFS format, the `--erofs-version` flag on `bootc container ukify` and `compute-composefs-digest`, and the provisional BLS deploy key computed at install time. V2 remains available via `--erofs-version=v2` and is always generated alongside V1, so a deployment can still be booted via the legacy `composefs=` karg. This keeps the install path consistent with the upgrade and GC paths, which already prefer V1. Critically, a V1 digest must be written as a `composefs.digest=v1-...` karg, not the legacy `composefs=` shorthand (which upstream reserves for V2). Add `build_composefs_karg`, which selects the correct form via composefs-boot's own `ComposefsCmdline::new_v1`/`new_v2` and `to_cmdline_arg`, and use it everywhere bootc writes a new karg (install, upgrade, `container ukify`, soft-reboot) instead of the version-unaware helper that only ever emitted `composefs=`. Assisted-by: AI Signed-off-by: Colin Walters <walters@verbum.org>
Older and newer composefs-rs tooling can differ in xattr filtering and EROFS defaults, which otherwise breaks UKI upgrades across bootc versions. Search supported combinations for the digest embedded in the UKI so a newer client can adapt to an older target. Keep the missing-deployment resilience fixture syntactically valid so typed argument parsing reaches the warning path it is intended to exercise. Assisted-by: AI Signed-off-by: Colin Walters <walters@verbum.org>
We want to support upgrades from older bootc. Keep a legacy V2 digest in UKIs with a V1-capable initramfs, and select V2-only when resealing a legacy initramfs so the mounted root and deployment state retain the same identity. Determine compatibility from the actual initramfs rather than installed userspace. Preserve strict repository requirements when recovering a non-default boot image; changing serialization must not relax integrity. Assisted-by: AI Signed-off-by: Colin Walters <walters@verbum.org>
45e92e4 to
f839490
Compare
OK! Finally that landed. I rebased this, but it's still somewhat raw, especially the last commit. |
| repo_requires_fsverity: bool, | ||
| missing_fsverity_allowed: bool, | ||
| uki_allows_missing_fsverity: bool, |
There was a problem hiding this comment.
Too many bool here...I think we may need an enum
| Ok(()) | ||
| } | ||
|
|
||
| /// Validate every primary UKI before any bootloader or ESP operation. The |
There was a problem hiding this comment.
Hmmm I'm not sure, I think we need to be robust to corrupted state and allow recovery. Bailing early hurts that.
This seems more like a bootc internals fsck style thing.
| .file_path | ||
| .file_name() | ||
| .ok_or_else(|| anyhow!("Could not get UKI file name"))? | ||
| .to_string_lossy() |
There was a problem hiding this comment.
No. We should require UTF-8.
| const COMPOSEFS_DIGEST_V1_FEATURE: &str = "/usr/lib/bootc/initramfs-features/composefs-digest-v1"; | ||
| const COMPOSEFS_DIGEST_V1_FEATURE_CONTENT: &[u8] = b"composefs-digest-v1 state-v1\n"; | ||
|
|
||
| /// Query `lsinitrd` without unpacking or executing any initramfs contents. |
There was a problem hiding this comment.
No, this is awful. I don't want ukify to be parsing the initramfs.
Among other things I don't want to hard depend on dracut.
I think we should just default to injecting both EROFS kargs right?
There was a problem hiding this comment.
If we just want the cmdline, we have a function in composefs-rs for that. get_uki_cmdline_buffered
There was a problem hiding this comment.
Minor thing here, but this is now definitely out of sync with https://github.com/composefs/composefs-rs/blob/main/crates/composefs-setup-root/src/main.rs
Not sure if we'd want to keep them in sync
|
|
||
| fn parse_composefs_candidates(cmdline: &str) -> Result<Vec<ComposefsCmdline<Sha512HashValue>>> { | ||
| let mut candidates = Vec::new(); | ||
| for token in split_cmdline(cmdline) { |
There was a problem hiding this comment.
We should really be using the kernel-cmdline crate here
| fn mount_composefs_candidate( | ||
| sysroot: &OwnedFd, | ||
| candidate: &ComposefsCmdline<Sha512HashValue>, | ||
| allow_missing_fsverity: bool, |
There was a problem hiding this comment.
I think it's worth documenting that allow_missing_verity is coming from the repo and not the cmdline
| fn parse_composefs_candidates(cmdline: &str) -> Result<Vec<ComposefsCmdline<Sha512HashValue>>> { | ||
| let mut candidates = Vec::new(); | ||
| for token in split_cmdline(cmdline) { | ||
| if token.starts_with(&format!("{KARG_COMPOSEFS_DIGEST}=")) |
There was a problem hiding this comment.
I think we should only allow one of each, as in at max composefs.digest=abc123 composefs=a1b2c3. I don't think we should allow multiple of composefs.digest= or composefs= in the kernel cmdline
| cmdline.remove(&ParameterKey::from(COMPOSEFS_CMDLINE)); | ||
| cmdline.remove(&ParameterKey::from(COMPOSEFS_DIGEST_CMDLINE)); |
There was a problem hiding this comment.
In initrarmfs/src/lib.rs these are imported from composefs-rs under the names KARG_V2 and KARG_COMPOSEFS_DIGEST respectively. We should just use one import location, or at least have the same names for constants
| let cmdline = uki::get_cmdline_buffered(&mut uki_reader).context("Getting UKI cmdline")?; | ||
| let composefs_info = ComposefsBootCmdline::<Sha512HashValue>::from_cmdline(&cmdline) | ||
| .context("Parsing composefs=")? | ||
| .ok_or_else(|| anyhow::anyhow!("No composefs image in UKI cmdline"))?; |
There was a problem hiding this comment.
| .ok_or_else(|| anyhow::anyhow!("No composefs image in UKI cmdline"))?; | |
| .ok_or_else(|| anyhow::anyhow!("No composefs digest in UKI cmdline"))?; |
| let entries = | ||
| get_boot_resources(&fs, &*repo).context("Extracting boot entries from OCI image")?; | ||
|
|
||
| // If the UKI was built by tooling using a different xattr filtering |
There was a problem hiding this comment.
We already do this when pulling the repo. Why again?
There was a problem hiding this comment.
Also, these functions have nothing to do with "boot" itself. They should be in a separate file, like boot_utils.rs or something
| digest: digest.into(), | ||
| /// Search for either supported composefs kernel command line parameter. | ||
| pub(crate) fn find_in_cmdline(cmdline: &Cmdline) -> Option<Self> { | ||
| let parsed = BootComposefsCmdline::<Sha512HashValue>::from_cmdline(cmdline).ok()??; |
There was a problem hiding this comment.
Not a fan of ?? without any context
|
|
||
| let is_composefs = (tap is_composefs) | ||
|
|
||
| if not $is_composefs { |
There was a problem hiding this comment.
This test specifically is failing with
content: error: Installing to disk: Setting up composefs boot: The UKI requests insecure composefs operation, but this repository requires fs-verity. Use --allow-missing-fsverity only when missing fs-verity is explicitly supported for this install.
content: Connection to localhost closed.
There was a problem hiding this comment.
I guess in the install path --allow-missing-verity is not being respected?
This adapts bootc to build on top of the work in composefs/composefs-rs#297
A toplevel goal here is supporting both the v1 and v2 EROFS formats, which means we'll work with RHEL9 era systems.
Right now
bootc container ukifystill generatescomposefs=i.e. v2, but I'd like to change that to do both - it's a pretty cheap thing (the main cost is generating the fsverity digests).