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

srenatus · 2019-06-17T12:53:52Z

🔩 Description

skip the v1 default policy for service_groups: it's implemented by additions to the viewer and editor roles in v2 (v2.1)
add an integration test to ensure we don't introduce "unknown well-known" (🤦‍♂) v1 policies again
add the required mapping logic to have customers' v1 policies for service_groups properly migrated
add some docs indicating how to properly add new default policies to A2

👍 Definition of Done

Three ✅ above.

👟 Demo Script / Repro Steps

rebuild components/authz-service
chef-automate iam reset-to-v1
export TOK=$(chef-automate admin-token)

create a custom service_groups v1 policy:

curl -kH "api-token: $TOK" https://localhost/api/v0/auth/policies -d '{"action":"*","subjects" ["user:*"],"resource":"service_groups","effect":"allow"}'

upgrade to v2:

# chef-automate iam upgrade-to-v2

Upgrading to IAM v2
Migrating v1 policies...
Creating default teams Editors and Viewers...
Skipped: Editors team already exists
Skipped: Viewers team already exists

Migrating existing teams...

Success: Enabled IAM v2

☝️ notice that there's none of these errors anymore:

Skipped policies:
  convert v1 policy "aee14d59-da0b-4974-ba6d-1a018b024874"
  unknown "well-known" policy

  convert v1 policy "9afdf13e-74ab-4797-8a31-32ece05938b4"
  could not derive v2 resource: did not recognize base v1 resource term: service_groups

⛓️ Related Resources

✅ Checklist

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

srenatus

Some inline reviewer notes 🙃

srenatus · 2019-06-17T14:18:30Z

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

@@ -419,133 +420,127 @@ func customPolicyFromV1(pol *storage_v1.Policy) (*storage.Policy, error) {

 // Basically implements "Current Resource" -> "New Resource Names (RFR)" column of
 // https://docs.google.com/spreadsheets/d/1ccaYDJdMnHBfFgmNC1n2_S1YOnMJ-OgkYd8ySbb-mS0/edit#gid=363200100
-func convertV1Resource(resource string) ([]string, error) {


Drive-by: this function's return value was never more than a slice-of-one. So, I've made it a string, and fixed the fallout.

srenatus · 2019-06-17T14:19:16Z

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

 	case "notifications":
-		return combineTermsIntoResourceSlice(terms), nil
-	case "*":


This was a bug: *:foo:bar would have been converted to *. It's now tighter, above

srenatus · 2019-06-17T14:19:54Z

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

@@ -2239,6 +2239,21 @@ func TestV1PolicyMigration(t *testing.T) {
 				}),
 			),
 		},
