add act-as powers #23549

deads2k · 2016-03-28T19:43:04Z

This adds the ability for a user to Act-As another user if he has the ability to verb="impersonate", resource="users", resourceName=<value of Act-As Header>.

@erictune @kubernetes/kube-iam @liggitt @smarterclayton

I'll need to update the authorizer to handle resource names through the interface, but I don't plan to update the abac policy file.

k8s-bot · 2016-03-28T20:14:10Z

GCE e2e build/test failed for commit dc718777fad2ea724d839eee85899485a06b46ed.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

k8s-bot · 2016-03-28T20:37:17Z

GCE e2e build/test passed for commit 85844c419933c4a507225d52bfa542f8922685e1.

erictune · 2016-03-28T21:00:25Z

pkg/apiserver/handlers.go

+		newCtx, _ := requestContextMapper.Get(req)
+		oldUser, _ := api.UserFrom(ctx)
+		newUser, _ := api.UserFrom(newCtx)
+		glog.V(3).Infof("%v is acting as %v on %v", oldUser, newUser, req.URL)


Is this request going to go into the HTTP log as the original or the impersonated user? I think the original user is more useful from a debugging and audit logging perspective, so we should try to preserve that. If it is possible to decorate the logs lines with the impersonated user name, that would be nice, but not as important as logging the original user.

isn't it logging both the original and new user?

from this point on, anything that pulls the user out of the context will get the impersonated user

I was talking about the logs that the go http server emits to stdout and go into /var/log/kube-apiserver.log on a typical kubernetes cluster. That doesn't use glog.

I thought that was logging the username here but it is just logging the User-Agent header. So, never mind.

Please do httplog.LogOf(req, w).Addf("%v is acting as %v", oldUser, newUser) to actually associate this message with the request in addition to logging directly here.

That's nifty… didn't know about that

We unwrap that thing so often I'm surprised we still keep it around.

erictune · 2016-03-28T23:47:09Z

looks good to me, but let lavalamp review too.

lavalamp · 2016-03-28T23:55:43Z

test/integration/auth_test.go

