mirror of
https://github.com/netbirdio/netbird.git
synced 2026-10-10 23:49:09 +02:00
[management] prevent dangling group refs in agent-network ACLs. (#7060)
Block deleting a group referenced as a source group by an agent network policy, and drop unresolvable groups from synthesised private-service ACLs. A deleted group survived in agent_network_policies.source_groups and was carried into the injected in-memory policy, where network-map assembly resolved it to a nil group and panicked on every proxy peer sync.
This commit is contained in:
@@ -6,6 +6,7 @@ import (
|
|||||||
"fmt"
|
"fmt"
|
||||||
"slices"
|
"slices"
|
||||||
|
|
||||||
|
agentNetworkTypes "github.com/netbirdio/netbird/management/internals/modules/agentnetwork/types"
|
||||||
"github.com/rs/xid"
|
"github.com/rs/xid"
|
||||||
log "github.com/sirupsen/logrus"
|
log "github.com/sirupsen/logrus"
|
||||||
|
|
||||||
@@ -744,6 +745,10 @@ func validateDeleteGroup(ctx context.Context, transaction store.Store, group *ty
|
|||||||
return &GroupLinkError{"network router", linkedRouter.ID}
|
return &GroupLinkError{"network router", linkedRouter.ID}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
if isLinked, linkedPolicy := isGroupLinkedToAgentNetworkPolicy(ctx, transaction, group.AccountID, group.ID); isLinked {
|
||||||
|
return &GroupLinkError{"agent network policy", linkedPolicy.Name}
|
||||||
|
}
|
||||||
|
|
||||||
return checkGroupLinkedToSettings(ctx, transaction, group)
|
return checkGroupLinkedToSettings(ctx, transaction, group)
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -875,6 +880,26 @@ func isGroupLinkedToNetworkRouter(ctx context.Context, transaction store.Store,
|
|||||||
return false, nil
|
return false, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// isGroupLinkedToAgentNetworkPolicy checks if a group is used as a source group by any
|
||||||
|
// agent network policy in the account.
|
||||||
|
func isGroupLinkedToAgentNetworkPolicy(ctx context.Context, transaction store.Store, accountID string, groupID string) (bool, *agentNetworkTypes.Policy) {
|
||||||
|
policies, err := transaction.GetAccountAgentNetworkPolicies(ctx, store.LockingStrengthNone, accountID)
|
||||||
|
if err != nil {
|
||||||
|
log.WithContext(ctx).Errorf("error retrieving agent network policies while checking group linkage: %v", err)
|
||||||
|
return false, nil
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, policy := range policies {
|
||||||
|
if policy == nil {
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
if slices.Contains(policy.SourceGroups, groupID) {
|
||||||
|
return true, policy
|
||||||
|
}
|
||||||
|
}
|
||||||
|
return false, nil
|
||||||
|
}
|
||||||
|
|
||||||
// areGroupChangesAffectPeers checks if any changes to the specified groups will affect peers.
|
// areGroupChangesAffectPeers checks if any changes to the specified groups will affect peers.
|
||||||
// It fetches each collection once and checks all groupIDs against them in memory.
|
// It fetches each collection once and checks all groupIDs against them in memory.
|
||||||
func areGroupChangesAffectPeers(ctx context.Context, transaction store.Store, accountID string, groupIDs []string) (bool, error) {
|
func areGroupChangesAffectPeers(ctx context.Context, transaction store.Store, accountID string, groupIDs []string) (bool, error) {
|
||||||
|
|||||||
@@ -18,6 +18,7 @@ import (
|
|||||||
"golang.org/x/exp/maps"
|
"golang.org/x/exp/maps"
|
||||||
|
|
||||||
nbdns "github.com/netbirdio/netbird/dns"
|
nbdns "github.com/netbirdio/netbird/dns"
|
||||||
|
agentNetworkTypes "github.com/netbirdio/netbird/management/internals/modules/agentnetwork/types"
|
||||||
"github.com/netbirdio/netbird/management/server/groups"
|
"github.com/netbirdio/netbird/management/server/groups"
|
||||||
"github.com/netbirdio/netbird/management/server/networks"
|
"github.com/netbirdio/netbird/management/server/networks"
|
||||||
"github.com/netbirdio/netbird/management/server/networks/resources"
|
"github.com/netbirdio/netbird/management/server/networks/resources"
|
||||||
@@ -125,6 +126,11 @@ func TestDefaultAccountManager_DeleteGroup(t *testing.T) {
|
|||||||
"grp-for-integration",
|
"grp-for-integration",
|
||||||
"only service users with admin power can delete integration group",
|
"only service users with admin power can delete integration group",
|
||||||
},
|
},
|
||||||
|
{
|
||||||
|
"agent network policy",
|
||||||
|
"grp-for-agent-network-policy",
|
||||||
|
"agent network policy",
|
||||||
|
},
|
||||||
}
|
}
|
||||||
|
|
||||||
for _, testCase := range testCases {
|
for _, testCase := range testCases {
|
||||||
@@ -218,6 +224,11 @@ func TestDefaultAccountManager_DeleteGroups(t *testing.T) {
|
|||||||
groupIDs: []string{"grp-for-integration"},
|
groupIDs: []string{"grp-for-integration"},
|
||||||
expectedReasons: []string{"only service users with admin power can delete integration group"},
|
expectedReasons: []string{"only service users with admin power can delete integration group"},
|
||||||
},
|
},
|
||||||
|
{
|
||||||
|
name: "agent network policy",
|
||||||
|
groupIDs: []string{"grp-for-agent-network-policy"},
|
||||||
|
expectedReasons: []string{"agent network policy"},
|
||||||
|
},
|
||||||
{
|
{
|
||||||
name: "successfully delete multiple groups",
|
name: "successfully delete multiple groups",
|
||||||
groupIDs: []string{"group-1", "group-2"},
|
groupIDs: []string{"group-1", "group-2"},
|
||||||
@@ -406,6 +417,14 @@ func initTestGroupAccount(am *DefaultAccountManager) (*DefaultAccountManager, *t
|
|||||||
Peers: make([]string, 0),
|
Peers: make([]string, 0),
|
||||||
}
|
}
|
||||||
|
|
||||||
|
groupForAgentNetworkPolicy := &types.Group{
|
||||||
|
ID: "grp-for-agent-network-policy",
|
||||||
|
AccountID: "account-id",
|
||||||
|
Name: "Group for agent network policies",
|
||||||
|
Issued: types.GroupIssuedAPI,
|
||||||
|
Peers: make([]string, 0),
|
||||||
|
}
|
||||||
|
|
||||||
routeResource := &route.Route{
|
routeResource := &route.Route{
|
||||||
ID: "example route",
|
ID: "example route",
|
||||||
Groups: []string{groupForRoute.ID},
|
Groups: []string{groupForRoute.ID},
|
||||||
@@ -461,6 +480,18 @@ func initTestGroupAccount(am *DefaultAccountManager) (*DefaultAccountManager, *t
|
|||||||
_ = am.CreateGroup(context.Background(), accountID, groupAdminUserID, groupForSetupKeys)
|
_ = am.CreateGroup(context.Background(), accountID, groupAdminUserID, groupForSetupKeys)
|
||||||
_ = am.CreateGroup(context.Background(), accountID, groupAdminUserID, groupForUsers)
|
_ = am.CreateGroup(context.Background(), accountID, groupAdminUserID, groupForUsers)
|
||||||
_ = am.CreateGroup(context.Background(), accountID, groupAdminUserID, groupForIntegration)
|
_ = am.CreateGroup(context.Background(), accountID, groupAdminUserID, groupForIntegration)
|
||||||
|
_ = am.CreateGroup(context.Background(), accountID, groupAdminUserID, groupForAgentNetworkPolicy)
|
||||||
|
|
||||||
|
agentNetworkPolicy := &agentNetworkTypes.Policy{
|
||||||
|
ID: "example agent network policy",
|
||||||
|
AccountID: accountID,
|
||||||
|
Name: "Example agent network policy",
|
||||||
|
Enabled: true,
|
||||||
|
SourceGroups: []string{groupForAgentNetworkPolicy.ID},
|
||||||
|
}
|
||||||
|
if err := am.Store.SaveAgentNetworkPolicy(context.Background(), agentNetworkPolicy); err != nil {
|
||||||
|
return nil, nil, err
|
||||||
|
}
|
||||||
|
|
||||||
acc, err := am.Store.GetAccount(context.Background(), account.Id)
|
acc, err := am.Store.GetAccount(context.Background(), account.Id)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
|
|||||||
@@ -1707,14 +1707,34 @@ func (a *Account) injectPrivateServicePolicies(svc *service.Service, proxyPeers
|
|||||||
if len(proxyPeers) == 0 {
|
if len(proxyPeers) == 0 {
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
// A service's AccessGroups can name groups that no longer exist — persisted
|
||||||
|
// services and the agent-network synthesiser both carry the ids verbatim from
|
||||||
|
// their own state. An unresolvable source authorises nothing, so drop it here
|
||||||
|
// rather than let the network-map assembly resolve it to a nil group.
|
||||||
|
sources := a.existingGroupIDs(svc.AccessGroups)
|
||||||
|
if len(sources) == 0 {
|
||||||
|
return
|
||||||
|
}
|
||||||
for _, proxyPeer := range proxyPeers {
|
for _, proxyPeer := range proxyPeers {
|
||||||
a.Policies = append(a.Policies, a.createPrivateServicePolicy(svc, proxyPeer))
|
a.Policies = append(a.Policies, a.createPrivateServicePolicy(svc, proxyPeer, sources))
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
func (a *Account) createPrivateServicePolicy(svc *service.Service, proxyPeer *nbpeer.Peer) *Policy {
|
// existingGroupIDs returns the subset of groupIDs that resolve to a group in the account,
|
||||||
|
// preserving the input order.
|
||||||
|
func (a *Account) existingGroupIDs(groupIDs []string) []string {
|
||||||
|
out := make([]string, 0, len(groupIDs))
|
||||||
|
for _, groupID := range groupIDs {
|
||||||
|
if _, ok := a.Groups[groupID]; ok {
|
||||||
|
out = append(out, groupID)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
return out
|
||||||
|
}
|
||||||
|
|
||||||
|
func (a *Account) createPrivateServicePolicy(svc *service.Service, proxyPeer *nbpeer.Peer, accessGroups []string) *Policy {
|
||||||
policyID := fmt.Sprintf("private-access-%s-%s", svc.ID, proxyPeer.ID)
|
policyID := fmt.Sprintf("private-access-%s-%s", svc.ID, proxyPeer.ID)
|
||||||
sources := append([]string(nil), svc.AccessGroups...)
|
sources := append([]string(nil), accessGroups...)
|
||||||
return &Policy{
|
return &Policy{
|
||||||
ID: policyID,
|
ID: policyID,
|
||||||
Name: fmt.Sprintf("Private Access to %s", svc.Name),
|
Name: fmt.Sprintf("Private Access to %s", svc.Name),
|
||||||
|
|||||||
Reference in New Issue
Block a user