fix: write iroh secret keys with 0600 permissions - #28
Merged
Merged
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The new Unix write helper should avoid in-place truncation to prevent partial/corrupt key files on crash and to eliminate a local-FD exposure window by using an atomic temp-file + rename approach.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (2)
What changed in this PR
This PR hardens local secret-key persistence for the Rust tunnel agent by ensuring iroh secret keys are written with restrictive permissions (0600) on Unix, preventing other local users from reading tunnel identities due to permissive umask defaults.
Changes:
- Adds a shared
write_secret_keyhelper inRepoto write keys with0600on Unix (and tighten permissions on overwrite), while keeping non-Unix behavior as a plain write. - Routes all secret-key write sites in
connect-lib/libthrough the helper (connect key, per-tunnel listen key, and timestamped listen key files). - Refactors the bin listen handler to reuse
Repo::save_listen_key_for_tunnelinstead of duplicating the per-tunnel key write logic.
| File | Description |
|---|---|
| connect-lib/lib/src/repo.rs | Introduces write_secret_key and updates all key-write paths; adds Unix-only permission tests. |
| connect-lib/bin/src/main.rs | Removes inline per-tunnel key persistence and delegates to Repo::save_listen_key_for_tunnel. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Collaborator
Author
|
@copilot resolve the merge conflicts on this branch. |
Resolved on this branch in |
|
|
rawkode
force-pushed
the
claude/codebase-quality-scan-wq7519
branch
from
September 19, 2026 15:49
9e679de to
91bf60f
Compare
connect_key, the per-project and per-tunnel listen_key files, and the timestamped listen_key.* files were all written with tokio::fs::write under the default umask, leaving the iroh identity readable by any local user. Route every key write through a single write_secret_key helper that creates the file with mode 0600 on Unix and tightens the mode on an existing file so already-installed keys become private the next time they are rewritten. Non-Unix platforms keep the plain write. The listen handler in the binary had its own inline copy of the per-tunnel key write; it now calls Repo::save_listen_key_for_tunnel like the resume path does. This commit was created with the assistance of a LLM.
Review follow-up. The helper truncated the existing key file in place, so a crash between truncate and write could leave an empty or partial key, and a local user who had already opened the previously world-readable file could read the new key through their descriptor. Write to a fresh temporary file beside the target (created with mode 0600 on Unix), fsync it, and rename it over the key path. The rename is atomic on every supported platform, so the old inode is never modified and a failed write leaves the previous key intact. The temporary file is removed on any failure. The doc comment no longer claims the non-Unix state directory lives under the user's profile; Repo accepts any base directory, so it describes the inherited access control instead. Tests are converted to the Result-returning style the workspace lint gate now requires, and two are added: writes leave no temp files, and a failed write cleans up its own temp file. This commit was created with the assistance of a LLM.
rawkode
force-pushed
the
claude/codebase-quality-scan-wq7519
branch
from
September 19, 2026 15:58
91bf60f to
4db882f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Every iroh secret key (
connect_key, the per-project and per-tunnellisten_key, and the timestampedlisten_key.*files) was written withtokio::fs::writeunder the default umask, so the tunnel's identity was readable by any local user.Changes
connect-lib/lib/src/repo.rs: new privatewrite_secret_keyhelper. It writes the key to a fresh temporary file beside the target, created with mode 0600 on Unix, fsyncs it, and renames it over the key path. The rename is atomic on every supported platform, so a crash mid-write cannot leave a truncated key, and a reader holding a descriptor to a previously world-readable file never sees the new key. The temp file is removed on any failure. All three key write sites (listen_key,save_listen_key_for_tunnel,create_key) go through it.connect-lib/bin/src/main.rs: the listen handler had its own inline copy of the per-tunnel key write. It now callsRepo::save_listen_key_for_tunnel, the same path resume uses.Not in scope
Existing keys are replaced only when rewritten. A stable per-tunnel key is never rewritten, so already-installed keys stay at their current mode until the read path also tightens them. Left out to keep this to writes only.
Verification
Branch is merged up to
developat 144f60d (PR #27), so the full lint gate applies.cargo fmt --all --checkcargo clippy --workspace --all-targetscargo test --workspacego build ./...andgo test ./internal/exec/The
#[cfg(not(unix))]path shares the temp-and-rename code and differs only by not setting mode bits. It was not compiled locally; the release workflow's Windows targets cover it.