fix skopeo copy can't decrypt to docker-daemon image

[ningmingxiao]

Signed-off-by: ningmingxiao ning.mingxiao@zte.com.cn

[ningmingxiao]

fix containers/skopeo#1703

[mtrmac]

Thanks for the PR.

This is not accurate: manifest.DockerV2Schema2ForeignLayerMediaTypeGzip just isn’t an encrypted MIME type, and this conversion logic is correct to reject encrypted inputs.

Besides the conceptual cleanliness point, this would also drop the encrypted MIME type even if not decrypting at all! That case must continue to fail.

I suppose the change from encrypted to non-encrypted MIME types should happen before even considering a format conversion.

[ningmingxiao]

will this new commit be ok? @mtrmac

[mtrmac]

I’m sorry about the previous very brief comment.

Just moving UpdateLayerInfos would, I think, work here, but the flip side is that the order of convertManifestIfRequiredWithUpdate vs. UpdateLayerInfos would be different in OCI vs. schema2, and we couldn’t make it consistent, otherwise a single step “convert from schema2 to OCI, and encrypt” would break. The code is messy and rife with with similar corner cases, so consistency of the implementation is valuable for maintenance.

---

Also, I suspect (but didn’t verify) that this implementation modifies the original image, which must not happen. That’s best ensured with a unit test within the existing test function — that would also help with making sure the use case in question doesn’t break.

---

I think we have to bite the conceptual bullet, accept, and express more the complexity … in this case I think manifestOCI1.UpdatedImage should do a separate check for info.CryptoOperation == types.Decrypt (and maybe, eventually, not in this PR, some other edits) and make an update of that kind first, so that it moves the image towards a “more interoperable” variant, then call convertManifestIfRequiredWithUpdate, and finally the original UpdateLayerInfos.

[mtrmac]

Also, I suspect (but didn’t verify) that this implementation modifies the original image

oh, maybe not, UpdateLayerInfos allocates a new slice. Either way, a test would be the best way to make sure.

[mtrmac]

Also, without unit tests we can’t be confident this will keep working.

[mtrmac]

I don’t understand at all what this is intended to do. m is local to this function, and just a pointer. Assigning to it, AFAICS, does nothing visible to the caller.

[mtrmac]

This comment is not correct an this place.

[mtrmac]

As mentioned previously, this would break one-step ”convert to OCI and encrypt”.

We need to split that, and do the decryption MIME type changes before format conversions, and encryption MIME type changes after format conversions.

[mtrmac]

#1932 should make all of this work. Thanks for your work in this area!