From 4e7a9a35dbcca83b209447f5f8597cc7095f80fd Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Fri, 3 Feb 2023 11:08:09 -0800 Subject: [PATCH 1/7] Replaces `strings.HasPrefix(..., "arn:")` with `arn.IsARN()` --- .../iam/group_policy_attachment_test.go | 3 ++- .../iam/role_policy_attachment_test.go | 3 ++- .../iam/user_policy_attachment_test.go | 3 ++- internal/service/iam/wait.go | 22 +++++++++---------- 4 files changed, 17 insertions(+), 14 deletions(-) diff --git a/internal/service/iam/group_policy_attachment_test.go b/internal/service/iam/group_policy_attachment_test.go index d77f947eecb9..986d51e437db 100644 --- a/internal/service/iam/group_policy_attachment_test.go +++ b/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 diff --git a/internal/service/iam/role_policy_attachment_test.go b/internal/service/iam/role_policy_attachment_test.go index f4b088830191..09ebf25c8fda 100644 --- a/internal/service/iam/role_policy_attachment_test.go +++ b/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"]) } diff --git a/internal/service/iam/user_policy_attachment_test.go b/internal/service/iam/user_policy_attachment_test.go index 4875b263bec6..9bb090e8c60c 100644 --- a/internal/service/iam/user_policy_attachment_test.go +++ b/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"]) } diff --git a/internal/service/iam/wait.go b/internal/service/iam/wait.go index 428fb3958e8f..72dd1b89270b 100644 --- a/internal/service/iam/wait.go +++ b/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 } } From 9897c79b78c9c598b103a6ea7d74f2a6333eb1c3 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Fri, 3 Feb 2023 11:08:41 -0800 Subject: [PATCH 2/7] Adds semgrep rule --- .ci/semgrep/aws/arn.yml | 7 +++++++ .github/workflows/semgrep-ci.yml | 2 ++ GNUmakefile | 1 + 3 files changed, 10 insertions(+) create mode 100644 .ci/semgrep/aws/arn.yml diff --git a/.ci/semgrep/aws/arn.yml b/.ci/semgrep/aws/arn.yml new file mode 100644 index 000000000000..563b27b0a34a --- /dev/null +++ b/.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 diff --git a/.github/workflows/semgrep-ci.yml b/.github/workflows/semgrep-ci.yml index 596e01c208c7..a3de3b19924e 100644 --- a/.github/workflows/semgrep-ci.yml +++ b/.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' \ diff --git a/GNUmakefile b/GNUmakefile index 87a2b4f3f8d1..2b93a2a4308c 100644 --- a/GNUmakefile +++ b/GNUmakefile @@ -275,6 +275,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' \ From d29df3cbd1f1e3ec6671f6c0a623d08ea0f5012b Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Fri, 3 Feb 2023 12:25:50 -0800 Subject: [PATCH 3/7] Retries KMS Key Grants until IAM principals are valid --- internal/service/kms/grant.go | 43 ++++++++++++++---------------- internal/service/kms/grant_test.go | 10 +++---- 2 files changed, 25 insertions(+), 28 deletions(-) diff --git a/internal/service/kms/grant.go b/internal/service/kms/grant.go index 78adb7e3ad36..fcc929ea8465 100644 --- a/internal/service/kms/grant.go +++ b/internal/service/kms/grant.go @@ -10,6 +10,7 @@ import ( "time" "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/aws/arn" "github.com/aws/aws-sdk-go/service/kms" "github.com/hashicorp/aws-sdk-go-base/v2/awsv1shim/v2/tfawserr" "github.com/hashicorp/terraform-plugin-sdk/v2/diag" @@ -164,11 +165,8 @@ func resourceGrantCreate(ctx context.Context, d *schema.ResourceData, meta inter // Error Codes: https://docs.aws.amazon.com/sdk-for-go/api/service/kms/#KMS.CreateGrant // Under some circumstances a newly created IAM Role doesn't show up and causes // an InvalidArnException to be thrown. - if tfawserr.ErrCodeEquals(err, kms.ErrCodeDependencyTimeoutException) || - tfawserr.ErrCodeEquals(err, kms.ErrCodeInternalException) || - tfawserr.ErrCodeEquals(err, kms.ErrCodeInvalidArnException) { - return resource.RetryableError( - fmt.Errorf("creating KMS Grant for Key (%s), retrying: %w", keyId, err)) + if tfawserr.ErrCodeEquals(err, kms.ErrCodeDependencyTimeoutException, kms.ErrCodeInternalException, kms.ErrCodeInvalidArnException) { + return resource.RetryableError(err) } return resource.NonRetryableError(err) } @@ -216,35 +214,22 @@ func resourceGrantRead(ctx context.Context, d *schema.ResourceData, meta interfa return diags } - // The grant sometimes contains principals that identified by their unique id: "AROAJYCVIVUZIMTXXXXX" - // instead of "arn:aws:...", in this case don't update the state file - if strings.HasPrefix(*grant.GranteePrincipal, "arn:aws") { + if grant.GranteePrincipal != nil { d.Set("grantee_principal", grant.GranteePrincipal) - } else { - log.Printf( - "[WARN] Unable to update grantee principal state %s for grant id %s for key id %s.", - *grant.GranteePrincipal, grantId, keyId) } - if grant.RetiringPrincipal != nil { - if strings.HasPrefix(*grant.RetiringPrincipal, "arn:aws") { - d.Set("retiring_principal", grant.RetiringPrincipal) - } else { - log.Printf( - "[WARN] Unable to update retiring principal state %s for grant id %s for key id %s", - *grant.RetiringPrincipal, grantId, keyId) - } + d.Set("retiring_principal", grant.RetiringPrincipal) } if err := d.Set("operations", aws.StringValueSlice(grant.Operations)); err != nil { - log.Printf("[DEBUG] Error setting operations for grant %s with error %s", grantId, err) + return sdkdiag.AppendErrorf(diags, "setting operations: %s", err) } if aws.StringValue(grant.Name) != "" { d.Set("name", grant.Name) } if grant.Constraints != nil { if err := d.Set("constraints", flattenGrantConstraints(grant.Constraints)); err != nil { - log.Printf("[DEBUG] Error setting constraints for grant %s with error %s", grantId, err) + return sdkdiag.AppendErrorf(diags, "setting constraints: %s", err) } } @@ -324,6 +309,18 @@ func findGrantByIdRetry(ctx context.Context, conn *kms.KMS, keyId string, grantI return resource.NonRetryableError(err) } + if principal := aws.StringValue(grant.GranteePrincipal); principal != "" { + if !arn.IsARN(principal) { + return resource.RetryableError(fmt.Errorf("grantee principal is invalid: %q", principal)) + } + } + + if principal := aws.StringValue(grant.RetiringPrincipal); principal != "" { + if !arn.IsARN(principal) { + return resource.RetryableError(fmt.Errorf("retiring principal is invalid: %q", principal)) + } + } + return nil }) if tfresource.TimedOut(err) { @@ -523,7 +520,7 @@ func flattenGrantConstraints(constraint *kms.GrantConstraints) *schema.Set { } func decodeGrantID(id string) (string, string, error) { - if strings.HasPrefix(id, "arn:aws") { + if arn.IsARN(id) { arnParts := strings.Split(id, "/") if len(arnParts) != 2 { return "", "", fmt.Errorf("unexpected format of ARN (%q), expected KeyID:GrantID", id) diff --git a/internal/service/kms/grant_test.go b/internal/service/kms/grant_test.go index 5ac0bb4f0891..b7e94e972a11 100644 --- a/internal/service/kms/grant_test.go +++ b/internal/service/kms/grant_test.go @@ -34,8 +34,8 @@ func TestAccKMSGrant_basic(t *testing.T) { resource.TestCheckResourceAttr(resourceName, "operations.#", "2"), resource.TestCheckTypeSetElemAttr(resourceName, "operations.*", "Encrypt"), resource.TestCheckTypeSetElemAttr(resourceName, "operations.*", "Decrypt"), - resource.TestCheckResourceAttrSet(resourceName, "grantee_principal"), - resource.TestCheckResourceAttrSet(resourceName, "key_id"), + resource.TestCheckResourceAttrPair(resourceName, "grantee_principal", "aws_iam_role.test", "arn"), + resource.TestCheckResourceAttrPair(resourceName, "key_id", "aws_kms_key.test", "key_id"), ), }, { @@ -112,7 +112,7 @@ func TestAccKMSGrant_withRetiringPrincipal(t *testing.T) { Config: testAccGrantConfig_retiringPrincipal(rName), Check: resource.ComposeTestCheckFunc( testAccCheckGrantExists(resourceName), - resource.TestCheckResourceAttrSet(resourceName, "retiring_principal"), + resource.TestCheckResourceAttrPair(resourceName, "retiring_principal", "aws_iam_role.test", "arn"), ), }, { @@ -174,8 +174,8 @@ func TestAccKMSGrant_arn(t *testing.T) { resource.TestCheckResourceAttr(resourceName, "operations.#", "2"), resource.TestCheckTypeSetElemAttr(resourceName, "operations.*", "Encrypt"), resource.TestCheckTypeSetElemAttr(resourceName, "operations.*", "Decrypt"), - resource.TestCheckResourceAttrSet(resourceName, "grantee_principal"), - resource.TestCheckResourceAttrSet(resourceName, "key_id"), + resource.TestCheckResourceAttrPair(resourceName, "grantee_principal", "aws_iam_role.test", "arn"), + resource.TestCheckResourceAttrPair(resourceName, "key_id", "aws_kms_key.test", "arn"), ), }, { From 71267d6c2de1a606b74d6abc5fb3a0198011b35f Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Fri, 3 Feb 2023 12:26:15 -0800 Subject: [PATCH 4/7] Updates semgrep check --- .ci/semgrep/aws/arn.yml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.ci/semgrep/aws/arn.yml b/.ci/semgrep/aws/arn.yml index 563b27b0a34a..917c24f894ea 100644 --- a/.ci/semgrep/aws/arn.yml +++ b/.ci/semgrep/aws/arn.yml @@ -3,5 +3,8 @@ rules: languages: [go] message: Prefer `aws.IsARN` to `strings.HasPrefix` patterns: - - pattern: strings.HasPrefix($STR, "arn:") + - pattern: strings.HasPrefix($STR, $ARN) + - metavariable-regex: + metavariable: $ARN + regex: ^\"arn:[^"]*\"$ severity: WARNING From ee478cda72075bd6d051c3a93bc4506f53c31fee Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Fri, 3 Feb 2023 12:36:53 -0800 Subject: [PATCH 5/7] Adds CHANGELOG entry --- .changelog/29245.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/29245.txt diff --git a/.changelog/29245.txt b/.changelog/29245.txt new file mode 100644 index 000000000000..afe860821c6c --- /dev/null +++ b/.changelog/29245.txt @@ -0,0 +1,3 @@ +```release-note:bug +resource/aws_kms_grant: Retries until valid principal ARNs are returned instead of not updating state +``` From 0f4863f77a4c5c84538a9e9690b90486d7cf0f96 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Fri, 3 Feb 2023 12:50:36 -0800 Subject: [PATCH 6/7] Adds semgrep rule exclusion --- internal/service/kms/grant.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/service/kms/grant.go b/internal/service/kms/grant.go index fcc929ea8465..9c358bc7985c 100644 --- a/internal/service/kms/grant.go +++ b/internal/service/kms/grant.go @@ -214,10 +214,10 @@ func resourceGrantRead(ctx context.Context, d *schema.ResourceData, meta interfa return diags } - if grant.GranteePrincipal != nil { + if grant.GranteePrincipal != nil { // nosemgrep:ci.helper-schema-ResourceData-Set-extraneous-nil-check d.Set("grantee_principal", grant.GranteePrincipal) } - if grant.RetiringPrincipal != nil { + if grant.RetiringPrincipal != nil { // nosemgrep:ci.helper-schema-ResourceData-Set-extraneous-nil-check d.Set("retiring_principal", grant.RetiringPrincipal) } From 3f12c0eeb60881c9f6d02fe4be521b3c6ef5f06a Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Fri, 3 Feb 2023 12:51:20 -0800 Subject: [PATCH 7/7] Removes unneeded semgrep rule path exclusion --- .ci/.semgrep.yml | 2 -- internal/service/apigateway/integration.go | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/.ci/.semgrep.yml b/.ci/.semgrep.yml index 79ac465d4dd7..3f1094361e9d 100644 --- a/.ci/.semgrep.yml +++ b/.ci/.semgrep.yml @@ -226,8 +226,6 @@ rules: paths: include: - internal/ - exclude: - - internal/service/apigateway/integration.go patterns: - pattern-either: - pattern: | diff --git a/internal/service/apigateway/integration.go b/internal/service/apigateway/integration.go index 46f572ef1d66..7d20febd758e 100644 --- a/internal/service/apigateway/integration.go +++ b/internal/service/apigateway/integration.go @@ -279,7 +279,7 @@ func resourceIntegrationRead(ctx context.Context, d *schema.ResourceData, meta i d.Set("cache_namespace", integration.CacheNamespace) d.Set("connection_id", integration.ConnectionId) d.Set("connection_type", apigateway.ConnectionTypeInternet) - if integration.ConnectionType != nil { + if integration.ConnectionType != nil { // nosemgrep:ci.helper-schema-ResourceData-Set-extraneous-nil-check d.Set("connection_type", integration.ConnectionType) } d.Set("content_handling", integration.ContentHandling)