fix(core): suppress false-positive E9002 validation warning for ICMP security group rules - #38395
fix(core): suppress false-positive E9002 validation warning for ICMP security group rules#38395fifthist wants to merge 1 commit into
Conversation
…security group rules
The `@aws/cloudformation-validate` engine's `E9002` rule ("FromPort is
greater than ToPort") does not account for ICMP/ICMPv6 security group
rules, where `FromPort`/`ToPort` encode the ICMP type/code (with `-1`
meaning "all") rather than an ordered port range. As a result, common
rules such as `Port.icmpPing()` (which renders `FromPort: 8, ToPort: -1`)
emit a spurious warning on `cdk synth`.
Rather than suppressing `E9002` wholesale (which would also hide genuine
TCP/UDP range misconfigurations), correlate each `E9002` finding back to
the specific offending security group rule and drop it only when that
rule uses an ICMP protocol. Real range errors — including on the same
resource — continue to be reported.
Closes aws#38389.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
The pull request linter fails with the following errors:
❌ Fixes must contain a change to an integration test file and the resulting snapshot.
If you believe this pull request should receive an exemption, please comment and provide a justification. A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed, add Clarification Request to a comment.
✅ A exemption request has been requested. Please wait for a maintainer's review.
|
Exemption Request This change does not lend itself to an integration test, and I'd like to request an exemption on that basis. The fix only filters a synth-time validation diagnostic — it makes the The behavior is instead covered by unit tests in
There is also no existing integration-test surface for the core validation plugin ( Happy to add anything else that would help review. |
Closes #38389.
Reason for this change
Since the
@aws/cloudformation-validateengine was wired into synth-time validation,cdk synthemits a spurious warning for perfectly valid ICMP security group rules:The engine's
E9002rule is a generic "FromPortmust be<= ToPort" port-range check. It does not account for the fact that for ICMP/ICMPv6 rules,FromPortandToPortare not a port range — they encode the ICMP type and code, where-1means "all". So a completely ordinary rule like:renders the valid CloudFormation below and yet trips
E9002:There is no consumer-side workaround short of acknowledging/suppressing the rule globally, which is undesirable (see below).
Description of changes
The finding is a false positive that originates in the upstream engine's rule, so the fix lives where CDK already curates engine output: the
CloudFormationValidatePlugin.Instead of suppressing
E9002globally, the plugin now correlates eachE9002finding back to the specific security group rule that produced it and drops the finding only when that rule uses an ICMP protocol. The engine emits oneE9002diagnostic per offending rule, and its message names the exact ports ("FromPort 8 is greater than ToPort -1"), so we match those ports against the rule declared in the template and check itsIpProtocol. ICMP is recognized whether given by name (icmp/icmpv6) or IANA number (1/58). The check covers inlineSecurityGroupIngress/SecurityGroupEgressarrays as well as the standaloneAWS::EC2::SecurityGroupIngress/Egressresources.The template is parsed lazily (once per stack, only when an
E9002finding is actually present), so there is no cost for the common case.A code comment links back to this issue so the workaround can be removed once the engine understands ICMP rules.
Why not simply add
E9002to theIGNORE_RULESlist?That was the originally suggested fix, but adding
E9002toIGNORE_RULES(or excluding it per-service via the engine'sexclude.services) suppresses the rule for every resource, which would silently hide genuine misconfigurations thatE9002is meant to catch.E9002is a real, useful rule for TCP/UDP — it only misfires on ICMP.Concretely, all of the following are legitimate
E9002errors that a blanket ignore would swallow:The targeted approach keeps those errors visible. This is verified explicitly by the tests, including the case where a single security group mixes an ICMP rule and a bad TCP range — only the ICMP finding is dropped:
E9002beforeE9002after this changeicmp, FromPort 8, ToPort -1 (Port.icmpPing())icmpv6, FromPort 128, ToPort -11(numeric ICMP), FromPort 8, ToPort -1tcp, FromPort 443, ToPort 80 (real typo)A per-service exclude (
AWS::EC2/AWS::EC2::SecurityGroup) has the same problem: security groups carry both ICMP and TCP/UDP rules, so scoping the suppression to the service would still hide the TCP typo above. Only correlating down to the individual rule's protocol preserves the rule's value while removing the false positive.Describe any new or updated permissions being added
None.
Description of how you validated changes
Added unit tests in
cloudformation-validate-plugin.test.ts:E9002for ICMP rules — parametrized overicmp,icmpv6, and the numeric protocol1.E9002is still reported for a genuine TCP range error (FromPort: 443, ToPort: 80), guarding against over-suppression.E9002finding (the TCP one), proving the suppression is per-rule, not per-resource.I also reproduced the original false positive and validated the suppression logic directly against the pinned engine version (
@aws/cloudformation-validate@1.5.1-beta) for every case in the table above.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license