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

(RDS): Rotation applications are very old and insecure #18249

Open
iurquiza opened this issue Jan 3, 2022 · 18 comments
Open

(RDS): Rotation applications are very old and insecure #18249

iurquiza opened this issue Jan 3, 2022 · 18 comments
Labels
@aws-cdk/aws-rds Related to Amazon Relational Database bug This issue is a bug. effort/medium Medium work item – several days of effort p2

Comments

@iurquiza
Copy link

iurquiza commented Jan 3, 2022

What is the problem?

I deployed a MySQL RDS instance in isolated subnets and added a lambda to rotate the database credentials using the addRotationMultiUser method. The Lambda function is provision correctly, but fails when it call the set_secret method. Connecting to the database fails with the following error:

[ERROR] ModuleNotFoundError: No module named 'asn1crypto' Traceback (most recent call last): File "/var/task/lambda_function.py", line 78, in lambda_handler

The dependency could be missing or the issue could be caused by a version update. Lock the version using a requirements.txt file when installing the dependencies.
pip install -r requirements.txt

Reproduction Steps

rds-stack.txt

What did you expect to happen?

Create the "Secrets Manager RDS MySQL Handler" Lambda and rotate the database credentials successfully without throwing errors.

What actually happened?

[ERROR] ModuleNotFoundError: No module named 'asn1crypto' Traceback (most recent call last): File "/var/task/lambda_function.py", line 78, in lambda_handler

CDK CLI Version

1.137.0

Framework Version

No response

Node.js Version

14.15.5

OS

macOS Big Sur Version 11.6.2

Language

Typescript

Language Version

3.9.7

Other information

No response

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

skinny85 commented Jan 3, 2022

Hey @iurquiza,

