Skip to content

Commit

Permalink
Avoid calls to RecordDigestUncompressedPair that involve encrypted data
Browse files Browse the repository at this point in the history
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>
  • Loading branch information
mtrmac committed Apr 29, 2022
1 parent 1cb6653 commit d1d16eb
Showing 1 changed file with 32 additions and 16 deletions.
48 changes: 32 additions & 16 deletions copy/copy.go
Original file line number Diff line number Diff line change
Expand Up @@ -1253,9 +1253,17 @@ func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, to
return types.BlobInfo{}, "", errors.Wrap(diffIDResult.err, "computing layer DiffID")
}
logrus.Debugf("Computed DiffID %s for layer %s", diffIDResult.digest, srcInfo.Digest)
// This is safe because we have just computed diffIDResult.Digest ourselves, and in the process
// we have read all of the input blob, so srcInfo.Digest must have been validated by digestingReader.
ic.c.blobInfoCache.RecordDigestUncompressedPair(srcInfo.Digest, diffIDResult.digest)
// Don’t record any associations that involve encrypted data. This is a bit crude,
// some blob substitutions (replacing pulls of encrypted data with local reuse of known decryption outcomes)
// might be safe, but it’s not trivially obvious, so let’s be conservative for now.
// This crude approach also means we don’t need to record whether a blob is encrypted
// in the blob info cache (which would probably be necessary for any more complex logic),
// and the simplicity is attractive.
if !encryptingOrDecrypting {
// This is safe because we have just computed diffIDResult.Digest ourselves, and in the process
// we have read all of the input blob, so srcInfo.Digest must have been validated by digestingReader.
ic.c.blobInfoCache.RecordDigestUncompressedPair(srcInfo.Digest, diffIDResult.digest)
}
diffID = diffIDResult.digest
}
}
Expand Down Expand Up @@ -1604,19 +1612,27 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcStream io.Reader, sr
return types.BlobInfo{}, errors.Errorf("Internal error writing blob %s, blob with digest %s saved with digest %s", srcInfo.Digest, inputInfo.Digest, uploadedInfo.Digest)
}
if digestingReader.validationSucceeded {
// If compressionOperation != types.PreserveOriginal, we now have two reliable digest values:
// srcinfo.Digest describes the pre-compressionOperation input, verified by digestingReader
// uploadedInfo.Digest describes the post-compressionOperation output, computed by PutBlob
// (because inputInfo.Digest == "", this must have been computed afresh).
switch compressionOperation {
case types.PreserveOriginal:
break // Do nothing, we have only one digest and we might not have even verified it.
case types.Compress:
c.blobInfoCache.RecordDigestUncompressedPair(uploadedInfo.Digest, srcInfo.Digest)
case types.Decompress:
c.blobInfoCache.RecordDigestUncompressedPair(srcInfo.Digest, uploadedInfo.Digest)
default:
return types.BlobInfo{}, errors.Errorf("Internal error: Unexpected compressionOperation value %#v", compressionOperation)
// Don’t record any associations that involve encrypted data. This is a bit crude,
// some blob substitutions (replacing pulls of encrypted data with local reuse of known decryption outcomes)
// might be safe, but it’s not trivially obvious, so let’s be conservative for now.
// This crude approach also means we don’t need to record whether a blob is encrypted
// in the blob info cache (which would probably be necessary for any more complex logic),
// and the simplicity is attractive.
if !encrypted && !decrypted {
// If compressionOperation != types.PreserveOriginal, we now have two reliable digest values:
// srcinfo.Digest describes the pre-compressionOperation input, verified by digestingReader
// uploadedInfo.Digest describes the post-compressionOperation output, computed by PutBlob
// (because inputInfo.Digest == "", this must have been computed afresh).
switch compressionOperation {
case types.PreserveOriginal:
break // Do nothing, we have only one digest and we might not have even verified it.
case types.Compress:
c.blobInfoCache.RecordDigestUncompressedPair(uploadedInfo.Digest, srcInfo.Digest)
case types.Decompress:
c.blobInfoCache.RecordDigestUncompressedPair(srcInfo.Digest, uploadedInfo.Digest)
default:
return types.BlobInfo{}, errors.Errorf("Internal error: Unexpected compressionOperation value %#v", compressionOperation)
}
}
if uploadCompressorName != "" && uploadCompressorName != internalblobinfocache.UnknownCompression {
c.blobInfoCache.RecordDigestCompressorName(uploadedInfo.Digest, uploadCompressorName)
Expand Down

0 comments on commit d1d16eb

Please sign in to comment.