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

Commits on Apr 29, 2022

  1. Don't require DiffID computation when encryption is involved

    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>
    mtrmac committed Apr 29, 2022
    Copy the full SHA
    1cb6653 View commit details
    Browse the repository at this point in the history
  2. Avoid calls to RecordDigestUncompressedPair that involve encrypted data

    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>
    mtrmac committed Apr 29, 2022
    Copy the full SHA
    d1d16eb View commit details
    Browse the repository at this point in the history