Expect UncompressedDigest to be set for partial pulls, enforce DiffID match #2613
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
added a commit
to mtrmac/libpod
that referenced
this pull request
Oct 30, 2024
Signed-off-by: Miloslav Trmač <[email protected]>
mtrmac
added a commit
to mtrmac/libpod
that referenced
this pull request
Oct 30, 2024
Signed-off-by: Miloslav Trmač <[email protected]>
mtrmac
added a commit
to mtrmac/libpod
that referenced
this pull request
Oct 30, 2024
Signed-off-by: Miloslav Trmač <[email protected]>
mtrmac
commented
Oct 31, 2024
mtrmac
force-pushed
the
wip-authentic
branch
from
090e94d to
b859a03
Compare
mtrmac
added a commit
to mtrmac/libpod
that referenced
this pull request
Nov 18, 2024
Signed-off-by: Miloslav Trmač <[email protected]>
mtrmac
added a commit
to mtrmac/libpod
that referenced
this pull request
Nov 18, 2024
Signed-off-by: Miloslav Trmač <[email protected]>
mtrmac
added a commit
to mtrmac/libpod
that referenced
this pull request
Nov 18, 2024
Signed-off-by: Miloslav Trmač <[email protected]>
mtrmac
force-pushed
the
wip-authentic
branch
3 times, most recently
from
e46c8d0 to
eb0db7b
Compare
mtrmac
added a commit
to mtrmac/libpod
that referenced
this pull request
Nov 25, 2024
> go mod edit -replace github.com/containers/image/v5=github.com/mtrmac/image/v5@wip-authentic Signed-off-by: Miloslav Trmač <[email protected]>
mtrmac
force-pushed
the
wip-authentic
branch
4 times, most recently
from
95cdcf3 to
57b0637
Compare
mtrmac
added a commit
to mtrmac/libpod
that referenced
this pull request
Nov 26, 2024
> go mod edit -replace github.com/containers/image/v5=github.com/mtrmac/image/v5@wip-authentic Signed-off-by: Miloslav Trmač <[email protected]>
mtrmac
force-pushed
the
wip-authentic
branch
3 times, most recently
from
137b760 to
4fb4df8
Compare
mtrmac
added a commit
to mtrmac/libpod
that referenced
this pull request
Nov 28, 2024
> go mod edit -replace github.com/containers/image/v5=github.com/mtrmac/image/v5@wip-authentic Signed-off-by: Miloslav Trmač <[email protected]>
mtrmac
added a commit
to mtrmac/libpod
that referenced
this pull request
Nov 29, 2024
> go mod edit -replace github.com/containers/image/v5=github.com/mtrmac/image/v5@wip-authentic Signed-off-by: Miloslav Trmač <[email protected]>
mtrmac
force-pushed
the
wip-authentic
branch
from
4fb4df8 to
7987093
Compare
|
@giuseppe RFC. I still need to address / review some corner cases, but I think the broad outline is settled now, and Podman tests are passing. |
mtrmac
force-pushed
the
wip-authentic
branch
2 times, most recently
from
d7fdde4 to
c1036a6
Compare
mtrmac
force-pushed
the
wip-authentic
branch
from
c1036a6 to
290bc1e
Compare
|
@giuseppe PTAL for an early review. This is mostly untested, but it should be feature-complete and comprehensive. Contrary to the original plan for containers/storage#2180 , this minimizes the impact on |
|
LGTM, though given my unfamiliarity with the codebase a review from someone else would be much more valuable. |
mtrmac
added a commit
to mtrmac/libpod
that referenced
this pull request
Jan 21, 2025
> go mod edit -replace github.com/containers/image/v5=github.com/mtrmac/image/v5@wip-authentic Signed-off-by: Miloslav Trmač <[email protected]>
|
LGTM |
|
Great work @mtrmac ! |
reused.Digest is not always blobDigest, it might be uncompressedDigest; but we must have a blobDiffIDs entry for reused.Digest. Signed-off-by: Miloslav Trmač <[email protected]>
Signed-off-by: Miloslav Trmač <[email protected]>
Signed-off-by: Miloslav Trmač <[email protected]>
... because we will start enforcing that the DiffID values match. Signed-off-by: Miloslav Trmač <[email protected]>
We will use the trustedLayerIdentityData for other purposes in the caller as well. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
Keep the commit queuing logic together, this is more of an implementation detail of commitLayer. Only moves unchanged code, should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
It's fairly isolated from the rest of the function, and if split, it can have unit tests. Those tests are valuable to ensure that layer IDs continue to behave the expected way and maximize layer reuse (although we are not making an API commitment to layer ID values). Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
... to simplify some of the repetitive logging code. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
untrustedLayerDiffID currently specializes the "not available yet" case; also specialize the "image does not provide this at all" case, which we will need to handle. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
Two different locations in the function need the data, and the caller must have it available; so always passing it in simplifies the implementation and removes an impossible error path. This might hypothetically make layer reuse a bit worse, if we happened to learn something for trustedLayerIdentityData from processing other layers of the same image, but reusing the same layer twice within an image should be rare. Signed-off-by: Miloslav Trmač <[email protected]>
…ema1 images Should not change behavior; we call GetTOCDigest in copy.imageCopier.copyLayer before reaching PutBlobPartial, so the new error path should not be reachable. Signed-off-by: Miloslav Trmač <[email protected]>
…ID values If a layer has a TOC, require that it must have a DiffID commitment, or refuse to pull it partially. Layers without a TOC continue to be allowed to use the partial pull code path, and we don't even require config's RootFS.DiffID to be present. Signed-off-by: Miloslav Trmač <[email protected]>
Remove some completely redundant comments to shorten the code, clarify where appropriate. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
- If a layer has a TOC digest (i.e. could possibly be pulled partially), and c/storage has computed the uncompressed digest, require that the config's RootFS.DiffIDs exists and matches. This fixes the "view ambiguity" of partially-pulled layers. - For _all_ layers, if RootFS.DiffIDs exists and we know the layer's uncompressed digest, also require the RootFS.DiffIDs value to match. This might be a compatibility break, but Docker requires these values anyway. - We happen to allow setting DiffIDs to empty values, if the layer does not have a TOC digest (so there is no risk of "view ambiguity"). Signed-off-by: Miloslav Trmač <[email protected]>
mtrmac
added a commit
to mtrmac/libpod
that referenced
this pull request
Jan 21, 2025
This resolves the "signing ambiguity" by requiring that images must have a DiffID entry, and it must match, in partial pulls. Signed-off-by: Miloslav Trmač <[email protected]>
mtrmac
added a commit
to mtrmac/libpod
that referenced
this pull request
Jan 22, 2025
This resolves the "signing ambiguity" by requiring that images must have a DiffID entry, and it must match, in partial pulls. Signed-off-by: Miloslav Trmač <[email protected]>
mtrmac
added a commit
to mtrmac/libpod
that referenced
this pull request
Jan 22, 2025
This resolves the "signing ambiguity" by requiring that images must have a DiffID entry, and it must match, in partial pulls. Signed-off-by: Miloslav Trmač <[email protected]>
mtrmac
added a commit
to mtrmac/libpod
that referenced
this pull request
Jan 22, 2025
This resolves the "signing ambiguity" by requiring that images must have a DiffID entry, and it must match, in partial pulls. Signed-off-by: Miloslav Trmač <[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.
Requires containers/storage#2155
RootFS.DiffIDs, or refuse to pull it partially.RootFS.DiffIDsto be present.RootFS.DiffIDsexists and matches. This fixes the “view ambiguity” of partially-pulled layers.RootFS.DiffIDsexists and we know the layer’s uncompressed digest, also require theRootFS.DiffIDvalue to match. This might be a compatibility break, but Docker requires these values anyway.DiffIDsto empty values, if the layer does not have a TOC digest (so there is no risk of “view ambiguity”).See individual commit messages for details.