fix: Guard against nil traits in GetInheritedMembershipRequires (#61354)

- Deep-copy `accesslist.Requires` to avoid mutation
- Guard against nil `accesslist.Requires.Traits` before iterating
- Add test coverage to prevent regression
This commit is contained in:
Maxim
2025-11-19 13:30:35 +00:00
committed by GitHub
parent 8c69943745
commit 131b4b89db
3 changed files with 220 additions and 0 deletions
+11
View File
@@ -292,6 +292,17 @@ func (r *Requires) IsEmpty() bool {
return len(r.Roles) == 0 && len(r.Traits) == 0
}
// Clone returns a deep copy of the [Requires]
func (r *Requires) Clone() Requires {
if r == nil {
return Requires{}
}
return Requires{
Roles: slices.Clone(r.Roles),
Traits: r.Traits.Clone(),
}
}
// Grants describes what access is granted by membership to the access list.
type Grants struct {
// Roles are the roles that are granted to users who are members of the access list.
+4
View File
@@ -759,6 +759,7 @@ func collectAncestors(ctx context.Context, accessList *accesslist.AccessList, ki
// inherited from any ancestor lists, and the Access List's own MembershipRequires.
func GetInheritedMembershipRequires(ctx context.Context, accessList *accesslist.AccessList, g AccessListAndMembersGetter) (*accesslist.Requires, error) {
ownRequires := accessList.GetMembershipRequires()
ownRequires = ownRequires.Clone()
ancestors, err := GetAncestorsFor(ctx, accessList, RelationshipKindMember, g)
if err != nil {
return &ownRequires, trace.Wrap(err)
@@ -766,6 +767,9 @@ func GetInheritedMembershipRequires(ctx context.Context, accessList *accesslist.
roles := ownRequires.Roles
traits := ownRequires.Traits
if traits == nil {
traits = trait.Traits{}
}
for _, ancestor := range ancestors {
requires := ancestor.GetMembershipRequires()
+205
View File
@@ -26,6 +26,7 @@ import (
"testing"
"time"
"github.com/google/uuid"
"github.com/gravitational/trace"
"github.com/jonboulle/clockwork"
"github.com/stretchr/testify/require"
@@ -34,6 +35,7 @@ import (
"github.com/gravitational/teleport/api/types"
"github.com/gravitational/teleport/api/types/accesslist"
"github.com/gravitational/teleport/api/types/header"
"github.com/gravitational/teleport/api/types/trait"
"github.com/gravitational/teleport/lib/itertools/stream"
)
@@ -675,6 +677,209 @@ func TestGetInheritedGrants(t *testing.T) {
require.Equal(t, expectedGrants, grants)
}
func TestGetInheritedRequires(t *testing.T) {
t.Parallel()
ctx := t.Context()
clock := clockwork.NewFakeClock()
type testListSpec struct {
name string
requires accesslist.Requires
memberOf []string
}
tests := []struct {
name string
lists []testListSpec
targetName string
expected *accesslist.Requires
}{
{
name: "NilLeafRequires_Chain3",
lists: []testListSpec{
{
name: "root",
requires: accesslist.Requires{
Roles: []string{"root-role"},
Traits: map[string][]string{
"env": {"prod"},
},
},
},
{
name: "intermediate",
requires: accesslist.Requires{
Roles: []string{"intermediate-role"},
Traits: map[string][]string{
"env": {"testing"},
"team": {"backend"},
},
},
memberOf: []string{"root"},
},
{
name: "leaf",
requires: accesslist.Requires{},
memberOf: []string{"intermediate"},
},
},
targetName: "leaf",
expected: &accesslist.Requires{
Roles: []string{"intermediate-role", "root-role"},
Traits: trait.Traits{
"env": {"prod", "testing"},
"team": {"backend"},
},
},
},
{
name: "LeafHasOwnRequires_Chain3",
lists: []testListSpec{
{
name: "root",
requires: accesslist.Requires{
Roles: []string{"root-role"},
Traits: trait.Traits{
"env": {"prod"},
},
},
},
{
name: "intermediate",
requires: accesslist.Requires{
Roles: []string{"app-role"},
Traits: trait.Traits{
"env": {"staging"},
"team": {"backend"},
},
},
memberOf: []string{"root"},
},
{
name: "leaf",
requires: accesslist.Requires{
Roles: []string{"leaf-role"},
Traits: trait.Traits{
"team": {"infra"},
"region": {"us-east-1"},
},
},
memberOf: []string{"intermediate"},
},
},
targetName: "leaf",
expected: &accesslist.Requires{
Roles: []string{"app-role", "leaf-role", "root-role"},
Traits: trait.Traits{
"env": {"prod", "staging"},
"team": {"backend", "infra"},
"region": {"us-east-1"},
},
},
},
{
name: "NilLeafRequires_Diamond",
lists: []testListSpec{
{
name: "root",
requires: accesslist.Requires{
Roles: []string{"root-role"},
Traits: trait.Traits{
"env": {"prod"},
},
},
},
{
name: "parent-a",
requires: accesslist.Requires{
Roles: []string{"a-role"},
Traits: trait.Traits{
"env": {"staging"},
"team": {"a-team"},
},
},
memberOf: []string{"root"},
},
{
name: "parent-b",
requires: accesslist.Requires{
Roles: []string{"b-role"},
Traits: trait.Traits{
"team": {"b-team"},
"region": {"eu-west-1"},
},
},
memberOf: []string{"root"},
},
{
name: "leaf",
requires: accesslist.Requires{},
memberOf: []string{"parent-a", "parent-b"},
},
},
targetName: "leaf",
expected: &accesslist.Requires{
Roles: []string{"a-role", "b-role", "root-role"},
Traits: trait.Traits{
"env": {"prod", "staging"},
"team": {"a-team", "b-team"},
"region": {"eu-west-1"},
},
},
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
listsByID := make(map[string]*accesslist.AccessList, len(tc.lists))
listsByName := make(map[string]*accesslist.AccessList, len(tc.lists))
for _, ls := range tc.lists {
acl, err := accesslist.NewAccessList(
header.Metadata{
Name: uuid.NewString(),
},
accesslist.Spec{
Title: ls.name,
Description: ls.name,
MembershipRequires: ls.requires,
},
)
require.NoError(t, err)
listsByID[acl.GetName()] = acl
listsByName[ls.name] = acl
}
members := make(map[string][]*accesslist.AccessListMember)
for _, ls := range tc.lists {
child := listsByName[ls.name]
for _, parentName := range ls.memberOf {
parent := listsByName[parentName]
child.Status.MemberOf = append(child.Status.MemberOf, parent.GetName())
members[parent.GetName()] = append(members[parent.GetName()], newAccessListMember(t, parent.GetName(), child.GetName(), accesslist.MembershipKindList, clock))
}
}
getter := &mockAccessListAndMembersGetter{
accessLists: listsByID,
members: members,
}
leaf := listsByName[tc.targetName]
originalLeafRequires := leaf.Spec.MembershipRequires.Clone()
requires, err := GetInheritedMembershipRequires(ctx, leaf, getter)
require.NoError(t, err)
// Should be expected
require.Equal(t, tc.expected, requires)
// Original should not be mutated
require.Equal(t, originalLeafRequires, leaf.Spec.MembershipRequires)
})
}
}
func TestGetMembersFor_FlattensAndStopsOnCycles(t *testing.T) {
clock := clockwork.NewFakeClock()
ctx := context.Background()