Filter condition not removing offending security group rule when another rule already satisfies the condition

[rohanpower]

Describe the bug

I have the following filter configured that is split into two parts.

1. The first part should remove any rules that are not using port 443 and open to 0.0.0.0/0 (this is working and not the issue).
2. The second part should allow rules when open to 0.0.0.0/0 on port 80 and tagged with 'application:apache', otherwise remove.

Rule:

    filters:
      - or:
          - type: ingress
            OnlyPorts: [ 443 ]
            Cidr:
              value: "0.0.0.0/0"
          - type: ingress
            OnlyPorts: [ 443 ]
            CidrV6:
              value: "::/0"
      - not:
          - type: value
            key: "tag:application"
            value: "apache"
            value_type: normalize
            op: equal
          - type: ingress
            Ports: [ 80 ]
            Cidr:
              value: "0.0.0.0/0"

What did you expect to happen?

What should happen
When the SG is tagged correctly, I expect the rule with port 80 to be allowed and any other rule to be removed.

What is happening:
When adding multiple rules to a correctly tagged SG, and one of those rules is port 80 & 0.0.0.0/0 both rules are being allowed, when in actual fact the rule that allows a port that is not port 80 should be removed.

For example:
I add the following two rules:

Ingress - Port 80 - 0.0.0.0/0
Ingress - Port 9999 - 0.0.0.0/0

Both rules are allowed to stay, however I expected the Port 9999 rule to be removed.

Cloud Provider

Amazon Web Services (AWS)

Cloud Custodian version and dependency information

v0.9.8

Policy

- name: security-group-risk-remediate
    resource: security-group
    description: |
      Remove any rule from a security group that allows 0.0.0.0/0 or ::/0 (IPv6) ingress
    mode:
        type: cloudtrail
        role: "arn:aws:iam::{account_id}:role/role"
        events:
          - source: ec2.amazonaws.com
            event: AuthorizeSecurityGroupIngress
            ids: "requestParameters.groupId"
          - source: ec2.amazonaws.com
            event: AuthorizeSecurityGroupEgress
            ids: "requestParameters.groupId"
          - source: ec2.amazonaws.com
            event: RevokeSecurityGroupEgress
            ids: "requestParameters.groupId"
          - source: ec2.amazonaws.com
            event: RevokeSecurityGroupIngress
            ids: "requestParameters.groupId"
    filters:
      - or:
          - type: ingress
            OnlyPorts: [ 443 ]
            Cidr:
              value: "0.0.0.0/0"
          - type: ingress
            OnlyPorts: [ 443 ]
            CidrV6:
              value: "::/0"
      - not:
          - type: value
            key: "tag:application"
            value: "apache"
            value_type: normalize
            op: equal
          - type: ingress
            Ports: [ 80 ]
            Cidr:
              value: "0.0.0.0/0"
    actions:
        - type: remove-permissions
          ingress: matched

Relevant log/traceback output

"requestParameters": {
            "groupId": "sg-xxxx",
            "ipPermissions": {
                "items": [
                    {
                        "ipProtocol": "tcp",
                        "fromPort": 80,
                        "toPort": 80,
                        "groups": {},
                        "ipRanges": {
                            "items": [
                                {
                                    "cidrIp": "0.0.0.0/0"
                                }
                            ]
                        },
                        "ipv6Ranges": {},
                        "prefixListIds": {}
                    },
                    {
                        "ipProtocol": "tcp",
                        "fromPort": 9999,
                        "toPort": 9999,
                        "groups": {},
                        "ipRanges": {
                            "items": [
                                {
                                    "cidrIp": "0.0.0.0/0"
                                }
                            ]
                        },
                        "ipv6Ranges": {},
                        "prefixListIds": {}
                    }
                ]
            }
        }

---

Filter #1 applied 1->1 filter: 
{
    "or": [
        {
            "type": "ingress",
            "OnlyPorts": [
                443
            ],
            "Cidr": {
                "value": "0.0.0.0/0",
                "key": "CidrIp"
            }
        },
        {
            "type": "ingress",
            "OnlyPorts": [
                443
            ],
            "CidrV6": {
                "value": "::/0"
            }
        }
    ]
}

---

Filter #2 applied 1->0 filter: 
{
    "not": [
        {
            "type": "value",
            "key": "tag:application",
            "value": "apache",
            "value_type": "normalize",
            "op": "equal"
        },
        {
            "type": "ingress",
            "Ports": [
                80
            ],
            "Cidr": {
                "value": "0.0.0.0/0",
                "key": "CidrIp"
            }
        }
    ]
}

Extra information or context

No response

[ajkerrigan]

