diff --git a/pkg/cmd/server/bootstrappolicy/policy.go b/pkg/cmd/server/bootstrappolicy/policy.go index 49fa9c902d82..d33007533d45 100644 --- a/pkg/cmd/server/bootstrappolicy/policy.go +++ b/pkg/cmd/server/bootstrappolicy/policy.go @@ -355,6 +355,8 @@ func GetBootstrapClusterRoles() []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 GetBootstrapClusterRoles() []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(), diff --git a/pkg/cmd/server/origin/master.go b/pkg/cmd/server/origin/master.go index f008b3ae218c..2d3402a05fe5 100644 --- a/pkg/cmd/server/origin/master.go +++ b/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, diff --git a/pkg/ingress/admission/ingress_admission.go b/pkg/ingress/admission/ingress_admission.go index 4c3101a777c3..bd04b8ca03ec 100644 --- a/pkg/ingress/admission/ingress_admission.go +++ b/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") + } } } } diff --git a/pkg/ingress/admission/ingress_admission_test.go b/pkg/ingress/admission/ingress_admission_test.go index bfa88f674881..93e393129bc0 100644 --- a/pkg/ingress/admission/ingress_admission_test.go +++ b/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 diff --git a/pkg/route/api/validation/validation.go b/pkg/route/api/validation/validation.go index 74b41b8763a4..7d70d0f11b64 100644 --- a/pkg/route/api/validation/validation.go +++ b/pkg/route/api/validation/validation.go @@ -11,7 +11,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" @@ -21,7 +20,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") @@ -94,7 +93,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 diff --git a/pkg/route/api/validation/validation_test.go b/pkg/route/api/validation/validation_test.go index 683c3ebc4963..f109a2a465ce 100644 --- a/pkg/route/api/validation/validation_test.go +++ b/pkg/route/api/validation/validation_test.go @@ -1113,7 +1113,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{ @@ -1131,7 +1131,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{ diff --git a/pkg/route/registry/route/etcd/etcd.go b/pkg/route/registry/route/etcd/etcd.go index 019e14afe50b..498960c4ba46 100644 --- a/pkg/route/registry/route/etcd/etcd.go +++ b/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 := ®istry.Store{ Copier: kapi.Scheme, diff --git a/pkg/route/registry/route/etcd/etcd_test.go b/pkg/route/registry/route/etcd/etcd_test.go index b0f52e519447..9c5eadb06a15 100644 --- a/pkg/route/registry/route/etcd/etcd_test.go +++ b/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) } diff --git a/pkg/route/registry/route/strategy.go b/pkg/route/registry/route/strategy.go index 6d673b56c0e4..70b0337c6a91 100644 --- a/pkg/route/registry/route/strategy.go +++ b/pkg/route/registry/route/strategy.go @@ -3,17 +3,17 @@ package route import ( "fmt" - "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" - utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/apimachinery/pkg/util/validation/field" apirequest "k8s.io/apiserver/pkg/endpoints/request" "k8s.io/apiserver/pkg/storage" "k8s.io/apiserver/pkg/storage/names" kapi "k8s.io/kubernetes/pkg/api" + kvalidation "k8s.io/kubernetes/pkg/api/validation" + authorizationapi "github.com/openshift/origin/pkg/authorization/api" "github.com/openshift/origin/pkg/route" "github.com/openshift/origin/pkg/route/api" "github.com/openshift/origin/pkg/route/api/validation" @@ -22,19 +22,26 @@ import ( // HostGeneratedAnnotationKey is the key for an annotation set to "true" if the route's host was generated const HostGeneratedAnnotationKey = "openshift.io/host.generated" +// Registry is an interface for performing subject access reviews +type SubjectAccessReviewInterface interface { + CreateSubjectAccessReview(ctx apirequest.Context, subjectAccessReview *authorizationapi.SubjectAccessReview) (*authorizationapi.SubjectAccessReviewResponse, error) +} + type routeStrategy struct { runtime.ObjectTyper names.NameGenerator route.RouteAllocator + sarClient SubjectAccessReviewInterface } // NewStrategy initializes the default logic that applies when creating and updating // Route objects via the REST API. -func NewStrategy(allocator route.RouteAllocator) routeStrategy { +func NewStrategy(allocator route.RouteAllocator, sarClient SubjectAccessReviewInterface) routeStrategy { return routeStrategy{ - kapi.Scheme, - names.SimpleNameGenerator, - allocator, + ObjectTyper: kapi.Scheme, + NameGenerator: names.SimpleNameGenerator, + RouteAllocator: allocator, + sarClient: sarClient, } } @@ -45,11 +52,6 @@ func (routeStrategy) NamespaceScoped() bool { func (s routeStrategy) PrepareForCreate(ctx apirequest.Context, obj runtime.Object) { route := obj.(*api.Route) route.Status = api.RouteStatus{} - err := s.allocateHost(route) - if err != nil { - // TODO: this will be changed when moved to a controller - utilruntime.HandleError(errors.NewInternalError(fmt.Errorf("allocation error: %v for route: %#v", err, obj))) - } } func (s routeStrategy) PrepareForUpdate(ctx apirequest.Context, obj, old runtime.Object) { @@ -67,7 +69,36 @@ func (s routeStrategy) PrepareForUpdate(ctx apirequest.Context, obj, old runtime // allocateHost allocates a host name ONLY if the route doesn't specify a subdomain wildcard policy and // the host name on the route is empty and an allocator is configured. // It must first allocate the shard and may return an error if shard allocation fails. -func (s routeStrategy) allocateHost(route *api.Route) error { +func (s routeStrategy) allocateHost(ctx apirequest.Context, route *api.Route) field.ErrorList { + hostSet := len(route.Spec.Host) > 0 + certSet := route.Spec.TLS != nil && (len(route.Spec.TLS.CACertificate) > 0 || len(route.Spec.TLS.Certificate) > 0 || len(route.Spec.TLS.DestinationCACertificate) > 0 || len(route.Spec.TLS.Key) > 0) + if hostSet || certSet { + user, ok := apirequest.UserFrom(ctx) + if !ok { + return field.ErrorList{field.InternalError(field.NewPath("spec", "host"), fmt.Errorf("unable to verify host field can be set"))} + } + res, err := s.sarClient.CreateSubjectAccessReview( + ctx, + &authorizationapi.SubjectAccessReview{ + User: user.GetName(), + Action: authorizationapi.Action{ + Verb: "create", + Group: api.GroupName, + Resource: "routes/custom-host", + }, + }, + ) + if err != nil { + return field.ErrorList{field.InternalError(field.NewPath("spec", "host"), err)} + } + if !res.Allowed { + if hostSet { + return field.ErrorList{field.Forbidden(field.NewPath("spec", "host"), "you do not have permission to set the host field of the route")} + } + return field.ErrorList{field.Forbidden(field.NewPath("spec", "tls"), "you do not have permission to set certificate fields on the route")} + } + } + if route.Spec.WildcardPolicy == api.WildcardPolicySubdomain { // Don't allocate a host if subdomain wildcard policy. return nil @@ -77,7 +108,7 @@ func (s routeStrategy) allocateHost(route *api.Route) error { // TODO: this does not belong here, and should be removed shard, err := s.RouteAllocator.AllocateRouterShard(route) if err != nil { - return errors.NewInternalError(fmt.Errorf("allocation error: %v for route: %#v", err, route)) + return field.ErrorList{field.InternalError(field.NewPath("spec", "host"), fmt.Errorf("allocation error: %v for route: %#v", err, route))} } route.Spec.Host = s.RouteAllocator.GenerateHostname(route, shard) if route.Annotations == nil { @@ -88,9 +119,11 @@ func (s routeStrategy) allocateHost(route *api.Route) error { return nil } -func (routeStrategy) Validate(ctx apirequest.Context, obj runtime.Object) field.ErrorList { +func (s routeStrategy) Validate(ctx apirequest.Context, obj runtime.Object) field.ErrorList { route := obj.(*api.Route) - return validation.ValidateRoute(route) + errs := s.allocateHost(ctx, route) + errs = append(errs, validation.ValidateRoute(route)...) + return errs } func (routeStrategy) AllowCreateOnUpdate() bool { @@ -101,10 +134,64 @@ func (routeStrategy) AllowCreateOnUpdate() bool { func (routeStrategy) Canonicalize(obj runtime.Object) { } -func (routeStrategy) ValidateUpdate(ctx apirequest.Context, obj, old runtime.Object) field.ErrorList { +func (s routeStrategy) ValidateUpdate(ctx apirequest.Context, obj, old runtime.Object) field.ErrorList { oldRoute := old.(*api.Route) objRoute := obj.(*api.Route) - return validation.ValidateRouteUpdate(objRoute, oldRoute) + errs := s.validateHostUpdate(ctx, objRoute, oldRoute) + errs = append(errs, validation.ValidateRouteUpdate(objRoute, oldRoute)...) + return errs +} + +func certificateChanged(route, older *api.Route) bool { + switch { + case route.Spec.TLS != nil && older.Spec.TLS != nil: + a, b := route.Spec.TLS, older.Spec.TLS + return a.CACertificate != b.CACertificate || + a.Certificate != b.Certificate || + a.DestinationCACertificate != b.DestinationCACertificate || + a.Key != b.Key + case route.Spec.TLS == nil && older.Spec.TLS == nil: + return false + default: + return true + } +} + +func (s routeStrategy) validateHostUpdate(ctx apirequest.Context, route, older *api.Route) field.ErrorList { + hostChanged := route.Spec.Host != older.Spec.Host + certChanged := certificateChanged(route, older) + if !hostChanged && !certChanged { + return nil + } + user, ok := apirequest.UserFrom(ctx) + if !ok { + return field.ErrorList{field.InternalError(field.NewPath("spec", "host"), fmt.Errorf("unable to verify host field can be changed"))} + } + res, err := s.sarClient.CreateSubjectAccessReview( + ctx, + &authorizationapi.SubjectAccessReview{ + User: user.GetName(), + Action: authorizationapi.Action{ + Verb: "update", + Group: "route.openshift.io", + Resource: "routes/custom-host", + }, + }, + ) + if err != nil { + return field.ErrorList{field.InternalError(field.NewPath("spec", "host"), err)} + } + if !res.Allowed { + if hostChanged { + return kvalidation.ValidateImmutableField(route.Spec.Host, older.Spec.Host, field.NewPath("spec", "host")) + } + errs := kvalidation.ValidateImmutableField(route.Spec.TLS.CACertificate, older.Spec.TLS.CACertificate, field.NewPath("spec", "tls", "caCertificate")) + errs = append(errs, kvalidation.ValidateImmutableField(route.Spec.TLS.Certificate, older.Spec.TLS.Certificate, field.NewPath("spec", "tls", "certificate"))...) + errs = append(errs, kvalidation.ValidateImmutableField(route.Spec.TLS.DestinationCACertificate, older.Spec.TLS.DestinationCACertificate, field.NewPath("spec", "tls", "destinationCACertificate"))...) + errs = append(errs, kvalidation.ValidateImmutableField(route.Spec.TLS.Key, older.Spec.TLS.Key, field.NewPath("spec", "tls", "key"))...) + return errs + } + return nil } func (routeStrategy) AllowUnconditionalUpdate() bool { @@ -115,7 +202,7 @@ type routeStatusStrategy struct { routeStrategy } -var StatusStrategy = routeStatusStrategy{NewStrategy(nil)} +var StatusStrategy = routeStatusStrategy{NewStrategy(nil, nil)} func (routeStatusStrategy) PrepareForUpdate(ctx apirequest.Context, obj, old runtime.Object) { newRoute := obj.(*api.Route) diff --git a/pkg/route/registry/route/strategy_test.go b/pkg/route/registry/route/strategy_test.go index 1c88882bf21c..cc6a4a9a2e7e 100644 --- a/pkg/route/registry/route/strategy_test.go +++ b/pkg/route/registry/route/strategy_test.go @@ -3,11 +3,15 @@ package route import ( "testing" - "github.com/openshift/origin/pkg/route/api" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/validation/field" + "k8s.io/apiserver/pkg/authentication/user" apirequest "k8s.io/apiserver/pkg/endpoints/request" kapi "k8s.io/kubernetes/pkg/api" + + authorizationapi "github.com/openshift/origin/pkg/authorization/api" + "github.com/openshift/origin/pkg/route/api" ) type testAllocator struct { @@ -20,12 +24,23 @@ func (t testAllocator) GenerateHostname(*api.Route, *api.RouterShard) string { return "mygeneratedhost.com" } +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 TestEmptyHostDefaulting(t *testing.T) { ctx := apirequest.NewContext() - strategy := NewStrategy(testAllocator{}) + strategy := NewStrategy(testAllocator{}, &testSAR{allow: true}) hostlessCreatedRoute := &api.Route{} - strategy.PrepareForCreate(ctx, hostlessCreatedRoute) + strategy.Validate(ctx, hostlessCreatedRoute) if hostlessCreatedRoute.Spec.Host != "mygeneratedhost.com" { t.Fatalf("Expected host to be allocated, got %s", hostlessCreatedRoute.Spec.Host) } @@ -52,48 +67,231 @@ func TestEmptyHostDefaulting(t *testing.T) { func TestHostWithWildcardPolicies(t *testing.T) { ctx := apirequest.NewContext() - strategy := NewStrategy(testAllocator{}) + ctx = apirequest.WithUser(ctx, &user.DefaultInfo{Name: "bob"}) tests := []struct { name string - host string + host, oldHost string wildcardPolicy api.WildcardPolicyType + tls, oldTLS *api.TLSConfig expected string + errs int + allow bool }{ { - name: "nohostemptypolicy", + name: "no-host-empty-policy", expected: "mygeneratedhost.com", + allow: true, }, { - name: "nohostnopolicy", + name: "no-host-nopolicy", wildcardPolicy: api.WildcardPolicyNone, expected: "mygeneratedhost.com", + allow: true, }, { - name: "nohostwildcardsubdomain", + name: "no-host-wildcard-subdomain", wildcardPolicy: api.WildcardPolicySubdomain, expected: "", + allow: true, + errs: 1, }, { - name: "hostemptypolicy", + name: "host-empty-policy", host: "empty.policy.test", expected: "empty.policy.test", + allow: true, }, { - name: "hostnopolicy", + name: "host-no-policy", host: "no.policy.test", wildcardPolicy: api.WildcardPolicyNone, expected: "no.policy.test", + allow: true, }, { - name: "hostwildcardsubdomain", + name: "host-wildcard-subdomain", host: "wildcard.policy.test", wildcardPolicy: api.WildcardPolicySubdomain, expected: "wildcard.policy.test", + allow: true, + }, + { + name: "custom-host-permission-denied", + host: "another.test", + expected: "another.test", + wildcardPolicy: api.WildcardPolicyNone, + allow: false, + errs: 1, + }, + { + name: "tls-permission-denied-destination", + tls: &api.TLSConfig{Termination: api.TLSTerminationReencrypt, DestinationCACertificate: "a"}, + wildcardPolicy: api.WildcardPolicyNone, + allow: false, + errs: 1, + }, + { + name: "tls-permission-denied-cert", + tls: &api.TLSConfig{Termination: api.TLSTerminationEdge, Certificate: "a"}, + wildcardPolicy: api.WildcardPolicyNone, + allow: false, + errs: 1, + }, + { + name: "tls-permission-denied-ca-cert", + tls: &api.TLSConfig{Termination: api.TLSTerminationEdge, CACertificate: "a"}, + wildcardPolicy: api.WildcardPolicyNone, + allow: false, + errs: 1, + }, + { + name: "tls-permission-denied-key", + tls: &api.TLSConfig{Termination: api.TLSTerminationEdge, Key: "a"}, + wildcardPolicy: api.WildcardPolicyNone, + allow: false, + errs: 1, + }, + { + name: "no-host-but-allowed", + expected: "mygeneratedhost.com", + wildcardPolicy: api.WildcardPolicyNone, + allow: false, + }, + { + name: "update-changed-host-denied", + host: "new.host", + expected: "new.host", + oldHost: "original.host", + wildcardPolicy: api.WildcardPolicyNone, + allow: false, + errs: 1, + }, + { + name: "update-changed-host-allowed", + host: "new.host", + expected: "new.host", + oldHost: "original.host", + wildcardPolicy: api.WildcardPolicyNone, + allow: true, + errs: 0, + }, + { + name: "key-unchanged", + host: "host", + expected: "host", + oldHost: "host", + tls: &api.TLSConfig{Termination: api.TLSTerminationEdge, Key: "a"}, + oldTLS: &api.TLSConfig{Termination: api.TLSTerminationEdge, Key: "a"}, + wildcardPolicy: api.WildcardPolicyNone, + allow: false, + errs: 0, + }, + { + name: "key-changed", + host: "host", + expected: "host", + oldHost: "host", + tls: &api.TLSConfig{Termination: api.TLSTerminationEdge, Key: "a"}, + oldTLS: &api.TLSConfig{Termination: api.TLSTerminationEdge, Key: "b"}, + wildcardPolicy: api.WildcardPolicyNone, + allow: false, + errs: 1, + }, + { + name: "certificate-unchanged", + host: "host", + expected: "host", + oldHost: "host", + tls: &api.TLSConfig{Termination: api.TLSTerminationEdge, Certificate: "a"}, + oldTLS: &api.TLSConfig{Termination: api.TLSTerminationEdge, Certificate: "a"}, + wildcardPolicy: api.WildcardPolicyNone, + allow: false, + errs: 0, + }, + { + name: "certificate-changed", + host: "host", + expected: "host", + oldHost: "host", + tls: &api.TLSConfig{Termination: api.TLSTerminationEdge, Certificate: "a"}, + oldTLS: &api.TLSConfig{Termination: api.TLSTerminationEdge, Certificate: "b"}, + wildcardPolicy: api.WildcardPolicyNone, + allow: false, + errs: 1, + }, + { + name: "ca-certificate-unchanged", + host: "host", + expected: "host", + oldHost: "host", + tls: &api.TLSConfig{Termination: api.TLSTerminationEdge, CACertificate: "a"}, + oldTLS: &api.TLSConfig{Termination: api.TLSTerminationEdge, CACertificate: "a"}, + wildcardPolicy: api.WildcardPolicyNone, + allow: false, + errs: 0, + }, + { + name: "ca-certificate-changed", + host: "host", + expected: "host", + oldHost: "host", + tls: &api.TLSConfig{Termination: api.TLSTerminationEdge, CACertificate: "a"}, + oldTLS: &api.TLSConfig{Termination: api.TLSTerminationEdge, CACertificate: "b"}, + wildcardPolicy: api.WildcardPolicyNone, + allow: false, + errs: 1, + }, + { + name: "key-unchanged", + host: "host", + expected: "host", + oldHost: "host", + tls: &api.TLSConfig{Termination: api.TLSTerminationEdge, Key: "a"}, + oldTLS: &api.TLSConfig{Termination: api.TLSTerminationEdge, Key: "a"}, + wildcardPolicy: api.WildcardPolicyNone, + allow: false, + errs: 0, + }, + { + name: "key-changed", + host: "host", + expected: "host", + oldHost: "host", + tls: &api.TLSConfig{Termination: api.TLSTerminationEdge, Key: "a"}, + oldTLS: &api.TLSConfig{Termination: api.TLSTerminationEdge, Key: "b"}, + wildcardPolicy: api.WildcardPolicyNone, + allow: false, + errs: 1, + }, + { + name: "destination-ca-certificate-unchanged", + host: "host", + expected: "host", + oldHost: "host", + tls: &api.TLSConfig{Termination: api.TLSTerminationReencrypt, DestinationCACertificate: "a"}, + oldTLS: &api.TLSConfig{Termination: api.TLSTerminationReencrypt, DestinationCACertificate: "a"}, + wildcardPolicy: api.WildcardPolicyNone, + allow: false, + errs: 0, + }, + { + name: "destination-ca-certificate-changed", + host: "host", + expected: "host", + oldHost: "host", + tls: &api.TLSConfig{Termination: api.TLSTerminationReencrypt, DestinationCACertificate: "a"}, + oldTLS: &api.TLSConfig{Termination: api.TLSTerminationReencrypt, DestinationCACertificate: "b"}, + wildcardPolicy: api.WildcardPolicyNone, + allow: false, + errs: 1, }, } for _, tc := range tests { + sar := &testSAR{allow: tc.allow} + strategy := NewStrategy(testAllocator{}, sar) + route := &api.Route{ ObjectMeta: metav1.ObjectMeta{ Namespace: "wildcard", @@ -104,12 +302,44 @@ func TestHostWithWildcardPolicies(t *testing.T) { Spec: api.RouteSpec{ Host: tc.host, WildcardPolicy: tc.wildcardPolicy, + TLS: tc.tls, + To: api.RouteTargetReference{ + Name: "test", + Kind: "Service", + }, }, } - strategy.PrepareForCreate(ctx, route) + var errs field.ErrorList + if len(tc.oldHost) > 0 { + oldRoute := &api.Route{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "wildcard", + Name: tc.name, + UID: types.UID("wild"), + ResourceVersion: "1", + }, + Spec: api.RouteSpec{ + Host: tc.oldHost, + WildcardPolicy: tc.wildcardPolicy, + TLS: tc.oldTLS, + To: api.RouteTargetReference{ + Name: "test", + Kind: "Service", + }, + }, + } + errs = strategy.ValidateUpdate(ctx, route, oldRoute) + } else { + errs = strategy.Validate(ctx, route) + } + if route.Spec.Host != tc.expected { - t.Fatalf("test case %s expected host %s, got %s", tc.name, tc.expected, route.Spec.Host) + t.Errorf("test case %s expected host %s, got %s", tc.name, tc.expected, route.Spec.Host) + continue + } + if len(errs) != tc.errs { + t.Errorf("test case %s unexpected errors: %v %#v", tc.name, errs, sar) } } } diff --git a/test/testdata/bootstrappolicy/bootstrap_cluster_roles.yaml b/test/testdata/bootstrappolicy/bootstrap_cluster_roles.yaml index 910520cedae1..9eed3292f961 100644 --- a/test/testdata/bootstrappolicy/bootstrap_cluster_roles.yaml +++ b/test/testdata/bootstrappolicy/bootstrap_cluster_roles.yaml @@ -935,6 +935,14 @@ items: - patch - update - watch + - apiGroups: + - route.openshift.io + - "" + attributeRestrictions: null + resources: + - routes/custom-host + verbs: + - create - apiGroups: - route.openshift.io - "" @@ -1317,6 +1325,14 @@ items: - patch - update - watch + - apiGroups: + - route.openshift.io + - "" + attributeRestrictions: null + resources: + - routes/custom-host + verbs: + - create - apiGroups: - route.openshift.io - ""