Merge pull request #13905 from smarterclayton/make_route_set_require_…

…permission

Merged by openshift-bot

pkg/cmd/server/bootstrappolicy/policy.go

@@ -355,6 +355,8 @@ func GetOpenshiftBootstrapClusterRoles() []authorizationapi.ClusterRole {
 				authorizationapi.NewRule(read...).Groups(quotaGroup, legacyQuotaGroup).Resources("appliedclusterresourcequotas").RuleOrDie(),
 
 				authorizationapi.NewRule(readWrite...).Groups(routeGroup, legacyRouteGroup).Resources("routes").RuleOrDie(),
+				// admins can create routes with custom hosts
+				authorizationapi.NewRule("create").Groups(routeGroup, legacyRouteGroup).Resources("routes/custom-host").RuleOrDie(),
 				authorizationapi.NewRule(read...).Groups(routeGroup, legacyRouteGroup).Resources("routes/status").RuleOrDie(),
 				// an admin can run routers that write back conditions to the route
 				authorizationapi.NewRule("update").Groups(routeGroup, legacyRouteGroup).Resources("routes/status").RuleOrDie(),
@@ -413,6 +415,8 @@ func GetOpenshiftBootstrapClusterRoles() []authorizationapi.ClusterRole {
 				authorizationapi.NewRule(read...).Groups(quotaGroup, legacyQuotaGroup).Resources("appliedclusterresourcequotas").RuleOrDie(),
 
 				authorizationapi.NewRule(readWrite...).Groups(routeGroup, legacyRouteGroup).Resources("routes").RuleOrDie(),
+				// editors can create routes with custom hosts
+				authorizationapi.NewRule("create").Groups(routeGroup, legacyRouteGroup).Resources("routes/custom-host").RuleOrDie(),
 				authorizationapi.NewRule(read...).Groups(routeGroup, legacyRouteGroup).Resources("routes/status").RuleOrDie(),
 
 				authorizationapi.NewRule(readWrite...).Groups(templateGroup, legacyTemplateGroup).Resources("templates", "templateconfigs", "processedtemplates", "templateinstances").RuleOrDie(),

pkg/cmd/server/origin/master.go

@@ -631,11 +631,6 @@ func (c *MasterConfig) GetRestStorage() map[schema.GroupVersion]map[string]rest.
 	checkStorageErr(err)
 	deployConfigRegistry := deployconfigregistry.NewRegistry(deployConfigStorage)
 
-	routeAllocator := c.RouteAllocator()
-
-	routeStorage, routeStatusStorage, err := routeetcd.NewREST(c.RESTOptionsGetter, routeAllocator)
-	checkStorageErr(err)
-
 	hostSubnetStorage, err := hostsubnetetcd.NewREST(c.RESTOptionsGetter)
 	checkStorageErr(err)
 	netNamespaceStorage, err := netnamespaceetcd.NewREST(c.RESTOptionsGetter)
@@ -721,6 +716,10 @@ func (c *MasterConfig) GetRestStorage() map[schema.GroupVersion]map[string]rest.
 	imageStreamImageStorage := imagestreamimage.NewREST(imageRegistry, imageStreamRegistry)
 	imageStreamImageRegistry := imagestreamimage.NewRegistry(imageStreamImageStorage)
 
+	routeAllocator := c.RouteAllocator()
+	routeStorage, routeStatusStorage, err := routeetcd.NewREST(c.RESTOptionsGetter, routeAllocator, subjectAccessReviewRegistry)
+	checkStorageErr(err)
+
 	buildGenerator := &buildgenerator.BuildGenerator{
 		Client: buildgenerator.Client{
 			GetBuildConfigFunc:      buildConfigRegistry.GetBuildConfig,

pkg/ingress/admission/ingress_admission.go

@@ -7,10 +7,15 @@ import (
 	"io"
 	"reflect"
 
+	"k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/util/sets"
+	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/apiserver/pkg/admission"
+	"k8s.io/apiserver/pkg/authorization/authorizer"
 	kextensions "k8s.io/kubernetes/pkg/apis/extensions"
 
+	oadmission "github.com/openshift/origin/pkg/cmd/server/admission"
 	configlatest "github.com/openshift/origin/pkg/cmd/server/api/latest"
 	"github.com/openshift/origin/pkg/ingress/admission/api"
 )
@@ -31,9 +36,12 @@ func init() {
 
 type ingressAdmission struct {
 	*admission.Handler
-	config *api.IngressAdmissionConfig
+	config     *api.IngressAdmissionConfig
+	authorizer authorizer.Authorizer
 }
 
+var _ = oadmission.WantsAuthorizer(&ingressAdmission{})
+
 func NewIngressAdmission(config *api.IngressAdmissionConfig) *ingressAdmission {
 	return &ingressAdmission{
 		Handler: admission.NewHandler(admission.Create, admission.Update),
@@ -60,19 +68,77 @@ func readConfig(reader io.Reader) (*api.IngressAdmissionConfig, error) {
 	return config, nil
 }
 
+func (r *ingressAdmission) SetAuthorizer(a authorizer.Authorizer) {
+	r.authorizer = a
+}
+
+func (r *ingressAdmission) Validate() error {
+	if r.authorizer == nil {
+		return fmt.Errorf("%s needs an Openshift Authorizer", IngressAdmission)
+	}
+	return nil
+}
+
 func (r *ingressAdmission) Admit(a admission.Attributes) error {
-	if a.GetResource().GroupResource() == kextensions.Resource("ingresses") && a.GetOperation() == admission.Update {
-		if r.config == nil || r.config.AllowHostnameChanges == false {
-			oldIngress, ok := a.GetOldObject().(*kextensions.Ingress)
-			if !ok {
-				return nil
-			}
-			newIngress, ok := a.GetObject().(*kextensions.Ingress)
-			if !ok {
-				return nil
+	if a.GetResource().GroupResource() == kextensions.Resource("ingresses") {
+		switch a.GetOperation() {
+		case admission.Create:
+			if ingress, ok := a.GetObject().(*kextensions.Ingress); ok {
+				// if any rules have a host, check whether the user has permission to set them
+				for i, rule := range ingress.Spec.Rules {
+					if len(rule.Host) > 0 {
+						attr := authorizer.AttributesRecord{
+							User:            a.GetUserInfo(),
+							Verb:            "create",
+							Namespace:       a.GetNamespace(),
+							Resource:        "routes",
+							Subresource:     "custom-host",
+							APIGroup:        "route.openshift.io",
+							ResourceRequest: true,
+						}
+						kind := schema.GroupKind{Group: a.GetResource().Group, Kind: a.GetResource().Resource}
+						allow, _, err := r.authorizer.Authorize(attr)
+						if err != nil {
+							return errors.NewInvalid(kind, ingress.Name, field.ErrorList{field.InternalError(field.NewPath("spec", "rules").Index(i), err)})
+						}
+						if !allow {
+							return errors.NewInvalid(kind, ingress.Name, field.ErrorList{field.Forbidden(field.NewPath("spec", "rules").Index(i), "you do not have permission to set host fields in ingress rules")})
+						}
+						break
+					}
+				}
 			}
-			if !haveHostnamesChanged(oldIngress, newIngress) {
-				return fmt.Errorf("cannot change hostname")
+		case admission.Update:
+			if r.config == nil || r.config.AllowHostnameChanges == false {
+				oldIngress, ok := a.GetOldObject().(*kextensions.Ingress)
+				if !ok {
+					return nil
+				}
+				newIngress, ok := a.GetObject().(*kextensions.Ingress)
+				if !ok {
+					return nil
+				}
+				if !haveHostnamesChanged(oldIngress, newIngress) {
+					attr := authorizer.AttributesRecord{
+						User:            a.GetUserInfo(),
+						Verb:            "update",
+						Namespace:       a.GetNamespace(),
+						Name:            a.GetName(),
+						Resource:        "routes",
+						Subresource:     "custom-host",
+						APIGroup:        "route.openshift.io",
+						ResourceRequest: true,
+					}
+					kind := schema.GroupKind{Group: a.GetResource().Group, Kind: a.GetResource().Resource}
+					allow, _, err := r.authorizer.Authorize(attr)
+					if err != nil {
+						return errors.NewInvalid(kind, newIngress.Name, field.ErrorList{field.InternalError(field.NewPath("spec", "rules"), err)})
+					}
+					if allow {
+						return nil
+					}
+					return fmt.Errorf("cannot change hostname")
+				}
 			}
 		}
 	}

pkg/ingress/admission/ingress_admission_test.go

@@ -5,13 +5,22 @@ import (
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apiserver/pkg/admission"
+	"k8s.io/apiserver/pkg/authorization/authorizer"
 	kextensions "k8s.io/kubernetes/pkg/apis/extensions"
 
 	"github.com/openshift/origin/pkg/ingress/admission/api"
 )
 
-func TestAdmission(t *testing.T) {
+type fakeAuthorizer struct {
+	allow bool
+	err   error
+}
+
+func (a *fakeAuthorizer) Authorize(authorizer.Attributes) (bool, string, error) {
+	return a.allow, "", a.err
+}
 
+func TestAdmission(t *testing.T) {
 	var newIngress *kextensions.Ingress
 	var oldIngress *kextensions.Ingress
 
@@ -21,6 +30,7 @@ func TestAdmission(t *testing.T) {
 		oldHost, newHost string
 		op               admission.Operation
 		admit            bool
+		allow            bool
 	}{
 		{
 			admit:    true,
@@ -51,6 +61,15 @@ func TestAdmission(t *testing.T) {
 			oldHost:  "bar.com",
 			testName: "changing hostname should fail",
 		},
+		{
+			admit:    true,
+			allow:    true,
+			config:   emptyConfig(),
+			op:       admission.Update,
+			newHost:  "foo.com",
+			oldHost:  "bar.com",
+			testName: "changing hostname should succeed if the user has permission",
+		},
 		{
 			admit:    false,
 			config:   nil,
@@ -74,6 +93,22 @@ func TestAdmission(t *testing.T) {
 			newHost:  "foo.com",
 			testName: "add new hostname with upstream rules",
 		},
+		{
+			admit:    false,
+			allow:    false,
+			config:   emptyConfig(),
+			op:       admission.Create,
+			newHost:  "foo.com",
+			testName: "setting the host should require permission",
+		},
+		{
+			admit:    true,
+			allow:    true,
+			config:   emptyConfig(),
+			op:       admission.Create,
+			newHost:  "foo.com",
+			testName: "setting the host should pass if user has permission",
+		},
 	}
 	for _, test := range tests {
 		if len(test.newHost) > 0 {
@@ -94,6 +129,7 @@ func TestAdmission(t *testing.T) {
 			}
 		}
 		handler := NewIngressAdmission(test.config)
+		handler.SetAuthorizer(&fakeAuthorizer{allow: test.allow})
 
 		if len(test.oldHost) > 0 {
 			//Provides the previous state of an ingress object

pkg/route/api/validation/validation.go

@@ -15,7 +15,6 @@ import (
 	kvalidation "k8s.io/apimachinery/pkg/util/validation"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/kubernetes/pkg/api/validation"
-	kval "k8s.io/kubernetes/pkg/api/validation"
 
 	oapi "github.com/openshift/origin/pkg/api"
 	cmdutil "github.com/openshift/origin/pkg/cmd/util"
@@ -25,7 +24,7 @@ import (
 // ValidateRoute tests if required fields in the route are set.
 func ValidateRoute(route *routeapi.Route) field.ErrorList {
 	//ensure meta is set properly
-	result := kval.ValidateObjectMeta(&route.ObjectMeta, true, oapi.GetNameValidationFunc(kval.ValidatePodName), field.NewPath("metadata"))
+	result := validation.ValidateObjectMeta(&route.ObjectMeta, true, oapi.GetNameValidationFunc(validation.ValidatePodName), field.NewPath("metadata"))
 
 	specPath := field.NewPath("spec")
 
@@ -98,7 +97,6 @@ func ValidateRoute(route *routeapi.Route) field.ErrorList {
 
 func ValidateRouteUpdate(route *routeapi.Route, older *routeapi.Route) field.ErrorList {
 	allErrs := validation.ValidateObjectMetaUpdate(&route.ObjectMeta, &older.ObjectMeta, field.NewPath("metadata"))
-	allErrs = append(allErrs, validation.ValidateImmutableField(route.Spec.Host, older.Spec.Host, field.NewPath("spec", "host"))...)
 	allErrs = append(allErrs, validation.ValidateImmutableField(route.Spec.WildcardPolicy, older.Spec.WildcardPolicy, field.NewPath("spec", "wildcardPolicy"))...)
 	allErrs = append(allErrs, ValidateRoute(route)...)
 	return allErrs

pkg/route/api/validation/validation_test.go

@@ -1250,7 +1250,7 @@ func TestValidateRouteUpdate(t *testing.T) {
 				},
 			},
 			change:         func(route *api.Route) { route.Spec.Host = "" },
-			expectedErrors: 1,
+			expectedErrors: 0, // now controlled by rbac
 		},
 		{
 			route: &api.Route{
@@ -1268,7 +1268,7 @@ func TestValidateRouteUpdate(t *testing.T) {
 				},
 			},
 			change:         func(route *api.Route) { route.Spec.Host = "other" },
-			expectedErrors: 1,
+			expectedErrors: 0, // now controlled by rbac
 		},
 		{
 			route: &api.Route{

pkg/route/registry/route/etcd/etcd.go

@@ -20,8 +20,8 @@ type REST struct {
 }
 
 // NewREST returns a RESTStorage object that will work against routes.
-func NewREST(optsGetter restoptions.Getter, allocator route.RouteAllocator) (*REST, *StatusREST, error) {
-	strategy := rest.NewStrategy(allocator)
+func NewREST(optsGetter restoptions.Getter, allocator route.RouteAllocator, sarClient rest.SubjectAccessReviewInterface) (*REST, *StatusREST, error) {
+	strategy := rest.NewStrategy(allocator, sarClient)
 
 	store := &registry.Store{
 		Copier:            kapi.Scheme,

pkg/route/registry/route/etcd/etcd_test.go

@@ -11,6 +11,7 @@ import (
 	etcdtesting "k8s.io/apiserver/pkg/storage/etcd/testing"
 	"k8s.io/kubernetes/pkg/registry/registrytest"
 
+	authorizationapi "github.com/openshift/origin/pkg/authorization/api"
 	routetypes "github.com/openshift/origin/pkg/route"
 	"github.com/openshift/origin/pkg/route/api"
 	_ "github.com/openshift/origin/pkg/route/api/install"
@@ -34,9 +35,20 @@ func (a *testAllocator) GenerateHostname(*api.Route, *api.RouterShard) string {
 	return a.Hostname
 }
 
+type testSAR struct {
+	allow bool
+	err   error
+	sar   *authorizationapi.SubjectAccessReview
+}
+
+func (t *testSAR) CreateSubjectAccessReview(ctx apirequest.Context, subjectAccessReview *authorizationapi.SubjectAccessReview) (*authorizationapi.SubjectAccessReviewResponse, error) {
+	t.sar = subjectAccessReview
+	return &authorizationapi.SubjectAccessReviewResponse{Allowed: t.allow}, t.err
+}
+
 func newStorage(t *testing.T, allocator routetypes.RouteAllocator) (*REST, *etcdtesting.EtcdTestServer) {
 	etcdStorage, server := registrytest.NewEtcdStorage(t, "")
-	storage, _, err := NewREST(restoptions.NewSimpleGetter(etcdStorage), allocator)
+	storage, _, err := NewREST(restoptions.NewSimpleGetter(etcdStorage), allocator, &testSAR{allow: true})
 	if err != nil {
 		t.Fatal(err)
 	}