feat(secretsmanager/rds): support credential rotation #2052

jogold · 2019-03-19T12:01:55Z

Add construct for rotation schedule, secret target attachment and RDS rotation single user.

This is needed in order to implement something like cluster.addRotationSingleUser(...) in aws-rds which will allow to create a cluster (or instance, no construct yet) with a secrets manager generated master password and add rotation to it (PR to follow if this one is accepted).

Question: where does the construct RdsRotationSingleUser fit? Here in aws-secretsmanager or in aws-rds?

Pull Request Checklist

Testing
- Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
- CLI change?: coordinate update of integration tests with team
- cdk-init template change?: coordinated update of integration tests with team
Docs
- jsdocs: All public APIs documented
- README: README and/or documentation topic updated
Title and Description
- Change type: title prefixed with fix, feat will appear in changelog
- Title: use lower-case and doesn't end with a period
- Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
- Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
Sensitive Modules (requires 2 PR approvers)
- IAM Policy Document (in @aws-cdk/aws-iam)
- EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
- Grant APIs (only if not based on official documentation with a reference)

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

Add construct for rotation schedule, secret target attachment and RDS rotation single user.

rix0rrr

Please also describe these classes in the README. A section should be something like "how to set up credential rotation"

rix0rrr · 2019-03-19T12:15:53Z

packages/@aws-cdk/aws-secretsmanager/lib/rds-rotation-single-user.ts

+  /**
+   * The connections object of the RDS database instance or cluster.
+   */
+  connections: ec2.Connections;


I think I'd rather you pass an IConnectable here. Any reason that wouldn't work?

You are right, this should have been an IConnectable.

rix0rrr · 2019-03-19T13:22:16Z

packages/@aws-cdk/aws-secretsmanager/lib/secret-target-attachment.ts

+/**
+ * Options to add a secret attachement to a secret.
+ */
+export interface SecretTargetAttachmentOptions {


Feel like this should be an interface like:

export interface ISecretAttachmentTarget { asSecretAttachmentTarget(): SecretAttachmentTargetProps; } export interface SecretAttachmentTargetProps { targetId: string; targetType: AttachmentTargetType; } export interface SecretTargetAttachmentOptions { target: ISecretAttachmentTarget; }

And then have the DBInstance and DBCluster classes implement ISecretAttachmentTarget.

rix0rrr · 2019-03-19T13:25:55Z

packages/@aws-cdk/aws-secretsmanager/lib/secret-target-attachment.ts

+    // This allows to reference the secret after attachment (dependency). When
+    // creating a secret for a RDS cluster or instance this is the secret that
+    // will be used as the input for the rotation.
+    this.secret = Secret.import(this, 'Secret', {


It feels like this is something that users might forget, especially since these docs are in implementation notes.

Another idea (not necessarily saying you must do this), how about changing this into:

class AttachedSecret implements ISecret { ... }

So that an AttachedSecret can be used anywhere a secret can be used? And if not that, then at least put these notes in a public comment somewhere, seems very relevant to this construct's usage.

I like this idea of the AttachedSecret

rix0rrr · 2019-03-19T13:28:08Z

packages/@aws-cdk/aws-secretsmanager/lib/rds-rotation-single-user.ts

@@ -0,0 +1,204 @@
+import ec2 = require('@aws-cdk/aws-ec2');


Feels like this whole file should live in the aws-rds package.

OK to move the RDS rotation, in the same PR then? It breaks the conventional commit + squash convention here...

Not sure what you mean. The only thing it might break is the scope, if the scope were a single package.

I would feel comfortable with a feat(secretsmanager/rds): support credential rotation.

jogold · 2019-03-19T19:25:11Z

Since there's already a breaking change for aws-rds coming from #2025, can we add here the switch from kmsKeyArn: string to kmsKey: kms.IEncryptionKey in the DatabaseClusterProps?

rix0rrr

Jonathan, I think this is just fantastic. Thank you!

rix0rrr · 2019-03-20T10:38:56Z

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

@@ -53,3 +53,28 @@ attributes:
 ```ts
 const writeAddress = cluster.clusterEndpoint.socketAddress;   // "HOSTNAME:PORT"
 ```
+
+### Rotating master password
+When the master password is generated and stored in AWS Secrets Manager, it can be rotated automatically: [example of setting up master password rotation](test/integ.cluster-rotation.lit.ts)


This needs to be on a line by itself.

rix0rrr · 2019-03-20T10:39:20Z

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

 */
-export class DatabaseCluster extends cdk.Construct implements IDatabaseCluster {
+export abstract class DatabaseClusterBase extends cdk.Construct implements IDatabaseCluster {


rix0rrr · 2019-03-20T10:40:11Z

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

-   * Parameter or the SSM Parameter Store instead.
+   * Do not put passwords in your CDK code directly.
+   *
+   * @default a Secrets Manager generated password


rix0rrr · 2019-03-20T10:46:36Z

Will merge after current release process is over. You just missed the cut, sorry, it will be in the next release.

jogold · 2019-03-20T10:47:44Z

No prob, have you seen this?

Since there's already a breaking change for aws-rds coming from #2025, can we add here the switch from kmsKeyArn: string to kmsKey: kms.IEncryptionKey in the DatabaseClusterProps?

rix0rrr · 2019-03-20T10:52:24Z

Oh totally. Though can you create a new PR for that please?

jogold · 2019-03-20T20:36:30Z

Oh totally. Though can you create a new PR for that please?

See #2063

feat(secretsmanager): add construct for RDS rotation single user

0a39df7

Add construct for rotation schedule, secret target attachment and RDS rotation single user.

jogold requested a review from a team as a code owner March 19, 2019 12:01

rix0rrr reviewed Mar 19, 2019

View reviewed changes

jogold added 8 commits March 19, 2019 15:59

Use a IConnectable for rds rotation

1f1742b

Replace SecretTargetAttachment by AttachedSecret

bce6128

Move rds rotation to aws-rds

81f2c5b

Implement ISecretAttachmentTarget for cluster

6554f2f

Update secretsmanager README

2a99fb4

Add secret and rotation to cluster

87cf249

Add integration tests for cluster rotation

d16eaa2

Update README for aws-rds

e381a81

jogold changed the title ~~feat(secretsmanager): add construct for RDS rotation single user~~ feat(secretsmanager/rds): support credential rotation Mar 19, 2019

jogold added 4 commits March 19, 2019 19:51

Typo

e2b9f13

Fix dependencies

f014a07

Rename test

9aa5fe7

Remove default username in DatabaseSecret

f4b4403

jogold added 3 commits March 19, 2019 20:29

Fix typos

304c1d5

Merge branch 'master' into secret-rotation

ea1a32e

Update README for aws-rds

df59fca

rix0rrr approved these changes Mar 20, 2019

View reviewed changes

Move literal example to new line

fdb3236

Merge branch 'master' into secret-rotation

312d551

rix0rrr merged commit bf79c82 into aws:master Mar 20, 2019

jogold deleted the secret-rotation branch March 20, 2019 18:19

jogold mentioned this pull request Apr 24, 2019

feat(rds): add support for database instances #2187

Merged

4 tasks

This was referenced Dec 12, 2019

[Snyk] Upgrade @aws-cdk/cdk from 0.22.0 to 0.35.0 MechanicalRock/scdk#3

Closed

[Snyk] Upgrade @aws-cdk/cdk from 0.24.1 to 0.35.0 MechanicalRock/cdk-constructs#8

Open

[Snyk] Upgrade @aws-cdk/cdk from 0.24.1 to 0.35.0 MechanicalRock/account-reaper#6

Open

njlynch mentioned this pull request Apr 14, 2021

Another integration: secretsmanager.ISecretAttachmentTarget #2890

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(secretsmanager/rds): support credential rotation #2052

feat(secretsmanager/rds): support credential rotation #2052

jogold commented Mar 19, 2019 •

edited

Loading

rix0rrr left a comment

rix0rrr Mar 19, 2019

jogold Mar 19, 2019

rix0rrr Mar 19, 2019

rix0rrr Mar 19, 2019

jogold Mar 19, 2019

rix0rrr Mar 19, 2019

jogold Mar 19, 2019 •

edited

Loading

rix0rrr Mar 19, 2019

jogold commented Mar 19, 2019 •

edited

Loading

rix0rrr left a comment

rix0rrr Mar 20, 2019

rix0rrr Mar 20, 2019

rix0rrr Mar 20, 2019

rix0rrr commented Mar 20, 2019

jogold commented Mar 20, 2019

rix0rrr commented Mar 20, 2019

jogold commented Mar 20, 2019

feat(secretsmanager/rds): support credential rotation #2052

feat(secretsmanager/rds): support credential rotation #2052

Conversation

jogold commented Mar 19, 2019 • edited Loading

Pull Request Checklist

rix0rrr 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

Choose a reason for hiding this comment

jogold Mar 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jogold commented Mar 19, 2019 • edited Loading

rix0rrr 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

rix0rrr commented Mar 20, 2019

jogold commented Mar 20, 2019

rix0rrr commented Mar 20, 2019

jogold commented Mar 20, 2019

jogold commented Mar 19, 2019 •

edited

Loading

jogold Mar 19, 2019 •

edited

Loading

jogold commented Mar 19, 2019 •

edited

Loading