[management] Decide gateway cluster ownership before the account's own view

The ownership check only ran when the account's own view of the cluster
came back empty, so an account holding any row for the host — its own
canonical proxy — skipped it entirely. A row another account left behind
under a non-canonical spelling was then never seen, and the pin the check
exists to refuse went through. That is the case worth catching, not the
one to skip: two accounts claiming one hostname is the ambiguity the
connect-time conflict check prevents going forward and cannot see for a
row written before addresses were canonicalized, and the endpoint pinned
here cannot be moved afterwards.

Ask ownership first, and narrow what counts as foreign while doing it.
The query treated a shared proxy as outside the account, which was
harmless while it ran only for a host the account had no view of — a
shared row would have given it one — but refuses the cluster most
accounts pin to once it runs first. Only a row owned by a different
account is foreign now, which is also what the name says.
This commit is contained in:
mlsmaycon
2026-09-12 09:10:55 +00:00
parent b449b31cc6
commit a403a8d275
5 changed files with 87 additions and 32 deletions
@@ -1083,24 +1083,29 @@ func (m *managerImpl) bootstrapSelfAddressed(ctx context.Context, settings *type
// all: pinning ahead of a proxy's first connection is a legitimate order — the
// dedicated path claims an address the same way, before any proxy declares it.
func (m *managerImpl) validateGatewayCluster(ctx context.Context, accountID, clusterAddr string) error {
// Ownership is decided first, before anything the account's own view can
// answer. A host another account's proxy declares is refused even when
// this account has a row for it too: two accounts claiming one hostname is
// the ambiguity the connect-time conflict check exists to prevent, and the
// endpoint pinned here cannot be moved afterwards, so the ambiguous case
// has to fail closed. Asking the account's view first would skip this
// whenever the account had any row of its own, which is exactly when a
// collision is worth catching. Shared proxies are not foreign — they are
// what most accounts pin to.
foreign, err := m.store.HasForeignAccountProxyAtHost(ctx, clusterAddr, accountID)
if err != nil {
return fmt.Errorf("check proxy cluster ownership: %w", err)
}
if foreign {
return status.Errorf(status.InvalidArgument,
"proxy cluster %s is not available to this account", clusterAddr)
}
declared, err := m.accountClusterSpellings(ctx, accountID, clusterAddr)
if err != nil {
return err
}
if len(declared) == 0 {
// Not in the account's view. A shared cluster would have been in it,
// so a proxy row elsewhere for this address can only be another
// account's BYOP cluster: its proxies filter foreign mappings out on
// delivery, making the pin dead on arrival.
foreign, err := m.store.HasProxyOutsideAccountAtHost(ctx, clusterAddr, accountID)
if err != nil {
return fmt.Errorf("check proxy cluster ownership: %w", err)
}
if foreign {
return status.Errorf(status.InvalidArgument,
"proxy cluster %s is not available to this account", clusterAddr)
}
// No proxy has ever declared this address: an address-first pin.
return nil
}
@@ -374,6 +374,51 @@ func TestCreateSettingsRejectsForeignCluster(t *testing.T) {
}
}
// TestCreateSettingsRejectsHostAnotherAccountClaims pins that ownership is
// decided before the account's own view, not after it.
//
// Two accounts holding rows for one hostname is the ambiguity the connect-time
// conflict check prevents going forward and cannot see for a row written
// before addresses were canonicalized. Deciding on the account's own view
// first would skip the ownership question exactly when the account has a row
// of its own — which is when a collision is worth catching — and the endpoint
// pinned here cannot be moved afterwards.
func TestCreateSettingsRejectsHostAnotherAccountClaims(t *testing.T) {
ctx := context.Background()
f := newBootstrapFixture(t)
// account1's own row is canonical and perfectly serviceable on its own.
f.seedProxy(t, "own", "account1", "shared.example.com", ptrTo(true))
// account2 holds a legacy, non-canonical spelling of the same host.
f.seedProxy(t, "foreign", "account2", "Shared.Example.com", ptrTo(true))
f.expectPermission("account1", "user1", modules.AgentNetworkSettings, operations.Create, true)
_, err := f.createSettings(ctx, "account1", "user1", "shared.example.com", "")
require.Error(t, err, "a host another account also claims must be refused")
var sErr *status.Error
require.ErrorAs(t, err, &sErr)
assert.Equal(t, status.InvalidArgument, sErr.Type())
assert.Contains(t, err.Error(), "not available to this account")
_, err = f.store.GetAgentNetworkSettings(ctx, store.LockingStrengthNone, "account1")
assert.Error(t, err, "no row may be left behind by a rejected bootstrap")
}
// TestCreateSettingsAcceptsSharedClusterAlongsideOwnProxy pins the other side
// of that ordering: a shared (NetBird-operated) proxy is not foreign, so
// asking the ownership question first must not refuse the cluster most
// accounts pin to.
func TestCreateSettingsAcceptsSharedClusterAlongsideOwnProxy(t *testing.T) {
ctx := context.Background()
f := newBootstrapFixture(t)
f.seedProxy(t, "shared", "", "eu.proxy.example.com", ptrTo(true))
f.seedProxy(t, "own", "account1", "eu.proxy.example.com", ptrTo(true))
f.expectPermission("account1", "user1", modules.AgentNetworkSettings, operations.Create, true)
created, err := f.createSettings(ctx, "account1", "user1", "eu.proxy.example.com", "")
require.NoError(t, err, "a shared cluster must stay pinnable")
assert.Equal(t, "eu.proxy.example.com", created.ProxyAddress)
}
// TestCreateSettingsAcceptsOwnPrivateCluster pins the BYOP happy path: the
// account's own cluster with a connected embedded proxy is a valid pin.
func TestCreateSettingsAcceptsOwnPrivateCluster(t *testing.T) {