From 0df2eb7b259658b7645e1bea0d51a38af8c1a67c Mon Sep 17 00:00:00 2001 From: mlsmaycon Date: Sat, 12 Sep 2026 17:02:05 +0000 Subject: [PATCH] [management] Refuse pins onto a host another account's gateway already claims A host can be claimed by a pin as well as by a proxy row, and the proxy-row check cannot see that. Another account's labeled pin beneath a host makes the host its cluster, so a self-addressed endpoint on it would never be served; another account's self-addressed endpoint on a host makes the proxy declaring it theirs, so a label beneath it would never be served either. Both bootstrap paths now refuse those shapes before the insert. Neither question touches the shared-cluster shape: labeled pins under one cluster are asked about in neither direction, so any number of accounts still pin beneath a shared cluster. Two self-addressed endpoints on one hostname stay the domain unique index's conflict to refuse. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01Sa3DsBDP3VciAi4PPG17L6 --- .../internals/modules/agentnetwork/manager.go | 32 +++++++++++++- .../agentnetwork/settings_bootstrap_test.go | 42 +++++++++++++++++++ .../server/store/sql_store_agentnetwork.go | 30 +++++++++++++ management/server/store/store.go | 2 + management/server/store/store_mock.go | 30 +++++++++++++ 5 files changed, 135 insertions(+), 1 deletion(-) diff --git a/management/internals/modules/agentnetwork/manager.go b/management/internals/modules/agentnetwork/manager.go index d715004d3..d4524a218 100644 --- a/management/internals/modules/agentnetwork/manager.go +++ b/management/internals/modules/agentnetwork/manager.go @@ -1039,6 +1039,12 @@ func (m *managerImpl) bootstrapSelfAddressed(ctx context.Context, settings *type 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 @@ -1071,6 +1077,13 @@ func (m *managerImpl) bootstrapLabeled(ctx context.Context, settings *types.Sett 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() @@ -1130,11 +1143,28 @@ func (m *managerImpl) requireHostNotForeign(ctx context.Context, accountID, host return fmt.Errorf("check proxy host ownership: %w", err) } if foreign { - return status.Errorf(status.InvalidArgument, "proxy cluster %s is not available to this account", host) + 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 a69b04b71..2e4be721b 100644 --- a/management/internals/modules/agentnetwork/settings_bootstrap_test.go +++ b/management/internals/modules/agentnetwork/settings_bootstrap_test.go @@ -372,3 +372,45 @@ func TestCreateSettingsUnknownHostIsPinnable(t *testing.T) { 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_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 55a8c319c..ed87cd037 100644 --- a/management/server/store/store.go +++ b/management/server/store/store.go @@ -341,6 +341,8 @@ type Store interface { 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 4b8284212..399a07a19 100644 --- a/management/server/store/store_mock.go +++ b/management/server/store/store_mock.go @@ -3080,6 +3080,36 @@ func (mr *MockStoreMockRecorder) HasForeignAccountProxyAtHost(ctx, host, account 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()