Added kms:GenerateDataKey action to KMSEncryptPolicy policy

[elmaimbo]

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.

[aaythapa]

Thank you for contributing! Will do a review of this internally

[aaythapa]

This will break backwards compatibility for people using KMSEncryptPolicy unfortunately. We can create a new policy called KMSEncryptPolicy_v2 with the new permissions. Example PR you can follow: #2929

[elmaimbo]

My change is backward compatible because I'm only adding a new permission, not removing anything.

[aaythapa]

Will double check this with another team member but I believe adding a new permissions will also be backwards incompatible. If a user has a SAM Function with KMSEncryptPolicy attached then the transformed template will have role with

PolicyDocument": {
       "Statement": [
        {
         "Action": "kms:Encrypt",
         "Effect": "Allow",
         "Resource": {
          ...
         }

with your change the transformed template will be

PolicyDocument": {
       "Statement": [
        {
         "Action": [
          "kms:Encrypt",
          "kms:GenerateDataKey"
         ],
         "Effect": "Allow",
         "Resource": {
          ...
         }

The user didn't make any changes to their SAM template but the transformed CFN template changed which causes re-deployments which we consider backwards incompatible

[aaythapa]

Can we add some unit tests for this? You can follow this guide: https://github.com/aws/serverless-application-model/blob/develop/DEVELOPMENT_GUIDE.md#transform-tests

[elmaimbo]

I presume (hope) this isn't required if I'm modifying an existing policy? I.e. Based on earlier assertion that the change is backward compatible?

[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?

[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?

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]

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

[aaythapa]

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]

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

[elmaimbo]

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.