Fix unwanted reuse of encrypted layers #1533
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mtrmac
force-pushed
the
uncompressed-reuse
branch
from
7cf0f24
to
f016c22
Compare
|
@vrothberg @nalind @hshiina @lumjjb PTAL. I haven’t tested this PR in practice yet, still I’d like to maximize the opportunity for others to find problems in this PR. |
mtrmac
force-pushed
the
uncompressed-reuse
branch
from
f016c22
to
b86c63d
Compare
mtrmac
changed the title
mtrmac
force-pushed
the
uncompressed-reuse
branch
from
b86c63d
to
496dcbb
Compare
|
Updated now to remove one more place where “we have to compute DiffIDs” was set when encryption is involved. (I can find no record of why there were two places doing this.) Tested with rm ~/.local/share/containers/cache/blob-info-cache-v1.boltdb
rm -rf t t-{encrypted,unencrypted,decrypted}
# --dest-decompress to make sure the (uncompressed digest, compressed digest)-association-based reuse logic is involved on pushes
bin/skopeo --debug --override-os linux copy --format oci --dest-decompress docker://quay.io/libpod/alpine:latest dir:t
bin/skopeo --debug --override-os linux copy --encryption-key jwe:./encryption-test-public.pem dir:t docker://$registry/first-encrypted
bin/skopeo --debug --override-os linux copy docker://$registry/first-encrypted dir:t-encrypted
# This used to reuse the encrypted blob from first-encrypted, and is no longer doing so.
bin/skopeo --debug --override-os linux copy dir:t docker://$registry/second-unencrypted
bin/skopeo --debug --override-os linux copy docker://$registry/second-unencrypted dir:t-unencrypted
# Sanity check that the encrypted content can be decrypted
bin/skopeo --debug --override-os linux copy --decryption-key encryption-test-private.pem dir:t-encrypted dir:t-decrypted
# (And manually inspect the 4 t* directories) |
hshiina
pushed a commit
to hshiina/buildah
that referenced
this pull request
Apr 28, 2022
This fix modifies the test "commit oci encrypt to registry" to verify that encrypted layers are not reused for a non-encrypted image. see: containers/image#1533
hshiina
pushed a commit
to hshiina/buildah
that referenced
this pull request
Apr 28, 2022
This fix modifies the test "commit oci encrypt to registry" to verify that encrypted layers are not reused for a non-encrypted image. see: containers/image#1533 Signed-off-by: Hironori Shiina <[email protected]>
|
I reproduced this bug in buildah tests(containers/buildah#3941) and confirmed the test succeeds with this fix. Thanks! |
|
@rhatdan, @vrothberg this was reported as a RHEL bug, so it would be useful to include into the upcoming vendor dance. |
AFAICS encryption doesn't _actually_ need diffIDs, at least I can't see any encryption-related use of the computed value; the boolean seems to only be set to avoid reuse (which risks unwanted reuse of plaintext data, or revealing relationships between ciphertext/plaintext to network observers). So, use two extra variables to be clear about the logic of the code. (Also reorder one condition to check a local variable first before calling methods, a microoptimizaion.) Should not change outcome of the operation (but the time and CPU usage no longer spent to unnecessarily compute DiffID values might be noticeable). Signed-off-by: Miloslav Trmač <[email protected]>
Operations that involve encryption/decryption are already restricted e.g. not to use TryReusingBlob; but operations that don't themselves involve encryption could still find encrypted blobs in the blob info cache, and potentially use them in other contexts. To avoid that, use a somewhat big hammer of just not calling RecordDigestUncompressedPair on that. Note that this does not help if the blob info cache has already added such entries before this change; it only makes a difference for the future. We continue to call RecordKnownLocation with encrypted data; simple copies of encrypted images from one registry to another (which don't encrypt/decrypt as part of the copy) can benefit from e.g. cross-repo blob reuse just fine. It seems likely that a more precise logic which records more data and allows more blob reuse could be built, but it's not trivially obvious to me that it would be safe, so this change only does the conservative thing to avoid known breakage. There is another RecordDigestUncompressedPair call in c/image/storage; that one is safe, because it only works on a pair of unencrypted digests (for a compressed layer, PutBlobWithOptions receives an empty digest value, and a necessarily decrypted data stream; using that, it computes is own digests of the decrypted possibly-compressed and unencrypted uncommpressed data streams). Signed-off-by: Miloslav Trmač <[email protected]>
mtrmac
force-pushed
the
uncompressed-reuse
branch
from
496dcbb
to
d1d16eb
Compare
hshiina
pushed a commit
to hshiina/buildah
that referenced
this pull request
May 4, 2022
This fix modifies the test "commit oci encrypt to registry" to verify that encrypted layers are not reused for a non-encrypted image. see: containers/image#1533 Signed-off-by: Hironori Shiina <[email protected]>
hshiina
pushed a commit
to hshiina/buildah
that referenced
this pull request
May 4, 2022
This fix modifies the test "commit oci encrypt to registry" to verify that encrypted layers are not reused for a non-encrypted image. see: containers/image#1533 Signed-off-by: Hironori Shiina <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Per #1314 :
Operations that involve encryption/decryption are already restricted e.g. not to use
TryReusingBlob; but operations that don't themselves involve encryption could still find encrypted blobs in the blob info cache, and potentially use them in other contexts.To avoid that, use a somewhat big hammer of just not calling
RecordDigestUncompressedPairon that. Note that this does not help if the blob info cache has already added such entries before this change; it only makes a difference for the future.We continue to call
RecordKnownLocationwith encrypted data; simple copies of encrypted images from one registry to another (which don't encrypt/decrypt as part of the copy) can benefit from e.g. cross-repo blob reuse just fine.It seems likely that a more precise logic which records more data and allows more blob reuse could be built, but it's not trivially obvious to me that it would be safe, so this change only does the conservative thing to avoid known breakage.
See individual commit messages for details.