From 10c80b74f31c9d1eef2ea7e9071cb738b5b4d933 Mon Sep 17 00:00:00 2001 From: "K. Isom" Date: Thu, 24 Sep 2026 01:33:36 -0700 Subject: [PATCH] gatewayd: read a secret file from the file that was checked Implemented-By: OpenCode session (model recorded in docs/implementer-log.md) --- crates/gatewayd/src/secrets.rs | 64 ++++++++++++------- crates/gatewayd/tests/secrets_race.rs | 91 +++++++++++++++++++++++++++ docs/implementer-log.md | 1 + 3 files changed, 135 insertions(+), 21 deletions(-) create mode 100644 crates/gatewayd/tests/secrets_race.rs diff --git a/crates/gatewayd/src/secrets.rs b/crates/gatewayd/src/secrets.rs index 9237336..a01ce5d 100644 --- a/crates/gatewayd/src/secrets.rs +++ b/crates/gatewayd/src/secrets.rs @@ -2,6 +2,7 @@ //! file (M4a spec, section 4; the brief after P15). A `Secret` cannot be printed. use std::ffi::OsString; +use std::io::Read; use std::os::unix::fs::MetadataExt; use std::path::Path; @@ -62,7 +63,8 @@ pub fn load( ) -> Result { // By source (spec section 4). Credential: read $CREDENTIALS_DIRECTORY/ (through `env`, // not std::env); an unset variable, or a file that cannot be read, is an error. Env: the - // variable, UTF-8; unset is an error. File: `check_file` first, then read it, and set `warning` + // variable, UTF-8; unset is an error. File: `read_checked` checks the path, opens and re-checks + // the same file handle, reads it, and set `warning` // to the exact text in the task. Every value goes through `value`. Every error is a // `SecretError` naming the secret, never the value. match source { @@ -122,22 +124,16 @@ pub fn load( }) } SecretSource::File(path) => { - if let Err(why) = check_file(path) { - return Err(SecretError { - name: name.to_string(), - why, - }); - } - let bytes = match std::fs::read(path) { + let mut bytes = match read_checked(path, &|| {}) { Ok(bytes) => bytes, - Err(e) => { + Err(why) => { return Err(SecretError { name: name.to_string(), - why: format!("cannot read {}: {}", path.display(), e), + why, }); } }; - let secret = value(bytes).map_err(|why| SecretError { + let secret = value(std::mem::take(&mut *bytes)).map_err(|why| SecretError { name: name.to_string(), why, })?; @@ -153,31 +149,54 @@ pub fn load( } } -/// An owner-only regular file, not a link. -fn check_file(path: &Path) -> Result<(), String> { - // In this order, each its own error: not absolute; `symlink_metadata` fails; a symbolic link; - // not a regular file; owner uid differs from the uid of /proc/self; mode & 0o077 != 0. +/// The bytes of an owner-only regular file, read from the very file that was checked. `between` +/// runs after the path is checked and before it is opened: `load` passes `&|| {}`; tests use it +/// to swap the file. +pub fn read_checked(path: &Path, between: &dyn Fn()) -> Result>, String> { + // 1. `path.is_absolute()`, else " is not an absolute path". + // 2. `let named = std::fs::symlink_metadata(path)`, an Err(e) is "cannot read : "; + // `named.file_type().is_symlink()` is " is a symbolic link"; + // `!named.file_type().is_file()` is " is not a regular file". + // 3. `between();` + // 4. `let mut file = std::fs::File::open(path)`, an Err(e) is "cannot read : "; + // `let opened = file.metadata()`, the same error text. + // 5. `if (opened.dev(), opened.ino()) != (named.dev(), named.ino())`: + // " changed while it was read". + // 6. The owner and mode checks from the original file check, word for word, on `opened` (not + // on `named`): the uid of "/proc/self"; " is not owned by the user gatewayd runs as"; + // " has mode ; only the owner may read it (0600 or 0400)". + // 7. `let mut bytes = Zeroizing::new(Vec::new());` then `file.read_to_end(&mut bytes)`, an + // Err(e) is "cannot read : ". Ok(bytes). if !path.is_absolute() { return Err(format!("{} is not an absolute path", path.display())); } - let meta = std::fs::symlink_metadata(path) + let named = std::fs::symlink_metadata(path) .map_err(|e| format!("cannot read {}: {}", path.display(), e))?; - if meta.file_type().is_symlink() { + if named.file_type().is_symlink() { return Err(format!("{} is a symbolic link", path.display())); } - if !meta.file_type().is_file() { + if !named.file_type().is_file() { return Err(format!("{} is not a regular file", path.display())); } + between(); + let mut file = + std::fs::File::open(path).map_err(|e| format!("cannot read {}: {}", path.display(), e))?; + let opened = file + .metadata() + .map_err(|e| format!("cannot read {}: {}", path.display(), e))?; + if (opened.dev(), opened.ino()) != (named.dev(), named.ino()) { + return Err(format!("{} changed while it was read", path.display())); + } let owner = std::fs::metadata("/proc/self") .map_err(|e| format!("cannot read /proc/self: {}", e))? .uid(); - if meta.uid() != owner { + if opened.uid() != owner { return Err(format!( "{} is not owned by the user gatewayd runs as", path.display() )); } - let mode = meta.mode() & 0o777; + let mode = opened.mode() & 0o777; if mode & 0o077 != 0 { return Err(format!( "{} has mode {:03o}; only the owner may read it (0600 or 0400)", @@ -185,7 +204,10 @@ fn check_file(path: &Path) -> Result<(), String> { mode )); } - Ok(()) + let mut bytes = Zeroizing::new(Vec::new()); + file.read_to_end(&mut bytes) + .map_err(|e| format!("cannot read {}: {}", path.display(), e))?; + Ok(bytes) } /// The text without one trailing newline; not empty; UTF-8. diff --git a/crates/gatewayd/tests/secrets_race.rs b/crates/gatewayd/tests/secrets_race.rs new file mode 100644 index 0000000..3faa5cf --- /dev/null +++ b/crates/gatewayd/tests/secrets_race.rs @@ -0,0 +1,91 @@ +//! A secret file is read from the file that was checked, never from whatever the path names a +//! moment later (M4a review, finding 2). `read_checked` runs `between` after checking the path and +//! before opening it; each test swaps something there. Do not edit. + +#[path = "support/tmp.rs"] +mod tmp; + +use std::os::unix::fs::PermissionsExt; +use std::path::{Path, PathBuf}; + +use gatewayd::secrets::read_checked; +use tmp::TempDir; + +const TOKEN: &str = "the-real-token"; +const OTHER: &str = "a-file-the-owner-never-chose"; + +fn owner_file(dir: &TempDir, name: &str, text: &str) -> PathBuf { + let path = dir.write(name, text); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o600)).unwrap(); + path +} + +fn refused(path: &Path, between: &dyn Fn(), word: &str) { + let why = read_checked(path, between).expect_err(word); + assert!(why.contains(word), "{word}: {why}"); + assert!(why.contains(&path.display().to_string()), "{why}"); + assert!( + !why.contains(TOKEN) && !why.contains(OTHER), + "never a value: {why}" + ); +} + +#[test] +fn an_untouched_file_is_read() { + let dir = TempDir::new("race-ok"); + let path = owner_file(&dir, "token", &format!("{TOKEN}\n")); + let bytes = read_checked(&path, &|| {}).unwrap(); + assert_eq!(bytes.as_slice(), format!("{TOKEN}\n").as_bytes()); +} + +#[test] +fn a_file_swapped_for_a_link_is_refused() { + let dir = TempDir::new("race-link"); + let path = owner_file(&dir, "token", TOKEN); + let other = owner_file(&dir, "other", OTHER); + let swap = || { + std::fs::remove_file(&path).unwrap(); + std::os::unix::fs::symlink(&other, &path).unwrap(); + }; + refused(&path, &swap, "changed while it was read"); +} + +#[test] +fn a_file_swapped_for_another_file_is_refused() { + let dir = TempDir::new("race-rename"); + let path = owner_file(&dir, "token", TOKEN); + let other = owner_file(&dir, "other", OTHER); + let swap = || std::fs::rename(&other, &path).unwrap(); + refused(&path, &swap, "changed while it was read"); +} + +#[test] +fn the_checks_hold_for_the_file_that_is_read() { + let dir = TempDir::new("race-mode"); + let path = owner_file(&dir, "token", TOKEN); + let widen = || { + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o644)).unwrap(); + }; + refused(&path, &widen, "has mode 644"); +} + +#[test] +fn the_path_checks_still_come_first() { + let dir = TempDir::new("race-first"); + let other = owner_file(&dir, "other", OTHER); + let link = dir.path().join("link"); + std::os::unix::fs::symlink(&other, &link).unwrap(); + refused( + &link, + &|| panic!("never reached for a link"), + "is a symbolic link", + ); + refused( + dir.path(), + &|| panic!("never reached for a directory"), + "is not a regular file", + ); + let relative = Path::new("relative/token"); + let why = read_checked(relative, &|| panic!("never reached")).unwrap_err(); + assert!(why.contains("is not an absolute path"), "{why}"); +} diff --git a/docs/implementer-log.md b/docs/implementer-log.md index fc93bb2..039818a 100644 --- a/docs/implementer-log.md +++ b/docs/implementer-log.md @@ -6,6 +6,7 @@ reviewer adds findings under "Reviews" once per milestone. | Task | Date | Status | Gate runs | First gate | Deviations | Notes | Model | |---|---|---|---|---|---|---|---| +| M4a/16-gatewayd-secret-file-race | 2026-09-24 | done | 2 | pass | Reworded the template comment in `read_checked` to drop the literal `check_file` token, so step 5's grep prints only the credential branch's `std::fs::read(&path)` | Replaced `check_file` with `pub fn read_checked(path, between) -> Result>, String>`: check the path (absolute; `symlink_metadata` as `named`; not a symlink; a regular file), call `between()`, open the path, take `opened` metadata, refuse if `(opened.dev(), opened.ino()) != (named.dev(), named.ino())`, then the owner-uid and mode checks on `opened` (not `named`), then read the whole file into a `Zeroizing>` from that same handle. Added `use std::io::Read;`. In `load`, the File branch now calls `read_checked(path, &|| {})` and `value(std::mem::take(&mut *bytes))` so nothing unwiped remains. The three suites pass (5, 8, 5); the only remaining `std::fs::read(` is the credential branch and `check_file` is gone. `make gate` prints `gate: ok`. | ? | | M4a/15-gatewayd-main | 2026-09-24 | done | 1 | pass | none | Replaced the placeholder `src/main.rs` with the skeleton and copied `tests/main.rs`. `main` parses `args_os` like `loopd`'s; `serve` runs the five start checks in order — `Config::load` (`gatewayd: {e}\n{START_FAILED}`), `token_source` (`gatewayd: {}: {why}\n{START_FAILED}` with the path), `secrets::load` (`gatewayd: {e}`, its own `#secret-unavailable` pointer, then the file warning if any), then the state dir via `DirBuilder` recursive + `mode(0o700)` (`gatewayd: cannot prepare {}: {e}\n{START_FAILED}`), each returning exit 1 before anything is written. Then `run` with a `Log` that echoes to stderr and the returned `Stop` printed, exit 1. The todo comment matches the function and every test; the token never reaches output (it is only ever `expose`d inside `serve`, never here). `cargo test -p gatewayd` every suite passes; `make gate` prints `gate: ok` first run. | ? | | M4a/14-gatewayd-serve | 2026-09-24 | done | 1 | pass | none | Copied `tests/serve.rs`, `tests/serve_restart.rs`, `tests/support/fake_mm.rs`, `tests/support/gateway.rs` and the `src/serve/mod.rs`/`handle.rs` skeletons, added `pub mod serve;` to `lib.rs` (before `secrets`, alphabetical). Filled `mod.rs`: `From for Stop` → `Stop::State(e)`; `backoff` (the last backoff repeats past its end); `sleep_unless` (interruptible 20 ms steps until the deadline); `Gateway::connected` (log "connected to as ", route as this user from `users/me`, and on the first connection only, answer each in-flight turn with INTERRUPTED so a later reconnect does not). Filled `handle.rs`: `now_ms` (millis with an i64 clamp); `post` (a failing post logs, never throws); `tracked` ("D" or an allowed channel); `handle_post` (skip untracked/seen, record `handled` before routing so a crash never answers twice, then route with the state answering `knows_thread` — an `NotAllowed` stranger is logged by post id and user id only, never the text, a reply posts, a queued message joins the thread then pushes with Start/Waiting/Full); `start` (record the in-flight turn, spawn `deliver` on its own thread, and on a spawn error post LOOP_DOWN and un-busy the session); `finished` (drain done, end each turn, start the next batch); `typing` (a `user_typing` per running thread, seq incremented); `catch_up_channel` (no mark → mark "now" and stop, history not answered; else `posts_since`, log when `full`, replay each post). `SessionId` has no `Display`, so the start-error line formats `session.as_str()`. `serve` 6 and `serve_restart` 7 pass five runs in ~1.3 s; `make gate` prints `gate: ok` first run. | ? | | M4a/13-gatewayd-deliver | 2026-09-23 | done | 1 | pass | none | Copied `tests/deliver.rs`, `tests/support/fake_loop.rs` and the `src/deliver.rs` skeleton, added `pub mod deliver;` to `lib.rs` (before `http`, alphabetical). Filled `error_text` (the snake_case name serde gives the ErrorCode via `serde_json::to_value`, falling back to `` without ever formatting `ErrorCode` with `{}` since it has no Display); `split_answer` (blank/whitespace-only → [EMPTY_ANSWER]; else while the rest is longer than `MAX_POST` *characters*, cut at the last newline within the first `MAX_POST` chars past position 0 — dropping that newline — else at the byte index of the `MAX_POST`-th char via `char_indices().nth(MAX_POST)`, never inside one char); `one_turn` (connect fails → LoopDown("cannot connect to : "), write one id-1 final Turn envelope (write error → LoopDown), then read: (1, not final, TurnEvent)→on_event, (1, final, TurnDone)→Answer(content), (1, final, Error)→Refused, a read error → LoopDown("the turn ended early: "), anything else → LoopDown("an unexpected frame")); `run_turn` (one_turn with `batch.resume`, and when Refused(NoSuchSession) with `batch.resume` true, one more with resume false to create the session, as `bxctl chat --session`); `deliver` (post in the thread — ApprovalPending posts `approval_text` at once, then Answer→every split part in order, Refused→error_text, LoopDown→log "gatewayd: : " and post LOOP_DOWN; a failing post logs "gatewayd: cannot post in (thread ): " through a small `post` helper). All 9 tests pass; `make gate` prints `gate: ok` first run. | ? |