[client] Support non-PTY no-command interactive SSH sessions #5093
Conversation
📝 WalkthroughWalkthroughThis PR injects loggers across SSH privilege-dropping and command-execution paths, renames and consolidates session handlers (handleCommand→handleExecution, handlePty→handlePtyLogin), updates NewPrivilegeDropper to accept a logger, and adjusts PTY/exec lifecycles (including proxy non-interactive session flow). Changes
Sequence Diagram(s)sequenceDiagram
participant ClientSession
participant Proxy as ProxyServer
participant BackendSession
participant Shell
ClientSession->>Proxy: open non-interactive session
Proxy->>BackendSession: create backend session
Proxy->>BackendSession: attach Stdin/Stdout/Stderr
Proxy->>BackendSession: start shell
alt shell started successfully
BackendSession->>Shell: shell runs
Shell-->>Proxy: exit (with code)
Proxy->>ClientSession: propagate exit code via handleProxyExitCode
else shell start failed
BackendSession-->>Proxy: error
Proxy->>ClientSession: log error and propagate exit code (via handleProxyExitCode)
end
Note over Proxy,ClientSession: also monitors ClientSession.Context().Done to stop backend
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🧰 Additional context used🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-11-14T13:05:31.729ZApplied to files:
📚 Learning: 2025-11-13T00:29:53.247ZApplied to files:
⏰ 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)
🔇 Additional comments (1)
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}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
client/ssh/server/server_config_test.go (1)
548-551: Test validates basic functionality but could be strengthened.The test verifies that non-PTY shell sessions don't error out, which validates the core fix. However, it doesn't confirm the shell is actually functional or produces expected output.
Consider verifying the session output to ensure the shell was properly initialized and can accept/return data, especially since the PR mentions platform-specific behavior (Windows runs without ConPTY, Unix uses
su -lwith fallback).client/ssh/server/command_execution_windows.go (2)
245-258: Inconsistent logger usage in fallback path.Line 249 uses the global
log.Debugfwhile theloggerparameter is available in scope. For consistency with the logger-threading pattern introduced in this PR, use the injected logger.Suggested fix
func (s *Server) prepareCommandEnv(logger *log.Entry, localUser *user.User, session ssh.Session) []string { username, domain := s.parseUsername(localUser.Username) userEnv, err := s.getUserEnvironment(logger, username, domain) if err != nil { - log.Debugf("failed to get user environment for %s\\%s, using fallback: %v", domain, username, err) + logger.Debugf("failed to get user environment for %s\\%s, using fallback: %v", domain, username, err) env := prepareUserEnv(localUser, getUserShell(localUser.Uid))
315-320: Consider refactoring to avoid empty Server instantiation.Creating an empty
&Server{}solely to callgetUserEnvironmentWithTokenis a code smell. The method doesn't appear to use any Server state. Consider makinggetUserEnvironmentWithTokena standalone function or passing the required dependencies directly.client/ssh/server/sftp_windows.go (1)
34-38: Consider passing a logger for consistency.Unlike the Unix SFTP subprocess case, this runs in the server process where session context is available. For consistency with other Windows privilege-dropping paths that thread loggers (e.g.,
executePtyCommandWithUserToken), consider accepting and passing a logger here.client/ssh/server/command_execution_unix.go (1)
140-149: Consider removing unused logger parameter or documenting intent.The logger parameter is accepted but unused (
_). If this is for API consistency or future use, consider adding a brief comment. Otherwise, if no logging is planned here, the parameter could potentially be removed from this platform-specific implementation.client/ssh/server/command_execution.go (1)
79-80: Log message may be misleading whenUsedFallbackis true buterris nil.When
privilegeResult.UsedFallbackis true anderris nil, the log message "su command failed" is inaccurate—the su command wasn't attempted or didn't fail; rather, a fallback was already used during privilege checking.Suggested improvement
if err != nil || privilegeResult.UsedFallback { - logger.Debugf("su command failed, falling back to executor: %v", err) + if err != nil { + logger.Debugf("su command failed, falling back to executor: %v", err) + } else { + logger.Debugf("using executor due to privilege fallback") + } cmd, cleanup, err := s.createExecutorCommand(logger, session, localUser, hasPty)client/ssh/server/executor_windows.go (3)
191-214: Consider extractingisLocalUseras a standalone function.Creating a
PrivilegeDropperinstance at line 194 solely to callisLocalUseris somewhat indirect sinceisLocalUserdoesn't use the logger field. This could be simplified.Optional refactor
+// isLocalUser determines if this is a local user vs domain user +func isLocalUser(domain string) bool { + hostname, err := os.Hostname() + if err != nil { + hostname = "localhost" + } + return domain == "" || domain == "." || + strings.EqualFold(domain, hostname) +} func generateS4UUserToken(logger *log.Entry, username, domain string) (windows.Handle, error) { userCpn := buildUserCpn(username, domain) - - pd := NewPrivilegeDropper(logger) - isDomainUser := !pd.isLocalUser(domain) + isDomainUser := !isLocalUser(domain)Then update the
PrivilegeDroppermethod to call the standalone function if needed.
481-492: Minor inconsistencies: typo and global logger usage.
- Line 481: Comment has typo "useVerifier" → should be "user"
- Line 484: Uses global
log.Debugfinstead of a logger parameter, inconsistent with the rest of the file's patternSuggested fix
-// userExists checks if the target useVerifier exists on the system -func userExists(fullUsername, username, domain string) error { +// userExists checks if the target user exists on the system +func (pd *PrivilegeDropper) userExists(fullUsername, username, domain string) error { if _, err := lookupUser(fullUsername); err != nil { - log.Debugf("User %s not found: %v", fullUsername, err) + pd.log().Debugf("User %s not found: %v", fullUsername, err) if domain != "" && domain != "." { _, err = lookupUser(username) }And update the call site in
createToken:- if err := userExists(fullUsername, username, domain); err != nil { + if err := pd.userExists(fullUsername, username, domain); err != nil {
242-247: Pre-existing global logger usage remains.For future consistency,
cleanupLsaConnectionandlookupPrincipalName(lines 292, 311, 316) still use the globallog.Debugf. Consider updating these in a follow-up if full logger consistency is desired.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
client/cmd/ssh_exec_unix.goclient/cmd/ssh_sftp_unix.goclient/ssh/proxy/proxy.goclient/ssh/server/command_execution.goclient/ssh/server/command_execution_js.goclient/ssh/server/command_execution_unix.goclient/ssh/server/command_execution_windows.goclient/ssh/server/executor_unix.goclient/ssh/server/executor_unix_test.goclient/ssh/server/executor_windows.goclient/ssh/server/server.goclient/ssh/server/server_config_test.goclient/ssh/server/session_handlers.goclient/ssh/server/session_handlers_js.goclient/ssh/server/sftp_windows.goclient/ssh/server/userswitching_unix.goclient/ssh/server/userswitching_windows.goclient/ssh/server/winpty/conpty.go
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: lixmal
Repo: netbirdio/netbird PR: 4015
File: client/ssh/server/server_test.go:396-406
Timestamp: 2025-11-14T11:11:50.812Z
Learning: On Windows, the NetBird SSH server only supports PowerShell as the shell (powershell.exe or pwsh.exe). cmd.exe and other shells are not supported due to parsing quirks and complexity.
📚 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/session_handlers_js.goclient/ssh/server/winpty/conpty.goclient/ssh/server/executor_unix_test.goclient/ssh/server/session_handlers.goclient/ssh/server/userswitching_windows.goclient/ssh/server/command_execution_unix.goclient/ssh/server/command_execution.goclient/ssh/server/executor_windows.goclient/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/winpty/conpty.goclient/ssh/proxy/proxy.goclient/ssh/server/sftp_windows.goclient/ssh/server/userswitching_unix.goclient/ssh/server/executor_unix_test.goclient/ssh/server/server_config_test.goclient/ssh/server/executor_unix.goclient/cmd/ssh_exec_unix.goclient/ssh/server/session_handlers.goclient/ssh/server/userswitching_windows.goclient/ssh/server/command_execution_js.goclient/ssh/server/command_execution_unix.goclient/ssh/server/command_execution.goclient/cmd/ssh_sftp_unix.goclient/ssh/server/executor_windows.goclient/ssh/server/command_execution_windows.go
📚 Learning: 2026-01-05T06:33:50.474Z
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.
Applied to files:
client/ssh/proxy/proxy.goclient/cmd/ssh_exec_unix.go
🧬 Code graph analysis (10)
client/ssh/server/session_handlers_js.go (2)
client/ssh/server/server.go (1)
Server(137-175)client/ssh/server/user_utils.go (1)
PrivilegeCheckResult(50-66)
client/ssh/server/userswitching_unix.go (2)
client/ssh/server/executor_unix.go (1)
NewPrivilegeDropper(43-45)client/ssh/server/executor_windows.go (1)
NewPrivilegeDropper(46-48)
client/ssh/server/executor_unix_test.go (2)
client/ssh/server/executor_unix.go (1)
NewPrivilegeDropper(43-45)client/ssh/server/executor_windows.go (1)
NewPrivilegeDropper(46-48)
client/cmd/ssh_exec_unix.go (2)
client/ssh/server/executor_unix.go (1)
NewPrivilegeDropper(43-45)client/ssh/server/executor_windows.go (1)
NewPrivilegeDropper(46-48)
client/ssh/server/userswitching_windows.go (4)
client/ssh/server/server.go (1)
Server(137-175)shared/management/status/error.go (1)
Errorf(70-75)client/ssh/server/executor_unix.go (1)
NewPrivilegeDropper(43-45)client/ssh/server/executor_windows.go (1)
NewPrivilegeDropper(46-48)
client/ssh/server/command_execution_js.go (1)
client/ssh/server/server.go (1)
Server(137-175)
client/ssh/server/command_execution_unix.go (3)
client/ssh/server/server.go (1)
Server(137-175)shared/management/status/error.go (1)
Errorf(70-75)client/ssh/server/user_utils.go (1)
PrivilegeCheckResult(50-66)
client/cmd/ssh_sftp_unix.go (2)
client/ssh/server/executor_unix.go (1)
NewPrivilegeDropper(43-45)client/ssh/server/executor_windows.go (1)
NewPrivilegeDropper(46-48)
client/ssh/server/executor_windows.go (1)
client/ssh/server/executor_unix.go (2)
NewPrivilegeDropper(43-45)PrivilegeDropper(38-40)
client/ssh/server/command_execution_windows.go (3)
client/ssh/server/executor_unix.go (1)
NewPrivilegeDropper(43-45)client/ssh/server/executor_windows.go (1)
NewPrivilegeDropper(46-48)client/ssh/server/winpty/conpty.go (1)
ExecutePtyWithUserToken(59-71)
⏰ 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). (13)
- GitHub Check: Client / Unit
- GitHub Check: iOS / Build
- GitHub Check: Android / Build
- GitHub Check: release_ui
- GitHub Check: release
- GitHub Check: Build Cache
- GitHub Check: release_ui_darwin
- GitHub Check: Client / Unit
- GitHub Check: Linux
- GitHub Check: JS / Lint
- GitHub Check: Client / Unit
- GitHub Check: Windows
- GitHub Check: Darwin
🔇 Additional comments (29)
client/ssh/server/server.go (1)
338-338: LGTM!The comment update correctly reflects the new behavior: with the added support for
ssh -T <host>(non-PTY interactive sessions), those sessions now get proper session handling and are tracked in thesessionsmap. Only-Nand port-forwarding-only connections remain as "authenticated connections without sessions."client/ssh/server/server_config_test.go (2)
486-487: LGTM!Test name and description accurately reflect the new purpose: verifying that non-PTY shell sessions (
ssh -T) work regardless of port forwarding configuration.
494-518: LGTM!Good test matrix covering all port forwarding configuration combinations. The simplified struct is appropriate since the expected outcome (success) is now uniform across all test cases.
client/ssh/proxy/proxy.go (1)
217-239: LGTM! Shell-driven lifecycle for non-interactive sessions.The implementation correctly:
- Attaches session I/O streams before starting the shell
- Uses a buffered channel to receive the wait result asynchronously
- Handles both client context cancellation and shell completion via select
- Propagates exit codes properly via
handleProxyExitCodeThis aligns with the PR objective of supporting
ssh -T <host>(interactive login without PTY).client/ssh/server/winpty/conpty.go (1)
59-71: LGTM! Context sourcing simplified appropriately.Removing the explicit
ctxparameter and deriving it fromsession.Context()internally is a cleaner API. This aligns with the learning that session-scoped contexts are appropriate for the SSH proxy lifecycle. The change ensures consistent context sourcing across the codebase.client/ssh/server/command_execution_windows.go (2)
270-281: LGTM! Handler correctly threads logger.The
handlePtyLoginfunction properly accepts and uses the logger parameter, maintaining consistent logging context throughout the PTY login flow.
300-342: LGTM! PTY execution with proper logger propagation.The function correctly:
- Accepts and uses the logger for tracing and debug output
- Creates a new PrivilegeDropper with the logger
- Handles token lifecycle with proper cleanup in defer
- Delegates to
winpty.ExecutePtyWithUserTokenwith the sessionclient/cmd/ssh_sftp_unix.go (1)
39-39: LGTM! Nil logger is acceptable for standalone SFTP subprocess.Passing
niltoNewPrivilegeDropperis appropriate here since this is a standalone subprocess invoked via command-line. The subprocess has its own logging vialog.Tracefand doesn't need a logger context threaded from a parent session.client/ssh/server/session_handlers_js.go (1)
12-22: LGTM! Consistent handler naming and signature.The rename from
handlePtytohandlePtyLoginaligns with the broader refactor across the PR. The logger parameter is properly utilized for error logging, maintaining API consistency with Unix and Windows implementations.client/ssh/server/executor_unix_test.go (5)
18-19: LGTM! Test adapted to new constructor signature.Passing
nilfor the logger in tests is appropriate since test assertions don't require structured logging context.
76-77: LGTM!
110-111: LGTM!
159-160: LGTM!
229-230: LGTM!client/ssh/server/session_handlers.go (1)
65-71: Clear and correct session dispatch logic.The two-path routing correctly handles:
- PTY interactive login (
ssh <host>) →handlePtyLogin- All other cases (commands,
-t,-T) →handleExecutionThis aligns well with the PR objective to support
ssh -T <host>for non-PTY interactive sessions.client/cmd/ssh_exec_unix.go (1)
55-55: LGTM -nillogger is appropriate here.This is a spawned child process for privilege dropping that communicates via stdout/stderr. Structured logging isn't needed in this context, and the executor handles failures by exiting with specific exit codes. Based on learnings, this aligns with the established pattern.
client/ssh/server/userswitching_unix.go (1)
184-195: Logger propagation looks correct.The logger parameter is properly threaded through to
NewPrivilegeDropper(logger)and used for debug logging. The cleanup function being a no-op on Unix is appropriate since there's no token handle to close (unlike Windows).client/ssh/server/userswitching_windows.go (3)
91-100: Logger propagation correctly implemented for Windows.The logger is properly threaded to
createUserSwitchCommandand debug logging uses the provided logger instance.
115-121: WindowsExecutorConfig construction simplified.The
Interactivefield has been removed as per PR objectives. Based on learnings, the PTY-related fields (Pty,PtyWidth,PtyHeight) are intentionally unused and will be implemented in a future update.
123-138: Cleanup function properly handles token lifecycle.The cleanup function correctly closes the Windows token handle and logs any errors via the provided logger. This ensures proper resource cleanup after process creation.
client/ssh/server/executor_unix.go (2)
38-53: Well-designed logger integration with safe fallback.The
log()helper method provides a clean pattern for optional logging - returns the injected logger if available, otherwise falls back to the standard logger. This ensures consistent logging behavior across all call sites.
217-233: Login shell execution enables non-PTY interactive sessions.The change to always use
-l(login shell) flag is correct for proper shell initialization. This is the key change that enablesssh -T <host>to work correctly - it spawns a login shell even without PTY allocation. The safe command logging viasafeLogCommand()properly protects sensitive command content from being exposed in logs.client/ssh/server/command_execution_js.go (1)
18-30: LGTM - Stub signatures correctly aligned with other platforms.The function signatures now accept
*log.Entryas the first parameter, maintaining consistency with Unix and Windows implementations. The logger is appropriately ignored in these stubs since the platform doesn't support SSH server command execution.client/ssh/server/command_execution_unix.go (2)
102-129: Good addition of username validation and debug logging.The username validation at the start of
createSuCommandis a good security practice, though I note thatcreateExecutorCommandinuserswitching_unix.goalso validates the username. This provides defense-in-depth but consider whether both validation points are necessary.The debug logging of the constructed
sucommand (line 124) is helpful for troubleshooting while keeping the command arguments visible (unlike the executor path which masks the command content).
162-178: Improved PTY login error handling.The
handlePtyLoginfunction now provides better user feedback by writing an error message tosession.Stderr()before exiting. This helps users understand why their session failed rather than just seeing a disconnection.client/ssh/server/command_execution.go (1)
15-57: LGTM!The function properly threads the logger through the command creation and execution paths. The PTY vs non-PTY branching is clean, and error handling appropriately logs and exits with meaningful messages.
client/ssh/server/executor_windows.go (3)
30-40: LGTM!The struct is clean. Based on learnings, the
Pty,PtyWidth, andPtyHeightfields are intentionally reserved for a future update.
42-56: LGTM!The
PrivilegeDropperimplementation with logger support is consistent with the Unix implementation pattern shown inexecutor_unix.go. Thelog()helper provides a safe fallback when no logger is configured.
575-578: LGTM!The stub correctly returns an error indicating
suis unavailable on Windows, with a signature matching the cross-platform interface.
|
| // NewPrivilegeDropper creates a new privilege dropper | ||
| func NewPrivilegeDropper() *PrivilegeDropper { | ||
| return &PrivilegeDropper{} | ||
| func NewPrivilegeDropper(logger *log.Entry) *PrivilegeDropper { |
There was a problem hiding this comment.
Maybe options pattern would be nicer instead of pass nil
Describe your changes
ssh -T <host>(interactive login without PTY):-t, but without ConPTYsu -l(with fallback to the executor / privilege dropper) instead ofloginsessionfield to some log messagesInteractivefieldIssue ticket number and link
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
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.