fix(rds): customizing secret results in unusable password and lost attachment #11237

jogold · 2020-10-31T18:11:25Z

When the excludeCharacters prop is updated the secret is correctly
regenerated but the database is not updated because the
MasterUserPassword prop does not change. Moreover the connection
fields created by the attachment are lost because the
AWS::SecretsManager::SecretAttachment doesn't "rerun" (it references
the same secret).

Introduce fromGeneratedSecret() and fromPassword() static methods
in Credentials. When used the MasterUsername prop of the database
references the username as a string instead of a dynamic reference to
the username field of the secret. Moreover the logical id of the secret
is a hash of its customization options. This way the secret gets replaced
when it's customized, the database gets updated and the attachement reruns.
This without updating the MasterUsername prop, avoiding a replacement
of the database.

For instances that were created from a snapshot the MasterUsername prop
is not specified so there's no replacement risk. Add a new static method
fromGeneratedSecret() in SnapshotCredentials to replace the secret
when its customization options change (also hash in logical id).

Deprecate Credentials.fromUsername() but keep it as default to avoid
a breaking change that would replace existing databases that were not
created from a snapshot.

Deprecate SnapshotCredentials.fromGeneratedPassword() which doesn't
replace the secret when customization options are changed.

Closes #11040

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

…tachment When the `excludeCharacters` prop is updated the secret is correctly regenerated but the database is not updated because the `MasterUserPassword` prop does not change. Moreover the connection fields created by the attachment are lost because the secret is regenerated but the `AWS::SecretsManager::SecretAttachment` doesn't "rerun". Introduce a new `fromFixedUsername()` static method in `Credentials`. When used the `MasterUsername` prop of the database references the username as a string instead of a dynamic reference to the username field of the secret. Moreover the logical id of the secret is a hash of its customization options. This way the secret gets replaced when it's customized, the database gets updated and the attachement reruns. This without updating the `MasterUsername` prop, avoiding a replacement of the database. Closes aws#11040

gitpod-io · 2020-10-31T18:11:28Z

skinny85

Thanks for your efforts @jogold ! Some initial comments I have.

packages/@aws-cdk/aws-rds/lib/database-secret.ts

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

packages/@aws-cdk/aws-rds/lib/props.ts

skinny85 · 2020-11-02T21:52:38Z

packages/@aws-cdk/aws-rds/lib/props.ts

