Skip to content

Commit

Permalink
fix(rds): customizing secret results in unusable password and lost at…
Browse files Browse the repository at this point in the history
…tachment (#11237)

When the `excludeCharacters` prop is updated the secret is correctly
regenerated but the database is not updated because the
`MasterUserPassword` prop does not change. Moreover the connection
fields created by the attachment are lost because the
`AWS::SecretsManager::SecretAttachment` doesn't "rerun" (it references
the same secret).

Introduce `fromGeneratedSecret()` and `fromPassword()` static methods
in `Credentials`. When used the `MasterUsername` prop of the database
references the username as a string instead of a dynamic reference to
the username field of the secret. Moreover the logical id of the secret
is a hash of its customization options. This way the secret gets replaced
when it's customized, the database gets updated and the attachement reruns.
This without updating the `MasterUsername` prop, avoiding a replacement
of the database.

For instances that were created from a snapshot the `MasterUsername` prop
is not specified so there's no replacement risk. Add a new static method
`fromGeneratedSecret()` in `SnapshotCredentials` to replace the secret
when its customization options change (also hash in logical id).

Deprecate `Credentials.fromUsername()` but keep it as default to avoid
a breaking change that would replace existing databases that were not
created from a snapshot.

Deprecate `SnapshotCredentials.fromGeneratedPassword()` which doesn't
replace the secret when customization options are changed.

