Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

KMS - Ability to import an AWS managed key by its alias #5953

Closed
Dzhuneyt opened this issue Jan 24, 2020 · 7 comments · Fixed by #8299
Closed

KMS - Ability to import an AWS managed key by its alias #5953

Dzhuneyt opened this issue Jan 24, 2020 · 7 comments · Fixed by #8299
Assignees
Labels
@aws-cdk/aws-kms Related to AWS Key Management effort/medium Medium work item – several days of effort feature-request A feature should be added or improved.

Comments

@Dzhuneyt
Copy link
Contributor

It would be nice to be able to reuse existing/default keys that come with every AWS account, without having to hardcode their full ARNs, e.g. by providing just their alias.

Currently, it's only possible through:
const key = kms.Key.fromKeyArn(this, "default", "arn:aws:kms:us-east-1:xxxxx:key/390a2c1f-xxxx-4abd-xxxx-c17e04362ba9")

It would be nice to have something like:
const key = kms.Key.fromKeyAlias(this, "default", "alias:aws/s3")

Like it is currently possible to be done using Terraform:
https://www.terraform.io/docs/providers/aws/d/kms_key.html

Use Case

I am currently creating a CloudTrail that sends log files to an S3 bucket. The CloudTrail has the option for "encrypting logs using KMS". However, in order to pass it the default S3 key that AWS provided me, I need to be able to find/import it. The only possibility currently, is the following method, which is far from an ideal solution because it requires me to hardcode the key ID in the ARN (a highly dynamic string). This makes the CDK stack less reusable and portable across regions and AWS accounts (another account will have a different key ID for the default S3 key for example).

Proposed Solution

A new method like:
const key = kms.Key.fromAlias(this, "default", "alias:kms/s3")

Other

Current code:

export class Cloudtrail extends cdk.Stack {
    constructor(scope: cdk.Construct, id: string, props?: cdk.StackProps) {
        super(scope, id, props);

        const key = kms.Key.fromKeyArn(this, "default", "arn:aws:kms:us-east-1:xxxxx:key/390a2c1f-xxxx-4abd-xxxx-c17e04362ba9")

        const trail = new cloudtrail.Trail(this, 'CloudTrail', {
            sendToCloudWatchLogs: true,
            includeGlobalServiceEvents: true,
            kmsKey: key
        });
    }
}

This is a 🚀 Feature Request

@Dzhuneyt Dzhuneyt added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Jan 24, 2020
@SomayaB SomayaB added the @aws-cdk/aws-kms Related to AWS Key Management label Jan 28, 2020
@skinny85 skinny85 added effort/medium Medium work item – several days of effort and removed needs-triage This issue or PR still needs to be triaged. labels Feb 6, 2020
@njlynch
Copy link
Contributor

njlynch commented May 27, 2020

I see two different options for implementing support for key aliases. The current IKey interface only has keyArn and keyId as members, and doesn't have the alias as a first-class member.

The most robust and involved option would be to handle lookup of the key ARN from the alias using a context query, similar to how HostedZone.fromLookup or LookupMachineImage.getImage works today. This would do a query of the KMS list-aliases API to retrieve the ARN that matches the alias. This approach has the downside of the active lookup (i.e., requiring the stack-level account and region to be set). However, this method ensures consistency in that the key ARN will always be present for synthesis.

The above assumes that every time we need a reference to a KMS Key, we need the Key ARN; however, many CloudFormation resources that reference KMS keys allow for using the Key alias directly, as long as the key and stack are in the same account. Here are a few documented examples: (CloudTrail Trail, CodeBuild Pipeline, DocDB Cluster).

An alternative approach would make use of the fact that many resources can directly use the Key alias, and not do a lookup unless necessary. The current IKey interface could be extended to include one (or more) aliases, and to use that alias directly in the synthesized output (where appropriate). The difficulties I see with this approach are that: (1) whether an alias can be used in place of the ARN is not consistent on a resource-by-resource basis, and (2) the grant* methods on IKey require the ARN to create grants.

Rather than forcing the alias-only key to conform to the IKey interface, a new interface (IKeyAlias) could be created which would be used (and accepted) anywhere a simple reference to an alias is needed. This would be a fairly intrusive and backwards-incompatible change though, so I would want a pre-review and/or guidance from the core team before undertaking this effort.

In lieu of any conflicting guidance or opinions, I can take a crack at the initial approach (context query lookup).

@skinny85
Copy link
Contributor

@njlynch I think I have a simpler solution for all of this :). How about if we had this method available (name TBD, of course):

const awsS3Key: kms.IAlias = kms.Alias.fromAliasName(this, 'AwsS3Key', 'aws/s3');

Remember that kms.IAlias extends kms.IKey, so this awsS3Key could be used anywhere an IKey is required.

Thoughts on this @njlynch ?

@njlynch
Copy link
Contributor

njlynch commented May 28, 2020

@skinny85 - Yes, I think I missed that IAlias extends IKey. To make this approach work, you'd need to make the aliasTargetKey property optional in IAlias, AliasAttributes, AliasBase, and Alias. All of the AliasBase methods that implement the IKey interface -- by delegating to the aliasTargetKey -- would also need to have null checks & throw errors if you attempt to use them. Something like this:

  public grant(grantee: iam.IGrantable, ...actions: string[]): iam.Grant {
+    if (this.aliasTargetKey === undefined) {
+      throw new Error('Alias without an aliasTargetKey does not support "grant"');
+    }
    return this.aliasTargetKey.grant(grantee, ...actions);
  }

With this approach, a new method isn't necessary; the existing kms.Alias.fromAliasAttributes could be used.

This approach is certainly the simplest, although it feels a bit hacky to effectively error out all of the IKey capabilities to make the IAlias work without a corresponding key. That being said, it's easy to implement and none of the existing tests fail (except for test coverage), so I can proceed down this route if you're happy with it.

@skinny85
Copy link
Contributor

Changing aliasTargetKey to optional is out of the question, as that would be a backwards-incompatible change. Whatever object was returned from Alias.fromAliasName(): IAlias (I'm thinking that name is actually not that great - perhaps Alias.awsManagedKey(), or something like that, is better) has to raise an exception when the aliasTargetKey property is accessed.

I'm also thinking about IKey.keyArn. As weird as that sounds, I think it should return aws/s3 in this case, same as IKey.keyId and IAlias.aliasName...

@njlynch if you want to give this change a shot in a PR, got for it! I'll gladly review it 🙂.

Thanks,
Adam

@mergify mergify bot closed this as completed in #8299 Jun 17, 2020
mergify bot pushed a commit that referenced this issue Jun 17, 2020
Supports importing an Alias by alias name only, without the underlying reference
to the Key ARN. Useful in scenarios where only the alias is needed in a
reference by a Construct.

fixes #5953 

----

Tested via a simple repro script:
```ts
    const key = kms.Alias.fromAliasName(this, 'myKey', 'alias/myTestKey');
    new cloudtrail.Trail(this, 'CloudTrail', {
        sendToCloudWatchLogs: true,
        kmsKey: key
    });
```

----

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

Hello, I don't think this addresses the initial request/concern.
Sometimes, Alias name cannot be used in IAM Policies, but the actual KMS key ARN must be used.
The above call, kms.Alias.fromAliasName(this, 'myKey', 'alias/myTestKey'), returns aliasName, which is great. But then I would like to get a Key ARN from that alias name.

If I do it like this (I'm using python),

kms.Alias.from_alias_name(self, "myKey", "alias/myTestKey").alias_target_key , but I get this error:

jsii.errors.JSIIError: Cannot access aliasTargetKey on an Alias imnported by Alias.fromAliasName().

Please advise. How can I lookup key arn from its alias name/arn?

@njlynch
Copy link
Contributor

njlynch commented Jun 30, 2020

@dtserekhman-starz - This feature was tracking the more lightweight version where only the alias was needed (e.g., with CloudTrail). Can you open a new feature request for that "lookupKey" functionality?

@dtserekhman-starz
Copy link

@njlynch Opened #8822. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-kms Related to AWS Key Management effort/medium Medium work item – several days of effort feature-request A feature should be added or improved.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants