template broker should use SAR, not impersonation

pkg/cmd/admin/project/new_project.go

@@ -127,7 +127,7 @@ func (o *NewProjectOptions) Run(useNodeSelector bool) error {
 		}
 	}
 
-	for _, binding := range bootstrappolicy.GetBootstrapServiceAccountProjectRoleBindings(o.ProjectName) {
+	for _, binding := range bootstrappolicy.GetBootstrapProjectRoleBindings(o.ProjectName) {
 		addRole := &policy.RoleModificationOptions{
 			RoleName:            binding.RoleRef.Name,
 			RoleNamespace:       binding.RoleRef.Namespace,

pkg/cmd/server/bootstrappolicy/constants.go

@@ -62,6 +62,7 @@ const (
 	RegistryViewerRoleName = "registry-viewer"
 	RegistryEditorRoleName = "registry-editor"
 
+	TemplateServiceBrokerRoleName       = "templateservicebroker"
 	TemplateServiceBrokerClientRoleName = "templateservicebroker-client"
 
 	BuildStrategyDockerRoleName          = "system:build-strategy-docker"
@@ -129,4 +130,6 @@ const (
 	BuildStrategyJenkinsPipelineRoleBindingName = BuildStrategyJenkinsPipelineRoleName + "-binding"
 
 	OpenshiftSharedResourceViewRoleBindingName = OpenshiftSharedResourceViewRoleName + "s"
+
+	TemplateServiceBrokerRoleBindingName = TemplateServiceBrokerRoleName + "-binding"
 )

pkg/cmd/server/bootstrappolicy/infra_sa_policy.go

@@ -17,6 +17,7 @@ import (
 	authorizationapi "github.com/openshift/origin/pkg/authorization/api"
 	authorizationapiv1 "github.com/openshift/origin/pkg/authorization/api/v1"
 	buildapi "github.com/openshift/origin/pkg/build/api"
+	templateapi "github.com/openshift/origin/pkg/template/api"
 
 	// we need the conversions registered for our init block
 	_ "github.com/openshift/origin/pkg/authorization/api/install"
@@ -91,6 +92,12 @@ const (
 
 	InfraGarbageCollectorControllerServiceAccountName = "garbage-collector-controller"
 	GarbageCollectorControllerRoleName                = "system:garbage-collector-controller"
+
+	InfraTemplateInstanceControllerServiceAccountName = "template-instance-controller"
+	TemplateInstanceControllerRoleName                = "system:template-instance-controller"
+
+	InfraTemplateServiceBrokerServiceAccountName = "template-service-broker"
+	TemplateServiceBrokerControllerRoleName      = "system:template-service-broker"
 )
 
 type InfraServiceAccounts struct {
@@ -1122,4 +1129,57 @@ func init() {
 	if err != nil {
 		panic(err)
 	}
+
+	err = InfraSAs.addServiceAccount(
+		InfraTemplateInstanceControllerServiceAccountName,
+		authorizationapi.ClusterRole{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: TemplateInstanceControllerRoleName,
+			},
+			Rules: []authorizationapi.PolicyRule{
+				{
+					APIGroups: []string{templateapi.GroupName},
+					// "impersonate" is required for the API server to accept updates to
+					// TemplateInstance objects where the requester username is not the
+					// API caller.
+					Verbs:     sets.NewString("get", "list", "watch", "update", "impersonate"),
+					Resources: sets.NewString("templateinstances"),
+				},
+				{
+					APIGroups: []string{kapi.GroupName},
+					Verbs:     sets.NewString("get"),
+					Resources: sets.NewString("secrets"),
+				},
+				{
+					APIGroups: []string{"*"},
+					// delete is needed to avoid error: "cannot set an ownerRef on a
+					// resource you can't delete"
+					Verbs:     sets.NewString("create", "delete"),
+					Resources: sets.NewString("*"),
+				},
+			},
+		},
+	)
+	if err != nil {
+		panic(err)
+	}
+
+	err = InfraSAs.addServiceAccount(
+		InfraTemplateServiceBrokerServiceAccountName,
+		authorizationapi.ClusterRole{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: TemplateServiceBrokerControllerRoleName,
+			},
+			Rules: []authorizationapi.PolicyRule{
+				{
+					APIGroups: []string{templateapi.GroupName},
+					Verbs:     sets.NewString("get", "create", "update", "delete"),
+					Resources: sets.NewString("brokertemplateinstances"),
+				},
+			},
+		},
+	)
+	if err != nil {
+		panic(err)
+	}
 }

pkg/cmd/server/bootstrappolicy/policy.go

@@ -929,6 +929,39 @@ func GetOpenshiftBootstrapClusterRoles() []authorizationapi.ClusterRole {
 				},
 			},
 		},
+		{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: TemplateServiceBrokerRoleName,
+				Annotations: map[string]string{
+					roleSystemOnly: roleIsSystemOnly,
+				},
+			},
+			Rules: []authorizationapi.PolicyRule{
+				{
+					APIGroups: []string{authorizationapi.LegacyGroupName},
+					Verbs:     sets.NewString("create"),
+					Resources: sets.NewString("subjectaccessreviews"),
+				},
+				{
+					APIGroups: []string{templateapi.GroupName},
+					// "impersonate" is required for the API server to accept creation of
+					// TemplateInstance objects with the requester username set to an
+					// identity which is not the API caller.
+					Verbs:     sets.NewString("get", "create", "delete", "impersonate"),
+					Resources: sets.NewString("templateinstances"),
+				},
+				{
+					APIGroups: []string{kapi.GroupName},
+					Verbs:     sets.NewString("get", "list", "create", "delete"),
+					Resources: sets.NewString("secrets"),
+				},
+				{
+					APIGroups: []string{kapi.GroupName},
+					Verbs:     sets.NewString("list"),
+					Resources: sets.NewString("services"),
+				},
+			},
+		},
 	}
 
 	// TODO check if we really need to do this

