Updates to DB password via SecretManager does not work

[mjslane]

When RDS is first set up using secret manager a secret is created with the following format

{
  "password": "******",
  "dbname": "*****",
  "engine": "postgres",
  "port": 5432,
  "host": "******.rds.amazonaws.com",
  "username": "******"
}

If a change is made to the CDK that causes the password to be regenerated the password is updated but dbname, host, port and engine is removed from the secret. If services rely on this information being there it breaks the service.

Reproduction Steps

1. Deploy a stack using RDS for example

      const instance = new DatabaseInstance(this, `my-rds-instance`,{
            engine: DatabaseInstanceEngine.POSTGRES,
            instanceClass: props.rdsInstanceClass,
            masterUsername: props.rdsMasterUserName,
            vpc: props.vpc,
            enablePerformanceInsights: props.rdsInsights,
            vpcPlacement: {subnetType: SubnetType.PRIVATE},
            databaseName: "my-dbname",
            instanceIdentifier: `my-db-identifier`,
            removalPolicy: props.rdsRemovalPolicy,
            deletionProtection: deletionProtection
        });
        //fix secret issues
        const dbSecret = instance.node.tryFindChild('Secret') as DatabaseSecret;
        const cfnSecret = dbSecret.node.defaultChild as CfnSecret;
        cfnSecret.addPropertyOverride('GenerateSecretString.ExcludeCharacters', '\'\\;@$"`!/');

Check the secret generated.

Update the excluded characters by adding or removing some.

Redeploy

Check the secret again.

Environment

• **CLI Version :1.33.1 (build 8ad4d34)
• **OS :OSX Catalina
• **Language :typescript

This is 🐛 Bug Report

[nija-at]

Thanks for filing this issue. I was able to reproduce this.

This is a weird one -

I can confirm that the only thing that changes in the CloudFormation template between the two changes is the value of ExcludeCharacters. Everything else in the template is exactly the same, so I suspect something is different about how the Secrets Manager backend handles a create vs an update request.

However before that, the dbname, host and dbengine should not even be present in the secret generated in the first place. The SecretStringTemplate is configured to only contain the username and ARN of the mastersecret besides the generated password. It's not clear why it got into the secret manager value in the first place.
I can confirm this the case from the generated CloudFormation template -

"GenerateSecretString": {
  "ExcludeCharacters": "\"@/\\`",
  "GenerateStringKey": "password",
  "PasswordLength": 30,
  "SecretStringTemplate": "{\"username\":\"myadmin\"}"
}

@jogold / @skinny85 - anything either of you know of that I missed?

[jogold]

The dbname, host and dbengine come from the AWS::SecretsManager::SecretTargetAttachment resource. This resource exist only to avoid a circular dependency when creating a DB together with a secret. The DB needs the secret and the secret to be "usable" needs the DB connection info.

The AWS::SecretsManager::SecretTargetAttachment resource completes the final link between a Secrets Manager secret and the associated database. This is required because each has a dependency on the other.

Conceptually a AWS::SecretsManager::SecretTargetAttachment is just a secret and it's implemented like that in the CDK:

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

