M4a task 16: read a secret file from the file that was checked (review finding 2)
The test swaps the file between the check and the open through a `between` hook. A reference fix passed it and the gate (769 tests) in the working tree, caught two mutations, and was removed. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,113 @@
|
||||
# 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.
|
||||
Reference in New Issue
Block a user