serverless-application-model icon indicating copy to clipboard operation
serverless-application-model copied to clipboard

Published 20 hours ago • verified publisher icon aws

Added kms:GenerateDataKey action to KMSEncryptPolicy policy

Open elmaimbo opened this issue 1 year ago • 6 comments

Issue

#1796 KMSEncryptPolicy cannot be used to encrypt

Description of changes

The KMSEncryptPolicy policy lacks the permission required to encrypt data using a data key. This trivial fix adds the missing permission.

Description of how you validated changes

Copied and pasted the change into the policy on AWS (that had been deployed using SAM), and confirmed that this allowed the encrypt function to work.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

elmaimbo avatar Sep 30 '24 01:09 elmaimbo

I see in the issue comments that someone requested these Actions

kms:Encrypt
kms:ReEncrypt*
kms:GenerateDataKey*
kms:DescribeKey

Any reason why we shouldn't include kms:ReEncrypt* and kms:DescribeKey here?

aaythapa avatar Oct 02 '24 18:10 aaythapa

I see in the issue comments that someone requested these Actions

kms:Encrypt
kms:ReEncrypt*
kms:GenerateDataKey*
kms:DescribeKey

Any reason why we shouldn't include kms:ReEncrypt* and kms:DescribeKey here?

TBH I wasn't sure what these additional permissions were used for, and rather than adding permissions that might not be needed, I was just trying to resolve my own simple use-case of encrypting a value (i.e. principle of least privilege).

I will change my specification of "kms:GenerateDataKey" to "kms:GenerateDataKey*", to allow the use of all of these encryption functions:

  • kms:GenerateDataKey
  • kms:GenerateDataKeyWithoutPlaintext
  • kms:GenerateDataKeyPair
  • kms:GenerateDataKeyPairWithoutPlaintext

But regarding the others: I think the pertinent question is whether it is more appropriate to include kms:ReEncryptFrom, ReEncryptTo and kms:DescribeKey in the KMSEncryptPolicy definition, or create separate policy definitions for those? (I don't believe I'm qualified to answer that question myself, and TBH I have no need of these additional permissions.)

elmaimbo avatar Oct 03 '24 00:10 elmaimbo

I noticed that almost all other policies listed actions explicitly, rather than using "*" (wildcard), so I've done the same in the latest commit.

elmaimbo avatar Oct 03 '24 07:10 elmaimbo

I think the pertinent question is whether it is more appropriate to include kms:ReEncryptFrom, ReEncryptTo and kms:DescribeKey in the KMSEncryptPolicy definition, or create separate policy definitions for those? (I don't believe I'm qualified to answer that question myself, and TBH I have no need of these additional permissions.)

I can take a look at this

aaythapa avatar Oct 04 '24 23:10 aaythapa

We've reviewed the PR internally and the actions that were added are all GET operations so nothing sensitive.

We're good to merge after the change is made backwards compatible, I explained here why this change would break compatibility.

@elmaimbo

aaythapa avatar Oct 11 '24 16:10 aaythapa

We've reviewed the PR internally and the actions that were added are all GET operations so nothing sensitive.

We're good to merge after the change is made backwards compatible, I explained here why this change would break compatibility.

@elmaimbo

This is now complete in the latest commit.

Please let me know if anything is still outstanding?

Thanks, Nick.

elmaimbo avatar Oct 13 '24 02:10 elmaimbo

@aaythapa can you please fix the status of this pull request. I seem to have inadvertently marked it for review. Sorry about that! :-(

elmaimbo avatar Oct 24 '24 19:10 elmaimbo