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(kms): key rotation period #29928

Merged
merged 9 commits into from
Apr 24, 2024
Merged

feat(kms): key rotation period #29928

merged 9 commits into from
Apr 24, 2024

Conversation

badmintoncryer
Copy link
Contributor

@badmintoncryer badmintoncryer commented Apr 22, 2024

Issue # (if applicable)

Closes #29927.

Reason for this change

Cloudformation supports for configuring period of automatic key rotation but CDK does not.

Description of changes

Added rotationPeriod to KeyProps.

Description of how you validated changes

I've added both unit and integ tests.

Checklist


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@github-actions github-actions bot added the star-contributor [Pilot] contributed between 25-49 PRs to the CDK label Apr 22, 2024
@aws-cdk-automation aws-cdk-automation requested a review from a team April 22, 2024 22:59
@github-actions github-actions bot added feature-request A feature should be added or improved. p2 labels Apr 22, 2024
*
* @default - 365 days.
*/
readonly rotationPeriod?: Duration;
Copy link
Contributor Author

@badmintoncryer badmintoncryer Apr 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've simply added rotationPeriod properties to KeyProps.

However, there are other possible implementation approaches, and I'm struggling to decide which one is best. Could I please get your opinions, reviewers?

  1. add rotationPeriod property (current implementation)
new kms.Key(this, 'MyKey', {
  enableKeyRotation: true,
  rotationPeriod: Duration.days(180), // Add
});
  1. deprecate enableKeyRotation and define KeyRotation property
export interface KeyRotation {
  enableKeyRotation: boolean,
  rotationPeriod?: Duration,
}

new kms.Key(this, 'MyKey', {
  keyRotation: {
    enableKeyRotation: true,
    rotationPeriod: Duration.days(180), // optional
  },
});
  1. deprecate enableKeyRotation and add only rotationPeriod
new kms.Key(this, 'MyKey', {
  rotationPeriod: Duration.days(180), // Implicitly enable key rotation by defining this property
});

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the current implementation is that terrible. It probably wouldn't have been my first choice if enableKeyRotation wasn't already there, but given the circumstances, you're not introducing a deprecation or breaking change, you're allowing the existing construct to customize its default value.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nmussy Thank you for your opinion. I'm relieved to hear that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the implementation you chose @badmintoncryer, just with a small modification; if rotationPeriod is defined and enableKeyRotation is undefined, let's set enableKeyRotation to true.

@badmintoncryer badmintoncryer marked this pull request as ready for review April 22, 2024 23:13
@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Apr 22, 2024
*
* @default - 365 days.
*/
readonly rotationPeriod?: Duration;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the implementation you chose @badmintoncryer, just with a small modification; if rotationPeriod is defined and enableKeyRotation is undefined, let's set enableKeyRotation to true.

packages/aws-cdk-lib/aws-kms/lib/key.ts Outdated Show resolved Hide resolved
packages/aws-cdk-lib/aws-kms/lib/key.ts Outdated Show resolved Hide resolved
@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Apr 23, 2024
Co-authored-by: Calvin Combs <66279577+comcalvi@users.noreply.github.com>
@mergify mergify bot dismissed comcalvi’s stale review April 24, 2024 01:38

Pull request has been modified.

@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Apr 24, 2024
@badmintoncryer
Copy link
Contributor Author

@comcalvi
Thank you for your review! I've updated my implementation. Could you please confirm it again?

@@ -518,12 +520,30 @@ test('key with some options', () => {
});
});

test('set rotationPeriod without enabling enableKeyRotation', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also add a test with enableKeyRotation explicitly set to true? Other than that this looks ready to ship.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this test covers that case.

const key = new kms.Key(stack, 'MyKey', {
    enableKeyRotation: true,
    enabled: false,
    pendingWindow: cdk.Duration.days(7),
    rotationPeriod: cdk.Duration.days(180),
  });

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you are correct, it does.

@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Apr 24, 2024
@mergify mergify bot dismissed comcalvi’s stale review April 24, 2024 16:45

Pull request has been modified.

@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Apr 24, 2024
Copy link
Contributor

mergify bot commented Apr 24, 2024

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 aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Apr 24, 2024
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify mergify bot merged commit a6a5219 into aws:main Apr 24, 2024
12 checks passed
Copy link
Contributor

mergify bot commented Apr 24, 2024

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).

@badmintoncryer badmintoncryer deleted the keyRotation branch April 24, 2024 23:50
@aws-cdk-automation
Copy link
Collaborator

Comments on closed issues and PRs are hard for our team to see. If you need help, please open a new issue that references this one.

@aws aws locked as resolved and limited conversation to collaborators Jul 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature-request A feature should be added or improved. p2 star-contributor [Pilot] contributed between 25-49 PRs to the CDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kms: support for key rotation period
4 participants