Handle empty statement projects for v2 and v2.1 #886

msorens · 2019-07-12T23:26:53Z

🔩 Description

In 2.1 policy statements must have a non-empty project.
Any policies created in v1, when migrated, will get "all projects" (*) assigned during migration.
Any policies created in v2.0, however, do not go through any migration. Therefore they must be created with the same "all projects" (*) silently appended if no projects are specified.

Bug #1: Creating a policy in v2.0 has an empty projects list on statements.
Bug #2: Creating a policy in v2.1 with no statement projects generates an invalid policy, i.e. a policy with no projects.

👍 Definition of Done

V2.0 policies with an empty projects list are assigned the default "all projects" (*).
Attempt to create v2.1 policies with an empty projects list are rejected.

👟 Demo Script / Repro Steps

rebuild components/authz-service

In v2.0, create a policy with no statement projects, e.g. :

curl -sSkH "api-token: $TOK" -d \
'{"id":"team-view-pol", "name":"team viewer", "members":["user:local:coolbean"], \
"statements": [{"role": "team-viewer"}]}' $TARGET_HOST/apis/iam/v2beta/policies

Read it back and observe it has the default * assigned. Here's one way to do that:

authQuery -o -2 policies id team-view-pol | jq

Of course, if you explicitly include all projects in the statement (projects: ["*"]), that should return an identical result.

In v2.1, if you attempt to create a policy with no statement projects you will now receive an error, e.g.:

{
  "error": "error parsing policy \"team-view-pol3\": parse statement: project statements must include projects",
  "message": "error parsing policy \"team-view-pol3\": parse statement: project statements must include projects",
  "code": 3,
  "details": []
}

⛓️ Related Resources

✅ Checklist

Necessary tests added/updated?
Necessary docs added/updated?
Code actually executed?
Vetting performed (unit tests, lint, etc.)?

msorens · 2019-07-12T23:29:20Z

components/authz-service/server/v2/policy.go

 	vChan chan api.Version) (PolicyServer, PolicyRefresher, error) {

-	return NewPoliciesServer(ctx, l, memstore.New(), e, pl, vChan)
+	return NewPoliciesServer(ctx, l, memstore.New(), e, pl, vSwitch, vChan)


Of course, vSwitch and vChan are related but they serve different functions, so I thought it best to keep them cleanly separate.

I don't think I'm clear on what the difference is anymore. Could we have a helpful comment somewhere?

vSwitch is two things:

an UnaryServerInterceptor, denying v1 reqs when using v2 and vice-versa (this PreconditionFailed response drives the gateway IAM version switching)

a struct keeping an api.Version

It would be better, I think, as outlined in #853, to have a version field in the server struct, and have that server implement the Interceptor.

vChan is a channel we're using to flip the version in that struct; based on a successful migration (upgrade-to-v2) or read on server startup (ie. when it's already been switched to IAM v2). It's a shortcut; it shouldn't be needed. We're creating a portal between two pieces of the service (the migration logic and the policies server), and have the former throw something through it at the latter. It's not how things should be, this is not something inherently calling for the use of a channel.

msorens · 2019-07-12T23:30:05Z

components/authz-service/server/v2/policy.go

 	projects := make([]string, len(statement.Projects))