Closes #11040

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
jogold committed Nov 6, 2020
1 parent 52da8cb commit a4567f5
Show file tree
Hide file tree
Showing 12 changed files with 951 additions and 54 deletions.
8 changes: 4 additions & 4 deletions packages/@aws-cdk/aws-rds/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ your instances will be launched privately or publicly:
```ts
const cluster = new rds.DatabaseCluster(this, 'Database', {
engine: rds.DatabaseClusterEngine.auroraMysql({ version: rds.AuroraMysqlEngineVersion.VER_2_08_1 }),
credentials: rds.Credentials.fromUsername('clusteradmin'), // Optional - will default to admin
credentials: rds.Credentials.fromGeneratedSecret('clusteradmin'), // Optional - will default to 'admin' username and generated password
instanceProps: {
// optional , defaults to t3.medium
instanceType: ec2.InstanceType.of(ec2.InstanceClass.BURSTABLE2, ec2.InstanceSize.SMALL),
Expand Down Expand Up @@ -70,7 +70,7 @@ const instance = new rds.DatabaseInstance(this, 'Instance', {
engine: rds.DatabaseInstanceEngine.oracleSe2({ version: rds.OracleEngineVersion.VER_19_0_0_0_2020_04_R1 }),
// optional, defaults to m5.large
instanceType: ec2.InstanceType.of(ec2.InstanceClass.BURSTABLE3, ec2.InstanceSize.SMALL),
credentials: rds.Credentials.fromUsername('syscdk'), // Optional - will default to admin
credentials: rds.Credentials.fromGeneratedSecret('syscdk'), // Optional - will default to 'admin' username and generated password
vpc,
vpcSubnets: {
subnetType: ec2.SubnetType.PRIVATE
Expand Down Expand Up @@ -146,13 +146,13 @@ const engine = rds.DatabaseInstanceEngine.postgres({ version: rds.PostgresEngine
new rds.DatabaseInstance(this, 'InstanceWithUsername', {
engine,
vpc,
credentials: rds.Credentials.fromUsername('postgres'), // Creates an admin user of postgres with a generated password
credentials: rds.Credentials.fromGeneratedSecret('postgres'), // Creates an admin user of postgres with a generated password
});

new rds.DatabaseInstance(this, 'InstanceWithUsernameAndPassword', {
engine,
vpc,
credentials: rds.Credentials.fromUsername('postgres', { password: SecretValue.ssmSecure('/dbPassword', 1) }), // Use password from SSM
credentials: rds.Credentials.fromPassword('postgres', SecretValue.ssmSecure('/dbPassword', '1')), // Use password from SSM
});

const mySecret = secretsmanager.Secret.fromSecretName(this, 'DBSecret', 'myDBLoginInfo');
Expand Down
12 changes: 2 additions & 10 deletions packages/@aws-cdk/aws-rds/lib/cluster.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,9 @@ import { Annotations, Duration, RemovalPolicy, Resource, Token } from '@aws-cdk/
import { Construct } from 'constructs';
import { IClusterEngine } from './cluster-engine';
import { DatabaseClusterAttributes, IDatabaseCluster } from './cluster-ref';
import { DatabaseSecret } from './database-secret';
import { Endpoint } from './endpoint';
import { IParameterGroup } from './parameter-group';
import { applyRemovalPolicy, DEFAULT_PASSWORD_EXCLUDE_CHARS, defaultDeletionProtection, setupS3ImportExport } from './private/util';
import { applyRemovalPolicy, DEFAULT_PASSWORD_EXCLUDE_CHARS, defaultDeletionProtection, renderCredentials, setupS3ImportExport } from './private/util';
import { BackupProps, Credentials, InstanceProps, PerformanceInsightRetention, RotationSingleUserOptions, RotationMultiUserOptions } from './props';
import { DatabaseProxy, DatabaseProxyOptions, ProxyTarget } from './proxy';
import { CfnDBCluster, CfnDBClusterProps, CfnDBInstance } from './rds.generated';
Expand Down Expand Up @@ -489,14 +488,7 @@ export class DatabaseCluster extends DatabaseClusterNew {
this.singleUserRotationApplication = props.engine.singleUserRotationApplication;
this.multiUserRotationApplication = props.engine.multiUserRotationApplication;

let credentials = props.credentials ?? Credentials.fromUsername(props.engine.defaultUsername ?? 'admin');
if (!credentials.secret && !credentials.password) {
credentials = Credentials.fromSecret(new DatabaseSecret(this, 'Secret', {
username: credentials.username,
encryptionKey: credentials.encryptionKey,
excludeCharacters: credentials.excludeCharacters,
}));
}
const credentials = renderCredentials(this, props.engine, props.credentials);
const secret = credentials.secret;

const cluster = new CfnDBCluster(this, 'Resource', {
Expand Down
33 changes: 31 additions & 2 deletions packages/@aws-cdk/aws-rds/lib/database-secret.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import * as crypto from 'crypto';
import * as kms from '@aws-cdk/aws-kms';
import * as secretsmanager from '@aws-cdk/aws-secretsmanager';
import { Aws } from '@aws-cdk/core';
import { Aws, Names } from '@aws-cdk/core';
import { Construct } from 'constructs';
import { DEFAULT_PASSWORD_EXCLUDE_CHARS } from './private/util';

Expand Down Expand Up @@ -33,6 +34,18 @@ export interface DatabaseSecretProps {
* @default " %+~`#$&*()|[]{}:;<>?!'/@\"\\"
*/
readonly excludeCharacters?: string;

/**
* Whether to replace this secret when the criteria for the password change.
*
* This is achieved by overriding the logical id of the AWS::SecretsManager::Secret
* with a hash of the options that influence the password generation. This
* way a new secret will be created when the password is regenerated and the
* cluster or instance consuming this secret will have its credentials updated.
*
* @default false
*/
readonly replaceOnPasswordCriteriaChanges?: boolean;
}

/**
Expand All @@ -42,6 +55,8 @@ export interface DatabaseSecretProps {
*/
export class DatabaseSecret extends secretsmanager.Secret {
constructor(scope: Construct, id: string, props: DatabaseSecretProps) {
const excludeCharacters = props.excludeCharacters ?? DEFAULT_PASSWORD_EXCLUDE_CHARS;

super(scope, id, {
encryptionKey: props.encryptionKey,
description: `Generated by the CDK for stack: ${Aws.STACK_NAME}`,
Expand All @@ -52,8 +67,22 @@ export class DatabaseSecret extends secretsmanager.Secret {
masterarn: props.masterSecret?.secretArn,
}),
generateStringKey: 'password',
excludeCharacters: props.excludeCharacters ?? DEFAULT_PASSWORD_EXCLUDE_CHARS,
excludeCharacters,
},
});

