Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions proxy/cmd/proxy/cmd/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ var (
certLockMethod string
wgPort int
proxyProtocol bool
preSharedKey string
)

var rootCmd = &cobra.Command{
Expand Down Expand Up @@ -84,6 +85,7 @@ func init() {
rootCmd.Flags().StringVar(&certLockMethod, "cert-lock-method", envStringOrDefault("NB_PROXY_CERT_LOCK_METHOD", "auto"), "Certificate lock method for cross-replica coordination: auto, flock, or k8s-lease")
rootCmd.Flags().IntVar(&wgPort, "wg-port", envIntOrDefault("NB_PROXY_WG_PORT", 0), "WireGuard listen port (0 = random). Fixed port only works with single-account deployments")
rootCmd.Flags().BoolVar(&proxyProtocol, "proxy-protocol", envBoolOrDefault("NB_PROXY_PROXY_PROTOCOL", false), "Enable PROXY protocol on TCP listeners to preserve client IPs behind L4 proxies")
rootCmd.Flags().StringVar(&preSharedKey, "pre-shared-key", envStringOrDefault("NB_PROXY_PRE_SHARED_KEY", ""), "Define a pre-shared key for the tunnel between proxy and peers")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

PSK exposed in process listing via CLI flag.

WireGuard's own tooling (wg(8)) explicitly states that "command line arguments are not considered private on most systems" for pre-shared keys — they are visible to other processes via ps aux and /proc/<pid>/cmdline.

The existing ProxyToken deliberately has no CLI flag (lines 109–112) and is env-var-only precisely to avoid this leak. The WireGuard PSK is a cryptographic secret and warrants the same treatment. NB_PROXY_PRE_SHARED_KEY is already available as a safe alternative.

Consider removing the --pre-shared-key flag entirely and reading the PSK exclusively from the environment variable, mirroring the ProxyToken pattern:

🔒 Proposed fix: env-var-only PSK
-	rootCmd.Flags().StringVar(&preSharedKey, "pre-shared-key", envStringOrDefault("NB_PROXY_PRE_SHARED_KEY", ""), "Define a pre-shared key for the tunnel between proxy and peers")

In runServer:

+	preSharedKey := os.Getenv("NB_PROXY_PRE_SHARED_KEY")
 	srv := proxy.Server{

And remove the package-level preSharedKey variable.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@proxy/cmd/proxy/cmd/root.go` at line 88, Remove the CLI flag registration
that exposes the PSK (rootCmd.Flags().StringVar(&preSharedKey, "pre-shared-key",
...)) and stop using the package-level preSharedKey variable; instead read the
pre-shared key exclusively from the environment using
envStringOrDefault("NB_PROXY_PRE_SHARED_KEY", "") at runtime (same pattern used
for ProxyToken) and use that value inside runServer and any other consumers;
also delete the package-level preSharedKey declaration so there is no flag or
global var exposing the secret.

}

// Execute runs the root command.
Expand Down Expand Up @@ -156,6 +158,7 @@ func runServer(cmd *cobra.Command, args []string) error {
CertLockMethod: nbacme.CertLockMethod(certLockMethod),
WireguardPort: wgPort,
ProxyProtocol: proxyProtocol,
PreSharedKey: preSharedKey,
}

ctx, stop := signal.NotifyContext(context.Background(), syscall.SIGTERM, syscall.SIGINT)
Expand Down
22 changes: 14 additions & 8 deletions proxy/internal/roundtrip/netbird.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,13 @@ func (e *clientEntry) acquireInflight(backend backendKey) (release func(), ok bo
}
}

// ClientConfig holds configuration for the embedded NetBird client.
type ClientConfig struct {
MgmtAddr string
WGPort int
PreSharedKey string
}

type statusNotifier interface {
NotifyStatus(ctx context.Context, accountID, serviceID, domain string, connected bool) error
}
Expand All @@ -98,10 +105,9 @@ type managementClient interface {
// backed by underlying NetBird connections.
// Clients are keyed by AccountID, allowing multiple domains to share the same connection.
type NetBird struct {
mgmtAddr string
proxyID string
proxyAddr string
wgPort int
clientCfg ClientConfig
logger *log.Logger
mgmtClient managementClient
transportCfg transportConfig
Expand Down Expand Up @@ -229,11 +235,12 @@ func (n *NetBird) createClientEntry(ctx context.Context, accountID types.Account
// The peer has already been created via CreateProxyPeer RPC with the public key.
client, err := embed.New(embed.Options{
DeviceName: deviceNamePrefix + n.proxyID,
ManagementURL: n.mgmtAddr,
ManagementURL: n.clientCfg.MgmtAddr,
PrivateKey: privateKey.String(),
LogLevel: log.WarnLevel.String(),
BlockInbound: true,
WireguardPort: &n.wgPort,
WireguardPort: &n.clientCfg.WGPort,
PreSharedKey: n.clientCfg.PreSharedKey,
})
if err != nil {
return nil, fmt.Errorf("create netbird client: %w", err)
Expand Down Expand Up @@ -536,18 +543,17 @@ func (n *NetBird) ListClientsForStartup() map[types.AccountID]*embed.Client {
return result
}

// NewNetBird creates a new NetBird transport. Set wgPort to 0 for a random
// NewNetBird creates a new NetBird transport. Set clientCfg.WGPort to 0 for a random
// OS-assigned port. A fixed port only works with single-account deployments;
// multiple accounts will fail to bind the same port.
func NewNetBird(mgmtAddr, proxyID, proxyAddr string, wgPort int, logger *log.Logger, notifier statusNotifier, mgmtClient managementClient) *NetBird {
func NewNetBird(proxyID, proxyAddr string, clientCfg ClientConfig, logger *log.Logger, notifier statusNotifier, mgmtClient managementClient) *NetBird {
if logger == nil {
logger = log.StandardLogger()
}
return &NetBird{
mgmtAddr: mgmtAddr,
proxyID: proxyID,
proxyAddr: proxyAddr,
wgPort: wgPort,
clientCfg: clientCfg,
logger: logger,
clients: make(map[types.AccountID]*clientEntry),
statusNotifier: notifier,
Expand Down
18 changes: 15 additions & 3 deletions proxy/internal/roundtrip/netbird_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,11 @@ func (m *mockStatusNotifier) calls() []statusCall {
// mockNetBird creates a NetBird instance for testing without actually connecting.
// It uses an invalid management URL to prevent real connections.
func mockNetBird() *NetBird {
return NewNetBird("http://invalid.test:9999", "test-proxy", "invalid.test", 0, nil, nil, &mockMgmtClient{})
return NewNetBird("test-proxy", "invalid.test", ClientConfig{
MgmtAddr: "http://invalid.test:9999",
WGPort: 0,
PreSharedKey: "",
}, nil, nil, &mockMgmtClient{})
}

func TestNetBird_AddPeer_CreatesClientForNewAccount(t *testing.T) {
Expand Down Expand Up @@ -282,7 +286,11 @@ func TestNetBird_RoundTrip_RequiresExistingClient(t *testing.T) {

func TestNetBird_AddPeer_ExistingStartedClient_NotifiesStatus(t *testing.T) {
notifier := &mockStatusNotifier{}
nb := NewNetBird("http://invalid.test:9999", "test-proxy", "invalid.test", 0, nil, notifier, &mockMgmtClient{})
nb := NewNetBird("test-proxy", "invalid.test", ClientConfig{
MgmtAddr: "http://invalid.test:9999",
WGPort: 0,
PreSharedKey: "",
}, nil, notifier, &mockMgmtClient{})
accountID := types.AccountID("account-1")

// Add first domain — creates a new client entry.
Expand All @@ -308,7 +316,11 @@ func TestNetBird_AddPeer_ExistingStartedClient_NotifiesStatus(t *testing.T) {

func TestNetBird_RemovePeer_NotifiesDisconnection(t *testing.T) {
notifier := &mockStatusNotifier{}
nb := NewNetBird("http://invalid.test:9999", "test-proxy", "invalid.test", 0, nil, notifier, &mockMgmtClient{})
nb := NewNetBird("test-proxy", "invalid.test", ClientConfig{
MgmtAddr: "http://invalid.test:9999",
WGPort: 0,
PreSharedKey: "",
}, nil, notifier, &mockMgmtClient{})
accountID := types.AccountID("account-1")

err := nb.AddPeer(context.Background(), accountID, domain.Domain("domain1.test"), "key-1", "svc-1")
Expand Down
8 changes: 7 additions & 1 deletion proxy/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,8 @@ type Server struct {
// When enabled, the real client IP is extracted from the PROXY header
// sent by upstream L4 proxies that support PROXY protocol.
ProxyProtocol bool
// PreSharedKey used for tunnel between proxy and peers (set globally not per account)
PreSharedKey string
}

// NotifyStatus sends a status update to management about tunnel connectivity
Expand Down Expand Up @@ -163,7 +165,11 @@ func (s *Server) ListenAndServe(ctx context.Context, addr string) (err error) {

// Initialize the netbird client, this is required to build peer connections
// to proxy over.
s.netbird = roundtrip.NewNetBird(s.ManagementAddress, s.ID, s.ProxyURL, s.WireguardPort, s.Logger, s, s.mgmtClient)
s.netbird = roundtrip.NewNetBird(s.ID, s.ProxyURL, roundtrip.ClientConfig{
MgmtAddr: s.ManagementAddress,
WGPort: s.WireguardPort,
PreSharedKey: s.PreSharedKey,
}, s.Logger, s, s.mgmtClient)

tlsConfig, err := s.configureTLS(ctx)
if err != nil {
Expand Down
Loading