From a403a8d2759e8f8e2de07878e0c3d3f2ea4aec2d Mon Sep 17 00:00:00 2001 From: mlsmaycon Date: Sat, 12 Sep 2026 09:10:55 +0000 Subject: [PATCH] [management] Decide gateway cluster ownership before the account's own view MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ownership check only ran when the account's own view of the cluster came back empty, so an account holding any row for the host — its own canonical proxy — skipped it entirely. A row another account left behind under a non-canonical spelling was then never seen, and the pin the check exists to refuse went through. That is the case worth catching, not the one to skip: two accounts claiming one hostname is the ambiguity the connect-time conflict check prevents going forward and cannot see for a row written before addresses were canonicalized, and the endpoint pinned here cannot be moved afterwards. Ask ownership first, and narrow what counts as foreign while doing it. The query treated a shared proxy as outside the account, which was harmless while it ran only for a host the account had no view of — a shared row would have given it one — but refuses the cluster most accounts pin to once it runs first. Only a row owned by a different account is foreign now, which is also what the name says. --- .../internals/modules/agentnetwork/manager.go | 31 +++++++------ .../agentnetwork/settings_bootstrap_test.go | 45 +++++++++++++++++++ management/server/store/sql_store.go | 29 +++++++----- management/server/store/store.go | 2 +- management/server/store/store_mock.go | 12 ++--- 5 files changed, 87 insertions(+), 32 deletions(-) diff --git a/management/internals/modules/agentnetwork/manager.go b/management/internals/modules/agentnetwork/manager.go index 0fbca2a4b..44c7ce59e 100644 --- a/management/internals/modules/agentnetwork/manager.go +++ b/management/internals/modules/agentnetwork/manager.go @@ -1083,24 +1083,29 @@ func (m *managerImpl) bootstrapSelfAddressed(ctx context.Context, settings *type // all: pinning ahead of a proxy's first connection is a legitimate order — the // dedicated path claims an address the same way, before any proxy declares it. func (m *managerImpl) validateGatewayCluster(ctx context.Context, accountID, clusterAddr string) error { + // Ownership is decided first, before anything the account's own view can + // answer. A host another account's proxy declares is refused even when + // this account has a row for it too: two accounts claiming one hostname is + // the ambiguity the connect-time conflict check exists to prevent, and the + // endpoint pinned here cannot be moved afterwards, so the ambiguous case + // has to fail closed. Asking the account's view first would skip this + // whenever the account had any row of its own, which is exactly when a + // collision is worth catching. Shared proxies are not foreign — they are + // what most accounts pin to. + foreign, err := m.store.HasForeignAccountProxyAtHost(ctx, clusterAddr, accountID) + if err != nil { + return fmt.Errorf("check proxy cluster ownership: %w", err) + } + if foreign { + return status.Errorf(status.InvalidArgument, + "proxy cluster %s is not available to this account", clusterAddr) + } + declared, err := m.accountClusterSpellings(ctx, accountID, clusterAddr) if err != nil { return err } - if len(declared) == 0 { - // Not in the account's view. A shared cluster would have been in it, - // so a proxy row elsewhere for this address can only be another - // account's BYOP cluster: its proxies filter foreign mappings out on - // delivery, making the pin dead on arrival. - foreign, err := m.store.HasProxyOutsideAccountAtHost(ctx, clusterAddr, accountID) - if err != nil { - return fmt.Errorf("check proxy cluster ownership: %w", err) - } - if foreign { - return status.Errorf(status.InvalidArgument, - "proxy cluster %s is not available to this account", clusterAddr) - } // No proxy has ever declared this address: an address-first pin. return nil } diff --git a/management/internals/modules/agentnetwork/settings_bootstrap_test.go b/management/internals/modules/agentnetwork/settings_bootstrap_test.go index 42f3f5f5d..34f7ccb59 100644 --- a/management/internals/modules/agentnetwork/settings_bootstrap_test.go +++ b/management/internals/modules/agentnetwork/settings_bootstrap_test.go @@ -374,6 +374,51 @@ func TestCreateSettingsRejectsForeignCluster(t *testing.T) { } } +// TestCreateSettingsRejectsHostAnotherAccountClaims pins that ownership is +// decided before the account's own view, not after it. +// +// Two accounts holding rows for one hostname is the ambiguity the connect-time +// conflict check prevents going forward and cannot see for a row written +// before addresses were canonicalized. Deciding on the account's own view +// first would skip the ownership question exactly when the account has a row +// of its own — which is when a collision is worth catching — and the endpoint +// pinned here cannot be moved afterwards. +func TestCreateSettingsRejectsHostAnotherAccountClaims(t *testing.T) { + ctx := context.Background() + f := newBootstrapFixture(t) + // account1's own row is canonical and perfectly serviceable on its own. + f.seedProxy(t, "own", "account1", "shared.example.com", ptrTo(true)) + // account2 holds a legacy, non-canonical spelling of the same host. + f.seedProxy(t, "foreign", "account2", "Shared.Example.com", ptrTo(true)) + f.expectPermission("account1", "user1", modules.AgentNetworkSettings, operations.Create, true) + + _, err := f.createSettings(ctx, "account1", "user1", "shared.example.com", "") + require.Error(t, err, "a host another account also claims must be refused") + var sErr *status.Error + require.ErrorAs(t, err, &sErr) + assert.Equal(t, status.InvalidArgument, sErr.Type()) + assert.Contains(t, err.Error(), "not available to this account") + + _, err = f.store.GetAgentNetworkSettings(ctx, store.LockingStrengthNone, "account1") + assert.Error(t, err, "no row may be left behind by a rejected bootstrap") +} + +// TestCreateSettingsAcceptsSharedClusterAlongsideOwnProxy pins the other side +// of that ordering: a shared (NetBird-operated) proxy is not foreign, so +// asking the ownership question first must not refuse the cluster most +// accounts pin to. +func TestCreateSettingsAcceptsSharedClusterAlongsideOwnProxy(t *testing.T) { + ctx := context.Background() + f := newBootstrapFixture(t) + f.seedProxy(t, "shared", "", "eu.proxy.example.com", ptrTo(true)) + f.seedProxy(t, "own", "account1", "eu.proxy.example.com", ptrTo(true)) + f.expectPermission("account1", "user1", modules.AgentNetworkSettings, operations.Create, true) + + created, err := f.createSettings(ctx, "account1", "user1", "eu.proxy.example.com", "") + require.NoError(t, err, "a shared cluster must stay pinnable") + assert.Equal(t, "eu.proxy.example.com", created.ProxyAddress) +} + // TestCreateSettingsAcceptsOwnPrivateCluster pins the BYOP happy path: the // account's own cluster with a connected embedded proxy is a valid pin. func TestCreateSettingsAcceptsOwnPrivateCluster(t *testing.T) { diff --git a/management/server/store/sql_store.go b/management/server/store/sql_store.go index ed96d6f8a..6cd3abd95 100644 --- a/management/server/store/sql_store.go +++ b/management/server/store/sql_store.go @@ -6441,22 +6441,27 @@ func (s *SqlStore) HasActiveProxyAtClusterAddress(ctx context.Context, clusterAd // match is exact, and stays exact so it uses the cluster_address index: // addresses are canonicalised where they are written (canonicalProxyAddress on // the proxy-connect path), so one host has one spelling in this column. -// HasProxyOutsideAccountAtHost reports the same thing as -// IsClusterAddressConflicting, folding case on both sides. +// HasForeignAccountProxyAtHost reports whether a proxy owned by another +// account declares this host, folding case on both sides. // -// The two exist separately because their callers differ in cost and in what -// they can assume. IsClusterAddressConflicting runs on every account-scoped -// proxy connect, where both sides are canonical and the match must stay exact -// to use the cluster_address index. This one runs once per account, when an -// agent network bootstraps, and is the only thing standing between that -// account and pinning its immutable endpoint to a cluster somebody else runs -// — so it also has to see a row written before addresses were canonicalized, -// which is worth a scan on a path taken once. -func (s *SqlStore) HasProxyOutsideAccountAtHost(ctx context.Context, host, accountID string) (bool, error) { +// Shared proxies (account_id IS NULL) are deliberately not foreign: they are +// what most accounts pin their gateway to. What this catches is two accounts +// claiming one hostname, which IsClusterAddressConflicting prevents going +// forward but cannot see for a row written before addresses were +// canonicalized. +// +// It folds case where IsClusterAddressConflicting stays exact because the +// callers differ in cost and in what they can assume. That one runs on every +// account-scoped proxy connect, where both sides are canonical and the match +// must stay exact to use the cluster_address index. This one runs once per +// account, when an agent network bootstraps, and is the only thing standing +// between that account and pinning its immutable endpoint to a cluster +// somebody else runs — worth a scan on a path taken once. +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 NULL OR account_id != ?)", host, accountID). + 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) diff --git a/management/server/store/store.go b/management/server/store/store.go index 9a6f8ffd9..55a8c319c 100644 --- a/management/server/store/store.go +++ b/management/server/store/store.go @@ -340,7 +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) - HasProxyOutsideAccountAtHost(ctx context.Context, host, accountID 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 94ca0e015..4b8284212 100644 --- a/management/server/store/store_mock.go +++ b/management/server/store/store_mock.go @@ -3065,19 +3065,19 @@ func (mr *MockStoreMockRecorder) HasActiveProxyAtClusterAddress(ctx, clusterAddr return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "HasActiveProxyAtClusterAddress", reflect.TypeOf((*MockStore)(nil).HasActiveProxyAtClusterAddress), ctx, clusterAddress) } -// HasProxyOutsideAccountAtHost mocks base method. -func (m *MockStore) HasProxyOutsideAccountAtHost(ctx context.Context, host, accountID string) (bool, error) { +// 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, "HasProxyOutsideAccountAtHost", ctx, host, accountID) + ret := m.ctrl.Call(m, "HasForeignAccountProxyAtHost", ctx, host, accountID) ret0, _ := ret[0].(bool) ret1, _ := ret[1].(error) return ret0, ret1 } -// HasProxyOutsideAccountAtHost indicates an expected call of HasProxyOutsideAccountAtHost. -func (mr *MockStoreMockRecorder) HasProxyOutsideAccountAtHost(ctx, host, accountID any) *gomock.Call { +// 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, "HasProxyOutsideAccountAtHost", reflect.TypeOf((*MockStore)(nil).HasProxyOutsideAccountAtHost), ctx, host, accountID) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "HasForeignAccountProxyAtHost", reflect.TypeOf((*MockStore)(nil).HasForeignAccountProxyAtHost), ctx, host, accountID) } // IncrementAgentNetworkConsumption mocks base method.