switch to upstream impersonation #16155

deads2k · 2017-09-05T20:01:14Z

This switches us onto the upstream impersonation filter. That means a couple noteworthy things:

we no longer have separate permissions for impersonating system users (just like upstream)
if you granted powers for particular systemusers, you need to add to your role.
we no longer auto-add known openshift groups. You can specify --as-groups if you need the power. Our examples are users granted powers as users.

@openshift/sig-security
@liggitt as we discussed.

impersonation no longer applies group information automatically, so you may need to use `--as-groups` and now all users and groups are authorized against "users" or "groups", not "systemusers" or "systemgroups", so you may need to update roles

simo5 · 2017-09-05T20:07:05Z

Is this going to cause upgrade issues on existing clusters ?

enj · 2017-09-05T20:19:14Z

we no longer have separate permissions for impersonating system users (just like upstream)

I liked having the distinction, but it is probably is not worth the cost to maintain / be different than kube.

if you granted powers for particular systemusers, you need to add to your role.

This is a breaking change, so I assume we need to have something to reconcile this?

we no longer auto-add known openshift groups. You can specify --as-groups if you need the power. Our examples are users granted powers as users.

This is probably going to annoy / break people in general. Some specific issues I can see are the differences between user foo does a can-i vs user bar impersonating user foo does a can-i. Basically, the more you use groups, the more confusing this change will be.

With all of that said, impersonation is a very powerful and rarely used (by normal users) feature. So we may be able to get away with this. But I feel like we did not have release notes around this in 3.6, so it seems out of place to have such drastic changes.

deads2k · 2017-09-05T21:11:46Z

This is a breaking change, so I assume we need to have something to reconcile this?

This pull adds the permission for the stock roles (only one). I suspect it's extremely uncommon. If you wanted to make a migration tool it would be possible.

Is this going to cause upgrade issues on existing clusters ?

It's possible, though fairly unlikely. It's an advanced use-case, it's not recommended for normal operation other than sudoer (which will still work), and no users can do it by default.

With all of that said, impersonation is a very powerful and rarely used (by normal users) feature. So we may be able to get away with this. But I feel like we did not have release notes around this in 3.6, so it seems out of place to have such drastic changes.

We will do it at some point, it's just a question of when you pull off the bandaid. Now seems as good a time as any. The CLI already has the updates required to describe what needs changing.

liggitt · 2017-09-05T21:15:50Z

The release where we switch to upstream RBAC objects is a sensible time to make this change as well, given that upstream bindings don't have system users and system groups. I think the benefit of consistency with upstream and the limited impact make this a reasonable time to make the switch. We should still document it well in the release notes.

enj · 2017-09-06T01:46:41Z

@smarterclayton @pweil- @eparis any comments?

soltysh

+1 on my end

deads2k · 2017-09-06T16:24:01Z

test/cmd/quota.sh

@@ -13,7 +13,7 @@ os::test::junit::declare_suite_start "cmd/quota"

 os::test::junit::declare_suite_start "cmd/quota/clusterquota"

-os::cmd::expect_success 'oc new-project quota-foo --as=deads'
+os::cmd::expect_success 'oc new-project quota-foo --as=deads --as-group=system:authenticated --as-group=system:authenticated:oauth'


this is probably the most painful part of the switch. We have a non-standard group applied to our oauth users and we use it only for project requests.

deads2k · 2017-09-06T16:28:39Z

I added the translation for the existing scope headers and opened #16175 to fix our hardcoded clients to work against both openshift and kube.

enj · 2017-09-06T16:32:21Z

pkg/cmd/server/bootstrappolicy/policy.go

