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.5k stars 3.84k forks source link

aws_secretsmanager: Make cross-account secret sharing easier with KMS Key alias #28284

Open pergardebrink opened 9 months ago

pergardebrink commented 9 months ago

Describe the feature

Short description

I'd like a way to use secret.grantRead(role) that would automatically generate the appropriate policy using the alias condition for cross-account secret sharing**

How it works without this feature

Let's say I have a secret in AccountA (111111111111) and what to share that with AccountB (222222222222).

Account A CDK Stack

const encryptionKey = new kms.Key(this, "Key", {
    alias = "mysecretkeyalias"
});

const secret = new secretsmanager.Secret(this, "Secret", {
    encryptionKey: encryptionKey,
    secretName: "MySecret"
});

const accountB = new iam.AccountPrincipal("222222222222");

secret.grantRead(accountB);

To consume this secret from AccountB, I'd need to grant access to both the secret and the kms key, but I'd like to use the alias instead of the key name for many reasons (easier configuration for multi-region for example as I wouldn't have to pass different key arns), which currently have to look something like this:

Account B CDK Stack

const role = new iam.Role(this, "ConsumerRole", {
    // Setup the role
});

const secret = secretsmanager.Secret.FromSecretAttributes(this, "Secret", {
    secretPartialArn: "arn:aws:secretsmanager:us-east-1:11111111111:secret:MySecret"
});

secret.grantRead(role);

role.addToPrincipalPolicy({
    resources: ["arn:aws:kms:us-east-1:11111111111:key/*"],
    actions: ["kms:decrypt"],
    conditions: {
        "ForAnyValue:StringEquals": {
            "kms:ResourceAliases": "alias/mysecretkeyalias",
        },
    },
}):

Use Case

When it's not the same team/people/company owning the source AWS account and/or the secret/key as the consuming account, it would be convenient if it's possible to reference the key by alias to ease configuration as well as making it possible to switch kms key for the secret without having to change on the consuming side.

For multi-region deployments, it's also better as I'd not have to specify multiple secrets + kms keys, but could instead use the secret partialarn and the kms key alias, which could be the same across all regions.

Proposed Solution

It would be nice if there was some way to specify the alias and that it would produce a policy with a kms key alias condition

const role = new iam.Role(this, "ConsumerRole", {
    // Setup the role
});

const kmsAlias = kms.Alias.FromAliasArn(this, "Alias", "arn:aws:kms:us-east-1:111111111111:alias/mysecretkeyalias");

const secret = secretsmanager.Secret.FromSecretAttributes(this, "Secret", {
    encryptionKey: kmsAlias,
    secretPartialArn: "arn:aws:secretsmanager:us-east-1:11111111111:secret:MySecret"
});

secret.grantRead(role);

Expected policy:

{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Effect": "Allow",
      "Action": [
        "kms:Decrypt",
      ],
      "Resource": [
        "arn:aws:kms:us-east-1:11111111111:key/*",
      ],
      "Condition": {
        "ForAnyValue:StringLike": {
          "kms:ResourceAliases": "alias/mysecretkeyalias"
        }
      }
    }
  ]
}

Other Information

I assume there could be side-effects with my proposed solution that could cause this feature to not be possible or a good solution

Acknowledgements

CDK version used

2.114.1

Environment details (OS name and version, etc.)

Windows 11

pergardebrink commented 9 months ago

I realize that this is similar to to this issue https://github.com/aws/aws-cdk/issues/23545

I see benefits of only having to specify the accountId as opposed to the full alias arn, but then I assume that the region needs to be inferred as it might not always be the intention to give access to all keys with the same alias in all regions in the other account (least privilege). But there should also be a way to specify cross region access as well if so.

I might be able to create a PR if the api from either mine or the linked issue (or both) is considered good.

pahud commented 9 months ago

Thanks for the feedback and we appreciate your pull requests.

pergardebrink commented 9 months ago

@pahud thanks. I'm willing to create a pull request for this, but I have some questions on decisions from the past in the CDK constructs for Alias, to make sure I don't break things, or that I break things in a good way (as I think there's going to have to be breaking changes here).

My main thoughts are about the following comment that suggest that it was intentional not to implement the grant* methods for an alias that is not created by the stack (an imported alias). Or at least that it was not given priority to implement it? https://github.com/aws/aws-cdk/blob/14e5e50e9e4a23ab7db5bbccf874e6a5fe731e34/packages/aws-cdk-lib/aws-kms/lib/alias.ts#L171

My proposal for Secret:

If the imported secret with FromSecretAttributes is cross-account, it won't add the ViaService condition, but instead defer grant the KMS permission to the IKey grant* functions (see below)

My proposal for KMS Key Alias:

  1. Add the following overload to Alias:

    kms.Alias.fromAliasArn(scope: Construct, id: string, aliasArn: string): IAlias
    • Any call to grant* methods on the returned Alias will add to the principal policy (since we do not have access to the key we cannot add resource policy)
    • The policy will look like the "Expected policy" in my first post in this issue and use the kms:ResourceAliases condition with a resource based on the arn of the alias (same account and region)
  2. The kms.Alias.fromAliasName will start implementing the same grant* logic as the fromAliasArn in 1. (breaking change)

  3. Inspired by the issue https://github.com/aws/aws-cdk/issues/23545, kms.Alias.fromAliasAttributes will have more options in the AliasAttributes (comments condensed here for readability);

    // Properties of a reference to an existing KMS Alias
    export interface AliasAttributes {
    // Specifies the alias name. This value must begin with alias/ followed by a name (i.e. alias/ExampleAlias)
    readonly aliasName: string;
    // Specifies the account where the alias is defined.
    readonly account: string;
    // Specifies the region where the alias is defined.
    readonly region: string;
    // The customer master key (CMK) to which the Alias refers (optional)
    readonly aliasTargetKey: IKey;
    }

    For 3. I think there's some things that would need to be addressed:

    • If aliasTargetKey is specified, all grant methods would use the current behaviors, and delegate the grant call to the actual KMS key and not the alias
    • if aliasTargetKey is not specified, all grant* methods would be using the same logic as 1. and 2. to use the alias condition instead

I can start in parallell to see if I can create a draft PR based on the above, or if opinions and concerns appear before that I can adapt.

mtlaurentys commented 7 months ago

@pergardebrink Instead of the proposed, would you consider:

export interface AliasAttributes {
  readonly aliasName: string;
  readonly aliasTargetKey?: IKey;
  readonly externalAliasOwner?: {
    readonly partition: string;
    readonly region: string;
    readonly account: string;
  }
}

For the points you raised on 3, I think your suggestions make sense. That said, I don't know why aliasTargetKey is used in the first place, so take that lightly

I'm currently needing this feature. If you don't plan to take this in the near feature (2-3 weeks), could I work on this?