[client] Add port forwarding to ssh proxy#5031
Conversation
📝 WalkthroughWalkthroughAdds destination-port validation in the CLI; exposes port-forwards in proto/status; adds bidirectional I/O helpers; refactors proxy to reuse a backend client and bridge forwarded channels; overhauls server session/connection state with privilege checks and per-connection port-forward tracking. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as SSH CLI
participant PROXY as SSH Proxy
participant SVR as SSH Server
participant BACK as Backend SSH
participant STATE as Server State
CLI->>SVR: Connect & authenticate (JWT)
SVR->>STATE: record connection, pending JWT
CLI->>PROXY: Request port-forward (-L / -R)
note right of SVR `#f9f2d0`: CLI destination-port validated\n(validateDestinationPort)
SVR->>SVR: validateDestinationPort(addr)
alt invalid port
SVR-->>CLI: formatted validation error
else valid port
SVR->>STATE: checkPrivilegedPortAccess(port)
alt privileged denied
STATE-->>CLI: deny request
else allowed
SVR->>PROXY: forward request
PROXY->>BACK: getOrCreateBackendClient(jwtToken)
BACK-->>PROXY: backend channel established
PROXY->>CLI: bridge channel (BidirectionalCopy / BidirectionalCopyWithContext)
SVR->>STATE: addConnectionPortForward(connKey, forward)
end
end
CLI->>SVR: Close connection
SVR->>STATE: handleConnectionClose(connKey)
STATE-->>SVR: cleanup & logs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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 (6)
client/ssh/common.go (2)
197-215: Consider logging or propagating copy errors.Both directions silently discard
io.Copyerrors, which could hide connection failures, partial transfers, or write errors. While this simplifies the API, it makes debugging port-forwarding issues more difficult.Recommended enhancement
Consider at least logging errors for operational visibility:
func BidirectionalCopy(rw1, rw2 io.ReadWriter) { done := make(chan struct{}, 2) go func() { - _, _ = io.Copy(rw2, rw1) + if _, err := io.Copy(rw2, rw1); err != nil { + log.Debugf("bidirectional copy (rw1->rw2) error: %v", err) + } done <- struct{}{} }() go func() { - _, _ = io.Copy(rw1, rw2) + if _, err := io.Copy(rw1, rw2); err != nil { + log.Debugf("bidirectional copy (rw2->rw1) error: %v", err) + } done <- struct{}{} }() <-done <-done }
217-244: Same error suppression concern applies here.Similar to
BidirectionalCopy, this function discardsio.Copyerrors which limits debugging capability for port-forwarding connections. Additionally, connection close errors are also suppressed.Recommended enhancement
func BidirectionalCopyWithContext(ctx context.Context, conn1, conn2 io.ReadWriteCloser) { done := make(chan struct{}, 2) go func() { - _, _ = io.Copy(conn2, conn1) + if _, err := io.Copy(conn2, conn1); err != nil { + log.Debugf("bidirectional copy with context (conn1->conn2) error: %v", err) + } done <- struct{}{} }() go func() { - _, _ = io.Copy(conn1, conn2) + if _, err := io.Copy(conn1, conn2); err != nil { + log.Debugf("bidirectional copy with context (conn2->conn1) error: %v", err) + } done <- struct{}{} }() select { case <-ctx.Done(): case <-done: select { case <-ctx.Done(): case <-done: } } - _ = conn1.Close() - _ = conn2.Close() + if err := conn1.Close(); err != nil { + log.Debugf("close conn1: %v", err) + } + if err := conn2.Close(); err != nil { + log.Debugf("close conn2: %v", err) + } }client/cmd/ssh.go (1)
637-641: Good validation and improved logging.The destination port validation correctly applies to the target addresses (where we connect) and not the bind addresses (where we listen). This properly allows port 0 for bind addresses like
ssh -R 0:localhost:80while rejecting invalid destination ports. The switch from print to debug logging is also an improvement.Optional: Add clarifying comment
Consider adding a comment to clarify the validation scope:
+// Validate destination port - bind addresses are not validated, +// allowing port 0 for OS auto-selection if err := validateDestinationPort(remoteAddr); err != nil { return fmt.Errorf("invalid remote address: %w", err) }Also applies to: 659-663
client/ssh/server/server_config_test.go (1)
553-567: Clarify test assertion for allowed sessions.When
tt.expectAllowedis true, the test only logs output and error but doesn't make any assertions. This means test cases for allowed sessions will always pass regardless of actual behavior.Consider adding explicit assertions for the allowed case:
🔎 Proposed fix
output, err := client.ExecuteCommand(ctx, "") if tt.expectAllowed { // When allowed, the session stays open until cancelled // ExecuteCommand with empty command should return without error - // (though output may be empty since we're in port-forward-only mode) - t.Logf("Output: %q, Error: %v", output, err) + // The session should succeed (no "port forwarding is disabled" message) + if err != nil { + assert.NotContains(t, err.Error(), "port forwarding is disabled", + "Should not get port forwarding disabled message when enabled") + } + assert.NotContains(t, string(output), "port forwarding is disabled", + "Should not get port forwarding disabled message when enabled") } else {client/ssh/server/session_handlers.go (1)
104-114: Consider direct session key lookup for updateSessionType.The current implementation iterates through all sessions to find the matching one. While the sessions map is typically small, you could store the sessionKey in the session context to enable O(1) lookup:
func (s *Server) updateSessionType(session ssh.Session, sessionType string) { s.mu.Lock() defer s.mu.Unlock() - for _, state := range s.sessions { - if state.session == session { - state.sessionType = sessionType - return - } + // Alternative: if sessionKey were stored in context during registerSession, + // this could be O(1) lookup instead of O(n) iteration. + // For now, the iteration is acceptable given typical session counts. + for _, state := range s.sessions { + if state.session == session { + state.sessionType = sessionType + return + } } }client/ssh/proxy/proxy.go (1)
209-222: Unused backend session in non-interactive handler.The
serverSessionis created but not used for any I/O operations. Since non-interactive sessions are used for port forwarding where channels handle the data, this session appears unnecessary. Consider removing it or clarifying why it's needed:🔎 Proposed simplification
func (p *SSHProxy) handleNonInteractiveSession(session ssh.Session, sshClient *cryptossh.Client) { - serverSession, err := sshClient.NewSession() - if err != nil { - _, _ = fmt.Fprintf(p.stderr, "create server session: %v\n", err) - return - } - defer func() { _ = serverSession.Close() }() - + // Non-interactive sessions (ssh -N/-T) are used for port forwarding. + // Port forwarding operates through channel handlers, not session I/O. + // We just wait for the client to disconnect. <-session.Context().Done() if err := session.Exit(0); err != nil { log.Debugf("session exit: %v", err) } }If the backend session is intentionally kept open to maintain the connection state on the server side, please add a comment explaining this.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
client/proto/daemon.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (12)
client/cmd/ssh.goclient/proto/daemon.protoclient/server/server.goclient/ssh/client/client.goclient/ssh/common.goclient/ssh/proxy/proxy.goclient/ssh/server/port_forwarding.goclient/ssh/server/server.goclient/ssh/server/server_config_test.goclient/ssh/server/session_handlers.goclient/ssh/server/sftp.goclient/status/status.go
🧰 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/proxy/proxy.go
🧬 Code graph analysis (3)
client/ssh/client/client.go (1)
client/ssh/common.go (1)
BidirectionalCopy(200-215)
client/ssh/server/server_config_test.go (3)
client/ssh/server/server.go (2)
Config(185-191)New(204-218)client/ssh/server/user_utils.go (1)
PrivilegeCheckResult(50-66)client/ssh/server/test.go (1)
StartTestServer(12-45)
client/ssh/server/session_handlers.go (1)
client/ssh/server/server.go (2)
Server(137-175)New(204-218)
⏰ 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). (15)
- GitHub Check: Require docs PR URL or explicit "not needed"
- GitHub Check: release_ui
- GitHub Check: FreeBSD Port / Build & Test
- GitHub Check: release
- GitHub Check: release_ui_darwin
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
- GitHub Check: Build Cache
- GitHub Check: Android / Build
- GitHub Check: iOS / Build
- GitHub Check: Client / Unit
- GitHub Check: Linux
- GitHub Check: Client / Unit
- GitHub Check: Darwin
- GitHub Check: Windows
🔇 Additional comments (29)
client/proto/daemon.proto (1)
375-375: LGTM! Clean protobuf schema extension.The addition of the
portForwardsfield toSSHSessionInfofollows protobuf best practices with sequential field numbering and is consistent with the rest of the message structure.client/server/server.go (1)
1125-1133: LGTM! Proper integration of port forwards into SSH server state.The addition of
PortForwardsto the session info is consistent with other fields and correctly wires the port-forwarding data from the session state to the protobuf response.client/status/status.go (1)
84-90: LGTM! Clean struct extension for port forwards.The
PortForwardsfield is properly added with appropriate JSON/YAML tags andomitemptyfor clean serialization.client/cmd/ssh.go (1)
674-701: Well-implemented validation with clear error handling.The
validateDestinationPortfunction correctly:
- Exempts Unix socket paths from port validation
- Rejects port 0 for destination addresses (while allowing it for bind addresses via non-validation)
- Validates the standard port range (1-65535)
- Provides clear, actionable error messages
The logic properly supports the PR's port 0 feature for bind addresses while preventing invalid destination ports.
client/ssh/server/sftp.go (2)
21-31: LGTM! Clean session lifecycle tracking with structured logging.The session registration, JWT association, and per-session logger setup follow a consistent pattern with the main session handler. The deferred cleanup and logging provide good observability.
38-68: Consistent structured logging throughout the SFTP handler.All log statements now use the per-session logger with appropriate log levels (Debug for routine operations, Warn for access denials, Error for failures).
client/ssh/server/server_config_test.go (1)
227-308: Comprehensive privileged port access tests.Good coverage of:
- Non-root users denied binding to privileged ports (80, 443) for remote/tcpip-forward
- Port 0 (auto-select) allowed for non-root users
- Root users allowed to bind privileged ports
- Local port forwarding not restricted by privileged port checks (connects outbound, doesn't bind)
The test structure with table-driven tests is clean and readable.
client/ssh/client/client.go (3)
559-573: Clean error handling and simplified data transfer.Good improvements:
- Using typed
ssh.OpenChannelErrorwithReasoncheck provides structured error handling instead of string matching.nbssh.BidirectionalCopyconsolidates the bidirectional copy pattern, reducing code duplication.
628-628: Simplified error message.The simplified error message
"remote port forwarding denied by server"is user-friendly. The detailed reason is now logged server-side which is appropriate.
687-687: Consistent use of BidirectionalCopy.Both local and remote forward handlers now use the same bidirectional copy helper, improving consistency and maintainability.
client/ssh/server/port_forwarding.go (4)
1-7: Clear security documentation.The package documentation clearly explains the security model: port forwarding runs without privilege separation (unlike shell execution), and the attack surface is limited to well-tested
io.Copycode. The privileged port restriction is documented.
118-145: Well-designed privileged port access check.The implementation correctly:
- Skips check on Windows (no privileged port restriction)
- Only applies to bind operations (
remote,tcpip-forward), not outboundlocalconnections- Allows port 0 (auto-select, will get port >= 1024)
- Allows privileged users to bind privileged ports
- Provides clear error messages with username and port
239-265: Robust logger fallback chain.The logger construction follows a sensible priority:
- Session state (if session exists)
- Connection state (for port-forward-only connections)
- Fallback to context user/address
This ensures logging works correctly across all connection states.
366-366: Context-aware bidirectional copy.Using
BidirectionalCopyWithContextensures connections are properly cleaned up when the SSH context is cancelled, preventing resource leaks.client/ssh/server/session_handlers.go (3)
15-33: Clean JWT username association.The function properly:
- Uses the auth key to look up pending JWT username
- Associates it with the session state for logging and status
- Cleans up the pending entry to prevent memory leaks and reuse
81-102: Well-implemented non-interactive session handler.The handler correctly:
- Updates session type for status display
- Validates port forwarding is enabled before allowing the session
- Provides clear user feedback when denied
- Blocks until context cancellation (allowing port forwarding to work)
- Exits cleanly with status 0
This enables
ssh -Nandssh -Tuse cases for port forwarding.
116-139: Clean session registration with readable keys.Using a short hash of the session ID creates human-readable session keys for logging (e.g.,
user@192.168.1.1:54321-a1b2c3d4) while maintaining uniqueness.client/ssh/server/server.go (6)
110-135: Well-structured state types for session and connection tracking.The separation of concerns is clear:
connState: tracks authenticated connections (username, address, port forwards, JWT username)sessionState: tracks active SSH sessions (session, type, JWT username)authKey/connKey: type-safe keys prevent mixing up different map lookups
310-344: Comprehensive status reporting.The updated
GetStatuscorrectly handles:
- Active sessions with commands/shells
- Authenticated connections without sessions (port-forwarding only)
- Deduplication via
reportedAddrsto avoid showing connections twice- Dynamic command label (
cmdPortForwardingvscmdNonInteractive) based on active port forwards
346-372: Smart session info building with port forward awareness.The logic to upgrade
cmdInteractiveShellorcmdNonInteractivetocmdPortForwardingwhen forwards exist provides accurate status display showing what the session is actually being used for.
626-659: Safe port forward tracking with duplicate prevention.
addConnectionPortForward:
- Uses
slices.Containsto prevent duplicate entries- Creates connection state for non-JWT auth paths
- Retrieves JWT username from pending auth if available
removeConnectionPortForward:
- Uses
slices.DeleteFuncfor clean removal- Safe no-op if connection doesn't exist
661-695: Connection lifecycle tracking with trackedConn wrapper.The
trackedConnpattern is a clean way to detect connection close:
sync.Onceensures cleanup runs exactly once even if Close() is called multiple timeshandleConnectionCloselogs port forwarding connection closure for observability- Connection state is cleaned up to prevent memory leaks
863-867: Port forward tracking for local (direct-tcpip) forwards.The handler now tracks local port forwards in connection state, ensuring they appear in status output. The
-L host:portformat is consistent with SSH command-line syntax.client/ssh/proxy/proxy.go (6)
46-54: Thread-safe backend client management.The design is sound:
muprotectsbackendClientfor concurrent accessjwtTokenis set once before any handlers run (safe without sync)forwardedChannelsOnceensures forwarded channel handling starts only once
74-90: Clean backend client cleanup on Close.The Close method properly:
- Acquires lock before accessing backendClient
- Sets backendClient to nil to prevent use-after-close
- Logs errors at debug level (non-critical)
- Closes the gRPC connection
292-337: Well-implemented local port forwarding proxy.The handler correctly:
- Parses the direct-tcpip payload
- Obtains or creates the backend client
- Opens a corresponding channel to the backend
- Properly propagates backend rejection reasons to the client
- Uses context-aware bidirectional copy for clean shutdown
439-476: Correct remote port forwarding proxy implementation.The handler properly:
- Parses the tcpip-forward request payload
- Forwards the request to the backend server
- Handles port 0 response (extracts allocated port from payload)
- Uses
forwardedChannelsOnceto start the channel handler exactly once- Returns the backend's response to the client
505-532: Thread-safe backend client singleton.
getOrCreateBackendClientimplements a proper lazy-initialized singleton:
- Acquires write lock
- Double-checks if client exists
- Creates and stores client if needed
- Returns existing or new client
getBackendClientprovides read-only access for cancel handlers.
534-576: Clean forwarded channel bridging.The forwarded-tcpip channel handling correctly:
- Retrieves the client's server connection from context
- Listens for backend forwarded channels
- Opens corresponding channels to the client
- Bridges data bidirectionally with context-aware cleanup
This completes the remote port forwarding proxy chain.
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @client/ssh/server/server_config_test.go:
- Around line 556-565: Tests in the tt.expectAllowed branch only log output and
error without asserting success; update the success branch (where
tt.expectAllowed is true and ExecuteCommand is called) to assert that err is nil
(e.g., assert.NoError(t, err) or assert.Nil) and that the error message does not
contain "port forwarding is disabled" (and optionally assert expected
characteristics of output, e.g., non-failure or specific content), using the
existing output and err variables and the ExecuteCommand call to verify the
session was accepted rather than silently logged.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/ssh/server/server_config_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
client/ssh/server/server_config_test.go (1)
client/ssh/server/user_utils.go (1)
PrivilegeCheckResult(50-66)
⏰ 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). (23)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Signal / Unit (amd64)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Signal / Unit (386)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Client / Unit (386)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client / Unit
- GitHub Check: release
- GitHub Check: FreeBSD Port / Build & Test
- GitHub Check: release_ui
- GitHub Check: release_ui_darwin
- GitHub Check: Client / Unit
- GitHub Check: Darwin
- GitHub Check: Windows
- GitHub Check: Linux
- GitHub Check: Client / Unit
- GitHub Check: iOS / Build
- GitHub Check: Android / Build
- GitHub Check: JS / Lint
🔇 Additional comments (1)
client/ssh/server/server_config_test.go (1)
293-296: > Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 3
Fix all issues with AI Agents 🤖
In @client/ssh/proxy/proxy.go:
- Around line 142-156: The backend client reuse in getOrCreateBackendClient
breaks user isolation: modify SSHProxy to track which user the current
backendClient belongs to (e.g., add a backendClientUser field) and change
getOrCreateBackendClient to compare the incoming session.User() to that tracked
user; if different, close and recreate the backend client for the new user (or
return an error if you intend to enforce single-user proxies). Update places
using backendClient to set backendClientUser when creating a client and ensure
backendClient is not reused across different users; alternatively, explicitly
document and enforce a single-user policy in SSHProxy to avoid accidental
multi-user reuse.
♻️ Duplicate comments (1)
client/ssh/server/server_config_test.go (1)
479-568: Port-forwarding-only session tests address the intended behavior.The test validates that sessions without PTY/command are allowed when port forwarding is enabled. The assertions for the
expectAllowedcase (lines 559-561) now includeassert.NoErrorandassert.NotContains, addressing the concern from the previous review about missing success case assertions.
🧹 Nitpick comments (3)
client/ssh/common.go (1)
221-223: Consider addingnet.ErrClosedto expected copy errors.When connections are closed to terminate bidirectional copy,
net.ErrClosedis commonly returned alongsideio.EOF. Adding it would reduce noise in debug logs.🔎 Suggested improvement
func isExpectedCopyError(err error) bool { - return errors.Is(err, io.EOF) || errors.Is(err, context.Canceled) + return errors.Is(err, io.EOF) || errors.Is(err, context.Canceled) || errors.Is(err, net.ErrClosed) }client/ssh/proxy/proxy.go (2)
49-51: Clarify the concurrency safety claim.The comment states "concurrent access is safe without additional synchronization," but what makes this safe is the happens-before relationship:
jwtTokenis written at line 105 beforeHandleConnis called at line 137, which means all handler invocations see the write. Consider rephrasing to: "jwtToken is set once in runProxySSHServer before HandleConn, establishing a happens-before relationship with all handler reads."
74-90: The code is reasonably safe due to its architecture:Connect()blocks onsshServer.HandleConn(), which prevents new handlers from being spawned after the SSH connection closes, andClose()is called only afterHandleConn()returns. All handlers usesession.Context()orsshCtx.Done()for lifecycle management—notably,handleForwardedChannelsexplicitly exits onsshCtx.Done()(line 550). The theoretical race condition you describe (new backend client creation afterClose()nullsp.backendClient) is unlikely to occur because handlers cannot be active after the SSH connection closes.However,
handleForwardedChannelsis spawned as an async goroutine and could theoretically be in-flight when the connection ends. While it has proper context checks, adding an explicit shutdown flag (similar to the pattern inrelay/testec2/tun/proxy.go) would make the code more defensive and eliminate this edge case entirely.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
client/ssh/auth/auth.goclient/ssh/auth/auth_test.goclient/ssh/client/client.goclient/ssh/common.goclient/ssh/proxy/proxy.goclient/ssh/server/port_forwarding.goclient/ssh/server/server.goclient/ssh/server/server_config_test.go
🧰 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/proxy/proxy.go
🧬 Code graph analysis (4)
client/ssh/client/client.go (2)
client/ssh/common.go (1)
BidirectionalCopy(200-219)shared/management/status/error.go (1)
Errorf(70-75)
client/ssh/auth/auth.go (2)
shared/management/status/error.go (1)
Errorf(70-75)shared/sshauth/userhash.go (1)
HashUserID(14-23)
client/ssh/auth/auth_test.go (2)
shared/management/status/error.go (1)
Error(54-57)client/ssh/auth/auth.go (2)
ErrNoMachineUserMapping(23-23)ErrUserNotMappedToOSUser(24-24)
client/ssh/server/port_forwarding.go (3)
client/ssh/server/server.go (1)
Server(137-175)client/ssh/server/user_utils.go (2)
PrivilegeCheckRequest(42-47)PrivilegeCheckResult(50-66)client/ssh/common.go (1)
BidirectionalCopyWithContext(228-256)
⏰ 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). (23)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Client / Unit (386)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Signal / Unit (386)
- GitHub Check: Signal / Unit (amd64)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Windows
- GitHub Check: Darwin
- GitHub Check: Linux
- GitHub Check: Client / Unit
- GitHub Check: release_ui_darwin
- GitHub Check: release_ui
- GitHub Check: release
- GitHub Check: FreeBSD Port / Build & Test
- GitHub Check: Client / Unit
- GitHub Check: iOS / Build
- GitHub Check: Android / Build
- GitHub Check: JS / Lint
- GitHub Check: Client / Unit
🔇 Additional comments (31)
client/ssh/auth/auth_test.go (1)
27-27: LGTM! Test updates correctly adapt to newAuthorizesignature.All test calls properly discard the new success message return value using the blank identifier, while continuing to assert on error values and types. The test coverage remains comprehensive for all authorization scenarios including wildcards, machine user mappings, and concurrent access.
Also applies to: 48-56, 83-97, 124-141, 161-161, 181-181, 214-219, 239-246, 270-281, 304-304, 352-364, 396-396, 429-445, 465-471, 495-505, 529-553, 581-609
client/ssh/auth/auth.go (2)
101-123: LGTM! Method signature change provides good observability.The updated
Authorizemethod now returns a descriptive success message alongside the error, enabling better audit logging of how authorization was granted (wildcard vs specific mapping). Error messages include helpful context (OS user, JWT user, index) for debugging while preserving error chains with%wforerrors.Is()checks.
128-149: LGTM! Clear authorization path messages.The
checkMachineUserMappingmethod provides distinct success messages for wildcard grants vs explicit mappings, which aids in auditing and debugging authorization decisions.client/ssh/common.go (2)
197-219: LGTM with a note on connection lifecycle.The implementation correctly waits for both copy directions to complete. The caller is responsible for closing connections to unblock the other direction when one side finishes - this is documented and appropriate for the use cases in this PR.
225-256: LGTM! Context-aware bidirectional copy with proper cleanup.The nested select pattern correctly handles:
- Context cancellation - immediately proceeds to cleanup
- One direction completing - waits for either context or second direction
- Both directions completing - proceeds to cleanup
Closing both connections on return ensures resources are released.
client/ssh/server/server.go (9)
110-116: LGTM! Well-structured connection state type.The
connStatetype cleanly captures the connection metadata needed for status display and port forward tracking.
130-135: LGTM! Clear session state structure.The
sessionStatetype properly tracks the session reference, type, and JWT username for status reporting.
137-175: LGTM! Server struct properly initialized with new state maps.The new tracking maps (
sessions,pendingAuthJWT,connections) are correctly initialized inNew()and properly cleared inStop().
310-344: LGTM! Status reporting includes all connection types.
GetStatuscorrectly reports both active sessions and authenticated connections without sessions (port-forwarding-only or idle). The use ofreportedAddrsmap prevents duplicate reporting.
346-372: LGTM! Session info builder with port forward context.
buildSessionInfoproperly merges session data with connection state to include port forwards and updates the command display to indicate port forwarding activity.
631-664: LGTM! Port forward tracking with proper synchronization.
addConnectionPortForwardcorrectly handles both cases: updating existing connection state and creating new state for non-JWT auth paths.removeConnectionPortForwardproperly cleans up usingslices.DeleteFunc.
666-700: LGTM! Connection close tracking with sync.Once safety.The
trackedConnwrapper properly usessync.Onceto ensurehandleConnectionCloseis called exactly once, preventing double-cleanup. TheconnLoggerhelper provides consistent logging context.
763-768: LGTM! Connection validator wraps with tracking.The
connectionValidatornow returns atrackedConnto enable automatic cleanup when connections close.
868-871: LGTM! Local port forwarding properly tracked.Port forwards are now recorded via
addConnectionPortForwardfor status visibility.client/ssh/client/client.go (3)
549-574: LGTM! Improved local port forward handling.The structured error check using
ssh.OpenChannelErrorwithReason == ssh.Prohibitedis more robust than string matching. Usingnbssh.BidirectionalCopyconsolidates the bidirectional transfer logic.
621-631: LGTM! Concise error messaging.The simplified error message "remote port forwarding denied by server" is clear and avoids exposing implementation details.
663-688: LGTM! Consistent use of bidirectional copy helper.Remote forward channel handling now uses the centralized
BidirectionalCopyhelper, matching the local forward implementation.client/ssh/server/server_config_test.go (1)
227-308: LGTM! Comprehensive privileged port access tests.The test cases cover the key scenarios:
- Non-root users denied for privileged ports (< 1024)
- Non-root users allowed for unprivileged ports and port 0
- Root users allowed for privileged ports
- Local forwards exempt from bind restrictions
client/ssh/server/port_forwarding.go (5)
1-7: Excellent security documentation in package comment.The comment clearly explains the security model: port forwarding runs without privilege separation (unlike shell execution), with risk mitigated by using well-tested
io.Copyand enforcing privileged port restrictions.
118-145: LGTM! Correct privileged port enforcement.The implementation properly:
- Skips the check on Windows (no privileged port concept)
- Only applies to bind operations (remote/tcpip-forward), not local forwards
- Allows port 0 (OS auto-selects >= 1024)
- Allows privileged users (root/Administrator) to bind any port
239-265: LGTM! Request logger with rich context.The
getRequestLoggermethod properly builds logging context by checking session state first, then connection state, with appropriate fallback. JWT username is included when available for audit trails.
341-367: LGTM! Remote forward connection handling with context-aware cleanup.Using
BidirectionalCopyWithContextensures both the network connection and SSH channel are closed when the context is cancelled or data transfer completes.
186-196: Forward key format is consistent between setup and cancellation.Both
setupDirectForward(line 321) andcancelTcpipForwardHandler(line 186) use identicalforwardKeyconstruction:fmt.Sprintf("%s:%d", payload.Host, payload.Port). When port 0 is used for auto-selection, the setup stores the key with port 0, and cancellation matches using the same port 0 value, ensuring correct key lookup.client/ssh/proxy/proxy.go (8)
100-101: LGTM!The logging enhancement improves observability of the connection flow.
104-105: LGTM!Storing the JWT token on the struct before starting the SSH server ensures all handlers can safely access it.
200-207: LGTM!Adding error checking for
session.Exit()improves robustness.
294-340: LGTM!The direct-tcpip handler correctly parses the payload, establishes the backend channel, and bridges I/O with proper error handling and context cancellation support.
481-506: LGTM!The cancel handler correctly retrieves the existing backend client and forwards the cancellation request.
530-535: LGTM!Proper use of read lock for concurrent access to the backend client.
561-579: LGTM!The individual forwarded channel handler correctly bridges backend and client channels with proper error handling.
209-224: Verify necessity of idle session creation or remove it to match server pattern.The backend session is created but never started (no
Shell(),Run(), or subsystem request). The server-sidehandleNonInteractiveSession(client/ssh/server/session_handlers.go:83) does not create a session at all—it waits directly on the context for port forwarding channels, which operate independently. The proxy's approach differs without clear justification. Either verify that the backend requires an active session to exist for port forwarding to work, or remove the session creation to match the server's simpler and proven-working pattern.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @client/ssh/server/server_config_test.go:
- Around line 300-303: The test constructs PrivilegeCheckResult.User with only
Username, which can miss UID/GID-based logic; update the test in
server_config_test.go to populate a full user.User (set Uid and Gid as strings
like "0"/non-"0", and HomeDir) or obtain a real user via user.Lookup to reflect
actual privilege semantics, and adjust the test cases so when you expect
privileged access the User.Uid is "0" and when not expected it is non-zero;
ensure the PrivilegeCheckResult assignment uses the augmented user object so
checkPrivilegedPortAccess exercises UID-based checks.
♻️ Duplicate comments (1)
client/ssh/server/server_config_test.go (1)
569-573: Add error assertion for the denial case.The
else if err != nilcondition (line 569) means that ifexpectAllowedis false buterris nil, the test silently passes without catching this failure. When port forwarding is disabled, the session should always fail with an error.🔎 Proposed fix
- } else if err != nil { + } else { // When denied, we expect an error message about port forwarding being disabled + require.Error(t, err, "Session should be denied when port forwarding is disabled") assert.Contains(t, err.Error(), "port forwarding is disabled", "Should get port forwarding disabled message") }
🧹 Nitpick comments (1)
client/ssh/server/server_config_test.go (1)
235-235: Consider removing unnecessary configuration.Since the test directly calls
checkPrivilegedPortAccessrather than going through the full port forwarding flow, settingSetAllowRemotePortForwarding(true)may not be necessary. Consider removing it for clarity unless it's required by the method under test.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/ssh/server/server_config_test.go
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: lixmal
Repo: netbirdio/netbird PR: 5031
File: client/ssh/proxy/proxy.go:537-559
Timestamp: 2026-01-05T06:33:43.265Z
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.
🧬 Code graph analysis (1)
client/ssh/server/server_config_test.go (3)
client/ssh/server/server.go (2)
Config(185-191)New(204-218)client/ssh/server/user_utils.go (1)
PrivilegeCheckResult(50-66)client/ssh/server/test.go (1)
StartTestServer(12-45)
⏰ 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, mysql)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client / Unit (386)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Windows
- GitHub Check: Linux
- GitHub Check: Darwin
- GitHub Check: iOS / Build
- GitHub Check: Client / Unit
- GitHub Check: Android / Build
- GitHub Check: JS / Lint
- GitHub Check: Client / Unit
- GitHub Check: release
- GitHub Check: release_ui_darwin
- GitHub Check: FreeBSD Port / Build & Test
- GitHub Check: Client / Unit
2 participants
Describe your changes
Implement port forwarding for the SSH proxy
Allow user switching for port forwarding
Allow SSH sessions without command/tty (
ssh -T <host>orssh -N <host>)Allow port 0 (auto select) for port forwarding (e.g.
ssh -R 0:localhost:80)Improve status output:
Issue 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
Tests
✏️ Tip: You can customize this high-level summary in your review settings.