Audit support resource wildcard matching #55306

hzxuzhonghu · 2017-11-08T08:30:46Z

What this PR does / why we need it:

audit policy support "resource/subresources" wildcard matching "resource/", "/subresource","*"

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #55305

Special notes for your reviewer:

Release note:

[advanced audit] support subresources wildcard matching.

hzxuzhonghu · 2017-11-08T08:34:10Z

/assign @sttts @crassirostris

hzxuzhonghu · 2017-11-08T08:47:25Z

/cc @CaoShuFeng

CaoShuFeng · 2017-11-08T09:05:04Z

/asign @ericchiang

CaoShuFeng · 2017-11-08T09:05:20Z

/ok-to-test

CaoShuFeng · 2017-11-08T09:07:18Z

staging/src/k8s.io/apiserver/pkg/apis/audit/types.go

@@ -220,8 +220,9 @@ type GroupResources struct {
 	// +optional
 	Group string
 	// Resources is a list of resources within the API group. Subresources are
-	// matched using a "/" to indicate the subresource. For example, "pods/log"
-	// would match request to the log subresource of pods. The top level resource
+	// matched using a "/" to indicate the subresource. Also "/*" can be used


Please run:
hack/update-all.sh

CaoShuFeng · 2017-11-08T09:10:44Z

staging/src/k8s.io/apiserver/pkg/audit/policy/checker.go

@@ -163,6 +161,11 @@ func ruleMatchesResource(r *audit.PolicyRule, attrs authorizer.Attributes) bool
 						return true
 					}
 				}
