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

penniman26 · 2024-01-25T01:30:17Z

Fixes Redshift User Secret Multi-User Rotation for new Users by including masterarn in the Secret's Serialized JSON Object Text.

Note: This doesn't affect existing users (nor fixes roation for them) since the secret string template is only used when the secret is first created. For those existing secrets, the secret text will need to be updated to include masterarn using the GetSecretValue and UpdateSecret SecretManager APIs.

Closes #28852

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

Fixes Redshift User Secret Multi-User Rotation for new Users by including `masterarn` in the Secret's Serialized JSON Object Text. Note: This doesn't affect existing users (nor fixes roation for them) since the secret string template is only used when the secret is first created. For those existing secrets, the secret text will need to be updated to include `masterarn` using the GetSecretValue and UpdateSecret SecretManager APIs. closes aws#28852 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

aws-cdk-automation · 2024-01-25T01:58:04Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 4ac9db6
Result: SUCCEEDED
Build Logs (available for 30 days)

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

lpizzinidev

Thanks 👍
Looks good overall, I left a couple of comments on the implementation.
Also, can you please update the title to describe the bug (not the solution)?

lpizzinidev · 2024-01-25T18:19:20Z

packages/@aws-cdk/aws-redshift-alpha/test/user.test.ts


  beforeEach(() => {
    stack = new cdk.Stack();
    vpc = new ec2.Vpc(stack, 'VPC');
-    cluster = new redshift.Cluster(stack, 'Cluster', {
+    const clusterImpl = new redshift.Cluster(stack, 'Cluster', {


Why can't we use the global variable? 🤔

cluster is ICluster which does not have a .secret property. I figured that the rest of the tests wanted to continue to use the ICluster interface so I have this awkward code that has both an ICluster global variable and a Cluster local variable pointing to the same object but different types.

https://docs.aws.amazon.com/cdk/api/v2/docs/@aws-cdk_aws-redshift-alpha.ICluster.html
https://github.com/aws/aws-cdk/blob/v2.122.0/packages/%40aws-cdk/aws-redshift-alpha/lib/cluster.ts

Tests are running successfully if we change the type declaration:

let cluster: redshift.Cluster;

I think it should be safe to do so to avoid declaring the two variables on different scopes.

lpizzinidev · 2024-01-25T18:44:39Z

packages/@aws-cdk/aws-redshift-alpha/lib/user.ts

@@ -153,6 +153,7 @@ export class User extends UserBase {
    const secret = new DatabaseSecret(this, 'Secret', {
      username,
      encryptionKey: props.encryptionKey,
+      masterSecret: props.adminUser,


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.

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?

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.

penniman26 · 2024-01-26T01:56:49Z

Thanks 👍 Looks good overall, I left a couple of comments on the implementation. Also, can you please update the title to describe the bug (not the solution)?

Thank you and will do!

lpizzinidev · 2024-01-27T05:45:22Z

packages/@aws-cdk/aws-redshift-alpha/test/user.test.ts


  beforeEach(() => {
    stack = new cdk.Stack();
    vpc = new ec2.Vpc(stack, 'VPC');
-    cluster = new redshift.Cluster(stack, 'Cluster', {
+    const clusterImpl = new redshift.Cluster(stack, 'Cluster', {


Tests are running successfully if we change the type declaration:

let cluster: redshift.Cluster;

I think it should be safe to do so to avoid declaring the two variables on different scopes.

lpizzinidev · 2024-01-27T16:08:35Z

packages/@aws-cdk/aws-redshift-alpha/lib/user.ts

@@ -153,6 +153,7 @@ export class User extends UserBase {
    const secret = new DatabaseSecret(this, 'Secret', {
      username,
      encryptionKey: props.encryptionKey,
+      masterSecret: props.adminUser,


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.

comcalvi · 2024-04-25T15:08:29Z

@penniman26 This is a great change! Thank you for your patience as get to reviewing PRs. I'll merge this one once the changes from @lpizzinidev have been addressed.

aws-cdk-automation · 2024-09-29T00:11:38Z

This PR has been in the MERGE CONFLICTS state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

aws-cdk-automation · 2024-10-07T00:10:57Z

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

github-actions · 2024-10-07T00:11:15Z

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

aws-cdk-automation · 2024-10-07T00:13:12Z

The pull request linter fails with the following errors:

❌ The first word of the pull request title should not be capitalized. If the title starts with a CDK construct, it should be in backticks "``".

PRs must pass status checks before we can provide a meaningful review.

If you would like to request an exemption from the status checks or clarification on feedback, please leave a comment on this PR containing Exemption Request and/or Clarification Request.

penniman26 mentioned this pull request Jan 25, 2024

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

Closed

aws-cdk-automation requested a review from a team January 25, 2024 01:30

github-actions bot added bug This issue is a bug. p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Jan 25, 2024

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jan 25, 2024

lpizzinidev suggested changes Jan 25, 2024

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jan 25, 2024

lpizzinidev suggested changes Jan 27, 2024

View reviewed changes

github-actions bot mentioned this pull request Feb 1, 2024

Monthly PR metrics report - Jan 2024 #28952

Closed

aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Oct 7, 2024

aws-cdk-automation closed this Oct 7, 2024

github-actions bot locked as resolved and limited conversation to collaborators Oct 7, 2024

aws-cdk-automation requested changes Oct 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

penniman26 commented Jan 25, 2024

aws-cdk-automation commented Jan 25, 2024

lpizzinidev left a comment

lpizzinidev Jan 25, 2024

penniman26 Jan 25, 2024

lpizzinidev Jan 27, 2024

lpizzinidev Jan 25, 2024

penniman26 Jan 25, 2024 •

edited

Loading

lpizzinidev Jan 27, 2024

penniman26 commented Jan 26, 2024

lpizzinidev Jan 27, 2024

lpizzinidev Jan 27, 2024

comcalvi commented Apr 25, 2024

aws-cdk-automation commented Sep 29, 2024

aws-cdk-automation commented Oct 7, 2024

github-actions bot commented Oct 7, 2024

aws-cdk-automation left a comment

aws-cdk-automation commented Oct 7, 2024

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

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

Conversation

penniman26 commented Jan 25, 2024

aws-cdk-automation commented Jan 25, 2024

AWS CodeBuild CI Report

lpizzinidev left a comment

Choose a reason for hiding this comment

lpizzinidev Jan 25, 2024

Choose a reason for hiding this comment

penniman26 Jan 25, 2024

Choose a reason for hiding this comment

lpizzinidev Jan 27, 2024

Choose a reason for hiding this comment

lpizzinidev Jan 25, 2024

Choose a reason for hiding this comment

penniman26 Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

lpizzinidev Jan 27, 2024

Choose a reason for hiding this comment

penniman26 commented Jan 26, 2024

lpizzinidev Jan 27, 2024

Choose a reason for hiding this comment

lpizzinidev Jan 27, 2024

Choose a reason for hiding this comment

comcalvi commented Apr 25, 2024

aws-cdk-automation commented Sep 29, 2024

aws-cdk-automation commented Oct 7, 2024

github-actions bot commented Oct 7, 2024

aws-cdk-automation left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Oct 7, 2024

penniman26 Jan 25, 2024 •

edited

Loading