Don't give builds/custom access to system:authenticated by default #11411

coreydaley · 2016-10-18T13:39:18Z

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

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

liggitt · 2016-10-18T14:14:32Z

let's reach agreement first in the bz that this is something we want to do in the default policy

liggitt · 2016-10-24T12:32:47Z

test/cmd/policy.sh

 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_failure 'oadm policy remove-cluster-role-from-group system:build-strategy-custom system:authenticated'


reverse this test and update the one below: add the role and test os::cmd::expect_success_and_text 'oadm policy who-can create builds/custom' 'system:authenticated'

liggitt · 2016-10-26T01:33:05Z

Need to fix up TestPolicyBasedRestrictionOfBuildConfigCreateAndInstantiateByStrategy

coreydaley · 2016-10-26T01:56:11Z

@liggitt
Yep, I've been working on that test today, along with another one that fails, and writing a couple of new tests for the custom strategy not being available by default.

Thanks for checking in on me though, I appreciate it!

coreydaley · 2016-10-26T16:59:39Z

@liggitt @deads2k Please review

liggitt · 2016-10-27T00:35:50Z

test/cmd/policy.sh

 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'


Custom builds are no longer like any of the other build types, the comments over the blocks of tests no longer apply. Group the custom build tests together and comment them separately

liggitt · 2016-10-27T00:39:34Z

test/integration/build_admission_test.go

@@ -23,7 +35,7 @@ func TestPolicyBasedRestrictionOfBuildCreateAndCloneByStrategy(t *testing.T) {
 	builds := map[string]*buildapi.Build{}

 	// Create builds to setup test
-	for _, strategy := range buildStrategyTypes() {
+	for _, strategy := range buildStrategyTypesDefault() {


I'd rather keep trying to create all types and ensure an error is returned for build types not granted by default (applies to all of these tests)

I think you could just add a couple things to the beginning of the original test and keep the remainder of the file unmodified:

New: ensure restricted types fail, unrestricted types succeed
New: Grant restricted
… continue with original test case:
Ensure all types succeed
Remove permissions
Ensure all types fail

liggitt · 2016-10-27T00:44:13Z

test/integration/build_admission_test.go

@@ -127,8 +139,43 @@ func TestPolicyBasedRestrictionOfBuildConfigCreateAndInstantiateByStrategy(t *te
 	}
 }

-func buildStrategyTypes() []string {
-	return []string{"source", "docker", "custom", "jenkinspipeline"}


I don't think this test is needed if you add the restricted/allowed check to the beginning of the previous test

I think this comment referred to the test that I removed, but ended up on the wrong line somehow?

liggitt · 2016-10-27T00:44:28Z

test/integration/build_admission_test.go

@@ -201,14 +248,14 @@ 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} {


Shouldn't need to change this

liggitt · 2016-10-27T00:45:55Z

test/integration/imagechange_buildtrigger_test.go

+		RoleBindingAccessor: clusterRoleBindingAccessor,
+		Subjects:            subjects,
+	}
+	options.AddRole()


Need to wait for role change to become effective (see other integration tests for example)

This is corrected in the line below, not sure why this isn't marked as outdated now.

coreydaley · 2016-10-28T14:22:36Z

@liggitt The requested updates have been made, and the tests are still passing, please review

liggitt · 2016-10-31T15:29:28Z

test/cmd/policy.sh

 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'


move this line after the custom build test block

liggitt · 2016-10-31T15:32:04Z

test/integration/imagechange_buildtrigger_test.go

@@ -61,6 +62,30 @@ func TestSimpleImageChangeBuildTriggerFromImageStreamTagDockerWithConfigChange(t
 func TestSimpleImageChangeBuildTriggerFromImageStreamTagCustom(t *testing.T) {
 	defer testutil.DumpEtcdOnFailure(t)
 	projectAdminClient := setup(t)
+
+	clusterAdminClient, err := testutil.GetClusterAdminClient(testutil.GetBaseDir() + "/openshift.local.config/master/admin.kubeconfig")


don't hard-code relative paths like this, change setup() to also return the clusterAdminClient

liggitt · 2016-10-31T15:36:03Z

test/integration/build_admission_test.go

+			var err error
+			builds[string(strategy)+clientType], err = createBuild(t, client.Builds(testutil.Namespace()), strategy)
+			_, restricted := restrictedStrategies[strategy]
+			if err != nil && !restricted {


check specifically for a forbidden error (kapierrors.IsForbidden(err))

liggitt · 2016-10-31T18:02:09Z

LGTM once jenkins is green

coreydaley · 2016-10-31T20:30:07Z

[test]

coreydaley · 2016-10-31T20:44:41Z

Possibly due to flake #11661

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

openshift-bot · 2016-11-01T14:05:29Z

Evaluated for origin test up to 206991f

openshift-bot · 2016-11-01T15:34:47Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10915/) (Base Commit: df195a9)

coreydaley · 2016-11-01T15:35:18Z

@liggitt tests have passed, please merge

liggitt · 2016-11-01T15:38:40Z

LGTM, [merge]

openshift-bot · 2016-11-01T15:46:22Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10915/) (Image: devenv-rhel7_5291)

openshift-bot · 2016-11-01T15:46:23Z

Evaluated for origin merge up to 206991f

liggitt self-assigned this Oct 18, 2016

liggitt reviewed Oct 24, 2016

View reviewed changes

liggitt suggested changes Oct 27, 2016

View reviewed changes

liggitt reviewed Oct 31, 2016

View reviewed changes

liggitt approved these changes Oct 31, 2016

View reviewed changes

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

206991f

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

openshift-bot merged commit 8e676c1 into openshift:master Nov 1, 2016

coreydaley deleted the fix_bug_1380462 branch November 1, 2016 20:36

php-coder mentioned this pull request Apr 5, 2017

Document that Custom Build Strategy is disabled by default and how to enable it openshift/openshift-docs#4081

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't give builds/custom access to system:authenticated by default #11411

Don't give builds/custom access to system:authenticated by default #11411

coreydaley commented Oct 18, 2016

liggitt commented Oct 18, 2016

liggitt Oct 24, 2016

liggitt commented Oct 26, 2016

coreydaley commented Oct 26, 2016

coreydaley commented Oct 26, 2016

liggitt Oct 27, 2016

liggitt Oct 27, 2016

liggitt Oct 27, 2016

liggitt Oct 27, 2016

coreydaley Oct 31, 2016

liggitt Oct 27, 2016

liggitt Oct 27, 2016

coreydaley Oct 31, 2016

coreydaley commented Oct 28, 2016 •

edited

Loading

liggitt Oct 31, 2016

liggitt Oct 31, 2016 •

edited

Loading

liggitt Oct 31, 2016

liggitt commented Oct 31, 2016

coreydaley commented Oct 31, 2016

coreydaley commented Oct 31, 2016 •

edited

Loading

openshift-bot commented Nov 1, 2016

openshift-bot commented Nov 1, 2016

coreydaley commented Nov 1, 2016

liggitt commented Nov 1, 2016

openshift-bot commented Nov 1, 2016 •

edited

Loading

openshift-bot commented Nov 1, 2016

		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'

Don't give builds/custom access to system:authenticated by default #11411

Don't give builds/custom access to system:authenticated by default #11411

Conversation

coreydaley commented Oct 18, 2016

liggitt commented Oct 18, 2016

Choose a reason for hiding this comment

liggitt commented Oct 26, 2016

coreydaley commented Oct 26, 2016

coreydaley commented Oct 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coreydaley commented Oct 28, 2016 • edited Loading

Choose a reason for hiding this comment

liggitt Oct 31, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Oct 31, 2016

coreydaley commented Oct 31, 2016

coreydaley commented Oct 31, 2016 • edited Loading

openshift-bot commented Nov 1, 2016

openshift-bot commented Nov 1, 2016

coreydaley commented Nov 1, 2016

liggitt commented Nov 1, 2016

openshift-bot commented Nov 1, 2016 • edited Loading

openshift-bot commented Nov 1, 2016

coreydaley commented Oct 28, 2016 •

edited

Loading

liggitt Oct 31, 2016 •

edited

Loading

coreydaley commented Oct 31, 2016 •

edited

Loading

openshift-bot commented Nov 1, 2016 •

edited

Loading