diff --git a/management/internals/modules/agentnetwork/manager.go b/management/internals/modules/agentnetwork/manager.go index 46582e91d..56d8ace43 100644 --- a/management/internals/modules/agentnetwork/manager.go +++ b/management/internals/modules/agentnetwork/manager.go @@ -1030,13 +1030,24 @@ 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 is the arbiter of availability. +// 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. 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 { @@ -1051,7 +1062,7 @@ func (m *managerImpl) bootstrapSelfAddressed(ctx context.Context, settings *type } return fmt.Errorf("create agent network settings: %w", err) } - return nil + return m.confirmGatewayClusterOwnership(ctx, settings) } // validateGatewayCluster rejects a labeled bootstrap pinned to a cluster that @@ -1232,25 +1243,12 @@ func (m *managerImpl) bootstrapLabeled(ctx context.Context, settings *types.Sett return fmt.Errorf("allocate agent network endpoint for account %s: %d attempts exhausted", settings.AccountID, maxDomainAllocationAttempts) } -// confirmGatewayClusterOwnership re-asks, once the settings row is committed, -// whether another account's proxy declares the pinned cluster, and withdraws -// the row if one does. -// -// validateGatewayCluster answered that before the insert, but the two are -// separate statements: a foreign proxy can register at the host in between, -// and its own availability check — run before its row is written — would not -// have seen this pin yet either. Re-reading after the write closes that -// window from this side, and Manager.Connect does the same from the proxy's: -// both claimants write before they re-read, so of two concurrent claims at -// least one re-reads after the other has committed and backs off. Each -// statement runs autocommit, so that re-read sees every commit before it on -// sqlite, postgres and mysql alike. Both may back off, which costs the caller -// a retry; neither keeps a claim the other holds, which is the invariant. -// No lock spans the proxies and settings tables portably, and a claims table -// would be more machinery than the property needs. -// -// Only ownership is re-asked. The capability check is about what the cluster -// can do, not who holds it, and does not race a claim. +// 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 { diff --git a/management/internals/modules/agentnetwork/settings_bootstrap_test.go b/management/internals/modules/agentnetwork/settings_bootstrap_test.go index 0e7a9f6a6..ca1920a00 100644 --- a/management/internals/modules/agentnetwork/settings_bootstrap_test.go +++ b/management/internals/modules/agentnetwork/settings_bootstrap_test.go @@ -2,6 +2,7 @@ package agentnetwork import ( "context" + "errors" "runtime" "strings" "testing" @@ -433,6 +434,45 @@ func (s *claimingStore) CreateAgentNetworkSettings(ctx context.Context, settings 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, @@ -444,14 +484,7 @@ func TestCreateSettingsWithdrawsPinClaimedDuringBootstrap(t *testing.T) { const host = "shared.example.com" f := newBootstrapFixtureWith(t, func(st store.Store) store.Store { - return &claimingStore{Store: st, t: t, claim: &proxy.Proxy{ - ID: "foreign", - ClusterAddress: host, - Status: proxy.StatusConnected, - LastSeen: time.Now().UTC(), - AccountID: ptrTo("account2"), - Capabilities: proxy.Capabilities{Private: ptrTo(true)}, - }} + 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. @@ -473,6 +506,113 @@ func TestCreateSettingsWithdrawsPinClaimedDuringBootstrap(t *testing.T) { 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