template broker should use SAR, not impersonation

Author: Jim Minter

pkg/authorization/authorizer/subjects_test.go

@@ -48,6 +48,7 @@ func TestSubjects(t *testing.T) {
 			"system:kube-scheduler",
 			"system:serviceaccount:openshift-infra:build-controller",
 			"system:serviceaccount:openshift-infra:deployer-controller",
+			"system:serviceaccount:openshift-infra:template-instance-controller",
 		),
 		expectedGroups: sets.NewString("RootUsers", "system:cluster-admins", "system:cluster-readers", "system:masters", "system:nodes"),
 	}

pkg/authorization/util/util.go

@@ -0,0 +1,46 @@
+package util
+
+import (
+	"errors"
+
+	kerrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/apiserver/pkg/authentication/user"
+	"k8s.io/kubernetes/pkg/apis/authorization"
+	"k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/typed/authorization/internalversion"
+)
+
+// AddUserToSAR adds the requisite user information to a SubjectAccessReview.
+// It returns the modified SubjectAccessReview.
+func AddUserToSAR(user user.Info, sar *authorization.SubjectAccessReview) *authorization.SubjectAccessReview {
+	sar.Spec.User = user.GetName()
+	copy(sar.Spec.Groups, user.GetGroups())
+	sar.Spec.Extra = map[string]authorization.ExtraValue{}
+
+	for k, v := range user.GetExtra() {
+		sar.Spec.Extra[k] = authorization.ExtraValue(v)
+	}
+
+	return sar
+}
+
+// Authorize verifies that a given user is permitted to carry out a given
+// action.  If this cannot be determined, or if the user is not permitted, an
+// error is returned.
+func Authorize(sarClient internalversion.SubjectAccessReviewInterface, user user.Info, resourceAttributes *authorization.ResourceAttributes) error {
+	sar := AddUserToSAR(user, &authorization.SubjectAccessReview{
+		Spec: authorization.SubjectAccessReviewSpec{
+			ResourceAttributes: resourceAttributes,
+		},
+	})
+
+	resp, err := sarClient.Create(sar)
+	if err == nil && resp != nil && resp.Status.Allowed {
+		return nil
+	}
+
+	if err == nil {
+		err = errors.New(resp.Status.Reason)
+	}
+	return kerrors.NewForbidden(schema.GroupResource{Group: resourceAttributes.Group, Resource: resourceAttributes.Resource}, resourceAttributes.Name, err)
+}

pkg/client/cache/index.go

@@ -7,12 +7,10 @@ import (
 	buildutil "github.com/openshift/origin/pkg/build/util"
 	deployapi "github.com/openshift/origin/pkg/deploy/api"
 	imageapi "github.com/openshift/origin/pkg/image/api"
-	templateapi "github.com/openshift/origin/pkg/template/api"
 )
 
 const (
 	ImageStreamReferenceIndex = "imagestreamref"
-	TemplateUIDIndex          = "templateuid"
 )
 
 // ImageStreamReferenceIndexFunc is a default index function that indexes based on image stream references.
@@ -75,7 +73,3 @@ func ImageStreamReferenceIndexFunc(obj interface{}) ([]string, error) {
 	}
 	return nil, fmt.Errorf("image stream reference index not implemented for %#v", obj)
 }
-
-func TemplateUIDIndexFunc(obj interface{}) ([]string, error) {
-	return []string{string(obj.(*templateapi.Template).UID)}, nil
-}

pkg/client/cache/template.go

@@ -62,7 +62,7 @@ const (
 	RegistryViewerRoleName = "registry-viewer"
 	RegistryEditorRoleName = "registry-editor"
 
-	TemplateServiceBrokerClientRoleName = "templateservicebroker-client"
+	TemplateServiceBrokerClientRoleName = "system:openshift:templateservicebroker-client"
 
 	BuildStrategyDockerRoleName          = "system:build-strategy-docker"
 	BuildStrategyCustomRoleName          = "system:build-strategy-custom"

pkg/cmd/server/bootstrappolicy/constants.go

@@ -92,6 +92,19 @@ func init() {
 			eventsRule(),
 		},
 	})
+
+	// template-instance-controller
+	addControllerRole(rbac.ClusterRole{
+		ObjectMeta: metav1.ObjectMeta{Name: saRolePrefix + InfraTemplateInstanceControllerServiceAccountName},
+		Rules: []rbac.PolicyRule{
+			rbac.NewRule("create").Groups(kAuthzGroup).Resources("subjectaccessreviews").RuleOrDie(),
+			rbac.NewRule("get", "list", "watch").Groups(templateGroup).Resources("subjectaccessreviews").RuleOrDie(),
+			rbac.NewRule("update").Groups(templateGroup).Resources("templateinstances/status").RuleOrDie(),
+		},
+	})
+
+	controllerRoleBindings = append(controllerRoleBindings,
+		rbac.NewClusterBinding(EditRoleName).SAs(DefaultOpenShiftInfraNamespace, InfraTemplateInstanceControllerServiceAccountName).BindingOrDie())
 }
 
 // ControllerRoles returns the cluster roles used by controllers

