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

(aws-rds): addRotationMultiUser() changes the username, adds _clone suffix #20704

Closed
ahammond opened this issue Jun 10, 2022 · 10 comments · Fixed by #20720
Closed

(aws-rds): addRotationMultiUser() changes the username, adds _clone suffix #20704

ahammond opened this issue Jun 10, 2022 · 10 comments · Fixed by #20720
Labels
@aws-cdk/aws-rds Related to Amazon Relational Database guidance Question that needs advice or information.

Comments

@ahammond
Copy link
Contributor

Describe the bug

After our rotator lambda ran, we discovered that the username in the secret had been change to add a _clone suffix.

❯ aws secretsmanager list-secret-version-ids --region="$AWS_REGION" --secret-id="ColdStorageWriter"
{
    "Versions": [
        {
            "VersionId": "6a00bb61-5e22-4cb4-ab46-eb6b78402d05",
            "VersionStages": [
                "AWSCURRENT",
                "AWSPENDING"
            ],
            "LastAccessedDate": "2022-06-09T17:00:00-07:00",
            "CreatedDate": "2022-06-08T15:12:32.509000-07:00",
            "KmsKeyIds": [
                "redacted"
            ]
        },
        {
            "VersionId": "ec55412e-18ee-46ce-8aa5-e5199b2ece1e",
            "VersionStages": [
                "AWSPREVIOUS"
            ],
            "LastAccessedDate": "2022-06-07T17:00:00-07:00",
            "CreatedDate": "2022-05-09T13:31:13.330000-07:00",
            "KmsKeyIds": [
                "redacted"
            ]
        }
    ],
    "ARN": "redacted",
    "Name": "ColdStorageWriter"
}

❯ aws secretsmanager get-secret-value --region="$AWS_REGION" --secret-id="ColdStorageWriter" --version-id="ec55412e-18ee-46ce-8aa5-e5199b2ece1e"
{
    "ARN": "arn:aws:secretsmanager:us-west-2:514308641592:secret:ColdStorageWriter-rZPWY6",
    "Name": "ColdStorageWriter",
    "VersionId": "ec55412e-18ee-46ce-8aa5-e5199b2ece1e",
    "SecretString": "{\"password\":\"redacted\",\"masterarn\":\"redacted did not change",\"username\":\"cold_storage_writer\",\"host\":\"redacted\",\"engine\":\"postgres\",\"proxyHost\":\"redacted\"}",
    "VersionStages": [
        "AWSPREVIOUS"
    ],
    "CreatedDate": "2022-05-09T13:31:13.330000-07:00"
}

❯ aws secretsmanager get-secret-value --region="$AWS_REGION" --secret-id="ColdStorageWriter"
{
    "ARN": "arn:aws:secretsmanager:us-west-2:514308641592:secret:ColdStorageWriter-rZPWY6",
    "Name": "ColdStorageWriter",
    "VersionId": "6a00bb61-5e22-4cb4-ab46-eb6b78402d05",
    "SecretString": "{\"password\": \"redacted", \"masterarn\": \"redacted\", \"username\": \"cold_storage_writer_clone\", \"host\": \"redacted\", \"engine\": \"postgres\", \"proxyHost\": \"redacted\"}",
    "VersionStages": [
        "AWSCURRENT",
        "AWSPENDING"
    ],
    "CreatedDate": "2022-06-08T15:12:32.509000-07:00"
}

Expected Behavior

The rotator should rotate the password without completely undocumented side-effects like, for example, changing the username.

Current Behavior

Our users with addRotationMultiUser() are getting their usernames changed.

Reproduction Steps

import { Aurora } from '@time-loop/cdk-aurora';
import { App, aws_ec2, aws_kms, Stack, StackProps } from 'aws-cdk-lib';
import { Construct } from 'constructs';
import { Namer } from 'multi-convention-namer';

export class AuroraDemoStack extends Stack {
constructor(scope: Construct, props: StackProps) {
const id = new Namer(['aurora', 'demo']);
super(scope, id.pascal, props);

const vpc = aws_ec2.Vpc.fromLookup(this, 'Vpc', {
  isDefault: true,
});

const kmsKey = new aws_kms.Key(this, 'Key', {
  description: `${id.pascal} encryption key`,
});

const a = new Aurora(this, id, {
  defaultDatabaseName: 'demo',
  instances: 1, // It's just a demo
  kmsKey,
  vpc,
});

}
}

// for development, use account/region from cdk cli
const devEnv = {
account: process.env.CDK_DEFAULT_ACCOUNT,
region: process.env.CDK_DEFAULT_REGION,
};

