Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

aws_s3_object (et al.): Remove kms:DescribeKey on default S3 KMS key #39782

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open
11 changes: 11 additions & 0 deletions .changelog/39782.txt
Original file line number Diff line number Diff line change
@@ -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
```
1 change: 0 additions & 1 deletion internal/service/kms/exports.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ var (
DiffSuppressKeyOrAlias = diffSuppressKeyOrAlias
FindAliasByName = findAliasByName
FindDefaultKeyARNForService = findDefaultKeyARNForService
FindKeyByID = findKeyByID
ValidateKey = validateKey
ValidateKeyOrAlias = validateKeyOrAlias
)
1 change: 1 addition & 0 deletions internal/service/kms/exports_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ var (
AliasNamePrefix = aliasNamePrefix
FindCustomKeyStoreByID = findCustomKeyStoreByID
FindGrantByTwoPartKey = findGrantByTwoPartKey
FindKeyByID = findKeyByID
FindKeyPolicyByTwoPartKey = findKeyPolicyByTwoPartKey
GrantParseResourceID = grantParseResourceID
KeyARNOrIDEqual = keyARNOrIDEqual
Expand Down
34 changes: 3 additions & 31 deletions internal/service/s3/bucket_object.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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 {
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)
Expand All @@ -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
}

Expand Down Expand Up @@ -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")
Expand Down
2 changes: 0 additions & 2 deletions internal/service/s3/enum.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@ import (
"github.com/aws/aws-sdk-go-v2/aws/arn"
)

const defaultKMSKeyAlias = "alias/aws/s3"

type bucketNameType int

const (
Expand Down
34 changes: 3 additions & 31 deletions internal/service/s3/object.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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 {
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)
Expand All @@ -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
}

Expand Down Expand Up @@ -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{})

Expand Down
4 changes: 0 additions & 4 deletions internal/service/s3/object_copy.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note this call above:

d.Set(names.AttrKMSKeyID, output.SSEKMSKeyId)

return sdkdiag.AppendFromErr(diags, err)
}

return diags
}

Expand Down
147 changes: 147 additions & 0 deletions internal/service/s3/object_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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)
}
Loading