if (props.replaceOnPasswordCriteriaChanges) {
const hash = crypto.createHash('md5');
hash.update(JSON.stringify({
// Use here the options that influence the password generation.
// If at some point we add other password customization options
// they sould be added here below (e.g. `passwordLength`).
excludeCharacters,
}));
const logicalId = `${Names.uniqueId(this)}${hash.digest('hex')}`;

const secret = this.node.defaultChild as secretsmanager.CfnSecret;
secret.overrideLogicalId(logicalId.slice(-255)); // Take last 255 chars
}
}
}
12 changes: 3 additions & 9 deletions packages/@aws-cdk/aws-rds/lib/instance.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import { Endpoint } from './endpoint';
import { IInstanceEngine } from './instance-engine';
import { IOptionGroup } from './option-group';
import { IParameterGroup } from './parameter-group';
import { applyRemovalPolicy, DEFAULT_PASSWORD_EXCLUDE_CHARS, defaultDeletionProtection, engineDescription, setupS3ImportExport } from './private/util';
import { applyRemovalPolicy, DEFAULT_PASSWORD_EXCLUDE_CHARS, defaultDeletionProtection, engineDescription, renderCredentials, setupS3ImportExport } from './private/util';
import { Credentials, PerformanceInsightRetention, RotationMultiUserOptions, RotationSingleUserOptions, SnapshotCredentials } from './props';
import { DatabaseProxy, DatabaseProxyOptions, ProxyTarget } from './proxy';
import { CfnDBInstance, CfnDBInstanceProps } from './rds.generated';
Expand Down Expand Up @@ -947,14 +947,7 @@ export class DatabaseInstance extends DatabaseInstanceSource implements IDatabas
constructor(scope: Construct, id: string, props: DatabaseInstanceProps) {
super(scope, id, props);

let credentials = props.credentials ?? Credentials.fromUsername(props.engine.defaultUsername ?? 'admin');
if (!credentials.secret && !credentials.password) {
credentials = Credentials.fromSecret(new DatabaseSecret(this, 'Secret', {
username: credentials.username,
encryptionKey: credentials.encryptionKey,
excludeCharacters: credentials.excludeCharacters,
}));
}
const credentials = renderCredentials(this, props.engine, props.credentials);
const secret = credentials.secret;

const instance = new CfnDBInstance(this, 'Resource', {
Expand Down Expand Up @@ -1032,6 +1025,7 @@ export class DatabaseInstanceFromSnapshot extends DatabaseInstanceSource impleme
username: credentials.username,
encryptionKey: credentials.encryptionKey,
excludeCharacters: credentials.excludeCharacters,
replaceOnPasswordCriteriaChanges: credentials.replaceOnPasswordCriteriaChanges,
});
}

Expand Down
26 changes: 26 additions & 0 deletions packages/@aws-cdk/aws-rds/lib/private/util.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import * as iam from '@aws-cdk/aws-iam';
import * as s3 from '@aws-cdk/aws-s3';
import { Construct, CfnDeletionPolicy, CfnResource, RemovalPolicy } from '@aws-cdk/core';
import { DatabaseSecret } from '../database-secret';
import { IEngine } from '../engine';
import { Credentials } from '../props';

