Add PodSecurityPolicy information to audit logs #58143

CaoShuFeng · 2018-01-11T08:39:34Z

Release note:

PodSecurityPolicy admission information is added to audit logs

CaoShuFeng · 2018-01-16T02:17:17Z

/assign @sttts @liggitt
What's your opinion about this version?

sttts · 2018-01-16T07:42:40Z

plugin/pkg/admission/security/podsecuritypolicy/admission.go

 	if err != nil {
 		return admission.NewForbidden(a, err)
 	}
 	if apiequality.Semantic.DeepEqual(pod, allowedPod) {
+		ae := a.GetAuditEvent()
+		audit.LogAnnotations(ae, "podsecuritypolicy/validate", pspName)


we need better keys here, maybe podsecuritypolicy.admission.k8s.io/policy

👍 we should always use full names

sttts · 2018-01-16T07:44:37Z

plugin/pkg/auth/authorizer/rbac/rbac.go

-	allowed bool
-	errors  []error
+	allowed            bool
+	clusterRoleBinding string


would move these into its own struct. maybe struct reason

sttts · 2018-01-16T07:45:11Z

plugin/pkg/auth/authorizer/rbac/rbac.go

@@ -74,6 +82,21 @@ func (r *RBACAuthorizer) Authorize(requestAttributes authorizer.Attributes) (aut

 	r.authorizationRuleResolver.VisitRulesFor(requestAttributes.GetUser(), requestAttributes.GetNamespace(), ruleCheckingVisitor.visit)
 	if ruleCheckingVisitor.allowed {
+		ae := requestAttributes.GetAuditEvent()
+		if ae != nil {
+			if ruleCheckingVisitor.clusterRoleBinding != "" {


len(...) != 0 is our convention

sttts · 2018-01-16T07:46:13Z

staging/src/k8s.io/apiserver/pkg/admission/attributes.go

@@ -32,9 +33,10 @@ type attributesRecord struct {
 	object      runtime.Object
 	oldObject   runtime.Object
 	userInfo    user.Info
+	ae          *audit.Event


sttts · 2018-01-16T07:47:57Z

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

+	// PSP will store admission information in Annotations, like name of the policy which admits
+	// the request.
+	// +optional
+	Annotations map[string]string


@tallclair @crassirostris @ericchiang @soltysh this is about the API. Are we happy with this representation?

I'm fine with this interface, but I remember someone was not happy with it

We have plugin names (those which are also used in the apiserver flags). We could make this more structured using them if we concentrate on admission. We could render the whole chain here in the event.

RBAC would then be another step, probably with an explicit authorization field.

I'm not too happy with the name, but can't come up with a better one, so I guess I'll just have to live with it ;).
As for the structured, having map[string]string give us more freedom, if we decide to add additional information. Maybe we should name the field as AdditionalInfo or something like that? At the same time, I'm worried this might end up being a grab bag for all the things we want to have here. Hm....

I just spoke with @sttts and we agreed it would be nice to:

be able to decide which admission and rbac plugins are logged, so that would require additional rule changes

Authorization logging happens in authorizingVisitor which is perfect, but admission happens only in PSP, I'd imagine this should happen a level higher to be able to do what I said in 1.

@tallclair @crassirostris @ericchiang @CaoShuFeng opinions?

be able to decide which admission and rbac plugins are logged, so that would require additional rule changes

This feels like something that should be resolved with filters, if we ever get around to implementing them. We don't currently allow fine grained selection of what's logged, so I think the current approach is consistent.

Authorization logging happens in authorizingVisitor which is perfect,

Only for RBAC, it's an implementation detail.

but admission happens only in PSP, I'd imagine this should happen a level higher to be able to do what I said in 1.

I see this field as arbitrary metadata that plugins invoked in the serving path can add to. I think the authorization & admission interfaces should enable this on a case-by-case basis.

I'm happy with the current unstructured approach.

sttts · 2018-01-16T07:49:19Z

staging/src/k8s.io/apiserver/pkg/endpoints/handlers/create.go

@@ -94,7 +94,7 @@ func createHandler(r rest.NamedCreater, scope RequestScope, typer runtime.Object
 		audit.LogRequestObject(ae, obj, scope.Resource, scope.Subresource, scope.Serializer)

 		userInfo, _ := request.UserFrom(ctx)
-		admissionAttributes := admission.NewAttributesRecord(obj, nil, scope.Kind, namespace, name, scope.Resource, scope.Subresource, admission.Create, userInfo)
+		admissionAttributes := admission.NewAttributesRecordWithAudit(obj, nil, scope.Kind, namespace, name, scope.Resource, scope.Subresource, admission.Create, userInfo, ae)


do we really need this new func name?

sttts · 2018-01-16T07:49:53Z

staging/src/k8s.io/apiserver/pkg/admission/attributes.go

 }

-func NewAttributesRecord(object runtime.Object, oldObject runtime.Object, kind schema.GroupVersionKind, namespace, name string, resource schema.GroupVersionResource, subresource string, operation Operation, userInfo user.Info) Attributes {
+func NewAttributesRecordWithAudit(object runtime.Object, oldObject runtime.Object, kind schema.GroupVersionKind, namespace, name string, resource schema.GroupVersionResource, subresource string, operation Operation, userInfo user.Info, ae *audit.Event) Attributes {


I would keep the old func name and make that argument mandatory (possibly nil)

OK.
I will modify all NewAttributesRecord functions.

$ LANG=C grep "NewAttributesRecord" * -r | grep "\.go" | wc -l 262

:)

262

https://www.gnu.org/software/sed/manual/html_node/index.html ;)

For easier review, please do that in another commit.

99% of those calls are in tests btw. Was surprised that the number is that high.

sttts · 2018-01-16T07:50:30Z

What's your opinion about this version?

Much better. It's what I expected 👍

CaoShuFeng · 2018-01-16T07:58:40Z

Much better. It's what I expected 👍

Thanks. I will address all your comments and add some unit tests.

liggitt · 2018-01-16T17:31:17Z

pkg/registry/rbac/validation/rule.go

@@ -42,7 +42,7 @@ type AuthorizationRuleResolver interface {

 	// VisitRulesFor invokes visitor() with each rule that applies to a given user in a given namespace, and each error encountered resolving those rules.
 	// If visitor() returns false, visiting is short-circuited.
-	VisitRulesFor(user user.Info, namespace string, visitor func(rule *rbac.PolicyRule, err error) bool)
+	VisitRulesFor(user user.Info, namespace string, visitor func(rule *rbac.PolicyRule, clusterRolebinding, clusterRole, roleBinding, role string, err error) bool)


prefer (*rbac.RoleRef, *rbac.PolicyRule, ...), since we have the reference already?

hmm, I guess you need the binding name/namespace as well

liggitt · 2018-01-16T17:34:26Z

staging/src/k8s.io/apiserver/pkg/authorization/authorizer/interfaces.go

@@ -61,6 +62,9 @@ type Attributes interface {

 	// GetPath returns the path of the request
 	GetPath() string
+
+	// GetAuditEvent returns the audit event of the request
+	GetAuditEvent() *audit.Event


this interface is supposed to be fully serializable so it can be delegated via a subject access review. an audit.Event pointer breaks that, and implies getting a writeable object out of the attributes, which reverses the information flow of this interface

Which means that we have to expose the audit annotations (or any other data structure we choose) for authz/n in a different way here. Also a delgated authorizer should fill in values.

authz already has the ability to return a reason along with the authorization decision. can we use that from the outside, rather than plumbing audit into all the authorizers? that would work with remote authorization as well without any changes to the authz APIs

@tallclair @crassirostris can you comment whether the reason is enough?

I would expect the following behavior:

when authorization failed, the reason would contain aggregated reasons from all authorizers that provided failure reasons

when authorization succeeded, the reason would contain the reason from the succeeding authorizer if it provided one

the authorizer reason is currently plumbed all the way up to https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/endpoints/filters/authorization.go#L49 which seems like a much easier place to get a handle to the audit event

PR that makes RBAC return a detailed reason for what it allows that includes the subject, binding, and role: #58531

liggitt · 2018-01-16T17:36:17Z

plugin/pkg/auth/authorizer/rbac/rbac.go

@@ -74,6 +82,21 @@ func (r *RBACAuthorizer) Authorize(requestAttributes authorizer.Attributes) (aut

 	r.authorizationRuleResolver.VisitRulesFor(requestAttributes.GetUser(), requestAttributes.GetNamespace(), ruleCheckingVisitor.visit)
 	if ruleCheckingVisitor.allowed {
+		ae := requestAttributes.GetAuditEvent()


getting an object out of requestAttributes in order to write back to it is a fundamental change in the way the authorizer attributes are used, and breaks remote delegation cases for that interface

See above. We need an explicit data structure here. Not a huge event with unclear ownership.

tallclair · 2018-06-03T19:16:08Z

/status approved-for-milestone

k8s-github-robot · 2018-06-03T19:16:42Z

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@CaoShuFeng @justinsb @liggitt @sttts @tallclair

Pull Request Labels

sig/auth: Pull Request will be escalated to these SIGs if needed.
priority/important-soon: Escalate to the pull request owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
kind/feature: New functionality.

Help

sttts · 2018-06-04T10:25:24Z

staging/src/k8s.io/apiserver/pkg/admission/attributes.go

+		record.annotations = make(map[string]string)
+	}
+	if v, ok := record.annotations[key]; ok && v != value {
+		return fmt.Errorf("admission annotations are not allowd to be overwritten, key:%q, old value: %q, new value:%q", key, record.annotations[key], value)


this switched to an error from a warning? Am fine with this.

Yes.
According to @tallclair 's suggestion.

sttts · 2018-06-04T10:25:55Z

staging/src/k8s.io/apiserver/pkg/admission/attributes_test.go

+			"podsecuritypolicy.admission.k8s.io/validate-policy": "privileged",
+			"podsecuritypolicy.admission.k8s.io/admit-policy":    "privileged",
+		},
+		"unexpected final annotations")


nit: odd formatting. usually we put the ) on the next line.

sttts · 2018-06-04T10:26:54Z

staging/src/k8s.io/apiserver/pkg/admission/interfaces.go

+// AnnotationsGetter allows users to get annotations from Attributes. An alternate Attribute should implement
+// this interface.
+type AnnotationsGetter interface {
+	GetAnnotations() map[string]string


why do we need this?

I don't see this used. Premature abstraction? Would drop it.

This interface is used according to @liggitt 's suggestion.

d7474b8#r29220311

It's used in WithAudit decorator to check the attribute interface.
Would be useful for alternate Attributes.

ok, sounds reasonable.

WithAudit admission decorator log annotations to audit events set by the decorated admission controller

sttts · 2018-06-04T11:56:58Z

/lgtm
/approve

sttts · 2018-06-04T11:58:10Z

@tallclair @liggitt approved?

liggitt · 2018-06-04T12:01:14Z

/approve

k8s-ci-robot · 2018-06-04T12:01:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CaoShuFeng, liggitt, 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 files:

~~plugin/pkg/admission/security/podsecuritypolicy/OWNERS~~ [liggitt]
~~staging/src/k8s.io/apiserver/OWNERS~~ [liggitt,sttts]

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

fejta-bot · 2018-06-04T14:20:18Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

jberkus · 2018-06-04T16:02:03Z

/retest pull-kubernetes-e2e-kops-aws

liggitt · 2018-06-04T16:23:13Z

/retest

k8s-github-robot · 2018-06-04T19:26:18Z

Automatic merge from submit-queue (batch tested with PRs 61610, 64591, 58143, 63929). If you want to cherry-pick this change to another branch, please follow the instructions here.

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. support annotations for admission webhook Depends on: #58143 **Release note**: ```release-note Support annotations for remote admission webhooks. ```

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. support annotations for admission webhook Depends on: kubernetes/kubernetes#58143 **Release note**: ```release-note Support annotations for remote admission webhooks. ``` Kubernetes-commit: 4e76bb487e50996468bea6638f8ade45911953de

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 11, 2018

k8s-github-robot assigned tallclair and justinsb Jan 11, 2018

CaoShuFeng mentioned this pull request Jan 11, 2018

Add PodSecurityPolicy admission information to audit logs #58075

Closed

crassirostris added the area/audit label Jan 11, 2018

CaoShuFeng force-pushed the audit_annotation_another_version branch from 68fdf4c to 3c05701 Compare January 15, 2018 10:32

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 15, 2018

k8s-ci-robot assigned liggitt and sttts Jan 16, 2018

sttts reviewed Jan 16, 2018

View reviewed changes

liggitt reviewed Jan 16, 2018

View reviewed changes

CaoShuFeng force-pushed the audit_annotation_another_version branch from 3c05701 to c77617a Compare January 19, 2018 09:14

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 19, 2018

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 19, 2018

liggitt mentioned this pull request Jan 19, 2018

return reason for allowed rbac authorizations #58531

Merged

k8s-ci-robot added the status/approved-for-milestone label Jun 3, 2018

k8s-github-robot removed the milestone/needs-approval label Jun 3, 2018

CaoShuFeng force-pushed the audit_annotation_another_version branch from edeaa55 to e71fa21 Compare June 4, 2018 02:15

sttts reviewed Jun 4, 2018

View reviewed changes

CaoShuFeng force-pushed the audit_annotation_another_version branch from e71fa21 to 2414228 Compare June 4, 2018 11:23

CaoShuFeng added 3 commits June 4, 2018 19:24

support AddAnnotation in admission attributes

72ef2dc

add WithAudit admission decorator

c38a704

WithAudit admission decorator log annotations to audit events set by the decorated admission controller

Log policy name from pod security policy

2414228

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

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

k8s-github-robot merged commit 08c15a6 into kubernetes:master Jun 4, 2018

Add PodSecurityPolicy information to audit logs #58143

Add PodSecurityPolicy information to audit logs #58143

Conversation

CaoShuFeng commented Jan 11, 2018 • edited

CaoShuFeng commented Jan 16, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sttts commented Jan 16, 2018

CaoShuFeng commented Jan 16, 2018

Choose a reason for hiding this comment

liggitt Jan 17, 2018 • edited

Choose a reason for hiding this comment

liggitt Jan 16, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Jan 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Jan 19, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Jan 19, 2018 • edited

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

tallclair commented Jun 3, 2018

k8s-github-robot commented Jun 3, 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

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

sttts commented Jun 4, 2018

sttts commented Jun 4, 2018

liggitt commented Jun 4, 2018

k8s-ci-robot commented Jun 4, 2018

fejta-bot commented Jun 4, 2018

jberkus commented Jun 4, 2018

liggitt commented Jun 4, 2018

k8s-github-robot commented Jun 4, 2018

CaoShuFeng commented Jan 11, 2018 •

edited

liggitt Jan 17, 2018 •

edited

liggitt Jan 16, 2018 •

edited

liggitt Jan 19, 2018 •

edited

liggitt Jan 19, 2018 •

edited

liggitt Jan 19, 2018 •

edited