[A2-882] authz-service: fix v1 to v2 policy migration issues with app…

…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]>
chef · Jun 20, 2019 · 02b91e3 · 02b91e3
1 parent 53e9f1e
commit 02b91e3
Show file tree

Hide file tree

Showing 4 changed files with 93 additions and 50 deletions.
diff --git a/components/authz-service/README.md b/components/authz-service/README.md
@@ -615,6 +615,20 @@ 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
+
+To add a new default policy, the following is needed:
+
+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 (system) policy: they are defined in [server/v2/system](server/v2/system.go#L31).
+3. A new IAM v2 default policy (which can be deleted by users), or any additions
+   to default roles, are done in [datamigrations](storage/postgres/datamigration/sql/).
+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 haven't been deleted). The procedure for
+   converting legacy policies is defined in [server/v2/migration](server/v2/migration.go).
+
 ### Policy API
 
 Documentation of the complete Policy API are available [here](TBD).

diff --git a/components/authz-service/server/v2/migration.go b/components/authz-service/server/v2/migration.go
@@ -126,6 +126,7 @@ var v1PoliciesToSkip = map[string]struct{}{
 	constants_v1.OcErchefIngestEventsPolicyID:           {},
 	constants_v1.CSNginxComplianceProfilesPolicyID:      {},
 	constants_v1.CSNginxComplianceDataCollectorPolicyID: {},
+	constants_v1.ApplicationsServiceGroupsPolicyID:      {},
 }
 
 var v1CfgmgmtPolicies = map[string]struct{}{
@@ -389,7 +390,7 @@ func customPolicyFromV1(pol *storage_v1.Policy) (*storage.Policy, error) {
 	}
 
 	// Note: v1 only had (custom) allow policies
-	statement, err := storage.NewStatement(storage.Allow, "", []string{}, resource, action)
+	statement, err := storage.NewStatement(storage.Allow, "", []string{}, []string{resource}, action)
 	if err != nil {
 		return nil, errors.Wrap(err, "format v2 statement")
 	}
@@ -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) {
+func convertV1Resource(resource string) (string, error) {
 	terms := strings.Split(resource, ":")
 	if len(terms) == 0 {
-		return nil, errors.New("there was no resource passed")
+		return "", errors.New("there was no resource passed")
+	}
+
+	if len(terms) == 1 && terms[0] == "*" {
+		return "*", nil
 	}
 
 	switch terms[0] {
 	case "service_info":
 		switch terms[1] {
 		case "version":
-			return combineTermsIntoResourceSlice([]string{"system", "service", "version"}), nil
+			return "system:service:version", nil
 		case "health":
-			return combineTermsIntoResourceSlice([]string{"system", "health"}), nil
+			return "system:health", nil
 		}
-		return combineTermsIntoResourceSlice([]string{"system", "*"}), nil
+		return "system:*", nil
 	case "auth":
 		terms = changeTerm(terms, "auth", "iam")
 		terms = changeTerm(terms, "api_tokens", "tokens")
-		return combineTermsIntoResourceSlice(terms), nil
+		return combineTermsIntoResource(terms...), nil
 	case "users":
 		// "users" -> "iam:usersSelf"
 		terms[0] = "usersSelf"
-		terms = append([]string{"iam"}, terms...)
-		return combineTermsIntoResourceSlice(terms), nil
+		return combineTermsIntoResource(prefixTerms("iam", terms)...), nil
 	case "auth_introspection":
 		// Special case
 		if terms[1] == "*" {
-			return combineTermsIntoResourceSlice([]string{"iam", "introspect"}), nil
+			return "iam:introspect", nil
 		}
 		terms = changeTerm(terms, "auth_introspection", "iam")
 		terms = changeTerm(terms, "introspect_all", "introspect")
 		terms = changeTerm(terms, "introspect_some", "introspect")
-		return combineTermsIntoResourceSlice(terms), nil
+		return combineTermsIntoResource(terms...), nil
 	case "cfgmgmt":
 		return convertV1Cfgmgmt(terms)
 	case "compliance":
 		// Special case
 		if resource == "compliance:profiles:market" {
-			return combineTermsIntoResourceSlice([]string{"compliance", "marketProfiles"}), nil
+			return "compliance:marketProfiles", nil
 		}
-		terms = deleteTerm(terms, "storage")
-		return combineTermsIntoResourceSlice(terms), nil
+		return combineTermsIntoResource(deleteTerm(terms, "storage")...), nil
 	case "events":
 		return convertV1Events(terms)
 	case "ingest":
 		// Special case: "ingest:status" -> "infra:ingest:status" (no wildcards to worry about)
 		if terms[1] == "status" {
-			terms := []string{"infra", "ingest", "status"}
-			return combineTermsIntoResourceSlice(terms), nil
+			return "infra:ingest:status", nil
 		}
 
 		// Special case: "ingest:unified_events" -> "infra:unifiedEvents" (no wildcards to worry about)
 		if terms[1] == "unified_events" {
-			terms := []string{"infra", "unifiedEvents"}
-			return combineTermsIntoResourceSlice(terms), nil
+			return "infra:unifiedEvents", nil
 		}
 
 		terms = changeTerm(terms, "ingest", "infra")
 		terms = changeTerm(terms, "unified_events", "unifiedEvents")
-		return combineTermsIntoResourceSlice(terms), nil
+		return combineTermsIntoResource(terms...), nil
 	case "license":
 		if len(terms) == 1 {
-			return []string{"system:license"}, nil
+			return "system:license", nil
 		}
 		// if len(terms) == 2 aka license:* or license:status
-		return []string{"system:status"}, nil
+		return "system:status", nil
 	case "nodemanagers":
 		// "nodemanagers" -> "infra:nodeManagers"
-		terms[0] = "nodeManagers"
-		terms = append([]string{"infra"}, terms...)
-		return combineTermsIntoResourceSlice(terms), nil
+		return combineTermsIntoResource(prefixTerms("infra", changeTerm(terms, "nodemanagers", "nodeManagers"))...),
+			nil
 	case "nodes":
 		// "nodes" -> "infra:nodes"
-		terms = append([]string{"infra"}, terms...)
-		return combineTermsIntoResourceSlice(terms), nil
+		return combineTermsIntoResource(prefixTerms("infra", terms)...), nil
 	case "secrets":
 		// "secrets" -> "secrets:secrets"
-		terms = append([]string{"secrets"}, terms...)
-		return combineTermsIntoResourceSlice(terms), nil
+		return combineTermsIntoResource(prefixTerms("secrets", terms)...), nil
 	case "telemetry":
 		// either telemetry:config or telemetry:* maps to system:config
-		return combineTermsIntoResourceSlice([]string{"system", "config"}), nil
+		return "system:config", nil
 	case "notifications":
-		return combineTermsIntoResourceSlice(terms), nil
-	case "*":
-		return []string{"*"}, nil
+		return resource, nil // unchanged
+	case "service_groups":
+		return "applications:serviceGroups", nil
 	default:
-		// TODO: should we just warn the logs in this case and not crash the whole migration on the off
-		// chance that the user created a policy where they mistyped a resource name?
-		return nil, fmt.Errorf("did not recognize base v1 resource term: %s", terms[0])
+		return "", fmt.Errorf("did not recognize base v1 resource term: %s", terms[0])
 	}
 }
 
-func convertV1Cfgmgmt(terms []string) ([]string, error) {
+func convertV1Cfgmgmt(terms []string) (string, error) {
 	if terms[1] == "stats" {
-		return combineTermsIntoResourceSlice([]string{"infra", "nodes"}), nil
+		return "infra:nodes", nil
 	}
 	// cfgmgmt:nodes:{node_id}:runs -> infra:nodes:{node_id}
 	// cfgmgmt:nodes:{node_id}:runs:{run_id}" -> infra:nodes:{node_id}
 	if len(terms) >= 4 && terms[3] == "runs" {
-		return combineTermsIntoResourceSlice([]string{"infra", "nodes", terms[2]}), nil
+		return combineTermsIntoResource("infra", "nodes", terms[2]), nil
 	}
 	// cfgmgmt:nodes:{node_id}:attribute -> infra:nodes:{node_id}
 	if len(terms) >= 4 && terms[3] == "attribute" {
-		return combineTermsIntoResourceSlice([]string{"infra", "nodes", terms[2]}), nil
+		return combineTermsIntoResource("infra", "nodes", terms[2]), nil
 	}
 	// cfgmgmt:nodes:{node_id}:* -> infra:nodes:{node_id}
 	if len(terms) >= 3 && terms[2] == "node_id" {
-		return combineTermsIntoResourceSlice([]string{"infra", "nodes", terms[2]}), nil
+		return combineTermsIntoResource("infra", "nodes", terms[2]), nil
 	}
 	terms = changeTerm(terms, "cfgmgmt", "infra")
 	terms = changeTerm(terms, "marked-nodes", "markedNodes")
-	return combineTermsIntoResourceSlice(terms), nil
+	return combineTermsIntoResource(terms...), nil
 }
 
-func convertV1Events(terms []string) ([]string, error) {
+func convertV1Events(terms []string) (string, error) {
 	// "events:*" -> "event:events"
 	if len(terms) == 1 {
-		return combineTermsIntoResourceSlice([]string{"event:events"}), nil
+		return "event:events", nil
 	}
 
 	switch terms[1] {
 	case "types", "tasks", "strings":
-		return combineTermsIntoResourceSlice([]string{"event:events"}), nil
+		return "event:events", nil
 	default:
-		terms = changeTerm(terms, "event", "events")
-		return combineTermsIntoResourceSlice(terms), nil
+		return combineTermsIntoResource(changeTerm(terms, "event", "events")...), nil
 	}
 }
 
@@ -558,6 +553,10 @@ func changeTerm(terms []string, original, updated string) []string {
 	return terms
 }
 
+func prefixTerms(prefix string, terms []string) []string {
+	return append([]string{prefix}, terms...)
+}
+
 func deleteTerm(terms []string, original string) []string {
 	for i, term := range terms {
 		if term == original {
@@ -567,8 +566,8 @@ func deleteTerm(terms []string, original string) []string {
 	return terms
 }
 
-func combineTermsIntoResourceSlice(terms []string) []string {
-	return []string{strings.Join(terms, ":")}
+func combineTermsIntoResource(terms ...string) string {
+	return strings.Join(terms, ":")
 }
 
 func convertV1Action(action string, resource string) ([]string, error) {

diff --git a/components/authz-service/server/v2/migration_internal_test.go b/components/authz-service/server/v2/migration_internal_test.go
@@ -2239,6 +2239,21 @@ func TestV1PolicyMigration(t *testing.T) {
 				}),
 			),
 		},
+		"service_groups,list": {
+			storage_v1.Policy{
+				ID:       polID,
+				Subjects: []string{"user:local:albertine", "team:local:admins"},
+				Action:   "list",
+				Resource: "service_groups",
+			},
+			checks(
+				hasStatements(storage_v2.Statement{
+					Effect:    storage_v2.Allow,
+					Resources: []string{"applications:serviceGroups"},
+					Actions:   []string{"*:list"},
+				}),
+			),
+		},
 		"v1 default compliance token upload profile policy": {
 			wellknown(t, constants_v1.ComplianceTokenUploadProfilesPolicyID),
 			checks(hasID(constants_v2.ComplianceTokenPolicyID)),
@@ -2279,6 +2294,10 @@ func TestV1PolicyMigration(t *testing.T) {
 			wellknown(t, constants_v1.CSNginxComplianceDataCollectorPolicyID),
 			checks(isSkipped()),
 		},
+		"v1 default application policy": {
+			wellknown(t, constants_v1.ApplicationsServiceGroupsPolicyID),
+			checks(isSkipped()),
+		},
 	}
 
 	for desc, test := range cases {

diff --git a/integration/tests/iam_v2.sh b/integration/tests/iam_v2.sh
@@ -32,14 +32,25 @@ hab_curl() {
 
 do_test_deploy() {
     log_info "Adding v1 policy (that should be migrated)"
-    local token
+    local token output
     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
+        log_error "Non-zero exit code, output:"
+        log_error "$output"
+        return 1
+    else
+        # in CI, we don't want any policies to be skipped
+        if grep -q "Skipped policies" <<< "$output"; then
+            log_error "Expected no skipped policies, output:"
+            log_error "$output"
+            return 1
+        fi
+    fi
 
     # ensure service startup works with IAM v2:
     # - kill authz-service to force startup,