From ccf7dc508c8f8a08680d9ba3591f01ca8cad0e44 Mon Sep 17 00:00:00 2001 From: Chris Marchesi Date: Fri, 11 Dec 2015 16:21:44 -0800 Subject: [PATCH] IAM policy normalization to prevent false updates * normalizePolicyDocument function designed to be used as a StateFunc function that can be used to match IAM policy with what AWS sets. --- .../aws/resource_aws_elasticsearch_domain.go | 2 +- .../providers/aws/resource_aws_s3_bucket.go | 17 +- builtin/providers/aws/structure.go | 159 ++++++++++++++++ builtin/providers/aws/structure_test.go | 169 ++++++++++++++++++ 4 files changed, 331 insertions(+), 16 deletions(-) diff --git a/builtin/providers/aws/resource_aws_elasticsearch_domain.go b/builtin/providers/aws/resource_aws_elasticsearch_domain.go index 372b0e3bbb19..24be00e3d8b6 100644 --- a/builtin/providers/aws/resource_aws_elasticsearch_domain.go +++ b/builtin/providers/aws/resource_aws_elasticsearch_domain.go @@ -23,7 +23,7 @@ func resourceAwsElasticSearchDomain() *schema.Resource { Schema: map[string]*schema.Schema{ "access_policies": &schema.Schema{ Type: schema.TypeString, - StateFunc: normalizeJson, + StateFunc: normalizePolicyDocument, Optional: true, }, "advanced_options": &schema.Schema{ diff --git a/builtin/providers/aws/resource_aws_s3_bucket.go b/builtin/providers/aws/resource_aws_s3_bucket.go index d4e384b64ac6..6b1c540a8b57 100644 --- a/builtin/providers/aws/resource_aws_s3_bucket.go +++ b/builtin/providers/aws/resource_aws_s3_bucket.go @@ -46,7 +46,7 @@ func resourceAwsS3Bucket() *schema.Resource { "policy": &schema.Schema{ Type: schema.TypeString, Optional: true, - StateFunc: normalizeJson, + StateFunc: normalizePolicyDocument, }, "cors_rule": &schema.Schema{ @@ -436,7 +436,7 @@ func resourceAwsS3BucketRead(d *schema.ResourceData, meta interface{}) error { if err := d.Set("policy", ""); err != nil { return err } - } else if err := d.Set("policy", normalizeJson(*v)); err != nil { + } else if err := d.Set("policy", normalizePolicyDocument(*v)); err != nil { return err } } @@ -1267,19 +1267,6 @@ func removeNil(data map[string]interface{}) map[string]interface{} { return withoutNil } -func normalizeJson(jsonString interface{}) string { - if jsonString == nil || jsonString == "" { - return "" - } - var j interface{} - err := json.Unmarshal([]byte(jsonString.(string)), &j) - if err != nil { - return fmt.Sprintf("Error parsing JSON: %s", err) - } - b, _ := json.Marshal(j) - return string(b[:]) -} - func normalizeRegion(region string) string { // Default to us-east-1 if the bucket doesn't have a region: // http://docs.aws.amazon.com/AmazonS3/latest/API/RESTBucketGETlocation.html diff --git a/builtin/providers/aws/structure.go b/builtin/providers/aws/structure.go index 40b0ff4ea365..9d5ac5f7736f 100644 --- a/builtin/providers/aws/structure.go +++ b/builtin/providers/aws/structure.go @@ -4,6 +4,7 @@ import ( "bytes" "encoding/json" "fmt" + "log" "sort" "strconv" "strings" @@ -25,6 +26,7 @@ import ( "github.com/aws/aws-sdk-go/service/redshift" "github.com/aws/aws-sdk-go/service/route53" "github.com/hashicorp/terraform/helper/schema" + "github.com/mitchellh/mapstructure" ) // Takes the result of flatmap.Expand for an array of listeners and @@ -1274,3 +1276,160 @@ func flattenApiGatewayThrottleSettings(settings *apigateway.ThrottleSettings) [] return result } + +// normalizeJson is a JSON-in, JSON-out function that assures JSON strings +// supplied by the user appear the same as they would in AWS. +// +// Note that this function does not *sort* JSON, and as such may be insufficient +// to guard against unneeded changes in things such as IAM and resource policy +// documents. For such functionality, see normalizePolicyDocument. +func normalizeJson(jsonString interface{}) string { + if jsonString == nil { + return "" + } + j := make(map[string]interface{}) + err := json.Unmarshal([]byte(jsonString.(string)), &j) + if err != nil { + return fmt.Sprintf("Error parsing JSON: %s", err) + } + b, _ := json.Marshal(j) + return string(b[:]) +} + +// IAM policy document normalization logic. +// +// AWS will change policies that are submitted to conform to the following +// guidelines, even though other forms are not necessarily syntatically correct: +// +// * When a policy statement has no Sid specified, it will create a Sid entry +// with blank data. +// * When a policy statement only has one action, but is specified in an array, +// it will truncate the array and use a single element string action instead. +// (ie: "Action": [ "s3:ListBucket" ] becomes "Action": "s3:ListBucket") +// * Multiple actions will be sorted alphabetically. +// * The general policy document structure also follows an ordered structure +// that needs to be mimicked in order for the document to truly match. +// +// The objective of these functions is to accept a valid Policy JSON file, +// fix it to conform the following above guidelines, and then return the JSON. +// + +// struct for the top-level of the document. +type policyDocument struct { + Version string + Id string `json:",omitempty"` + Statement []policyDocumentStatement +} + +// struct for policy document statements. +type policyDocumentStatement struct { + Sid string + Effect string + Principal interface{} `json:",omitempty"` + NotPrincipal interface{} `json:",omitempty"` + Action interface{} `json:",omitempty"` + NotAction interface{} `json:",omitempty"` + Resource interface{} `json:",omitempty"` + NotResource interface{} `json:",omitempty"` + Condition map[string]map[string]interface{} `json:",omitempty"` +} + +type policyStatementPrincipal struct { + AWS interface{} `json:",omitempty"` + CanonicalUser interface{} `json:",omitempty"` + Federated interface{} `json:",omitempty"` + Service interface{} `json:",omitempty"` +} + +// policyDocArray allows us to define a slice of interfaces to sort, versus +// a slice of strings. +type policyDocArray []interface{} + +func (a policyDocArray) Len() int { return len(a) } +func (a policyDocArray) Swap(i, j int) { a[i], a[j] = a[j], a[i] } + +// policyDocArray.Less() is implemented to sort only on neighboring string values, +// so if the array contains non-string values, this will give unstable results. +func (a policyDocArray) Less(i, j int) bool { + switch ival := a[i].(type) { + case string: + if jval, ok := a[j].(string); ok { + log.Printf("[DEBUG] sort: %s < %s", ival, jval) + return ival < jval + } + } + return false +} + +// normalizePolicyDocument takes the JSON policy, normalizes the statements +// and sorts any arrays (namely actions). +func normalizePolicyDocument(jsonString interface{}) string { + if jsonString == nil { + return "" + } + var j policyDocument + err := json.Unmarshal([]byte(jsonString.(string)), &j) + if err != nil { + return fmt.Sprintf("Error parsing JSON: %s", err) + } + normalizePolicyStatements(j.Statement) + b, _ := json.Marshal(j) + return string(b[:]) +} + +// normalizePolicyStatements calls out to normalize the various fields in the +// policy document struct that could have arrays. This then calls out to +// normalizeStatementElements. +func normalizePolicyStatements(statements []policyDocumentStatement) { + for i, v := range statements { + statements[i].Action = normalizeStatementElements(v.Action) + statements[i].NotAction = normalizeStatementElements(v.NotAction) + statements[i].Resource = normalizeStatementElements(v.Resource) + statements[i].NotResource = normalizeStatementElements(v.NotResource) + // Principal and NotPrincipal + // more than likely if this fails, we are not dealing with a principal + // struct, possibly just a "*" in as the principal. we ignore errors + var principalStruct policyStatementPrincipal + var notPrincipalStruct policyStatementPrincipal + if err := mapstructure.Decode(statements[i].Principal, &principalStruct); err == nil && statements[i].Principal != nil { + principalStruct.AWS = normalizeStatementElements(principalStruct.AWS) + principalStruct.CanonicalUser = normalizeStatementElements(principalStruct.CanonicalUser) + principalStruct.Federated = normalizeStatementElements(principalStruct.Federated) + principalStruct.Service = normalizeStatementElements(principalStruct.Service) + statements[i].Principal = principalStruct + } + if err := mapstructure.Decode(statements[i].NotPrincipal, ¬PrincipalStruct); err == nil && statements[i].NotPrincipal != nil { + notPrincipalStruct.AWS = normalizeStatementElements(notPrincipalStruct.AWS) + notPrincipalStruct.CanonicalUser = normalizeStatementElements(notPrincipalStruct.CanonicalUser) + notPrincipalStruct.Federated = normalizeStatementElements(notPrincipalStruct.Federated) + notPrincipalStruct.Service = normalizeStatementElements(notPrincipalStruct.Service) + statements[i].NotPrincipal = notPrincipalStruct + } + // The Condition object - 2-level deep flattening + for ce := range statements[i].Condition { + for ck, cv := range statements[i].Condition[ce] { + statements[i].Condition[ce][ck] = normalizeStatementElements(cv) + } + } + } +} + +// normalizeStatementElements does one of two things: +// * If element is a single-element array/slice, it will "flatten" it and +// return the individual value. +// * If element is a multi-value array, the string values in the array are +// sorted. +func normalizeStatementElements(element interface{}) interface{} { + log.Printf("[DEBUG] checking to either truncate or sort policy doc element %v ", element) + switch element := element.(type) { + case []interface{}: + if len(element) < 2 { + log.Printf("[DEBUG] single element %s found in policy, returning single element", element[0]) + return element[0] + } + log.Printf("[DEBUG] multiple elements %v found in policy, sorting strings", element) + sort.Sort(policyDocArray(element)) + return element + } + return element +} diff --git a/builtin/providers/aws/structure_test.go b/builtin/providers/aws/structure_test.go index 80b3711aa59b..650ecd8db6b9 100644 --- a/builtin/providers/aws/structure_test.go +++ b/builtin/providers/aws/structure_test.go @@ -1,6 +1,9 @@ package aws import ( + "encoding/json" + "fmt" + "log" "reflect" "strings" "testing" @@ -16,6 +19,7 @@ import ( "github.com/aws/aws-sdk-go/service/route53" "github.com/hashicorp/terraform/flatmap" "github.com/hashicorp/terraform/helper/schema" + "github.com/mitchellh/mapstructure" ) // Returns test configuration @@ -942,3 +946,168 @@ func TestFlattenApiGatewayThrottleSettings(t *testing.T) { t.Fatalf("Expected 'rate_limit' to equal %f, got %f", expectedRateLimit, rateLimitFloat) } } + +// Returns a test policy document with the scenarios we wnat to test against +// included, ie: out-of-order keys, unsorted actions, etc. +const testPolicyDocument = ` +{ + "Statement": [ + { + "Action": "es:*", + "Condition": { + "IpAddress": { + "aws:SourceIp": [ + "192.168.0.10/32" + ] + } + }, + "Effect": "Allow", + "Principal": { + "AWS": "*" + }, + "Resource": [ + "arn:aws:es:us-east-1:123456789012:domain/test/*" + ], + "Sid": "ES" + }, + { + "Action": [ "s3:GetObject", "s3:PutObject", "s3:DeleteObject" ], + "Effect": "Allow", + "Resource": "arn:aws:s3:::test-bucket/*", + "Principal": { "AWS": [ + "arn:aws:iam::123456789012:role/TestRole" + ]} + }, + { + "Action": [ "s3:ListBucket" ], + "Effect": "Allow", + "Resource": "arn:aws:s3:::test-bucket", + "Principal": { "AWS": [ + "arn:aws:iam::987654321012:role/TestRole", + "arn:aws:iam::123456789012:role/TestRole" + ]} + }, + { + "Action": [ "s3:ListBucket" ], + "Effect": "Allow", + "Resource": "arn:aws:s3:::test-bucket", + "Principal": "*" + }, + { + "Action": [ "s3:ListBucket" ], + "Effect": "Allow", + "Resource": "arn:aws:s3:::test-bucket", + "NotPrincipal": { "AWS": [ + "arn:aws:iam::123456789012:role/TestRole" + ]} + } + ], + "Version": "2012-10-17" +} +` + +func testPolicyDocumentTop() policyDocument { + normalizedPolicyJSON := normalizePolicyDocument(testPolicyDocument) + var normalizedPolicyStruct policyDocument + log.Printf("[DEBUG] json doc after normalization: %s", normalizedPolicyJSON) + err := json.Unmarshal([]byte(normalizedPolicyJSON), &normalizedPolicyStruct) + if err != nil { + // Things are really broken + panic(fmt.Errorf("Error parsing normalized JSON document: %s", err)) + } + return normalizedPolicyStruct +} + +func testPolicyDocumentStatementIndex(i int) policyDocumentStatement { + return testPolicyDocumentTop().Statement[i] +} + +func TestNormalizePolicyDocumentTopLevel(t *testing.T) { + normalizedPolicyStruct := testPolicyDocumentTop() + if normalizedPolicyStruct.Version != "2012-10-17" { + t.Fatalf("Expected Version to be 2012-10-17, got %s", normalizedPolicyStruct.Version) + } + if normalizedPolicyStruct.Id != "" { + t.Fatalf("Expected Id to be blank, got %s", normalizedPolicyStruct.Id) + } +} + +func TestNormalizePolicyDocumentEmptySid(t *testing.T) { + normalizedPolicyStatementStruct := testPolicyDocumentStatementIndex(1) + if normalizedPolicyStatementStruct.Sid != "" { + t.Fatalf("Expected Sid to blank, got %s", normalizedPolicyStatementStruct.Sid) + } +} + +func TestNormalizePolicyDocumentFlattenActionArray(t *testing.T) { + normalizedPolicyStatementStruct := testPolicyDocumentStatementIndex(2) + switch action := normalizedPolicyStatementStruct.Action.(type) { + default: + t.Fatalf("Expected Action to be string, got %T", action) + case string: + } +} + +func TestNormalizePolicyDocumentFlattenConditionArray(t *testing.T) { + normalizedPolicyStatementStruct := testPolicyDocumentStatementIndex(0) + switch action := normalizedPolicyStatementStruct.Condition["IpAddress"]["aws:SourceIp"].(type) { + default: + t.Fatalf("Expected Condition[\"IpAddress\"][\"aws:SourceIp\"] to be string, got %T", action) + case string: + } +} + +func TestNormalizePolicyDocumentSortActions(t *testing.T) { + normalizedPolicyStatementStruct := testPolicyDocumentStatementIndex(1) + if normalizedPolicyStatementStruct.Action.([]interface{})[0].(string) != "s3:DeleteObject" { + t.Fatalf("Expected first action to be s3:DeleteObject, got %s", normalizedPolicyStatementStruct.Action) + } +} + +func TestNormalizePolicyDocumentWildcardPrincipal(t *testing.T) { + normalizedPolicyStatementStruct := testPolicyDocumentStatementIndex(3) + if normalizedPolicyStatementStruct.Principal != "*" { + t.Fatalf("Expected wildcard principal, got %v", normalizedPolicyStatementStruct.Principal) + } +} + +func TestNormalizePolicyDocumentFlattenedPrincipalAWS(t *testing.T) { + normalizedPolicyStatementStruct := testPolicyDocumentStatementIndex(1) + var principalStruct policyStatementPrincipal + _ = mapstructure.Decode(normalizedPolicyStatementStruct.Principal, &principalStruct) + if principalStruct.AWS != "arn:aws:iam::123456789012:role/TestRole" { + t.Fatalf("Expected non-array AWS principal, got %v", principalStruct.AWS) + } +} + +func TestNormalizePolicyDocumentMissingNotPrincipal(t *testing.T) { + normalizedPolicyStatementStruct := testPolicyDocumentStatementIndex(1) + if normalizedPolicyStatementStruct.NotPrincipal != nil { + t.Fatalf("Expected missing NotPrincipal, got %v", normalizedPolicyStatementStruct.NotPrincipal) + } +} + +func TestNormalizePolicyDocumentFlattenedNotPrincipalAWS(t *testing.T) { + normalizedPolicyStatementStruct := testPolicyDocumentStatementIndex(4) + var notPrincipalStruct policyStatementPrincipal + _ = mapstructure.Decode(normalizedPolicyStatementStruct.NotPrincipal, ¬PrincipalStruct) + if notPrincipalStruct.AWS != "arn:aws:iam::123456789012:role/TestRole" { + t.Fatalf("Expected non-array AWS non-principal, got %v", notPrincipalStruct.AWS) + } +} + +func TestNormalizePolicyDocumentMissingPrincipal(t *testing.T) { + normalizedPolicyStatementStruct := testPolicyDocumentStatementIndex(4) + if normalizedPolicyStatementStruct.Principal != nil { + t.Fatalf("Expected missing Principal, got %v", normalizedPolicyStatementStruct.Principal) + } +} + +func TestNormalizePolicyDocumentSortedPrincipalAWS(t *testing.T) { + normalizedPolicyStatementStruct := testPolicyDocumentStatementIndex(2) + var principalStruct policyStatementPrincipal + _ = mapstructure.Decode(normalizedPolicyStatementStruct.Principal, &principalStruct) + if principalStruct.AWS.([]interface{})[0].(string) != "arn:aws:iam::123456789012:role/TestRole" { + t.Fatalf("Expected first principal to be arn:aws:iam::123456789012:role/TestRole, got %s", principalStruct.AWS.([]interface{})[0].(string)) + } +}