mirror of
https://github.com/netbirdio/netbird.git
synced 2026-09-12 17:59:06 +02:00
[management] Refuse a proxy claiming a host another account's gateway pinned
The bootstrap check refused pinning onto a cluster another account runs, but nothing held that decision afterwards: a proxy registration consulted the proxies table alone, so the same host could be claimed by another account a moment later, or a week later, and the pin it stranded was immutable. Validating only at bootstrap meant the check was true when it ran and not after. Make the claim symmetric. IsClusterAddressAvailable now treats a gateway pin as what it is — a claim on the host, served by whichever proxy declares that address — and refuses a proxy from a different account, since an account-scoped proxy never receives another account's mappings and so cannot serve the pin it would displace. Both claims are checked in one place so a caller cannot consult one and forget the other. An account claiming the address its own gateway is pinned to is the documented order, not a conflict: pin first, deploy the proxy after. Shared (NetBird-operated) proxies register without an account and are unaffected; an account-scoped proxy reaching for a shared cluster address that accounts are pinned to was already refused by the proxy-row conflict and now stays refused even if those rows are momentarily gone. Whichever of the two lands first now wins and the second is refused, which is what the bootstrap check could not do on its own. A genuinely concurrent pair can still pass both checks — closing that needs an invariant spanning the proxies and settings tables, not a wider read.
This commit is contained in:
@@ -26,6 +26,7 @@ type store interface {
|
|||||||
GetProxyByAccountID(ctx context.Context, accountID string) (*proxy.Proxy, error)
|
GetProxyByAccountID(ctx context.Context, accountID string) (*proxy.Proxy, error)
|
||||||
CountProxiesByAccountID(ctx context.Context, accountID string) (int64, error)
|
CountProxiesByAccountID(ctx context.Context, accountID string) (int64, error)
|
||||||
IsClusterAddressConflicting(ctx context.Context, clusterAddress, accountID string) (bool, error)
|
IsClusterAddressConflicting(ctx context.Context, clusterAddress, accountID string) (bool, error)
|
||||||
|
HasGatewayPinnedByOtherAccount(ctx context.Context, host, accountID string) (bool, error)
|
||||||
DeleteAccountCluster(ctx context.Context, clusterAddress, accountID string) error
|
DeleteAccountCluster(ctx context.Context, clusterAddress, accountID string) error
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -169,12 +170,36 @@ func (m *Manager) CountAccountProxies(ctx context.Context, accountID string) (in
|
|||||||
return m.store.CountProxiesByAccountID(ctx, accountID)
|
return m.store.CountProxiesByAccountID(ctx, accountID)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// IsClusterAddressAvailable reports whether the account may claim this cluster
|
||||||
|
// address.
|
||||||
|
//
|
||||||
|
// Two kinds of claim make an address unavailable, and both are checked here so
|
||||||
|
// that no caller can consult one and forget the other. A proxy row is the
|
||||||
|
// obvious one. An agent network gateway pinned to the address by another
|
||||||
|
// account is the second: that pin is immutable and is served by whichever
|
||||||
|
// proxy declares the address, so letting a proxy from a different account take
|
||||||
|
// it strands the pin — an account-scoped proxy never receives another
|
||||||
|
// account's mappings. An account claiming an address its own gateway is pinned
|
||||||
|
// to is the intended order, not a conflict: pin first, deploy the proxy after.
|
||||||
func (m *Manager) IsClusterAddressAvailable(ctx context.Context, clusterAddress, accountID string) (bool, error) {
|
func (m *Manager) IsClusterAddressAvailable(ctx context.Context, clusterAddress, accountID string) (bool, error) {
|
||||||
conflicting, err := m.store.IsClusterAddressConflicting(ctx, clusterAddress, accountID)
|
conflicting, err := m.store.IsClusterAddressConflicting(ctx, clusterAddress, accountID)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return false, err
|
return false, err
|
||||||
}
|
}
|
||||||
return !conflicting, nil
|
if conflicting {
|
||||||
|
return false, nil
|
||||||
|
}
|
||||||
|
|
||||||
|
pinned, err := m.store.HasGatewayPinnedByOtherAccount(ctx, clusterAddress, accountID)
|
||||||
|
if err != nil {
|
||||||
|
return false, err
|
||||||
|
}
|
||||||
|
if pinned {
|
||||||
|
log.WithContext(ctx).Infof("cluster address %s is pinned as another account's agent network gateway, refusing claim by account %s", clusterAddress, accountID)
|
||||||
|
return false, nil
|
||||||
|
}
|
||||||
|
|
||||||
|
return true, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
func (m *Manager) DeleteAccountCluster(ctx context.Context, clusterAddress, accountID string) error {
|
func (m *Manager) DeleteAccountCluster(ctx context.Context, clusterAddress, accountID string) error {
|
||||||
|
|||||||
@@ -24,6 +24,7 @@ type mockStore struct {
|
|||||||
getProxyByAccountIDFunc func(ctx context.Context, accountID string) (*proxy.Proxy, error)
|
getProxyByAccountIDFunc func(ctx context.Context, accountID string) (*proxy.Proxy, error)
|
||||||
countProxiesByAccountIDFunc func(ctx context.Context, accountID string) (int64, error)
|
countProxiesByAccountIDFunc func(ctx context.Context, accountID string) (int64, error)
|
||||||
isClusterAddressConflictingFunc func(ctx context.Context, clusterAddress, accountID string) (bool, error)
|
isClusterAddressConflictingFunc func(ctx context.Context, clusterAddress, accountID string) (bool, error)
|
||||||
|
hasGatewayPinnedByOtherAccountFunc func(ctx context.Context, host, accountID string) (bool, error)
|
||||||
deleteAccountClusterFunc func(ctx context.Context, clusterAddress, accountID string) error
|
deleteAccountClusterFunc func(ctx context.Context, clusterAddress, accountID string) error
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -84,6 +85,12 @@ func (m *mockStore) IsClusterAddressConflicting(ctx context.Context, clusterAddr
|
|||||||
}
|
}
|
||||||
return false, nil
|
return false, nil
|
||||||
}
|
}
|
||||||
|
func (m *mockStore) HasGatewayPinnedByOtherAccount(ctx context.Context, host, accountID string) (bool, error) {
|
||||||
|
if m.hasGatewayPinnedByOtherAccountFunc != nil {
|
||||||
|
return m.hasGatewayPinnedByOtherAccountFunc(ctx, host, accountID)
|
||||||
|
}
|
||||||
|
return false, nil
|
||||||
|
}
|
||||||
func (m *mockStore) DeleteAccountCluster(ctx context.Context, clusterAddress, accountID string) error {
|
func (m *mockStore) DeleteAccountCluster(ctx context.Context, clusterAddress, accountID string) error {
|
||||||
if m.deleteAccountClusterFunc != nil {
|
if m.deleteAccountClusterFunc != nil {
|
||||||
return m.deleteAccountClusterFunc(ctx, clusterAddress, accountID)
|
return m.deleteAccountClusterFunc(ctx, clusterAddress, accountID)
|
||||||
@@ -338,3 +345,65 @@ func TestGetActiveClusterAddressesForAccount(t *testing.T) {
|
|||||||
require.NoError(t, err)
|
require.NoError(t, err)
|
||||||
assert.Equal(t, expected, result)
|
assert.Equal(t, expected, result)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// TestIsClusterAddressAvailableConsidersGatewayPins pins that a proxy row is
|
||||||
|
// not the only claim on an address.
|
||||||
|
//
|
||||||
|
// An agent network gateway pinned to the address by another account is
|
||||||
|
// immutable and is served by whichever proxy declares that address, so a proxy
|
||||||
|
// from a different account taking it strands the pin — the mapping paths never
|
||||||
|
// hand an account-scoped proxy another account's mappings. Refusing the later
|
||||||
|
// claimant is what makes the bootstrap-time ownership check hold over time
|
||||||
|
// rather than only at the instant it runs: without this, an address a gateway
|
||||||
|
// pinned while no proxy served it could be taken a moment, or a week, later.
|
||||||
|
func TestIsClusterAddressAvailableConsidersGatewayPins(t *testing.T) {
|
||||||
|
ctx := context.Background()
|
||||||
|
|
||||||
|
tests := []struct {
|
||||||
|
name string
|
||||||
|
conflicting bool
|
||||||
|
pinned bool
|
||||||
|
available bool
|
||||||
|
}{
|
||||||
|
{name: "free address", available: true},
|
||||||
|
{name: "claimed by a proxy", conflicting: true},
|
||||||
|
{name: "pinned by another account's gateway", pinned: true},
|
||||||
|
{name: "claimed both ways", conflicting: true, pinned: true},
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, tt := range tests {
|
||||||
|
t.Run(tt.name, func(t *testing.T) {
|
||||||
|
st := &mockStore{
|
||||||
|
isClusterAddressConflictingFunc: func(context.Context, string, string) (bool, error) {
|
||||||
|
return tt.conflicting, nil
|
||||||
|
},
|
||||||
|
hasGatewayPinnedByOtherAccountFunc: func(context.Context, string, string) (bool, error) {
|
||||||
|
return tt.pinned, nil
|
||||||
|
},
|
||||||
|
}
|
||||||
|
m, err := NewManager(st, noop.NewMeterProvider().Meter(""))
|
||||||
|
require.NoError(t, err)
|
||||||
|
|
||||||
|
available, err := m.IsClusterAddressAvailable(ctx, "gw.example.com", "account1")
|
||||||
|
require.NoError(t, err)
|
||||||
|
assert.Equal(t, tt.available, available)
|
||||||
|
})
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// TestIsClusterAddressAvailableSurfacesGatewayPinError pins that a failed pin
|
||||||
|
// lookup refuses the claim rather than falling through to available: this runs
|
||||||
|
// on the proxy-connect path, where "could not tell" must not read as "yes".
|
||||||
|
func TestIsClusterAddressAvailableSurfacesGatewayPinError(t *testing.T) {
|
||||||
|
st := &mockStore{
|
||||||
|
hasGatewayPinnedByOtherAccountFunc: func(context.Context, string, string) (bool, error) {
|
||||||
|
return false, errors.New("db down")
|
||||||
|
},
|
||||||
|
}
|
||||||
|
m, err := NewManager(st, noop.NewMeterProvider().Meter(""))
|
||||||
|
require.NoError(t, err)
|
||||||
|
|
||||||
|
available, err := m.IsClusterAddressAvailable(context.Background(), "gw.example.com", "account1")
|
||||||
|
require.Error(t, err)
|
||||||
|
assert.False(t, available)
|
||||||
|
}
|
||||||
|
|||||||
@@ -315,6 +315,32 @@ func (s *SqlStore) GetAllAgentNetworkSettings(ctx context.Context, lockStrength
|
|||||||
return settings, nil
|
return settings, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// HasGatewayPinnedByOtherAccount reports whether an account other than the
|
||||||
|
// given one has its agent network gateway pinned to this host.
|
||||||
|
//
|
||||||
|
// A pin is a claim on the host, the same way a proxy row is: the pinned
|
||||||
|
// endpoint is served by whichever proxy declares that address, and an
|
||||||
|
// account-scoped proxy only ever receives its own account's mappings. A proxy
|
||||||
|
// from a different account taking the address therefore cannot serve the pin
|
||||||
|
// and silently strands it. The pin is immutable, so the account that holds it
|
||||||
|
// cannot move out of the way — the later claimant is the one to refuse.
|
||||||
|
//
|
||||||
|
// Both sides are canonical (settings normalize on write, proxy addresses
|
||||||
|
// canonicalize at connect), so the match is exact and uses the proxy_address
|
||||||
|
// index.
|
||||||
|
func (s *SqlStore) HasGatewayPinnedByOtherAccount(ctx context.Context, host, accountID string) (bool, error) {
|
||||||
|
var count int64
|
||||||
|
result := s.db.
|
||||||
|
Model(&agentNetworkTypes.Settings{}).
|
||||||
|
Where("proxy_address = ? AND account_id != ?", host, accountID).
|
||||||
|
Count(&count)
|
||||||
|
if result.Error != nil {
|
||||||
|
log.WithContext(ctx).Errorf("failed to check agent network gateway pins by proxy address: %v", result.Error)
|
||||||
|
return false, status.Errorf(status.Internal, "check agent network gateway pins")
|
||||||
|
}
|
||||||
|
return count > 0, nil
|
||||||
|
}
|
||||||
|
|
||||||
// GetAgentNetworkSettingsByProxyAddress returns every Settings row whose
|
// GetAgentNetworkSettingsByProxyAddress returns every Settings row whose
|
||||||
// gateway is served by the proxy declaring the given cluster address. Used by
|
// gateway is served by the proxy declaring the given cluster address. Used by
|
||||||
// cluster-scoped synthesis to find the accounts a shared proxy serves.
|
// cluster-scoped synthesis to find the accounts a shared proxy serves.
|
||||||
|
|||||||
@@ -374,6 +374,7 @@ type Store interface {
|
|||||||
GetAgentNetworkSettings(ctx context.Context, lockStrength LockingStrength, accountID string) (*agentNetworkTypes.Settings, error)
|
GetAgentNetworkSettings(ctx context.Context, lockStrength LockingStrength, accountID string) (*agentNetworkTypes.Settings, error)
|
||||||
GetAllAgentNetworkSettings(ctx context.Context, lockStrength LockingStrength) ([]*agentNetworkTypes.Settings, error)
|
GetAllAgentNetworkSettings(ctx context.Context, lockStrength LockingStrength) ([]*agentNetworkTypes.Settings, error)
|
||||||
GetAgentNetworkSettingsByProxyAddress(ctx context.Context, lockStrength LockingStrength, proxyAddress string) ([]*agentNetworkTypes.Settings, error)
|
GetAgentNetworkSettingsByProxyAddress(ctx context.Context, lockStrength LockingStrength, proxyAddress string) ([]*agentNetworkTypes.Settings, error)
|
||||||
|
HasGatewayPinnedByOtherAccount(ctx context.Context, host, accountID string) (bool, error)
|
||||||
GetAgentNetworkSettingsByDomain(ctx context.Context, lockStrength LockingStrength, domain string) (*agentNetworkTypes.Settings, error)
|
GetAgentNetworkSettingsByDomain(ctx context.Context, lockStrength LockingStrength, domain string) (*agentNetworkTypes.Settings, error)
|
||||||
CreateAgentNetworkSettings(ctx context.Context, settings *agentNetworkTypes.Settings) error
|
CreateAgentNetworkSettings(ctx context.Context, settings *agentNetworkTypes.Settings) error
|
||||||
SaveAgentNetworkSettings(ctx context.Context, settings *agentNetworkTypes.Settings) error
|
SaveAgentNetworkSettings(ctx context.Context, settings *agentNetworkTypes.Settings) error
|
||||||
|
|||||||
@@ -3080,6 +3080,21 @@ func (mr *MockStoreMockRecorder) HasForeignAccountProxyAtHost(ctx, host, account
|
|||||||
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "HasForeignAccountProxyAtHost", reflect.TypeOf((*MockStore)(nil).HasForeignAccountProxyAtHost), ctx, host, accountID)
|
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "HasForeignAccountProxyAtHost", reflect.TypeOf((*MockStore)(nil).HasForeignAccountProxyAtHost), ctx, host, accountID)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// HasGatewayPinnedByOtherAccount mocks base method.
|
||||||
|
func (m *MockStore) HasGatewayPinnedByOtherAccount(ctx context.Context, host, accountID string) (bool, error) {
|
||||||
|
m.ctrl.T.Helper()
|
||||||
|
ret := m.ctrl.Call(m, "HasGatewayPinnedByOtherAccount", ctx, host, accountID)
|
||||||
|
ret0, _ := ret[0].(bool)
|
||||||
|
ret1, _ := ret[1].(error)
|
||||||
|
return ret0, ret1
|
||||||
|
}
|
||||||
|
|
||||||
|
// HasGatewayPinnedByOtherAccount indicates an expected call of HasGatewayPinnedByOtherAccount.
|
||||||
|
func (mr *MockStoreMockRecorder) HasGatewayPinnedByOtherAccount(ctx, host, accountID any) *gomock.Call {
|
||||||
|
mr.mock.ctrl.T.Helper()
|
||||||
|
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "HasGatewayPinnedByOtherAccount", reflect.TypeOf((*MockStore)(nil).HasGatewayPinnedByOtherAccount), ctx, host, accountID)
|
||||||
|
}
|
||||||
|
|
||||||
// IncrementAgentNetworkConsumption mocks base method.
|
// 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 {
|
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()
|
m.ctrl.T.Helper()
|
||||||
|
|||||||
Reference in New Issue
Block a user