[client] Use setsid to avoid the parent process from being killed via HUP by login#4900
[client] Use setsid to avoid the parent process from being killed via HUP by login#4900
Conversation
WalkthroughAdded a util‑linux probe and Server field Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
lixmal
changed the title
lixmal
force-pushed
the
fix-util-linux-login-ssh
branch
from
40e8af6 to
8946677
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/ssh/server/userswitching_unix.go (1)
109-126: Setsid wrapping logic is sound with graceful degradation.The implementation correctly wraps util-linux login with
setsid -w -cto preventvhangup()from killing the parent process. The flags are appropriate:
-wensures setsid waits for the child to exit-csets the controlling terminalThe graceful degradation (lines 118-122) logs a warning but continues without setsid if it's unavailable. While this means the fix won't be applied in such cases, it prevents complete failure and is a reasonable trade-off.
The TODO comment on line 112 suggests eliminating the external setsid dependency by using
syscall.Setsid()+TIOCSCTTY+syscall.Exec(). Would you like me to help implement this approach or open an issue to track it?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
client/ssh/server/command_execution_js.go(1 hunks)client/ssh/server/command_execution_unix.go(2 hunks)client/ssh/server/command_execution_windows.go(1 hunks)client/ssh/server/server.go(2 hunks)client/ssh/server/userswitching_unix.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-14T13:05:31.729Z
Learnt from: lixmal
Repo: netbirdio/netbird PR: 4015
File: client/ssh/server/userswitching_windows.go:89-139
Timestamp: 2025-11-14T13:05:31.729Z
Learning: In client/ssh/server/executor_windows.go, the WindowsExecutorConfig struct's Pty, PtyWidth, and PtyHeight fields are intentionally left unused for now and will be implemented in a future update.
Applied to files:
client/ssh/server/command_execution_windows.go
🧬 Code graph analysis (3)
client/ssh/server/command_execution_js.go (1)
client/ssh/server/server.go (1)
Server(115-143)
client/ssh/server/userswitching_unix.go (1)
client/ssh/server/server.go (1)
Server(115-143)
client/ssh/server/command_execution_unix.go (1)
client/ssh/server/server.go (1)
Server(115-143)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Client / Unit (386)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: iOS / Build
- GitHub Check: Android / Build
- GitHub Check: Client / Unit
- GitHub Check: Linux
- GitHub Check: Darwin
- GitHub Check: Windows
- GitHub Check: Client / Unit
- GitHub Check: release
- GitHub Check: JS / Lint
- GitHub Check: Client / Unit
🔇 Additional comments (9)
client/ssh/server/command_execution_js.go (1)
45-48: LGTM!The stub implementation correctly returns false for the JS/WASM platform, consistent with other platform-specific detection methods in this file.
client/ssh/server/userswitching_unix.go (2)
90-90: Good refactoring to dedicated helper.Delegating Linux-specific login command construction to
getLinuxLoginCmdimproves code organization and maintainability.
98-107: Arch Linux special case handling looks correct.The conditional logic properly detects Arch Linux without PAM remote support and adjusts the login arguments accordingly. This addresses platform-specific differences in login implementations.
client/ssh/server/server.go (2)
141-142: Field addition follows existing pattern.The
loginIsUtilLinuxfield is appropriately placed and follows the same caching pattern assuSupportsPty, storing the detection result for use during command execution.
197-197: Initialization follows established pattern.The detection is appropriately performed once during server startup, matching the approach used for
suSupportsPtydetection on line 196.client/ssh/server/command_execution_windows.go (2)
386-389: LGTM!The stub implementation correctly returns false for Windows, matching the pattern of other Unix-specific detection methods and enabling cross-platform compilation.
393-400: Defensive validation added for PTY command execution.This check prevents
executeCommandWithPtyfrom being called with an empty command string, logging an error and exiting with status 1 if the condition is violated.Note: This validation appears unrelated to the main PR objective (setsid wrapping for util-linux login). It's a defensive check that may have been added as a separate fix. The Unix version of
executeCommandWithPty(client/ssh/server/command_execution_unix.go) does not have an equivalent check.Could you clarify if this validation change is intentionally part of this PR, or if it was discovered and fixed during related work? If it's a separate concern, consider extracting it to a dedicated commit for clearer history.
client/ssh/server/command_execution_unix.go (2)
166-166: Logging improvement provides better visibility.Changing from logging just
execCmd.Pathtostrings.Join(execCmd.Args, " ")provides full visibility into the constructed command, including the setsid wrapper and all arguments. This aids in debugging and verification of the changes introduced in this PR.
78-95: Detection logic is sound and assumptions are verified.The implementation correctly identifies util-linux login by:
- Running
login --versionwith an appropriate 500ms timeout- Falling back to false (assuming shadow-utils) if the command fails—shadow-utils login does not support
--version- Checking for "util-linux" in the output string—util-linux consistently outputs
login (util-linux) X.Y.ZThe string matching approach is pragmatic and reliable across distributions, with no concerns about format changes in the near term.
lixmal
force-pushed
the
fix-util-linux-login-ssh
branch
from
8946677 to
fcf8a17
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/ssh/server/userswitching_unix.go (1)
128-132: Consider makingfileExistsa package-level helper.This is a simple utility function with a receiver that isn't used. Consider making it a standalone function if used elsewhere, or keeping as-is if this is the only usage site.
-func (s *Server) fileExists(path string) bool { +func fileExists(path string) bool { _, err := os.Stat(path) return err == nil }You'd also need to update the call sites at lines 103.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
client/ssh/server/command_execution_js.go(1 hunks)client/ssh/server/command_execution_unix.go(3 hunks)client/ssh/server/command_execution_windows.go(1 hunks)client/ssh/server/server.go(2 hunks)client/ssh/server/userswitching_unix.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- client/ssh/server/server.go
- client/ssh/server/command_execution_js.go
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-14T13:05:31.729Z
Learnt from: lixmal
Repo: netbirdio/netbird PR: 4015
File: client/ssh/server/userswitching_windows.go:89-139
Timestamp: 2025-11-14T13:05:31.729Z
Learning: In client/ssh/server/executor_windows.go, the WindowsExecutorConfig struct's Pty, PtyWidth, and PtyHeight fields are intentionally left unused for now and will be implemented in a future update.
Applied to files:
client/ssh/server/command_execution_windows.go
📚 Learning: 2025-11-13T00:29:53.247Z
Learnt from: lixmal
Repo: netbirdio/netbird PR: 4015
File: client/cmd/ssh_exec_unix.go:53-74
Timestamp: 2025-11-13T00:29:53.247Z
Learning: In client/ssh/server/executor_unix.go, the method ExecuteWithPrivilegeDrop(ctx context.Context, config ExecutorConfig) has a void return type (no error return). It handles failures by exiting the process directly with appropriate exit codes rather than returning errors to the caller.
Applied to files:
client/ssh/server/command_execution_windows.go
🧬 Code graph analysis (3)
client/ssh/server/userswitching_unix.go (1)
client/ssh/server/server.go (1)
Server(115-143)
client/ssh/server/command_execution_unix.go (1)
client/ssh/server/server.go (1)
Server(115-143)
client/ssh/server/command_execution_windows.go (1)
client/ssh/server/server.go (1)
Server(115-143)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Signal / Unit (amd64)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client / Unit (386)
- GitHub Check: Client / Unit
- GitHub Check: Windows
- GitHub Check: Linux
- GitHub Check: Darwin
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
- GitHub Check: iOS / Build
- GitHub Check: Android / Build
- GitHub Check: JS / Lint
- GitHub Check: release_ui_darwin
- GitHub Check: release
🔇 Additional comments (6)
client/ssh/server/command_execution_unix.go (2)
79-100: Well-implemented detection with proper safeguards.The implementation correctly:
- Guards with
runtime.GOOS != "linux"even under unix build tag (defensive)- Uses a reasonable 500ms timeout to avoid blocking
- Gracefully handles errors by returning false (shadow-utils assumption)
- Documents the rationale with a reference to the Debian bug
171-171: Improved logging for debugging.Logging
execCmd.Argsinstead of justexecCmd.Pathprovides better visibility into the full command being executed, which is helpful for troubleshooting login issues. Since this is an interactive shell command (not user-provided secrets), this is safe to log.client/ssh/server/command_execution_windows.go (2)
386-389: Correct platform stub implementation.The Windows stub correctly returns
falsesince util-linux is not applicable on Windows. This maintains interface consistency across platforms.
392-403: Good defensive guard for empty commands.Adding explicit handling for empty command strings prevents unexpected behavior during PTY execution. The early exit with proper logging and session exit is appropriate.
client/ssh/server/userswitching_unix.go (2)
90-90: Clean delegation to platform-specific helper.The refactoring to delegate Linux-specific logic to
getLinuxLoginCmdimproves code organization and maintainability.
98-126: Solid implementation addressing the vhangup() issue.The logic correctly:
- Handles the Arch Linux edge case for missing
/etc/pam.d/remote- Returns early if util-linux is not detected
- Gracefully falls back when
setsidis unavailable (with warning)- Uses
-w -cflags appropriately:-wwaits for the child process and returns its exit status,-csets the current terminal as the controlling terminal for the new session, together preventing vhangup() from killing the parentThe TODO comment (lines 112-113) acknowledges a cleaner future approach using
syscall.Setsid()+TIOCSCTTY+syscall.Exec()to eliminate the external dependency.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
client/ssh/server/userswitching_unix.go (2)
99-127:getLinuxLoginCmdcorrectly handles Arch and util‑linuxsetsidwrapping; consider cachingsetsidlookupThe Arch‑specific PAM check, the default
-f <user> -h <remoteIP> -pbranch, and the conditionalsetsid -w -c <loginPath> ...wrapping whenloginIsUtilLinuxis true all look consistent with the intended behavior and provide a reasonable fallback whensetsidis missing (warning + plainlogin).One improvement you might consider (non‑blocking) is caching the
setsidlookup / availability in theServerstruct or via async.Once, so you avoid repeatedexec.LookPath("setsid")calls and duplicate warnings on every new session.Please verify this on at least one util‑linux system (and one non‑util‑linux system) to confirm that:
- the
setsidpath solves the original HUP issue, and- the fallback path still behaves as before when
loginIsUtilLinuxis false orsetsidis not present.
129-133:fileExistshelper is fine for current use; optional refinement if you need richer error handlingUsing
os.Statand returningerr == nilis sufficient for simple checks like/etc/arch-releaseand/etc/pam.d/remote, where treating any error as “does not exist / not usable” is acceptable.If you ever need to distinguish “file truly missing” from other failures (permissions, transient I/O), you could extend this helper to branch on specific error kinds rather than collapsing everything into a boolean.
If you decide to tighten this helper later, please review all its call sites to ensure they actually want differentiated error handling instead of the current simple boolean.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/ssh/server/userswitching_unix.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
client/ssh/server/userswitching_unix.go (1)
client/ssh/server/server.go (1)
Server(115-143)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Client / Unit (386)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
- GitHub Check: release
- GitHub Check: Windows
- GitHub Check: release_ui_darwin
- GitHub Check: Linux
- GitHub Check: Android / Build
- GitHub Check: Client / Unit
- GitHub Check: Darwin
🔇 Additional comments (1)
client/ssh/server/userswitching_unix.go (1)
90-91: Linux branch delegation togetLinuxLoginCmdlooks correctDelegating the Linux-specific bits into
getLinuxLoginCmdkeepsgetLoginCmdfocused, and reusingaddrPort.Addr().String()preserves the previous host handling while enabling the new util‑linux logic.
|
Working on Ubuntu 25.10 on multiple devices. |
|
This is working perfectly for me. |
|
Interesting! For me, I get: root@vps:~# cat /proc/self/loginuid
4294967295
It doesn't matter whether I log in as root or my normal user, I still get the unset value. This is running on Ubuntu 25.10 with the binaries from this PR. I'm not sure if this expected behaviour? |
4 participants
Describe your changes
Issue ticket number and link
#4869
Stack
Checklist
Documentation
Select exactly one:
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
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.