Fix signature handling with additionalimagestore #166

gursewak1997 · 2025-11-27T22:43:44Z

Copy images to local storage without signatures before bootc install to avoid signature invalidation errors. Falls back to original behavior if copy fails.

gemini-code-assist

Code Review

This pull request introduces a workaround to handle image signature invalidation errors by copying the image without signatures before installation. My review focuses on improving the robustness of the added shell script logic. I've suggested using trap for reliable temporary file cleanup and quoting variables to prevent potential shell injection or word-splitting issues.

crates/kit/src/to_disk.rs

cgwalters · 2025-12-01T21:54:28Z

crates/kit/src/to_disk.rs

+            # without signatures, since bootc install requires changing layer representation.
+            export STORAGE_OPTS=additionalimagestore=${AIS}
+            SOURCE_REF={SOURCE_IMGREF}
+            LOCAL_IMGREF="containers-storage:bcvk-temp-install:latest"


It's going to be quite a UX hit to make a temporary deep copy of the input image just for this. I already feel the speed hit from having one copy, going to two would be painful.

I'm not opposed to a workaround but I think we should at least spend some time investigating better fixes - which would probably be in https://github.com/containers/container-libs/

See especially the conversation in containers/image#2590

I think we can reopen a related issue from that discussion or so?

In the short term maybe we can detect if the image has attached signatures and only do the fallback copy then?

