mirror of
https://github.com/netbirdio/netbird.git
synced 2026-09-12 17:59:06 +02:00
[management] Keep the ingress address contract and the ownership fallback
Two follow-ups to canonicalizing the proxy address. netip accepts a zoned literal where net.ParseIP did not, so "fe80::1%eth0" started passing validation and would have been stored as a cluster key. An address scoped to one host's interface cannot name a cluster others reach, so zones are rejected, as before. Making the ownership query exact also left the bootstrap check without a fallback for rows written before canonicalization: a foreign cluster stored as "BYOP.Account2.Example.com" no longer matches the normalized address, reads as never declared, and the pin it should refuse goes through. Split the two callers instead of choosing between them. IsClusterAddressConflicting stays exact for the per-connect path that needs the index; HasProxyOutsideAccountAtHost folds case for the bootstrap, which runs once per account and is the only thing standing between it and pinning its immutable endpoint to somebody else's cluster. The settings delete guard already made that trade for the same reason. Restores the foreign-casing case that went with the exact query, and adds the zone cases to the ingress test.
This commit is contained in:
@@ -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)
|
||||
}
|
||||
|
||||
@@ -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))
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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()
|
||||
|
||||
Reference in New Issue
Block a user