mirror of
https://github.com/netbirdio/netbird.git
synced 2026-09-17 04:09:07 +02:00
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).
This commit is contained in:
@@ -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
|
||||
}
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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())
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user