search

aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.55k stars 3.87k forks source link

KMS: Key not granting permissions to admins anymore #31528

Open icelava opened 5 days ago

icelava commented 5 days ago

Describe the bug

Previously with CDK 2.133, we were able to define additional IAM user ARNs for reference and adding them to a Key Admins array. They ended up in the key policy as expected.

var admins = this._platConfig.UploadsBucket.Admins.Select(u => User.FromUserArn(this, u, u));
var keyName = $"appuploads-key";
var uploadsKey = new Key(this, keyName, new KeyProps
    {
        Alias = keyName,
        Description = $"appuploads key.",
        Enabled = true,
        Admins = admins.ToArray()
    });

Now with 2.159.1 (build c66f4e3) it appears to ignore the array thus removing the previously defined users in the key policy. Even though the admins IEnumerable has IUser objects.

[~] AWS::KMS::Key appuploads-key appuploadskeyC9108645
 └─ [~] KeyPolicy
     └─ [~] .Statement:
         └─ @@ -6,28 +6,5 @@
            [ ]       "AWS": "arn:aws:iam::ACCOUNTID:root"
            [ ]     },
            [ ]     "Resource": "*"
            [-]   },
            [-]   {
            [-]     "Action": [
            [-]       "kms:CancelKeyDeletion",
            [-]       "kms:Create*",
            [-]       "kms:Delete*",
            [-]       "kms:Describe*",
            [-]       "kms:Disable*",
            [-]       "kms:Enable*",
            [-]       "kms:Get*",
            [-]       "kms:List*",
            [-]       "kms:Put*",
            [-]       "kms:Revoke*",
            [-]       "kms:ScheduleKeyDeletion",
            [-]       "kms:TagResource",
            [-]       "kms:UntagResource",
            [-]       "kms:Update*"
            [-]     ],
            [-]     "Effect": "Allow",
            [-]     "Principal": {
            [-]       "AWS": "arn:aws:iam::ACCOUNTID:user/old.user"
            [-]     },
            [-]     "Resource": "*"
            [ ]   }
            [ ] ]

Regression Issue

Last Known Working CDK Version

1.133

Expected Behavior

The referenced IUsers added to Key.Admins array end up in key policy.

Current Behavior

Referenced IUsers removed from key policy.

Reproduction Steps

  1. Update CDK CLI to 2.159.
  2. cdk diff existing stack with KMS Key with additional IAM users as Admins.

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.159.1 (build c66f4e3)

Framework Version

No response

Node.js Version

v22.3.0

OS

Windows 11 23H2

Language

.NET

Language Version

C# 10

Other information

No response

pahud commented 5 days ago

investigating

pahud commented 5 days ago

Yes I noticed the changes between 2.141.0 and 2.142.0 and this could due to this fix https://github.com/aws/aws-cdk/pull/30023

Given the code below:

    const principal = iam.User.fromUserArn(this, 'User', `arn:aws:iam::${Stack.of(this).account}:user/foo`);
    new kms.Key(this, 'DummyKey', {
      admins: [principal]
    })

In 2.141.0, this would create both IAM Policy for the User as well as the KMS key policy

Key Policy added:

 {
            "Effect": "Allow",
            "Principal": {
                "AWS": "arn:aws:iam::ACCOUNT_ID:user/foo"
            },
            "Action": [
                "kms:CancelKeyDeletion",
                "kms:Create*",
                "kms:Delete*",
                "kms:Describe*",
                "kms:Disable*",
                "kms:Enable*",
                "kms:Get*",
                "kms:List*",
                "kms:Put*",
                "kms:Revoke*",
                "kms:ScheduleKeyDeletion",
                "kms:TagResource",
                "kms:UntagResource",
                "kms:Update*"
            ],
            "Resource": "*"
        }

In 2.142.0, this would ONLY add the IAM policy to the User. No additional key policy would be added.

Please note this is consist with the behavior of IAM Role with KMS Key. For example if I import an IAM role like this and deploy in 2.141.0

    const principal = iam.Role.fromRoleArn(this, 'Role', `arn:aws:iam::${Stack.of(this).account}:role/foo`);
    new kms.Key(this, 'DummyKey', {
      admins: [principal]
    })

You will notice no key policy would be added. Only IAM policy for the role would be added. If you upgrade from 2.141.0 to 2.142.0 or even the latest 2.159.1 and run cdk diff. There would be no differences.

