From 61e1742885ff19b4e34cacfbac448d0bc42244fd Mon Sep 17 00:00:00 2001 From: mlsmaycon Date: Thu, 3 Sep 2026 10:19:04 +0000 Subject: [PATCH] [management] Canonicalize the proxy cluster address where it is stored MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The proxy-connect path already computes the canonical form of the address a proxy declares — ValidateDomains returns lowercase punycode — and then throws it away, storing the string as declared. cluster_address is the key every capability, ownership and routing lookup matches on, so one host could sit in that column under two spellings, and the previous commit compensated with LOWER() in the ownership query, which gives up the cluster_address index on a query that runs for every account-scoped proxy connect. Keep the canonical form instead. Connect is the only writer of the column (heartbeats touch last_seen and status), and proxy rows are session state rebuilt on every connect rather than durable config, so the column converges without a migration and the lookups can stay exact and indexed. Folding happens before punycode conversion, not after: idna lowercases the ASCII it produces but does not case-fold the unicode it consumes, so PRÖXY.example.com and pröxy.example.com would otherwise encode to two different labels for one host. The agent network check keeps comparing normalised forms in memory, which costs nothing there — it is a pass over the account's cluster list, not a query — and covers rows written before this landed. --- .../internals/modules/agentnetwork/manager.go | 17 ++++---- .../agentnetwork/settings_bootstrap_test.go | 30 ++++--------- management/internals/shared/grpc/proxy.go | 42 +++++++++++++++---- .../shared/grpc/proxy_address_test.go | 34 +++++++++++++++ management/server/store/sql_store.go | 8 ++-- 5 files changed, 90 insertions(+), 41 deletions(-) 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)