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

Correctly handle encryption/decryption changes in non-OCI formats #1932

Merged
merged 7 commits into from
Sep 8, 2023

Conversation

mtrmac
Copy link
Collaborator

@mtrmac mtrmac commented Apr 28, 2023

This

@mtrmac mtrmac force-pushed the encryption-UpdatedImage branch 4 times, most recently from daa120c to 4141d23 Compare May 5, 2023 17:59
@mtrmac mtrmac added the kind/bug A defect in an existing functionality (or a PR fixing it) label Jun 30, 2023
@mtrmac mtrmac force-pushed the encryption-UpdatedImage branch 5 times, most recently from a796912 to 879a27a Compare July 19, 2023 18:28
... to cut down on the repetitiveness.

Should not change (test) behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac mtrmac force-pushed the encryption-UpdatedImage branch 2 times, most recently from 0e54f28 to ea39042 Compare September 6, 2023 22:24
@mtrmac
Copy link
Collaborator Author

mtrmac commented Sep 6, 2023

This is now ready for review. To test:

openssl genrsa -out ociprivate.key 1024  
openssl rsa -in ociprivate.key -pubout > ocipublic.key
skopeo --override-os linux --insecure-policy copy --encryption-key jwe:./ocipublic.key docker://quay.io/libpod/alpine oci:nginx_encrypted
bin/skopeo --debug --override-os linux --insecure-policy copy --decryption-key ./ociprivate.key oci:nginx_encrypted docker-archive:t.tar

@mtrmac mtrmac marked this pull request as ready for review September 6, 2023 22:35
@mtrmac mtrmac changed the title WIP: Correctly handle encryption/decryption changes in non-OCI formats Correctly handle encryption/decryption changes in non-OCI formats Sep 6, 2023
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM

// FIXME: s/Zsdt/Zstd/ after ocicrypt with https://github.com/containers/ocicrypt/pull/91 is released
case ociencspec.MediaTypeLayerEnc, ociencspec.MediaTypeLayerGzipEnc, ociencspec.MediaTypeLayerZstdEnc,
ociencspec.MediaTypeLayerNonDistributableEnc, ociencspec.MediaTypeLayerNonDistributableGzipEnc, ociencspec.MediaTypeLayerNonDistributableZsdtEnc:
return nil, fmt.Errorf("Error during manifest conversion: %q: encrypted layers are not supported in docker images", layers[idx].MediaType)
Copy link
Member

Choose a reason for hiding this comment

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

Please don't start error messages with Error.
Otherwize podman and buildah end up with
Error: Error ...

Copy link
Member

Choose a reason for hiding this comment

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

I know there are other examples, which I can open a PR to fix.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the reminder, I have fixed the newly added errors at least.

... and add tests.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
…rmats

Previously this would try first converting to the other format,
and that would fail because the other format can't represent encrypted layers.

So, do the layer edits for decryption first.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
…yption

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@rhatdan
Copy link
Member

rhatdan commented Sep 8, 2023

LGTM

@rhatdan rhatdan merged commit a9b09b3 into containers:main Sep 8, 2023
9 checks passed
@mtrmac mtrmac deleted the encryption-UpdatedImage branch September 11, 2023 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug A defect in an existing functionality (or a PR fixing it)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants