Skip to content

Commit

Permalink
Always collect deny arm of kubernetes_resources (#28275)
Browse files Browse the repository at this point in the history
Denied `kubernetes_resources` weren't properly loaded when collecting
resources for list filtering.

This PR collects all deny arms - if the `kubernetes_labels` match the
cluster, the access is denied - of Kubernetes resources.

Fixes #28262
  • Loading branch information
tigrato committed Jun 26, 2023
1 parent 3f852e6 commit af404e3
Show file tree
Hide file tree
Showing 3 changed files with 61 additions and 5 deletions.
54 changes: 54 additions & 0 deletions lib/kube/proxy/pod_rbac_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ func TestListPodRBAC(t *testing.T) {
usernameWithFullAccess = "full_user"
usernameWithNamespaceAccess = "default_user"
usernameWithLimitedAccess = "limited_user"
usernameWithDenyRule = "denied_user"
testPodName = "test"
)
// kubeMock is a Kubernetes API mock for the session tests.
Expand Down Expand Up @@ -141,6 +142,39 @@ func TestListPodRBAC(t *testing.T) {
},
)

// create a user allowed to access all namespaces except the default namespace.
// (kubernetes_user and kubernetes_groups specified)
userWithDenyRule, _ := testCtx.CreateUserAndRole(
testCtx.Context,
t,
usernameWithDenyRule,
RoleSpec{
Name: usernameWithDenyRule,
KubeUsers: roleKubeUsers,
KubeGroups: roleKubeGroups,
SetupRoleFunc: func(r types.Role) {
r.SetKubeResources(types.Allow,
[]types.KubernetesResource{
{
Kind: types.KindKubePod,
Name: types.Wildcard,
Namespace: types.Wildcard,
},
},
)
r.SetKubeResources(types.Deny,
[]types.KubernetesResource{
{
Kind: types.KindKubePod,
Name: types.Wildcard,
Namespace: metav1.NamespaceDefault,
},
},
)
},
},
)

type args struct {
user types.User
namespace string
Expand Down Expand Up @@ -234,6 +268,26 @@ func TestListPodRBAC(t *testing.T) {
},
},
},
{
name: "list pods in every namespace for user with default namespace deny rule",
args: args{
user: userWithDenyRule,
},
want: want{
listPodsResult: []string{
"dev/nginx-1",
"dev/nginx-2",
},
getTestPodResult: &kubeerrors.StatusError{
ErrStatus: metav1.Status{
Status: "Failure",
Message: "pods \"test\" is forbidden: User \"denied_user\" cannot get resource \"pods\" in API group \"\" in the namespace \"default\"",
Code: 403,
Reason: metav1.StatusReasonForbidden,
},
},
},
},
{
name: "list default namespace pods for user with limited access and a resource access request",
args: args{
Expand Down
11 changes: 6 additions & 5 deletions lib/services/role.go
Original file line number Diff line number Diff line change
Expand Up @@ -2815,11 +2815,12 @@ func (set RoleSet) GetKubeResources(cluster types.KubeCluster, userTraits wrappe
}

for _, role := range set {
matchLabels, _, err := checkRoleLabelsMatch(types.Deny, role, userTraits, cluster, false)
if err != nil || !matchLabels {
continue
}

// deny rules are not checked for labels because they are greedy. It means that
// if there is a deny rule for a cluster, it will deny access to all resources
// in that cluster, regardless of kubernetes_resources (i.e. making them irrelevant).
// If the goal is to deny access to a specific resource, it should be done by collecting
// all kube resources in deny rules and ignoring if the role matches or not
// the cluster (i.e. no labels check).
denied = append(denied, role.GetKubeResources(types.Deny)...)
}

Expand Down
1 change: 1 addition & 0 deletions lib/services/role_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6662,6 +6662,7 @@ func TestGetKubeResources(t *testing.T) {
},
clusterLabels: map[string]string{"env": "staging"},
expectAllowed: []types.KubernetesResource{podA, podB},
expectDenied: []types.KubernetesResource{podA},
},
} {
t.Run(tc.desc, func(t *testing.T) {
Expand Down

0 comments on commit af404e3

Please sign in to comment.