Verify size in verify.ReadCloser

[imjasonh]

This prevents cases where a descriptor says it describes a 100-byte blob, but then serves 1TB blob, which could lead to resource exhaustion. Instead, we'll only read as many bytes as the descriptor claims there should be, and fail if the digest doesn't match after reading that many. This also verifies the content isn't shorter than described.

This adds a HEAD request to remote.Layer to fetch the size of the blob before reading and verifying it. If that's not acceptable we could work around it.

[jonjohnsonjr]

I'm tempted to have these be separate things because in some cases we don't know the size but we do know the digest (e.g. diffids and schema 1).

[imjasonh]

I'm tempted to have these be separate things because in some cases we don't know the size but we do know the digest (e.g. diffids and schema 1).

That's reasonable. It doesn't look like we use verify at all with either of those though, so maybe we should update verify.ReadCloser to take size into account, then add verify.ReadCloserNoSize (naming tbd) when we need it. That might be something we could use with remote.Layer for instance.

[jonjohnsonjr]

That might be something we could use with remote.Layer for instance.

Might still be useful to use the Content-Length for that, but also the response body probably does that itself... we should maybe be confirming Size == Content-Length and doing a LimitReader?

[codecov-commenter]

Codecov Report

Merging #1044 (c404bd4) into main (f0ce227) will increase coverage by 0.09%.
The diff coverage is 86.66%.

@@            Coverage Diff             @@
##             main    #1044      +/-   ##
==========================================
+ Coverage   75.03%   75.13%   +0.09%     
==========================================
  Files         107      107              
  Lines        5079     5083       +4     
==========================================
+ Hits         3811     3819       +8     
+ Misses        725      719       -6     
- Partials      543      545       +2     

Impacted Files	Coverage Δ
pkg/v1/remote/descriptor.go	73.36% <71.42%> (ø)
internal/verify/verify.go	100.00% <100.00%> (ø)
pkg/v1/remote/image.go	79.72% <100.00%> (ø)
pkg/v1/tarball/image.go	76.82% <0.00%> (+2.43%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f0ce227...c404bd4. Read the comment docs.

[imjasonh]

I decided to verify using Content-Length in fetchBlob so that remote.Layer doesn't have to do a HEAD. This made the diff smaller.

[jonjohnsonjr]

Do we want to validate config blob size as well?

go-containerregistry/pkg/v1/remote/image.go

Line 103 in 4c244d6

body, err := r.fetchBlob(r.context, m.Config.Digest)

[imjasonh]

Do we want to validate config blob size as well?

go-containerregistry/pkg/v1/remote/image.go

Line 103 in 4c244d6

body, err := r.fetchBlob(r.context, m.Config.Digest)

That'll be validated against the Content-Length at least. I'm not sure what to do if the descriptor size disagrees with the content-length header. Nothing would catch that with this version, but nothing would have caught it before either AFAIK.