14:00:09 <lajoskatona> #startmeeting neutron_drivers
14:00:09 <opendevmeet> Meeting started Fri Jul  8 14:00:09 2022 UTC and is due to finish in 60 minutes.  The chair is lajoskatona. Information about MeetBot at http://wiki.debian.org/MeetBot.
14:00:09 <opendevmeet> Useful Commands: #action #agreed #help #info #idea #link #topic #startvote.
14:00:09 <opendevmeet> The meeting name has been set to 'neutron_drivers'
14:00:11 <lajoskatona> o/
14:00:12 <mlavalle> o/
14:00:13 <slaweq> o/
14:00:28 <njohnston> o/
14:00:51 <ralonsoh> hi
14:01:24 <obondarev> hi. Just realised I missed PTL meeting, sorry
14:01:36 <haleyb> hi
14:01:40 <amotoki> hi
14:03:16 <lajoskatona> Ok, I think we can start
14:03:26 <lajoskatona> The first RFE for today:
14:03:30 <lajoskatona> [RFE] Adding the "rekey" parameter to the api for strongswan like "dpd_action" (#link https://bugs.launchpad.net/neutron/+bug/1979044)
14:04:28 <lajoskatona> it is for VPNasS, so I asked mnaser to come and tell his opinion if he as some time
14:05:23 <mlavalle> yeah, mnaser's is a very relevant opinion. amotoki's is too
14:05:25 <mnaser> I looked at it and I think it’s actually a pretty good idea however I’m not sure if I have Amelia time to be able to look into implementing this but I’d be happy to review somebody makes a chance address
14:05:58 <lajoskatona> mnaser: thanks
14:06:08 <obondarev> first dumb question that I have is: if it's device specific, can't device driver take care of setting needed 'rekey' value
14:06:14 <amotoki> what I am not sure is 'rekey' proposed here should be determined by neutron API users
14:06:21 <lajoskatona> That is my feeling also, that extending the API to have a new key: rekey is a good idea
14:06:31 <amotoki> or it depends on devices or vpn situations
14:08:02 <lajoskatona> obondarev, amotoki: good point
14:08:27 <obondarev> from description it seems it depends on the device, and that strongswan is not aware of underlying device
14:08:39 <obondarev> *strongswan driver
14:09:04 <obondarev> and that's where a user should step in and advice the driver
14:09:26 <opendevreview> Merged openstack/neutron-dynamic-routing master: Consume BGP service plugin queue in RPC workers  https://review.opendev.org/c/openstack/neutron-dynamic-routing/+/842383
14:09:41 <obondarev> so maybe the driver itself should have the ability to identify the device somehow?
14:10:22 <slaweq> obondarev++
14:10:44 <lajoskatona> transparently for the enduser?
14:10:58 <amotoki> AFAIK rekey itself is not specific to strongswan. other VPN products also support it.
14:11:07 <obondarev> yes, not sure about the technical possibility however
14:11:15 <amotoki> but I am still not sure it should be visible to VPNaaS users
14:12:33 <mnaser> Im looking at https://docs.aws.amazon.com/vpn/latest/s2svpn/VPNTunnels.html
14:13:02 <mlavalle> ahhh, looking at the alternative... always wise
14:13:02 <mnaser> It seems like they don’t provide option to disable it
14:15:38 <mnaser> GCP doesn’t either
14:15:48 <mnaser> So I feel like it would be wise to ask or understand why the user wants to do this
14:16:04 <amotoki> yeah, it is not a good idea to disable rekey in general
14:16:34 <mnaser> I guess maybe a poor implementation on the other side…
14:17:06 <obondarev> ++ and if there is a possibility to identify the needed value from within the strongswan driver
14:18:07 <amotoki> it looks better to ask the rfe submitter more detail on what situations requires rekey=no
14:19:06 <lajoskatona> ok, so this is not something that user should set on API, but more a technical question which the driver can determine and set, am I right?
14:19:36 <amotoki> I think so
14:19:39 <obondarev> that's my vision also
14:21:28 <amotoki> if it depends on vpn peers, we may need to pass rekey parameter to a driverr like strongswan but perhaps we first need to clarify when such situation happens
14:22:01 <lajoskatona> ok
14:23:29 <lajoskatona> mnaser: shall I ask you to write down to the RFE what information should be necessary as my VPN background is very weak :-(
14:24:08 <mnaser> sure, I can update the issue asking that
14:24:24 <lajoskatona> manser: thanks for the help
14:24:46 <lajoskatona> And thanks everybody for the discussion
14:25:03 <lajoskatona> If there is no more comments for this RFE we can move on to the next one
14:25:56 <lajoskatona> [RFE] Firewall Group Ordering on Port Association (#link https://bugs.launchpad.net/neutron/+bug/1979816)
14:26:37 <lajoskatona> This one comes from a security bug, which I made public, after long discussion under it, and agreeing that we don't need to have it private:
14:26:47 <lajoskatona> https://bugs.launchpad.net/neutron/+bug/1978497
14:27:47 <lajoskatona> if I understand well the behaviur here is already documented in the original spec for FWaaS
14:28:39 <opendevreview> Merged openstack/neutron master: [sqlalchemy-20] Remove retry decorator from update_floatingip_status  https://review.opendev.org/c/openstack/neutron/+/848701
14:28:44 <opendevreview> Merged openstack/neutron master: Remove workaround for LP#1767422  https://review.opendev.org/c/openstack/neutron/+/848312
14:28:59 <lajoskatona> somewhere here: https://specs.openstack.org/openstack/neutron-specs/specs/newton/fwaas-api-2.0.html#multiple-firewall-policies
14:30:21 <ralonsoh> that's the point: does it depends on the order of those groups or in evaluating the security for a specific traffic (that means, evaluating all rules)?
14:30:45 <ralonsoh> according to the documentation: This spec defines that packets will be allowed if any one of the firewall groups associated with that Neutron port allows the packet.
14:31:05 <ralonsoh> that means you need to evaluate all rules (or at least find one allowing this traffic)
14:32:09 <slaweq> but it also says that in future it should be changed and ordering should be added
14:32:21 <slaweq> I think that current RFE is exactly about that
14:32:48 <mlavalle> meaning that future has arrived
14:33:44 <ralonsoh> so perfect then (just one concern: if implemented, some environments will change their current behaviour)
14:33:56 <mlavalle> yeap
14:34:06 <mlavalle> it should go with a release note
14:34:16 <lajoskatona> +1
14:34:38 <slaweq> maybe ordering could be optional
14:34:55 <slaweq> if none of groups would have it, behaviour would stay as it is now
14:34:56 <slaweq> wdyt?
14:35:10 <atimmins> Agree on the release note, but fwiw, if an environment is working today, where ordering does not matter, then it would continue to work if ordering was added.
14:35:38 <mlavalle> slaweq: +++
14:35:50 <mlavalle> yeah make it optional
14:35:58 <obondarev> I'm not sure that currently this behaviour is predictable, according to the rfe it's not
14:35:59 <amotoki> as of now, we can define the order of rules inside a firewall policy and a point is that a rule can define 'deny' action.
14:36:01 <ralonsoh> atimmins, you need to guarantee no rule is in conflict with othrs
14:36:10 <amotoki> what happens if the order of groups are ambiguous
14:36:16 <amotoki> ?
14:36:53 <haleyb> isn't it inderminate today? if I have rules that both allow and deny a packet whichever is first wins
14:37:16 <obondarev> haleyb: ++
14:37:26 <ralonsoh> now "allow" should prevail, according to the documentation
14:37:29 <haleyb> Even if I feel allow should always win
14:37:37 <atimmins> That's the current behavior. Ordering changes any time a policy is edited, and if denies are present, traffic could start to get blocked unintentionally.
14:38:28 <haleyb> ralonsoh: and the opposite argument is that deny should prevail, else it's a vulnerability
14:38:29 <lajoskatona> by optional you mean to add an API extension with a new ordered=True/false option?
14:39:37 <obondarev> to me current behaviour seems buggy, so should be fixed without any options
14:39:42 <amotoki> i think there are two directions: one is to improve the behavior to be more determinestic. hte ohter is to introduce ordered=True/false. what do you think?
14:39:50 <ralonsoh> atimmins, sorry, I'm confused now. Does it means the current behaviour changes with the order?
14:39:54 <ralonsoh> it shouldn't now
14:41:01 <ralonsoh> I have the same impression as obondarev. And agree with amotoki to make the current behaviour deterministic, before implementing anything new
14:41:25 <atimmins> Yes, the spec is not accurate. Changing the order of groups on a port could impact whether traffic is allowed or denied. iptables processes the rules in order.
14:41:56 <atimmins> This is why there should be an order to groups, just as there is an order to rules within a policy.
14:42:28 <haleyb> I wonder what OVS does? But I agree making it deterministic is first step
14:42:35 <lajoskatona> +1
14:42:50 <obondarev> the question I have is: should it be ordered, or just 'deny' actions should be of highest priority?
14:43:03 <obondarev> the question I have is: should it be ordered, or just 'deny' actions should be of highest priority?
14:43:41 <atimmins> No, this would limit the functionality that iptables provides natively.
14:43:47 <mlavalle> no, deny should be the default. But once something is defined, it should behave the way the user defined
14:44:06 <amotoki> obondarev: or another option is to make the behaviro described in the initial spec
14:44:12 <lajoskatona> that's my understanding and it should be after editing also
14:44:48 <lajoskatona> amotoki: you mean as "future" work?
14:45:05 <amotoki> lajoskatona: yes
14:45:15 <amotoki> but we can discuss what is the right future
14:45:34 <amotoki> the initial spec is just one idea on multiple fw groups
14:46:56 <obondarev> so I guess we agree with the RFE/bug and need a spec, right?
14:47:22 <atimmins> imo, it's far more work to have neutron consolidate/reconcile rules among multiple groups, compared to adding an ordering to the port-group associations. The former could also have unintended consequences.
14:47:25 <ralonsoh> no sorry, I think we need first to determine if the current behaviour is what is defined in the spec
14:47:42 <amotoki> lajoskatona: correction: what I mean is the paragraph "This spec defines that packets will be allowed if any one of the firewall groups associated ...." in https://specs.openstack.org/openstack/neutron-specs/specs/newton/fwaas-api-2.0.html#multiple-firewall-policies
14:48:07 <amotoki> lajoskatona: I do not mean "In future phases, ..." paragraph in the above spec
14:48:44 <ralonsoh> ^^ this is the key point here that fwaas is "maybe" not honoring
14:48:46 <amotoki> ralonsoh: +1
14:48:47 <lajoskatona> amotoki: ok, thanks
14:48:58 <obondarev> ralonsoh: agree
14:50:55 <lajoskatona> ok, so generally the idea is good, but before starting any work the current situation must be checked compared to the original design /spec
14:51:32 <lajoskatona> and after that we can have a spec for example where the details can be discussed to have the good direction and decisions
14:51:40 <lajoskatona> Is that correct?
14:51:43 <mlavalle> +1
14:51:45 <ralonsoh> +1
14:51:56 <atimmins> I think the original bug report provides sufficient evidence that the original spec is incorrect, but totally fine if others would like to verify.
14:52:33 <amotoki> lajoskatona: is "the idea" the ordering of groups or does it mean to match the current implemetation with the initial spec?
14:52:36 <obondarev> atimmins: you mean the spec is incorrect or the implementation?
14:52:36 <lajoskatona> atimmins:  you mean this one: https://bugs.launchpad.net/neutron/+bug/1978497 ?
14:52:49 <atimmins> Yes, that one
14:53:12 <atimmins> The statement in the spec is incorrect based on the behavior we see today.
14:53:49 <lajoskatona> atimmins: than the first step should be to make the implementation in sync with the spec, am I right?
14:55:12 <atimmins> I find it more desirable to add ordering to the groups
14:56:19 <atimmins> I'm not immediately aware of how one would make the implementation in sync with the spec, as it is contrary to how traditional ACLs work.
14:56:21 <lajoskatona> with and API extension like ordered=True?
14:56:31 <opendevreview> Miro Tomaska proposed openstack/neutron master: [WIP] Need to update unit tests once we agree on implementation! Fix eventlet bug in get_hypervisor_hostname  https://review.opendev.org/c/openstack/neutron/+/849122
14:57:04 <obondarev> honestly, I'm a bit confused there are 'deny' rules in a "deny until explicitly allowed" concept, by maybe it's just my lack of expertise in this topic
14:57:28 * mlavalle has to leave to a doctor's appointment (routine check). On demand agenda topic can wait to Neutron's meeting on Tuesday
14:57:51 <lajoskatona> malavalle: ack, thanks for joining
14:58:05 <atimmins> By adding a "position" column to the relevant table, we can provider the ordering. Maybe if the value is Null, then the behavior stays the same? IE. order for that groups does not matter?
14:58:49 <ralonsoh> atimmins, please, before adding anything new to a project, let's fix what does not match with the current spec
14:59:03 <ralonsoh> this is even more important if security is related
14:59:22 <lajoskatona> ralonsoh: +1
14:59:27 <amotoki> ralonsoh: +1
14:59:46 <slaweq> ++
15:00:15 <atimmins> I'm proposing the best way to fix the current spec is by implementing group ordering.
15:00:44 <lajoskatona> ok, so let's write sown in the current RFE, what is that is not working in the current implementation as it is written in the spec
15:01:05 <lajoskatona> atimmins: thanks for it
15:01:27 <atimmins> No problem, thanks everyone
15:01:34 <lajoskatona> perhaps the best would be to open a spec where in review the discussion is quite easy to go in the agreed direction
15:02:25 <lajoskatona> We reached the end of the hour, wdyt of a spec for this and countinue the discussion under it with more details what is the need compared to the original spec?
15:02:40 <ralonsoh> perfect
15:02:45 <amotoki> +1
15:02:58 <slaweq> +!
15:02:59 <slaweq> +1
15:03:07 <obondarev> +1
15:03:38 <lajoskatona> ok, thanks for the discussion, I will update the RFE, and let's see the spec :-)
15:03:46 <lajoskatona> #endmeeting