Skip to content

Commit dc7a17c

Browse files
jogoldjogold
authored
fix(rds): subnet selection not respected for multi user secret rotation (#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 #19233 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
1 parent 0da57da commit dc7a17c

File tree

7 files changed

+158
-60
lines changed

7 files changed

+158
-60
lines changed

‎packages/@aws-cdk/aws-rds/lib/cluster.ts

+5-7
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ import { IClusterEngine } from './cluster-engine';
1111
import { DatabaseClusterAttributes, IDatabaseCluster } from './cluster-ref';
1212
import { Endpoint } from './endpoint';
1313
import { IParameterGroup, ParameterGroup } from './parameter-group';
14-
import { DEFAULT_PASSWORD_EXCLUDE_CHARS, defaultDeletionProtection, renderCredentials, setupS3ImportExport, helperRemovalPolicy, renderUnless } from './private/util';
14+
import { applyDefaultRotationOptions, defaultDeletionProtection, renderCredentials, setupS3ImportExport, helperRemovalPolicy, renderUnless } from './private/util';
1515
import { BackupProps, Credentials, InstanceProps, PerformanceInsightRetention, RotationSingleUserOptions, RotationMultiUserOptions } from './props';
1616
import { DatabaseProxy, DatabaseProxyOptions, ProxyTarget } from './proxy';
1717
import { CfnDBCluster, CfnDBClusterProps, CfnDBInstance } from './rds.generated';
@@ -595,13 +595,11 @@ export class DatabaseCluster extends DatabaseClusterNew {
595595
}
596596

597597
return new secretsmanager.SecretRotation(this, id, {
598+
...applyDefaultRotationOptions(options, this.vpcSubnets),
598599
secret: this.secret,
599600
application: this.singleUserRotationApplication,
600601
vpc: this.vpc,
601-
vpcSubnets: this.vpcSubnets,
602602
target: this,
603-
...options,
604-
excludeCharacters: options.excludeCharacters ?? DEFAULT_PASSWORD_EXCLUDE_CHARS,
605603
});
606604
}
607605

@@ -612,13 +610,13 @@ export class DatabaseCluster extends DatabaseClusterNew {
612610
if (!this.secret) {
613611
throw new Error('Cannot add multi user rotation for a cluster without secret.');
614612
}
613+
615614
return new secretsmanager.SecretRotation(this, id, {
616-
...options,
617-
excludeCharacters: options.excludeCharacters ?? DEFAULT_PASSWORD_EXCLUDE_CHARS,
615+
...applyDefaultRotationOptions(options, this.vpcSubnets),
616+
secret: options.secret,
618617
masterSecret: this.secret,
619618
application: this.multiUserRotationApplication,
620619
vpc: this.vpc,
621-
vpcSubnets: this.vpcSubnets,
622620
target: this,
623621
});
624622
}

‎packages/@aws-cdk/aws-rds/lib/instance.ts

+5-7
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ import { Endpoint } from './endpoint';
1313
import { IInstanceEngine } from './instance-engine';
1414
import { IOptionGroup } from './option-group';
1515
import { IParameterGroup, ParameterGroup } from './parameter-group';
16-
import { DEFAULT_PASSWORD_EXCLUDE_CHARS, defaultDeletionProtection, engineDescription, renderCredentials, setupS3ImportExport, helperRemovalPolicy, renderUnless } from './private/util';
16+
import { applyDefaultRotationOptions, defaultDeletionProtection, engineDescription, renderCredentials, setupS3ImportExport, helperRemovalPolicy, renderUnless } from './private/util';
1717
import { Credentials, PerformanceInsightRetention, RotationMultiUserOptions, RotationSingleUserOptions, SnapshotCredentials } from './props';
1818
import { DatabaseProxy, DatabaseProxyOptions, ProxyTarget } from './proxy';
1919
import { CfnDBInstance, CfnDBInstanceProps } from './rds.generated';
@@ -931,13 +931,11 @@ abstract class DatabaseInstanceSource extends DatabaseInstanceNew implements IDa
931931
}
932932

933933
return new secretsmanager.SecretRotation(this, id, {
934+
...applyDefaultRotationOptions(options, this.vpcPlacement),
934935
secret: this.secret,
935936
application: this.singleUserRotationApplication,
936937
vpc: this.vpc,
937-
vpcSubnets: this.vpcPlacement,
938938
target: this,
939-
...options,
940-
excludeCharacters: options.excludeCharacters ?? DEFAULT_PASSWORD_EXCLUDE_CHARS,
941939
});
942940
}
943941

