feat(rds): default DatabaseCluser and DatabaseInstance `storageEncrypted` to `true` (under feature flag) #32695

blimmer · 2024-12-31T04:45:41Z

Issue # (if applicable)

Reason for this change

Today, if you don't remember to pass storageEncrypted: true to your DatabaseInstance or DatabaseCluster, your database's storage will be unencrypted.

In almost all cases, your database storage should be encrypted. Here are reasons I think we should use AWS-managed keys by default:

AWS Well-Architected security pillar says all data should be encrypted at rest.
There is no additional fee to use the AWS-managed KMS key
When creating RDS instances and clusters via the AWS Console UI, encryption with AWS-managed keys is enabled by default.
Adding encryption to a previously unencrypted database is an involved process that requires recreating your database from a snapshot. Applying encryption to an unencrypted database is time-consuming and a bad developer and user experience.
Other L2 constructs apply encryption by default, including Neptune and DocDb
If you don't encrypt your storage, you'll see warnings in the RDS UI telling you that your storage is unencrypted.
Users can easily retain the existing behavior.

Description of changes

This PR introduces a new feature flag, @aws-cdk/aws-rds:enableEncryptionAtRestByDefault. When set to true, storageEncrypted will default to true instead of undefined (which means "false").

To provide backward compatibility, this PR also introduces a new property, storageEncryptedLegacyDefaultValue. This allows leaving the StorageEncrypted property unset in the CloudFormation template, to avoid replacing Database instances/clusters.

Describe any new or updated permissions being added

N/A

Description of how you validated changes

Unit and integration tests.

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

… storageEncrypted

blimmer · 2024-12-31T04:47:07Z

...ges/@aws-cdk-testing/framework-integ/test/aws-appsync/test/integ.graphql-rds-serverlessv2.ts

@@ -35,6 +35,8 @@ class TestStack extends cdk.Stack {
      vpc,


This change affected a lot of integration tests. I opted to use the legacy behaviors for these tests because re-running the creation of all these databases would be extremely slow and expensive.

If someone at AWS with a "free"/"expensed" account would like to rerun all these failing tests, we could go that route, too.

I added explicit new tests for this behavior.

I think manually adding "StorageEncrypted": true to the templates would make those tests pass without the need to deploy the related resources (we expect the flag to work given that you added the new test).

In the past, I've received PR feedback that you should never manually update snapshot files. I just confirmed that I could add this property manually to the snapshots and that would be an alternative solution to updating the stack definitions.

I'd like a +1 from an approver that this would be OK before I spend the time doing it, based on previous feedback that snapshots should never be manually edited.

I've received PR feedback that you should never manually update snapshot files

👍 Sounds good!

About the isStorageLegacyUnencrypted comment.
Thanks for clarifying, I'm not sure why CFN defaults to unencrypted clusters and instances.
However, I left some comments on the implementation for small adjustments.

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.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

codecov · 2024-12-31T17:30:33Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.21%. Comparing base (7538a84) to head (abc94e0).

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #32695   +/-   ##
=======================================
  Coverage   82.21%   82.21%           
=======================================
  Files         119      119           
  Lines        6876     6876           
  Branches     1162     1162           
=======================================
  Hits         5653     5653           
  Misses       1120     1120           
  Partials      103      103

Flag	Coverage Δ
suite.unit	`82.21% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	`∅ <ø> (∅)`
packages/aws-cdk-lib/core	`82.21% <ø> (ø)`

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

lpizzinidev

To provide backward compatibility, this PR also introduces a new property, isStorageLegacyUnencrypted. This allows leaving the StorageEncrypted property unset in the CloudFormation template, to avoid replacing Database instances/clusters.

Shouldn't the default behavior be applied when @aws-cdk/aws-rds:enableEncryptionAtRestByDefault: false? 🤔
The extra variable seems to add unnecessary complexity here.

lpizzinidev · 2025-01-03T15:38:41Z

...ges/@aws-cdk-testing/framework-integ/test/aws-appsync/test/integ.graphql-rds-serverlessv2.ts

@@ -35,6 +35,8 @@ class TestStack extends cdk.Stack {
      vpc,


I think manually adding "StorageEncrypted": true to the templates would make those tests pass without the need to deploy the related resources (we expect the flag to work given that you added the new test).

blimmer · 2025-01-03T20:16:58Z

@lpizzinidev - thanks for the review. I commented on your inline comment, and below is a comment that you requested changed for (it's annoying that GitHub won't let you respond inline to those).

Suggestion to reuse feature flag vs `isStorageLegacyUnencrypted`

This would work for v2, but if the feature flag is ever removed (it looks like all v1 flags were removed from v2 - see docs), this would no longer work. My understanding is that when v3 is released, the feature flag will be removed, and we will need to remember to re-add something like isStorageLegacyUnencrypted.

This is very similar to the isFromLegacyInstanceProps provided for a similar reason, to prevent replacing database resources (docs).

Unfortunately, because the behavior is to pass undefined today, it makes it a bit challenging to work with. CloudFormation reports that changing undefined -> false causes resource replacement. I feel fairly strongly that the new behavior should be to always explicitly set the value to true or false, which led me to this solution.

If there's another way I should be thinking about this (like retaining that feature flag indefinitely - I don't know if that's an option), let me know.

…ryption-by-default

…Value

blimmer · 2025-01-04T18:14:43Z

@lpizzinidev - thanks for the thorough review and great comments. This is ready for re-review.

lpizzinidev

Thanks 🚀 Left comments for some final adjustments

packages/aws-cdk-lib/aws-rds/lib/private/util.ts

packages/aws-cdk-lib/aws-rds/README.md

…ryption-by-default

blimmer · 2025-01-06T21:15:38Z

@lpizzinidev - thanks! should be good to go.

lpizzinidev

Thanks 👍

…ryption-by-default

godwingrs22

Thanks @blimmer for raising this issue and for this contribution! I have left few comments to confirm my understanding. I also suggested few modifications around the feature flag. Looking forward to your thoughts on the suggested changes!

godwingrs22 · 2025-02-19T00:50:11Z

packages/aws-cdk-lib/aws-rds/lib/private/util.ts

+  // If a KMS key is provided, enable encryption at rest
+  if (storageEncryptionKey) {
+    return true;
+  }


Currently, when storageEncryptionKey is provided, the code forces storageEncrypted to true, regardless of what value the user explicitly set for storageEncrypted. This means even if a user specifically sets storageEncrypted: false, the encryption is still enabled, which seems to contradict the purpose of the storageEncrypted property.

In that case do we still need the same behaviour in the new approach? Would it be better to validate and throw error with clear message (when feature flag is enabled). Something like:

// Validation: Cannot disable encryption while providing an encryption key if (storageEncryptionKey && storageEncrypted === false) { throw new Error( 'Cannot set storageEncrypted to false when storageEncryptionKey is provided. ' + 'Either enable encryption or remove the storageEncryptionKey.' ); }

Please correct me if my understanding is wrong.

godwingrs22 · 2025-02-19T00:51:10Z

packages/aws-cdk-lib/aws-rds/lib/private/util.ts

+  if (storageEncryptedLegacyDefaultValue) {
+    if (storageEncrypted) {
+      throw new ValidationError('Cannot set `storageEncrypted` to `true` when `storageEncryptedLegacyDefaultValue` is `true`.', scope);
+    }
+
+    return undefined;
+  }


I didn't fully understood the usage of this new property. Could you help me to explain further with different scenarios?

godwingrs22 · 2025-02-19T00:55:36Z

packages/aws-cdk-lib/aws-rds/lib/instance.ts

+   * @default - If the `@aws-cdk/aws-rds:enableEncryptionAtRestByDefault` feature flag is set, true by default or if storageEncryptionKey is provided. If the flag is not set, undefined by default unless storageEncryptionKey is provided.
   */
  readonly storageEncrypted?: boolean;


nit: I'm not quite clear on the default behaviour based on reading the doc string. Would be nice to rephrase it.

godwingrs22 · 2025-02-19T00:58:18Z

packages/aws-cdk-lib/aws-rds/lib/instance.ts

+   *
+   * The legacy behavior left the `StorageEncrypted` CloudFormation property unset by default. This resulted in
+   * cluster storage being unencrypted. The new behavior always sets the `StorageEncrypted` property to `true` or
+   * `false` for clarity. However, CloudFormation will replace the Cluster if you change `StorageEncrypted` from


The new behavior always sets the StorageEncrypted property to true or false for clarity.

I don't think it is true based on the method implementation. I could see it may return undefined if the storageEncryptedLegacyDefaultValue is true and storageEncrypted is undefined or false.

godwingrs22 · 2025-02-19T01:17:47Z

packages/aws-cdk-lib/aws-rds/lib/private/util.ts

+export function getStorageEncryptedProperty(
+  scope: IConstruct,
+  storageEncrypted?: boolean,
+  storageEncryptedLegacyDefaultValue?: boolean,
+  storageEncryptionKey?: kms.IKey,
+): boolean | undefined {
+  const featureFlagEnabled = FeatureFlags.of(scope).isEnabled(cxapi.RDS_ENABLE_ENCRYPTION_AT_REST_BY_DEFAULT);


Please correct me if I understood the requirement wrongly. We want to make storageEncrypted default to true which will enable encryption at rest. If storageEncryptionKey is provided, then will use the provided kms key; if not, will use the default AWS managed key for encryption. If customer explicitly set storageEncrypted as false, then encryption will be disabled.

If feature flag is false (existing behaviour):

Existing CDK projects will have default feature flag as false. This preserves backward compatibility:

// No explicit settings - remains undefined new DatabaseCluster(this, 'Cluster', {}); // With encryption key - forces true new DatabaseCluster(this, 'Cluster', { storageEncryptionKey: key }); // With encryption key - forces true, ignores storageEncrypted value provided by customer new DatabaseCluster(this, 'Cluster', { storageEncrypted: false, storageEncryptionKey: key }); //Explicit false new DatabaseCluster(this, 'Cluster', { storageEncrypted: false }); // If storageEncrypted is true and no encryption key specified, uses default aws managed key new DatabaseCluster(this, 'Cluster', { storageEncrypted: true });

If feature flag is true (new behaviour)

New CDK projects will have the feature flag enabled by default, making storageEncrypted: true the default:

// No explicit settings - defaults to true new DatabaseCluster(this, 'Cluster', {}); // storageEncrypted: true and uses default AWS managed key for encryption // Explicit false is respected new DatabaseCluster(this, 'Cluster', { storageEncrypted: false }); // storageEncrypted: false // Uses provided encryption key new DatabaseCluster(this, 'Cluster2', { storageEncryptionKey: key // default storageEncrypted is true and takes the user provided key for encryption }); // Explicit encryption configuration new DatabaseCluster(this, 'Cluster1', { storageEncrypted: true, //explicit true storageEncryptionKey: key // Enables encryption with user provided key }); // Invalid configuration - will throw error new DatabaseCluster(this, 'Cluster', { storageEncrypted: false, storageEncryptionKey: key }); // Error: Cannot set storageEncrypted to false when storageEncryptionKey is provided

If the above requirements are true, i think the following code might work without the need for additional property.

export function getStorageEncrypted( scope: IConstruct, storageEncrypted?: boolean, storageEncryptionKey?: kms.IKey, ): boolean | undefined { const featureFlagEnabled = FeatureFlags.of(scope).isEnabled(cxapi.RDS_ENABLE_ENCRYPTION_AT_REST_BY_DEFAULT); // Feature flag is OFF - maintain existing behavior if (!featureFlagEnabled) { return storageEncryptionKey ? true : storageEncrypted; } // Feature flag is ON - new behavior // Validation: Cannot disable encryption while providing an encryption key if (storageEncryptionKey && storageEncrypted === false) { throw new Error( 'Cannot set storageEncrypted to false when storageEncryptionKey is provided. ' + 'Either enable encryption or remove the storageEncryptionKey.' ); } if (storageEncrypted !== undefined) { return storageEncrypted; } // Default to true when feature flag is ON and no explicit value provided return true; }

Let me know if this makes sense or if I've misunderstood any of the requirements.

Hey @godwingrs22 - the extra property is required when CDK v3 is released and the feature flag is removed. That's because the legacy default behavior leaves the storageEncrypted property undefined in the template. In effect, this means "unencrypted".

Without the extra property, when the feature flag is removed, you'd think that users could do this:

new DatabaseCluster(this, 'Cluster', { storageEncrypted: false });

However, changing the StorageEncrypted property from undefined to false reports that the cluster/instance will be replaced. We do not want to force our users to replace their cluster/instance when the feature flag is removed.

Therefore, I think we have to provide a way to retain the legacy behavior of not setting the StorageEncryption property at all essentially forever to prevent replacing existing databases that used the old pre-feature-flag behavior.

Does that make sense why I think we need this extra property?

See also #32695 (comment) where I discussed this above.

Hi @blimmer , Thanks for the clarification. Yes it makes sense to have the new property to preserve the legacy behaviour if the feature flag is removed in future.

…ryption-by-default

aws-cdk-automation · 2025-03-03T00:57:38Z

AWS CodeBuild CI Report

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

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

godwingrs22

Hi @blimmer, Thanks for the clarification. LGTM. I'll internally check with RDS service team once to see if there is any feedback from them on enabling the encryption at rest by default.

godwingrs22 · 2025-03-10T17:18:14Z

packages/aws-cdk-lib/aws-rds/lib/private/util.ts

+export function getStorageEncryptedProperty(
+  scope: IConstruct,
+  storageEncrypted?: boolean,
+  storageEncryptedLegacyDefaultValue?: boolean,
+  storageEncryptionKey?: kms.IKey,
+): boolean | undefined {
+  const featureFlagEnabled = FeatureFlags.of(scope).isEnabled(cxapi.RDS_ENABLE_ENCRYPTION_AT_REST_BY_DEFAULT);


Hi @blimmer , Thanks for the clarification. Yes it makes sense to have the new property to preserve the legacy behaviour if the feature flag is removed in future.

blimmer · 2025-03-10T17:24:34Z

OK, I will pause for your response before fixing up the PR to prep for final review / merge.

blimmer added 7 commits December 30, 2024 19:49

chore: add feature flag

d80befc

feat: enable cluster encryption by default under feature flag

ff3521d

fix: add isStorageLegacyUnencrypted to work around false/undefined on…

319e467

… storageEncrypted

fix: existing integration tests

83d402d

docs: add readme note about encryption

9ce2d46

feat: enable encryption by default for db instances

221b872

fix: existing integration tests

75192b0

blimmer marked this pull request as ready for review December 31, 2024 04:45

github-actions bot added effort/medium feature-request p2 labels Dec 31, 2024

aws-cdk-automation requested a review from a team December 31, 2024 04:45

github-actions bot added the admired-contributor label Dec 31, 2024

blimmer commented Dec 31, 2024

View reviewed changes

aws-cdk-automation previously requested changes Dec 31, 2024

View reviewed changes

fix: add default properties

04fe2b2

blimmer added 3 commits December 31, 2024 10:47

feat: add cluster integ test

d1c6c06

fix: code sample in readme

576289e

feat: add instance integ test

6bd263d

blimmer and others added 3 commits December 31, 2024 11:16

chore: add tests for thrown error

da306c0

fix: sync snapshot

6077185

Merge branch 'main' into enable-rds-encryption-by-default

4497f62

aws-cdk-automation added the pr/needs-community-review label Dec 31, 2024

moltar approved these changes Jan 1, 2025

View reviewed changes

github-actions bot mentioned this pull request Jan 1, 2025

Monthly PR metrics report #32703

Closed

lpizzinidev suggested changes Jan 3, 2025

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review label Jan 3, 2025

blimmer added 6 commits January 4, 2025 10:56

Merge branch 'main' of github.com:blimmer/aws-cdk into enable-rds-enc…

c089ab2

…ryption-by-default

refactor: isStorageLegacyUnencrypted -> storageEncryptedLegacyDefault…

dbea499

…Value

fix: merge conflict issue with feature flag markdown

4af1515

refactor: common getStorageEncryptedProperty implementation

3c317b0

fix: again

26a9325

docs: rework docs

82f8e77

blimmer requested a review from lpizzinidev January 4, 2025 18:14

lpizzinidev suggested changes Jan 5, 2025

View reviewed changes

packages/aws-cdk-lib/aws-rds/lib/private/util.ts Outdated Show resolved Hide resolved

packages/aws-cdk-lib/aws-rds/README.md Show resolved Hide resolved

blimmer added 3 commits January 6, 2025 14:11

Merge branch 'main' of github.com:blimmer/aws-cdk into enable-rds-enc…

9130e4e

…ryption-by-default

fix: union type

1086ef1

docs: readme update

a327f10

blimmer requested a review from lpizzinidev January 6, 2025 21:15

fix: method signature

becf0a7

lpizzinidev approved these changes Jan 7, 2025

View reviewed changes

aws-cdk-automation added the pr/needs-maintainer-review label Jan 7, 2025

Merge branch 'main' of github.com:blimmer/aws-cdk into enable-rds-enc…

bddf5b9

…ryption-by-default

blimmer force-pushed the enable-rds-encryption-by-default branch from 14795d0 to bddf5b9 Compare February 2, 2025 18:30

blimmer added 2 commits February 2, 2025 11:59

fix: linting errors

4754ddf

fix: integ tests

d072620

QuantumNeuralCoder assigned godwingrs22 Feb 4, 2025

Merge branch 'main' into enable-rds-encryption-by-default

af9c8fe

godwingrs22 assigned godwingrs22 and unassigned godwingrs22 Feb 6, 2025

godwingrs22 reviewed Feb 19, 2025

View reviewed changes

Merge branch 'main' of github.com:blimmer/aws-cdk into enable-rds-enc…

abc94e0

…ryption-by-default

godwingrs22 reviewed Mar 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rds): default DatabaseCluser and DatabaseInstance `storageEncrypted` to `true` (under feature flag) #32695

feat(rds): default DatabaseCluser and DatabaseInstance `storageEncrypted` to `true` (under feature flag) #32695

blimmer commented Dec 31, 2024 •

edited

Loading

blimmer Dec 31, 2024 •

edited

Loading

lpizzinidev Jan 3, 2025

blimmer Jan 3, 2025

lpizzinidev Jan 4, 2025 •

edited

Loading

aws-cdk-automation left a comment

codecov bot commented Dec 31, 2024 •

edited

Loading

lpizzinidev left a comment

lpizzinidev Jan 3, 2025

blimmer commented Jan 3, 2025 •

edited

Loading

blimmer commented Jan 4, 2025

lpizzinidev left a comment

blimmer commented Jan 6, 2025

lpizzinidev left a comment

godwingrs22 left a comment

godwingrs22 Feb 19, 2025

godwingrs22 Feb 19, 2025

godwingrs22 Feb 19, 2025

godwingrs22 Feb 19, 2025

godwingrs22 Feb 19, 2025

blimmer Mar 3, 2025 •

edited

Loading

godwingrs22 Mar 10, 2025

aws-cdk-automation commented Mar 3, 2025

godwingrs22 left a comment

godwingrs22 Mar 10, 2025

blimmer commented Mar 10, 2025

feat(rds): default DatabaseCluser and DatabaseInstance storageEncrypted to true (under feature flag) #32695

Are you sure you want to change the base?

feat(rds): default DatabaseCluser and DatabaseInstance storageEncrypted to true (under feature flag) #32695

Conversation

blimmer commented Dec 31, 2024 • edited Loading

Issue # (if applicable)

Reason for this change

Description of changes

Describe any new or updated permissions being added

Description of how you validated changes

Checklist

blimmer Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lpizzinidev Jan 4, 2025 • edited Loading

Choose a reason for hiding this comment

aws-cdk-automation left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 31, 2024 • edited Loading

Codecov Report

lpizzinidev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blimmer commented Jan 3, 2025 • edited Loading

Suggestion to reuse feature flag vs isStorageLegacyUnencrypted

blimmer commented Jan 4, 2025

lpizzinidev left a comment

Choose a reason for hiding this comment

blimmer commented Jan 6, 2025

lpizzinidev left a comment

Choose a reason for hiding this comment

godwingrs22 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blimmer Mar 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Mar 3, 2025

AWS CodeBuild CI Report

godwingrs22 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blimmer commented Mar 10, 2025

feat(rds): default DatabaseCluser and DatabaseInstance `storageEncrypted` to `true` (under feature flag) #32695

feat(rds): default DatabaseCluser and DatabaseInstance `storageEncrypted` to `true` (under feature flag) #32695

blimmer commented Dec 31, 2024 •

edited

Loading

blimmer Dec 31, 2024 •

edited

Loading

lpizzinidev Jan 4, 2025 •

edited

Loading

codecov bot commented Dec 31, 2024 •

edited

Loading

blimmer commented Jan 3, 2025 •

edited

Loading

Suggestion to reuse feature flag vs `isStorageLegacyUnencrypted`

blimmer Mar 3, 2025 •

edited

Loading