boxmaker/docs/plans/M4a/16-gatewayd-secret-file-race.md

# M4a task 16: read a secret file from the file that was checked

**Branch:** `m4a` (run `git switch m4a`; `git status --short` must be empty, otherwise stop)
**Commit subject:** `gatewayd: read a secret file from the file that was checked`

## Goal

**M4a review, finding 2.** `load` checks a secret file by its path (`check_file`: not a symbolic
link, a regular file, owned by us, mode 0600 or 0400), then reads it **by the path again**
(`std::fs::read(path)`). Between the two, someone who can write to the directory could replace the
file with a link to another file, or with another file, and `gatewayd` would read that instead.

The fix, without any new dependency: check the path, open it, then check the **opened** file, and
read from that same handle.

1. Check the path, as now: absolute; `symlink_metadata` (it does not follow links); not a link; a
   regular file. Keep that metadata as `named`.
2. Call `between()` (see below).
3. Open the path (`std::fs::File::open`).
4. Take the open file's metadata (`file.metadata()`, as `opened`). If its device and inode are not
   the ones in `named`, the path changed between the check and the open: refuse. Then the owner
   and mode checks, on `opened`.
5. Read the whole file from that handle into a `Zeroizing<Vec<u8>>`.

`between` exists only for the tests: it runs after step 1 and before step 3, and each test swaps
the file there. `load` passes `&|| {}`.

## Files

- Copy: `crates/gatewayd/tests/secrets_race.rs`
- Modify: `crates/gatewayd/src/secrets.rs`, `docs/implementer-log.md`

## Before anything else

Your **first two actions** are steps 1 and 2 below: copy the test, and see it fail. Everything you
need is in this file; read only `crates/gatewayd/src/secrets.rs` besides it.

## The changes, exactly

**1. Replace `fn check_file` with `pub fn read_checked`.** Delete `check_file` and write in its
place:

```rust
/// 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<Zeroizing<Vec<u8>>, String> {
    // 1. `path.is_absolute()`, else "<path> is not an absolute path".
    // 2. `let named = std::fs::symlink_metadata(path)`, an Err(e) is "cannot read <path>: <e>";
    //    `named.file_type().is_symlink()` is "<path> is a symbolic link";
    //    `!named.file_type().is_file()` is "<path> is not a regular file".
    // 3. `between();`
    // 4. `let mut file = std::fs::File::open(path)`, an Err(e) is "cannot read <path>: <e>";
    //    `let opened = file.metadata()`, the same error text.
    // 5. `if (opened.dev(), opened.ino()) != (named.dev(), named.ino())`:
    //    "<path> changed while it was read".
    // 6. The owner and mode checks from the old `check_file`, word for word, on `opened` (not on
    //    `named`): the uid of "/proc/self"; "<path> is not owned by the user gatewayd runs as";
    //    "<path> has mode <mode:03o>; 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 <path>: <e>". Ok(bytes).
    todo!()
}
```

Every message is built with `path.display()`, as in the old `check_file`. `dev`, `ino`, `uid` and
`mode` come from `std::os::unix::fs::MetadataExt`, which the file already imports. Add
`use std::io::Read;` for `read_to_end`.

**2. In `load`, the `SecretSource::File(path)` branch.** Replace the `check_file` call and the
`std::fs::read(path)` after it with:

```rust
            let mut bytes = match read_checked(path, &|| {}) {
                Ok(bytes) => bytes,
                Err(why) => {
                    return Err(SecretError {
                        name: name.to_string(),
                        why,
                    });
                }
            };
```

and the `value(bytes)` call in that branch becomes `value(std::mem::take(&mut *bytes))`, which
moves the bytes out and leaves nothing unwiped behind. The warning and the other two branches do
not change.

## Steps

- [ ] **1. Copy.** `git switch m4a`, then
  `cp docs/plans/M4a/files/crates/gatewayd/tests/secrets_race.rs crates/gatewayd/tests/`
- [ ] **2. See it fail.** `cargo test -p gatewayd --test secrets_race`. Expected: it does not
  compile (`read_checked` does not exist).
- [ ] **3. Make the two changes.** Write `read_checked` step by step as its comment says,
  `cargo check -p gatewayd` after it, then change `load`. Run `cargo fmt --all`.
- [ ] **4. See it pass.** `cargo test -p gatewayd --test secrets_race --test secrets --test main`.
  Expected: 5, 8 and 5 passed.
- [ ] **5. Check.** `grep -n "check_file\|std::fs::read(" crates/gatewayd/src/secrets.rs` prints
  only the credential branch's `std::fs::read(&path)`.
- [ ] **6. Run the gate.** `make gate`. Expected last line: `gate: ok`, with about 769 tests.
- [ ] **7. Log and commit.** `git add crates/gatewayd docs/implementer-log.md Cargo.lock && git commit`

This is the last follow-up task of M4a. Stop after the commit.

## Done when

- The three suites pass; `make gate` prints `gate: ok`.

## Stop and report if

- A comment above the `todo!()` does not fit, or contradicts a test: quote it.
- A test seems to need `unsafe`, `libc` or any new dependency.