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): clusters created from snapshots generate incorrect passwords #20473

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
25 changes: 22 additions & 3 deletions packages/@aws-cdk/aws-rds/lib/cluster.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ import { IClusterEngine } from './cluster-engine';
import { DatabaseClusterAttributes, IDatabaseCluster } from './cluster-ref';
import { Endpoint } from './endpoint';
import { IParameterGroup, ParameterGroup } from './parameter-group';
import { applyDefaultRotationOptions, defaultDeletionProtection, renderCredentials, setupS3ImportExport, helperRemovalPolicy, renderUnless } from './private/util';
import { BackupProps, Credentials, InstanceProps, PerformanceInsightRetention, RotationSingleUserOptions, RotationMultiUserOptions } from './props';
import { applyDefaultRotationOptions, defaultDeletionProtection, renderCredentials, setupS3ImportExport, helperRemovalPolicy, renderUnless, renderClusterSnapshotCredentials } from './private/util';
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';
Expand Down Expand Up @@ -661,9 +661,18 @@ export interface DatabaseClusterFromSnapshotProps extends DatabaseClusterBasePro
/**
* Credentials for the administrative user
*
* @deprecated - use {@link DatabaseClusterFromSnapshotProps.snapshotCredentials} instead
*
* @default - A username of 'admin' (or 'postgres' for PostgreSQL) and SecretsManager-generated password
*/
readonly credentials?: Credentials;

/**
* Credentials for the administrative user
*
* @default - No credentials are generated
*/
readonly snapshotCredentials?: ClusterSnapshotCredentials;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need this to be a different field because you want to change the default behavior, from how the current credentials field behaves (in the details)? Or does it need to be a different field because it means something completely different?

If it's the first, a feature flag might be better.

Copy link
Contributor Author

@peterwoodworth peterwoodworth May 25, 2022

Choose a reason for hiding this comment

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

I need a different field because I want the customer to use ClusterSnapshotCredentials, as its designed specifically for using credentials when creating clusters from snapshots.

First, I tried to modify the Credentials class and renderCredentials() to work with what the customer might already have, but the code got confusing (would have been pretty difficult to document functionality for users too), and would be hard to maintain in the future. So I deprecated the prop to have customer use ClusterSnapshotCredentials when creating a cluster from snapshot and setting credentials. This class allows you to reference a full existing secret, or it will generate a new secret for you based on your login credentials passed as a secret value.

Sidenote - I believe this latter functionality (generating a secret based on login credentials) in my PR is bugged at the moment, or at the least poorly documented. The problem is that the fromPassword() method doesn't describe that the user should probably also have a username field in their passed SecretValue. I originally wanted to set the secretStringValue of the generated secret to contain the username which is passed in directly through CDK code, but I couldn't figure out how to properly set the eventual SecretString field to contain anything other than the customer passed SecretValue (other than with sloppy escape hatches).

So, long way to answer that I want this to be something completely different. wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this a case where a breaking change might be acceptable? The current credentials prop is not usable as it does not set the password of a cluster created from a snapshot, moreover it doesn't work with rotation. So no one is using this to set a password?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it not possible to use the existing SnapshotCredentials class here?

Copy link
Contributor

Choose a reason for hiding this comment

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

So no one is using this to set a password?

Correction: it is maybe used with secrets containing the snapshot password and that need to benefit from the attachment.

Copy link
Contributor Author

@peterwoodworth peterwoodworth May 25, 2022

Choose a reason for hiding this comment

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

RDS is considered stable, so we can't make a breaking change. You can use Credentials.fromSecret() to use an existing secret that you know works, and there could be users out there doing that. As far as I can tell though, all the other methods on the Credentials class are broken (fromPassword ends up doing nothing as far as I can tell)

I tried using the existing SnapshotCredentials class as well, but that got a bit sloppy with the way it's correctly implemented for DatabaseInstanceFromSnapshot. Ends up being safest for the user to not make mistakes with a new class

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be possible to do this with existing SnapshotCredentials class, I will make some tests.

I see that with the new class you're creating a secret that is not a JSON but a string with the password. This will not work with attachment or rotation.

Copy link
Contributor Author

@peterwoodworth peterwoodworth May 25, 2022

Choose a reason for hiding this comment

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

I do think it's possible, but I personally wasn't a fan of how the documentation of the methods would have to have two different definitions depending on if the method is being used for an instance or cluster, and I was also would have rather just not let the customer be able to access the generate methods on the SnapshotCredentials class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think it would still be better to use the SnapshotCredentials class?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't able to figure out how to create a secret with JSON - I feel like I'm missing something simple but that's ideally what I'd like to do so that the user can pass in the username directly through CDK

}

/**
Expand All @@ -687,8 +696,18 @@ export class DatabaseClusterFromSnapshot extends DatabaseClusterNew {
constructor(scope: Construct, id: string, props: DatabaseClusterFromSnapshotProps) {
super(scope, id, props);

if (props.credentials && !props.credentials.password && !props.credentials.secret) {
Annotations.of(scope).addWarning('Cannot modify password of a cluster created from a snapshot. Use `snapshotCredentials` instead');
}
const credentials = renderCredentials(this, props.engine, props.credentials);
const secret = credentials.secret;

const snapshotCredentials = renderClusterSnapshotCredentials(this, props.snapshotCredentials);
let secret;
if (snapshotCredentials) {
secret = snapshotCredentials.secret;
} else if (credentials) {
secret = credentials.secret;
}

const cluster = new CfnDBCluster(this, 'Resource', {
...this.newCfnProps,
Expand Down
1 change: 1 addition & 0 deletions packages/@aws-cdk/aws-rds/lib/instance.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1090,6 +1090,7 @@ export class DatabaseInstanceFromSnapshot extends DatabaseInstanceSource impleme
excludeCharacters: credentials.excludeCharacters,
replaceOnPasswordCriteriaChanges: credentials.replaceOnPasswordCriteriaChanges,
replicaRegions: credentials.replicaRegions,
secretName: credentials.secretName,
});
}

Expand Down
28 changes: 26 additions & 2 deletions packages/@aws-cdk/aws-rds/lib/private/util.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
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 { RemovalPolicy } from '@aws-cdk/core';
import * as secretsmanager from '@aws-cdk/aws-secretsmanager';
import { Aws, RemovalPolicy } from '@aws-cdk/core';
import { DatabaseSecret } from '../database-secret';
import { IEngine } from '../engine';
import { CommonRotationUserOptions, Credentials } 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
Expand Down Expand Up @@ -111,6 +112,29 @@ export function renderCredentials(scope: Construct, engine: IEngine, credentials
return renderedCredentials;
}

/**
* Renders credentials for a cluster from snapshot
*/
export function renderClusterSnapshotCredentials(scope: Construct, credentials?: ClusterSnapshotCredentials): ClusterSnapshotCredentials | undefined {
let renderedCredentials = credentials;

if (renderedCredentials?.secret) return renderedCredentials;

if (renderedCredentials?.password) {
renderedCredentials = ClusterSnapshotCredentials.fromSecret(
new secretsmanager.Secret(scope, 'Secret', {
description: `Generated by the CDK for stack: ${Aws.STACK_NAME}`,
encryptionKey: renderedCredentials.encryptionKey,
replicaRegions: renderedCredentials.replicaRegions,
secretName: renderedCredentials.secretName,
secretStringValue: renderedCredentials.password,
}),
);
}

return renderedCredentials;
}

