mirror of
https://github.com/rustfs/rustfs.git
synced 2026-07-26 08:18:18 +00:00
Address P2 follow-ups from the 2026-07-10..12 merged-PR review (backlog#1210-1220) (#4783)
* fix(obs): open cleaner compression source with O_NOFOLLOW The compressor opened the source log via File::open, which follows a symlink at the final path component. Between the scanner selecting a regular file and this open, an attacker with write access to the log directory could swap the entry for a symlink (TOCTOU) pointing at, say, /etc/shadow, whose contents would then be copied into an archive. Open the source with O_NOFOLLOW on Unix so such a swap fails with ELOOP; the temp/archive path already refused symlinks, this closes the source side. Refs rustfs/backlog#1210 Co-Authored-By: heihutu <heihutu@gmail.com> * fix(obs): recompress instead of trusting leftover cleaner archives archive_header_ok only checked the first 2-4 magic bytes before treating an existing .gz/.zst as a completed prior result and letting the caller delete the source log. A file with valid magic but a truncated or forged body passes that check, so an attacker with write access to the log directory (or a crashed prior run) could plant such a stub and make the cleaner delete the real log without ever producing a usable archive — silent audit-data loss. Chosen fix: stop trusting cross-process leftovers entirely and always recompress the source in this pass, rather than fully decoding every leftover to validate it. Full-decode validation would add real CPU cost and decode-bug surface for a rare crash-recovery case; the existing atomic create_new+rename already overwrites whatever sits at the archive path (a planted symlink is replaced, never followed) with a freshly written, fsync'd archive, so a partial/forged leftover can never gate source deletion. This is the lowest-regression option. Refs rustfs/backlog#1211 Co-Authored-By: heihutu <heihutu@gmail.com> * fix(object-data-cache): cap memory-gate reservation at cache growth headroom The memory gate subtracts `admitted_since_refresh` from the snapshot's available bytes so a burst arriving faster than the 5 s refresh cannot over-allocate. That counter is GROSS: it only rolls over on the refresh and never rolls back when a fill is later evicted, cancelled, or loses the invalidation race. Under sustained high-throughput churn (net footprint flat and far below `max_capacity`) the raw counter balloons past the memory the cache actually holds, so `effective_available` collapses and the gate reports false memory pressure — skipping the hottest fills with SkippedMemoryPressure until the next 5 s refresh. This only lowers hit rate; it never returns wrong data and self-heals each refresh. Fix direction 1 (minimal regression): cap the reservation deduction at the cache's own growth headroom (`max_capacity - weighted_size()`) instead of letting the unbounded gross counter shrink the system-available budget. The cache can never hold more than `max_capacity`, so a burst adds at most that headroom of real memory before moka evicts to stay bounded (net-zero churn beyond that point) — capping the deduction there keeps the reservation honest without treating gross churn as growth. Chosen over net-accounting (direction 2, releasing bytes on every failure/cancel/eviction path) because that only plugs the leak on failed fills and would not address the core defect: churn of *successful* insert/evict fills over the 5 s window. It also touches only the gate plus one call site rather than every failure path in moka_backend. The cap only ever raises `effective_available`, so real memory pressure (a low snapshot at refresh) still suppresses fills; when the cache is at capacity the headroom is 0 and the deduction vanishes, correctly reflecting net-zero churn. `MokaBackend` now stores `max_capacity` and passes the live headroom into `allows_fill`. Adds targeted gate tests: gross churn far above headroom no longer falsely suppresses, yet the reservation still bounds a burst while the cache can genuinely grow. Refs rustfs/backlog#1212 Co-Authored-By: heihutu <heihutu@gmail.com> * test(ecstore): assert native O_DIRECT path runs in uring read test uring_preserves_o_direct_for_eligible_reads only compared bytes through LocalDisk::read_file_mmap_copy. On a filesystem that rejects O_DIRECT the read silently degrades to the buffered StdBackend fallback and the byte check still passes, so the test could go green without the native read_at_direct path ever executing -- a vacuous pass. Add a per-disk native_direct_reads counter on UringBackend, incremented only when pread_uring_direct completes, and rebuild the test to drive a real UringBackend's pread_bytes and assert the counter is non-zero (every eligible read went through the native tier). When io_uring or O_DIRECT is unavailable on the host filesystem (restricted CI runners, tmpfs), the test skips loudly via eprintln instead of asserting a tautology, while still checking byte-correctness on whatever tier served the read. The counter also gives a gray release a positive signal that the O_DIRECT tier is serving reads, not just a fallback count. Refs rustfs/backlog#1213 Co-Authored-By: heihutu <heihutu@gmail.com> * fix(ecstore): warn + count read-time EINVAL on native O_DIRECT reads classify_direct_read_error is only reached from the read side: the O_DIRECT open in pread_uring_direct already succeeded (an open-time refusal is handled earlier as DirectOpenError::ODirectRefused). So an EINVAL/EOPNOTSUPP arriving here is a read-time error on an fd the kernel accepted for O_DIRECT -- far more likely an alignment bug in the aligned read path than an unsupported filesystem. The old code latched the disk's native path off with only a once-per-disk debug trace, hiding a potential correctness regression behind a silent buffered-read downgrade. Diagnostics only: the fallback behaviour is unchanged (the native path is still latched off and the caller still reads via StdBackend). This adds a rustfs_io_uring_direct_read_einval_total counter and promotes the once-per-disk trace from debug to warn so an operator can see an alignment regression instead of an unexplained latency/CPU shift. Refs rustfs/backlog#1214 Co-Authored-By: heihutu <heihutu@gmail.com> * docs(ecstore): document data-blocks-first default and its tail-latency cost DEFAULT_RUSTFS_GET_DATA_BLOCKS_FIRST_READER_SETUP is true and must stay true: deferred-parity is the deliberate, already-rolled-out full-object GET default from backlog#1159/#923. Flipping it back to false in code would silently revert that rollout for every deployment that has not set the env var, so this commit only documents -- no behaviour change. The added notes explain what data-blocks-first does (schedule data shards up front, engage parity lazily on a missing/corrupt data shard), the known trade-off (parity is engaged late, so a slow-but-not-dead data drive raises GET p99 because the faster parity shards are not raced against it until a data shard is declared missing), and the operational rollback switch (RUSTFS_GET_DATA_BLOCKS_FIRST_READER_SETUP=false), which is intentionally an env override rather than a code default change. No metric was added: the low-risk observability hook for "slow data drive engaged deferred parity" would live at the deferred-stripe engage point, which is out of this file's scope; this change stays documentation-only to avoid touching the hot GET path. Refs rustfs/backlog#1215 Co-Authored-By: heihutu <heihutu@gmail.com> * docs(ecstore): document wide-directory walk stall hazard and tuning list_dir enumerates a whole directory in one os::read_dir call (count = -1), and the walk caller bounds that entire enumeration with the per-read stall budget (default 5s) as if it were a single read. For a wide, flat prefix -- one directory holding millions of immediate children -- a single readdir can exceed the budget on a healthy disk, trip DiskError::Timeout, and surface as a ListObjects 500 quorum failure though the drive is fine (a #2999 sub-class). This is documented, not rewritten: turning the one-shot readdir into a streaming/batched enumeration that refreshes the stall deadline between chunks is an architecture-level change with high regression surface (ordering, the count contract, quorum merge) and belongs in a separate follow-up. The supported mitigation today is operational, so the comments point wide-directory deployments at RUSTFS_DRIVE_WALKDIR_STALL_TIMEOUT_SECS and the high-latency drive-timeout profile, which widen the budget with no code change. Notes were added at list_dir, the scan_dir call site, and get_drive_walkdir_stall_timeout. No behaviour change. Refs rustfs/backlog#1216 Co-Authored-By: heihutu <heihutu@gmail.com> * docs(ecstore): document consumer-peek vs producer-stall coupling In list_path_raw the consumer's peek_timeout is drawn from the same source and same value (walkdir_stall_timeout, default 5s) as the producer-side walk stall budget, but the two measure different things: the producer stall bounds a single drive read, while the consumer peek bounds the gap between two ADJACENT entries arriving from a reader. Because they share a value, the consumer cannot wait meaningfully longer for the next entry than the producer is allowed to spend producing one. Walking a region dense with non-listable internal items can make a HEALTHY drive miss the budget between visible entries; the consumer then declares it stalled and detaches it, dropping a good drive from the merge and capping the "large prefix succeeds" guarantee. Documented, not decoupled: giving the consumer peek an independent, strictly-larger budget would cut these false detaches but equally delays detaching a genuinely dead drive and shifts listing tail-latency semantics, so it wants soak data before changing the default. The comment records the invariant any such follow-up must keep -- consumer peek >= producer stall, never stricter -- so it can never fail a drive before the producer would. No behaviour change. Refs rustfs/backlog#1217 Co-Authored-By: heihutu <heihutu@gmail.com> * fix(io-metrics): add time-based trigger for low-IOPS latency percentiles Percentiles were recomputed only every 128 IOs and seeded to 0, so a low-traffic deployment exported p95/p99 = 0/stale for a long time after startup. Add a 10s wall-clock trigger alongside the count throttle so the first recompute can fire before 128 samples accrue. Hot-path per-op mean update is unchanged. Refs rustfs/backlog#1218 Co-Authored-By: heihutu <heihutu@gmail.com> * test(e2e): cover codec-streaming parity under fault injection and NoSuchKey The codec-streaming compat A/B previously ran only against a healthy 4-disk EC set with successful full GETs: the DiskFaultHarness was constructed but never faulted, the error path was untested, and the range assertion silently compared legacy-vs-legacy (ranges always fall back to the duplex path), overstating what it proved. Add two genuinely-failable scenarios reusing the existing harness and fixtures: - Parity reconstruction A/B: take one data disk offline and re-run the full object matrix on both phases while the EC 2+2 set rebuilds each large object from the surviving shards. Assert codec == legacy byte-for-byte (sha256) and header-for-header, and assert the codec phase served the reconstructed objects with zero duplex-pipe fallback (the reader gate is drive-health-independent, so the codec fast path is really exercised through reconstruction). - NoSuchKey negative path: compare the HTTP status + S3 error code of a missing-key GET across the legacy and codec phases and require them to be identical (404/NoSuchKey), guarding against the codec env perturbing the error path. Also clarify the range-phase comment so it is not misread as codec-range correctness coverage: both sides are served by the same legacy range path, so the assertion only proves ranges keep working and keep falling back to legacy with the gates open. Verified: cargo check/--no-run pass and the test passes locally (1 passed; dup_codec=0 confirms the codec path ran). Refs rustfs/backlog#1219 Co-Authored-By: heihutu <heihutu@gmail.com> * ci(ecstore): exercise native O_DIRECT read path on an ext4 loopback The uring-integration leg ran on the runner's default TMPDIR, which may sit on tmpfs/overlayfs where open(O_DIRECT) fails and the native read_at_direct path silently latches off to the aligned StdBackend fallback. Mount a dedicated ext4 loopback and point TMPDIR at it so the real io_uring dep (bumped git->0.1.0->0.2.0->0.2.1) and the native O_DIRECT read path are actually covered rather than validated only by signature diffing. Refs rustfs/backlog#1220 Co-Authored-By: heihutu <heihutu@gmail.com> --------- Co-authored-by: heihutu <heihutu@gmail.com>
This commit is contained in:
houseme
@@ -184,25 +184,28 @@ fn create_tmp_archive(path: &Path, source_mode: Option<u32>) -> std::io::Result<
|
||||
opts.open(path)
|
||||
}
|
||||
|
||||
/// Cheap sanity check that an existing archive starts with the expected codec
|
||||
/// magic bytes, so a zero/truncated/foreign file is not trusted as a valid
|
||||
/// prior result. Only called on a confirmed regular, non-empty file.
|
||||
fn archive_header_ok(path: &Path, algorithm: CompressionAlgorithm) -> bool {
|
||||
use std::io::Read;
|
||||
let mut file = match File::open(path) {
|
||||
Ok(file) => file,
|
||||
Err(_) => return false,
|
||||
};
|
||||
let mut buf = [0u8; 4];
|
||||
let read = match file.read(&mut buf) {
|
||||
Ok(read) => read,
|
||||
Err(_) => return false,
|
||||
};
|
||||
match algorithm {
|
||||
// gzip: 0x1f 0x8b
|
||||
CompressionAlgorithm::Gzip => read >= 2 && buf[0] == 0x1f && buf[1] == 0x8b,
|
||||
// zstd frame magic: 0x28 0xb5 0x2f 0xfd (little-endian 0xFD2FB528)
|
||||
CompressionAlgorithm::Zstd => read >= 4 && buf == [0x28, 0xb5, 0x2f, 0xfd],
|
||||
/// Open the compression source file, refusing to follow a symlink at the final
|
||||
/// path component on Unix.
|
||||
///
|
||||
/// The scanner selects a regular log file, but between that scan and this open
|
||||
/// an attacker with write access to the log directory can swap the entry for a
|
||||
/// symlink (a classic TOCTOU race). `O_NOFOLLOW` makes the open fail with
|
||||
/// `ELOOP` in that case instead of silently reading whatever the link targets
|
||||
/// (for example `/etc/shadow`), which would otherwise be copied verbatim into a
|
||||
/// world-inspectable archive. The temp/archive path already refuses symlinks via
|
||||
/// `create_tmp_archive`; this closes the same gap on the source side.
|
||||
fn open_source_no_follow(path: &Path) -> std::io::Result<File> {
|
||||
#[cfg(unix)]
|
||||
{
|
||||
use std::os::unix::fs::OpenOptionsExt;
|
||||
std::fs::OpenOptions::new()
|
||||
.read(true)
|
||||
.custom_flags(libc::O_NOFOLLOW)
|
||||
.open(path)
|
||||
}
|
||||
#[cfg(not(unix))]
|
||||
{
|
||||
File::open(path)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -234,32 +237,21 @@ fn compress_with_writer<F>(
|
||||
where
|
||||
F: FnMut(&mut BufReader<File>, BufWriter<File>) -> Result<(u64, File), std::io::Error>,
|
||||
{
|
||||
// Idempotency: an existing *complete* archive means a previous pass already
|
||||
// handled this file, so we can skip recompression and let the caller delete
|
||||
// the source. But we must not trust just any entry at `archive_path`:
|
||||
// * `Path::exists()` follows symlinks, so a planted `<archive>.gz` symlink
|
||||
// would green-light deleting the source with no real archive;
|
||||
// * a zero-length/truncated archive left by a crashed run (see OLC-01)
|
||||
// would be trusted, then the source deleted — unrecoverable loss.
|
||||
// Use symlink_metadata (no follow) and require a regular, non-empty file
|
||||
// with a valid codec header before trusting it. Anything else falls through
|
||||
// to recompression, whose atomic create_new+rename replaces the bad entry
|
||||
// (a symlink is replaced, never followed or deleted through).
|
||||
match std::fs::symlink_metadata(archive_path) {
|
||||
Ok(meta) if meta.file_type().is_file() && meta.len() > 0 && archive_header_ok(archive_path, algorithm_used) => {
|
||||
debug!(event = EVENT_LOG_CLEANER_COMPRESSION_STATE, component = LOG_COMPONENT_OBS, subsystem = LOG_SUBSYSTEM_LOG_CLEANER, file = ?archive_path, state = "archive_exists", "log cleaner compression state changed");
|
||||
let input_bytes = std::fs::metadata(path).map(|m| m.len()).unwrap_or(0);
|
||||
return Ok(CompressionOutput {
|
||||
archive_path: archive_path.to_path_buf(),
|
||||
algorithm_used,
|
||||
input_bytes,
|
||||
output_bytes: meta.len(),
|
||||
});
|
||||
}
|
||||
Ok(_) => {
|
||||
warn!(event = EVENT_LOG_CLEANER_COMPRESSION_STATE, component = LOG_COMPONENT_OBS, subsystem = LOG_SUBSYSTEM_LOG_CLEANER, file = ?archive_path, result = "archive_untrusted_recompressing", "log cleaner compression state changed");
|
||||
}
|
||||
Err(_) => {}
|
||||
// Integrity over idempotency: never trust a pre-existing entry at
|
||||
// `archive_path` as a valid prior result. The former fast path accepted any
|
||||
// regular, non-empty file whose first 2-4 bytes matched the gzip/zstd magic.
|
||||
// That header check does not prove the body is complete: an attacker with
|
||||
// write access to the log directory (or a crashed prior run) can leave a file
|
||||
// that starts with valid magic but is truncated/forged, and trusting it would
|
||||
// let the caller delete the real source log — silent audit-data loss. Fully
|
||||
// decoding every leftover to validate it would add real CPU cost and
|
||||
// decode-bug surface, so instead we always recompress the source in this
|
||||
// pass. The atomic create_new+rename below overwrites whatever sits at
|
||||
// `archive_path` (a regular file or a planted symlink is replaced, never
|
||||
// followed or deleted through) with a freshly written, fully-synced archive,
|
||||
// so a partial or forged leftover can never be the basis for source deletion.
|
||||
if std::fs::symlink_metadata(archive_path).is_ok() {
|
||||
debug!(event = EVENT_LOG_CLEANER_COMPRESSION_STATE, component = LOG_COMPONENT_OBS, subsystem = LOG_SUBSYSTEM_LOG_CLEANER, file = ?archive_path, result = "leftover_archive_recompressing", "log cleaner compression state changed");
|
||||
}
|
||||
|
||||
if dry_run {
|
||||
@@ -281,7 +273,10 @@ where
|
||||
|
||||
// Create the output archive only after the dry-run short-circuit so this
|
||||
// helper remains side-effect free when the caller is evaluating policy.
|
||||
let input = File::open(path)?;
|
||||
// O_NOFOLLOW: refuse to open a symlink swapped in at the source path between
|
||||
// the scan and this open (TOCTOU), which could redirect the read to an
|
||||
// arbitrary privileged file.
|
||||
let input = open_source_no_follow(path)?;
|
||||
|
||||
// Read the source mode from the already-open fd (no extra path stat, no
|
||||
// TOCTOU) so the temp file is created restrictive — never wider than the
|
||||
@@ -385,3 +380,79 @@ fn archive_path(path: &Path, algorithm: CompressionAlgorithm) -> PathBuf {
|
||||
file_name.push(format!(".{ext}"));
|
||||
path.with_file_name(file_name)
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
fn default_options(algorithm: CompressionAlgorithm) -> CompressionOptions {
|
||||
CompressionOptions {
|
||||
algorithm,
|
||||
gzip_level: 6,
|
||||
zstd_level: 3,
|
||||
zstd_workers: 1,
|
||||
zstd_fallback_to_gzip: false,
|
||||
dry_run: false,
|
||||
}
|
||||
}
|
||||
|
||||
/// A symlink planted at the source path must be refused (not followed) so a
|
||||
/// TOCTOU swap cannot redirect compression to an arbitrary privileged file.
|
||||
#[cfg(unix)]
|
||||
#[test]
|
||||
fn source_symlink_is_rejected() {
|
||||
let dir = tempfile::tempdir().expect("tempdir");
|
||||
// Secret the attacker wants exfiltrated into the (broader-readable) archive.
|
||||
let secret = dir.path().join("secret.txt");
|
||||
std::fs::write(&secret, b"top secret contents").expect("write secret");
|
||||
|
||||
// The scanner-visible log path is actually a symlink to the secret.
|
||||
let source = dir.path().join("app.log");
|
||||
std::os::unix::fs::symlink(&secret, &source).expect("symlink");
|
||||
|
||||
let err = compress_file(&source, &default_options(CompressionAlgorithm::Gzip))
|
||||
.expect_err("compressing a symlinked source must fail");
|
||||
// O_NOFOLLOW surfaces as ELOOP; accept any error but ensure no archive
|
||||
// was produced from the symlink target.
|
||||
assert!(
|
||||
!dir.path().join("app.log.gz").exists(),
|
||||
"no archive should be created from a symlinked source, err={err}"
|
||||
);
|
||||
// The secret must not have been copied anywhere in the directory.
|
||||
assert!(secret.exists(), "secret should be untouched");
|
||||
}
|
||||
|
||||
/// A pre-existing archive whose header magic is valid but whose body is
|
||||
/// truncated/forged must not be trusted as a completed prior result: it must
|
||||
/// be recompressed into a complete archive, and the source must survive.
|
||||
#[test]
|
||||
fn truncated_magic_archive_is_not_trusted() {
|
||||
use std::io::Read;
|
||||
let dir = tempfile::tempdir().expect("tempdir");
|
||||
let source = dir.path().join("app.log");
|
||||
let payload = b"line1\nline2\nline3\n";
|
||||
std::fs::write(&source, payload).expect("write source");
|
||||
|
||||
// Forged archive: valid gzip magic (0x1f 0x8b) but a truncated body.
|
||||
let archive = dir.path().join("app.log.gz");
|
||||
std::fs::write(&archive, [0x1f_u8, 0x8b, 0x00]).expect("write forged archive");
|
||||
|
||||
let out = compress_file(&source, &default_options(CompressionAlgorithm::Gzip)).expect("compression should succeed");
|
||||
assert_eq!(out.archive_path, archive);
|
||||
|
||||
// The forged stub must have been replaced by a real archive that decodes
|
||||
// back to the full source — proving it was not trusted (trusting it would
|
||||
// let the caller delete the source and lose the log).
|
||||
let file = File::open(&archive).expect("open archive");
|
||||
let mut decoder = flate2::read::GzDecoder::new(file);
|
||||
let mut decoded = Vec::new();
|
||||
decoder
|
||||
.read_to_end(&mut decoded)
|
||||
.expect("archive must be a complete gzip stream");
|
||||
assert_eq!(decoded, payload, "recompressed archive must contain the full source");
|
||||
|
||||
// compress_file never deletes the source itself (the caller does, only
|
||||
// after a valid archive), so the source log is still present here.
|
||||
assert!(source.exists(), "source log must survive compression");
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user