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): add Key.fromKeyId(), and make Key.fromXXX().env.region reflect the region that the key was imported from #21464

Closed
2 tasks
madeline-k opened this issue Aug 4, 2022 · 2 comments · Fixed by #21519
Labels
@aws-cdk/aws-kms Related to AWS Key Management bug This issue is a bug. effort/small Small work item – less than a day of effort p2

Comments

@madeline-k
Copy link
Contributor

madeline-k commented Aug 4, 2022

Describe the feature

The ability to import a kms key based on the key id. And, the env for all imported keys should be the environment that they came from, not the environment of the current stack.

Use Case

When I already have the key id for a key, it is frustrating to have to go and get the ARN for that key id in order to import the key into my Construct.

const importedKey = kms.Key.fromKeyId(this, 'imported-key', 'xxx-xxx');

// should be the source region
importedKey.env.region

const anotherImportedKey = kms.Key.fromKeyId(this, 'imported-key-2', 'aws::us-east-1::.....');

// should be the source region
anotherImportedKey.env.region

Proposed Solution

I am not sure how exactly to implement fromKeyId, since you can't parse the ARN directly from the ID. But since fromKeyArn is able to get the key id without doing API lookups, I think it should be possible the other way.

Other Information

No response

Acknowledgements

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

CDK version used

all

Environment details (OS name and version, etc.)

MacOS

@madeline-k madeline-k added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Aug 4, 2022
@github-actions github-actions bot added the @aws-cdk/aws-kms Related to AWS Key Management label Aug 4, 2022
@kaizencc kaizencc added p2 effort/small Small work item – less than a day of effort and removed needs-triage This issue or PR still needs to be triaged. labels Aug 9, 2022
@kaizencc kaizencc added bug This issue is a bug. and removed feature-request A feature should be added or improved. labels Aug 9, 2022
@kaizencc
Copy link
Contributor

kaizencc commented Aug 9, 2022

For Key.fromKeyId(), I'm not sure if it is feasible. For example: if we have a key with id xxx-xxxxx and arn arn:aws:kms:<region>:<account>:key/xxx-xxxxx:

We can parse the keyId from the keyArn easily. However, we do not have information about the region and account when we try to go from id to arn. And we need to be able to create the keyArn, because there is a property keyBase.keyArn.

@mergify mergify bot closed this as completed in #21519 Aug 10, 2022
mergify bot pushed a commit that referenced this issue Aug 10, 2022
Fixes #21464. KMS keys imported using `fromKeyArn()` currently take the environment of the stack, not the environment from the arn.

This PR follows the precedent set in #19026 and #18255. It is essentially the same code change and tests. Ideally, we would have a mechanism for testing all `fromXxxArn` APIs to ensure they have the correct behavior. There are still many places where it does not. However, given the significant overhead of creating such a mechanism, I'm creating this one-off PR to unblock users in KMS.

----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
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.

josephedward pushed a commit to josephedward/aws-cdk that referenced this issue Aug 30, 2022
Fixes aws#21464. KMS keys imported using `fromKeyArn()` currently take the environment of the stack, not the environment from the arn.

This PR follows the precedent set in aws#19026 and aws#18255. It is essentially the same code change and tests. Ideally, we would have a mechanism for testing all `fromXxxArn` APIs to ensure they have the correct behavior. There are still many places where it does not. However, given the significant overhead of creating such a mechanism, I'm creating this one-off PR to unblock users in KMS.

----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
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 bug This issue is a bug. effort/small Small work item – less than a day of effort p2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants