Skip to content

Commit

Permalink
fix(rds): subnet selection not respected for multi user secret rotati…
Browse files Browse the repository at this point in the history 
…on (aws#19237)

The subnet selection was always overriden by the subnet selection of the
instance/cluster.

Avoid these kinds of errors by explicitely defining rotation options and
their defaults.

Closes aws#19233


----

*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 @TheRealAmazonKendra
jogold authored and TheRealAmazonKendra committed Mar 11, 2022
1 parent 66c5bea commit 7e2a69b
Show file tree
Hide file tree
Showing 7 changed files with 158 additions and 60 deletions.
12 changes: 5 additions & 7 deletions packages/@aws-cdk/aws-rds/lib/cluster.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { IClusterEngine } from './cluster-engine';
import { DatabaseClusterAttributes, IDatabaseCluster } from './cluster-ref';
import { Endpoint } from './endpoint';
import { IParameterGroup, ParameterGroup } from './parameter-group';
import { DEFAULT_PASSWORD_EXCLUDE_CHARS, defaultDeletionProtection, renderCredentials, setupS3ImportExport, helperRemovalPolicy, renderUnless } from './private/util';
import { applyDefaultRotationOptions, defaultDeletionProtection, renderCredentials, setupS3ImportExport, helperRemovalPolicy, renderUnless } 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 @@ -595,13 +595,11 @@ export class DatabaseCluster extends DatabaseClusterNew {
}

return new secretsmanager.SecretRotation(this, id, {
...applyDefaultRotationOptions(options, this.vpcSubnets),
secret: this.secret,
application: this.singleUserRotationApplication,
vpc: this.vpc,
vpcSubnets: this.vpcSubnets,
target: this,
...options,
excludeCharacters: options.excludeCharacters ?? DEFAULT_PASSWORD_EXCLUDE_CHARS,
});
}

Expand All @@ -612,13 +610,13 @@ export class DatabaseCluster extends DatabaseClusterNew {
if (!this.secret) {
throw new Error('Cannot add multi user rotation for a cluster without secret.');
}

return new secretsmanager.SecretRotation(this, id, {
...options,
excludeCharacters: options.excludeCharacters ?? DEFAULT_PASSWORD_EXCLUDE_CHARS,
...applyDefaultRotationOptions(options, this.vpcSubnets),
secret: options.secret,
masterSecret: this.secret,
application: this.multiUserRotationApplication,
vpc: this.vpc,
vpcSubnets: this.vpcSubnets,
target: this,
});
}
Expand Down
12 changes: 5 additions & 7 deletions packages/@aws-cdk/aws-rds/lib/instance.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { Endpoint } from './endpoint';
import { IInstanceEngine } from './instance-engine';
import { IOptionGroup } from './option-group';
import { IParameterGroup, ParameterGroup } from './parameter-group';
import { DEFAULT_PASSWORD_EXCLUDE_CHARS, defaultDeletionProtection, engineDescription, renderCredentials, setupS3ImportExport, helperRemovalPolicy, renderUnless } from './private/util';
import { applyDefaultRotationOptions, defaultDeletionProtection, engineDescription, renderCredentials, setupS3ImportExport, helperRemovalPolicy, renderUnless } 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 @@ -931,13 +931,11 @@ abstract class DatabaseInstanceSource extends DatabaseInstanceNew implements IDa
}

return new secretsmanager.SecretRotation(this, id, {
...applyDefaultRotationOptions(options, this.vpcPlacement),
secret: this.secret,
application: this.singleUserRotationApplication,
vpc: this.vpc,
vpcSubnets: this.vpcPlacement,
target: this,
...options,
excludeCharacters: options.excludeCharacters ?? DEFAULT_PASSWORD_EXCLUDE_CHARS,
});
}

Expand All @@ -948,13 +946,13 @@ abstract class DatabaseInstanceSource extends DatabaseInstanceNew implements IDa
if (!this.secret) {
throw new Error('Cannot add multi user rotation for an instance without secret.');
}

