feat(kms): Adds support for hmac and sm2 key spec #23866

daschaa · 2023-01-27T08:37:56Z

Fixes #23727

I have not added an integration test for SM2 because i can not deploy in the China Region.

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Construct Runtime Dependencies:

This PR adds new construct runtime dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- 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

gitpod-io · 2023-01-27T08:38:02Z

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

Jacco · 2023-01-27T13:10:50Z

Great to see another PR on this issue. There are a few differences :-)

I viewed the code and have a few remarks:

the grantGenerateMac and grantVerifyMac methods are available and callable without error on non-HMAC keys. Especially for imported keys it is not obvious to users of the IKey that they should not use grantDecrypt grantEncrypt.
the extra error that is triggered on enableKeyRotation=True for HMAC keys is not necessary since it would already triggered
when creating HMAC keys defaulting keyUsage to GENERATE_VERIFY_MAC give slightly better experience

I like the fact that your PR is a lot simpler than mine. I had to go through changing the context provider for kms keys which leads to all kinds of trouble...

daschaa · 2023-01-27T15:18:12Z

@Jacco What a coincidence that we both have pushed this in the same time. Thank you very much for the valuable feedback.

I agree that the grantVerify and grantGenerate methods do not check if the key is a HMAC one. But I'm not sure if I have to throw an error since the methods specifically tell what's happening and do not hide something. I guess the overhead in code/logic is not worth the benefit. In the end the policy is just extended.
I would like to hear a third opinion from someone of the CDK Team on this - I'm not sure what's best practice.

Regarding the check of the key rotation I just wanted to have a good error message :-)

And regarding the default usage I think this is a very good idea and I will definitely add this to the code.

Thanks again! ❤️

TheRealAmazonKendra

Usually I only provide feedback on the older PR when there's a duplicate, but since y'all are conversing on this issue, I'm going to provide feedback on both so that maybe in collaboration we can come to a better solution.

Just one comment inline but it has pretty major repercussions on the PR. Let me know what you think, please.

TheRealAmazonKendra · 2023-02-09T07:35:33Z

packages/@aws-cdk/aws-kms/test/key.test.ts

@@ -966,16 +967,40 @@ describe('key specs and key usages', () => {
    });
  });

