neutron accepts CIDR in security groups that are invalid in ovn

[Jake Yip]

We have found that there are some CIDR accepted by neutron, which does not work in networking ovn. Specifically, these are network CIDRs with the host bits set.

Steps to reproduce

- Create VM. Attach a floating IP to it

- Remove all security group. Attach a blank security group to it

- Add a security group rule and start ping

For example, if my IP is 10.10.10.175/26 (first 3 octets changed for privacy), the following security rules work

openstack security group rule create --protocol icmp --remote-ip 10.10.10.175/32 cidr
openstack security group rule create --protocol icmp --remote-ip 10.10.10.128/26 cidr

However, the following security group rule do not work

openstack security group rule create --protocol icmp --remote-ip 10.10.10.175/26 cidr

FWIW, in our testing, CIDRs like 10.10.10.175/26 work in other drivers, like linuxbridge and midonet.

[Jake Yip (waipengyip)]

this is a bit related to https://bugs.launchpad.net/horizon/+bug/1837339, in the sense that, if we update neutron to not accept CIDRs with host bits set, this will effectively fix the other bug.

[Hongbin Lu (hongbin.lu)]

It sounds fixing this bug requires a change of API behavior. I would let neutron driver team to triage this bug.

[Sam Morrison (sorrison)]

I guess the question is, is this something that should be fixed in OVN to make it work like other drivers or should it be fixed at the neutron level to not allow this on all drivers.

iptables allows these kind of CIDRs so my feeling is that it should be "fixed" at the ovn level?

[Slawek Kaplonski (slaweq)]

Personally I agree with Sam here. IMO we shouldn't change our API just because one of the drivers is not working properly now. Better would be IMHO to make this driver working in same way as the others.

[Jake Yip (waipengyip)]

I would like to point out that fixing it at the OVN level will not fix the security bug at https://bugs.launchpad.net/horizon/+bug/1837339/. specifically see comment #4.

FWIW I did some test on a linuxbridge port. A security group created with

 openstack security group rule create --protocol icmp --remote-ip 192.168.1.1/16 jake-invalid

shows up as `192.168.1.1/16` in neutron api, but in linuxbridge the iptables rule is:

 -A neutron-linuxbri-i4abb52c4-d -s 192.168.0.0/16 -p icmp -j RETURN

A secgroup rule of 192.168.1.1/0 is worse, it shows up as:

 -A neutron-linuxbri-i4abb52c4-d -p icmp -j RETURN

[Sam Morrison (sorrison)]

Ouch, yeah if that's the case I kinda feel this should be fixed at the API level

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.opendev.org/716806

[Carlos Goncalves (cgoncalves)]

In Jake's last comment #5, both CIDRs are valid, Neutron is rightfully accepting them and the firewall enforces the desired CIDR address range in iptables. I don't see a problem here. User error configuration is a separate problem.

[YAMAMOTO Takashi (yamamoto)]

at this point, i think it's better to keep api compatibility and fix ovn.

if there's some good way to warn users without breaking compatibiliy, it might be nice.
(like making the cli warn "this cidr means .., which might not what you want" without rejecting them)

[Slawek Kaplonski (slaweq)]

This bug was discussed on our last drivers meeting: http://eavesdrop.openstack.org/meetings/neutron_drivers/2020/neutron_drivers.2020-04-03-14.00.log.html#l-67

Summary of discussion:
* Brian proposed solution similar to what we did with ipv6-icmp protocol: https://review.opendev.org/#/c/453346/ - so to change our SG rules API in that way, that when user will set "wrong" cidr, Neutron will change it and store in DB correct network's cidr.
That is API change which isn't 100% backward compatible.

* Another proposal was to fix this bug on OVN driver's side to make it working in similar way as currently iptables and openvswitch drivers works. API will not be changed then at all. User should not make such mistakes and should be aware of what he is doing basically.

* Additionally to the second proposal, Nate proposed to add new (optional) API extension which would accept /0 cidrs only when it's given as 0.0.0.0/0 and return error in all other cases, like e.g. 192.168.1.1/0 mentioned in c#5 here.
Such solution isn't 100% backward compatible but should be discoverable by API extension and can be also disabled by default.

We will continue discussion about all those possible ways of fixing that issue on our next drivers meeting.

[Jake Yip (waipengyip)]

Thanks Slawek, all for commenting on this bug, and bringing it up in IRC. I have some questions:

* If we choose to fix it in OVN, do we have an idea what other drivers are impacted? So far I have only managed to test linuxbridge, midonet and OVN

* With above fix, could there be some drivers that, after coercing two SG rules to be identical, will throw an error when inserting duplicate rules?

* If we choose the new (optional) API extension fix, is it possible that it be an extension that accepts 'right'/'strict' CIDRs only? Disabled by default means no API behaviour change.

As an operator, my personal ordered preferences will be

1. Do not allow a user to make mistake
2. Warn the user if they make a mistake

