Fixes bug 1380462 - https://bugzilla.redhat.com/show_bug.cgi?id=1380462

Updated bootstrap policy to not grant builds/custom privs for system:authenticated users by default
Permissions can still be granted by an administrator if needed

Author: coreydaley

pkg/cmd/server/bootstrappolicy/policy.go

@@ -958,11 +958,6 @@ func GetBootstrapClusterRoleBindings() []authorizationapi.ClusterRoleBinding {
 			RoleRef:    kapi.ObjectReference{Name: BuildStrategyDockerRoleName},
 			Subjects:   []kapi.ObjectReference{{Kind: authorizationapi.SystemGroupKind, Name: AuthenticatedGroup}},
 		},
-		{
-			ObjectMeta: kapi.ObjectMeta{Name: BuildStrategyCustomRoleBindingName},
-			RoleRef:    kapi.ObjectReference{Name: BuildStrategyCustomRoleName},
-			Subjects:   []kapi.ObjectReference{{Kind: authorizationapi.SystemGroupKind, Name: AuthenticatedGroup}},
-		},
 		{
 			ObjectMeta: kapi.ObjectMeta{Name: BuildStrategySourceRoleBindingName},
 			RoleRef:    kapi.ObjectReference{Name: BuildStrategySourceRoleName},

test/cmd/policy.sh

@@ -59,22 +59,24 @@ os::cmd::expect_success_and_not_text 'oadm policy who-can create builds/custom'
 os::cmd::expect_success_and_not_text 'oadm policy who-can create builds/source' 'namespaced-user'
 os::cmd::expect_success_and_not_text 'oadm policy who-can create builds/jenkinspipeline' 'namespaced-user'
 os::cmd::expect_success_and_text     'oadm policy who-can create builds/docker' 'system:authenticated'
-os::cmd::expect_success_and_text     'oadm policy who-can create builds/custom' 'system:authenticated'
 os::cmd::expect_success_and_text     'oadm policy who-can create builds/source' 'system:authenticated'
 os::cmd::expect_success_and_text     'oadm policy who-can create builds/jenkinspipeline' 'system:authenticated'
 # if this method for removing access to docker/custom/source/jenkinspipeline builds changes, docs need to be updated as well
-os::cmd::expect_success 'oadm policy remove-cluster-role-from-group system:build-strategy-custom system:authenticated'
 os::cmd::expect_success 'oadm policy remove-cluster-role-from-group system:build-strategy-docker system:authenticated'
 os::cmd::expect_success 'oadm policy remove-cluster-role-from-group system:build-strategy-source system:authenticated'
 os::cmd::expect_success 'oadm policy remove-cluster-role-from-group system:build-strategy-jenkinspipeline system:authenticated'
 # ensure build strategy permissions no longer exist
 os::cmd::try_until_failure           'oadm policy who-can create builds/source | grep system:authenticated'
 os::cmd::expect_success_and_not_text 'oadm policy who-can create builds/docker' 'system:authenticated'
-os::cmd::expect_success_and_not_text 'oadm policy who-can create builds/custom' 'system:authenticated'
 os::cmd::expect_success_and_not_text 'oadm policy who-can create builds/source' 'system:authenticated'
 os::cmd::expect_success_and_not_text 'oadm policy who-can create builds/jenkinspipeline' 'system:authenticated'
-os::cmd::expect_success 'oadm policy reconcile-cluster-role-bindings --confirm'
 
+# ensure system:authenticated users can not create custom builds by default, but can if explicitly granted access
+os::cmd::expect_success_and_not_text 'oadm policy who-can create builds/custom' 'system:authenticated'
+os::cmd::expect_success 'oadm policy add-cluster-role-to-group system:build-strategy-custom system:authenticated'
+os::cmd::expect_success_and_text 'oadm policy who-can create builds/custom' 'system:authenticated'
+
+os::cmd::expect_success 'oadm policy reconcile-cluster-role-bindings --confirm'
 
 os::cmd::expect_success_and_text 'oc policy can-i --list' 'get update.*imagestreams/layers'
 os::cmd::expect_success_and_text 'oc policy can-i create pods --all-namespaces' 'yes'

test/integration/build_admission_test.go

@@ -15,13 +15,44 @@ import (
 	testserver "github.com/openshift/origin/test/util/server"
 )
 
+// all build strategy types
+func buildStrategyTypes() []string {
+	return []string{"source", "docker", "custom", "jenkinspipeline"}
+}
+
+// build strategy types that are not granted by default to system:authenticated
+func buildStrategyTypesRestricted() []string {
+	return []string{"custom"}
+}
+
 func TestPolicyBasedRestrictionOfBuildCreateAndCloneByStrategy(t *testing.T) {
 	defer testutil.DumpEtcdOnFailure(t)
 	clusterAdminClient, projectAdminClient, projectEditorClient := setupBuildStrategyTest(t, false)
 
 	clients := map[string]*client.Client{"admin": projectAdminClient, "editor": projectEditorClient}
 	builds := map[string]*buildapi.Build{}
 
+	restrictedStrategies := make(map[string]int)
+	for key, val := range buildStrategyTypesRestricted() {
+		restrictedStrategies[val] = key
+	}
+
+	// ensure that restricted strategy types can not be created
+	for _, strategy := range buildStrategyTypes() {
+		for clientType, client := range clients {
+			var err error
+			builds[string(strategy)+clientType], err = createBuild(t, client.Builds(testutil.Namespace()), strategy)
+			_, restricted := restrictedStrategies[strategy]
+			if kapierror.IsForbidden(err) && !restricted {
+				t.Errorf("unexpected error for strategy %s and client %s: %v", strategy, clientType, err)
+			} else if !kapierror.IsForbidden(err) && restricted {
+				t.Errorf("expected forbidden for strategy %s and client %s: Got success instead ", strategy, clientType)
+			}
+		}
+	}
+
+	grantRestrictedBuildStrategyRoleResources(t, clusterAdminClient, projectAdminClient, projectEditorClient)
+
 	// Create builds to setup test
 	for _, strategy := range buildStrategyTypes() {
 		for clientType, client := range clients {
@@ -40,7 +71,6 @@ func TestPolicyBasedRestrictionOfBuildCreateAndCloneByStrategy(t *testing.T) {
 			}
 		}
 	}
-
 	removeBuildStrategyRoleResources(t, clusterAdminClient, projectAdminClient, projectEditorClient)
 
 	// make sure builds are rejected
@@ -77,8 +107,28 @@ func TestPolicyBasedRestrictionOfBuildConfigCreateAndInstantiateByStrategy(t *te
 
 	clients := map[string]*client.Client{"admin": projectAdminClient, "editor": projectEditorClient}
 	buildConfigs := map[string]*buildapi.BuildConfig{}
+	restrictedStrategies := make(map[string]int)
+	for key, val := range buildStrategyTypesRestricted() {
+		restrictedStrategies[val] = key
+	}
 
-	// by default admins and editors can create all type of buildconfigs
+	// ensure that restricted strategy types can not be created
+	for _, strategy := range buildStrategyTypes() {
+		for clientType, client := range clients {
+			var err error
+			buildConfigs[string(strategy)+clientType], err = createBuildConfig(t, client.BuildConfigs(testutil.Namespace()), strategy)
+			_, restricted := restrictedStrategies[strategy]
+			if kapierror.IsForbidden(err) && !restricted {
+				t.Errorf("unexpected error for strategy %s and client %s: %v", strategy, clientType, err)
+			} else if !kapierror.IsForbidden(err) && restricted {
+				t.Errorf("expected forbidden for strategy %s and client %s: Got success instead ", strategy, clientType)
+			}
+		}
+	}
+
+	grantRestrictedBuildStrategyRoleResources(t, clusterAdminClient, projectAdminClient, projectEditorClient)
+
+	// by default admins and editors can create source, docker, and jenkinspipline buildconfigs
 	for _, strategy := range buildStrategyTypes() {
 		for clientType, client := range clients {
 			var err error
@@ -127,10 +177,6 @@ func TestPolicyBasedRestrictionOfBuildConfigCreateAndInstantiateByStrategy(t *te
 	}
 }
 
-func buildStrategyTypes() []string {
-	return []string{"source", "docker", "custom", "jenkinspipeline"}
-}
-
 func setupBuildStrategyTest(t *testing.T, includeControllers bool) (clusterAdminClient, projectAdminClient, projectEditorClient *client.Client) {
 	testutil.RequireEtcd(t)
 	namespace := testutil.Namespace()
@@ -202,13 +248,13 @@ func setupBuildStrategyTest(t *testing.T, includeControllers bool) (clusterAdmin
 func removeBuildStrategyRoleResources(t *testing.T, clusterAdminClient, projectAdminClient, projectEditorClient *client.Client) {
 	// remove resources from role so that certain build strategies are forbidden
 	for _, role := range []string{bootstrappolicy.BuildStrategyCustomRoleName, bootstrappolicy.BuildStrategyDockerRoleName, bootstrappolicy.BuildStrategySourceRoleName, bootstrappolicy.BuildStrategyJenkinsPipelineRoleName} {
-		remove := &policy.RoleModificationOptions{
+		options := &policy.RoleModificationOptions{
 			RoleNamespace:       "",
 			RoleName:            role,
 			RoleBindingAccessor: policy.NewClusterRoleBindingAccessor(clusterAdminClient),
 			Groups:              []string{"system:authenticated"},
 		}
-		if err := remove.RemoveRole(); err != nil {
+		if err := options.RemoveRole(); err != nil {
 			t.Fatalf("unexpected error: %v", err)
 		}
 	}
@@ -227,6 +273,25 @@ func removeBuildStrategyRoleResources(t *testing.T, clusterAdminClient, projectA
 	}
 }
 
+func grantRestrictedBuildStrategyRoleResources(t *testing.T, clusterAdminClient, projectAdminClient, projectEditorClient *client.Client) {
+	// grant resources to role so that restricted build strategies are available
+	for _, role := range []string{bootstrappolicy.BuildStrategyCustomRoleName} {
+		options := &policy.RoleModificationOptions{
+			RoleNamespace:       "",
+			RoleName:            role,
+			RoleBindingAccessor: policy.NewClusterRoleBindingAccessor(clusterAdminClient),
+			Groups:              []string{"system:authenticated"},
+		}
+		if err := options.AddRole(); err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+	}
+
+	if err := testutil.WaitForPolicyUpdate(projectEditorClient, testutil.Namespace(), "create", buildapi.Resource(authorizationapi.CustomBuildResource), true); err != nil {
+		t.Fatal(err)
+	}
+}
+
 func strategyForType(t *testing.T, strategy string) buildapi.BuildStrategy {
 	buildStrategy := buildapi.BuildStrategy{}
 	switch strategy {

test/integration/imagechange_buildtrigger_test.go

@@ -1,16 +1,17 @@
 package integration
 
 import (
-	"testing"
-
-	kapi "k8s.io/kubernetes/pkg/api"
-	watchapi "k8s.io/kubernetes/pkg/watch"
-
+	authorizationapi "github.com/openshift/origin/pkg/authorization/api"
 	buildapi "github.com/openshift/origin/pkg/build/api"
 	"github.com/openshift/origin/pkg/client"
+	"github.com/openshift/origin/pkg/cmd/admin/policy"
+	"github.com/openshift/origin/pkg/cmd/server/bootstrappolicy"
 	imageapi "github.com/openshift/origin/pkg/image/api"
 	testutil "github.com/openshift/origin/test/util"
 	testserver "github.com/openshift/origin/test/util/server"
+	kapi "k8s.io/kubernetes/pkg/api"
+	watchapi "k8s.io/kubernetes/pkg/watch"
+	"testing"
 )
 
 const (
@@ -20,7 +21,7 @@ const (
 
 func TestSimpleImageChangeBuildTriggerFromImageStreamTagSTI(t *testing.T) {
 	defer testutil.DumpEtcdOnFailure(t)
-	projectAdminClient := setup(t)
+	projectAdminClient, _ := setup(t)
 	imageStream := mockImageStream2(tag)
 	imageStreamMapping := mockImageStreamMapping(imageStream.Name, "someimage", tag, "registry:8080/openshift/test-image-trigger:"+tag)
 	strategy := stiStrategy("ImageStreamTag", streamName+":"+tag)
@@ -30,7 +31,7 @@ func TestSimpleImageChangeBuildTriggerFromImageStreamTagSTI(t *testing.T) {
 
 func TestSimpleImageChangeBuildTriggerFromImageStreamTagSTIWithConfigChange(t *testing.T) {
 	defer testutil.DumpEtcdOnFailure(t)
-	projectAdminClient := setup(t)
+	projectAdminClient, _ := setup(t)
 	imageStream := mockImageStream2(tag)
 	imageStreamMapping := mockImageStreamMapping(imageStream.Name, "someimage", tag, "registry:8080/openshift/test-image-trigger:"+tag)
 	strategy := stiStrategy("ImageStreamTag", streamName+":"+tag)
@@ -40,7 +41,7 @@ func TestSimpleImageChangeBuildTriggerFromImageStreamTagSTIWithConfigChange(t *t
 
 func TestSimpleImageChangeBuildTriggerFromImageStreamTagDocker(t *testing.T) {
 	defer testutil.DumpEtcdOnFailure(t)
-	projectAdminClient := setup(t)
+	projectAdminClient, _ := setup(t)
 	imageStream := mockImageStream2(tag)
 	imageStreamMapping := mockImageStreamMapping(imageStream.Name, "someimage", tag, "registry:8080/openshift/test-image-trigger:"+tag)
 	strategy := dockerStrategy("ImageStreamTag", streamName+":"+tag)
@@ -50,7 +51,7 @@ func TestSimpleImageChangeBuildTriggerFromImageStreamTagDocker(t *testing.T) {
 
 func TestSimpleImageChangeBuildTriggerFromImageStreamTagDockerWithConfigChange(t *testing.T) {
 	defer testutil.DumpEtcdOnFailure(t)
-	projectAdminClient := setup(t)
+	projectAdminClient, _ := setup(t)
 	imageStream := mockImageStream2(tag)
 	imageStreamMapping := mockImageStreamMapping(imageStream.Name, "someimage", tag, "registry:8080/openshift/test-image-trigger:"+tag)
 	strategy := dockerStrategy("ImageStreamTag", streamName+":"+tag)
@@ -60,7 +61,27 @@ func TestSimpleImageChangeBuildTriggerFromImageStreamTagDockerWithConfigChange(t
 
 func TestSimpleImageChangeBuildTriggerFromImageStreamTagCustom(t *testing.T) {
 	defer testutil.DumpEtcdOnFailure(t)
-	projectAdminClient := setup(t)
+	projectAdminClient, clusterAdminClient := setup(t)
+
+	clusterRoleBindingAccessor := policy.NewClusterRoleBindingAccessor(clusterAdminClient)
+	subjects := []kapi.ObjectReference{
+		{
+			Kind: authorizationapi.SystemGroupKind,
+			Name: bootstrappolicy.AuthenticatedGroup,
+		},
+	}
+	options := policy.RoleModificationOptions{
+		RoleNamespace:       testutil.Namespace(),
+		RoleName:            bootstrappolicy.BuildStrategyCustomRoleName,
+		RoleBindingAccessor: clusterRoleBindingAccessor,
+		Subjects:            subjects,
+	}
+	options.AddRole()
+
+	if err := testutil.WaitForPolicyUpdate(projectAdminClient, testutil.Namespace(), "create", buildapi.Resource(authorizationapi.CustomBuildResource), true); err != nil {
+		t.Fatal(err)
+	}
+
 	imageStream := mockImageStream2(tag)
 	imageStreamMapping := mockImageStreamMapping(imageStream.Name, "someimage", tag, "registry:8080/openshift/test-image-trigger:"+tag)
 	strategy := customStrategy("ImageStreamTag", streamName+":"+tag)
@@ -70,7 +91,31 @@ func TestSimpleImageChangeBuildTriggerFromImageStreamTagCustom(t *testing.T) {
 
 func TestSimpleImageChangeBuildTriggerFromImageStreamTagCustomWithConfigChange(t *testing.T) {
 	defer testutil.DumpEtcdOnFailure(t)
-	projectAdminClient := setup(t)
+	projectAdminClient, _ := setup(t)
+
+	clusterAdminClient, err := testutil.GetClusterAdminClient(testutil.GetBaseDir() + "/openshift.local.config/master/admin.kubeconfig")
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	clusterRoleBindingAccessor := policy.NewClusterRoleBindingAccessor(clusterAdminClient)
+	subjects := []kapi.ObjectReference{
+		{
+			Kind: authorizationapi.SystemGroupKind,
+			Name: bootstrappolicy.AuthenticatedGroup,
+		},
+	}
+	options := policy.RoleModificationOptions{
+		RoleNamespace:       testutil.Namespace(),
+		RoleName:            bootstrappolicy.BuildStrategyCustomRoleName,
+		RoleBindingAccessor: clusterRoleBindingAccessor,
+		Subjects:            subjects,
+	}
+	options.AddRole()
+
+	if err := testutil.WaitForPolicyUpdate(projectAdminClient, testutil.Namespace(), "create", buildapi.Resource(authorizationapi.CustomBuildResource), true); err != nil {
+		t.Fatal(err)
+	}
+
 	imageStream := mockImageStream2(tag)
 	imageStreamMapping := mockImageStreamMapping(imageStream.Name, "someimage", tag, "registry:8080/openshift/test-image-trigger:"+tag)
 	strategy := customStrategy("ImageStreamTag", streamName+":"+tag)
@@ -181,7 +226,7 @@ func mockImageStreamMapping(stream, image, tag, reference string) *imageapi.Imag
 	}
 }
 
-func setup(t *testing.T) *client.Client {
+func setup(t *testing.T) (*client.Client, *client.Client) {
 	testutil.RequireEtcd(t)
 	_, clusterAdminKubeConfigFile, err := testserver.StartTestMaster()
 	if err != nil {
@@ -203,7 +248,7 @@ func setup(t *testing.T) *client.Client {
 		t.Fatalf("unexpected error: %v", err)
 	}
 
-	return projectAdminClient
+	return projectAdminClient, clusterAdminClient
 }
 
 func runTest(t *testing.T, testname string, projectAdminClient *client.Client, imageStream *imageapi.ImageStream, imageStreamMapping *imageapi.ImageStreamMapping, config *buildapi.BuildConfig, tag string) {
@@ -429,7 +474,7 @@ func TestMultipleImageChangeBuildTriggers(t *testing.T) {
 		}
 		return bc
 	}
-	projectAdminClient := setup(t)
+	projectAdminClient, _ := setup(t)
 	config := multipleImageChangeBuildConfig()
 	triggersToTest := []struct {
 		triggerIndex int

test/testdata/bootstrappolicy/bootstrap_cluster_role_bindings.yaml

@@ -242,19 +242,6 @@ items:
   - kind: SystemGroup
     name: system:authenticated
   userNames: null
-- apiVersion: v1
-  groupNames:
-  - system:authenticated
-  kind: ClusterRoleBinding
-  metadata:
-    creationTimestamp: null
-    name: system:build-strategy-custom-binding
-  roleRef:
-    name: system:build-strategy-custom
-  subjects:
-  - kind: SystemGroup
-    name: system:authenticated
-  userNames: null
 - apiVersion: v1
   groupNames:
   - system:authenticated