@@ -924,6 +932,11 @@ func GetOpenshiftBootstrapClusterRoleBindings() []rbac.ClusterRoleBinding {
 		newOriginClusterBinding(MasterRoleBindingName, MasterRoleName).
 			Groups(MastersGroup).
 			BindingOrDie(),
+		// Everyone should be able to add a scope to their impersonation request.  It is purely tightening.
+		// This does not grant access to impersonate in general, only tighten if you already have permission.
+		newOriginClusterBinding(ScopeImpersonationRoleName, ScopeImpersonationRoleName).


Just use rbac.NewClusterBinding directly since do not need the origin wrapper.

Just use rbac.NewClusterBinding directly since do not need the origin wrapper.

This just matches the others around it

We already use rbac.NewClusterBinding in the places where the role and binding have the same name (there is at least one like that near the bottom).

enj · 2017-09-06T16:34:41Z

pkg/cmd/server/handlers/impersonation_scopes.go

+// TranslateLegacyScopeImpersonation is a filter that will translates user scope impersonation for openshift into the equivalent kube headers.
+func TranslateLegacyScopeImpersonation(handler http.Handler) http.Handler {
+	return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
+		for _, scope := range req.Header[authenticationapi.ImpersonateUserScopeHeader] {


Delete the original headers when you are done?

Delete the original headers when you are done?

I was going for as little mutation as possible. I could if you really want though.

as little mutation as possible

Sounds reasonable enough to me.

enj · 2017-09-06T16:50:35Z

I think in regards to dropping groups in impersonation, there is not much we can do to make the transition better. Since it will be annoying no matter when we do it, 3.7 seems a reasonable time as any (with the appropriate release notes).

In regards to the system[users|groups] distinction we could:

In 3.7:

Do everything is this PR
Have good 3.7 release notes around the deprecation of using system[users|groups] in impersonation {cluster}roles
Extend newAuthorizationAttributeBuilder with another RequestInfoResolver that automatically converts systemusers to users and systemgroups to groups if the verb is impersonate

In 3.8:

We drop the new RequestInfoResolver
We drop the constants for system[users|groups], and completely remove the concept from the API (conversion, roles, etc)
We add a 3.8 ansible pre-upgrade check (so 3.7 to 3.8) that fails the upgrade if any impersonation {cluster}roles exist that reference system[users|groups]

deads2k · 2017-09-06T17:13:54Z

In 3.8:

We drop the new RequestInfoResolver
We drop the constants for system[users|groups], and completely remove the concept from the API (conversion, roles, etc)
We add a 3.8 ansible pre-upgrade check (so 3.7 to 3.8) that fails the upgrade if any impersonation {cluster}roles exist that reference system[users|groups]

Why wouldn't we pull the bandaid off all at once. Doing it like this is likely to break people once for the "no auto groups" change in 3.7 and again for the "you forgot to change your role" in 3.8.

I wouldn't recommend doing the ansible pre-upgrade check since tightening rules hasn't been done in the field yet.

enj · 2017-09-06T17:19:59Z

Why wouldn't we pull the bandaid off all at once. Doing it like this is likely to break people once for the "no auto groups" change in 3.7 and again for the "you forgot to change your role" in 3.8.

If we want to break people as little as possible, then a new permanent RequestInfoResolver seems appropriate.

I wouldn't recommend doing the ansible pre-upgrade check since tightening rules hasn't been done in the field yet.

We could make the check smarter by only failing if system[users|groups] exist without the matching [users|groups].

deads2k · 2017-09-06T17:21:46Z

If we want to break people as little as possible, then a new permanent RequestInfoResolver seems appropriate.

But that's not what you'll be doing. Say a year from now we're running on top of kube. You'll break them all again right there. This change isn't about us tweaking things for our special snowflake to make it prettier. It's about getting rid of the snowflake entirely.

deads2k · 2017-09-06T17:28:43Z

Extend newAuthorizationAttributeBuilder with another RequestInfoResolver that automatically converts systemusers to users and systemgroups to groups if the verb is impersonate

What would this do anyway? The old rules are going to grant powers to systemusers, so this would be stomping the wrong direction. Doing it in the reverse direction would break every existing users rule. It doesn't seem like it works.

enj · 2017-09-06T17:32:59Z

What would this do anyway? The old rules are going to grant powers to systemusers, so this would be stomping the wrong direction. Doing it in the reverse direction would break every existing users rule. It doesn't seem like it works.

My head hurts.

@deads2k has convinced me on IRC that this is the way to go.

/lgtm

openshift-merge-robot · 2017-09-06T17:33:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, enj

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

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

~~pkg/cmd/OWNERS~~ [deads2k,enj]
~~test/cmd/OWNERS~~ [deads2k,enj]
~~test/testdata/OWNERS~~ [deads2k,enj]

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

openshift-bot · 2017-09-07T06:27:10Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-merge-robot · 2017-09-07T09:23:25Z

Automatic merge from submit-queue (batch tested with PRs 16098, 16155)

@liggitt

Automatic merge from submit-queue (batch tested with PRs 16098, 16155) switch to upstream impersonation This switches us onto the upstream impersonation filter. That means a couple noteworthy things: 1. we no longer have separate permissions for impersonating system users (just like upstream) 2. if you granted powers for particular systemusers, you need to add to your role. 3. we no longer auto-add known openshift groups. You can specify --as-groups if you need the power. Our examples are users granted powers as users. @openshift/sig-security @liggitt as we discussed. ```release-note impersonation no longer applies group information automatically, so you may need to use `--as-groups` and now all users and groups are authorized against "users" or "groups", not "systemusers" or "systemgroups", so you may need to update roles ```

deads2k assigned enj and simo5 Sep 5, 2017

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 5, 2017

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 5, 2017

enj added the needs-api-review label Sep 6, 2017

soltysh approved these changes Sep 6, 2017

View reviewed changes

deads2k force-pushed the server-41-impersonation branch from 8840aee to 7dda320 Compare September 6, 2017 16:23

openshift-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 Sep 6, 2017

deads2k commented Sep 6, 2017

View reviewed changes

deads2k force-pushed the server-41-impersonation branch from 7dda320 to 1d10b78 Compare September 6, 2017 16:25

deads2k mentioned this pull request Sep 6, 2017

switch all openshift impersonators to use kube user-extra headers #16175

Closed

enj suggested changes Sep 6, 2017

View reviewed changes

deads2k force-pushed the server-41-impersonation branch from 1d10b78 to a826468 Compare September 6, 2017 17:27

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 6, 2017

enj approved these changes Sep 6, 2017

View reviewed changes

switch to upstream impersonation

60b5fc9

deads2k force-pushed the server-41-impersonation branch from a826468 to 60b5fc9 Compare September 6, 2017 19:12

deads2k added lgtm Indicates that a PR is ready to be merged. and removed lgtm Indicates that a PR is ready to be merged. labels Sep 6, 2017

enj mentioned this pull request Sep 6, 2017

RBAC Migration Followup #23: Investigate distinctions between different user/group kinds #16032

Closed

openshift-merge-robot merged commit 664f7ea into openshift:master Sep 7, 2017

deads2k deleted the server-41-impersonation branch January 24, 2018 14:33

liggitt mentioned this pull request Feb 5, 2018

oc impersonification doesn't work on v3.7.1 #18441

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

switch to upstream impersonation #16155

switch to upstream impersonation #16155

deads2k commented Sep 5, 2017 •

edited

simo5 commented Sep 5, 2017

enj commented Sep 5, 2017

deads2k commented Sep 5, 2017

liggitt commented Sep 5, 2017

enj commented Sep 6, 2017

soltysh left a comment

deads2k Sep 6, 2017

deads2k commented Sep 6, 2017

enj Sep 6, 2017

deads2k Sep 6, 2017

enj Sep 6, 2017

enj Sep 6, 2017

deads2k Sep 6, 2017

enj Sep 6, 2017

enj commented Sep 6, 2017 •

edited

deads2k commented Sep 6, 2017

enj commented Sep 6, 2017

deads2k commented Sep 6, 2017

deads2k commented Sep 6, 2017

enj commented Sep 6, 2017

openshift-merge-robot commented Sep 6, 2017

openshift-bot commented Sep 7, 2017

openshift-merge-robot commented Sep 7, 2017

switch to upstream impersonation #16155

switch to upstream impersonation #16155

Conversation

deads2k commented Sep 5, 2017 • edited

simo5 commented Sep 5, 2017

enj commented Sep 5, 2017

deads2k commented Sep 5, 2017

liggitt commented Sep 5, 2017

enj commented Sep 6, 2017

soltysh left a comment

Choose a reason for hiding this comment

deads2k Sep 6, 2017

Choose a reason for hiding this comment

deads2k commented Sep 6, 2017

enj Sep 6, 2017

Choose a reason for hiding this comment

deads2k Sep 6, 2017

Choose a reason for hiding this comment

enj Sep 6, 2017

Choose a reason for hiding this comment

enj Sep 6, 2017

Choose a reason for hiding this comment

deads2k Sep 6, 2017

Choose a reason for hiding this comment

enj Sep 6, 2017

Choose a reason for hiding this comment

enj commented Sep 6, 2017 • edited

deads2k commented Sep 6, 2017

enj commented Sep 6, 2017

deads2k commented Sep 6, 2017

deads2k commented Sep 6, 2017

enj commented Sep 6, 2017

openshift-merge-robot commented Sep 6, 2017

openshift-bot commented Sep 7, 2017

openshift-merge-robot commented Sep 7, 2017

deads2k commented Sep 5, 2017 •

edited

enj commented Sep 6, 2017 •

edited