Skip to content

Fix signature handling with additionalimagestore #166

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
gursewak1997 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix signature handling with additionalimagestore #166

gursewak1997 wants to merge 1 commit into from

Conversation

@gursewak1997
Copy link
Collaborator

@gursewak1997 gursewak1997 commented Nov 27, 2025 •
edited by cgwalters
Loading

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

Closes: #126

@gursewak1997 gursewak1997 force-pushed the fix/to-disk-signatures branch from dadf28e to a8ed770 Compare November 27, 2025 22:44
gemini-code-assist[bot]
gemini-code-assist bot reviewed Nov 27, 2025
View reviewed changes
Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@gursewak1997 gursewak1997 force-pushed the fix/to-disk-signatures branch from a8ed770 to a710bde Compare November 27, 2025 22:48
@gursewak1997 gursewak1997 linked an issue Nov 28, 2025 that may be closed by this pull request
to-disk: Handle signatures #126
Open
cgwalters
cgwalters reviewed Dec 1, 2025
View reviewed changes
crates/kit/src/to_disk.rs Outdated
cat > "${SIG_POLICY}" <<'EOF'
{"default":[{"type":"insecureAcceptAnything"}],"transports":{"containers-storage":[{"type":"insecureAcceptAnything"}]}}
EOF
if skopeo copy --signature-policy "${SIG_POLICY}" --remove-signatures \
Copy link
Collaborator

@cgwalters cgwalters Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@gursewak1997 gursewak1997 force-pushed the fix/to-disk-signatures branch from a710bde to 0b83293 Compare December 1, 2025 23:04
cgwalters
cgwalters reviewed Dec 2, 2025
View reviewed changes
@gursewak1997 gursewak1997 force-pushed the fix/to-disk-signatures branch 3 times, most recently from e25b44d to 94a36f6 Compare December 3, 2025 00:03
cgwalters
cgwalters reviewed Dec 3, 2025
View reviewed changes
Copy link
Collaborator

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Offhand looks OK to me. Does it work?

crates/kit/src/to_disk.rs Outdated
# 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
Copy link
Collaborator

@cgwalters cgwalters Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

crates/kit/src/to_disk.rs Outdated
--env=STORAGE_OPTS \
{INSTALL_LOG} \
{SOURCE_IMGREF} \
"${SOURCE_REF}" \
Copy link
Collaborator

@cgwalters cgwalters Dec 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change looks unrelated?

@gursewak1997
Copy link
Collaborator Author

gursewak1997 commented Dec 3, 2025

Offhand looks OK to me. Does it work?

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

@gursewak1997 gursewak1997 force-pushed the fix/to-disk-signatures branch from 94a36f6 to ea1cb15 Compare December 3, 2025 20:24
cgwalters
cgwalters reviewed Dec 3, 2025
View reviewed changes
@gursewak1997 gursewak1997 force-pushed the fix/to-disk-signatures branch from ea1cb15 to e68793b Compare December 5, 2025 18:38
cgwalters
cgwalters reviewed Dec 5, 2025
View reviewed changes
Copy link
Collaborator

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nit, but looks sane to me; the key thing is verifying this works at least manually.

crates/kit/src/to_disk.rs
# Execute bootc installation, having the outer podman pull from
# the virtiofs store on the host, as well as the inner bootc.
# Mount /var/tmp into inner container to avoid cross-device link errors (issue #125)
export STORAGE_OPTS=additionalimagestore=${AIS}
Copy link
Collaborator

@cgwalters cgwalters Dec 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did this line move? See above

@gursewak1997 gursewak1997 force-pushed the fix/to-disk-signatures branch from e68793b to c1d7037 Compare December 5, 2025 19:37
@gursewak1997
Copy link
Collaborator Author

gursewak1997 commented Dec 5, 2025 •
edited
Loading

One nit, but looks sane to me; the key thing is verifying this works at least manually.

Did manually test it and the integration test test_to_disk_for_image_quay_io_almalinuxorg_almalinux_bootc_10_0 passed, which exercises the same code path.

@cgwalters
Copy link
Collaborator

cgwalters commented Dec 5, 2025

I edited this issue just now to have Closes: #126 but let's try to be sure we're doing that kind of thing in general in the future

@cgwalters
Copy link
Collaborator

cgwalters commented Dec 5, 2025

Hmmm I get the same error trying this out with the reproducer from #126

@gursewak1997 gursewak1997 force-pushed the fix/to-disk-signatures branch from c1d7037 to 9592974 Compare December 6, 2025 06:54
@gursewak1997
Fix signature handling with additionalimagestore
914df59 
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 <[email protected]>
@gursewak1997 gursewak1997 force-pushed the fix/to-disk-signatures branch from 9592974 to 914df59 Compare December 6, 2025 06:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cgwalters cgwalters cgwalters left review comments

+1 more reviewer

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

to-disk: Handle signatures

3 participants

@gursewak1997 @cgwalters