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.

Sign up for GitHub

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

fix(redshift): Fix Redshift User Secret Multi-User Rotation #28856

Closed
wants to merge 1 commit into from
Closed

fix(redshift): Fix Redshift User Secret Multi-User Rotation #28856

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions packages/@aws-cdk/aws-redshift-alpha/lib/database-secret.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,13 @@ export interface DatabaseSecretProps {
* @default default master key
*/
readonly encryptionKey?: kms.IKey;

/**
* The master secret which will be used to rotate this secret.
*
* @default - no master secret information will be included
*/
readonly masterSecret?: secretsmanager.ISecret;
}

/**
Expand All @@ -30,10 +37,13 @@ export class DatabaseSecret extends secretsmanager.Secret {
encryptionKey: props.encryptionKey,
generateSecretString: {
passwordLength: 30, // Redshift password could be up to 64 characters
secretStringTemplate: JSON.stringify({ username: props.username }),
secretStringTemplate: JSON.stringify({
username: props.username,
masterarn: props.masterSecret?.secretArn,
}),
generateStringKey: 'password',
excludeCharacters: '"@/\\\ \'',
},
});
}
}
}
1 change: 1 addition & 0 deletions packages/@aws-cdk/aws-redshift-alpha/lib/user.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,7 @@ export class User extends UserBase {
const secret = new DatabaseSecret(this, 'Secret', {
username,
encryptionKey: props.encryptionKey,
masterSecret: props.adminUser,
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about this.
Shouldn't we simply pass a DatabaseSecret with masterSecret set to the addRotationMultiUser method?
It seems like we might be assigning unnecessary permissions to the user here.
Also, I think we should validate that the secret for addRotationMultiUser has the required masterSecret value set here.

Copy link
Author

@penniman26 penniman26 Jan 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Absolutely! I was having the same conversation with a teammate this morning about that. The secret itself shouldn't need to encode the masterSecret reference especially since there is nothing secret about that reference (nor anything bad that could happen if an incorrect or different arn was set - the rotation lambda would just fetch wrong cluster admin credentials and fail to make the update!). Unfortunately, I think we may be bound by the implementation of addRotationMultiUser and the redshift multi-user rotation lambda secrets manager provides which expects masterarn to be encoded in the secret.

Re assigning unnecessary permissions, I see your point about sending masterSecret into DatabaseSecret but at impl time its just encoding the master secret ARN w/o additional permissions. Its the rotation lambda that has permission to the masterSecret and secret to perform the rotation.

On the validation, I don't know if we can perform that validation at build time since options.secret is a reference to ISecret and not DatbaseSecret :/ - thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense and I agree with your analysis of the current implementation.
Thanks for the detailed follow-up 👍
On validation, we could expose the masterSecret property publicly in DatabaseSecret and perform a type-check in addRotationMultiUser for the secret.
I think it might be worth adding to provide clearer feedback to users invoking the method without providing the necessary parameters.

});
const attachedSecret = secret.attach(props.cluster);
this.password = attachedSecret.secretValueFromJson('password');
Expand Down
19 changes: 19 additions & 0 deletions ...user.js.snapshot/ClusterMultiUserRotationIntegDefaultTestDeployAssert9A5C1D17.assets.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

36 changes: 36 additions & 0 deletions ...er.js.snapshot/ClusterMultiUserRotationIntegDefaultTestDeployAssert9A5C1D17.template.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 10 additions & 0 deletions ...ot/asset.6bdd909f81c84ffe7d00cf4d6a2dbac8606429bcc05b0db3da842c1941a532f2/handler-name.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

21 changes: 21 additions & 0 deletions ....snapshot/asset.6bdd909f81c84ffe7d00cf4d6a2dbac8606429bcc05b0db3da842c1941a532f2/index.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

65 changes: 65 additions & 0 deletions ...shot/asset.6bdd909f81c84ffe7d00cf4d6a2dbac8606429bcc05b0db3da842c1941a532f2/privileges.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading