[management] Prevent deleting groups referenced by reverse proxy services (#7062)

## Describe your changes

A group could be deleted while a reverse proxy service still referenced
it, silently breaking the service's access control: private services
list groups in `access_groups` as the peer allowlist, and SSO bearer
auth distributes tokens to `distribution_groups`.

Group deletion now runs through the same linkage validation as routes,
policies, and agent network policies: deleting a group that backs a
private service allowlist or an enabled bearer-auth distribution list
fails with a `GroupLinkError` naming the service domain. Disabled bearer
configs and stale `access_groups` on non-private services are inert and
do not block deletion.

Tests cover both linked cases in single and bulk deletion, and pin the
non-blocking cases. The test account seeds decoy services ahead of the
linked ones so the check is proven to scan the full service list.
This commit is contained in:
Maycon Santos
2026-08-05 03:24:20 +09:00
committed by GitHub
parent 2afa69b622
commit 6526fc2bec
2 changed files with 165 additions and 0 deletions

View File

@@ -7,6 +7,7 @@ import (
"slices"
agentNetworkTypes "github.com/netbirdio/netbird/management/internals/modules/agentnetwork/types"
"github.com/netbirdio/netbird/management/internals/modules/reverseproxy/service"
"github.com/rs/xid"
log "github.com/sirupsen/logrus"
@@ -745,6 +746,10 @@ func validateDeleteGroup(ctx context.Context, transaction store.Store, group *ty
return &GroupLinkError{"network router", linkedRouter.ID}
}
if isLinked, linkedService := isGroupLinkedToReverseProxyService(ctx, transaction, group.AccountID, group.ID); isLinked {
return &GroupLinkError{"reverse proxy service", linkedService.Domain}
}
if isLinked, linkedPolicy := isGroupLinkedToAgentNetworkPolicy(ctx, transaction, group.AccountID, group.ID); isLinked {
return &GroupLinkError{"agent network policy", linkedPolicy.Name}
}
@@ -880,6 +885,26 @@ func isGroupLinkedToNetworkRouter(ctx context.Context, transaction store.Store,
return false, nil
}
// isGroupLinkedToReverseProxyService checks if a group is used as an access group
// of a private reverse proxy service or as a bearer-auth distribution group.
func isGroupLinkedToReverseProxyService(ctx context.Context, transaction store.Store, accountID string, groupID string) (bool, *service.Service) {
services, err := transaction.GetAccountServices(ctx, store.LockingStrengthNone, accountID)
if err != nil {
log.WithContext(ctx).Errorf("error retrieving reverse proxy services while checking group linkage: %v", err)
return false, nil
}
for _, svc := range services {
if svc.Private && slices.Contains(svc.AccessGroups, groupID) {
return true, svc
}
if svc.Auth.BearerAuth != nil && svc.Auth.BearerAuth.Enabled && slices.Contains(svc.Auth.BearerAuth.DistributionGroups, groupID) {
return true, svc
}
}
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) {

View File

@@ -19,6 +19,7 @@ import (
nbdns "github.com/netbirdio/netbird/dns"
agentNetworkTypes "github.com/netbirdio/netbird/management/internals/modules/agentnetwork/types"
rpservice "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/service"
"github.com/netbirdio/netbird/management/server/groups"
"github.com/netbirdio/netbird/management/server/networks"
"github.com/netbirdio/netbird/management/server/networks/resources"
@@ -131,6 +132,16 @@ func TestDefaultAccountManager_DeleteGroup(t *testing.T) {
"grp-for-agent-network-policy",
"agent network policy",
},
{
"reverse proxy private service access group",
"grp-for-rp-private",
"reverse proxy service",
},
{
"reverse proxy bearer distribution group",
"grp-for-rp-bearer",
"reverse proxy service",
},
}
for _, testCase := range testCases {
@@ -229,6 +240,12 @@ func TestDefaultAccountManager_DeleteGroups(t *testing.T) {
groupIDs: []string{"grp-for-agent-network-policy"},
expectedReasons: []string{"agent network policy"},
},
{
name: "reverse proxy services",
groupIDs: []string{"grp-for-rp-private", "grp-for-rp-bearer"},
expectedReasons: []string{"reverse proxy service", "reverse proxy service"},
expectedNotDeleted: []string{"grp-for-rp-private", "grp-for-rp-bearer"},
},
{
name: "successfully delete multiple groups",
groupIDs: []string{"group-1", "group-2"},
@@ -296,6 +313,65 @@ func TestDefaultAccountManager_DeleteGroups(t *testing.T) {
}
}
func TestDefaultAccountManager_DeleteGroupUnlinkedFromReverseProxyService(t *testing.T) {
am, _, err := createManager(t)
require.NoError(t, err, "Failed to create account manager")
_, account, err := initTestGroupAccount(am)
require.NoError(t, err, "Failed to init testing account")
deletableGroups := []*types.Group{
{
ID: "grp-rp-bearer-disabled",
AccountID: account.Id,
Name: "Group only in a disabled bearer auth",
Issued: types.GroupIssuedAPI,
Peers: make([]string, 0),
},
{
ID: "grp-rp-nonprivate-access",
AccountID: account.Id,
Name: "Group only in a non-private service's access groups",
Issued: types.GroupIssuedAPI,
Peers: make([]string, 0),
},
}
for _, group := range deletableGroups {
require.NoError(t, am.CreateGroup(context.Background(), account.Id, groupAdminUserID, group))
}
// Disabled bearer auth and stale access groups on a non-private service
// are inert configuration and must not block group deletion.
services := []*rpservice.Service{
{
ID: "rp-svc-bearer-disabled",
AccountID: account.Id,
Domain: "bearer-disabled.services.example.com",
Auth: rpservice.AuthConfig{
BearerAuth: &rpservice.BearerAuthConfig{
Enabled: false,
DistributionGroups: []string{"grp-rp-bearer-disabled"},
},
},
},
{
ID: "rp-svc-nonprivate-access",
AccountID: account.Id,
Domain: "nonprivate.services.example.com",
Private: false,
AccessGroups: []string{"grp-rp-nonprivate-access"},
},
}
for _, svc := range services {
require.NoError(t, am.Store.CreateService(context.Background(), svc))
}
for _, group := range deletableGroups {
err = am.DeleteGroup(context.Background(), account.Id, groupAdminUserID, group.ID)
assert.NoError(t, err, "group %s is not referenced by an active reverse proxy gate and should be deletable", group.ID)
}
}
func TestDefaultAccountManager_DeleteGroupLinkedToFlowGroup(t *testing.T) {
am, _, err := createManager(t)
require.NoError(t, err)
@@ -425,6 +501,22 @@ func initTestGroupAccount(am *DefaultAccountManager) (*DefaultAccountManager, *t
Peers: make([]string, 0),
}
groupForRPPrivate := &types.Group{
ID: "grp-for-rp-private",
AccountID: "account-id",
Name: "Group for private reverse proxy service",
Issued: types.GroupIssuedAPI,
Peers: make([]string, 0),
}
groupForRPBearer := &types.Group{
ID: "grp-for-rp-bearer",
AccountID: "account-id",
Name: "Group for bearer reverse proxy service",
Issued: types.GroupIssuedAPI,
Peers: make([]string, 0),
}
routeResource := &route.Route{
ID: "example route",
Groups: []string{groupForRoute.ID},
@@ -481,6 +573,8 @@ func initTestGroupAccount(am *DefaultAccountManager) (*DefaultAccountManager, *t
_ = am.CreateGroup(context.Background(), accountID, groupAdminUserID, groupForUsers)
_ = am.CreateGroup(context.Background(), accountID, groupAdminUserID, groupForIntegration)
_ = am.CreateGroup(context.Background(), accountID, groupAdminUserID, groupForAgentNetworkPolicy)
_ = am.CreateGroup(context.Background(), accountID, groupAdminUserID, groupForRPPrivate)
_ = am.CreateGroup(context.Background(), accountID, groupAdminUserID, groupForRPBearer)
agentNetworkPolicy := &agentNetworkTypes.Policy{
ID: "example agent network policy",
@@ -493,6 +587,52 @@ func initTestGroupAccount(am *DefaultAccountManager) (*DefaultAccountManager, *t
return nil, nil, err
}
// The decoy services are created first so the linkage check has to scan
// past services that do not reference the groups under test.
rpServices := []*rpservice.Service{
{
ID: "rp-svc-private-decoy",
AccountID: accountID,
Domain: "private-decoy.services.example.com",
Private: true,
AccessGroups: []string{"unrelated-group"},
},
{
ID: "rp-svc-bearer-decoy",
AccountID: accountID,
Domain: "bearer-decoy.services.example.com",
Auth: rpservice.AuthConfig{
BearerAuth: &rpservice.BearerAuthConfig{
Enabled: true,
DistributionGroups: []string{"unrelated-group"},
},
},
},
{
ID: "rp-svc-private",
AccountID: accountID,
Domain: "private.services.example.com",
Private: true,
AccessGroups: []string{groupForRPPrivate.ID},
},
{
ID: "rp-svc-bearer",
AccountID: accountID,
Domain: "bearer.services.example.com",
Auth: rpservice.AuthConfig{
BearerAuth: &rpservice.BearerAuthConfig{
Enabled: true,
DistributionGroups: []string{groupForRPBearer.ID},
},
},
},
}
for _, svc := range rpServices {
if err := am.Store.CreateService(context.Background(), svc); err != nil {
return nil, nil, err
}
}
acc, err := am.Store.GetAccount(context.Background(), account.Id)
if err != nil {
return nil, nil, err