From d911649158a7a3da630324667f741560833ec16a Mon Sep 17 00:00:00 2001 From: mlsmaycon Date: Thu, 3 Sep 2026 07:21:44 +0000 Subject: [PATCH] [management] Compare agent network cluster addresses case-insensitively MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Proxies store their cluster address as they declared it, while proxy_address is normalised lowercase before validation. The capability and ownership lookups match cluster_address exactly, so feeding them the normalised form asked about a spelling the store may never have seen: a private cluster declared with capitals came back unproven and was refused, and — worse — another account's cluster declared with capitals came back as "never declared" and let the pin through. Compare identity on the normalised form but keep the stored spellings, and read the capability under each of them, any-true, the same way it aggregates over a cluster's proxies. Ownership gets the same treatment at the source: hostnames are case-insensitive, so two spellings of one host are one cluster and must conflict rather than being claimable side by side, which also closes the same gap in the proxy-registration availability check that shares the query. The e2e's wait for a stopped cluster to leave the active list now allows for the active window rather than 90s: a proxy that dies without closing its stream is only dropped once its last heartbeat ages past proxyActiveThreshold, so the old budget could fail the test on the slow path alone. --- .../settings_cluster_validation_test.go | 8 ++- .../internals/modules/agentnetwork/manager.go | 49 ++++++++++++------- .../agentnetwork/settings_bootstrap_test.go | 45 +++++++++++++++++ management/server/store/sql_store.go | 7 ++- 4 files changed, 90 insertions(+), 19 deletions(-) diff --git a/e2e/agentnetwork/settings_cluster_validation_test.go b/e2e/agentnetwork/settings_cluster_validation_test.go index af0382ad5..52b1084ad 100644 --- a/e2e/agentnetwork/settings_cluster_validation_test.go +++ b/e2e/agentnetwork/settings_cluster_validation_test.go @@ -143,10 +143,16 @@ func waitClusterPrivate(ctx context.Context, t *testing.T, c *harness.Combined, // offered, i.e. management sees no live proxy in it. The free-domain list is // built from the active clusters, so this is how a proxy going away becomes // observable — while the cluster's rows, and so its capability record, remain. +// +// The budget has to clear the active window, not just the disconnect: a proxy +// that closes its stream cleanly is marked disconnected at once, but one that +// dies without that is only dropped when its last heartbeat ages past +// proxyActiveThreshold (2 minutes), so a 90s deadline could fail the test on +// the slow path alone. func waitClusterAbsent(ctx context.Context, t *testing.T, c *harness.Combined, clusterAddr string) { t.Helper() - deadline := time.Now().Add(90 * time.Second) + deadline := time.Now().Add(3 * time.Minute) var last string for time.Now().Before(deadline) { domains, err := c.API().ReverseProxyDomains.List(ctx) diff --git a/management/internals/modules/agentnetwork/manager.go b/management/internals/modules/agentnetwork/manager.go index 7954cb46e..47d9551d0 100644 --- a/management/internals/modules/agentnetwork/manager.go +++ b/management/internals/modules/agentnetwork/manager.go @@ -889,12 +889,12 @@ 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 { - known, err := m.accountKnowsCluster(ctx, accountID, clusterAddr) + declared, err := m.accountClusterSpellings(ctx, accountID, clusterAddr) if err != nil { return err } - if !known { + 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 @@ -916,26 +916,41 @@ func (m *managerImpl) validateGatewayCluster(ctx context.Context, accountID, clu // unreported capability (nothing live in the cluster, or proxies predating // capability reporting) fail here: unusable and unproven are the same // answer for a decision that cannot be revisited later. - if private := m.store.GetClusterSupportsPrivate(ctx, clusterAddr); private == nil || !*private { - return status.Errorf(status.InvalidArgument, - "proxy cluster %s cannot serve the agent network gateway: the gateway is reachable only from connected peers, "+ - "which needs at least one connected embedded proxy (netbird proxy) in the cluster", clusterAddr) + // + // The capability is read per declared spelling and taken as any-true, the + // same way it aggregates over a cluster's proxies: the store matches + // cluster_address exactly, so a host two proxies spelled differently must + // not come back unproven just because it was asked about under one of them. + for _, address := range declared { + if private := m.store.GetClusterSupportsPrivate(ctx, address); private != nil && *private { + return nil + } } - return nil + + return status.Errorf(status.InvalidArgument, + "proxy cluster %s cannot serve the agent network gateway: the gateway is reachable only from connected peers, "+ + "which needs at least one connected embedded proxy (netbird proxy) in the cluster", clusterAddr) } -// accountKnowsCluster reports whether clusterAddr is a proxy cluster in the -// account's view — one of its own (BYOP) clusters or a shared one. The cluster -// listing is not gated on heartbeats, so this answer does not change while a -// cluster's proxies are merely offline. Addresses are stored as the proxy -// declared them, so both sides are normalised: hostnames are case-insensitive -// and the pin must not be sidesteppable by casing. -func (m *managerImpl) accountKnowsCluster(ctx context.Context, accountID, clusterAddr string) (bool, error) { +// accountClusterSpellings returns every proxy cluster address in the account's +// view — its own (BYOP) clusters plus the shared ones — that names the same +// host as clusterAddr. Empty means management holds no proxy row for that host +// in this account's view. +// +// Addresses are stored as the proxy declared them and hostnames are +// case-insensitive, so identity is compared on the normalised form while the +// stored spellings are what comes back: the capability lookups match +// cluster_address exactly, and handing one a normalised address it never +// stored would silently find nothing. 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 { - return false, fmt.Errorf("list proxy clusters: %w", err) + return nil, fmt.Errorf("list proxy clusters: %w", err) } + var spellings []string for _, cluster := range clusters { normalized, err := types.NormalizeHostname(cluster.Address) if err != nil { @@ -945,10 +960,10 @@ func (m *managerImpl) accountKnowsCluster(ctx context.Context, accountID, cluste continue } if normalized == clusterAddr { - return true, nil + spellings = append(spellings, cluster.Address) } } - return false, nil + return spellings, nil } // bootstrapLabeled allocates a labeled endpoint one label beneath the given diff --git a/management/internals/modules/agentnetwork/settings_bootstrap_test.go b/management/internals/modules/agentnetwork/settings_bootstrap_test.go index 9b6c29d80..b221dfbf0 100644 --- a/management/internals/modules/agentnetwork/settings_bootstrap_test.go +++ b/management/internals/modules/agentnetwork/settings_bootstrap_test.go @@ -379,3 +379,48 @@ func TestCreateSettingsAcceptsOwnPrivateCluster(t *testing.T) { require.NoError(t, err, "the account's own private cluster must be accepted") assert.Equal(t, "byop.account1.example.com", created.ProxyAddress) } + +// TestCreateSettingsMatchesClusterCasing pins hostname case-insensitivity +// across the whole check. Proxies declare their cluster address verbatim while +// proxy_address is normalised lowercase, so a cluster spelled with capitals is +// the same cluster: its own private capability must still be found (an exact +// lookup under the normalised spelling finds nothing and would refuse a +// perfectly good cluster), and another account's must still be recognised as +// theirs (a lookup that misses would read as "never declared" and let the pin +// through). +func TestCreateSettingsMatchesClusterCasing(t *testing.T) { + ctx := context.Background() + + t.Run("own private cluster is found", func(t *testing.T) { + f := newBootstrapFixture(t) + f.seedProxy(t, "proxy1", "", "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 private cluster declared with capitals must still be accepted") + 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)) + f.expectPermission("account1", "user1", modules.AgentNetworkSettings, operations.Create, true) + + _, err := f.createSettings(ctx, "account1", "user1", "central.example.com", "") + require.Error(t, err, "casing must not become a way past the capability check") + assert.Contains(t, err.Error(), "embedded proxy") + }) +} diff --git a/management/server/store/sql_store.go b/management/server/store/sql_store.go index 6337ebf1a..cae2ab0ce 100644 --- a/management/server/store/sql_store.go +++ b/management/server/store/sql_store.go @@ -6392,11 +6392,16 @@ 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 +// comparison is case-insensitive: cluster addresses are hostnames, stored as +// the proxy declared them, so two spellings of one host are one cluster and +// must conflict rather than being claimable side by side. func (s *SqlStore) IsClusterAddressConflicting(ctx context.Context, clusterAddress, accountID string) (bool, error) { var count int64 result := s.db. Model(&proxy.Proxy{}). - Where("cluster_address = ? AND (account_id IS NULL OR account_id != ?)", clusterAddress, accountID). + Where("LOWER(cluster_address) = LOWER(?) AND (account_id IS NULL OR account_id != ?)", clusterAddress, accountID). Count(&count) if result.Error != nil { return false, status.Errorf(status.Internal, "check cluster address conflict: %v", result.Error)