From fa1665fc3dbbe2e2021229862c72145edc13c65d Mon Sep 17 00:00:00 2001 From: Brad Ison Date: Thu, 6 Aug 2026 18:12:36 +0200 Subject: [PATCH] feat(reverseproxy): reserve a dedicated gateway address from cluster selection A self-addressed agent-network pin means the account's gateway proxy serves exactly the gateway, so its address is dropped from the account's reverse-proxy cluster allow list -- and because the free-domain suffix match is depth-independent, dropping the address rejects every name beneath it as well as the bare one. This closes the path where a tenant could create ordinary services under their own gateway hostname and have them delivered to single-purpose gateway infrastructure. The exclusion is derived from the account's own settings row: the allow list only ever contains the account's own BYOP addresses plus the shared public ones, so the account's own gateway address is the only one that ever needs excluding, and the store already records the fact. No config, no schema change -- a labeled pin reserves nothing, and a self-hosted deployment running a dedicated gateway gets the same invariant. A settings-lookup outage fails closed (error, not an empty reservation). Co-Authored-By: Claude Fable 5 --- .../reverseproxy/domain/manager/manager.go | 37 ++++- .../domain/manager/manager_test.go | 149 +++++++++++++++++- 2 files changed, 177 insertions(+), 9 deletions(-) diff --git a/management/internals/modules/reverseproxy/domain/manager/manager.go b/management/internals/modules/reverseproxy/domain/manager/manager.go index 3c0f0d73b..a9774d0e9 100644 --- a/management/internals/modules/reverseproxy/domain/manager/manager.go +++ b/management/internals/modules/reverseproxy/domain/manager/manager.go @@ -2,24 +2,28 @@ package manager import ( "context" + "errors" "fmt" "net" "strings" log "github.com/sirupsen/logrus" + agentnetworkTypes "github.com/netbirdio/netbird/management/internals/modules/agentnetwork/types" "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/domain" "github.com/netbirdio/netbird/management/server/account" "github.com/netbirdio/netbird/management/server/activity" "github.com/netbirdio/netbird/management/server/permissions" "github.com/netbirdio/netbird/management/server/permissions/modules" "github.com/netbirdio/netbird/management/server/permissions/operations" + nbstore "github.com/netbirdio/netbird/management/server/store" "github.com/netbirdio/netbird/management/server/types" "github.com/netbirdio/netbird/shared/management/status" ) type store interface { GetAccount(ctx context.Context, accountID string) (*types.Account, error) + GetAgentNetworkSettings(ctx context.Context, lockStrength nbstore.LockingStrength, accountID string) (*agentnetworkTypes.Settings, error) GetCustomDomain(ctx context.Context, accountID string, domainID string) (*domain.Domain, error) ListFreeDomains(ctx context.Context, accountID string) ([]string, error) @@ -311,17 +315,21 @@ func (m Manager) getClusterAllowList(ctx context.Context, accountID string) ([]s if err != nil { return nil, fmt.Errorf("get public cluster addresses: %w", err) } + reserved, err := m.reservedGatewayAddress(ctx, accountID) + if err != nil { + return nil, err + } seen := make(map[string]struct{}, len(byopAddresses)+len(publicAddresses)) merged := make([]string, 0, len(byopAddresses)+len(publicAddresses)) for _, addr := range byopAddresses { - if _, ok := seen[addr]; ok { + if _, ok := seen[addr]; ok || addr == reserved { continue } seen[addr] = struct{}{} merged = append(merged, addr) } for _, addr := range publicAddresses { - if _, ok := seen[addr]; ok { + if _, ok := seen[addr]; ok || addr == reserved { continue } seen[addr] = struct{}{} @@ -330,6 +338,31 @@ func (m Manager) getClusterAllowList(ctx context.Context, accountID string) ([]s return merged, nil } +// reservedGatewayAddress returns the account's agent-network gateway address +// when its settings pin is self-addressed — a proxy dedicated to serving +// exactly the gateway. Dropping that address from the cluster allow list keeps +// it from being offered as a cluster for ordinary services, and because the +// free-domain suffix match is depth-independent, dropping the address rejects +// every name beneath it as well as the bare one. Only the account's own +// gateway address can ever appear in its allow list (another tenant's gateway +// proxy is account-scoped to them), so this single-address exclusion is +// sufficient. Returns "" when the account has no settings row or a labeled +// (shared-cluster) pin. +func (m Manager) reservedGatewayAddress(ctx context.Context, accountID string) (string, error) { + settings, err := m.store.GetAgentNetworkSettings(ctx, nbstore.LockingStrengthNone, accountID) + if err != nil { + var sErr *status.Error + if errors.As(err, &sErr) && sErr.Type() == status.NotFound { + return "", nil + } + return "", fmt.Errorf("get agent network settings: %w", err) + } + if settings == nil || !settings.Dedicated() { + return "", nil + } + return settings.ProxyAddress, nil +} + func extractClusterFromCustomDomains(serviceDomain string, customDomains []*domain.Domain) (string, bool) { bestCluster := "" bestLen := -1 diff --git a/management/internals/modules/reverseproxy/domain/manager/manager_test.go b/management/internals/modules/reverseproxy/domain/manager/manager_test.go index 53a8dedae..3d952baf9 100644 --- a/management/internals/modules/reverseproxy/domain/manager/manager_test.go +++ b/management/internals/modules/reverseproxy/domain/manager/manager_test.go @@ -7,6 +7,12 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + + agentnetworkTypes "github.com/netbirdio/netbird/management/internals/modules/agentnetwork/types" + "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/domain" + nbstore "github.com/netbirdio/netbird/management/server/store" + "github.com/netbirdio/netbird/management/server/types" + "github.com/netbirdio/netbird/shared/management/status" ) type mockProxyManager struct { @@ -55,7 +61,7 @@ func TestGetClusterAllowList_BYOPMergedWithPublic(t *testing.T) { }, } - mgr := Manager{proxyManager: pm} + mgr := Manager{store: &stubStore{}, proxyManager: pm} result, err := mgr.getClusterAllowList(context.Background(), "acc-123") require.NoError(t, err) assert.Equal(t, []string{"byop.example.com", "eu.proxy.netbird.io"}, result) @@ -71,7 +77,7 @@ func TestGetClusterAllowList_DeduplicatesBYOPAndPublic(t *testing.T) { }, } - mgr := Manager{proxyManager: pm} + mgr := Manager{store: &stubStore{}, proxyManager: pm} result, err := mgr.getClusterAllowList(context.Background(), "acc-123") require.NoError(t, err) assert.Equal(t, []string{"shared.example.com", "byop.example.com", "eu.proxy.netbird.io"}, result) @@ -87,7 +93,7 @@ func TestGetClusterAllowList_NoBYOP_FallbackToShared(t *testing.T) { }, } - mgr := Manager{proxyManager: pm} + mgr := Manager{store: &stubStore{}, proxyManager: pm} result, err := mgr.getClusterAllowList(context.Background(), "acc-123") require.NoError(t, err) assert.Equal(t, []string{"eu.proxy.netbird.io", "us.proxy.netbird.io"}, result) @@ -100,7 +106,7 @@ func TestGetClusterAllowList_BYOPError_ReturnsError(t *testing.T) { }, } - mgr := Manager{proxyManager: pm} + mgr := Manager{store: &stubStore{}, proxyManager: pm} result, err := mgr.getClusterAllowList(context.Background(), "acc-123") require.Error(t, err) assert.Nil(t, result) @@ -117,7 +123,7 @@ func TestGetClusterAllowList_PublicError_ReturnsError(t *testing.T) { }, } - mgr := Manager{proxyManager: pm} + mgr := Manager{store: &stubStore{}, proxyManager: pm} result, err := mgr.getClusterAllowList(context.Background(), "acc-123") require.Error(t, err) assert.Nil(t, result) @@ -134,7 +140,7 @@ func TestGetClusterAllowList_BYOPEmptySlice_FallbackToShared(t *testing.T) { }, } - mgr := Manager{proxyManager: pm} + mgr := Manager{store: &stubStore{}, proxyManager: pm} result, err := mgr.getClusterAllowList(context.Background(), "acc-123") require.NoError(t, err) assert.Equal(t, []string{"eu.proxy.netbird.io"}, result) @@ -150,8 +156,137 @@ func TestGetClusterAllowList_PublicEmpty_BYOPOnly(t *testing.T) { }, } - mgr := Manager{proxyManager: pm} + mgr := Manager{store: &stubStore{}, proxyManager: pm} result, err := mgr.getClusterAllowList(context.Background(), "acc-123") require.NoError(t, err) assert.Equal(t, []string{"byop.example.com"}, result) } + +// stubStore satisfies the manager's narrow store interface for allow-list +// tests. Only the agent-network settings lookup participates; the default (a +// nil func) reads as "no settings row", the state most accounts are in. +type stubStore struct { + getAgentNetworkSettingsFunc func(ctx context.Context, accountID string) (*agentnetworkTypes.Settings, error) +} + +func (s *stubStore) GetAccount(context.Context, string) (*types.Account, error) { + panic("not used in allow-list tests") +} + +func (s *stubStore) GetAgentNetworkSettings(ctx context.Context, _ nbstore.LockingStrength, accountID string) (*agentnetworkTypes.Settings, error) { + if s.getAgentNetworkSettingsFunc != nil { + return s.getAgentNetworkSettingsFunc(ctx, accountID) + } + return nil, status.Errorf(status.NotFound, "agent network settings for account %s not found", accountID) +} + +func (s *stubStore) GetCustomDomain(context.Context, string, string) (*domain.Domain, error) { + panic("not used in allow-list tests") +} + +func (s *stubStore) ListFreeDomains(context.Context, string) ([]string, error) { + panic("not used in allow-list tests") +} + +func (s *stubStore) ListCustomDomains(context.Context, string) ([]*domain.Domain, error) { + panic("not used in allow-list tests") +} + +func (s *stubStore) CreateCustomDomain(context.Context, string, string, string, bool) (*domain.Domain, error) { + panic("not used in allow-list tests") +} + +func (s *stubStore) UpdateCustomDomain(context.Context, string, *domain.Domain) (*domain.Domain, error) { + panic("not used in allow-list tests") +} + +func (s *stubStore) DeleteCustomDomain(context.Context, string, string) error { + panic("not used in allow-list tests") +} + +// TestGetClusterAllowList_DedicatedGatewayAddressExcluded pins invariant (B)'s +// chokepoint: a self-addressed settings pin reserves the account's gateway +// address, so it is dropped from the allow list — which, because the +// free-domain suffix match is depth-independent, rejects every name beneath +// it as well as the bare one. Other addresses are unaffected. +func TestGetClusterAllowList_DedicatedGatewayAddressExcluded(t *testing.T) { + pm := &mockProxyManager{ + getActiveClusterAddressesForAccountFunc: func(_ context.Context, _ string) ([]string, error) { + return []string{"brave-otter.gateway.example.com", "byop.example.com"}, nil + }, + getActiveClusterAddressesFunc: func(_ context.Context) ([]string, error) { + return []string{"eu.proxy.netbird.io"}, nil + }, + } + st := &stubStore{ + getAgentNetworkSettingsFunc: func(_ context.Context, accountID string) (*agentnetworkTypes.Settings, error) { + assert.Equal(t, "acc-123", accountID) + return &agentnetworkTypes.Settings{ + AccountID: accountID, + Domain: "brave-otter.gateway.example.com", + ProxyAddress: "brave-otter.gateway.example.com", + }, nil + }, + } + + mgr := Manager{store: st, proxyManager: pm} + result, err := mgr.getClusterAllowList(context.Background(), "acc-123") + require.NoError(t, err) + assert.Equal(t, []string{"byop.example.com", "eu.proxy.netbird.io"}, result, + "the dedicated gateway address must be reserved from cluster selection") +} + +// TestGetClusterAllowList_LabeledPinDoesNotExclude pins the counterpart: a +// labeled pin means the gateway rides on a shared cluster serving ordinary +// services too, so nothing is reserved. +func TestGetClusterAllowList_LabeledPinDoesNotExclude(t *testing.T) { + pm := &mockProxyManager{ + getActiveClusterAddressesForAccountFunc: func(_ context.Context, _ string) ([]string, error) { + return []string{"byop.example.com"}, nil + }, + getActiveClusterAddressesFunc: func(_ context.Context) ([]string, error) { + return []string{"eu.proxy.netbird.io"}, nil + }, + } + st := &stubStore{ + getAgentNetworkSettingsFunc: func(_ context.Context, accountID string) (*agentnetworkTypes.Settings, error) { + return &agentnetworkTypes.Settings{ + AccountID: accountID, + Domain: "violet.eu.proxy.netbird.io", + ProxyAddress: "eu.proxy.netbird.io", + }, nil + }, + } + + mgr := Manager{store: st, proxyManager: pm} + result, err := mgr.getClusterAllowList(context.Background(), "acc-123") + require.NoError(t, err) + assert.Equal(t, []string{"byop.example.com", "eu.proxy.netbird.io"}, result, + "a labeled pin reserves nothing") +} + +// TestGetClusterAllowList_SettingsLookupError_ReturnsError pins that a store +// outage is surfaced rather than silently treated as "nothing reserved" — +// failing open here would offer a reserved gateway address for ordinary +// services. +func TestGetClusterAllowList_SettingsLookupError_ReturnsError(t *testing.T) { + pm := &mockProxyManager{ + getActiveClusterAddressesForAccountFunc: func(_ context.Context, _ string) ([]string, error) { + return []string{"byop.example.com"}, nil + }, + getActiveClusterAddressesFunc: func(_ context.Context) ([]string, error) { + return []string{"eu.proxy.netbird.io"}, nil + }, + } + st := &stubStore{ + getAgentNetworkSettingsFunc: func(_ context.Context, _ string) (*agentnetworkTypes.Settings, error) { + return nil, status.Errorf(status.Internal, "store outage") + }, + } + + mgr := Manager{store: st, proxyManager: pm} + result, err := mgr.getClusterAllowList(context.Background(), "acc-123") + require.Error(t, err) + assert.Nil(t, result) + assert.Contains(t, err.Error(), "agent network settings") +}