diff --git a/management/internals/modules/agentnetwork/manager.go b/management/internals/modules/agentnetwork/manager.go index 7a1e3b638..c7d436db5 100644 --- a/management/internals/modules/agentnetwork/manager.go +++ b/management/internals/modules/agentnetwork/manager.go @@ -899,7 +899,7 @@ func (m *managerImpl) validateGatewayCluster(ctx context.Context, accountID, clu // 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.IsClusterAddressConflicting(ctx, clusterAddr, accountID) + foreign, err := m.store.HasProxyOutsideAccountAtHost(ctx, clusterAddr, accountID) if err != nil { return fmt.Errorf("check proxy cluster ownership: %w", err) } diff --git a/management/internals/modules/agentnetwork/settings_bootstrap_test.go b/management/internals/modules/agentnetwork/settings_bootstrap_test.go index 96db03539..35a8ba5c1 100644 --- a/management/internals/modules/agentnetwork/settings_bootstrap_test.go +++ b/management/internals/modules/agentnetwork/settings_bootstrap_test.go @@ -402,6 +402,19 @@ func TestCreateSettingsMatchesClusterCasing(t *testing.T) { 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)) diff --git a/management/internals/shared/grpc/proxy.go b/management/internals/shared/grpc/proxy.go index e5dbf8c7c..027b1c3ab 100644 --- a/management/internals/shared/grpc/proxy.go +++ b/management/internals/shared/grpc/proxy.go @@ -888,7 +888,13 @@ func canonicalProxyAddress(addr string) (string, bool) { if addr == "" { return "", false } + // A zoned literal like "fe80::1%eth0" is scoped to one host's interface, + // so it cannot identify a cluster others reach; net.ParseIP rejected it + // before and netip must not start accepting it. if ip, err := netip.ParseAddr(addr); err == nil { + if ip.Zone() != "" { + return "", false + } return ip.String(), true } // Folded before punycode conversion, not just after: idna maps the ASCII diff --git a/management/internals/shared/grpc/proxy_address_test.go b/management/internals/shared/grpc/proxy_address_test.go index d26f06980..eb6ec1f18 100644 --- a/management/internals/shared/grpc/proxy_address_test.go +++ b/management/internals/shared/grpc/proxy_address_test.go @@ -51,6 +51,10 @@ func TestCanonicalProxyAddress(t *testing.T) { {name: "mixed case ipv6 canonicalised", addr: "2001:DB8::1", canonical: "2001:db8::1", ok: true}, {name: "empty string rejected", addr: "", ok: false}, {name: "space rejected", addr: "eu proxy.example.com", ok: false}, + // Scoped to one host's interface, so it cannot name a cluster others + // reach; net.ParseIP rejected these and netip must not accept them. + {name: "zoned ipv6 rejected", addr: "fe80::1%eth0", ok: false}, + {name: "unzoned link-local ipv6 accepted", addr: "fe80::1", canonical: "fe80::1", ok: true}, } for _, tt := range tests { diff --git a/management/server/store/sql_store.go b/management/server/store/sql_store.go index e96c5fe20..71f73b0ee 100644 --- a/management/server/store/sql_store.go +++ b/management/server/store/sql_store.go @@ -6374,11 +6374,13 @@ func (s *SqlStore) CountProxiesByAccountID(ctx context.Context, accountID string // queries. Backs the agent-network settings delete guard: settings cannot be // deleted while a proxy declares the endpoint hostname as its address. // -// The comparison folds case on both sides: the caller passes a normalized -// (lowercase) hostname, but proxies declare their cluster address verbatim -// and Connect stores it unchanged, so on case-sensitive collations a proxy -// declaring "GW.Example.com" would otherwise slip past the guard. Hostnames -// are case-insensitive per RFC 4343; the guard must be too. +// The comparison folds case on both sides. Addresses are canonicalized where +// they are written now (canonicalProxyAddress on the proxy-connect path), so +// this mostly matters for a row written before that: hostnames are +// case-insensitive per RFC 4343, and on a case-sensitive collation a proxy +// stored as "GW.Example.com" would otherwise slip past the guard. This runs +// only on the settings delete path, so folding costs nothing worth indexing +// around. func (s *SqlStore) HasActiveProxyAtClusterAddress(ctx context.Context, clusterAddress string) (bool, error) { var count int64 result := s.db. @@ -6397,6 +6399,29 @@ func (s *SqlStore) HasActiveProxyAtClusterAddress(ctx context.Context, clusterAd // match is exact, and stays exact so it uses the cluster_address index: // addresses are canonicalised where they are written (canonicalProxyAddress on // the proxy-connect path), so one host has one spelling in this column. +// HasProxyOutsideAccountAtHost reports the same thing as +// IsClusterAddressConflicting, folding case on both sides. +// +// The two exist separately because their callers differ in cost and in what +// they can assume. IsClusterAddressConflicting runs on every account-scoped +// proxy connect, where both sides are canonical and the match must stay exact +// to use the cluster_address index. This one runs once per account, when an +// agent network bootstraps, and is the only thing standing between that +// account and pinning its immutable endpoint to a cluster somebody else runs +// — so it also has to see a row written before addresses were canonicalized, +// which is worth a scan on a path taken once. +func (s *SqlStore) HasProxyOutsideAccountAtHost(ctx context.Context, host, accountID string) (bool, error) { + var count int64 + result := s.db. + Model(&proxy.Proxy{}). + Where("LOWER(cluster_address) = LOWER(?) AND (account_id IS NULL OR account_id != ?)", host, accountID). + Count(&count) + if result.Error != nil { + return false, status.Errorf(status.Internal, "check proxy host ownership: %v", result.Error) + } + return count > 0, nil +} + func (s *SqlStore) IsClusterAddressConflicting(ctx context.Context, clusterAddress, accountID string) (bool, error) { var count int64 result := s.db. diff --git a/management/server/store/store.go b/management/server/store/store.go index 7daeb28a9..1e5614b2c 100644 --- a/management/server/store/store.go +++ b/management/server/store/store.go @@ -337,6 +337,7 @@ type Store interface { CountProxiesByAccountID(ctx context.Context, accountID string) (int64, error) IsClusterAddressConflicting(ctx context.Context, clusterAddress, accountID string) (bool, error) HasActiveProxyAtClusterAddress(ctx context.Context, clusterAddress string) (bool, error) + HasProxyOutsideAccountAtHost(ctx context.Context, host, accountID string) (bool, error) DeleteAccountCluster(ctx context.Context, clusterAddress, accountID string) error GetCustomDomainsCounts(ctx context.Context) (total int64, validated int64, err error) diff --git a/management/server/store/store_mock.go b/management/server/store/store_mock.go index 70acb9f58..70fffa0c2 100644 --- a/management/server/store/store_mock.go +++ b/management/server/store/store_mock.go @@ -3020,6 +3020,21 @@ func (mr *MockStoreMockRecorder) HasActiveProxyAtClusterAddress(ctx, clusterAddr return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "HasActiveProxyAtClusterAddress", reflect.TypeOf((*MockStore)(nil).HasActiveProxyAtClusterAddress), ctx, clusterAddress) } +// HasProxyOutsideAccountAtHost mocks base method. +func (m *MockStore) HasProxyOutsideAccountAtHost(ctx context.Context, host, accountID string) (bool, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "HasProxyOutsideAccountAtHost", ctx, host, accountID) + ret0, _ := ret[0].(bool) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// HasProxyOutsideAccountAtHost indicates an expected call of HasProxyOutsideAccountAtHost. +func (mr *MockStoreMockRecorder) HasProxyOutsideAccountAtHost(ctx, host, accountID any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "HasProxyOutsideAccountAtHost", reflect.TypeOf((*MockStore)(nil).HasProxyOutsideAccountAtHost), ctx, host, accountID) +} + // IncrementAgentNetworkConsumption mocks base method. func (m *MockStore) IncrementAgentNetworkConsumption(ctx context.Context, accountID string, kind types.ConsumptionDimension, dimID string, windowSeconds int64, windowStart time.Time, tokensIn, tokensOut int64, costUSD float64) error { m.ctrl.T.Helper()