mirror of
https://github.com/netbirdio/netbird.git
synced 2026-07-19 04:59:56 +00:00
## Describe your changes Validate peer-supplied FQDN and hostname before they are written into the generated NetBird SSH client config (`client/ssh/config/manager.go`). These values originate from remote peers and were previously written verbatim into the config; malformed values (e.g. containing unexpected characters) could produce a broken or unintended config. FQDN/hostname are now checked with `domain.IsValidDomainNoWildcard`, and invalid, non-empty values are dropped with a warning. IPs are unaffected (already validated `netip.Addr`). Added a test covering malformed hostnames. ## Issue ticket number and link Internal input-validation hardening for peer-supplied hostnames in the generated SSH client config (`client/ssh/config/manager.go`). ## Stack - \#6726 <!-- branch-stack --> - \#6805 :point\_left: ### Checklist - [x] Is it a bug fix - [ ] Is a typo/documentation fix - [ ] Is a feature enhancement - [ ] It is a refactor - [x] Created tests that fail without the change (if possible) > By submitting this pull request, you confirm that you have read and agree to the terms of the [Contributor License Agreement](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTOR_LICENSE_AGREEMENT.md). ## Documentation Select exactly one: - [ ] I added/updated documentation for this change - [x] Documentation is **not needed** for this change (explain why) Internal client SSH config generation. No public API, gRPC, CLI/service flag, or configuration change — only input validation on peer-supplied hostnames before they are written to the generated ssh\_config. ### Docs PR URL (required if "docs added" is checked) Paste the PR link from <https://github.com/netbirdio/docs> here: N/A
231 lines
7.5 KiB
Go
231 lines
7.5 KiB
Go
package config
|
|
|
|
import (
|
|
"fmt"
|
|
"net/netip"
|
|
"os"
|
|
"path/filepath"
|
|
"runtime"
|
|
"strings"
|
|
"testing"
|
|
|
|
"github.com/stretchr/testify/assert"
|
|
"github.com/stretchr/testify/require"
|
|
)
|
|
|
|
func TestManager_SetupSSHClientConfig(t *testing.T) {
|
|
// Create temporary directory for test
|
|
tempDir, err := os.MkdirTemp("", "netbird-ssh-config-test")
|
|
require.NoError(t, err)
|
|
defer func() { assert.NoError(t, os.RemoveAll(tempDir)) }()
|
|
|
|
// Override manager paths to use temp directory
|
|
manager := &Manager{
|
|
sshConfigDir: filepath.Join(tempDir, "ssh_config.d"),
|
|
sshConfigFile: "99-netbird.conf",
|
|
}
|
|
|
|
// Test SSH config generation with peers
|
|
peers := []PeerSSHInfo{
|
|
{
|
|
Hostname: "peer1",
|
|
IP: netip.MustParseAddr("100.125.1.1"),
|
|
FQDN: "peer1.nb.internal",
|
|
},
|
|
{
|
|
Hostname: "peer2",
|
|
IP: netip.MustParseAddr("100.125.1.2"),
|
|
FQDN: "peer2.nb.internal",
|
|
},
|
|
}
|
|
|
|
err = manager.SetupSSHClientConfig(peers)
|
|
require.NoError(t, err)
|
|
|
|
// Read generated config
|
|
configPath := filepath.Join(manager.sshConfigDir, manager.sshConfigFile)
|
|
content, err := os.ReadFile(configPath)
|
|
require.NoError(t, err)
|
|
|
|
configStr := string(content)
|
|
|
|
// Verify the basic SSH config structure exists
|
|
assert.Contains(t, configStr, "# NetBird SSH client configuration")
|
|
assert.Contains(t, configStr, "Generated automatically - do not edit manually")
|
|
|
|
// Check that peer hostnames are included
|
|
assert.Contains(t, configStr, "100.125.1.1")
|
|
assert.Contains(t, configStr, "100.125.1.2")
|
|
assert.Contains(t, configStr, "peer1.nb.internal")
|
|
assert.Contains(t, configStr, "peer2.nb.internal")
|
|
|
|
// Check platform-specific UserKnownHostsFile
|
|
if runtime.GOOS == "windows" {
|
|
assert.Contains(t, configStr, "UserKnownHostsFile NUL")
|
|
} else {
|
|
assert.Contains(t, configStr, "UserKnownHostsFile /dev/null")
|
|
}
|
|
}
|
|
|
|
func TestGetSystemSSHConfigDir(t *testing.T) {
|
|
configDir := getSystemSSHConfigDir()
|
|
|
|
// Path should not be empty
|
|
assert.NotEmpty(t, configDir)
|
|
|
|
// Should be an absolute path
|
|
assert.True(t, filepath.IsAbs(configDir))
|
|
|
|
// On Unix systems, should start with /etc
|
|
// On Windows, should contain ProgramData
|
|
if runtime.GOOS == "windows" {
|
|
assert.Contains(t, strings.ToLower(configDir), "programdata")
|
|
} else {
|
|
assert.Contains(t, configDir, "/etc/ssh")
|
|
}
|
|
}
|
|
|
|
func TestManager_PeerLimit(t *testing.T) {
|
|
// Create temporary directory for test
|
|
tempDir, err := os.MkdirTemp("", "netbird-ssh-config-test")
|
|
require.NoError(t, err)
|
|
defer func() { assert.NoError(t, os.RemoveAll(tempDir)) }()
|
|
|
|
// Override manager paths to use temp directory
|
|
manager := &Manager{
|
|
sshConfigDir: filepath.Join(tempDir, "ssh_config.d"),
|
|
sshConfigFile: "99-netbird.conf",
|
|
}
|
|
|
|
// Generate many peers (more than limit)
|
|
var peers []PeerSSHInfo
|
|
for i := 0; i < MaxPeersForSSHConfig+10; i++ {
|
|
peers = append(peers, PeerSSHInfo{
|
|
Hostname: fmt.Sprintf("peer%d", i),
|
|
IP: netip.MustParseAddr(fmt.Sprintf("100.125.1.%d", i%254+1)),
|
|
FQDN: fmt.Sprintf("peer%d.nb.internal", i),
|
|
})
|
|
}
|
|
|
|
// Test that SSH config generation is skipped when too many peers
|
|
err = manager.SetupSSHClientConfig(peers)
|
|
require.NoError(t, err)
|
|
|
|
// Config should not be created due to peer limit
|
|
configPath := filepath.Join(manager.sshConfigDir, manager.sshConfigFile)
|
|
_, err = os.Stat(configPath)
|
|
assert.True(t, os.IsNotExist(err), "SSH config should not be created with too many peers")
|
|
}
|
|
|
|
func TestManager_MatchHostFormat(t *testing.T) {
|
|
tempDir, err := os.MkdirTemp("", "netbird-ssh-config-test")
|
|
require.NoError(t, err)
|
|
defer func() { assert.NoError(t, os.RemoveAll(tempDir)) }()
|
|
|
|
manager := &Manager{
|
|
sshConfigDir: filepath.Join(tempDir, "ssh_config.d"),
|
|
sshConfigFile: "99-netbird.conf",
|
|
}
|
|
|
|
peers := []PeerSSHInfo{
|
|
{Hostname: "peer1", IP: netip.MustParseAddr("100.125.1.1"), FQDN: "peer1.nb.internal"},
|
|
{Hostname: "peer2", IP: netip.MustParseAddr("100.125.1.2"), FQDN: "peer2.nb.internal"},
|
|
}
|
|
|
|
err = manager.SetupSSHClientConfig(peers)
|
|
require.NoError(t, err)
|
|
|
|
configPath := filepath.Join(manager.sshConfigDir, manager.sshConfigFile)
|
|
content, err := os.ReadFile(configPath)
|
|
require.NoError(t, err)
|
|
configStr := string(content)
|
|
|
|
// Must use "Match host" with comma-separated patterns, not a bare "Host" directive.
|
|
// A bare "Host" followed by "Match exec" is incorrect per ssh_config(5): the Host block
|
|
// ends at the next Match keyword, making it a no-op and leaving the Match exec unscoped.
|
|
assert.NotContains(t, configStr, "\nHost ", "should not use bare Host directive")
|
|
assert.Contains(t, configStr, "Match host \"100.125.1.1,peer1.nb.internal,peer1,100.125.1.2,peer2.nb.internal,peer2\"",
|
|
"should use Match host with comma-separated patterns")
|
|
}
|
|
|
|
func TestManager_HostPatternInjection(t *testing.T) {
|
|
tempDir, err := os.MkdirTemp("", "netbird-ssh-config-test")
|
|
require.NoError(t, err)
|
|
defer func() { assert.NoError(t, os.RemoveAll(tempDir)) }()
|
|
|
|
manager := &Manager{
|
|
sshConfigDir: filepath.Join(tempDir, "ssh_config.d"),
|
|
sshConfigFile: "99-netbird.conf",
|
|
}
|
|
|
|
// A malicious peer FQDN/hostname attempts to break out of the Match host
|
|
// directive and inject arbitrary ssh_config (a ProxyCommand executing a
|
|
// command). It must be rejected, not written to the config.
|
|
peers := []PeerSSHInfo{
|
|
{
|
|
Hostname: "evil\"\n ProxyCommand touch /tmp/pwned\nHost x",
|
|
IP: netip.MustParseAddr("100.125.1.1"),
|
|
FQDN: "evil\"\n ProxyCommand touch /tmp/pwned\nHost x.nb.internal",
|
|
},
|
|
{Hostname: "peer2", IP: netip.MustParseAddr("100.125.1.2"), FQDN: "peer2.nb.internal"},
|
|
}
|
|
|
|
err = manager.SetupSSHClientConfig(peers)
|
|
require.NoError(t, err)
|
|
|
|
configPath := filepath.Join(manager.sshConfigDir, manager.sshConfigFile)
|
|
content, err := os.ReadFile(configPath)
|
|
require.NoError(t, err)
|
|
configStr := string(content)
|
|
|
|
assert.NotContains(t, configStr, "ProxyCommand touch /tmp/pwned",
|
|
"injected directive must not appear in generated config")
|
|
assert.NotContains(t, configStr, "evil",
|
|
"malicious pattern must be dropped entirely")
|
|
// The valid peer must still be present, on a single Match host line.
|
|
assert.Contains(t, configStr, "Match host \"100.125.1.1,100.125.1.2,peer2.nb.internal,peer2\"",
|
|
"valid peers must survive, injected patterns dropped")
|
|
}
|
|
|
|
func TestManager_ForcedSSHConfig(t *testing.T) {
|
|
// Set force environment variable
|
|
t.Setenv(EnvForceSSHConfig, "true")
|
|
|
|
// Create temporary directory for test
|
|
tempDir, err := os.MkdirTemp("", "netbird-ssh-config-test")
|
|
require.NoError(t, err)
|
|
defer func() { assert.NoError(t, os.RemoveAll(tempDir)) }()
|
|
|
|
// Override manager paths to use temp directory
|
|
manager := &Manager{
|
|
sshConfigDir: filepath.Join(tempDir, "ssh_config.d"),
|
|
sshConfigFile: "99-netbird.conf",
|
|
}
|
|
|
|
// Generate many peers (more than limit)
|
|
var peers []PeerSSHInfo
|
|
for i := 0; i < MaxPeersForSSHConfig+10; i++ {
|
|
peers = append(peers, PeerSSHInfo{
|
|
Hostname: fmt.Sprintf("peer%d", i),
|
|
IP: netip.MustParseAddr(fmt.Sprintf("100.125.1.%d", i%254+1)),
|
|
FQDN: fmt.Sprintf("peer%d.nb.internal", i),
|
|
})
|
|
}
|
|
|
|
// Test that SSH config generation is forced despite many peers
|
|
err = manager.SetupSSHClientConfig(peers)
|
|
require.NoError(t, err)
|
|
|
|
// Config should be created despite peer limit due to force flag
|
|
configPath := filepath.Join(manager.sshConfigDir, manager.sshConfigFile)
|
|
_, err = os.Stat(configPath)
|
|
require.NoError(t, err, "SSH config should be created when forced")
|
|
|
|
// Verify config contains peer hostnames
|
|
content, err := os.ReadFile(configPath)
|
|
require.NoError(t, err)
|
|
configStr := string(content)
|
|
assert.Contains(t, configStr, "peer0.nb.internal")
|
|
assert.Contains(t, configStr, "peer1.nb.internal")
|
|
}
|