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

feat(aws-cdk-lib/aws-kms): Allow testing for the existence of a KMS key by alias name #31574

Open
2 tasks
neoakris opened this issue Sep 26, 2024 · 9 comments · May be fixed by #31676
Open
2 tasks

feat(aws-cdk-lib/aws-kms): Allow testing for the existence of a KMS key by alias name #31574

neoakris opened this issue Sep 26, 2024 · 9 comments · May be fixed by #31676
Labels
@aws-cdk/aws-kms Related to AWS Key Management effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p3

Comments

@neoakris
Copy link

neoakris commented Sep 26, 2024

Describe the feature

Here's a code snippet for some context:

import * as kms from 'aws-cdk-lib/aws-kms';
let kmsKey = kms.Key.fromLookup(stack, "pre-existing-kms-key", { aliasName: "alias/eks/test" })

Current State of CDK 2.133.0:

  • The above logic works when a kms-key with alias "eks/test" exists.
  • If no key exists, then the program stops with error
    [Error at /$STACK_NAME] Could not find any key with alias named eks/test
    I tried wrapping the above in try catch logic, but the program still does a hard stop with the above error as soon as it discovers the key doesn't exist.

Feature Request/Proposed Solution:

  • Update logic:
    If no key exists, instead of an error, return undefined. This will make it easier to allow if statements to handle that scenario.

Use Case

While using EKS Blueprints, every time I delete and recreate a cluster (semi-frequently for ephemeral sandbox / test environments), due to its defaults it'll create a new KMS key each time, then on delete I get orphaned kms keys.
EKS Blueprints allows passing in a pre-existing kms key to avoid this.

However I'd like to add logic for the following use case:

  1. specify a desired kms alias
  2. detect if it exists
  3. if not exist: create new key with kms alias, and use the newly created kms key.
  4. if exists: use kms key with alias.

Basically I wanted to be able to create a function named ensure_existence_of_kms_key_with_alias()

This will allow me to implement logic where when I create a cluster it'll create a kms key, but if I delete and recreate the cluster it'll reuse the original kms key. (This avoids orphaned keys, and is also advantageous in the scenario of if disk backups were ever created using a kms key, then restore might be easier if a consistent kms key were used.)

Other Information

The proposed feature/solution shouldn't be a breaking change.

Acknowledgements

  • I may be able to implement this feature request
  • This feature might incur a breaking change

CDK version used

2.133.0 (build dcc1e75)

Environment details (OS name and version, etc.)

MacOS Sonoma 14.6.1

@neoakris neoakris added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Sep 26, 2024
@github-actions github-actions bot added the @aws-cdk/aws-kms Related to AWS Key Management label Sep 26, 2024
@khushail khushail added investigating This issue is being investigated and/or work is in progress to resolve the issue. and removed needs-triage This issue or PR still needs to be triaged. labels Sep 26, 2024
@khushail khushail self-assigned this Sep 26, 2024
@neoakris
Copy link
Author

neoakris commented Sep 26, 2024

If anyone else runs into a similar scenario
The following is a snippet of a temporary workaround I'm considering/might use.

import { execSync } from 'child_process'; //work around kms UX issue
import console = require('console'); //can help debug feedback loop

const alias_key_to_lookup = "alias/eks/test";
const cmd = `aws kms list-aliases | jq '.Aliases[] | select(.AliasName == "${alias_key_to_lookup}") | .TargetKeyId'`
const cmd_results = execSync(cmd).toString();
console.log(cmd_results)
//TO DO: never pass un-sanitized input / would need tight input sanitization.

cmd_results = "" if not found
cmd_results = "ac97959f-c5fb-4de0-a584-2d741c05ea0b" if found
^-- basically a more graceful failure that's a lot easier to handle with if statement checks.

@khushail
Copy link
Contributor

khushail commented Sep 26, 2024

Hi @neoakris , thanks for requesting and sharing the workaround as well. I think this can be achieved using a flag type variable, say, KeyAliasExists(), which would return false if its not existing. Your usecase makes sense to me.

However if you would like to contribute, please feel free to follow our contribution guide. I would be marking this feature request as P3 for keeping it open for community contribution as well. Thanks.

@khushail khushail added p3 effort/small Small work item – less than a day of effort and removed investigating This issue is being investigated and/or work is in progress to resolve the issue. labels Sep 26, 2024
@khushail khushail removed their assignment Sep 26, 2024
@neoakris
Copy link
Author

You're right a flag makes more sense, it'd be just as effective as a string comparison, yet cleaner.

Thanks for the triage and suggestions.

@go-to-k
Copy link
Contributor

go-to-k commented Oct 8, 2024

Hi @khushail @neoakris ,

I have submitted the PR, where the fromLookup method returns a dummy key if the new option returnDummyKeyOnMissing is set to true for now.

The reasons for this are:

Allowing undefined in the return value of fromLookup is a breaking change, so I decided not to do that. I also considered allowing a dummy value to be freely specified (like the SSM Parameter case), but decided against that too as I didn't think there would be many use cases for it.

Of course, we can create a new method that is separate from the fromLookup method, such as Key.keyAliasExists().
However, given the design philosophy of the original code, I thought it would be better to fix it to the correct implementation of fromLookup.

Please let me know if you have any feedback.

@neoakris
Copy link
Author

neoakris commented Oct 8, 2024

@go-to-k my understanding of what you've said is once implemented/in a live release, if I specify the returnDummyKeyOnMissing option. I'll be able to do an if statement check against 1234abcd-12ab-34cd-56ef-1234567890ab when the key isn't pre-existing that works for me.

Thanks for the update!

@go-to-k
Copy link
Contributor

go-to-k commented Oct 8, 2024

That's right!

@khushail
Copy link
Contributor

khushail commented Oct 8, 2024

@go-to-k , your implementation with the existing design is very thoughful. Thanks for your efforts in submitting a PR!

@go-to-k
Copy link
Contributor

go-to-k commented Oct 8, 2024

@khushail Thank you! The pr/needs-cli-test-run label has been attached, what should I do? If I just wait, will the maintainer run the test for me?

@khushail
Copy link
Contributor

khushail commented Oct 8, 2024

@go-to-k , I am not really sure why this label is attached. You could ask in the PR from the maintainer.

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/small Small work item – less than a day of effort feature-request A feature should be added or improved. p3
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants