Fix race condition in SNS topic and topic policy resources

.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.
+```

.ci/semgrep/aws/arn.yml

@@ -0,0 +1,7 @@
+rules:
+  - id: prefer-isarn-to-stringshasprefix
+    languages: [go]
+    message: Prefer `aws.IsARN` to `strings.HasPrefix`
+    patterns:
+      - pattern: strings.HasPrefix($STR, "arn:")
+    severity: WARNING

.github/workflows/semgrep-ci.yml

@@ -28,6 +28,8 @@ jobs:
           semgrep $COMMON_PARAMS \
             --config .ci/.semgrep.yml \
             --config .ci/semgrep/acctest/ \
+            --config .ci/semgrep/aws/ \
+            --config .ci/semgrep/migrate/ \
             --config 'r/dgryski.semgrep-go.badnilguard' \
             --config 'r/dgryski.semgrep-go.errnilcheck' \
             --config 'r/dgryski.semgrep-go.marshaljson' \

GNUmakefile

@@ -274,6 +274,7 @@ semall:
 		--config .ci/.semgrep-service-name2.yml \
 		--config .ci/.semgrep-service-name3.yml \
 		--config .ci/semgrep/acctest/ \
+		--config .ci/semgrep/aws/ \
 		--config .ci/semgrep/migrate/ \
 		--config 'r/dgryski.semgrep-go.badnilguard' \
 		--config 'r/dgryski.semgrep-go.errnilcheck' \

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]

internal/service/iam/group_policy_attachment_test.go

@@ -6,6 +6,7 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/aws/aws-sdk-go-v2/aws/arn"
 	"github.com/aws/aws-sdk-go/aws"
 	"github.com/aws/aws-sdk-go/service/iam"
 	sdkacctest "github.com/hashicorp/terraform-plugin-sdk/v2/helper/acctest"
@@ -50,7 +51,7 @@ func TestAccIAMGroupPolicyAttachment_basic(t *testing.T) {
 						return fmt.Errorf("expected 1 state: %#v", s)
 					}
 					rs := s[0]
-					if !strings.HasPrefix(rs.Attributes["policy_arn"], "arn:") {
+					if !arn.IsARN(rs.Attributes["policy_arn"]) {
 						return fmt.Errorf("expected policy_arn attribute to be set and begin with arn:, received: %s", rs.Attributes["policy_arn"])
 					}
 					return nil

internal/service/iam/role_policy_attachment_test.go

@@ -7,6 +7,7 @@ import (
 	"testing"
 
 	"github.com/aws/aws-sdk-go/aws"
+	"github.com/aws/aws-sdk-go/aws/arn"
 	"github.com/aws/aws-sdk-go/service/iam"
 	"github.com/hashicorp/aws-sdk-go-base/v2/awsv1shim/v2/tfawserr"
 	sdkacctest "github.com/hashicorp/terraform-plugin-sdk/v2/helper/acctest"
@@ -52,7 +53,7 @@ func TestAccIAMRolePolicyAttachment_basic(t *testing.T) {
 
 					rs := s[0]
 
-					if !strings.HasPrefix(rs.Attributes["policy_arn"], "arn:") {
+					if !arn.IsARN(rs.Attributes["policy_arn"]) {
 						return fmt.Errorf("expected policy_arn attribute to be set and begin with arn:, received: %s", rs.Attributes["policy_arn"])
 					}
 

internal/service/iam/user_policy_attachment_test.go

@@ -7,6 +7,7 @@ import (
 	"testing"
 
 	"github.com/aws/aws-sdk-go/aws"
+	"github.com/aws/aws-sdk-go/aws/arn"
 	"github.com/aws/aws-sdk-go/service/iam"
 	sdkacctest "github.com/hashicorp/terraform-plugin-sdk/v2/helper/acctest"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
@@ -50,7 +51,7 @@ func TestAccIAMUserPolicyAttachment_basic(t *testing.T) {
 
 					rs := s[0]
 
-					if !strings.HasPrefix(rs.Attributes["policy_arn"], "arn:") {
+					if !arn.IsARN(rs.Attributes["policy_arn"]) {
 						return fmt.Errorf("expected policy_arn attribute to be set and begin with arn:, received: %s", rs.Attributes["policy_arn"])
 					}
 

internal/service/iam/wait.go

@@ -2,10 +2,10 @@ package iam
 
 import (
 	"context"
-	"strings"
 	"time"
 
 	"github.com/aws/aws-sdk-go/aws"
+	"github.com/aws/aws-sdk-go/aws/arn"
 	"github.com/aws/aws-sdk-go/service/iam"
 	"github.com/hashicorp/aws-sdk-go-base/v2/awsv1shim/v2/tfawserr"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
@@ -26,10 +26,14 @@ const (
 )
 
 func waitRoleARNIsNotUniqueID(ctx context.Context, conn *iam.IAM, id string, role *iam.Role) (*iam.Role, error) {
+	if arn.IsARN(aws.StringValue(role.Arn)) {
+		return role, nil
+	}
+
 	stateConf := &resource.StateChangeConf{
 		Pending:                   []string{RoleStatusARNIsUniqueID, RoleStatusNotFound},
 		Target:                    []string{RoleStatusARNIsARN},
-		Refresh:                   statusRoleCreate(ctx, conn, id, role),
+		Refresh:                   statusRoleCreate(ctx, conn, id),
 		Timeout:                   propagationTimeout,
 		NotFoundChecks:            10,
 		ContinuousTargetOccurence: 5,
@@ -44,13 +48,9 @@ func waitRoleARNIsNotUniqueID(ctx context.Context, conn *iam.IAM, id string, rol
 	return nil, err
 }
 
-func statusRoleCreate(ctx context.Context, conn *iam.IAM, id string, role *iam.Role) resource.StateRefreshFunc {
+func statusRoleCreate(ctx context.Context, conn *iam.IAM, id string) resource.StateRefreshFunc {
 	return func() (interface{}, string, error) {
-		if strings.HasPrefix(aws.StringValue(role.Arn), "arn:") {
-			return role, RoleStatusARNIsARN, nil
-		}
-
-		output, err := FindRoleByName(ctx, conn, id)
+		role, err := FindRoleByName(ctx, conn, id)
 
 		if tfresource.NotFound(err) {
 			return nil, RoleStatusNotFound, nil
@@ -60,11 +60,11 @@ func statusRoleCreate(ctx context.Context, conn *iam.IAM, id string, role *iam.R
 			return nil, "", err
 		}
 
-		if strings.HasPrefix(aws.StringValue(output.Arn), "arn:") {
-			return output, RoleStatusARNIsARN, nil
+		if arn.IsARN(aws.StringValue(role.Arn)) {
+			return role, RoleStatusARNIsARN, nil
 		}
 
-		return output, RoleStatusARNIsUniqueID, nil
+		return role, RoleStatusARNIsUniqueID, nil
 	}
 }
 

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),
 	}

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()