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

zstd:chunked and layer encryption don’t make sense together #2485

Closed
mtrmac opened this issue Jul 19, 2024 · 3 comments · Fixed by #2321
Closed

zstd:chunked and layer encryption don’t make sense together #2485

mtrmac opened this issue Jul 19, 2024 · 3 comments · Fixed by #2321

Comments

@mtrmac
Copy link
Collaborator

mtrmac commented Jul 19, 2024

zstd:chunked is built around the idea that we can independently read individual parts of the layer via range requests.

That’s not directly possible when the layer is encrypted (when the encryption is a completely abstract stream transformation). So we see chunked pull failures like

DEBU[0000] GET http://localhost:50000/v2/encrypted/blobs/sha256:… 
DEBU[0000] Failed to retrieve partial blob: read zstd:chunked manifest: validating and decompressing TOC: invalid blob checksum, expected checksum sha256:…, got sha256:… 

That doesn’t really hurt because we then fall back to the non-chunked pull path, still at least the effort during a push is a waste. (Also, the annotations are unencrypted and leak a bit of information about layer contents — probably not more than the unencrypted config, but, still.)

It might be possible to implement this if the encryption format committed to being seekable/restartable (and many of cipher modes can be used that way, at least as long as they are not AEAD). OTOH that would require plumbing encryption directly through the GetBlobAt call stack, a fair bit of complexity, in addition to having ocicrypt choose the right algorithms and provide APIs to support seekable encryption.

Short-term, I think it makes most sense to just refuse to do this; if the user asks for zstd:chunked encryption, we should automatically (with a warning?) downgrade to non-chunked zstd output. Maybe, if the user explicitly asks for chunked on the CLI (as opposed to the config file), we should outright fail.

Cc: @giuseppe @rhatdan — this seems to be a fairly obvious decision, at least for now, but there might be use cases I don’t know about.

@giuseppe
Copy link
Member

yes, I agree we cannot use partial pulls with encryption for now and it will be better to reject such combination.

@mtrmac
Copy link
Collaborator Author

mtrmac commented Jul 22, 2024

One more aspect: This also breaks the “convert non-chunked images to chunked [in order to use composefs]” path for encrypted images, because it is built around GetBlobAt, and GetBlobAt does not (can not currently) decrypt the returned data.

@mtrmac
Copy link
Collaborator Author

mtrmac commented Jul 23, 2024

#2321 now includes logic to use zstd instead of zstd:chunked when encrypting (or to fail if the configuration is to use exactly the requested algorithm).

@mtrmac mtrmac linked a pull request Jul 23, 2024 that will close this issue
Luap99 added a commit to Luap99/libpod that referenced this issue Aug 12, 2024
c/image now throws a warning when using encryption and zstd:chunked as
they do not work together[1]. As CI uses default configs from fedora it
means rawhide now defaults to zstd:chunked which trigger the warning
there. To work around that force zstd compression.

[1] containers/image#2485

Signed-off-by: Paul Holzinger <[email protected]>
TomSweeneyRedHat added a commit to TomSweeneyRedHat/podman that referenced this issue Aug 13, 2024
Add a fix to pull to address a zstd:chunked issue as noted in
containers/image#2485

Signed-off-by: tomsweeneyredhat <[email protected]>
TomSweeneyRedHat added a commit to TomSweeneyRedHat/podman that referenced this issue Aug 14, 2024
Add a fix to pull and push tests to address a zstd:chunked issue as noted in
containers/image#2485

Signed-off-by: tomsweeneyredhat <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants