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.

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

Audit support resource wildcard matching #55306

Merged
merged 3 commits into from Feb 13, 2018

Conversation

@hzxuzhonghu
Member

hzxuzhonghu commented Nov 8, 2017

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 hzxuzhonghu changed the title from Audit to Audit support subresources wildcard matching Nov 8, 2017

@hzxuzhonghu

This comment has been minimized.

Member

hzxuzhonghu commented Nov 8, 2017

@hzxuzhonghu

This comment has been minimized.

Member

hzxuzhonghu commented Nov 8, 2017

@k8s-ci-robot k8s-ci-robot requested a review from CaoShuFeng Nov 8, 2017

@CaoShuFeng

This comment has been minimized.

Member

CaoShuFeng commented Nov 8, 2017

/asign @ericchiang

@CaoShuFeng

This comment has been minimized.

Member

CaoShuFeng commented Nov 8, 2017

/ok-to-test

@@ -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

This comment has been minimized.

@CaoShuFeng

CaoShuFeng Nov 8, 2017

Member

Please run:
hack/update-all.sh

This comment has been minimized.

@hzxuzhonghu
@@ -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, "/*")) {

This comment has been minimized.

@CaoShuFeng

CaoShuFeng Nov 8, 2017

Member

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

This comment has been minimized.

@hzxuzhonghu

hzxuzhonghu Nov 8, 2017

Member

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
	}

This comment has been minimized.

@CaoShuFeng

CaoShuFeng Nov 8, 2017

Member

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

This comment has been minimized.

@hzxuzhonghu

hzxuzhonghu Nov 8, 2017

Member

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

This comment has been minimized.

@CaoShuFeng

CaoShuFeng Nov 8, 2017

Member

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?

This comment has been minimized.

@hzxuzhonghu

hzxuzhonghu Nov 8, 2017

Member

I know your meaning .

@CaoShuFeng

This comment has been minimized.

Member

CaoShuFeng commented Nov 8, 2017

Just for reference:
We had some talk here #48836

@CaoShuFeng

This comment has been minimized.

Member

CaoShuFeng commented Nov 8, 2017

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

@hzxuzhonghu

This comment has been minimized.

Member

hzxuzhonghu commented Nov 8, 2017

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

OK, thanks

@ericchiang

This comment has been minimized.

Member

ericchiang commented Nov 8, 2017

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

This comment has been minimized.

Member

hzxuzhonghu commented Nov 9, 2017

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

This comment has been minimized.

Member

hzxuzhonghu commented Nov 9, 2017

/retest

@hzxuzhonghu

This comment has been minimized.

Member

hzxuzhonghu commented Nov 9, 2017

@ericchiang Also I find in #48836 (comment)

@ericchiang

This comment has been minimized.

Member

ericchiang commented Nov 9, 2017

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

This comment has been minimized.

Member

hzxuzhonghu commented Nov 9, 2017

@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.

@hzxuzhonghu hzxuzhonghu changed the title from Audit support subresources wildcard matching to Audit support resource wildcard matching Nov 9, 2017

@hzxuzhonghu

This comment has been minimized.

Member

hzxuzhonghu commented Nov 9, 2017

@ALL audit policy support resources multiple wildcard matching patterns resource/*, */subresource, *

@hzxuzhonghu

This comment has been minimized.

Member

hzxuzhonghu commented Nov 9, 2017

/test pull-kubernetes-e2e-gce

@ericchiang

This comment has been minimized.

Member

ericchiang commented Nov 9, 2017

cc @kubernetes/sig-auth-pr-reviews

In case any else wants to weigh in on this.

@crassirostris

This comment has been minimized.

Member

crassirostris commented Jan 10, 2018

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

@crassirostris crassirostris referenced this pull request Jan 10, 2018

Closed

Advanced Auditing 1.10 umbrella bug #58083

11 of 11 tasks complete
@deads2k

This comment has been minimized.

Contributor

deads2k commented Jan 10, 2018

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

This comment has been minimized.

Member

ericchiang commented Jan 11, 2018

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

This comment has been minimized.

Contributor

soltysh commented Jan 16, 2018

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

This comment has been minimized.

Member

hzxuzhonghu commented Jan 16, 2018

@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

This comment has been minimized.

Member

hzxuzhonghu commented Jan 16, 2018

/assign @deads2k

@hzxuzhonghu

This comment has been minimized.

Member

hzxuzhonghu commented Jan 25, 2018

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

@hzxuzhonghu

This comment has been minimized.

Member

hzxuzhonghu commented Jan 30, 2018

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

@soltysh

This comment has been minimized.

Contributor

soltysh commented Feb 7, 2018

@deads2k kind ping?

@deads2k

This comment has been minimized.

Contributor

deads2k commented Feb 7, 2018

@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

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

This comment has been minimized.

@ericchiang

ericchiang Feb 8, 2018

Member

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?

This comment has been minimized.

@hzxuzhonghu

hzxuzhonghu Feb 8, 2018

Member

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

This comment has been minimized.

@soltysh

soltysh Feb 8, 2018

Contributor

What's wrong with when they overlap?

This comment has been minimized.

@soltysh

soltysh Feb 8, 2018

Contributor

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

This comment has been minimized.

@ericchiang

ericchiang Feb 8, 2018

Member

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

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

This comment has been minimized.

@ericchiang

ericchiang Feb 8, 2018

Member

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

This comment has been minimized.

@hzxuzhonghu
@ericchiang

This comment has been minimized.

Member

ericchiang commented Feb 8, 2018

Just doc questions, other than that lgtm

@soltysh

soltysh approved these changes Feb 8, 2018

/lgtm

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

This comment has been minimized.

@soltysh

soltysh Feb 8, 2018

Contributor

What's wrong with when they overlap?

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

This comment has been minimized.

@soltysh

soltysh Feb 8, 2018

Contributor

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

@hzxuzhonghu

This comment has been minimized.

Member

hzxuzhonghu commented Feb 9, 2018

@deads2k for approval.

@hzxuzhonghu

This comment has been minimized.

Member

hzxuzhonghu commented Feb 12, 2018

/retest
@deads2k need your approve.

@sttts

This comment has been minimized.

Contributor

sttts commented Feb 13, 2018

/approve

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 13, 2018

[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:

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

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 13, 2018

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

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 13, 2018

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

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

13 of 14 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-verify
Details
cla/linuxfoundation hzxuzhonghu authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce-canary Skipped
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment