get_firewall_required should use VIF parameter from neutron

Bug #1112912 reported by Nachi Ueno
108
This bug affects 20 people
Affects Status Importance Assigned to Milestone
OpenStack Compute (nova)
Fix Released
High
Nachi Ueno
neutron
Fix Released
Critical
Kevin Benton
tempest
Fix Released
Undecided
Kevin Benton

Bug Description

This bug report is from the discussion of
https://review.openstack.org/#/c/19126/17/nova/virt/libvirt/vif.py

I'm going to file this as a bug for tracking issue

The patch introduces get_firewall_required function.
But the patch checks only conf file.
This should use quantum VIF parameter.
https://github.com/openstack/quantum/blob/master/quantum/plugins/openvswitch/ovs_quantum_plugin.py#L513

Tags: sg-fw
Nachi Ueno (nati-ueno)
Changed in quantum:
milestone: none → grizzly-3
Akihiro Motoki (amotoki)
description: updated
Chuck Short (zulcss)
Changed in nova:
status: New → Confirmed
Revision history for this message
dan wendlandt (danwent) wrote :

assigning to nachi in quantum project, as this is a dependency for getting his quantum change in.

real work will be done in nova.

Changed in quantum:
assignee: nobody → Nachi Ueno (nati-ueno)
importance: Undecided → High
Akihiro Motoki (amotoki)
tags: added: sg-fw
Nachi Ueno (nati-ueno)
Changed in nova:
assignee: nobody → Nachi Ueno (nati-ueno)
Changed in nova:
importance: Undecided → High
milestone: none → grizzly-3
status: Confirmed → Triaged
Revision history for this message
Nachi Ueno (nati-ueno) wrote :

Akihiro and Dan and Me discussed about the quantum side spec.
The conclusion is following.

vif_require_iptables
vif_prevent_spoofing
vif_require_securitygroup

Note:
 why vif_ is needed? <- it is because the subnect ( nova or quantum ) is confusing no
 why just firewalling is not sufficient <- in nova, spoofing and securitygroup is different function, so we need to specify it.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to quantum (master)

Fix proposed to branch: master
Review: https://review.openstack.org/21946

Changed in quantum:
status: New → In Progress
Revision history for this message
Daniel Berrange (berrange) wrote : Re: get_firewall_required should use VIF parameter from quantum

@nachi: can you describe the intended semantics of each of those 3 options.

Revision history for this message
Akihiro Motoki (amotoki) wrote :

Three parameters have the following meaning.
- vif_require_securitygroup : If True, Quantum does not provide security group feature and Nova requires to provide security group feature.
- vif_prevent_spoofing : If True, Nova requires to setup IP/MAC spoofing filters (Quantum does not provide it). get_firewall_required() in libvirt/vif.py is expected to return True.
- vif_require_iptables : If True, Nova needs to make sure iptables works. If a bridge is OVS, hybrid vif plugging needs to be setup. There is no relation between this value and whether Quantum or Nova provides security group.

Revision history for this message
Akihiro Motoki (amotoki) wrote :

During reading nova/virt/libvirt code, I noticed that even if vif_require_securitygroup is passed from Quantum there is nothing
nova libvirt VIF driver can do. We need to configure firewall_driver in nova.conf.
What do you think nova is expected to do when vif_require_securitygroup is passed from Quantum?
IMO, to make this parameter work, we need to change the codes outside vif plugging.

In addition, libvirt nwfilter setup-ed by Nova defines two types of provider rules:
(a) rules to allow DHCP/RA packets and (b) rules to prevent IP/MAC spoofing.
What do you think about the relationship between vif_* parameters and the above nwfilter rules?

Revision history for this message
Nachi Ueno (nati-ueno) wrote :

Hi Akihito , Daniel

@Daniel,
We changed the paramter little bit

vif_security: {
require_securitygroup : boolean #If True, Quantum does not provide security group feature and Nova requires to provide security group feature,
prevent_spoofing :boolean # If True, Nova requires to setup IP/MAC spoofing filters (Quantum does not provide it). get_firewall_required() in libvirt/vif.py is expected to return True,
require_iptables : boolean # If True, Nova needs to make sure iptables works. If a bridge is
}

@Akihiro
I agree with you. May be this dynamic configuration may not be in G.
However we should have the function, so IMO it good to update Quantum side first.
( or may be, we should wait to add the parameter )

IMO prevent_spoofing is for both of (a) rules to allow DHCP/RA packets and (b) rules to prevent IP/MAC spoofing.
The reason is allowing only quantum's DHCP/RA server is for dhcp/RA spoofing. so IMO, both of (a) and (b) is for preventing spoofing.

Revision history for this message
Akihiro Motoki (amotoki) wrote :

@Nachi
Thanks for the clarification of the scope. I agree with you.
In G release, it sounds reasonable to configure libvirt firewall driver to NoopFirewall by manual.

Revision history for this message
Daniel Berrange (berrange) wrote :

> During reading nova/virt/libvirt code, I noticed that even if vif_require_securitygroup is passed from Quantum there is nothing
> nova libvirt VIF driver can do. We need to configure firewall_driver in nova.conf.

Yes, the current static configuration of firewall driver in Nova is fundamentally flawed. We need to remove that static configuration parameter and have the VIF driver dynamically decide on which firewall impl to use. At this point, we'll have to wait for the Havana release to make such a change though.

Revision history for this message
Daniel Berrange (berrange) wrote :

Looking at these parameters:

vif_security: {
require_securitygroup : boolean #If True, Quantum does not provide security group feature and Nova requires to provide security group feature,
prevent_spoofing :boolean # If True, Nova requires to setup IP/MAC spoofing filters (Quantum does not provide it). get_firewall_required() in libvirt/vif.py is expected to return True,
require_iptables : boolean # If True, Nova needs to make sure iptables works. If a bridge is
}

I think the first two parameters should be inverted. ie rather than telling Nova what todo, they should describe what Quantum has done. I'd also suggest having separate params for IP & MAC spoofing (even if Quantum does provide both at the same time). eg I'd prefer to see something like this:

vif_security {
   has_securitygroup: boolean. True if Quantum has provided a security group.
   has_ip_spoofing: boolean. True if Quantum has enabled IP spoofing protection.
   has_mac_spoofing: boolean. True if Quantum has enabled MAC spoofing protection
  require_iptables: boolean. True if Quantum requires support for iptables
}

Revision history for this message
dan wendlandt (danwent) wrote :

I'm fine with splitting mac + IP spoofing.

I think the main goal here is that it should always be unambiguous what is expected of the Nova virt layer. I don't really care how things are worded as long as that is the case.

My concern with wording things in terms of quantum is that just because quantum does or does not do something, it may not be unambiguous what what nova should do. Imagine a special use case of Quantum + Nova where no firewalling should be performed, hence, the quantum plugin does not support security groups. It then passes back has_securitygroup : false. However, this does not mean that it wants Nova to do security groups... quite the opposite, it doesn't want ANYONE to do security groups. If quantum passes back exactly what it wants Nova to do, we don't have a problem, as quantum could just tell Nova to not do security groups, and also not do security groups itself.

Revision history for this message
Daniel Berrange (berrange) wrote :

@dan: so the issue is really who is in charge of defining the overall policy for VIF network setup.

My mindset in comment #10 is that Quantum is providing a mechanism for creating VIFs, while Nova is providing the policy for configuring them with the guest, and as such Nova decides whether firewalling is required or not. So the scenario you describe is not an issue in this POV.

From your description it seems you believe Quantum is in charge of policy for how the VIF is configured & thus making the decision about whether firewalling is required or not, and Nova is only providing the mechanism.

IMHO it doesn't make sense for Quantum to be the thing declaring that a VM should have completely unfiltered network access. This is a decision for Nova to make, since it is in charge of VM managmeent & policy. Quantum is merely providing a way to connect a VM to a physical network

Revision history for this message
dan wendlandt (danwent) wrote :

Ok, now understand where you are coming from better. Well, at least we've gotten to the essence of the question :)

My understanding of your main goal was to eliminate the need for configuration variables in nova + quantum to be manually in sync, which to me implies that one entity would have to decide the desired state, and push it to the others. Since we are talking about aspects of network filtering, my thinking is that the network service would own the decision, pushing this information to Nova.

A key question here is which service "owns" network filtering policy, given that it is technically possible to implement filtering in both systems. Having it owned in two places doesn't make sense to me, since the policies could conflict unless manually synced. The reality is that Nova was originally designed in the absence of a network service, and thus took on certain network capabilities initially. We've moved functionality like security groups, floating IPs, etc. to Quantum, though we will not yet remove these capabilities from Nova for obvious backward compat reasons. As a result, it seems reasonable that when quantum is in use, the user has decided that quantum owns the definition of network behavior.

Revision history for this message
Nachi Ueno (nati-ueno) wrote :

so we move this to the grizzly-rc1?
I feel we need to still spec discussion here.

dan wendlandt (danwent)
Changed in quantum:
milestone: grizzly-3 → grizzly-rc1
Revision history for this message
Daniel Berrange (berrange) wrote :

@dan: i guess the Nova admin is still getting to choose which security filter is applied to each VM, so in that sense Nova is still in charge of overall functional policy. As such I'm ok with the naming proposal in comment #7, if we split the MAC & IP spoofing to separate parameters.

Thierry Carrez (ttx)
Changed in nova:
milestone: grizzly-3 → grizzly-rc1
Revision history for this message
Nachi Ueno (nati-ueno) wrote :

Hi folks

May be we should add dhcp stuff also.
please check this one

vif_security: {
require_securitygroup : boolean #If True, Quantum does not provide security group feature and Nova requires to provide security group feature,
prevent_ip_spoofing :boolean # If True, Nova requires to setup IP spoofing filters (Quantum does not provide it).
prevent_mac_spoofing :boolean # If True, Nova requires to setup MAC spoofing filters (Quantum does not provide it).
prevent_dhcp_spoofing :boolean # If True, Nova requires to setup MAC spoofing filters (Quantum does not provide it).get_firewall_required() in libvirt/vif.py is expected to return True,
require_iptables : boolean # If True, Nova needs to make sure iptables works. If a bridge is
}

Nachi Ueno (nati-ueno)
Changed in quantum:
milestone: grizzly-rc1 → havana-1
Changed in nova:
milestone: grizzly-rc1 → havana-1
Gary Kotton (garyk)
tags: added: grizzly-backport-potential
Changed in quantum:
milestone: havana-1 → havana-2
Changed in nova:
milestone: havana-1 → none
Changed in neutron:
milestone: havana-2 → none
Nachi Ueno (nati-ueno)
Changed in neutron:
milestone: none → havana-3
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to nova (master)

Fix proposed to branch: master
Review: https://review.openstack.org/44596

Changed in nova:
status: Triaged → In Progress
Changed in neutron:
milestone: havana-3 → havana-rc1
Changed in neutron:
milestone: havana-rc1 → none
Revision history for this message
Akihiro Motoki (amotoki) wrote : Re: get_firewall_required should use VIF parameter from quantum

As reported in bug 1252620, hybrid VIF driver no longer exists in Nova and Neutron security group with OVS agent does not work.

We need to address this issue ASAP.
ML2 with OVS agent is the reference implementation, so I believe it is critical.

Changed in neutron:
milestone: none → icehouse-1
importance: High → Critical
Revision history for this message
Salvatore Orlando (salvatore-orlando) wrote :

The issue was introduced with this commit: http://github.com/openstack-dev/devstack/commit/1143f7e45fd2760b8d5fecc8fbd598078ba92fd3

which uncovered that the hybrid plugging strategy is relying on get_firewall_required which just return a conf variable for nova.

Currently all neutron tests are running without security groups enforced, and all people using devstack with ml2/ovs/lb plugins are basically just unable to use security groups with default settings.

summary: - get_firewall_required should use VIF parameter from quantum
+ get_firewall_required should use VIF parameter from neutron
Revision history for this message
Daniel Berrange (berrange) wrote :

@salvatore, yes, per the TODO item in the code that get_firewall_required method was known to need more work. It was/is blocked on Neutron being able to provide suitable metadata when plugging the VIF. Unfortunately it seems the changeset fixing the neutron side of things in comment #17 has been abandoned.

Revision history for this message
Yair Fried (yfried) wrote :

@nati-ueno - you can use this unmerged test to check rules are enforced.
just remove the line ignoring negative connectivity test
https://review.openstack.org/#/c/55101/

Revision history for this message
Nachi Ueno (nati-ueno) wrote :

It looks like we should fix this ASAP
I'll work on this issue tomorrow.

Also, we should have negative test (ping should fail under the security group policy applied) in the gating

Revision history for this message
Yair Fried (yfried) wrote :

@nachi, the patch I posted contains negative tests but can't be merged until bug is fixed

Thierry Carrez (ttx)
Changed in neutron:
milestone: icehouse-1 → icehouse-2
Revision history for this message
Nachi Ueno (nati-ueno) wrote :

This is neutron side patch
https://review.openstack.org/#/c/21946/

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to neutron (master)

Fix proposed to branch: master
Review: https://review.openstack.org/67281

Thierry Carrez (ttx)
Changed in neutron:
milestone: icehouse-2 → icehouse-3
Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Fix proposed to branch: master
Review: https://review.openstack.org/72452

Changed in neutron:
assignee: Nachi Ueno (nati-ueno) → Robert Kukura (rkukura)
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Related fix merged to neutron (master)

Reviewed: https://review.openstack.org/72452
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=be8a06894390af032e8e0aea2108da4780678cc7
Submitter: Jenkins
Branch: master

commit be8a06894390af032e8e0aea2108da4780678cc7
Author: Bob Kukura <email address hidden>
Date: Mon Feb 3 23:18:44 2014 -0500

    Replace binding:capabilities with binding:vif_details

    In addition to binding:vif_type, the neutron core plugin needs to
    supply various information to nova's VIF driver, such as VIF security
    details and PCI details when SR-IOV is being used. This information is
    read-only, requires admin privileges, and is not intended for normal
    users. Rather than add separate mechanisms throughout the stack for
    each such requirement, the binding:capabilities port attibute, which
    is a dictionary and is not currently not used by nova, is renamed to
    binding:vif_details to serve as a general-purpose mechanism for
    supplying binding-specific details to the VIF driver.

    This patch does not remove or replace the CAP_PORT_FILTER boolean
    previously used in binding:capabilities. A separate patch should
    implement the specific key/value pairs carried by binding:vif_details
    to implement VIF security. Another patch will implement the key/value
    pairs needed for SR-IOV.

    The ML2 plugin now allows the bound mechanism driver to supply the
    binding:vif_details dictionary content, instead of just the
    CAP_PORT_FILTER boolean previously carried by the binding:capabilities
    attribute.

    DocImpact: Need to update portbinding extension API, but no impact on
    user or administrator documentation.

    Implements: blueprint vif-details
    Related-Bug: 1112912
    Change-Id: I34be746fcfa73c70f72b4f9add8eff3ac88c723f

Robert Kukura (rkukura)
Changed in neutron:
assignee: Robert Kukura (rkukura) → Nachi Ueno (nati-ueno)
Changed in neutron:
status: In Progress → Fix Committed
Nachi Ueno (nati-ueno)
Changed in neutron:
status: Fix Committed → In Progress
Thierry Carrez (ttx)
Changed in neutron:
milestone: icehouse-3 → icehouse-rc1
Revision history for this message
OpenStack Infra (hudson-openstack) wrote :

Reviewed: https://review.openstack.org/67281
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=f87e7d964c19cc2be33226df66f0c823af993d49
Submitter: Jenkins
Branch: master

commit f87e7d964c19cc2be33226df66f0c823af993d49
Author: Nachi Ueno <email address hidden>
Date: Thu Jan 16 10:54:26 2014 -0800

    Add enable_security_group option

    Using noop driver to disable security group is confusing.
    In this commit, we introduce enable_security_group in server side.

    DocImpact
    UpgradeImpact

    Implements bp: security-group-config-cleanup
    Related-Bug: 1112912
    Change-Id: Ice44a4e2a519c64e613eeb24372de46726473339

Revision history for this message
Tracy Jones (tjones-i) wrote :

the nova part will be addressed by

https://bugs.launchpad.net/neutron/+bug/1297469

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to neutron (master)

Fix proposed to branch: master
Review: https://review.openstack.org/83280

Changed in neutron:
assignee: Nachi Ueno (nati-ueno) → Kevin Benton (kevinbenton)
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to tempest (master)

Fix proposed to branch: master
Review: https://review.openstack.org/83292

Changed in tempest:
assignee: nobody → Kevin Benton (kevinbenton)
status: New → In Progress
Revision history for this message
Kevin Benton (kevinbenton) wrote :

Change combining neutron, nova, and tempest with the negative security group check tested as a devstack patch here: https://review.openstack.org/#/c/83281/

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to neutron (master)

Reviewed: https://review.openstack.org/83280
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=8c42ba115b58cc2c7486be9fee89e1facedb5f76
Submitter: Jenkins
Branch: master

commit 8c42ba115b58cc2c7486be9fee89e1facedb5f76
Author: Kevin Benton <email address hidden>
Date: Thu Mar 27 02:40:12 2014 +0000

    Adds OVS_HYBRID_PLUG flag to portbindings

    Adds a flag to the ML2, openvswitch, and BigSwitch
    plugins to inform nova that the OVS hybrid plugging
    strategy should be used.

    Closes-Bug: #1112912
    Change-Id: If004db60e084f4cea095ca9ecccb0537240d4183

Changed in neutron:
status: In Progress → Fix Committed
Revision history for this message
Russell Bryant (russellb) wrote :
Changed in nova:
milestone: none → icehouse-rc1
Changed in nova:
status: In Progress → Fix Committed
Revision history for this message
Thierry Carrez (ttx) wrote :

Is this exploitable?

Changed in ossa:
status: New → Incomplete
information type: Public → Public Security
Alan Pevec (apevec)
tags: removed: grizzly-backport-potential
Jeremy Stanley (fungi)
Changed in ossa:
assignee: nobody → Jeremy Stanley (fungi)
Revision history for this message
Kevin Benton (kevinbenton) wrote :

For information on what was affected. This would only be for deployments using ML2, neutron security groups, and the LibvirtGenericVIFDriver nova vif driver.

This does not affect LibvirtHybridOVSBridgeDriver based nova deployments. The LibvirtHybridOVSBridgeDriver was not removed until Icehouse.

Thierry Carrez (ttx)
Changed in nova:
status: Fix Committed → Fix Released
Thierry Carrez (ttx)
Changed in neutron:
status: Fix Committed → Fix Released
Revision history for this message
shankar ganesh p j (pjshankarganesh) wrote :

Is the Fix available in latest Havana maintenance release or should we wait till Icehouse release?

Revision history for this message
Jeremy Stanley (fungi) wrote :

This is only fixed in Icehouse release candidate 1 as far as I can tell. I'm still trying to get a handle on whether it's actually an exploitable security vulnerability in Havana warranting an advisory--it was opened during the Grizzly development cycle, but Thierry only just switched it to be a security bug a week ago so we could probe the risk exposure on it.

Revision history for this message
Nachi Ueno (nati-ueno) wrote :

This bug only affects Icehouse release. so no need to backport for previous one.

Revision history for this message
Jeremy Stanley (fungi) wrote :

Great--thanks Nachi!

information type: Public Security → Public
no longer affects: ossa
Thierry Carrez (ttx)
Changed in nova:
milestone: icehouse-rc1 → 2014.1
Thierry Carrez (ttx)
Changed in neutron:
milestone: icehouse-rc1 → 2014.1
Revision history for this message
Attila Fazekas (afazekas) wrote :
Changed in tempest:
status: In Progress → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.