audit newest impersonated user info in the ResponseStarted, ResponseComplete audit stage #48184

CaoShuFeng · 2017-06-28T10:04:25Z

Impersonation will automatically add system:authenticated, system:serviceaccounts group to the impersonated user info. This pr use the newest impersonated user info in the second audit event. This will help users to debug rbac problems.

Release note:

[advanced audit] audit newest impersonated user info in the ResponseStarted, ResponseComplete audit stage

@liggitt @sttts

k8s-ci-robot · 2017-06-28T10:04:32Z

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

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. I understand the commands that are listed here.

liggitt · 2017-06-29T04:08:26Z

staging/src/k8s.io/apiserver/pkg/audit/request.go

+// UpdateEventAttribs updates authorizer attributes according to attribs
+func UpdateEventAttribs(ev *auditinternal.Event, attribs authorizer.Attributes) {
+	if user := attribs.GetUser(); user != nil {
+		ev.User.Username = user.GetName()


mutating this seems wrong. I'd want both the requesting user and effective/impersonated user in my audit log

@liggitt
Thanks for your review.
I will research it.

+1

We have to extend the event type for that I guess.

/cc @timstclair

I'm not sure if I'm following what this is doing, but ev.User should be the "real" user, and ev.ImpersonatedUser should hold the the impersonated user attributes. What are we missing?

I agree with @liggitt that the audit event should be thought of as "append only". I.e. we shouldn't change information from earlier stages.

Hi, @tallclair
I think we can extend the event type, add a new ev.EffectiveUser to events.

The ev.User in the log is the "real" user, but it is not the "effective" user. Because impersonation will change the user info after the audit filter gathers event info.

Can we add this to the TODO list @tallclair ?

@ericchiang What's your opinion?

Usually EffectiveUser is equal to ImpersonatedUser.
But when user run kubectl with --as=username option, the difference comes.

This tables shows the difference, we can see that some groups are automatically added.

The related codes are here:
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/endpoints/filters/impersonation.go#L87
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/endpoints/filters/impersonation.go#L121

If we don't want to add a new field "EffectiveUser", I think we can copy some codes form impersonation.go to audit.go, and we can also add the same groups to the impersonated user.
Then the impersonated user will become effective user.

Don't copy code, I'd expect impersonated user to hold the output of the impersonation filter, what you are calling the effective user

I'd expect impersonated user to hold the output of the impersonation filter, what you are calling the effective user

I aggree.

I updated the code.
Please review again, thanks.

ncdc · 2017-07-17T14:17:26Z

/unassign
/assign @liggitt
fyi @mattmoyer

ericchiang · 2017-07-17T16:02:04Z

Okay, so the issue is that the API server adds additional groups to the impersonated user that the audit log isn't picking up.

Rather than mutating the event log later, can we move impersonation before audit log in the handler chain and have the impersonation filter (filters.WithImpersionation) add another context key indicating the original user?

Something like:

package request

// Existing API

func UserFrom(ctx Context) (user.Info, bool) {}
func WithUser(parent Context, user user.Info) Context {}

// NewAPI. Maybe parameter names could be improved?

// WithImpersionation indicates that the user performing the request is now impersonating the
// provided user. Further calls to WithUser on the returned context will return the impersonating
// user info.
func WithImpersionation(parent Context, impersonator, impersonating user.Info) Context {}

// ImpersionationFrom returns the full impersonation information from the request. If the request
// did not use impersonation, it returns false indicating that UserFrom should be used instead.
func ImpersionationFrom(ctx Context) (impersonator, impersonating, user.Info, ok bool) {}

Then have the audit log mechanism use ImpersionationFrom?

cc @sttts

sttts · 2017-07-18T06:25:40Z

can we move impersonation before audit log in the handler chain and have the impersonation filter

We can. But then we loose the 403 from impersonation. @soltysh was working on a catch-up audit handler to get the 403s from authentication. I guess his approach will also work with impersonation if we switch the handler chain order. @soltysh ?

CaoShuFeng · 2017-07-21T03:16:25Z

Sorry to disturb you guys again and again.
What about just use my current solution?

As far as I can see, this only add the automatically added groups into the impersonated user. No info will be deleted.
Further more, this only effect the request with impersonated user setted but impersonated groups not setted. I most cases, this change has no effect.
If we document this well, I thint it will not do any harm.

Thanks very much.

soltysh · 2017-08-07T13:41:12Z

We can. But then we loose the 403 from impersonation. @soltysh was working on a catch-up audit handler to get the 403s from authentication. I guess his approach will also work with impersonation if we switch the handler chain order. @soltysh ?

That shouldn't be the problem. The issue I'm trying to address in openshift (so far) is to be able to log failed authz requests, which our current implementation omits due to design decision we've made at the beginning. @crassirostris @sttts @ericchiang not sure if we want to address this at some point in k8s?

ericchiang · 2017-08-07T15:47:32Z

/ok-to-test

Some possible options:

Merge as is.
Remove impersonation logic from audit.NewEventFromRequest and just do it here. Don't capture request impersonation headers, only the actual user that gets authenticated.
Differentiate between request impersonation headers and actual impersonated user.

I think I'm for the first one.

CaoShuFeng · 2017-08-14T07:15:08Z

/test pull-kubernetes-e2e-gce-etcd3

crassirostris · 2017-08-30T11:38:48Z

@CaoShuFeng What's the status of this PR?

soltysh · 2017-08-30T14:29:09Z

staging/src/k8s.io/apiserver/pkg/endpoints/filters/impersonation.go

@@ -142,6 +143,11 @@ func WithImpersonation(handler http.Handler, requestContextMapper request.Reques
 			}
 		}

+		if !groupsSpecified && username != user.Anonymous {


Now I'm puzzled. Why we need to do this, if we have this snippet in place and audit happens before impersonation?

We have 4 stages.
This change will effect the other three stages rather than "StageRequestReceived".

We do this because the snippet may not complete.

Reference:
https://github.com/kubernetes/kubernetes/pull/48184/files/3989d45adfccd57ffe1fdab15d3e04ab0262e152#diff-4bcea545afa7a35798d0ff9db0ad0e1aR121

https://github.com/kubernetes/kubernetes/pull/48184/files/3989d45adfccd57ffe1fdab15d3e04ab0262e152#diff-4bcea545afa7a35798d0ff9db0ad0e1aR89

As far as I see the groups and UID might be different than in the old code. Can we drop the old snippet and use this instead? Having two code paths for the same things is confusing.

👍 to what Stefan said, replace the old code with the new one, entirely. I did not understand you correctly, initially.

soltysh · 2017-08-31T13:55:06Z

@CaoShuFeng will you be able to update this one asap, as well?

CaoShuFeng · 2017-08-31T14:53:19Z

@CaoShuFeng will you be able to update this one asap, as well?

I have get off work today. I will update this tomorrow morning.

Log the newest impersonated user info in the second audit event. This will help users to debug rbac problems.

sttts · 2017-09-01T08:57:02Z

/lgtm
/approve no-issue

k8s-github-robot · 2017-09-01T08:57:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CaoShuFeng, sttts

Associated issue requirement bypassed by: sttts

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

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

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

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-09-03T06:50:04Z

Automatic merge from submit-queue (batch tested with PRs 51301, 50497, 50112, 48184, 50993)

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 28, 2017

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

k8s-github-robot assigned deads2k and ncdc Jun 28, 2017

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jun 28, 2017

liggitt reviewed Jun 29, 2017

View reviewed changes

CaoShuFeng force-pushed the impersonate_audit branch from 060b659 to 3989d45 Compare July 17, 2017 05:43

CaoShuFeng changed the title ~~audit real authorizer attributes in the second audit stage~~ audit real impersonated user info in the ResponseStarted, ResponseComplete audit stage Jul 17, 2017

CaoShuFeng changed the title ~~audit real impersonated user info in the ResponseStarted, ResponseComplete audit stage~~ audit newest impersonated user info in the ResponseStarted, ResponseComplete audit stage Jul 17, 2017

k8s-ci-robot assigned liggitt and unassigned ncdc Jul 17, 2017

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 Aug 7, 2017

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

soltysh reviewed Aug 30, 2017

View reviewed changes

ericchiang added the area/audit label Aug 31, 2017

audit real impersonated user info

1c3dc52

Log the newest impersonated user info in the second audit event. This will help users to debug rbac problems.

CaoShuFeng force-pushed the impersonate_audit branch from 3989d45 to 1c3dc52 Compare September 1, 2017 03:27

k8s-ci-robot removed the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 1, 2017

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 1, 2017

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 1, 2017

k8s-ci-robot assigned sttts Sep 1, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 1, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 1, 2017

soltysh approved these changes Sep 1, 2017

View reviewed changes

soltysh added this to the v1.8 milestone Sep 1, 2017

k8s-github-robot merged commit 134b667 into kubernetes:master Sep 3, 2017

ericchiang added the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label Sep 7, 2017

CaoShuFeng deleted the impersonate_audit branch November 6, 2017 05:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

audit newest impersonated user info in the ResponseStarted, ResponseComplete audit stage #48184

audit newest impersonated user info in the ResponseStarted, ResponseComplete audit stage #48184

CaoShuFeng commented Jun 28, 2017 •

edited

Loading

k8s-ci-robot commented Jun 28, 2017

liggitt Jun 29, 2017

CaoShuFeng Jun 29, 2017

sttts Jul 3, 2017

timstclair Jul 5, 2017

CaoShuFeng Jul 13, 2017

CaoShuFeng Jul 14, 2017 •

edited

Loading

CaoShuFeng Jul 14, 2017

liggitt Jul 14, 2017

CaoShuFeng Jul 14, 2017

CaoShuFeng Jul 17, 2017

ncdc commented Jul 17, 2017

ericchiang commented Jul 17, 2017 •

edited

Loading

sttts commented Jul 18, 2017

CaoShuFeng commented Jul 21, 2017

soltysh commented Aug 7, 2017

ericchiang commented Aug 7, 2017

CaoShuFeng commented Aug 14, 2017

crassirostris commented Aug 30, 2017

soltysh Aug 30, 2017

CaoShuFeng Aug 31, 2017

CaoShuFeng Aug 31, 2017 •

edited

Loading

sttts Aug 31, 2017

soltysh Aug 31, 2017

CaoShuFeng Sep 1, 2017

soltysh commented Aug 31, 2017

CaoShuFeng commented Aug 31, 2017

sttts commented Sep 1, 2017

k8s-github-robot commented Sep 1, 2017

k8s-github-robot commented Sep 3, 2017

audit newest impersonated user info in the ResponseStarted, ResponseComplete audit stage #48184

audit newest impersonated user info in the ResponseStarted, ResponseComplete audit stage #48184

Conversation

CaoShuFeng commented Jun 28, 2017 • edited Loading

k8s-ci-robot commented Jun 28, 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

CaoShuFeng Jul 14, 2017 • edited Loading

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

ncdc commented Jul 17, 2017

ericchiang commented Jul 17, 2017 • edited Loading

sttts commented Jul 18, 2017

CaoShuFeng commented Jul 21, 2017

soltysh commented Aug 7, 2017

ericchiang commented Aug 7, 2017

CaoShuFeng commented Aug 14, 2017

crassirostris commented Aug 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CaoShuFeng Aug 31, 2017 • edited Loading

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

soltysh commented Aug 31, 2017

CaoShuFeng commented Aug 31, 2017

sttts commented Sep 1, 2017

k8s-github-robot commented Sep 1, 2017

k8s-github-robot commented Sep 3, 2017

CaoShuFeng commented Jun 28, 2017 •

edited

Loading

CaoShuFeng Jul 14, 2017 •

edited

Loading

ericchiang commented Jul 17, 2017 •

edited

Loading

CaoShuFeng Aug 31, 2017 •

edited

Loading