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

Fix unwanted reuse of encrypted layers #1533

Merged
merged 2 commits into from May 1, 2022

Conversation

mtrmac
Copy link
Collaborator

@mtrmac mtrmac commented Apr 27, 2022

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 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.

See individual commit messages for details.

@mtrmac
Copy link
Collaborator Author

mtrmac commented Apr 27, 2022

@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.

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.

Changes LGTM

@mtrmac mtrmac changed the title Fix unwanted reuse of compressed layers Fix unwanted reuse of encrypted layers Apr 28, 2022
@mtrmac
Copy link
Collaborator Author

mtrmac commented Apr 28, 2022

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)

@mtrmac mtrmac marked this pull request as ready for review April 28, 2022 18:33
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 <shiina.hironori@jp.fujitsu.com>
@hshiina
Copy link

hshiina commented Apr 28, 2022

I reproduced this bug in buildah tests(containers/buildah#3941) and confirmed the test succeeds with this fix. Thanks!

@mtrmac
Copy link
Collaborator Author

mtrmac commented Apr 29, 2022

@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č <mitr@redhat.com>
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č <mitr@redhat.com>
Copy link
Contributor

@lumjjb lumjjb left a comment

Choose a reason for hiding this comment

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

LGTM! @mtrmac this is an awesome PR! It opens this up for more granular encryption policy!

@rhatdan rhatdan merged commit 7f8c6e0 into containers:main May 1, 2022
@mtrmac mtrmac deleted the uncompressed-reuse branch May 2, 2022 16:49
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 <shiina.hironori@jp.fujitsu.com>
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 <shiina.hironori@jp.fujitsu.com>
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 this pull request may close these issues.

None yet

5 participants