diff --git a/src/cli.zig b/src/cli.zig index 0dcb2da9..e9854bb9 100644 --- a/src/cli.zig +++ b/src/cli.zig @@ -251,7 +251,7 @@ pub const usage_text = \\ graff login codex [--refresh] ChatGPT/Codex OAuth login (PKCE) \\ graff login kimi Kimi Code OAuth login (device-code) \\ graff login xai Grok/SuperGrok OAuth login (device-code) - \\ graff key set store a key (macOS Keychain, else 0600 file) + \\ graff key set store a key (Keychain; POSIX 0600 file; Windows home ACL) \\ graff key list show which providers have keys \\ graff models [refresh] list the live catalog; refresh Codex + models.dev metadata \\ graff route … dry-run which provider/billing a model lands on (no API call) diff --git a/src/credential_store.zig b/src/credential_store.zig index a9c5d03b..8fc192e7 100644 --- a/src/credential_store.zig +++ b/src/credential_store.zig @@ -7,11 +7,10 @@ //! silent logout with nothing left to recover from. Write a per-writer-unique //! temp file in the target's own directory, fsync it, then rename over the //! target, so a reader sees either the whole old file or the whole new one. -//! -//! The guarantee is against a crashed process, not against power loss: the temp -//! file is fsynced but the containing directory is not, so some filesystems can -//! still lose the rename across a hard power cut. Same shape — and same limit — -//! as learn_store.writeAtomicReplace / eval_memory.writeNotes. +//! On POSIX, fsync the containing directory after the rename so that directory +//! entry is also durable across power loss. Zig 0.17 cannot portably flush +//! Windows directory handles, so Windows keeps the atomic replacement and +//! synced file contents but has a weaker power-loss guarantee. const std = @import("std"); const builtin = @import("builtin"); @@ -83,6 +82,19 @@ fn writeDestination(io: Io, dir: Io.Dir, sub_path: []const u8, allocator: Alloca } } +fn syncDirectory(io: Io, dir: Io.Dir) !void { + if (builtin.os.tag == .windows) return; + // Linux openDir handles may use O_PATH, which fsync rejects. Reopen the + // exact createFileAtomic destination directory read-only as a File. + const file = try dir.openFile(io, ".", .{ + .allow_directory = true, + .follow_symlinks = false, + .resolve_beneath = true, + }); + defer file.close(io); + try file.sync(io); +} + /// Replace `dir`/`sub_path` with `bytes` atomically. `sub_path` may carry /// directory components; createFileAtomic keeps the temp file in the target's /// own directory, so the rename never crosses a filesystem. If `sub_path` is a @@ -116,6 +128,7 @@ pub fn replaceFile(io: Io, dir: Io.Dir, sub_path: []const u8, bytes: []const u8, try atomic.file.writeStreamingAll(io, bytes); try atomic.file.sync(io); try atomic.replace(io); + try syncDirectory(io, atomic.dir); } /// #477: the process $HOME, pinned once at startup (startup.zig, beside @@ -203,6 +216,47 @@ test "replaceFile: renames a whole new file into place, never truncating the tar try std.testing.expectEqual(@as(usize, 1), try entryCount(io, tmp.dir)); } +test "replaceFile: nested target directories keep the replacement atomic" { + if (builtin.os.tag == .windows) return; + const io = std.testing.io; + var tmp = std.testing.tmpDir(.{ .iterate = true }); + defer tmp.cleanup(); + try tmp.dir.createDir(io, "nested", private_dir); + try tmp.dir.createDir(io, "nested/credentials", private_dir); + try tmp.dir.writeFile(io, .{ .sub_path = "nested/credentials/key.json", .data = "old" }); + + const original = try tmp.dir.openFile(io, "nested/credentials/key.json", .{}); + defer original.close(io); + try replaceFile(io, tmp.dir, "nested/credentials/key.json", "new", private_file); + + var read_buffer: [16]u8 = undefined; + var reader = original.reader(io, &read_buffer); + const before = try reader.interface.allocRemaining(std.testing.allocator, .limited(64)); + defer std.testing.allocator.free(before); + try std.testing.expectEqualStrings("old", before); + const after = try tmp.dir.readFileAlloc(io, "nested/credentials/key.json", std.testing.allocator, .limited(64)); + defer std.testing.allocator.free(after); + try std.testing.expectEqualStrings("new", after); + + // createFileAtomic must consume its sibling temporary file in the actual + // containing directory, not leave it beside the nested target. + var parent = try tmp.dir.openDir(io, "nested/credentials", .{ .iterate = true }); + defer parent.close(io); + try std.testing.expectEqual(@as(usize, 1), try entryCount(io, parent)); +} + +test "syncDirectory: an opened nested destination directory is syncable" { + if (builtin.os.tag == .windows) return; + const io = std.testing.io; + var tmp = std.testing.tmpDir(.{}); + defer tmp.cleanup(); + try tmp.dir.createDir(io, "nested", private_dir); + try tmp.dir.createDir(io, "nested/credentials", private_dir); + const parent = try tmp.dir.openDir(io, "nested/credentials", .{}); + defer parent.close(io); + try syncDirectory(io, parent); +} + // The #477 g_home fallback test lives in startup_tests.zig (with the other // credential-scope regressions): exactly ONE test in the suite may mutate // g_home, because the parallel test runner makes two mutators a coin flip. @@ -357,6 +411,18 @@ test "#405: replaceFile rejects a symlink cycle without replacing either link" { try std.testing.expectEqual(@as(usize, 2), try entryCount(io, tmp.dir)); } +test "replaceFile: private mode stays 0600 through repeated replacements" { + if (!Io.File.Permissions.has_executable_bit) return; + const io = std.testing.io; + var tmp = std.testing.tmpDir(.{ .iterate = true }); + defer tmp.cleanup(); + + try replaceFile(io, tmp.dir, "secret.json", "first", private_file); + try std.testing.expectEqual(@as(std.posix.mode_t, 0o600), (try tmp.dir.statFile(io, "secret.json", .{})).permissions.toMode() & 0o777); + try replaceFile(io, tmp.dir, "secret.json", "second", private_file); + try std.testing.expectEqual(@as(std.posix.mode_t, 0o600), (try tmp.dir.statFile(io, "secret.json", .{})).permissions.toMode() & 0o777); +} + test "writeOAuth: the credential file is 0600 inside 0700 directories" { if (builtin.os.tag == .windows) return; const io = std.testing.io; diff --git a/src/keys_cli.zig b/src/keys_cli.zig index 30eca996..3a41099c 100644 --- a/src/keys_cli.zig +++ b/src/keys_cli.zig @@ -3,8 +3,10 @@ //! //! Safe API-key store: on macOS, keys live in the login Keychain (service //! "simple-harness", account=provider id) via the `security` CLI — never on -//! disk in plaintext. Elsewhere they fall back to a 0600 file -//! (~/.simple-harness-keys.json). `harness key set ` writes; +//! disk in plaintext. Other platforms use ~/.simple-harness-keys.json: mode +//! 0600 on POSIX, while Windows inherits the home directory's ACL because Zig +//! 0.17 exposes no portable API for installing an owner-only DACL. +//! `harness key set ` writes; //! startup reads the selected provider first (env always wins), then fills the //! remaining providers when a picker, switch, resume, or fallback needs them. //! @@ -109,7 +111,8 @@ pub fn storeKey(io: Io, gpa: Allocator, arena: Allocator, home: []const u8, prov const term = child.wait(io) catch return false; return term == .exited and term.exited == 0; } - // Linux/other: merge into a 0600 JSON file. + // Non-macOS: merge into one JSON file. POSIX uses 0600; Windows inherits + // the home directory ACL because Io.File.Permissions has no ACL semantics. _ = gpa; const path = std.fmt.allocPrint(arena, "{s}/{s}", .{ home, keys_file }) catch return false; var obj: std.json.ObjectMap = .empty; @@ -122,10 +125,10 @@ pub fn storeKey(io: Io, gpa: Allocator, arena: Allocator, home: []const u8, prov var aw: Io.Writer.Allocating = .init(arena); var s: std.json.Stringify = .{ .writer = &aw.writer }; s.write(Value{ .object = obj }) catch return false; - // Atomic + 0600: this is a read-modify-write of EVERY provider's key, so a - // truncate-in-place that dies mid-write does not lose one key, it loses all - // of them at once. 0600 also makes the "0600 JSON file" above true: the old - // createFile took the umask default, i.e. world-readable API keys. + // Atomic + private on POSIX: this is a read-modify-write of EVERY + // provider's key, so a truncate-in-place that dies mid-write does not lose + // one key, it loses all of them at once. The old POSIX createFile took the + // umask default, which could leave API keys world-readable. credential_store.replaceFile(io, Io.Dir.cwd(), path, aw.writer.buffered(), credential_store.private_file) catch return false; return true; } @@ -154,6 +157,64 @@ pub fn loadStoredKey(io: Io, arena: Allocator, home: []const u8, provider: []con return null; } +test "file-backed key store merges and loads keys on its live platforms" { + if (builtin.os.tag == .macos) return; + const io = std.testing.io; + var tmp = std.testing.tmpDir(.{}); + defer tmp.cleanup(); + var arena_state = std.heap.ArenaAllocator.init(std.testing.allocator); + defer arena_state.deinit(); + const arena = arena_state.allocator(); + var home_buf: [std.fs.max_path_bytes]u8 = undefined; + const home = home_buf[0..try tmp.dir.realPath(io, &home_buf)]; + + try std.testing.expect(storeKey(io, std.testing.allocator, arena, home, "openai", "sk-first")); + try std.testing.expect(storeKey(io, std.testing.allocator, arena, home, "anthropic", "sk-second")); + try std.testing.expectEqualStrings("sk-first", loadStoredKey(io, arena, home, "openai").?); + try std.testing.expectEqualStrings("sk-second", loadStoredKey(io, arena, home, "anthropic").?); + + if (Io.File.Permissions.has_executable_bit) { + const path = try std.fmt.allocPrint(arena, "{s}/{s}", .{ home, keys_file }); + const mode = (try Io.Dir.cwd().statFile(io, path, .{})).permissions.toMode() & 0o777; + try std.testing.expectEqual(@as(std.posix.mode_t, 0o600), mode); + } +} + +test "file-backed key store preserves keys across repeated read-modify-writes" { + if (builtin.os.tag == .macos) return; + const io = std.testing.io; + var tmp = std.testing.tmpDir(.{ .iterate = true }); + defer tmp.cleanup(); + var arena_state = std.heap.ArenaAllocator.init(std.testing.allocator); + defer arena_state.deinit(); + const arena = arena_state.allocator(); + var home_buf: [std.fs.max_path_bytes]u8 = undefined; + const home = home_buf[0..try tmp.dir.realPath(io, &home_buf)]; + + try std.testing.expect(storeKey(io, std.testing.allocator, arena, home, "openai", "sk-openai-initial")); + try std.testing.expect(storeKey(io, std.testing.allocator, arena, home, "anthropic", "sk-anthropic-initial")); + try std.testing.expect(storeKey(io, std.testing.allocator, arena, home, "deepseek", "sk-deepseek-initial")); + + // Each update rereads the file produced by the preceding update. Checking + // after every pass catches a lost unrelated provider at the exact write + // that drops it, rather than only observing the final state. + for (0..8) |round| { + const openai_key = try std.fmt.allocPrint(arena, "sk-openai-{d}", .{round}); + const anthropic_key = try std.fmt.allocPrint(arena, "sk-anthropic-{d}", .{round}); + try std.testing.expect(storeKey(io, std.testing.allocator, arena, home, "openai", openai_key)); + try std.testing.expect(storeKey(io, std.testing.allocator, arena, home, "anthropic", anthropic_key)); + try std.testing.expectEqualStrings(openai_key, loadStoredKey(io, arena, home, "openai").?); + try std.testing.expectEqualStrings(anthropic_key, loadStoredKey(io, arena, home, "anthropic").?); + try std.testing.expectEqualStrings("sk-deepseek-initial", loadStoredKey(io, arena, home, "deepseek").?); + } + + if (Io.File.Permissions.has_executable_bit) { + const path = try std.fmt.allocPrint(arena, "{s}/{s}", .{ home, keys_file }); + const mode = (try Io.Dir.cwd().statFile(io, path, .{})).permissions.toMode() & 0o777; + try std.testing.expectEqual(@as(std.posix.mode_t, 0o600), mode); + } +} + pub const StoredKeyScope = union(enum) { all, provider: []const u8, diff --git a/src/startup_keys.zig b/src/startup_keys.zig index 3461fb74..3ab9805b 100644 --- a/src/startup_keys.zig +++ b/src/startup_keys.zig @@ -205,7 +205,7 @@ pub fn resolveKeys(io: Io, gpa: Allocator, arena: Allocator, environ_map: anytyp std.process.fatal( \\no API key found. quickest fixes: \\ graff login free codegraff key (device-code OAuth) - \\ graff key set store a key (macOS Keychain, else 0600 file) + \\ graff key set store a key (Keychain; POSIX 0600 file; Windows home ACL) \\ export ANTHROPIC_API_KEY=sk-ant-… or CODEGRAFF/DEEPSEEK/OPENAI/MINIMAX/XIAOMI/KIMI/MOONSHOT/XAI/ZAI _API_KEY \\a Codex CLI login (~/.codex/auth.json) is also picked up automatically. , .{});