@@ -948,13 +946,13 @@ abstract class DatabaseInstanceSource extends DatabaseInstanceNew implements IDa
948946
if (!this.secret) {
949947
throw new Error('Cannot add multi user rotation for an instance without secret.');
950948
}
949+
951950
return new secretsmanager.SecretRotation(this, id, {
952-
...options,
953-
excludeCharacters: options.excludeCharacters ?? DEFAULT_PASSWORD_EXCLUDE_CHARS,
951+
...applyDefaultRotationOptions(options, this.vpcPlacement),
952+
secret: options.secret,
954953
masterSecret: this.secret,
955954
application: this.multiUserRotationApplication,
956955
vpc: this.vpc,
957-
vpcSubnets: this.vpcPlacement,
958956
target: this,
959957
});
960958
}

‎packages/@aws-cdk/aws-rds/lib/private/util.ts

+13-1
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,10 @@
1+
import * as ec2 from '@aws-cdk/aws-ec2';
12
import * as iam from '@aws-cdk/aws-iam';
23
import * as s3 from '@aws-cdk/aws-s3';
34
import { RemovalPolicy } from '@aws-cdk/core';
45
import { DatabaseSecret } from '../database-secret';
56
import { IEngine } from '../engine';
6-
import { Credentials } from '../props';
7+
import { CommonRotationUserOptions, Credentials } from '../props';
78

