From ea72aed1b19bd82b53ac6d673c9533351f99f717 Mon Sep 17 00:00:00 2001 From: corymhall <43035978+corymhall@users.noreply.github.com> Date: Tue, 23 Apr 2024 14:24:35 -0400 Subject: [PATCH 1/4] feat: add new securityGroupNoRuleManagementConflicts rule The [docs](https://www.pulumi.com/registry/packages/aws/api-docs/ec2/securitygroup/) have strongly worded note about not using inline rules along with separate security group resources. This PR adds a new rule which will warn the user when they use both together. re pulumi/pulumi-aws#3788 --- .gitignore | 1 + integration-tests/compute/index.ts | 112 +++++++++++++++++++++++++ integration-tests/compute/package.json | 4 +- integration-tests/integration_test.go | 32 +++++++ src/compute.ts | 59 ++++++++++++- 5 files changed, 205 insertions(+), 3 deletions(-) diff --git a/.gitignore b/.gitignore index 6484f7c..7ddc32b 100644 --- a/.gitignore +++ b/.gitignore @@ -6,3 +6,4 @@ **/package-lock.json yarn-error.log +**/Pulumi.*.yaml diff --git a/integration-tests/compute/index.ts b/integration-tests/compute/index.ts index 8acb580..c4db620 100644 --- a/integration-tests/compute/index.ts +++ b/integration-tests/compute/index.ts @@ -20,6 +20,14 @@ const testScenario = config.getNumber("scenario"); console.log(`Running test scenario #${testScenario}`); +const vpc = new aws.ec2.Vpc('test-vpc', { }); +const subnet = new aws.ec2.Subnet('test-subnet-1', { + vpcId: vpc.id, +}); +const subnet2 = new aws.ec2.Subnet('test-subnet-2', { + vpcId: vpc.id, +}); + const ami = pulumi.output(aws.ec2.getAmi({ filters: [{ name: "name", @@ -53,6 +61,7 @@ let elbArgs: aws.elb.LoadBalancerArgs = { enabled: true, bucket: elbBucket.arn, }, + subnets: [subnet.id, subnet2.id], listeners: [], }; @@ -61,6 +70,7 @@ let elbV2Args: aws.lb.LoadBalancerArgs = { enabled: true, bucket: elbBucket.arn, }, + subnets: [subnet.id, subnet2.id], enableDeletionProtection: true, }; @@ -69,9 +79,15 @@ let albArgs: aws.alb.LoadBalancerArgs = { enabled: true, bucket: elbBucket.arn, }, + subnets: [subnet.id, subnet2.id], enableDeletionProtection: true, }; +let sgArgs: aws.ec2.SecurityGroupArgs = {}; +let sgRuleArgs: ((id: pulumi.Input) => aws.ec2.SecurityGroupRuleArgs) | undefined; +let sgEgressRuleArgs: ((id: pulumi.Input) => aws.vpc.SecurityGroupEgressRuleArgs) | undefined; +let sgIngressRuleArgs: ((id: pulumi.Input) => aws.vpc.SecurityGroupIngressRuleArgs) | undefined; + switch (testScenario) { case 1: // Happy Path. @@ -170,6 +186,92 @@ switch (testScenario) { }] }; break; + case 10: + // No SecurityGroupRule of type 'egress' for a SecurityGroup with inline egress rules. + sgArgs = { + egress: [{ + toPort: 80, + fromPort: 80, + cidrBlocks: ['0.0.0.0/0'], + protocol: 'tcp', + }], + }; + + sgRuleArgs = (id: pulumi.Input): aws.ec2.SecurityGroupRuleArgs => { + return { + type: 'egress', + protocol: 'tcp', + cidrBlocks: ['0.0.0.0/0'], + fromPort: 81, + toPort: 81, + securityGroupId: id, + } + } + break; + case 11: + // No SecurityGroupRule of type 'ingress' for a SecurityGroup with inline ingress rules. + sgArgs = { + ingress: [{ + toPort: 80, + fromPort: 80, + cidrBlocks: ['0.0.0.0/0'], + protocol: 'tcp', + }], + }; + + sgRuleArgs = (id: pulumi.Input): aws.ec2.SecurityGroupRuleArgs => { + return { + type: 'ingress', + protocol: 'tcp', + cidrBlocks: ['0.0.0.0/0'], + fromPort: 81, + toPort: 81, + securityGroupId: id, + } + } + break; + case 12: + // No SecurityGroupIngressRule for a SecurityGroup with inline ingress rules. + sgArgs = { + ingress: [{ + toPort: 80, + fromPort: 80, + cidrBlocks: ['0.0.0.0/0'], + protocol: 'tcp', + }], + }; + + sgIngressRuleArgs = (id: pulumi.Input): aws.vpc.SecurityGroupIngressRuleArgs => { + return { + ipProtocol: 'tcp', + cidrIpv4: '0.0.0.0/0', + fromPort: 81, + toPort: 81, + securityGroupId: id, + } + } + break; + case 13: + // No SecurityGroupEgressRule for a SecurityGroup with inline egress rules. + sgArgs = { + egress: [{ + toPort: 80, + fromPort: 80, + cidrBlocks: ['0.0.0.0/0'], + protocol: 'tcp', + }], + }; + + sgEgressRuleArgs = (id: pulumi.Input): aws.vpc.SecurityGroupEgressRuleArgs => { + return { + ipProtocol: 'tcp', + cidrIpv4: '0.0.0.0/0', + fromPort: 81, + toPort: 81, + securityGroupId: id, + } + } + break; default: throw new Error(`Unexpected test scenario ${testScenario}`); } @@ -178,3 +280,13 @@ export const ec2Instance = new aws.ec2.Instance("test-ec2-instance", ec2Instance export const elb = new aws.elb.LoadBalancer("test-elb", elbArgs); export const elbV2 = new aws.lb.LoadBalancer("test-elb-v2", elbV2Args); export const alb = new aws.alb.LoadBalancer("test-alb", albArgs); +export const sg = new aws.ec2.SecurityGroup('test-sg', sgArgs); +if (sgRuleArgs !== undefined) { + new aws.ec2.SecurityGroupRule('test-sg-rule', sgRuleArgs(sg.id)); +} +if (sgEgressRuleArgs) { + new aws.vpc.SecurityGroupEgressRule('test-sg-egress-rule', sgEgressRuleArgs(sg.id)); +} +if (sgIngressRuleArgs) { + new aws.vpc.SecurityGroupIngressRule('test-sg-ingress-rule', sgIngressRuleArgs(sg.id)); +} diff --git a/integration-tests/compute/package.json b/integration-tests/compute/package.json index 3599419..a3a4fcb 100644 --- a/integration-tests/compute/package.json +++ b/integration-tests/compute/package.json @@ -3,10 +3,10 @@ "main": "index.ts", "dependencies": { "@pulumi/pulumi": "^3.0.0", - "@pulumi/aws": "^4.0.0", + "@pulumi/aws": "^6.0.0", "@pulumi/awsx": "^0.30.0" }, "resolutions": { - "@pulumi/aws": "^4.0.0" + "@pulumi/aws": "^6.0.0" } } diff --git a/integration-tests/integration_test.go b/integration-tests/integration_test.go index 66b2f79..8f52d65 100644 --- a/integration-tests/integration_test.go +++ b/integration-tests/integration_test.go @@ -250,5 +250,37 @@ func TestComputeEC2(t *testing.T) { "The EC2 instance root block device must be encrypted.", }, }, + // Test scenario 10 - A egress type SecurityGroupRule attached to a SecurityGroup with inline rules + { + WantErrors: []string{ + "mandatory", + "security-group-no-rule-management-conflicts", + "SecurityGroupRule test-sg-rule defines rules for SecurityGroup test-sg which has inline 'egress' rules", + }, + }, + // Test scenario 11 - A ingress type SecurityGroupRule attached to a SecurityGroup with inline rules + { + WantErrors: []string{ + "mandatory", + "security-group-no-rule-management-conflicts", + "SecurityGroupRule test-sg-rule defines rules for SecurityGroup test-sg which has inline 'ingress' rules", + }, + }, + // Test scenario 12 - A SecurityGroupIngressRule attached to a SecurityGroup with inline rules + { + WantErrors: []string{ + "mandatory", + "security-group-no-rule-management-conflicts", + "SecurityGroupIngressRule test-sg-ingress-rule defines rules for SecurityGroup test-sg which has inline 'ingress' rules", + }, + }, + // Test scenario 13 - A SecurityGroupEgressRule attached to a SecurityGroup with inline rules + { + WantErrors: []string{ + "mandatory", + "security-group-no-rule-management-conflicts", + "SecurityGroupEgressRule test-sg-egress-rule defines rules for SecurityGroup test-sg which has inline 'egress' rules", + }, + }, }) } diff --git a/src/compute.ts b/src/compute.ts index b9fbe3d..d2487d5 100644 --- a/src/compute.ts +++ b/src/compute.ts @@ -14,7 +14,14 @@ import * as aws from "@pulumi/aws"; -import { EnforcementLevel, ResourceValidationPolicy, validateResourceOfType } from "@pulumi/policy"; +import { + EnforcementLevel, + ReportViolation, + ResourceValidationPolicy, + StackValidationArgs, + StackValidationPolicy, + validateResourceOfType, +} from "@pulumi/policy"; import { registerPolicy } from "./awsGuard"; import { PolicyArgs } from "./policyArgs"; @@ -27,6 +34,7 @@ declare module "./awsGuard" { ec2VolumeInUse?: EnforcementLevel | (Ec2VolumeInUseArgs & PolicyArgs); elbAccessLoggingEnabled?: EnforcementLevel; encryptedVolumes?: EnforcementLevel | (EncryptedVolumesArgs & PolicyArgs); + securityGroupNoRuleManagementConflicts?: EnforcementLevel; } } @@ -168,3 +176,52 @@ export const encryptedVolumes: ResourceValidationPolicy = { }), }; registerPolicy("encryptedVolumes", encryptedVolumes); + +/** @internal */ +export const securityGroupNoRuleManagementConflicts: StackValidationPolicy = { + name: "security-group-no-rule-management-conflicts", + description: + "Checks that ec2.SecurityGroup resources do not conflict with vpc.SecurityGroupEgressRule, vpc.SecurityGroupIngressRule, or ec2.SecurityGroupRule.\n"+ + "See https://www.pulumi.com/registry/packages/aws/api-docs/ec2/securitygroup/ for more details", + validateStack: (args: StackValidationArgs, reportViolation: ReportViolation) => { + args.resources.forEach((resource) => { + let currentType: string; + let type: "ingress" | "egress"; + switch (resource.type) { + case "aws:vpc/securityGroupEgressRule:SecurityGroupEgressRule": + currentType = "SecurityGroupEgressRule"; + type = "egress"; + break; + case "aws:vpc/securityGroupIngressRule:SecurityGroupIngressRule": + currentType = "SecurityGroupIngressRule"; + type = "ingress"; + break; + case "aws:ec2/securityGroupRule:SecurityGroupRule": + currentType = "SecurityGroupRule"; + type = resource.props["type"]; + break; + default: + return; + } + + if (resource.propertyDependencies["securityGroupId"]) { + resource.propertyDependencies["securityGroupId"].forEach((dep) => { + if (dep.type !== "aws:ec2/securityGroup:SecurityGroup" || !dep.props) { + return; + } + if (type === "ingress" && dep.props["ingress"] && dep.props["ingress"].length > 0) { + reportViolation( + `${currentType} ${resource.name} defines rules for SecurityGroup ${dep.name} which has inline 'ingress' rules`, + ); + } + if (type === "egress" && dep.props["egress"] && dep.props["egress"].length > 0) { + reportViolation( + `${currentType} ${resource.name} defines rules for SecurityGroup ${dep.name} which has inline 'egress' rules`, + ); + } + }); + } + }); + }, +}; +registerPolicy("securityGroupNoRuleManagementConflicts", securityGroupNoRuleManagementConflicts); From 5fdbce8289ac7f8f24746fcf50c4532fa034b0d2 Mon Sep 17 00:00:00 2001 From: corymhall <43035978+corymhall@users.noreply.github.com> Date: Fri, 26 Apr 2024 14:07:05 -0400 Subject: [PATCH 2/4] adding urn to reportViolation --- src/compute.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/compute.ts b/src/compute.ts index d2487d5..2bd76e1 100644 --- a/src/compute.ts +++ b/src/compute.ts @@ -211,12 +211,12 @@ export const securityGroupNoRuleManagementConflicts: StackValidationPolicy = { } if (type === "ingress" && dep.props["ingress"] && dep.props["ingress"].length > 0) { reportViolation( - `${currentType} ${resource.name} defines rules for SecurityGroup ${dep.name} which has inline 'ingress' rules`, + `${currentType} ${resource.name} defines rules for SecurityGroup ${dep.name} which has inline 'ingress' rules`, resource.urn, ); } if (type === "egress" && dep.props["egress"] && dep.props["egress"].length > 0) { reportViolation( - `${currentType} ${resource.name} defines rules for SecurityGroup ${dep.name} which has inline 'egress' rules`, + `${currentType} ${resource.name} defines rules for SecurityGroup ${dep.name} which has inline 'egress' rules`, resource.urn, ); } }); From 37bc4c997d09cf7e6170741a585f71caa402a215 Mon Sep 17 00:00:00 2001 From: corymhall <43035978+corymhall@users.noreply.github.com> Date: Fri, 26 Apr 2024 14:12:05 -0400 Subject: [PATCH 3/4] update changelog --- CHANGELOG.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 131c00a..b5b7c83 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,8 +1,8 @@ -## HEAD (Unreleased) -- Added in checks for `alb` and `lb` to the compute and network policies +## 0.5.0 ---- +- Added in checks for `alb` and `lb` to the compute and network policies +- add new securityGroupNoRuleManagementConflicts rule [#108](https://github.com/pulumi/pulumi-policy-aws/pull/108) ## 0.4.0 (2022-09-22) From 47d7913fcb52f5edf1f05ca96719894bdae799b7 Mon Sep 17 00:00:00 2001 From: corymhall <43035978+corymhall@users.noreply.github.com> Date: Fri, 26 Apr 2024 14:17:49 -0400 Subject: [PATCH 4/4] hopefully fix workflow --- .github/workflows/master.yml | 1 + .github/workflows/release.yml | 1 + .github/workflows/run-acceptance-tests.yml | 1 + 3 files changed, 3 insertions(+) diff --git a/.github/workflows/master.yml b/.github/workflows/master.yml index 9b0ef4f..99bee55 100644 --- a/.github/workflows/master.yml +++ b/.github/workflows/master.yml @@ -69,6 +69,7 @@ jobs: with: path: ci-scripts repository: pulumi/scripts + - run: echo "ci-scripts" >> .git/info/exclude # actions/checkout#197 - name: Configure AWS Credentials uses: aws-actions/configure-aws-credentials@v1 with: diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index aa06899..1ce782e 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -69,6 +69,7 @@ jobs: with: path: ci-scripts repository: pulumi/scripts + - run: echo "ci-scripts" >> .git/info/exclude # actions/checkout#197 - name: Configure AWS Credentials uses: aws-actions/configure-aws-credentials@v1 with: diff --git a/.github/workflows/run-acceptance-tests.yml b/.github/workflows/run-acceptance-tests.yml index 5b1e2db..4f7703e 100644 --- a/.github/workflows/run-acceptance-tests.yml +++ b/.github/workflows/run-acceptance-tests.yml @@ -88,6 +88,7 @@ jobs: with: path: ci-scripts repository: pulumi/scripts + - run: echo "ci-scripts" >> .git/info/exclude # actions/checkout#197 - name: Configure AWS Credentials uses: aws-actions/configure-aws-credentials@v1 with: