AWS Security Group Cidr Ingress bugfix

[anovis]

There is currently a bug in the ingress filter on security group which fails to match any resource when specifying a list of cidr blocks with value_type cidr. This is because the parse_cidr only handles a single value. This pr handles support for parsing a list of cidr.

  - name: vpc
    resource: security-group
    filters:
      - type: ingress
        Cidr:
          op: in
          value_type: cidr
          value: 
            - 10.10.10.10/32
            - 0.0.0.0/0

[kapilt]

'x' in y for a value_type: cidr is very different then 'x' in ['y'] the later looks a lot more like equality containment without a value_type. value_type cidr is to perform cidr math, and its unclear that this change doesn't instead make that conditional and ambiguous in interpretation based on the value, instead of explicitly supported.

[anovis]

this ended up being a little more complicated than I initially thought. seems to be related to #4834 as well. added tests to verify that the filter works for both ni and in operators . I didn't see any good way to modify the Valuefilter to handle this case. happy to get your thoughts on this

[mithun]

FWIW, this should be both a bug and a feature request.

Bug

Using a value_type: cidr and value: [] (list) silently ignores the value: []. This can confuse a user to think that a policy is working, whereas in reality they are using a syntax combination that is not supported.

Feature

This impacts both type: ingress and type: egress.

Primary goal is to allow a check -- For each ingress/egress rule that has IpRanges, check if ipaddress.ip_network(IpRange) is/is-not a subnet_of ANY-of ipaddress.ip_network(values defined in policy)

Given the current behavior of value_type: cidr, and the fact that c7n.utils.parse_cidr currently returns either an IPv4Address or IPv4Network object -- it feels like we might need a value_type: cidr_list (or similar) that only deals with IPv4Network objects and allows for the any(x.subnet_of(y)) behavior.

[kapilt]

i'm feeling a bit sensitive to the additional complexity here vs supporting something more general case which would be ingress filter chaining (ie multiple ingress filters operating on the same set of matched permissions).

[mithun]

ingress filter chaining

That would help with this use case as well. What would a chained-ingress filter look like? Also, Doesn't the match-operator already implicitly support matching multiple-conditions?

IMO, updating the ValueFilter to allow matching(i.e., cidr-math) against a list of CIDRs will probably be more useful since the capability will be available across more resources and use-cases.

[anovis]

the chaining is difficult since the annotations dont contain anything about the original filter. only the cidr that matched the filter, but nothing about the op.

one way we could do it would be to add a new operator not-in-by-item or another name that handles the ni per item. it would look like this all([x not in y_elem for y_elem in y]) and would allow for multiple cidr blocks being checked in the same filter. this would help with the chaining since it fails on the ni operator

[tjstansell]

For simplicity, I'd prefer if we had something that supported value_from ... for instance:

  - name: vpc
    resource: security-group
    filters:
      - type: ingress
        Cidr:
          op: in
          value_type: cidr
          value_from: s3://some-bucket/invalid-cidrs.txt

But also isn't this a more general problem with any filter that specifies a value_type and a list of values as process_value_type() seems to be assuming a single sentinel value. Wouldn't it make sense that if both op: (in|not-in) was specified and value_type is specified that it handles that by doing an implied "or" for each of the values?

[anovis]

@tjstansell I was originally trying to find a way for process_value_type() to handle lists for value types, but it is tough since a few of them dont work with lists. for example size or swap. and the operator is seperate than the value type so we would have to make changes in the match function as well. i figured by adding new op's this way would have minimal impacts to the current value filters.

[tjstansell]

Now that I've slept some and I'm looking at this fresh, in-each reads like the resource value should match each of the supplied values... which implies an and. And if we we're really only supporting cidrs as lists anyway to handle things this way, perhaps we're going about this the wrong way...

For cidrs specifically, it might make more sense to special case the list form and continue just using in and not-in. There's currently the special case of passing in a single IP Address as a value and then it reverses the comparison. That could still happen. But if you pass in a list of cidrs, it could always treat them all as cidrs and combine them into a single IPv4Network object, or maybe we have a new IPv4NetworkSet object that could be used as a set of networks .... then add __contains__ support for comparing if an IPv4Network or IPv4Address is contained within that set...

Personally, I'd like our cidr handling to be able to know how to combine cidrs as well ... so we could, for instance, find security groups that have multiple ingress rules that, when combined, might equal 0.0.0.0/0 ... maybe it'd be a different filter than what we have now, but ideally we should have a mechanism to combine and simplify cidr networks. This could be the beginning of that.

[anovis]

thanks @tjstansell ! your suggestions make a lot of sense. I added a ipv4networklist class that handles the cidr list comparison now for the in,ni operators. i can look into combining cidr blocks, but i feel that may be out of scope for this issue, but happy to add that in the future.

[tjstansell]

Thanks for these changes! I like this direction. And yeah, we can look at how to combine cidr blocks later...

[anovis]

thanks @tjstansell . this is much cleaner than what i started with. @kapilt happy to get your thoughts on this pr.

[linux-foundation-easycla[bot]]

• :white_check_mark: login: ajkerrigan / name: AJ Kerrigan (5516deb275dfc17813bd926a48e6ffc4a8b225e0, a61b436cdd016d5e3b11a1a0ab6ada383265de21)
• :white_check_mark: login: cahn1 / name: Kayden Ahn (609d957d6d33cbdc543d6adc36d6e6193959cc01, b9d62a280a5d058084ed33aeea0b12be89b90ef7)
• :white_check_mark: login: anovis / name: Austen (e1752348428ba30d55d73d4af7e7f015e9cabb08)
• :white_check_mark: login: kapilt / name: Kapil Thangavelu (f66f4b775e17d7d7349536a47a6990db7ff79ccf)
• :x: The commit (cf95f9eb239812b80f66f02c67c39562963f94c9, 9d50f8b12ef75160c1f9ee5b5ec2492c6b9a4fb6, 5c020a6e378919ce6f3d5ba7e6d7315cab4e97d9, ea4a7a3e44c215c2e176ad779695683eecc2e6c9, e1a56bda80ebb5d7efee196f1775628d7935c055, 26d3b9b880e1642244e64abbdff07b37dacdac6b, 650d963377c6d205104271385720e4b1d5f270ff, c4a409d5f4d16b388e97088a110bae8939203c86, 9e7f869f0af263341121df06940968baeb5245cc, a81f564418d7fb70bbf9acff2f5ac848948975ab, 73f476c0731f5854967f60a69d1f2177c4e7da48, 43177d1b789805ad3d162493901c97747a82f9d0). This user is missing the User's ID, preventing the EasyCLA check. Consult GitHub Help to resolve.For further assistance with EasyCLA, please submit a support request ticket.

[ajkerrigan]

Trying to revive this PR following the work in #7129 and some discussion in our last community meeting. The idea is to take the core of the logic from here, lay some of @cahn1's tests and docs/examples/tests on top, and merge in latest since it's been a while.

And if the end result passes a sniff test, bring it all home and close this out.

[tjstansell]

Most of my current thoughts are captured in https://github.com/cloud-custodian/cloud-custodian/pull/7129#issuecomment-1079720353 but I'll also summarize here. Since c7n currently does not handle lists of cidrs at all and we're adding support for this, I would argue now is the time to make it behave how we'd like to see it going forward, rather than implement something now then try to change behavior later. So related to that, I think that we should always treat a list of cidrs as the shortest aggregate of the cidrs. We should be combining and minimizing that representation so we can easily do comparisons between sets of cidrs. Looking through this PR again, it's trying to maintain whether something is an IPNetwork or an IPAddress, comparing pieces individually. We should treat everything as an IPNetwork and we should collapse them, so if you pass in a list of IPAddresses that make up a /30, it treats them as a /30.

[PratMis]

I think @anovis already resigned the cla. Looks like the pr is just failing on some old commits that may be associated with his capital one email? FYI @kapilt @castrojo

[castrojo]

/easycla