podman save: enforce signature removal #11714
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vrothberg The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
LGTM |
Enforce the removal of signatures in `podman save` to restore behavior prior to the migration to libimage. We may consider improving on that in the future. For details, please refer to the excellent summary by @mtrmac [1]. [NO TESTS NEEDED] - manually verified but exisiting tests need some further investigation (see [1]). [1] containers#11669 (comment) Signed-off-by: Valentin Rothberg <[email protected]>
vrothberg
force-pushed
the
save-remove-signatures
branch
from
0ff9e28 to
01bf8a6
Compare
There was a problem hiding this comment.
I was actually wrong, podman save supports docker-dir (c/image/directory), which can support signatures.
So I’m not, in the end, sure we want this right now — AFAICS this trades one regression for another.
OTOH I appreciate the point that many users just use podman save without specifying the format, so that case might not matter that much. Still…
| @@ -305,8 +305,6 @@ type ImageSaveOptions struct { | |||
| OciAcceptUncompressedLayers bool | |||
| // Output - write image to the specified path. | |||
| Output string | |||
| // Do not save the signature from the source image | |||
| RemoveSignatures bool | |||
There was a problem hiding this comment.
If this is the HTTP API, is it OK to change it like this? I guess unrecognized fields don’t cause failures?
There was a problem hiding this comment.
Thanks for checking. It's an internal (non-REST) API. The REST handlers always set it to true.
There was a problem hiding this comment.
My mistake, I should write down how things work one of these days. It’s pkg/domain/infra/tunnel.ImageEngine.Save(entities.ImageSaveOptions)→pkg/bindings/images.Export(pkg/bindings/images.ExportOptions)→pkg/api/handlers/libpod.ExportImages→pkg/domain/infra/abi.ImageEngine.Save(entities.ImageSaveOptions), right?
There was a problem hiding this comment.
Yes, that is the exact flow.
In the interest of time, with Podman v3.4 around the corner, I suggest merging and then refine from there. As you pointed out in the other PR, the fact that the tests didn't bark during the libimage migration is concerning. So I am afraid of a potential rabbit hole. |
Works for me, just please make sure we have an issue tracking this. #11669 was closed; do you want to reopen it? |
|
Here's the follow-up issue: #11718 |
Oops, my mistake. |
|
/lgtm |
github-actions
bot
added
the
locked - please file new issue/PR
Enforce the removal of signatures in
podman saveto restore behaviorprior to the migration to libimage. We may consider improving on that
in the future. For details, please refer to the excellent summary by
@mtrmac [1].
[1] #11669 (comment)
Signed-off-by: Valentin Rothberg [email protected]