From 37fadae8b7990121d077167290f8000504e2a681 Mon Sep 17 00:00:00 2001 From: Tiago Silva Date: Wed, 31 Jul 2024 18:54:44 +0100 Subject: [PATCH 1/3] [kube] fix greedy deny rule blocking namespace list when blocking other resources This PR fixes an edge case where the deny rule for blocking access to a resource becomes greedy and blocks access to the whole namespace. eg: ``` allow: kubernetes_labels: '*': '*' kubernetes_resources: - kind: '*' name: '*' namespace: '*' verbs: - '*' deny: kubernetes_resources: - kind: secret name: '*' namespace: '*' verbs: - '*' ``` With the example above, access to secrets must be blocked but the user is allowed to access every other resource in any namespace. The previous model was greedy and blocked access to namespace list. --- lib/kube/proxy/forwarder.go | 4 +- lib/services/role.go | 2 +- lib/utils/replace.go | 11 +-- lib/utils/replace_test.go | 136 +++++++++++++++++++++++++++++++++++- 4 files changed, 144 insertions(+), 9 deletions(-) diff --git a/lib/kube/proxy/forwarder.go b/lib/kube/proxy/forwarder.go index 0d6b547bfb7be..1ef3671ff6e64 100644 --- a/lib/kube/proxy/forwarder.go +++ b/lib/kube/proxy/forwarder.go @@ -1134,14 +1134,14 @@ func matchKubernetesResource(resource types.KubernetesResource, allowed, denied // utils.KubeResourceMatchesRegex checks if the resource.Kind is strictly equal // to each entry and validates if the Name and Namespace fields matches the // regex allowed by each entry. - result, err := utils.KubeResourceMatchesRegex(resource, denied) + result, err := utils.KubeResourceMatchesRegex(resource, denied, types.Deny) if err != nil { return false, trace.Wrap(err) } else if result { return false, nil } - result, err = utils.KubeResourceMatchesRegex(resource, allowed) + result, err = utils.KubeResourceMatchesRegex(resource, allowed, types.Allow) if err != nil { return false, trace.Wrap(err) } diff --git a/lib/services/role.go b/lib/services/role.go index 8c760d56e98e6..21589e2b41cc3 100644 --- a/lib/services/role.go +++ b/lib/services/role.go @@ -2421,7 +2421,7 @@ func NewKubernetesResourceMatcher(resource types.KubernetesResource) *Kubernetes // Match matches a Kubernetes Resource against provided role and condition. func (m *KubernetesResourceMatcher) Match(role types.Role, condition types.RoleConditionType) (bool, error) { - result, err := utils.KubeResourceMatchesRegex(m.resource, role.GetKubeResources(condition)) + result, err := utils.KubeResourceMatchesRegex(m.resource, role.GetKubeResources(condition), condition) return result, trace.Wrap(err) } diff --git a/lib/utils/replace.go b/lib/utils/replace.go index cf3448247e6f0..ca5c177fc53f1 100644 --- a/lib/utils/replace.go +++ b/lib/utils/replace.go @@ -149,14 +149,14 @@ const ( // input is the resource we are checking for access. // resources is a list of resources that the user has access to - collected from // their roles that match the Kubernetes cluster where the resource is defined. -func KubeResourceMatchesRegex(input types.KubernetesResource, resources []types.KubernetesResource) (bool, error) { +// cond is the deny or allow condition of the role that we are evaluating. +func KubeResourceMatchesRegex(input types.KubernetesResource, resources []types.KubernetesResource, cond types.RoleConditionType) (bool, error) { if len(input.Verbs) != 1 { return false, trace.BadParameter("only one verb is supported, input: %v", input.Verbs) } // isClusterWideResource is true if the resource is cluster-wide, e.g. a // namespace resource or a clusterrole. isClusterWideResource := slices.Contains(types.KubernetesClusterWideResourceKinds, input.Kind) - verb := input.Verbs[0] // If the user is list/read/watch a namespace, they should be able to see the // namespace they have resources defined for. @@ -197,6 +197,10 @@ func KubeResourceMatchesRegex(input types.KubernetesResource, resources []types. // This means that if the user has access to pods in the "foo" namespace, // they should be able to see the "foo" namespace in the list of namespaces // but only if the request is read-only. + isDeny := cond == types.Deny + if isDeny && resource.Kind != types.Wildcard { + continue + } if ok, err := MatchString(input.Name, resource.Namespace); err != nil || ok { return ok, trace.Wrap(err) } @@ -250,7 +254,6 @@ func KubeResourceCouldMatchRules(input types.KubernetesResource, resources []typ // permissions for. targetsReadOnlyNamespace := input.Kind == types.KindKubeNamespace && slices.Contains([]string{types.KubeVerbGet, types.KubeVerbList, types.KubeVerbWatch}, verb) - for _, resource := range resources { // If the resource has a wildcard verb, it matches all verbs. // Otherwise, the resource must have the verb we're looking for otherwise @@ -284,7 +287,7 @@ func KubeResourceCouldMatchRules(input types.KubernetesResource, resources []typ // This means that if the user has access to pods in the "foo" namespace, // they should be able to see the "foo" namespace in the list of namespaces // but only if the request is read-only. - isAllowOrFullDeny := !isDeny || resource.Name == types.Wildcard && resource.Namespace == types.Wildcard + isAllowOrFullDeny := !isDeny || resource.Name == types.Wildcard && resource.Namespace == types.Wildcard && resource.Kind == types.Wildcard if isAllowOrFullDeny { return isAllowOrFullDeny, nil } diff --git a/lib/utils/replace_test.go b/lib/utils/replace_test.go index 1c424ec2f30a7..7a844e819a232 100644 --- a/lib/utils/replace_test.go +++ b/lib/utils/replace_test.go @@ -162,6 +162,7 @@ func TestKubeResourceMatchesRegex(t *testing.T) { name string input types.KubernetesResource resources []types.KubernetesResource + action types.RoleConditionType matches bool assert require.ErrorAssertionFunc }{ @@ -180,8 +181,83 @@ func TestKubeResourceMatchesRegex(t *testing.T) { }, }, assert: require.Error, + action: types.Allow, + matches: false, + }, + { + name: "list namespace matches resource", + input: types.KubernetesResource{ + Kind: types.KindNamespace, + Verbs: []string{types.KubeVerbList}, + }, + resources: []types.KubernetesResource{ + { + Kind: types.KindKubeSecret, + Namespace: "*", + Name: "*", + Verbs: []string{types.Wildcard}, + }, + }, + assert: require.NoError, + action: types.Allow, + matches: true, + }, + { + name: "list namespace doesn't match denying secrets", + input: types.KubernetesResource{ + Kind: types.KindNamespace, + Verbs: []string{types.KubeVerbList}, + }, + resources: []types.KubernetesResource{ + { + Kind: types.KindKubeSecret, + Namespace: "*", + Name: "*", + Verbs: []string{types.Wildcard}, + }, + }, + assert: require.NoError, + action: types.Deny, matches: false, }, + { + name: "get namespace doesn't match denying secrets", + input: types.KubernetesResource{ + Kind: types.KindNamespace, + Name: "default", + Verbs: []string{types.KubeVerbGet}, + }, + resources: []types.KubernetesResource{ + { + Kind: types.KindKubeSecret, + Namespace: "*", + Name: "*", + Verbs: []string{types.Wildcard}, + }, + }, + assert: require.NoError, + action: types.Deny, + matches: false, + }, + { + name: "get secret matches denying secrets", + input: types.KubernetesResource{ + Kind: types.KindKubeSecret, + Name: "default", + Verbs: []string{types.KubeVerbGet}, + }, + resources: []types.KubernetesResource{ + { + Kind: types.KindKubeSecret, + Namespace: "*", + Name: "*", + Verbs: []string{types.Wildcard}, + }, + }, + assert: require.NoError, + action: types.Deny, + matches: true, + }, { name: "input matches single resource with wildcard verb", input: types.KubernetesResource{ @@ -199,6 +275,7 @@ func TestKubeResourceMatchesRegex(t *testing.T) { }, }, assert: require.NoError, + action: types.Allow, matches: true, }, { @@ -218,6 +295,7 @@ func TestKubeResourceMatchesRegex(t *testing.T) { }, }, assert: require.NoError, + action: types.Allow, matches: true, }, { @@ -237,6 +315,7 @@ func TestKubeResourceMatchesRegex(t *testing.T) { }, }, assert: require.NoError, + action: types.Allow, matches: false, }, { @@ -255,6 +334,7 @@ func TestKubeResourceMatchesRegex(t *testing.T) { }, }, assert: require.NoError, + action: types.Allow, matches: false, }, { @@ -286,6 +366,7 @@ func TestKubeResourceMatchesRegex(t *testing.T) { }, }, assert: require.NoError, + action: types.Allow, matches: true, }, { @@ -305,6 +386,7 @@ func TestKubeResourceMatchesRegex(t *testing.T) { }, }, assert: require.NoError, + action: types.Allow, matches: true, }, { @@ -324,6 +406,7 @@ func TestKubeResourceMatchesRegex(t *testing.T) { }, }, assert: require.NoError, + action: types.Allow, matches: false, }, { @@ -342,6 +425,7 @@ func TestKubeResourceMatchesRegex(t *testing.T) { Verbs: []string{types.Wildcard}, }, }, + action: types.Allow, assert: require.Error, }, { @@ -359,6 +443,7 @@ func TestKubeResourceMatchesRegex(t *testing.T) { Name: "podname", }, }, + action: types.Allow, assert: require.NoError, }, { @@ -376,6 +461,7 @@ func TestKubeResourceMatchesRegex(t *testing.T) { }, }, assert: require.NoError, + action: types.Allow, matches: true, }, { @@ -394,6 +480,7 @@ func TestKubeResourceMatchesRegex(t *testing.T) { }, }, assert: require.NoError, + action: types.Allow, matches: true, }, { @@ -412,6 +499,7 @@ func TestKubeResourceMatchesRegex(t *testing.T) { }, }, assert: require.NoError, + action: types.Allow, matches: false, }, { @@ -430,6 +518,7 @@ func TestKubeResourceMatchesRegex(t *testing.T) { }, }, assert: require.NoError, + action: types.Allow, matches: true, }, @@ -449,6 +538,7 @@ func TestKubeResourceMatchesRegex(t *testing.T) { }, }, assert: require.NoError, + action: types.Allow, matches: false, }, @@ -468,6 +558,7 @@ func TestKubeResourceMatchesRegex(t *testing.T) { }, }, assert: require.NoError, + action: types.Allow, matches: true, }, { @@ -486,6 +577,7 @@ func TestKubeResourceMatchesRegex(t *testing.T) { }, }, assert: require.NoError, + action: types.Allow, matches: false, }, @@ -505,6 +597,7 @@ func TestKubeResourceMatchesRegex(t *testing.T) { }, }, assert: require.NoError, + action: types.Allow, matches: true, }, { @@ -523,6 +616,7 @@ func TestKubeResourceMatchesRegex(t *testing.T) { }, }, assert: require.NoError, + action: types.Allow, matches: false, }, { @@ -547,12 +641,13 @@ func TestKubeResourceMatchesRegex(t *testing.T) { }, }, assert: require.NoError, + action: types.Allow, matches: false, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := KubeResourceMatchesRegex(tt.input, tt.resources) + got, err := KubeResourceMatchesRegex(tt.input, tt.resources, tt.action) tt.assert(t, err) require.Equal(t, tt.matches, got) }) @@ -619,6 +714,42 @@ func TestKubeResourceCouldMatchRules(t *testing.T) { assert: require.NoError, matches: true, }, + { + name: "input doesn't match kind deny", + input: types.KubernetesResource{ + Kind: types.KindNamespace, + Verbs: []string{types.KubeVerbList}, + }, + resources: []types.KubernetesResource{ + { + Kind: types.KindKubeSecret, + Namespace: "*", + Name: "*", + Verbs: []string{types.Wildcard}, + }, + }, + action: types.Deny, + assert: require.NoError, + matches: false, + }, + { + name: "input doesn't match kind allow", + input: types.KubernetesResource{ + Kind: types.KindNamespace, + Verbs: []string{types.KubeVerbList}, + }, + resources: []types.KubernetesResource{ + { + Kind: types.KindKubeSecret, + Namespace: "*", + Name: "*", + Verbs: []string{types.Wildcard}, + }, + }, + action: types.Allow, + assert: require.NoError, + matches: true, + }, { name: "input matches single resource with wildcard verb", input: types.KubernetesResource{ @@ -1030,9 +1161,10 @@ func TestKubeResourceCouldMatchRules(t *testing.T) { }, }, assert: require.NoError, - matches: true, + matches: false, action: types.Deny, }, + { name: "list namespace with resource denying update access to namespace", input: types.KubernetesResource{ From efcae62d76acd2be4d98e1dc5941fd126bb027fe Mon Sep 17 00:00:00 2001 From: Tiago Silva Date: Thu, 1 Aug 2024 16:01:01 +0100 Subject: [PATCH 2/3] add extra test --- lib/utils/replace_test.go | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/lib/utils/replace_test.go b/lib/utils/replace_test.go index 7a844e819a232..e2583d4606500 100644 --- a/lib/utils/replace_test.go +++ b/lib/utils/replace_test.go @@ -220,6 +220,25 @@ func TestKubeResourceMatchesRegex(t *testing.T) { action: types.Deny, matches: false, }, + { + name: "get namespace match denying everything", + input: types.KubernetesResource{ + Kind: types.KindNamespace, + Name: "default", + Verbs: []string{types.KubeVerbGet}, + }, + resources: []types.KubernetesResource{ + { + Kind: types.Wildcard, + Namespace: types.Wildcard, + Name: types.Wildcard, + Verbs: []string{types.Wildcard}, + }, + }, + assert: require.NoError, + action: types.Deny, + matches: true, + }, { name: "get namespace doesn't match denying secrets", input: types.KubernetesResource{ From 324d7759ceb0791eabc9f33ddd9bc423a23a87ef Mon Sep 17 00:00:00 2001 From: Tiago Silva Date: Thu, 1 Aug 2024 16:18:15 +0100 Subject: [PATCH 3/3] handle comments --- lib/utils/replace.go | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/lib/utils/replace.go b/lib/utils/replace.go index ca5c177fc53f1..686c39127d532 100644 --- a/lib/utils/replace.go +++ b/lib/utils/replace.go @@ -191,16 +191,12 @@ func KubeResourceMatchesRegex(input types.KubernetesResource, resources []types. if ok, err := MatchString(input.Namespace, resource.Name); err != nil || ok { return ok, trace.Wrap(err) } - case targetsReadOnlyNamespace && resource.Kind != types.KindKubeNamespace && resource.Namespace != "": + case targetsReadOnlyNamespace && cond == types.Allow && resource.Kind != types.KindKubeNamespace && resource.Namespace != "": // If the user requests a read-only namespace get/list/watch, they should // be able to see the list of namespaces they have resources defined in. // This means that if the user has access to pods in the "foo" namespace, // they should be able to see the "foo" namespace in the list of namespaces // but only if the request is read-only. - isDeny := cond == types.Deny - if isDeny && resource.Kind != types.Wildcard { - continue - } if ok, err := MatchString(input.Name, resource.Namespace); err != nil || ok { return ok, trace.Wrap(err) } @@ -281,19 +277,13 @@ func KubeResourceCouldMatchRules(input types.KubernetesResource, resources []typ if ok, err := MatchString(input.Namespace, resource.Name); err != nil || ok && isAllowOrFullDeny { return isAllowOrFullDeny || isDeny, trace.Wrap(err) } - case targetsReadOnlyNamespace && resource.Kind != types.KindKubeNamespace && resource.Namespace != "": + case targetsReadOnlyNamespace && !isDeny && resource.Kind != types.KindKubeNamespace && resource.Namespace != "": // If the user requests a read-only namespace get/list/watch, they should // be able to see the list of namespaces they have resources defined in. // This means that if the user has access to pods in the "foo" namespace, // they should be able to see the "foo" namespace in the list of namespaces // but only if the request is read-only. - isAllowOrFullDeny := !isDeny || resource.Name == types.Wildcard && resource.Namespace == types.Wildcard && resource.Kind == types.Wildcard - if isAllowOrFullDeny { - return isAllowOrFullDeny, nil - } - if ok, err := MatchString(input.Name, resource.Namespace); err != nil || ok && isAllowOrFullDeny { - return ok && isAllowOrFullDeny, trace.Wrap(err) - } + return true, nil default: if input.Kind != resource.Kind && resource.Kind != types.Wildcard { continue