const app = new App();

new AuroraDemoStack(app, { env: devEnv });

app.synth();

Possible Solution

I think that the decision to SAM in the rotator functions has been a mess. These rotator functions should be rewritten in TypeScript and inlined into the code and actually managed.

Related issues

Additional Information/Context

No response

CDK CLI Version

2.27.0 (build 8e89048)

Framework Version

2.27.0

Node.js Version

v16.13.1

OS

Darwin Kernel Version 21.5.0: Tue Apr 26 21:08:29 PDT 2022; root:xnu-8020.121.3~4/RELEASE_ARM64_T8101 arm64

Language

Typescript

Language Version

4.7.3

Other information

No response

@ahammond ahammond added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jun 10, 2022
@github-actions github-actions bot added the @aws-cdk/aws-rds Related to Amazon Relational Database label Jun 10, 2022
@ahammond
Copy link
Contributor Author

From the logs:

  | 2022-06-08T15:12:30.850-07:00 | START RequestId: c4527649-ebfb-48f7-be79-85d9043c7b1a Version: $LATEST
-- | -- | --
  | 2022-06-08T15:12:31.100-07:00 | [INFO] 2022-06-08T22:12:31.100Z c4527649-ebfb-48f7-be79-85d9043c7b1a Found credentials in environment variables.
  | 2022-06-08T15:12:32.521-07:00 | [INFO] 2022-06-08T22:12:32.521Z c4527649-ebfb-48f7-be79-85d9043c7b1a createSecret: Successfully put secret for ARN redacted and version 6a00bb61-5e22-4cb4-ab46-eb6b78402d05.
  | 2022-06-08T15:12:32.557-07:00 | END RequestId: c4527649-ebfb-48f7-be79-85d9043c7b1a
  | 2022-06-08T15:12:32.557-07:00 | REPORT RequestId: c4527649-ebfb-48f7-be79-85d9043c7b1a Duration: 1706.67 ms Billed Duration: 1707 ms Memory Size: 128 MB Max Memory Used: 69 MB Init Duration: 395.95 ms
  | 2022-06-08T15:12:32.560-07:00 | START RequestId: 2b62b783-8553-4f16-9ae5-d16259bf6c8e Version: $LATEST
  | 2022-06-08T15:12:33.461-07:00 | [INFO] 2022-06-08T22:12:33.461Z 2b62b783-8553-4f16-9ae5-d16259bf6c8e Successfully established SSL/TLS connection as user 'cold_storage_writer' with host: 'redacted'
  | 2022-06-08T15:12:33.681-07:00 | [INFO] 2022-06-08T22:12:33.681Z 2b62b783-8553-4f16-9ae5-d16259bf6c8e Successfully established SSL/TLS connection as user 'cold_storage_manager' with host: 'redacted'
  | 2022-06-08T15:12:33.708-07:00 | [INFO] 2022-06-08T22:12:33.708Z 2b62b783-8553-4f16-9ae5-d16259bf6c8e setSecret: Successfully set password for cold_storage_writer_clone in PostgreSQL DB for secret arn redacted.
  | 2022-06-08T15:12:33.757-07:00 | END RequestId: 2b62b783-8553-4f16-9ae5-d16259bf6c8e
  | 2022-06-08T15:12:33.757-07:00 | REPORT RequestId: 2b62b783-8553-4f16-9ae5-d16259bf6c8e Duration: 1196.35 ms Billed Duration: 1197 ms Memory Size: 128 MB Max Memory Used: 73 MB
  | 2022-06-08T15:12:35.438-07:00 | START RequestId: 78b21a09-635e-4eb5-9043-9960b600b4fd Version: $LATEST
  | 2022-06-08T15:12:35.698-07:00 | [INFO] 2022-06-08T22:12:35.697Z 78b21a09-635e-4eb5-9043-9960b600b4fd finishSecret: Successfully set AWSCURRENT stage to version 6a00bb61-5e22-4cb4-ab46-eb6b78402d05 for secret redacted.
  | 2022-06-08T15:12:35.757-07:00 | END RequestId: 78b21a09-635e-4eb5-9043-9960b600b4fd
  | 2022-06-08T15:12:35.757-07:00 | REPORT RequestId: 78b21a09-635e-4eb5-9043-9960b600b4fd Duration: 317.90 ms Billed Duration: 318 ms Memory Size: 128 MB Max Memory Used: 73 MB

@jogold
Copy link
Contributor

jogold commented Jun 11, 2022

Hi @ahammond,

