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)