diff --git a/management/internals/modules/agentnetwork/manager.go b/management/internals/modules/agentnetwork/manager.go index 47d9551d0..7a1e3b638 100644 --- a/management/internals/modules/agentnetwork/manager.go +++ b/management/internals/modules/agentnetwork/manager.go @@ -937,13 +937,16 @@ func (m *managerImpl) validateGatewayCluster(ctx context.Context, accountID, clu // 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. +// Addresses are canonicalised where they are written (canonicalProxyAddress on +// the proxy-connect path), so a stored spelling normally is the normalised +// form. Identity is still compared on the normalised form rather than +// byte-equal, which costs nothing here — this is an in-memory pass over the +// account's clusters, not a query — and covers a row written before that +// landed. What comes back is the stored spelling either way, because the +// capability lookup matches cluster_address exactly and would silently find +// nothing under a spelling the store never held. 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 { diff --git a/management/internals/modules/agentnetwork/settings_bootstrap_test.go b/management/internals/modules/agentnetwork/settings_bootstrap_test.go index b221dfbf0..96db03539 100644 --- a/management/internals/modules/agentnetwork/settings_bootstrap_test.go +++ b/management/internals/modules/agentnetwork/settings_bootstrap_test.go @@ -380,14 +380,15 @@ func TestCreateSettingsAcceptsOwnPrivateCluster(t *testing.T) { 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). +// TestCreateSettingsMatchesClusterCasing pins that a cluster spelled with +// capitals in the store is still recognised as the same cluster the normalised +// proxy_address names. Addresses are canonicalised where they are written +// (canonicalProxyAddress on the proxy-connect path), so this is the belt to +// that braces: it covers a row written before that landed, and any future +// writer that skips it. The comparison is in memory over the account's cluster +// list, so it costs nothing at the query — the capability lookup is still +// asked under the spelling the store actually holds, which is what an exact, +// indexed match needs. func TestCreateSettingsMatchesClusterCasing(t *testing.T) { ctx := context.Background() @@ -401,19 +402,6 @@ 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 cee50b270..e5dbf8c7c 100644 --- a/management/internals/shared/grpc/proxy.go +++ b/management/internals/shared/grpc/proxy.go @@ -13,6 +13,7 @@ import ( "math" "net" "net/http" + "net/netip" "net/url" "os" "strconv" @@ -495,7 +496,8 @@ func (s *ProxyServiceServer) validateProxyConnect(proxyID, address string, ctx c if proxyID == "" { return proxyConnectParams{}, status.Errorf(codes.InvalidArgument, "proxy_id is required") } - if !isProxyAddressValid(address) { + address, ok := canonicalProxyAddress(address) + if !ok { return proxyConnectParams{}, status.Errorf(codes.InvalidArgument, "proxy address is invalid") } @@ -872,16 +874,38 @@ func (s *ProxyServiceServer) snapshotServiceMappings(ctx context.Context, conn * return mappings, nil } +// canonicalProxyAddress validates a proxy address (domain name or IP address) +// and returns the form the store keeps. +// +// cluster_address is the key every capability, ownership and routing lookup +// matches on, and this is the only path that writes it, so the address is +// canonicalised once here rather than pushing case-insensitivity into each of +// those queries: hostnames are case-insensitive and may be unicode, so they +// fold to lowercase punycode (what domain.ValidateDomains already computes and +// this used to throw away), and an IP literal goes through netip so a mixed +// case IPv6 address does not become a second key for the same host. +func canonicalProxyAddress(addr string) (string, bool) { + if addr == "" { + return "", false + } + if ip, err := netip.ParseAddr(addr); err == nil { + return ip.String(), true + } + // Folded before punycode conversion, not just after: idna maps the ASCII + // output to lowercase but does not case-fold the unicode input, so + // "PRÖXY.example.com" and "pröxy.example.com" would otherwise encode to + // two different labels for one host. + canonical, err := domain.ValidateDomains([]string{strings.ToLower(addr)}) + if err != nil || len(canonical) != 1 { + return "", false + } + return string(canonical[0]), true +} + // isProxyAddressValid validates a proxy address (domain name or IP address) func isProxyAddressValid(addr string) bool { - if addr == "" { - return false - } - if net.ParseIP(addr) != nil { - return true - } - _, err := domain.ValidateDomains([]string{addr}) - return err == nil + _, ok := canonicalProxyAddress(addr) + return ok } // isStreamClosed returns true for errors that indicate normal stream diff --git a/management/internals/shared/grpc/proxy_address_test.go b/management/internals/shared/grpc/proxy_address_test.go index 824a57226..d26f06980 100644 --- a/management/internals/shared/grpc/proxy_address_test.go +++ b/management/internals/shared/grpc/proxy_address_test.go @@ -27,3 +27,37 @@ func TestIsProxyAddressValid(t *testing.T) { }) } } + +// TestCanonicalProxyAddress pins the canonical form the store keeps. Every +// capability, ownership and routing lookup matches cluster_address exactly, so +// one host must have exactly one spelling in that column — which is what lets +// those queries stay exact (and keep using the index) instead of folding case +// per query. +func TestCanonicalProxyAddress(t *testing.T) { + tests := []struct { + name string + addr string + canonical string + ok bool + }{ + {name: "lowercase domain unchanged", addr: "eu.proxy.netbird.io", canonical: "eu.proxy.netbird.io", ok: true}, + {name: "mixed case domain folded", addr: "EU.Proxy.NetBird.io", canonical: "eu.proxy.netbird.io", ok: true}, + {name: "uppercase domain folded", addr: "BYOP.PROXY.EXAMPLE.COM", canonical: "byop.proxy.example.com", ok: true}, + {name: "unicode domain punycoded", addr: "pröxy.example.com", canonical: "xn--prxy-6qa.example.com", ok: true}, + // Same host, and idna alone would encode the two cases to different + // labels, so this is the one that proves the fold happens first. + {name: "mixed case unicode folds to the same label", addr: "PRÖXY.example.com", canonical: "xn--prxy-6qa.example.com", ok: true}, + {name: "ipv4 unchanged", addr: "203.0.113.10", canonical: "203.0.113.10", ok: true}, + {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}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + canonical, ok := canonicalProxyAddress(tt.addr) + assert.Equal(t, tt.ok, ok) + assert.Equal(t, tt.canonical, canonical) + }) + } +} diff --git a/management/server/store/sql_store.go b/management/server/store/sql_store.go index cae2ab0ce..e96c5fe20 100644 --- a/management/server/store/sql_store.go +++ b/management/server/store/sql_store.go @@ -6394,14 +6394,14 @@ func (s *SqlStore) HasActiveProxyAtClusterAddress(ctx context.Context, clusterAd // 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. +// 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. func (s *SqlStore) IsClusterAddressConflicting(ctx context.Context, clusterAddress, 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 != ?)", clusterAddress, accountID). + Where("cluster_address = ? 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)