Of course, I understand there might be technical considerations like not breaking API.

[Slawek Kaplonski (slaweq)]

Hi Jake Yip,

We were discussing this again on drivers meeting today. Here is what we decided to do:

1. As first step we would like to fix only issue on OVN driver's side that it will convert CIDR from rule to the form which is acceptable by OVN - and that can be also backported to the stable branches later,

2. As next step we may introduce API extension which will add new read-only field to security group rule resource in API. This new field will be something like "normalized_cidr" and it will be actually cidr which will be effectively used by driver. So e.g. if user will create rule with 1.2.3.4/24 we will store in this "normalized_cidr" 1.2.3.0/24 value and that will be used by drivers. But 1.2.3.4/24 will still be also visible in API for user.

Is that solution ok for You? Are You going to work on this or should we assign someone else for that?

[Jake Yip (waipengyip)]

Hi Slawek,

Sorry for the late reply; I was away.

The solution sounds great.

Will be happy for you to take over.

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: master
Review: https://review.opendev.org/736386

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: master
Review: https://review.opendev.org/743630

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/736386
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=17f2ba3afbbbb929155c2ac3fa396784badd981b
Submitter: Zuul
Branch: master

commit 17f2ba3afbbbb929155c2ac3fa396784badd981b
Author: Slawek Kaplonski <email address hidden>
Date: Wed Jun 17 23:28:16 2020 +0200

    [ovn] Use normalized remote prefix IPs in OVN driver

    OVN firewall driver can't silently normalize CIRDs given in
    the security group rule's "remote_ip_prefix".
    Because of that if user created rule with not normalized CIDR, it
    wasn't applied by the OVN driver.
    Now OVN driver will normalize such rules before applying them.

    The OVN driver will now also check if SG rules with same normalized
    and same direction, port range, protocol and ethertype already exists in
    the SG. If so, it will not add or remove rule in the OVN.
    Rule will be added or removed only if there is no other same rules in
    the SG.

    Change-Id: I0d9295545384844e81b0ffe3aa7483324f9a9ae5
    Related-Bug: #1869129

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: stable/ussuri
Review: https://review.opendev.org/743845

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/743630
Committed: https://git.openstack.org/cgit/openstack/neutron-lib/commit/?id=8ae0c1f21f7d8046bb5636571a622a263f5409fd
Submitter: Zuul
Branch: master

commit 8ae0c1f21f7d8046bb5636571a622a263f5409fd
Author: Slawek Kaplonski <email address hidden>
Date: Tue Jul 28 22:59:12 2020 +0200

    Add API definition for SG rule's normalized_cidr field

    Change-Id: I4bbd8a93342805b5f182c31651f94c09c24c9593
    Related-Bug: #1869129

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/743845
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=9e699c3743857009f7e33bf0a2d780a6cfbeafb2
Submitter: Zuul
Branch: stable/ussuri

commit 9e699c3743857009f7e33bf0a2d780a6cfbeafb2
Author: Slawek Kaplonski <email address hidden>
Date: Wed Jun 17 23:28:16 2020 +0200

    [ovn] Use normalized remote prefix IPs in OVN driver

    OVN firewall driver can't silently normalize CIRDs given in
    the security group rule's "remote_ip_prefix".
    Because of that if user created rule with not normalized CIDR, it
    wasn't applied by the OVN driver.
    Now OVN driver will normalize such rules before applying them.

    The OVN driver will now also check if SG rules with same normalized
    and same direction, port range, protocol and ethertype already exists in
    the SG. If so, it will not add or remove rule in the OVN.
    Rule will be added or removed only if there is no other same rules in
    the SG.

    Change-Id: I0d9295545384844e81b0ffe3aa7483324f9a9ae5
    Related-Bug: #1869129
    (cherry picked from commit 17f2ba3afbbbb929155c2ac3fa396784badd981b)

[OpenStack Infra (hudson-openstack)]

Change abandoned by Brian Haley (<email address hidden>) on branch: master
Review: https://review.opendev.org/716806
Reason: This was fixed in a different way, see bug for details

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: master
Review: https://review.opendev.org/c/openstack/neutron/+/801707

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/c/openstack/neutron/+/801707
Committed: https://opendev.org/openstack/neutron/commit/5a0a2b7847da067817640404f53e0807755e08d7
Submitter: "Zuul (22348)"
Branch: master