+				if strings.HasSuffix(res, "/*") {
+					if strings.HasPrefix(resource, strings.TrimRight(res, "/*")) {


Rather than check the prefix, I think this is more safe:
attrs.GetResource() == strings.TrimRight(res, "/*")

I think they are the same

resource := attrs.GetResource() // If subresource, the resource in the policy must match "(resource)/(subresource)" if sr := attrs.GetSubresource(); sr != "" { resource = resource + "/" + sr }

They are not 100% same.
What if user has such illegal resource in policy: "pods/log/*" ?

Should we support this illegal resource? I think we should not support wrong config.

Should we support this illegal resource? I think we should not support wrong config.

With attrs.GetResource() == strings.TrimRight(res, "/*") we can avoid this, so why not?

We validate policy format here:
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/apis/audit/validation/validation.go

I know your meaning .

CaoShuFeng · 2017-11-08T09:18:54Z

Just for reference:
We had some talk here #48836

CaoShuFeng · 2017-11-08T09:26:46Z

This is a change which has effect to users.
So, a release note would be better.

hzxuzhonghu · 2017-11-08T09:27:46Z

This is a change which has effect to users.
So, a release note would be better.

OK, thanks

ericchiang · 2017-11-08T22:35:39Z

FYI RBAC supports "*/(subresource)" for resources like "*/scale" but not "(resource)/*" because we didn't identify a valid use case.

What's the use case here? Subresources can have dramatically different characteristics.

hzxuzhonghu · 2017-11-09T01:32:54Z

FYI RBAC supports "/(subresource)" for resources like "/scale" but not "(resource)/*" because we didn't identify a valid use case.
What's the use case here? Subresources can have dramatically different characteristics.

How about "pods/log" "pods/status"? By the way, // TODO: consider adding options like "pods/*" to match all subresources.` exist .

hzxuzhonghu · 2017-11-09T01:33:25Z

/retest

hzxuzhonghu · 2017-11-09T02:03:56Z

@ericchiang Also I find in #48836 (comment)

ericchiang · 2017-11-09T04:44:50Z

How about "pods/log" "pods/status"?

This seems hypothetical. Why not be explicit and enumerate them? "pod/status" and "pod/exec" have dramatically different security implications. Are there any resources where this would be useful since almost everything else just has /status and /scale.

Not that this couldn't be useful, but do we even want to encourage pods/*? Which seems like obvious way to use this feature.

hzxuzhonghu · 2017-11-09T06:14:46Z

@ericchiang Compared to other policy rules,such as rbac/initializer, etc. They all have Resources, but have little difference, some match "/subresource", some match "resource/" and some match "". I think we should support "resource/" wildcard matching at least, if not match all patterns.

crassirostris · 2018-01-10T13:42:59Z

Contents of the PR look reasonable, but the implications of this change are questionable from the security perspective

deads2k · 2018-01-10T14:46:33Z

FYI RBAC supports "/(subresource)" for resources like "/scale" but not "(resource)/*" because we didn't identify a valid use case.

What's the use case here? Subresources can have dramatically different characteristics.

@ericchiang This is a strong point for RBAC, but is it as strong an argument for auditing? For authorization decisions, we don't want to accidentally expose anything, but for auditing there's an argument for "I want to record all activity on pods". If a new subresource is added for pods, I probably do want to start recording activity for it.

The power to use those endpoints (RBAC) should remain an explicit opt-in, but the power to record activity on those endpoints could reasonably desire to default on.

ericchiang · 2018-01-11T00:02:00Z

The power to use those endpoints (RBAC) should remain an explicit opt-in, but the power to record activity on those endpoints could reasonably desire to default on.

@deads2k sure I can go for that.

soltysh · 2018-01-16T11:17:31Z

Imagine a new subresource comes around that's not covered by this policy. It requires manual intervention to silence this resource, which is far from optimal.

I was about to stress what @deads2k wrote above. If a new subresource comes in, you do want to know about it and react. I agree with majority here, I don't think there's a strong use-case for this. None of the resources we have already has that many subresources that it's a problem to list them. I'm in the camp for 'explicit is better than implicit'.

hzxuzhonghu · 2018-01-16T11:26:50Z

@soltysh audit is different from RBAC, I think @deads2k mean to support it.

The power to use those endpoints (RBAC) should remain an explicit opt-in, but the power to record activity on those endpoints could reasonably desire to default on.

hzxuzhonghu · 2018-01-16T11:27:53Z

/assign @deads2k

hzxuzhonghu · 2018-01-25T04:00:30Z

Does this make sense, if so we should take it a step forward.

hzxuzhonghu · 2018-01-30T06:34:06Z

@deads2k @ericchiang @soltysh Have you achieve an consensus? I would like this pr being merged in v1.10.

soltysh · 2018-02-07T19:06:06Z

@deads2k kind ping?

deads2k · 2018-02-07T20:32:33Z

@deads2k @ericchiang @soltysh Have you achieve an consensus? I would like this pr being merged in v1.10.

I think we're there. pods/* is ok for audit, but not ok for rbac

ericchiang · 2018-02-08T00:22:44Z

staging/src/k8s.io/apiserver/pkg/apis/audit/types.go

+	// 'pods/*' means all subresources of pods.
+	// '*/scale' means all scale subresources.
+	//
+	// If wildcard is present, the validation rule will ensure resources do not


If wildcard is present, the validation rule will ensure resources do not overlap with each other.

A little confused by this statement. What's it trying to indicate?

It means Resources []string elements should not indicate same resource.

What's wrong with when they overlap?

I don't see any validation you're talking about.

Yeah I don't see this either. Can you drop this sentence?

ericchiang · 2018-02-08T00:25:54Z

staging/src/k8s.io/apiserver/pkg/apis/audit/types.go

+	// Resources is a list of resources this rule applies to.
+	//
+	// For example:
+	// 'pods' means pods.


nit: Use "matches" instead of "means"?

'pods' matches requests to pods

'pods/logs' matches requests to the logs subresource of pods

'*' matches requests to all resources and subresources

'pods/*' matches requests to all subresources of pods

'*/scale' matches requests to the scale subresource of any resource

ericchiang · 2018-02-08T00:26:43Z

Just doc questions, other than that lgtm

soltysh

/lgtm

soltysh · 2018-02-08T12:47:22Z

staging/src/k8s.io/apiserver/pkg/apis/audit/types.go

+	// 'pods/*' means all subresources of pods.
+	// '*/scale' means all scale subresources.
+	//
+	// If wildcard is present, the validation rule will ensure resources do not


What's wrong with when they overlap?

soltysh · 2018-02-08T12:48:56Z

staging/src/k8s.io/apiserver/pkg/apis/audit/types.go

+	// 'pods/*' means all subresources of pods.
+	// '*/scale' means all scale subresources.
+	//
+	// If wildcard is present, the validation rule will ensure resources do not


I don't see any validation you're talking about.

hzxuzhonghu · 2018-02-09T09:53:27Z

@deads2k for approval.

hzxuzhonghu · 2018-02-12T13:41:56Z

/retest
@deads2k need your approve.

sttts · 2018-02-13T12:53:08Z

/approve

k8s-ci-robot · 2018-02-13T12:53:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hzxuzhonghu, soltysh, sttts

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these OWNERS Files:

~~staging/src/k8s.io/apiserver/OWNERS~~ [sttts]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-github-robot · 2018-02-13T12:53:43Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-02-13T13:38:25Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

hzxuzhonghu changed the title ~~Audit~~ Audit support subresources wildcard matching Nov 8, 2017

k8s-ci-robot assigned crassirostris and sttts Nov 8, 2017

k8s-ci-robot requested a review from CaoShuFeng November 8, 2017 08:47

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Nov 8, 2017

CaoShuFeng reviewed Nov 8, 2017

View reviewed changes

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Nov 8, 2017

hzxuzhonghu force-pushed the audit branch from 704d299 to 2bec17d Compare November 8, 2017 09:40

hzxuzhonghu force-pushed the audit branch 3 times, most recently from 7011b3a to 3e0963c Compare November 9, 2017 07:36

crassirostris mentioned this pull request Jan 10, 2018

Advanced Auditing 1.10 umbrella bug #58083

Closed

11 tasks

k8s-ci-robot assigned deads2k Jan 16, 2018

ericchiang reviewed Feb 8, 2018

View reviewed changes

hzxuzhonghu added 3 commits February 8, 2018 09:41

audit support wildcard matching subresources

6e83d88

add test case

cc135e9

run hack/update-all.sh

08c024f

hzxuzhonghu force-pushed the audit branch from 5da96b2 to 08c024f Compare February 8, 2018 01:42

soltysh approved these changes Feb 8, 2018

View reviewed changes

k8s-ci-robot assigned soltysh Feb 8, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 8, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 13, 2018

k8s-github-robot merged commit 10f2544 into kubernetes:master Feb 13, 2018

Audit support resource wildcard matching #55306

Audit support resource wildcard matching #55306

Conversation

hzxuzhonghu commented Nov 8, 2017 • edited Loading

hzxuzhonghu commented Nov 8, 2017

hzxuzhonghu commented Nov 8, 2017

CaoShuFeng commented Nov 8, 2017

CaoShuFeng commented Nov 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CaoShuFeng commented Nov 8, 2017 • edited Loading

CaoShuFeng commented Nov 8, 2017

hzxuzhonghu commented Nov 8, 2017

ericchiang commented Nov 8, 2017 • edited Loading

hzxuzhonghu commented Nov 9, 2017

hzxuzhonghu commented Nov 9, 2017

hzxuzhonghu commented Nov 9, 2017

ericchiang commented Nov 9, 2017 • edited Loading

hzxuzhonghu commented Nov 9, 2017 • edited Loading

crassirostris commented Jan 10, 2018

deads2k commented Jan 10, 2018

ericchiang commented Jan 11, 2018

soltysh commented Jan 16, 2018

hzxuzhonghu commented Jan 16, 2018

hzxuzhonghu commented Jan 16, 2018

hzxuzhonghu commented Jan 25, 2018

hzxuzhonghu commented Jan 30, 2018

soltysh commented Feb 7, 2018

deads2k commented Feb 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericchiang Feb 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericchiang commented Feb 8, 2018

soltysh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hzxuzhonghu commented Feb 9, 2018

hzxuzhonghu commented Feb 12, 2018

sttts commented Feb 13, 2018

k8s-ci-robot commented Feb 13, 2018

k8s-github-robot commented Feb 13, 2018

k8s-github-robot commented Feb 13, 2018

hzxuzhonghu commented Nov 8, 2017 •

edited

Loading

CaoShuFeng commented Nov 8, 2017 •

edited

Loading

ericchiang commented Nov 8, 2017 •

edited

Loading

ericchiang commented Nov 9, 2017 •

edited

Loading

hzxuzhonghu commented Nov 9, 2017 •

edited

Loading

ericchiang Feb 8, 2018 •

edited

Loading