/**
* The default set of characters we exclude from generated passwords for database users.
Expand Down Expand Up @@ -90,3 +92,27 @@ export function defaultDeletionProtection(deletionProtection?: boolean, removalP
? deletionProtection
: (removalPolicy === RemovalPolicy.RETAIN ? true : undefined);
}

/**
* Renders the credentials for an instance or cluster
*/
export function renderCredentials(scope: Construct, engine: IEngine, credentials?: Credentials): Credentials {
let renderedCredentials = credentials ?? Credentials.fromUsername(engine.defaultUsername ?? 'admin'); // For backwards compatibilty

if (!renderedCredentials.secret && !renderedCredentials.password) {
renderedCredentials = Credentials.fromSecret(
new DatabaseSecret(scope, 'Secret', {
username: renderedCredentials.username,
encryptionKey: renderedCredentials.encryptionKey,
excludeCharacters: renderedCredentials.excludeCharacters,
// if username must be referenced as a string we can safely replace the
// secret when customization options are changed without risking a replacement
replaceOnPasswordCriteriaChanges: credentials?.usernameAsString,
}),
// pass username if it must be referenced as a string
credentials?.usernameAsString ? renderedCredentials.username : undefined,
);
}

return renderedCredentials;
}
102 changes: 87 additions & 15 deletions packages/@aws-cdk/aws-rds/lib/props.ts
Original file line number Diff line number Diff line change
Expand Up @@ -116,18 +116,9 @@ export interface BackupProps {
}

