mirror of
https://github.com/rustfs/rustfs.git
synced 2026-09-07 12:35:54 +00:00
fix(s3): report FULL_OBJECT checksum type for multipart objects (#7060)
* fix(ecstore): persist merged checksum type for full-object multipart complete_multipart_upload built the object-level checksum record from a ChecksumType copied before the MULTIPART / INCLUDES_MULTIPART flags were merged in. ChecksumType::merge takes &mut self, so the merge updated the local variable while the copy already inside the Checksum struct stayed behind. The composite branch rebuilt the Checksum from the merged type and was unaffected; the full-object branch never rebuilt it, so those flags never reached disk. rustfs_rio::read_checksums only sets its multipart flag and only emits the "x-amz-checksum-type" = "FULL_OBJECT" entry inside its MULTIPART branch, so a full-object multipart object read back as non-multipart with no type entry, and GetObject and HeadObject answered with no x-amz-checksum-type header at all where AWS returns FULL_OBJECT. Hand the full-object branch the merged type instead of rebuilding the Checksum: the value must stay the running merge produced by add_part, because hashing the concatenated part digests would yield the COMPOSITE value, a different number than the one the client sent. The serialization now lives in multipart_object_checksum_record so both shapes are covered by unit tests. Records written by earlier builds carry the bare algorithm type with no MULTIPART flags and no trailing part block; they keep reading back to the same checksum value, and the FULL_OBJECT reader arm predates this change so older peers parse the new record shape correctly too. Found while root-causing rustfs#6825. * fix(s3): reject contradicting multipart checksum type as client error A CompleteMultipartUpload declaring an x-amz-checksum-type that contradicts the type recorded at CreateMultipartUpload answered 500 InternalError, telling the caller to retry a request that can only ever fail. The storage layer does refuse the combination, but through a generic error that maps to InternalError. Validate the header against the recorded type in the usecase, where the upload metadata returned by get_multipart_info is already in hand, and answer InvalidRequest naming both types, matching AWS. The storage-layer check stays as a backstop for non-HTTP callers. Uploads created without a checksum algorithm record no type, so there is nothing to contradict and the header is left alone rather than newly rejected. Replication is unaffected: replication_put_object_options already excludes x-amz-checksum-type from the metadata it forwards. * test(e2e): cover full-object multipart checksum type round-trip Adds an end-to-end test that a CRC32 FULL_OBJECT multipart upload reports x-amz-checksum-type: FULL_OBJECT and the unsuffixed full-object value on both GetObject and HeadObject, and one that a CompleteMultipartUpload contradicting the recorded type is rejected as InvalidRequest while leaving the upload intact. Extends the existing CRC64NVME multipart test with the same checksum-type assertion. * fix(s3): keep checksum-type validation off the s3s error macro The s3s footprint ratchet (scripts/check_s3s_footprint.sh) counts s3_error! invocation lines and is lower-only: new code must route through the gateway abstractions rather than widen the direct s3s surface the s3gate migration is shrinking. Raise the contradiction through ApiError::invalid_request instead. The response is byte-for-byte identical -- From<ApiError> for S3Error carries the InvalidRequest code and the message through unchanged -- and the usecase already returns ApiError elsewhere, so this is the idiomatic path rather than a way around the counter. The explanatory comment deliberately says "the s3s error macro" instead of naming the macro: the ratchet counts raw matches, so spelling it out in a comment tripped the same check.
This commit is contained in:
@@ -2776,14 +2776,7 @@ impl crate::storage_api_contracts::multipart::MultipartOperations for SetDisks {
|
||||
);
|
||||
|
||||
if checksum_type.is_set() {
|
||||
checksum_type
|
||||
.merge(rustfs_rio::ChecksumType::MULTIPART)
|
||||
.merge(rustfs_rio::ChecksumType::INCLUDES_MULTIPART);
|
||||
if !checksum_type.full_object_requested() {
|
||||
checksum = rustfs_rio::Checksum::new_from_data(checksum_type, &checksum_combined)
|
||||
.ok_or_else(|| Error::other("checksum new_from_data failed"))?;
|
||||
}
|
||||
fi.checksum = Some(checksum.to_bytes(&checksum_combined));
|
||||
fi.checksum = Some(multipart_object_checksum_record(checksum, checksum_type, &checksum_combined)?);
|
||||
}
|
||||
|
||||
fi.metadata.remove(rustfs_rio::RUSTFS_MULTIPART_CHECKSUM);
|
||||
@@ -3432,6 +3425,43 @@ fn resolve_complete_etag(opts: &ObjectOptions, uploaded_parts: &[CompletePart])
|
||||
get_complete_multipart_md5(uploaded_parts)
|
||||
}
|
||||
|
||||
/// Serialize the object-level checksum record that `complete_multipart_upload`
|
||||
/// persists into `FileInfo::checksum`.
|
||||
///
|
||||
/// `full_object` is the running checksum accumulated with
|
||||
/// [`rustfs_rio::Checksum::add_part`] over the parts (only meaningful when
|
||||
/// `checksum_type` asks for a full-object checksum); `combined` is the
|
||||
/// concatenation of the raw per-part digests.
|
||||
///
|
||||
/// Both shapes must be written with MULTIPART | INCLUDES_MULTIPART set. Those
|
||||
/// flags are what make [`rustfs_rio::read_checksums`] treat the object as
|
||||
/// multipart and — for the full-object shape — emit
|
||||
/// `x-amz-checksum-type: FULL_OBJECT`, which GET/HEAD echo back. `merge` takes
|
||||
/// `&mut self`, so the caller's `ChecksumType` and the copy already inside
|
||||
/// `full_object` drift apart at the merge; the full-object branch therefore has
|
||||
/// to be handed the merged type explicitly. It must *not* be rebuilt from
|
||||
/// `combined`: hashing the concatenated part digests yields the COMPOSITE value,
|
||||
/// a different number than the merged full-object one the client sent.
|
||||
fn multipart_object_checksum_record(
|
||||
mut full_object: rustfs_rio::Checksum,
|
||||
mut checksum_type: rustfs_rio::ChecksumType,
|
||||
combined: &[u8],
|
||||
) -> Result<Bytes> {
|
||||
checksum_type
|
||||
.merge(rustfs_rio::ChecksumType::MULTIPART)
|
||||
.merge(rustfs_rio::ChecksumType::INCLUDES_MULTIPART);
|
||||
|
||||
let checksum = if checksum_type.full_object_requested() {
|
||||
full_object.checksum_type = checksum_type;
|
||||
full_object
|
||||
} else {
|
||||
rustfs_rio::Checksum::new_from_data(checksum_type, combined)
|
||||
.ok_or_else(|| Error::other("checksum new_from_data failed"))?
|
||||
};
|
||||
|
||||
Ok(checksum.to_bytes(combined))
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
@@ -8485,4 +8515,113 @@ mod tests {
|
||||
let computed = resolve_complete_etag(&ObjectOptions::default(), &[]);
|
||||
assert_eq!(computed, get_complete_multipart_md5(&[]));
|
||||
}
|
||||
|
||||
/// Accumulate the per-part digests and the running full-object checksum the
|
||||
/// way the `complete_multipart_upload` part loop does, so the record tests
|
||||
/// below exercise the same inputs `multipart_object_checksum_record` gets in
|
||||
/// production.
|
||||
fn accumulate_parts(
|
||||
checksum_type: rustfs_rio::ChecksumType,
|
||||
parts: &[&[u8]],
|
||||
merge_full_object: bool,
|
||||
) -> (rustfs_rio::Checksum, Vec<u8>) {
|
||||
let mut running = rustfs_rio::Checksum {
|
||||
checksum_type,
|
||||
..Default::default()
|
||||
};
|
||||
let mut combined = Vec::new();
|
||||
for part in parts {
|
||||
let part_checksum = rustfs_rio::Checksum::new_from_data(checksum_type, part).expect("part checksum");
|
||||
if merge_full_object {
|
||||
running.add_part(&part_checksum, part.len() as i64).expect("add_part");
|
||||
}
|
||||
combined.extend_from_slice(part_checksum.raw.as_slice());
|
||||
}
|
||||
(running, combined)
|
||||
}
|
||||
|
||||
/// A full-object multipart checksum must be persisted with the MULTIPART /
|
||||
/// INCLUDES_MULTIPART flags, so the reader reports the object as multipart
|
||||
/// and emits `x-amz-checksum-type: FULL_OBJECT`. Before the fix the record
|
||||
/// carried the pre-merge type (the copy `Checksum` took at construction),
|
||||
/// the reader never entered its MULTIPART branch, and GET/HEAD answered with
|
||||
/// no checksum-type header at all.
|
||||
#[test]
|
||||
fn full_object_multipart_record_persists_merged_type_and_full_object_value() {
|
||||
let part1 = b"full-object multipart part one payload".as_slice();
|
||||
let part2 = b"full-object multipart part two payload".as_slice();
|
||||
let checksum_type = rustfs_rio::ChecksumType::from_string_with_obj_type("crc32", "FULL_OBJECT");
|
||||
assert!(checksum_type.full_object_requested());
|
||||
|
||||
let (running, combined) = accumulate_parts(checksum_type, &[part1, part2], true);
|
||||
let record = multipart_object_checksum_record(running, checksum_type, &combined).expect("record");
|
||||
|
||||
let (map, is_multipart) = rustfs_rio::read_checksums(record.as_ref(), 0);
|
||||
assert!(is_multipart, "full-object multipart record must read back as multipart");
|
||||
assert_eq!(
|
||||
map.get("x-amz-checksum-type").map(String::as_str),
|
||||
Some("FULL_OBJECT"),
|
||||
"full-object record must drive the FULL_OBJECT response header, got {map:?}"
|
||||
);
|
||||
|
||||
// The persisted value stays the full-object checksum (CRC32 of the whole
|
||||
// object), never the COMPOSITE hash of the concatenated part digests, and
|
||||
// it carries no `-<parts>` suffix.
|
||||
let whole: Vec<u8> = part1.iter().chain(part2.iter()).copied().collect();
|
||||
let full_object = rustfs_rio::Checksum::new_from_data(rustfs_rio::ChecksumType::CRC32, &whole).expect("full object");
|
||||
let composite = rustfs_rio::Checksum::new_from_data(rustfs_rio::ChecksumType::CRC32, &combined).expect("composite");
|
||||
assert_ne!(full_object.encoded, composite.encoded, "test would not discriminate the two shapes");
|
||||
assert_eq!(map.get("CRC32").map(String::as_str), Some(full_object.encoded.as_str()));
|
||||
|
||||
// Per-part digests are still recoverable from the record (INCLUDES_MULTIPART).
|
||||
let (part_map, _) = rustfs_rio::read_checksums(record.as_ref(), 2);
|
||||
let part2_checksum = rustfs_rio::Checksum::new_from_data(rustfs_rio::ChecksumType::CRC32, part2).expect("part 2");
|
||||
assert_eq!(part_map.get("CRC32").map(String::as_str), Some(part2_checksum.encoded.as_str()));
|
||||
}
|
||||
|
||||
/// The composite shape is unchanged by the fix: value hashed from the
|
||||
/// concatenated part digests, `-<parts>` suffix, no FULL_OBJECT entry.
|
||||
#[test]
|
||||
fn composite_multipart_record_keeps_part_count_suffix() {
|
||||
let part1 = b"composite multipart part one payload".as_slice();
|
||||
let part2 = b"composite multipart part two payload".as_slice();
|
||||
let checksum_type = rustfs_rio::ChecksumType::from_string("sha256");
|
||||
assert!(!checksum_type.full_object_requested());
|
||||
|
||||
let (running, combined) = accumulate_parts(checksum_type, &[part1, part2], false);
|
||||
let record = multipart_object_checksum_record(running, checksum_type, &combined).expect("record");
|
||||
|
||||
let (map, is_multipart) = rustfs_rio::read_checksums(record.as_ref(), 0);
|
||||
assert!(is_multipart);
|
||||
assert_eq!(map.get("x-amz-checksum-type"), None, "composite must not claim FULL_OBJECT");
|
||||
let composite = rustfs_rio::Checksum::new_from_data(rustfs_rio::ChecksumType::SHA256, &combined).expect("composite");
|
||||
assert_eq!(map.get("SHA256").map(String::as_str), Some(format!("{}-2", composite.encoded).as_str()));
|
||||
}
|
||||
|
||||
/// Records written by builds from before the fix carry the bare algorithm
|
||||
/// type with no MULTIPART flags and no trailing part block. Those bytes must
|
||||
/// keep reading back to the same checksum value they always did — the fix
|
||||
/// only changes what newly completed uploads write.
|
||||
#[test]
|
||||
fn legacy_full_object_record_without_flags_still_reads_back() {
|
||||
let part1 = b"full-object multipart part one payload".as_slice();
|
||||
let part2 = b"full-object multipart part two payload".as_slice();
|
||||
let checksum_type = rustfs_rio::ChecksumType::from_string_with_obj_type("crc32", "FULL_OBJECT");
|
||||
let (running, combined) = accumulate_parts(checksum_type, &[part1, part2], true);
|
||||
|
||||
// Exactly what the pre-fix code emitted: the unmerged type, serialized
|
||||
// with the part digests offered but never appended.
|
||||
let legacy = running.to_bytes(&combined);
|
||||
|
||||
let (map, is_multipart) = rustfs_rio::read_checksums(legacy.as_ref(), 0);
|
||||
assert!(!is_multipart, "legacy record has no MULTIPART flag");
|
||||
assert_eq!(map.get("x-amz-checksum-type"), None, "legacy record carries no type entry");
|
||||
let whole: Vec<u8> = part1.iter().chain(part2.iter()).copied().collect();
|
||||
let full_object = rustfs_rio::Checksum::new_from_data(rustfs_rio::ChecksumType::CRC32, &whole).expect("full object");
|
||||
assert_eq!(
|
||||
map.get("CRC32").map(String::as_str),
|
||||
Some(full_object.encoded.as_str()),
|
||||
"legacy records must keep returning their stored checksum value"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user