thanks for opening the issue. Can you show the CDK you use for creating the Instance (I'm mostly interested in the engine).

Considering we simply use the ready-made Serverless Application Repositories apps to perform this rotation, I wonder whether it's a problem with one of them.

Thanks,
Adam

@skinny85 skinny85 added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Jan 3, 2022
@iurquiza
Copy link
Author

iurquiza commented Jan 4, 2022

I attached the TypeScript code in the rd-stack.txt. It would not let me attach a .ts file. Below are the file contents:

import * as cdk from '@aws-cdk/core';
import * as ec2 from '@aws-cdk/aws-ec2';
import * as rds from '@aws-cdk/aws-rds';
import * as secretmanager from '@aws-cdk/aws-secretsmanager';

export interface RdsStackProps extends cdk.NestedStackProps {
  readonly vpc: ec2.IVpc;
}
export class RdsStack extends cdk.NestedStack {
  public readonly credsSecretNamePrefix: string;

  public readonly credsAdminSecret: secretmanager.ISecret;

  public readonly credsReaderSecret: secretmanager.ISecret;

  public readonly credsWriterSecret: secretmanager.ISecret;

  public readonly dbServer: rds.DatabaseInstance;

  public readonly dbName: string;

  constructor(scope: cdk.Stack, id: string, props: RdsStackProps) {
    super(scope, id, props);
    const prefix = this.node.tryGetContext('PREFIX').toLowerCase();

    this.credsSecretNamePrefix = `/${prefix}/rds/creds/`.toLowerCase();
    const credsAdminSecretName = `${this.credsSecretNamePrefix}admin`;
    const credsReaderSecretName = `${this.credsSecretNamePrefix}reader`;
    const credsWriterSecretName = `${this.credsSecretNamePrefix}writer`;
    this.credsAdminSecret = new rds.DatabaseSecret(this, 'RdsCredentialsAdmin', {
      secretName: credsAdminSecretName,
      username: 'admin',
    });

    this.dbName = `${prefix}Db`;
    this.dbServer = new rds.DatabaseInstance(this, 'RdsInstance', {
      databaseName: this.dbName,
      vpcSubnets: {
        onePerAz: true,
        subnetType: ec2.SubnetType.PRIVATE_ISOLATED,
      },
      credentials: rds.Credentials.fromSecret(this.credsAdminSecret),
      vpc: props.vpc,
      port: 3306,
      allocatedStorage: 20,
      instanceIdentifier: prefix,
      engine: rds.DatabaseInstanceEngine.mysql({
        version: rds.MysqlEngineVersion.VER_8_0_26,
      }),
      instanceType: ec2.InstanceType.of(ec2.InstanceClass.T2, ec2.InstanceSize.LARGE),
    });
    // potentially allow connections to the RDS instance...

    this.credsReaderSecret = new rds.DatabaseSecret(this, 'RdsCredentialsReader', {
      secretName: credsReaderSecretName,
      username: 'reader',
      masterSecret: this.dbServer.secret,
    });

    this.credsWriterSecret = new rds.DatabaseSecret(this, 'RdsCredentialsWriter', {
      secretName: credsWriterSecretName,
      username: 'writer',
      masterSecret: this.dbServer.secret,
    });

    const readerUserSecretAttached = this.credsReaderSecret.attach(this.dbServer);
    this.dbServer.addRotationMultiUser('RdsCredentialsReaderRotation', {
      secret: readerUserSecretAttached,
    });

    const writerUserSecretAttached = this.credsWriterSecret.attach(this.dbServer);
    this.dbServer.addRotationMultiUser('RdsCredentialsWriterRotation', {
      secret: writerUserSecretAttached,
    });
  }
}

@skinny85
Copy link
Contributor

skinny85 commented Jan 4, 2022

Hmm. There is one interesting thing I see in the resulting template, and that is the version of the application we use for rotation:

Mappings:
  InstanceRdsCredentialsReaderRotationSARMapping495081D4:
    aws:
      applicationId: arn:aws:serverlessrepo:us-east-1:297356227824:applications/SecretsManagerRDSMySQLRotationMultiUser
      semanticVersion: 1.1.60

1.1.60 is pretty old - I see this application is now at version 1.1.217.

@iurquiza can you try bumping this version to 1.1.217, and see if that helps?

The code to do that:

        const rotation = instance.addRotationMultiUser('RdsCredentialsReaderRotation', {
            secret: readerUserSecretAttached,
        });

        const sarMapping = rotation.node.findChild('SARMapping') as cdk.CfnMapping;
        sarMapping.setValue('aws', 'semanticVersion', '1.1.217');

Thanks,
Adam

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Jan 4, 2022
@peterwoodworth peterwoodworth added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. needs-reproduction This issue needs reproduction. labels Jan 4, 2022
@github-actions
Copy link

github-actions bot commented Jan 6, 2022

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

@github-actions github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Jan 6, 2022
@iurquiza
Copy link
Author

iurquiza commented Jan 10, 2022

I believe updating the sarMapping addressed the original issue, but the function is still failing to connect to the database. I need more time to test and verify that the issue has been resolved. This change packages the Lambda handler and its dependencies differently making it difficult to trace the error; the source code for the rotation function is no hosted locally. I will post an update a soon as I have more information.

@github-actions github-actions bot removed closing-soon This issue will automatically close in 4 days unless further comments are made. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. labels Jan 10, 2022
@ryparker ryparker added p2 and removed needs-triage This issue or PR still needs to be triaged. labels Jan 10, 2022
@skinny85
Copy link
Contributor

@iurquiza make sure the connectivity is in place for the rotation Lambda to reach all of the needed services (here is the CDK documentation on the topic).

@iurquiza
Copy link
Author

iurquiza commented Jan 25, 2022

I updated my application to use HostedRotations. See below:

    this.credsWriterSecret = new rds.DatabaseSecret(this, 'RdsCredentialsWriter', {
      secretName: credsWriterSecretName,
      username: 'writer',
      masterSecret: this.dbServer.secret,
    });

    const hostedRotationWriter = secretmanager.HostedRotation.mysqlMultiUser({
      masterSecret: this.credsAdminSecret,
      vpc: props.vpc,
      functionName: 'HostedRotationWriterFn',
      vpcSubnets: props.vpc.selectSubnets({
        subnetType: ec2.SubnetType.PRIVATE_ISOLATED,
      }),
    });

    const writerUserSecret = this.credsWriterSecret.attach(this.dbServer);
    writerUserSecret.addRotationSchedule('RotationScheduleWriter', { hostedRotation: hostedRotationWriter });

    this.dbServer.connections.allowDefaultPortFrom(hostedRotationWriter);

@skinny85
Copy link
Contributor

@iurquiza And...? Did that fix the problem? 😛

@iurquiza
Copy link
Author

iurquiza commented Jan 26, 2022

Yes, the rotation happens successfully when using HostedRotation; I am assuming that it is using a more up to date version of the Rotation Lambda.
Also, your suggestion to update the semanticVersion when using

this.dbServer.addRotationMultiUser('RdsCredentialsReaderRotation', {
  secret: readerUserSecretAttached,
});

got rid of the asn1crypto missing dependency error. I just couldn't get a reference of the rotation function to allow its security group and role to access the database. The HostedRotation approach was easier.

On a separate note, being able to decouple or specify the Hosted Rotation function when creating the rotation schedule would be useful. I don't see a reason to have to deploy multiple identical lambdas to rotate multiple secrets associated with the same database. The rotation function's execution roles are also identical; they use a wildcard in the secretmanager IAM policy.
I tried defining a single HostedRotation and reusing it, but when adding the rotation schedule addRotationSchedule to the second secret CDK complained about there already being a resource with the rotation function's functionName.

@skinny85
Copy link
Contributor

@iurquiza Thanks for the info. Seems like we should re-phrase this as a Feature Request for the SecretsManager module to allow re-using the hosted rotation Lambda function - did I understand you correctly?

Thanks,
Adam

@iurquiza
Copy link
Author

@skinny85 Yes, that is correct. I was overly verbose because I couldn't come up with a concise way of describing what I meant. Thanks for rephrasing it.

@skinny85
Copy link
Contributor

Sounds good, I've opened #18756 to track it.

Let's re-purpose this to see how we can update the RDS rotation function versions (and whether we can do it automatically somehow, without manual intervention).

@skinny85 skinny85 changed the title (aws-rds): Missing asn1crypto dependency for "Secrets Manager RDS MySQL Handler" (RDS): Rotation applications are very old and insecure Jan 31, 2022
@skinny85 skinny85 added effort/medium Medium work item – several days of effort and removed p2 needs-reproduction This issue needs reproduction. labels Jan 31, 2022
@skinny85 skinny85 added the p1 label Jan 31, 2022
@skinny85 skinny85 removed their assignment Jan 31, 2022
@michaelbrewer
Copy link
Contributor

michaelbrewer commented Feb 3, 2022

The following code generates hosted functions from the Secrets Manager rotation function templates which are Python 3.7 and includes outdated insecure dependencies

from aws_cdk import core, aws_ec2 as ec2
from aws_cdk.aws_rds import DatabaseCluster, DatabaseClusterEngine, InstanceProps, AuroraEngineVersion
from aws_cdk.core import Duration


class ExampleStack(core.Stack):
    def __init__(self, scope: core.Construct, construct_id: str, **kwargs) -> None:
        super().__init__(scope, construct_id, **kwargs)

        vpc = ec2.Vpc(self, "VPC")

        db = DatabaseCluster(
            self,
            "Database",
            engine=DatabaseClusterEngine.aurora(version=AuroraEngineVersion.VER_1_22_2),
            instance_props=InstanceProps(vpc=vpc),
        )
        db.add_rotation_single_user(automatically_after=Duration.days(7))

image

image

References:

@michaelbrewer
Copy link
Contributor

@skinny85 - i have also logged this with the aws-secrets-manager-rotation-lambdas repo: aws-samples/aws-secrets-manager-rotation-lambdas#78

@skinny85
Copy link
Contributor

skinny85 commented Feb 3, 2022

Thanks @michaelbrewer!

@sukrithanda
Copy link

Any updates on cdk using the most recent rotation lambda? Would be great if it could use the lambda that supports SSL https://aws.amazon.com/about-aws/whats-new/2021/12/aws-secrets-manager-enables-ssl-connections-rotating-database/

@skinny85
Copy link
Contributor

skinny85 commented Mar 8, 2022

Nothing beyond the workaround mentioned in #18249 (comment) I'm afraid.

@ahammond
Copy link
Contributor

Any updates here? I see that python 3.7 deprecation is enroute and I'm wondering if that is an excuse to finally address this.

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 bug This issue is a bug. effort/medium Medium work item – several days of effort p2
Projects
None yet
Development

No branches or pull requests

8 participants