Skip to content

ROX-27053: Fix subjects handling for user and group kind #13410

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
mtodor merged 2 commits into from
Dec 4, 2024
Merged

ROX-27053: Fix subjects handling for user and group kind #13410

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
d645526
Improve handling of non-namespaces subjects
mtodor Nov 22, 2024
d62f2c1
Add unit tests for pipeline
mtodor Nov 27, 2024
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 @@ func CheckVolumeAccessIsLimited(ctx framework.ComplianceContext) {
// 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) {
if subject.GetKind() == storage.SubjectKind_SERVICE_ACCOUNT {
allServiceAccounts.Add(subject)
}
Expand Down Expand Up @@ -69,7 +69,7 @@ func CheckVolumeAccessIsLimited(ctx framework.ComplianceContext) {
// 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) {
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 LimitedUsersAndGroupsWithClusterAdmin(ctx framework.ComplianceContext) {
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) {
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 @@ func getSubjectFromStores(ctx context.Context, subjectName string, roleDS k8sRol
}

var subject *storage.Subject
for _, subj := range relevantBindings[0].GetSubjects() {
for _, subj := range k8srbac.GetSubjectsAdjustedByKind(relevantBindings[0]) {
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 @@ func getSubjectsGrantedClusterAdmin(roles []*storage.K8SRole, roleBindings []*st
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 @@ func isSubjectClusterAdmin(subject *storage.Subject, roles []*storage.K8SRole, r
subjectsWithClusterAdmin := NewSubjectSet()
for _, binding := range roleBindings {
if clusterAdminRoleIDs.Contains(binding.GetRoleId()) {
subjectsWithClusterAdmin.Add(binding.GetSubjects()...)
subjectsWithClusterAdmin.Add(GetSubjectsAdjustedByKind(binding)...)
}
}
for _, s := range subjectsWithClusterAdmin.ToSlice() {
Expand Down Expand Up @@ -116,7 +116,7 @@ func buildMap(roles []*storage.K8SRole, bindings []*storage.K8SRoleBinding) map[
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 @@ func CreateSubjectID(clusterID, subjectName string) string {
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.CloneVT()
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 GetSubjectForServiceAccount(sa *storage.ServiceAccount) *storage.Subject {
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) {
for _, kind := range kinds {
if subject.GetKind() == kind {
subjectsSet.Add(subject)
Expand All @@ -80,7 +106,7 @@ func GetAllSubjects(bindings []*storage.K8SRoleBinding, kinds ...storage.Subject
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) {
// 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 @@ -304,9 +304,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