Skip to content

Commit

Permalink
[kube] fix greedy deny rule blocking namespace list when blocking oth…
Browse files Browse the repository at this point in the history 
…er 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.
  • Loading branch information
@tigrato
tigrato committed Aug 1, 2024
1 parent 3c2dbf0 commit 0b875e1
Show file tree
Hide file tree
Showing 4 changed files with 143 additions and 9 deletions.
4 changes: 2 additions & 2 deletions lib/kube/proxy/forwarder.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
2 changes: 1 addition & 1 deletion lib/services/role.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
10 changes: 6 additions & 4 deletions lib/utils/replace.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,14 +149,13 @@ 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) {
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.
Expand Down Expand Up @@ -197,6 +196,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)
}
Expand Down Expand Up @@ -250,7 +253,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
Expand Down Expand Up @@ -284,7 +286,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
}
Expand Down
136 changes: 134 additions & 2 deletions lib/utils/replace_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}{
Expand All @@ -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{
Expand All @@ -199,6 +275,7 @@ func TestKubeResourceMatchesRegex(t *testing.T) {
},
},
assert: require.NoError,
action: types.Allow,
matches: true,
},
{
Expand All @@ -218,6 +295,7 @@ func TestKubeResourceMatchesRegex(t *testing.T) {
},
},
assert: require.NoError,
action: types.Allow,
matches: true,
},
{
Expand All @@ -237,6 +315,7 @@ func TestKubeResourceMatchesRegex(t *testing.T) {
},
},
assert: require.NoError,
action: types.Allow,
matches: false,
},
{
Expand All @@ -255,6 +334,7 @@ func TestKubeResourceMatchesRegex(t *testing.T) {
},
},
assert: require.NoError,
action: types.Allow,
matches: false,
},
{
Expand Down Expand Up @@ -286,6 +366,7 @@ func TestKubeResourceMatchesRegex(t *testing.T) {
},
},
assert: require.NoError,
action: types.Allow,
matches: true,
},
{
Expand All @@ -305,6 +386,7 @@ func TestKubeResourceMatchesRegex(t *testing.T) {
},
},
assert: require.NoError,
action: types.Allow,
matches: true,
},
{
Expand All @@ -324,6 +406,7 @@ func TestKubeResourceMatchesRegex(t *testing.T) {
},
},
assert: require.NoError,
action: types.Allow,
matches: false,
},
{
Expand All @@ -342,6 +425,7 @@ func TestKubeResourceMatchesRegex(t *testing.T) {
Verbs: []string{types.Wildcard},
},
},
action: types.Allow,
assert: require.Error,
},
{
Expand All @@ -359,6 +443,7 @@ func TestKubeResourceMatchesRegex(t *testing.T) {
Name: "podname",
},
},
action: types.Allow,
assert: require.NoError,
},
{
Expand All @@ -376,6 +461,7 @@ func TestKubeResourceMatchesRegex(t *testing.T) {
},
},
assert: require.NoError,
action: types.Allow,
matches: true,
},
{
Expand All @@ -394,6 +480,7 @@ func TestKubeResourceMatchesRegex(t *testing.T) {
},
},
assert: require.NoError,
action: types.Allow,
matches: true,
},
{
Expand All @@ -412,6 +499,7 @@ func TestKubeResourceMatchesRegex(t *testing.T) {
},
},
assert: require.NoError,
action: types.Allow,
matches: false,
},
{
Expand All @@ -430,6 +518,7 @@ func TestKubeResourceMatchesRegex(t *testing.T) {
},
},
assert: require.NoError,
action: types.Allow,
matches: true,
},

Expand All @@ -449,6 +538,7 @@ func TestKubeResourceMatchesRegex(t *testing.T) {
},
},
assert: require.NoError,
action: types.Allow,
matches: false,
},

Expand All @@ -468,6 +558,7 @@ func TestKubeResourceMatchesRegex(t *testing.T) {
},
},
assert: require.NoError,
action: types.Allow,
matches: true,
},
{
Expand All @@ -486,6 +577,7 @@ func TestKubeResourceMatchesRegex(t *testing.T) {
},
},
assert: require.NoError,
action: types.Allow,
matches: false,
},

Expand All @@ -505,6 +597,7 @@ func TestKubeResourceMatchesRegex(t *testing.T) {
},
},
assert: require.NoError,
action: types.Allow,
matches: true,
},
{
Expand All @@ -523,6 +616,7 @@ func TestKubeResourceMatchesRegex(t *testing.T) {
},
},
assert: require.NoError,
action: types.Allow,
matches: false,
},
{
Expand All @@ -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)
})
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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{
Expand Down

0 comments on commit 0b875e1

Please sign in to comment.