pkg/cmd/server/bootstrappolicy/policy_test.go

@@ -31,7 +31,7 @@ func TestOpenshiftRoles(t *testing.T) {
 }
 
 func TestBootstrapProjectRoleBindings(t *testing.T) {
-	roleBindings := bootstrappolicy.GetBootstrapServiceAccountProjectRoleBindings("myproject")
+	roleBindings := bootstrappolicy.GetBootstrapProjectRoleBindings("myproject")
 	list := &api.List{}
 	for i := range roleBindings {
 		list.Items = append(list.Items, &roleBindings[i])

pkg/cmd/server/bootstrappolicy/project_policy.go

@@ -42,3 +42,29 @@ func GetBootstrapServiceAccountProjectRoleBindings(namespace string) []authoriza
 		},
 	}
 }
+
+func GetBootstrapTemplateServiceBrokerProjectRoleBindings(namespace string) []authorizationapi.RoleBinding {
+	return []authorizationapi.RoleBinding{
+		{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      TemplateServiceBrokerRoleBindingName,
+				Namespace: namespace,
+			},
+			RoleRef: kapi.ObjectReference{
+				Name: TemplateServiceBrokerRoleName,
+			},
+			Subjects: []kapi.ObjectReference{
+				{
+					Kind:      authorizationapi.ServiceAccountKind,
+					Name:      InfraTemplateServiceBrokerServiceAccountName,
+					Namespace: DefaultOpenShiftInfraNamespace,
+				},
+			},
+		},
+	}
+}
+
+func GetBootstrapProjectRoleBindings(namespace string) []authorizationapi.RoleBinding {
+	return append(GetBootstrapServiceAccountProjectRoleBindings(namespace),
+		GetBootstrapTemplateServiceBrokerProjectRoleBindings(namespace)...)
+}

pkg/cmd/server/origin/master.go