89
// keep this import separate from other imports to reduce chance for merge conflicts with v2-main
910
// eslint-disable-next-line no-duplicate-imports, import/order
@@ -134,3 +135,14 @@ export function helperRemovalPolicy(basePolicy?: RemovalPolicy): RemovalPolicy {
134135
export function renderUnless<A>(value: A, suppressValue: A): A | undefined {
135136
return value === suppressValue ? undefined : value;
136137
}
138+
139+
/**
140+
* Applies defaults for rotation options
141+
*/
142+
export function applyDefaultRotationOptions(options: CommonRotationUserOptions, defaultvpcSubnets?: ec2.SubnetSelection): CommonRotationUserOptions {
143+
return {
144+
excludeCharacters: DEFAULT_PASSWORD_EXCLUDE_CHARS,
145+
vpcSubnets: defaultvpcSubnets,
146+
...options,
147+
};
148+
}

‎packages/@aws-cdk/aws-rds/lib/props.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -456,7 +456,7 @@ export abstract class SnapshotCredentials {
456456
/**
457457
* Properties common to single-user and multi-user rotation options.
458458
*/
459-
interface CommonRotationUserOptions {
459+
export interface CommonRotationUserOptions {
460460
/**
461461
* Specifies the number of days after the previous rotation
462462
* before Secrets Manager triggers the next automatic rotation.

‎packages/@aws-cdk/aws-rds/lib/serverless-cluster.ts

+4-7
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import { DatabaseSecret } from './database-secret';
1010
import { Endpoint } from './endpoint';
1111
import { IParameterGroup } from './parameter-group';
1212
import { DATA_API_ACTIONS } from './perms';
13-
import { defaultDeletionProtection, DEFAULT_PASSWORD_EXCLUDE_CHARS, renderCredentials } from './private/util';
13+
import { applyDefaultRotationOptions, defaultDeletionProtection, renderCredentials } from './private/util';
1414
import { Credentials, RotationMultiUserOptions, RotationSingleUserOptions, SnapshotCredentials } from './props';
1515
import { CfnDBCluster, CfnDBClusterProps } from './rds.generated';
1616
import { ISubnetGroup, SubnetGroup } from './subnet-group';
@@ -558,13 +558,11 @@ export class ServerlessCluster extends ServerlessClusterNew {
558558
}
559559

560560
return new secretsmanager.SecretRotation(this, id, {
561+
...applyDefaultRotationOptions(options, this.vpcSubnets),
561562
secret: this.secret,
562563
application: this.singleUserRotationApplication,
563564
vpc: this.vpc,
564-
vpcSubnets: this.vpcSubnets,
565565
target: this,
566-
...options,
567-
excludeCharacters: options.excludeCharacters ?? DEFAULT_PASSWORD_EXCLUDE_CHARS,
568566
});
569567
}
570568

@@ -581,12 +579,11 @@ export class ServerlessCluster extends ServerlessClusterNew {
581579
}
582580

583581
return new secretsmanager.SecretRotation(this, id, {
584-
...options,
585-
excludeCharacters: options.excludeCharacters ?? DEFAULT_PASSWORD_EXCLUDE_CHARS,
582+
...applyDefaultRotationOptions(options, this.vpcSubnets),
583+
secret: options.secret,
586584
masterSecret: this.secret,
587585
application: this.multiUserRotationApplication,
588586
vpc: this.vpc,
589-
vpcSubnets: this.vpcSubnets,
590587
target: this,
591588
});
592589
}

‎packages/@aws-cdk/aws-rds/test/cluster.test.ts

+65-18
Original file line numberDiff line numberDiff line change
@@ -889,15 +889,18 @@ describe('cluster', () => {
889889
});
890890
});
891891

892-
test('addRotationSingleUser() with options', () => {
892+
test('addRotationSingleUser() with custom automaticallyAfter, excludeCharacters and vpcSubnets', () => {
893893
// GIVEN
894894
const stack = new cdk.Stack();
895-
const vpcWithIsolated = new ec2.Vpc(stack, 'Vpc', {
896-
subnetConfiguration: [
897-
{ name: 'public', subnetType: ec2.SubnetType.PUBLIC },
898-
{ name: 'private', subnetType: ec2.SubnetType.PRIVATE_WITH_NAT },
899-
{ name: 'isolated', subnetType: ec2.SubnetType.PRIVATE_ISOLATED },
900-
],
895+
const vpcWithIsolated = ec2.Vpc.fromVpcAttributes(stack, 'Vpc', {
896+
vpcId: 'vpc-id',
897+
availabilityZones: ['az1'],
898+
publicSubnetIds: ['public-subnet-id-1', 'public-subnet-id-2'],
899+
publicSubnetNames: ['public-subnet-name-1', 'public-subnet-name-2'],
900+
privateSubnetIds: ['private-subnet-id-1', 'private-subnet-id-2'],
901+
privateSubnetNames: ['private-subnet-name-1', 'private-subnet-name-2'],
902+
isolatedSubnetIds: ['isolated-subnet-id-1', 'isolated-subnet-id-2'],
903+
isolatedSubnetNames: ['isolated-subnet-name-1', 'isolated-subnet-name-2'],
901904
});
902905

903906
// WHEN
@@ -935,20 +938,64 @@ describe('cluster', () => {
935938
{ Ref: 'AWS::URLSuffix' },
936939
]],
937940
},
938-
functionName: 'DatabaseRotationSingleUser458A45BE',
939-
vpcSubnetIds: {
941+
vpcSubnetIds: 'private-subnet-id-1,private-subnet-id-2',
942+
excludeCharacters: '°_@',
943+
},
944+
});
945+
});
946+
947+
test('addRotationMultiUser() with custom automaticallyAfter, excludeCharacters and vpcSubnets', () => {
948+
// GIVEN
949+
const stack = new cdk.Stack();
950+
const vpcWithIsolated = ec2.Vpc.fromVpcAttributes(stack, 'Vpc', {
951+
vpcId: 'vpc-id',
952+
availabilityZones: ['az1'],
953+
publicSubnetIds: ['public-subnet-id-1', 'public-subnet-id-2'],
954+
publicSubnetNames: ['public-subnet-name-1', 'public-subnet-name-2'],
955+
privateSubnetIds: ['private-subnet-id-1', 'private-subnet-id-2'],
956+
privateSubnetNames: ['private-subnet-name-1', 'private-subnet-name-2'],
957+
isolatedSubnetIds: ['isolated-subnet-id-1', 'isolated-subnet-id-2'],
958+
isolatedSubnetNames: ['isolated-subnet-name-1', 'isolated-subnet-name-2'],
959+
});
960+
const userSecret = new DatabaseSecret(stack, 'UserSecret', { username: 'user' });
961+
962+
// WHEN
963+
// DB in isolated subnet (no internet connectivity)
964+
const cluster = new DatabaseCluster(stack, 'Database', {
965+
engine: DatabaseClusterEngine.AURORA_MYSQL,
966+
instanceProps: {
967+
instanceType: ec2.InstanceType.of(ec2.InstanceClass.BURSTABLE2, ec2.InstanceSize.SMALL),
968+
vpc: vpcWithIsolated,
969+
vpcSubnets: { subnetType: ec2.SubnetType.PRIVATE_ISOLATED },
970+
},
971+
});
972+
973+
// Rotation in private subnet (internet via NAT)
974+
cluster.addRotationMultiUser('user', {
975+
secret: userSecret.attach(cluster),
976+
automaticallyAfter: cdk.Duration.days(15),
977+
excludeCharacters: '°_@',
978+
vpcSubnets: { subnetType: ec2.SubnetType.PRIVATE_WITH_NAT },
979+
});
980+
981+
// THEN
982+
Template.fromStack(stack).hasResourceProperties('AWS::SecretsManager::RotationSchedule', {
983+
RotationRules: {
984+
AutomaticallyAfterDays: 15,
985+
},
986+
});
987+
988+
Template.fromStack(stack).hasResourceProperties('AWS::Serverless::Application', {
989+
Parameters: {
990+
endpoint: {
940991
'Fn::Join': ['', [
941-
{ Ref: 'VpcprivateSubnet1SubnetCEAD3716' },
942-
',',
943-
{ Ref: 'VpcprivateSubnet2Subnet2DE7549C' },
992+
'https://secretsmanager.',
993+
{ Ref: 'AWS::Region' },
994+
'.',
995+
{ Ref: 'AWS::URLSuffix' },
944996
]],
945997
},
946-
vpcSecurityGroupIds: {
947-
'Fn::GetAtt': [
948-
'DatabaseRotationSingleUserSecurityGroupAC6E0E73',
949-
'GroupId',
950-
],
951-
},
998+
vpcSubnetIds: 'private-subnet-id-1,private-subnet-id-2',
952999
excludeCharacters: '°_@',
9531000
},
9541001
});

‎packages/@aws-cdk/aws-rds/test/instance.test.ts

+65-19
Original file line numberDiff line numberDiff line change
@@ -797,14 +797,17 @@ describe('instance', () => {
797797
});
798798
});
799799

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

810813
// WHEN
@@ -839,26 +842,69 @@ describe('instance', () => {
839842
{ Ref: 'AWS::URLSuffix' },
840843
]],
841844
},
842-
functionName: 'DatabaseRotationSingleUser458A45BE',
843-
vpcSubnetIds: {
845+
vpcSubnetIds: 'private-subnet-id-1,private-subnet-id-2',
846+
excludeCharacters: '°_@',
847+
},
848+
});
849+
});
850+
851+
test('addRotationMultiUser() with custom automaticallyAfter, excludeCharacters and vpcSubnets', () => {
852+
// GIVEN
853+
const vpcWithIsolated = ec2.Vpc.fromVpcAttributes(stack, 'Vpc', {
854+
vpcId: 'vpc-id',
855+
availabilityZones: ['az1'],
856+
publicSubnetIds: ['public-subnet-id-1', 'public-subnet-id-2'],
857+
publicSubnetNames: ['public-subnet-name-1', 'public-subnet-name-2'],
858+
privateSubnetIds: ['private-subnet-id-1', 'private-subnet-id-2'],
859+
privateSubnetNames: ['private-subnet-name-1', 'private-subnet-name-2'],
860+
isolatedSubnetIds: ['isolated-subnet-id-1', 'isolated-subnet-id-2'],
861+
isolatedSubnetNames: ['isolated-subnet-name-1', 'isolated-subnet-name-2'],
862+
});
863+
const userSecret = new rds.DatabaseSecret(stack, 'UserSecret', { username: 'user' });
864+
865+
// WHEN
866+
// DB in isolated subnet (no internet connectivity)
867+
const instance = new rds.DatabaseInstance(stack, 'Database', {
868+
engine: rds.DatabaseInstanceEngine.postgres({ version: rds.PostgresEngineVersion.VER_10 }),
869+
vpc: vpcWithIsolated,
870+
vpcSubnets: { subnetType: ec2.SubnetType.PRIVATE_ISOLATED },
871+
});
872+
873+
// Rotation in private subnet (internet via NAT)
874+
instance.addRotationMultiUser('user', {
875+
secret: userSecret.attach(instance),
876+
automaticallyAfter: cdk.Duration.days(15),
877+
excludeCharacters: '°_@',
878+
vpcSubnets: { subnetType: ec2.SubnetType.PRIVATE_WITH_NAT },
879+
});
880+
881+
// THEN
882+
Template.fromStack(stack).hasResourceProperties('AWS::SecretsManager::RotationSchedule', {
883+
RotationRules: {
884+
AutomaticallyAfterDays: 15,
885+
},
886+
});
887+
888+
vpcWithIsolated.selectSubnets({
889+
subnetType: ec2.SubnetType.PRIVATE_WITH_NAT,
890+
}).subnetIds;
891+
892+
Template.fromStack(stack).hasResourceProperties('AWS::Serverless::Application', {
893+
Parameters: {
894+
endpoint: {
844895
'Fn::Join': ['', [
845-
{ Ref: 'VpcprivateSubnet1SubnetCEAD3716' },
846-
',',
847-
{ Ref: 'VpcprivateSubnet2Subnet2DE7549C' },
896+
'https://secretsmanager.',
897+
{ Ref: 'AWS::Region' },
898+
'.',
899+
{ Ref: 'AWS::URLSuffix' },
848900
]],
849901
},
850-
vpcSecurityGroupIds: {
851-
'Fn::GetAtt': [
852-
'DatabaseRotationSingleUserSecurityGroupAC6E0E73',
853-
'GroupId',
854-
],
855-
},
902+
vpcSubnetIds: 'private-subnet-id-1,private-subnet-id-2',
856903
excludeCharacters: '°_@',
857904
},
858905
});
859906
});
860907

861-
862908
test('addRotationSingleUser() with VPC interface endpoint', () => {
863909
// GIVEN
864910
const vpcIsolatedOnly = new ec2.Vpc(stack, 'Vpc', { natGateways: 0 });

0 commit comments

Comments
 (0)