+
+	if len(statement.Projects) == 0 {
+		if s.isBeta2p1() {


Everything above here was to be able to have this function exist--now we can distinguish v2/v2.1 and take appropriate action.

msorens · 2019-07-12T23:30:56Z

components/authz-service/server/v2/policy_test.go

@@ -467,6 +469,159 @@ func TestCreatePolicy(t *testing.T) {
 	}
 }

+func TestCreatePolicy2p1(t *testing.T) {


Add some explicit 2.1 tests where it matters: create and update with no projects.

msorens · 2019-07-12T23:31:54Z

components/authz-service/server/v2/policy_test.go

@@ -69,7 +69,7 @@ func TestCreatePolicy(t *testing.T) {
 			assert.Equal(t, []string{"cfgmgmt:nodes:*"}, resp.Statements[0].Actions)
 			assert.Equal(t, []string{"cfgmgmt:delete", "cfgmgmt:list"}, resp.Statements[0].Resources)
 			assert.Empty(t, resp.Statements[0].Role)
-			assert.Empty(t, resp.Statements[0].Projects)
+			assert.Equal(t, []string{constants_v2.AllProjectsExternalID}, resp.Statements[0].Projects)


This affirms the change that no projects provided in 2.0 now is assigned a default "all projects".

msorens · 2019-07-12T23:33:09Z

components/authz-service/server/v2/policy_test.go

@@ -919,19 +1074,17 @@ func TestUpdatePolicy(t *testing.T) {
 		}},
 		{"successfully updates policy statements", func(t *testing.T) {
 			storedPol, _ := addSomePoliciesToStore(t, store, prng)
-			// change original statement effect to allow


Comments in this test were outdated (read: wrong)... I repurposed the test to handle both explicit and implicit "all" cases.

msorens · 2019-07-12T23:33:38Z

components/authz-service/server/v2/policy_test.go

 			s0 := pol.Statements[0]
 			assert.Equal(t, api_v2.Statement_ALLOW, s0.Effect)
 			assert.Equal(t, []string{"compliance:profiles"}, s0.Resources)
 			assert.Equal(t, []string{"compliance:profiles:upload"}, s0.Actions)
+			assert.Equal(t, []string{constants_v2.AllProjectsExternalID}, s0.Projects)


Implicit or explicit look the same in the end.

If the implicit way (as in, no projects in the statement means it applies to all projects) still works like before, why do we need to start enforcing projects on all v2.1 statements?

But it has never been legal to have a statement with no projects.

Do you mean according to the acceptance requirements?

msorens · 2019-07-12T23:34:13Z

components/authz-service/server/v2/policy_test.go

+		storageProject = constants_v2.AllProjectsExternalID
+	}
+	assert.Equal(t, storageProject, apiProject, "statement projects differ")
+}


Needed just this one little tweak to handle the internal/external representation differences.

msorens · 2019-07-12T23:34:56Z

components/authz-service/server/v2/policy_test.go

+	return setupV2WithMigrationState(t, nil, writer, nil, nil, make(chan api_v2.Version, 1),
+		// Returning MigrationStatus of "SuccessBeta2.1" means we've migrated successfully to IAM v2.1
+		func(s storage.MigrationStatusProvider) error { return s.SuccessBeta1(context.Background()) })
+}


Thanks, @srenatus, for the framework that made adding v2.1 just plug-n-play, as it were!

blakestier · 2019-07-13T02:42:43Z

components/authz-service/server/server.go

-	v2PolServer, policyRefresher, err := v2.NewPostgresPolicyServer(ctx, l, e, migrationsConfig,
-		dataMigrationsConfig, v1Server.Storage(), vChan)
+	v2PolServer, policyRefresher, err := v2.NewPostgresPolicyServer(
+		ctx, l, e, migrationsConfig, dataMigrationsConfig, v1Server.Storage(), switcher, vChan)


New addition is the switcher (line change gave me pause)

blakestier · 2019-07-13T02:59:46Z

components/authz-service/server/v2/policy.go

@@ -140,7 +146,7 @@ func (s *policyServer) CreatePolicy(

 	// API requests always create custom policies.

-	pol, err := policyFromAPI(
+	pol, err := s.policyFromAPI(


I'm curious why the change to have this be a function on server . . . maybe it's too late for me to be reviewing? 🙃

No, that's a fair question! It is (almost) turtles all the way down. In other words, at the bottom, we need to access the new vSwitch which is on s. Follow that back up to here: s.vSwitch => s.isBeta2p1 => s.statementFromAPI => s.policyFromAPI.

🤔 Is that a smell? 🤷‍♂

srenatus · 2019-07-15T07:59:49Z

components/automate-cli/pkg/diagnostics/integration/iam_v2.go

+	}
+	return resp.Version.Major, nil
+}
+
 func init() {


I'm afraid init() is rarely the correct move 😉
I'd propse we do the following:

add an iam-v2 tag to diagnostic tests that require IAM v2 (here: https://github.com/chef/automate/pull/886/files#diff-78d181d2f500dd5af4a70ec53130c49cR52)

use diagnostic filters for these, like https://github.com/chef/automate/blob/master/integration/tests/upgrade_current_master.sh#L4

upgrade-to-v2 in the integration/tests/*.sh scripts that run the iam v2 diagnostic tests

✔️ we've squashed that away now.

bcmdarroch

I'd like to better understand why we need to start enforcing projects on all v2.1 policy statements if the implicit empty projects still works (at least it does according to this UpdatePolicy test case)

bcmdarroch · 2019-07-15T22:54:56Z

components/authz-service/server/v2/policy.go

 	vChan chan api.Version) (PolicyServer, PolicyRefresher, error) {

-	return NewPoliciesServer(ctx, l, memstore.New(), e, pl, vChan)
+	return NewPoliciesServer(ctx, l, memstore.New(), e, pl, vSwitch, vChan)


I don't think I'm clear on what the difference is anymore. Could we have a helpful comment somewhere?

bcmdarroch · 2019-07-15T23:11:57Z

components/authz-service/server/v2/policy_test.go

 			s0 := pol.Statements[0]
 			assert.Equal(t, api_v2.Statement_ALLOW, s0.Effect)
 			assert.Equal(t, []string{"compliance:profiles"}, s0.Resources)
 			assert.Equal(t, []string{"compliance:profiles:upload"}, s0.Actions)
+			assert.Equal(t, []string{constants_v2.AllProjectsExternalID}, s0.Projects)


If the implicit way (as in, no projects in the statement means it applies to all projects) still works like before, why do we need to start enforcing projects on all v2.1 statements?

srenatus

This is #853 territory 😄

I'd prefer we didn't do it this way: We're adding the version switch construct, which "knows" the version, to the server. Instead, we should make the server aware of the version, and have the version switch read that.

srenatus · 2019-07-16T11:37:35Z

This failure is unrelated to diagnostics: https://buildkite.com/chef/chef-automate-master-verify-private/builds/3158#9da55f3b-f5ce-4db4-8e61-f8f52a268eea/579-1323

srenatus · 2019-07-17T08:48:10Z

components/authz-service/server/v2/policy.go

@@ -902,3 +922,8 @@ func (s *policyServer) setVersionForInterceptorSwitch(v api.Version) {
 		s.vChan <- v
 	}
 }
+
+func (s *policyServer) isBeta2p1() bool {


This falls apart when we're using IAM v2 endpoints with authz-service running on v1. This is what our inspec tests have been doing for a while now.

srenatus · 2019-07-17T11:31:16Z

Team: If you look at the tests changes, you'll note that for v2, this will now include "*" in the statements' bodies: a2-iam-v2-integration/iam_v2.

It's a by-product of this approach to resolving the issue. I guess we could fiddle around a bit to remove this again if you're not on v2.1; but we already have accepted that there's projects keys in the return messages... so, adding this doesn't seem too bad.

What I find confusing, however, is that only the statements' projects is set. 🤔

srenatus · 2019-07-17T11:46:17Z

Also, with disabling the iam-v1 token diagnostic for the iam v2 run, something has become apparent (to me): We have no upgrade test safety whatsoever for iam v2 or iam v2.1 from our diagnostics framework. I think I'll duplicate the upgrade tests...?

Fine, let's do the dirty thing once more -- there's a card for tackling this version business already.

srenatus · 2019-07-17T13:46:31Z

Still working on getting the added diagnostics test to fail properly, and then to fix it.

blakestier · 2019-07-17T15:46:03Z

...ervice/storage/postgres/datamigration/sql/09_make_projects_wildcard_an_actual_project.up.sql

@@ -0,0 +1,5 @@
+BEGIN;
+
+INSERT INTO iam_projects (id, name, type) VALUES ('*', 'Wildcard project', 'chef-managed');


we do have the ~~ALL PROJECTS~~ project . . . cannot recall why we didn't use * all the way down . . .

(I'm not clear on why we need this migration)

Frankly, I've made @msorens' approach work. I don't know! 💦

Er, something is amiss 🤔

I'm also confused. This migration makes me think we're already populating empty statements with ~~All Projects~~

blakestier · 2019-07-17T15:50:03Z

components/automate-cli/pkg/diagnostics/integration/auth_policies.go

@@ -20,7 +20,7 @@ type authPoliciesSave struct {
 func CreateAuthPoliciesDiagnostic() diagnostics.Diagnostic {
 	return diagnostics.Diagnostic{
 		Name: "auth-policies",
-		Tags: diagnostics.Tags{"auth"},
+		Tags: diagnostics.Tags{"auth", "iam-v1"},


Oof shouldn't we have a test where we do migrate the v1 policies? 😩

This is not the v1-to-v2 migration, this is used to (among other things) test an actual upgrade of the A2 version installed.

Also, we've got "did these policies get migrated?" tests; but they only assert that a migrated policy is there, I don't think they assert that they're working. However, implicitly, I believe this is tested for admin tokens 🤔

blakestier · 2019-07-17T15:54:31Z

integration/tests/iam_v2_upgrade_diagnostics.sh

@@ -10,8 +10,8 @@ test_upgrades=true
 # do_test_deploy runs _before_ the upgrade, so we upgrade _from a state with
 # IAMv2 enabled_, and the diagnostics are run twice: before and after upgrade.
 do_test_deploy() {
-    log_info "run chef-automate iam upgrade-to-v2 --skip-policy-migration"
-    chef-automate iam upgrade-to-v2 --skip-policy-migration || return 1
+    log_info "run chef-automate iam upgrade-to-v2"


oh this is what i meant in my prev comment 😂

srenatus · 2019-07-18T11:02:32Z

@blakestier as per discussion in the deployment-team channel -- since we're testing upgrades here, too (trying to do a much as we can reasonably do in one test), the first deployment is happening with code in dev. So, our changes to the deployment-service (through diagnostics) aren't there when we need them. I've thus split them off into their own PR: #946. When that's merged, we can rebase this one, and see where we get with it.

update I was a bit absent-minded there. With #948 we should be fine.

update I was a bit absent-minded there. Again. With #951 we should be fine.

We were not dealing with empty statement projects correctly. For v2, it should be coerced to a single element list containing "*" indicating all projects. For v2.1, it should fail as an error. Signed-off-by: michael sorens <[email protected]>

Signed-off-by: michael sorens <[email protected]>

Signed-off-by: Stephan Renatus <[email protected]>

This is returned now. The question we'll have to discuss is if this is acceptable. I think it might be, since we've already included a projects field in the responses for iam v2 (not v2.1). The distinction just isn't as clear-cut as I'd like it to be right now, but it might not be worth fixing now we're so deep into this. Signed-off-by: Stephan Renatus <[email protected]>

These would only pass if we migrated the v1 policies. We don't. Signed-off-by: Stephan Renatus <[email protected]>

Is it that easy? We'll see. Signed-off-by: Stephan Renatus <[email protected]>

Signed-off-by: Stephan Renatus <[email protected]>

Disabled user/team diagnostics when explicitly testing v2 -- there are no corresponding v2 (or v2.1) tests yet! Signed-off-by: Stephan Renatus <[email protected]>

tylercloke

If this gets things working again, let's do it! We have a card to deal refactoring how all the version handling is working so that seems like the place to address any tech debt here. Thanks for digging into this 🕵️‍♀️

tylercloke · 2019-07-19T17:07:17Z

integration/tests/iam_v2_upgrade_diagnostics.sh

+    log_info "run chef-automate iam upgrade-to-v2 --skip-policy-migration"
+    chef-automate iam upgrade-to-v2 --skip-policy-migration || return 1
+
+    # ensure service startup works with IAM v2:


msorens added automate-auth auth-team anything that needs to be on the auth team board labels Jul 12, 2019

msorens self-assigned this Jul 12, 2019

msorens commented Jul 12, 2019

View reviewed changes

msorens requested a review from a team July 12, 2019 23:35

blakestier reviewed Jul 13, 2019

View reviewed changes

srenatus reviewed Jul 15, 2019

View reviewed changes

bcmdarroch reviewed Jul 15, 2019

View reviewed changes

srenatus previously requested changes Jul 16, 2019

View reviewed changes

msorens force-pushed the ms/empty-projects-fix branch from 0a3f7a6 to 27a798b Compare July 16, 2019 21:12

srenatus force-pushed the ms/empty-projects-fix branch from 27a798b to 94b07f6 Compare July 17, 2019 07:27

msorens requested a review from a team as a code owner July 17, 2019 08:32

srenatus reviewed Jul 17, 2019

View reviewed changes

srenatus force-pushed the ms/empty-projects-fix branch 2 times, most recently from 344f98e to a2b9366 Compare July 17, 2019 11:16

srenatus force-pushed the ms/empty-projects-fix branch from 9a4ae3f to 7d659b2 Compare July 17, 2019 13:45

srenatus self-assigned this Jul 17, 2019

blakestier reviewed Jul 17, 2019

View reviewed changes

blakestier force-pushed the ms/empty-projects-fix branch from 571ddd3 to 7d659b2 Compare July 17, 2019 17:08

srenatus force-pushed the ms/empty-projects-fix branch from 94c8b22 to 83723bd Compare July 17, 2019 17:31

blakestier force-pushed the ms/empty-projects-fix branch 2 times, most recently from ef084eb to c4a8e11 Compare July 17, 2019 19:21

blakestier force-pushed the ms/empty-projects-fix branch 5 times, most recently from bc211ce to 9a48129 Compare July 17, 2019 23:50

srenatus force-pushed the ms/empty-projects-fix branch 2 times, most recently from 4e010e9 to 67875a4 Compare July 18, 2019 14:55

srenatus mentioned this pull request Jul 18, 2019

diagnostics: add iam v2 upgrade test #953

Closed

blakestier added WIP DO-NOT-MERGE labels Jul 18, 2019

blakestier force-pushed the ms/empty-projects-fix branch from 67875a4 to bc37cf8 Compare July 19, 2019 16:09

msorens and others added 11 commits July 19, 2019 09:17

Add unit tests for v2.1

14f41aa

Signed-off-by: michael sorens <[email protected]>

diagnostics/iamv2: use tag to disable for non-iam-v2 integration tests

243f7ed

Signed-off-by: Stephan Renatus <[email protected]>

inspec: move iam-v2 controls into a2-iam-v2-integration

890402f

Signed-off-by: Stephan Renatus <[email protected]>

integration: add IAM v2 diagnostics run

59fd441

Signed-off-by: Stephan Renatus <[email protected]>

backup_ontop.sh: skip iam-v2, too

74f6dec

Signed-off-by: Stephan Renatus <[email protected]>

integration/iam_v2_only_diagnostics: skip iam v1 tests

04615f2

These would only pass if we migrated the v1 policies. We don't. Signed-off-by: Stephan Renatus <[email protected]>

iam_v2_only_diagnostics: remove cruft, enable upgrades

9d07ffc

Is it that easy? We'll see. Signed-off-by: Stephan Renatus <[email protected]>

integration: add comment, rename test

de01a46

Signed-off-by: Stephan Renatus <[email protected]>

diagnostics/iamv2: disable more iam-v1

b2f0173

Disabled user/team diagnostics when explicitly testing v2 -- there are no corresponding v2 (or v2.1) tests yet! Signed-off-by: Stephan Renatus <[email protected]>

blakestier force-pushed the ms/empty-projects-fix branch from bc37cf8 to b2f0173 Compare July 19, 2019 16:18

blakestier removed DO-NOT-MERGE WIP labels Jul 19, 2019

tylercloke approved these changes Jul 19, 2019

View reviewed changes

blakestier merged commit 1e932f3 into master Jul 19, 2019

blakestier deleted the ms/empty-projects-fix branch July 19, 2019 17:21

srenatus mentioned this pull request Jul 22, 2019

nightly integration tests: skip diagnostics/iamv2 #994

Merged

		@@ -0,0 +1,5 @@
		BEGIN;

		INSERT INTO iam_projects (id, name, type) VALUES ('*', 'Wildcard project', 'chef-managed');

Handle empty statement projects for v2 and v2.1 #886

Handle empty statement projects for v2 and v2.1 #886

Conversation

msorens commented Jul 12, 2019

🔩 Description

👍 Definition of Done

👟 Demo Script / Repro Steps

⛓️ Related Resources

✅ Checklist

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

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

msorens Jul 13, 2019 • edited Loading

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

bcmdarroch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srenatus left a comment

Choose a reason for hiding this comment

srenatus commented Jul 16, 2019

Choose a reason for hiding this comment

srenatus commented Jul 17, 2019

srenatus commented Jul 17, 2019

srenatus commented Jul 17, 2019

blakestier Jul 17, 2019 • edited Loading

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

srenatus commented Jul 18, 2019 • edited Loading

tylercloke left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msorens Jul 13, 2019 •

edited

Loading

blakestier Jul 17, 2019 •

edited

Loading

srenatus commented Jul 18, 2019 •

edited

Loading