Images with signatures: Copy to local storage without signatures (fixes issue #126)
Images without signatures: Use original reference directly (no performance hit)
If signature check fails: Falls back to assuming no signatures (uses original reference)

cgwalters · 2025-12-01T21:55:08Z

crates/kit/src/to_disk.rs

+            cat > "${SIG_POLICY}" <<'EOF'
+{"default":[{"type":"insecureAcceptAnything"}],"transports":{"containers-storage":[{"type":"insecureAcceptAnything"}]}}
+EOF
+            if skopeo copy --signature-policy "${SIG_POLICY}" --remove-signatures \


It's not clear to me what the if is doing here. If skopeo fails due to being say out of disk space then we just...try to ignore that? That seems odd.

cgwalters · 2025-12-02T16:14:59Z

crates/kit/src/to_disk.rs

+            # Note: If skopeo inspect fails, we assume no signatures to avoid false positives
+            # but this means we might miss signatures if inspect fails for other reasons
+            HAS_SIGNATURES=0
+            if skopeo inspect --storage-opt "additionalimagestore=${AIS}" "${SOURCE_REF}" 2>/dev/null | \


I don't think --storage-opt is necessary here, it gets inherited via the env var above right?

cgwalters · 2025-12-02T16:25:53Z

crates/kit/src/to_disk.rs

+            # but this means we might miss signatures if inspect fails for other reasons
+            HAS_SIGNATURES=0
+            if skopeo inspect --storage-opt "additionalimagestore=${AIS}" "${SOURCE_REF}" 2>/dev/null | \
+               grep -q '"Signatures"'; then


We're parsing JSON via grep which is pretty hacky; if e.g. one of the labels happens to contain the string "Signatures" we'll end up taking this code path incorrectly.

Also more importantly, does this even work? I don't see skopeo emitting that string when I run e.g. skopeo inspect -n containers-storage:registry.redhat.io/rhel9/rhel-bootc:9.6 |grep -i signatures

Also on that topic, it'd really help to have a regression test added for this too (though for that particular image it will need to be opt-in as it requires setting up RH pull secrets).

Hmm something a bit confusing to me here is actually...this I think isn't just "is the image signed" (because quay.io/centos-bootc/centos-bootc:stream10 is also signed) it's actually "does the container policy enforce signatures" and that's defined by the /etc/containers/policy.json in the target container image.

The RHEL image has

$ cat /etc/containers/policy.json { "default": [ { "type": "insecureAcceptAnything" } ], "transports": { "docker": { "registry.access.redhat.com": [ { "type": "signedBy", "keyType": "GPGKeys", "keyPaths": ["/etc/pki/rpm-gpg/RPM-GPG-KEY-redhat-release", "/etc/pki/rpm-gpg/RPM-GPG-KEY-redhat-beta"] } ], "registry.redhat.io": [ { "type": "signedBy", "keyType": "GPGKeys", "keyPaths": ["/etc/pki/rpm-gpg/RPM-GPG-KEY-redhat-release", "/etc/pki/rpm-gpg/RPM-GPG-KEY-redhat-beta"] } ] }, "docker-daemon": { "": [ { "type": "insecureAcceptAnything" } ] } } } $

(And centos-bootc has that exact thing too, but we're not enforcing it there)

So...if this is all true, one possible approach here is to just override /etc/containers/policy.json inside the to-disk flow instead.

We're parsing JSON via grep which is pretty hacky;

The real root problem here is writing nontrivial logic in shell script; I tried to minimize what we were doing here from the start. A possible approach to improve this is to move the logic to be executed from a systemd unit, then we just start and attach to that unit's output over ssh.

All the setup stuff can run before ssh even. I'll look at this.

Copy images to local storage without signatures before bootc install to avoid signature invalidation errors. Falls back to original behavior if copy fails. Signed-off-by: gursewak1997 <gursmangat@gmail.com>

cgwalters

Offhand looks OK to me. Does it work?

cgwalters · 2025-12-03T19:24:39Z

crates/kit/src/to_disk.rs

+            
+            # Create permissive policy.json (use /var/tmp since it's mounted into podman container)
+            # Mount as directory to /etc/containers so podman creates the directory if it doesn't exist
+            POLICY_DIR=/var/tmp/bcvk-policy-dir


I know this is an ephemeral isolated system but it's probably still good to avoid hardcoded filenames in /tmp or /var/tmp. So we could use e.g. policydir=$(mktemp -d) or so.

It might be a nicer cleanup to have this policy JSON string as a literal file in the Rust source that we include via include_str! and then template into the shell script.

cgwalters · 2025-12-03T19:24:59Z

crates/kit/src/to_disk.rs

                --env=STORAGE_OPTS \
                {INSTALL_LOG} \
-                {SOURCE_IMGREF} \
+                "${SOURCE_REF}" \


This change looks unrelated?

gursewak1997 · 2025-12-03T19:27:15Z

Offhand looks OK to me. Does it work?

Some minor issues rn. Working on some changes to make the integration tests pass.

gursewak1997 force-pushed the fix/to-disk-signatures branch from dadf28e to a8ed770 Compare November 27, 2025 22:44

gemini-code-assist bot reviewed Nov 27, 2025

View reviewed changes

crates/kit/src/to_disk.rs Outdated Show resolved Hide resolved

gursewak1997 force-pushed the fix/to-disk-signatures branch from a8ed770 to a710bde Compare November 27, 2025 22:48

gursewak1997 linked an issue Nov 28, 2025 that may be closed by this pull request

to-disk: Handle signatures #126

Open

cgwalters reviewed Dec 1, 2025

View reviewed changes

gursewak1997 force-pushed the fix/to-disk-signatures branch from a710bde to 0b83293 Compare December 1, 2025 23:04

cgwalters reviewed Dec 2, 2025

View reviewed changes

gursewak1997 force-pushed the fix/to-disk-signatures branch 2 times, most recently from 500563a to e25b44d Compare December 2, 2025 22:07

Fix signature handling with additionalimagestore

94a36f6

Copy images to local storage without signatures before bootc install to avoid signature invalidation errors. Falls back to original behavior if copy fails. Signed-off-by: gursewak1997 <gursmangat@gmail.com>

gursewak1997 force-pushed the fix/to-disk-signatures branch from e25b44d to 94a36f6 Compare December 3, 2025 00:03

cgwalters reviewed Dec 3, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix signature handling with additionalimagestore #166

Fix signature handling with additionalimagestore #166

gursewak1997 commented Nov 27, 2025

Uh oh!

gemini-code-assist bot left a comment

Uh oh!

Uh oh!

cgwalters Dec 1, 2025

Uh oh!

gursewak1997 Dec 2, 2025

Uh oh!

cgwalters Dec 1, 2025

Uh oh!

cgwalters Dec 2, 2025

Uh oh!

cgwalters Dec 2, 2025 •

edited

Loading

Uh oh!

cgwalters Dec 2, 2025

Uh oh!

cgwalters left a comment

Uh oh!

cgwalters Dec 3, 2025

Uh oh!

cgwalters Dec 3, 2025

Uh oh!

gursewak1997 commented Dec 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Fix signature handling with additionalimagestore #166

Are you sure you want to change the base?

Fix signature handling with additionalimagestore #166

Conversation

gursewak1997 commented Nov 27, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

cgwalters Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

gursewak1997 Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

cgwalters Dec 1, 2025

Choose a reason for hiding this comment

Uh oh!

cgwalters Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

cgwalters Dec 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cgwalters Dec 2, 2025

Choose a reason for hiding this comment

Uh oh!

cgwalters left a comment

Choose a reason for hiding this comment

Uh oh!

cgwalters Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

cgwalters Dec 3, 2025

Choose a reason for hiding this comment

Uh oh!

gursewak1997 commented Dec 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cgwalters Dec 2, 2025 •

edited

Loading