commit 5a0a2b7847da067817640404f53e0807755e08d7
Author: Jake Yip <email address hidden>
Date: Tue Jul 20 17:03:08 2021 +1000

    Allow ovn_db_sync to continue on duplicate normalised CIDR

    OVN now uses normalised CIDR when adding a security group rule[1]. It
    uses may_exist=True for adding ACL (secgroup rule), in case there are
    multiple CIDRs in neutron that normalises to the same.

    Do the same in ovn_db_sync, so that the sync don't fail hard on such
    duplicates.

    [1] https://review.opendev.org/c/openstack/neutron/+/736386/

    Change-Id: I9d9c21e460029e4a6a845520bfcc2889ad20429b
    Related-Bug: #1869129
    Closes-Bug: #1961112

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: stable/xena
Review: https://review.opendev.org/c/openstack/neutron/+/833559

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: stable/wallaby
Review: https://review.opendev.org/c/openstack/neutron/+/833560

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: stable/victoria
Review: https://review.opendev.org/c/openstack/neutron/+/833561

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: stable/ussuri
Review: https://review.opendev.org/c/openstack/neutron/+/833562

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/c/openstack/neutron/+/833559
Committed: https://opendev.org/openstack/neutron/commit/bf308a12a1f87ead18f7c8478ada93dc292bfcba
Submitter: "Zuul (22348)"
Branch: stable/xena

commit bf308a12a1f87ead18f7c8478ada93dc292bfcba
Author: Jake Yip <email address hidden>
Date: Tue Jul 20 17:03:08 2021 +1000

    Allow ovn_db_sync to continue on duplicate normalised CIDR

    OVN now uses normalised CIDR when adding a security group rule[1]. It
    uses may_exist=True for adding ACL (secgroup rule), in case there are
    multiple CIDRs in neutron that normalises to the same.

    Do the same in ovn_db_sync, so that the sync don't fail hard on such
    duplicates.

    [1] https://review.opendev.org/c/openstack/neutron/+/736386/

    Change-Id: I9d9c21e460029e4a6a845520bfcc2889ad20429b
    Related-Bug: #1869129
    Closes-Bug: #1961112
    (cherry picked from commit 5a0a2b7847da067817640404f53e0807755e08d7)

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/c/openstack/neutron/+/833560
Committed: https://opendev.org/openstack/neutron/commit/691a7ceeecb020e302e8d9a2ec3ceb67741144a8
Submitter: "Zuul (22348)"
Branch: stable/wallaby

commit 691a7ceeecb020e302e8d9a2ec3ceb67741144a8
Author: Jake Yip <email address hidden>
Date: Tue Jul 20 17:03:08 2021 +1000

    Allow ovn_db_sync to continue on duplicate normalised CIDR

    OVN now uses normalised CIDR when adding a security group rule[1]. It
    uses may_exist=True for adding ACL (secgroup rule), in case there are
    multiple CIDRs in neutron that normalises to the same.

    Do the same in ovn_db_sync, so that the sync don't fail hard on such
    duplicates.

    [1] https://review.opendev.org/c/openstack/neutron/+/736386/

    Change-Id: I9d9c21e460029e4a6a845520bfcc2889ad20429b
    Related-Bug: #1869129
    Closes-Bug: #1961112
    (cherry picked from commit 5a0a2b7847da067817640404f53e0807755e08d7)

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/c/openstack/neutron/+/833561
Committed: https://opendev.org/openstack/neutron/commit/10c8d590af7fc4e1355e38d455bbd4ecd4bb2a4c
Submitter: "Zuul (22348)"
Branch: stable/victoria

commit 10c8d590af7fc4e1355e38d455bbd4ecd4bb2a4c
Author: Jake Yip <email address hidden>
Date: Tue Jul 20 17:03:08 2021 +1000

    Allow ovn_db_sync to continue on duplicate normalised CIDR

    OVN now uses normalised CIDR when adding a security group rule[1]. It
    uses may_exist=True for adding ACL (secgroup rule), in case there are
    multiple CIDRs in neutron that normalises to the same.

    Do the same in ovn_db_sync, so that the sync don't fail hard on such
    duplicates.

    [1] https://review.opendev.org/c/openstack/neutron/+/736386/

    Change-Id: I9d9c21e460029e4a6a845520bfcc2889ad20429b
    Related-Bug: #1869129
    Closes-Bug: #1961112
    (cherry picked from commit 5a0a2b7847da067817640404f53e0807755e08d7)

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/c/openstack/neutron/+/833562
Committed: https://opendev.org/openstack/neutron/commit/f1fe5260c7f22415fef7e098a2b66e84f116c649
Submitter: "Zuul (22348)"
Branch: stable/ussuri

commit f1fe5260c7f22415fef7e098a2b66e84f116c649
Author: Jake Yip <email address hidden>
Date: Tue Jul 20 17:03:08 2021 +1000

    Allow ovn_db_sync to continue on duplicate normalised CIDR

    OVN now uses normalised CIDR when adding a security group rule[1]. It
    uses may_exist=True for adding ACL (secgroup rule), in case there are
    multiple CIDRs in neutron that normalises to the same.

    Do the same in ovn_db_sync, so that the sync don't fail hard on such
    duplicates.

    [1] https://review.opendev.org/c/openstack/neutron/+/736386/

    Change-Id: I9d9c21e460029e4a6a845520bfcc2889ad20429b
    Related-Bug: #1869129
    Closes-Bug: #1961112
    (cherry picked from commit 5a0a2b7847da067817640404f53e0807755e08d7)