-  test('invalid combinations of key specs and key usages', () => {
+  test.each([


This really concerns me. I know you didn't write this original contract but this many invalid options that won't be discovered until synth time makes me uneasy. Is there a contract update we can make here to enforce the valid combos better?

Maybe it would be better to create a separate class for Hmac keys? Maybe there actually should be a few classes:

SymmetricKey

only one to have enableKeyRotation in props

usage is always ENCRYPT_DECRYPT

HmacKey

usage is always GENERATE_VERIFY_MAC

ShaKey

usage is always SIGN_VERIFY

RsaKey

usage options SIGN_VERIFY or ENCRYPT_DESCRYPT

But also the base needs some (breaking?) refactorings because I think a key that has no ENCRYPT_DECRYPT usage should not have grantDescrypt/grantEncrypt methods.

@Jacco How can we ensure backwards compatibility with this separation?
When someone upgrades to a potential version with a class for each key, how will the Basic Key class behave?

@TheRealAmazonKendra Do you mean extending the tests to make the invalid combinations clearer? Or changing the logic inside the production code?

Jacco · 2023-02-12T12:22:30Z

Maybe it would be better to create a separate class for Hmac keys? Maybe there actually should be a few classes:

SymmetricKey

only one to have enableKeyRotation in props
usage is always ENCRYPT_DECRYPT

HmacKey

usage is always GENERATE_VERIFY_MAC

ShaKey

usage is always SIGN_VERIFY

RsaKey

usage options SIGN_VERIFY or ENCRYPT_DESCRYPT

But also the base needs some (breaking?) refactorings because I think a key that has no ENCRYPT_DECRYPT usage should not have grantDescrypt/grantEncrypt methods.

Jacco · 2023-02-12T20:04:25Z

@daschaa yeah I know, will be hard. Maybe in a new package? aws-kms-v2? I actually haven't seen anything like that happening in the cdk project.

aws-cdk-automation · 2023-03-03T01:49:21Z

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

Pull request has been modified.

daschaa · 2023-03-03T13:40:19Z

@TheRealAmazonKendra I have changed the tests so that a test case for every deny list entry is added. It's not yet perfect, because I decided to not export the deny list from the Key class but instead duplicating it to the tests. My thought was that the deny list should stay internal and if i export it, it is publicly available - I think this wouldn't be so good because we don't want to expose the list just for the tests.

I'm looking forward for your feedback and I hope I understood your review correctly 😄

MrArnoldPalmer

I agree that the grantVerify and grantGenerate methods do not check if the key is a HMAC one. But I'm not sure if I have to throw an error since the methods specifically tell what's happening and do not hide something. I guess the overhead in code/logic is not worth the benefit. In the end the policy is just extended.
I would like to hear a third opinion from someone of the CDK Team on this - I'm not sure what's best practice.

So usually if we have the ability to detect an incompatible combination during synth, than we will and throw an error. So if the user is able to call grantGenerateMac and grantVerifyMac on keys where that policy doesn't make sense, I do think we should throw an error there.

MrArnoldPalmer · 2023-03-07T15:46:57Z

Otherwise I feel like this looks pretty good. Agreed that invalid combination issue is annoying and would be nice if we could fix but I don't see a way without breaking changes.

Pull request has been modified.

daschaa · 2023-03-11T13:51:23Z

I agree that the grantVerify and grantGenerate methods do not check if the key is a HMAC one. But I'm not sure if I have to throw an error since the methods specifically tell what's happening and do not hide something. I guess the overhead in code/logic is not worth the benefit. In the end the policy is just extended.
I would like to hear a third opinion from someone of the CDK Team on this - I'm not sure what's best practice.

So usually if we have the ability to detect an incompatible combination during synth, than we will and throw an error. So if the user is able to call grantGenerateMac and grantVerifyMac on keys where that policy doesn't make sense, I do think we should throw an error there.

@MrArnoldPalmer I added a guard statement to the grant*Mac methods. Can you have a look if it is good like that? :)

MrArnoldPalmer

Just a leftover console.log and a question.

packages/@aws-cdk/aws-kms/lib/key.ts

Pull request has been modified.

MrArnoldPalmer

Looks good, thank you @daschaa

mergify · 2023-03-22T22:41:14Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2023-03-23T00:45:49Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: c400686
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2023-03-23T00:46:10Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

Fixes aws#23727 I have not added an integration test for SM2 because i can not deploy in the China Region. ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Construct Runtime Dependencies: * [ ] This PR adds new construct runtime dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-construct-runtime-dependencies) ### New Features * [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [x] 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*

✨ (kms): Adds support for hmac and sm2 keys

2f9ba16

github-actions bot added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 labels Jan 27, 2023

aws-cdk-automation requested a review from a team January 27, 2023 08:38

github-actions bot added the admired-contributor [Pilot] contributed between 13-24 PRs to the CDK label Jan 27, 2023

aws-cdk-automation previously requested changes Jan 27, 2023

View reviewed changes

✨ (kms): Adds policy stuff for new hmac

7f3c62f

TheRealAmazonKendra previously requested changes Feb 9, 2023

View reviewed changes

daschaa and others added 2 commits March 3, 2023 14:36

✅ Changes tests to have test case for every deny list entry

df36bf2

Merge branch 'main' into 23727_fix_kms_hmac

22625a3

MrArnoldPalmer previously requested changes Mar 7, 2023

View reviewed changes

MrArnoldPalmer assigned MrArnoldPalmer and unassigned MrArnoldPalmer Mar 7, 2023

✨ Adds extra check to grantGenerateMac and grantVerifyMac

9064f1b

daschaa and others added 2 commits March 9, 2023 08:22

🚨 Fixes linter errors

2ac440f

Merge branch 'main' into 23727_fix_kms_hmac

b8d446e

daschaa requested a review from MrArnoldPalmer March 20, 2023 12:10

MrArnoldPalmer previously requested changes Mar 20, 2023

View reviewed changes

packages/@aws-cdk/aws-kms/lib/key.ts Outdated Show resolved Hide resolved

packages/@aws-cdk/aws-kms/lib/key.ts Outdated Show resolved Hide resolved

Removes debugging log

347f27d

daschaa and others added 2 commits March 22, 2023 09:08

♻️ Removes guard statements and adds grant methods to key interface

6b58d7d

Merge branch 'main' into 23727_fix_kms_hmac

fe0ea8c

daschaa requested a review from MrArnoldPalmer March 22, 2023 08:10

MrArnoldPalmer approved these changes Mar 22, 2023

View reviewed changes

Merge branch 'main' into 23727_fix_kms_hmac

c400686

mergify bot merged commit f2f3c21 into aws:main Mar 23, 2023

filletofish mentioned this pull request Jul 2, 2023

feat(scheduler): ScheduleGroup #26196

Merged

feat(kms): Adds support for hmac and sm2 key spec #23866

feat(kms): Adds support for hmac and sm2 key spec #23866

Uh oh!

Conversation

daschaa commented Jan 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

All Submissions:

Adding new Construct Runtime Dependencies:

New Features

Uh oh!

gitpod-io bot commented Jan 27, 2023

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

Jacco commented Jan 27, 2023

Uh oh!

daschaa commented Jan 27, 2023

Uh oh!

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

Uh oh!

TheRealAmazonKendra Feb 9, 2023

Choose a reason for hiding this comment

Uh oh!

Jacco Feb 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

daschaa Feb 12, 2023

Choose a reason for hiding this comment

Uh oh!

daschaa Feb 12, 2023

Choose a reason for hiding this comment

Uh oh!

Jacco commented Feb 12, 2023

Uh oh!

Jacco commented Feb 12, 2023

Uh oh!

aws-cdk-automation commented Mar 3, 2023

Uh oh!

daschaa commented Mar 3, 2023

Uh oh!

MrArnoldPalmer left a comment

Choose a reason for hiding this comment

Uh oh!

MrArnoldPalmer commented Mar 7, 2023

Uh oh!

daschaa commented Mar 11, 2023

Uh oh!

MrArnoldPalmer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MrArnoldPalmer left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Mar 22, 2023

Uh oh!

aws-cdk-automation commented Mar 23, 2023

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Mar 23, 2023

Uh oh!

Uh oh!

daschaa commented Jan 27, 2023 •

edited

Loading

Jacco Feb 12, 2023 •

edited

Loading