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.

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(rds): clusters created from snapshots generate incorrect passwords #20504

Merged
merged 8 commits into from
Jun 2, 2022

Conversation

jogold
Copy link
Contributor

@jogold jogold commented May 26, 2022

Deprecate credentials and explain how it is broken.

Replace it with snapshotCredentials that offer the expected behavior.

Fixes #20434
Closes #20473


All Submissions:

Adding new Unconventional Dependencies:

  • This PR adds new unconventional dependencies following the process described here

New Features

  • Have you added the new feature to an integration test?
    • Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

@gitpod-io
Copy link

gitpod-io bot commented May 26, 2022

@aws-cdk-automation aws-cdk-automation requested a review from a team May 26, 2022 09:57
// eslint-disable-next-line no-duplicate-imports, import/order
import { Construct as CoreConstruct } from '@aws-cdk/core';

class TestStack extends Stack {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@peterwoodworth please check the integ test here.

I'm doing the following:

  • Create a new cluster with a generated secret
  • Take a snapshot (using a custom resource)
  • Create a cluster from this snapshot with SnapshotCredentials.fromGeneratedSecret() to change the master user password in this cluster
  • Add rotation to this cluster

This results in a cluster created from the snapshot with new credentials. If I go in the Secrets Manager console I can click "Rotate secret immediately" and change the master user password of this cluster without any issues.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rix0rrr FYI

@jogold
Copy link
Contributor Author

jogold commented May 26, 2022

TODO: add unit tests

@github-actions github-actions bot added bug This issue is a bug. effort/medium Medium work item – several days of effort p1 labels May 26, 2022

const cluster = new CfnDBCluster(this, 'Resource', {
...this.newCfnProps,
snapshotIdentifier: props.snapshotIdentifier,
masterUserPassword: secret?.secretValueFromJson('password')?.unsafeUnwrap() ?? credentials?.password?.unsafeUnwrap(), // Safe usage
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the functionality we're trying to avoid, since CloudFormation doesn't support setting this property when also setting the snapshotIdentifier

It deploys - but from my testing last week, the masterUserPassword prop ends up getting ignored and as a result the generated secret has an incorrect password. I had to manually change the password of the secret which was generated to be the secret which the initial db was created with to be able to login.

Were you able to successfully login to the DB after creating a cluster this way? I'm also curious if you were able to login to the DB after rotating credentials.

Copy link
Contributor Author

@jogold jogold May 27, 2022

Choose a reason for hiding this comment

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

The documentation is incorrect. It is supported, it works exactly like for a DB instance (where the doc also says that the master password should not be specified for a snapshot). For a cluster you need to wait until all instances finish deploying before RDS changes the password (for an instance you also have to wait, first the instance is restored from the snapshot with the snapshot password then the new password is applied).

Rotation is the confirmation that it works. It only succeeds (= sets the new password in Secrets Manager) if it can log in to the cluster (see code)

You can deploy my integ test to test this.

Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

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

Thanks for getting to the bottom of this folks!

@mergify
Copy link
Contributor

mergify bot commented Jun 1, 2022

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 jogold changed the base branch from v1-main to main June 2, 2022 07:10
@jogold
Copy link
Contributor Author

jogold commented Jun 2, 2022

@rix0rrr can you reapprove here?

Changed the base branch to main + the build was failing with:

@aws-cdk/integ-runner: FAIL test/runner/snapshot-test-runner.test.js
@aws-cdk/integ-runner:   � IntegTest runSnapshotTests › diff asset hashes
@aws-cdk/integ-runner:     Cannot read integ manifest 'test/test-data/test-with-snapshot-assets-diff.integ.snapshot/manifest.json': Unexpected end of JSON input
@aws-cdk/integ-runner:       32 |
@aws-cdk/integ-runner:       33 |     } catch (e) {
@aws-cdk/integ-runner:     > 34 |       throw new Error(`Cannot read integ manifest '${fileName}': ${e.message}`);
@aws-cdk/integ-runner:          |             ^
@aws-cdk/integ-runner:       35 |     }
@aws-cdk/integ-runner:       36 |   }
@aws-cdk/integ-runner:       37 |
@aws-cdk/integ-runner:       at Function.fromFile (lib/runner/private/cloud-assembly.ts:34:13)
@aws-cdk/integ-runner:       at Function.fromPath (lib/runner/private/cloud-assembly.ts:51:37)
@aws-cdk/integ-runner:       at IntegSnapshotRunner.readAssembly (lib/runner/snapshot-test-runner.ts:223:45)
@aws-cdk/integ-runner:       at IntegSnapshotRunner.testSnapshot (lib/runner/snapshot-test-runner.ts:55:33)
@aws-cdk/integ-runner:       at Object.<anonymous> (test/runner/snapshot-test-runner.test.ts:175:31)

which has nothing to do with this PR, I've seen this a few times, maybe some kind of race condition in @aws-cdk/integ-runner.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 3362dd4
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify
Copy link
Contributor

mergify bot commented Jun 2, 2022

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

@MitchWijt
Copy link

@jogold I am currently trying this functionality out, And I have been stuck for almost a week, since I can't login to my Cluster created from a snapshot even though I am using the snapshotCredentials prop. Is it mandatory to add a rotation to the cluster in order for the master secret to be changed?

@jogold
Copy link
Contributor Author

jogold commented Aug 31, 2022

Is it mandatory to add a rotation to the cluster in order for the master secret to be changed?

No you don't need to add a rotation to the cluster.

Are you using rds.SnapshotCredentials.fromGeneratedSecret()?

The integ test at https://github.com/jogold/aws-cdk/blob/main/packages/@aws-cdk/aws-rds/test/integ.cluster-snapshot.ts shows a working example. It uses a rotation to "prove" that the master password is correctly updated, otherwise the rotation would fail.

Are you also not able to log in with the "old" password? Do you see logs in the RDS console with something like "resetting master credentials"?

@MitchWijt
Copy link

@jogold
I am indeed using rds.SnapshotCredentials.fromGeneratedSecret().

I am able to login using the "old" password. But not with the new password generated from rds.SnapshotCredentials.fromGeneratedSecret()

@jogold
Copy link
Contributor Author

jogold commented Sep 1, 2022

@MitchWijt can you share the AWS::RDS::DBCluster and AWS::SecretsManager::SecretTargetAttachment parts of the resulting CF template?

@MitchWijt
Copy link

MitchWijt commented Sep 5, 2022

@jogold

DBCluster:

"HabitatAuroraDBFromSnapshotpostgreshabitatAuroraTessnapshotDatabaseClusterFromSnapshotC3665919": {
   "Type": "AWS::RDS::DBCluster",
   "Properties": {
    "Engine": "aurora-postgresql",
    "BackupRetentionPeriod": 7,
    "CopyTagsToSnapshot": true,
    "DatabaseName": "postgres",
    "DBClusterIdentifier": "databasecluster-habitataurorates-snapshot",
    "DBClusterParameterGroupName": "default.aurora-postgresql13",
    "DBSubnetGroupName": {
     "Ref": "HabitatAuroraDBFromSnapshotpostgreshabitatAuroraTessnapshotDatabaseClusterFromSnapshotSubnetsCDF23442"
    },
    "DeletionProtection": true,
    "EnableCloudwatchLogsExports": [
     "postgresql"
    ],
    "EngineMode": "provisioned",
    "EngineVersion": "13.6",
    "Port": 5432,
    "SnapshotIdentifier": "arn:aws:rds:eu-west-1:12345:cluster-snapshot:snapshot-tester",
    "Tags": [
     {
      "Key": "STACK_NAME",
      "Value": "HabitatAuroraFromSnapshot"
     }
    ],
    "VpcSecurityGroupIds": [
     {
      "Fn::GetAtt": [
       "HabitatAuroraDBFromSnapshotpostgreshabitatAuroraTessnapshotDatabaseClusterFromSnapshotSecurityGroupA3CCFD10",
       "GroupId"
      ]
     }
    ]
   },
   "UpdateReplacePolicy": "Retain",
   "DeletionPolicy": "Retain",
   "Metadata": {
    "aws:cdk:path": "HabitatAuroraFromSnapshot/HabitatAuroraDBFromSnapshot/postgres-habitatAuroraTes-snapshot/DatabaseClusterFromSnapshot/Resource"
   }
  }

SecretTargetAttachment:

"HabitatAuroraDBFromSnapshotpostgreshabitatAuroraTessnapshotDatabaseClusterFromSnapshotSecretAttachment31BD3F16": {
   "Type": "AWS::SecretsManager::SecretTargetAttachment",
   "Properties": {
    "SecretId": {
     "Ref": "HabitatAuroraDBFromSnapshotpostgreshabitatAuroraTessnapshotDatabaseClusterFromSnapshotSecret07179FD8"
    },
    "TargetId": {
     "Ref": "HabitatAuroraDBFromSnapshotpostgreshabitatAuroraTessnapshotDatabaseClusterFromSnapshotC3665919"
    },
    "TargetType": "AWS::RDS::DBCluster"
   },
   "Metadata": {
    "aws:cdk:path": "HabitatAuroraFromSnapshot/HabitatAuroraDBFromSnapshot/postgres-habitatAuroraTes-snapshot/DatabaseClusterFromSnapshot/Secret/Attachment/Resource"
   }
  }

@MitchWijt
Copy link

@jogold The problem is that it indeed does create a new secret in the secrets manager. But I can't seem to login with the secret that is being returned from the cluster. (that newly created secret)

@jogold
Copy link
Contributor Author

jogold commented Sep 5, 2022

@jogold The problem is that it indeed does create a new secret in the secrets manager. But I can't seem to login with the secret that is being returned from the cluster. (that newly created secret)

From your CF template I don't see the MasterUserPassword property being set. That's why the password is not updated. You should have something like this in your template

"MasterUserPassword": {
"Fn::Join": [
"",
[
"{{resolve:secretsmanager:",
{
"Ref": "cdkintegclustersnapshotFromSnapshotSnapshotSecretD93327943fdaad7efa858a3daf9490cf0a702aeb"
},
":SecretString:password::}}"
]
]
},
"SnapshotIdentifier": {

Are you using snapshotCredentials or the deprecated credentials property? You need snapshotCredentials, see

snapshotCredentials: rds.SnapshotCredentials.fromGeneratedSecret('admin'),

@MitchWijt
Copy link

@jogold

I indeed do not see anything related to the MasterUserPassword. And yes I am using snapshotCredentials :
snapshotCredentials: SnapshotCredentials.fromGeneratedSecret('postgres')

@MitchWijt
Copy link

MitchWijt commented Sep 5, 2022

Here is the CDK code I am using at the moment:

I am on aws-cdk-lib version: 2.38.1

new rds.DatabaseClusterFromSnapshot(this, `DatabaseClusterFromSnapshot`, {
            defaultDatabaseName: "postgres",
            engine: rds.DatabaseClusterEngine.auroraPostgres({
                version: AuroraPostgresEngineVersion.VER_13_6,
            }),
            instances: 1,
            backup: {
                retention: Duration.days(7)
            },
            removalPolicy: RemovalPolicy.RETAIN,
            instanceProps: {
                vpc: vpc,
                instanceType: CustomInstanceType.SERVERLESS as unknown as ec2.InstanceType,
                autoMinorVersionUpgrade: true,
                publiclyAccessible: false,
                vpcSubnets: {
                    subnets: vpc.isolatedSubnets
                },
            },
            cloudwatchLogsExports: ["postgresql"],
            snapshotCredentials: rds.SnapshotCredentials.fromGeneratedSecret('postgres'),
            snapshotIdentifier: props.snapshotIdentifier
})

@MitchWijt
Copy link

@jogold I just tried again doing a cdk synth. Had my linked npm path wrongly set. Seems like MasterUserPass is present this time. But I am still nog able to login with the new password:

"HabitatAuroraDBFromSnapshotpostgreshabitatAuroraTessnapshotDatabaseClusterFromSnapshotC3665919": {
   "Type": "AWS::RDS::DBCluster",
   "Properties": {
    "Engine": "aurora-postgresql",
    "BackupRetentionPeriod": 7,
    "CopyTagsToSnapshot": true,
    "DatabaseName": "postgres",
    "DBClusterIdentifier": "databasecluster-habitataurorates-snapshot",
    "DBClusterParameterGroupName": "default.aurora-postgresql13",
    "DBSubnetGroupName": {
     "Ref": "HabitatAuroraDBFromSnapshotpostgreshabitatAuroraTessnapshotDatabaseClusterFromSnapshotSubnetsCDF23442"
    },
    "DeletionProtection": true,
    "EnableCloudwatchLogsExports": [
     "postgresql"
    ],
    "EngineMode": "provisioned",
    "EngineVersion": "13.6",
    "MasterUserPassword": {
     "Fn::Join": [
      "",
      [
       "{{resolve:secretsmanager:",
       {
        "Ref": "HabitatAuroraFromSnapshotHabitatAuroraDBFromSnapshotpostgreshabitatAuroraTessnapshotDatabaseClusterFromSnapshotSnapshotSecret5D8A78963fdaad7efa858a3daf9490cf0a702aeb"
       },
       ":SecretString:password::}}"
      ]
     ]
    },
    "Port": 5432,
    "SnapshotIdentifier": "arn:aws:rds:eu-west-1:123456:cluster-snapshot:snapshot-testereeeeee",
    "Tags": [
     {
      "Key": "STACK_NAME",
      "Value": "HabitatAuroraFromSnapshot"
     }
    ],
    "VpcSecurityGroupIds": [
     {
      "Fn::GetAtt": [
       "HabitatAuroraDBFromSnapshotpostgreshabitatAuroraTessnapshotDatabaseClusterFromSnapshotSecurityGroupA3CCFD10",
       "GroupId"
      ]
     }
    ]
   },
   "UpdateReplacePolicy": "Retain",
   "DeletionPolicy": "Retain",
   "Metadata": {
    "aws:cdk:path": "HabitatAuroraFromSnapshot/HabitatAuroraDBFromSnapshot/postgres-habitatAuroraTes-snapshot/DatabaseClusterFromSnapshot/Resource"
   }
  },

@MitchWijt
Copy link

Seems like im getting 2 SecretTargetAttachments, for 2 generated secrets:

Secret 1 + Secret TargetAttachment:

"HabitatAuroraFromSnapshotHabitatAuroraDBFromSnapshotpostgreshabitatAuroraTessnapshotDatabaseClusterFromSnapshotSnapshotSecret5D8A78963fdaad7efa858a3daf9490cf0a702aeb": {
   "Type": "AWS::SecretsManager::Secret",
   "Properties": {
    "Description": {
     "Fn::Join": [
      "",
      [
       "Generated by the CDK for stack: ",
       {
        "Ref": "AWS::StackName"
       }
      ]
     ]
    },
    "GenerateSecretString": {
     "ExcludeCharacters": " %+~`#$&*()|[]{}:;<>?!'/@\"\\",
     "GenerateStringKey": "password",
     "PasswordLength": 30,
     "SecretStringTemplate": "{\"username\":\"postgres\"}"
    },
    "Tags": [
     {
      "Key": "STACK_NAME",
      "Value": "HabitatAuroraFromSnapshot"
     }
    ]
   },
   "UpdateReplacePolicy": "Delete",
   "DeletionPolicy": "Delete",
   "Metadata": {
    "aws:cdk:path": "HabitatAuroraFromSnapshot/HabitatAuroraDBFromSnapshot/postgres-habitatAuroraTes-snapshot/DatabaseClusterFromSnapshot/SnapshotSecret/Resource"
   }
  }
"HabitatAuroraDBFromSnapshotpostgreshabitatAuroraTessnapshotDatabaseClusterFromSnapshotSnapshotSecretAttachment7A7C8C14": {
   "Type": "AWS::SecretsManager::SecretTargetAttachment",
   "Properties": {
    "SecretId": {
     "Ref": "HabitatAuroraFromSnapshotHabitatAuroraDBFromSnapshotpostgreshabitatAuroraTessnapshotDatabaseClusterFromSnapshotSnapshotSecret5D8A78963fdaad7efa858a3daf9490cf0a702aeb"
    },
    "TargetId": {
     "Ref": "HabitatAuroraDBFromSnapshotpostgreshabitatAuroraTessnapshotDatabaseClusterFromSnapshotC3665919"
    },
    "TargetType": "AWS::RDS::DBCluster"
   },
   "Metadata": {
    "aws:cdk:path": "HabitatAuroraFromSnapshot/HabitatAuroraDBFromSnapshot/postgres-habitatAuroraTes-snapshot/DatabaseClusterFromSnapshot/SnapshotSecret/Attachment/Resource"
   }
  }

Secret 2 + SecretTargetAttachment:

"HabitatAuroraDBFromSnapshotpostgreshabitatAuroraTessnapshotDatabaseClusterFromSnapshotSecret07179FD8": {
 "Type": "AWS::SecretsManager::Secret",
 "Properties": {
  "Description": {
   "Fn::Join": [
    "",
    [
     "Generated by the CDK for stack: ",
     {
      "Ref": "AWS::StackName"
     }
    ]
   ]
  },
  "GenerateSecretString": {
   "ExcludeCharacters": " %+~`#$&*()|[]{}:;<>?!'/@\"\\",
   "GenerateStringKey": "password",
   "PasswordLength": 30,
   "SecretStringTemplate": "{\"username\":\"postgres\"}"
  },
  "Tags": [
   {
    "Key": "STACK_NAME",
    "Value": "HabitatAuroraFromSnapshot"
   }
  ]
 },
 "UpdateReplacePolicy": "Delete",
 "DeletionPolicy": "Delete",
 "Metadata": {
  "aws:cdk:path": "HabitatAuroraFromSnapshot/HabitatAuroraDBFromSnapshot/postgres-habitatAuroraTes-snapshot/DatabaseClusterFromSnapshot/Secret/Resource"
 }
}
 "HabitatAuroraDBFromSnapshotpostgreshabitatAuroraTessnapshotDatabaseClusterFromSnapshotSecretAttachment31BD3F16": {
 "Type": "AWS::SecretsManager::SecretTargetAttachment",
 "Properties": {
  "SecretId": {
   "Ref": "HabitatAuroraDBFromSnapshotpostgreshabitatAuroraTessnapshotDatabaseClusterFromSnapshotSecret07179FD8"
  },
  "TargetId": {
   "Ref": "HabitatAuroraDBFromSnapshotpostgreshabitatAuroraTessnapshotDatabaseClusterFromSnapshotC3665919"
  },
  "TargetType": "AWS::RDS::DBCluster"
 },
 "Metadata": {
  "aws:cdk:path": "HabitatAuroraFromSnapshot/HabitatAuroraDBFromSnapshot/postgres-habitatAuroraTes-snapshot/DatabaseClusterFromSnapshot/Secret/Attachment/Resource"
 }
}

Could that be the issue?

@jogold
Copy link
Contributor Author

jogold commented Sep 5, 2022

Could that be the issue?

This is because of the default of the deprecated prop, see #20777 (comment)

The secret referenced by the MasterUserPassword CF property should contain the new password of the cluster. You may have to wait a bit after deploy until it is updated in all your instances.

@MitchWijt
Copy link

@jogold Will this master password change throw out a RDS-EVENT-0016 event when it finished changing the password?
Screenshot 2022-09-05 at 14 44 06

@jogold
Copy link
Contributor Author

jogold commented Sep 5, 2022

@jogold Will this master password change throw out a RDS-EVENT-0016 event when it finished changing the password? Screenshot 2022-09-05 at 14 44 06

I don't know.

@MitchWijt
Copy link

MitchWijt commented Sep 6, 2022

@jogold To give a little update. I decided to try again and wait for about 15 minutes after cluster creation. But I still can't login with the secret that was generated by the snapshotCredentials prop.

I confirmed that it still has the old password (from the original cluster). From which I am able to login with

@MitchWijt
Copy link

MitchWijt commented Sep 7, 2022

@jogold FYI: this has become a P1 bug at the moment:
#21730

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue is a bug. effort/medium Medium work item – several days of effort p1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RDS: Cannot connect to Serverless Cluster created from snapshot
5 participants