/**
* The RemovalPolicy that should be applied to a "helper" resource, if the base resource has the given removal policy
*
Expand Down
114 changes: 105 additions & 9 deletions packages/@aws-cdk/aws-rds/lib/props.ts
Original file line number Diff line number Diff line change
Expand Up @@ -307,9 +307,9 @@ export abstract class Credentials {
}

/**
* Options used in the {@link SnapshotCredentials.fromGeneratedPassword} method.
* Options used in the {@link SnapshotCredentials.fromPassword} method.
*/
export interface SnapshotCredentialsFromGeneratedPasswordOptions {
export interface GeneratedSecretForSnapshotCredentialsOptions {
/**
* KMS encryption key to encrypt the generated secret.
*
Expand All @@ -318,18 +318,30 @@ export interface SnapshotCredentialsFromGeneratedPasswordOptions {
readonly encryptionKey?: kms.IKey;

/**
* The characters to exclude from the generated password.
* A list of regions where to replicate this secret.
*
* @default - the DatabaseSecret default exclude character set (" %+~`#$&*()|[]{}:;<>?!'/@\"\\")
* @default - Secret is not replicated
*/
readonly excludeCharacters?: string;
readonly replicaRegions?: secretsmanager.ReplicaRegion[];

/**
* A list of regions where to replicate this secret.
* Name of the generated secret
*
* @default - Secret is not replicated
* @default - default name is generated
*/
readonly replicaRegions?: secretsmanager.ReplicaRegion[];
readonly secretName?: string;
}

/**
* Options used in the {@link SnapshotCredentials.fromGeneratedPassword} method.
*/
export interface SnapshotCredentialsFromGeneratedPasswordOptions extends GeneratedSecretForSnapshotCredentialsOptions {
/**
* The characters to exclude from the generated password.
*
* @default - the DatabaseSecret default exclude character set (" %+~`#$&*()|[]{}:;<>?!'/@\"\\")
*/
readonly excludeCharacters?: string;
}

/**
Expand Down Expand Up @@ -370,7 +382,10 @@ export abstract class SnapshotCredentials {
* Update the snapshot login with an existing password.
*/
public static fromPassword(password: SecretValue): SnapshotCredentials {
return { generatePassword: false, password };
return {
generatePassword: false,
password,
};
}

/**
Expand Down Expand Up @@ -437,6 +452,13 @@ export abstract class SnapshotCredentials {
*/
public abstract readonly secret?: secretsmanager.ISecret;

/**
* Name of secret to create
*
* @default - default name is generated
*/
public abstract readonly secretName?: string;

/**
* The characters to exclude from the generated password.
* Only used if {@link generatePassword} if true.
Expand All @@ -453,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.
*/
Expand Down
26 changes: 21 additions & 5 deletions packages/@aws-cdk/aws-rds/lib/serverless-cluster.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,8 @@ import { DatabaseSecret } from './database-secret';
import { Endpoint } from './endpoint';
import { IParameterGroup } from './parameter-group';
import { DATA_API_ACTIONS } from './perms';
import { applyDefaultRotationOptions, defaultDeletionProtection, renderCredentials } from './private/util';
import { Credentials, RotationMultiUserOptions, RotationSingleUserOptions, SnapshotCredentials } from './props';
import { applyDefaultRotationOptions, defaultDeletionProtection, renderClusterSnapshotCredentials, renderCredentials } from './private/util';
import { ClusterSnapshotCredentials, Credentials, RotationMultiUserOptions, RotationSingleUserOptions, SnapshotCredentials } from './props';
import { CfnDBCluster, CfnDBClusterProps } from './rds.generated';
import { ISubnetGroup, SubnetGroup } from './subnet-group';

Expand Down Expand Up @@ -652,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;
}

/**
Expand All @@ -673,13 +684,19 @@ export class ServerlessClusterFromSnapshot extends ServerlessClusterNew {

this.enableDataApi = props.enableDataApi;

let credentials = props.credentials;
let 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,
Expand All @@ -692,7 +709,6 @@ export class ServerlessClusterFromSnapshot extends ServerlessClusterNew {
const cluster = new CfnDBCluster(this, 'Resource', {
...this.newCfnProps,
snapshotIdentifier: props.snapshotIdentifier,
masterUserPassword: secret?.secretValueFromJson('password')?.unsafeUnwrap() ?? credentials?.password?.unsafeUnwrap(), // Safe usage
});

this.clusterIdentifier = cluster.ref;
Expand Down
Loading