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

fix(rds): customizing secret results in unusable password and lost attachment #11237

Merged
merged 27 commits into from
Nov 6, 2020
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
ac9db36
fix(rds): customizing secret results in unusable password and lost at…
jogold Oct 31, 2020
753b6de
DatabaseInstanceFromSnapshot
jogold Nov 1, 2020
91b557f
make it unique
jogold Nov 2, 2020
8f87ae4
comments
jogold Nov 2, 2020
c3c7f57
more comments
jogold Nov 2, 2020
40dcaad
Update packages/@aws-cdk/aws-rds/lib/props.ts
jogold Nov 2, 2020
bf2f863
PR feedback
jogold Nov 3, 2020
e2ffabc
Merge branch 'rds-fixed-username' of github.com:jogold/aws-cdk into r…
jogold Nov 3, 2020
d6c8cd9
Merge branch 'master' into rds-fixed-username
jogold Nov 3, 2020
409b3fe
fromGeneratedPassword
jogold Nov 3, 2020
fe67b5a
username cannot be optional
jogold Nov 3, 2020
780f075
minor
jogold Nov 3, 2020
40213fa
deprecated doc
jogold Nov 3, 2020
5cc3165
Apply suggestions from code review
jogold Nov 3, 2020
446d269
address more PR feedback
jogold Nov 3, 2020
78c0943
add logical id expectations
jogold Nov 3, 2020
071a227
Merge branch 'master' into rds-fixed-username
jogold Nov 3, 2020
2d7abf0
logical id and hash
jogold Nov 4, 2020
4e56f48
replaceOnPasswordChanges
jogold Nov 4, 2020
21a1995
Update packages/@aws-cdk/aws-rds/README.md
jogold Nov 4, 2020
5da4c6a
Merge branch 'master' into rds-fixed-username
jogold Nov 4, 2020
f360d8c
replaceOnPasswordCriteriaChanges
jogold Nov 4, 2020
66d7a65
Merge branch 'rds-fixed-username' of github.com:jogold/aws-cdk into r…
jogold Nov 4, 2020
bec7ba3
fromGeneratedSecret
jogold Nov 5, 2020
f26a5a7
Merge branch 'master' into rds-fixed-username
jogold Nov 5, 2020
5c200b9
SecretsManager -> Secrets Manager
jogold Nov 5, 2020
87552f5
Merge branch 'master' into rds-fixed-username
jogold Nov 6, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 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.fromGeneratedPassword('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.fromGeneratedPassword('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.fromGeneratedPassword('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('admin', SecretValue.ssmSecure('/dbPassword', '1')), // Use password from SSM
jogold marked this conversation as resolved.
Show resolved Hide resolved
});

const mySecret = secretsmanager.Secret.fromSecretName(this, 'DBSecret', 'myDBLoginInfo');
Expand Down Expand Up @@ -412,7 +412,7 @@ in the cloud without managing any database instances.

The following example initializes an Aurora Serverless PostgreSql cluster.
Aurora Serverless clusters can specify scaling properties which will be used to
automatically scale the database cluster seamlessly based on the workload.
automatically scale the database cluster seamlessly based on the workload.

```ts
import * as ec2 from '@aws-cdk/aws-ec2';
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
26 changes: 26 additions & 0 deletions packages/@aws-cdk/aws-rds/lib/database-secret.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
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';
Expand Down Expand Up @@ -33,6 +34,16 @@ export interface DatabaseSecretProps {
* @default " %+~`#$&*()|[]{}:;<>?!'/@\"\\"
*/
readonly excludeCharacters?: string;

/**
* Whether to override 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 overrideLogicalId?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder whether a name like replaceOnPasswordChanges might be better here?

}

/**
Expand All @@ -55,5 +66,20 @@ export class DatabaseSecret extends secretsmanager.Secret {
excludeCharacters: props.excludeCharacters ?? DEFAULT_PASSWORD_EXCLUDE_CHARS,
},
});

if (props.overrideLogicalId) {
const hash = crypto.createHash('md5');
hash.update(JSON.stringify({
path: this.node.path, // make it unique
// 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: props.excludeCharacters,
jogold marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So excludeCharacters is actually optional. Which means this object will have undefined there as the default. Is that a mistake? Should the default value be filled here? (See line 66 above)

}));
const logicalId = `${this.node.id.replace(/[^A-Za-z0-9]/g, '')}${hash.digest('hex')}`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like id is too short here. You'll wind up with Secret<some-hash> everywhere, which is too ambiguous. Can you use uniqueId here instead? (No need for the replace part then).


const secret = this.node.defaultChild as secretsmanager.CfnSecret;
secret.overrideLogicalId(logicalId.slice(-255)); // Take last 255 chars
}
}
}
14 changes: 5 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,9 @@ export class DatabaseInstanceFromSnapshot extends DatabaseInstanceSource impleme
username: credentials.username,
encryptionKey: credentials.encryptionKey,
excludeCharacters: credentials.excludeCharacters,
// Always replace secret when customization options are changed
// (MasterUsername is not specified for an instance created from a snapshot)
overrideLogicalId: true,
skinny85 marked this conversation as resolved.
Show resolved Hide resolved
});
}

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
overrideLogicalId: credentials?.usernameAsString,
}),
// pass username if it must be referenced as a string
credentials?.usernameAsString ? renderedCredentials.username : undefined,
);
}

return renderedCredentials;
}
71 changes: 58 additions & 13 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 SecretsManager.
*/
public static fromGeneratedPassword(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.
*
* @deprecated use `fromGeneratedPassword()` or `fromPassword()` for new Clusters and Instances.
* Note that switching from `fromUsername()` to `fromGeneratedPassword()` or `fromPassword()` for already deployed
* Clusters or Instances will result in their replacement!
*/
public static fromUsername(username: string, options: CredentialsFromUsernameOptions = {}): Credentials {
return {
Expand All @@ -171,10 +203,15 @@ 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
*/
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 +223,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.
jogold marked this conversation as resolved.
Show resolved Hide resolved
*
* @default false
*/
public abstract readonly usernameAsString?: boolean;

/**
* 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 @@ -4,10 +4,9 @@ import * as secretsmanager from '@aws-cdk/aws-secretsmanager';
import { Resource, Duration, Token, Annotations, RemovalPolicy, IResource, Stack } 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 { 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 @@ -376,14 +375,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