+   * Creates Credentials for the given username, and optional password and key.
+   * If no password is provided, one will be generated and stored in SecretsManager.
+   */
+  public static fromFixedUsername(username: string, options: CredentialsFromUsernameOptions = {}): Credentials {


We also have a mechanism for making the username optional, by defaulting to either the engine-specific default, or admin if that wasn't provided... Should we make username here optional too? Maybe through something like

export interface CredentialsFromFixedUsernameOptions extends CredentialsFromUsernameOptions { readonly username?: string; } export abstract class Credentials { public static fromFixedUsername(options: CredentialsFromFixedUsernameOptions = {}): Credentials { // ... } }

?

The default is currently handled in the instance/cluster code when credentials is not specified. To avoid a breaking change it still falls back to fromUsername(). Do you want to move the default handling in here?

I wonder whether it makes sense for customers to do something like this:

new rds.DatabaseCluster(this, 'Cluster', { // ... credentials: rds.Credentials.fromFixedUsername({ excludeCharacters: '!&*^#@()' }), });

"I don't care about the username - just use the default, but I want to change something about the other properties"

Does something like this make sense?

Since Credentials has a username property that cannot be undefined, I think this change means that we need to make Credentials "engine aware" to find the right default? What do you suggest?

packages/@aws-cdk/aws-rds/lib/props.ts

skinny85 · 2020-11-02T21:55:38Z

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

-      masterUsername: credentials.username,
+      masterUsername: props.credentials?.usernameAsString
+        ? props.credentials.username
+        : credentials.username,


This code reads kind of weird, I have to say. I'm wondering whether we're making Credentials too low-level. It seems like the Instance class shouldn't have to figure out all of these details... it should be able to say masterUsername: credentials.username, and the correct value should be returned from the Credentials class itself...

Agree, will see how to improve this.

The problem I have here is that the code currently calls fromSecret(new DatabaseSecret(...)) when the user specifies fromUsername() without password.

fromSecret() expects a secretsmanager.Secret so it cannot know when to render the username as a string. To do that I'll have to add an additional username argument to fromSecret() and specify it based on the new usernameAsString. This means that it's still the instance responsibility? Any suggestions?

How about creating a new, module-private factory for Credentials that we can use here? Like _fromSecretUsernameRendered(secret: ISecret, usernameAsString: boolean): Credentials?

We'll (also) need to pass the username, right?

Because you cannot extract a field out of a ISecret without creating a dynamic reference.

Sure 🙂

Then maybe it's enough to have a username?: string as the second argument, and if it's undefined, you use the reference to the Secret? I'm sure you'll figure out the details 🙂.

Chosen to add a optional username to fromSecret() which allows users using fromSecret() to also have the behavior of the username referenced as a string.

The same kind of logic is now a few lines above.

skinny85 · 2020-11-02T21:56:26Z

packages/@aws-cdk/aws-rds/lib/cluster.ts

-      masterUsername: credentials.username,
+      masterUsername: props.credentials?.usernameAsString
+        ? props.credentials.username
+        : credentials.username,


Same comment here as in Instance - there's something off about the code here, it's too low-level.

skinny85 · 2020-11-02T21:59:27Z

packages/@aws-cdk/aws-rds/test/integ.instance.lit.expected.json

@@ -534,7 +534,7 @@
        ]
      }
    },
-    "InstanceSecret478E0A47": {
+    "653e16d6b669bc291aa4827a4721a98c": {


Let's add either id, or even uniqueId, to the generated hash - right now, these are too unreadable as just random hashes.

skinny85 · 2020-11-02T22:01:59Z

packages/@aws-cdk/aws-rds/test/test.instance.ts

@@ -301,7 +301,7 @@ export = {
      expect(stack).to(haveResourceLike('AWS::RDS::DBInstance', {
        MasterUsername: ABSENT,
        MasterUserPassword: {
-          'Fn::Join': ['', ['{{resolve:secretsmanager:', { Ref: 'InstanceSecret478E0A47' }, ':SecretString:password::}}']],
+          'Fn::Join': ['', ['{{resolve:secretsmanager:', { Ref: '29ae4bfb9ee908ebe1611900a1e2469e' }, ':SecretString:password::}}']],


Why is this change needed? Nothing changed in the body of this test, so this is very worrying that this needs updating... This would mean a database replacement for a customer when they upgrade...

It's a DatabaseFromSnaphot() so its secret logical id is now a hash. As the test shows MasterUsername is expected to be ABSENT so there won't be any replacement.

packages/@aws-cdk/aws-rds/test/integ.instance.lit.ts

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

Pull request has been modified.

…ds-fixed-username

jogold · 2020-11-03T15:49:21Z

@skinny85 I finally decided to introduce two new static methods to replace fromUsername(): fromGeneratedPassword() and fromPassword(). The naming is better than fromFixedUsername() which has no real meaning for people starting with aws-rds (it's more a reference to a bug fix than what it really does).

Since making username optional in Credentials is a breaking change those methods still require a username.

I also moved common code to a new util.

skinny85

Very nice. We're almost there. A couple of minor notes.

packages/@aws-cdk/aws-rds/lib/props.ts

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

packages/@aws-cdk/aws-rds/lib/cluster.ts

packages/@aws-cdk/aws-rds/lib/database-secret.ts

packages/@aws-cdk/aws-rds/lib/props.ts

skinny85 · 2020-11-03T18:32:51Z

packages/@aws-cdk/aws-rds/test/test.instance.ts

+    const cfnSecret1 = db1Secret.node.defaultChild as cdk.CfnResource;
+    const db2Secret = db2.node.tryFindChild('Secret') as rds.DatabaseSecret;
+    const cfnSecret2 = db2Secret.node.defaultChild as cdk.CfnResource;
+    test.notEqual(cfnSecret1.logicalId, cfnSecret2.logicalId);


logicalIds are tokens, so you need to resolve them first before you do the comparison (try it be attempting to make this test fail by changing the hashing algorithm, it shouldn't work - this assertion should always pass).

of course! moved these kind of assertions to test.database-secret.ts where it actually belongs.

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

skinny85

We're almost there! I have 2 minor comments/questions.

skinny85 · 2020-11-03T22:28:20Z

packages/@aws-cdk/aws-rds/lib/database-secret.ts

+        // Use here the options that influence the password generation.
+        // If at some point we add other password customization options
+        // they sould be added here below (e.g. `passwordLength`).
+        excludeCharacters: props.excludeCharacters,


So excludeCharacters is actually optional. Which means this object will have undefined there as the default. Is that a mistake? Should the default value be filled here? (See line 66 above)

skinny85 · 2020-11-03T22:38:52Z

packages/@aws-cdk/aws-rds/lib/database-secret.ts

+        // they sould be added here below (e.g. `passwordLength`).
+        excludeCharacters: props.excludeCharacters,
+      }));
+      const logicalId = `${this.node.id.replace(/[^A-Za-z0-9]/g, '')}${hash.digest('hex')}`;


I feel like id is too short here. You'll wind up with Secret<some-hash> everywhere, which is too ambiguous. Can you use uniqueId here instead? (No need for the replace part then).

skinny85 · 2020-11-03T23:25:18Z

packages/@aws-cdk/aws-rds/lib/database-secret.ts

+   *
+   * @default false
+   */
+  readonly overrideLogicalId?: boolean;


I wonder whether a name like replaceOnPasswordChanges might be better here?

jogold · 2020-11-04T10:31:41Z

@skinny85 I think we're good here.

I added a BREAKING CHANGE to inform that the master password of instances created from snapshot will be updated. Can you check it?

@njlynch you might want to have a look at this PR since you introduced the Credentials class. Thanks!

skinny85 · 2020-11-04T17:22:07Z

I added a BREAKING CHANGE to inform that the master password of instances created from snapshot will be updated. Can you check it?

This module is now stable. We can't have breaking changes to it. Please revert the changes to DatabaseInstanceFromSnapshot.

jogold · 2020-11-04T17:35:12Z

I added a BREAKING CHANGE to inform that the master password of instances created from snapshot will be updated. Can you check it?

This module is now stable. We can't have breaking changes to it. Please revert the changes to DatabaseInstanceFromSnapshot.

Got it. But do you really consider this as a breaking change? If the secret is consumed programmatically or if rotation is in place it will have no impact... your call. The password will change but the new one is available in Secrets Manager, you never loose access.

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

njlynch · 2020-11-04T17:44:56Z

packages/@aws-cdk/aws-rds/lib/database-secret.ts

+   *
+   * @default false
+   */
+  readonly replaceOnPasswordChanges?: boolean;


Should this be more appropriately named replaceOnPasswordCriteriaChanges? or something like that? I'm afraid the name and documentation as-is may mislead people to think the secret may be replaced when rotation is configured.

I'm ok with this name, it's indeed better. @skinny85 do you validate this name?

Co-authored-by: Nick Lynch <nlynch@amazon.com>

jogold · 2020-11-04T18:36:43Z

I added a BREAKING CHANGE to inform that the master password of instances created from snapshot will be updated. Can you check it?

This module is now stable. We can't have breaking changes to it. Please revert the changes to DatabaseInstanceFromSnapshot.

Got it. But do you really consider this as a breaking change? If the secret is consumed programmatically or if rotation is in place it will have no impact... your call. The password will change but the new one is available in Secrets Manager, you never loose access.

If you want to avoid this we can expose the replaceOnPasswordChanges prop in SnapshotCredentials.fromGeneratedPassword() so that users can opt-in for the new behavior.

…ds-fixed-username

skinny85 · 2020-11-04T22:59:30Z

I added a BREAKING CHANGE to inform that the master password of instances created from snapshot will be updated. Can you check it?

This module is now stable. We can't have breaking changes to it. Please revert the changes to DatabaseInstanceFromSnapshot.

Got it. But do you really consider this as a breaking change? If the secret is consumed programmatically or if rotation is in place it will have no impact... your call. The password will change but the new one is available in Secrets Manager, you never loose access.

If you want to avoid this we can expose the replaceOnPasswordChanges prop in SnapshotCredentials.fromGeneratedPassword() so that users can opt-in for the new behavior.

I don't love that. How about new static factory methods in SnapshotCredentials, similar to the new static factory methods in Credentials?

jogold · 2020-11-05T16:38:11Z

I don't love that. How about new static factory methods in SnapshotCredentials, similar to the new static factory methods in Credentials?

Any suggestion for the name? I would expect fromGeneratedPassword() in Credentials and SnapshotCredentials to behave the same way but it will not be the case if we add a new static method in SnapshotCredentials.

How about fromGeneratedSecret() both in Credentials and SnapshotCredentials then?

skinny85 · 2020-11-05T18:25:30Z

I don't love that. How about new static factory methods in SnapshotCredentials, similar to the new static factory methods in Credentials?

Any suggestion for the name? I would expect fromGeneratedPassword() in Credentials and SnapshotCredentials to behave the same way but it will not be the case if we add a new static method in SnapshotCredentials.

How about fromGeneratedSecret() both in Credentials and SnapshotCredentials then?

That's a good point about consistency between Credentials and SnapshotCredentials.

I like fromGeneratedSecret() well enough.

skinny85

LGTM, thanks for taking care of this @jogold !

Pull request has been modified.

jogold · 2020-11-06T17:24:40Z

@skinny85 can you reapprove here? Had to merge from master because the build was stuck...

aws-cdk-automation · 2020-11-06T17:51:17Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 87552f5
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2020-11-06T20:02:41Z

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

jogold added 4 commits November 1, 2020 16:57

DatabaseInstanceFromSnapshot

753b6de

make it unique

91b557f

comments

8f87ae4

more comments

c3c7f57

SomayaB assigned skinny85 Nov 2, 2020

skinny85 previously requested changes Nov 2, 2020

View reviewed changes

Update packages/@aws-cdk/aws-rds/lib/props.ts

40dcaad

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

jogold added 7 commits November 3, 2020 12:21

PR feedback

bf2f863

Merge branch 'rds-fixed-username' of github.com:jogold/aws-cdk into r…

e2ffabc

…ds-fixed-username

Merge branch 'master' into rds-fixed-username

d6c8cd9

fromGeneratedPassword

409b3fe

username cannot be optional

fe67b5a

minor

780f075

deprecated doc

40213fa

skinny85 reviewed Nov 3, 2020

View reviewed changes

jogold and others added 4 commits November 3, 2020 21:04

Apply suggestions from code review

5cc3165

Co-authored-by: Adam Ruka <adamruka85@gmail.com>

address more PR feedback

446d269

add logical id expectations

78c0943

Merge branch 'master' into rds-fixed-username

071a227

jogold requested a review from skinny85 November 3, 2020 20:58

skinny85 reviewed Nov 3, 2020

View reviewed changes

jogold added 2 commits November 4, 2020 11:08

logical id and hash

2d7abf0

replaceOnPasswordChanges

4e56f48

njlynch reviewed Nov 4, 2020

View reviewed changes

jogold and others added 2 commits November 4, 2020 18:55

Update packages/@aws-cdk/aws-rds/README.md

21a1995

Co-authored-by: Nick Lynch <nlynch@amazon.com>

Merge branch 'master' into rds-fixed-username

5da4c6a

jogold added 2 commits November 4, 2020 21:17

replaceOnPasswordCriteriaChanges

f360d8c

Merge branch 'rds-fixed-username' of github.com:jogold/aws-cdk into r…

66d7a65

…ds-fixed-username

jogold added 2 commits November 5, 2020 21:51

fromGeneratedSecret

bec7ba3

Merge branch 'master' into rds-fixed-username

f26a5a7

jogold requested a review from skinny85 November 5, 2020 20:59

SecretsManager -> Secrets Manager

5c200b9

skinny85 previously approved these changes Nov 5, 2020

View reviewed changes

Merge branch 'master' into rds-fixed-username

87552f5

skinny85 approved these changes Nov 6, 2020

View reviewed changes

mergify bot merged commit a4567f5 into aws:master Nov 6, 2020

jogold deleted the rds-fixed-username branch November 6, 2020 20:18

jogold mentioned this pull request May 21, 2022

aws-rds: generate password/secret with SnapshotCredentials #20432

Closed

fix(rds): customizing secret results in unusable password and lost attachment #11237

fix(rds): customizing secret results in unusable password and lost attachment #11237

Conversation

jogold commented Oct 31, 2020 • edited Loading

gitpod-io bot commented Oct 31, 2020 • edited Loading

skinny85 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jogold Nov 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jogold Nov 3, 2020 • edited Loading

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

jogold Nov 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jogold Nov 3, 2020 • edited Loading

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

jogold commented Nov 3, 2020 • edited Loading

skinny85 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skinny85 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

jogold commented Nov 4, 2020 • edited Loading

skinny85 commented Nov 4, 2020

jogold commented Nov 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jogold commented Nov 4, 2020

skinny85 commented Nov 4, 2020

jogold commented Nov 5, 2020

skinny85 commented Nov 5, 2020

skinny85 left a comment

Choose a reason for hiding this comment

jogold commented Nov 6, 2020

aws-cdk-automation commented Nov 6, 2020

AWS CodeBuild CI Report

mergify bot commented Nov 6, 2020

jogold commented Oct 31, 2020 •

edited

Loading

gitpod-io bot commented Oct 31, 2020 •

edited

Loading

jogold Nov 2, 2020 •

edited

Loading

jogold Nov 3, 2020 •

edited

Loading

jogold Nov 2, 2020 •

edited

Loading

jogold Nov 3, 2020 •

edited

Loading

jogold commented Nov 3, 2020 •

edited

Loading

jogold commented Nov 4, 2020 •

edited

Loading

jogold commented Nov 4, 2020 •

edited

Loading