+func TestActAsIsForbidden(t *testing.T) {
+	framework.DeleteAllEtcdKeys()
+
+	// This file has alice and bob in it.


deads2k · 2016-03-29T12:28:05Z

Two things:

We never settled on the header name: Act-As, X-Authentication-Acting-As, Impersonate-User? I like Act-As best, but that might just be because that's how the original proposal was titled.
The authorization check for service accounts (https://github.com/kubernetes/kubernetes/pull/23549/files#diff-6073dcd8ae44859aa2bee1af7531dfe0R437). It's a synthetic check, so we can make it check whatever we want. I chose the simple implementation for this pull ("users"), but I'd like to expand it allow a check for act-as against the exact serviceaccounts resource when an SA user is specified. I think that more accurately expresses what they're trying to do. If this is non-contentious I'll do it here. If we need to talk about it an issue, we can do that so we don't block the utility in this pull.

k8s-bot · 2016-03-29T13:05:00Z

GCE e2e build/test passed for commit 7469b3ad8cc156021c05b9b3a7954d5ad2dfc6cf.

k8s-bot · 2016-03-29T13:06:48Z

GCE e2e build/test passed for commit 4a985123d8c39b79ef31a8588091828e72349eb0.

erictune · 2016-03-30T16:48:07Z

Apparently "X-" is deprecated for headers. "Act-As" is so terse that I think it is unlikely to ever become a standard for this sort of thing, and doesn't document itself if someone is trying to understand our protocol by looking at the headers. "Impersonation" is sort of a term-of-art, and it is suggestive that the header relates to auth. So, I vote for David's third option, Impersonate-User.

liggitt · 2016-03-30T16:51:24Z

Agree. I would change the verb check to "impersonate" as well

erictune · 2016-03-30T16:55:03Z

Fine with impersonate verb as well.

For your item 2, David, are you suggesting something like this for a service account:

    actingAsAttributes := &authorizer.AttributesRecord{
            User:     requestor,
            Verb:     "act-as",
            APIGroup: api.GroupName,
            Resource: "serviceAccounts",
            ResourceName:    serviceAccountName,
            ResourceRequest: true,
        }

And possibly code to verify existence of the service account object in question in the namespace?

erictune · 2016-03-30T16:58:05Z

pkg/apiserver/handlers.go

+			Verb:     "act-as",
+			APIGroup: api.GroupName,
+			Resource: "users",
+			// ResourceName:    requestedSubject,


If I approve the "GetExtra" PR the would you put requestor user.Info.GetExtras["full-username"] or something like that?

If I approve the "GetExtra" PR the would you put requestor user.Info.GetExtras["full-username"] or something like that?

I think I'd like to namespace the key, but I'm ok with that. Would you prefer that to filling in something more structured on the context itself?

deads2k · 2016-03-30T17:13:49Z

For your item 2, David, are you suggesting something like this for a service account:

Yes.

And possibly code to verify existence of the service account object in question in the namespace?

I don't know that I'd force existence. I could imagine it being useful to impersonate someone who can't actually log in themselves.

k8s-bot · 2016-04-05T13:56:13Z

GCE e2e build/test failed for commit a40dff392f38e3428ae37a3fd57282b0083aeb4c.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

k8s-bot · 2016-04-06T14:14:02Z

GCE e2e build/test passed for commit 11a328b48ecedb70a5d57cdde8b8b8553e041532.

roberthbailey · 2016-04-06T18:35:03Z

/cc @cjcullen

k8s-github-robot · 2016-04-08T14:15:36Z

@k8s-bot test this

Tests are more than 48 hours old. Re-running tests.

k8s-bot · 2016-04-08T14:47:25Z

GCE e2e build/test passed for commit 11a328b48ecedb70a5d57cdde8b8b8553e041532.

k8s-github-robot · 2016-04-10T14:50:19Z

@k8s-bot test this

Tests are more than 48 hours old. Re-running tests.

k8s-bot · 2016-04-10T15:18:08Z

GCE e2e build/test passed for commit 11a328b48ecedb70a5d57cdde8b8b8553e041532.

k8s-github-robot · 2016-04-12T15:24:47Z

@k8s-bot test this

Tests are more than 48 hours old. Re-running tests.

k8s-bot · 2016-04-12T16:01:51Z

GCE e2e build/test passed for commit 11a328b48ecedb70a5d57cdde8b8b8553e041532.

k8s-github-robot · 2016-04-14T13:34:13Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-04-14T14:06:14Z

GCE e2e build/test passed for commit 11a328b48ecedb70a5d57cdde8b8b8553e041532.

k8s-github-robot · 2016-04-14T15:42:50Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-bot · 2016-04-14T16:24:12Z

GCE e2e build/test passed for commit 11a328b48ecedb70a5d57cdde8b8b8553e041532.

k8s-bot · 2016-04-14T17:26:58Z

GCE e2e build/test passed for commit ac4c545.

k8s-reviewable · 2017-03-23T14:28:56Z

This change is

googlebot added the cla: yes label Mar 28, 2016

k8s-github-robot assigned lavalamp Mar 28, 2016

k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 28, 2016

deads2k force-pushed the acting-as branch from dc71877 to 85844c4 Compare March 28, 2016 19:58

deads2k mentioned this pull request Mar 28, 2016

add acting-as request handling openshift/origin#8006

Merged

deads2k mentioned this pull request Mar 28, 2016

Added proposal for "acting-as". #16933

Closed

erictune reviewed Mar 28, 2016
View reviewed changes

lavalamp reviewed Mar 28, 2016
View reviewed changes

deads2k force-pushed the acting-as branch 2 times, most recently from 7469b3a to 4a98512 Compare March 29, 2016 12:33

k8s-github-robot added the kind/old-docs label Mar 29, 2016

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

erictune reviewed Mar 30, 2016
View reviewed changes

deads2k force-pushed the acting-as branch from 4a98512 to c97040a Compare March 30, 2016 19:58

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

deads2k force-pushed the acting-as branch from a40dff3 to 8f36240 Compare April 5, 2016 19:21

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 5, 2016

deads2k added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 5, 2016

deads2k force-pushed the acting-as branch from 8f36240 to 11a328b Compare April 6, 2016 13:38

deads2k added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Apr 6, 2016

add act-as powers

ac4c545

deads2k force-pushed the acting-as branch from 11a328b to ac4c545 Compare April 14, 2016 16:49

deads2k added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Apr 14, 2016

lavalamp merged commit 1d96b4c into kubernetes:master Apr 14, 2016

aanm mentioned this pull request May 13, 2016

Track which user created resources #13925

Closed

erictune mentioned this pull request Jun 9, 2016

Finish implementing Acting-As #27152

Closed

deads2k deleted the acting-as branch September 6, 2016 17:22

add act-as powers #23549

add act-as powers #23549

Conversation

deads2k commented Mar 28, 2016 • edited by liggitt Loading

k8s-bot commented Mar 28, 2016

k8s-bot commented Mar 28, 2016

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

smarterclayton Mar 29, 2016 via email

Choose a reason for hiding this comment

erictune commented Mar 28, 2016

Choose a reason for hiding this comment

deads2k commented Mar 29, 2016

k8s-bot commented Mar 29, 2016

k8s-bot commented Mar 29, 2016

erictune commented Mar 30, 2016

liggitt commented Mar 30, 2016

erictune commented Mar 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Mar 30, 2016

k8s-bot commented Apr 5, 2016

k8s-bot commented Apr 6, 2016

roberthbailey commented Apr 6, 2016

k8s-github-robot commented Apr 8, 2016

k8s-bot commented Apr 8, 2016

k8s-github-robot commented Apr 10, 2016

k8s-bot commented Apr 10, 2016

k8s-github-robot commented Apr 12, 2016

k8s-bot commented Apr 12, 2016

k8s-github-robot commented Apr 14, 2016

k8s-bot commented Apr 14, 2016

k8s-github-robot commented Apr 14, 2016

k8s-bot commented Apr 14, 2016

k8s-bot commented Apr 14, 2016

k8s-reviewable commented Mar 23, 2017

deads2k commented Mar 28, 2016 •

edited by liggitt

Loading