[client,management] Rewrite the SSH feature #4015
Conversation
This reverts commit 26fc32f.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 29
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/ui/client_ui.go (1)
1247-1303: Reset JWT TTL UI when the server clears itWhen the management config omits
SSHJWTCacheTTL, we neither resets.sshJWTCacheTTLnor cleariSSHJWTCacheTTL. The old TTL stays visible and will be re-sent on the next save, effectively recreating a cache the server just disabled. Please reset the state/UI when the value is absent.- if cfg.SSHJWTCacheTTL != nil { - s.sshJWTCacheTTL = *cfg.SSHJWTCacheTTL - } + if cfg.SSHJWTCacheTTL != nil { + s.sshJWTCacheTTL = *cfg.SSHJWTCacheTTL + } else { + s.sshJWTCacheTTL = 0 + } … - if cfg.SSHJWTCacheTTL != nil { - s.iSSHJWTCacheTTL.SetText(strconv.Itoa(*cfg.SSHJWTCacheTTL)) - } + if cfg.SSHJWTCacheTTL != nil { + s.iSSHJWTCacheTTL.SetText(strconv.Itoa(*cfg.SSHJWTCacheTTL)) + } else { + s.iSSHJWTCacheTTL.SetText("") + }
🧹 Nitpick comments (6)
client/internal/peer/env.go (1)
13-18: Appropriate platform-specific handling for JS/Wasm environments.The logic correctly forces relay connections for JavaScript/Wasm environments where direct peer-to-peer networking is unavailable due to browser restrictions. This is a sound architectural choice that enables browser-based clients while preserving existing behavior for native platforms.
Consider adding a brief comment explaining the rationale:
func isForceRelayed() bool { + // JS/Wasm environments (browsers) cannot establish direct peer connections if runtime.GOOS == "js" { return true } return strings.EqualFold(os.Getenv(EnvKeyNBForceRelay), "true") }client/internal/peer/conn.go (1)
669-671: Document the platform-specific behavior.The platform guard on line 669 creates divergent behavior for JS/WASM: the ICE disconnection check is skipped entirely, allowing the connection to be considered healthy via relay alone. On other platforms, the function enforces that ICE must be connected or in-progress.
Adding a brief inline comment would help future maintainers understand why JS/WASM environments receive special treatment.
Apply this diff to add documentation:
+ // On JS/WASM, skip strict ICE checks to allow relay-only connections, + // as browser environments may have ICE limitations. if runtime.GOOS != "js" && conn.statusICE.Get() == worker.StatusDisconnected && !conn.workerICE.InProgress() { return false }client/ssh/server/winpty/conpty_test.go (3)
121-133: Consider using defer for handle cleanup.The handles are cleaned up at the end, but if any assertion fails before line 132, the handles will leak. Using
defer closeHandles(...)immediately after successful creation would ensure cleanup even on assertion failure.Apply this diff:
func TestCreateConPtyPipes(t *testing.T) { inputRead, inputWrite, outputRead, outputWrite, err := createConPtyPipes() require.NoError(t, err, "Should create ConPty pipes successfully") + defer closeHandles(inputRead, inputWrite, outputRead, outputWrite) // Verify all handles are valid assert.NotEqual(t, windows.InvalidHandle, inputRead, "Input read handle should be valid") assert.NotEqual(t, windows.InvalidHandle, inputWrite, "Input write handle should be valid") assert.NotEqual(t, windows.InvalidHandle, outputRead, "Output read handle should be valid") assert.NotEqual(t, windows.InvalidHandle, outputWrite, "Output write handle should be valid") - - // Clean up handles - closeHandles(inputRead, inputWrite, outputRead, outputWrite) }
135-147: Consider using defer for ConPty handle cleanup.The ConPty handle cleanup at line 145 will be skipped if the assertion at line 142 fails, potentially leaking the pseudo-console. Using
deferwould ensure cleanup regardless of assertion outcomes.Apply this diff:
hPty, err := createConPty(80, 24, inputRead, outputWrite) require.NoError(t, err, "Should create ConPty successfully") assert.NotEqual(t, windows.InvalidHandle, hPty, "ConPty handle should be valid") + defer func() { + if ret, _, _ := procClosePseudoConsole.Call(uintptr(hPty)); ret == 0 { + t.Logf("Failed to close ConPty") + } + }() - - // Clean up ConPty - ret, _, _ := procClosePseudoConsole.Call(uintptr(hPty)) - assert.NotEqual(t, uintptr(0), ret, "Should close ConPty successfully") }
149-187: LGTM with minor observation.The test covers important edge cases including empty environments and empty strings. Note that the
hasErrorfield is defined but never used (all cases set it tofalse). If there are malformed environment scenarios that should error, consider adding test cases for them.client/proto/daemon.proto (1)
275-285: Field numbering is non-sequential in GetConfigResponse.The SSH-related fields use non-sequential numbering: 21, 24, 22, 23, 25. While protobuf permits this, sequential numbering (21, 22, 23, 24, 25) is clearer and suggests these fields were added together. This pattern can cause confusion during maintenance.
Consider renumbering for clarity if this is early in the release cycle:
bool enableSSHRoot = 21; -bool enableSSHSFTP = 24; -bool enableSSHLocalPortForwarding = 22; -bool enableSSHRemotePortForwarding = 23; -bool disableSSHAuth = 25; -int32 sshJWTCacheTTL = 26; +bool enableSSHLocalPortForwarding = 22; +bool enableSSHRemotePortForwarding = 23; +bool enableSSHSFTP = 24; +bool disableSSHAuth = 25; +int32 sshJWTCacheTTL = 26;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
client/proto/daemon.pb.gois excluded by!**/*.pb.goclient/proto/daemon_grpc.pb.gois excluded by!**/*.pb.gogo.sumis excluded by!**/*.sumshared/management/proto/management.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (105)
.github/workflows/check-license-dependencies.yml(1 hunks)client/android/client.go(1 hunks)client/android/preferences.go(1 hunks)client/cmd/root.go(0 hunks)client/cmd/ssh.go(1 hunks)client/cmd/ssh_exec_unix.go(1 hunks)client/cmd/ssh_sftp_unix.go(1 hunks)client/cmd/ssh_sftp_windows.go(1 hunks)client/cmd/ssh_test.go(1 hunks)client/cmd/testutil_test.go(1 hunks)client/cmd/up.go(3 hunks)client/embed/embed.go(4 hunks)client/firewall/uspfilter/filter.go(1 hunks)client/firewall/uspfilter/filter_test.go(2 hunks)client/firewall/uspfilter/nat_stateful_test.go(1 hunks)client/internal/acl/manager.go(0 hunks)client/internal/acl/manager_test.go(0 hunks)client/internal/connect.go(2 hunks)client/internal/debug/debug.go(1 hunks)client/internal/engine.go(11 hunks)client/internal/engine_ssh.go(1 hunks)client/internal/engine_test.go(8 hunks)client/internal/login.go(2 hunks)client/internal/peer/conn.go(1 hunks)client/internal/peer/env.go(2 hunks)client/internal/peer/status.go(3 hunks)client/internal/profilemanager/config.go(3 hunks)client/internal/profilemanager/config_test.go(1 hunks)client/internal/profilemanager/profilemanager.go(1 hunks)client/internal/routemanager/dynamic/route.go(1 hunks)client/ios/NetBirdSDK/client.go(1 hunks)client/proto/daemon.proto(7 hunks)client/server/jwt_cache.go(1 hunks)client/server/network.go(1 hunks)client/server/server.go(9 hunks)client/server/server_test.go(1 hunks)client/server/setconfig_test.go(5 hunks)client/server/state_generic.go(1 hunks)client/server/state_linux.go(1 hunks)client/ssh/client.go(0 hunks)client/ssh/client/client.go(1 hunks)client/ssh/client/client_test.go(1 hunks)client/ssh/client/terminal_unix.go(1 hunks)client/ssh/client/terminal_windows.go(1 hunks)client/ssh/common.go(1 hunks)client/ssh/config/manager.go(1 hunks)client/ssh/config/manager_test.go(1 hunks)client/ssh/config/shutdown_state.go(1 hunks)client/ssh/detection/detection.go(1 hunks)client/ssh/login.go(0 hunks)client/ssh/lookup.go(0 hunks)client/ssh/lookup_darwin.go(0 hunks)client/ssh/proxy/proxy.go(1 hunks)client/ssh/proxy/proxy_test.go(1 hunks)client/ssh/server.go(0 hunks)client/ssh/server/command_execution.go(1 hunks)client/ssh/server/command_execution_js.go(1 hunks)client/ssh/server/command_execution_unix.go(1 hunks)client/ssh/server/command_execution_windows.go(1 hunks)client/ssh/server/compatibility_test.go(1 hunks)client/ssh/server/executor_unix.go(1 hunks)client/ssh/server/executor_unix_test.go(1 hunks)client/ssh/server/executor_windows.go(1 hunks)client/ssh/server/jwt_test.go(1 hunks)client/ssh/server/port_forwarding.go(1 hunks)client/ssh/server/server.go(1 hunks)client/ssh/server/server_config_test.go(1 hunks)client/ssh/server/server_test.go(1 hunks)client/ssh/server/session_handlers.go(1 hunks)client/ssh/server/session_handlers_js.go(1 hunks)client/ssh/server/sftp.go(1 hunks)client/ssh/server/sftp_js.go(1 hunks)client/ssh/server/sftp_test.go(1 hunks)client/ssh/server/sftp_unix.go(1 hunks)client/ssh/server/sftp_windows.go(1 hunks)client/ssh/server/shell.go(1 hunks)client/ssh/server/test.go(1 hunks)client/ssh/server/user_utils.go(1 hunks)client/ssh/server/user_utils_js.go(1 hunks)client/ssh/server/user_utils_test.go(1 hunks)client/ssh/server/userswitching_js.go(1 hunks)client/ssh/server/userswitching_unix.go(1 hunks)client/ssh/server/userswitching_windows.go(1 hunks)client/ssh/server/winpty/conpty.go(1 hunks)client/ssh/server/winpty/conpty_test.go(1 hunks)client/ssh/server_mock.go(0 hunks)client/ssh/server_test.go(0 hunks)client/ssh/ssh.go(3 hunks)client/ssh/testutil/user_helpers.go(1 hunks)client/ssh/window_freebsd.go(0 hunks)client/ssh/window_unix.go(0 hunks)client/ssh/window_windows.go(0 hunks)client/system/info.go(2 hunks)client/ui/client_ui.go(6 hunks)client/wasm/cmd/main.go(3 hunks)client/wasm/internal/ssh/client.go(3 hunks)client/wasm/internal/ssh/key.go(0 hunks)go.mod(8 hunks)management/internals/server/modules.go(1 hunks)management/server/account.go(14 hunks)management/server/account/manager.go(3 hunks)management/server/account_test.go(26 hunks)management/server/auth/manager.go(2 hunks)management/server/auth/manager_mock.go(3 hunks)management/server/auth/manager_test.go(5 hunks)
💤 Files with no reviewable changes (14)
- client/internal/acl/manager_test.go
- client/internal/acl/manager.go
- client/ssh/lookup.go
- client/ssh/server_mock.go
- client/ssh/window_freebsd.go
- client/ssh/lookup_darwin.go
- client/ssh/server_test.go
- client/ssh/client.go
- client/cmd/root.go
- client/wasm/internal/ssh/key.go
- client/ssh/login.go
- client/ssh/window_unix.go
- client/ssh/server.go
- client/ssh/window_windows.go
🧰 Additional context used
🧬 Code graph analysis (69)
client/server/state_generic.go (1)
client/ssh/config/shutdown_state.go (1)
ShutdownState(4-7)
client/internal/login.go (1)
management/client/grpc.go (1)
c(367-373)
client/server/state_linux.go (2)
client/ssh/config/manager.go (1)
Manager(86-89)client/ssh/config/shutdown_state.go (1)
ShutdownState(4-7)
client/ssh/server/test.go (2)
client/ssh/server/server.go (1)
Server(108-131)client/ssh/server_mock.go (1)
srv(39-44)
client/ssh/config/manager_test.go (1)
client/ssh/config/manager.go (4)
Manager(86-89)PeerSSHInfo(92-96)MaxPeersForSSHConfig(23-23)EnvForceSSHConfig(21-21)
client/ssh/server/sftp_windows.go (4)
client/ssh/server/server.go (1)
Server(108-131)client/ssh/server/executor_windows.go (1)
NewPrivilegeDropper(45-47)client/ssh/server/executor_unix.go (1)
NewPrivilegeDropper(41-43)client/ssh/server.go (2)
srv(136-214)srv(216-244)
client/cmd/ssh_exec_unix.go (1)
client/ssh/server/executor_unix.go (2)
NewPrivilegeDropper(41-43)ExecutorConfig(27-35)
client/ssh/server/server_test.go (3)
client/ssh/ssh.go (2)
GeneratePrivateKey(36-59)ED25519(24-24)client/ssh/server/server.go (3)
Config(141-147)New(151-163)Server(108-131)client/ssh/server.go (1)
Stop(35-44)
client/ssh/server/session_handlers_js.go (2)
client/ssh/server/server.go (1)
Server(108-131)client/ssh/server/user_utils.go (1)
PrivilegeCheckResult(50-66)
client/ssh/config/shutdown_state.go (1)
client/ssh/config/manager.go (1)
Manager(86-89)
client/ssh/server/command_execution_js.go (1)
client/ssh/server.go (1)
srv(136-214)
client/ssh/server/userswitching_windows.go (3)
client/ssh/server/server.go (1)
Server(108-131)client/ssh/server/executor_windows.go (2)
WindowsExecutorConfig(30-41)NewPrivilegeDropper(45-47)client/ssh/server/executor_unix.go (1)
NewPrivilegeDropper(41-43)
client/ssh/server/shell.go (2)
client/ssh/server/server.go (1)
InternalSSHPort(33-33)client/ssh/server.go (4)
getUserShell(260-274)prepareUserEnv(119-125)srv(136-214)srv(216-244)
client/server/server_test.go (1)
management/server/account.go (1)
BuildManager(191-296)
management/internals/server/modules.go (1)
management/server/account.go (1)
BuildManager(191-296)
client/ssh/server/session_handlers.go (3)
client/ssh/server/server.go (3)
Server(108-131)New(151-163)PrivilegedUserError(51-53)client/ssh/server/port_forwarding.go (2)
SessionKey(16-16)ConnectionKey(19-19)client/ssh/server/user_utils.go (2)
ErrPrivilegeRequired(15-15)ErrPrivilegedUserSwitch(16-16)
client/cmd/ssh_sftp_unix.go (1)
client/ssh/server/executor_unix.go (5)
NewPrivilegeDropper(41-43)ExecutorConfig(27-35)ExitCodePrivilegeDropFail(21-21)ExitCodeShellExecFail(22-22)ExitCodeSuccess(20-20)
management/server/account/manager.go (1)
shared/auth/user.go (1)
UserAuth(7-28)
client/ssh/server/sftp.go (2)
client/ssh/server/server.go (1)
Server(108-131)client/ssh/server/user_utils.go (2)
PrivilegeCheckRequest(42-47)FeatureSFTP(38-38)
client/ssh/server/executor_unix_test.go (1)
client/ssh/server/executor_unix.go (1)
ExecutorConfig(27-35)
client/wasm/cmd/main.go (1)
client/ssh/detection/detection.go (2)
ServerType(26-26)DetectSSHServerType(59-99)
client/ssh/common.go (5)
client/ssh/proxy/proxy.go (1)
New(45-59)client/ssh/config/manager.go (1)
New(99-105)client/proto/daemon_grpc.pb.go (1)
DaemonServiceClient(20-73)client/proto/daemon.pb.go (9)
GetPeerSSHHostKeyRequest(4716-4722)GetPeerSSHHostKeyRequest(4735-4735)GetPeerSSHHostKeyRequest(4750-4752)RequestJWTAuthRequest(4835-4841)RequestJWTAuthRequest(4854-4854)RequestJWTAuthRequest(4869-4871)WaitJWTTokenRequest(4981-4989)WaitJWTTokenRequest(5002-5002)WaitJWTTokenRequest(5017-5019)client/ssh/server.go (1)
srv(75-86)
client/ssh/server/command_execution_unix.go (2)
client/ssh/server/user_utils.go (1)
PrivilegeCheckResult(50-66)client/ssh/server.go (2)
srv(136-214)srv(216-244)
client/cmd/testutil_test.go (1)
management/server/account.go (1)
BuildManager(191-296)
client/firewall/uspfilter/nat_stateful_test.go (4)
client/firewall/uspfilter/filter.go (1)
Create(156-158)client/firewall/uspfilter/filter_test.go (1)
IFaceMock(32-37)client/iface/device/device_filter.go (1)
PacketFilter(11-26)client/iface/iface.go (1)
DefaultMTU(28-28)
client/ssh/server/sftp_unix.go (1)
client/ssh/server/server.go (1)
Server(108-131)
client/ssh/server/command_execution.go (3)
client/ssh/server/user_utils.go (1)
PrivilegeCheckResult(50-66)client/ssh/server.go (2)
srv(136-214)srv(216-244)client/ssh/client.go (1)
c(24-85)
client/cmd/ssh_sftp_windows.go (1)
client/ssh/server/executor_windows.go (3)
ExitCodeValidationFail(27-27)ExitCodeShellExecFail(26-26)ExitCodeSuccess(22-22)
client/ssh/client/terminal_windows.go (3)
client/ssh/client/client.go (1)
Client(37-44)client/ssh/client.go (2)
c(24-85)client(14-16)client/ssh/window_windows.go (1)
setWinSize(7-9)
client/wasm/internal/ssh/client.go (3)
client/embed/embed.go (1)
Client(33-41)client/ssh/common.go (1)
CreateHostKeyCallback(140-159)client/internal/config.go (1)
PrivateKey(66-115)
client/embed/embed.go (2)
client/ssh/common.go (2)
ErrPeerNotFound(27-27)VerifyHostKey(113-128)client/internal/engine.go (1)
Engine(138-211)
client/ssh/server/compatibility_test.go (7)
client/ssh/proxy/proxy_test.go (1)
TestMain(35-55)client/ssh/client/client_test.go (1)
TestMain(26-42)client/ssh/testutil/user_helpers.go (3)
CleanupTestUsers(87-93)GetTestUsername(20-45)IsCI(48-59)client/ssh/ssh.go (2)
GeneratePrivateKey(36-59)ED25519(24-24)client/ssh/server/server.go (2)
Config(141-147)New(151-163)client/ssh/server/test.go (1)
StartTestServer(12-45)client/ssh/server_test.go (3)
TestServer_AddAuthorizedKey(11-49)TestServer_PubKeyHandler(81-121)TestServer_RemoveAuthorizedKey(51-79)
client/ssh/proxy/proxy.go (5)
client/proto/daemon_grpc.pb.go (2)
DaemonServiceClient(20-73)NewDaemonServiceClient(79-81)client/internal/profilemanager/profilemanager.go (1)
GetLoginHint(137-152)client/ssh/common.go (3)
RequestJWTToken(71-107)NewDaemonHostKeyVerifier(43-47)CreateHostKeyCallback(140-159)client/ssh/detection/detection.go (1)
ProxyIdentifier(18-18)version/version.go (1)
NetbirdVersion(18-20)
client/cmd/ssh.go (7)
client/ssh/server/server.go (2)
DefaultSSHPort(30-30)New(151-163)client/cmd/root.go (1)
SetFlagsFromEnvVars(198-219)util/log.go (2)
FindFirstLogPath(77-84)InitLog(32-74)client/ssh/proxy/proxy.go (1)
New(45-59)client/ssh/client/client.go (3)
Dial(284-310)DialOptions(275-281)Client(37-44)client/ssh/detection/detection.go (4)
ServerTypeRegular(31-31)Dialer(35-37)Timeout(23-23)DetectSSHServerType(59-99)client/ssh/client.go (1)
c(24-85)
management/server/auth/manager.go (1)
shared/auth/user.go (1)
UserAuth(7-28)
client/internal/peer/status.go (5)
client/ssh/server/server.go (1)
New(151-163)management/server/peer.go (2)
am(861-903)am(160-232)client/internal/peer/status_test.go (1)
TestStatus_UpdatePeerFQDN(61-78)management/server/mock_server/account_mock.go (1)
am(429-434)client/ssh/server.go (1)
srv(75-86)
client/ssh/server/user_utils_test.go (2)
client/ssh/server/server.go (1)
Server(108-131)client/ssh/server/user_utils.go (1)
PrivilegeCheckRequest(42-47)
client/ssh/server/winpty/conpty.go (2)
client/ssh/client.go (1)
c(24-85)client/ssh/server.go (1)
srv(136-214)
management/server/auth/manager_test.go (1)
shared/auth/user.go (1)
UserAuth(7-28)
client/ssh/server/user_utils.go (1)
client/ssh/server.go (1)
srv(136-214)
client/internal/profilemanager/config.go (1)
client/internal/config.go (6)
config(226-428)createNewConfig(192-203)ManagementURL(45-63)update(205-224)PrivateKey(66-115)UpdateConfig(151-157)
client/ssh/client/terminal_unix.go (1)
client/ssh/client/client.go (1)
Client(37-44)
client/ssh/server/sftp_test.go (3)
client/ssh/ssh.go (2)
GeneratePrivateKey(36-59)ED25519(24-24)client/ssh/server/server.go (2)
Config(141-147)New(151-163)client/ssh/server.go (1)
Stop(35-44)
client/ssh/server/server_config_test.go (5)
client/ssh/ssh.go (2)
GeneratePrivateKey(36-59)ED25519(24-24)client/ssh/server/server.go (2)
Config(141-147)New(151-163)client/ssh/server/test.go (1)
StartTestServer(12-45)client/ssh/client/client.go (2)
Dial(284-310)DialOptions(275-281)client/ssh/server.go (1)
srv(136-214)
client/internal/peer/conn.go (1)
client/internal/peer/worker/state.go (1)
StatusDisconnected(10-10)
client/server/server.go (5)
client/ssh/server/server.go (2)
Server(108-131)New(151-163)client/proto/daemon.pb.go (21)
GetPeerSSHHostKeyRequest(4716-4722)GetPeerSSHHostKeyRequest(4735-4735)GetPeerSSHHostKeyRequest(4750-4752)GetPeerSSHHostKeyResponse(4762-4774)GetPeerSSHHostKeyResponse(4787-4787)GetPeerSSHHostKeyResponse(4802-4804)RequestJWTAuthRequest(4835-4841)RequestJWTAuthRequest(4854-4854)RequestJWTAuthRequest(4869-4871)RequestJWTAuthResponse(4881-4899)RequestJWTAuthResponse(4912-4912)RequestJWTAuthResponse(4927-4929)WaitJWTTokenRequest(4981-4989)WaitJWTTokenRequest(5002-5002)WaitJWTTokenRequest(5017-5019)WaitJWTTokenResponse(5036-5046)WaitJWTTokenResponse(5059-5059)WaitJWTTokenResponse(5074-5076)GetConfigResponse(1097-1132)GetConfigResponse(1145-1145)GetConfigResponse(1160-1162)client/internal/engine.go (1)
Engine(138-211)client/internal/profilemanager/profilemanager.go (1)
GetLoginHint(137-152)client/internal/auth/oauth.go (1)
NewOAuthFlow(69-81)
client/ssh/client/client_test.go (5)
client/ssh/testutil/user_helpers.go (4)
CleanupTestUsers(87-93)GetTestUsername(20-45)IsCI(48-59)IsSystemAccount(62-76)client/ssh/ssh.go (2)
GeneratePrivateKey(36-59)ED25519(24-24)client/ssh/server/server.go (3)
Config(141-147)New(151-163)Server(108-131)client/ssh/server/test.go (1)
StartTestServer(12-45)client/ssh/client/client.go (3)
Dial(284-310)DialOptions(275-281)Client(37-44)
management/server/auth/manager_mock.go (1)
shared/auth/user.go (1)
UserAuth(7-28)
client/firewall/uspfilter/filter_test.go (2)
client/firewall/uspfilter/filter.go (1)
Create(156-158)client/iface/iface.go (1)
DefaultMTU(28-28)
client/ssh/testutil/user_helpers.go (2)
client/ssh/lookup_darwin.go (1)
idUserNameLookup(26-47)client/ssh/server.go (1)
prepareUserEnv(119-125)
client/ssh/server/server.go (5)
shared/auth/jwt/validator.go (2)
Validator(56-63)NewValidator(72-85)shared/auth/jwt/extractor.go (3)
ClaimsExtractor(34-37)NewClaimsExtractor(58-68)WithAudience(43-47)shared/auth/user.go (1)
UserAuth(7-28)client/ssh/detection/detection.go (2)
ServerIdentifier(16-16)JWTRequiredMarker(20-20)version/version.go (1)
NetbirdVersion(18-20)
client/ui/client_ui.go (4)
client/ssh/server/server.go (1)
New(151-163)client/server/server.go (1)
New(99-110)client/iface/iface.go (2)
MinMTU(29-29)MaxMTU(30-30)client/proto/daemon.pb.go (3)
SetConfigRequest(3788-3829)SetConfigRequest(3842-3842)SetConfigRequest(3857-3859)
client/ssh/server/executor_unix.go (1)
client/ssh/server/executor_windows.go (4)
NewPrivilegeDropper(45-47)PrivilegeDropper(43-43)ExitCodeSuccess(22-22)ExitCodeShellExecFail(26-26)
client/internal/engine_ssh.go (6)
client/internal/engine.go (1)
Engine(138-211)client/ssh/server/server.go (5)
New(151-163)JWTConfig(133-138)Config(141-147)InternalSSHPort(33-33)Server(108-131)client/ssh/config/manager.go (2)
New(99-105)PeerSSHInfo(92-96)client/firewall/manager/protocol.go (1)
ProtocolTCP(9-9)shared/management/proto/management.pb.go (9)
SSHConfig(2091-2102)SSHConfig(2117-2117)SSHConfig(2132-2134)JWTConfig(1619-1628)JWTConfig(1643-1643)JWTConfig(1658-1660)RemotePeerConfig(2007-2021)RemotePeerConfig(2036-2036)RemotePeerConfig(2051-2053)client/ssh/config/shutdown_state.go (1)
ShutdownState(4-7)
client/ssh/server/port_forwarding.go (2)
client/ssh/server/server.go (1)
Server(108-131)client/ssh/server/user_utils.go (1)
PrivilegeCheckRequest(42-47)
client/ssh/server/command_execution_windows.go (4)
client/ssh/server/server.go (1)
Server(108-131)client/ssh/server/executor_windows.go (1)
NewPrivilegeDropper(45-47)client/ssh/server/user_utils.go (1)
PrivilegeCheckResult(50-66)client/ssh/server/winpty/conpty.go (3)
PtyConfig(36-42)UserConfig(45-48)ExecutePtyWithUserToken(59-71)
client/ssh/server/jwt_test.go (6)
client/ssh/server/server.go (4)
JWTConfig(133-138)Config(141-147)New(151-163)Server(108-131)client/ssh/server/test.go (1)
StartTestServer(12-45)client/ssh/detection/detection.go (5)
Dialer(35-37)Timeout(23-23)DetectSSHServerType(59-99)ServerTypeNetBirdNoJWT(30-30)ServerTypeNetBirdJWT(29-29)client/ssh/testutil/user_helpers.go (1)
GetTestUsername(20-45)client/ssh/client/client.go (3)
Dial(284-310)Client(37-44)DialOptions(275-281)shared/auth/jwt/validator.go (2)
JSONWebKey(44-54)Jwks(26-29)
client/internal/connect.go (2)
util/common.go (1)
ReturnBoolWithDefaultTrue(51-58)client/internal/config.go (1)
config(226-428)
client/ssh/proxy/proxy_test.go (7)
client/ssh/testutil/user_helpers.go (2)
CleanupTestUsers(87-93)GetTestUsername(20-45)client/ssh/proxy/proxy.go (2)
SSHProxy(37-43)New(45-59)client/proto/daemon_grpc.pb.go (1)
NewDaemonServiceClient(79-81)client/ssh/server/server.go (4)
Config(141-147)JWTConfig(133-138)New(151-163)Server(108-131)client/ssh/server/test.go (1)
StartTestServer(12-45)client/proto/daemon.pb.go (12)
GetPeerSSHHostKeyRequest(4716-4722)GetPeerSSHHostKeyRequest(4735-4735)GetPeerSSHHostKeyRequest(4750-4752)GetPeerSSHHostKeyResponse(4762-4774)GetPeerSSHHostKeyResponse(4787-4787)GetPeerSSHHostKeyResponse(4802-4804)RequestJWTAuthResponse(4881-4899)RequestJWTAuthResponse(4912-4912)RequestJWTAuthResponse(4927-4929)WaitJWTTokenResponse(5036-5046)WaitJWTTokenResponse(5059-5059)WaitJWTTokenResponse(5074-5076)shared/auth/jwt/validator.go (2)
JSONWebKey(44-54)Jwks(26-29)
client/ssh/config/manager.go (1)
client/ssh/common.go (3)
NetBirdSSHConfigFile(19-19)UnixSSHConfigDir(21-21)WindowsSSHConfigDir(22-22)
client/android/preferences.go (3)
client/ios/NetBirdSDK/preferences.go (1)
Preferences(8-10)client/internal/profilemanager/config.go (1)
ReadConfig(772-774)client/internal/config.go (1)
config(226-428)
client/internal/engine.go (1)
client/ssh/config/shutdown_state.go (1)
ShutdownState(4-7)
client/ssh/server/executor_windows.go (2)
client/ssh/server/executor_unix.go (2)
NewPrivilegeDropper(41-43)PrivilegeDropper(38-38)client/ssh/server/server.go (2)
New(151-163)Server(108-131)
client/ssh/server/userswitching_unix.go (4)
client/ssh/server/executor_unix.go (2)
NewPrivilegeDropper(41-43)ExecutorConfig(27-35)client/ssh/server/user_utils.go (1)
PrivilegeCheckResult(50-66)client/ssh/server.go (1)
prepareUserEnv(119-125)client/ssh/login.go (1)
getLoginCmd(12-36)
client/internal/engine_test.go (4)
shared/management/proto/management.pb.go (15)
NetworkMap(1857-1887)NetworkMap(1902-1902)NetworkMap(1917-1919)PeerConfig(1757-1773)PeerConfig(1788-1788)PeerConfig(1803-1805)SSHConfig(2091-2102)SSHConfig(2117-2117)SSHConfig(2132-2134)JWTConfig(1619-1628)JWTConfig(1643-1643)JWTConfig(1658-1660)RemotePeerConfig(2007-2021)RemotePeerConfig(2036-2036)RemotePeerConfig(2051-2053)client/ssh/server/server.go (1)
JWTConfig(133-138)client/internal/engine.go (2)
Engine(138-211)EngineConfig(81-135)management/server/account.go (1)
BuildManager(191-296)
management/server/account.go (1)
shared/auth/user.go (1)
UserAuth(7-28)
client/ssh/client/client.go (5)
client/ssh/detection/detection.go (3)
Timeout(23-23)Dialer(35-37)DetectSSHServerType(59-99)client/ssh/common.go (3)
RequestJWTToken(71-107)NewDaemonHostKeyVerifier(43-47)CreateHostKeyCallback(140-159)client/internal/profilemanager/profilemanager.go (1)
GetLoginHint(137-152)client/proto/daemon_grpc.pb.go (1)
NewDaemonServiceClient(79-81)client/ssh/proxy/proxy.go (1)
New(45-59)
client/cmd/up.go (1)
client/internal/config.go (1)
config(226-428)
management/server/account_test.go (2)
shared/auth/user.go (1)
UserAuth(7-28)management/server/account.go (1)
BuildManager(191-296)
🪛 Gitleaks (8.28.0)
client/ssh/server/jwt_test.go
[high] 593-593: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ 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). (25)
- GitHub Check: Management / Benchmark (amd64, postgres)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Management / Benchmark (API) (amd64, postgres)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Management / Benchmark (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Benchmark (API) (amd64, sqlite)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Management / Integration (amd64, postgres)
- GitHub Check: Client / Unit (386)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
- GitHub Check: Windows
- GitHub Check: Linux
- GitHub Check: Darwin
- GitHub Check: release_ui_darwin
- GitHub Check: release
- GitHub Check: Client / Unit
- GitHub Check: release_ui
- GitHub Check: Android / Build
- GitHub Check: iOS / Build
- GitHub Check: JS / Lint
🔇 Additional comments (41)
client/ios/NetBirdSDK/client.go (1)
23-24: Import reordering looks good.The import reorganization has no functional impact. Both imports are used correctly in the code and there are no conflicts or ordering-related issues.
client/server/network.go (1)
3-16: Import reordering aligns with Go conventions.The import block has been reorganized to follow standard Go practices: standard library → third-party → local/internal packages. The
domainimport is correctly positioned with other NetBird internal imports and is properly used throughout the file (e.g.,domain.Listat line 21,domain.Domainat line 88)..github/workflows/check-license-dependencies.yml (2)
22-33: Process substitution refactoring fixes variable scoping bug.The switch from a piped while loop to process substitution (
done < <(find ...)) is a subtle but important correctness fix. In the original approach, the loop would execute in a subshell, causingFOUND_ISSUES=1assignments inside the loop to be lost after the subshell exits. The new structure keeps the loop in the current shell context, properly preserving theFOUND_ISSUESaccumulator. Combined with improved error handling (2>/dev/null,|| true), the script is now more robust and correct.
38-38: License error message is now more explicit and actionable.The updated message clearly identifies the AGPLv3 licensing constraint and prohibits import of affected packages by BSD-licensed code. This aligns well with the PR's broader effort to enforce stricter license compliance across the codebase.
client/firewall/uspfilter/filter.go (5)
38-42: LGTM: Clean composite key design.The
serviceKeystruct properly encapsulates the protocol/port pair for netstack service identification. The struct is suitable for use as a map key since both fields are comparable types.
1406-1415: LGTM: Proper service registration with correct locking.The method correctly constructs the composite key and registers the service with appropriate mutex protection. The debug logging is helpful for troubleshooting.
1417-1425: LGTM: Clean unregistration logic.The unregistration correctly mirrors the registration logic with proper key construction and mutex protection.
1427-1439: LGTM: Protocol conversion mapping is correct.The conversion from
nftypes.Protocoltogopacket.LayerTypehandles the expected protocol types correctly. The default case returningLayerType(0)is acceptable for this internal helper method.
1465-1487: LGTM: Service lookup logic is correct.The method properly constructs the composite key for lookup and uses read lock appropriately. The logic correctly handles edge cases (insufficient layers, non-TCP/UDP protocols).
client/firewall/uspfilter/filter_test.go (1)
1119-1252: LGTM: Comprehensive test coverage for forwarding logic.The test thoroughly validates the
shouldForwardmethod across all key scenarios:
- Local forwarding disabled
- Traffic to other local interfaces
- NetBird IP handling with/without netstack
- Service registration impact
The table-driven approach and helper function make the test maintainable and easy to extend.
client/firewall/uspfilter/nat_stateful_test.go (2)
15-48: LGTM: Test validates core DNAT behavior.The test correctly verifies:
- Inbound traffic to port 22 is translated to 22022 (aligns with SSH port changes in PR)
- Return traffic is not double-translated (important for stateful NAT)
The test logic is sound and the assertions are appropriate.
51-85: LGTM: Multi-rule DNAT test is thorough.The test properly validates that multiple DNAT rules can be configured and work correctly for different peers. This is an important scenario for real-world deployments with multiple SSH-enabled peers.
client/ssh/server/shell.go (6)
18-23: LGTM!Constants are appropriately defined with clear naming and security annotation.
25-34: LGTM!Platform detection and shell selection dispatch is clean and correct.
36-50: LGTM!The security rationale for avoiding cmd.exe is sound, and the PowerShell preference order with hardcoded fallback is reasonable for Windows environments.
52-64: LGTM!Fallback chain from /etc/passwd → SHELL env → /bin/sh is sensible.
111-149: LGTM!Environment variable whitelisting logic is correct and follows standard SSH practices. The exact and prefix matching handles common locale, terminal, and editor variables appropriately.
151-175: LGTM!SSH environment variable preparation correctly follows SSH protocol standards with appropriate error handling and fallbacks. The use of
InternalSSHPortconstant is consistent with the codebase.client/ssh/server/winpty/conpty_test.go (4)
55-119: LGTM!The test provides comprehensive coverage of Windows command-line argument escaping and quoting behavior, including edge cases like nested quotes, backslashes, empty arguments, and paths with spaces.
189-211: LGTM!The test properly manages Windows tokens with appropriate cleanup, and correctly skips in short mode since token operations can be expensive. Resource management is thorough with both deferred and explicit cleanup.
241-267: Verify double-close safety.Similar to
TestWindowsHandleReader, the write handle is explicitly closed at line 256, thendefer closeHandles(...)at line 246 will attempt to close it again. Ensure thatcloseHandlessafely handles already-closed handles.
269-289: LGTM!The benchmark appropriately measures the full ConPty creation lifecycle including pipe setup and cleanup. Error handling with
b.Fatalensures the benchmark stops on failure, and cleanup is comprehensive.client/ssh/server/userswitching_windows.go (1)
89-99: LGTM - Windows executor command creation looks correct.The validation and command creation flow is appropriate for Windows user switching. The username validation before executor creation provides proper input sanitization.
client/server/state_linux.go (1)
11-19: LGTM - SSH config state registration.The SSH config shutdown state is properly registered alongside other shutdown states for Linux platforms.
client/server/state_generic.go (1)
9-15: LGTM - SSH config state registration.The SSH config shutdown state is properly registered for non-Linux platforms.
client/internal/peer/status.go (2)
70-70: LGTM - SSH host key field addition.The SSHHostKey field properly extends the peer state to track SSH host keys.
576-590: LGTM - UpdatePeerSSHHostKey implementation.The method follows the established pattern for peer state updates with proper mutex locking and error handling.
management/server/account_test.go (1)
2951-2951: Let me verify the test file one more time to ensure I understand the full context:No changes required. The nil config parameter is safe.
Based on comprehensive verification:
- BuildManager stores the config parameter directly without any nil checks (expected pattern)
- The config field is never dereferenced or accessed anywhere in DefaultAccountManager
- This is a valid Go testing pattern—nil pointers can be safely passed for unused fields
The test scenario is correct as-is.
client/internal/profilemanager/profilemanager.go (1)
136-152: LGTM!The
GetLoginHint()function is well-implemented with appropriate error handling and debug logging. The function correctly retrieves the email from the active profile state to use as a login hint, gracefully returning an empty string on any failure.client/ssh/server/userswitching_js.go (1)
1-8: LGTM!The JS/WASM stub correctly returns
errNotSupportedfor user switching functionality, which is not available in the JavaScript/WebAssembly environment.client/ssh/server/sftp_js.go (1)
1-12: LGTM!The JS/WASM stub appropriately returns
errNotSupportedforparseUserCredentials, which is consistent with other JS/WASM-specific implementations in this SSH server package.client/internal/login.go (1)
127-131: LGTM!The SSH-related feature flags are correctly propagated through both the login and registration flows. The implementation is consistent across both
doMgmLoginandregisterPeerfunctions.Also applies to: 158-162
client/server/setconfig_test.go (3)
75-75: LGTM!The SSH JWT cache TTL field is properly tested with appropriate variable definition, request population, and assertions. The test correctly verifies that the TTL value is persisted to the configuration.
Also applies to: 106-106, 151-152
173-204: LGTM!The
expectedFieldsmap is correctly updated to include all SSH-related configuration fields. The comprehensive field coverage ensures that any future additions will be caught by the reflection-based test.
233-263: LGTM!The CLI flag mapping is correctly updated to include all SSH-related flags. This ensures that the SSH configuration flags are properly wired from the CLI to the SetConfigRequest.
management/internals/server/modules.go (1)
69-69: LGTM!The addition of
s.configas the second parameter correctly aligns with the updatedBuildManagersignature, enabling configuration propagation through the account manager initialization flow.client/ssh/server/user_utils_js.go (1)
1-8:errNotSupportedis properly defined in the server package.The variable is defined in
client/ssh/server/command_execution_js.goat line 13, making it accessible to thevalidateUsernamefunction inuser_utils_js.gosince both files are in the sameserverpackage. No issues found.client/ssh/config/shutdown_state.go (1)
14-22: Cleanup reuses the SSH manager correctly.Delegating to
RemoveSSHClientConfigwith the captured directory/file mirrors the manager API and keeps shutdown tidy.client/ssh/server/command_execution_js.go (1)
15-36: JS stub correctly flags unsupported command execution.Returning
errNotSupportedacross the executor entry points cleanly blocks wasm/js builds from invoking unimplemented flows.client/server/server_test.go (1)
314-314: BuildManager call updated with config dependency.Supplying the freshly created
configsatisfies the new signature while keeping the test harness intact.client/ssh/server/session_handlers_js.go (1)
12-21: JS PTY handler responds with explicit unsupported message.Writing the stderr notice and exiting with code 1 makes the wasm/js limitation clear without destabilizing the session.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (4)
client/cmd/ssh.go (1)
384-397: Long-form SSH flags never take effect.
--port,--user,--known-hosts, and--identityare registered withfs.Int/fs.String, which return pointers that are immediately discarded. As a result, those long-form flags never updateflags.Port,flags.Username, etc., sonetbird ssh --port 22022 hoststill dials the default port.Apply this diff to bind the aliases correctly:
fs.IntVar(&flags.Port, "p", sshserver.DefaultSSHPort, "SSH port") - fs.Int("port", sshserver.DefaultSSHPort, "SSH port") + fs.IntVar(&flags.Port, "port", sshserver.DefaultSSHPort, "SSH port") fs.StringVar(&flags.Username, "u", "", sshUsernameDesc) - fs.String("user", "", sshUsernameDesc) + fs.StringVar(&flags.Username, "user", "", sshUsernameDesc) fs.StringVar(&flags.Login, "login", "", sshUsernameDesc+" (alias for --user)") fs.BoolVar(&flags.RequestPTY, "t", false, "Force pseudo-terminal allocation") - fs.Bool("tty", false, "Force pseudo-terminal allocation") + fs.BoolVar(&flags.RequestPTY, "tty", false, "Force pseudo-terminal allocation") fs.BoolVar(&flags.StrictHostKeyChecking, "strict-host-key-checking", true, "Enable strict host key checking") fs.StringVar(&flags.KnownHostsFile, "o", "", "Path to known_hosts file") - fs.String("known-hosts", "", "Path to known_hosts file") + fs.StringVar(&flags.KnownHostsFile, "known-hosts", "", "Path to known_hosts file") fs.StringVar(&flags.IdentityFile, "i", "", "Path to SSH private key file") - fs.String("identity", "", "Path to SSH private key file") + fs.StringVar(&flags.IdentityFile, "identity", "", "Path to SSH private key file")client/ssh/server/command_execution_windows.go (1)
108-125: Unload the loaded profile before returning.Line 124 returns after
LoadUserProfileWsucceeds, but we never callUnloadUserProfileWonprofile.hProfile. That leaves the user’s registry hive mounted and leaks handles, which breaks subsequent logons after a few sessions. Please load the procedure once, deferUnloadUserProfileW(userToken, profile.hProfile)on success, and log any unload error without masking the return value.userenv := windows.NewLazySystemDLL("userenv.dll") loadUserProfileW := userenv.NewProc("LoadUserProfileW") + unloadUserProfileW := userenv.NewProc("UnloadUserProfileW") @@ if profile.lpProfilePath == nil { return "", fmt.Errorf("LoadUserProfileW returned null profile path") } + if profile.hProfile != 0 { + defer func() { + ret, _, unloadErr := unloadUserProfileW.Call(uintptr(userToken), uintptr(profile.hProfile)) + if ret == 0 { + log.Debugf("UnloadUserProfileW failed: %v", unloadErr) + } + }() + } + profilePath := windows.UTF16PtrToString(profile.lpProfilePath) return profilePath, nilclient/ssh/client/terminal_windows.go (1)
80-108: Restore console state if PTY request fails.When
session.RequestPtyon Line 107 fails, we bail out while stdout/stdin remain in VT/raw mode. The user’s console stays broken (no echo) until they restart the terminal. Restore the saved console state before returning and wrap the error for context.- return session.RequestPty("xterm-256color", h, w, modes) + if err := session.RequestPty("xterm-256color", h, w, modes); err != nil { + if restoreErr := c.restoreWindowsConsoleState(); restoreErr != nil { + log.Debugf("restore Windows console state: %v", restoreErr) + } + return fmt.Errorf("request pty: %w", err) + } + + return nilclient/ssh/client/terminal_unix.go (1)
26-61: Restore raw TTY on errors before returning.Line 60 returns
c.setupTerminalwhile the terminal is already in raw mode and the signal goroutine is running. IfRequestPtyfails, the function exits without restoring the tty, leaving the user’s shell unusable (stty sanerequired). Move thec.setupTerminalcall immediately afterterm.MakeRaw, restore the tty on error, and only storec.terminalState/spawn the watcher once the PTY is successfully negotiated.- state, err := term.MakeRaw(fd) - if err != nil { - return c.setupNonTerminalMode(ctx, session) - } - - c.terminalState = state - c.terminalFd = fd - - sigChan := make(chan os.Signal, 1) - signal.Notify(sigChan, syscall.SIGINT, syscall.SIGTERM, syscall.SIGQUIT) - - go func() { + state, err := term.MakeRaw(fd) + if err != nil { + return c.setupNonTerminalMode(ctx, session) + } + + if err := c.setupTerminal(session, fd); err != nil { + if restoreErr := term.Restore(fd, state); restoreErr != nil { + log.Debugf("restore terminal state: %v", restoreErr) + } + return err + } + + c.terminalState = state + c.terminalFd = fd + + sigChan := make(chan os.Signal, 1) + signal.Notify(sigChan, syscall.SIGINT, syscall.SIGTERM, syscall.SIGQUIT) + + go func() { defer signal.Stop(sigChan) select { case <-ctx.Done(): if err := term.Restore(fd, state); err != nil { @@ - }() - - return c.setupTerminal(session, fd) + }() + + return nil
🧹 Nitpick comments (8)
client/ssh/server/command_execution_unix.go (5)
61-76: Consider extracting the timeout constant.The 500ms timeout is hardcoded. Consider extracting it to a named constant for maintainability.
+const suDetectionTimeout = 500 * time.Millisecond + func (s *Server) detectSuPtySupport(ctx context.Context) bool { - ctx, cancel := context.WithTimeout(ctx, 500*time.Millisecond) + ctx, cancel := context.WithTimeout(ctx, suDetectionTimeout) defer cancel()
122-131: Consider extracting default terminal type.The default terminal type
"xterm-256color"is hardcoded. Consider extracting it to a named constant.+const defaultTerminalType = "xterm-256color" + func (s *Server) executeCommandWithPty(logger *log.Entry, session ssh.Session, execCmd *exec.Cmd, privilegeResult PrivilegeCheckResult, ptyReq ssh.Pty, winCh <-chan ssh.Window) bool { termType := ptyReq.Term if termType == "" { - termType = "xterm-256color" + termType = defaultTerminalType }
175-193: Consider extracting default window size constants.The default terminal dimensions (80 columns × 24 rows) are hardcoded. Consider extracting them to named constants.
+const ( + defaultTerminalCols = 80 + defaultTerminalRows = 24 +) + func (s *Server) startPtyCommandWithSize(execCmd *exec.Cmd, ptyReq ssh.Pty) (*os.File, error) { winSize := &pty.Winsize{ Cols: uint16(ptyReq.Window.Width), Rows: uint16(ptyReq.Window.Height), } if winSize.Cols == 0 { - winSize.Cols = 80 + winSize.Cols = defaultTerminalCols } if winSize.Rows == 0 { - winSize.Rows = 24 + winSize.Rows = defaultTerminalRows }
211-234: Consider adjusting log levels for expected PTY errors.The IO copy error handlers log
io.EOFandsyscall.EIOatWarnlevel, but these are often expected during normal PTY operation (e.g., when the command exits or the terminal is closed). Consider usingDebuglevel for these expected cases.if _, err := io.Copy(ptmx, session); err != nil { if !errors.Is(err, io.EOF) && !errors.Is(err, syscall.EIO) { logger.Warnf("Pty input copy error: %v", err) + } else { + logger.Debugf("Pty input copy ended: %v", err) } }Apply similar changes to the output copy handler.
251-273: Consider extracting termination timeout constant.The 5-second timeout is hardcoded. Consider extracting it to a named constant for maintainability and configurability.
+const processTerminationTimeout = 5 * time.Second + func (s *Server) handlePtySessionCancellation(logger *log.Entry, session ssh.Session, execCmd *exec.Cmd, ptyMgr *ptyManager, done <-chan error) { logger.Debugf("Pty session cancelled, terminating command") if err := ptyMgr.Close(); err != nil { logger.Debugf("Pty close during session cancellation: %v", err) } s.killProcessGroup(execCmd) select { case err := <-done: if err != nil { logger.Debugf("Pty command terminated after session cancellation with error: %v", err) } else { logger.Debugf("Pty command terminated after session cancellation") } - case <-time.After(5 * time.Second): + case <-time.After(processTerminationTimeout): - logger.Warnf("Pty command did not terminate within 5 seconds after session cancellation") + logger.Warnf("Pty command did not terminate within %v after session cancellation", processTerminationTimeout) }client/cmd/ssh.go (3)
79-81: Clarify deprecated flag handling.The
--identityflag is marked deprecated but is still parsed and passed to the SSH client (lines 441, 498). If the flag is truly deprecated and non-functional, consider removing it from the flag set entirely. If it still works but is discouraged, the deprecation message should clarify what users should use instead.
171-180: Consider unexportinggetEnvOrDefaultor consolidating withSetFlagsFromEnvVars.This function duplicates the environment variable lookup logic from
client/cmd/root.go'sSetFlagsFromEnvVars. Since it's only used within this file, either unexport it (rename togetEnvOrDefault) or refactor to share the common logic.
476-484: Document the username fallback behavior.The fallback to
"root"whenSUDO_USERanduser.Current()are unavailable is reasonable for many scenarios, but it might cause unexpected behavior on systems where root login is disabled or the user expects their local username. Consider logging a debug message when falling back to "root" to aid troubleshooting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
client/cmd/ssh.go(1 hunks)client/ssh/client/terminal_unix.go(1 hunks)client/ssh/client/terminal_windows.go(1 hunks)client/ssh/server/command_execution.go(1 hunks)client/ssh/server/command_execution_js.go(1 hunks)client/ssh/server/command_execution_unix.go(1 hunks)client/ssh/server/command_execution_windows.go(1 hunks)client/ssh/server/executor_windows.go(1 hunks)client/ssh/server/server.go(1 hunks)client/ssh/server/server_test.go(1 hunks)client/ssh/server/sftp_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (11)
client/ssh/server/sftp_test.go (2)
client/ssh/ssh.go (2)
GeneratePrivateKey(36-59)ED25519(24-24)client/ssh/server/server.go (2)
Config(143-149)New(153-165)
client/ssh/server/executor_windows.go (1)
client/ssh/server/executor_unix.go (2)
NewPrivilegeDropper(41-43)PrivilegeDropper(38-38)
client/ssh/server/command_execution.go (2)
client/ssh/server/server.go (1)
Server(108-133)client/ssh/server/user_utils.go (1)
PrivilegeCheckResult(50-66)
client/ssh/client/terminal_windows.go (1)
client/ssh/client/client.go (1)
Client(37-44)
client/ssh/server/command_execution_js.go (2)
client/ssh/server/server.go (1)
Server(108-133)client/ssh/server/user_utils.go (1)
PrivilegeCheckResult(50-66)
client/cmd/ssh.go (6)
client/ssh/server/server.go (2)
DefaultSSHPort(30-30)New(153-165)client/cmd/root.go (1)
SetFlagsFromEnvVars(198-219)util/log.go (2)
FindFirstLogPath(77-84)InitLog(32-74)client/ssh/proxy/proxy.go (1)
New(45-59)client/ssh/client/client.go (3)
Dial(284-310)DialOptions(275-281)Client(37-44)client/ssh/detection/detection.go (4)
ServerTypeRegular(31-31)Dialer(35-37)Timeout(23-23)DetectSSHServerType(59-99)
client/ssh/server/server_test.go (2)
client/ssh/ssh.go (2)
GeneratePrivateKey(36-59)ED25519(24-24)client/ssh/server/server.go (3)
Config(143-149)New(153-165)Server(108-133)
client/ssh/client/terminal_unix.go (1)
client/ssh/client/client.go (1)
Client(37-44)
client/ssh/server/server.go (5)
shared/auth/jwt/validator.go (2)
Validator(56-63)NewValidator(72-85)shared/auth/jwt/extractor.go (3)
ClaimsExtractor(34-37)NewClaimsExtractor(58-68)WithAudience(43-47)shared/auth/user.go (1)
UserAuth(7-28)client/ssh/detection/detection.go (2)
ServerIdentifier(16-16)JWTRequiredMarker(20-20)version/version.go (1)
NetbirdVersion(18-20)
client/ssh/server/command_execution_unix.go (2)
client/ssh/server/server.go (2)
New(153-165)Server(108-133)client/ssh/server/user_utils.go (1)
PrivilegeCheckResult(50-66)
client/ssh/server/command_execution_windows.go (4)
client/ssh/server/executor_windows.go (1)
NewPrivilegeDropper(45-47)client/ssh/server/executor_unix.go (1)
NewPrivilegeDropper(41-43)client/ssh/server/user_utils.go (1)
PrivilegeCheckResult(50-66)client/ssh/server/winpty/conpty.go (3)
PtyConfig(36-42)UserConfig(45-48)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). (10)
- GitHub Check: release
- GitHub Check: release_ui
- GitHub Check: Windows
- GitHub Check: Linux
- GitHub Check: Android / Build
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
- GitHub Check: Client / Unit
- GitHub Check: Build Cache
- GitHub Check: Client / Unit
🔇 Additional comments (9)
client/ssh/server/command_execution_unix.go (8)
102-120: LGTM!The shell command argument construction and environment preparation logic is clean and follows security best practices by filtering session environment variables through
acceptEnv.
133-149: LGTM!PTY handling logic correctly creates the command, handles errors gracefully with appropriate user-facing messages, and delegates to the common PTY execution path.
151-173: LGTM!The PTY command execution orchestration correctly spawns concurrent goroutines for window resize handling and bidirectional IO, while the main path waits for command completion. The deferred PTY cleanup ensures resource safety.
195-209: LGTM!Window resize handling correctly listens to resize events and updates PTY size. Debug-level logging for resize errors is appropriate since these are typically non-critical.
236-249: LGTM!The completion waiting logic correctly handles both normal command termination and session cancellation using a select statement, ensuring graceful shutdown in either case.
275-287: LGTM!Command completion handling correctly distinguishes between success and failure cases, delegating to appropriate session exit handlers.
289-293: LGTM!Process group setup correctly uses
Setpgidto create a new process group, enabling proper cleanup of the entire process tree.
36-46: Race condition in Close() method.The
Close()method has a race condition. Afteronce.Do()returns, a goroutine callingClose()will acquireRLockand readpm.closeErr. However,once.Do()only guarantees the function won't run multiple times—not that it has completed when it returns to subsequent callers. If another goroutine is currently executing the close logic insideonce.Do(), the read ofpm.closeErrraces with the write at line 40.Apply this diff to fix the race:
func (pm *ptyManager) Close() error { pm.once.Do(func() { pm.mu.Lock() + defer pm.mu.Unlock() pm.closed = true pm.closeErr = pm.file.Close() - pm.mu.Unlock() }) - pm.mu.RLock() - defer pm.mu.RUnlock() + pm.mu.RLock() + defer pm.mu.RUnlock() return pm.closeErr }Alternatively, simplify by removing the RLock after
once.Do()sinceonce.Do()already blocks until completion:func (pm *ptyManager) Close() error { + var err error pm.once.Do(func() { pm.mu.Lock() pm.closed = true - pm.closeErr = pm.file.Close() + err = pm.file.Close() + pm.closeErr = err pm.mu.Unlock() }) - pm.mu.RLock() - defer pm.mu.RUnlock() - return pm.closeErr + return err }Actually, the simplest fix:
sync.Once.Doblocks until the function completes, so subsequent calls will see the updatedcloseErr. However, reads ofpm.closeErroutside of locks are racy. The second diff above captures the error in a local variable to avoid the race.Likely an incorrect or invalid review comment.
client/cmd/ssh.go (1)
135-136: Verify environment variable handling with custom flag parsing.
SetFlagsFromEnvVarsis designed for Cobra's automatic flag parsing, but this command usesDisableFlagParsing: true(line 122) and custom parsing. Confirm that environment variables are correctly applied to SSH flags, or handle them explicitly invalidateSSHArgsWithoutFlagParsing.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
client/proto/daemon.proto (1)
641-647: Align SSH forwarding flag names in SetConfigRequest
LoginRequestalready exposesenableSSHLocalPortForwarding/enableSSHRemotePortForwarding, butSetConfigRequestdrops the “ing”. That inconsistency leaks into every generated client and is a source of bugs once the proto is frozen. Please rename these fields now and regenerate the stubs to keep the public API coherent.- optional bool enableSSHLocalPortForward = 31; - optional bool enableSSHRemotePortForward = 32; + optional bool enableSSHLocalPortForwarding = 31; + optional bool enableSSHRemotePortForwarding = 32;client/server/server.go (1)
1379-1408: Usecfginstead ofs.configto avoid nil pointer dereference.As previously flagged,
s.configcan be nil when this RPC is called before a profile is active. Thecfgvariable loaded from disk at line 1351 should be used instead to prevent a panic.Apply this diff to fix the issue:
enableSSHRoot := false - if s.config.EnableSSHRoot != nil { - enableSSHRoot = *s.config.EnableSSHRoot + if cfg.EnableSSHRoot != nil { + enableSSHRoot = *cfg.EnableSSHRoot } enableSSHSFTP := false - if s.config.EnableSSHSFTP != nil { - enableSSHSFTP = *s.config.EnableSSHSFTP + if cfg.EnableSSHSFTP != nil { + enableSSHSFTP = *cfg.EnableSSHSFTP } enableSSHLocalPortForwarding := false - if s.config.EnableSSHLocalPortForwarding != nil { - enableSSHLocalPortForwarding = *s.config.EnableSSHLocalPortForwarding + if cfg.EnableSSHLocalPortForwarding != nil { + enableSSHLocalPortForwarding = *cfg.EnableSSHLocalPortForwarding } enableSSHRemotePortForwarding := false - if s.config.EnableSSHRemotePortForwarding != nil { - enableSSHRemotePortForwarding = *s.config.EnableSSHRemotePortForwarding + if cfg.EnableSSHRemotePortForwarding != nil { + enableSSHRemotePortForwarding = *cfg.EnableSSHRemotePortForwarding } disableSSHAuth := false - if s.config.DisableSSHAuth != nil { - disableSSHAuth = *s.config.DisableSSHAuth + if cfg.DisableSSHAuth != nil { + disableSSHAuth = *cfg.DisableSSHAuth } sshJWTCacheTTL := int32(0) - if s.config.SSHJWTCacheTTL != nil { - sshJWTCacheTTL = int32(*s.config.SSHJWTCacheTTL) + if cfg.SSHJWTCacheTTL != nil { + sshJWTCacheTTL = int32(*cfg.SSHJWTCacheTTL) }
📜 Review details
Configuration used: CodeRabbit UI
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 (9)
client/cmd/status.go(1 hunks)client/internal/engine_ssh.go(1 hunks)client/internal/routemanager/manager.go(1 hunks)client/proto/daemon.proto(9 hunks)client/server/server.go(9 hunks)client/ssh/server/server.go(1 hunks)client/ssh/server/session_handlers.go(1 hunks)client/status/status.go(11 hunks)client/status/status_test.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- client/ssh/server/session_handlers.go
🧰 Additional context used
🧬 Code graph analysis (6)
client/cmd/status.go (1)
client/status/status.go (1)
ParseGeneralSummary(342-528)
client/status/status_test.go (2)
client/proto/daemon.pb.go (3)
SSHServerState(1941-1947)SSHServerState(1960-1960)SSHServerState(1975-1977)client/status/status.go (3)
SSHServerStateOutput(91-94)SSHSessionOutput(84-89)ParseGeneralSummary(342-528)
client/internal/engine_ssh.go (6)
client/ssh/server/server.go (6)
SessionInfo(161-166)New(170-184)JWTConfig(144-149)Config(152-158)InternalSSHPort(34-34)Server(115-142)client/internal/engine.go (1)
Engine(138-211)client/ssh/config/manager.go (2)
New(99-105)PeerSSHInfo(92-96)client/firewall/manager/protocol.go (1)
ProtocolTCP(9-9)shared/management/proto/management.pb.go (9)
SSHConfig(2091-2102)SSHConfig(2117-2117)SSHConfig(2132-2134)JWTConfig(1619-1628)JWTConfig(1643-1643)JWTConfig(1658-1660)RemotePeerConfig(2007-2021)RemotePeerConfig(2036-2036)RemotePeerConfig(2051-2053)client/ssh/config/shutdown_state.go (1)
ShutdownState(4-7)
client/server/server.go (4)
client/proto/daemon.pb.go (30)
FullStatus(1994-2008)FullStatus(2021-2021)FullStatus(2036-2038)SSHServerState(1941-1947)SSHServerState(1960-1960)SSHServerState(1975-1977)SSHSessionInfo(1872-1880)SSHSessionInfo(1893-1893)SSHSessionInfo(1908-1910)GetPeerSSHHostKeyRequest(4846-4852)GetPeerSSHHostKeyRequest(4865-4865)GetPeerSSHHostKeyRequest(4880-4882)GetPeerSSHHostKeyResponse(4892-4904)GetPeerSSHHostKeyResponse(4917-4917)GetPeerSSHHostKeyResponse(4932-4934)RequestJWTAuthRequest(4965-4971)RequestJWTAuthRequest(4984-4984)RequestJWTAuthRequest(4999-5001)RequestJWTAuthResponse(5011-5029)RequestJWTAuthResponse(5042-5042)RequestJWTAuthResponse(5057-5059)WaitJWTTokenRequest(5111-5119)WaitJWTTokenRequest(5132-5132)WaitJWTTokenRequest(5147-5149)WaitJWTTokenResponse(5166-5176)WaitJWTTokenResponse(5189-5189)WaitJWTTokenResponse(5204-5206)GetConfigResponse(1097-1132)GetConfigResponse(1145-1145)GetConfigResponse(1160-1162)client/ssh/server/server.go (2)
Server(115-142)New(170-184)client/internal/profilemanager/profilemanager.go (1)
GetLoginHint(137-152)client/internal/auth/oauth.go (1)
NewOAuthFlow(69-81)
client/status/status.go (1)
client/proto/daemon.pb.go (3)
SSHServerState(1941-1947)SSHServerState(1960-1960)SSHServerState(1975-1977)
client/ssh/server/server.go (6)
client/ssh/server/port_forwarding.go (3)
SessionKey(16-16)ConnectionKey(19-19)ForwardKey(22-22)shared/auth/jwt/validator.go (2)
Validator(56-63)NewValidator(72-85)shared/auth/jwt/extractor.go (3)
ClaimsExtractor(34-37)NewClaimsExtractor(58-68)WithAudience(43-47)shared/auth/user.go (1)
UserAuth(7-28)client/ssh/detection/detection.go (2)
ServerIdentifier(16-16)JWTRequiredMarker(20-20)version/version.go (1)
NetbirdVersion(18-20)
⏰ 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). (24)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Management / Benchmark (API) (amd64, sqlite)
- GitHub Check: Management / Benchmark (API) (amd64, postgres)
- GitHub Check: Management / Integration (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: Management / Integration (amd64, sqlite)
- GitHub Check: Management / Benchmark (amd64, postgres)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Signal / Unit (386)
- GitHub Check: Management / Benchmark (amd64, sqlite)
- GitHub Check: Signal / Unit (amd64)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
- GitHub Check: release_ui
- GitHub Check: release
- GitHub Check: Linux
- GitHub Check: Windows
- GitHub Check: Android / Build
🔇 Additional comments (14)
client/internal/routemanager/manager.go (1)
41-41: LGTM! Valid import reorganization.The import has been appropriately moved to group it with other internal NetBird packages, improving consistency.
client/status/status.go (4)
84-94: LGTM!The new SSH session and server state output structures are well-defined, follow existing patterns in the file, and properly use
omitemptyfor optional fields.
208-230: LGTM!The
mapSSHServerfunction correctly handles the nil case and properly maps all proto fields to the output structure.
342-342: LGTM!The SSH server status rendering logic is well-implemented with proper pluralization, clear session display format, and conditional JWT username display.
Also applies to: 447-480, 505-505, 522-522
826-833: LGTM!The anonymization logic for SSH sessions correctly handles both
host:portand plain IP addresses, and anonymizes sensitive command information.client/server/server.go (9)
49-50: LGTM!JWT cache field is properly initialized in the constructor, and the default TTL of 0 (disabled) is a safe default.
Also applies to: 88-88, 108-108
382-392: LGTM!SSH configuration flags are properly assigned from the request message, with appropriate nil checks for pointer fields.
1094-1124: LGTM!The
getSSHServerStatefunction properly handles nil checks for bothconnectClientandengine, and correctly maps session information to the proto structure.
1126-1176: LGTM!The
GetPeerSSHHostKeyRPC properly validates inputs, handles nil cases, and enriches the response with peer information when available.
1178-1197: LGTM!The
getJWTCacheTTLhelper function properly handles nil cases and provides appropriate debug logging for cache TTL configuration.
1199-1262: LGTM!The
RequestJWTAuthRPC correctly implements the JWT authentication flow with efficient cache checking, proper hint handling, and complete response construction.
1264-1305: LGTM!The
WaitJWTTokenRPC properly validates the device code, waits for token completion, conditionally caches the token, and cleans up the OAuth flow state.
1307-1312: LGTM!The
isUnixRunningDesktophelper correctly detects desktop environments on Unix systems using standard environment variables.
1664-1664: LGTM!The SSH host key is properly included in the peer state conversion to proto format.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
client/cmd/ssh.go (1)
388-401: Long-form SSH flags still don't work—critical regression.Lines 389, 391, 398, and 400 call
fs.Int/fs.Stringwithout binding the returned pointer to any variable, so--port,--user,--known-hosts, and--identityhave no effect. This is the same issue raised in a previous review (marked as addressed), but the fix was not applied. Users runningnetbird ssh --port 22022 hostwill silently connect to the default port instead.Apply this diff to bind the long-form flags correctly:
func createSSHFlagSet() (*flag.FlagSet, *sshFlags) { defaultConfigPath := getEnvOrDefault("CONFIG", configPath) defaultLogLevel := getEnvOrDefault("LOG_LEVEL", logLevel) fs := flag.NewFlagSet("ssh-flags", flag.ContinueOnError) fs.SetOutput(nil) flags := &sshFlags{} fs.IntVar(&flags.Port, "p", sshserver.DefaultSSHPort, "SSH port") - fs.Int("port", sshserver.DefaultSSHPort, "SSH port") + fs.IntVar(&flags.Port, "port", sshserver.DefaultSSHPort, "SSH port") fs.StringVar(&flags.Username, "u", "", sshUsernameDesc) - fs.String("user", "", sshUsernameDesc) + fs.StringVar(&flags.Username, "user", "", sshUsernameDesc) fs.StringVar(&flags.Login, "login", "", sshUsernameDesc+" (alias for --user)") fs.BoolVar(&flags.RequestPTY, "t", false, "Force pseudo-terminal allocation") - fs.Bool("tty", false, "Force pseudo-terminal allocation") + fs.BoolVar(&flags.RequestPTY, "tty", false, "Force pseudo-terminal allocation") fs.BoolVar(&flags.StrictHostKeyChecking, "strict-host-key-checking", true, "Enable strict host key checking") fs.StringVar(&flags.KnownHostsFile, "o", "", "Path to known_hosts file") - fs.String("known-hosts", "", "Path to known_hosts file") + fs.StringVar(&flags.KnownHostsFile, "known-hosts", "", "Path to known_hosts file") fs.StringVar(&flags.IdentityFile, "i", "", "Path to SSH private key file") - fs.String("identity", "", "Path to SSH private key file") + fs.StringVar(&flags.IdentityFile, "identity", "", "Path to SSH private key file") fs.BoolVar(&flags.SkipCachedToken, "no-cache", false, "Skip cached JWT token and force fresh authentication") fs.StringVar(&flags.ConfigPath, "c", defaultConfigPath, "Netbird config file location") - fs.String("config", defaultConfigPath, "Netbird config file location") + fs.StringVar(&flags.ConfigPath, "config", defaultConfigPath, "Netbird config file location") fs.StringVar(&flags.LogLevel, "l", defaultLogLevel, "sets Netbird log level") - fs.String("log-level", defaultLogLevel, "sets Netbird log level") + fs.StringVar(&flags.LogLevel, "log-level", defaultLogLevel, "sets Netbird log level") return fs, flags }client/ui/client_ui.go (2)
501-507: Keep the settings dialog open until a save actually succeeds.The deferred
s.wSettings.Close()still fires whenapplySettingsChangesrejects invalid input (e.g., a non-numeric SSH JWT TTL), so the window vanishes and the user loses the chance to fix the mistake—same issue as previously reported. Please close the window only after a successful save and surface the error to the user.- iMngURL := strings.TrimSpace(s.iMngURL.Text) - defer s.wSettings.Close() - - if s.hasSettingsChanged(iMngURL, port, mtu) { - s.applySettingsChanges(iMngURL, port, mtu) - } + iMngURL := strings.TrimSpace(s.iMngURL.Text) + + if !s.hasSettingsChanged(iMngURL, port, mtu) { + s.wSettings.Close() + return + } + + if err := s.applySettingsChanges(iMngURL, port, mtu); err != nil { + dialog.ShowError(err, s.wSettings) + return + } + + s.wSettings.Close()
554-567: Bubble up config-build failures instead of swallowing them.When
buildSetConfigRequestfails (again, invalid JWT TTL is an easy repro) we just log and return silently, so the caller never learns why the save didn’t happen. Please haveapplySettingsChangesreturn an error and let the caller decide how to notify the user.-func (s *serviceClient) applySettingsChanges(iMngURL string, port, mtu int64) { - s.managementURL = iMngURL - s.preSharedKey = s.iPreSharedKey.Text - s.mtu = uint16(mtu) +func (s *serviceClient) applySettingsChanges(iMngURL string, port, mtu int64) error { req, err := s.buildSetConfigRequest(iMngURL, port, mtu) if err != nil { - log.Errorf("build config request: %v", err) - return + return err } if err := s.sendConfigUpdate(req); err != nil { - dialog.ShowError(fmt.Errorf("Failed to set configuration: %v", err), s.wSettings) + return fmt.Errorf("failed to set configuration: %w", err) } + + s.managementURL = iMngURL + s.preSharedKey = s.iPreSharedKey.Text + s.mtu = uint16(mtu) + + return nil }
🧹 Nitpick comments (2)
client/cmd/ssh.go (1)
175-184: Consider reusing SetFlagsFromEnvVars logic.
getEnvOrDefaultduplicates the WT_/NB_ prefix lookup pattern already implemented inSetFlagsFromEnvVars(client/cmd/root.go lines 197-218). If this helper is only used in a few places, inlining the logic is fine, but if it grows, consider consolidating the environment variable handling to reduce maintenance overhead.client/proto/daemon.proto (1)
275-285: Field numbering is non-sequential in GetConfigResponse.Fields 21 (enableSSHRoot), 24 (enableSSHSFTP), 22 (enableSSHLocalPortForwarding), 23 (enableSSHRemotePortForwarding), 25 (disableSSHAuth), 26 (sshJWTCacheTTL) are out of order. While proto3 allows non-sequential field numbers, the unusual ordering suggests these were added incrementally. This is valid but may cause minor confusion during code review or when extending the message.
📜 Review details
Configuration used: CodeRabbit UI
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 (14)
client/cmd/ssh.go(1 hunks)client/cmd/ssh_sftp_windows.go(1 hunks)client/cmd/up.go(3 hunks)client/internal/engine_ssh.go(1 hunks)client/proto/daemon.proto(9 hunks)client/server/jwt_cache.go(1 hunks)client/server/server.go(9 hunks)client/server/setconfig_test.go(5 hunks)client/ssh/client/client.go(1 hunks)client/ssh/client/client_test.go(1 hunks)client/ssh/client/terminal_unix.go(1 hunks)client/ssh/client/terminal_windows.go(1 hunks)client/ssh/server/shell.go(1 hunks)client/ui/client_ui.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- client/ssh/server/shell.go
- client/server/jwt_cache.go
- client/internal/engine_ssh.go
- client/ssh/client/terminal_windows.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-13T00:29:53.220Z
Learnt from: lixmal
Repo: netbirdio/netbird PR: 4015
File: client/cmd/ssh_exec_unix.go:53-74
Timestamp: 2025-11-13T00:29:53.220Z
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/client/terminal_unix.goclient/ssh/client/client_test.goclient/cmd/ssh.go
🧬 Code graph analysis (7)
client/cmd/ssh_sftp_windows.go (1)
client/ssh/server/executor_windows.go (3)
ExitCodeValidationFail(27-27)ExitCodeShellExecFail(26-26)ExitCodeSuccess(22-22)
client/ssh/client/client.go (5)
client/ssh/detection/detection.go (3)
Timeout(23-23)Dialer(35-37)DetectSSHServerType(59-99)client/ssh/common.go (4)
AddJWTAuth(132-136)RequestJWTToken(71-107)NewDaemonHostKeyVerifier(43-47)CreateHostKeyCallback(140-159)client/internal/profilemanager/profilemanager.go (1)
GetLoginHint(137-152)client/proto/daemon_grpc.pb.go (1)
NewDaemonServiceClient(79-81)client/ssh/proxy/proxy.go (1)
New(45-59)
client/ssh/client/terminal_unix.go (1)
client/ssh/client/client.go (1)
Client(37-44)
client/ssh/client/client_test.go (5)
client/ssh/testutil/user_helpers.go (4)
CleanupTestUsers(87-93)GetTestUsername(20-45)IsCI(48-59)IsSystemAccount(62-76)client/ssh/ssh.go (2)
GeneratePrivateKey(36-59)ED25519(24-24)client/ssh/server/server.go (3)
Config(152-158)New(170-184)Server(115-142)client/ssh/server/test.go (1)
StartTestServer(12-45)client/ssh/client/client.go (3)
Dial(284-310)DialOptions(275-281)Client(37-44)
client/ui/client_ui.go (2)
client/iface/iface.go (2)
MinMTU(29-29)MaxMTU(30-30)client/proto/daemon.pb.go (3)
SetConfigRequest(3918-3959)SetConfigRequest(3972-3972)SetConfigRequest(3987-3989)
client/cmd/ssh.go (6)
client/ssh/server/server.go (2)
DefaultSSHPort(31-31)New(170-184)client/cmd/root.go (1)
SetFlagsFromEnvVars(198-219)util/log.go (2)
FindFirstLogPath(77-84)InitLog(32-74)client/ssh/proxy/proxy.go (1)
New(45-59)client/ssh/client/client.go (3)
Dial(284-310)DialOptions(275-281)Client(37-44)client/ssh/detection/detection.go (4)
ServerTypeRegular(31-31)Dialer(35-37)Timeout(23-23)DetectSSHServerType(59-99)
client/server/server.go (5)
client/proto/daemon.pb.go (30)
FullStatus(1994-2008)FullStatus(2021-2021)FullStatus(2036-2038)SSHServerState(1941-1947)SSHServerState(1960-1960)SSHServerState(1975-1977)SSHSessionInfo(1872-1880)SSHSessionInfo(1893-1893)SSHSessionInfo(1908-1910)GetPeerSSHHostKeyRequest(4846-4852)GetPeerSSHHostKeyRequest(4865-4865)GetPeerSSHHostKeyRequest(4880-4882)GetPeerSSHHostKeyResponse(4892-4904)GetPeerSSHHostKeyResponse(4917-4917)GetPeerSSHHostKeyResponse(4932-4934)RequestJWTAuthRequest(4965-4971)RequestJWTAuthRequest(4984-4984)RequestJWTAuthRequest(4999-5001)RequestJWTAuthResponse(5011-5029)RequestJWTAuthResponse(5042-5042)RequestJWTAuthResponse(5057-5059)WaitJWTTokenRequest(5111-5119)WaitJWTTokenRequest(5132-5132)WaitJWTTokenRequest(5147-5149)WaitJWTTokenResponse(5166-5176)WaitJWTTokenResponse(5189-5189)WaitJWTTokenResponse(5204-5206)GetConfigResponse(1097-1132)GetConfigResponse(1145-1145)GetConfigResponse(1160-1162)client/ssh/server/server.go (2)
Server(115-142)New(170-184)client/internal/engine.go (1)
Engine(138-211)client/internal/profilemanager/profilemanager.go (1)
GetLoginHint(137-152)client/internal/auth/oauth.go (1)
NewOAuthFlow(69-81)
⏰ 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: Android / Build
- GitHub Check: iOS / Build
- GitHub Check: JS / Lint
- GitHub Check: Client / Unit
- GitHub Check: Build Cache
- GitHub Check: release_ui
- GitHub Check: release
- GitHub Check: release_ui_darwin
- GitHub Check: Linux
- GitHub Check: Windows
- GitHub Check: Client / Unit
- GitHub Check: Darwin
- GitHub Check: Client / Unit
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (5)
client/ssh/client/client.go (1)
283-310: Critical: Daemon address resolved too late for host key verification.This issue was flagged in a previous review but remains unfixed.
createHostKeyCallbackis invoked at line 285 before deriving the default daemon address (lines 304-307), soopts.DaemonAddris empty when the callback closure is created. Consequently,tryDaemonVerificationat line 447 will always bail out with "no daemon address" and skip daemon-backed host-key verification, falling back to local known_hosts only. This reproduces the "key not found in NetBird daemon or any known_hosts file" failure reported after toggling SSH in the PR objectives.Apply this diff to resolve the daemon address before creating the callback:
func Dial(ctx context.Context, addr, user string, opts DialOptions) (*Client, error) { + daemonAddr := opts.DaemonAddr + if daemonAddr == "" { + daemonAddr = getDefaultDaemonAddr() + } + opts.DaemonAddr = daemonAddr + hostKeyCallback, err := createHostKeyCallback(opts) if err != nil { return nil, fmt.Errorf("create host key callback: %w", err) } config := &ssh.ClientConfig{ User: user, Timeout: 30 * time.Second, HostKeyCallback: hostKeyCallback, } if opts.IdentityFile != "" { authMethod, err := createSSHKeyAuth(opts.IdentityFile) if err != nil { return nil, fmt.Errorf("create SSH key auth: %w", err) } config.Auth = append(config.Auth, authMethod) } - daemonAddr := opts.DaemonAddr - if daemonAddr == "" { - daemonAddr = getDefaultDaemonAddr() - } - return dialWithJWT(ctx, "tcp", addr, config, daemonAddr, opts.SkipCachedToken) }client/ssh/proxy/proxy.go (2)
95-98: Keep the proxy host key stable across runs.Generating a new host key on every proxy invocation causes native SSH clients to detect a host key change and fail with "REMOTE HOST IDENTIFICATION HAS CHANGED!" errors. This issue was previously flagged and remains unresolved.
As suggested in the prior review, persist a single proxy host key (e.g., in NetBird's config directory) or generate a deterministic key so native clients see a consistent host identity across sessions.
Also applies to: 174-186
147-152: Usesession.RawCommand()to preserve command quoting and signal success explicitly.While exit code handling has been partially added (line 150 calls
handleProxyExitCode), the command execution still has issues:
strings.Join(session.Command(), " ")loses original shell quoting, breaking commands with quotes or special characters.- Successful command completion (nil error) doesn't explicitly call
session.Exit(0), so the client may not receive the correct exit status.Apply the previously suggested fix using
session.RawCommand():- if len(session.Command()) > 0 { - if err := serverSession.Run(strings.Join(session.Command(), " ")); err != nil { + if rawCmd := session.RawCommand(); rawCmd != "" { + if err := serverSession.Run(rawCmd); err != nil { log.Debugf("run command: %v", err) p.handleProxyExitCode(session, err) + return } + _ = session.Exit(0) return }Similarly, ensure shell execution signals success:
if err = serverSession.Shell(); err != nil { log.Debugf("start shell: %v", err) return } if err := serverSession.Wait(); err != nil { log.Debugf("session wait: %v", err) p.handleProxyExitCode(session, err) + return } + _ = session.Exit(0)client/ssh/server/winpty/conpty_test.go (1)
28-32: Update cmd.exe expectation to use/C.This test is still asserting the PowerShell-only
-Commandflag forcmd.exe. Native cmd needs/C, otherwise the launched process terminates with a syntax error. Please switch the expectation (and the implementation) to/Cso we don’t regress on Windows command execution.Apply this diff:
{ name: "CMD with command", shell: "cmd.exe", command: "dir", - expected: []string{"cmd.exe", "-Command", "dir"}, + expected: []string{"cmd.exe", "/C", "dir"},client/ssh/server/command_execution_windows.go (1)
106-123: Unload user profiles afterLoadUserProfileW.
LoadUserProfileWmounts the user’s registry hive and hands back a profile handle. We never callUnloadUserProfileW, so every SSH session leaks a hive and prevents clean logoff. Please unload the profile before returning.Apply this diff:
userenv := windows.NewLazySystemDLL("userenv.dll") loadUserProfileW := userenv.NewProc("LoadUserProfileW") + unloadUserProfileW := userenv.NewProc("UnloadUserProfileW") @@ if profile.lpProfilePath == nil { return "", fmt.Errorf("LoadUserProfileW returned null profile path") } + if profile.hProfile != 0 { + defer func() { + if ret, _, unloadErr := unloadUserProfileW.Call(uintptr(userToken), uintptr(profile.hProfile)); ret == 0 { + log.Debugf("UnloadUserProfileW failed: %v", unloadErr) + } + }() + } + profilePath := windows.UTF16PtrToString(profile.lpProfilePath) return profilePath, nil
🧹 Nitpick comments (2)
client/ui/client_ui.go (1)
645-665: Consider adding user feedback for reconnection.The async reconnection logic (lines 645-665) silently handles errors with logging only. If reconnection fails (e.g., daemon unresponsive), the user won't know whether the new settings were applied. Consider showing a notification or status message to inform the user of reconnection success or failure.
client/ssh/client/client.go (1)
165-177: Consider extracting the termination timeout constant.Both
ExecuteCommandWithIOandExecuteCommandWithPTYuse a hardcoded 100ms timeout (lines 171 and 210) after sending SIGTERM on context cancellation. Extracting this as a named constant would clarify its purpose and make it easier to tune if needed.Example:
+const ( + gracefulTerminationTimeout = 100 * time.Millisecond +) + // ExecuteCommandWithIO executes a command with interactive I/O connected to local terminal func (c *Client) ExecuteCommandWithIO(ctx context.Context, command string) error { // ... select { case <-ctx.Done(): _ = session.Signal(ssh.SIGTERM) select { case <-done: return ctx.Err() - case <-time.After(100 * time.Millisecond): + case <-time.After(gracefulTerminationTimeout): return ctx.Err() } case err := <-done: return c.handleCommandError(err) } }Apply the same change to line 210 in
ExecuteCommandWithPTY.Also applies to: 204-216
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
client/cmd/ssh.go(1 hunks)client/ssh/client/client.go(1 hunks)client/ssh/proxy/proxy.go(1 hunks)client/ssh/server/command_execution.go(1 hunks)client/ssh/server/command_execution_js.go(1 hunks)client/ssh/server/command_execution_unix.go(1 hunks)client/ssh/server/command_execution_windows.go(1 hunks)client/ssh/server/executor_windows.go(1 hunks)client/ssh/server/server_test.go(1 hunks)client/ssh/server/sftp_windows.go(1 hunks)client/ssh/server/userswitching_unix.go(1 hunks)client/ssh/server/userswitching_windows.go(1 hunks)client/ssh/server/winpty/conpty.go(1 hunks)client/ssh/server/winpty/conpty_test.go(1 hunks)client/ui/client_ui.go(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- client/ssh/server/command_execution_js.go
- client/cmd/ssh.go
- client/ssh/server/userswitching_unix.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-13T00:29:53.220Z
Learnt from: lixmal
Repo: netbirdio/netbird PR: 4015
File: client/cmd/ssh_exec_unix.go:53-74
Timestamp: 2025-11-13T00:29:53.220Z
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/server/sftp_windows.goclient/ssh/proxy/proxy.goclient/ssh/server/userswitching_windows.goclient/ssh/server/executor_windows.goclient/ssh/server/command_execution_windows.goclient/ssh/server/command_execution.goclient/ssh/server/command_execution_unix.go
🧬 Code graph analysis (10)
client/ssh/server/server_test.go (2)
client/ssh/ssh.go (2)
GeneratePrivateKey(36-59)ED25519(24-24)client/ssh/server/server.go (3)
Config(152-158)New(170-184)Server(115-142)
client/ui/client_ui.go (3)
client/server/server.go (1)
New(99-110)client/iface/iface.go (2)
MinMTU(29-29)MaxMTU(30-30)client/proto/daemon.pb.go (3)
SetConfigRequest(3918-3959)SetConfigRequest(3972-3972)SetConfigRequest(3987-3989)
client/ssh/server/sftp_windows.go (2)
client/ssh/server/server.go (1)
Server(115-142)client/ssh/server/executor_windows.go (1)
NewPrivilegeDropper(45-47)
client/ssh/client/client.go (5)
client/ssh/detection/detection.go (3)
Timeout(23-23)Dialer(35-37)DetectSSHServerType(59-99)client/ssh/common.go (4)
AddJWTAuth(132-136)RequestJWTToken(71-107)NewDaemonHostKeyVerifier(43-47)CreateHostKeyCallback(140-159)client/internal/profilemanager/profilemanager.go (1)
GetLoginHint(137-152)client/proto/daemon_grpc.pb.go (1)
NewDaemonServiceClient(79-81)client/ssh/proxy/proxy.go (1)
New(45-59)
client/ssh/proxy/proxy.go (5)
client/proto/daemon_grpc.pb.go (2)
DaemonServiceClient(20-73)NewDaemonServiceClient(79-81)client/internal/profilemanager/profilemanager.go (1)
GetLoginHint(137-152)client/ssh/common.go (3)
RequestJWTToken(71-107)NewDaemonHostKeyVerifier(43-47)CreateHostKeyCallback(140-159)client/ssh/detection/detection.go (1)
ProxyIdentifier(18-18)version/version.go (1)
NetbirdVersion(18-20)
client/ssh/server/userswitching_windows.go (3)
client/ssh/server/server.go (1)
Server(115-142)client/ssh/server/executor_windows.go (2)
WindowsExecutorConfig(30-41)NewPrivilegeDropper(45-47)client/ssh/server/executor_unix.go (1)
NewPrivilegeDropper(41-43)
client/ssh/server/executor_windows.go (1)
client/ssh/server/server.go (2)
New(170-184)Server(115-142)
client/ssh/server/command_execution_windows.go (3)
client/ssh/server/executor_windows.go (1)
NewPrivilegeDropper(45-47)client/ssh/server/user_utils.go (1)
PrivilegeCheckResult(50-66)client/ssh/server/winpty/conpty.go (3)
PtyConfig(36-42)UserConfig(45-48)ExecutePtyWithUserToken(59-71)
client/ssh/server/command_execution.go (2)
client/ssh/server/server.go (1)
Server(115-142)client/ssh/server/user_utils.go (1)
PrivilegeCheckResult(50-66)
client/ssh/server/command_execution_unix.go (2)
client/ssh/server/server.go (2)
New(170-184)Server(115-142)client/ssh/server/user_utils.go (1)
PrivilegeCheckResult(50-66)
🔇 Additional comments (28)
client/ui/client_ui.go (5)
478-511: LGTM! Error handling properly addressed.The refactored
saveSettingsmethod correctly addresses the past review concern about keeping the dialog open on validation failures. Each error path (lines 491, 497, 505) now shows a dialog and returns early, only closing the window after successful save.
441-446: LGTM! SSH UI elements properly initialized.The SSH-related UI controls (checkboxes for server features and entry for JWT cache TTL) are properly initialized with descriptive labels.
670-690: LGTM! Clean tabbed interface refactor.The settings form has been successfully refactored from a single form to a multi-tab layout (Connection, Network, SSH), improving organization and scalability. Save/Cancel button wiring is correct.
1252-1309: LGTM! Defensive nil checks for SSH config fields.The nil checks for SSH configuration fields (lines 1252-1269 and 1291-1308) ensure safe handling when fields are not present. While the pattern is repetitive, it's correct and maintainable.
1379-1386: LGTM! SSH field mapping is correct.The SSH configuration fields are properly mapped from the proto response to the config struct, including appropriate type conversion for the JWT cache TTL (int32 to int).
client/ssh/client/client.go (1)
505-536: LGTM: Local listener properly closed on context cancellation.The previous review concern about the listener not being closed when the context ends has been addressed. Line 532 explicitly closes
localListenerafter<-ctx.Done(), and line 514 includes anerrors.Is(err, net.ErrClosed)guard in the deferred close to handle the case where the listener has already been closed. This prevents both the goroutine leak and the hanging port binding.client/ssh/proxy/proxy.go (1)
188-240: LGTM: Helper implementations are well-structured.The
stdioConnimplementation correctly wraps stdin/stdout as anet.Connwith proper synchronization. The SFTP bridge properly handles bidirectional copying with context cancellation and waits for both goroutines. Port forwarding handlers appropriately reject forwarding requests (consistent with proxy-only mode), anddialBackendcorrectly configures JWT authentication with appropriate timeouts and host key verification.Also applies to: 242-244, 246-352, 354-383
client/ssh/server/userswitching_windows.go (4)
18-87: LGTM - Comprehensive Windows username validationThe username validation implementation correctly enforces SAM Account Name rules including length limits (20 chars), invalid character checks, control character rejection, and format validation. The extraction of username from domain formats before validation is appropriate.
141-157: LGTM - parseUsername correctly handles Windows domain formatsThe function appropriately handles both
DOMAIN\usernameandusername@domainformats, defaulting to "." for local users.
159-248: LGTM - Privilege management follows Windows API patterns correctlyThe double-call pattern in
hasPrivilege(first with null buffer to get size, then with allocated buffer) and the adjust-then-verify pattern inenablePrivilegeare standard and correct for Windows privilege management APIs.
91-100: Cleanup callback is properly invoked—no issues foundThe cleanup callback returned by
createExecutorCommandis correctly deferred and invoked in both code paths:
- Non-PTY path (line 46): cleanup is passed to
executeCommand, which defers it at line 97. It executes after the process completes.- PTY path (line 52): cleanup is deferred directly in
handleCommand. It executes afterexecuteCommandWithPtycompletes.Both defer statements execute after
execCmd.Start()(line 120), ensuring the token handle remains open during process execution and is properly closed afterward. There are no handle leaks.client/ssh/server/executor_windows.go (3)
64-92: LGTM - Command creation with proper validationThe function validates required fields (username, shell) before proceeding and correctly delegates to
CreateWindowsProcessAsUserfor token-based process creation.
181-455: LGTM - S4U authentication implementation follows Windows patternsThe S4U (Service for User) logon implementation correctly:
- Separates domain and local user paths (Kerberos vs MSV1_0)
- Uses appropriate authentication packages
- Translates SAM names to UPN format for domain users
- Constructs logon structures with proper buffer management (aside from the issue flagged separately)
- Follows the same trust-based approach as OpenSSH for Windows
519-565: LGTM - Token lifecycle management pattern is correctThe token management pattern is sound:
CreateWindowsProcessAsUsercreates an impersonation token, defers its closure, duplicates it to a primary token viacreateProcessWithToken, and returns the primary tokencreateProcessWithTokenduplicates the source token to create an independent primary token handle stored incmd.SysProcAttr.Token- The primary token is returned to the caller with clear documentation (lines 520, 542) that the caller must close it after starting the process
The impersonation token's defer-close at lines 527-531 is safe because
DuplicateTokenExcreates an independent handle. Callers in bothuserswitching_windows.goandsftp_windows.goproperly close the returned primary token after process startup.client/ssh/server/sftp_windows.go (2)
17-53: LGTM - Token lifecycle correctly managedThe token handling pattern is sound and contradicts the concern in past review comments:
- Line 35: Creates impersonation token
- Lines 40-44: Defers closing the impersonation token (safe to close after duplication)
- Line 46:
createProcessWithTokencallsDuplicateTokenEx, creating an independent primary token handle- Line 52: Returns the primary token to the caller
The impersonation token's defer-close is correct because
DuplicateTokenExproduces a new, independent handle. The primary token remains valid and is properly closed byexecuteSftpCommandafter the process starts.
55-82: LGTM - Process execution with proper resource cleanupThe function correctly:
- Defers closing the primary token after process completion (lines 57-61)
- Wires SSH session I/O to the SFTP command (lines 63-65)
- Starts and waits for the process (lines 67-79)
- Treats
ExitErroras non-fatal (lines 72-76), appropriate for SFTP client-initiated disconnectsThe token is closed after
Wait()returns, ensuring the handle remains valid throughout the process lifetime.client/ssh/server/command_execution_unix.go (12)
23-34: LGTM! Thread-safe PTY lifecycle management.The
ptyManageruses appropriate synchronization primitives (sync.Oncefor idempotent close,sync.RWMutexfor concurrent access) to safely manage the PTY file lifecycle across multiple goroutines.
36-46: LGTM! Idempotent close with proper synchronization.The use of
sync.Onceensures the file is closed exactly once, even under concurrent calls, and the cachedcloseErris safely returned to all callers.
48-59: LGTM! Setsize is thread-safe; File() relies on controlled lifecycle.
Setsizecorrectly uses read-locking and validates the closed state.File()returns the internal file without locking, which is safe given thatrunPtyCommandblocks inwaitForPtyCompletionbefore closing the PTY (line 164), ensuring the file remains valid during I/O operations.
61-76: LGTM! Reasonable heuristic for su --pty detection.The method uses a short timeout and falls back gracefully when
su --helpfails, which is appropriate since not allsuimplementations support--help.
102-120: LGTM! Clean environment preparation helpers.Both methods provide clear, focused logic for building shell command arguments and preparing the command environment with proper filtering via
acceptEnv.
122-149: LGTM! Clean PTY command setup and error handling.Both methods properly initialize PTY commands with sensible defaults (TERM fallback) and handle errors gracefully by writing user-friendly messages to stderr before exiting.
151-193: LGTM! Well-structured PTY lifecycle orchestration.
runPtyCommandproperly coordinates the PTY lifecycle with deferred cleanup, goroutine-based window resize handling, and I/O management.startPtyCommandWithSizeprovides sensible fallback dimensions (80×24).
195-209: LGTM! Proper window resize handling with graceful error handling.The method correctly uses
selectto handle both session cancellation and channel closure, and appropriately logs resize errors without failing the session.
211-234: LGTM! Standard bidirectional PTY I/O with appropriate error filtering.The two goroutines handle I/O independently and correctly filter
io.EOFandsyscall.EIO(both expected when a PTY is closed), while warning on unexpected errors. The output goroutine's deferredsession.Close()ensures proper cleanup.
236-287: LGTM! Robust command completion handling with proper cancellation.The select correctly discriminates between normal completion and session cancellation. The cancellation path properly closes the PTY first (to unblock I/O goroutines) before killing the process group, and uses a reasonable 5-second timeout. Exit code 130 is standard for SIGINT/SIGTERM.
289-293: LGTM! Correct process group setup for kill propagation.Setting
Setpgid: trueensures the command and all its children are in a new process group, which allowskillProcessGroupto terminate the entire tree.
295-329: LGTM! Improved graceful shutdown with polling.The implementation now correctly polls for process exit (every 50ms using
kill(pgid, 0)) rather than unconditionally blocking, and only sendsSIGKILLafter the 500ms grace period if the process group is still alive. This addresses the previous review concern about always waiting even when processes exit quickly.
@coderabbitai let's not review code outside of the PR context |
|
Tip For best results, initiate chat on the files or code changes. @mlsmaycon Understood! I'll focus my reviews exclusively on the code changes within the PR diff. I won't review or comment on code outside the PR context. Thank you for the clarification. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
client/ssh/server/command_execution.go (1)
60-93: UsedFallback logic still attempts unnecessary su command.Line 77 checks
privilegeResult.UsedFallbackto fallback to executor, butcreateSuCommandis already called at line 76 before this check. According to thePrivilegeCheckResultdocumentation, "WhenUsedFallbackis true, all privilege checks have already been performed and no additional privilege dropping or root checks are needed."The code should check
UsedFallback(orRequiresUserSwitching) before attemptingcreateSuCommandto avoid unnecessary work and align with the semantic intent.Apply this diff to check upfront:
- // Try su first for system integration (PAM/audit) when privileged - cmd, err := s.createSuCommand(session, localUser, hasPty) - if err != nil || privilegeResult.UsedFallback { - log.Debugf("su command failed, falling back to executor: %v", err) + var cmd *exec.Cmd + var err error + + // When no user switching is needed (UsedFallback), go directly to executor + if privilegeResult.UsedFallback { + log.Debugf("Using executor directly (no user switching needed)") cmd, cleanup, err := s.createExecutorCommand(session, localUser, hasPty) if err != nil { return nil, nil, fmt.Errorf("create command with privileges: %w", err) } cmd.Env = s.prepareCommandEnv(localUser, session) return cmd, cleanup, nil } - - if err != nil { - return nil, nil, fmt.Errorf("create command with privileges: %w", err) + + // Try su first for system integration (PAM/audit) when privileged + cmd, err = s.createSuCommand(session, localUser, hasPty) + if err != nil { + log.Debugf("su command failed, falling back to executor: %v", err) + cmd, cleanup, err := s.createExecutorCommand(session, localUser, hasPty) + if err != nil { + return nil, nil, fmt.Errorf("create command with privileges: %w", err) + } + cmd.Env = s.prepareCommandEnv(localUser, session) + return cmd, cleanup, nil } cmd.Env = s.prepareCommandEnv(localUser, session) return cmd, func() {}, nilclient/ssh/proxy/proxy.go (2)
104-108: Regenerating host keys breaks native OpenSSH clients.A fresh ED25519 key is generated on every proxy invocation. Native OpenSSH/SFTP clients record this key in
known_hosts, but on the next connection a different key is presented, triggering "REMOTE HOST IDENTIFICATION HAS CHANGED!" errors. Please persist a single proxy host key (e.g., cache it under NetBird's data directory) to provide a consistent identity across sessions.
156-172: Command quoting still broken despite exit code fix.Exit status propagation has been improved with
handleProxyExitCode, but the command quoting issue remains.strings.Join(session.Command(), " ")at line 157 drops original quoting, so commands likessh host 'echo "hi there"'are mangled. Usesession.RawCommand()to preserve the exact command string the client sent.Apply this diff:
- if len(session.Command()) > 0 { - if err := serverSession.Run(strings.Join(session.Command(), " ")); err != nil { + if rawCmd := session.RawCommand(); rawCmd != "" { + if err := serverSession.Run(rawCmd); err != nil { log.Debugf("run command: %v", err) p.handleProxyExitCode(session, err) } return }client/ssh/server/executor_windows.go (2)
320-357: Domain S4U UPN buffer sizing fix looks correctThe new
prepareDomainS4ULogonimplementation now copies the UPN into a[1025]uint16buffer, matching themaxUPNLen+1sizing used forTranslateNameW, so the previous risk of overrunning a[512]uint16buffer is resolved. TheunicodeStringlengths also correctly use(len(upnUtf16)-1)*2forLengthandlen(upnUtf16)*2forMaximumLength, which matchesUNICODE_STRINGsemantics for a UTF-16 UPN payload.No further changes needed here from a safety perspective.
457-539: Token lifetime handling now separates impersonation vs. primary token; ensure all call sites close the primary tokenThe refactored flow:
CreateWindowsProcessAsUserobtains an S4U impersonation token viacreateToken, defersCloseHandle(token), and thencreateProcessWithTokenduplicates it to a primary token, attaches it tocmd.SysProcAttr.Token, and returnscmdplus theprimaryToken.This addresses the earlier leak of the duplicated primary token: the impersonation token is closed internally, and the returned
windows.Tokenis the primary token that must be closed by the caller after the child process has been started/finished.Two follow‑ups to keep this robust:
- Make sure every call site that receives the
windows.TokenfromCreateWindowsExecutorCommand/CreateWindowsProcessAsUserdoes adefer windows.CloseHandle(windows.Handle(primaryToken))(or equivalent) after creating the command and before callingStart/Wait.- To reduce the chance of future leaks, you might consider a tiny helper wrapper (e.g., a struct that owns
cmd+ token and exposesRun()/Wait()while closing the token internally once done), so callers can’t forget the cleanup.You can quickly verify that all call sites are closing the returned token with something like:
#!/bin/bash # Search for Windows executor usage and inspect token handling around it. rg -n 'CreateWindowsExecutorCommand\(' -C4 rg -n 'CreateWindowsProcessAsUser\(' -C4Also applies to: 541-565
🧹 Nitpick comments (7)
client/ssh/server/executor_windows.go (1)
181-207: Consider converting NTSTATUS error values to Win32 error codes for improved error diagnosticsThe S4U authentication path is well-structured and functionally correct. However, LsaLogonUser and other LSA APIs return NTSTATUS values that can be mapped to familiar Win32 ERROR_* codes for easier logging, searching, and display for operators and support teams.
Currently, error logging at lines 216–261 (initializeLsaConnection, cleanupLsaConnection, lookupAuthenticationPackage) and 408–455 (performS4ULogon) outputs raw NTSTATUS hex values (e.g.,
0x%x). When logging or returning user-facing error messages, call LsaNtStatusToWinError or RtlNtStatusToDosError to produce Win32 error codes, while preserving NTSTATUS/SubStatus for control-flow logic when needed.client/ui/client_ui.go (6)
56-59: TTL max constant is reasonable; consider centralizing limitThe 24h
maxSSHJWTCacheTTLcap is sensible and prevents pathological cache durations. If other layers (CLI flags, daemon config) also impose TTL limits, consider sharing a single constant to avoid future drift in allowed ranges.
457-477: Connection tab refactor is clean; optional UX tweakSplitting out
getConnectionFormand showing the active profile name is straightforward and improves structure. If you want to avoid a blank “Profile” row on errors, you could populate it with a fallback like “(unknown)” instead of an empty label, but this is purely cosmetic.
479-512: Save flow and PSK validation behaviorThe new
saveSettingsflow correctly:
- Checks daemon feature flags,
- Validates the pre-shared key,
- Parses numeric fields,
- Keeps the dialog open and surfaces errors instead of closing on failure.
validateSettings’s use ofcensoredPreSharedKeymakes sense only if some code path ever setsiPreSharedKey.Textto that sentinel. In this file, the entry is always populated with the real key or empty text, so the sentinel-based special casing may now be redundant.Apply either:
- Wire
censoredPreSharedKeyinto the UI when you want to represent a stored-but-hidden PSK, or- Drop the sentinel checks and treat empty string as “no PSK / unchanged” to simplify the flow.
Also applies to: 514-521
523-545: Port and MTU validation look good; minor robustness tweakThe interface port is now range-checked (1–65535) and MTU is validated against
iface.MinMTU/iface.MaxMTU, which addresses earlier validation gaps.You might optionally
TrimSpaceon the port text (as you already do for MTU) so values like" 51820 "are accepted without confusing the user.
547-560: Baseline vs change detection after successful/failed savesThe combination of
hasSettingsChanged,hasSSHChanges, andapplySettingsChangesworks, but the baseline state updates are asymmetric:
applySettingsChangesupdates onlymanagementURL,preSharedKey, andmtubefore building/sending the request, and never updates the other tracked fields (RosenpassPermissive, interface name/port, network flags, SSH flags,sshJWTCacheTTL).- If
buildSetConfigRequestorsendConfigUpdatefails, the few fields you do overwrite no longer match the actual daemon config, so subsequent saves may treat them as “unchanged” even though the daemon still has the old values.- After a successful save, the non-updated baseline fields mean
hasSettingsChanged/hasSSHChangescan continue to report changes and trigger redundantSetConfig+ reconnect cycles.This isn’t breaking, but it’s subtle and could cause unnecessary reconnects or misleading state in edge error cases.
Consider updating all tracked baseline fields only after
sendConfigUpdatesucceeds (or by re-callinggetSrvConfigon success), e.g.:func (s *serviceClient) applySettingsChanges(iMngURL string, port, mtu int64) error { - s.managementURL = iMngURL - s.preSharedKey = s.iPreSharedKey.Text - s.mtu = uint16(mtu) - req, err := s.buildSetConfigRequest(iMngURL, port, mtu) if err != nil { return fmt.Errorf("build config request: %w", err) } if err := s.sendConfigUpdate(req); err != nil { return fmt.Errorf("set configuration: %w", err) } + + // Only update baselines after a successful apply. + s.managementURL = iMngURL + s.preSharedKey = s.iPreSharedKey.Text + s.mtu = uint16(mtu) + s.RosenpassPermissive = s.sRosenpassPermissive.Checked + s.interfaceName = s.iInterfaceName.Text + s.interfacePort = int(port) + s.networkMonitor = s.sNetworkMonitor.Checked + s.disableDNS = s.sDisableDNS.Checked + s.disableClientRoutes = s.sDisableClientRoutes.Checked + s.disableServerRoutes = s.sDisableServerRoutes.Checked + s.blockLANAccess = s.sBlockLANAccess.Checked + s.enableSSHRoot = s.sEnableSSHRoot.Checked + s.enableSSHSFTP = s.sEnableSSHSFTP.Checked + s.enableSSHLocalPortForward = s.sEnableSSHLocalPortForward.Checked + s.enableSSHRemotePortForward = s.sEnableSSHRemotePortForward.Checked + s.disableSSHAuth = s.sDisableSSHAuth.Checked + if text := strings.TrimSpace(s.iSSHJWTCacheTTL.Text); text != "" { + if val, err := strconv.Atoi(text); err == nil { + s.sshJWTCacheTTL = val + } + } + return nil }(Adjust fields as needed, or alternatively re-populate everything via
getSrvConfigon success.)Also applies to: 562-577, 721-737
1258-1275: SSH fields and TTL are correctly round-tripped in getSrvConfig
getSrvConfignow:
- Copies SSH-related fields and TTL from
profilemanager.Configinto the internal baseline booleans/int.- When advanced settings are shown, it sets the SSH checkboxes and TTL entry text from the same config.
This keeps the UI in sync with daemon config on window open. Given
protoConfigToConfigalways assignsSSHJWTCacheTTL, thenilchecks around TTL are slightly defensive, but harmless.Also applies to: 1297-1314
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
client/cmd/ssh.go(1 hunks)client/ssh/client/client.go(1 hunks)client/ssh/client/terminal_unix.go(1 hunks)client/ssh/proxy/proxy.go(1 hunks)client/ssh/server/command_execution.go(1 hunks)client/ssh/server/command_execution_unix.go(1 hunks)client/ssh/server/executor_windows.go(1 hunks)client/ui/client_ui.go(7 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-13T00:29:53.220Z
Learnt from: lixmal
Repo: netbirdio/netbird PR: 4015
File: client/cmd/ssh_exec_unix.go:53-74
Timestamp: 2025-11-13T00:29:53.220Z
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/client/terminal_unix.goclient/ssh/server/executor_windows.goclient/cmd/ssh.goclient/ssh/proxy/proxy.goclient/ssh/server/command_execution.goclient/ssh/server/command_execution_unix.go
🧬 Code graph analysis (7)
client/ssh/client/client.go (5)
client/ssh/detection/detection.go (3)
Timeout(23-23)Dialer(35-37)DetectSSHServerType(59-99)client/ssh/common.go (4)
AddJWTAuth(132-136)RequestJWTToken(71-107)NewDaemonHostKeyVerifier(43-47)CreateHostKeyCallback(140-159)client/internal/profilemanager/profilemanager.go (1)
GetLoginHint(137-152)client/proto/daemon_grpc.pb.go (1)
NewDaemonServiceClient(79-81)client/ssh/proxy/proxy.go (1)
New(46-61)
client/ssh/client/terminal_unix.go (1)
client/ssh/client/client.go (1)
Client(37-44)
client/cmd/ssh.go (6)
client/ssh/server/server.go (2)
DefaultSSHPort(31-31)New(170-184)client/cmd/root.go (1)
SetFlagsFromEnvVars(198-219)util/log.go (2)
FindFirstLogPath(77-84)InitLog(32-74)client/ssh/proxy/proxy.go (1)
New(46-61)client/ssh/client/client.go (3)
Dial(284-311)DialOptions(275-281)Client(37-44)client/ssh/detection/detection.go (4)
ServerTypeRegular(31-31)Dialer(35-37)Timeout(23-23)DetectSSHServerType(59-99)
client/ssh/proxy/proxy.go (5)
client/proto/daemon_grpc.pb.go (1)
DaemonServiceClient(20-73)client/internal/profilemanager/profilemanager.go (1)
GetLoginHint(137-152)client/ssh/common.go (3)
RequestJWTToken(71-107)NewDaemonHostKeyVerifier(43-47)CreateHostKeyCallback(140-159)client/ssh/detection/detection.go (1)
ProxyIdentifier(18-18)version/version.go (1)
NetbirdVersion(18-20)
client/ssh/server/command_execution.go (2)
client/ssh/server/server.go (1)
Server(115-142)client/ssh/server/user_utils.go (1)
PrivilegeCheckResult(50-66)
client/ui/client_ui.go (2)
client/iface/iface.go (2)
MinMTU(29-29)MaxMTU(30-30)client/proto/daemon.pb.go (3)
SetConfigRequest(3918-3959)SetConfigRequest(3972-3972)SetConfigRequest(3987-3989)
client/ssh/server/command_execution_unix.go (2)
client/ssh/server/server.go (1)
Server(115-142)client/ssh/server/user_utils.go (1)
PrivilegeCheckResult(50-66)
🔇 Additional comments (40)
client/ssh/client/terminal_unix.go (3)
17-68: Previous issues successfully resolved.Both the critical TTY restoration issue and the major fd selection issue from previous reviews have been addressed:
- Line 24 correctly uses stdin fd for raw mode and PTY negotiation
- Lines 31-36 properly restore terminal state before returning on setupTerminal error
- Signal handling goroutine is started only after successful PTY setup
The terminal lifecycle management is now correct and will not leave users with an unusable shell.
70-77: LGTM: Platform-appropriate no-ops.Both
setupNonTerminalModeandrestoreWindowsConsoleStatecorrectly implement no-ops for Unix systems.
79-127: LGTM: Standard PTY setup.The terminal setup correctly:
- Retrieves terminal dimensions
- Defines standard terminal modes (control characters, speeds)
- Defaults to xterm-256color when TERM is unset
- Requests PTY with appropriate parameters
client/ssh/server/command_execution_unix.go (8)
23-59: LGTM: Thread-safe PTY lifecycle management.The
ptyManagercorrectly implements thread-safe operations:
sync.Onceensures Close is idempotent- RWMutex protects concurrent access to state
- Setsize guards against operations on closed PTY
61-76: LGTM: Runtime detection of su capabilities.The detection logic appropriately:
- Uses a 500ms timeout to prevent hanging
- Runs
su --helpto query capabilities- Returns false on error (safe default)
78-100: Previous issue resolved: su --pty validation.Line 91 correctly checks
s.suSupportsPtybefore adding the--ptyflag, addressing the previous review concern. The command construction properly handles privilege switching via su with appropriate flags.
102-120: LGTM: Environment and command preparation helpers.Both helper functions correctly:
getShellCommandArgs: Constructs appropriate shell invocationprepareCommandEnv: Composes environment from user context, SSH variables, and filtered session environment
122-193: LGTM: Well-structured PTY execution pipeline.The PTY lifecycle management is cleanly separated:
- Environment setup (TERM variable)
- PTY creation with sensible 80x24 defaults
- Deferred cleanup with ptyManager
- Proper error propagation and user messaging
195-234: LGTM: Robust PTY IO and resize handling.Both handlers correctly:
handlePtyWindowResize: Responds to window changes and context cancellationhandlePtyIO: Bidirectional IO in separate goroutines with proper EOF/EIO handlingThe error filtering (EOF, EIO) is appropriate for PTY lifecycle.
236-287: LGTM: Comprehensive completion and cancellation handling.The completion flow correctly:
- Waits for command completion or context cancellation
- On cancellation: closes PTY, kills process group, enforces 5s timeout, exits with 130
- On completion: propagates exit codes appropriately
289-329: Previous issue resolved: SIGTERM grace period with polling.The
killProcessGroupfunction now correctly implements the polling approach from the previous review discussion:
- Sends SIGTERM first
- Polls process existence every 50ms using
kill(pgid, 0)- Returns immediately if process exits
- Only sends SIGKILL after 500ms grace period
- No unnecessary delays when processes exit quickly
This addresses the previous concern about immediate SIGKILL and ensures graceful shutdown.
client/ssh/server/command_execution.go (5)
15-58: LGTM: Clear command orchestration.The
handleCommandmethod appropriately:
- Distinguishes PTY from non-PTY execution paths
- Provides user-facing error messages on command creation failure
- Properly defers cleanup for PTY commands
95-131: Previous issue resolved: Stderr stream separation.Line 111 now correctly routes stderr to
session.Stderr()instead of the main session stream, allowing clients to distinguish stdout from stderr output. This addresses the previous review concern.The command execution flow is well-structured with proper:
- Process group setup
- Stdin pipe management
- Working directory validation
- Error handling
133-143: LGTM: Clean stdin handling.The IO handler correctly manages stdin with:
- Deferred pipe close for cleanup
- Error logging for diagnostic purposes
145-173: LGTM: Comprehensive cleanup with cancellation handling.The cleanup logic properly handles both paths:
- Normal completion: delegates to handleCommandCompletion
- Cancellation: kills process group, enforces 5s timeout, exits with 130 (standard for interrupt)
175-207: LGTM: Correct exit code propagation.Both handlers correctly manage command completion:
handleCommandCompletion: Routes success/failure appropriatelyhandleSessionExit: Properly extracts exit codes fromExitErrorand defaults to 1 for non-exit errorsclient/ssh/proxy/proxy.go (3)
42-68: gRPC connection cleanup implemented.The gRPC connection leak flagged in previous reviews has been properly addressed. The connection is now stored in the struct and released via the
Close()method.
255-299: SFTP subsystem implementation looks solid.The SFTP bridge properly handles session lifecycle, pipe setup, bidirectional I/O copying, and context cancellation. Error handling and cleanup are appropriate.
251-361: Port forwarding correctly disabled in proxy mode.The proxy appropriately rejects direct-tcpip and tcpip-forward requests. This is a reasonable design decision since port forwarding would bypass the proxy's authentication/verification layer.
client/ssh/client/client.go (6)
36-124: Client lifecycle management looks solid.The
Clientstruct properly wrapscrypto/ssh.Clientwith lifecycle management, terminal state restoration, and context-aware session handling. Cleanup paths are well-structured.
126-231: Command execution methods are well-designed.The three execution variants (
ExecuteCommand,ExecuteCommandWithIO,ExecuteCommandWithPTY) provide appropriate levels of control. Context cancellation sendsSIGTERMwith a grace period, and error handling properly distinguishes SSH exit errors from connection failures.
284-311: Daemon address resolution timing fixed.The daemon address is now properly resolved before
createHostKeyCallback(lines 285-289), ensuring daemon-backed host key verification works correctly. This addresses the previous review concern about "key not found in NetBird daemon" errors.
462-472: Host key verification error message improved.The error message at line 471 now accurately reflects that only known_hosts files are checked in this fallback path. Consider including the hostname placeholder
%sin the actual formatted output for better diagnostics.
507-537: Local port forwarding context cancellation fixed.The listener is now properly closed when the context is cancelled (lines 532-536), addressing the previous review concern. This ensures the accept goroutine terminates and the port is released.
574-693: Remote port forwarding implementation looks correct.The remote forwarding flow properly handles
tcpip-forwardrequests, channel acceptance, cancellation, and bidirectional I/O. Context cancellation is properly wired through the channel handling loop.client/cmd/ssh.go (8)
66-91: Flag registration and subcommand wiring look correct.The
init()function properly registers SSH-related flags onupCmdandsshCmd, and wires the new subcommands (sftp,proxy,detect) into the command tree.
130-175: Goroutine error handling properly fixed.The direct
os.Exit(1)call in the goroutine has been replaced with proper error propagation viaerrCh(lines 156-170). This allows deferred cleanup and signal handlers to execute correctly.
235-360: Custom global flag parsing is well-structured.The custom parsing logic (
extractGlobalFlags,parseGlobalArgs,parseFlag, etc.) properly handles global flags that appear before thesshsubcommand. The handler-map pattern and separate equals/spaced-format parsers make the code maintainable despiteDisableFlagParsingrequirements.
463-498: Hostname and command parsing is well-structured.The
parseHostnameAndCommandfunction properly handlesuser@hostformat, provides sensible username fallbacks (SUDO_USER→ current user →root), and extracts commands from remaining arguments.
500-572: SSH execution flow is clean and correct.The
runSSHfunction properly orchestrates connection establishment, cleanup, port forwarding, and command/terminal execution. Exit code propagation viaos.Exitat line 555 is appropriate in this context (after returning from the goroutine, preserving SSH exit semantics).
627-720: Port forward specification parsing covers common formats.The parsing logic handles 2/3/4-part specifications, IPv6 addresses, and Unix socket paths. The
isUnixSocketheuristic (lines 710-712) is intentionally simple—sufficient for distinguishing socket syntax from network addresses at parse time, as clarified in previous review discussions.
722-763: SSH proxy command implementation looks solid.The
sshProxyFnproperly initializes the proxy, handles JWT authentication, and ensures cleanup viadefer proxy.Close(). Error handling and logging setup are appropriate.
774-795: SSH detect command uses exit codes as output.The
sshDetectFncallsos.Exitdirectly with specific exit codes (0, 1, 2) to signal server type to SSH'sMatch execdirective. As discussed in previous reviews, this design is acceptable for an internal utility command where the exit code is the output rather than an error condition.client/ui/client_ui.go (7)
262-295: Struct extensions for SSH settings and TTL look consistentThe added UI widget pointers and mirrored boolean/int fields on
serviceClientalign with existing patterns (e.g., network flags) and give a clear source of truth for change detection. No functional concerns here.
442-447: SSH controls wiring and resized settings window are fineThe new SSH-related checkboxes and TTL entry are correctly instantiated and later used in forms and request building; bumping the settings window size to 600×400 matches the new tabbed layout.
Also applies to: 450-451
579-605: SetConfigRequest construction and TTL validation are soundThe new
buildSetConfigRequestcorrectly:
- Uses active profile + current OS user.
- Maps interface/MTU and network flags into the protobuf.
- Adds all SSH toggles to
SetConfigRequest.For SSH JWT cache TTL:
- It trims the text, parses as
int32, enforces0 ≤ TTL ≤ maxSSHJWTCacheTTL, and only then assigns toreq.SshJWTCacheTTL.- The
"0=disabled"semantics match the SSH tab label.No functional issues; the TTL handling addresses prior concerns about missing upper bounds.
Also applies to: 612-617, 618-629, 631-633
638-672: Config update and reconnect logic looks correct
sendConfigUpdatereuses the daemon client, performsSetConfig, and, if the service is currently connected, asynchronously issues aDownfollowed byUpto apply changes. Errors are logged and won’t crash the UI. This is a reasonable trade-off between responsiveness and simplicity.
674-694: Tabbed settings UI is a solid improvementThe new
getSettingsFormwith Connection/Network/SSH tabs, shared Save/Cancel buttons, and separategetNetworkForm/getSSHFormhelpers makes the settings much clearer and keeps each concern localized. Wiring Save tos.saveSettingsand Cancel to just close the window is consistent and predictable.Also applies to: 696-706, 708-718
721-737: SSH change detection behavior on invalid TTL is sensible
hasSSHChanges:
- Treats an unparseable TTL as “changed” to force validation on save.
- Compares the parsed TTL against the baseline
sshJWTCacheTTLotherwise, along with all SSH toggles.This matches
buildSetConfigRequest’s stricter validation and ensures that invalid TTL edits aren’t silently ignored.
1385-1393: Proto → config mapping for SSH fields and TTL is consistentMapping the new SSH flags and TTL from
GetConfigResponseintoprofilemanager.Configmirrors how existing fields are handled and provides a single source for both the UI and other client components. Using a pointer forSSHJWTCacheTTLis appropriate and keeps the “0=disabled” semantics while still distinguishing presence from absence at the config layer.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
client/cmd/ssh.go (1)
85-86: Redundant flag registrations for-Land-R(documentation only).These
StringArrayPcalls register-Land-Rflags but don't bind them to variables, and they're never used for parsing sinceDisableFlagParsing: trueis set on line 124. They exist solely for help text generation, while the actual parsing happens inparseCustomSSHFlags(lines 210-213). This creates maintenance overhead—updating the behavior requires changing both the help text and the parsing logic.Consider adding a comment clarifying that these registrations are for documentation only, or refactor to use Cobra's native flag parsing by removing
DisableFlagParsingand binding these flags tolocalForwardsandremoteForwardsdirectly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/cmd/ssh.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-13T00:29:53.220Z
Learnt from: lixmal
Repo: netbirdio/netbird PR: 4015
File: client/cmd/ssh_exec_unix.go:53-74
Timestamp: 2025-11-13T00:29:53.220Z
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/cmd/ssh.go
🧬 Code graph analysis (1)
client/cmd/ssh.go (6)
client/ssh/server/server.go (2)
DefaultSSHPort(31-31)New(170-184)client/cmd/root.go (1)
SetFlagsFromEnvVars(198-219)util/log.go (2)
FindFirstLogPath(77-84)InitLog(32-74)client/ssh/proxy/proxy.go (1)
New(46-61)client/ssh/client/client.go (3)
Dial(284-311)DialOptions(275-281)Client(37-44)client/ssh/detection/detection.go (4)
ServerTypeRegular(31-31)Dialer(35-37)Timeout(23-23)DetectSSHServerType(59-99)
🔇 Additional comments (2)
client/cmd/ssh.go (2)
381-411: Flag bindings correctly implemented.All long-form and short-form flags are properly bound using
IntVar,StringVar, andBoolVar, ensuring that both--portand-p(and similar pairs) update the same struct fields. This addresses the issue raised in previous reviews.
156-175: Clean error propagation with context cancellation.The error handling properly uses a buffered channel (
errCh) to propagate errors from the SSH goroutine, avoiding directos.Exitcalls and allowing deferred cleanup to execute. The signal handling and context cancellation flow is correct.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
management/server/account.go (1)
1228-1228: Fix typo in log message.The log message contains "accountssssssss" instead of "account".
Apply this diff:
- log.Errorf("failed to get account onboarding for accountssssssss %s: %v", accountID, err) + log.Errorf("failed to get account onboarding for account %s: %v", accountID, err)
🧹 Nitpick comments (2)
client/server/server_test.go (1)
320-321: BuildManager call signature fixedConfig passed as second arg looks good. Consider renaming local variable from config to cfg to avoid shadowing the imported package name.
client/internal/engine_test.go (1)
219-223: Optional: Remove unreachable return statements.The
returnstatements at lines 222 and 228 are unreachable becauset.Fatalterminates execution. While harmless, removing them would clean up the code.Apply this diff:
key, err := wgtypes.GeneratePrivateKey() if err != nil { t.Fatal(err) - return } sshKey, err := nbssh.GeneratePrivateKey(nbssh.ED25519) if err != nil { t.Fatal(err) - return }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
.github/workflows/check-license-dependencies.yml(1 hunks)client/cmd/ssh.go(1 hunks)client/cmd/testutil_test.go(2 hunks)client/internal/engine_test.go(9 hunks)client/server/server_test.go(2 hunks)client/ui/client_ui.go(7 hunks)go.mod(8 hunks)management/internals/server/modules.go(1 hunks)management/internals/shared/grpc/conversion.go(4 hunks)management/internals/shared/grpc/server.go(1 hunks)management/server/account.go(14 hunks)management/server/account/manager.go(3 hunks)management/server/account_test.go(26 hunks)management/server/dns_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- client/cmd/ssh.go
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
🧰 Additional context used
🧬 Code graph analysis (9)
management/internals/server/modules.go (1)
management/server/account.go (1)
BuildManager(178-264)
client/ui/client_ui.go (3)
client/server/server.go (1)
New(99-110)client/iface/iface.go (2)
MinMTU(29-29)MaxMTU(30-30)client/proto/daemon.pb.go (12)
SetConfigRequest(3918-3959)SetConfigRequest(3972-3972)SetConfigRequest(3987-3989)StatusRequest(850-858)StatusRequest(871-871)StatusRequest(886-888)DownRequest(973-977)DownRequest(990-990)DownRequest(1005-1007)UpRequest(762-768)UpRequest(781-781)UpRequest(796-798)
client/server/server_test.go (2)
management/server/account.go (1)
BuildManager(178-264)management/server/integrations/port_forwarding/controller.go (1)
NewControllerMock(20-22)
management/server/dns_test.go (3)
management/server/account.go (1)
BuildManager(178-264)management/server/integrated_validator.go (6)
MockIntegratedValidator(125-128)MockIntegratedValidator(153-155)MockIntegratedValidator(157-159)MockIntegratedValidator(161-163)MockIntegratedValidator(165-167)MockIntegratedValidator(169-171)management/server/integrations/port_forwarding/controller.go (1)
NewControllerMock(20-22)
management/server/account/manager.go (1)
shared/auth/user.go (1)
UserAuth(7-28)
client/cmd/testutil_test.go (1)
management/server/account.go (1)
BuildManager(178-264)
client/internal/engine_test.go (4)
shared/management/proto/management.pb.go (6)
SSHConfig(2091-2102)SSHConfig(2117-2117)SSHConfig(2132-2134)JWTConfig(1619-1628)JWTConfig(1643-1643)JWTConfig(1658-1660)client/ssh/server/server.go (1)
JWTConfig(144-149)client/internal/engine.go (2)
Engine(138-211)EngineConfig(81-135)management/server/account.go (1)
BuildManager(178-264)
management/server/account.go (1)
shared/auth/user.go (1)
UserAuth(7-28)
management/server/account_test.go (2)
shared/auth/user.go (1)
UserAuth(7-28)management/server/account.go (1)
BuildManager(178-264)
⏰ 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: JS / Lint
- GitHub Check: release_ui_darwin
- GitHub Check: iOS / Build
- GitHub Check: Android / Build
- GitHub Check: Linux
- GitHub Check: Darwin
- GitHub Check: release
- GitHub Check: release_ui
- GitHub Check: Build Cache
- GitHub Check: Windows
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
🔇 Additional comments (28)
.github/workflows/check-license-dependencies.yml (1)
22-52: Improved bash practices and clearer licensing messaging.The refactored logic properly uses process substitution and explicit error handling. The changes are sound:
- Process substitution (line 42):
done < <(find ...)avoids subshell scoping issues compared to piping, ensuring variables persist correctly.- Error suppression (line 34):
2>/dev/nullcleanly suppresses permission errors.- Condition logic (line 35): Correctly changed from
-z(empty) to-n(non-empty) to detect problematic matches.- Licensing message (line 47): Explicitly references AGPLv3 constraints, providing clearer guidance than the original wording.
The workflow remains robust and maintainable.
Please verify that the AGPLv3 licensing constraints referenced on line 47 correctly align with any new SSH-related dependencies introduced in this PR. Have any AGPLv3-licensed packages been added as dependencies?
client/ui/client_ui.go (7)
58-58: LGTM: Clear constant for SSH JWT Cache TTL maximum.The 24-hour maximum is appropriate for JWT caching and the constant improves maintainability.
269-300: LGTM: SSH-related fields follow existing conventions.The widget declarations and state tracking fields are well-organized and consistent with the existing codebase structure.
489-682: LGTM: Settings save flow is well-structured.The refactored flow properly validates inputs, builds the configuration request, sends updates, and handles errors gracefully. The dialog now stays open on validation failures (addressing previous feedback), and the reconnection logic correctly applies settings changes to active connections.
684-729: LGTM: Tabbed settings form improves organization.The refactored UI separates connection, network, and SSH settings into logical tabs. The SSH JWT Cache TTL label clearly indicates that 0 disables the feature.
628-639: LGTM: SSH JWT Cache TTL validation is thorough.The validation properly checks the range [0, 86400] and provides clear error messages. The decision to treat an empty field as "no change" is reasonable, though it requires consistent handling in
hasSSHChanges(addressed in separate comment).
1268-1325: LGTM: SSH config loading handles optional fields correctly.The defensive nil-checks ensure the code handles missing SSH configuration gracefully. State and UI updates only occur when the server provides values.
1395-1402: LGTM: Proto-to-config SSH field mapping is correct.The conversion properly maps all SSH-related fields from the protobuf response to the config structure, with appropriate type conversions for the TTL field.
client/cmd/testutil_test.go (1)
121-121: BuildManager call updated correctlyPassing config as the second argument matches the new signature. LGTM.
management/internals/server/modules.go (1)
69-69: BuildManager wiring includes server configUsing s.config as the new second parameter is correct. LGTM.
management/internals/shared/grpc/server.go (1)
649-649: toPeerConfig now receives server configThe additional config argument is correctly passed. LGTM.
management/server/account_test.go (2)
2962-2963: Test helper BuildManager call adjustedAdded nil as the new second parameter (config). Looks correct.
47-48: Verification confirmed: no stale UserAuth references remainAll references in management/server/account_test.go consistently use the new
auth.UserAuthtype fromgithub.com/netbirdio/netbird/shared/auth(imported at line 47). Migration is complete across all 30+ usages throughout the file. No references to oldnbcontext.UserAuthormanagement/server/auth.UserAuthtypes detected.management/internals/shared/grpc/conversion.go (2)
86-105: Peer SSH config assembled correctlytoPeerConfig now includes SSH settings and receives server config. LGTM on structure.
86-105: Review comment is incorrectThe review assumes a TTL field exists in the
SSHConfigproto message, but verification shows:
- The
SSHConfigmessage inshared/management/proto/management.protocontains only:sshEnabled,sshPubKey, andjwtConfig—no TTL field.- The
JWTConfigmessage lacks TTL fields as well (contains onlyissuer,audience,keysLocation,maxTokenAge).- The file
management/internals/shared/grpc/conversion.gois not modified in this PR (onlyclient/cmd/ssh.gochanged).- No proto definition changes exist in this PR that would add a TTL field to
SSHConfig.The
sshJWTCacheTTLfields found inclient/proto/daemon.protoare in different message structures unrelated to the management server'sSSHConfig.Likely an incorrect or invalid review comment.
management/server/dns_test.go (1)
227-228: BuildManager call signature alignedAdded nil as the new config argument. LGTM.
management/server/account/manager.go (1)
9-10: UserAuth type migration verified—no issues foundThe codebase has been successfully migrated to use
github.com/netbirdio/netbird/shared/auth.UserAuthconsistently. All interface methods, implementers, mocks, and call sites (lines 49–53, 121–127, and throughout) use the correct shared auth type. No oldmanagement/server/auth.UserAuthor mixed-type usages detected.client/internal/engine_test.go (7)
27-27: LGTM! Import changes are clean.The stdnet import addition and nbssh aliasing align well with the SSH subsystem refactoring.
Also applies to: 51-51
225-229: LGTM! SSH key generation and EngineConfig wiring are correct.The ED25519 key generation follows best practices, and passing the SSH key to EngineConfig aligns with the new SSHKey field in engine.go.
Also applies to: 247-247
259-259: LGTM! Error handling improvements are idiomatic.Switching to
require.NoErroris the standard testify pattern, and the added nil assertions forsshServerprovide good cleanup verification.Also applies to: 285-285, 307-307, 320-320, 335-338
293-301: LGTM! JwtConfig structure matches protobuf definitions.The JWT configuration fields (Issuer, Audience, KeysLocation, MaxTokenAge) align with the protobuf definitions and support the new JWT-based SSH authentication flow.
340-399: LGTM! New test functions provide solid coverage.
TestEngine_SSHUpdateLogicvalidates that the SSH server respects configuration flags (BlockInbound, ServerSSHAllowed), andTestEngine_SSHServerConsistencyensures error handling leaves no partial state. Both add valuable coverage for the SSH lifecycle.
813-816: LGTM! Network setup is correct.Using
stdnet.NewNetfor test network isolation is appropriate, and error handling is correct.
1628-1628: LGTM! BuildManager call updated correctly.The call site correctly passes the new
configparameter as the second argument, matching the updated signature in account.go.management/server/account.go (4)
18-18: LGTM! Import additions support the refactoring.The
shared/authandnbconfigimports enable the UserAuth type migration and server configuration integration.Also applies to: 30-30
87-88: LGTM! Config field addition is well-documented.Adding server configuration access to the account manager enables configuration-driven behavior and is properly initialized in BuildManager.
180-180: LGTM! BuildManager signature updated correctly.The addition of the
configparameter as the second argument is a breaking but necessary change. The config is properly stored in the manager struct and call sites are updated throughout the PR.Also applies to: 202-202
1017-1017: LGTM! UserAuth type migration is consistent.The mechanical refactoring from
nbcontext.UserAuthtoauth.UserAuthis applied consistently across all function signatures. The type structure remains identical, so no behavior changes are expected.Also applies to: 1066-1066, 1085-1085, 1116-1116, 1280-1280, 1324-1324, 1482-1482, 1561-1561, 1609-1609
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
management/internals/shared/grpc/conversion.go (1)
365-397: Default JWKS path follows OIDC standard but may not work for all providers.The code falls back to
issuer + "/.well-known/jwks.json"whenAuthKeysLocationis not explicitly configured (lines 387-390). While this follows the OIDC discovery standard and works for most providers, it may fail for some (e.g., certain Okta or Keycloak configurations with custom paths).The current implementation is a pragmatic compromise: it requires an explicit
issuer(returnsnilif missing) and allows explicitAuthKeysLocationoverride, but provides a sensible default for standard OIDC providers. Given that the author marked this as "fixed" and the approach aligns with OIDC standards, this is acceptable.client/ssh/server/command_execution.go (1)
60-89: The UsedFallback logic issue persists despite past review.Line 77 still treats
privilegeResult.UsedFallbackas an error condition alongside actual errors. According to thePrivilegeCheckResultdocumentation in user_utils.go, whenUsedFallback=true, "all privilege checks have already been performed and no additional privilege dropping or root checks are needed."The current flow attempts
createSuCommandeven whenUsedFallback=true, which is semantically incorrect. When no user switching is required (indicated byRequiresUserSwitching=falseorUsedFallback=true), we should skipsuentirely and callcreateExecutorCommanddirectly.Apply this refactoring to check privilege switching semantics upfront:
func (s *Server) createCommand(privilegeResult PrivilegeCheckResult, session ssh.Session, hasPty bool) (*exec.Cmd, func(), error) { localUser := privilegeResult.User // If PTY requested but su doesn't support --pty, skip su and use executor // This ensures PTY functionality is provided (executor runs within our allocated PTY) if hasPty && !s.suSupportsPty { log.Debugf("PTY requested but su doesn't support --pty, using executor for PTY functionality") cmd, cleanup, err := s.createExecutorCommand(session, localUser, hasPty) if err != nil { return nil, nil, fmt.Errorf("create command with privileges: %w", err) } cmd.Env = s.prepareCommandEnv(localUser, session) return cmd, cleanup, nil } + // If no user switching is needed, use executor directly + if !privilegeResult.RequiresUserSwitching { + log.Debugf("no user switching required, using executor") + cmd, cleanup, err := s.createExecutorCommand(session, localUser, hasPty) + if err != nil { + return nil, nil, fmt.Errorf("create command with privileges: %w", err) + } + cmd.Env = s.prepareCommandEnv(localUser, session) + return cmd, cleanup, nil + } + // Try su first for system integration (PAM/audit) when privileged cmd, err := s.createSuCommand(session, localUser, hasPty) - if err != nil || privilegeResult.UsedFallback { + if err != nil { log.Debugf("su command failed, falling back to executor: %v", err) cmd, cleanup, err := s.createExecutorCommand(session, localUser, hasPty) if err != nil { return nil, nil, fmt.Errorf("create command with privileges: %w", err) } cmd.Env = s.prepareCommandEnv(localUser, session) return cmd, cleanup, nil } cmd.Env = s.prepareCommandEnv(localUser, session) return cmd, func() {}, nil }Based on learnings
🧹 Nitpick comments (3)
client/ssh/server/userswitching_unix.go (2)
62-106: Avoid hard failure if remote address can’t be parsed forlogin -h
getLoginCmdcurrently fails the whole PTY setup ifnetip.ParseAddrPort(remoteAddr.String())fails, or ifloginis missing. That’s reasonable in many cases, but on atypical transports (non-TCPnet.Addrimplementations, funkyString()formats, or minimal systems withoutlogin) this will turn into a user-visible session failure even though we could still start a shell without-h.Consider degrading gracefully when the host can’t be parsed or
-hisn’t supported instead of returning an error, e.g. by omitting the-hargument and logging a warning:func (s *Server) getLoginCmd(username string, remoteAddr net.Addr) (string, []string, error) { loginPath, err := exec.LookPath("login") if err != nil { return "", nil, fmt.Errorf("login command not available: %w", err) } - addrPort, err := netip.ParseAddrPort(remoteAddr.String()) - if err != nil { - return "", nil, fmt.Errorf("parse remote address: %w", err) - } + var host string + if addrPort, err := netip.ParseAddrPort(remoteAddr.String()); err == nil { + host = addrPort.Addr().String() + } else { + log.Warnf("SSH login: unable to parse remote address %q, proceeding without -h: %v", remoteAddr.String(), err) + } switch runtime.GOOS { case "linux": // Special handling for Arch Linux without /etc/pam.d/remote if s.fileExists("/etc/arch-release") && !s.fileExists("/etc/pam.d/remote") { return loginPath, []string{"-f", username, "-p"}, nil } - return loginPath, []string{"-f", username, "-h", addrPort.Addr().String(), "-p"}, nil + if host != "" { + return loginPath, []string{"-f", username, "-h", host, "-p"}, nil + } + return loginPath, []string{"-f", username, "-p"}, nil case "darwin", "freebsd", "openbsd", "netbsd", "dragonfly": - return loginPath, []string{"-fp", "-h", addrPort.Addr().String(), username}, nil + if host != "" { + return loginPath, []string{"-fp", "-h", host, username}, nil + } + return loginPath, []string{"-fp", username}, nil default: return "", nil, fmt.Errorf("unsupported Unix platform for login command: %s", runtime.GOOS) } }This keeps the “best-effort”
-hbehavior where possible but avoids breaking sessions solely due to address formatting or missing PAM configs.
108-153: Credential parsing and group fallback are robust; minor duplication opportunityParsing UID/GID as 32-bit and falling back to the primary GID when supplementary group lookup fails is a good, defensive default that avoids hard failures when NSS is misconfigured.
If you ever want to simplify this path, you could avoid the second
user.LookupingetSupplementaryGroupsand instead calllocalUser.GroupIds()directly fromparseUserCredentials, since you already have*user.Userthere. That would remove one extra lookup without changing behavior.client/ssh/server/userswitching_windows.go (1)
18-87: ---Windows username validation follows sAMAccountName limits but may reject modern local accounts
The 20-character cap aligns with the legacy sAMAccountName constraint (pre-Windows 2000 standard), which is appropriate for backward compatibility with older tools and services. However, modern Windows local accounts support names up to 256 characters, and UPN / email-style logon names can be significantly longer.
The current validation will reject legitimate modern local accounts that exceed 20 characters. If your SSH server environment includes modern Windows local accounts or UPN-based identities, consider relaxing or making the length limit configurable. For now, the conservative 20-character enforcement is a reasonable choice for legacy SAM compatibility, with the understanding that it trades broader account support for stricter backward compatibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
client/cmd/ssh.go(1 hunks)client/cmd/ssh_test.go(1 hunks)client/ssh/server/command_execution.go(1 hunks)client/ssh/server/user_utils.go(1 hunks)client/ssh/server/userswitching_unix.go(1 hunks)client/ssh/server/userswitching_windows.go(1 hunks)client/ui/client_ui.go(7 hunks)management/internals/shared/grpc/conversion.go(4 hunks)management/server/account.go(15 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-13T00:29:53.220Z
Learnt from: lixmal
Repo: netbirdio/netbird PR: 4015
File: client/cmd/ssh_exec_unix.go:53-74
Timestamp: 2025-11-13T00:29:53.220Z
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/cmd/ssh.goclient/ssh/server/user_utils.goclient/ssh/server/command_execution.goclient/ssh/server/userswitching_windows.goclient/ssh/server/userswitching_unix.go
🧬 Code graph analysis (7)
client/ui/client_ui.go (4)
client/server/server.go (1)
New(99-110)client/ssh/server/server.go (1)
New(170-184)client/iface/iface.go (2)
MinMTU(29-29)MaxMTU(30-30)client/proto/daemon.pb.go (3)
SetConfigRequest(3918-3959)SetConfigRequest(3972-3972)SetConfigRequest(3987-3989)
client/cmd/ssh.go (6)
client/ssh/server/server.go (2)
DefaultSSHPort(31-31)New(170-184)client/cmd/root.go (1)
SetFlagsFromEnvVars(198-219)util/log.go (2)
FindFirstLogPath(77-84)InitLog(32-74)client/ssh/proxy/proxy.go (1)
New(46-61)client/ssh/client/client.go (3)
Dial(284-311)DialOptions(275-281)Client(37-44)client/ssh/detection/detection.go (4)
ServerTypeRegular(31-31)Dialer(35-37)Timeout(23-23)DetectSSHServerType(59-99)
client/ssh/server/user_utils.go (1)
client/ssh/server/server.go (4)
New(170-184)Server(115-142)PrivilegedUserError(52-54)UserNotFoundError(65-68)
client/ssh/server/command_execution.go (2)
client/ssh/server/server.go (1)
Server(115-142)client/ssh/server/user_utils.go (1)
PrivilegeCheckResult(50-66)
client/ssh/server/userswitching_windows.go (2)
client/ssh/server/server.go (1)
Server(115-142)client/ssh/server/executor_windows.go (2)
WindowsExecutorConfig(30-41)NewPrivilegeDropper(45-47)
client/ssh/server/userswitching_unix.go (4)
client/ssh/server/server.go (2)
New(170-184)Server(115-142)client/ssh/server/executor_windows.go (1)
NewPrivilegeDropper(45-47)client/ssh/server/executor_unix.go (2)
NewPrivilegeDropper(41-43)ExecutorConfig(27-35)client/ssh/server/user_utils.go (1)
PrivilegeCheckResult(50-66)
management/server/account.go (1)
shared/auth/user.go (1)
UserAuth(7-28)
⏰ 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). (24)
- GitHub Check: Management / Integration (amd64, postgres)
- GitHub Check: Management / Benchmark (API) (amd64, sqlite)
- GitHub Check: Management / Benchmark (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Benchmark (API) (amd64, postgres)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Management / Benchmark (amd64, postgres)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Client / Unit (386)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Linux
- GitHub Check: Darwin
- GitHub Check: Windows
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
- GitHub Check: release_ui
- GitHub Check: release_ui_darwin
- GitHub Check: Client / Unit
- GitHub Check: release
- GitHub Check: Android / Build
- GitHub Check: Client / Unit
🔇 Additional comments (34)
management/internals/shared/grpc/conversion.go (4)
6-7: LGTM: Imports support JWT config derivation.The new imports
net/urlandstringsare appropriately added to support the JWT configuration derivation logic inbuildJWTConfigandderiveIssuerFromTokenEndpoint.
86-104: LGTM: SSH config construction properly conditionalizes JWT config.The refactoring extracts SSH config construction into a clear two-step process: first creating the base config with
SshEnabled, then conditionally populatingJwtConfigonly when SSH is enabled. This approach is clean and avoids unnecessary JWT config derivation for non-SSH peers.
109-109: LGTM: Config parameter properly threaded through.The call site correctly passes the
configparameter totoPeerConfig, maintaining consistency with the updated signature.
399-411: LGTM: Issuer derivation logic is correct.The function properly extracts the issuer URL from a token endpoint by parsing the URL and formatting it as
scheme://host/. Error handling is appropriate, returning an empty string for invalid input rather than panicking.management/server/account.go (5)
18-18: LGTM: Imports support auth type migration and config integration.The new imports enable the migration to
auth.UserAuthfrom the shared auth package and the integration of management server configuration for JWT config derivation used in SSH features.Also applies to: 30-30
87-88: LGTM: Config field properly documented.The new
configfield is clearly documented and will be used to provide JWT configuration to peers with SSH enabled.
178-220: LGTM: Config parameter properly integrated into BuildManager.The
configparameter is correctly added to theBuildManagersignature and stored in theDefaultAccountManagerstruct. This enables SSH-enabled peers to receive JWT configuration for SSH authentication. The parameter ordering (after context, before store) is logical.
1228-1228: LGTM: Typo fix in error log.The error log message is corrected from "accountssssssss" to "account", improving log readability.
1017-1017: LGTM: Auth type migration is consistent and correct.All function signatures are consistently updated to use
auth.UserAuthfrom the shared auth package instead ofnbcontext.UserAuth. This consolidation aligns with the broader refactoring to centralize auth types. The mechanical nature of these changes reduces the risk of introducing bugs.Also applies to: 1066-1066, 1085-1085, 1116-1116, 1280-1280, 1324-1324, 1482-1482, 1561-1561, 1609-1609
client/cmd/ssh.go (1)
1-808: LGTM! Previous review concerns have been addressed.The SSH command implementation is comprehensive and well-structured. All issues from previous reviews have been successfully addressed:
- Long-form flags (
--port,--user,--known-hosts,--identity,--config,--log-level) are properly bound usingIntVar/StringVar/BoolVar(lines 390-408)- Error propagation from the SSH goroutine now uses an error channel pattern instead of direct
os.Exit(lines 156-162)- Parse errors are returned directly rather than falling back and masking invalid flags (lines 428-430)
The remaining direct
os.Exitcalls insshDetectFn(lines 787-808) andexecuteSSHCommand(line 555) serve specific purposes (SSH server detection exit codes and SSH command exit code propagation) and have been accepted by maintainers.The implementation provides robust SSH functionality with proper context handling, port forwarding, error reporting, and comprehensive flag parsing.
client/cmd/ssh_test.go (1)
1-717: Excellent test coverage!This comprehensive test suite provides strong validation for the SSH command implementation:
- Flag parsing tests cover basic scenarios, user@host format, commands with flags, and double-dash separators
- Conflict prevention tests ensure SSH flags don't interfere with remote command flags (e.g.,
ls -la,grep -r)- Non-interactive execution tests validate command execution behavior
- Regression tests guard against reintroduction of past issues (e.g.,
ls -labeing misparsed)- Port forwarding tests cover local/remote forwards, multiple forwards, bind addresses, Unix sockets, and wildcard handling
- Invalid flag rejection tests ensure proper error handling and prevent invalid flags from being misinterpreted as hostnames
The tests validate fixes for issues identified in previous reviews (particularly invalid flag rejection at lines 671-717) and provide good coverage of edge cases.
client/ssh/server/user_utils.go (7)
1-39: LGTM! Clean foundation with good testing support.The dependency injection pattern for testing runtime checks is well-designed, and the error definitions are clear and appropriately exported.
41-110: LGTM! Well-documented privilege checking API.The
PrivilegeCheckResult.UsedFallbackfield documentation clearly states the semantic intent: "all privilege checks have already been performed and no additional privilege dropping or root checks are needed." This design properly separates privilege checking from execution logic.
112-176: LGTM! Robust privilege checking flow.The Unix non-privileged shortcut correctly sets
UsedFallback=truewhen falling back to the current user, and theRequiresUserSwitchingcalculation even on lookup failure ensures accurate metadata for callers.
193-254: Windows domain logic appears sound but complex.The user comparison logic correctly handles domain-qualified usernames. The key decision at lines 246-250 (allowing bare username to match domain-qualified current user) aligns with expected Windows behavior where local user context is assumed when no domain is specified.
256-291: LGTM! Clean wrapper methods with proper synchronization.The wrapper methods appropriately delegate to
CheckPrivilegesand handle the result correctly.
293-343: LGTM! Comprehensive privileged user detection.The privileged username detection covers Unix (root) and Windows (Administrator, SYSTEM, etc.) with appropriate domain stripping. The fail-safe behavior on lookup errors (returning false) is correct for security.
345-411: LGTM! Thorough Windows privilege detection with proper RID handling.The SID checking logic comprehensively covers well-known privileged accounts and domain admin RIDs. The error handling in
isWindowsElevatedcorrectly defaults to non-privileged on failure, maintaining security.client/ssh/server/command_execution.go (4)
15-58: LGTM! Clear command execution entry point with appropriate error handling.The PTY vs. non-PTY path selection is clean, and user-facing error messages provide helpful context without exposing internal details.
91-127: LGTM! Robust command execution with proper stream separation.The stderr wiring at line 107 correctly directs error output to
session.Stderr(), maintaining proper stream separation for SSH clients. The working directory fallback (lines 109-114) provides good resilience.
129-169: LGTM! Proper I/O handling with graceful cancellation.The cancellation flow (lines 149-164) correctly terminates the process group and waits with a timeout. Exit code 130 is conventional for interrupted commands.
171-203: LGTM! Correct exit code propagation.The exit handling properly preserves command exit codes from
exec.ExitErrorand provides sensible defaults for other error types.client/ui/client_ui.go (10)
58-58: LGTM - Well-chosen JWT cache TTL maximum.The 24-hour maximum for SSH JWT cache TTL is a reasonable security boundary and is clearly defined as a constant for maintainability.
269-300: LGTM - SSH state fields properly declared.The SSH-related UI widgets and state fields follow the existing naming conventions and are properly organized with clear intent.
489-522: LGTM - Robust settings save flow.The refactored save flow properly validates inputs, handles errors, and keeps the dialog open when validation fails, allowing users to correct their input. The feature flag check ensures configuration updates respect daemon-level restrictions.
524-555: LGTM - Comprehensive input validation.The validation logic properly checks port range (1-65535), MTU bounds (using
iface.MinMTUandiface.MaxMTU), and pre-shared key format with clear, actionable error messages.
589-646: LGTM - Well-structured config request builder.The request builder properly validates SSH JWT cache TTL (0 to 86,400 seconds), includes SSH checkbox settings unconditionally (they always have a value), and only includes the TTL field when explicitly set by the user (empty field means "no change"). The validation and error handling are correct.
684-729: LGTM - Clean tabbed settings interface.The refactored settings UI cleanly organizes options into Connection, Network, and SSH tabs with proper button wiring. The 600×400 window size may feel tight with 19 total form items across tabs, but the tabbed approach makes navigation manageable.
731-747: LGTM - Correct SSH change detection logic.The change detection properly handles all SSH settings: empty TTL field preserves current value, unparseable input triggers validation, and all SSH state changes are correctly detected. This aligns with the request-building logic in
buildSetConfigRequest.
1268-1324: LGTM - SSH settings properly loaded and bound to UI.The server config loading correctly handles SSH settings with defensive nil checks throughout. The UI properly populates all SSH widgets from the loaded configuration.
1395-1402: LGTM - Proto config conversion handles SSH settings correctly.The conversion from proto to internal config properly handles all SSH settings. The SSH JWT cache TTL is always converted to a non-nil pointer, treating unset proto values as 0 (disabled), which aligns with the UI's "0=disabled" semantics on line 726.
56-1405: Excellent SSH UI integration - all previous concerns addressed.This comprehensive SSH feature integration demonstrates solid engineering:
- Validation: Port (1-65535), MTU (576-8192), and SSH JWT cache TTL (0-86,400) are all validated with clear error messages
- User experience: Settings dialog remains open on validation errors, allowing users to correct input without losing their changes
- Architecture: Clean separation into Connection, Network, and SSH tabs with well-organized helper methods
- Error handling: All error paths properly handled with user-friendly dialogs
- Change detection: Smart logic that treats empty fields as "no change" and detects actual modifications
- Integration: Feature flags properly checked, daemon reconnection handled gracefully
All issues from previous review rounds have been successfully resolved. The code is production-ready.
client/ssh/server/userswitching_unix.go (1)
25-60: POSIX-style username validation is sensible and conservativeThe validation logic (non-empty, 32-char cap, portable character set, rejection of
./.., and warning-only handling of fully numeric names) looks appropriate for SSH user switching and avoids common footguns. It’s strict enough for security while still allowing typical Unix accounts.client/ssh/server/userswitching_windows.go (1)
159-274: API stability confirmed—Windows privilege APIs remain unchanged; keep focused tests close to unsafe codeThe Win32 privilege APIs (LookupPrivilegeValue, GetTokenInformation, AdjustTokenPrivileges) remain supported with no Microsoft deprecation announced, and there are no known deprecations or breaking changes in golang.org/x/sys/windows for these token APIs.
The
hasPrivilege,enablePrivilege, andenableUserSwitchinghelpers implement the standard Windows token pattern correctly and close token handles safely on error paths. The unsafe pointer arithmetic aroundwindows.Tokenprivilegesfollows the expected layout—no changes there.Keep focused tests close to this code (especially verifying privilege absent, present-but-disabled, and successfully enabled states) to catch regressions and catch any layout shifts if
x/sys/windowsinternals change.
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
client/ssh/server/userswitching_unix.go (1)
188-200: Improve error context by checkingprivilegeResult.Error.Good job adding the nil check for
privilegeResult.User(this resolves the previous review concern). However, the error message can be improved by also checkingprivilegeResult.Error, which contains the reason for denial according to thePrivilegeCheckResultdocumentation.Apply this diff to provide better error context:
func (s *Server) createPtyCommand(privilegeResult PrivilegeCheckResult, ptyReq ssh.Pty, session ssh.Session) (*exec.Cmd, error) { localUser := privilegeResult.User if localUser == nil { + if privilegeResult.Error != nil { + return nil, fmt.Errorf("privilege check failed: %w", privilegeResult.Error) + } return nil, errors.New("no user in privilege result") } if privilegeResult.UsedFallback { return s.createDirectPtyCommand(session, localUser, ptyReq), nil } return s.createPtyLoginCommand(localUser, ptyReq, session) }client/ssh/server/command_execution_windows.go (1)
71-124: Profile handle must be unloaded to prevent resource leak.
LoadUserProfileWmounts the user's registry hive and returns a handle inprofile.hProfilethat must be released withUnloadUserProfileW. Without this cleanup, each SSH session leaks registry handles and mounted hives, eventually exhausting resources and blocking subsequent logons. This is a critical operational issue that will degrade service over time.Apply this diff to properly release the profile handle:
userenv := windows.NewLazySystemDLL("userenv.dll") loadUserProfileW := userenv.NewProc("LoadUserProfileW") + unloadUserProfileW := userenv.NewProc("UnloadUserProfileW") ret, _, err := loadUserProfileW.Call( uintptr(userToken), uintptr(unsafe.Pointer(&profile)), ) if ret == 0 { return "", fmt.Errorf("LoadUserProfileW: %w", err) } if profile.lpProfilePath == nil { return "", fmt.Errorf("LoadUserProfileW returned null profile path") } + if profile.hProfile != 0 { + defer func() { + if _, _, unloadErr := unloadUserProfileW.Call(uintptr(userToken), uintptr(profile.hProfile)); unloadErr != windows.ERROR_SUCCESS { + log.Debugf("UnloadUserProfileW failed: %v", unloadErr) + } + }() + } + profilePath := windows.UTF16PtrToString(profile.lpProfilePath) return profilePath, nil
🧹 Nitpick comments (2)
client/ssh/server/userswitching_unix.go (1)
131-153: Consider reusing the existinguser.Userobject to avoid redundant lookup.
getSupplementaryGroupsperforms a freshuser.Lookupcall, but the callerparseUserCredentialsalready has a*user.Userobject that was passed in. You can eliminate the redundant lookup by changing the signature to accept*user.Userdirectly and callinglocalUser.GroupIds().Apply this diff to eliminate the redundant lookup:
-func (s *Server) getSupplementaryGroups(username string) ([]uint32, error) { - u, err := user.Lookup(username) - if err != nil { - return nil, fmt.Errorf("lookup user %s: %w", username, err) - } - - groupIDStrings, err := u.GroupIds() +func (s *Server) getSupplementaryGroups(u *user.User) ([]uint32, error) { + groupIDStrings, err := u.GroupIds() if err != nil { - return nil, fmt.Errorf("get group IDs for user %s: %w", username, err) + return nil, fmt.Errorf("get group IDs for user %s: %w", u.Username, err) } groups := make([]uint32, len(groupIDStrings)) for i, gidStr := range groupIDStrings { gid64, err := strconv.ParseUint(gidStr, 10, 32) if err != nil { - return nil, fmt.Errorf("invalid group ID %s for user %s: %w", gidStr, username, err) + return nil, fmt.Errorf("invalid group ID %s for user %s: %w", gidStr, u.Username, err) } groups[i] = uint32(gid64) } return groups, nil }And update the caller in
parseUserCredentials:- groups, err := s.getSupplementaryGroups(localUser.Username) + groups, err := s.getSupplementaryGroups(localUser)client/ssh/server/command_execution.go (1)
78-88: Consider skipping su when UsedFallback is true.When
privilegeResult.UsedFallbackis true, the comment inuser_utils.goindicates "all privilege checks have already been performed and no additional privilege dropping or root checks are needed - the current user is the target." The current code still attemptscreateSuCommandfirst, which will immediately fail and fall back to the executor. For semantic consistency and to avoid the unnecessary su attempt, consider checkingUsedFallbackupfront and going directly to the executor path when true.Apply this diff for more semantically correct behavior:
// Try su first for system integration (PAM/audit) when privileged - cmd, err := s.createSuCommand(session, localUser, hasPty) - if err != nil || privilegeResult.UsedFallback { + if privilegeResult.UsedFallback { + log.Debugf("using fallback user, skipping su and using executor") + cmd, cleanup, err := s.createExecutorCommand(session, localUser, hasPty) + if err != nil { + return nil, nil, fmt.Errorf("create command with privileges: %w", err) + } + cmd.Env = s.prepareCommandEnv(localUser, session) + return cmd, cleanup, nil + } + + cmd, err := s.createSuCommand(session, localUser, hasPty) + if err != nil { log.Debugf("su command failed, falling back to executor: %v", err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
client/cmd/ssh.go(1 hunks)client/ssh/server/command_execution.go(1 hunks)client/ssh/server/command_execution_windows.go(1 hunks)client/ssh/server/userswitching_unix.go(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-13T00:29:53.220Z
Learnt from: lixmal
Repo: netbirdio/netbird PR: 4015
File: client/cmd/ssh_exec_unix.go:53-74
Timestamp: 2025-11-13T00:29:53.220Z
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.goclient/cmd/ssh.goclient/ssh/server/command_execution_windows.goclient/ssh/server/userswitching_unix.go
📚 Learning: 2025-11-14T13:05:31.708Z
Learnt from: lixmal
Repo: netbirdio/netbird PR: 4015
File: client/ssh/server/userswitching_windows.go:89-139
Timestamp: 2025-11-14T13:05:31.708Z
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.goclient/ssh/server/userswitching_unix.go
📚 Learning: 2025-11-14T11:11:50.779Z
Learnt from: lixmal
Repo: netbirdio/netbird PR: 4015
File: client/ssh/server/server_test.go:396-406
Timestamp: 2025-11-14T11:11:50.779Z
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.
Applied to files:
client/ssh/server/command_execution_windows.go
🧬 Code graph analysis (4)
client/ssh/server/command_execution.go (2)
client/ssh/server/server.go (1)
Server(115-142)client/ssh/server/user_utils.go (1)
PrivilegeCheckResult(50-66)
client/cmd/ssh.go (6)
client/ssh/server/server.go (2)
DefaultSSHPort(31-31)New(170-184)client/cmd/root.go (1)
SetFlagsFromEnvVars(198-219)util/log.go (2)
FindFirstLogPath(77-84)InitLog(32-74)client/ssh/proxy/proxy.go (1)
New(46-61)client/ssh/client/client.go (3)
Dial(284-311)DialOptions(275-281)Client(37-44)client/ssh/detection/detection.go (4)
ServerTypeRegular(31-31)Dialer(35-37)Timeout(23-23)DetectSSHServerType(59-99)
client/ssh/server/command_execution_windows.go (5)
client/ssh/server/server.go (1)
Server(115-142)client/ssh/server/executor_windows.go (1)
NewPrivilegeDropper(45-47)client/ssh/server/executor_unix.go (1)
NewPrivilegeDropper(41-43)client/ssh/server/user_utils.go (1)
PrivilegeCheckResult(50-66)client/ssh/server/winpty/conpty.go (3)
PtyConfig(36-42)UserConfig(45-48)ExecutePtyWithUserToken(59-71)
client/ssh/server/userswitching_unix.go (4)
client/ssh/server/server.go (2)
New(170-184)Server(115-142)client/ssh/server/executor_windows.go (1)
NewPrivilegeDropper(45-47)client/ssh/server/executor_unix.go (2)
NewPrivilegeDropper(41-43)ExecutorConfig(27-35)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 / Benchmark (amd64, postgres)
- GitHub Check: Management / Integration (amd64, postgres)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Management / Benchmark (API) (amd64, sqlite)
- GitHub Check: Management / Benchmark (API) (amd64, postgres)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Management / Benchmark (amd64, sqlite)
- GitHub Check: Client / Unit (386)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Android / Build
- GitHub Check: release
- GitHub Check: release_ui_darwin
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
- GitHub Check: Linux
- GitHub Check: Client / Unit
- GitHub Check: Darwin
- GitHub Check: Windows
- GitHub Check: Client / Unit
🔇 Additional comments (9)
client/ssh/server/userswitching_unix.go (5)
25-60: LGTM! Comprehensive username validation.The validation logic follows POSIX standards with appropriate security checks (path components, numeric usernames) and practical limits.
76-100: LGTM! Platform-specific login command construction is correct.The platform-specific handling (including the Arch Linux PAM edge case) is well-implemented with proper error handling.
62-74: LGTM! PTY command creation logic is correct.Both
createPtyLoginCommandandcreateDirectPtyCommandproperly handle their respective privilege paths with appropriate environment and working directory setup.Also applies to: 202-214
216-233: LGTM! Environment preparation logic is sound.The environment assembly properly combines user, SSH, and terminal environments with appropriate filtering through
acceptEnvfor security.
155-181: All helper functions are properly defined in the codebase.Verification confirms that
getUserShell,prepareUserEnv,prepareSSHEnv, andacceptEnvare all defined inclient/ssh/server/shell.goand are accessible within the same package. No issues found.client/cmd/ssh.go (2)
390-408: Flag bindings correctly implemented.Both short and long flag forms now properly bind to the same struct fields using
IntVar/StringVar. For example,-pand--portboth updateflags.Port,-uand--userboth updateflags.Username, etc. This resolves the previous issue where long-form flags were not taking effect.
156-175: Error handling properly refactored.The goroutine now sends errors through
errChinstead of callingos.Exitdirectly, allowing proper cleanup via deferred functions and context cancellation. The mainselecthandles the error channel, signal interrupts, and context completion correctly.client/ssh/server/command_execution.go (1)
109-110: Stderr properly wired to separate stream.The code now correctly routes stderr to
session.Stderr()instead of collapsing both stdout and stderr to the session stream. This allows SSH clients and tooling to distinguish between the two streams.client/ssh/server/command_execution_windows.go (1)
16-58: Command execution flow looks well-structured.The Windows-specific command execution properly handles user environment retrieval, token management with cleanup via defer, and graceful fallback when profile loading fails. The flow correctly propagates the environment through to command preparation.
Describe your changes
NetBird SSH Client
SSH Server
New Flags
UI
Changes
jwksconfig to peers with enabled ssh serverjwtwith max token agejwts from the IdP and send these for authentication with remote ssh peersnetbird ssh detectcommand to detect if the remote peer is running the NetBird servernetbird ssh proxycommand for native ssh and sftp clients. The proxy requestsjwts just likenetbird ssh, passes them for authentication and bridges the connection between the native client and the remote server.Auth Flows
netbird ssh <peer>)flowchart TD A[User: netbird ssh <peer>] --> B[Connect to peer:22] B --> C[Detect Server Type] C --> D[Send 'netbird-detect' request] D --> E{Server Response} E -->|No NetBird identifier| F[Regular SSH Server] E -->|NetBird + JWT required| G[NetBird with JWT Auth] E -->|NetBird + No JWT| H[NetBird without JWT] F --> I[Standard SSH Connection] H --> I G --> J[Request JWT from NetBird daemon] J --> J1{Check JWT cache} J1 -->|Cache valid| J2[Use cached JWT token] J1 -->|No cache/expired| J3[OIDC flow:<br/>User authorizes via IDP callback] J3 --> J4[Receive & cache JWT token] J4 --> K[Connect to peer SSH server] J2 --> K K --> M[Send JWT authentication request] M --> N{JWT Valid?} N -->|No| O[Connection Rejected] N -->|Yes| P[SSH Session Established] I --> P P --> Q[Interactive Shell / Execute Command / Port Forwarding] style G fill:#f57c00,color:#fff style J fill:#1976d2,color:#fff style J1 fill:#1976d2,color:#fff style J3 fill:#e65100,color:#fff style P fill:#388e3c,color:#fffflowchart TD A[User: ssh <peer>] --> B[OpenSSH loads config:<br/>/etc/ssh/ssh_config.d/99-netbird.conf] B --> D{Host matches NetBird pattern?} D -->|No| E[Standard SSH connection] D -->|Yes| F[Run detection check:<br/>netbird ssh detect <peer> 22] F --> G{Is NetBird SSH server?} G -->|No| E G -->|Yes - JWT required| H[Activate ProxyCommand:<br/>netbird ssh proxy <peer> 22] H --> I[Local SSH Proxy Started] I --> J[OpenSSH connects to proxy via stdio] J --> K[Proxy requests JWT from daemon] K --> K1{Check JWT cache} K1 -->|Cache valid| K2[Use cached JWT token] K1 -->|No cache/expired| K3[OIDC flow:<br/>User authorizes via IDP callback] K3 --> K4[Receive & cache JWT token] K4 --> L[Proxy connects to peer SSH server] K2 --> L L --> M[Proxy sends JWT authentication] M --> N{JWT Valid?} N -->|No| O[Connection Rejected] N -->|Yes| P[Proxy establishes session] P --> Q[Bidirectional forwarding:<br/>OpenSSH ↔ Proxy ↔ Peer SSH Server] Q --> R[User interacts with remote shell] style H fill:#f57c00,color:#fff style I fill:#1976d2,color:#fff style K fill:#1976d2,color:#fff style K1 fill:#1976d2,color:#fff style K3 fill:#e65100,color:#fff style Q fill:#388e3c,color:#fff classDef proxyBox fill:#1565c0,stroke:#0d47a1,stroke-width:2px,color:#fff class I,K,L,M,P,Q proxyBoxIssue ticket number and link
Fixes #4759 #4672 #4456 #4039 #3985 #2498 #4457
Stack
Checklist
Documentation
Select exactly one:
Summary by CodeRabbit