From 1d46575f61060de2b2a2b3a7ba2e4981f4dd7735 Mon Sep 17 00:00:00 2001 From: Kevin Wang Date: Mon, 3 Aug 2026 21:24:48 -0700 Subject: [PATCH] fix(util): honor the requested random output path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `dstack-util rand` parsed `-o/--output` and then ignored it, always writing to stdout. The command reported success while the requested file was never created. Write the file through `safe_write_with_mode`, which the workspace already depends on, rather than hand-rolling the write: - The output is key material, so it is created 0600 and never exists under wider permissions. - The replacement is a single rename, and both the file and its directory are fsynced. A truncated random file is a silently weak secret — it looks exactly like a successful result, and nothing downstream can tell the difference. - If any step fails, nothing is left behind. A hand-rolled `create_new` + write leaves a short file that the next run then refuses to replace, so a transient ENOSPC would wedge the command with a partial secret in place. Re-running replaces the file rather than failing on an existing one, matching `openssl rand -out`. If a no-clobber mode is wanted later it should be an explicit flag rather than the implicit default. Four tests, all confirmed to fail against the previous behaviour: the file is created at the requested path with no temporary left behind, it is 0600, `-x` doubles the length and emits hex, and a re-run replaces it. --- dstack/dstack-util/src/main.rs | 82 ++++++++++++++++++++++++++++++++-- 1 file changed, 79 insertions(+), 3 deletions(-) diff --git a/dstack/dstack-util/src/main.rs b/dstack/dstack-util/src/main.rs index 9bb2e5e3b..a9947e73b 100644 --- a/dstack/dstack-util/src/main.rs +++ b/dstack/dstack-util/src/main.rs @@ -698,9 +698,16 @@ fn cmd_rand(rand_args: RandArgs) -> Result<()> { if rand_args.hex { data = hex::encode(data).into_bytes(); } - io::stdout() - .write_all(&data) - .context("Failed to write random data")?; + if let Some(output) = rand_args.output { + // key material: owner-only, and never half-written — a truncated + // random file would pass for a valid secret. + safe_write::safe_write_with_mode(&output, &data, 0o600) + .with_context(|| format!("Failed to write random output {output}"))?; + } else { + io::stdout() + .write_all(&data) + .context("Failed to write random data")?; + } Ok(()) } @@ -1344,3 +1351,72 @@ async fn main() -> Result<()> { Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + use std::os::unix::fs::PermissionsExt; + + fn rand_args(output: Option, bytes: usize, hex: bool) -> RandArgs { + RandArgs { bytes, output, hex } + } + + /// `-o` used to be parsed and then ignored, so the file was never created + /// and the bytes went to stdout instead. + #[test] + fn rand_writes_to_the_requested_output_path() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("secret.bin"); + + cmd_rand(rand_args(Some(path.display().to_string()), 32, false)).unwrap(); + + assert_eq!(fs::metadata(&path).unwrap().len(), 32); + // nothing but the target: no temporary file left behind. + assert_eq!(fs::read_dir(dir.path()).unwrap().count(), 1); + } + + /// The output is key material, so it must never be readable by anyone else. + #[test] + fn rand_output_is_owner_only() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("secret.bin"); + + cmd_rand(rand_args(Some(path.display().to_string()), 32, false)).unwrap(); + + let mode = fs::metadata(&path).unwrap().permissions().mode() & 0o777; + assert_eq!(mode, 0o600, "random output must be 0600, got {mode:o}"); + } + + #[test] + fn rand_hex_output_is_twice_as_long() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("secret.hex"); + + cmd_rand(rand_args(Some(path.display().to_string()), 16, true)).unwrap(); + + let body = fs::read(&path).unwrap(); + assert_eq!(body.len(), 32); + assert!(body.iter().all(|b| b.is_ascii_hexdigit())); + } + + /// Re-running must replace the file rather than failing, so a retry after a + /// partial or interrupted run cannot wedge the caller. + #[test] + fn rand_replaces_an_existing_output() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("secret.bin"); + + cmd_rand(rand_args(Some(path.display().to_string()), 8, false)).unwrap(); + let first = fs::read(&path).unwrap(); + + cmd_rand(rand_args(Some(path.display().to_string()), 32, false)).unwrap(); + let second = fs::read(&path).unwrap(); + + assert_eq!(first.len(), 8); + assert_eq!(second.len(), 32); + assert_eq!( + fs::metadata(&path).unwrap().permissions().mode() & 0o777, + 0o600 + ); + } +}