/**
* Options for creating a Login from a username.
* Base options for creating Credentials.
*/
export interface CredentialsFromUsernameOptions {
/**
* Password
*
* Do not put passwords in your CDK code directly.
*
* @default - a Secrets Manager generated password
*/
readonly password?: SecretValue;

export interface CredentialsBaseOptions {
/**
* KMS encryption key to encrypt the generated secret.
*
Expand All @@ -144,14 +135,55 @@ export interface CredentialsFromUsernameOptions {
readonly excludeCharacters?: string;
}

/**
* Options for creating Credentials from a username.
*/
export interface CredentialsFromUsernameOptions extends CredentialsBaseOptions {
/**
* Password
*
* Do not put passwords in your CDK code directly.
*
* @default - a Secrets Manager generated password
*/
readonly password?: SecretValue;
}

/**
* Username and password combination
*/
export abstract class Credentials {
/**
* Creates Credentials with a password generated and stored in Secrets Manager.
*/
public static fromGeneratedSecret(username: string, options: CredentialsBaseOptions = {}): Credentials {
return {
...options,
username,
usernameAsString: true,
};
}

/**
* Creates Credentials from a password
*
* Do not put passwords in your CDK code directly.
*/
public static fromPassword(username: string, password: SecretValue): Credentials {
return {
username,
password,
usernameAsString: true,
};
}

/**
* Creates Credentials for the given username, and optional password and key.
* If no password is provided, one will be generated and stored in SecretsManager.
* If no password is provided, one will be generated and stored in Secrets Manager.
*
* @deprecated use `fromGeneratedSecret()` or `fromPassword()` for new Clusters and Instances.
* Note that switching from `fromUsername()` to `fromGeneratedSecret()` or `fromPassword()` for already deployed
* Clusters or Instances will result in their replacement!
*/
public static fromUsername(username: string, options: CredentialsFromUsernameOptions = {}): Credentials {
return {
Expand All @@ -161,7 +193,7 @@ export abstract class Credentials {
}

/**
* Creates Credentials from an existing SecretsManager ``Secret`` (or ``DatabaseSecret``)
* Creates Credentials from an existing Secrets Manager ``Secret`` (or ``DatabaseSecret``)
*
* The Secret must be a JSON string with a ``username`` and ``password`` field:
* ```
Expand All @@ -171,10 +203,16 @@ export abstract class Credentials {
* "password": <required: password>,
* }
* ```
*
* @param secret The secret where the credentials are stored
* @param username The username defined in the secret. If specified the username
* will be referenced as a string and not a dynamic reference to the username
* field in the secret. This allows to replace the secret without replacing the
* instance or cluster.
*/
public static fromSecret(secret: secretsmanager.ISecret): Credentials {
public static fromSecret(secret: secretsmanager.ISecret, username?: string): Credentials {
return {
username: secret.secretValueFromJson('username').toString(),
username: username ?? secret.secretValueFromJson('username').toString(),
password: secret.secretValueFromJson('password'),
encryptionKey: secret.encryptionKey,
secret,
Expand All @@ -186,6 +224,14 @@ export abstract class Credentials {
*/
public abstract readonly username: string;

/**
* Whether the username should be referenced as a string and not as a dynamic
* reference to the username in the secret.
*
* @default false
*/
public abstract readonly usernameAsString?: boolean;

/**
* Password
*
Expand Down Expand Up @@ -241,10 +287,29 @@ export interface SnapshotCredentialsFromGeneratedPasswordOptions {
* Credentials to update the password for a ``DatabaseInstanceFromSnapshot``.
*/
export abstract class SnapshotCredentials {
/**
* Generate a new password for the snapshot, using the existing username and an optional encryption key.
* The new credentials are stored in Secrets Manager.
*
* Note - The username must match the existing master username of the snapshot.
*/
public static fromGeneratedSecret(username: string, options: SnapshotCredentialsFromGeneratedPasswordOptions = {}): SnapshotCredentials {
return {
...options,
generatePassword: true,
replaceOnPasswordCriteriaChanges: true,
username,
};
}

/**
* Generate a new password for the snapshot, using the existing username and an optional encryption key.
*
* Note - The username must match the existing master username of the snapshot.
*
* @deprecated use `fromGeneratedSecret()` for new Clusters and Instances.
* Note that switching from `fromGeneratedPassword()` to `fromGeneratedSecret()` for already deployed
* Clusters or Instances will update their master password.
*/
public static fromGeneratedPassword(username: string, options: SnapshotCredentialsFromGeneratedPasswordOptions = {}): SnapshotCredentials {
return {
Expand Down Expand Up @@ -295,6 +360,13 @@ export abstract class SnapshotCredentials {
*/
public abstract readonly generatePassword: boolean;

/**
* Whether to replace the generated secret when the criteria for the password change.
*
* @default false
*/
public abstract readonly replaceOnPasswordCriteriaChanges?: boolean;

/**
* The master user password.
*
Expand Down
12 changes: 2 additions & 10 deletions packages/@aws-cdk/aws-rds/lib/serverless-cluster.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,10 @@ import * as secretsmanager from '@aws-cdk/aws-secretsmanager';
import { Resource, Duration, Token, Annotations, RemovalPolicy, IResource, Stack, Lazy } from '@aws-cdk/core';
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 { applyRemovalPolicy, defaultDeletionProtection, DEFAULT_PASSWORD_EXCLUDE_CHARS } from './private/util';
import { applyRemovalPolicy, defaultDeletionProtection, DEFAULT_PASSWORD_EXCLUDE_CHARS, renderCredentials } from './private/util';
import { Credentials, RotationMultiUserOptions, RotationSingleUserOptions } from './props';
import { CfnDBCluster } from './rds.generated';
import { ISubnetGroup, SubnetGroup } from './subnet-group';
Expand Down Expand Up @@ -420,14 +419,7 @@ export class ServerlessCluster extends ServerlessClusterBase {
}
}

let credentials = props.credentials ?? Credentials.fromUsername(props.engine.defaultUsername ?? 'admin');
if (!credentials.secret && !credentials.password) {
credentials = Credentials.fromSecret(new DatabaseSecret(this, 'Secret', {
username: credentials.username,
encryptionKey: credentials.encryptionKey,
excludeCharacters: credentials.excludeCharacters,
}));
}
const credentials = renderCredentials(this, props.engine, props.credentials);
const secret = credentials.secret;

// bind the engine to the Cluster
Expand Down
Loading

0 comments on commit a4567f5

Please sign in to comment.