Impersonate user for ns update during project update #11647

enj · 2016-10-28T17:41:26Z

https://bugzilla.redhat.com/show_bug.cgi?id=1386401

Signed-off-by: Monis Khan [email protected]

deads2k · 2016-10-28T17:43:47Z

pkg/project/registry/project/proxy/proxy.go

 	}

 	return projectutil.ConvertNamespace(namespace), false, nil
 }

+func (s *REST) getImpersonatingClientFor(user user.Info) kclient.NamespaceInterface {


get a client config and do this:

impersonatingConfig := a.privilegedRESTClientConfig oldWrapTransport := impersonatingConfig.WrapTransport impersonatingConfig.WrapTransport = func(rt http.RoundTripper) http.RoundTripper { return authenticationclient.NewImpersonatingRoundTripper(attributes.GetUserInfo(), oldWrapTransport(rt)) }

or make a helper down in the pkg/auth/client package.

that reconstructs from config on every request, potentially down to the point of re-reading and re-parsing CA and cert files... needs to be split up to only build the base transport once

deads2k · 2016-10-28T17:48:12Z

pkg/project/registry/project/proxy/proxy.go

@@ -154,19 +158,38 @@ func (s *REST) Update(ctx kapi.Context, name string, objInfo rest.UpdatedObjectI
 		return nil, false, fmt.Errorf("not a project: %#v", obj)
 	}

-	s.updateStrategy.PrepareForUpdate(ctx, obj, oldObj)
-	if errs := s.updateStrategy.ValidateUpdate(ctx, obj, oldObj); len(errs) > 0 {


If you get a validation error and it is in metadata, you can try the impersonating update on the namespace.

Otherwise, you can escalate using your privileged client.

enj · 2016-10-31T19:53:34Z

@deads2k @liggitt PTAL. I will add some oc tests to verify the behavior. Config copying / new client creation only happens on metadata errors.

enj · 2016-10-31T19:54:07Z

[test]

enj · 2016-11-01T14:40:31Z

re[test] Flake #11661

enj · 2016-11-01T18:24:25Z

test/cmd/projects.sh

+# test that standard user still cannot edit immutable fields
+os::cmd::expect_failure_and_text 'oc annotate project nstestproj key1=val1 --overwrite' 'The Project "nstestproj" is invalid: metadata.annotations\[key1\]: Invalid value: "val1": field is immutable, try updating the namespace'
+os::cmd::expect_failure_and_text 'oc annotate namespace nstestproj key2=val2 --overwrite' 'Error from server: User "nstest" cannot "patch" "namespaces" with name "nstestproj" in project "nstestproj"'
+# this will now be successful because there is no diff between the old and new project


I would be curious to know if you guys consider this behavior valid (seems a bit confusing).

enj · 2016-11-01T19:38:43Z

test/cmd/projects.sh

+os::cmd::expect_success_and_text 'oc login -u nstest -p nstest' 'Login successful.'
+os::cmd::expect_failure_and_text 'oc label project nstestproj keyA=valA --overwrite' 'The Project "nstestproj" is invalid: metadata.labels\[keyA\]: Invalid value: "valA": field is immutable, try updating the namespace'
+os::cmd::expect_failure_and_text 'oc label namespace nstestproj keyB=valB --overwrite' 'Error from server: User "nstest" cannot "patch" "namespaces" with name "nstestproj" in project "nstestproj"'
+os::cmd::expect_success_and_text 'oc label project nstestproj keyC=valC --overwrite' 'project "nstestproj" not labeled'


oc label (unlike oc annotate) checks to see if there is a diff between the two objects when printing the success message.

pweil- · 2017-02-02T13:51:11Z

bump

deads2k · 2017-03-01T21:15:32Z

pkg/auth/client/impersonate.go

@@ -48,3 +51,21 @@ func cloneRequest(r *http.Request) *http.Request {
 	}
 	return r2
 }
+
+func buildImpersonatingConfig(user user.Info, impersonatingConfig *restclient.Config) *restclient.Config {


Why a pointer? It makes reasoning about whether you're wrapping the same transport deeper and deeper over time harder since I have to find the callers.

deads2k · 2017-03-01T21:17:41Z

pkg/cmd/server/origin/master_config.go

+
+	// PrivilegedLoopbackOpenShiftClientConfig is the client configuration used to call OpenShift APIs from system components
+	// To apply different access control to a system component, create a client config specifically for that component.
+	PrivilegedLoopbackOpenShiftClientConfig restclient.Config


Is this one actually different? We no longer allow the proxy to kube configuration.

deads2k · 2017-03-01T21:19:12Z

pkg/project/registry/project/proxy/proxy.go

 	if err != nil {
 		return nil, false, err
 	}

 	return projectutil.ConvertNamespace(namespace), false, nil
 }

+func (s *REST) tryImpersonatingNamespaceUpdateOnMetadataError(namespace *kapi.Namespace, ctx kapi.Context, errs field.ErrorList) (*kapi.Namespace, error) {
+	// If the error is in the metadata's annotations or labels, we try to impersonate the user and update the namespace
+	if containsAnnotationOrLabelMetadataError(errs) {


This decision should be made before the method call. Maybe a switch/case after checking the strategy.

deads2k · 2017-03-01T21:20:36Z

rebase, update your description, decide if its still a WIP

mfojtik · 2017-03-31T08:54:23Z

@enj ping :)

Signed-off-by: Monis Khan <[email protected]>

openshift-bot · 2017-04-06T15:05:40Z

Evaluated for origin test up to a7aae0e

enj · 2017-04-06T15:09:15Z

@deads2k Ready for another review.

I have reservations about adding something like this. Is the slight extra convenience really worth all this complexity? Very few people have access to mutate namespaces, so I feel like they should be knowledgeable enough to know the difference. My biggest concern is that any mistake on my part here could break multitenancy.

cc @liggitt @mfojtik @soltysh @smarterclayton @pweil- for thoughts.

pweil- · 2017-04-06T15:20:38Z

Is the fundamental question here whether or not the project API is supposed to be a proxy to the (highly privileged) namespace API? I would argue no but agree that the project vs namespace is confusing. Is there a better way to display the error message if the user has access to the namespace API?

enj · 2017-04-06T15:33:07Z

Is the fundamental question here whether or not the project API is supposed to be a proxy to the (highly privileged) namespace API? I would argue no but ...

We literally call the file proxy.go so I would say yes.

... agree that the project vs namespace is confusing.

I agree that it is confusing but I am not sure how to make it less so.

Is there a better way to display the error message if the user has access to the namespace API?

We could certainly do the check, but it would be an expensive loopback REST call.

openshift-bot · 2017-04-06T22:13:20Z

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/606/) (Base Commit: 4b16943)

enj · 2017-04-06T22:34:10Z

Flake #13662

soltysh

Unit test please, all the rest looks ok. Just a few questions, mostly.

soltysh · 2017-04-07T08:20:21Z

pkg/cmd/server/origin/master.go

@@ -16,7 +16,7 @@ import (
 	restful "github.com/emicklei/go-restful"
 	"github.com/golang/glog"
 	"github.com/prometheus/client_golang/prometheus"
-	"gopkg.in/natefinch/lumberjack.v2"
+	lumberjack "gopkg.in/natefinch/lumberjack.v2" // goimports will remove this if the alias is not set


whoa, that's interesting... I don't see that problem locally, but I might be using an older version of goimports.

I tried upgrading my goimports but it kept removing this...

soltysh · 2017-04-07T08:37:11Z

pkg/project/registry/project/proxy/proxy.go

@@ -156,6 +163,14 @@ func (s *REST) Update(ctx kapi.Context, name string, objInfo rest.UpdatedObjectI

 	s.updateStrategy.PrepareForUpdate(ctx, obj, oldObj)
 	if errs := s.updateStrategy.ValidateUpdate(ctx, obj, oldObj); len(errs) > 0 {
+		// If the user has the ability to update the namespace directly, they can circumvent most of the


Maybe a stupid question, but I was thought that stupid are only those questions that weren't asked ;) Is it possible to have edit rights on project but not on namespace?

Yes. As far as the authorizer is concerned, they are completely separate resources. Almost no one has rights to do anything with namespaces. Also, almost no one can create projects. This is how the projectrequest flow works (you say I want a project, the system makes a project under specific constraints, and then the underlying namespace is created).

soltysh · 2017-04-07T08:40:07Z

pkg/project/registry/project/proxy/proxy.go

+		return nil, false
+	}
+
+	impersonatingClient, err := authclient.NewImpersonatingKubernetesClientset(user, s.config)


From an audit standpoint, this will look weird. Normally, you'd get:

request to update project

request to update namespace

Now you're getting:

request to update project

request to update project as someone else

This might look fishy in the logs and for someone doing security audit.

I guess both you and @pweil- voiced that concern in the main thread. I'm looking from an audit point of view ;)

The audit flow as I see it now:

Success case

Request by user to update project

No validation error

Request by loopback client to update namespace

Success

Failure case

Request by user to update project

Validation error

Failure

The audit flow if this PR is merged:

Success case 1

Request by user to update project

No validation error

Request by loopback client to update namespace

Success

Success case 2

Request by user to update project

Immutable field error

Request by loopback client that is impersonating the user to update namespace

User can update namespaces

Success

Failure case 1

Request by user to update project

Non immutable field error

Failure

Failure case 2

Request by user to update project

Immutable field error

Request by loopback client that is impersonating the user to update namespace

User cannot update namespaces

Failure

soltysh · 2017-04-07T08:41:46Z

pkg/project/registry/project/proxy/proxy_test.go

@@ -100,7 +101,7 @@ func TestCreateProjectOK(t *testing.T) {

 func TestGetProjectOK(t *testing.T) {
 	mockClient := fake.NewSimpleClientset(&kapi.Namespace{ObjectMeta: kapi.ObjectMeta{Name: "foo"}})
-	storage := NewREST(mockClient.Core().Namespaces(), &mockLister{}, nil, nil)
+	storage := NewREST(mockClient.Core().Namespaces(), restclient.Config{}, &mockLister{}, nil, nil)
 	project, err := storage.Get(kapi.NewContext(), "foo")
 	if project == nil {
 		t.Error("Unexpected nil project")


Can we have an explicit test for that new functionality here. The more tests, the better, especially when it comes to security.

smarterclayton · 2017-04-07T23:21:54Z

Rather than spending a bunch of time on this - I'd rather let users be able to edit a set of "safe" annotations on their project. I don't really care that admins have to use namespace to edit their namespace. Project is a limited permission view, with specific constraints. Why add complexity?

smarterclayton · 2017-04-07T23:22:22Z

I wish I'd seen this sooner. I would really have just preferred to add a better message.

enj · 2017-04-08T02:57:24Z

Adds too much complexity for no real gains.

deads2k · 2017-04-10T12:45:12Z

Adds too much complexity for no real gains.

Looked pretty simple to me. Oh well.

deads2k reviewed Oct 28, 2016

View reviewed changes

enj force-pushed the enj/i/ns_project_auth/1386401 branch from b7a64c3 to 81c81d0 Compare October 31, 2016 19:45

enj force-pushed the enj/i/ns_project_auth/1386401 branch 2 times, most recently from 97880d7 to a80f4d4 Compare November 1, 2016 13:15

enj force-pushed the enj/i/ns_project_auth/1386401 branch from a80f4d4 to c1641a0 Compare November 1, 2016 18:14

enj commented Nov 1, 2016

View reviewed changes

enj force-pushed the enj/i/ns_project_auth/1386401 branch 2 times, most recently from 5d97d57 to db3ccba Compare November 1, 2016 19:36

enj commented Nov 1, 2016

View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 26, 2016

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 20, 2017

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 23, 2017

enj mentioned this pull request Mar 1, 2017

Adding oc_project to lib_openshift. openshift/openshift-ansible#3325

Merged

deads2k reviewed Mar 1, 2017

View reviewed changes

openshift-bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 24, 2017

enj mentioned this pull request Apr 5, 2017

Cleanup impersonating code #13630

Merged

Impersonate user for ns update during project update

a7aae0e

Signed-off-by: Monis Khan <[email protected]>

enj force-pushed the enj/i/ns_project_auth/1386401 branch from db3ccba to a7aae0e Compare April 6, 2017 14:58

enj changed the title ~~WIP: Impersonate user for ns update during project update~~ Impersonate user for ns update during project update Apr 6, 2017

enj removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 6, 2017

enj mentioned this pull request Apr 6, 2017

oc exec hangs for six hours #13662

Closed

soltysh suggested changes Apr 7, 2017

View reviewed changes

enj closed this Apr 8, 2017

Impersonate user for ns update during project update #11647

Impersonate user for ns update during project update #11647

Uh oh!

Conversation

enj commented Oct 28, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enj commented Oct 31, 2016

Uh oh!

enj commented Oct 31, 2016

Uh oh!

enj commented Nov 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pweil- commented Feb 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deads2k commented Mar 1, 2017

Uh oh!

mfojtik commented Mar 31, 2017

Uh oh!

openshift-bot commented Apr 6, 2017

Uh oh!

enj commented Apr 6, 2017

Uh oh!

pweil- commented Apr 6, 2017

Uh oh!

enj commented Apr 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-bot commented Apr 6, 2017

Uh oh!

enj commented Apr 6, 2017

Uh oh!

soltysh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smarterclayton commented Apr 7, 2017

Uh oh!

smarterclayton commented Apr 7, 2017

Uh oh!

enj commented Apr 8, 2017

Uh oh!

deads2k commented Apr 10, 2017

Uh oh!

Uh oh!

enj commented Oct 28, 2016 •

edited

Loading

enj commented Apr 6, 2017 •

edited

Loading