return new secretsmanager.SecretRotation(this, id, {
...options,
excludeCharacters: options.excludeCharacters ?? DEFAULT_PASSWORD_EXCLUDE_CHARS,
...applyDefaultRotationOptions(options, this.vpcPlacement),
secret: options.secret,
masterSecret: this.secret,
application: this.multiUserRotationApplication,
vpc: this.vpc,
vpcSubnets: this.vpcPlacement,
target: this,
});
}
Expand Down
14 changes: 13 additions & 1 deletion packages/@aws-cdk/aws-rds/lib/private/util.ts
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,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 { RemovalPolicy } from '@aws-cdk/core';
import { DatabaseSecret } from '../database-secret';
import { IEngine } from '../engine';
import { Credentials } from '../props';
import { 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 @@ -134,3 +135,14 @@ export function helperRemovalPolicy(basePolicy?: RemovalPolicy): RemovalPolicy {
export function renderUnless<A>(value: A, suppressValue: A): A | undefined {
return value === suppressValue ? undefined : value;
}

/**
* Applies defaults for rotation options
*/
export function applyDefaultRotationOptions(options: CommonRotationUserOptions, defaultvpcSubnets?: ec2.SubnetSelection): CommonRotationUserOptions {
return {
excludeCharacters: DEFAULT_PASSWORD_EXCLUDE_CHARS,
vpcSubnets: defaultvpcSubnets,
...options,
};
}
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-rds/lib/props.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -456,7 +456,7 @@ export abstract class SnapshotCredentials {
/**
* Properties common to single-user and multi-user rotation options.
*/
interface CommonRotationUserOptions {
export interface CommonRotationUserOptions {
/**
* Specifies the number of days after the previous rotation
* before Secrets Manager triggers the next automatic rotation.
Expand Down
11 changes: 4 additions & 7 deletions packages/@aws-cdk/aws-rds/lib/serverless-cluster.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { DatabaseSecret } from './database-secret';
import { Endpoint } from './endpoint';
import { IParameterGroup } from './parameter-group';
import { DATA_API_ACTIONS } from './perms';
import { defaultDeletionProtection, DEFAULT_PASSWORD_EXCLUDE_CHARS, renderCredentials } from './private/util';
import { applyDefaultRotationOptions, defaultDeletionProtection, renderCredentials } from './private/util';
import { Credentials, RotationMultiUserOptions, RotationSingleUserOptions, SnapshotCredentials } from './props';
import { CfnDBCluster, CfnDBClusterProps } from './rds.generated';
import { ISubnetGroup, SubnetGroup } from './subnet-group';
Expand Down Expand Up @@ -558,13 +558,11 @@ export class ServerlessCluster extends ServerlessClusterNew {
}

return new secretsmanager.SecretRotation(this, id, {
...applyDefaultRotationOptions(options, this.vpcSubnets),
secret: this.secret,
application: this.singleUserRotationApplication,
vpc: this.vpc,
vpcSubnets: this.vpcSubnets,
target: this,
...options,
excludeCharacters: options.excludeCharacters ?? DEFAULT_PASSWORD_EXCLUDE_CHARS,
});
}

Expand All @@ -581,12 +579,11 @@ export class ServerlessCluster extends ServerlessClusterNew {
}

return new secretsmanager.SecretRotation(this, id, {
...options,
excludeCharacters: options.excludeCharacters ?? DEFAULT_PASSWORD_EXCLUDE_CHARS,
...applyDefaultRotationOptions(options, this.vpcSubnets),
secret: options.secret,
masterSecret: this.secret,
application: this.multiUserRotationApplication,
vpc: this.vpc,
vpcSubnets: this.vpcSubnets,
target: this,
});
}
Expand Down
83 changes: 65 additions & 18 deletions packages/@aws-cdk/aws-rds/test/cluster.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -889,15 +889,18 @@ describe('cluster', () => {
});
});

test('addRotationSingleUser() with options', () => {
test('addRotationSingleUser() with custom automaticallyAfter, excludeCharacters and vpcSubnets', () => {
// GIVEN
const stack = new cdk.Stack();
const vpcWithIsolated = new ec2.Vpc(stack, 'Vpc', {
subnetConfiguration: [
{ name: 'public', subnetType: ec2.SubnetType.PUBLIC },
{ name: 'private', subnetType: ec2.SubnetType.PRIVATE_WITH_NAT },
{ name: 'isolated', subnetType: ec2.SubnetType.PRIVATE_ISOLATED },
],
const vpcWithIsolated = ec2.Vpc.fromVpcAttributes(stack, 'Vpc', {
vpcId: 'vpc-id',
availabilityZones: ['az1'],
publicSubnetIds: ['public-subnet-id-1', 'public-subnet-id-2'],
publicSubnetNames: ['public-subnet-name-1', 'public-subnet-name-2'],
privateSubnetIds: ['private-subnet-id-1', 'private-subnet-id-2'],
privateSubnetNames: ['private-subnet-name-1', 'private-subnet-name-2'],
isolatedSubnetIds: ['isolated-subnet-id-1', 'isolated-subnet-id-2'],
isolatedSubnetNames: ['isolated-subnet-name-1', 'isolated-subnet-name-2'],
});

// WHEN
Expand Down Expand Up @@ -935,20 +938,64 @@ describe('cluster', () => {
{ Ref: 'AWS::URLSuffix' },
]],
},
functionName: 'DatabaseRotationSingleUser458A45BE',
vpcSubnetIds: {
vpcSubnetIds: 'private-subnet-id-1,private-subnet-id-2',
excludeCharacters: '°_@',
},
});
});

test('addRotationMultiUser() with custom automaticallyAfter, excludeCharacters and vpcSubnets', () => {
// GIVEN
const stack = new cdk.Stack();
const vpcWithIsolated = ec2.Vpc.fromVpcAttributes(stack, 'Vpc', {
vpcId: 'vpc-id',
availabilityZones: ['az1'],
publicSubnetIds: ['public-subnet-id-1', 'public-subnet-id-2'],
publicSubnetNames: ['public-subnet-name-1', 'public-subnet-name-2'],
privateSubnetIds: ['private-subnet-id-1', 'private-subnet-id-2'],
privateSubnetNames: ['private-subnet-name-1', 'private-subnet-name-2'],
isolatedSubnetIds: ['isolated-subnet-id-1', 'isolated-subnet-id-2'],
isolatedSubnetNames: ['isolated-subnet-name-1', 'isolated-subnet-name-2'],
});
const userSecret = new DatabaseSecret(stack, 'UserSecret', { username: 'user' });

// WHEN
// DB in isolated subnet (no internet connectivity)
const cluster = new DatabaseCluster(stack, 'Database', {
engine: DatabaseClusterEngine.AURORA_MYSQL,
instanceProps: {
instanceType: ec2.InstanceType.of(ec2.InstanceClass.BURSTABLE2, ec2.InstanceSize.SMALL),
vpc: vpcWithIsolated,
vpcSubnets: { subnetType: ec2.SubnetType.PRIVATE_ISOLATED },
},
});

// Rotation in private subnet (internet via NAT)
cluster.addRotationMultiUser('user', {
secret: userSecret.attach(cluster),
automaticallyAfter: cdk.Duration.days(15),
excludeCharacters: '°_@',
vpcSubnets: { subnetType: ec2.SubnetType.PRIVATE_WITH_NAT },
});

// THEN
Template.fromStack(stack).hasResourceProperties('AWS::SecretsManager::RotationSchedule', {
RotationRules: {
AutomaticallyAfterDays: 15,
},
});

Template.fromStack(stack).hasResourceProperties('AWS::Serverless::Application', {
Parameters: {
endpoint: {
'Fn::Join': ['', [
{ Ref: 'VpcprivateSubnet1SubnetCEAD3716' },
',',
{ Ref: 'VpcprivateSubnet2Subnet2DE7549C' },
'https://secretsmanager.',
{ Ref: 'AWS::Region' },
'.',
{ Ref: 'AWS::URLSuffix' },
]],
},
vpcSecurityGroupIds: {
'Fn::GetAtt': [
'DatabaseRotationSingleUserSecurityGroupAC6E0E73',
'GroupId',
],
},
vpcSubnetIds: 'private-subnet-id-1,private-subnet-id-2',
excludeCharacters: '°_@',
},
});
Expand Down
84 changes: 65 additions & 19 deletions packages/@aws-cdk/aws-rds/test/instance.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -797,14 +797,17 @@ describe('instance', () => {
});
});

test('addRotationSingleUser() with options', () => {
test('addRotationSingleUser() with custom automaticallyAfter, excludeCharacters and vpcSubnets', () => {
// GIVEN
const vpcWithIsolated = new ec2.Vpc(stack, 'Vpc', {
subnetConfiguration: [
{ name: 'public', subnetType: ec2.SubnetType.PUBLIC },
{ name: 'private', subnetType: ec2.SubnetType.PRIVATE_WITH_NAT },
{ name: 'isolated', subnetType: ec2.SubnetType.PRIVATE_ISOLATED },
],
const vpcWithIsolated = ec2.Vpc.fromVpcAttributes(stack, 'Vpc', {
vpcId: 'vpc-id',
availabilityZones: ['az1'],
publicSubnetIds: ['public-subnet-id-1', 'public-subnet-id-2'],
publicSubnetNames: ['public-subnet-name-1', 'public-subnet-name-2'],
privateSubnetIds: ['private-subnet-id-1', 'private-subnet-id-2'],
privateSubnetNames: ['private-subnet-name-1', 'private-subnet-name-2'],
isolatedSubnetIds: ['isolated-subnet-id-1', 'isolated-subnet-id-2'],
isolatedSubnetNames: ['isolated-subnet-name-1', 'isolated-subnet-name-2'],
});

// WHEN
Expand Down Expand Up @@ -839,26 +842,69 @@ describe('instance', () => {
{ Ref: 'AWS::URLSuffix' },
]],
},
functionName: 'DatabaseRotationSingleUser458A45BE',
vpcSubnetIds: {
vpcSubnetIds: 'private-subnet-id-1,private-subnet-id-2',
excludeCharacters: '°_@',
},
});
});

test('addRotationMultiUser() with custom automaticallyAfter, excludeCharacters and vpcSubnets', () => {
// GIVEN
const vpcWithIsolated = ec2.Vpc.fromVpcAttributes(stack, 'Vpc', {
vpcId: 'vpc-id',
availabilityZones: ['az1'],
publicSubnetIds: ['public-subnet-id-1', 'public-subnet-id-2'],
publicSubnetNames: ['public-subnet-name-1', 'public-subnet-name-2'],
privateSubnetIds: ['private-subnet-id-1', 'private-subnet-id-2'],
privateSubnetNames: ['private-subnet-name-1', 'private-subnet-name-2'],
isolatedSubnetIds: ['isolated-subnet-id-1', 'isolated-subnet-id-2'],
isolatedSubnetNames: ['isolated-subnet-name-1', 'isolated-subnet-name-2'],
});
const userSecret = new rds.DatabaseSecret(stack, 'UserSecret', { username: 'user' });

// WHEN
// DB in isolated subnet (no internet connectivity)
const instance = new rds.DatabaseInstance(stack, 'Database', {
engine: rds.DatabaseInstanceEngine.postgres({ version: rds.PostgresEngineVersion.VER_10 }),
vpc: vpcWithIsolated,
vpcSubnets: { subnetType: ec2.SubnetType.PRIVATE_ISOLATED },
});

// Rotation in private subnet (internet via NAT)
instance.addRotationMultiUser('user', {
secret: userSecret.attach(instance),
automaticallyAfter: cdk.Duration.days(15),
excludeCharacters: '°_@',
vpcSubnets: { subnetType: ec2.SubnetType.PRIVATE_WITH_NAT },
});

// THEN
Template.fromStack(stack).hasResourceProperties('AWS::SecretsManager::RotationSchedule', {
RotationRules: {
AutomaticallyAfterDays: 15,
},
});

vpcWithIsolated.selectSubnets({
subnetType: ec2.SubnetType.PRIVATE_WITH_NAT,
}).subnetIds;

Template.fromStack(stack).hasResourceProperties('AWS::Serverless::Application', {
Parameters: {
endpoint: {
'Fn::Join': ['', [
{ Ref: 'VpcprivateSubnet1SubnetCEAD3716' },
',',
{ Ref: 'VpcprivateSubnet2Subnet2DE7549C' },
'https://secretsmanager.',
{ Ref: 'AWS::Region' },
'.',
{ Ref: 'AWS::URLSuffix' },
]],
},
vpcSecurityGroupIds: {
'Fn::GetAtt': [
'DatabaseRotationSingleUserSecurityGroupAC6E0E73',
'GroupId',
],
},
vpcSubnetIds: 'private-subnet-id-1,private-subnet-id-2',
excludeCharacters: '°_@',
},
});
});


test('addRotationSingleUser() with VPC interface endpoint', () => {
// GIVEN
const vpcIsolatedOnly = new ec2.Vpc(stack, 'Vpc', { natGateways: 0 });
Expand Down

0 comments on commit 7e2a69b

Please sign in to comment.