diff --git a/packages/aws-cdk-lib/core/lib/validation/cloudformation-validate-plugin.ts b/packages/aws-cdk-lib/core/lib/validation/cloudformation-validate-plugin.ts index 9f7f7b364a713..a32cd82d6b413 100644 --- a/packages/aws-cdk-lib/core/lib/validation/cloudformation-validate-plugin.ts +++ b/packages/aws-cdk-lib/core/lib/validation/cloudformation-validate-plugin.ts @@ -1,5 +1,6 @@ +import * as fs from 'fs'; import { RegoEngine, TemplateFile, version } from '@aws/cloudformation-validate'; -import type { Engine, EngineConfig, RuleInfo, Severity } from '@aws/cloudformation-validate'; +import type { DetailedDiagnostic, Engine, EngineConfig, RuleInfo, Severity } from '@aws/cloudformation-validate'; import type { PolicyValidationPluginReport, PolicyViolatingResource } from './report'; import type { IPolicyValidationPlugin, IPolicyValidationContext } from './validation'; @@ -124,7 +125,17 @@ export class CloudFormationValidatePlugin implements IPolicyValidationPlugin { severityLevel: 'WARN', }); + // Parsed lazily (and only once) because it is only needed to filter out + // specific engine false positives, which most templates don't trigger. + let parsedTemplate: any; + const template = () => parsedTemplate ??= JSON.parse(fs.readFileSync(templatePath, 'utf-8')); + for (const diagnostic of report.diagnostics) { + // The engine reports a false positive for ICMP security group rules; drop those findings. + if (diagnostic.ruleId === 'E9002' && isIcmpPortRangeFalsePositive(diagnostic, template())) { + continue; + } + const severity = mapSeverity(diagnostic.severity); const violatingResource: PolicyViolatingResource = { @@ -177,6 +188,70 @@ function mapSeverity(severity: Severity): string { } } +const SECURITY_GROUP_RULE_TYPES = new Set([ + 'AWS::EC2::SecurityGroup', + 'AWS::EC2::SecurityGroupIngress', + 'AWS::EC2::SecurityGroupEgress', +]); + +/** + * Whether an `E9002` ("FromPort is greater than ToPort") finding is a false positive on an ICMP rule. + * + * For ICMP/ICMPv6 security group rules the `FromPort`/`ToPort` fields encode the ICMP type and code + * (where `-1` means "all"), not an ordered port range, so e.g. `Port.icmpPing()` legitimately renders + * `FromPort: 8, ToPort: -1`. The engine's generic range check flags these as errors. We correlate the + * finding back to the offending rule (by the ports named in the message, since the property path only + * points at the rule array) and drop it only when that rule uses an ICMP protocol, leaving genuine + * TCP/UDP range errors — including on the same resource — reported. + * + * Remove once the engine understands ICMP rules: . + */ +function isIcmpPortRangeFalsePositive(diagnostic: DetailedDiagnostic, template: any): boolean { + if (!diagnostic.resourceType || !SECURITY_GROUP_RULE_TYPES.has(diagnostic.resourceType)) { + return false; + } + + const match = /FromPort (-?\d+) is greater than ToPort (-?\d+)/.exec(diagnostic.message ?? ''); + if (!match) { + return false; + } + const fromPort = Number(match[1]); + const toPort = Number(match[2]); + + const resource = template?.Resources?.[diagnostic.resourceId ?? '']; + if (!resource) { + return false; + } + + const props = resource.Properties ?? {}; + // A rule can live in a SecurityGroup's inline ingress/egress arrays, or be the standalone + // SecurityGroupIngress/Egress resource's own properties. + const rules = [ + ...(Array.isArray(props.SecurityGroupIngress) ? props.SecurityGroupIngress : []), + ...(Array.isArray(props.SecurityGroupEgress) ? props.SecurityGroupEgress : []), + props, + ]; + + return rules.some((rule) => rule != null + && isIcmpProtocol(rule.IpProtocol) + && Number(rule.FromPort) === fromPort + && Number(rule.ToPort) === toPort); +} + +/** + * Whether an `IpProtocol` value refers to ICMP or ICMPv6, given either as a name or an IANA number. + */ +function isIcmpProtocol(protocol: unknown): boolean { + if (typeof protocol === 'string') { + const normalized = protocol.toLowerCase(); + return normalized === 'icmp' || normalized === 'icmpv6' || normalized === '1' || normalized === '58'; + } + if (typeof protocol === 'number') { + return protocol === 1 || protocol === 58; + } + return false; +} + // Rules that the engine will report but we want to ignore because CDK creates // the violation and customers don't control it. const IGNORE_RULES = new Set([ diff --git a/packages/aws-cdk-lib/core/test/validation/cloudformation-validate-plugin.test.ts b/packages/aws-cdk-lib/core/test/validation/cloudformation-validate-plugin.test.ts index b1b0542ffd305..78484ad8d9d09 100644 --- a/packages/aws-cdk-lib/core/test/validation/cloudformation-validate-plugin.test.ts +++ b/packages/aws-cdk-lib/core/test/validation/cloudformation-validate-plugin.test.ts @@ -300,6 +300,82 @@ describe('CloudFormationValidatePlugin', () => { fs.rmSync(tmpDir, { recursive: true }); }); + + describe('ICMP E9002 false positive suppression', () => { + function validateTemplate(resources: Record) { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cdk-validate-')); + const templatePath = path.join(tmpDir, 'template.json'); + fs.writeFileSync(templatePath, JSON.stringify({ Resources: resources })); + try { + return new core.CloudFormationValidatePlugin().validate({ + templatePaths: [templatePath], + stackTemplates: [{ stackConstructPath: 'TestStack', templatePath }], + appConstruct: new Construct(undefined as any, ''), + accountId: undefined, + region: undefined, + }); + } finally { + fs.rmSync(tmpDir, { recursive: true }); + } + } + + // For ICMP rules, FromPort/ToPort are the ICMP type/code (with -1 meaning "all"), not an ordered + // port range, so the engine's generic "FromPort > ToPort" check (E9002) is a false positive. + test.each([ + ['icmp', 8], + ['icmpv6', 128], + ['1', 8], // IANA protocol number for ICMP + ])('does not report E9002 for an ICMP rule (IpProtocol: %s)', (ipProtocol, fromPort) => { + const report = validateTemplate({ + IcmpSg: { + Type: 'AWS::EC2::SecurityGroup', + Properties: { + GroupDescription: 'test', + SecurityGroupIngress: [ + { CidrIp: '0.0.0.0/0', FromPort: fromPort, IpProtocol: ipProtocol, ToPort: -1 }, + ], + }, + }, + }); + + expect(report.violations.find(v => v.ruleName === 'E9002')).toBeUndefined(); + }); + + test('still reports E9002 for a genuine TCP range error', () => { + const report = validateTemplate({ + BadTcpSg: { + Type: 'AWS::EC2::SecurityGroup', + Properties: { + GroupDescription: 'test', + SecurityGroupIngress: [ + { CidrIp: '0.0.0.0/0', FromPort: 443, IpProtocol: 'tcp', ToPort: 80 }, + ], + }, + }, + }); + + expect(report.violations.find(v => v.ruleName === 'E9002')).toBeDefined(); + }); + + test('suppresses only the ICMP finding when a resource mixes ICMP and a bad TCP range', () => { + const report = validateTemplate({ + MixedSg: { + Type: 'AWS::EC2::SecurityGroup', + Properties: { + GroupDescription: 'test', + SecurityGroupIngress: [ + { CidrIp: '0.0.0.0/0', FromPort: 8, IpProtocol: 'icmp', ToPort: -1 }, + { CidrIp: '0.0.0.0/0', FromPort: 443, IpProtocol: 'tcp', ToPort: 80 }, + ], + }, + }, + }); + + const e9002 = report.violations.filter(v => v.ruleName === 'E9002'); + expect(e9002).toHaveLength(1); + expect(e9002[0].description).toContain('FromPort 443 is greater than ToPort 80'); + }); + }); }); describe('CDK_VALIDATION environment variable', () => {