Fix tests and windows username validation

This commit is contained in:
Viktor Liu
2025-07-03 01:58:15 +02:00
parent 76f9e11b29
commit 6e15882c11
5 changed files with 75 additions and 38 deletions

View File

@@ -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",
}

View File

@@ -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) {

View File

@@ -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

View File

@@ -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{

View File

@@ -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