[management] Withdraw a cluster address claim that is lost after the write

A cluster address is claimed two ways: an account-scoped proxy row, and an
agent network gateway pin on the address. Each side checked the other
before writing — IsClusterAddressAvailable before SaveProxy,
HasForeignAccountProxyAtHost before the settings insert — but check and
write are separate autocommit statements, so two concurrent claimants could
each pass their check and both commit, leaving a pin no proxy will ever
serve next to the proxy row that displaces it.

Both sides now re-read after they write. Manager.Connect re-asks
availability once the proxy row is committed and, if the address is no
longer free or the answer is inconclusive, deletes its own row and returns
ErrClusterAddressUnavailable, which the connect path reports as
AlreadyExists exactly as the pre-write check would have. bootstrapLabeled
re-asks ownership once the settings row is committed and withdraws the pin
on the same terms. Because both write before they re-read, of two
concurrent claimants at least one re-reads after the other has committed
and backs off — on sqlite, postgres and mysql alike, since each statement
sees every commit before it. 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, so the re-read is the
whole mechanism. DeleteProxy is session-guarded like DisconnectProxy, so a
stale session withdrawing itself cannot take out a newer session's row.

Reported by CodeRabbit on #7402 (CWE-362).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Sa3DsBDP3VciAi4PPG17L6

management/internals/modules/reverseproxy/proxy/manager/manager.go

@@ -2,6 +2,7 @@ package manager
 
 import (
 	"context"
+	"fmt"
 	"time"
 
 	log "github.com/sirupsen/logrus"
@@ -14,6 +15,7 @@ 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)
@@ -74,6 +76,12 @@ 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,
@@ -84,6 +92,43 @@ func (m *Manager) Connect(ctx context.Context, proxyID, sessionID, clusterAddres
 	return p, nil
 }
 
+// confirmClusterAddressClaim re-asks, once the proxy's row is committed,
+// whether the account may hold the address, and withdraws the row if not.
+//
+// The connect path checks IsClusterAddressAvailable before Connect, but that
+// read and the write here are separate statements: another claim — a foreign
+// proxy row, or another account's agent network gateway pin — can land in
+// between, and its own check would not have seen this row yet either.
+// Re-reading after the write closes that window from this side, and the
+// gateway bootstrap does the same from its side: 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 a reconnect; 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.
+//
+// The row is withdrawn on an inconclusive re-read too: a claim that cannot be
+// confirmed must not stand, and the proxy reconnects on its own.
+func (m *Manager) confirmClusterAddressClaim(ctx context.Context, p *proxy.Proxy, accountID string) error {
+	available, err := m.IsClusterAddressAvailable(ctx, p.ClusterAddress, accountID)
+	if err == nil && 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)
+	}
+	if err != nil {
+		return fmt.Errorf("confirm claim on cluster address %s: %w", p.ClusterAddress, err)
+	}
+	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 {

management/internals/modules/reverseproxy/proxy/manager/manager_test.go

@@ -26,6 +26,7 @@ type mockStore struct {
 	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 {
@@ -40,6 +41,12 @@ 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)
@@ -407,3 +414,113 @@ func TestIsClusterAddressAvailableSurfacesGatewayPinError(t *testing.T) {
 	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_WithdrawsClaimWhenRecheckFails pins fail-closed: a re-read that
+// cannot answer leaves the row withdrawn and the connect refused, rather than
+// letting a claim stand that was never confirmed. The error is the store's,
+// not ErrClusterAddressUnavailable — nothing established that the address is
+// taken.
+func TestConnect_WithdrawsClaimWhenRecheckFails(t *testing.T) {
+	accountID := "acc-1"
+	var withdrawn int
+	s := &mockStore{
+		hasGatewayPinnedByOtherAccountFunc: func(_ context.Context, _, _ string) (bool, error) {
+			return false, errors.New("db unavailable")
+		},
+		deleteProxyFunc: func(_ context.Context, _, _ string) error { withdrawn++; 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, 1, withdrawn, "an unconfirmed claim must be withdrawn")
+}
+
+// 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)
+}

management/internals/modules/reverseproxy/proxy/proxy.go

@@ -1,6 +1,7 @@
 package proxy
 
 import (
+	"errors"
 	"time"
 )
 
@@ -9,6 +10,14 @@ const (
 	StatusDisconnected = "disconnected"
 )
 
+// ErrClusterAddressUnavailable is returned by Manager.Connect when the
+// cluster address turned out to be claimed by someone else once the proxy's
+// own row was written — a conflicting proxy row or another account's agent
+// network gateway pin that landed between the availability check and the
+// write. The proxy's row has been withdrawn by then; the caller reports the
+// address as taken, exactly as if the pre-write check had caught it.
+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 {