[management] Ask ownership for self-addressed pins too

A self-addressed bootstrap stores the hostname as proxy_address, which is exactly what a proxy registration is refused on when another account holds it there. It asked nobody whether it could: the domain unique index arbitrated between pins, and a foreign proxy already declaring the host was never consulted, before or after the insert. Any account could therefore pin an endpoint onto a host another account's proxy serves — owning nothing — and lock that proxy out on its next reconnect, and a proxy racing such a pin could end with both claims standing, since only the labeled path re-read ownership after its write. The self-addressed path now asks HasForeignAccountProxyAtHost before the insert and confirmGatewayClusterOwnership after it, the same as the labeled one. Address-first stays intact: only a row owned by a different account refuses, so pinning ahead of any proxy, or onto the account's own, is unchanged. Also pins the bootstrap side's failure paths — an ownership re-read that cannot answer leaves no pin behind and surfaces the store's error, and a withdrawal that fails still reports the claim as lost — and shortens the helper's comment to point at the shared argument on proxy.ErrClusterAddressUnavailable rather than restate it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Sa3DsBDP3VciAi4PPG17L6
2026-09-12 17:59:06 +02:00 · 2026-09-12 13:36:35 +00:00
parent 68da6bf3aa
commit e6c69f674d
2 changed files with 167 additions and 29 deletions
@@ -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 {
@@ -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