+		"service_groups,list": {


ℹ️ Converting a custom v1 policy for service groups into a custom v2 policy.

srenatus · 2019-06-17T14:20:21Z

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

@@ -2279,6 +2294,10 @@ func TestV1PolicyMigration(t *testing.T) {
 			wellknown(t, constants_v1.CSNginxComplianceDataCollectorPolicyID),
 			checks(isSkipped()),
 		},
+		"v1 default application policy": {


ℹ️ Skipping the default v1 application policy: it's handled by viewer and editor roles in v2.

I raised the question to Kyleen, to whom this actually matters 😁, and she has confirmed it is not the same. What they want is "all users to have default access to all [application] things". Right now anyone with a viewer role or on the viewers team has access, but all users are not viewers by default. So I think we need a migration for this policy. See https://chefio.slack.com/archives/CFCE1NQ9W/p1560797578017700

I'd like to pull in Susan's feedback here. In IAM v2, we don't really follow any concept of "all users having default access" anymore.

Also, if we encourage our customers not to migrate v1 policies, we cannot rely on them for any nice out-of-the-box experience when using IAM v2.

srenatus · 2019-06-17T14:21:47Z

integration/tests/iam_v2.sh

+        log_error "$output"
+        return 1
+    else
+        # in CI, we don't want any policies to be skipped


This will catch "unknown well-known" policies creeping in again in the future. I've ensured that the test works by running it on master in this build

blakestier · 2019-06-17T15:34:06Z

components/authz-service/README.md

+2. A new IAM v2 policy: they are defined in [server/v2/system](server/v2/system.go#L31).
+3. Migration logic: When upgrading from IAM v1 to v2 (v2.1), v1 policies are
+   converted. The conversion logic needs to be made aware of the new v1 policies, and
+   how (and if) they are to be migrated (if they hadn't been deleted). The procedure for


I think we want "if they haven't been deleted" . . . but that is confusing, also. You mean if they haven't been migrated?

You're right about the grammar, for course -- what this was trying to describe is that the migration code only migrates v1 policies that haven't been deleted. There used to be shipped-with-A2 v1 policies that the user could delete. Not all "default policies" were non-deletable.

blakestier · 2019-06-17T15:34:16Z

components/authz-service/README.md

@@ -615,6 +615,18 @@ For others, it will be necessary to add and remove policies
 to lock the system down so that known users are authorized to access
 specific resources.

+#### Adding New Default Policies


👏 👏 👏

blakestier · 2019-06-17T15:39:08Z

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

+		return "", errors.New("there was no resource passed")
+	}
+
+	if len(terms) == 1 && terms[0] == "*" {


blakestier · 2019-06-17T15:54:09Z

integration/tests/iam_v2.sh

    token=$(chef-automate admin-token)
    hab_curl --fail -s -k -H "api-token: $token" \
      -d "$(jo subjects="$(jo -a user:local:alice team:local:ops)" action=read resource=auth:users)" \
      https://localhost/api/v0/auth/policies

    log_info "run chef-automate iam upgrade-to-v2"
-    chef-automate iam upgrade-to-v2 || return 1
+    if ! output=$(chef-automate iam upgrade-to-v2); then


~~Will this also catch chef-automate iam upgrade-to-v2 --beta2.1? (since we could possibly go from v1 -> v2.1, we'd need that)~~ oops wrong script 😳. Do we need it in the v2p1.sh?

Don't think so -- it's the same migration code.

blakestier

I was running out of affirming emojis. Thanks so much for this! 🥇

tylercloke

Is this like known unknowns? xD Nice cleanup but one bit of update that should happen for the readme instructions.

tylercloke · 2019-06-17T19:37:54Z

components/authz-service/README.md

+
+1. A new IAM v1 policy: these are defined in [migrations](storage/postgres/migration/sql/),
+   and the [v1 constants package](constants/v1/constants.go#L3-L4).
+2. A new IAM v2 policy: they are defined in [server/v2/system](server/v2/system.go#L31).


We should have them use the datamigrations for new V2 policies, otherwise they won't be applied to existing V2 installations.

msorens · 2019-06-17T18:37:54Z

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

 	if terms[1] == "stats" {
-		return combineTermsIntoResourceSlice([]string{"infra", "nodes"}), nil
+		return "infra:nodes", nil


Using combineTermsIntoResource here still has value--that method is then the single place that knows there are colons involved. (And similarly on "event:events" below.)

Yeah, but I've opted for keeping this simple. No need to overthink it, the colons aren't going to change in IAM v2. 😉

msorens · 2019-06-17T20:38:07Z

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

@@ -2279,6 +2294,10 @@ func TestV1PolicyMigration(t *testing.T) {
 			wellknown(t, constants_v1.CSNginxComplianceDataCollectorPolicyID),
 			checks(isSkipped()),
 		},
+		"v1 default application policy": {


I raised the question to Kyleen, to whom this actually matters 😁, and she has confirmed it is not the same. What they want is "all users to have default access to all [application] things". Right now anyone with a viewer role or on the viewers team has access, but all users are not viewers by default. So I think we need a migration for this policy. See https://chefio.slack.com/archives/CFCE1NQ9W/p1560797578017700

…icy" This will resolve the following, irritating error: $ chef-automate iam upgrade-to-v2 --result-json output.json Upgrading to IAM v2 Migrating v1 policies... Skipped policies: convert v1 policy "aee14d59-da0b-4974-ba6d-1a018b024874" unknown "well-known" policy ...by skipping this new default v1 policy in migration. Skipping is OK because the application access rights are implemented as additions to the viewer and editor roles in IAM v2 (v2.1). Signed-off-by: Stephan Renatus <[email protected]>

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

…lications (#612) * authz-service/migration: skip well-known "ApplicationServiceGroupsPolicy" This will resolve the following, irritating error: $ chef-automate iam upgrade-to-v2 --result-json output.json Upgrading to IAM v2 Migrating v1 policies... Skipped policies: convert v1 policy "aee14d59-da0b-4974-ba6d-1a018b024874" unknown "well-known" policy ...by skipping this new default v1 policy in migration. Skipping is OK because the application access rights are implemented as additions to the viewer and editor roles in IAM v2 (v2.1). * [integration] iam v2: assert that no policy is skipped * authz-service/migration: remove comment -- it's only a warning * authz-service/migration: migrate custom v1 policies for app tab * authz-service/migration: bikeshed migration mapping function types * authz-service: add docs re: adding new default policies Signed-off-by: Stephan Renatus <[email protected]>

srenatus added bug 🐛 Something isn't working automate-auth iamv2 This issue or pull request applies to iamv2 work for Automate labels Jun 17, 2019

srenatus self-assigned this Jun 17, 2019

srenatus force-pushed the sr/authz-service/v1-to-v2-policy-migration-issues-with-applications branch 2 times, most recently from 35a0d7b to 90bf1e5 Compare June 17, 2019 13:45

srenatus marked this pull request as ready for review June 17, 2019 14:15

srenatus changed the title ~~authz-service: fix v1 to v2 policy migration issues with applications~~ A2-882: authz-service: fix v1 to v2 policy migration issues with applications Jun 17, 2019

srenatus changed the title ~~A2-882: authz-service: fix v1 to v2 policy migration issues with applications~~ [A2-882] authz-service: fix v1 to v2 policy migration issues with applications Jun 17, 2019

srenatus commented Jun 17, 2019

View reviewed changes

blakestier reviewed Jun 17, 2019

View reviewed changes

blakestier approved these changes Jun 17, 2019

View reviewed changes

tylercloke approved these changes Jun 17, 2019

View reviewed changes

msorens reviewed Jun 17, 2019

View reviewed changes

srenatus added 5 commits June 18, 2019 12:00

[integration] iam v2: assert that no policy is skipped

94bfe26

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

authz-service/migration: remove comment -- it's only a warning

b0a784c

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

authz-service/migration: migrate custom v1 policies for app tab

e94be67

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

authz-service/migration: bikeshed migration mapping function types

c9ab975

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

srenatus force-pushed the sr/authz-service/v1-to-v2-policy-migration-issues-with-applications branch from 0ed4408 to 4c3bb9b Compare June 18, 2019 10:02

authz-service: add docs re: adding new default policies

d7a596c

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

srenatus force-pushed the sr/authz-service/v1-to-v2-policy-migration-issues-with-applications branch from 4c3bb9b to d7a596c Compare June 18, 2019 10:08

msorens approved these changes Jun 18, 2019

View reviewed changes

srenatus merged commit db4c12a into master Jun 18, 2019

chef-ci deleted the sr/authz-service/v1-to-v2-policy-migration-issues-with-applications branch June 18, 2019 15:13

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-882] authz-service: fix v1 to v2 policy migration issues with applications #612

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

srenatus commented Jun 17, 2019 •

edited

Loading

srenatus left a comment

srenatus Jun 17, 2019

srenatus Jun 17, 2019

srenatus Jun 17, 2019

blakestier Jun 17, 2019

srenatus Jun 17, 2019

blakestier Jun 17, 2019

msorens Jun 17, 2019

srenatus Jun 18, 2019

srenatus Jun 17, 2019

blakestier Jun 17, 2019

srenatus Jun 17, 2019

blakestier Jun 17, 2019

blakestier Jun 17, 2019

blakestier Jun 17, 2019 •

edited

Loading

srenatus Jun 17, 2019

blakestier left a comment

tylercloke left a comment

tylercloke Jun 17, 2019

msorens Jun 17, 2019

srenatus Jun 18, 2019

msorens Jun 17, 2019

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

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

Conversation

srenatus commented Jun 17, 2019 • edited Loading

🔩 Description

👍 Definition of Done

👟 Demo Script / Repro Steps

⛓️ Related Resources

✅ Checklist

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

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

blakestier Jun 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blakestier left a comment

Choose a reason for hiding this comment

tylercloke 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

srenatus commented Jun 17, 2019 •

edited

Loading

blakestier Jun 17, 2019 •

edited

Loading