From a5967b7ac4fe49883d1c120b9e7493e31d4aa774 Mon Sep 17 00:00:00 2001 From: peterwoodworth Date: Tue, 24 May 2022 16:20:26 -0700 Subject: [PATCH] fix(rds): clusters created from snapshots generate incorrect passwords --- packages/@aws-cdk/aws-rds/lib/cluster.ts | 13 +- packages/@aws-cdk/aws-rds/lib/private/util.ts | 20 ++-- packages/@aws-cdk/aws-rds/lib/props.ts | 77 +++++++++++- .../aws-rds/lib/serverless-cluster.ts | 37 +++++- .../serverless-cluster-from-snapshot.test.ts | 113 +----------------- 5 files changed, 132 insertions(+), 128 deletions(-) diff --git a/packages/@aws-cdk/aws-rds/lib/cluster.ts b/packages/@aws-cdk/aws-rds/lib/cluster.ts index a139db38cb2a6..74e0313a3a340 100644 --- a/packages/@aws-cdk/aws-rds/lib/cluster.ts +++ b/packages/@aws-cdk/aws-rds/lib/cluster.ts @@ -12,7 +12,7 @@ import { DatabaseClusterAttributes, IDatabaseCluster } from './cluster-ref'; import { Endpoint } from './endpoint'; import { IParameterGroup, ParameterGroup } from './parameter-group'; import { applyDefaultRotationOptions, defaultDeletionProtection, renderCredentials, setupS3ImportExport, helperRemovalPolicy, renderUnless, renderClusterSnapshotCredentials } from './private/util'; -import { BackupProps, Credentials, InstanceProps, PerformanceInsightRetention, RotationSingleUserOptions, RotationMultiUserOptions, SnapshotCredentials } from './props'; +import { BackupProps, Credentials, InstanceProps, PerformanceInsightRetention, RotationSingleUserOptions, RotationMultiUserOptions, ClusterSnapshotCredentials } from './props'; import { DatabaseProxy, DatabaseProxyOptions, ProxyTarget } from './proxy'; import { CfnDBCluster, CfnDBClusterProps, CfnDBInstance } from './rds.generated'; import { ISubnetGroup, SubnetGroup } from './subnet-group'; @@ -661,7 +661,7 @@ export interface DatabaseClusterFromSnapshotProps extends DatabaseClusterBasePro /** * Credentials for the administrative user * - * @deprecated - use `snapshotCredentials` instead + * @deprecated - use {@link DatabaseClusterFromSnapshotProps.snapshotCredentials} instead * * @default - A username of 'admin' (or 'postgres' for PostgreSQL) and SecretsManager-generated password */ @@ -672,7 +672,7 @@ export interface DatabaseClusterFromSnapshotProps extends DatabaseClusterBasePro * * @default - No credentials are generated */ - readonly snapshotCredentials?: SnapshotCredentials; + readonly snapshotCredentials?: ClusterSnapshotCredentials; } /** @@ -702,7 +702,12 @@ export class DatabaseClusterFromSnapshot extends DatabaseClusterNew { const credentials = renderCredentials(this, props.engine, props.credentials); const snapshotCredentials = renderClusterSnapshotCredentials(this, props.snapshotCredentials); - const secret = snapshotCredentials.secret || credentials.secret; + let secret; + if (snapshotCredentials) { + secret = snapshotCredentials.secret; + } else if (credentials) { + secret = credentials.secret; + } const cluster = new CfnDBCluster(this, 'Resource', { ...this.newCfnProps, diff --git a/packages/@aws-cdk/aws-rds/lib/private/util.ts b/packages/@aws-cdk/aws-rds/lib/private/util.ts index 29c58f60a43f5..e9a04cac56392 100644 --- a/packages/@aws-cdk/aws-rds/lib/private/util.ts +++ b/packages/@aws-cdk/aws-rds/lib/private/util.ts @@ -2,10 +2,10 @@ import * as ec2 from '@aws-cdk/aws-ec2'; import * as iam from '@aws-cdk/aws-iam'; import * as s3 from '@aws-cdk/aws-s3'; import * as secretsmanager from '@aws-cdk/aws-secretsmanager'; -import { Annotations, RemovalPolicy } from '@aws-cdk/core'; +import { Aws, RemovalPolicy } from '@aws-cdk/core'; import { DatabaseSecret } from '../database-secret'; import { IEngine } from '../engine'; -import { CommonRotationUserOptions, Credentials, SnapshotCredentials } from '../props'; +import { ClusterSnapshotCredentials, CommonRotationUserOptions, Credentials } from '../props'; // keep this import separate from other imports to reduce chance for merge conflicts with v2-main // eslint-disable-next-line no-duplicate-imports, import/order @@ -115,22 +115,18 @@ export function renderCredentials(scope: Construct, engine: IEngine, credentials /** * Renders credentials for a cluster from snapshot */ -export function renderClusterSnapshotCredentials(scope: Construct, credentials?: SnapshotCredentials): SnapshotCredentials { +export function renderClusterSnapshotCredentials(scope: Construct, credentials?: ClusterSnapshotCredentials): ClusterSnapshotCredentials | undefined { let renderedCredentials = credentials; - if (!renderedCredentials) return { generatePassword: false }; + if (renderedCredentials?.secret) return renderedCredentials; - if (renderedCredentials.generatePassword) Annotations.of(scope).addWarning('Cannot generate new password for clusters created from snapshot. Use `fromSecret()` or `fromPassword()` instead'); - - if (renderedCredentials.secret) return renderedCredentials; - - if (renderedCredentials.password) { - renderedCredentials = SnapshotCredentials.fromSecret( + if (renderedCredentials?.password) { + renderedCredentials = ClusterSnapshotCredentials.fromSecret( new secretsmanager.Secret(scope, 'Secret', { - //username: renderedCredentials.username, - secretName: renderedCredentials.secretName, + description: `Generated by the CDK for stack: ${Aws.STACK_NAME}`, encryptionKey: renderedCredentials.encryptionKey, replicaRegions: renderedCredentials.replicaRegions, + secretName: renderedCredentials.secretName, secretStringValue: renderedCredentials.password, }), ); diff --git a/packages/@aws-cdk/aws-rds/lib/props.ts b/packages/@aws-cdk/aws-rds/lib/props.ts index 5911b61ae11cf..290fb8d54532c 100644 --- a/packages/@aws-cdk/aws-rds/lib/props.ts +++ b/packages/@aws-cdk/aws-rds/lib/props.ts @@ -381,9 +381,8 @@ export abstract class SnapshotCredentials { /** * Update the snapshot login with an existing password. */ - public static fromPassword(password: SecretValue, options: GeneratedSecretForSnapshotCredentialsOptions = {}): SnapshotCredentials { + public static fromPassword(password: SecretValue): SnapshotCredentials { return { - ...options, generatePassword: false, password, }; @@ -476,6 +475,80 @@ export abstract class SnapshotCredentials { public abstract readonly replicaRegions?: secretsmanager.ReplicaRegion[]; } +/** + * Credentials to create or reference a secret for a cluster created from snapshot + * + * The login credentials of the cluster are inherited from the snapshot and cannot be changed + */ +export abstract class ClusterSnapshotCredentials { + /** + * Create a new secret with the appropriate credentials + * + * **NOTE:** Do NOT store your password directly in CDK code + */ + // eslint-disable-next-line max-len + public static fromPassword(username: string, password: SecretValue, options: GeneratedSecretForSnapshotCredentialsOptions = {}): ClusterSnapshotCredentials { + return { + ...options, + username, + password, + }; + } + + /** + * Reference a secret to attach to your cluster + */ + public static fromSecret(secret: secretsmanager.ISecret, options: GeneratedSecretForSnapshotCredentialsOptions = {}): ClusterSnapshotCredentials { + return { + ...options, + secret, + }; + } + + /** + * KMS encryption key to encrypt the generated secret. + * + * @default - default master key + */ + public abstract readonly encryptionKey?: kms.IKey; + + /** + * The master user password. + * + * **NOTE:** Do NOT store your password directly in CDK code + */ + public abstract readonly password?: SecretValue; + + /** + * Secret used to instantiate this Login. + * + * @default - none + */ + public abstract readonly secret?: secretsmanager.ISecret; + + /** + * Name of secret to create + * + * @default - default name is generated + */ + public abstract readonly secretName?: string; + + /** + * A list of regions where to replicate the generated secret. + * + * @default - Secret is not replicated + */ + public abstract readonly replicaRegions?: secretsmanager.ReplicaRegion[]; + + /** + * The master user name. + * + * Must be the **current** master user name of the snapshot. + * It is not possible to change the master user name of a RDS instance. + */ + public abstract readonly username?: string; +} + /** * Properties common to single-user and multi-user rotation options. */ diff --git a/packages/@aws-cdk/aws-rds/lib/serverless-cluster.ts b/packages/@aws-cdk/aws-rds/lib/serverless-cluster.ts index 9bac0cb70cd79..597f4fa7d00ba 100644 --- a/packages/@aws-cdk/aws-rds/lib/serverless-cluster.ts +++ b/packages/@aws-cdk/aws-rds/lib/serverless-cluster.ts @@ -6,11 +6,12 @@ import { Resource, Duration, Token, Annotations, RemovalPolicy, IResource, Stack import * as cxapi from '@aws-cdk/cx-api'; import { Construct } from 'constructs'; import { IClusterEngine } from './cluster-engine'; +import { DatabaseSecret } from './database-secret'; import { Endpoint } from './endpoint'; import { IParameterGroup } from './parameter-group'; import { DATA_API_ACTIONS } from './perms'; import { applyDefaultRotationOptions, defaultDeletionProtection, renderClusterSnapshotCredentials, renderCredentials } from './private/util'; -import { Credentials, RotationMultiUserOptions, RotationSingleUserOptions, SnapshotCredentials } from './props'; +import { ClusterSnapshotCredentials, Credentials, RotationMultiUserOptions, RotationSingleUserOptions, SnapshotCredentials } from './props'; import { CfnDBCluster, CfnDBClusterProps } from './rds.generated'; import { ISubnetGroup, SubnetGroup } from './subnet-group'; @@ -651,9 +652,20 @@ export interface ServerlessClusterFromSnapshotProps extends ServerlessClusterNew * Note - It is not possible to change the master username for a snapshot; * however, it is possible to provide (or generate) a new password. * + * @deprecated - Use {@link ServerlessClusterFromSnapshotProps.snapshotCredentials} instead + * * @default - The existing username and password from the snapshot will be used. */ readonly credentials?: SnapshotCredentials; + + /** + * Credentials to create or reference a secret for a cluster created from snapshot + * + * The login credentials of the cluster are inherited from the snapshot and cannot be changed + * + * @default - no credentials are created + */ + readonly snapshotCredentials?: ClusterSnapshotCredentials; } /** @@ -672,8 +684,27 @@ export class ServerlessClusterFromSnapshot extends ServerlessClusterNew { this.enableDataApi = props.enableDataApi; - const credentials = renderClusterSnapshotCredentials(this, props.credentials); - const secret = credentials.secret; + const credentials = props.credentials; + let secret; + + const snapshotCredentials = renderClusterSnapshotCredentials(this, props.snapshotCredentials); + if (snapshotCredentials) secret = snapshotCredentials.secret; + + if (!secret && credentials?.generatePassword) { + if (!credentials.username) { + throw new Error('`credentials` `username` must be specified when `generatePassword` is set to true'); + } + + Annotations.of(this).addWarning('Generating a password for a cluster created from a snapshot will result in creating a secret with an incorrect password. Use `snapshotCredentials` instead.'); + + secret = new DatabaseSecret(this, 'Secret', { + username: credentials.username, + encryptionKey: credentials.encryptionKey, + excludeCharacters: credentials.excludeCharacters, + replaceOnPasswordCriteriaChanges: credentials.replaceOnPasswordCriteriaChanges, + replicaRegions: credentials.replicaRegions, + }); + } const cluster = new CfnDBCluster(this, 'Resource', { ...this.newCfnProps, diff --git a/packages/@aws-cdk/aws-rds/test/serverless-cluster-from-snapshot.test.ts b/packages/@aws-cdk/aws-rds/test/serverless-cluster-from-snapshot.test.ts index ffe67313ccc9a..2c50a022d3b6a 100644 --- a/packages/@aws-cdk/aws-rds/test/serverless-cluster-from-snapshot.test.ts +++ b/packages/@aws-cdk/aws-rds/test/serverless-cluster-from-snapshot.test.ts @@ -1,8 +1,7 @@ -import { Match, Template } from '@aws-cdk/assertions'; +import { Template } from '@aws-cdk/assertions'; import * as ec2 from '@aws-cdk/aws-ec2'; -import * as kms from '@aws-cdk/aws-kms'; import * as cdk from '@aws-cdk/core'; -import { DatabaseClusterEngine, DatabaseSecret, ServerlessClusterFromSnapshot, SnapshotCredentials } from '../lib'; +import { ClusterSnapshotCredentials, DatabaseClusterEngine, ServerlessClusterFromSnapshot } from '../lib'; describe('serverless cluster from snapshot', () => { test('create a serverless cluster from a snapshot', () => { @@ -41,122 +40,22 @@ describe('serverless cluster from snapshot', () => { }); }); - test('can generate a new snapshot password', () => { + test('can create new secret for snapshot using password from an existing SecretValue', () => { const stack = testStack(); const vpc = new ec2.Vpc(stack, 'VPC'); + const secretValue = cdk.SecretValue.secretsManager('mysecretid'); // WHEN new ServerlessClusterFromSnapshot(stack, 'ServerlessDatabase', { engine: DatabaseClusterEngine.AURORA_MYSQL, vpc, snapshotIdentifier: 'mySnapshot', - credentials: SnapshotCredentials.fromGeneratedSecret('admin', { - excludeCharacters: '"@/\\', - }), - }); - - // THEN - Template.fromStack(stack).hasResourceProperties('AWS::RDS::DBCluster', { - MasterUsername: Match.absent(), - MasterUserPassword: { - 'Fn::Join': ['', [ - '{{resolve:secretsmanager:', - { Ref: 'ServerlessDatabaseSecret813910E98ee0a797cad8a68dbeb85f8698cdb5bb' }, - ':SecretString:password::}}', - ]], - }, - }); - Template.fromStack(stack).hasResourceProperties('AWS::SecretsManager::Secret', { - Description: { - 'Fn::Join': ['', ['Generated by the CDK for stack: ', { Ref: 'AWS::StackName' }]], - }, - GenerateSecretString: { - ExcludeCharacters: '\"@/\\', - GenerateStringKey: 'password', - PasswordLength: 30, - SecretStringTemplate: '{"username":"admin"}', - }, - }); - }); - - test('fromGeneratedSecret with replica regions', () => { - const stack = testStack(); - const vpc = new ec2.Vpc(stack, 'VPC'); - - // WHEN - new ServerlessClusterFromSnapshot(stack, 'ServerlessDatabase', { - engine: DatabaseClusterEngine.AURORA_MYSQL, - vpc, - snapshotIdentifier: 'mySnapshot', - credentials: SnapshotCredentials.fromGeneratedSecret('admin', { - replicaRegions: [{ region: 'eu-west-1' }], - }), + snapshotCredentials: ClusterSnapshotCredentials.fromPassword('admin', secretValue), }); // THEN Template.fromStack(stack).hasResourceProperties('AWS::SecretsManager::Secret', { - ReplicaRegions: [ - { - Region: 'eu-west-1', - }, - ], - }); - }); - - test('throws if generating a new password without a username', () => { - const stack = testStack(); - const vpc = new ec2.Vpc(stack, 'VPC'); - - // WHEN - expect(() => new ServerlessClusterFromSnapshot(stack, 'ServerlessDatabase', { - engine: DatabaseClusterEngine.AURORA_MYSQL, - vpc, - snapshotIdentifier: 'mySnapshot', - credentials: { generatePassword: true }, - })).toThrow(/`credentials` `username` must be specified when `generatePassword` is set to true/); - }); - - test('can set a new snapshot password from an existing SecretValue', () => { - const stack = testStack(); - const vpc = new ec2.Vpc(stack, 'VPC'); - - // WHEN - new ServerlessClusterFromSnapshot(stack, 'ServerlessDatabase', { - engine: DatabaseClusterEngine.AURORA_MYSQL, - vpc, - snapshotIdentifier: 'mySnapshot', - credentials: SnapshotCredentials.fromPassword(cdk.SecretValue.unsafePlainText('mysecretpassword')), - }); - - // THEN - Template.fromStack(stack).hasResourceProperties('AWS::RDS::DBCluster', { - MasterUsername: Match.absent(), - MasterUserPassword: 'mysecretpassword', - }); - }); - - test('can set a new snapshot password from an existing Secret', () => { - const stack = testStack(); - const vpc = new ec2.Vpc(stack, 'VPC'); - - // WHEN - const secret = new DatabaseSecret(stack, 'DBSecret', { - username: 'admin', - encryptionKey: new kms.Key(stack, 'PasswordKey'), - }); - new ServerlessClusterFromSnapshot(stack, 'ServerlessDatabase', { - engine: DatabaseClusterEngine.AURORA_MYSQL, - vpc, - snapshotIdentifier: 'mySnapshot', - credentials: SnapshotCredentials.fromSecret(secret), - }); - - // THEN - Template.fromStack(stack).hasResourceProperties('AWS::RDS::DBCluster', { - MasterUsername: Match.absent(), - MasterUserPassword: { - 'Fn::Join': ['', ['{{resolve:secretsmanager:', { Ref: 'DBSecretD58955BC' }, ':SecretString:password::}}']], - }, + SecretString: '{{resolve:secretsmanager:mysecretid:SecretString:::}}', }); }); });