From cc28140d9b041f6157d0e3763e9979b78d8ffe0f Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Fri, 15 Dec 2023 11:28:11 -0500 Subject: [PATCH] Don't enforce container sigpolicy by default Closes: https://github.com/containers/bootc/issues/218 Basically this effort has not been really successful and adds more pain than it solves. We need to have a solution that works for podman too. In many scenarios, TLS is sufficient - or at least, we're far from the only thing that if fetched from a compromised server would result in a compromised system (e.g. privileged containers). Signed-off-by: Colin Walters --- .github/workflows/ci.yml | 2 +- docs/index.md | 2 +- docs/install.md | 7 ++----- lib/src/cli.rs | 33 ++++++++++++++++++++++++++++++--- lib/src/install.rs | 33 ++++++++++++++++++++++++++------- lib/src/privtests.rs | 2 +- tests/kolainst/install | 2 +- 7 files changed, 62 insertions(+), 19 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6dda696b..c1f075fb 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -136,7 +136,7 @@ jobs: run: | set -xeuo pipefail sudo podman run --rm -ti --privileged -v /:/target -v ./usr/bin/bootc:/usr/bin/bootc --pid=host --security-opt label=disable \ - quay.io/centos-bootc/fedora-bootc-dev:eln bootc install to-filesystem --target-no-signature-verification \ + quay.io/centos-bootc/fedora-bootc-dev:eln bootc install to-filesystem \ --karg=foo=bar --disable-selinux --replace=alongside /target ls -al /boot/loader/ sudo grep foo=bar /boot/loader/entries/*.conf diff --git a/docs/index.md b/docs/index.md index 15d49e6b..ea442f29 100644 --- a/docs/index.md +++ b/docs/index.md @@ -106,7 +106,7 @@ For more information, please see [install.md](install.md). If you have [an operating system already using ostree](https://ostreedev.github.io/ostree/#operating-systems-and-distributions-using-ostree) then you can use `bootc switch`: ``` -$ bootc switch --no-signature-verification quay.io/examplecorp/custom:latest +$ bootc switch quay.io/examplecorp/custom:latest ``` This will preserve existing state in `/etc` and `/var` - for example, diff --git a/docs/install.md b/docs/install.md index 8dcad616..e18fc66c 100644 --- a/docs/install.md +++ b/docs/install.md @@ -58,7 +58,7 @@ to an existing system and install your container image. Failure to run Here's an example of using `bootc install` (root/elevated permission required): ```bash -podman run --rm --privileged --pid=host --security-opt label=type:unconfined_t bootc install to-disk --target-no-signature-verification /path/to/disk +podman run --rm --privileged --pid=host --security-opt label=type:unconfined_t bootc install to-disk /path/to/disk ``` Note that while `--privileged` is used, this command will not perform any @@ -85,10 +85,7 @@ This can be overridden via `--target_imgref`; this is handy in cases like perfor installation in a manufacturing environment from a mirrored registry. By default, the installation process will verify that the container (representing the target OS) -can fetch its own updates. A common cause of failure here is not changing the security settings -in `/etc/containers/policy.json` in the target OS to verify signatures. - -If you are pushing an unsigned image, you **MUST** specify `bootc install --target-no-signature-verification`. +can fetch its own updates. Additionally note that to perform an install with a target image reference set to an authenticated registry, you must provide a pull secret. One path is to embed the pull secret into diff --git a/lib/src/cli.rs b/lib/src/cli.rs index 020ce3e6..0460d789 100644 --- a/lib/src/cli.rs +++ b/lib/src/cli.rs @@ -55,10 +55,18 @@ pub(crate) struct SwitchOpts { #[clap(long, default_value = "registry")] pub(crate) transport: String, - /// Explicitly opt-out of requiring any form of signature verification. - #[clap(long)] + /// This argument is deprecated and does nothing. + #[clap(long, hide = true)] pub(crate) no_signature_verification: bool, + /// This is the inverse of the previous `--target-no-signature-verification` (which is now + /// a no-op). + /// + /// Enabling this option enforces that `/etc/containers/policy.json` includes a + /// default policy which requires signatures. + #[clap(long)] + pub(crate) enforce_container_sigpolicy: bool, + /// Enable verification via an ostree remote #[clap(long)] pub(crate) ostree_remote: Option, @@ -363,7 +371,7 @@ async fn switch(opts: SwitchOpts) -> Result<()> { name: opts.target.to_string(), }; let sigverify = sigpolicy_from_opts( - opts.no_signature_verification, + !opts.enforce_container_sigpolicy, opts.ostree_remote.as_deref(), ); let target = ostree_container::OstreeImageReference { sigverify, imgref }; @@ -475,3 +483,22 @@ async fn run_from_opt(opt: Opt) -> Result<()> { Opt::Man(manopts) => crate::docgen::generate_manpages(&manopts.directory), } } + +#[test] +fn test_parse_install_args() { + // Verify we still process the legacy --target-no-signature-verification + let o = Opt::try_parse_from([ + "bootc", + "install", + "to-filesystem", + "--target-no-signature-verification", + "/target", + ]) + .unwrap(); + let o = match o { + Opt::Install(InstallOpts::ToFilesystem(fsopts)) => fsopts, + o => panic!("Expected filesystem opts, not {o:?}"), + }; + assert!(o.target_opts.target_no_signature_verification); + assert_eq!(o.filesystem_opts.root_path.as_str(), "/target"); +} diff --git a/lib/src/install.rs b/lib/src/install.rs index 621eca15..99223ef3 100644 --- a/lib/src/install.rs +++ b/lib/src/install.rs @@ -67,11 +67,26 @@ pub(crate) struct InstallTargetOpts { #[clap(long)] pub(crate) target_imgref: Option, - /// Explicitly opt-out of requiring any form of signature verification. - #[clap(long)] + /// This command line argument does nothing; it exists for compatibility. + /// + /// As of newer versions of bootc, this value is enabled by default, + /// i.e. it is not enforced that a signature + /// verification policy is enabled. Hence to enable it, one can specify + /// `--target-no-signature-verification=false`. + /// + /// It is likely that the functionality here will be replaced with a different signature + /// enforcement scheme in the future that integrates with `podman`. + #[clap(long, hide = true)] #[serde(default)] pub(crate) target_no_signature_verification: bool, + /// This is the inverse of the previous `--target-no-signature-verification` (which is now + /// a no-op). Enabling this option enforces that `/etc/containers/policy.json` includes a + /// default policy which requires signatures. + #[clap(long)] + #[serde(default)] + pub(crate) enforce_container_sigpolicy: bool, + /// Enable verification via an ostree remote #[clap(long)] pub(crate) target_ostree_remote: Option, @@ -80,10 +95,8 @@ pub(crate) struct InstallTargetOpts { /// Specifying this option suppresses the check; use this when you know the issues it might find /// are addressed. /// - /// Two main reasons this might fail: - /// - /// - Forgetting `--target-no-signature-verification` if needed - /// - Using a registry which requires authentication, but not embedding the pull secret in the image. + /// A common reason this may fail is when one is using an image which requires registry authentication, + /// but not embedding the pull secret in the image so that updates can be fetched by the installed OS "day 2". #[clap(long)] #[serde(default)] pub(crate) skip_fetch_check: bool, @@ -916,8 +929,14 @@ async fn prepare_install( // Parse the target CLI image reference options and create the *target* image // reference, which defaults to pulling from a registry. + if target_opts.target_no_signature_verification { + // Perhaps log this in the future more prominently, but no reason to annoy people. + tracing::debug!( + "Use of --target-no-signature-verification flag which is enabled by default" + ); + } let target_sigverify = sigpolicy_from_opts( - target_opts.target_no_signature_verification, + !target_opts.enforce_container_sigpolicy, target_opts.target_ostree_remote.as_deref(), ); let target_imgname = target_opts diff --git a/lib/src/privtests.rs b/lib/src/privtests.rs index f85855a2..f59c5309 100644 --- a/lib/src/privtests.rs +++ b/lib/src/privtests.rs @@ -152,7 +152,7 @@ fn test_install_filesystem(image: &str, blockdev: &Utf8Path) -> Result<()> { let mountpoint: &Utf8Path = mountpoint_dir.path().try_into().unwrap(); // And run the install - cmd!(sh, "podman run --rm --privileged --pid=host --env=RUST_LOG -v /usr/bin/bootc:/usr/bin/bootc -v {mountpoint}:/target-root {image} bootc install to-filesystem --target-no-signature-verification /target-root").run()?; + cmd!(sh, "podman run --rm --privileged --pid=host --env=RUST_LOG -v /usr/bin/bootc:/usr/bin/bootc -v {mountpoint}:/target-root {image} bootc install to-filesystem /target-root").run()?; cmd!(sh, "umount -R {mountpoint}").run()?; diff --git a/tests/kolainst/install b/tests/kolainst/install index 4bfda0c5..121dc2af 100755 --- a/tests/kolainst/install +++ b/tests/kolainst/install @@ -30,7 +30,7 @@ case "${AUTOPKGTEST_REBOOT_MARK:-}" in EOF podman build -t localhost/testimage . podman run --rm -ti --privileged --pid=host --env RUST_LOG=error,bootc_lib::install=debug \ - localhost/testimage bootc install to-disk --target-no-signature-verification --skip-fetch-check --karg=foo=bar ${DEV} + localhost/testimage bootc install to-disk --skip-fetch-check --karg=foo=bar ${DEV} # In theory we could e.g. wipe the bootloader setup on the primary disk, then reboot; # but for now let's just sanity test that the install command executes. lsblk ${DEV}