Skip to content

ROX-27053: Fix subjects handling for user and group kind [backport] #14414

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
janisz merged 2 commits into from
Feb 28, 2025
Merged

ROX-27053: Fix subjects handling for user and group kind [backport] #14414

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
12e4b2a
ROX-27053: Fix subjects handling for user and group kind (#13410)
mtodor Dec 4, 2024
cfc4a3f
CloneVT to Clone
janisz Feb 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions central/compliance/checks/common/rbac.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
// Collect a list of all known service accounts with bound permissions.
allServiceAccounts := k8srbac.NewSubjectSet()
for _, binding := range ctx.Data().K8sRoleBindings() {
for _, subject := range binding.GetSubjects() {
for _, subject := range k8srbac.GetSubjectsAdjustedByKind(binding) {

Check warning on line 25 in central/compliance/checks/common/rbac.go

View check run for this annotation

Codecov / codecov/patch

central/compliance/checks/common/rbac.go#L25 

Added line #L25 was not covered by tests
if subject.GetKind() == storage.SubjectKind_SERVICE_ACCOUNT {
allServiceAccounts.Add(subject)
}
Expand Down Expand Up @@ -69,7 +69,7 @@
// These should be groups, not shared users.
func AdministratorUsersPresent(ctx framework.ComplianceContext) {
for _, binding := range ctx.Data().K8sRoleBindings() {
for _, subject := range binding.GetSubjects() {
for _, subject := range k8srbac.GetSubjectsAdjustedByKind(binding) {

Check warning on line 72 in central/compliance/checks/common/rbac.go

View check run for this annotation

Codecov / codecov/patch

central/compliance/checks/common/rbac.go#L72 

Added line #L72 was not covered by tests
if subject.GetKind() == storage.SubjectKind_USER {
if adminNames.Contains(strings.ToLower(subject.GetName())) {
framework.Fail(ctx, "Use the GROUP subject kind instead of USER, when specifying administrative accounts.")
Expand Down Expand Up @@ -118,7 +118,7 @@
func listSubjectsWithAccess(predicate func(sub *storage.Subject) bool, roles []*storage.K8SRole, bindings []*storage.K8SRoleBinding, pr *storage.PolicyRule) k8srbac.SubjectSet {
allSubjects := k8srbac.NewSubjectSet()
for _, binding := range bindings {
for _, subject := range binding.GetSubjects() {
for _, subject := range k8srbac.GetSubjectsAdjustedByKind(binding) {

Check warning on line 121 in central/compliance/checks/common/rbac.go

View check run for this annotation

Codecov / codecov/patch

central/compliance/checks/common/rbac.go#L121 

Added line #L121 was not covered by tests
if predicate(subject) {
allSubjects.Add(subject)
}
Expand Down
2 changes: 1 addition & 1 deletion central/compliance/checks/nist80053/check_ac_14/check.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ func checkNoExtraPrivilegesForUnauthenticated(ctx framework.ComplianceContext) {
clusterRoleIDs := set.NewStringSet()
namespaceRoleIDs := make(map[string]set.StringSet)
for _, binding := range k8sRoleBindings {
for _, subject := range binding.GetSubjects() {
for _, subject := range k8srbac.GetSubjectsAdjustedByKind(binding) {
if subject.GetName() == systemUnauthenticatedSubject && subject.GetKind() == storage.SubjectKind_GROUP {
if k8srbac.IsClusterRoleBinding(binding) {
clusterRoleIDs.Add(binding.GetRoleId())
Expand Down
2 changes: 1 addition & 1 deletion central/rbac/service/get_subject.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@
}

var subject *storage.Subject
for _, subj := range relevantBindings[0].GetSubjects() {
for _, subj := range k8srbac.GetSubjectsAdjustedByKind(relevantBindings[0]) {

Check warning on line 39 in central/rbac/service/get_subject.go

View check run for this annotation

Codecov / codecov/patch

central/rbac/service/get_subject.go#L39 

Added line #L39 was not covered by tests
if subj.GetKind() != storage.SubjectKind_USER || subj.GetKind() != storage.SubjectKind_GROUP {
continue
}
Expand Down
135 changes: 135 additions & 0 deletions central/sensor/service/pipeline/rolebindings/pipeline_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
package rolebindings

import (
"testing"

"github.com/stackrox/rox/generated/storage"
"github.com/stackrox/rox/pkg/k8srbac"
"github.com/stackrox/rox/pkg/protoassert"
)

func Test_enrichSubjects(t *testing.T) {
clusterId := "cluster-id-1"
clusterName := "cluster-name-1"

tests := map[string]struct {
binding *storage.K8SRoleBinding
expect *storage.K8SRoleBinding
}{
"nil rolebinding": {
binding: nil,
expect: nil,
},
"nil subjects": {
binding: &storage.K8SRoleBinding{},
expect: &storage.K8SRoleBinding{},
},
"empty subjects": {
binding: &storage.K8SRoleBinding{Subjects: make([]*storage.Subject, 0)},
expect: &storage.K8SRoleBinding{Subjects: make([]*storage.Subject, 0)},
},
"all rolebinding kinds": {
binding: &storage.K8SRoleBinding{
ClusterId: clusterId,
ClusterName: clusterName,
Subjects: []*storage.Subject{
{Kind: storage.SubjectKind_UNSET_KIND, Name: "sub-1"},
{Kind: storage.SubjectKind_SERVICE_ACCOUNT, Name: "sub-2"},
{Kind: storage.SubjectKind_USER, Name: "sub-3"},
{Kind: storage.SubjectKind_GROUP, Name: "sub-4"},
},
},
expect: &storage.K8SRoleBinding{
ClusterId: clusterId,
ClusterName: clusterName,
Subjects: []*storage.Subject{
{
Id: k8srbac.CreateSubjectID(clusterId, "sub-1"),
Kind: storage.SubjectKind_UNSET_KIND,
Name: "sub-1",
ClusterId: clusterId,
ClusterName: clusterName,
},
{
Id: k8srbac.CreateSubjectID(clusterId, "sub-2"),
Kind: storage.SubjectKind_SERVICE_ACCOUNT,
Name: "sub-2",
ClusterId: clusterId,
ClusterName: clusterName,
},
{
Id: k8srbac.CreateSubjectID(clusterId, "sub-3"),
Kind: storage.SubjectKind_USER,
Name: "sub-3",
ClusterId: clusterId,
ClusterName: clusterName,
},
{
Id: k8srbac.CreateSubjectID(clusterId, "sub-4"),
Kind: storage.SubjectKind_GROUP,
Name: "sub-4",
ClusterId: clusterId,
ClusterName: clusterName,
},
},
},
},
"all rolebinding kinds with namespace": {
binding: &storage.K8SRoleBinding{
ClusterId: clusterId,
ClusterName: clusterName,
Subjects: []*storage.Subject{
{Kind: storage.SubjectKind_UNSET_KIND, Name: "sub-1", Namespace: "ns-1"},
{Kind: storage.SubjectKind_SERVICE_ACCOUNT, Name: "sub-2", Namespace: "ns-2"},
{Kind: storage.SubjectKind_USER, Name: "sub-3", Namespace: "ns-3"},
{Kind: storage.SubjectKind_GROUP, Name: "sub-4", Namespace: "ns-4"},
},
},
expect: &storage.K8SRoleBinding{
ClusterId: clusterId,
ClusterName: clusterName,
Subjects: []*storage.Subject{
{
Id: k8srbac.CreateSubjectID(clusterId, "sub-1"),
Kind: storage.SubjectKind_UNSET_KIND,
Name: "sub-1",
Namespace: "ns-1",
ClusterId: clusterId,
ClusterName: clusterName,
},
{
Id: k8srbac.CreateSubjectID(clusterId, "sub-2"),
Kind: storage.SubjectKind_SERVICE_ACCOUNT,
Name: "sub-2",
Namespace: "ns-2",
ClusterId: clusterId,
ClusterName: clusterName,
},
{
Id: k8srbac.CreateSubjectID(clusterId, "sub-3"),
Kind: storage.SubjectKind_USER,
Name: "sub-3",
Namespace: "ns-3",
ClusterId: clusterId,
ClusterName: clusterName,
},
{
Id: k8srbac.CreateSubjectID(clusterId, "sub-4"),
Kind: storage.SubjectKind_GROUP,
Name: "sub-4",
Namespace: "ns-4",
ClusterId: clusterId,
ClusterName: clusterName,
},
},
},
},
}

for testName, tt := range tests {
t.Run(testName, func(t *testing.T) {
enrichSubjects(tt.binding)
protoassert.Equal(t, tt.expect, tt.binding)
})
}
}
6 changes: 3 additions & 3 deletions pkg/k8srbac/evaluator_impl.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@
subjectsWithClusterAdmin := NewSubjectSet()
for _, binding := range roleBindings {
if !IsDefaultRoleBinding(binding) && clusterAdminRoleIDs.Contains(binding.GetRoleId()) {
subjectsWithClusterAdmin.Add(binding.GetSubjects()...)
subjectsWithClusterAdmin.Add(GetSubjectsAdjustedByKind(binding)...)
}
}
return subjectsWithClusterAdmin.ToSlice()
Expand All @@ -70,7 +70,7 @@
subjectsWithClusterAdmin := NewSubjectSet()
for _, binding := range roleBindings {
if clusterAdminRoleIDs.Contains(binding.GetRoleId()) {
subjectsWithClusterAdmin.Add(binding.GetSubjects()...)
subjectsWithClusterAdmin.Add(GetSubjectsAdjustedByKind(binding)...)

Check warning on line 73 in pkg/k8srbac/evaluator_impl.go

View check run for this annotation

Codecov / codecov/patch

pkg/k8srbac/evaluator_impl.go#L73 

Added line #L73 was not covered by tests
}
}
for _, s := range subjectsWithClusterAdmin.ToSlice() {
Expand Down Expand Up @@ -116,7 +116,7 @@
if _, hasEntry := roleIDToSubjects[binding.GetRoleId()]; !hasEntry {
roleIDToSubjects[binding.GetRoleId()] = NewSubjectSet()
}
roleIDToSubjects[binding.GetRoleId()].Add(binding.GetSubjects()...)
roleIDToSubjects[binding.GetRoleId()].Add(GetSubjectsAdjustedByKind(binding)...)
}

// Complete the map so that we can test subjects and get the role for it.
Expand Down
30 changes: 28 additions & 2 deletions pkg/k8srbac/subjects.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,32 @@
return fmt.Sprintf("%s:%s", clusterEncoded, subjectEncoded)
}

// GetSubjectsAdjustedByKind returns subjects adjusted by kind scope.
// User and Group kind do not have namespace defined and such entities should not exist,
// but k8s storage allows it. Docs:
// https://kubernetes.io/docs/reference/kubernetes-api/authorization-resources/role-binding-v1/ -> subjects.namespace
func GetSubjectsAdjustedByKind(binding *storage.K8SRoleBinding) []*storage.Subject {
if binding == nil {
return nil
}

adjustedSubjectSet := NewSubjectSet()
for _, subject := range binding.GetSubjects() {
// Minimize number of CloneVT() calls.
if subject.GetNamespace() != "" && (subject.GetKind() == storage.SubjectKind_USER || subject.GetKind() == storage.SubjectKind_GROUP) {
adjustedSubject := subject.Clone()
adjustedSubject.Namespace = ""
adjustedSubjectSet.Add(adjustedSubject)

continue
}

adjustedSubjectSet.Add(subject)
}

return adjustedSubjectSet.ToSlice()
}

// SplitSubjectID returns the components of the ID
func SplitSubjectID(id string) (string, string, error) {
clusterEncoded, subjectEncoded := stringutils.Split2(id, ":")
Expand Down Expand Up @@ -64,7 +90,7 @@
func GetAllSubjects(bindings []*storage.K8SRoleBinding, kinds ...storage.SubjectKind) []*storage.Subject {
subjectsSet := NewSubjectSet()
for _, binding := range bindings {
for _, subject := range binding.GetSubjects() {
for _, subject := range GetSubjectsAdjustedByKind(binding) {

Check warning on line 93 in pkg/k8srbac/subjects.go

View check run for this annotation

Codecov / codecov/patch

pkg/k8srbac/subjects.go#L93 

Added line #L93 was not covered by tests
for _, kind := range kinds {
if subject.GetKind() == kind {
subjectsSet.Add(subject)
Expand All @@ -80,7 +106,7 @@
func GetSubject(subjectName string, bindings []*storage.K8SRoleBinding) (*storage.Subject, bool, error) {
// Find the subject we want.
for _, binding := range bindings {
for _, subject := range binding.GetSubjects() {
for _, subject := range GetSubjectsAdjustedByKind(binding) {

Check warning on line 109 in pkg/k8srbac/subjects.go

View check run for this annotation

Codecov / codecov/patch

pkg/k8srbac/subjects.go#L109 

Added line #L109 was not covered by tests
// We only want to look for a user or a group.
if subject.GetKind() != storage.SubjectKind_USER && subject.GetKind() != storage.SubjectKind_GROUP {
continue
Expand Down
106 changes: 106 additions & 0 deletions pkg/k8srbac/subjects_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,106 @@
package k8srbac

import (
"testing"

"github.com/stackrox/rox/generated/storage"
"github.com/stackrox/rox/pkg/protoassert"
)

func TestGetSubjectsAdjustedByKind(t *testing.T) {
tests := map[string]struct {
rb *storage.K8SRoleBinding
expect []*storage.Subject
}{
"nil rolebinding": {
rb: nil,
expect: nil,
},
"nil subjects": {
rb: &storage.K8SRoleBinding{},
expect: nil,
},
"empty subjects": {
rb: &storage.K8SRoleBinding{Subjects: make([]*storage.Subject, 0)},
expect: nil,
},
"all kinds supported": {
rb: &storage.K8SRoleBinding{Subjects: []*storage.Subject{
{Kind: storage.SubjectKind_UNSET_KIND},
{Kind: storage.SubjectKind_SERVICE_ACCOUNT},
{Kind: storage.SubjectKind_USER},
{Kind: storage.SubjectKind_GROUP},
}},
expect: []*storage.Subject{
{Kind: storage.SubjectKind_UNSET_KIND},
{Kind: storage.SubjectKind_SERVICE_ACCOUNT},
{Kind: storage.SubjectKind_USER},
{Kind: storage.SubjectKind_GROUP},
},
},
"namespaced kinds preserve namespace": {
rb: &storage.K8SRoleBinding{Subjects: []*storage.Subject{
{Kind: storage.SubjectKind_UNSET_KIND, Namespace: "namespace"},
{Kind: storage.SubjectKind_SERVICE_ACCOUNT, Namespace: "namespace"},
}},
expect: []*storage.Subject{
{Kind: storage.SubjectKind_UNSET_KIND, Namespace: "namespace"},
{Kind: storage.SubjectKind_SERVICE_ACCOUNT, Namespace: "namespace"},
},
},
"non-namespaced kinds are adjusted": {
rb: &storage.K8SRoleBinding{Subjects: []*storage.Subject{
{Kind: storage.SubjectKind_USER, Namespace: "namespace"},
{Kind: storage.SubjectKind_GROUP, Namespace: "namespace"},
}},
expect: []*storage.Subject{
{Kind: storage.SubjectKind_USER},
{Kind: storage.SubjectKind_GROUP},
},
},
"only non-namespaced kinds are adjusted for list of mixed kinds": {
rb: &storage.K8SRoleBinding{Subjects: []*storage.Subject{
{Kind: storage.SubjectKind_UNSET_KIND, Namespace: "namespace"},
{Kind: storage.SubjectKind_UNSET_KIND},
{Kind: storage.SubjectKind_SERVICE_ACCOUNT, Namespace: "namespace"},
{Kind: storage.SubjectKind_USER, Namespace: "namespace"},
{Kind: storage.SubjectKind_GROUP, Namespace: "namespace"},
}},
expect: []*storage.Subject{
{Kind: storage.SubjectKind_UNSET_KIND, Namespace: "namespace"},
{Kind: storage.SubjectKind_UNSET_KIND},
{Kind: storage.SubjectKind_SERVICE_ACCOUNT, Namespace: "namespace"},
{Kind: storage.SubjectKind_USER},
{Kind: storage.SubjectKind_GROUP},
},
},
"non-namespaced duplicates are adjusted": {
rb: &storage.K8SRoleBinding{Subjects: []*storage.Subject{
{Kind: storage.SubjectKind_USER, Namespace: "is-first-in-the-list"},
{Kind: storage.SubjectKind_USER},
{Kind: storage.SubjectKind_GROUP},
{Kind: storage.SubjectKind_GROUP, Namespace: "is-second-in-the-list"},
}},
expect: []*storage.Subject{
{Kind: storage.SubjectKind_USER},
{Kind: storage.SubjectKind_GROUP},
},
},
"only namespace removed": {
rb: &storage.K8SRoleBinding{Subjects: []*storage.Subject{
{Kind: storage.SubjectKind_USER, Name: "user-1", Namespace: "namespace", Id: "cluster-1:user-1", ClusterId: "cluster-1", ClusterName: "cluster-name"},
{Kind: storage.SubjectKind_GROUP, Name: "group-1", Namespace: "namespace", Id: "cluster-1:group-1", ClusterId: "cluster-1", ClusterName: "cluster-name"},
}},
expect: []*storage.Subject{
{Kind: storage.SubjectKind_USER, Name: "user-1", Id: "cluster-1:user-1", ClusterId: "cluster-1", ClusterName: "cluster-name"},
{Kind: storage.SubjectKind_GROUP, Name: "group-1", Id: "cluster-1:group-1", ClusterId: "cluster-1", ClusterName: "cluster-name"},
},
},
}

for testName, tt := range tests {
t.Run(testName, func(t *testing.T) {
protoassert.ElementsMatch(t, tt.expect, GetSubjectsAdjustedByKind(tt.rb))
})
}
}
5 changes: 2 additions & 3 deletions qa-tests-backend/src/test/groovy/K8sRbacTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,9 +309,8 @@ class K8sRbacTest extends BaseSpecification {
assert stackroxSubjects.size() == orchestratorSubjects.size()
for (Rbac.Subject sub : stackroxSubjects) {
K8sSubject subject = orchestratorSubjects.find {
it.name == sub.name &&
it.namespace == sub.namespace &&
it.kind.toLowerCase() == sub.kind.toString().toLowerCase()
// orchestratorSubjects contains only User and Group kind where namespace is not relevant.
it.name == sub.name && it.kind.toLowerCase() == sub.kind.toString().toLowerCase()
}
assert subject
}
Expand Down
Loading