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") +}