diff --git a/.changelog/39782.txt b/.changelog/39782.txt new file mode 100644 index 000000000000..5ba0ea53cdac --- /dev/null +++ b/.changelog/39782.txt @@ -0,0 +1,11 @@ +```release-note:enhancement +resource/aws_s3_bucket_object: Remove the call to `kms:DescribeKey` for the S3 default AWS managed key (`alias/aws/s3`) on Read +``` + +```release-note:enhancement +resource/aws_s3_object: Remove the call to `kms:DescribeKey` for the S3 default AWS managed key (`alias/aws/s3`) on Read +``` + +```release-note:enhancement +resource/aws_s3_object_copy: Remove the call to `kms:DescribeKey` for the S3 default AWS managed key (`alias/aws/s3`) on Read +``` \ No newline at end of file diff --git a/internal/service/kms/exports.go b/internal/service/kms/exports.go index cd4d4c22f1e4..dc49d80335a0 100644 --- a/internal/service/kms/exports.go +++ b/internal/service/kms/exports.go @@ -9,7 +9,6 @@ var ( DiffSuppressKeyOrAlias = diffSuppressKeyOrAlias FindAliasByName = findAliasByName FindDefaultKeyARNForService = findDefaultKeyARNForService - FindKeyByID = findKeyByID ValidateKey = validateKey ValidateKeyOrAlias = validateKeyOrAlias ) diff --git a/internal/service/kms/exports_test.go b/internal/service/kms/exports_test.go index 88bff98afcff..084d50094c42 100644 --- a/internal/service/kms/exports_test.go +++ b/internal/service/kms/exports_test.go @@ -19,6 +19,7 @@ var ( AliasNamePrefix = aliasNamePrefix FindCustomKeyStoreByID = findCustomKeyStoreByID FindGrantByTwoPartKey = findGrantByTwoPartKey + FindKeyByID = findKeyByID FindKeyPolicyByTwoPartKey = findKeyPolicyByTwoPartKey GrantParseResourceID = grantParseResourceID KeyARNOrIDEqual = keyARNOrIDEqual diff --git a/internal/service/s3/bucket_object.go b/internal/service/s3/bucket_object.go index 7030b9cdb168..631a49255c52 100644 --- a/internal/service/s3/bucket_object.go +++ b/internal/service/s3/bucket_object.go @@ -29,7 +29,6 @@ import ( "github.com/hashicorp/terraform-provider-aws/internal/errs/sdkdiag" "github.com/hashicorp/terraform-provider-aws/internal/flex" "github.com/hashicorp/terraform-provider-aws/internal/sdkv2" - tfkms "github.com/hashicorp/terraform-provider-aws/internal/service/kms" tftags "github.com/hashicorp/terraform-provider-aws/internal/tags" "github.com/hashicorp/terraform-provider-aws/internal/tfresource" itypes "github.com/hashicorp/terraform-provider-aws/internal/types" @@ -139,13 +138,6 @@ func resourceBucketObject() *schema.Resource { Optional: true, Computed: true, ValidateFunc: verify.ValidARN, - DiffSuppressFunc: func(k, old, new string, d *schema.ResourceData) bool { - // ignore diffs where the user hasn't specified a kms_key_id but the bucket has a default KMS key configured - if new == "" && d.Get("server_side_encryption") == types.ServerSideEncryptionAwsKms { - return true - } - return false - }, }, "metadata": { Type: schema.TypeMap, @@ -242,6 +234,9 @@ func resourceBucketObjectRead(ctx context.Context, d *schema.ResourceData, meta d.Set(names.AttrContentType, output.ContentType) // See https://forums.aws.amazon.com/thread.jspa?threadID=44003 d.Set("etag", strings.Trim(aws.ToString(output.ETag), `"`)) + if output.SSEKMSKeyId != nil { // nosemgrep:ci.helper-schema-ResourceData-Set-extraneous-nil-check + d.Set(names.AttrKMSKeyID, output.SSEKMSKeyId) + } d.Set("metadata", output.Metadata) d.Set("object_lock_legal_hold_status", output.ObjectLockLegalHoldStatus) d.Set("object_lock_mode", output.ObjectLockMode) @@ -256,10 +251,6 @@ func resourceBucketObjectRead(ctx context.Context, d *schema.ResourceData, meta d.Set("version_id", output.VersionId) d.Set("website_redirect", output.WebsiteRedirectLocation) - if err := resourceBucketObjectSetKMS(ctx, d, meta, output.SSEKMSKeyId); err != nil { - return sdkdiag.AppendFromErr(diags, err) - } - return diags } @@ -507,25 +498,6 @@ func resourceBucketObjectUpload(ctx context.Context, d *schema.ResourceData, met return append(diags, resourceBucketObjectRead(ctx, d, meta)...) } -func resourceBucketObjectSetKMS(ctx context.Context, d *schema.ResourceData, meta interface{}, sseKMSKeyId *string) error { - // Only set non-default KMS key ID (one that doesn't match default) - if sseKMSKeyId != nil { - // retrieve S3 KMS Default Master Key - conn := meta.(*conns.AWSClient).KMSClient(ctx) - keyMetadata, err := tfkms.FindKeyByID(ctx, conn, defaultKMSKeyAlias) - if err != nil { - return fmt.Errorf("Failed to describe default S3 KMS key (%s): %s", defaultKMSKeyAlias, err) - } - - if kmsKeyID := aws.ToString(sseKMSKeyId); kmsKeyID != aws.ToString(keyMetadata.Arn) { - log.Printf("[DEBUG] S3 object is encrypted using a non-default KMS Key ID: %s", kmsKeyID) - d.Set(names.AttrKMSKeyID, sseKMSKeyId) - } - } - - return nil -} - func resourceBucketObjectCustomizeDiff(_ context.Context, d *schema.ResourceDiff, meta interface{}) error { if hasBucketObjectContentChanges(d) { return d.SetNewComputed("version_id") diff --git a/internal/service/s3/enum.go b/internal/service/s3/enum.go index 66435fe78854..dea45d082add 100644 --- a/internal/service/s3/enum.go +++ b/internal/service/s3/enum.go @@ -9,8 +9,6 @@ import ( "github.com/aws/aws-sdk-go-v2/aws/arn" ) -const defaultKMSKeyAlias = "alias/aws/s3" - type bucketNameType int const ( diff --git a/internal/service/s3/object.go b/internal/service/s3/object.go index a9d35e3176bf..274df8ee1b0c 100644 --- a/internal/service/s3/object.go +++ b/internal/service/s3/object.go @@ -31,7 +31,6 @@ import ( "github.com/hashicorp/terraform-provider-aws/internal/errs/sdkdiag" "github.com/hashicorp/terraform-provider-aws/internal/flex" "github.com/hashicorp/terraform-provider-aws/internal/sdkv2" - tfkms "github.com/hashicorp/terraform-provider-aws/internal/service/kms" tftags "github.com/hashicorp/terraform-provider-aws/internal/tags" "github.com/hashicorp/terraform-provider-aws/internal/tfresource" itypes "github.com/hashicorp/terraform-provider-aws/internal/types" @@ -165,13 +164,6 @@ func resourceObject() *schema.Resource { Optional: true, Computed: true, ValidateFunc: verify.ValidARN, - DiffSuppressFunc: func(k, old, new string, d *schema.ResourceData) bool { - // ignore diffs where the user hasn't specified a kms_key_id but the bucket has a default KMS key configured - if new == "" && d.Get("server_side_encryption") == types.ServerSideEncryptionAwsKms { - return true - } - return false - }, }, "metadata": { Type: schema.TypeMap, @@ -302,6 +294,9 @@ func resourceObjectRead(ctx context.Context, d *schema.ResourceData, meta interf d.Set(names.AttrContentType, output.ContentType) // See https://forums.aws.amazon.com/thread.jspa?threadID=44003 d.Set("etag", strings.Trim(aws.ToString(output.ETag), `"`)) + if output.SSEKMSKeyId != nil { // nosemgrep:ci.helper-schema-ResourceData-Set-extraneous-nil-check + d.Set(names.AttrKMSKeyID, output.SSEKMSKeyId) + } d.Set("metadata", output.Metadata) d.Set("object_lock_legal_hold_status", output.ObjectLockLegalHoldStatus) d.Set("object_lock_mode", output.ObjectLockMode) @@ -316,10 +311,6 @@ func resourceObjectRead(ctx context.Context, d *schema.ResourceData, meta interf d.Set("version_id", output.VersionId) d.Set("website_redirect", output.WebsiteRedirectLocation) - if err := setObjectKMSKeyID(ctx, meta, d, aws.ToString(output.SSEKMSKeyId)); err != nil { - return sdkdiag.AppendFromErr(diags, err) - } - return diags } @@ -601,25 +592,6 @@ func resourceObjectUpload(ctx context.Context, d *schema.ResourceData, meta inte return append(diags, resourceObjectRead(ctx, d, meta)...) } -func setObjectKMSKeyID(ctx context.Context, meta interface{}, d *schema.ResourceData, sseKMSKeyID string) error { - // Only set non-default KMS key ID (one that doesn't match default). - if sseKMSKeyID != "" { - // Read S3 KMS default master key. - keyMetadata, err := tfkms.FindKeyByID(ctx, meta.(*conns.AWSClient).KMSClient(ctx), defaultKMSKeyAlias) - - if err != nil { - return fmt.Errorf("reading default S3 KMS key (%s): %s", defaultKMSKeyAlias, err) - } - - if sseKMSKeyID != aws.ToString(keyMetadata.Arn) { - log.Printf("[DEBUG] S3 object is encrypted using a non-default KMS key: %s", sseKMSKeyID) - d.Set(names.AttrKMSKeyID, sseKMSKeyID) - } - } - - return nil -} - func validateMetadataIsLowerCase(v interface{}, k string) (ws []string, errors []error) { value := v.(map[string]interface{}) diff --git a/internal/service/s3/object_copy.go b/internal/service/s3/object_copy.go index a7e739f336f6..b6fedba198d6 100644 --- a/internal/service/s3/object_copy.go +++ b/internal/service/s3/object_copy.go @@ -396,10 +396,6 @@ func resourceObjectCopyRead(ctx context.Context, d *schema.ResourceData, meta in d.Set("version_id", output.VersionId) d.Set("website_redirect", output.WebsiteRedirectLocation) - if err := setObjectKMSKeyID(ctx, meta, d, aws.ToString(output.SSEKMSKeyId)); err != nil { - return sdkdiag.AppendFromErr(diags, err) - } - return diags } diff --git a/internal/service/s3/object_test.go b/internal/service/s3/object_test.go index a56a6ed35cbc..0fda26be0a03 100644 --- a/internal/service/s3/object_test.go +++ b/internal/service/s3/object_test.go @@ -23,7 +23,11 @@ import ( "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" sdkacctest "github.com/hashicorp/terraform-plugin-testing/helper/acctest" "github.com/hashicorp/terraform-plugin-testing/helper/resource" + "github.com/hashicorp/terraform-plugin-testing/knownvalue" + "github.com/hashicorp/terraform-plugin-testing/plancheck" + "github.com/hashicorp/terraform-plugin-testing/statecheck" "github.com/hashicorp/terraform-plugin-testing/terraform" + "github.com/hashicorp/terraform-plugin-testing/tfjsonpath" "github.com/hashicorp/terraform-provider-aws/internal/acctest" "github.com/hashicorp/terraform-provider-aws/internal/conns" tfs3 "github.com/hashicorp/terraform-provider-aws/internal/service/s3" @@ -1883,6 +1887,130 @@ func TestAccS3Object_optInRegion(t *testing.T) { }) } +func TestAccS3Object_defaultKMS(t *testing.T) { + ctx := acctest.Context(t) + var obj s3.GetObjectOutput + resourceName := "aws_s3_object.object" + rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(ctx, t) }, + ErrorCheck: acctest.ErrorCheck(t, names.S3ServiceID), + ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, + CheckDestroy: testAccCheckObjectDestroy(ctx), + Steps: []resource.TestStep{ + { + Config: testAccObjectConfig_defaultKMS(rName, "stuff"), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction(resourceName, plancheck.ResourceActionCreate), + }, + }, + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckObjectExists(ctx, resourceName, &obj), + testAccCheckObjectBody(&obj, "stuff"), + ), + ConfigStateChecks: []statecheck.StateCheck{ + statecheck.ExpectKnownValue(resourceName, tfjsonpath.New(names.AttrKMSKeyID), knownvalue.NotNull()), + }, + }, + }, + }) +} + +func TestAccS3Object_defaultKMSUpgrade(t *testing.T) { + ctx := acctest.Context(t) + var obj s3.GetObjectOutput + resourceName := "aws_s3_object.object" + rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(ctx, t) }, + ErrorCheck: acctest.ErrorCheck(t, names.S3ServiceID), + CheckDestroy: testAccCheckObjectDestroy(ctx), + Steps: []resource.TestStep{ + { + ExternalProviders: map[string]resource.ExternalProvider{ + "aws": { + Source: "hashicorp/aws", + VersionConstraint: "5.72.1", + }, + }, + Config: testAccObjectConfig_defaultKMS(rName, "stuff"), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction(resourceName, plancheck.ResourceActionCreate), + }, + }, + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckObjectExists(ctx, resourceName, &obj), + testAccCheckObjectBody(&obj, "stuff"), + ), + ConfigStateChecks: []statecheck.StateCheck{ + statecheck.ExpectKnownValue(resourceName, tfjsonpath.New(names.AttrKMSKeyID), knownvalue.NotNull()), + }, + }, + { + ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, + Config: testAccObjectConfig_defaultKMS(rName, "stuff"), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction(resourceName, plancheck.ResourceActionNoop), + }, + }, + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckObjectExists(ctx, resourceName, &obj), + testAccCheckObjectBody(&obj, "stuff"), + ), + }, + }, + }) +} + +func TestAccS3Object_basicUpgrade(t *testing.T) { + ctx := acctest.Context(t) + var obj s3.GetObjectOutput + resourceName := "aws_s3_object.object" + rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(ctx, t) }, + ErrorCheck: acctest.ErrorCheck(t, names.S3ServiceID), + CheckDestroy: testAccCheckObjectDestroy(ctx), + Steps: []resource.TestStep{ + { + ExternalProviders: map[string]resource.ExternalProvider{ + "aws": { + Source: "hashicorp/aws", + VersionConstraint: "5.72.1", + }, + }, + Config: testAccObjectConfig_basic(rName), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction(resourceName, plancheck.ResourceActionCreate), + }, + }, + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckObjectExists(ctx, resourceName, &obj), + ), + }, + { + ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, + Config: testAccObjectConfig_basic(rName), + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction(resourceName, plancheck.ResourceActionNoop), + }, + }, + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckObjectExists(ctx, resourceName, &obj), + ), + }, + }, + }) +} + func testAccCheckObjectVersionIDDiffers(first, second *s3.GetObjectOutput) resource.TestCheckFunc { return func(s *terraform.State) error { if aws.ToString(first.VersionId) == aws.ToString(second.VersionId) { @@ -3040,3 +3168,22 @@ resource "aws_s3_object" "object" { } `, rName)) } + +func testAccObjectConfig_defaultKMS(rName string, content string) string { + return fmt.Sprintf(` +data "aws_kms_key" "test" { + key_id = "alias/aws/s3" +} + +resource "aws_s3_bucket" "test" { + bucket = %[1]q +} + +resource "aws_s3_object" "object" { + bucket = aws_s3_bucket.test.bucket + key = "test-key" + content = %[2]q + kms_key_id = data.aws_kms_key.test.arn +} +`, rName, content) +}