So I am pretty sure this is a consistent behavior with what is happening for the kms key with an IAM role.

I will continue investigate and update here.

pahud commented 5 days ago

Hi @icelava 

var admins = this._platConfig.UploadsBucket.Admins.Select(u => User.FromUserArn(this, u, u));

Can you let me know, if the UserArn is from the same account of the deploying stack or different account?

pahud commented 4 days ago

OK I think I found the root cause.

Because of this bug in 2.141.0, which was fixed in https://github.com/aws/aws-cdk/pull/30023, when we import User from same account like

    const userPrincipal = iam.User.fromUserArn(this, 'User', `arn:aws:iam::123456789012:user/foo`);

CDK would always use Aws.AccountId to represent the grantee principal account and will always run this code logic

https://github.com/aws/aws-cdk/blob/3d1c06e5bf1fd128e81d6cf850130d4edc09ab02/packages/aws-cdk-lib/aws-kms/lib/key.ts#L184-L186

If we check addToPrincipalOrResource(), it first addToPrincipal(), which will succeed but sameAccount would be false here:

https://github.com/aws/aws-cdk/blob/5e95ba2b7a24b2598cf00890e1e7d569914f27c9/packages/aws-cdk-lib/aws-iam/lib/grant.ts#L141-L143

So it moves on to create resource policy here:

https://github.com/aws/aws-cdk/blob/5e95ba2b7a24b2598cf00890e1e7d569914f27c9/packages/aws-cdk-lib/aws-iam/lib/grant.ts#L145-L151

This explains why in 2.141.0 it would create both principal policy as well as resource policy.

In 2.142.0, because of the fix, sameAccount would be true so it won't move on to create the resource policy. This explains why it would be removed when upgrading from 2.141.0 to 2.142.0

icelava commented 4 days ago

Can you let me know, if the UserArn is from the same account of the deploying stack or different account?

Yes the admin user is from the same AWS account.

icelava commented 4 days ago

So the key policy is redundant since the IAM user personal policy permits it?

UPDATE

I checked said user and it does not have any policy statement to access said key. Neither did cdk diff attempt to add new policy for the user.

UPDATE

Think I know what's happening. When the KMS key adds admins, the resultant CloudFormation template assigns the seemingly random name arnawsiamACCOUNTIDuserUSERNAMEPolicyBAA0AB37 for the inline policy to said user.

Thing is, when we have multiple application stacks using the same code patterns to assign admins to keys, the same user gets the same policy name and gets overwritten over and over. While the original stack doesn't know anything has happened to the inline policy, it has in fact "drifted" to match the policy of the last deploy stack. That's why the original key ARN that was to be permitted for has gone missing.

How the random identifier suffix is generated is problematic.

UPDATE

To break this cycle of repitition, looks like a more unique name for the CDK construct has to be fashioned.

var admins = this._platConfig.UploadsBucket.Admins.Select(u => User.FromUserArn(this, $"{_platConfig.ProductName}-appuploads-key-{u}-{_platConfig.Environment}", u));

pahud commented 4 days ago

When admins is defined, the Key construct essentially runs grantAdmin() on the principals which runs grant() on them with admin permissions. The whole implementation is here.

https://github.com/aws/aws-cdk/blob/218331bd1f87897bf6cbd42a2059d570b59bfb3f/packages/aws-cdk-lib/aws-kms/lib/key.ts#L808

Basically, when CDK grants resource access permission to an IAM principal, if they both belong to the same account with the deploying stack, which I believe is your case, CDK would run addToPrincipalOrResource() which will first add an IAM Policy for that identity principal and won't add resource-based policy if the previous add is successful. That explains why you'd see identity-based policy only but no resource-based policy.

And you are right, if you have multiple Keys granting to the same principal, you would end up with multiple AWS::IAM::Policy resources being associated with a single IAM principal and that might not be something you expect. In this case, I won't recommend using admins prop, instead, you probably want to create a single iam.Policy for that IAM principal and attach all necessary statements to that policy. You have full control about all the required policy for that.

icelava commented 3 days ago

That will be difficult. Each application stack is not aware of each other and being self-contained assigns permissions to their respective administrators/developers. Anyway making the IUser construct ID unique across all stacks lets them maintain their own inine policies.

(Although the 2048 character limit on all inline policies is very restrictive.)