pkg/cmd/server/bootstrappolicy/controller_policy.go

@@ -6,17 +6,17 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
 	kapi "k8s.io/kubernetes/pkg/api"
+	"k8s.io/kubernetes/pkg/apis/authorization"
 	"k8s.io/kubernetes/pkg/apis/certificates"
 	"k8s.io/kubernetes/pkg/apis/extensions"
 	"k8s.io/kubernetes/pkg/apis/storage"
 
-	// we need the conversions registered for our init block
 	authorizationapi "github.com/openshift/origin/pkg/authorization/api"
-	_ "github.com/openshift/origin/pkg/authorization/api/install"
 	authorizationapiv1 "github.com/openshift/origin/pkg/authorization/api/v1"
 	buildapi "github.com/openshift/origin/pkg/build/api"
 	deployapi "github.com/openshift/origin/pkg/deploy/api"
 	imageapi "github.com/openshift/origin/pkg/image/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"
@@ -56,6 +56,16 @@ const (
 
 	InfraNodeBootstrapServiceAccountName = "node-bootstrapper"
 	NodeBootstrapRoleName                = "system:node-bootstrapper"
+
+	// template instance controller watches for TemplateInstance object creation
+	// and instantiates templates as a result.
+	InfraTemplateInstanceControllerServiceAccountName = "template-instance-controller"
+
+	// template service broker is an open service broker-compliant API
+	// implementation which serves up OpenShift templates.  It uses the
+	// TemplateInstance backend for most of the heavy lifting.
+	InfraTemplateServiceBrokerServiceAccountName = "template-service-broker"
+	TemplateServiceBrokerControllerRoleName      = "system:openshift:template-service-broker"
 )
 
 type InfraServiceAccounts struct {
@@ -588,4 +598,45 @@ func init() {
 		panic(err)
 	}
 
+	err = InfraSAs.addServiceAccount(
+		InfraTemplateServiceBrokerServiceAccountName,
+		authorizationapi.ClusterRole{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: TemplateServiceBrokerControllerRoleName,
+			},
+			Rules: []authorizationapi.PolicyRule{
+				{
+					APIGroups: []string{authorization.GroupName},
+					Verbs:     sets.NewString("create"),
+					Resources: sets.NewString("subjectaccessreviews"),
+				},
+				{
+					APIGroups: []string{templateapi.GroupName},
+					Verbs:     sets.NewString("get", "create", "update", "delete"),
+					Resources: sets.NewString("brokertemplateinstances"),
+				},
+				{
+					APIGroups: []string{templateapi.GroupName},
+					// "assign" 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", "assign"),
+					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"),
+				},
+			},
+		},
+	)
+	if err != nil {
+		panic(err)
+	}
 }

pkg/cmd/server/bootstrappolicy/infra_sa_policy.go

@@ -38,5 +38,8 @@ func (c *MasterConfig) NewOpenshiftControllerInitializers() (map[string]controll
 	deploymentTrigger := controller.DeploymentTriggerControllerConfig{Codec: codec}
 	ret["deploymenttrigger"] = deploymentTrigger.RunController
 
+	templateInstance := controller.TemplateInstanceControllerConfig{}
+	ret["templateinstance"] = templateInstance.RunController
+
 	return ret, nil
 }

pkg/cmd/server/origin/controller.go

