Merge pull request #230 from cgwalters/no-sigverify

Don't enforce container sigpolicy by default

Author: cgwalters

.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

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,

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 <image> bootc install to-disk --target-no-signature-verification /path/to/disk
+podman run --rm --privileged --pid=host --security-opt label=type:unconfined_t <image> 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

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<String>,
@@ -364,7 +372,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 };
@@ -476,3 +484,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");
+}

lib/src/fixtures/spec-ostree-remote.yaml

@@ -0,0 +1,26 @@
+# This one drops the now-optional signature schema
+apiVersion: org.containers.bootc/v1alpha1
+kind: BootcHost
+metadata:
+  name: host
+spec:
+  image:
+    image: quay.io/fedora/fedora-coreos:stable
+    transport: registry
+    signature: !ostreeRemote "fedora"
+status:
+  booted:
+    image:
+      image:
+        image: quay.io/otherexample/otherimage:latest
+        transport: registry
+      version: 20231230.1
+      timestamp: 2023-12-30T16:10:11Z
+      imageDigest: sha256:b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c
+    incompatible: false
+    pinned: false
+    ostree:
+      checksum: 41af286dc0b172ed2f1ca934fd2278de4a1192302ffa07087cea2682e7d372e3
+      deploySerial: 0
+  rollback: null
+  isContainer: false

lib/src/fixtures/spec-v1.yaml

@@ -0,0 +1,25 @@
+# This one drops the now-optional signature schema
+apiVersion: org.containers.bootc/v1alpha1
+kind: BootcHost
+metadata:
+  name: host
+spec:
+  image:
+    image: quay.io/otherexample/otherimage:latest
+    transport: registry
+status:
+  booted:
+    image:
+      image:
+        image: quay.io/otherexample/otherimage:latest
+        transport: registry
+      version: 20231230.1
+      timestamp: 2023-12-30T16:10:11Z
+      imageDigest: sha256:b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c
+    incompatible: false
+    pinned: false
+    ostree:
+      checksum: 41af286dc0b172ed2f1ca934fd2278de4a1192302ffa07087cea2682e7d372e3
+      deploySerial: 0
+  rollback: null
+  isContainer: false

lib/src/install.rs

@@ -67,11 +67,26 @@ pub(crate) struct InstallTargetOpts {
     #[clap(long)]
     pub(crate) target_imgref: Option<String>,
 
-    /// 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<String>,
@@ -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,
@@ -914,8 +927,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

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()?;
 

lib/src/spec.rs

@@ -51,8 +51,9 @@ pub struct ImageReference {
     pub image: String,
     /// The container image transport
     pub transport: String,
-    /// Disable signature verification
-    pub signature: ImageSignature,
+    /// Signature verification type
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub signature: Option<ImageSignature>,
 }
 
 /// The status of the booted image
@@ -132,12 +133,33 @@ mod tests {
     use super::*;
 
     #[test]
-    fn test_parse_spec() {
+    fn test_parse_spec_v0() {
         const SPEC_FIXTURE: &str = include_str!("fixtures/spec.yaml");
         let host: Host = serde_yaml::from_str(SPEC_FIXTURE).unwrap();
         assert_eq!(
             host.spec.image.as_ref().unwrap().image.as_str(),
             "quay.io/example/someimage:latest"
         );
     }
+
+    #[test]
+    fn test_parse_spec_v1() {
+        const SPEC_FIXTURE: &str = include_str!("fixtures/spec-v1.yaml");
+        let host: Host = serde_yaml::from_str(SPEC_FIXTURE).unwrap();
+        assert_eq!(
+            host.spec.image.as_ref().unwrap().image.as_str(),
+            "quay.io/otherexample/otherimage:latest"
+        );
+        assert_eq!(host.spec.image.as_ref().unwrap().signature, None);
+    }
+
+    #[test]
+    fn test_parse_ostreeremote() {
+        const SPEC_FIXTURE: &str = include_str!("fixtures/spec-ostree-remote.yaml");
+        let host: Host = serde_yaml::from_str(SPEC_FIXTURE).unwrap();
+        assert_eq!(
+            host.spec.image.as_ref().unwrap().signature,
+            Some(ImageSignature::OstreeRemote("fedora".into()))
+        );
+    }
 }

lib/src/status.rs

@@ -51,8 +51,12 @@ fn transport_to_string(transport: ostree_container::Transport) -> String {
 
 impl From<OstreeImageReference> for ImageReference {
     fn from(imgref: OstreeImageReference) -> Self {
+        let signature = match imgref.sigverify {
+            ostree_container::SignatureSource::ContainerPolicyAllowInsecure => None,
+            v => Some(v.into()),
+        };
         Self {
-            signature: imgref.sigverify.into(),
+            signature,
             transport: transport_to_string(imgref.imgref.transport),
             image: imgref.imgref.name,
         }
@@ -61,8 +65,12 @@ impl From<OstreeImageReference> for ImageReference {
 
 impl From<ImageReference> for OstreeImageReference {
     fn from(img: ImageReference) -> Self {
+        let sigverify = match img.signature {
+            Some(v) => v.into(),
+            None => ostree_container::SignatureSource::ContainerPolicyAllowInsecure,
+        };
         Self {
-            sigverify: img.signature.into(),
+            sigverify,
             imgref: ostree_container::ImageReference {
                 /// SAFETY: We validated the schema in kube-rs
                 transport: img.transport.as_str().try_into().unwrap(),
@@ -284,3 +292,27 @@ pub(crate) async fn status(opts: super::cli::StatusOpts) -> Result<()> {
 
     Ok(())
 }
+
+#[test]
+fn test_convert_signatures() {
+    use std::str::FromStr;
+    let ir_unverified = &OstreeImageReference::from_str(
+        "ostree-unverified-registry:quay.io/someexample/foo:latest",
+    )
+    .unwrap();
+    let ir_ostree = &OstreeImageReference::from_str(
+        "ostree-remote-registry:fedora:quay.io/fedora/fedora-coreos:stable",
+    )
+    .unwrap();
+
+    let ir = ImageReference::from(ir_unverified.clone());
+    assert_eq!(ir.image, "quay.io/someexample/foo:latest");
+    assert_eq!(ir.signature, None);
+
+    let ir = ImageReference::from(ir_ostree.clone());
+    assert_eq!(ir.image, "quay.io/fedora/fedora-coreos:stable");
+    assert_eq!(
+        ir.signature,
+        Some(ImageSignature::OstreeRemote("fedora".into()))
+    );
+}