Repository navigation
Add end-to-end checksum verification on GetObject - #2272
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds ChangesChecksum verification
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Caller
participant getObject
participant HTTPResponse
participant ChecksumVerifyingReader
Caller->>getObject: Request ranged object
getObject->>HTTPResponse: Fetch ranged response
HTTPResponse-->>getObject: Return response body
getObject->>ChecksumVerifyingReader: Wrap body when verification applies
Caller->>ChecksumVerifyingReader: Read response bytes
ChecksumVerifyingReader-->>Caller: Return bytes or checksum error
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
jiuker
changed the title
GetObject (streaming) and FGetObject (file download) paths, ensuring the received data matches the server-advertised checksum.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api-get-object-file.go`:
- Around line 74-80: Update FGetObject’s checksum flow around ObjectChecksum so
resumed downloads include the existing .part.minio bytes in checkSumHasher
before newly downloaded data is appended; preserve the current append/resume
behavior and ensure checksum verification compares the digest of the complete
object.
- Around line 78-80: Update the checksum finalization flow in the checksum
handling around ObjectChecksum so it does not call Sum on a nil checkSumHasher
when no supported server checksum algorithm is available. Guard the finalization
logic with checkSumHasher != nil, or return an explicit
unsupported/missing-checksum error, while preserving normal checksum
verification for supported algorithms.
In `@api-get-object.go`:
- Around line 131-134: Restrict checksum initialization in the read path around
ObjectChecksum to complete sequential reads starting at offset zero, excluding
ReadAt and any ranged or restarted stream. Reset or disable the checksum hasher
when offset-driven re-fetches, seeks, or separate ranges occur, and apply the
same guard consistently at the additional checksum handling blocks.
In `@checksum.go`:
- Line 350: Remove the extra trailing blank line immediately after
ObjectChecksum in checksum.go, leaving only the required newline and preserving
all surrounding code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6e8378f7-7bb7-4553-bf06-adc3548dafc7
📒 Files selected for processing (3)
api-get-object-file.goapi-get-object.gochecksum.go
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api-get-object-file.go (1)
134-140:⚠️ Potential issue | 🔴 CriticalGuard checksum finalization against a nil hasher.
When checksum verification is enabled but no supported checksum was initialized, Line 135 calls
Sum(nil)on a nilhash.Hashand panics. Return an explicit missing/unsupported-checksum error or guard finalization withcheckSumHasher != nil.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api-get-object-file.go` around lines 134 - 140, Guard checksum finalization in the opts.Checksum branch before calling checkSumHasher.Sum(nil). When no supported checksum initialized checkSumHasher, return an explicit missing/unsupported-checksum error (or skip finalization via a nil check) while preserving existing mismatch handling for initialized hashers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api-get-object-file.go`:
- Around line 110-112: Update the checksum initialization in the object download
resume flow around ObjectChecksum so existing partial .part.minio files are
included in the digest before appending. When checksum verification is enabled,
hash the existing partial file contents before continuing, or discard and
restart the partial file; do not restrict initialization to st.Size() == 0.
---
Outside diff comments:
In `@api-get-object-file.go`:
- Around line 134-140: Guard checksum finalization in the opts.Checksum branch
before calling checkSumHasher.Sum(nil). When no supported checksum initialized
checkSumHasher, return an explicit missing/unsupported-checksum error (or skip
finalization via a nil check) while preserving existing mismatch handling for
initialized hashers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 11a45ac7-52d3-4b1d-a754-0da2709d4fdc
📒 Files selected for processing (1)
api-get-object-file.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api-get-object-file.go`:
- Around line 116-118: The checksum verification flow around ObjectChecksum must
also cover resumed downloads with an existing .part.minio prefix. When
opts.Checksum is enabled and the partial file is non-empty, initialize the
hasher and feed it the existing prefix before appending new data, or discard and
restart that partial file; ensure the later verification cannot be bypassed and
corrupted content is not finalized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1907a4eb-3f3d-4c3d-aa30-db3f8282e113
📒 Files selected for processing (1)
api-get-object-file.go
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
api-get-object-file.go (1)
112-124: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not bypass verification for resumed downloads.
When
.part.minioalready contains bytes,st.Size() == 0is false, socheckSumHasherremains nil and the file is renamed without checksum verification. Hash the existing prefix before appending, or discard/restart the partial file when verification is enabled.Also applies to: 134-140
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api-get-object-file.go` around lines 112 - 124, The resumed-download path must not skip checksum verification when the existing part file is non-empty. Update the checksum setup around ObjectChecksum and io.CopyN to either hash the existing filePart prefix before appending, preserving verification across the complete file, or discard and restart the partial file whenever opts.Checksum is enabled; ensure the final rename occurs only after the complete checksum is validated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api-get-object.go`:
- Around line 76-81: Update checksum verification around the checkSumHasher
state and the ReadAt, seek, and range re-fetch paths so hashing occurs only for
one complete sequential stream beginning at offset zero. Reset or disable the
hasher whenever a range request, non-sequential offset, or stream restart
occurs, and only compare the digest once the full linear read has completed.
In `@checksum.go`:
- Around line 319-348: Update ObjectChecksum to enable full-object verification
only when obj.ChecksumMode is FULL_OBJECT; for COMPOSITE (and other composite
modes), avoid treating the advertised composite checksum as a hash of downloaded
bytes by implementing per-part composite calculation or returning an explicit
unsupported-mode error.
---
Duplicate comments:
In `@api-get-object-file.go`:
- Around line 112-124: The resumed-download path must not skip checksum
verification when the existing part file is non-empty. Update the checksum setup
around ObjectChecksum and io.CopyN to either hash the existing filePart prefix
before appending, preserving verification across the complete file, or discard
and restart the partial file whenever opts.Checksum is enabled; ensure the final
rename occurs only after the complete checksum is validated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 48b3c1ef-9ffd-4349-8f64-b47168d63d48
📒 Files selected for processing (3)
api-get-object-file.goapi-get-object.gochecksum.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api-get-object-file.go`:
- Around line 121-124: Update the filePart opening logic used by checksum
verification so the descriptor passed to io.CopyN can be read, either by opening
it read-write or by using a separate read-only descriptor. Preserve append-only
writes for the resumed range and ensure the existing checksum flow in the
partial-file handling remains intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a21fd169-1034-41bc-93cf-0ea68d54540f
📒 Files selected for processing (1)
api-get-object-file.go
harshavardhana
left a comment
harshavardhana
left a comment
There was a problem hiding this comment.
This not correct fix @jiuker the checksum must be computed as a stream on the reader basically you need to use a io.Pipe() and then at the EOF verify the checksum is correct, if not fail the operation.
Check how our aistor hash.Reader does this - that is how you need to implement this.
9ca66c7 to
598ed53
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api-get-object.go (1)
523-528: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep
o.objectInfo.Sizeas the total object size.
Statstores the remaining size in persistent metadata. A laterSeekcomparesnewOffsetwitho.objectInfo.Size, so a valid absolute offset can incorrectly returnio.EOFafterStatfollows a read.
api-get-object.go#L523-L528: copyo.objectInfoto a local result and set the remaining size only on that copy.api-get-object.go#L551-L553: apply the same local-copy behavior after the initialStatrequest.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api-get-object.go` around lines 523 - 528, Keep o.objectInfo.Size as the total object size by copying o.objectInfo to a local result before calculating the remaining size, then set the adjusted Size only on that copy and return it at api-get-object.go lines 523-528. Apply the same local-copy behavior after the initial Stat request at api-get-object.go lines 551-553; both sites must preserve the persistent metadata for later Seek operations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api-get-object.go`:
- Around line 825-829: Update the checksum-wrapping logic around
NewChecksumVerifyingReader so eligibility is based on whether the response plus
any already verified prefix covers the complete object, not merely whether
headers["Range"] is set. Verify normal full-object responses, skip arbitrary
partial ranges, and for resumed FGetObject downloads incorporate the existing
prefix with the received suffix before comparing against the full-object
checksum.
In `@checksum.go`:
- Around line 332-353: Update the MD5 and SHA256 branches in
NewChecksumVerifyingReader to use the configured md5Hasher and sha256Hasher
functions instead of the default implementations returned by
ChecksumType.Hasher(). Preserve the existing expected-checksum assignments and
reader construction for both cases.
- Around line 373-377: Move checksum comparison out of the io.EOF-only branch
and expose an explicit final verification operation on the checksum reader, such
as a method on its checksum wrapper. Call that verification immediately after
io.CopyN in FGetObject and before the file rename, while preserving the existing
mismatch error details; do not depend on objectReader.Close for verification.
---
Outside diff comments:
In `@api-get-object.go`:
- Around line 523-528: Keep o.objectInfo.Size as the total object size by
copying o.objectInfo to a local result before calculating the remaining size,
then set the adjusted Size only on that copy and return it at api-get-object.go
lines 523-528. Apply the same local-copy behavior after the initial Stat request
at api-get-object.go lines 551-553; both sites must preserve the persistent
metadata for later Seek operations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c3964c0a-6d71-4654-b184-9b7247a4b0cf
📒 Files selected for processing (2)
api-get-object.gochecksum.go
This comment was marked as outdated.
This comment was marked as outdated.
allanrogerr
left a comment
allanrogerr
left a comment
There was a problem hiding this comment.
Two small resource fixes on top of the current head. Both are in the paths this PR adds. Full re-validation is in a separate comment.
Picking the checksum by value rather than by algorithm name was the right callThe current head decides which checksum to verify by looking at which checksum value the response carries. The first version looked at the Almost no server sends that header on a download:
On AIStor the header arrived on So the first version verified downloads on AIStor Verified on the current headHead
Threads I opened that are now settledSix of them: the algorithm-name gate, the same condition in the download path, the resume path that expected checksum headers on a partial response, the unused import, the download option together with its documentation, and the exported reader type with its read behaviour. The results above are the evidence that they are settled rather than merely closed. The new documentation note, that a read stopping before the end of the object is not verified, is accurate and worth keeping. Still open
|
|
Note that the RDMA fix with dependabot is in #2276 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api-get-object-file.go`:
- Around line 101-109: Update the object retrieval flow around c.getObject and
the checksum setup so every failure path closes resources: close
opts.checkSumReader before returning when c.getObject fails, defer
objectReader.Close() immediately after a successful call, and remove the
existing success-only close so errors from io.CopyN or VerifyChecksum also
release the response body and verifier.
In `@checksum-verify_test.go`:
- Around line 170-209: Extend the checksum cases in the FGetObject test to cover
a full-download response containing corrupt server bytes, while retaining the
existing corrupted-prefix case. In the checksum mismatch branch, compute or
retain the corresponding filePartPath and assert that the .part.minio file does
not exist after the error, for every mismatch case; preserve the existing
destination-not-committed assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3e3d1e9c-7b69-4de9-a5a3-c0cd981a5a5c
📒 Files selected for processing (5)
api-get-object-file.goapi-get-object.goapi-get-options.gochecksum-verify_test.gochecksum.go
harshavardhana
changed the title
GetObject (streaming) and FGetObject (file download) paths, ensuring the received data matches the server-advertised checksum.
Evidence for the change requested in review 4816874198@harshavardhana your review asked for the checksum to be computed as a stream on the reader, verified at the end of the object, and for the operation to fail on a mismatch. That review sits on Head Streaming, then verified at the end. The download body is wrapped in a reader that hashes every chunk as it is read. When the read reaches the end of the object it verifies the total, and returns the mismatch error in place of the end-of-file signal, so the caller's read fails ( File downloads fail before the file is committed. Measured against live servers. One byte of the response body was corrupted in transit:
The committed tests also pass with the race detector: One difference from your wording. There is no Your review cannot be cleared from this side, and I have not dismissed it. It needs either your re-review or a dismissal by a maintainer. |
@jiuker resolved this without io.Pipe. The download body is wrapped in a reader that hashes each chunk as it is read, verifies at EOF, and returns the mismatch error in place of io.EOF. |
@allanrogerr - Could you rephrase this in your own words? I've read it twice - and I still don't understand what the issue is? |
|
Here // Read reads data and hashes it. At EOF the checksum is verified; on
// mismatch the mismatch error is returned in place of io.EOF.
func (c *checksumVerifyingReader) Read(p []byte) (int, error) {
n, err := c.ReadCloser.Read(p)
if n > 0 {
n2, werr := c.Write(p[:n])
if werr != nil {
return n, werr
}
if n2 != n {
return n, io.ErrShortWrite
}
}
if err == io.EOF {
if verr := c.VerifyChecksum(); verr != nil {
return n, verr
}
}
return n, err
}which is the same as io.TeeReader. @allanrogerr @harshavardhana |
@klauspost It's my idea, paraphrased. Basically Harsha wanted:
This has been done and the PR should be mergeable. I'll reduce on the verbosity and increase the clarity in future. |
4 participants
fix #2246
Summary
Add end-to-end checksum verification to
GetObject(streaming) andFGetObject(file download): when
GetObjectOptions.Checksumis set and the responseadvertises a full-object checksum, the received data is verified against it.
Changes
checksum.gochecksumVerifyingReaderwrapping the response body: it hashes data asit is read and verifies at EOF (streaming) or via an explicit
VerifyChecksum()call (file download). The hasher is selected by whichx-amz-checksum-*value the response carries; only full-object checksums(
x-amz-checksum-type: FULL_OBJECT) are verified.api-get-object.gogetObjectresponses are wrapped with the verifying reader, sosequential
GetObjectreads fail with a checksum mismatch error at EOF ifthe payload was corrupted in transit.
api-get-object-file.goFGetObjectverifies the downloaded file before renaming it into place.Resumed downloads reuse the checksum metadata from the initial
StatObjectcall (ranged responses do not repeat the checksum headers) and hash the
existing partial file first, so the whole object is verified. The part file
is opened read-write to allow that; on mismatch the partial file is removed.
.github/workflows/go-rdma.yml2026.06.24so the RDMAbuild no longer tracks vcpkg master.
Tests
checksum-verify_test.go: stub-server tests covering match, mismatch,no-checksum-headers, and composite-mode responses for
GetObject, plusfull-download, resume-with-correct-prefix, and resume-with-corrupted-prefix
for
FGetObject.Summary by CodeRabbit
Bug Fixes