Skip to content
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.

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

podman save - signature support #11718

Closed
vrothberg opened this issue Sep 23, 2021 · 5 comments
Closed

podman save - signature support #11718

vrothberg opened this issue Sep 23, 2021 · 5 comments
Labels
locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. stale-issue

Comments

@vrothberg
Copy link
Member

vrothberg commented Sep 23, 2021

Taking an excellent summary by @mtrmac over from #11669 to not lose track:


History: If I’m reading the history correctly, #7956 did hard-code --remove-signatures at the API/engine abstraction level (which seems more work than any of the other options), and it was undone/lost in #10147 . I can’t see that the latter change was intentional.

Tests: … but I also wonder why the podman save remove signature test introduced in the first PR didn’t start failing. (Or rather, I wonder if/how it ever worked, the "save", "remove-signatures=true", without -- before --remove looks suspicious, and I can’t see that PR hooking a CLI flag up. I didn’t try actually running the test, to be fair.)

API: Isn’t pkg/domain/entities.ImageSaveOptions, with RemoveSignatures defaulting to false, a committed ABI by now? If so, I don’t think libimage can just decide to hard-code RemoveSignatures to true (or maybe it would require the caller to opt-in via something like ImageSaveOptions.RemoveSignaturesIfNecessary).

Behavior: I could conceptually see the case for either adding a --remove-signature flag that the user would have to affirmatively opt in to acknowledge that signatures won’t be represented, or just defaulting to that for formats that don’t support signatures (which is currently all of them, but that might not be the case forever), on the theory that the user would rather get something than just a failure, and missing signatures would end up a… teachable moment? The discussion in #7956 strongly leaned towards the latter, although it didn’t discuss the possible format evolution.

I guess if we don’t ask for an explicit opt-in, it would be cleaner for the knowledge about which formats don’t support signatures to be captured captured in libimage, not every single caller of that library. (Or maybe directly in c/image; right now c/image provides that information on a formed ImageDestination, which can be more informative but is a bad fit for image save; this should be a Can[Ever]SupportSignatures property of a transport — and we can’t change the ImageTransport interface directly, so that would involve adding a bit of infrastructure.)

So I think ideally the Podman CLI should set a new RemoveSignaturesIfNecessary boolean that gets forwarded all the way to libimage, and libimage should call a new c/image facility (c/image/transports.CanSupportSignatures(types.ImageTransport) -> {yes, no,unknown} — or maybe “unknown” should map to ”yes”). And I can quite understand the objection that this would be too much work.

Originally posted by @mtrmac in #11669 (comment)

@mtrmac
Copy link
Collaborator

mtrmac commented Sep 23, 2021

Note: podman save --format docker-dir does support signatures, so the code shouldn’t hard-code removing them in all cases.

@mtrmac
Copy link
Collaborator

mtrmac commented Sep 23, 2021

API: Isn’t pkg/domain/entities.ImageSaveOptions, with RemoveSignatures defaulting to false, a committed ABI by now?

No; the HTTP API uses /libpod/images/export, which has consistently hard-coded RemoveSignatures.

@github-actions
Copy link

A friendly reminder that this issue had no activity for 30 days.

@vrothberg
Copy link
Member Author

Just noticed it's a duplicate of #6265, so I am closing this one here.

@mtrmac
Copy link
Collaborator

mtrmac commented Jul 13, 2022

Despite the issue titles, from my quick skim the two have basically nothing in common. Maybe both should be closed in favor of shorter and more explicit issues :)

@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 20, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. stale-issue
Projects
None yet
Development

No branches or pull requests

2 participants