Skip to content

Commit

Permalink
fix RDS Aurora enrollment security group tips
Browse files Browse the repository at this point in the history
  • Loading branch information
GavinFrazar committed Oct 25, 2024
1 parent d82ee84 commit 0b87111
Show file tree
Hide file tree
Showing 4 changed files with 58 additions and 37 deletions.
11 changes: 6 additions & 5 deletions lib/srv/discovery/common/database.go
Original file line number Diff line number Diff line change
Expand Up @@ -466,11 +466,12 @@ func MetadataFromRDSV2Cluster(rdsCluster *rdstypes.DBCluster, rdsInstance *rdsty
Region: parsedARN.Region,
AccountID: parsedARN.AccountID,
RDS: types.RDS{
ClusterID: aws.StringValue(rdsCluster.DBClusterIdentifier),
ResourceID: aws.StringValue(rdsCluster.DbClusterResourceId),
IAMAuth: aws.BoolValue(rdsCluster.IAMDatabaseAuthenticationEnabled),
Subnets: subnets,
VPCID: vpcID,
ClusterID: aws.StringValue(rdsCluster.DBClusterIdentifier),
ResourceID: aws.StringValue(rdsCluster.DbClusterResourceId),
IAMAuth: aws.BoolValue(rdsCluster.IAMDatabaseAuthenticationEnabled),
Subnets: subnets,
VPCID: vpcID,
SecurityGroups: rdsSecurityGroupInfo(rdsCluster.VpcSecurityGroups),
},
}, nil
}
Expand Down
15 changes: 14 additions & 1 deletion lib/srv/discovery/common/database_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -572,6 +572,11 @@ func TestDatabaseFromRDSV2Cluster(t *testing.T) {
Endpoint: aws.String("localhost"),
ReaderEndpoint: aws.String("reader.host"),
Port: aws.Int32(3306),
VpcSecurityGroups: []rdsTypesV2.VpcSecurityGroupMembership{
{VpcSecurityGroupId: aws.String("")},
{VpcSecurityGroupId: aws.String("sg-1")},
{VpcSecurityGroupId: aws.String("sg-2")},
},
CustomEndpoints: []string{
"myendpoint1.cluster-custom-example.us-east-1.rds.amazonaws.com",
"myendpoint2.cluster-custom-example.us-east-1.rds.amazonaws.com",
Expand All @@ -589,6 +594,10 @@ func TestDatabaseFromRDSV2Cluster(t *testing.T) {
ClusterID: "cluster-1",
ResourceID: "resource-1",
IAMAuth: true,
SecurityGroups: []string{
"sg-1",
"sg-2",
},
},
}

Expand Down Expand Up @@ -673,7 +682,11 @@ func TestDatabaseFromRDSV2Cluster(t *testing.T) {
ResourceID: "resource-1",
IAMAuth: true,
Subnets: []string{"subnet-123", "subnet-456"},
VPCID: "vpc-123",
SecurityGroups: []string{
"sg-1",
"sg-2",
},
VPCID: "vpc-123",
},
},
})
Expand Down
2 changes: 2 additions & 0 deletions web/packages/shared/utils/text.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ test('pluralize', () => {
expect(pluralize(0, 'apple')).toBe('apples');
expect(pluralize(1, 'apple')).toBe('apple');
expect(pluralize(2, 'apple')).toBe('apples');
expect(pluralize(undefined, 'apple')).toBe('apples');
expect(pluralize(null, 'apple')).toBe('apples');
});

test('capitalizeFirstLetter', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -203,25 +203,22 @@ function withTips(
securityGroups: SecurityGroup[],
db: AwsRdsDatabase
): SecurityGroupWithRecommendation[] {
if (!db || !securityGroups) {
// return early without trying to add tips if there's no selected db or
// security groups given.
return securityGroups;
}
const trustedGroups = getTrustedSecurityGroups(securityGroups, db);
return securityGroups.map(group => {
const isTrusted = trustedGroups.has(group.id);
const isOutboundAllowed = allowsOutbound(group);
return {
...group,
tips: getTips(db, isTrusted, isOutboundAllowed),
// we recommend when either are true because security group rules are
// additive, meaning they can select multiple groups for a combined effect
// of satisfying the database inbound rules and the ECS task outbound
// rules.
recommended: isTrusted || isOutboundAllowed,
};
});
return (
securityGroups?.map(group => {
const isTrusted = trustedGroups?.has(group.id) ?? false;
const isOutboundAllowed = allowsOutbound(group);
return {
...group,
tips: getTips(db, isTrusted, isOutboundAllowed),
// we recommend when either are true because security group rules are
// additive, meaning they can select multiple groups for a combined effect
// of satisfying the database inbound rules and the ECS task outbound
// rules.
recommended: isTrusted || isOutboundAllowed,
};
}) ?? securityGroups
);
}

function getTips(
Expand All @@ -232,7 +229,7 @@ function getTips(
const result: string[] = [];
if (isTrusted) {
result.push(
`The inbound rules of the database ${pluralize(db.securityGroups.length, 'security group')} allow traffic from this security group`
`The inbound rules of the database ${pluralize(db.securityGroups?.length, 'security group')} allow traffic from this security group`
);
}
if (allowsOutbound) {
Expand All @@ -242,11 +239,16 @@ function getTips(
}

function allowsOutbound(sg: SecurityGroup): boolean {
return sg.outboundRules.some(rule => {
const havePorts = allowsOutboundToPorts(rule);
// this is a heuristic, because an exhaustive analysis is non-trivial.
return havePorts && rule.cidrs.some(cidr => cidr.cidr === '0.0.0.0/0');
});
return (
sg.outboundRules?.some(rule => {
if (!rule) {
return false;
}
const havePorts = allowsOutboundToPorts(rule);
// this is a heuristic, because an exhaustive analysis is non-trivial.
return havePorts && rule.cidrs?.some(cidr => cidr.cidr === '0.0.0.0/0');
}) ?? false
);
}

function allowsOutboundToPorts(rule: SecurityGroupRule): boolean {
Expand All @@ -268,18 +270,21 @@ function getTrustedSecurityGroups(
securityGroups: SecurityGroup[],
db: AwsRdsDatabase
): Set<string> {
const securityGroupsById = new Map(
securityGroups.map(group => [group.id, group])
);
const trustedGroups = new Set<string>();
if (!db) {
return trustedGroups;
}

const dbPort = getPort(db);
const trustedGroups = new Set<string>();
db.securityGroups.forEach(groupId => {
const securityGroupsById = new Map(
securityGroups?.map(group => [group.id, group])
);
db?.securityGroups?.forEach(groupId => {
const group = securityGroupsById.get(groupId);
if (!group) {
return;
}
group.inboundRules.forEach(rule => {
group.inboundRules?.forEach(rule => {
if (!rule.groups?.length) {
// we only care about rules that reference other security groups.
return;
Expand Down Expand Up @@ -307,6 +312,6 @@ function ruleAllowsPort(rule: SecurityGroupRule, port: number): boolean {
}

function getPort(db: AwsRdsDatabase): number {
const [, port = '-1'] = db.uri.split(':');
const [, port = '-1'] = db?.uri?.split(':') ?? [];
return parseInt(port, 10);
}

0 comments on commit 0b87111

Please sign in to comment.