The multi user strategy works with two sets of credentials to maximize availability, have a look at
https://docs.aws.amazon.com/secretsmanager/latest/userguide/rotating-secrets_strategies.html#rotating-secrets-two-users.

This is the expected behavior.

@corymhall corymhall added guidance Question that needs advice or information. and removed bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jun 13, 2022
@corymhall corymhall removed their assignment Jun 13, 2022
@ahammond
Copy link
Contributor Author

@jogold this is totally on me. Thank you for the link, that totally clarifies things! I was working on the mistaken assumption that addRotationSingleUser was only for managing the admin user and that addRotationMultiUser was for managing the rotation of other users, and that this was just awkward naming. Is there any way to get the addRotationSingleUser applied on a user that isn't the admin user? Also, would you guys accept a doc PR that adds the above link to the docstrings for these two methods?

@mergify mergify bot closed this as completed in #20720 Jun 13, 2022
mergify bot pushed a commit that referenced this issue Jun 13, 2022
Adds docs to clarify the semantics of rotations. 

Closes #20704

----

### All Submissions:

* [x] Have you followed the guidelines in our 
[Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] 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*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@jogold
Copy link
Contributor

jogold commented Jun 14, 2022

Is there any way to get the addRotationSingleUser applied on a user that isn't the admin user?

Assuming you have a secretsmanager.Secret that has all the required info in it and that the DB user referenced in that secret has the permissions to change its own password:

const myHostedRotation = HostedRotation.mysqlSingleUser({ vpc: myVpc });
mySecret.addRotationSchedule('Rotation', {
  hostedRotation: myHostedRotation,
});
myDb.allowDefaultPortFrom(myHostedRotation);

PS1: if you want to play with excludeCharacters then you need to use the "older" implementation with secretsmanager.SecretRotation.

PS2: JSON format for secret

{
  "engine": "<required: database engine>",
  "host": "<required: instance host name>",
  "username": "<required: username>",
  "password": "<required: password>",
  "dbname": "<optional: database name>",
  "port": "<optional: if not specified, default port will be used>"
}

@ahammond
Copy link
Contributor Author

@jogold WRT the excludeCharacters stuff, does the new stuff default to DMS compatible secrets? That was the driver for adding the excludeCharacters support in the original.

To clarify, in order to add a user to my aurora instance via CDK, I would need to

@jogold
Copy link
Contributor

jogold commented Jun 16, 2022

WRT the excludeCharacters stuff, does the new stuff default to DMS compatible secrets?

The code is at https://github.com/aws-samples/aws-secrets-manager-rotation-lambdas/blob/80b407354909519cf4f2d744c2d9dace09b05d39/SecretsManagerRDSPostgreSQLRotationSingleUser/lambda_function.py#L118 => ':/@"\'\\'

  • create a SecretsManager secret with the necessary fields (I think we can do this in cdk directly)

Yes, you can use a secretsmanager.Secret or a rds.DatabaseSecret, do not forgot to attach() it to your instance/cluster (this adds the DB info in the secret)

Yes or create it by any other means

  • add a HostedRotation

You can add it immediately, it will only start/succeed when the user is correctly created in the DB

@ahammond
Copy link
Contributor Author

@jogold this is awesome! Thank you so much, secret.attach() will let me delete a few dozen lines of code!

Is there a canonical example that shows all of this stuff?

Also, I recognize that set of excludeCharacters, it's the same ones that were breaking DMS a couple of years back. Not happy to see that stuff return.

@jogold
Copy link
Contributor

jogold commented Jun 16, 2022

Also, I recognize that set of excludeCharacters, it's the same ones that were breaking DMS a couple of years back. Not happy to see that stuff return.

Looks like they added ExcludeCharacters so in the mean time you should be able to do:

const rotationSchedule = mySecret.addRotationSchedule('Rotation', {
  hostedRotation: HostedRotation.mysqlSingleUser({ vpc: myVpc }),
});
const cfnRotationSchedule = rotationSchedule.node.defaultChild as CfnRotationSchedule;
cfnRotationSchedule.addPropertyOverride('HostedRotationLambda.ExcludeCharacters', '<your chars>');

Will open a PR to add this option.

@jogold
Copy link
Contributor

jogold commented Jun 16, 2022

Will open a PR to add this option.

#20768

daschaa pushed a commit to daschaa/aws-cdk that referenced this issue Jul 9, 2022
Adds docs to clarify the semantics of rotations. 

Closes aws#20704

----

### All Submissions:

* [x] Have you followed the guidelines in our 
[Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] 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*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-rds Related to Amazon Relational Database guidance Question that needs advice or information.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants