diff --git a/client/ssh/config/manager_test.go b/client/ssh/config/manager_test.go index 1f9dd4f14..9733b4be6 100644 --- a/client/ssh/config/manager_test.go +++ b/client/ssh/config/manager_test.go @@ -210,11 +210,18 @@ func TestManager_DirectoryFallback(t *testing.T) { t.Setenv("HOME", tempDir) // Create manager with non-writable system directories - // Use /dev/null as parent to ensure failure on all systems + // Use paths that will fail on all systems + var failPath string + if runtime.GOOS == "windows" { + failPath = "NUL:" // Special device that can't be used as directory on Windows + } else { + failPath = "/dev/null" // Special device that can't be used as directory on Unix + } + manager := &Manager{ - sshConfigDir: "/dev/null/ssh_config.d", // Should fail + sshConfigDir: failPath + "/ssh_config.d", // Should fail sshConfigFile: "99-netbird.conf", - knownHostsDir: "/dev/null/ssh_known_hosts.d", // Should fail + knownHostsDir: failPath + "/ssh_known_hosts.d", // Should fail knownHostsFile: "99-netbird", userKnownHosts: "known_hosts_netbird", } diff --git a/client/ssh/server/executor_unix_test.go b/client/ssh/server/executor_unix_test.go index b0f4350b8..0c5108f57 100644 --- a/client/ssh/server/executor_unix_test.go +++ b/client/ssh/server/executor_unix_test.go @@ -4,6 +4,7 @@ package server import ( "context" + "fmt" "os" "os/exec" "os/user" @@ -135,13 +136,15 @@ func TestPrivilegeDropper_ActualPrivilegeDrop(t *testing.T) { } // Find a non-root user to test with - testUser, err := user.Lookup("nobody") + testUser, err := findNonRootUser() if err != nil { - // Try to find any non-root user - testUser, err = findNonRootUser() - if err != nil { - t.Skip("No suitable non-root user found for testing") - } + t.Skip("No suitable non-root user found for testing") + } + + // Verify the user actually exists by looking it up again + _, err = user.LookupId(testUser.Uid) + if err != nil { + t.Skipf("Test user %s (UID %s) does not exist on this system: %v", testUser.Username, testUser.Uid, err) } uid64, err := strconv.ParseUint(testUser.Uid, 10, 32) @@ -187,30 +190,40 @@ func TestPrivilegeDropper_ActualPrivilegeDrop(t *testing.T) { // findNonRootUser finds any non-root user on the system for testing func findNonRootUser() (*user.User, error) { - // Try common non-root users - commonUsers := []string{"nobody", "daemon", "bin", "sys", "sync", "games", "man", "lp", "mail", "news", "uucp", "proxy", "www-data", "backup", "list", "irc"} + // Try common non-root users, but avoid "nobody" on macOS due to negative UID issues + commonUsers := []string{"daemon", "bin", "sys", "sync", "games", "man", "lp", "mail", "news", "uucp", "proxy", "www-data", "backup", "list", "irc"} for _, username := range commonUsers { if u, err := user.Lookup(username); err == nil { - uid64, err := strconv.ParseUint(u.Uid, 10, 32) + // Parse as signed integer first to handle negative UIDs + uid64, err := strconv.ParseInt(u.Uid, 10, 32) if err != nil { continue } - if uid64 != 0 { // Not root + // Skip negative UIDs (like nobody=-2 on macOS) and root + if uid64 > 0 && uid64 != 0 { return u, nil } } } - // If no common users found, create a minimal user info for testing - // This won't actually work for privilege dropping but allows the test structure - return &user.User{ - Uid: "65534", // Standard nobody UID - Gid: "65534", // Standard nobody GID - Username: "nobody", - Name: "nobody", - HomeDir: "/nonexistent", - }, nil + // If no common users found, try to find any regular user with UID > 100 + // This helps on macOS where regular users start at UID 501 + allUsers := []string{"vma", "user", "test", "admin"} + for _, username := range allUsers { + if u, err := user.Lookup(username); err == nil { + uid64, err := strconv.ParseInt(u.Uid, 10, 32) + if err != nil { + continue + } + if uid64 > 100 { // Regular user + return u, nil + } + } + } + + // If no common users found, return an error + return nil, fmt.Errorf("no suitable non-root user found on this system") } func TestPrivilegeDropper_ExecuteWithPrivilegeDrop_Validation(t *testing.T) { diff --git a/client/ssh/server/server_config_test.go b/client/ssh/server/server_config_test.go index 1d649486b..1b10b1766 100644 --- a/client/ssh/server/server_config_test.go +++ b/client/ssh/server/server_config_test.go @@ -298,8 +298,14 @@ func TestServer_PortConflictHandling(t *testing.T) { assert.Error(t, err, "Second client should fail to bind to same port") if err != nil { // The error should indicate the address is already in use - assert.Contains(t, strings.ToLower(err.Error()), "address already in use", - "Error should indicate port conflict") + errMsg := strings.ToLower(err.Error()) + if runtime.GOOS == "windows" { + assert.Contains(t, errMsg, "only one usage of each socket address", + "Error should indicate port conflict") + } else { + assert.Contains(t, errMsg, "address already in use", + "Error should indicate port conflict") + } } // Cancel first client's context and wait for it to finish diff --git a/client/ssh/server/sftp_test.go b/client/ssh/server/sftp_test.go index 1f96f15de..9cbd950da 100644 --- a/client/ssh/server/sftp_test.go +++ b/client/ssh/server/sftp_test.go @@ -24,6 +24,10 @@ func TestSSHServer_SFTPSubsystem(t *testing.T) { t.Skip("Skipping SFTP test when running as root - may have protocol compatibility issues") } + // Get current user for SSH connection + currentUser, err := user.Current() + require.NoError(t, err, "Should be able to get current user") + // Generate host key for server hostKey, err := ssh.GeneratePrivateKey(ssh.ED25519) require.NoError(t, err) @@ -88,9 +92,7 @@ func TestSSHServer_SFTPSubsystem(t *testing.T) { require.NoError(t, err) hostPubKey := hostPrivParsed.PublicKey() - // Get current user for SSH connection - currentUser, err := user.Current() - require.NoError(t, err, "Should be able to get current user for test") + // (currentUser already obtained at function start) // Create SSH client connection clientConfig := &cryptossh.ClientConfig{ @@ -131,6 +133,10 @@ func TestSSHServer_SFTPSubsystem(t *testing.T) { } func TestSSHServer_SFTPDisabled(t *testing.T) { + // Get current user for SSH connection + currentUser, err := user.Current() + require.NoError(t, err, "Should be able to get current user") + // Generate host key for server hostKey, err := ssh.GeneratePrivateKey(ssh.ED25519) require.NoError(t, err) @@ -194,9 +200,7 @@ func TestSSHServer_SFTPDisabled(t *testing.T) { require.NoError(t, err) hostPubKey := hostPrivParsed.PublicKey() - // Get current user for SSH connection - currentUser, err := user.Current() - require.NoError(t, err, "Should be able to get current user for test") + // (currentUser already obtained at function start) // Create SSH client connection clientConfig := &cryptossh.ClientConfig{ diff --git a/client/ssh/server/userswitching_windows.go b/client/ssh/server/userswitching_windows.go index a77b303e8..f5dddf2b7 100644 --- a/client/ssh/server/userswitching_windows.go +++ b/client/ssh/server/userswitching_windows.go @@ -21,16 +21,23 @@ func validateUsername(username string) error { return fmt.Errorf("username cannot be empty") } + // Handle domain\username format - extract just the username part for validation + usernameToValidate := username + if idx := strings.LastIndex(username, `\`); idx != -1 { + usernameToValidate = username[idx+1:] + } + // Windows SAM Account Name limits: 20 characters for users, 16 for computers - // We use 20 as the general limit - if len(username) > 20 { + // We use 20 as the general limit (applies to username part only) + if len(usernameToValidate) > 20 { return fmt.Errorf("username too long (max 20 characters for Windows)") } // Check for Windows SAM Account Name invalid characters // Prohibited: " / \ [ ] : ; | = , + * ? < > + // Note: backslash is allowed in full username (domain\user) but not in the user part invalidChars := []rune{'"', '/', '\\', '[', ']', ':', ';', '|', '=', ',', '+', '*', '?', '<', '>'} - for _, char := range username { + for _, char := range usernameToValidate { for _, invalid := range invalidChars { if char == invalid { return fmt.Errorf("username contains invalid character '%c'", char) @@ -43,18 +50,18 @@ func validateUsername(username string) error { } // Period cannot be the final character - if strings.HasSuffix(username, ".") { + if strings.HasSuffix(usernameToValidate, ".") { return fmt.Errorf("username cannot end with a period") } // Check for reserved patterns - if username == "." || username == ".." { + if usernameToValidate == "." || usernameToValidate == ".." { return fmt.Errorf("username cannot be '.' or '..'") } - // Warn about @ character (causes login issues) - if strings.Contains(username, "@") { - log.Warnf("username '%s' contains '@' character which may cause login issues", username) + // Warn about @ character (causes login issues) - check in username part only + if strings.Contains(usernameToValidate, "@") { + log.Warnf("username '%s' contains '@' character which may cause login issues", usernameToValidate) } return nil