[a2-694] Add policy for applications service IAM v1 and v2 #462

kmacgugan · 2019-05-31T00:05:09Z

For v1 add resource service_groups
For v2 add resource applications:serviceGroups

Signed-off-by: kmacgugan [email protected]

🔩 Description

👍 Definition of Done

👟 Demo Script / Repro Steps

⛓️ Related Resources

✅ Checklist

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

- For v1 add resource service_groups - For v2 add resource applications:serviceGroups Signed-off-by: kmacgugan <[email protected]>

susanev · 2019-05-31T01:01:03Z

do we want any docs changes related to this?
v1: https://automate.chef.io/docs/default-policies/
v2: https://automate.chef.io/docs/iam-v2-api-reference/

should we be updating the viewer and editor iam roles as well?

srenatus

Looks good to me. However, I suppose this needs some discussion for IAM v2 -- is this supposed to be accessible out of the box for viewers/editors? If so, it needs to be added to their default policies.

We've recently merged a change that adds these users locally, and upgrades IAM to v2, as part of start_all_services. If you login using viewer or editor (password chefautomate), you can observe the current behaviour, without these additions.

srenatus · 2019-05-31T07:18:48Z

api/external/applications/applications.proto

@@ -21,41 +21,41 @@ service ApplicationsService {
    // TODO (dan, 2/2019): need to replace this once we have resources and such


So can this comment now go away?

bcmdarroch · 2019-05-31T17:38:45Z

components/authz-service/storage/v1/storage.go

@@ -300,6 +300,14 @@ func DefaultPolicies() (map[string]*Policy, error) {
 			Effect:   "allow",
 			Version:  1,
 		},
+		constants.ApplicationsServiceGroupsPolicyID: {


You've added this policy to our system policies, which are hidden policies that govern permissions that Automate needs to work correctly (i.e. every user always needs to be able to get the license status so they won't be always blocked by the license modal). These policies never change.

My guess is that we should be able to change permissions on this API. In that case, instead of adding this policy, we'll want to add the API's v2 action to the role(s) that should have access. Maybe Viewer and Editor?

actually just kidding about that system policy comment! i misread the code. this is correct for adding a v1 default policy 👍

for v2 permissions:

add applications:* to the Editor role

add applications:serviceGroups:list to the Viewer role

add a datamigration updating those two roles with their new actions (similar to this migration)

If we make changes to these roles please also update the iam v2 docs, thx!

bcmdarroch · 2019-05-31T18:30:40Z

api/external/applications/applications.proto

-    option (chef.automate.api.iam.policy).resource = "infra:nodes";
-    option (chef.automate.api.iam.policy).action = "infra:nodes:list";
+    option (chef.automate.api.iam.policy).resource = "applications:serviceGroups";
+    option (chef.automate.api.iam.policy).action = "applications:serviceGroups:list";


If you can GetServices, can you always get their healthcounts and get services by SG?

Yes, at least you should be able to.

gotcha, makes sense that all the resources/actions should be the same then. 👍

bcmdarroch · 2019-05-31T18:49:54Z

components/authz-service/storage/postgres/migration/sql/45_add_application_policies.up.sql

+
+INSERT INTO policies
+    VALUES ('aee14d59-da0b-4974-ba6d-1a018b024874',
+            '{"action": "*", "effect": "allow", "resource": "applications:serviceGroups", "subjects": ["user:*"]}',


the resource here must match the v1 resource.

Suggested change

'{"action": "*", "effect": "allow", "resource": "applications:serviceGroups", "subjects": ["user:*"]}',

'{"action": "*", "effect": "allow", "resource": "service_group", "subjects": ["user:*"]}',

-Update v1 policies to actually be v1 policy and not the v2 ones Signed-off-by: kmacgugan <[email protected]>

bcmdarroch · 2019-05-31T22:34:57Z

components/authz-service/storage/v2/storage.go

@@ -254,6 +255,7 @@ func DefaultRoles() []Role {
 			"event:*:list",
 			"ingest:*:get",
 			"ingest:*:list",
+			"applications:*:list",


is there a possibility there will be an individual getService in the near future? if so we may want to add applications:*:get as well while we're here

bcmdarroch · 2019-05-31T22:37:56Z

components/authz-service/storage/postgres/migration/sql/45_add_application_policies.up.sql

+    SET policy_data='{"action": "list", "effect": "allow", "resource": "service_groups", "subjects": ["user:*"]}',
+        deletable=TRUE;
+
+UPDATE iam_roles


ah i forgot to mention one thing 😅
hopefully last thing to fix: move L13-23 into a new migration file under the postgres/datamigrations/sql directory

tylercloke

Please make the change @bcmdarroch mentioned to the sql migration but otherwise g2g.

msorens

Generally looks good; three corrections needed.

msorens · 2019-06-01T00:12:02Z

components/authz-service/storage/postgres/migration/sql/45_add_application_policies.up.sql

+            1,
+            TRUE)
+    ON CONFLICT (id) DO UPDATE
+    SET policy_data='{"action": "list", "effect": "allow", "resource": "service_groups", "subjects": ["user:*"]}',


This action must be the same as the action on L5.

msorens · 2019-06-01T00:23:01Z

components/authz-service/storage/v2/storage.go

@@ -234,6 +234,7 @@ func DefaultRoles() []Role {
 			"ingest:*",
 			"secrets:*",
 			"telemetry:*",
+			"applications:*",


This is incompatible with 45_add_application_policies.up.sql and will result in the entry being added to the database twice. Just delete this line and all is well.

Same for L258 below.

Signed-off-by: kmacgugan <[email protected]>

blakestier · 2019-06-03T22:39:00Z

components/authz-service/storage/postgres/datamigration/sql/07_update_v2_roles.up.sql

+        actions = actions || '{applications:*}'
+    WHERE
+        id = 'editor';
+END;


What is this END doing that's distinct from COMMIT?

probably i just wanted commit and end snuck in there. Commit will commit a transaction, end just delineates a block of code, like if it were in a loop. I think maybe I copied this structure from a loop somewhere?

Signed-off-by: kmacgugan <[email protected]>

kmacgugan · 2019-06-04T00:33:28Z

@susanev I added docs for default policies.

afiune · 2019-06-04T13:04:20Z

api/external/applications/applications.proto

  };
  rpc GetServices(ServicesReq) returns (ServicesRes) {
    // TODO (afiune, 4/2019): need to replace this once we have resources and such
    // created in the auth system
    option (google.api.http).get = "/beta/applications/services";
-    option (chef.automate.api.policy).resource = "nodes";
+    option (chef.automate.api.policy).resource = "service_groups";


I don't think this endpoint has anything to do with service-groups here. This is only listing all services so we would probably need a different resource?

For any future readers of this, we decided to stick with service_groups because this matches the labeling in the UI, so that is what users will expect to see.

mjingle

Thank you for updating the docs portion! Doc additions looks good to me.

[a2-694] Add policy for applications service IAM v1 and v2

3a9da67

- For v1 add resource service_groups - For v2 add resource applications:serviceGroups Signed-off-by: kmacgugan <[email protected]>

susanev added automate-auth eas labels May 31, 2019

srenatus approved these changes May 31, 2019

View reviewed changes

bcmdarroch reviewed May 31, 2019

View reviewed changes

-Add v2 IAM policies to viewer and editor role

7c7959a

-Update v1 policies to actually be v1 policy and not the v2 ones Signed-off-by: kmacgugan <[email protected]>

kmacgugan force-pushed the km/applications-policy branch from 2bfa0c1 to 7c7959a Compare May 31, 2019 22:19

bcmdarroch reviewed May 31, 2019

View reviewed changes

tylercloke approved these changes May 31, 2019

View reviewed changes

msorens suggested changes Jun 1, 2019

View reviewed changes

Split iam role migration to datamigration directory

7a0b296

Signed-off-by: kmacgugan <[email protected]>

msorens approved these changes Jun 3, 2019

View reviewed changes

blakestier reviewed Jun 3, 2019

View reviewed changes

kmacgugan added 2 commits June 3, 2019 16:34

Regenerate protos, remove END; from sql

ce1d770

Signed-off-by: kmacgugan <[email protected]>

Add documents for default policies

43ace07

Signed-off-by: kmacgugan <[email protected]>

kmacgugan requested a review from a team as a code owner June 4, 2019 00:33

susanev added the documentation Anything related to the Automate docs. label Jun 4, 2019

afiune reviewed Jun 4, 2019

View reviewed changes

mjingle approved these changes Jun 4, 2019

View reviewed changes

kmacgugan merged commit 7392f96 into master Jun 5, 2019

chef-ci deleted the km/applications-policy branch June 5, 2019 21:07

srenatus mentioned this pull request Jun 17, 2019

[A2-882] authz-service: fix v1 to v2 policy migration issues with applications #612

Merged

8 tasks

susanev added the auth-team anything that needs to be on the auth team board label Jul 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[a2-694] Add policy for applications service IAM v1 and v2 #462

[a2-694] Add policy for applications service IAM v1 and v2 #462

kmacgugan commented May 31, 2019

susanev commented May 31, 2019 •

edited

Loading

srenatus left a comment

srenatus May 31, 2019

bcmdarroch May 31, 2019

bcmdarroch May 31, 2019

susanev May 31, 2019

bcmdarroch May 31, 2019 •

edited

Loading

kmacgugan May 31, 2019

bcmdarroch May 31, 2019

bcmdarroch May 31, 2019 •

edited

Loading

bcmdarroch May 31, 2019

bcmdarroch May 31, 2019

tylercloke left a comment

msorens left a comment

msorens Jun 1, 2019

msorens Jun 1, 2019

blakestier Jun 3, 2019

kmacgugan Jun 3, 2019

kmacgugan commented Jun 4, 2019

afiune Jun 4, 2019

danielsdeleo Jun 4, 2019

mjingle left a comment

		@@ -21,41 +21,41 @@ service ApplicationsService {
		// TODO (dan, 2/2019): need to replace this once we have resources and such

	'{"action": "", "effect": "allow", "resource": "applications:serviceGroups", "subjects": ["user:"]}',
	'{"action": "", "effect": "allow", "resource": "service_group", "subjects": ["user:"]}',

[a2-694] Add policy for applications service IAM v1 and v2 #462

[a2-694] Add policy for applications service IAM v1 and v2 #462

Conversation

kmacgugan commented May 31, 2019

🔩 Description

👍 Definition of Done

👟 Demo Script / Repro Steps

⛓️ Related Resources

✅ Checklist

susanev commented May 31, 2019 • edited Loading

srenatus left a 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

bcmdarroch May 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcmdarroch May 31, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tylercloke left a comment

Choose a reason for hiding this comment

msorens left a 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

kmacgugan commented Jun 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjingle left a comment

Choose a reason for hiding this comment

susanev commented May 31, 2019 •

edited

Loading

bcmdarroch May 31, 2019 •

edited

Loading

bcmdarroch May 31, 2019 •

edited

Loading