@@ -8,6 +8,7 @@ import (
 	deployercontroller "github.com/openshift/origin/pkg/deploy/controller/deployer"
 	deployconfigcontroller "github.com/openshift/origin/pkg/deploy/controller/deploymentconfig"
 	triggercontroller "github.com/openshift/origin/pkg/deploy/controller/generictrigger"
+	templatecontroller "github.com/openshift/origin/pkg/template/controller"
 )
 
 type DeployerControllerConfig struct {
@@ -25,6 +26,9 @@ type DeploymentTriggerControllerConfig struct {
 	Codec runtime.Codec
 }
 
+type TemplateInstanceControllerConfig struct {
+}
+
 func (c *DeployerControllerConfig) RunController(ctx ControllerContext) (bool, error) {
 	internalDeployerKubeClient, err := ctx.ClientBuilder.KubeInternalClient(bootstrappolicy.InfraDeployerControllerServiceAccountName)
 	if err != nil {
@@ -87,3 +91,31 @@ func (c *DeploymentTriggerControllerConfig) RunController(ctx ControllerContext)
 
 	return true, nil
 }
+
+func (c *TemplateInstanceControllerConfig) RunController(ctx ControllerContext) (bool, error) {
+	saName := bootstrappolicy.InfraTemplateInstanceControllerServiceAccountName
+
+	internalKubeClient, err := ctx.ClientBuilder.KubeInternalClient(saName)
+	if err != nil {
+		return true, err
+	}
+
+	deprecatedOcClient, err := ctx.ClientBuilder.DeprecatedOpenshiftClient(saName)
+	if err != nil {
+		return true, err
+	}
+
+	templateClient, err := ctx.ClientBuilder.OpenshiftTemplateClient(saName)
+	if err != nil {
+		return true, err
+	}
+
+	go templatecontroller.NewTemplateInstanceController(
+		deprecatedOcClient,
+		internalKubeClient,
+		templateClient.Template(),
+		ctx.TemplateInformers.Template().InternalVersion().TemplateInstances(),
+	).Run(5, ctx.Stop)
+
+	return true, nil
+}

pkg/cmd/server/origin/controller/apps.go

@@ -9,6 +9,8 @@ import (
 
 	osclient "github.com/openshift/origin/pkg/client"
 	"github.com/openshift/origin/pkg/controller/shared"
+	templateinformer "github.com/openshift/origin/pkg/template/generated/informers/internalversion"
+	templateclient "github.com/openshift/origin/pkg/template/generated/internalclientset"
 )
 
 type ControllerContext struct {
@@ -17,6 +19,8 @@ type ControllerContext struct {
 	// ClientBuilder will provide a client for this controller to use
 	ClientBuilder ControllerClientBuilder
 
+	TemplateInformers templateinformer.SharedInformerFactory
+
 	DeprecatedOpenshiftInformers shared.InformerFactory
 
 	// Stop is the stop channel
@@ -34,6 +38,7 @@ type ControllerClientBuilder interface {
 	KubeInternalClientOrDie(name string) kclientsetinternal.Interface
 	DeprecatedOpenshiftClient(name string) (osclient.Interface, error)
 	DeprecatedOpenshiftClientOrDie(name string) osclient.Interface
+	OpenshiftTemplateClient(name string) (templateclient.Interface, error)
 }
 
 // InitFunc is used to launch a particular controller.  It may run additional "should I activate checks".
@@ -76,3 +81,11 @@ func (b OpenshiftControllerClientBuilder) DeprecatedOpenshiftClientOrDie(name st
 	}
 	return client
 }
+
+func (b OpenshiftControllerClientBuilder) OpenshiftTemplateClient(name string) (templateclient.Interface, error) {
+	clientConfig, err := b.Config(name)
+	if err != nil {
+		return nil, err
+	}
+	return templateclient.NewForConfig(clientConfig)
+}

pkg/cmd/server/origin/controller/interfaces.go

@@ -512,9 +512,9 @@ func (c *MasterConfig) InstallProtectedAPI(server *apiserver.GenericAPIServer) (
 			templateapi.ServiceBrokerRoot,
 			templateservicebroker.NewBroker(
 				c.PrivilegedLoopbackClientConfig,
-				c.PrivilegedLoopbackOpenShiftClient,
-				c.PrivilegedLoopbackKubernetesClientsetInternal.Core(),
-				c.Informers,
+				c.PrivilegedLoopbackKubernetesClientsetInternal,
+				c.Options.PolicyConfig.OpenShiftInfrastructureNamespace,
+				c.TemplateInformers.Template().InternalVersion().Templates(),
 				c.Options.TemplateServiceBrokerConfig.TemplateNamespaces,
 			),
 		)
@@ -895,12 +895,13 @@ func (c *MasterConfig) GetRestStorage() map[schema.GroupVersion]map[string]rest.
 	}
 
 	if c.Options.TemplateServiceBrokerConfig != nil {
-		templateInstanceStorage, err := templateinstanceetcd.NewREST(c.RESTOptionsGetter, c.PrivilegedLoopbackOpenShiftClient)
+		templateInstanceStorage, templateInstanceStatusStorage, err := templateinstanceetcd.NewREST(c.RESTOptionsGetter, c.PrivilegedLoopbackKubernetesClientsetInternal)
 		checkStorageErr(err)
 		brokerTemplateInstanceStorage, err := brokertemplateinstanceetcd.NewREST(c.RESTOptionsGetter)
 		checkStorageErr(err)
 
 		storage[templateapiv1.SchemeGroupVersion]["templateinstances"] = templateInstanceStorage
+		storage[templateapiv1.SchemeGroupVersion]["templateinstances/status"] = templateInstanceStatusStorage
 		storage[templateapiv1.SchemeGroupVersion]["brokertemplateinstances"] = brokerTemplateInstanceStorage
 	}