provider/aws: Reorganize and generalize AWS IAM policy normalization

Earlier work in #6956 caused the IAM policy documents generated by the
aws_iam_policy_document data source to follow the normalization
conventions used by most AWS services.

However, use of this data source is optional and so hand-authored IAM
policy documents used with other resources can still suffer from
normalization issues.

By reorganizing the code a little we can make re-usable normalization and
validation functions, which we will be able to use across many different
resource implementations, pending changes in subsequent commits.

This is a continuation of initial work done by David Tolnay in issue
#7785.

This will cause some minor changes to the result of the
aws_iam_policy_document data source: string sets are now sorted in
forward lexographic order rather than reverse, and "Statements" and
"Sid" will now be omitted when empty, for consistency with all of
the other attributes.

builtin/providers/aws/data_source_aws_iam_policy_document.go

@@ -112,12 +112,12 @@ func dataSourceAwsIamPolicyDocumentRead(d *schema.ResourceData, meta interface{}
 		}
 
 		if resources := cfgStmt["resources"].(*schema.Set).List(); len(resources) > 0 {
-			stmt.Resources = dataSourceAwsIamPolicyDocumentReplaceVarsInList(
+			stmt.Resources = dataSourceAwsIamPolicyDocumentReplaceVarsInSet(
 				iamPolicyDecodeConfigStringList(resources),
 			)
 		}
 		if resources := cfgStmt["not_resources"].(*schema.Set).List(); len(resources) > 0 {
-			stmt.NotResources = dataSourceAwsIamPolicyDocumentReplaceVarsInList(
+			stmt.NotResources = dataSourceAwsIamPolicyDocumentReplaceVarsInSet(
 				iamPolicyDecodeConfigStringList(resources),
 			)
 		}
@@ -150,52 +150,45 @@ func dataSourceAwsIamPolicyDocumentRead(d *schema.ResourceData, meta interface{}
 	return nil
 }
 
-func dataSourceAwsIamPolicyDocumentReplaceVarsInList(in interface{}) interface{} {
-	switch v := in.(type) {
-	case string:
-		return dataSourceAwsIamPolicyDocumentVarReplacer.Replace(v)
-	case []string:
-		out := make([]string, len(v))
-		for i, item := range v {
-			out[i] = dataSourceAwsIamPolicyDocumentVarReplacer.Replace(item)
-		}
-		return out
-	default:
-		panic("dataSourceAwsIamPolicyDocumentReplaceVarsInList: input not string nor []string")
+func dataSourceAwsIamPolicyDocumentReplaceVarsInSet(in IAMPolicyStringSet) IAMPolicyStringSet {
+	out := make(IAMPolicyStringSet, len(in))
+	for i, item := range in {
+		out[i] = dataSourceAwsIamPolicyDocumentVarReplacer.Replace(item)
 	}
+	return out
 }
 
 func dataSourceAwsIamPolicyDocumentMakeConditions(in []interface{}) IAMPolicyStatementConditionSet {
-	out := make([]IAMPolicyStatementCondition, len(in))
+	out := make(IAMPolicyStatementConditionSet, len(in))
 	for i, itemI := range in {
 		item := itemI.(map[string]interface{})
 		out[i] = IAMPolicyStatementCondition{
 			Test:     item["test"].(string),
 			Variable: item["variable"].(string),
-			Values: dataSourceAwsIamPolicyDocumentReplaceVarsInList(
+			Values: dataSourceAwsIamPolicyDocumentReplaceVarsInSet(
 				iamPolicyDecodeConfigStringList(
 					item["values"].(*schema.Set).List(),
 				),
 			),
 		}
 	}
-	return IAMPolicyStatementConditionSet(out)
+	return out
 }
 
 func dataSourceAwsIamPolicyDocumentMakePrincipals(in []interface{}) IAMPolicyStatementPrincipalSet {
-	out := make([]IAMPolicyStatementPrincipal, len(in))
+	out := make(IAMPolicyStatementPrincipalSet, len(in))
 	for i, itemI := range in {
 		item := itemI.(map[string]interface{})
 		out[i] = IAMPolicyStatementPrincipal{
 			Type: item["type"].(string),
-			Identifiers: dataSourceAwsIamPolicyDocumentReplaceVarsInList(
+			Identifiers: dataSourceAwsIamPolicyDocumentReplaceVarsInSet(
 				iamPolicyDecodeConfigStringList(
 					item["identifiers"].(*schema.Set).List(),
 				),
 			),
 		}
 	}
-	return IAMPolicyStatementPrincipalSet(out)
+	return out
 }
 
 func dataSourceAwsIamPolicyPrincipalSchema() *schema.Schema {

builtin/providers/aws/data_source_aws_iam_policy_document_test.go

@@ -117,13 +117,12 @@ var testAccAWSIAMPolicyDocumentExpectedJSON = `{
       "Sid": "1",
       "Effect": "Allow",
       "Action": [
-        "s3:ListAllMyBuckets",
-        "s3:GetBucketLocation"
+        "s3:GetBucketLocation",
+        "s3:ListAllMyBuckets"
       ],
       "Resource": "arn:aws:s3:::*"
     },
     {
-      "Sid": "",
       "Effect": "Allow",
       "Action": "s3:ListBucket",
       "Resource": "arn:aws:s3:::foo",
@@ -133,26 +132,24 @@ var testAccAWSIAMPolicyDocumentExpectedJSON = `{
       "Condition": {
         "StringLike": {
           "s3:prefix": [
-            "home/${aws:username}/",
-            "home/"
+            "home/",
+            "home/${aws:username}/"
           ]
         }
       }
     },
     {
-      "Sid": "",
       "Effect": "Allow",
       "Action": "s3:*",
       "Resource": [
-        "arn:aws:s3:::foo/home/${aws:username}/*",
-        "arn:aws:s3:::foo/home/${aws:username}"
+        "arn:aws:s3:::foo/home/${aws:username}",
+        "arn:aws:s3:::foo/home/${aws:username}/*"
       ],
       "Principal": {
         "AWS": "arn:blahblah:example"
       }
     },
     {
-      "Sid": "",
       "Effect": "Deny",
       "NotAction": "s3:*",
       "NotResource": "arn:aws:s3:::*"

builtin/providers/aws/iam_policy_model.go

@@ -2,94 +2,224 @@ package aws
 
 import (
 	"encoding/json"
+	"fmt"
 	"sort"
 )
 
+// IAMPolicyDoc is an in-memory representation of an IAM policy document
+// with annotations to marshal to and unmarshal from JSON policy syntax.
+//
+// Round-tripping through this struct will normalize a policy, but it
+// is only guaranteed to work with policy versions 2012-10-17 or earlier.
+// Newer policies may be silently corrupted by round-tripping through
+// this structure. (At the time of writing there is no newer policy version.)
 type IAMPolicyDoc struct {
 	Version    string                `json:",omitempty"`
 	Id         string                `json:",omitempty"`
-	Statements []*IAMPolicyStatement `json:"Statement"`
+	Statements []*IAMPolicyStatement `json:"Statement,omitempty"`
 }
 
 type IAMPolicyStatement struct {
-	Sid           string
+	Sid           string                         `json:",omitempty"`
 	Effect        string                         `json:",omitempty"`
-	Actions       interface{}                    `json:"Action,omitempty"`
-	NotActions    interface{}                    `json:"NotAction,omitempty"`
-	Resources     interface{}                    `json:"Resource,omitempty"`
-	NotResources  interface{}                    `json:"NotResource,omitempty"`
+	Actions       IAMPolicyStringSet             `json:"Action,omitempty"`
+	NotActions    IAMPolicyStringSet             `json:"NotAction,omitempty"`
+	Resources     IAMPolicyStringSet             `json:"Resource,omitempty"`
+	NotResources  IAMPolicyStringSet             `json:"NotResource,omitempty"`
 	Principals    IAMPolicyStatementPrincipalSet `json:"Principal,omitempty"`
 	NotPrincipals IAMPolicyStatementPrincipalSet `json:"NotPrincipal,omitempty"`
 	Conditions    IAMPolicyStatementConditionSet `json:"Condition,omitempty"`
 }
 
 type IAMPolicyStatementPrincipal struct {
 	Type        string
-	Identifiers interface{}
+	Identifiers IAMPolicyStringSet
 }
 
 type IAMPolicyStatementCondition struct {
 	Test     string
 	Variable string
-	Values   interface{}
+	Values   IAMPolicyStringSet
 }
 
+// IAMPolicyStringSet is a specialization of []string that has special normalization
+// rules for unmarshalling from JSON. Specifically, a single-item list is considered
+// equivalent to a plain string, and a multi-item list is sorted into lexographical
+// order to reflect that the ordering is not meaningful.
+//
+// When marshalling, the set is again ordered lexographically (in case it has been
+// modified by code that didn't preserve the order) and single-item lists are serialized
+// as primitive strings, since IAM considers this to be the normalized form.
+type IAMPolicyStringSet []string
+
 type IAMPolicyStatementPrincipalSet []IAMPolicyStatementPrincipal
 type IAMPolicyStatementConditionSet []IAMPolicyStatementCondition
 
+func (ss *IAMPolicyStringSet) UnmarshalJSON(data []byte) error {
+	var single string
+	err := json.Unmarshal(data, &single)
+	if err == nil {
+		*ss = IAMPolicyStringSet{single}
+		return nil
+	}
+
+	var set []string
+	err = json.Unmarshal(data, &set)
+	if err == nil {
+		*ss = IAMPolicyStringSet(set)
+		sort.Strings(*ss)
+		return nil
+	}
+
+	return fmt.Errorf("must be string or array of strings")
+}
+
+func (ss IAMPolicyStringSet) MarshalJSON() ([]byte, error) {
+	if len(ss) == 1 {
+		return json.Marshal(ss[0])
+	}
+
+	sort.Strings([]string(ss))
+	return json.Marshal([]string(ss))
+}
+
 func (ps IAMPolicyStatementPrincipalSet) MarshalJSON() ([]byte, error) {
-	raw := map[string]interface{}{}
+	raw := map[string]IAMPolicyStringSet{}
 
 	for _, p := range ps {
-		switch i := p.Identifiers.(type) {
-		case []string:
-			if _, ok := raw[p.Type]; !ok {
-				raw[p.Type] = make([]string, 0, len(i))
-			}
-			sort.Sort(sort.Reverse(sort.StringSlice(i)))
-			raw[p.Type] = append(raw[p.Type].([]string), i...)
-		case string:
-			raw[p.Type] = i
-		default:
-			panic("Unsupported data type for IAMPolicyStatementPrincipalSet")
+		if _, ok := raw[p.Type]; !ok {
+			raw[p.Type] = make(IAMPolicyStringSet, 0, len(p.Identifiers))
 		}
+		raw[p.Type] = append(raw[p.Type], p.Identifiers...)
 	}
 
-	return json.Marshal(&raw)
+	return json.Marshal(raw)
+}
+
+func (ps *IAMPolicyStatementPrincipalSet) UnmarshalJSON(data []byte) error {
+	var raw map[string]IAMPolicyStringSet
+	err := json.Unmarshal(data, &raw)
+	if err != nil {
+		return err
+	}
+
+	principalTypes := make([]string, 0, len(raw))
+	for k := range raw {
+		principalTypes = append(principalTypes, k)
+	}
+	sort.Strings(principalTypes)
+
+	*ps = make(IAMPolicyStatementPrincipalSet, 0, len(raw))
+	for _, principalType := range principalTypes {
+		*ps = append(*ps, IAMPolicyStatementPrincipal{
+			Type:        principalType,
+			Identifiers: raw[principalType],
+		})
+	}
+	return nil
 }
 
 func (cs IAMPolicyStatementConditionSet) MarshalJSON() ([]byte, error) {
-	raw := map[string]map[string]interface{}{}
+	raw := map[string]map[string]IAMPolicyStringSet{}
 
 	for _, c := range cs {
 		if _, ok := raw[c.Test]; !ok {
-			raw[c.Test] = map[string]interface{}{}
+			raw[c.Test] = map[string]IAMPolicyStringSet{}
 		}
-		switch i := c.Values.(type) {
-		case []string:
-			if _, ok := raw[c.Test][c.Variable]; !ok {
-				raw[c.Test][c.Variable] = make([]string, 0, len(i))
-			}
-			sort.Sort(sort.Reverse(sort.StringSlice(i)))
-			raw[c.Test][c.Variable] = append(raw[c.Test][c.Variable].([]string), i...)
-		case string:
-			raw[c.Test][c.Variable] = i
-		default:
-			panic("Unsupported data type for IAMPolicyStatementConditionSet")
+		if _, ok := raw[c.Test][c.Variable]; !ok {
+			raw[c.Test][c.Variable] = make(IAMPolicyStringSet, 0, len(c.Values))
 		}
+		raw[c.Test][c.Variable] = append(raw[c.Test][c.Variable], c.Values...)
 	}
 
 	return json.Marshal(&raw)
 }
 
-func iamPolicyDecodeConfigStringList(lI []interface{}) interface{} {
-	if len(lI) == 1 {
-		return lI[0].(string)
+func (cs *IAMPolicyStatementConditionSet) UnmarshalJSON(data []byte) error {
+	var raw map[string]map[string]IAMPolicyStringSet
+	err := json.Unmarshal(data, &raw)
+	if err != nil {
+		return err
+	}
+
+	tests := make([]string, 0, len(raw))
+	count := 0
+	for k, v := range raw {
+		tests = append(tests, k)
+		count += len(v)
 	}
-	ret := make([]string, len(lI))
-	for i, vI := range lI {
-		ret[i] = vI.(string)
+	sort.Strings(tests)
+
+	*cs = make(IAMPolicyStatementConditionSet, 0, count)
+	for _, test := range tests {
+		variables := make([]string, 0, len(raw[test]))
+		for k := range raw[test] {
+			variables = append(variables, k)
+		}
+		sort.Strings(variables)
+
+		for _, variable := range variables {
+			*cs = append(*cs, IAMPolicyStatementCondition{
+				Test:     test,
+				Variable: variable,
+				Values:   raw[test][variable],
+			})
+		}
+	}
+	return nil
+}
+
+// NormalizeIAMPolicyJSON takes an IAM policy in JSON format and produces
+// an equivalent JSON document with normalizations applied. In particular,
+// single-element string lists are serialized as standalone strings,
+// multi-element string lists are sorted lexographically, and the
+// policy element attributes are written in a predictable order.
+//
+// In the event of an error, the result is the input buffer, verbatim.
+func NormalizeIAMPolicyJSON(in []byte) ([]byte, error) {
+	doc := &IAMPolicyDoc{}
+	err := json.Unmarshal([]byte(in), doc)
+	if err != nil {
+		return in, err
+	}
+
+	// The Unmarshal/Marshal process provides sufficient normalization
+	// for our purposes.
+
+	resultBytes, err := json.Marshal(doc)
+	if err != nil {
+		return in, err
+	}
+
+	return resultBytes, nil
+}
+
+// iamPolicyJSONStateFunc can be used as a StateFunc for attributes that
+// take IAM policies in JSON format.
+// Should usually be used in conjunction with iamPolicyJSONValidateFunc.
+func iamPolicyJSONStateFunc(jsonSrcI interface{}) string {
+	// Safe to ignore the error because NormalizeIAMPolicyJSON will pass through
+	// the given string verbatim if it's not valid.
+	result, _ := NormalizeIAMPolicyJSON([]byte(jsonSrcI.(string)))
+	return string(result)
+}
+
+// Can be used as a ValidateFunc for attributes that take IAM policies in JSON format.
+// Does simple syntactic validation.
+func iamPolicyJSONValidateFunc(jsonSrcI interface{}, _ string) ([]string, []error) {
+	_, err := NormalizeIAMPolicyJSON([]byte(jsonSrcI.(string)))
+	if err != nil {
+		return nil, []error{err}
+	}
+
+	return nil, nil
+}
+
+func iamPolicyDecodeConfigStringList(configList []interface{}) IAMPolicyStringSet {
+	ret := make([]string, len(configList))
+	for i, valueI := range configList {
+		ret[i] = valueI.(string)
 	}
-	sort.Sort(sort.Reverse(sort.StringSlice(ret)))
+	sort.Strings(ret)
 	return ret
 }