diff --git a/.changelog/29226.txt b/.changelog/29226.txt new file mode 100644 index 00000000000..f3ec5c402ed --- /dev/null +++ b/.changelog/29226.txt @@ -0,0 +1,8 @@ + +```release-note:bug +resource/aws_sns_topic: Fixes potential race condition when reading policy document. +``` + +```release-note:bug +resource/aws_sns_topic_policy: Fixes potential race condition when reading policy document. +``` diff --git a/internal/acctest/acctest.go b/internal/acctest/acctest.go index 17a7c0c9c61..ccd0e590a23 100644 --- a/internal/acctest/acctest.go +++ b/internal/acctest/acctest.go @@ -2,6 +2,7 @@ package acctest import ( "context" + "encoding/json" "fmt" "log" "os" @@ -37,6 +38,7 @@ import ( tforganizations "github.com/hashicorp/terraform-provider-aws/internal/service/organizations" tfsts "github.com/hashicorp/terraform-provider-aws/internal/service/sts" "github.com/hashicorp/terraform-provider-aws/internal/tfresource" + "github.com/jmespath/go-jmespath" ) const ( @@ -582,6 +584,52 @@ func CheckResourceAttrEquivalentJSON(resourceName, attributeName, expectedJSON s } } +func CheckResourceAttrJMESPair(nameFirst, keyFirst, jmesPath, nameSecond, keySecond string) resource.TestCheckFunc { + return func(s *terraform.State) error { + first, err := PrimaryInstanceState(s, nameFirst) + if err != nil { + return err + } + + second, err := PrimaryInstanceState(s, nameSecond) + if err != nil { + return err + } + + vFirst, okFirst := first.Attributes[keyFirst] + if !okFirst { + return fmt.Errorf("%s: Attribute %q not set", nameFirst, keyFirst) + } + + var jsonData any + err = json.Unmarshal([]byte(vFirst), &jsonData) + if err != nil { + return fmt.Errorf("%s: Expected attribute %q to be JSON: %w", nameFirst, keyFirst, err) + } + + result, err := jmespath.Search(jmesPath, jsonData) + if err != nil { + return fmt.Errorf("Invalid JMESPath %q: %w", jmesPath, err) + } + + value, ok := result.(string) + if !ok { + return fmt.Errorf("%s: Attribute %q, JMESPath %q, expected single string, got %#v", nameFirst, keyFirst, jmesPath, result) + } + + vSecond, okSecond := second.Attributes[keySecond] + if !okSecond { + return fmt.Errorf("%s: Attribute %q, JMESPath %q is %q, but %q is not set in %s", nameFirst, keyFirst, jmesPath, value, keySecond, nameSecond) + } + + if value != vSecond { + return fmt.Errorf("%s: Attribute %q, JMESPath %q, expected %q, got %q", nameFirst, keyFirst, jmesPath, vSecond, value) + } + + return nil + } +} + // Copied and inlined from the SDK testing code func PrimaryInstanceState(s *terraform.State, name string) (*terraform.InstanceState, error) { rs, ok := s.RootModule().Resources[name] diff --git a/internal/service/sns/find.go b/internal/service/sns/find.go index 9e6d89c751e..8ea72516788 100644 --- a/internal/service/sns/find.go +++ b/internal/service/sns/find.go @@ -2,6 +2,7 @@ package sns import ( "context" + "errors" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/sns" @@ -35,7 +36,34 @@ func FindPlatformApplicationAttributesByARN(ctx context.Context, conn *sns.SNS, return aws.StringValueMap(output.Attributes), nil } +// FindTopicAttributesByARN returns topic attributes, ensuring that any Policy field is populated with +// valid principals, i.e. the principal is either an AWS Account ID or an ARN func FindTopicAttributesByARN(ctx context.Context, conn *sns.SNS, arn string) (map[string]string, error) { + var attributes map[string]string + err := tfresource.Retry(ctx, propagationTimeout, func() *resource.RetryError { + var err error + attributes, err = GetTopicAttributesByARN(ctx, conn, arn) + if err != nil { + return resource.NonRetryableError(err) + } + + valid, err := policyHasValidAWSPrincipals(attributes[TopicAttributeNamePolicy]) + if err != nil { + return resource.NonRetryableError(err) + } + if !valid { + return resource.RetryableError(errors.New("contains invalid principals")) + } + + return nil + }) + + return attributes, err +} + +// GetTopicAttributesByARN returns topic attributes without any validation. Any principals in a Policy field +// may contain Unique IDs instead of valid values. To ensure policies are valid, use FindTopicAttributesByARN +func GetTopicAttributesByARN(ctx context.Context, conn *sns.SNS, arn string) (map[string]string, error) { input := &sns.GetTopicAttributesInput{ TopicArn: aws.String(arn), } diff --git a/internal/service/sns/topic.go b/internal/service/sns/topic.go index d3899a7fc45..dc89fa61a17 100644 --- a/internal/service/sns/topic.go +++ b/internal/service/sns/topic.go @@ -2,6 +2,7 @@ package sns import ( "context" + "encoding/json" "fmt" "log" "regexp" @@ -19,9 +20,11 @@ import ( "github.com/hashicorp/terraform-provider-aws/internal/attrmap" "github.com/hashicorp/terraform-provider-aws/internal/conns" "github.com/hashicorp/terraform-provider-aws/internal/create" + "github.com/hashicorp/terraform-provider-aws/internal/errs/sdkdiag" tftags "github.com/hashicorp/terraform-provider-aws/internal/tags" "github.com/hashicorp/terraform-provider-aws/internal/tfresource" "github.com/hashicorp/terraform-provider-aws/internal/verify" + "github.com/jmespath/go-jmespath" ) var ( @@ -290,7 +293,8 @@ func resourceTopicCreate(ctx context.Context, d *schema.ResourceData, meta inter return resourceTopicRead(ctx, d, meta) } -func resourceTopicRead(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics { +func resourceTopicRead(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics { + var diags diag.Diagnostics conn := meta.(*conns.AWSClient).SNSConn() defaultTagsConfig := meta.(*conns.AWSClient).DefaultTagsConfig ignoreTagsConfig := meta.(*conns.AWSClient).IgnoreTagsConfig @@ -302,9 +306,8 @@ func resourceTopicRead(ctx context.Context, d *schema.ResourceData, meta interfa d.SetId("") return nil } - if err != nil { - return diag.Errorf("reading SNS Topic (%s): %s", d.Id(), err) + return sdkdiag.AppendErrorf(diags, "reading SNS Topic (%s): %s", d.Id(), err) } err = topicAttributeMap.APIAttributesToResourceData(attributes, d) @@ -336,23 +339,76 @@ func resourceTopicRead(ctx context.Context, d *schema.ResourceData, meta interfa } if err != nil { - return diag.Errorf("listing tags for SNS Topic (%s): %s", d.Id(), err) + return sdkdiag.AppendErrorf(diags, "listing tags for SNS Topic (%s): %s", d.Id(), err) } tags = tags.IgnoreAWS().IgnoreConfig(ignoreTagsConfig) //lintignore:AWSR002 if err := d.Set("tags", tags.RemoveDefaultConfig(defaultTagsConfig).Map()); err != nil { - return diag.Errorf("setting tags: %s", err) + return sdkdiag.AppendErrorf(diags, "setting tags: %s", err) } if err := d.Set("tags_all", tags.Map()); err != nil { - return diag.Errorf("setting tags_all: %s", err) + return sdkdiag.AppendErrorf(diags, "setting tags_all: %s", err) } return nil } +// policyHasValidAWSPrincipals validates that the Principals in an IAM Policy are valid +// Assumes that non-"AWS" Principals are valid +// The value can be a single string or a slice of strings +// Valid strings are either an ARN or an AWS account ID +func policyHasValidAWSPrincipals(policy string) (bool, error) { // nosemgrep:ci.aws-in-func-name + var policyData any + err := json.Unmarshal([]byte(policy), &policyData) + if err != nil { + return false, fmt.Errorf("parsing policy: %w", err) + } + + result, err := jmespath.Search("Statement[*].Principal.AWS", policyData) + if err != nil { + return false, fmt.Errorf("parsing policy: %w", err) + } + + principals, ok := result.([]any) + if !ok { + return false, fmt.Errorf(`parsing policy: unexpected result: (%[1]T) "%[1]v"`, result) + } + + for _, principal := range principals { + switch x := principal.(type) { + case string: + if !isValidAWSPrincipal(x) { + return false, nil + } + case []string: + for _, s := range x { + if !isValidAWSPrincipal(s) { + return false, nil + } + } + } + } + + return true, nil +} + +// isValidAWSPrincipal returns true if a string is either an ARN, an AWS account ID, or `*` +func isValidAWSPrincipal(principal string) bool { // nosemgrep:ci.aws-in-func-name + if principal == "*" { + return true + } + if arn.IsARN(principal) { + return true + } + if regexp.MustCompile(`^\d{12}$`).MatchString(principal) { + return true + } + return false +} + func resourceTopicUpdate(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics { conn := meta.(*conns.AWSClient).SNSConn() diff --git a/internal/service/sns/topic_policy_doc_test.go b/internal/service/sns/topic_policy_doc_test.go new file mode 100644 index 00000000000..9863f62c71b --- /dev/null +++ b/internal/service/sns/topic_policy_doc_test.go @@ -0,0 +1,264 @@ +package sns + +import ( + "encoding/json" + "testing" + + "github.com/hashicorp/terraform-provider-aws/internal/errs" +) + +func TestPolicyHasValidAWSPrincipals(t *testing.T) { // nosemgrep:ci.aws-in-func-name + t.Parallel() + + testcases := map[string]struct { + json string + valid bool + err func(t *testing.T, err error) + }{ + "single_arn": { + json: `{ + "Statement":[ + { + "Effect":"Allow", + "Principal":{ + "AWS": "arn:aws:iam::123456789012:role/role-name" + }, + "Action": "*", + "Resource": "*" + } + ] +}`, // lintignore:AWSAT005 + valid: true, + }, + "account_id": { + json: `{ + "Statement":[ + { + "Effect":"Allow", + "Principal":{ + "AWS": "123456789012" + }, + "Action": "*", + "Resource": "*" + } + ] +}`, + valid: true, + }, + "wildcard": { + json: `{ + "Statement":[ + { + "Effect":"Allow", + "Principal":{ + "AWS": "*" + }, + "Action": "*", + "Resource": "*" + } + ] +}`, + valid: true, + }, + "unique_id": {json: `{ + "Statement":[ + { + "Effect":"Allow", + "Principal":{ + "AWS": "AROAS5MHDZS6NEXAMPLE" + }, + "Action": "*", + "Resource": "*" + } + ] +}`, + valid: false, + }, + "non_AWS_principal": {json: `{ + "Statement":[ + { + "Effect":"Allow", + "Principal":{ + "Federated": "cognito-identity.amazonaws.com" + }, + "Action": "*", + "Resource": "*" + } + ] +}`, + valid: true, + }, + "multiple_arns": { + json: `{ + "Statement":[ + { + "Effect":"Allow", + "Principal":{ + "AWS": [ + "arn:aws:iam::123456789012:role/role-name", + "arn:aws:iam::123456789012:role/another-role-name" + ] + }, + "Action": "*", + "Resource": "*" + } + ] +}`, // lintignore:AWSAT005 + valid: true, + }, + "mixed_principals": { + json: `{ + "Statement":[ + { + "Effect":"Allow", + "Principal":{ + "AWS": [ + "arn:aws:iam::123456789012:role/role-name", + "AROAS5MHDZS6NEXAMPLE" + ] + }, + "Action": "*", + "Resource": "*" + } + ] +}`, // lintignore:AWSAT005 + valid: true, + }, + "multiple_statements_valid": { + json: `{ + "Statement":[ + { + "Effect":"Allow", + "Principal":{ + "AWS": "arn:aws:iam::123456789012:role/role-name" + }, + "Action": "*", + "Resource": "*" + }, + { + "Effect":"Allow", + "Principal":{ + "AWS": "arn:aws:iam::123456789012:role/another-role-name" + }, + "Action": "*", + "Resource": "*" + } + ] +}`, // lintignore:AWSAT005 + valid: true, + }, + "multiple_statements_invalid": { + json: `{ + "Statement":[ + { + "Effect":"Allow", + "Principal":{ + "AWS": "arn:aws:iam::123456789012:role/role-name" + }, + "Action": "*", + "Resource": "*" + }, + { + "Effect":"Allow", + "Principal":{ + "AWS": "AROAS5MHDZS6NEXAMPLE" + }, + "Action": "*", + "Resource": "*" + } + ] +}`, // lintignore:AWSAT005 + valid: false, + }, + "empty_string": { + json: "", + err: func(t *testing.T, err error) { + if !errs.IsA[*json.SyntaxError](err) { + t.Fatalf("expected JSON syntax error, got %#v", err) + } + }, + }, + "invalid_json": { + json: `{ + "Statement":[ + { + "Effect":"Allow" + "Principal":{ + "AWS": "arn:aws:iam::123456789012:role/role-name" + }, + "Action": "*", + "Resource": "*" + } + ] +}`, // lintignore:AWSAT005 + err: func(t *testing.T, err error) { + if !errs.IsA[*json.SyntaxError](err) { + t.Fatalf("expected JSON syntax error, got %#v", err) + } + }, + }, + } + + for name, testcase := range testcases { + testcase := testcase + t.Run(name, func(t *testing.T) { + t.Parallel() + + valid, err := policyHasValidAWSPrincipals(testcase.json) + + if testcase.err == nil { + if err != nil { + t.Fatalf("expected no error, got %s", err) + } + } else { + if err == nil { + t.Fatalf("expected error, not none") + } + testcase.err(t, err) + } + + if a, e := valid, testcase.valid; a != e { + t.Fatalf("expected %t, got %t", e, a) + } + }) + } +} + +func TestIsValidAWSPrincipal(t *testing.T) { // nosemgrep:ci.aws-in-func-name + t.Parallel() + + testcases := map[string]struct { + value string + valid bool + }{ + "role_arn": { + value: "arn:aws:iam::123456789012:role/role-name", // lintignore:AWSAT005 + valid: true, + }, + "root_arn": { + value: "arn:aws:iam::123456789012:root", // lintignore:AWSAT005 + valid: true, + }, + "account_id": { + value: "123456789012", + valid: true, + }, + "unique_id": { + value: "AROAS5MHDZS6NEXAMPLE", + valid: false, + }, + } + + for name, testcase := range testcases { + testcase := testcase + t.Run(name, func(t *testing.T) { + t.Parallel() + + a := isValidAWSPrincipal(testcase.value) + + if e := testcase.valid; a != e { + t.Fatalf("expected %t, got %t", e, a) + } + }) + } +} diff --git a/internal/service/sns/topic_policy_test.go b/internal/service/sns/topic_policy_test.go index f56744df6fe..505cee02bd1 100644 --- a/internal/service/sns/topic_policy_test.go +++ b/internal/service/sns/topic_policy_test.go @@ -168,7 +168,7 @@ func testAccCheckTopicPolicyDestroy(ctx context.Context) resource.TestCheckFunc continue } - _, err := tfsns.FindTopicAttributesByARN(ctx, conn, rs.Primary.ID) + _, err := tfsns.GetTopicAttributesByARN(ctx, conn, rs.Primary.ID) if tfresource.NotFound(err) { continue diff --git a/internal/service/sns/topic_test.go b/internal/service/sns/topic_test.go index 0d2197c61f7..c0cce3ffa0a 100644 --- a/internal/service/sns/topic_test.go +++ b/internal/service/sns/topic_test.go @@ -252,8 +252,9 @@ func TestAccSNSTopic_withIAMRole(t *testing.T) { Steps: []resource.TestStep{ { Config: testAccTopicConfig_iamRole(rName), - Check: resource.ComposeTestCheckFunc( + Check: resource.ComposeAggregateTestCheckFunc( testAccCheckTopicExists(ctx, resourceName, &attributes), + acctest.CheckResourceAttrJMESPair(resourceName, "policy", "Statement[0].Principal.AWS", "aws_iam_role.example", "arn"), ), }, { @@ -561,7 +562,7 @@ func testAccCheckTopicHasPolicy(ctx context.Context, n string, expectedPolicyTex equivalent, err := awspolicy.PoliciesAreEquivalent(actualPolicyText, expectedPolicyText) if err != nil { - return fmt.Errorf("Error testing policy equivalence: %s", err) + return fmt.Errorf("testing policy equivalence: %s", err) } if !equivalent { @@ -618,7 +619,7 @@ func testAccCheckTopicDestroy(ctx context.Context) resource.TestCheckFunc { continue } - _, err := tfsns.FindTopicAttributesByARN(ctx, conn, rs.Primary.ID) + _, err := tfsns.GetTopicAttributesByARN(ctx, conn, rs.Primary.ID) if tfresource.NotFound(err) { continue @@ -648,7 +649,7 @@ func testAccCheckTopicExists(ctx context.Context, n string, v *map[string]string conn := acctest.Provider.Meta().(*conns.AWSClient).SNSConn() - output, err := tfsns.FindTopicAttributesByARN(ctx, conn, rs.Primary.ID) + output, err := tfsns.GetTopicAttributesByARN(ctx, conn, rs.Primary.ID) if err != nil { return err @@ -739,50 +740,50 @@ func testAccTopicConfig_iamRole(rName string) string { return fmt.Sprintf(` data "aws_partition" "current" {} -resource "aws_iam_role" "example" { +resource "aws_sns_topic" "test" { name = %[1]q - path = "/test/" - assume_role_policy = <