diff --git a/management/internals/modules/agentnetwork/manager.go b/management/internals/modules/agentnetwork/manager.go index efcc944be..d4524a218 100644 --- a/management/internals/modules/agentnetwork/manager.go +++ b/management/internals/modules/agentnetwork/manager.go @@ -1036,6 +1036,15 @@ func (m *managerImpl) bootstrapSelfAddressed(ctx context.Context, settings *type if err != nil { return status.Errorf(status.InvalidArgument, "invalid endpoint: %s", err) } + if err := m.requireHostNotForeign(ctx, settings.AccountID, hostname); err != nil { + return err + } + // Another account's labeled pin beneath this hostname makes it their + // cluster: a proxy serving them there would never serve this endpoint. + // The domain unique index already arbitrates two endpoints on one name. + if err := m.requireNotClaimedByOtherAccount(ctx, settings.AccountID, hostname, m.store.HasGatewayClusterPinnedByOtherAccount); err != nil { + return err + } settings.Domain = hostname settings.ProxyAddress = hostname @@ -1065,6 +1074,16 @@ func (m *managerImpl) bootstrapLabeled(ctx context.Context, settings *types.Sett if err != nil { return status.Errorf(status.InvalidArgument, "invalid proxy_address: %s", err) } + if err := m.requireHostNotForeign(ctx, settings.AccountID, parent); err != nil { + return err + } + // Another account's endpoint at this exact hostname means the proxy that + // declares it is theirs, so nothing would serve a label beneath it. Other + // accounts' labeled pins under the same cluster are not asked about: a + // shared cluster carries many of them by design. + if err := m.requireNotClaimedByOtherAccount(ctx, settings.AccountID, parent, m.store.HasGatewayEndpointByOtherAccount); err != nil { + return err + } for attempt := 1; attempt <= maxDomainAllocationAttempts; attempt++ { label := labelgen.PickTuple() @@ -1111,6 +1130,41 @@ func (m *managerImpl) bootstrapLabeled(ctx context.Context, settings *types.Sett return fmt.Errorf("allocate agent network endpoint for account %s: %d attempts exhausted", settings.AccountID, maxDomainAllocationAttempts) } +// requireHostNotForeign refuses to pin the account's gateway onto a host that +// another account's proxy declares. The pin's proxy_address is what selects +// the proxy that serves the endpoint, and an account-scoped proxy only ever +// receives its own account's mappings, so such a pin could never be served — +// and the endpoint it assigns is immutable. Shared proxies are not foreign, and +// a host no proxy has declared stays pinnable: claiming the address before the +// proxy's first connection is the documented order. +func (m *managerImpl) requireHostNotForeign(ctx context.Context, accountID, host string) error { + foreign, err := m.store.HasForeignAccountProxyAtHost(ctx, host, accountID) + if err != nil { + return fmt.Errorf("check proxy host ownership: %w", err) + } + if foreign { + return errHostNotAvailable(host) + } + return nil +} + +// requireNotClaimedByOtherAccount refuses the pin when another account's +// gateway settings already claim the host in the shape claimed answers for. +func (m *managerImpl) requireNotClaimedByOtherAccount(ctx context.Context, accountID, host string, claimed func(context.Context, string, string) (bool, error)) error { + taken, err := claimed(ctx, host, accountID) + if err != nil { + return fmt.Errorf("check agent network gateway claims at host: %w", err) + } + if taken { + return errHostNotAvailable(host) + } + return nil +} + +func errHostNotAvailable(host string) error { + return status.Errorf(status.InvalidArgument, "proxy cluster %s is not available to this account", host) +} + // isUniqueConstraintError reports whether err is a database unique-constraint // violation, matched on the driver message because CreateAgentNetworkSettings // deliberately returns the driver error unwrapped. diff --git a/management/internals/modules/agentnetwork/settings_bootstrap_test.go b/management/internals/modules/agentnetwork/settings_bootstrap_test.go index fea62353e..2e4be721b 100644 --- a/management/internals/modules/agentnetwork/settings_bootstrap_test.go +++ b/management/internals/modules/agentnetwork/settings_bootstrap_test.go @@ -5,12 +5,14 @@ import ( "runtime" "strings" "testing" + "time" - "go.uber.org/mock/gomock" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" "github.com/netbirdio/netbird/management/internals/modules/agentnetwork/types" + "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/proxy" "github.com/netbirdio/netbird/management/server/account" "github.com/netbirdio/netbird/management/server/permissions" "github.com/netbirdio/netbird/management/server/permissions/modules" @@ -70,6 +72,50 @@ func (f *bootstrapFixture) createSettings(ctx context.Context, accountID, userID return f.manager.CreateSettings(ctx, userID, types.DefaultSettings(accountID), proxyAddress, endpoint) } +func ptrTo[T any](v T) *T { return &v } + +// seedProxy registers a proxy in clusterAddr, heartbeating now, so the labeled +// bootstrap path has a real cluster to validate against. accountID empty makes +// it a shared (NetBird-operated) cluster; private mirrors the capability an +// embedded `netbird proxy` reports, nil an unreported one. +func (f *bootstrapFixture) seedProxy(t *testing.T, proxyID, accountID, clusterAddr string, private *bool) { + t.Helper() + f.seedProxyAt(t, proxyID, accountID, clusterAddr, private, time.Now().UTC()) +} + +// seedProxyAt is seedProxy with an explicit last-seen, for cases that need a +// proxy whose heartbeat has aged past the active window while its row (and so +// its cluster) is still on record. +func (f *bootstrapFixture) seedProxyAt(t *testing.T, proxyID, accountID, clusterAddr string, private *bool, lastSeen time.Time) { + t.Helper() + p := &proxy.Proxy{ + ID: proxyID, + ClusterAddress: clusterAddr, + Status: proxy.StatusConnected, + LastSeen: lastSeen, + Capabilities: proxy.Capabilities{Private: private}, + } + if accountID != "" { + p.AccountID = &accountID + } + require.NoError(t, f.store.SaveProxy(context.Background(), p), "seeding a proxy must succeed") +} + +// requireForeignClusterRefusal asserts the refusal a pin onto another +// account's host gets, and that it left no row behind. +func (f *bootstrapFixture) requireForeignClusterRefusal(t *testing.T, err error, accountID string) { + t.Helper() + require.Error(t, err, "another account's host must be refused") + var sErr *status.Error + require.ErrorAs(t, err, &sErr) + assert.Equal(t, status.InvalidArgument, sErr.Type(), "rejection must be a validation error") + assert.Contains(t, err.Error(), "not available to this account", + "the error must say the host is not the account's to use") + + _, err = f.store.GetAgentNetworkSettings(context.Background(), store.LockingStrengthNone, accountID) + assert.Error(t, err, "no row may be left behind by a rejected bootstrap") +} + // TestCreateSettingsRequiresPermission pins the gate: bootstrap assigns the // account's immutable endpoint, a settings write requiring the settings // Create permission — and a denial leaves no row behind. @@ -230,3 +276,141 @@ func TestCreateProviderHasNoSettingsSideEffects(t *testing.T) { _, err = f.store.GetAgentNetworkSettings(ctx, store.LockingStrengthNone, "account1") assert.Error(t, err, "provider create must not conjure a settings row") } + +// TestCreateSettingsRejectsForeignCluster pins tenant consistency on the pin: +// an account may not pin its gateway onto a host another account's proxy +// declares. That proxy only ever receives its own account's mappings, so the +// pin could never be served, and the endpoint it assigns is immutable. +// Ownership is decided on the proxy rows, not on heartbeat freshness — a +// cluster whose proxies are merely offline is still somebody's — and on the +// normalised host, since proxies declare their address as the operator +// spelled it. +func TestCreateSettingsRejectsForeignCluster(t *testing.T) { + ctx := context.Background() + + cases := map[string]struct { + spelling string + lastSeen time.Time + }{ + "live": {"byop.account2.example.com", time.Now().UTC()}, + "offline": {"byop.account2.example.com", time.Now().UTC().Add(-time.Hour)}, + "spelled in caps": {"BYOP.Account2.Example.com", time.Now().UTC()}, + } + for name, tc := range cases { + t.Run("labeled "+name, func(t *testing.T) { + f := newBootstrapFixture(t) + f.seedProxyAt(t, "proxy1", "account2", tc.spelling, ptrTo(true), tc.lastSeen) + f.expectPermission("account1", "user1", modules.AgentNetworkSettings, operations.Create, true) + + _, err := f.createSettings(ctx, "account1", "user1", "byop.account2.example.com", "") + f.requireForeignClusterRefusal(t, err, "account1") + }) + t.Run("self-addressed "+name, func(t *testing.T) { + f := newBootstrapFixture(t) + f.seedProxyAt(t, "proxy1", "account2", tc.spelling, ptrTo(true), tc.lastSeen) + f.expectPermission("account1", "user1", modules.AgentNetworkSettings, operations.Create, true) + + _, err := f.createSettings(ctx, "account1", "user1", "", "byop.account2.example.com") + f.requireForeignClusterRefusal(t, err, "account1") + }) + } +} + +// TestCreateSettingsSharedClusterStaysPinnable pins the constraint the +// ownership check must respect: a shared (NetBird-operated) cluster is not +// anybody's, so any number of accounts pin their gateways to it — including +// an account that also runs a proxy of its own elsewhere. +func TestCreateSettingsSharedClusterStaysPinnable(t *testing.T) { + ctx := context.Background() + f := newBootstrapFixture(t) + f.seedProxy(t, "shared", "", "eu.proxy.netbird.io", ptrTo(true)) + f.seedProxy(t, "own", "account1", "byop.account1.example.com", ptrTo(true)) + + for _, account := range []string{"account1", "account2"} { + f.expectPermission(account, "user", modules.AgentNetworkSettings, operations.Create, true) + created, err := f.createSettings(ctx, account, "user", "eu.proxy.netbird.io", "") + require.NoError(t, err, "a shared cluster must stay pinnable by %s", account) + assert.Equal(t, "eu.proxy.netbird.io", created.ProxyAddress) + } +} + +// TestCreateSettingsOwnClusterIsPinnable is the BYOP order in both directions: +// the account's own proxy is not a competing claim, whether the pin is labeled +// beneath its cluster or self-addressed onto the very host it declares. +func TestCreateSettingsOwnClusterIsPinnable(t *testing.T) { + ctx := context.Background() + + t.Run("labeled", func(t *testing.T) { + f := newBootstrapFixture(t) + f.seedProxy(t, "own", "account1", "byop.account1.example.com", ptrTo(true)) + f.expectPermission("account1", "user1", modules.AgentNetworkSettings, operations.Create, true) + + created, err := f.createSettings(ctx, "account1", "user1", "byop.account1.example.com", "") + require.NoError(t, err, "the account's own cluster must be pinnable") + assert.True(t, strings.HasSuffix(created.Domain, ".byop.account1.example.com")) + }) + t.Run("self-addressed", func(t *testing.T) { + f := newBootstrapFixture(t) + f.seedProxy(t, "own", "account1", "gw.account1.example.com", ptrTo(true)) + f.expectPermission("account1", "user1", modules.AgentNetworkSettings, operations.Create, true) + + created, err := f.createSettings(ctx, "account1", "user1", "", "gw.account1.example.com") + require.NoError(t, err, "the host the account's own proxy declares must be pinnable") + assert.Equal(t, "gw.account1.example.com", created.ProxyAddress) + }) +} + +// TestCreateSettingsUnknownHostIsPinnable pins the address-first order: a host +// no proxy has ever declared is nobody's, so the pin goes through and the +// proxy is deployed after. +func TestCreateSettingsUnknownHostIsPinnable(t *testing.T) { + ctx := context.Background() + f := newBootstrapFixture(t) + f.expectPermission("account1", "user1", modules.AgentNetworkSettings, operations.Create, true) + + created, err := f.createSettings(ctx, "account1", "user1", "future.example.com", "") + require.NoError(t, err, "a host no proxy has declared must stay pinnable") + assert.Equal(t, "future.example.com", created.ProxyAddress) +} + +// TestCreateSettingsRejectsHostAnotherAccountPinned covers claims made by pins +// rather than proxies, which the proxy-row check cannot see. A labeled pin +// beneath a host makes that host the other account's cluster, so a +// self-addressed endpoint on it would never be served; a self-addressed +// endpoint on a host makes the proxy declaring it theirs, so a label beneath +// it would never be served either. Neither is a shared-cluster shape: many +// labeled pins under one cluster are asked about in neither direction. +func TestCreateSettingsRejectsHostAnotherAccountPinned(t *testing.T) { + ctx := context.Background() + + t.Run("self-addressed onto another account's cluster", func(t *testing.T) { + f := newBootstrapFixture(t) + f.expectPermission("account2", "user2", modules.AgentNetworkSettings, operations.Create, true) + _, err := f.createSettings(ctx, "account2", "user2", "gw.example.com", "") + require.NoError(t, err, "account2's labeled pin beneath the host must go through first") + + f.expectPermission("account1", "user1", modules.AgentNetworkSettings, operations.Create, true) + _, err = f.createSettings(ctx, "account1", "user1", "", "gw.example.com") + f.requireForeignClusterRefusal(t, err, "account1") + }) + + t.Run("labeled beneath another account's endpoint", func(t *testing.T) { + f := newBootstrapFixture(t) + f.expectPermission("account2", "user2", modules.AgentNetworkSettings, operations.Create, true) + _, err := f.createSettings(ctx, "account2", "user2", "", "gw.example.com") + require.NoError(t, err, "account2's self-addressed endpoint must go through first") + + f.expectPermission("account1", "user1", modules.AgentNetworkSettings, operations.Create, true) + _, err = f.createSettings(ctx, "account1", "user1", "gw.example.com", "") + f.requireForeignClusterRefusal(t, err, "account1") + }) + + t.Run("labeled beside another account's labeled pin stays allowed", func(t *testing.T) { + f := newBootstrapFixture(t) + for _, account := range []string{"account1", "account2"} { + f.expectPermission(account, "user", modules.AgentNetworkSettings, operations.Create, true) + _, err := f.createSettings(ctx, account, "user", "eu.proxy.netbird.io", "") + require.NoError(t, err, "labeled pins under one cluster are the shared-cluster shape and must not refuse each other") + } + }) +} diff --git a/management/server/store/sql_store.go b/management/server/store/sql_store.go index d2d197a03..1e99251b2 100644 --- a/management/server/store/sql_store.go +++ b/management/server/store/sql_store.go @@ -6471,6 +6471,25 @@ func (s *SqlStore) IsClusterAddressConflicting(ctx context.Context, clusterAddre return count > 0, nil } +// HasForeignAccountProxyAtHost reports whether a proxy owned by a different +// account declares this host. Shared proxies (account_id IS NULL) are not +// foreign: a shared cluster is what most accounts pin their agent network +// gateway to. The match folds case because proxies declare their address as +// the operator spelled it while the caller's host is normalised; that costs a +// scan of the proxies table, taken once per account when its gateway is +// bootstrapped, not on the per-connect path IsClusterAddressConflicting serves. +func (s *SqlStore) HasForeignAccountProxyAtHost(ctx context.Context, host, accountID string) (bool, error) { + var count int64 + result := s.db. + Model(&proxy.Proxy{}). + Where("LOWER(cluster_address) = LOWER(?) AND account_id IS NOT NULL AND account_id != ?", host, accountID). + Count(&count) + if result.Error != nil { + return false, status.Errorf(status.Internal, "check proxy host ownership: %v", result.Error) + } + return count > 0, nil +} + func (s *SqlStore) DeleteAccountCluster(ctx context.Context, clusterAddress, accountID string) error { result := s.db. Where("cluster_address = ? AND account_id = ?", clusterAddress, accountID). diff --git a/management/server/store/sql_store_agentnetwork.go b/management/server/store/sql_store_agentnetwork.go index e75e36320..8a92f7147 100644 --- a/management/server/store/sql_store_agentnetwork.go +++ b/management/server/store/sql_store_agentnetwork.go @@ -315,6 +315,36 @@ func (s *SqlStore) GetAllAgentNetworkSettings(ctx context.Context, lockStrength return settings, nil } +// HasGatewayClusterPinnedByOtherAccount reports whether another account has a +// labeled agent network gateway pinned beneath host, making host its cluster. +// A self-addressed endpoint on the very same hostname is not counted: that +// collision is the domain unique index's to refuse, as a conflict. Case-folded, +// since a settings row written before hostnames were normalised may carry +// capitals; one row per account, so the scan is cheap. +func (s *SqlStore) HasGatewayClusterPinnedByOtherAccount(ctx context.Context, host, accountID string) (bool, error) { + return s.countGatewayRowsByOtherAccount(ctx, "LOWER(proxy_address) = LOWER(?) AND LOWER(domain) <> LOWER(proxy_address)", host, accountID) +} + +// HasGatewayEndpointByOtherAccount reports whether host is another account's +// agent network endpoint hostname (domain). Case-folded for the same reason as +// HasGatewayClusterPinnedByOtherAccount. +func (s *SqlStore) HasGatewayEndpointByOtherAccount(ctx context.Context, host, accountID string) (bool, error) { + return s.countGatewayRowsByOtherAccount(ctx, "LOWER(domain) = LOWER(?)", host, accountID) +} + +func (s *SqlStore) countGatewayRowsByOtherAccount(ctx context.Context, predicate, host, accountID string) (bool, error) { + var count int64 + result := s.db. + Model(&agentNetworkTypes.Settings{}). + Where(predicate+" AND account_id != ?", host, accountID). + Count(&count) + if result.Error != nil { + log.WithContext(ctx).Errorf("failed to check agent network gateway claims at host: %v", result.Error) + return false, status.Errorf(status.Internal, "check agent network gateway claims at host") + } + return count > 0, nil +} + // GetAgentNetworkSettingsByProxyAddress returns every Settings row whose // gateway is served by the proxy declaring the given cluster address. Used by // cluster-scoped synthesis to find the accounts a shared proxy serves. diff --git a/management/server/store/store.go b/management/server/store/store.go index 7a92de0a6..97da95b4c 100644 --- a/management/server/store/store.go +++ b/management/server/store/store.go @@ -342,6 +342,9 @@ type Store interface { CountProxiesByAccountID(ctx context.Context, accountID string) (int64, error) IsClusterAddressConflicting(ctx context.Context, clusterAddress, accountID string) (bool, error) HasActiveProxyAtClusterAddress(ctx context.Context, clusterAddress string) (bool, error) + HasForeignAccountProxyAtHost(ctx context.Context, host, accountID string) (bool, error) + HasGatewayClusterPinnedByOtherAccount(ctx context.Context, host, accountID string) (bool, error) + HasGatewayEndpointByOtherAccount(ctx context.Context, host, accountID string) (bool, error) DeleteAccountCluster(ctx context.Context, clusterAddress, accountID string) error GetCustomDomainsCounts(ctx context.Context) (total int64, validated int64, err error) diff --git a/management/server/store/store_mock.go b/management/server/store/store_mock.go index 04f79d30a..399a07a19 100644 --- a/management/server/store/store_mock.go +++ b/management/server/store/store_mock.go @@ -3065,6 +3065,51 @@ func (mr *MockStoreMockRecorder) HasActiveProxyAtClusterAddress(ctx, clusterAddr return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "HasActiveProxyAtClusterAddress", reflect.TypeOf((*MockStore)(nil).HasActiveProxyAtClusterAddress), ctx, clusterAddress) } +// HasForeignAccountProxyAtHost mocks base method. +func (m *MockStore) HasForeignAccountProxyAtHost(ctx context.Context, host, accountID string) (bool, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "HasForeignAccountProxyAtHost", ctx, host, accountID) + ret0, _ := ret[0].(bool) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// HasForeignAccountProxyAtHost indicates an expected call of HasForeignAccountProxyAtHost. +func (mr *MockStoreMockRecorder) HasForeignAccountProxyAtHost(ctx, host, accountID any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "HasForeignAccountProxyAtHost", reflect.TypeOf((*MockStore)(nil).HasForeignAccountProxyAtHost), ctx, host, accountID) +} + +// HasGatewayClusterPinnedByOtherAccount mocks base method. +func (m *MockStore) HasGatewayClusterPinnedByOtherAccount(ctx context.Context, host, accountID string) (bool, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "HasGatewayClusterPinnedByOtherAccount", ctx, host, accountID) + ret0, _ := ret[0].(bool) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// HasGatewayClusterPinnedByOtherAccount indicates an expected call of HasGatewayClusterPinnedByOtherAccount. +func (mr *MockStoreMockRecorder) HasGatewayClusterPinnedByOtherAccount(ctx, host, accountID any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "HasGatewayClusterPinnedByOtherAccount", reflect.TypeOf((*MockStore)(nil).HasGatewayClusterPinnedByOtherAccount), ctx, host, accountID) +} + +// HasGatewayEndpointByOtherAccount mocks base method. +func (m *MockStore) HasGatewayEndpointByOtherAccount(ctx context.Context, host, accountID string) (bool, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "HasGatewayEndpointByOtherAccount", ctx, host, accountID) + ret0, _ := ret[0].(bool) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// HasGatewayEndpointByOtherAccount indicates an expected call of HasGatewayEndpointByOtherAccount. +func (mr *MockStoreMockRecorder) HasGatewayEndpointByOtherAccount(ctx, host, accountID any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "HasGatewayEndpointByOtherAccount", reflect.TypeOf((*MockStore)(nil).HasGatewayEndpointByOtherAccount), ctx, host, accountID) +} + // IncrementAgentNetworkConsumption mocks base method. func (m *MockStore) IncrementAgentNetworkConsumption(ctx context.Context, accountID string, kind types.ConsumptionDimension, dimID string, windowSeconds int64, windowStart time.Time, tokensIn, tokensOut int64, costUSD float64) error { m.ctrl.T.Helper()