From 5502ea08acbe94e837b323bec70446410f13eb49 Mon Sep 17 00:00:00 2001 From: mlsmaycon Date: Sat, 12 Sep 2026 16:08:07 +0000 Subject: [PATCH] [management] Scope the change to the private-capability check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The PR grew past its purpose. What it needs to do is refuse to bootstrap an agent network endpoint onto a cluster that cannot serve it, which is the private capability check on the picked cluster. Everything that accreted around it — canonicalising proxy addresses at connect, refusing another account's cluster or a host another account pinned, withdrawing a claim lost to a concurrent one, folding casing on migrated settings rows — is security work in its own right and moves to follow-up PRs, where each can be reviewed against its own threat rather than as a rider on this one. This restores main's version of every file outside that purpose and reduces the validation to: a cluster the account can see must have a live embedded proxy, and a cluster management holds no row for stays pinnable (address-first). The e2e test and the fixture seeds are unchanged. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01Sa3DsBDP3VciAi4PPG17L6 --- .../internals/modules/agentnetwork/manager.go | 89 +---- .../agentnetwork/settings_bootstrap_test.go | 310 +----------------- .../reverseproxy/proxy/manager/manager.go | 64 +--- .../proxy/manager/manager_store_test.go | 91 ----- .../proxy/manager/manager_test.go | 214 ------------ .../modules/reverseproxy/proxy/proxy.go | 20 -- management/internals/shared/grpc/proxy.go | 54 +-- .../shared/grpc/proxy_address_test.go | 38 --- .../grpc/proxy_connect_authorizer_test.go | 27 +- .../migration/migration_agentnetwork.go | 97 +----- management/server/migration/migration_test.go | 103 +----- management/server/store/sql_store.go | 64 +--- .../server/store/sql_store_agentnetwork.go | 26 -- .../sql_store_agentnetwork_gatewaypin_test.go | 73 ----- .../store/sql_store_proxy_disconnect_test.go | 44 --- management/server/store/store.go | 6 - management/server/store/store_mock.go | 44 --- 17 files changed, 41 insertions(+), 1323 deletions(-) delete mode 100644 management/internals/modules/reverseproxy/proxy/manager/manager_store_test.go delete mode 100644 management/server/store/sql_store_agentnetwork_gatewaypin_test.go diff --git a/management/internals/modules/agentnetwork/manager.go b/management/internals/modules/agentnetwork/manager.go index 56d8ace43..4f31e0791 100644 --- a/management/internals/modules/agentnetwork/manager.go +++ b/management/internals/modules/agentnetwork/manager.go @@ -1030,24 +1030,13 @@ func (m *managerImpl) CreateSettings(ctx context.Context, userID string, setting // bootstrapSelfAddressed claims the given hostname as the account's endpoint, // served only by a proxy declaring exactly that address (Domain == -// ProxyAddress). The domain unique index arbitrates between pins; ownership -// against another account's proxy is asked the same way as for a labeled pin, -// because the hostname lands in proxy_address, which is what a proxy -// registration is refused on when another account holds it there. +// ProxyAddress). The domain unique index is the arbiter of availability. func (m *managerImpl) bootstrapSelfAddressed(ctx context.Context, settings *types.Settings, endpoint string) error { hostname, err := types.NormalizeHostname(endpoint) if err != nil { return status.Errorf(status.InvalidArgument, "invalid endpoint: %s", err) } - foreign, err := m.store.HasForeignAccountProxyAtHost(ctx, hostname, settings.AccountID) - if err != nil { - return fmt.Errorf("check proxy cluster ownership: %w", err) - } - if foreign { - return errForeignCluster(hostname) - } - settings.Domain = hostname settings.ProxyAddress = hostname if err := m.store.CreateAgentNetworkSettings(ctx, settings); err != nil { @@ -1062,7 +1051,7 @@ func (m *managerImpl) bootstrapSelfAddressed(ctx context.Context, settings *type } return fmt.Errorf("create agent network settings: %w", err) } - return m.confirmGatewayClusterOwnership(ctx, settings) + return nil } // validateGatewayCluster rejects a labeled bootstrap pinned to a cluster that @@ -1078,10 +1067,10 @@ func (m *managerImpl) bootstrapSelfAddressed(ctx context.Context, settings *type // the `private` capability, the same flag the dashboard renders as // supports_private when it gates NetBird-only services. // -// Without this check the bootstrap happily pins to any hostname the caller -// names, including a cluster the account cannot use or one with no embedded -// proxy — and the endpoint it allocates is immutable, so the account is left -// with a dead gateway that only a DeleteSettings/re-bootstrap can undo. +// Without this check the bootstrap happily pins to any cluster the caller +// names, including one with no embedded proxy — and the endpoint it allocates +// is immutable, so the account is left with a dead gateway that only a +// DeleteSettings/re-bootstrap can undo. // // Whether management knows the cluster is decided on the proxy rows // themselves, never on how fresh their heartbeats are: a cluster's rows @@ -1094,23 +1083,6 @@ 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 errForeignCluster(clusterAddr) - } - declared, err := m.accountClusterSpellings(ctx, accountID, clusterAddr) if err != nil { return err @@ -1146,16 +1118,13 @@ func (m *managerImpl) validateGatewayCluster(ctx context.Context, accountID, clu // host as clusterAddr. Empty means management holds no proxy row for that host // in this account's view. // -// Addresses are canonicalised where they are written (canonicalProxyAddress on -// the proxy-connect path), so a stored spelling normally is the normalised -// form. Identity is still compared on the normalised form rather than -// byte-equal, which costs nothing here — this is an in-memory pass over the -// account's clusters, not a query — and covers a row written before that -// landed. What comes back is the stored spelling either way, because the -// capability lookup matches cluster_address exactly and would silently find -// nothing under a spelling the store never held. The cluster listing is not -// gated on heartbeats, so this answer does not change while a cluster's -// proxies are merely offline. +// A proxy declares its cluster address as the operator spelled it, so identity +// is compared on the normalised form rather than byte-equal — an in-memory pass +// over the account's clusters, not a query. What comes back is the stored +// spelling, because the capability lookup matches cluster_address exactly and +// would silently find nothing under a spelling the store never held. The +// cluster listing is not gated on heartbeats, so this answer does not change +// while a cluster's proxies are merely offline. func (m *managerImpl) accountClusterSpellings(ctx context.Context, accountID, clusterAddr string) ([]string, error) { clusters, err := m.store.GetProxyClusters(ctx, accountID) if err != nil { @@ -1237,40 +1206,10 @@ func (m *managerImpl) bootstrapLabeled(ctx context.Context, settings *types.Sett } return fmt.Errorf("create agent network settings: %w", err) } - return m.confirmGatewayClusterOwnership(ctx, settings) - } - - return fmt.Errorf("allocate agent network endpoint for account %s: %d attempts exhausted", settings.AccountID, maxDomainAllocationAttempts) -} - -// confirmGatewayClusterOwnership re-reads ownership once the settings row is -// committed and withdraws the row if another account's proxy now declares the -// host; see proxy.ErrClusterAddressUnavailable for why the re-read is what -// closes the race with a concurrent proxy registration. Only ownership is -// re-read: the capability check is about what the cluster can do, not who -// holds it, and does not race a claim. -func (m *managerImpl) confirmGatewayClusterOwnership(ctx context.Context, settings *types.Settings) error { - foreign, err := m.store.HasForeignAccountProxyAtHost(ctx, settings.ProxyAddress, settings.AccountID) - if err == nil && !foreign { return nil } - if delErr := m.store.DeleteAgentNetworkSettings(ctx, settings.AccountID); delErr != nil { - log.WithContext(ctx).Errorf("failed to withdraw agent network settings for account %s after losing the claim on %s: %v", - settings.AccountID, settings.ProxyAddress, delErr) - } - if err != nil { - return fmt.Errorf("confirm proxy cluster ownership: %w", err) - } - log.WithContext(ctx).Warnf("proxy cluster %s was claimed by another account while account %s bootstrapped onto it, withdrawing the pin", - settings.ProxyAddress, settings.AccountID) - return errForeignCluster(settings.ProxyAddress) -} - -// errForeignCluster is the refusal for a cluster another account's proxy -// declares, worded the same whether it is caught before or after the insert. -func errForeignCluster(clusterAddr string) error { - return status.Errorf(status.InvalidArgument, "proxy cluster %s is not available to this account", clusterAddr) + return fmt.Errorf("allocate agent network endpoint for account %s: %d attempts exhausted", settings.AccountID, maxDomainAllocationAttempts) } // isUniqueConstraintError reports whether err is a database unique-constraint diff --git a/management/internals/modules/agentnetwork/settings_bootstrap_test.go b/management/internals/modules/agentnetwork/settings_bootstrap_test.go index ca1920a00..6ac8c869b 100644 --- a/management/internals/modules/agentnetwork/settings_bootstrap_test.go +++ b/management/internals/modules/agentnetwork/settings_bootstrap_test.go @@ -2,7 +2,6 @@ package agentnetwork import ( "context" - "errors" "runtime" "strings" "testing" @@ -36,16 +35,6 @@ type bootstrapFixture struct { } func newBootstrapFixture(t *testing.T) *bootstrapFixture { - t.Helper() - return newBootstrapFixtureWith(t, func(st store.Store) store.Store { return st }) -} - -// newBootstrapFixtureWith hands the manager the real store as seen through -// wrap, while the fixture keeps the unwrapped store for seeding and -// assertions. It exists for cases that need something to happen between two -// of the manager's store calls — a competing claim landing mid-bootstrap — -// which a real store cannot be made to do on cue. -func newBootstrapFixtureWith(t *testing.T, wrap func(store.Store) store.Store) *bootstrapFixture { t.Helper() if runtime.GOOS == "windows" { t.Skip("sqlite store not properly supported on Windows yet") @@ -66,7 +55,7 @@ func newBootstrapFixtureWith(t *testing.T, wrap func(store.Store) store.Store) * vendor := &stubLister{} return &bootstrapFixture{ - manager: NewManager(wrap(st), perms, accounts, nil, WithModelLister(vendor)), + manager: NewManager(st, perms, accounts, nil, WithModelLister(vendor)), store: st, perms: perms, vendor: vendor, @@ -356,279 +345,6 @@ func TestCreateSettingsRequiresPrivateCluster(t *testing.T) { assert.Error(t, err, "no row may be left behind by a rejected bootstrap") } -// TestCreateSettingsRejectsForeignCluster pins tenant isolation on the pin: an -// account-owned (BYOP) cluster belongs to the account that runs it and is not -// one another account may hang its gateway beneath, even though it is -// private-capable. Ownership does not lapse with the heartbeat either, so the -// refusal holds while the foreign cluster is offline. -func TestCreateSettingsRejectsForeignCluster(t *testing.T) { - ctx := context.Background() - - cases := map[string]time.Time{ - "live": time.Now().UTC(), - "offline": time.Now().UTC().Add(-time.Hour), - } - for name, lastSeen := range cases { - t.Run(name, func(t *testing.T) { - f := newBootstrapFixture(t) - f.seedProxyAt(t, "proxy1", "account2", "byop.account2.example.com", ptrTo(true), lastSeen) - f.expectPermission("account1", "user1", modules.AgentNetworkSettings, operations.Create, true) - - _, err := f.createSettings(ctx, "account1", "user1", "byop.account2.example.com", "") - require.Error(t, err, "another account's BYOP cluster must be rejected") - 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 cluster is not the account's to use") - }) - } -} - -// 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") -} - -// claimingStore is a store.Store on which another account's proxy registers -// at the host being pinned in the moment the settings row is written — the -// interleaving a concurrent proxy connect produces when it passes its own -// availability check before this bootstrap's row exists, so neither side's -// pre-write check sees the other. -type claimingStore struct { - store.Store - t *testing.T - claim *proxy.Proxy - claimed bool -} - -func (s *claimingStore) CreateAgentNetworkSettings(ctx context.Context, settings *types.Settings) error { - if !s.claimed { - s.claimed = true - require.NoError(s.t, s.Store.SaveProxy(ctx, s.claim), "the competing claim must land") - } - return s.Store.CreateAgentNetworkSettings(ctx, settings) -} - -// foreignClaim is the competing claim the race tests let land: another -// account's embedded proxy at host. -func foreignClaim(host string) *proxy.Proxy { - return &proxy.Proxy{ - ID: "foreign", - ClusterAddress: host, - Status: proxy.StatusConnected, - LastSeen: time.Now().UTC(), - AccountID: ptrTo("account2"), - Capabilities: proxy.Capabilities{Private: ptrTo(true)}, - } -} - -// failingStore is a store.Store that fails a named call on its nth invocation, -// for the paths where the bootstrap's own bookkeeping cannot be completed: -// an ownership re-read that cannot answer, or a withdrawal that does not go -// through. -type failingStore struct { - store.Store - failOwnershipOn int - failDelete bool - ownershipCalls int -} - -func (s *failingStore) HasForeignAccountProxyAtHost(ctx context.Context, host, accountID string) (bool, error) { - s.ownershipCalls++ - if s.ownershipCalls == s.failOwnershipOn { - return false, errors.New("store unavailable") - } - return s.Store.HasForeignAccountProxyAtHost(ctx, host, accountID) -} - -func (s *failingStore) DeleteAgentNetworkSettings(ctx context.Context, accountID string) error { - if s.failDelete { - return errors.New("delete failed") - } - return s.Store.DeleteAgentNetworkSettings(ctx, accountID) -} - -// TestCreateSettingsWithdrawsPinClaimedDuringBootstrap covers the window -// between validateGatewayCluster and the insert: a foreign proxy that claims -// the host in that window is seen by the ownership re-read after the write, -// and the pin is withdrawn rather than left standing on a cluster that will -// never serve it. The refusal reads exactly as it would have had the -// pre-write check caught the claim. -func TestCreateSettingsWithdrawsPinClaimedDuringBootstrap(t *testing.T) { - ctx := context.Background() - const host = "shared.example.com" - - f := newBootstrapFixtureWith(t, func(st store.Store) store.Store { - return &claimingStore{Store: st, t: t, claim: foreignClaim(host)} - }) - // A shared embedded cluster, so the pre-write validation passes on its - // own merits and only the claim landing mid-bootstrap can refuse it. - f.seedEmbeddedCluster(t, host) - f.expectPermission("account1", "user1", modules.AgentNetworkSettings, operations.Create, true) - - _, err := f.createSettings(ctx, "account1", "user1", host, "") - require.Error(t, err, "a host claimed by another account mid-bootstrap 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, "the pin written before the claim was seen must be withdrawn") - - foreign, err := f.store.HasForeignAccountProxyAtHost(ctx, host, "account1") - require.NoError(t, err) - assert.True(t, foreign, "the competing claim, having landed first, keeps the host") -} - -// TestCreateSettingsSelfAddressedRejectsForeignHost pins that a self-addressed -// pin is subject to the same ownership rule as a labeled one. The hostname is -// stored as proxy_address, which is exactly what a proxy registration is -// refused on when another account holds it there — so without this check any -// account could pin an endpoint onto a host another account's proxy already -// declares and lock that proxy out on its next reconnect, owning nothing. -func TestCreateSettingsSelfAddressedRejectsForeignHost(t *testing.T) { - ctx := context.Background() - f := newBootstrapFixture(t) - f.seedProxy(t, "foreign", "account2", "gw.example.com", ptrTo(true)) - f.expectPermission("account1", "user1", modules.AgentNetworkSettings, operations.Create, true) - - _, err := f.createSettings(ctx, "account1", "user1", "", "gw.example.com") - require.Error(t, err, "a hostname another account's proxy declares 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") -} - -// TestCreateSettingsSelfAddressedAcceptsOwnHost is the address-first order the -// self-addressed path exists for, in both directions: a hostname no proxy has -// declared, and one the account's own proxy already declares. -func TestCreateSettingsSelfAddressedAcceptsOwnHost(t *testing.T) { - ctx := context.Background() - f := newBootstrapFixture(t) - f.seedProxy(t, "own", "account1", "gw.example.com", ptrTo(true)) - f.expectPermission("account1", "user1", modules.AgentNetworkSettings, operations.Create, true) - - created, err := f.createSettings(ctx, "account1", "user1", "", "gw.example.com") - require.NoError(t, err, "the account's own proxy is not a competing claim") - assert.Equal(t, "gw.example.com", created.ProxyAddress) -} - -// TestCreateSettingsSelfAddressedWithdrawsPinClaimedDuringBootstrap is the -// self-addressed twin of the labeled race: the competing proxy lands as the -// row is written, the re-read sees it, and the pin is withdrawn. -func TestCreateSettingsSelfAddressedWithdrawsPinClaimedDuringBootstrap(t *testing.T) { - ctx := context.Background() - const host = "gw.example.com" - - f := newBootstrapFixtureWith(t, func(st store.Store) store.Store { - return &claimingStore{Store: st, t: t, claim: foreignClaim(host)} - }) - f.expectPermission("account1", "user1", modules.AgentNetworkSettings, operations.Create, true) - - _, err := f.createSettings(ctx, "account1", "user1", "", host) - require.Error(t, err, "a host claimed by another account mid-bootstrap must be refused") - var sErr *status.Error - require.ErrorAs(t, err, &sErr) - assert.Equal(t, status.InvalidArgument, sErr.Type()) - - _, err = f.store.GetAgentNetworkSettings(ctx, store.LockingStrengthNone, "account1") - assert.Error(t, err, "the pin written before the claim was seen must be withdrawn") -} - -// TestCreateSettingsWithdrawsPinWhenOwnershipRecheckFails pins fail-closed on -// the bootstrap side: a re-read that cannot answer leaves no pin behind and -// surfaces the store's error rather than a validation refusal, since nothing -// established that the cluster is somebody else's. -func TestCreateSettingsWithdrawsPinWhenOwnershipRecheckFails(t *testing.T) { - ctx := context.Background() - const host = "shared.example.com" - - // The first ownership call is the pre-write check and must pass; the - // second is the re-read. - f := newBootstrapFixtureWith(t, func(st store.Store) store.Store { - return &failingStore{Store: st, failOwnershipOn: 2} - }) - f.seedEmbeddedCluster(t, host) - f.expectPermission("account1", "user1", modules.AgentNetworkSettings, operations.Create, true) - - _, err := f.createSettings(ctx, "account1", "user1", host, "") - require.Error(t, err) - var sErr *status.Error - assert.False(t, errors.As(err, &sErr) && sErr.Type() == status.InvalidArgument, - "an inconclusive re-read is not a validation refusal: %v", err) - assert.ErrorContains(t, err, "store unavailable", "the store's error must be the one surfaced") - - _, err = f.store.GetAgentNetworkSettings(ctx, store.LockingStrengthNone, "account1") - assert.Error(t, err, "a pin that could not be confirmed must not stand") -} - -// TestCreateSettingsRefusesEvenWhenWithdrawalFails pins that a lost claim is -// reported as lost whatever happens to the compensating delete: the caller -// must not be told it holds a cluster another account's proxy declares. -func TestCreateSettingsRefusesEvenWhenWithdrawalFails(t *testing.T) { - ctx := context.Background() - const host = "shared.example.com" - - f := newBootstrapFixtureWith(t, func(st store.Store) store.Store { - return &failingStore{Store: &claimingStore{Store: st, t: t, claim: foreignClaim(host)}, failDelete: true} - }) - f.seedEmbeddedCluster(t, host) - f.expectPermission("account1", "user1", modules.AgentNetworkSettings, operations.Create, true) - - _, err := f.createSettings(ctx, "account1", "user1", host, "") - require.Error(t, err) - var sErr *status.Error - require.ErrorAs(t, err, &sErr) - assert.Equal(t, status.InvalidArgument, sErr.Type(), "a failed withdrawal must not turn a lost claim into a held one") - assert.Contains(t, err.Error(), "not available to this account") -} - -// 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) { @@ -644,13 +360,10 @@ func TestCreateSettingsAcceptsOwnPrivateCluster(t *testing.T) { // TestCreateSettingsMatchesClusterCasing pins that a cluster spelled with // capitals in the store is still recognised as the same cluster the normalised -// proxy_address names. Addresses are canonicalised where they are written -// (canonicalProxyAddress on the proxy-connect path), so this is the belt to -// that braces: it covers a row written before that landed, and any future -// writer that skips it. The comparison is in memory over the account's cluster -// list, so it costs nothing at the query — the capability lookup is still -// asked under the spelling the store actually holds, which is what an exact, -// indexed match needs. +// proxy_address names, in both directions: a private cluster is accepted and a +// centralised one is refused, whatever the casing. The comparison is in memory +// over the account's cluster list; the capability lookup is still asked under +// the spelling the store actually holds, which is what an exact match needs. func TestCreateSettingsMatchesClusterCasing(t *testing.T) { ctx := context.Background() @@ -664,19 +377,6 @@ func TestCreateSettingsMatchesClusterCasing(t *testing.T) { assert.Equal(t, "eu.proxy.example.com", created.ProxyAddress) }) - t.Run("foreign cluster is still foreign", func(t *testing.T) { - f := newBootstrapFixture(t) - f.seedProxy(t, "proxy1", "account2", "BYOP.Account2.Example.com", ptrTo(true)) - f.expectPermission("account1", "user1", modules.AgentNetworkSettings, operations.Create, true) - - _, err := f.createSettings(ctx, "account1", "user1", "byop.account2.example.com", "") - require.Error(t, err, "another account's cluster must be refused whatever its casing") - 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") - }) - t.Run("non-private cluster is still refused", func(t *testing.T) { f := newBootstrapFixture(t) f.seedProxy(t, "proxy1", "", "Central.Example.com", ptrTo(false)) diff --git a/management/internals/modules/reverseproxy/proxy/manager/manager.go b/management/internals/modules/reverseproxy/proxy/manager/manager.go index 67085e9b0..943766004 100644 --- a/management/internals/modules/reverseproxy/proxy/manager/manager.go +++ b/management/internals/modules/reverseproxy/proxy/manager/manager.go @@ -2,7 +2,6 @@ package manager import ( "context" - "fmt" "time" log "github.com/sirupsen/logrus" @@ -15,7 +14,6 @@ import ( type store interface { SaveProxy(ctx context.Context, p *proxy.Proxy) error DisconnectProxy(ctx context.Context, proxyID, sessionID string) error - DeleteProxy(ctx context.Context, proxyID, sessionID string) error UpdateProxyHeartbeat(ctx context.Context, p *proxy.Proxy) error GetActiveProxyClusterAddresses(ctx context.Context) ([]string, error) GetActiveProxyClusterAddressesForAccount(ctx context.Context, accountID string) ([]string, error) @@ -28,7 +26,6 @@ type store interface { GetProxyByAccountID(ctx context.Context, accountID string) (*proxy.Proxy, error) CountProxiesByAccountID(ctx context.Context, accountID string) (int64, error) IsClusterAddressConflicting(ctx context.Context, clusterAddress, accountID string) (bool, error) - HasGatewayPinnedByOtherAccount(ctx context.Context, host, accountID string) (bool, error) DeleteAccountCluster(ctx context.Context, clusterAddress, accountID string) error } @@ -76,12 +73,6 @@ func (m *Manager) Connect(ctx context.Context, proxyID, sessionID, clusterAddres return nil, err } - if accountID != nil { - if err := m.confirmClusterAddressClaim(ctx, p, *accountID); err != nil { - return nil, err - } - } - log.WithContext(ctx).WithFields(log.Fields{ "proxyID": proxyID, "sessionID": sessionID, @@ -92,35 +83,6 @@ func (m *Manager) Connect(ctx context.Context, proxyID, sessionID, clusterAddres return p, nil } -// confirmClusterAddressClaim re-reads availability once the proxy's row is -// committed and withdraws the row if the claim is lost; see -// proxy.ErrClusterAddressUnavailable for why the re-read is what closes the -// race with a concurrent claim. An inconclusive re-read refuses the connect -// but only marks the row disconnected: SaveProxy upserts on the proxy ID, so -// on a reconnect the row is a claim the account already held, and a transient -// store error must not surrender it. -func (m *Manager) confirmClusterAddressClaim(ctx context.Context, p *proxy.Proxy, accountID string) error { - available, err := m.IsClusterAddressAvailable(ctx, p.ClusterAddress, accountID) - if err != nil { - if discErr := m.store.DisconnectProxy(ctx, p.ID, p.SessionID); discErr != nil { - log.WithContext(ctx).Errorf("failed to mark proxy %s session %s disconnected after an inconclusive claim check on %s: %v", - p.ID, p.SessionID, p.ClusterAddress, discErr) - } - return fmt.Errorf("confirm claim on cluster address %s: %w", p.ClusterAddress, err) - } - if available { - return nil - } - - if delErr := m.store.DeleteProxy(ctx, p.ID, p.SessionID); delErr != nil { - log.WithContext(ctx).Errorf("failed to withdraw proxy %s session %s after losing the claim on %s: %v", - p.ID, p.SessionID, p.ClusterAddress, delErr) - } - log.WithContext(ctx).Warnf("cluster address %s was claimed while proxy %s registered for account %s, withdrawing its row", - p.ClusterAddress, p.ID, accountID) - return fmt.Errorf("cluster address %s: %w", p.ClusterAddress, proxy.ErrClusterAddressUnavailable) -} - // Disconnect marks a proxy as disconnected in the database. func (m *Manager) Disconnect(ctx context.Context, proxyID, sessionID string) error { if err := m.store.DisconnectProxy(ctx, proxyID, sessionID); err != nil { @@ -207,36 +169,12 @@ func (m *Manager) CountAccountProxies(ctx context.Context, accountID string) (in return m.store.CountProxiesByAccountID(ctx, accountID) } -// IsClusterAddressAvailable reports whether the account may claim this cluster -// address. -// -// Two kinds of claim make an address unavailable, and both are checked here so -// that no caller can consult one and forget the other. A proxy row is the -// obvious one. An agent network gateway pinned to the address by another -// account is the second: that pin is immutable and is served by whichever -// proxy declares the address, so letting a proxy from a different account take -// it strands the pin — an account-scoped proxy never receives another -// account's mappings. An account claiming an address its own gateway is pinned -// to is the intended order, not a conflict: pin first, deploy the proxy after. func (m *Manager) IsClusterAddressAvailable(ctx context.Context, clusterAddress, accountID string) (bool, error) { conflicting, err := m.store.IsClusterAddressConflicting(ctx, clusterAddress, accountID) if err != nil { return false, err } - if conflicting { - return false, nil - } - - pinned, err := m.store.HasGatewayPinnedByOtherAccount(ctx, clusterAddress, accountID) - if err != nil { - return false, err - } - if pinned { - log.WithContext(ctx).Infof("cluster address %s is pinned as another account's agent network gateway, refusing claim by account %s", clusterAddress, accountID) - return false, nil - } - - return true, nil + return !conflicting, nil } func (m *Manager) DeleteAccountCluster(ctx context.Context, clusterAddress, accountID string) error { diff --git a/management/internals/modules/reverseproxy/proxy/manager/manager_store_test.go b/management/internals/modules/reverseproxy/proxy/manager/manager_store_test.go deleted file mode 100644 index e7fa349fd..000000000 --- a/management/internals/modules/reverseproxy/proxy/manager/manager_store_test.go +++ /dev/null @@ -1,91 +0,0 @@ -package manager - -import ( - "context" - "runtime" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - "go.opentelemetry.io/otel/metric/noop" - - agentNetworkTypes "github.com/netbirdio/netbird/management/internals/modules/agentnetwork/types" - "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/proxy" - nbstore "github.com/netbirdio/netbird/management/server/store" - nbtypes "github.com/netbirdio/netbird/management/server/types" -) - -// newStoreBackedManager wires the manager to a real sqlite store, for the -// cases where what matters is how the store's own queries answer the -// post-write re-read — which the function-field mock cannot say. -func newStoreBackedManager(t *testing.T) (*Manager, nbstore.Store) { - t.Helper() - if runtime.GOOS == "windows" { - t.Skip("sqlite store not properly supported on Windows yet") - } - t.Setenv("NETBIRD_STORE_ENGINE", string(nbtypes.SqliteStoreEngine)) - - st, cleanUp, err := nbstore.NewTestStoreFromSQL(context.Background(), "", t.TempDir()) - require.NoError(t, err, "test store setup must succeed") - t.Cleanup(cleanUp) - - mgr, err := NewManager(st, noop.NewMeterProvider().Meter("test")) - require.NoError(t, err) - return mgr, st -} - -// TestConnect_RealStore_ConfirmsOwnClaims drives the post-write re-read -// through the real queries. Every account-scoped connect now reads its own -// just-written row back, so the whole path depends on the store excluding the -// account's own claims: its own proxy row on a reconnect, and its own gateway -// pin when the account deploys a proxy at the address it pinned first. -func TestConnect_RealStore_ConfirmsOwnClaims(t *testing.T) { - ctx := context.Background() - accountID := "account1" - const host = "byop.account1.example.com" - - t.Run("a reconnect keeps the proxy's own row", func(t *testing.T) { - mgr, st := newStoreBackedManager(t) - - _, err := mgr.Connect(ctx, "proxy-1", "session-1", host, "10.0.0.1", &accountID, nil) - require.NoError(t, err, "first connect must succeed") - _, err = mgr.Connect(ctx, "proxy-1", "session-2", host, "10.0.0.1", &accountID, nil) - require.NoError(t, err, "a reconnect must not be refused by the row it is replacing") - - rows, err := st.GetAllProxies(ctx) - require.NoError(t, err) - require.Len(t, rows, 1, "a reconnect upserts the same row") - assert.Equal(t, "session-2", rows[0].SessionID, "the row must carry the new session") - assert.Equal(t, proxy.StatusConnected, rows[0].Status) - }) - - t.Run("the account's own gateway pin is not a competing claim", func(t *testing.T) { - mgr, st := newStoreBackedManager(t) - settings := agentNetworkTypes.DefaultSettings(accountID) - settings.Domain = host - settings.ProxyAddress = host - require.NoError(t, st.CreateAgentNetworkSettings(ctx, settings), "seeding the account's own pin must succeed") - - _, err := mgr.Connect(ctx, "proxy-1", "session-1", host, "10.0.0.1", &accountID, nil) - require.NoError(t, err, "pin first, deploy the proxy after is the documented order") - - rows, err := st.GetAllProxies(ctx) - require.NoError(t, err) - assert.Len(t, rows, 1, "the proxy's row must stand next to the account's own pin") - }) - - t.Run("another account's gateway pin withdraws the row", func(t *testing.T) { - mgr, st := newStoreBackedManager(t) - settings := agentNetworkTypes.DefaultSettings("account2") - settings.Domain = host - settings.ProxyAddress = host - require.NoError(t, st.CreateAgentNetworkSettings(ctx, settings), "seeding the other account's pin must succeed") - - _, err := mgr.Connect(ctx, "proxy-1", "session-1", host, "10.0.0.1", &accountID, nil) - require.ErrorIs(t, err, proxy.ErrClusterAddressUnavailable) - - rows, err := st.GetAllProxies(ctx) - require.NoError(t, err) - assert.Empty(t, rows, "a withdrawn registration must leave no row behind") - }) -} diff --git a/management/internals/modules/reverseproxy/proxy/manager/manager_test.go b/management/internals/modules/reverseproxy/proxy/manager/manager_test.go index 4c906a69f..5c44470a3 100644 --- a/management/internals/modules/reverseproxy/proxy/manager/manager_test.go +++ b/management/internals/modules/reverseproxy/proxy/manager/manager_test.go @@ -24,9 +24,7 @@ type mockStore struct { getProxyByAccountIDFunc func(ctx context.Context, accountID string) (*proxy.Proxy, error) countProxiesByAccountIDFunc func(ctx context.Context, accountID string) (int64, error) isClusterAddressConflictingFunc func(ctx context.Context, clusterAddress, accountID string) (bool, error) - hasGatewayPinnedByOtherAccountFunc func(ctx context.Context, host, accountID string) (bool, error) deleteAccountClusterFunc func(ctx context.Context, clusterAddress, accountID string) error - deleteProxyFunc func(ctx context.Context, proxyID, sessionID string) error } func (m *mockStore) SaveProxy(ctx context.Context, p *proxy.Proxy) error { @@ -41,12 +39,6 @@ func (m *mockStore) DisconnectProxy(ctx context.Context, proxyID, sessionID stri } return nil } -func (m *mockStore) DeleteProxy(ctx context.Context, proxyID, sessionID string) error { - if m.deleteProxyFunc != nil { - return m.deleteProxyFunc(ctx, proxyID, sessionID) - } - return nil -} func (m *mockStore) UpdateProxyHeartbeat(ctx context.Context, p *proxy.Proxy) error { if m.updateProxyHeartbeatFunc != nil { return m.updateProxyHeartbeatFunc(ctx, p) @@ -92,12 +84,6 @@ func (m *mockStore) IsClusterAddressConflicting(ctx context.Context, clusterAddr } return false, nil } -func (m *mockStore) HasGatewayPinnedByOtherAccount(ctx context.Context, host, accountID string) (bool, error) { - if m.hasGatewayPinnedByOtherAccountFunc != nil { - return m.hasGatewayPinnedByOtherAccountFunc(ctx, host, accountID) - } - return false, nil -} func (m *mockStore) DeleteAccountCluster(ctx context.Context, clusterAddress, accountID string) error { if m.deleteAccountClusterFunc != nil { return m.deleteAccountClusterFunc(ctx, clusterAddress, accountID) @@ -352,203 +338,3 @@ func TestGetActiveClusterAddressesForAccount(t *testing.T) { require.NoError(t, err) assert.Equal(t, expected, result) } - -// TestIsClusterAddressAvailableConsidersGatewayPins pins that a proxy row is -// not the only claim on an address. -// -// An agent network gateway pinned to the address by another account is -// immutable and is served by whichever proxy declares that address, so a proxy -// from a different account taking it strands the pin — the mapping paths never -// hand an account-scoped proxy another account's mappings. Refusing the later -// claimant is what makes the bootstrap-time ownership check hold over time -// rather than only at the instant it runs: without this, an address a gateway -// pinned while no proxy served it could be taken a moment, or a week, later. -func TestIsClusterAddressAvailableConsidersGatewayPins(t *testing.T) { - ctx := context.Background() - - tests := []struct { - name string - conflicting bool - pinned bool - available bool - }{ - {name: "free address", available: true}, - {name: "claimed by a proxy", conflicting: true}, - {name: "pinned by another account's gateway", pinned: true}, - {name: "claimed both ways", conflicting: true, pinned: true}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - st := &mockStore{ - isClusterAddressConflictingFunc: func(context.Context, string, string) (bool, error) { - return tt.conflicting, nil - }, - hasGatewayPinnedByOtherAccountFunc: func(context.Context, string, string) (bool, error) { - return tt.pinned, nil - }, - } - m, err := NewManager(st, noop.NewMeterProvider().Meter("")) - require.NoError(t, err) - - available, err := m.IsClusterAddressAvailable(ctx, "gw.example.com", "account1") - require.NoError(t, err) - assert.Equal(t, tt.available, available) - }) - } -} - -// TestIsClusterAddressAvailableSurfacesGatewayPinError pins that a failed pin -// lookup refuses the claim rather than falling through to available: this runs -// on the proxy-connect path, where "could not tell" must not read as "yes". -func TestIsClusterAddressAvailableSurfacesGatewayPinError(t *testing.T) { - st := &mockStore{ - hasGatewayPinnedByOtherAccountFunc: func(context.Context, string, string) (bool, error) { - return false, errors.New("db down") - }, - } - m, err := NewManager(st, noop.NewMeterProvider().Meter("")) - require.NoError(t, err) - - available, err := m.IsClusterAddressAvailable(context.Background(), "gw.example.com", "account1") - require.Error(t, err) - assert.False(t, available) -} - -// TestConnect_WithdrawsClaimLostDuringRegistration covers the window between -// the connect path's availability check and the row being written: a claim -// that lands there — another account's proxy row or gateway pin — is seen by -// the re-read after the write, and the proxy's own row is withdrawn rather -// than left standing next to it. The refusal carries -// ErrClusterAddressUnavailable so the connect path reports it exactly as it -// would have had the pre-write check caught it. -func TestConnect_WithdrawsClaimLostDuringRegistration(t *testing.T) { - accountID := "acc-1" - - cases := map[string]func(s *mockStore, landed *bool){ - "another account pinned its gateway to the address": func(s *mockStore, landed *bool) { - s.hasGatewayPinnedByOtherAccountFunc = func(_ context.Context, _, _ string) (bool, error) { return *landed, nil } - }, - "another account's proxy declared the address": func(s *mockStore, landed *bool) { - s.isClusterAddressConflictingFunc = func(_ context.Context, _, _ string) (bool, error) { return *landed, nil } - }, - } - for name, arm := range cases { - t.Run(name, func(t *testing.T) { - landed := false - var withdrawn []string - s := &mockStore{ - // The competing claim commits as this row is written: the - // pre-write check (not exercised here) saw nothing, the - // re-read must. - saveProxyFunc: func(_ context.Context, _ *proxy.Proxy) error { landed = true; return nil }, - deleteProxyFunc: func(_ context.Context, proxyID, sessionID string) error { - withdrawn = append(withdrawn, proxyID+"/"+sessionID) - return nil - }, - } - arm(s, &landed) - - mgr := newTestManager(s) - p, err := mgr.Connect(context.Background(), "proxy-1", "session-1", "gw.example.com", "10.0.0.1", &accountID, nil) - require.ErrorIs(t, err, proxy.ErrClusterAddressUnavailable, "a claim lost after the write must surface as the address being unavailable") - assert.Nil(t, p, "no record may be handed back for a withdrawn registration") - assert.Equal(t, []string{"proxy-1/session-1"}, withdrawn, "exactly this session's row must be withdrawn") - }) - } -} - -// TestConnect_KeepsClaimWhenRecheckFails pins what an inconclusive re-read -// does: the connect is refused with the store's error, not -// ErrClusterAddressUnavailable, since nothing established that the address is -// taken — and the row is marked disconnected rather than deleted. SaveProxy -// upserts on the proxy ID, so on a reconnect that row is the claim the account -// has held since its first connect; a transient store error must not hand the -// address to whoever asks next. -func TestConnect_KeepsClaimWhenRecheckFails(t *testing.T) { - accountID := "acc-1" - var disconnected []string - s := &mockStore{ - hasGatewayPinnedByOtherAccountFunc: func(_ context.Context, _, _ string) (bool, error) { - return false, errors.New("db unavailable") - }, - disconnectProxyFunc: func(_ context.Context, proxyID, sessionID string) error { - disconnected = append(disconnected, proxyID+"/"+sessionID) - return nil - }, - deleteProxyFunc: func(_ context.Context, proxyID, _ string) error { - t.Fatalf("an inconclusive re-read must not withdraw the row, but proxy %s was deleted", proxyID) - return nil - }, - } - - mgr := newTestManager(s) - _, err := mgr.Connect(context.Background(), "proxy-1", "session-1", "gw.example.com", "10.0.0.1", &accountID, nil) - require.Error(t, err) - assert.NotErrorIs(t, err, proxy.ErrClusterAddressUnavailable, "an inconclusive re-read is not a conflict") - assert.ErrorContains(t, err, "db unavailable", "the store's error must be the one surfaced") - assert.Equal(t, []string{"proxy-1/session-1"}, disconnected, "the refused session must not stay marked connected") -} - -// TestConnect_RefusesEvenWhenWithdrawalFails pins that a lost claim is -// reported as lost whatever happens to the compensating delete: the caller -// must never be told it holds an address another claim already has, and the -// stale row is the reaper's problem, not a reason to lie. -func TestConnect_RefusesEvenWhenWithdrawalFails(t *testing.T) { - accountID := "acc-1" - s := &mockStore{ - hasGatewayPinnedByOtherAccountFunc: func(_ context.Context, _, _ string) (bool, error) { return true, nil }, - deleteProxyFunc: func(_ context.Context, _, _ string) error { - return errors.New("delete failed") - }, - } - - mgr := newTestManager(s) - p, err := mgr.Connect(context.Background(), "proxy-1", "session-1", "gw.example.com", "10.0.0.1", &accountID, nil) - require.ErrorIs(t, err, proxy.ErrClusterAddressUnavailable, "a failed withdrawal must not turn a lost claim into a held one") - assert.Nil(t, p) -} - -// TestConnect_ConfirmedClaimKeepsRow is the common case: nothing landed in the -// window, the re-read confirms the claim, and the row stays. -func TestConnect_ConfirmedClaimKeepsRow(t *testing.T) { - accountID := "acc-1" - s := &mockStore{ - deleteProxyFunc: func(_ context.Context, proxyID, _ string) error { - t.Fatalf("a confirmed claim must not be withdrawn, but proxy %s was", proxyID) - return nil - }, - } - - mgr := newTestManager(s) - p, err := mgr.Connect(context.Background(), "proxy-1", "session-1", "gw.example.com", "10.0.0.1", &accountID, nil) - require.NoError(t, err) - require.NotNil(t, p) - assert.Equal(t, proxy.StatusConnected, p.Status) -} - -// TestConnect_SharedProxySkipsClaimRecheck pins that a shared, NetBird-operated -// proxy — no account on its token — is not subject to the claim re-read: the -// connect path never asks availability for it before the write either, and a -// shared cluster is what accounts pin their gateways to, not a claim against -// them. -func TestConnect_SharedProxySkipsClaimRecheck(t *testing.T) { - s := &mockStore{ - isClusterAddressConflictingFunc: func(_ context.Context, _, _ string) (bool, error) { - t.Fatal("a shared proxy must not be checked for address conflicts") - return false, nil - }, - hasGatewayPinnedByOtherAccountFunc: func(_ context.Context, _, _ string) (bool, error) { - t.Fatal("a shared proxy must not be checked against gateway pins") - return false, nil - }, - deleteProxyFunc: func(_ context.Context, _, _ string) error { - t.Fatal("a shared proxy's row must not be withdrawn") - return nil - }, - } - - mgr := newTestManager(s) - _, err := mgr.Connect(context.Background(), "proxy-1", "session-1", "eu.proxy.netbird.io", "10.0.0.1", nil, nil) - require.NoError(t, err) -} diff --git a/management/internals/modules/reverseproxy/proxy/proxy.go b/management/internals/modules/reverseproxy/proxy/proxy.go index bf36e7820..4404b0d24 100644 --- a/management/internals/modules/reverseproxy/proxy/proxy.go +++ b/management/internals/modules/reverseproxy/proxy/proxy.go @@ -1,7 +1,6 @@ package proxy import ( - "errors" "time" ) @@ -10,25 +9,6 @@ const ( StatusDisconnected = "disconnected" ) -// ErrClusterAddressUnavailable is returned by Manager.Connect when the cluster -// address turns out to be claimed by someone else once the proxy's own row is -// written: a conflicting proxy row, or another account's agent network gateway -// pinned to the address. The row has been withdrawn by then, and the caller -// reports the address as taken exactly as if the pre-write check had caught it. -// -// Both kinds of claim are made the same way, write then re-read then withdraw, -// and the re-read is the whole mechanism. Each side's availability check and -// its write are separate autocommit statements, so two concurrent claimants -// can each pass their check with neither row committed yet. Because both write -// before they re-read, of two concurrent claims at least one re-reads after -// the other has committed and backs off; each statement sees every commit -// before it on sqlite, postgres and mysql alike. Both may back off, which -// costs a retry; neither keeps a claim the other holds. No lock spans the -// proxies and settings tables portably, and a claims table would be more -// machinery than the property needs. The gateway side of the same protocol is -// agentnetwork's confirmGatewayClusterOwnership. -var ErrClusterAddressUnavailable = errors.New("cluster address is not available") - // Capabilities describes what a proxy can handle, as reported via gRPC. // Nil fields mean the proxy never reported this capability. type Capabilities struct { diff --git a/management/internals/shared/grpc/proxy.go b/management/internals/shared/grpc/proxy.go index 47cc4cf5b..2fc969ad0 100644 --- a/management/internals/shared/grpc/proxy.go +++ b/management/internals/shared/grpc/proxy.go @@ -13,7 +13,6 @@ import ( "math" "net" "net/http" - "net/netip" "net/url" "os" "strconv" @@ -496,8 +495,7 @@ func (s *ProxyServiceServer) validateProxyConnect(proxyID, address string, ctx c if proxyID == "" { return proxyConnectParams{}, status.Errorf(codes.InvalidArgument, "proxy_id is required") } - address, ok := canonicalProxyAddress(address) - if !ok { + if !isProxyAddressValid(address) { return proxyConnectParams{}, status.Errorf(codes.InvalidArgument, "proxy address is invalid") } @@ -571,12 +569,6 @@ func (s *ProxyServiceServer) registerProxyConnection(ctx context.Context, params proxyRecord, err := s.proxyManager.Connect(ctx, params.proxyID, sessionID, params.address, peerInfo, accountID, caps) if err != nil { cancel() - if errors.Is(err, proxy.ErrClusterAddressUnavailable) { - // The claim was lost to a concurrent one after validateProxyConnect - // saw the address free; the row has been withdrawn. Same answer - // as the pre-write check gives, so the proxy treats both alike. - return nil, nil, status.Errorf(codes.AlreadyExists, "cluster address %s is already in use", params.address) - } if accountID != nil { return nil, nil, status.Errorf(codes.Internal, "failed to register BYOP proxy: %v", err) } @@ -880,44 +872,16 @@ func (s *ProxyServiceServer) snapshotServiceMappings(ctx context.Context, conn * return mappings, nil } -// canonicalProxyAddress validates a proxy address (domain name or IP address) -// and returns the form the store keeps. -// -// cluster_address is the key every capability, ownership and routing lookup -// matches on, and this is the only path that writes it, so the address is -// canonicalised once here rather than pushing case-insensitivity into each of -// those queries: hostnames are case-insensitive and may be unicode, so they -// fold to lowercase punycode (what domain.ValidateDomains already computes and -// this used to throw away), and an IP literal goes through netip so a mixed -// case IPv6 address does not become a second key for the same host. -func canonicalProxyAddress(addr string) (string, bool) { - if addr == "" { - return "", false - } - // A zoned literal like "fe80::1%eth0" is scoped to one host's interface, - // so it cannot identify a cluster others reach; net.ParseIP rejected it - // before and netip must not start accepting it. - if ip, err := netip.ParseAddr(addr); err == nil { - if ip.Zone() != "" { - return "", false - } - return ip.String(), true - } - // Folded before punycode conversion, not just after: idna maps the ASCII - // output to lowercase but does not case-fold the unicode input, so - // "PRÖXY.example.com" and "pröxy.example.com" would otherwise encode to - // two different labels for one host. - canonical, err := domain.ValidateDomains([]string{strings.ToLower(addr)}) - if err != nil || len(canonical) != 1 { - return "", false - } - return string(canonical[0]), true -} - // isProxyAddressValid validates a proxy address (domain name or IP address) func isProxyAddressValid(addr string) bool { - _, ok := canonicalProxyAddress(addr) - return ok + if addr == "" { + return false + } + if net.ParseIP(addr) != nil { + return true + } + _, err := domain.ValidateDomains([]string{addr}) + return err == nil } // isStreamClosed returns true for errors that indicate normal stream diff --git a/management/internals/shared/grpc/proxy_address_test.go b/management/internals/shared/grpc/proxy_address_test.go index eb6ec1f18..824a57226 100644 --- a/management/internals/shared/grpc/proxy_address_test.go +++ b/management/internals/shared/grpc/proxy_address_test.go @@ -27,41 +27,3 @@ func TestIsProxyAddressValid(t *testing.T) { }) } } - -// TestCanonicalProxyAddress pins the canonical form the store keeps. Every -// capability, ownership and routing lookup matches cluster_address exactly, so -// one host must have exactly one spelling in that column — which is what lets -// those queries stay exact (and keep using the index) instead of folding case -// per query. -func TestCanonicalProxyAddress(t *testing.T) { - tests := []struct { - name string - addr string - canonical string - ok bool - }{ - {name: "lowercase domain unchanged", addr: "eu.proxy.netbird.io", canonical: "eu.proxy.netbird.io", ok: true}, - {name: "mixed case domain folded", addr: "EU.Proxy.NetBird.io", canonical: "eu.proxy.netbird.io", ok: true}, - {name: "uppercase domain folded", addr: "BYOP.PROXY.EXAMPLE.COM", canonical: "byop.proxy.example.com", ok: true}, - {name: "unicode domain punycoded", addr: "pröxy.example.com", canonical: "xn--prxy-6qa.example.com", ok: true}, - // Same host, and idna alone would encode the two cases to different - // labels, so this is the one that proves the fold happens first. - {name: "mixed case unicode folds to the same label", addr: "PRÖXY.example.com", canonical: "xn--prxy-6qa.example.com", ok: true}, - {name: "ipv4 unchanged", addr: "203.0.113.10", canonical: "203.0.113.10", ok: true}, - {name: "mixed case ipv6 canonicalised", addr: "2001:DB8::1", canonical: "2001:db8::1", ok: true}, - {name: "empty string rejected", addr: "", ok: false}, - {name: "space rejected", addr: "eu proxy.example.com", ok: false}, - // Scoped to one host's interface, so it cannot name a cluster others - // reach; net.ParseIP rejected these and netip must not accept them. - {name: "zoned ipv6 rejected", addr: "fe80::1%eth0", ok: false}, - {name: "unzoned link-local ipv6 accepted", addr: "fe80::1", canonical: "fe80::1", ok: true}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - canonical, ok := canonicalProxyAddress(tt.addr) - assert.Equal(t, tt.ok, ok) - assert.Equal(t, tt.canonical, canonical) - }) - } -} diff --git a/management/internals/shared/grpc/proxy_connect_authorizer_test.go b/management/internals/shared/grpc/proxy_connect_authorizer_test.go index c8dae29fb..d0d196d20 100644 --- a/management/internals/shared/grpc/proxy_connect_authorizer_test.go +++ b/management/internals/shared/grpc/proxy_connect_authorizer_test.go @@ -3,12 +3,11 @@ package grpc import ( "context" "errors" - "fmt" "testing" + "go.uber.org/mock/gomock" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "go.uber.org/mock/gomock" "google.golang.org/grpc/codes" grpcstatus "google.golang.org/grpc/status" @@ -167,27 +166,3 @@ func TestValidateProxyConnect_AuthorizerRunsLast(t *testing.T) { assert.Equal(t, codes.AlreadyExists, st.Code(), "an address conflict must keep its own status") assert.Zero(t, auth.called, "a conflicting address must be rejected before policy runs") } - -// TestRegisterProxyConnection_LostClaimIsAlreadyExists pins the status a proxy -// sees when its claim is lost after validateProxyConnect passed: the manager -// withdraws the row and reports ErrClusterAddressUnavailable, and the connect -// path must answer AlreadyExists — the same code the pre-write check gives — -// rather than the Internal it uses for a store failure, so the proxy handles -// the two paths to "address taken" identically. -func TestRegisterProxyConnection_LostClaimIsAlreadyExists(t *testing.T) { - ctrl := gomock.NewController(t) - mgr := proxy.NewMockManager(ctrl) - mgr.EXPECT(). - Connect(gomock.Any(), "proxy-1", gomock.Any(), "cluster.example.com", gomock.Any(), gomock.Any(), gomock.Any()). - Return(nil, fmt.Errorf("cluster address cluster.example.com: %w", proxy.ErrClusterAddressUnavailable)) - s := &ProxyServiceServer{proxyManager: mgr} - - _, _, err := s.registerProxyConnection(scopedCtx("acc-1"), proxyConnectParams{proxyID: "proxy-1", address: "cluster.example.com"}, &proxyConnection{}) - require.Error(t, err) - st, ok := grpcstatus.FromError(err) - require.True(t, ok) - assert.Equal(t, codes.AlreadyExists, st.Code(), "a claim lost after the check must read as the address being taken") - assert.Contains(t, st.Message(), "already in use") - _, tracked := s.connectedProxies.Load("proxy-1") - assert.False(t, tracked, "a refused registration must not be tracked as connected") -} diff --git a/management/server/migration/migration_agentnetwork.go b/management/server/migration/migration_agentnetwork.go index 2daaff2db..c6cda56c2 100644 --- a/management/server/migration/migration_agentnetwork.go +++ b/management/server/migration/migration_agentnetwork.go @@ -3,7 +3,6 @@ package migration import ( "context" "fmt" - "strings" log "github.com/sirupsen/logrus" "gorm.io/gorm" @@ -67,19 +66,12 @@ func MigrateAgentNetworkSettingsToDomain(ctx context.Context, db *gorm.DB) error } if hasCluster { - // The legacy bootstrap stored the cluster as the caller spelled - // it, trimmed but never folded, while every reader of these - // columns matches exactly against canonical lowercase: proxy - // addresses are canonicalised at connect, and the proxy's host - // map is keyed by the domain verbatim. Fold here so the reshaped - // row is addressable, rather than copying a spelling nothing - // will match. - concat := "LOWER(subdomain || '.' || cluster)" + concat := "subdomain || '.' || cluster" if tx.Name() == "mysql" { - concat = "LOWER(CONCAT(subdomain, '.', cluster))" + concat = "CONCAT(subdomain, '.', cluster)" } res := tx.Exec(fmt.Sprintf( - "UPDATE agent_network_settings SET domain = %s, proxy_address = LOWER(cluster) WHERE (domain IS NULL OR domain = '') AND cluster <> '' AND subdomain <> ''", + "UPDATE agent_network_settings SET domain = %s, proxy_address = cluster WHERE (domain IS NULL OR domain = '') AND cluster <> '' AND subdomain <> ''", concat, )) if res.Error != nil { @@ -96,9 +88,6 @@ func MigrateAgentNetworkSettingsToDomain(ctx context.Context, db *gorm.DB) error unmigratable, ) } - if err := failOnDuplicateAgentNetworkDomains(tx); err != nil { - return err - } if res.RowsAffected > 0 { log.WithContext(ctx).Infof("migrated %d agent_network_settings row(s) to domain/proxy_address", res.RowsAffected) @@ -121,83 +110,3 @@ func MigrateAgentNetworkSettingsToDomain(ctx context.Context, db *gorm.DB) error return nil }) } - -// agentNetworkSettingsIdentity is the post-reshape view of the two identity -// columns, enough for the normaliser to address the table without importing -// the current model. -type agentNetworkSettingsIdentity struct { - AccountID string `gorm:"primaryKey"` - Domain string `gorm:"type:varchar(255)"` - ProxyAddress string `gorm:"type:varchar(255)"` -} - -func (agentNetworkSettingsIdentity) TableName() string { return "agent_network_settings" } - -// NormalizeAgentNetworkSettingsIdentity lowercases domain and proxy_address on -// rows already reshaped by a release whose backfill copied the legacy cluster -// spelling verbatim. -// -// Every reader of these columns matches exactly against canonical lowercase: -// the gateway-pin check a proxy registration runs and cluster-scoped mapping -// synthesis look proxy_address up by the canonical address, the domain lookup -// is followed by an exact Go compare, and the proxy's host map is keyed by the -// domain verbatim. A row that kept capitals is invisible to all of them, so -// the value is repaired where it is stored rather than folded on every read. -// -// MySQL needs the predicate spelled byte-wise: under its default -// case-insensitive collation `domain <> LOWER(domain)` is false for every row, -// which would leave the rows unrepaired while the Go-side compares still miss -// them. Idempotent: the predicate selects only rows that would change, one -// pass over a table holding one row per account. Runs after the reshape, so -// the columns exist whenever the table does. -func NormalizeAgentNetworkSettingsIdentity(ctx context.Context, db *gorm.DB) error { - model := &agentNetworkSettingsIdentity{} - migrator := db.Migrator() - - if !migrator.HasTable(model) || !migrator.HasColumn(model, "Domain") || !migrator.HasColumn(model, "ProxyAddress") { - return nil - } - - if err := failOnDuplicateAgentNetworkDomains(db); err != nil { - return err - } - - predicate := "domain <> LOWER(domain) OR proxy_address <> LOWER(proxy_address)" - if db.Name() == "mysql" { - predicate = "BINARY domain <> BINARY LOWER(domain) OR BINARY proxy_address <> BINARY LOWER(proxy_address)" - } - res := db.Exec("UPDATE agent_network_settings SET domain = LOWER(domain), proxy_address = LOWER(proxy_address) WHERE " + predicate) - if res.Error != nil { - return fmt.Errorf("normalize agent_network_settings identity casing: %w", res.Error) - } - if res.RowsAffected > 0 { - log.WithContext(ctx).Infof("normalized casing on %d agent_network_settings row(s)", res.RowsAffected) - } - - return nil -} - -// failOnDuplicateAgentNetworkDomains refuses to continue when two settings -// rows would fold onto one endpoint hostname. Two accounts cannot share an -// endpoint, the unique index would refuse the fold with a driver message that -// names no row, and there is no right answer as to which account keeps the -// name, so the migration stops and says which hostname needs a human. -func failOnDuplicateAgentNetworkDomains(db *gorm.DB) error { - var rows []struct{ Domain string } - err := db.Raw("SELECT LOWER(domain) AS domain FROM agent_network_settings GROUP BY LOWER(domain) HAVING COUNT(*) > 1"). - Scan(&rows).Error - if err != nil { - return fmt.Errorf("check agent_network_settings for endpoints differing only by case: %w", err) - } - if len(rows) == 0 { - return nil - } - duplicates := make([]string, 0, len(rows)) - for _, row := range rows { - duplicates = append(duplicates, row.Domain) - } - return fmt.Errorf( - "agent_network_settings holds endpoints that differ only by case (%s); resolve them manually before upgrading", - strings.Join(duplicates, ", "), - ) -} diff --git a/management/server/migration/migration_test.go b/management/server/migration/migration_test.go index 662780770..868332fdf 100644 --- a/management/server/migration/migration_test.go +++ b/management/server/migration/migration_test.go @@ -757,11 +757,8 @@ func TestMigrateAgentNetworkSettingsToDomain_BackfillsAndDropsLegacyColumns(t *t db := setupDatabase(t) require.NoError(t, db.Migrator().DropTable(&legacyAgentNetworkSettings{})) require.NoError(t, db.AutoMigrate(&legacyAgentNetworkSettings{})) - // The cluster is spelled the way the legacy bootstrap kept it: as the - // caller typed it, trimmed but never folded. The subdomain was always - // server-assigned lowercase. require.NoError(t, db.Create(&legacyAgentNetworkSettings{ - AccountID: "acct-1", Cluster: "EU.Proxy.NetBird.io", Subdomain: "violet", EnableLogCollection: true, + AccountID: "acct-1", Cluster: "eu.proxy.netbird.io", Subdomain: "violet", EnableLogCollection: true, }).Error) require.NoError(t, db.Create(&legacyAgentNetworkSettings{ AccountID: "acct-2", Cluster: "us.proxy.netbird.io", Subdomain: "violet", @@ -773,10 +770,8 @@ func TestMigrateAgentNetworkSettingsToDomain_BackfillsAndDropsLegacyColumns(t *t var one, two agentNetworkTypes.Settings require.NoError(t, db.First(&one, "account_id = ?", "acct-1").Error) - assert.Equal(t, "violet.eu.proxy.netbird.io", one.Domain, - "domain must combine subdomain and cluster, folded to the canonical lowercase every reader compares against") - assert.Equal(t, "eu.proxy.netbird.io", one.ProxyAddress, - "proxy address must carry the cluster in canonical lowercase, matching what proxies register under") + assert.Equal(t, "violet.eu.proxy.netbird.io", one.Domain, "domain must combine subdomain and cluster") + assert.Equal(t, "eu.proxy.netbird.io", one.ProxyAddress, "proxy address must carry the cluster") assert.True(t, one.EnableLogCollection, "non-identity fields must ride through") require.NoError(t, db.First(&two, "account_id = ?", "acct-2").Error) assert.Equal(t, "violet.us.proxy.netbird.io", two.Domain, @@ -863,95 +858,3 @@ func TestMigrateAgentNetworkSettingsToDomain_ResumesAfterPartialDrop(t *testing. assert.Equal(t, "violet.eu.proxy.netbird.io", row.Domain, "migrated values must be untouched") assert.Equal(t, "eu.proxy.netbird.io", row.ProxyAddress, "migrated values must be untouched") } - -// TestNormalizeAgentNetworkSettingsIdentity_LowercasesReshapedRows covers rows -// a released reshape already copied verbatim: capitals kept from the legacy -// cluster spelling are folded in place, canonical rows are left alone, and -// non-identity fields ride through. -func TestNormalizeAgentNetworkSettingsIdentity_LowercasesReshapedRows(t *testing.T) { - ctx := context.Background() - db := setupDatabase(t) - require.NoError(t, db.Migrator().DropTable(&agentNetworkTypes.Settings{})) - require.NoError(t, db.AutoMigrate(&agentNetworkTypes.Settings{})) - require.NoError(t, db.Create(&agentNetworkTypes.Settings{ - AccountID: "acct-legacy", Domain: "Violet.EU.Proxy.NetBird.io", ProxyAddress: "EU.Proxy.NetBird.io", EnableLogCollection: true, - }).Error) - require.NoError(t, db.Create(&agentNetworkTypes.Settings{ - AccountID: "acct-canonical", Domain: "amber.us.proxy.netbird.io", ProxyAddress: "us.proxy.netbird.io", - }).Error) - - require.NoError(t, migration.NormalizeAgentNetworkSettingsIdentity(ctx, db)) - - var legacy, canonical agentNetworkTypes.Settings - require.NoError(t, db.First(&legacy, "account_id = ?", "acct-legacy").Error) - assert.Equal(t, "violet.eu.proxy.netbird.io", legacy.Domain, "a mixed-case endpoint must be folded where it is stored") - assert.Equal(t, "eu.proxy.netbird.io", legacy.ProxyAddress, "a mixed-case pin must be folded so exact lookups find it") - assert.True(t, legacy.EnableLogCollection, "non-identity fields must ride through") - require.NoError(t, db.First(&canonical, "account_id = ?", "acct-canonical").Error) - assert.Equal(t, "amber.us.proxy.netbird.io", canonical.Domain, "a canonical row must be left as it is") - assert.Equal(t, "us.proxy.netbird.io", canonical.ProxyAddress) - - require.NoError(t, migration.NormalizeAgentNetworkSettingsIdentity(ctx, db), - "a second run over a normalised table must be a no-op, not an error") -} - -// TestNormalizeAgentNetworkSettingsIdentity_SkipsMissingTable pins that a -// store which never had agent network settings is left untouched. -func TestNormalizeAgentNetworkSettingsIdentity_SkipsMissingTable(t *testing.T) { - ctx := context.Background() - db := setupDatabase(t) - require.NoError(t, db.Migrator().DropTable(&agentNetworkTypes.Settings{})) - - require.NoError(t, migration.NormalizeAgentNetworkSettingsIdentity(ctx, db), - "no table must be a no-op, not an error") - assert.False(t, db.Migrator().HasTable(&agentNetworkTypes.Settings{}), "the normaliser must not create the table") -} - -// TestNormalizeAgentNetworkSettingsIdentity_RefusesCaseOnlyCollision pins the -// loud failure: two rows that would fold onto one endpoint stop the migration -// with the hostname named, and neither row is touched, rather than letting the -// unique index refuse the fold with a driver message that names no row. -func TestNormalizeAgentNetworkSettingsIdentity_RefusesCaseOnlyCollision(t *testing.T) { - ctx := context.Background() - db := setupDatabase(t) - if db.Name() == "mysql" { - t.Skip("MySQL's default collation refuses two rows differing only by case at insert; the collision cannot exist there") - } - require.NoError(t, db.Migrator().DropTable(&agentNetworkTypes.Settings{})) - require.NoError(t, db.AutoMigrate(&agentNetworkTypes.Settings{})) - require.NoError(t, db.Create(&agentNetworkTypes.Settings{ - AccountID: "acct-1", Domain: "Violet.eu.proxy.netbird.io", ProxyAddress: "eu.proxy.netbird.io", - }).Error) - require.NoError(t, db.Create(&agentNetworkTypes.Settings{ - AccountID: "acct-2", Domain: "violet.eu.proxy.netbird.io", ProxyAddress: "eu.proxy.netbird.io", - }).Error) - - err := migration.NormalizeAgentNetworkSettingsIdentity(ctx, db) - require.Error(t, err, "two rows folding onto one endpoint must stop the migration") - assert.Contains(t, err.Error(), "violet.eu.proxy.netbird.io", "the failure must name the colliding hostname") - - var one agentNetworkTypes.Settings - require.NoError(t, db.First(&one, "account_id = ?", "acct-1").Error) - assert.Equal(t, "Violet.eu.proxy.netbird.io", one.Domain, "a refused normalisation must leave every row as it was") -} - -// TestMigrateAgentNetworkSettingsToDomain_RefusesCaseOnlyCollision pins the -// same loud failure on the reshape: legacy rows whose identities differ only -// by case would fold onto one endpoint, and the reshape must say so rather -// than leave AutoMigrate to fail on the unique index. -func TestMigrateAgentNetworkSettingsToDomain_RefusesCaseOnlyCollision(t *testing.T) { - ctx := context.Background() - db := setupDatabase(t) - require.NoError(t, db.Migrator().DropTable(&legacyAgentNetworkSettings{})) - require.NoError(t, db.AutoMigrate(&legacyAgentNetworkSettings{})) - require.NoError(t, db.Create(&legacyAgentNetworkSettings{ - AccountID: "acct-1", Cluster: "EU.proxy.netbird.io", Subdomain: "violet", - }).Error) - require.NoError(t, db.Create(&legacyAgentNetworkSettings{ - AccountID: "acct-2", Cluster: "eu.proxy.netbird.io", Subdomain: "violet", - }).Error) - - err := migration.MigrateAgentNetworkSettingsToDomain(ctx, db) - require.Error(t, err, "legacy rows folding onto one endpoint must stop the reshape") - assert.Contains(t, err.Error(), "violet.eu.proxy.netbird.io", "the failure must name the colliding hostname") -} diff --git a/management/server/store/sql_store.go b/management/server/store/sql_store.go index dd9309ad0..08ec45395 100644 --- a/management/server/store/sql_store.go +++ b/management/server/store/sql_store.go @@ -6292,25 +6292,6 @@ func (s *SqlStore) DisconnectProxy(ctx context.Context, proxyID, sessionID strin return nil } -// DeleteProxy removes the proxy's row, but only while it still carries the -// given session: a registration withdrawing its own claim must not take out a -// newer session's row for the same proxy. A row already superseded or gone is -// not an error — the claim it would have withdrawn is no longer this session's -// to withdraw. -func (s *SqlStore) DeleteProxy(ctx context.Context, proxyID, sessionID string) error { - result := s.db. - Where("id = ? AND session_id = ?", proxyID, sessionID). - Delete(&proxy.Proxy{}) - if result.Error != nil { - log.WithContext(ctx).Errorf("failed to delete proxy %s session %s: %v", proxyID, sessionID, result.Error) - return status.Errorf(status.Internal, "failed to delete proxy") - } - if result.RowsAffected == 0 { - log.WithContext(ctx).Debugf("proxy %s session %s: no row deleted (already gone or superseded by a newer session)", proxyID, sessionID) - } - return nil -} - // GetAllProxies returns all reverse proxy instance rows. func (s *SqlStore) GetAllProxies(ctx context.Context) ([]*proxy.Proxy, error) { var proxies []*proxy.Proxy @@ -6435,13 +6416,11 @@ func (s *SqlStore) CountProxiesByAccountID(ctx context.Context, accountID string // queries. Backs the agent-network settings delete guard: settings cannot be // deleted while a proxy declares the endpoint hostname as its address. // -// The comparison folds case on both sides. Addresses are canonicalized where -// they are written now (canonicalProxyAddress on the proxy-connect path), so -// this mostly matters for a row written before that: hostnames are -// case-insensitive per RFC 4343, and on a case-sensitive collation a proxy -// stored as "GW.Example.com" would otherwise slip past the guard. This runs -// only on the settings delete path, so folding costs nothing worth indexing -// around. +// The comparison folds case on both sides: the caller passes a normalized +// (lowercase) hostname, but proxies declare their cluster address verbatim +// and Connect stores it unchanged, so on case-sensitive collations a proxy +// declaring "GW.Example.com" would otherwise slip past the guard. Hostnames +// are case-insensitive per RFC 4343; the guard must be too. func (s *SqlStore) HasActiveProxyAtClusterAddress(ctx context.Context, clusterAddress string) (bool, error) { var count int64 result := s.db. @@ -6455,39 +6434,6 @@ func (s *SqlStore) HasActiveProxyAtClusterAddress(ctx context.Context, clusterAd return count > 0, nil } -// IsClusterAddressConflicting reports whether the address is already declared -// by a proxy outside the account — a shared proxy or another account's. The -// 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. -// HasForeignAccountProxyAtHost reports whether a proxy owned by another -// account declares this host, folding case on both sides. -// -// 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 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) IsClusterAddressConflicting(ctx context.Context, clusterAddress, accountID string) (bool, error) { var count int64 result := s.db. diff --git a/management/server/store/sql_store_agentnetwork.go b/management/server/store/sql_store_agentnetwork.go index 4d3d0222f..e75e36320 100644 --- a/management/server/store/sql_store_agentnetwork.go +++ b/management/server/store/sql_store_agentnetwork.go @@ -315,32 +315,6 @@ func (s *SqlStore) GetAllAgentNetworkSettings(ctx context.Context, lockStrength return settings, nil } -// HasGatewayPinnedByOtherAccount reports whether an account other than the -// given one has its agent network gateway pinned to this host. -// -// A pin is a claim on the host, the same way a proxy row is: the pinned -// endpoint is served by whichever proxy declares that address, and an -// account-scoped proxy only ever receives its own account's mappings. A proxy -// from a different account taking the address therefore cannot serve the pin -// and silently strands it. The pin is immutable, so the account that holds it -// cannot move out of the way — the later claimant is the one to refuse. -// -// Both sides are canonical (settings normalize on write, proxy addresses -// canonicalize at connect), so the match is exact and uses the proxy_address -// index. -func (s *SqlStore) HasGatewayPinnedByOtherAccount(ctx context.Context, host, accountID string) (bool, error) { - var count int64 - result := s.db. - Model(&agentNetworkTypes.Settings{}). - Where("proxy_address = ? AND account_id != ?", host, accountID). - Count(&count) - if result.Error != nil { - log.WithContext(ctx).Errorf("failed to check agent network gateway pins by proxy address: %v", result.Error) - return false, status.Errorf(status.Internal, "check agent network gateway pins") - } - 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/sql_store_agentnetwork_gatewaypin_test.go b/management/server/store/sql_store_agentnetwork_gatewaypin_test.go deleted file mode 100644 index aa28adef0..000000000 --- a/management/server/store/sql_store_agentnetwork_gatewaypin_test.go +++ /dev/null @@ -1,73 +0,0 @@ -package store - -import ( - "context" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - - agentNetworkTypes "github.com/netbirdio/netbird/management/internals/modules/agentnetwork/types" -) - -// TestHasGatewayPinnedByOtherAccount_RealStore drives the query a proxy -// registration asks before claiming a cluster address, against a real sqlite -// store. -// -// A gateway pin is a claim on the host: it is immutable, it is served by -// whichever proxy declares that address, and an account-scoped proxy only ever -// receives its own account's mappings — so a proxy from a different account -// taking the address strands the pin. The account's own pin is the opposite -// case and must stay claimable, because pinning first and deploying the proxy -// after is the documented order. -func TestHasGatewayPinnedByOtherAccount_RealStore(t *testing.T) { - ctx := context.Background() - s, cleanup, err := NewTestStoreFromSQL(ctx, "", t.TempDir()) - require.NoError(t, err, "real sqlite test store must come up") - defer cleanup() - - const ( - pinnedHost = "gw.account1.example.com" - freeHost = "nobody.example.com" - ) - - settings := agentNetworkTypes.DefaultSettings("account1") - settings.Domain = pinnedHost - settings.ProxyAddress = pinnedHost - require.NoError(t, s.CreateAgentNetworkSettings(ctx, settings), "seeding the pin must succeed") - - t.Run("another account is refused the host", func(t *testing.T) { - pinned, err := s.HasGatewayPinnedByOtherAccount(ctx, pinnedHost, "account2") - require.NoError(t, err) - assert.True(t, pinned, "a host another account pinned its gateway to is claimed") - }) - - t.Run("the pinning account may still claim it", func(t *testing.T) { - pinned, err := s.HasGatewayPinnedByOtherAccount(ctx, pinnedHost, "account1") - require.NoError(t, err) - assert.False(t, pinned, "an account must be able to deploy the proxy for its own pin") - }) - - t.Run("an unpinned host is free", func(t *testing.T) { - pinned, err := s.HasGatewayPinnedByOtherAccount(ctx, freeHost, "account2") - require.NoError(t, err) - assert.False(t, pinned, "a host no gateway is pinned to stays claimable") - }) - - t.Run("a labeled pin claims the cluster, not just the endpoint", func(t *testing.T) { - // A labeled bootstrap hangs