@@ -512,8 +512,6 @@ func (c *MasterConfig) InstallProtectedAPI(server *apiserver.GenericAPIServer) (
 			templateapi.ServiceBrokerRoot,
 			templateservicebroker.NewBroker(
 				c.PrivilegedLoopbackClientConfig,
-				c.PrivilegedLoopbackOpenShiftClient,
-				c.PrivilegedLoopbackKubernetesClientsetInternal.Core(),
 				c.Informers,
 				c.Options.TemplateServiceBrokerConfig.TemplateNamespaces,
 			),

pkg/cmd/server/origin/run_components.go

@@ -13,6 +13,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	utilwait "k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/apiserver/pkg/admission"
+	"k8s.io/client-go/rest"
 	"k8s.io/client-go/util/cert"
 	"k8s.io/client-go/util/flowcontrol"
 	kctrlmgr "k8s.io/kubernetes/cmd/kube-controller-manager/app"
@@ -593,8 +594,8 @@ func (c *MasterConfig) RunUnidlingController() {
 	cont.Run(utilwait.NeverStop)
 }
 
-func (c *MasterConfig) RunTemplateController() {
-	go templatecontroller.NewTemplateInstanceController(c.PrivilegedLoopbackClientConfig).Run(utilwait.NeverStop)
+func (c *MasterConfig) RunTemplateController(config *rest.Config) {
+	go templatecontroller.NewTemplateInstanceController(config).Run(utilwait.NeverStop)
 }
 
 func (c *MasterConfig) RunOriginToRBACSyncControllers() {

pkg/cmd/server/start/start_master.go

@@ -844,7 +844,12 @@ func startControllers(oc *origin.MasterConfig, kc *kubernetes.MasterConfig) erro
 	oc.RunIngressIPController(ingressIPClientInternal, ingressIPClientExternal)
 
 	if oc.Options.TemplateServiceBrokerConfig != nil {
-		oc.RunTemplateController()
+		templateInstanceControllerConfig, _, _, _, err := oc.GetServiceAccountClients(bootstrappolicy.InfraTemplateInstanceControllerServiceAccountName)
+		if err != nil {
+			glog.Fatalf("Could not get client: %v", err)
+		}
+
+		oc.RunTemplateController(templateInstanceControllerConfig)
 	}
 
 	glog.Infof("Started Origin Controllers")

pkg/project/registry/projectrequest/delegated/sample_template.go

@@ -44,9 +44,9 @@ func DefaultTemplate() *templateapi.Template {
 		panic(err)
 	}
 
-	serviceAccountRoleBindings := bootstrappolicy.GetBootstrapServiceAccountProjectRoleBindings(ns)
-	for i := range serviceAccountRoleBindings {
-		if err := templateapi.AddObjectsToTemplate(ret, []runtime.Object{&serviceAccountRoleBindings[i]}, authorizationapiv1.SchemeGroupVersion); err != nil {
+	bootstrapRoleBindings := bootstrappolicy.GetBootstrapProjectRoleBindings(ns)
+	for i := range bootstrapRoleBindings {
+		if err := templateapi.AddObjectsToTemplate(ret, []runtime.Object{&bootstrapRoleBindings[i]}, authorizationapiv1.SchemeGroupVersion); err != nil {
 			panic(err)
 		}
 	}

pkg/template/controller/templateinstance_controller.go

@@ -1,11 +1,15 @@
 package controller
 
 import (
+	"errors"
 	"fmt"
 
+	kerrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	watch "k8s.io/apimachinery/pkg/watch"
 	"k8s.io/apiserver/pkg/authentication/user"
@@ -15,23 +19,29 @@ import (
 	"k8s.io/kubernetes/pkg/kubectl/resource"
 
 	"github.com/openshift/origin/pkg/api/latest"
-	authclient "github.com/openshift/origin/pkg/auth/client"
+	authorizationapi "github.com/openshift/origin/pkg/authorization/api"
 	"github.com/openshift/origin/pkg/client"
 	"github.com/openshift/origin/pkg/config/cmd"
 	templateapi "github.com/openshift/origin/pkg/template/api"
 	templateapiv1 "github.com/openshift/origin/pkg/template/api/v1"
 	templateclientset "github.com/openshift/origin/pkg/template/generated/internalclientset"
 	internalversiontemplate "github.com/openshift/origin/pkg/template/generated/internalclientset/typed/template/internalversion"
+	kclientsetinternal "k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset"
 )
 
 type TemplateInstanceController struct {
-	restconfig     rest.Config
+	oc             *client.Client
+	kc             kclientsetinternal.Interface
 	templateclient internalversiontemplate.TemplateInterface
 	controller     cache.Controller
 }
 
-func NewTemplateInstanceController(restconfig rest.Config) *TemplateInstanceController {
-	c := TemplateInstanceController{restconfig: restconfig, templateclient: templateclientset.NewForConfigOrDie(&restconfig).Template()}
+func NewTemplateInstanceController(restconfig *rest.Config) *TemplateInstanceController {
+	c := TemplateInstanceController{
+		oc:             client.NewOrDie(restconfig),
+		kc:             kclientsetinternal.NewForConfigOrDie(restconfig),
+		templateclient: templateclientset.NewForConfigOrDie(restconfig).Template(),
+	}
 	_, c.controller = cache.NewInformer(
 		&cache.ListWatch{
 			ListFunc: func(options metav1.ListOptions) (runtime.Object, error) {
@@ -100,25 +110,35 @@ func (c *TemplateInstanceController) handle(templateInstance *templateapi.Templa
 	return err
 }
 
+func (c *TemplateInstanceController) authorize(u user.Info, action *authorizationapi.Action) error {
+	sar := authorizationapi.AddUserToSAR(u, &authorizationapi.SubjectAccessReview{Action: *action})
+	resp, err := c.oc.SubjectAccessReviews().Create(sar)
+	if err == nil && resp != nil && resp.Allowed {
+		return nil
+	}
+	if err == nil {
+		err = errors.New(resp.Reason)
+	}
+	return kerrors.NewForbidden(schema.GroupResource{Group: action.Group, Resource: action.Resource}, action.ResourceName, err)
+}
+
 func (c *TemplateInstanceController) provision(templateInstance *templateapi.TemplateInstance) error {
 	if templateInstance.Spec.Requester == nil || templateInstance.Spec.Requester.Username == "" {
 		return fmt.Errorf("spec.requester.username not set")
 	}
 
 	u := &user.DefaultInfo{Name: templateInstance.Spec.Requester.Username}
 
-	impersonatingConfig := authclient.NewImpersonatingConfig(u, c.restconfig)
-	impersonatedOC, err := client.New(&impersonatingConfig)
-	if err != nil {
-		return err
-	}
-
-	impersonatedKC, err := authclient.NewImpersonatingKubernetesClientset(u, c.restconfig)
-	if err != nil {
+	if err := c.authorize(u, &authorizationapi.Action{
+		Namespace: templateInstance.Namespace,
+		Verb:      "get",
+		Group:     kapi.GroupName,
+		Resource:  "secrets",
+	}); err != nil {
 		return err
 	}
 
-	secret, err := impersonatedKC.Core().Secrets(templateInstance.Namespace).Get(templateInstance.Spec.Secret.Name, metav1.GetOptions{})
+	secret, err := c.kc.Core().Secrets(templateInstance.Namespace).Get(templateInstance.Spec.Secret.Name, metav1.GetOptions{})
 	if err != nil {
 		return err
 	}
@@ -141,7 +161,16 @@ func (c *TemplateInstanceController) provision(templateInstance *templateapi.Tem
 		}
 	}
 
-	template, err = impersonatedOC.TemplateConfigs(templateInstance.Namespace).Create(template)
+	if err = c.authorize(u, &authorizationapi.Action{
+		Namespace: templateInstance.Namespace,
+		Verb:      "create",
+		Group:     templateapi.LegacyGroupName,
+		Resource:  "templateconfigs",
+	}); err != nil {
+		return err
+	}
+
+	template, err = c.oc.TemplateConfigs(templateInstance.Namespace).Create(template)
 	if err != nil {
 		return err
 	}
@@ -169,16 +198,32 @@ func (c *TemplateInstanceController) provision(templateInstance *templateapi.Tem
 			ObjectTyper: kapi.Scheme,
 			ClientMapper: resource.ClientMapperFunc(func(mapping *meta.RESTMapping) (resource.RESTClient, error) {
 				if latest.OriginKind(mapping.GroupVersionKind) {
-					return impersonatedOC, nil
+					return c.oc, nil
 				}
-				return impersonatedKC.Core().RESTClient(), nil
+				return c.kc.Core().RESTClient(), nil
 			}),
 		},
-		Op: cmd.Create,
+		Op: func(info *resource.Info, namespace string, obj runtime.Object) (runtime.Object, error) {
+			if err = c.authorize(u, &authorizationapi.Action{
+				Namespace: namespace,
+				Verb:      "create",
+				Group:     info.Mapping.GroupVersionKind.Group,
+				Resource:  info.Mapping.Resource,
+			}); err != nil {
+				return nil, err
+			}
+			return obj, nil
+		},
 	}
 	errs = bulk.Run(&kapi.List{Items: template.Objects}, templateInstance.Namespace)
 	if len(errs) > 0 {
-		return errs[0]
+		return utilerrors.NewAggregate(errs)
+	}
+
+	bulk.Op = cmd.Create
+	errs = bulk.Run(&kapi.List{Items: template.Objects}, templateInstance.Namespace)
+	if len(errs) > 0 {
+		return utilerrors.NewAggregate(errs)
 	}
 
 	return nil