From 791401060d2b95e5f51e3439c0649729132f571e Mon Sep 17 00:00:00 2001 From: Mohd Quamar Tyagi <104281681+Tyagiquamar@users.noreply.github.com> Date: Sat, 12 Sep 2026 16:07:23 +0530 Subject: [PATCH 1/2] [management] Prevent deleting groups referenced by agent network budget rules (#7450) `validateDeleteGroup` already refuses to delete a group that is still used by routes, policies, nameservers, setup keys, users, network routers, reverse proxy services, and agent network policies. Account-level agent network budget rules also store group IDs in `TargetGroups`, but that check was missing. Deleting such a group left a dangling ID on the budget rule. `budgetRuleApplies` then never matched callers by group, so the spend cap silently stopped applying. This adds `isGroupLinkedToAgentNetworkBudgetRule` and uses it in `validateDeleteGroup`, matching the existing helpers. --- management/server/group.go | 28 +++++++++++++++++++++ management/server/group_test.go | 44 +++++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+) diff --git a/management/server/group.go b/management/server/group.go index ca20a6b08..88295e2f6 100644 --- a/management/server/group.go +++ b/management/server/group.go @@ -774,6 +774,14 @@ func validateDeleteGroup(ctx context.Context, transaction store.Store, group *ty return &GroupLinkError{"agent network policy", linkedPolicy.Name} } + isLinked, linkedRule, err := isGroupLinkedToAgentNetworkBudgetRule(ctx, transaction, group.AccountID, group.ID) + if err != nil { + return status.Errorf(status.Internal, "failed to check agent network budget rules") + } + if isLinked { + return &GroupLinkError{"agent network budget rule", linkedRule.Name} + } + return checkGroupLinkedToSettings(ctx, transaction, group) } @@ -945,6 +953,26 @@ func isGroupLinkedToAgentNetworkPolicy(ctx context.Context, transaction store.St return false, nil } +// isGroupLinkedToAgentNetworkBudgetRule checks if a group is a target of any +// account-level agent network budget rule. +func isGroupLinkedToAgentNetworkBudgetRule(ctx context.Context, transaction store.Store, accountID string, groupID string) (bool, *agentNetworkTypes.AccountBudgetRule, error) { + rules, err := transaction.GetAccountAgentNetworkBudgetRules(ctx, store.LockingStrengthNone, accountID) + if err != nil { + log.WithContext(ctx).Errorf("error retrieving agent network budget rules while checking group linkage: %v", err) + return false, nil, err + } + + for _, rule := range rules { + if rule == nil { + continue + } + if slices.Contains(rule.TargetGroups, groupID) { + return true, rule, nil + } + } + return false, nil, nil +} + // areGroupChangesAffectPeers checks if any changes to the specified groups will affect peers. // It fetches each collection once and checks all groupIDs against them in memory. func areGroupChangesAffectPeers(ctx context.Context, transaction store.Store, accountID string, groupIDs []string) (bool, error) { diff --git a/management/server/group_test.go b/management/server/group_test.go index da056c8a9..fa351a43e 100644 --- a/management/server/group_test.go +++ b/management/server/group_test.go @@ -132,6 +132,11 @@ func TestDefaultAccountManager_DeleteGroup(t *testing.T) { "grp-for-agent-network-policy", "agent network policy", }, + { + "agent network budget rule", + "grp-for-agent-network-budget-rule", + "agent network budget rule", + }, { "reverse proxy private service access group", "grp-for-rp-private", @@ -152,6 +157,16 @@ func TestDefaultAccountManager_DeleteGroup(t *testing.T) { return } + group, getErr := am.GetGroup(context.Background(), account.Id, testCase.groupID, groupAdminUserID) + if getErr != nil { + t.Errorf("group %s should still exist after failed deletion: %s", testCase.groupID, getErr) + return + } + if group == nil { + t.Errorf("group %s was deleted despite the failed deletion", testCase.groupID) + return + } + var sErr *status.Error if errors.As(err, &sErr) { if sErr.Message != testCase.expectedReason { @@ -240,6 +255,12 @@ func TestDefaultAccountManager_DeleteGroups(t *testing.T) { groupIDs: []string{"grp-for-agent-network-policy"}, expectedReasons: []string{"agent network policy"}, }, + { + name: "agent network budget rule", + groupIDs: []string{"grp-for-agent-network-budget-rule"}, + expectedReasons: []string{"agent network budget rule"}, + expectedNotDeleted: []string{"grp-for-agent-network-budget-rule"}, + }, { name: "reverse proxy services", groupIDs: []string{"grp-for-rp-private", "grp-for-rp-bearer"}, @@ -501,6 +522,14 @@ func initTestGroupAccount(am *DefaultAccountManager) (*DefaultAccountManager, *t Peers: make([]string, 0), } + groupForAgentNetworkBudgetRule := &types.Group{ + ID: "grp-for-agent-network-budget-rule", + AccountID: "account-id", + Name: "Group for agent network budget rules", + Issued: types.GroupIssuedAPI, + Peers: make([]string, 0), + } + groupForRPPrivate := &types.Group{ ID: "grp-for-rp-private", AccountID: "account-id", @@ -573,6 +602,7 @@ func initTestGroupAccount(am *DefaultAccountManager) (*DefaultAccountManager, *t _ = am.CreateGroup(context.Background(), accountID, groupAdminUserID, groupForUsers) _ = am.CreateGroup(context.Background(), accountID, groupAdminUserID, groupForIntegration) _ = am.CreateGroup(context.Background(), accountID, groupAdminUserID, groupForAgentNetworkPolicy) + _ = am.CreateGroup(context.Background(), accountID, groupAdminUserID, groupForAgentNetworkBudgetRule) _ = am.CreateGroup(context.Background(), accountID, groupAdminUserID, groupForRPPrivate) _ = am.CreateGroup(context.Background(), accountID, groupAdminUserID, groupForRPBearer) @@ -587,6 +617,20 @@ func initTestGroupAccount(am *DefaultAccountManager) (*DefaultAccountManager, *t return nil, nil, err } + budgetRuleDecoy := agentNetworkTypes.NewAccountBudgetRule(accountID) + budgetRuleDecoy.Name = "Unrelated agent network budget rule" + budgetRuleDecoy.TargetGroups = []string{"unrelated-group"} + if err := am.Store.SaveAgentNetworkBudgetRule(context.Background(), budgetRuleDecoy); err != nil { + return nil, nil, err + } + + budgetRule := agentNetworkTypes.NewAccountBudgetRule(accountID) + budgetRule.Name = "Example agent network budget rule" + budgetRule.TargetGroups = []string{groupForAgentNetworkBudgetRule.ID} + if err := am.Store.SaveAgentNetworkBudgetRule(context.Background(), budgetRule); err != nil { + return nil, nil, err + } + // The decoy services are created first so the linkage check has to scan // past services that do not reference the groups under test. rpServices := []*rpservice.Service{ From 09cc91886f562343144a38040fcc7e0aeefed391 Mon Sep 17 00:00:00 2001 From: mlsmaycon Date: Sat, 12 Sep 2026 16:50:49 +0000 Subject: [PATCH 2/2] [management] Refuse to pin an agent network gateway onto another account's host MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An agent network bootstrap stores the cluster it pins to as proxy_address, and that value selects the proxy that serves the endpoint. An account-scoped proxy only ever receives its own account's mappings, so a pin onto a host another account's proxy declares can never be served — and the endpoint it assigns is immutable, so the account is left with a dead gateway until it deletes its settings and starts over. Nothing refused that pin: the domain unique index arbitrates between pins only, and knows nothing about proxies. Both bootstrap paths now ask, before the insert, whether a proxy owned by a different account declares the host. Shared proxies are not foreign — a shared cluster is what most accounts pin to, and any number of them may — and a host no proxy has declared stays pinnable, since claiming the address before the proxy's first connection is the documented order. Ownership is decided on the proxy rows, not on heartbeat freshness, and on the folded spelling, since proxies declare their address as the operator typed it. Registration is deliberately not changed: refusing a proxy because another account pinned its host would let a pin lock a tenant's proxy out once the stale-proxy reaper has dropped its rows. Left as is, the worst a race or a reaping window can produce is a dead pin for the account that made it. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01Sa3DsBDP3VciAi4PPG17L6 --- .../internals/modules/agentnetwork/manager.go | 24 +++ .../agentnetwork/settings_bootstrap_test.go | 144 +++++++++++++++++- management/server/store/sql_store.go | 19 +++ management/server/store/store.go | 1 + management/server/store/store_mock.go | 15 ++ 5 files changed, 202 insertions(+), 1 deletion(-) diff --git a/management/internals/modules/agentnetwork/manager.go b/management/internals/modules/agentnetwork/manager.go index efcc944be..d715004d3 100644 --- a/management/internals/modules/agentnetwork/manager.go +++ b/management/internals/modules/agentnetwork/manager.go @@ -1036,6 +1036,9 @@ 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 + } settings.Domain = hostname settings.ProxyAddress = hostname @@ -1065,6 +1068,9 @@ 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 + } for attempt := 1; attempt <= maxDomainAllocationAttempts; attempt++ { label := labelgen.PickTuple() @@ -1111,6 +1117,24 @@ 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 status.Errorf(status.InvalidArgument, "proxy cluster %s is not available to this account", host) + } + return nil +} + // 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..a69b04b71 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,99 @@ 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) +} diff --git a/management/server/store/sql_store.go b/management/server/store/sql_store.go index 08ec45395..ec4c2bb55 100644 --- a/management/server/store/sql_store.go +++ b/management/server/store/sql_store.go @@ -6446,6 +6446,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/store.go b/management/server/store/store.go index 6886536b9..55a8c319c 100644 --- a/management/server/store/store.go +++ b/management/server/store/store.go @@ -340,6 +340,7 @@ 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) 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..4b8284212 100644 --- a/management/server/store/store_mock.go +++ b/management/server/store/store_mock.go @@ -3065,6 +3065,21 @@ 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) +} + // 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()