[client] ssh: fix #5345 by implementing PTY support in executor and removing setsid wrapper#5382
Open
pieterhollander wants to merge 5 commits intonetbirdio:mainfrom
Open
Conversation
Implement PTY support in the executor privilege-dropping path using setsid(2) + TIOCSCTTY to establish a controlling terminal natively, replacing the external setsid wrapper that was previously needed for util-linux login's vhangup() behavior. - Add prepareControllingTerminal() with setsid/TIOCSCTTY and graceful error handling for containers and non-PTY scenarios - Use syscall.Exec() in the PTY path so the shell directly replaces the executor process - Remove setsid -c wrapper from getLinuxLoginCmd; pty.StartWithSize() sets Setsid and Setctty on the child process - Remove detectUtilLinuxLogin() and loginIsUtilLinux field (now dead code) - Fix executor tests to use current UID/GID instead of hardcoded 1000 - Add TestPrivilegeDropper_CreateExecutorCommandWithPTY
|
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements PTY support in the executor and removes the dependency on the external setsid wrapper for handling util-linux login's vhangup() requirements.
Changes:
- Implemented PTY support directly in the executor using
syscall.Setsid()andTIOCSCTTYioctl - Removed util-linux login detection and setsid wrapper logic
- Updated tests to use current user's UID/GID instead of hardcoded values
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| client/ssh/server/userswitching_unix.go | Removed setsid wrapper logic and util-linux login handling, updated comments to reflect executor-based approach |
| client/ssh/server/server.go | Removed loginIsUtilLinux field and detection call |
| client/ssh/server/executor_unix_test.go | Updated tests to use current user credentials and added PTY test coverage |
| client/ssh/server/executor_unix.go | Implemented PTY support with controlling terminal setup and exec syscall |
| client/ssh/server/command_execution_windows.go | Removed detectUtilLinuxLogin stub function |
| client/ssh/server/command_execution_unix.go | Removed detectUtilLinuxLogin function and runtime import |
| client/ssh/server/command_execution_js.go | Removed detectUtilLinuxLogin stub function |
| client/cmd/ssh_exec_unix.go | Updated flag help text to remove "will fail" message |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+228
to
+248
| if config.PTY { | ||
| if err := pd.prepareControllingTerminal(); err != nil { | ||
| _, _ = fmt.Fprintf(os.Stderr, "PTY setup failed: %v\n", err) | ||
| os.Exit(ExitCodeShellExecFail) | ||
| } | ||
|
|
||
| argv := []string{"-" + filepath.Base(config.Shell)} | ||
| if config.Command == "" { | ||
| log.Tracef("executing login shell via exec: %s", config.Shell) | ||
| } else { | ||
| argv = append(argv, "-c", config.Command) | ||
| cmdParts := strings.Fields(config.Command) | ||
| safeCmd := safeLogCommand(cmdParts) | ||
| log.Tracef("executing %s -c %s via exec", config.Shell, safeCmd) | ||
| } | ||
|
|
||
| if err := syscall.Exec(config.Shell, argv, os.Environ()); err != nil { | ||
| log.Debugf("exec failed: %v", err) | ||
| os.Exit(ExitCodeShellExecFail) | ||
| } | ||
| } |
There was a problem hiding this comment.
The new PTY execution path (prepareControllingTerminal + syscall.Exec) isn’t covered by tests in this package. Since behavior differs significantly from the non-PTY path, consider adding an integration-style test (similar to TestPrivilegeDropper_ActualPrivilegeDrop’s child-process pattern) that runs ssh exec --pty under a PTY and asserts it provides an interactive shell and preserves exit codes.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Contributor
📝 WalkthroughWalkthroughRemoves util‑linux login detection and associated wrapping across SSH server files; adds PTY-support path in the Unix privilege‑drop executor (preparing a controlling terminal and exec’ing the shell); updates tests to use dynamic UID/GID and tightens CLI --pty help text. Changes
Sequence DiagramsequenceDiagram
participant Client
participant ServerExec as "Unix Executor"
participant PTY as "PTY/Terminal"
participant Kernel as "OS Syscall"
Client->>ServerExec: Request command exec (with PTY, privilege drop)
ServerExec->>ServerExec: Validate privilege transition
alt PTY requested
ServerExec->>PTY: prepareControllingTerminal()
PTY->>Kernel: setsid()
alt EPERM (already session leader)
Kernel-->>PTY: EPERM
PTY->>Kernel: getsid()
Kernel-->>PTY: session id
else Success
Kernel-->>PTY: session created
end
PTY->>Kernel: ioctl(TIOCSCTTY)
alt Success
Kernel-->>PTY: controlling tty set
else ENOTTY/EINVAL
Kernel-->>PTY: error (log and continue)
end
PTY-->>ServerExec: terminal ready (or continued)
end
ServerExec->>Kernel: syscall.Exec(target shell/command)
Kernel-->>Client: process replaced / command started
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
🧹 Nitpick comments (3)
client/ssh/server/executor_unix_test.go (2)
78-81: Minor duplication: UID/GID setup repeated across three tests.
currentUID/currentGID(and the string conversions) appear identically in every test. A small shared helper or table-driven approach would reduce the noise.♻️ Example helper extraction
+func currentUIDGID(t *testing.T) (uid, gid uint32, uidStr, gidStr string) { + t.Helper() + uid = uint32(os.Geteuid()) + gid = uint32(os.Getegid()) + uidStr = strconv.FormatUint(uint64(uid), 10) + gidStr = strconv.FormatUint(uint64(gid), 10) + return +}Then in each test:
- currentUID := uint32(os.Geteuid()) - currentGID := uint32(os.Getegid()) - uidStr := strconv.FormatUint(uint64(currentUID), 10) - gidStr := strconv.FormatUint(uint64(currentGID), 10) + currentUID, currentGID, uidStr, gidStr := currentUIDGID(t)Also applies to: 115-116, 138-139
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/ssh/server/executor_unix_test.go` around lines 78 - 81, The tests in executor_unix_test.go duplicate the currentUID/currentGID retrieval and string conversion logic across multiple test cases (variables currentUID/currentGID and their strconv.Itoa conversions); extract that into a small helper (e.g., getCurrentUIDGID() or a test helper that returns uid,gid and uidStr,gidStr) and call it from each test (or convert the tests to a table-driven loop that uses the helper) so the duplicated setup is centralized and reused.
136-157:--cmdabsence doesn't exercise PTY-specific behaviour; consider a second sub-case.
Command: ""already implies--cmdwill be absent — theassert.NotContains(t, cmd.Args, "--cmd")assertion passes trivially without any PTY logic involved.
A more meaningful coverage addition would be a case wherePTY: trueandCommand: "ls -la"to verify that--ptyis still emitted and--cmdis also present (i.e., both flags coexist when a command is supplied in PTY mode).🧪 Suggested additional sub-case
assert.Contains(t, cmd.Args, "--pty") assert.NotContains(t, cmd.Args, "--cmd") + + // PTY with an explicit command: both --pty and --cmd should appear. + configWithCmd := ExecutorConfig{ + UID: currentUID, + GID: currentGID, + Groups: []uint32{currentGID}, + WorkingDir: "/home/testuser", + Shell: "/bin/bash", + Command: "ls -la", + PTY: true, + } + cmdWithCmd, err := pd.CreateExecutorCommand(context.Background(), configWithCmd) + require.NoError(t, err) + require.NotNil(t, cmdWithCmd) + assert.Contains(t, cmdWithCmd.Args, "--pty") + assert.Contains(t, cmdWithCmd.Args, "--cmd")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/ssh/server/executor_unix_test.go` around lines 136 - 157, Add a new sub-case in the existing test in executor_unix_test.go that sets PTY: true and Command: "ls -la" (instead of Command: ""), call the same code path that builds the cmd (referencing the local variable cmd and the existing test helper/assertions), and assert that cmd.Args contains "--pty", contains "--cmd", and that the provided command string (or its split tokens) appears after the "--cmd" flag; this verifies PTY-specific behavior when a command is supplied rather than relying on the trivial empty-Command case.client/ssh/server/executor_unix.go (1)
284-319: Robust controlling-terminal setup with good container tolerance.The
setsid+TIOCSCTTYpattern is the standard way to establish a controlling terminal, and the EPERM-on-setsid handling (verifying we're already a session leader before continuing) is a nice touch.One observation on lines 308-311: EPERM from
TIOCSCTTYis treated as a benign container/seccomp condition, but EPERM can also mean the terminal is already a controlling terminal of a different session. In the SSH privilege-drop context this is very unlikely (the PTY is freshly allocated for this session), so this trade-off seems acceptable. If you want to tighten it up, you could log the EPERM at a slightly higher level (Info vs Debug) to make debugging easier if it does mask a real issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/ssh/server/executor_unix.go` around lines 284 - 319, In prepareControllingTerminal, when unix.IoctlSetInt(stdinFD, unix.TIOCSCTTY, 0) returns EPERM the code currently logs with pd.log().Debugf; change that call to a higher-severity log (e.g., pd.log().Infof or pd.log().Warnf) and keep the same message and error payload so container/seccomp denials remain tolerated but EPERM is more visible for debugging; ensure you only change the log level in the EPERM branch that follows unix.IoctlSetInt and leave other error-handling branches untouched.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@client/ssh/server/executor_unix_test.go`:
- Around line 78-81: The tests in executor_unix_test.go duplicate the
currentUID/currentGID retrieval and string conversion logic across multiple test
cases (variables currentUID/currentGID and their strconv.Itoa conversions);
extract that into a small helper (e.g., getCurrentUIDGID() or a test helper that
returns uid,gid and uidStr,gidStr) and call it from each test (or convert the
tests to a table-driven loop that uses the helper) so the duplicated setup is
centralized and reused.
- Around line 136-157: Add a new sub-case in the existing test in
executor_unix_test.go that sets PTY: true and Command: "ls -la" (instead of
Command: ""), call the same code path that builds the cmd (referencing the local
variable cmd and the existing test helper/assertions), and assert that cmd.Args
contains "--pty", contains "--cmd", and that the provided command string (or its
split tokens) appears after the "--cmd" flag; this verifies PTY-specific
behavior when a command is supplied rather than relying on the trivial
empty-Command case.
In `@client/ssh/server/executor_unix.go`:
- Around line 284-319: In prepareControllingTerminal, when
unix.IoctlSetInt(stdinFD, unix.TIOCSCTTY, 0) returns EPERM the code currently
logs with pd.log().Debugf; change that call to a higher-severity log (e.g.,
pd.log().Infof or pd.log().Warnf) and keep the same message and error payload so
container/seccomp denials remain tolerated but EPERM is more visible for
debugging; ensure you only change the log level in the EPERM branch that follows
unix.IoctlSetInt and leave other error-handling branches untouched.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
pieterhollander
changed the title
pieterhollander
changed the title
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.
I tried to implement the existing TODO and fix #5345 myself.
I tested the PR on a few systems, including an Incus container.
The change does not change anything for end-users, but fixes Netbird SSH being unusable on containers.
Summary by CodeRabbit
New Features
Documentation
Chores
Tests