[client] Use atomic write/rename pattern for ssh config - #5867
Merged
Conversation
otherwise, ssh can fail due while the config file is being written and incomplete.
|
|
Contributor
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/ssh/config/manager.go`:
- Around line 228-235: The current code uses a fixed sshConfigPathTmp
(sshConfigPath + ".tmp") which can cause concurrent write/rename races; change
the logic in the function that writes the SSH config (references:
sshConfigPathTmp, writeFileWithTimeout, m.sshConfigDir, sshConfigPath,
sshConfig) to create a unique temp file in the same directory (e.g., via
os.CreateTemp or ioutil.TempFile), write to that temp file (using
writeFileWithTimeout or equivalent), ensure you close and remove the temp file
on any error paths, and then atomically rename the temp file to sshConfigPath;
also fix the error message to report the temp file name when write fails and
ensure cleanup happens on both write and rename failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 48e22866-c098-40d4-bc9c-0705dd0d0377
📒 Files selected for processing (1)
client/ssh/config/manager.go
Comment on lines
+228
to
235
| sshConfigPathTmp := sshConfigPath + ".tmp" | ||
|
|
||
| if err := os.MkdirAll(m.sshConfigDir, 0755); err != nil { | ||
| return fmt.Errorf("create SSH config directory %s: %w", m.sshConfigDir, err) | ||
| } | ||
|
|
||
| if err := writeFileWithTimeout(sshConfigPath, []byte(sshConfig), 0644); err != nil { | ||
| if err := writeFileWithTimeout(sshConfigPathTmp, []byte(sshConfig), 0644); err != nil { | ||
| return fmt.Errorf("write SSH config file %s: %w", sshConfigPath, err) |
Contributor
There was a problem hiding this comment.
Use a unique temp file path to avoid writer collisions.
Using a fixed *.tmp name allows concurrent calls to clobber each other and can produce rename/write races. Create a unique temp file in the same directory and clean it up on all failure paths.
Proposed fix
func (m *Manager) writeSSHConfig(sshConfig string) error {
sshConfigPath := filepath.Join(m.sshConfigDir, m.sshConfigFile)
- sshConfigPathTmp := sshConfigPath + ".tmp"
if err := os.MkdirAll(m.sshConfigDir, 0755); err != nil {
return fmt.Errorf("create SSH config directory %s: %w", m.sshConfigDir, err)
}
+
+ tmpFile, err := os.CreateTemp(m.sshConfigDir, m.sshConfigFile+".*.tmp")
+ if err != nil {
+ return fmt.Errorf("create temp SSH config file in %s: %w", m.sshConfigDir, err)
+ }
+ sshConfigPathTmp := tmpFile.Name()
+ _ = tmpFile.Close()
+ defer os.Remove(sshConfigPathTmp)
if err := writeFileWithTimeout(sshConfigPathTmp, []byte(sshConfig), 0644); err != nil {
return fmt.Errorf("write SSH config file %s: %w", sshConfigPath, err)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@client/ssh/config/manager.go` around lines 228 - 235, The current code uses a
fixed sshConfigPathTmp (sshConfigPath + ".tmp") which can cause concurrent
write/rename races; change the logic in the function that writes the SSH config
(references: sshConfigPathTmp, writeFileWithTimeout, m.sshConfigDir,
sshConfigPath, sshConfig) to create a unique temp file in the same directory
(e.g., via os.CreateTemp or ioutil.TempFile), write to that temp file (using
writeFileWithTimeout or equivalent), ensure you close and remove the temp file
on any error paths, and then atomically rename the temp file to sshConfigPath;
also fix the error message to report the temp file name when write fails and
ensure cleanup happens on both write and rename failures.
Contributor
Author
There was a problem hiding this comment.
concurrent calls should not happen unless multiple netbird clients are running. safer is better though, I agree - but I will leave it to the maintainers to improve this specific part incrementally (since my PR does not make things any less safe)
Contributor
There was a problem hiding this comment.
@lotheac, that's a fair point — a single running client is the normal case, so the fixed .tmp name collision risk is indeed theoretical. Your PR is strictly an improvement: writing directly to the final path was the unsafe baseline, and the atomic rename eliminates the partial-read race you set out to fix. The os.CreateTemp hardening would be a nice-to-have on top, but it's not a blocker for this change.
Would you like me to open a follow-up GitHub issue to track the os.CreateTemp improvement so it doesn't get lost?
ʕ •ᴥ•ʔ
🧠 Learnings used
Learnt from: lixmal
Repo: netbirdio/netbird PR: 5688
File: client/firewall/uspfilter/localip.go:20-21
Timestamp: 2026-03-27T14:40:41.583Z
Learning: In `client/firewall/uspfilter/localip.go` (netbirdio/netbird), `localIPManager.UpdateLocalIPs` is intentionally lock-free on writes. It is only ever called through `(uspfilter.Manager).UpdateLocalIPs` → `engine.handleSync`, which holds `e.syncMsgMux` for its entire execution, serializing all snapshot rebuilds. Do not flag the absence of an internal writer mutex on `localIPManager` as a race condition.
Learnt from: lixmal
Repo: netbirdio/netbird PR: 5826
File: client/internal/portforward/manager.go:35-38
Timestamp: 2026-04-08T14:55:48.251Z
Learning: In `client/internal/portforward/manager.go` (netbirdio/netbird), persisting permanent-lease (ttl=0/UPnP error 725 fallback) port mapping state for crash-recovery cleanup is intentionally deferred to a follow-up PR. The blocker is that `State.Cleanup` requires NAT gateway re-discovery via `nat.DiscoverGateway`, which can block startup for ~10 seconds when no gateway is present, affecting all clients. The TODO comment at line ~35 documents this constraint. Do not flag the missing state persistence for permanent leases as a blocking issue in this PR.
Learnt from: lixmal
Repo: netbirdio/netbird PR: 5687
File: client/internal/engine_ssh.go:44-50
Timestamp: 2026-03-25T06:24:52.840Z
Learning: In `client/internal/engine_ssh.go` (netbirdio/netbird), the IPv6 SSH setup steps — `AddListener` and `AddInboundDNAT` for the IPv6 overlay address — are intentionally best-effort and independent of each other, mirroring the v4 behavior. IPv6 soft-failure is by design so that IPv4 SSH continues to work when IPv6 cannot be set up. Do not flag the lack of atomicity between the IPv6 listener and IPv6 DNAT as a bug.
Learnt from: lixmal
Repo: netbirdio/netbird PR: 5530
File: management/internals/modules/reverseproxy/service/manager/manager.go:302-317
Timestamp: 2026-03-07T14:48:20.330Z
Learning: In `management/internals/modules/reverseproxy/service/manager/manager.go`, `checkPortConflict` uses `LockingStrengthUpdate` (SELECT FOR UPDATE) to serialize on existing port rows. For an unused port (no existing rows to lock), two concurrent transactions could both pass the DB check, but the proxy's OS-level port binding acts as the hard uniqueness guard — only one process can bind a given port. This dual-layer enforcement (DB best-effort + proxy binding) is the accepted design; no additional DB uniqueness constraint is needed.
Learnt from: lixmal
Repo: netbirdio/netbird PR: 5031
File: client/ssh/proxy/proxy.go:537-559
Timestamp: 2026-01-05T06:33:50.474Z
Learning: The NetBird SSH proxy (client/ssh/proxy/proxy.go) is ephemeral and tied to a single SSH client session. It operates through stdio and exists only for the lifetime of one client connection, so session-scoped contexts are appropriate for the proxy's lifecycle.
Learnt from: lixmal
Repo: netbirdio/netbird PR: 5587
File: shared/management/http/api/openapi.yml:3151-3171
Timestamp: 2026-03-13T06:22:59.436Z
Learning: Repo netbirdio/netbird PR `#5587`: For reverse proxy header authentication, HeaderAuthConfig.value is required in requests and cleared in responses. On update, Manager.preserveExistingAuthSecrets() calls preserveHeaderAuthHashes() before validateHeaderAuthValues() (management/internals/modules/reverseproxy/service/manager/manager.go Lines ~555–597), so sending value: "" preserves the existing hashed secret; providing a non-empty value rotates it.
Learnt from: bcmmbaga
Repo: netbirdio/netbird PR: 5441
File: management/server/user.go:745-749
Timestamp: 2026-02-24T19:32:13.189Z
Learning: In Go codebases like netbirdio/netbird, methods returning (T, error) should follow the convention: if an error is returned, propagate it and return early. When the caller handles the error immediately, explicit nil checks on the returned value are unnecessary; rely on the error to guard control flow and only inspect the value when err is nil. This guidance applies broadly to Store methods such as GetUserByUserID in Go code; prefer early return on error and avoid redundant nil checks after a successful error path.
Learnt from: lixmal
Repo: netbirdio/netbird PR: 5530
File: proxy/internal/udp/relay.go:276-278
Timestamp: 2026-03-07T13:59:16.506Z
Learning: For Go 1.25+ projects, use sync.WaitGroup.Go() to run goroutines because it handles Add(1) and defer Done() automatically. Do not flag wg.Go(...) as a compilation error in codebases that require Go 1.25 or newer. If supporting older Go versions, gate such usage with build tags or avoid wg.Go() to maintain compatibility; verify the module targets Go 1.25+ before adopting this pattern.
Learnt from: lixmal
Repo: netbirdio/netbird PR: 5688
File: client/firewall/uspfilter/forwarder/icmp.go:268-287
Timestamp: 2026-03-25T09:21:53.814Z
Learning: When using gVisor’s `header.ICMPv6Checksum(ICMPv6ChecksumParams)` (`gvisor.dev/gvisor/pkg/tcpip/header`), the pseudo-header checksum is computed internally from `Src`, `Dst`, and `Header`. Use `PayloadCsum`/`PayloadLen` only for scattered-buffer cases where the ICMPv6 payload bytes are not included in `Header`. If `Header` already contains the complete ICMPv6 message (ICMPv6 header + all data), call `ICMPv6Checksum` with `PayloadCsum: 0` and `PayloadLen: 0` (or omit those fields). Do not pass a separately computed pseudo-header checksum (e.g., `header.PseudoHeaderChecksum(...)`) as `PayloadCsum` in that case, because it will be double-counted and produce an incorrect checksum.
lixmal
approved these changes
May 4, 2026
lixmal
left a comment
lixmal
left a comment
Collaborator
There was a problem hiding this comment.
Atomic write/rename is the right fix. LGTM. Will follow up with a separate PR using a unique temp filename and cleanup-on-failure.
lixmal
changed the title
7 tasks
7 tasks
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
3 participants
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.
otherwise, ssh can fail due while the config file is being written and incomplete.
Describe your changes
We've seen some errors running
sshwhen netbird is managing /etc/ssh_config.d/99-netbird.conf --sshmight read an incomplete file if the netbird client happens to be in the middle of writing to it.Issue ticket number and link
none
Stack
Checklist
Documentation
Select exactly one:
This is a bug fix and does not need to be documented separately.
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit