Skip to content
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

Dynamic Audit Policy #71230

Open
wants to merge 2 commits into
base: master
from

Conversation

@pbarker
Copy link
Contributor

pbarker commented Nov 19, 2018

What type of PR is this?
/kind feature

What this PR does / why we need it:
Adds policy rules to the AuditSink API object #70818

Special notes for your reviewer:
This PR is based off discussions with @lavalamp and @tallclair to make the audit policy more composable and readable for the API objects.

Does this PR introduce a user-facing change?:

Add AuditClass object that allows for fine grained filtering of audit events for AuditSink API objects
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Nov 19, 2018

Hi @pbarker. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@pbarker

This comment has been minimized.

Copy link
Contributor Author

pbarker commented Nov 19, 2018

/cc @tallclair @lavalamp @liggitt
/sig auth

@k8s-ci-robot k8s-ci-robot requested review from lavalamp and tallclair Nov 19, 2018

@pbarker

This comment has been minimized.

Copy link
Contributor Author

pbarker commented Nov 19, 2018

/cc @liggitt

@k8s-ci-robot k8s-ci-robot requested a review from liggitt Nov 19, 2018

@yue9944882

This comment has been minimized.

Copy link
Member

yue9944882 commented Nov 20, 2018

/ok-to-test

@pbarker

This comment has been minimized.

Copy link
Contributor Author

pbarker commented Nov 20, 2018

/retest

1 similar comment
@pbarker

This comment has been minimized.

Copy link
Contributor Author

pbarker commented Nov 20, 2018

/retest

@WanLinghao

This comment has been minimized.

Copy link
Contributor

WanLinghao commented Nov 22, 2018

/cc

@k8s-ci-robot k8s-ci-robot requested a review from WanLinghao Nov 22, 2018

@WanLinghao

This comment has been minimized.

Copy link
Contributor

WanLinghao commented Nov 22, 2018

Hello, is there any reasons to complicate the struct with the existence of ClassRule, why don't we put AuditClass directly in Policy?

@pbarker

This comment has been minimized.

Copy link
Contributor Author

pbarker commented Nov 22, 2018

@WanLinghao the idea was to make it composable so that Sinks could share the classes, and an app developer could create a class for their application and have it packaged up with everything else. This was just the first direction we wanted to explore. I'm building a little CRD version now to test out these ideas that will serve as input, I'll ping you with the repo once is live

@WanLinghao

This comment has been minimized.

Copy link
Contributor

WanLinghao commented Nov 22, 2018

@pbarker thank you!

@pbarker pbarker force-pushed the pbarker:audit-policy branch from a37d373 to 963844c Nov 26, 2018

@pbarker pbarker force-pushed the pbarker:audit-policy branch from 4d2d6ab to 1376d55 Feb 25, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Feb 25, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: pbarker
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: smarterclayton

If they are not already assigned, you can assign the PR to them by writing /assign @smarterclayton in a comment when ready.

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

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

@pbarker pbarker force-pushed the pbarker:audit-policy branch from 1376d55 to 1dd0d57 Feb 25, 2019

@pbarker pbarker force-pushed the pbarker:audit-policy branch from 1dd0d57 to 706fbbb Feb 25, 2019

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Feb 25, 2019

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.


// RequestSelector selects requests by matching on the given attributes. Selectors are
// used to compose audit classes.
type RequestSelector struct {

This comment has been minimized.

Copy link
@shturec

shturec Feb 25, 2019

@pbarker, @tallclair, @liggitt You know labels add orthogonal, domain-specific semantics to k8s resources and it comes in very handy to be able to filter resource lists based on that. I'm not the first to come up with with "hey, how about label selectors in ..." obviously. For example a couple of folks raised this wrt RBAC and there were some valid concerns and obstacles shared wrt implementing it in this particular domain.
I'm wondering, whether the same constraints/concerns apply here too? What do you think about amending the struct with label selectors (not necessarily with this PR)?

This comment has been minimized.

Copy link
@tallclair

tallclair Feb 25, 2019

Member

Labels are tricky because there aren't any restrictions on them, and we want to make sure a malicious user can't go unnoticed by doctoring labels. I could see an argument for having a selector over audit annotations though (the name is unfortunate though, maybe we need labels too?), in which case you could have an admission controller that translated a whitelisted label into an audit annotation. This does create a bit of an ordering problem though, as the level is currently needed before admission runs.

This is out of scope for this PR though.

@pbarker

This comment has been minimized.

Copy link
Contributor Author

pbarker commented Feb 25, 2019

/retest

@liggitt liggitt removed their request for review Feb 25, 2019

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Feb 25, 2019

/assign @lavalamp

@liggitt liggitt added the api-review label Feb 25, 2019

@liggitt liggitt added this to Assigned in API Reviews Feb 25, 2019

@lavalamp
Copy link
Member

lavalamp left a comment

First pass. I have only looked at the types so far. Great concept factoring! Much better than the first take on this last year :)

Show resolved Hide resolved pkg/apis/auditregistration/types.go
Show resolved Hide resolved staging/src/k8s.io/api/auditregistration/v1alpha1/types.go
Show resolved Hide resolved staging/src/k8s.io/api/auditregistration/v1alpha1/types.go
// +optional
Stages []Stage `json:"stages" protobuf:"bytes,2,opt,name=stages"`

// PolicyRules define how classes should be handled.

This comment has been minimized.

Copy link
@lavalamp

lavalamp Mar 1, 2019

Member

"PolicyRules" doesn't match either the field or the type; the types aren't visible in json/yaml so please use the field name (with the json capitalization).

This comment has been minimized.

Copy link
@pbarker

pbarker Apr 10, 2019

Author Contributor

to be clear, should this be Rules or rules? since its at the beginning of a sentence

This comment has been minimized.

Copy link
@lavalamp

lavalamp Apr 10, 2019

Member

rules; You don't need to follow golang comment sentence structure requirements if that helps. I think you can also quote it with backticks to make it extra clear it's a field name reference. (@liggitt am I making that up?)

// PolicyRules define how classes should be handled.
// A request may fall under multiple audit classes.
// Rules are evaluated in order (first matching wins).
// Rules override the top Level & Stage.

This comment has been minimized.

Copy link
@lavalamp

lavalamp Mar 1, 2019

Member

"override" isn't clear, can you spell out exactly what this means?


// Resources in this attribute group. An empty list implies all kinds in all API groups.
// +optional
Resources []GroupResources `json:"resources,omitempty" protobuf:"bytes,5,rep,name=resources"`

This comment has been minimized.

Copy link
@lavalamp

lavalamp Mar 1, 2019

Member

Nit: add a blank line, just to be consistent :)

// Namespaces in this attribute group.
// The empty string "" matches non-namespaced resources.
// An empty list implies every namespace.
// Non-namespaced resources will only be matched if the empty string is present in the list

This comment has been minimized.

Copy link
@lavalamp

lavalamp Mar 1, 2019

Member

nit: s/only be matched if/be matched if and only if/

Resources []GroupResources `json:"resources,omitempty" protobuf:"bytes,5,rep,name=resources"`
// Namespaces in this attribute group.
// The empty string "" matches non-namespaced resources.
// An empty list implies every namespace.

This comment has been minimized.

Copy link
@lavalamp

lavalamp Mar 1, 2019

Member

But not cluster-scoped resources? That is a strange default.

Consider controlling including cluster-scoped resources with an additional field?

This comment has been minimized.

Copy link
@pbarker

pbarker Apr 11, 2019

Author Contributor

I feel there may have been subtleties in the policy implementation around this cc @tallclair

// *s are allowed, but only as the full, final step in the path, and are delimited by the path separator
// Examples:
// "/metrics" - Log requests for apiserver metrics
// "/healthz/*" - Log all health checks

This comment has been minimized.

Copy link
@lavalamp

lavalamp Mar 1, 2019

Member

I'd give an anti-example, too? Like "but not '/healthz*', it is missing a path separator"?

This comment has been minimized.

Copy link
@lavalamp

lavalamp Mar 1, 2019

Member

The empty list implies none of these? How do I get all of them?

This comment has been minimized.

Copy link
@pbarker

pbarker Apr 11, 2019

Author Contributor

**? (this is notoriously not supported in the standard lib)

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

This comment has been minimized.

Copy link
@lavalamp

lavalamp Mar 1, 2019

Member

s/wildcard/a wildcard/

(unless this is copied--then we should fix in both places in a separate change)

This comment has been minimized.

Copy link
@pbarker

pbarker Apr 11, 2019

Author Contributor

yep this is copied, I'll put in a separate PR

@WanLinghao

This comment has been minimized.

Copy link
Contributor

WanLinghao commented Mar 5, 2019

/milestone v1.14

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Mar 5, 2019

@WanLinghao: You must be a member of the kubernetes/kubernetes-milestone-maintainers GitHub team to set the milestone. If you believe you should be able to issue the /milestone command, please contact your and have them propose you as an additional delegate for this responsibility.

In response to this:

/milestone v1.14

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@spiffxp

This comment has been minimized.

Copy link
Member

spiffxp commented Mar 7, 2019

/hold
v1.14 release lead here, this is a size/XXL PR that is attempting to land right before Code Freeze... are you really sure this is a good idea?

@pbarker

This comment has been minimized.

Copy link
Contributor Author

pbarker commented Mar 7, 2019

hey @spiffxp we know this won't make 1.14 and aren't vying for that at this point, feel free to change the milestone to 1.15, thanks

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Mar 7, 2019

./milestone v1.15

@lavalamp

This comment has been minimized.

Copy link
Member

lavalamp commented Apr 2, 2019

Just checking in--looks like it's not my turn yet. Please ping me when it is! :)

@lavalamp lavalamp moved this from Assigned to Changes requested in API Reviews Apr 2, 2019

@WanLinghao

This comment has been minimized.

Copy link
Contributor

WanLinghao commented Apr 10, 2019

Hi everyone, is this patch ready to merge?

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Apr 10, 2019

@pbarker: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.