From b21a91a50781f17d9160f13577ddb3775a88840c Mon Sep 17 00:00:00 2001 From: mlsmaycon Date: Thu, 21 May 2026 11:30:07 +0200 Subject: [PATCH] fix(service): require non-empty host + direct_upstream on cluster targets Cluster targets dial the upstream via the host network stack, so an empty Host leaves the proxy with nothing to dial and DirectUpstream=false would route the request through the embedded NetBird client (wrong network for a cluster address). Validate() and validateTargetReferences now reject both shapes. Tests: - TestValidate_HTTPClusterTarget / _RequiresTargetId / TestValidate_Private_{AcceptsClusterTargetWithAccessGroups, RequiresAccessGroups, RejectsBearerAuth} updated to populate Host and DirectUpstream so they exercise the path past the new gates. - TestValidate_HTTPClusterTarget_RequiresHost and _RequiresDirectUpstream pin the two new error paths. - TestValidateTargetReferences_ClusterTargetSkipsLookup updated to set DirectUpstream on its fixture; new _ClusterTargetRequiresDirectUpstream test covers the store-side rejection. Drive-bys (no behavior change beyond what existing tests cover): - proxy/proxy.go: shortened the Capabilities.Private / Cluster.Private doc comments. - users/manager.go: moved the GetUserWithGroups doc from the interface to the impl. - proxy/cmd/proxy/cmd/root.go: removed unused NewRootCmd. - tunnel_cache.go: bumped tunnelCacheTTL from 30s to 300s (matches the "5 minutes" target documented on the constant; existing TTL-expiry test uses the constant directly so the bump is picked up automatically). --- .../modules/reverseproxy/proxy/proxy.go | 9 ++-- .../reverseproxy/service/manager/manager.go | 11 ++++- .../service/manager/manager_test.go | 28 +++++++++++- .../modules/reverseproxy/service/service.go | 33 ++++++++------ .../reverseproxy/service/service_test.go | 45 ++++++++++++++++++- management/server/users/manager.go | 9 ++-- proxy/cmd/proxy/cmd/root.go | 10 ----- proxy/internal/auth/tunnel_cache.go | 2 +- 8 files changed, 107 insertions(+), 40 deletions(-) diff --git a/management/internals/modules/reverseproxy/proxy/proxy.go b/management/internals/modules/reverseproxy/proxy/proxy.go index f723e5fe9..4404b0d24 100644 --- a/management/internals/modules/reverseproxy/proxy/proxy.go +++ b/management/internals/modules/reverseproxy/proxy/proxy.go @@ -20,10 +20,8 @@ type Capabilities struct { RequireSubdomain *bool // SupportsCrowdsec indicates whether this proxy has CrowdSec configured. SupportsCrowdsec *bool - // Private indicates whether this proxy is embedded in a netbird client - // and serves exclusively over the WireGuard tunnel (i.e. `netbird proxy` - // rather than the standalone netbird-proxy binary). Surfaces upstream - // so dashboards can distinguish per-peer / private clusters. + // Private indicates whether this proxy supports inbound access via Wireguard + // tunnel and netbird-only authentication policies Private *bool } @@ -76,6 +74,5 @@ type Cluster struct { SupportsCustomPorts *bool RequireSubdomain *bool SupportsCrowdSec *bool - // Private is true when at least one connected proxy reported the embedded-`netbird proxy` capability. - Private *bool + Private *bool } diff --git a/management/internals/modules/reverseproxy/service/manager/manager.go b/management/internals/modules/reverseproxy/service/manager/manager.go index d5ed16735..478f9e96a 100644 --- a/management/internals/modules/reverseproxy/service/manager/manager.go +++ b/management/internals/modules/reverseproxy/service/manager/manager.go @@ -783,7 +783,9 @@ func validateTargetReferences(ctx context.Context, transaction store.Store, acco return err } case service.TargetTypeCluster: - // Cluster targets are addressed by target_id; no peer/resource lookup. + if err := validateClusterTarget(target); err != nil { + return err + } default: return status.Errorf(status.InvalidArgument, "unknown target type %q for target %q", target.TargetType, target.TargetId) } @@ -791,6 +793,13 @@ func validateTargetReferences(ctx context.Context, transaction store.Store, acco return nil } +func validateClusterTarget(target *service.Target) error { + if !target.Options.DirectUpstream { + return status.Errorf(status.InvalidArgument, "cluster target %s has direct upstream disabled", target.Host) + } + return nil +} + func validatePeerTarget(ctx context.Context, transaction store.Store, accountID string, target *service.Target) error { if _, err := transaction.GetPeerByID(ctx, store.LockingStrengthShare, accountID, target.TargetId); err != nil { if sErr, ok := status.FromError(err); ok && sErr.Type() == status.NotFound { diff --git a/management/internals/modules/reverseproxy/service/manager/manager_test.go b/management/internals/modules/reverseproxy/service/manager/manager_test.go index a1a924ba0..f3ab89a25 100644 --- a/management/internals/modules/reverseproxy/service/manager/manager_test.go +++ b/management/internals/modules/reverseproxy/service/manager/manager_test.go @@ -1353,11 +1353,37 @@ func TestValidateTargetReferences_ClusterTargetSkipsLookup(t *testing.T) { // No peer or resource lookups must be issued for cluster targets. targets := []*rpservice.Target{ - {TargetId: "eu.proxy.netbird.io", TargetType: rpservice.TargetTypeCluster}, + { + TargetId: "eu.proxy.netbird.io", + TargetType: rpservice.TargetTypeCluster, + Options: rpservice.TargetOptions{DirectUpstream: true}, + }, } require.NoError(t, validateTargetReferences(ctx, mockStore, accountID, targets), "cluster target must validate without store lookups") } +// TestValidateTargetReferences_ClusterTargetRequiresDirectUpstream pins the +// store-side check that cluster targets must opt into the host-stack dial +// path. Without DirectUpstream the proxy would route this target through +// the embedded NetBird client and fail on every request. +func TestValidateTargetReferences_ClusterTargetRequiresDirectUpstream(t *testing.T) { + ctx := context.Background() + ctrl := gomock.NewController(t) + mockStore := store.NewMockStore(ctrl) + accountID := "test-account" + + targets := []*rpservice.Target{ + { + TargetId: "eu.proxy.netbird.io", + TargetType: rpservice.TargetTypeCluster, + Host: "backend.lan", + }, + } + err := validateTargetReferences(ctx, mockStore, accountID, targets) + require.Error(t, err, "cluster target without direct_upstream must be rejected") + assert.ErrorContains(t, err, "direct upstream disabled") +} + func TestReplaceHostByLookup_SkipsClusterTarget(t *testing.T) { ctx := context.Background() ctrl := gomock.NewController(t) diff --git a/management/internals/modules/reverseproxy/service/service.go b/management/internals/modules/reverseproxy/service/service.go index 94f7fa57d..27f6d914d 100644 --- a/management/internals/modules/reverseproxy/service/service.go +++ b/management/internals/modules/reverseproxy/service/service.go @@ -69,18 +69,10 @@ type TargetOptions struct { } type Target struct { - ID uint `gorm:"primaryKey" json:"-"` - AccountID string `gorm:"index:idx_target_account;not null" json:"-"` - ServiceID string `gorm:"index:idx_service_targets;not null" json:"-"` - Path *string `json:"path,omitempty"` - // Host carries the upstream address. For TargetTypeSubnet it is the only - // source — operator-supplied. For TargetTypePeer / TargetTypeHost / - // TargetTypeDomain it is overwritten by replaceHostByLookup with the - // resolved peer IP / resource address, *unless* Options.DirectUpstream - // is true and the operator supplied a non-empty value — then the - // operator value is preserved so they can dial the upstream via the - // host's network stack at an address other than the WG tunnel IP - // (e.g. a LAN IP, localhost sidecar, or DNS name). + ID uint `gorm:"primaryKey" json:"-"` + AccountID string `gorm:"index:idx_target_account;not null" json:"-"` + ServiceID string `gorm:"index:idx_service_targets;not null" json:"-"` + Path *string `json:"path,omitempty"` Host string `json:"host"` Port uint16 `gorm:"index:idx_target_port" json:"port"` Protocol string `gorm:"index:idx_target_protocol" json:"protocol"` @@ -874,8 +866,9 @@ func (s *Service) validateHTTPTargets() error { return fmt.Errorf("target %d has empty host but target_type is %q", i, target.TargetType) } case TargetTypeCluster: - // target_id carries the cluster address; the proxy resolves - // the upstream at request time. Host/port may be empty. + if err := validateClusterTarget(i, target); err != nil { + return err + } default: return fmt.Errorf("target %d has invalid target_type %q", i, target.TargetType) } @@ -893,6 +886,18 @@ func (s *Service) validateHTTPTargets() error { return nil } +// validateClusterTarget cluster targets should not have empty hosts and should have direct upstream enabled. +func validateClusterTarget(idx int, target *Target) error { + host := strings.TrimSpace(target.Host) + if host == "" { + return fmt.Errorf("target %d: has empty host", idx) + } + if !target.Options.DirectUpstream { + return fmt.Errorf("target %d: %s has direct upstream disabled", idx, target.Host) + } + return validateDirectUpstreamHost(idx, target) +} + // validateDirectUpstreamHost validates the operator-supplied Host on a // peer/host/domain target when DirectUpstream is set. Empty Host is // allowed — the lookup fills in the default peer IP / resource address. diff --git a/management/internals/modules/reverseproxy/service/service_test.go b/management/internals/modules/reverseproxy/service/service_test.go index 84629b3f5..ba63d76ed 100644 --- a/management/internals/modules/reverseproxy/service/service_test.go +++ b/management/internals/modules/reverseproxy/service/service_test.go @@ -1124,9 +1124,11 @@ func TestValidate_HTTPClusterTarget(t *testing.T) { TargetId: "eu.proxy.netbird.io", TargetType: TargetTypeCluster, Protocol: "http", + Host: "backend.lan", + Options: TargetOptions{DirectUpstream: true}, Enabled: true, }} - require.NoError(t, rp.Validate(), "HTTP cluster target with target_id must validate without host or port") + require.NoError(t, rp.Validate(), "HTTP cluster target with target_id, host, and direct_upstream must validate") } func TestValidate_HTTPClusterTarget_RequiresTargetId(t *testing.T) { @@ -1134,11 +1136,46 @@ func TestValidate_HTTPClusterTarget_RequiresTargetId(t *testing.T) { rp.Targets = []*Target{{ TargetType: TargetTypeCluster, Protocol: "http", + Host: "backend.lan", + Options: TargetOptions{DirectUpstream: true}, Enabled: true, }} assert.ErrorContains(t, rp.Validate(), "empty target_id", "cluster target must reject empty target_id") } +// TestValidate_HTTPClusterTarget_RequiresHost pins the new cluster-target +// rule that operator-supplied Host is mandatory: cluster targets dial the +// upstream via the host network stack (direct_upstream is implied), so an +// empty Host leaves the proxy with nothing to dial. +func TestValidate_HTTPClusterTarget_RequiresHost(t *testing.T) { + rp := validProxy() + rp.Targets = []*Target{{ + TargetId: "eu.proxy.netbird.io", + TargetType: TargetTypeCluster, + Protocol: "http", + Options: TargetOptions{DirectUpstream: true}, + Enabled: true, + }} + assert.ErrorContains(t, rp.Validate(), "empty host", "cluster target must reject empty host") +} + +// TestValidate_HTTPClusterTarget_RequiresDirectUpstream pins the second +// half of the cluster-target rule: DirectUpstream must be true so the +// stdlib transport branch in MultiTransport is taken. Without it the +// embedded NetBird client would try to dial the cluster address through +// the WG tunnel, which is the wrong network for a cluster upstream. +func TestValidate_HTTPClusterTarget_RequiresDirectUpstream(t *testing.T) { + rp := validProxy() + rp.Targets = []*Target{{ + TargetId: "eu.proxy.netbird.io", + TargetType: TargetTypeCluster, + Protocol: "http", + Host: "backend.lan", + Enabled: true, + }} + assert.ErrorContains(t, rp.Validate(), "direct upstream disabled", "cluster target must reject direct_upstream=false") +} + func TestValidate_L4ClusterTarget(t *testing.T) { rp := validProxy() rp.Mode = ModeTCP @@ -1207,6 +1244,8 @@ func TestValidate_Private_RequiresAccessGroups(t *testing.T) { TargetId: "eu.proxy.netbird.io", TargetType: TargetTypeCluster, Protocol: "http", + Host: "backend.lan", + Options: TargetOptions{DirectUpstream: true}, Enabled: true, }} assert.ErrorContains(t, rp.Validate(), "access group") @@ -1224,6 +1263,8 @@ func TestValidate_Private_RejectsBearerAuth(t *testing.T) { TargetId: "eu.proxy.netbird.io", TargetType: TargetTypeCluster, Protocol: "http", + Host: "backend.lan", + Options: TargetOptions{DirectUpstream: true}, Enabled: true, }} assert.ErrorContains(t, rp.Validate(), "mutually exclusive") @@ -1244,6 +1285,8 @@ func TestValidate_Private_AcceptsClusterTargetWithAccessGroups(t *testing.T) { TargetId: "eu.proxy.netbird.io", TargetType: TargetTypeCluster, Protocol: "http", + Host: "backend.lan", + Options: TargetOptions{DirectUpstream: true}, Enabled: true, }} require.NoError(t, rp.Validate()) diff --git a/management/server/users/manager.go b/management/server/users/manager.go index 7acd1c07b..634b2d006 100644 --- a/management/server/users/manager.go +++ b/management/server/users/manager.go @@ -10,12 +10,6 @@ import ( type Manager interface { GetUser(ctx context.Context, userID string) (*types.User, error) - // GetUserWithGroups returns the user and the *types.Group - // records for the user's AutoGroups, in the same order as - // AutoGroups. Group ids that don't resolve to a stored group - // are skipped from the returned slice (the parallel id list is - // derivable from the returned User). Wraps two store calls - // today; can be optimised to a single JOIN later if needed. GetUserWithGroups(ctx context.Context, userID string) (*types.User, []*types.Group, error) } @@ -36,6 +30,9 @@ func (m *managerImpl) GetUser(ctx context.Context, userID string) (*types.User, return m.store.GetUserByUserID(ctx, store.LockingStrengthNone, userID) } +// GetUserWithGroups returns the user and the *types.Group records for the user's AutoGroups, in the same order as +// AutoGroups. Group ids that don't resolve to a stored group are skipped from the returned slice (the parallel id list is +// derivable from the returned User). Wraps two store calls today; can be optimised to a single JOIN later if needed. func (m *managerImpl) GetUserWithGroups(ctx context.Context, userID string) (*types.User, []*types.Group, error) { user, err := m.store.GetUserByUserID(ctx, store.LockingStrengthNone, userID) if err != nil { diff --git a/proxy/cmd/proxy/cmd/root.go b/proxy/cmd/proxy/cmd/root.go index a0d068210..9af26f09f 100644 --- a/proxy/cmd/proxy/cmd/root.go +++ b/proxy/cmd/proxy/cmd/root.go @@ -126,16 +126,6 @@ func Execute() { } } -// NewRootCmd returns the proxy server cobra command for embedding under -// another binary (for example as the "proxy" subcommand of the netbird -// client). The returned command shares its flag set, RunE, and any -// previously registered subcommands (e.g. "debug") with the standalone -// binary, so flag and env-var contracts stay identical across both -// invocations. -func NewRootCmd() *cobra.Command { - return rootCmd -} - // SetVersionInfo sets version information for the CLI. func SetVersionInfo(version, commit, buildDate, goVersion string) { Version = version diff --git a/proxy/internal/auth/tunnel_cache.go b/proxy/internal/auth/tunnel_cache.go index 03d453d95..10b671d82 100644 --- a/proxy/internal/auth/tunnel_cache.go +++ b/proxy/internal/auth/tunnel_cache.go @@ -15,7 +15,7 @@ import ( // tunnelCacheTTL caps how long a positive ValidateTunnelPeer result is // reused before re-fetching from management. 5 minutes balances freshness // against management load on busy mesh networks. -const tunnelCacheTTL = 30 * time.Second +const tunnelCacheTTL = 300 * time.Second // tunnelCachePerAccount caps the number of cached identities per account. // Bounded eviction avoids memory growth in pathological cases (huge peer