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

Add PodSecurityPolicy information to audit logs #58143

Merged
merged 3 commits into from Jun 4, 2018

Conversation

@CaoShuFeng
Copy link
Member

CaoShuFeng commented Jan 11, 2018

Depends on: #58806
Fix #56209

Release note:

PodSecurityPolicy admission information is added to audit logs
@CaoShuFeng

This comment has been minimized.

Copy link
Member Author

CaoShuFeng commented Jan 16, 2018

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

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

This comment has been minimized.

@sttts

sttts Jan 16, 2018

Contributor

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

This comment has been minimized.

@soltysh

soltysh Jan 16, 2018

Contributor

👍 we should always use full names

allowed bool
errors []error
allowed bool
clusterRoleBinding string

This comment has been minimized.

@sttts

sttts Jan 16, 2018

Contributor

would move these into its own struct. maybe struct reason

@@ -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 != "" {

This comment has been minimized.

@sttts

sttts Jan 16, 2018

Contributor

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

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

This comment has been minimized.

@sttts

sttts Jan 16, 2018

Contributor

auditEvent

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

This comment has been minimized.

@sttts

sttts Jan 16, 2018

Contributor

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

This comment has been minimized.

@crassirostris

crassirostris Jan 16, 2018

Member

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

This comment has been minimized.

@sttts

sttts Jan 16, 2018

Contributor

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.

This comment has been minimized.

@soltysh

soltysh Jan 16, 2018

Contributor

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

This comment has been minimized.

@soltysh

soltysh Jan 16, 2018

Contributor

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

  1. be able to decide which admission and rbac plugins are logged, so that would require additional rule changes
  2. 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?

This comment has been minimized.

@tallclair

tallclair Jan 23, 2018

Member

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.

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

This comment has been minimized.

@sttts

sttts Jan 16, 2018

Contributor

do we really need this new func name?

}

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 {

This comment has been minimized.

@sttts

sttts Jan 16, 2018

Contributor

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

This comment has been minimized.

@CaoShuFeng

CaoShuFeng Jan 16, 2018

Author Member

OK.
I will modify all NewAttributesRecord functions.

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

:)

This comment has been minimized.

This comment has been minimized.

@sttts

sttts Jan 16, 2018

Contributor

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

This comment has been minimized.

Copy link
Contributor

sttts commented Jan 16, 2018

What's your opinion about this version?

Much better. It's what I expected 👍

@CaoShuFeng

This comment has been minimized.

Copy link
Member Author

CaoShuFeng commented Jan 16, 2018

Much better. It's what I expected 👍

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

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

This comment has been minimized.

@liggitt

liggitt Jan 16, 2018

Member

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

This comment has been minimized.

@liggitt

liggitt Jan 17, 2018

Member

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

@@ -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 comment has been minimized.

@liggitt

liggitt Jan 16, 2018

Member

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

This comment has been minimized.

@sttts

sttts Jan 19, 2018

Contributor

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.

This comment has been minimized.

@liggitt

liggitt Jan 19, 2018

Member

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

This comment has been minimized.

@sttts

sttts Jan 19, 2018

Contributor

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

This comment has been minimized.

@liggitt

liggitt Jan 19, 2018

Member

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

This comment has been minimized.

@liggitt

liggitt Jan 19, 2018

Member

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

This comment has been minimized.

@liggitt

liggitt Jan 19, 2018

Member

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

This comment has been minimized.

@CaoShuFeng

CaoShuFeng Jan 22, 2018

Author Member

Done.

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

This comment has been minimized.

@liggitt

liggitt Jan 16, 2018

Member

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

This comment has been minimized.

@sttts

sttts Jan 19, 2018

Contributor

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

@CaoShuFeng CaoShuFeng force-pushed the CaoShuFeng:audit_annotation_another_version branch from 3c05701 to c77617a Jan 19, 2018

@k8s-ci-robot k8s-ci-robot added size/XL and removed size/L labels Jan 19, 2018

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jun 3, 2018

[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

@CaoShuFeng CaoShuFeng force-pushed the CaoShuFeng:audit_annotation_another_version branch from edeaa55 to e71fa21 Jun 4, 2018

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 comment has been minimized.

@sttts

sttts Jun 4, 2018

Contributor

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

This comment has been minimized.

@CaoShuFeng

CaoShuFeng Jun 4, 2018

Author Member

Yes.
According to @tallclair 's suggestion.

This comment has been minimized.

@sttts

sttts Jun 4, 2018

Contributor

👍

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

This comment has been minimized.

@sttts

sttts Jun 4, 2018

Contributor

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

This comment has been minimized.

@CaoShuFeng

CaoShuFeng Jun 4, 2018

Author Member

Done.

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

This comment has been minimized.

@sttts

sttts Jun 4, 2018

Contributor

why do we need this?

This comment has been minimized.

@sttts

sttts Jun 4, 2018

Contributor

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

This comment has been minimized.

@CaoShuFeng

CaoShuFeng Jun 4, 2018

Author Member

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

d7474b8#r29220311

This comment has been minimized.

@CaoShuFeng

CaoShuFeng Jun 4, 2018

Author Member

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

This comment has been minimized.

@sttts

sttts Jun 4, 2018

Contributor

ok, sounds reasonable.

@CaoShuFeng CaoShuFeng force-pushed the CaoShuFeng:audit_annotation_another_version branch from e71fa21 to 2414228 Jun 4, 2018

CaoShuFeng added some commits Mar 22, 2018

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

This comment has been minimized.

Copy link
Contributor

sttts commented Jun 4, 2018

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Jun 4, 2018

@sttts

This comment has been minimized.

Copy link
Contributor

sttts commented Jun 4, 2018

@tallclair @liggitt approved?

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Jun 4, 2018

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jun 4, 2018

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

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

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Jun 4, 2018

/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

This comment has been minimized.

Copy link

jberkus commented Jun 4, 2018

/retest pull-kubernetes-e2e-kops-aws

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Jun 4, 2018

/retest

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jun 4, 2018

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.

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

18 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation CaoShuFeng 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-100-performance 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-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

k8s-github-robot pushed a commit that referenced this pull request Aug 22, 2018

Kubernetes Submit Queue
Merge pull request #58679 from CaoShuFeng/admission_webhook
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.
```

k8s-publishing-bot added a commit to kubernetes/api that referenced this pull request Aug 23, 2018

Merge pull request #58679 from CaoShuFeng/admission_webhook
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-publishing-bot added a commit to kubernetes/apiserver that referenced this pull request Aug 23, 2018

Merge pull request #58679 from CaoShuFeng/admission_webhook
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

sttts pushed a commit to sttts/api that referenced this pull request Sep 5, 2018

Merge pull request #58679 from CaoShuFeng/admission_webhook
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

sttts pushed a commit to sttts/apiserver that referenced this pull request Sep 5, 2018

Merge pull request #58679 from CaoShuFeng/admission_webhook
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-publishing-bot added a commit to kubernetes/api that referenced this pull request Sep 6, 2018

Merge pull request #58679 from CaoShuFeng/admission_webhook
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-publishing-bot added a commit to kubernetes/apiserver that referenced this pull request Sep 6, 2018

Merge pull request #58679 from CaoShuFeng/admission_webhook
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
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.