Skip to content
New issue

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

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

Already on GitHub? # to your account

aws-kms: Key.grant* methods misidentify Key region when enclosing Stack is different region; leads to wildcard resource ARNs #29308

Closed
ryderben opened this issue Feb 29, 2024 · 3 comments · Fixed by #29315
Assignees
Labels
@aws-cdk/aws-kms Related to AWS Key Management bug This issue is a bug. effort/medium Medium work item – several days of effort p1

Comments

@ryderben
Copy link

Describe the bug

This is a possible cause of #23991.

This problem is grant() determines the region of a Key using Stack.of(key).region, however the enclosing Stack's region may differ to that of the actual resource.

private isGranteeFromAnotherRegion(grantee: iam.IGrantable): boolean {
if (!iam.principalIsOwnedResource(grantee.grantPrincipal)) {
return false;
}
const bucketStack = Stack.of(this);
const identityStack = Stack.of(grantee.grantPrincipal);
return bucketStack.region !== identityStack.region;
}

One example of where these differ is when the Key was imported as a replicaKey within a Global DynamoDB TableV2 - in these contexts, resources under several regions must be scoped under a single-region Stack.

import * as cdk from "aws-cdk-lib";
import * as kms from "aws-cdk-lib/aws-kms";
import * as ddb from "aws-cdk-lib/aws-dynamodb";
import * as iam from "aws-cdk-lib/aws-iam";

const account = "123412341234";
const primaryRegion = "us-west-2";
const replicaRegion = "eu-north-1";
const kmsKeys = {
    "us-west-2":
        "arn:aws:kms:us-west-2:123412341234:key/5a84f9d3-7a10-4b37-afe7-95a496db8b79",
    "eu-north-1":
        "arn:aws:kms:eu-north-1:123412341234:key/e3ab59e5-3dc3-4bc4-9c3f-c790231d2287",
};

const app = new cdk.App();

const dbStack = new cdk.Stack(app, "DbStack", {
    env: { account, region: primaryRegion },
});
const table = new ddb.TableV2(dbStack, "Table", {
    partitionKey: { name: "id", type: ddb.AttributeType.STRING },
    replicas: [{ region: replicaRegion }],
    encryption: ddb.TableEncryptionV2.customerManagedKey(
        kms.Key.fromKeyArn(dbStack, "PrimaryTableKey", kmsKeys["us-west-2"]),
        { "eu-north-1": kmsKeys["eu-north-1"] },
    ),
});

for (const region of [primaryRegion, replicaRegion]) {
    const iamStack = new cdk.Stack(app, `IamStack-${region}`, {
        env: { region, account },
    });
    const role = new iam.Role(iamStack, "Role", {
        assumedBy: new iam.AccountPrincipal("123456789012"),
    });
    const replica = table.replica(region);
    replica.grantReadWriteData(role);
    // or alternatively: 
    // replica.encryptionKey.grantEncryptDecrypt(role);
}

In the below example, we attempt to grant an eu-north-1 key (via it's attached table replica) to an eu-north-1 IAM role. However, since the former was managed under a us-west-2 stack, we see the behaviour for when regions mismatch.

{
  "Action": [
    "kms:Decrypt",
    "kms:DescribeKey",
    "kms:Encrypt",
    "kms:GenerateDataKey*",
    "kms:ReEncrypt*"
  ],
  "Effect": "Allow",
  "Resource": "*"
}

Expected Behavior

For the above example, we expect the Resource in the statement to resolve to one ARN:

{
  "Action": [
    "kms:Decrypt",
    "kms:DescribeKey",
    "kms:Encrypt",
    "kms:GenerateDataKey*",
    "kms:ReEncrypt*"
  ],
  "Effect": "Allow",
  "Resource": "arn:aws:kms:eu-north-1:123412341234:key/e3ab59e5-3dc3-4bc4-9c3f-c790231d2287"
}

Current Behavior

Instead, the Resource field is a wild-card "*" - this is overly permissive.

{
  "Action": [
    "kms:Decrypt",
    "kms:DescribeKey",
    "kms:Encrypt",
    "kms:GenerateDataKey*",
    "kms:ReEncrypt*"
  ],
  "Effect": "Allow",
  "Resource": "*"
}

Reproduction Steps

A short, less practical reproduction.

import * as cdk from "aws-cdk-lib";
import * as kms from "aws-cdk-lib/aws-kms";
import * as iam from "aws-cdk-lib/aws-iam";

const app = new cdk.App();

const keyStack = new cdk.Stack(app, "KeyStack", {
    env: { account: "123412341234", region: "us-east-1" },
});
const key = kms.Key.fromKeyArn(
    keyStack,
    "Key",
    "arn:aws:kms:eu-north-1:123412341234:key/e3ab59e5-3dc3-4bc4-9c3f-c790231d2287",
);

const roleStack = new cdk.Stack(app, "RoleStack", {
    env: { account: "123412341234", region: "eu-north-1" },
});
const role = new iam.Role(roleStack, "Role", {
    assumedBy: new iam.AccountPrincipal("123456789012"),
});
key.grantEncryptDecrypt(role);

Possible Solution

Resources provided an env property which can be used instead of Stack.region: https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_apigateway.IResource.html#env .

Key.fromKeyArn already sets this appropriately:

environmentFromArn: keyArn,

Additional Information/Context

No response

CDK CLI Version

2.106.0

Framework Version

No response

Node.js Version

v18.16.0

OS

Amazon Linux 2

Language

TypeScript

Language Version

No response

Other information

No response

@ryderben ryderben added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Feb 29, 2024
@github-actions github-actions bot added the @aws-cdk/aws-kms Related to AWS Key Management label Feb 29, 2024
@pahud pahud added p1 and removed needs-triage This issue or PR still needs to be triaged. labels Feb 29, 2024
@pahud
Copy link
Contributor

pahud commented Feb 29, 2024

Thank you for the detailed report. We'll look into this for further investigation.

@pahud pahud added the effort/medium Medium work item – several days of effort label Feb 29, 2024
@GavinZZ
Copy link
Contributor

GavinZZ commented Feb 29, 2024

Thanks for the report. I can reproduce this issue and will work on a PR to fix it.

@GavinZZ GavinZZ self-assigned this Feb 29, 2024
GavinZZ added a commit that referenced this issue Mar 14, 2024
…ck is different region (#29315)

### Issue # (if applicable)

Closes #29308

### Reason for this change

This problem is grant() determines the region of a Key using
Stack.of(key).region, however the enclosing Stack's region may differ to
that of the actual resource. When this happens, the IAM policy generated
allows a `*` resource which is against the least privilege rule.

### Description of changes

KMS key already has `env` value on account and region, use this first.
If not exist, use stack account and region.

### Description of how you validated changes

New unit test

### Checklist
- [X] My code adheres to the [CONTRIBUTING
GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and
[DESIGN
GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

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

---------

Co-authored-by: paulhcsun <47882901+paulhcsun@users.noreply.github.com>
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
@aws-cdk/aws-kms Related to AWS Key Management bug This issue is a bug. effort/medium Medium work item – several days of effort p1
Projects
None yet
3 participants