Lines 388 to 391 in 9f35104

	/**
	* An attached secret.
	*/
	export class SecretTargetAttachment extends SecretBase implements ISecretTargetAttachment {

This allows to ref the secret (in fact the secret attachment) knowing that it will contain the connection information. This is needed for secret rotation for example.

I think the following happens here: when you udpate the AWS::SecretsManager::Secret resource by changing the ExcludeCharacters the secret gets updated and here it means regenerated (and this type of update does not require a CF resource replacement). But the AWS::SecretsManager::SecretTargetAttachment does not change so from a CF perspective it doesn't run again.

I haven't tried to reproduce it yet but does the AWS::RDS::DBInstance gets updated with the new master credentials when you update ExcludeCharacters? I'm not sure. The only solution would be to perform a secret rotation then?

[nija-at]

Thanks for the explanation @jogold.

@mjslane - can you add the following piece of code to the code snippet in your reproduction steps. This should replace the AWS::Secrets::SecretTargetAttachment resource type underneath and thereby get the output as you would expect -

const secretAttachment = dbSecret.node.tryFindChild('Attachment') as SecretTargetAttachment;
const cfnAttachment = secretAttachment.node.defaultChild as CfnSecretTargetAttachment;
cfnAttachment.overrideLogicalId('AttachmentLogicalId');

However, I suspect @jogold is right and this may not actually change the Database password. Could you try this out and confirm if it works, and actually changes the Database password as you would expect?

[mjslane]

We tried this, but in our stack the the secret ref is exported so other stacks can use it so we're getting an error on deleting the export.

stack-name Export secret-ref cannot be deleted as it is in use by other-stack

We also managed to trigger a similar change when adding a new security group to connect to the RDS instance using the instance.connections.allowFrom(securityGroup, port) method. It didn't change the password but did delete everything other than the password and username.

[mjslane]

As an update, when we added the security group rule the password for the database wasn't changed but the password value in the secret was changed. This meant that new instances of our services couldn't connect to the database as the credentials in secret manager were wrong.

[nija-at]

@skinny85 and I had a discussion around this yesterday.

I believe the correct solution here is that whenever a change occurs to the AWS::SecretsManager::Secret resource that can modify the password, both the associated AWS::SecretsManager::SecretAttachment and AWS::SecretsManager::SecretRotation should be mutated.

Mutating the SecretAttachment resource would ensure that the connection string generated by this that includes the dbname, engine, port, password, etc. is re-generated with the new password.

Mutating the SecretRotation resource would trigger a new asynchronous password rotation (needs to be confirmed) and keep the password in the Database and the Secret in sync.
However, there is likely going to be a period of time where the passwords are out of sync.

All of this still is likely to work but still needs verification.

[jogold]

@nija-at @skinny85 how about overriding the logical id of the CfnSecret with a hash of its props? This way it always gets replaced and the rest follows?

const resource = new secretsmanager.CfnSecret(this, 'Resource', {
  description: props.description,
  kmsKeyId: props.encryptionKey && props.encryptionKey.keyArn,
  generateSecretString: props.generateSecretString || {},
  name: this.physicalName,
});
const logicalId = crypto.createHash('sha256')
  .update(JSON.stringify(props.generateSecretString))
  .digest('hex');
resource.overrideLogicalId(logicalId);

[jogold]

Also if you're using secret rotation (the one from the CDK) it doesn't make sense to override excluded characters because the secret rotation application has its own hard coded set of excluded characters:

https://github.com/aws-samples/aws-secrets-manager-rotation-lambdas/blob/6ec39bbdd5867ba7d0c1d5ac2170278fe55fbddd/SecretsManagerRDSOracleRotationSingleUser/lambda_function.py#L116

https://github.com/aws-samples/aws-secrets-manager-rotation-lambdas/blob/6ec39bbdd5867ba7d0c1d5ac2170278fe55fbddd/SecretsManagerRDSMySQLRotationSingleUser/lambda_function.py#L116

[skinny85]

@nija-at @skinny85 how about overriding the logical id of the CfnSecret with a hash of its props? This way it always gets replaced and the rest follows?

const resource = new secretsmanager.CfnSecret(this, 'Resource', {
  description: props.description,
  kmsKeyId: props.encryptionKey && props.encryptionKey.keyArn,
  generateSecretString: props.generateSecretString || {},
  name: this.physicalName,
});
const logicalId = crypto.createHash('sha256')
  .update(JSON.stringify(props.generateSecretString))
  .digest('hex');
resource.overrideLogicalId(logicalId);

Yes, this is exactly how we thought to approach it :)

[skinny85]

Also if you're using secret rotation (the one from the CDK) it doesn't make sense to override excluded characters because the secret rotation application has its own hard coded set of excluded characters:

https://github.com/aws-samples/aws-secrets-manager-rotation-lambdas/blob/6ec39bbdd5867ba7d0c1d5ac2170278fe55fbddd/SecretsManagerRDSOracleRotationSingleUser/lambda_function.py#L116

https://github.com/aws-samples/aws-secrets-manager-rotation-lambdas/blob/6ec39bbdd5867ba7d0c1d5ac2170278fe55fbddd/SecretsManagerRDSMySQLRotationSingleUser/lambda_function.py#L116

Hmmm... in that case, I wonder whether we should have some validation for that case? Because the fact that this property will be ignored might catch people off guard...

[jogold]

Hmmm... in that case, I wonder whether we should have some validation for that case? Because the fact that this property will be ignored might catch people off guard...

It's currently not possible from the code to use the "managed" rotation if the secret was not created inside the L2 and in this case it's a DatabaseSecret with the same set of excluded characters. The issue here is when someone tries to override CFN properties...

Yes, this is exactly how we thought to approach it :)

Mutating the SecretRotation resource would trigger a new asynchronous password rotation (needs to be confirmed) and keep the password in the Database and the Secret in sync.

Since secret rotation is optional, I was suggesting to mutate the secret itself. This will for sure update the password in the DB (via CF).

But with the fact that the secret is created inside the L2, I'm actually not sure anymore: either the user chooses for a "managed" experience (secret and secret rotation) or he must pass a cdk.SecretValue for the masterUserPassword and implement his own rotation with addRotationSchedule() from aws-secretsmanager on his secret?

[github-actions]

This issue has not received any attention in 1 year. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.