Thanks for this issue report, the level of detail really helps follow what's going on in your policy!

That not filter is saying "match a security group unless it has an application tag set to apache and at least 1 inbound rule allowing port 80 from everywhere". So when you add that inbound rule for port 80, it doesn't matter which other rules you add. The security group as a whole won't match your filter anymore.

If I'm reading your intent properly, one option might be building on the OnlyPorts filters you've already written, and including a variation for your specially tagged security groups. Something like:

- name: security-group-risk-remediate
  resource: security-group
  description: |
    Remove any rule from a security group that allows 0.0.0.0/0 or ::/0 (IPv6) ingress
  filters:
    - or:
        - type: ingress
          IpProtocol: tcp
          OnlyPorts: [443]
          Cidr:
            value: "0.0.0.0/0"
        - type: ingress
          IpProtocol: tcp
          OnlyPorts: [443]
          CidrV6:
            value: "::/0"
        - and:
            - type: value
              key: "tag:application"
              value: "apache"
              value_type: normalize
              op: equal
            - type: ingress
              IpProtocol: tcp
              OnlyPorts: [443, 80]
              Cidr:
                value: "0.0.0.0/0"

Where the intent is "Allow 443 for most groups, but 443 & 80 for the apache case".

[rohanpower]

Thanks for the explanation. I understood the filter to match on each individual SG rule, but instead it's actually matching the whole SG (all rules) instead. Is there a way for each SG rule to be evaluated separately?

I did try that policy above however, but it didn't seem to work unfortunately. See trace below.

Filter #1 applied 1->1 filter: 
{
    "or": [
        {
            "type": "ingress",
            "OnlyPorts": [
                443
            ],
            "Cidr": {
                "value": "0.0.0.0/0",
                "key": "CidrIp"
            }
        },
        {
            "type": "ingress",
            "OnlyPorts": [
                443
            ],
            "CidrV6": {
                "value": "::/0"
            }
        },
        {
            "and": [
                {
                    "type": "value",
                    "key": "tag:application",
                    "value": "apache",
                    "value_type": "normalize",
                    "op": "equal"
                },
                {
                    "type": "ingress",
                    "IpProtocol": "tcp",
                    "OnlyPorts": [
                        443,
                        80
                    ],
                    "Cidr": {
                        "value": "0.0.0.0/0",
                        "key": "CidrIp"
                    }
                }
            ]
        }
    ]
}

[ajkerrigan]

The filters match at the resource level (whole SG in this case), but they also collect individual matching rules in a MatchedIpPermissions annotation. You can see that in the resources.json output, and it's how custodian knows which specific rules to remove when you use this action:

- type: remove-permissions
  ingress: matched

Testing that policy locally I see this in my output:

{
	"GroupName": "my-test-sg",
	"IpPermissions": [
		{
			"FromPort": 80,
			"IpProtocol": "tcp",
			"IpRanges": [
				{
					"CidrIp": "0.0.0.0/0"
				}
			],
			"Ipv6Ranges": [],
			"PrefixListIds": [],
			"ToPort": 80,
			"UserIdGroupPairs": []
		},
		{
			"FromPort": 9999,
			"IpProtocol": "tcp",
			"IpRanges": [
				{
					"CidrIp": "0.0.0.0/0"
				}
			],
			"Ipv6Ranges": [],
			"PrefixListIds": [],
			"ToPort": 9999,
			"UserIdGroupPairs": []
		},
		{
			"FromPort": 443,
			"IpProtocol": "tcp",
			"IpRanges": [
				{
					"CidrIp": "0.0.0.0/0"
				}
			],
			"Ipv6Ranges": [],
			"PrefixListIds": [],
			"ToPort": 443,
			"UserIdGroupPairs": []
		}
	],
	"OwnerId": "123456789012",
	"GroupId": "sg-111222333444",
	"IpPermissionsEgress": [],
	"Tags": [
		{
			"Key": "application",
			"Value": "apache"
		}
	],
	"VpcId": "vpc-111222333444",
	"MatchedIpPermissions": [
		{
			"FromPort": 9999,
			"IpProtocol": "tcp",
			"ToPort": 9999,
			"IpRanges": [
				{
					"CidrIp": "0.0.0.0/0"
				}
			],
			"Ipv6Ranges": [],
			"PrefixListIds": [],
			"UserIdGroupPairs": []
		}
	],
	"c7n:MatchedFilters": [
		"tag:application"
	]
}

So the whole group matches this collection of filters, but only the 9999 rule shows up in MatchedIpPermissions. I hope this helps some, but it's good to talk through because these sorts of complex filters can be pretty tough to follow.