Skip to content

Impersonate user for ns update during project update #11647

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Impersonate user for ns update during project update #11647

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions pkg/cmd/server/origin/master.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


kapi "k8s.io/kubernetes/pkg/api"
"k8s.io/kubernetes/pkg/api/meta"
Expand Down Expand Up @@ -774,7 +774,7 @@ func (c *MasterConfig) GetRestStorage() map[unversioned.GroupVersion]map[string]
}
deployConfigRollbackStorage := deployrollback.NewREST(configClient, kclient, c.ExternalVersionCodec)

projectStorage := projectproxy.NewREST(c.PrivilegedLoopbackKubernetesClientset.Core().Namespaces(), c.ProjectAuthorizationCache, c.ProjectAuthorizationCache, c.ProjectCache)
projectStorage := projectproxy.NewREST(c.PrivilegedLoopbackKubernetesClientset.Core().Namespaces(), c.PrivilegedLoopbackClientConfig, c.ProjectAuthorizationCache, c.ProjectAuthorizationCache, c.ProjectCache)

namespace, templateName, err := configapi.ParseNamespaceAndName(c.Options.ProjectConfig.ProjectRequestTemplate)
if err != nil {
Expand Down
13 changes: 8 additions & 5 deletions pkg/project/api/validation/validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ import (
"github.com/openshift/origin/pkg/util/labelselector"
)

// ImmutableProjectFieldError is used to determine if a field.Error was caused by changing an immutable project field
const ImmutableProjectFieldError = "field is immutable, try updating the namespace"

func ValidateProjectName(name string, prefix bool) []string {
if reasons := path.ValidatePathSegmentName(name, prefix); len(reasons) != 0 {
return reasons
Expand Down Expand Up @@ -61,14 +64,14 @@ func ValidateProjectUpdate(newProject *api.Project, oldProject *api.Project) fie
allErrs = append(allErrs, field.Invalid(field.NewPath("status"), oldProject.Spec.Finalizers, "field is immutable"))
}

// TODO this restriction exists because our authorizer/admission cannot properly express and restrict mutation on the field level.
// This restriction exists because our authorizer/admission cannot properly express and restrict mutation on the field level.
for name, value := range newProject.Annotations {
if name == oapi.OpenShiftDisplayName || name == oapi.OpenShiftDescription {
continue
}

if value != oldProject.Annotations[name] {
allErrs = append(allErrs, field.Invalid(field.NewPath("metadata", "annotations").Key(name), value, "field is immutable, try updating the namespace"))
allErrs = append(allErrs, field.Invalid(field.NewPath("metadata", "annotations").Key(name), value, ImmutableProjectFieldError))
}
}
// check for deletions
Expand All @@ -77,18 +80,18 @@ func ValidateProjectUpdate(newProject *api.Project, oldProject *api.Project) fie
continue
}
if _, inNew := newProject.Annotations[name]; !inNew {
allErrs = append(allErrs, field.Invalid(field.NewPath("metadata", "annotations").Key(name), value, "field is immutable, try updating the namespace"))
allErrs = append(allErrs, field.Invalid(field.NewPath("metadata", "annotations").Key(name), value, ImmutableProjectFieldError))
}
}

for name, value := range newProject.Labels {
if value != oldProject.Labels[name] {
allErrs = append(allErrs, field.Invalid(field.NewPath("metadata", "labels").Key(name), value, "field is immutable, , try updating the namespace"))
allErrs = append(allErrs, field.Invalid(field.NewPath("metadata", "labels").Key(name), value, ImmutableProjectFieldError))
}
}
for name, value := range oldProject.Labels {
if _, inNew := newProject.Labels[name]; !inNew {
allErrs = append(allErrs, field.Invalid(field.NewPath("metadata", "labels").Key(name), value, "field is immutable, try updating the namespace"))
allErrs = append(allErrs, field.Invalid(field.NewPath("metadata", "labels").Key(name), value, ImmutableProjectFieldError))
}
}

Expand Down
45 changes: 44 additions & 1 deletion pkg/project/registry/project/proxy/proxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,20 @@ import (
"k8s.io/kubernetes/pkg/api/rest"
"k8s.io/kubernetes/pkg/api/unversioned"
kcoreclient "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/typed/core/internalversion"
"k8s.io/kubernetes/pkg/client/restclient"
nsregistry "k8s.io/kubernetes/pkg/registry/core/namespace"
"k8s.io/kubernetes/pkg/runtime"
kstorage "k8s.io/kubernetes/pkg/storage"
"k8s.io/kubernetes/pkg/util/validation/field"
"k8s.io/kubernetes/pkg/watch"

oapi "github.com/openshift/origin/pkg/api"
authclient "github.com/openshift/origin/pkg/auth/client"
authorizationapi "github.com/openshift/origin/pkg/authorization/api"
"github.com/openshift/origin/pkg/authorization/authorizer/scope"
"github.com/openshift/origin/pkg/project/api"
projectapi "github.com/openshift/origin/pkg/project/api"
"github.com/openshift/origin/pkg/project/api/validation"
projectauth "github.com/openshift/origin/pkg/project/auth"
projectcache "github.com/openshift/origin/pkg/project/cache"
projectregistry "github.com/openshift/origin/pkg/project/registry/project"
Expand All @@ -28,6 +32,8 @@ import (
type REST struct {
// client can modify Kubernetes namespaces
client kcoreclient.NamespaceInterface
// config to build a client that can impersonate users
config restclient.Config
// lister can enumerate project lists that enforce policy
lister projectauth.Lister
// Allows extended behavior during creation, required
Expand All @@ -40,9 +46,10 @@ type REST struct {
}

// NewREST returns a RESTStorage object that will work against Project resources
func NewREST(client kcoreclient.NamespaceInterface, lister projectauth.Lister, authCache *projectauth.AuthorizationCache, projectCache *projectcache.ProjectCache) *REST {
func NewREST(client kcoreclient.NamespaceInterface, privilegedConfig restclient.Config, lister projectauth.Lister, authCache *projectauth.AuthorizationCache, projectCache *projectcache.ProjectCache) *REST {
return &REST{
client: client,
config: privilegedConfig,
lister: lister,
createStrategy: projectregistry.Strategy,
updateStrategy: projectregistry.Strategy,
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// project validation code. Thus, as a convenience, if we see that the validation errors are all in
// the project's immutable fields, we try to impersonate the user and update the namespace directly.
if containsOnlyImmutableProjectFieldErrors(errs) {
if namespace, ok := s.tryImpersonatingNamespaceUpdate(project, ctx); ok {
return projectutil.ConvertNamespace(namespace), false, nil
}
}
return nil, false, kerrors.NewInvalid(projectapi.Kind("Project"), project.Name, errs)
}

Expand All @@ -167,6 +182,34 @@ func (s *REST) Update(ctx kapi.Context, name string, objInfo rest.UpdatedObjectI
return projectutil.ConvertNamespace(namespace), false, nil
}

func (s *REST) tryImpersonatingNamespaceUpdate(project *api.Project, ctx kapi.Context) (*kapi.Namespace, bool) {
user, ok := kapi.UserFrom(ctx)
if !ok {
return nil, false
}

impersonatingClient, err := authclient.NewImpersonatingKubernetesClientset(user, s.config)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. request to update project
  2. request to update namespace

Now you're getting:

  1. request to update project
  2. request to update project as someone else

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The audit flow as I see it now:

Success case

  1. Request by user to update project
  2. No validation error
  3. Request by loopback client to update namespace
  4. Success

Failure case

  1. Request by user to update project
  2. Validation error
  3. Failure

The audit flow if this PR is merged:

Success case 1

  1. Request by user to update project
  2. No validation error
  3. Request by loopback client to update namespace
  4. Success

Success case 2

  1. Request by user to update project
  2. Immutable field error
  3. Request by loopback client that is impersonating the user to update namespace
  4. User can update namespaces
  5. Success

Failure case 1

  1. Request by user to update project
  2. Non immutable field error
  3. Failure

Failure case 2

  1. Request by user to update project
  2. Immutable field error
  3. Request by loopback client that is impersonating the user to update namespace
  4. User cannot update namespaces
  5. Failure

if err != nil {
return nil, false
}

namespace, err := impersonatingClient.Core().Namespaces().Update(projectutil.ConvertProject(project))
if err != nil {
return nil, false
}

return namespace, true // There was no error updating the namespace so the user has the rights to do so
}

func containsOnlyImmutableProjectFieldErrors(errs field.ErrorList) bool {
for _, err := range errs {
if err.Type != field.ErrorTypeInvalid || err.Detail != validation.ImmutableProjectFieldError {
return false
}
}
return true
}

var _ = rest.Deleter(&REST{})

// Delete deletes a Project specified by its name
Expand Down
7 changes: 4 additions & 3 deletions pkg/project/registry/project/proxy/proxy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"k8s.io/kubernetes/pkg/api/unversioned"
"k8s.io/kubernetes/pkg/auth/user"
"k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/fake"
"k8s.io/kubernetes/pkg/client/restclient"

oapi "github.com/openshift/origin/pkg/api"
"github.com/openshift/origin/pkg/project/api"
Expand Down Expand Up @@ -70,7 +71,7 @@ func TestCreateProjectBadObject(t *testing.T) {

func TestCreateInvalidProject(t *testing.T) {
mockClient := &fake.Clientset{}
storage := NewREST(mockClient.Core().Namespaces(), &mockLister{}, nil, nil)
storage := NewREST(mockClient.Core().Namespaces(), restclient.Config{}, &mockLister{}, nil, nil)
_, err := storage.Create(kapi.NewContext(), &api.Project{
ObjectMeta: kapi.ObjectMeta{
Annotations: map[string]string{oapi.OpenShiftDisplayName: "h\t\ni"},
Expand All @@ -83,7 +84,7 @@ func TestCreateInvalidProject(t *testing.T) {

func TestCreateProjectOK(t *testing.T) {
mockClient := &fake.Clientset{}
storage := NewREST(mockClient.Core().Namespaces(), &mockLister{}, nil, nil)
storage := NewREST(mockClient.Core().Namespaces(), restclient.Config{}, &mockLister{}, nil, nil)
_, err := storage.Create(kapi.NewContext(), &api.Project{
ObjectMeta: kapi.ObjectMeta{Name: "foo"},
})
Expand All @@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Expand Down
34 changes: 33 additions & 1 deletion test/cmd/projects.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,39 @@ os::cmd::expect_success_and_not_text 'oc config view -o jsonpath="{.contexts[*].
os::cmd::try_until_text 'oc projects' 'test6'
os::cmd::expect_success_and_text 'oc project test6' 'Now using project "test6"'
os::cmd::expect_success_and_text 'oc config view -o jsonpath="{.contexts[*].context.namespace}"' '\btest6\b'
echo 'projects command ok'

# test if namespace is updated when the user tries to edit immutable fields of a project but has rights to edit the namespace
os::test::junit::declare_suite_start "cmd/projects/namespace_update"
os::cmd::expect_success_and_text 'oc login -u nstest -p nstest' 'Login successful.' # login standard user
os::cmd::expect_success_and_text 'oc new-project nstestproj' 'Now using project "nstestproj" on server ' # make project
# test that standard user 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 \(Forbidden\): User "nstest" cannot "patch" "namespaces" with name "nstestproj" in project "nstestproj"'
os::cmd::expect_success_and_text 'oc login -u system:admin -n default' 'Login successful.|system:admin' # login as admin
# test that admin user can edit all fields regardless of which endpoint they go through
os::cmd::expect_success_and_text 'oc annotate project nstestproj key3=val3 --overwrite' 'project "nstestproj" annotated'
os::cmd::expect_success_and_text 'oc annotate namespace nstestproj key4=val4 --overwrite' 'namespace "nstestproj" annotated'
os::cmd::expect_success_and_text 'oc login -u nstest -p nstest' 'Login successful.' # login standard user again
# 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 \(Forbidden\): 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
os::cmd::expect_success_and_text 'oc annotate project nstestproj key3=val3 --overwrite' 'project "nstestproj" annotated'
# this fails early at the authorization stage even though there is no diff
os::cmd::expect_failure_and_text 'oc annotate namespace nstestproj key4=val4 --overwrite' 'Error from server \(Forbidden\): User "nstest" cannot "patch" "namespaces" with name "nstestproj" in project "nstestproj"'
# same tests as above but with labels instead of annotations
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 \(Forbidden\): User "nstest" cannot "patch" "namespaces" with name "nstestproj" in project "nstestproj"'
os::cmd::expect_success_and_text 'oc login -u system:admin -n default' 'Login successful.|system:admin'
os::cmd::expect_success_and_text 'oc label project nstestproj keyC=valC --overwrite' 'project "nstestproj" labeled'
os::cmd::expect_success_and_text 'oc label namespace nstestproj keyD=valD --overwrite' 'namespace "nstestproj" labeled'
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 \(Forbidden\): 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'
os::cmd::expect_failure_and_text 'oc label namespace nstestproj keyD=valD --overwrite' 'Error from server \(Forbidden\): User "nstest" cannot "patch" "namespaces" with name "nstestproj" in project "nstestproj"'
os::test::junit::declare_suite_end

echo 'projects command ok'

os::test::junit::declare_suite_end