(or: "Remove hardcoding to IPv4 in function that creates masquerade rules" :-) On 6/9/20 12:17 PM, Daniel P. Berrangé wrote: It's nice that adding capability is done by *removing* code rather than adding it! Reviewed-by: Laine Stump <laine(a)redhat.com&gt;

On 6/9/20 12:17 PM, Daniel P. Berrangé wrote: Any network defined with an older libvirt, and even newly defined networks that don't have ipv6 set explicitly in the input XML, won't have an explicit value saved in the config. So if we change the default, those existing networks will have different behavior. I don't see how we could change the default without those existing networks getting automatically changed to ipv6='yes'. Eww. I just noticed this file isn't doing autofree memory allocation on  many of its pointers... ...which leads to a memory leak here, since the code at the cleanup label doesn't free ipv6. For some reason I felt like spending an hour doing something mindless just now, so I made a patch to convert all the char* and xmlNodePtr* in network_conf.c to g_autofree (along with a few other g_autoptr things, and the standard cleanup/error label removals). I'll post that as a patch based on your patches; you can either just assume that my patch will go in after yours and fix the memory leak, or you can just change ipv6 to a g_autofree yourself and I'll adjust my patch accordingly. I don't know that I agree with the comment about ease of changing the default in the future (maybe I misunderstood), and if you want the patch to stand on its own you should fix the one memory leak (my patch will be posted soon, if not already), but it all looks good and works properly now (for defining the network at least - I haven't actually tried getting an IPv6 address on a guest yet, but I'll let you know if there are any problems :-) Reviewed-by: Laine Stump <laine(a)redhat.com&gt;

Now that we have support for IPv6 in the iptables helpers, and a new option in the XML schema, we can wire up support for it in the network driver. Signed-off-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; --- src/network/bridge_driver_linux.c | 23 +- .../nat-ipv6-masquerade-linux.args | 228 ++++++++++++++++++ .../nat-ipv6-masquerade.xml | 17 ++ tests/networkxml2firewalltest.c | 1 + 4 files changed, 262 insertions(+), 7 deletions(-) create mode 100644 tests/networkxml2firewalldata/nat-ipv6-masquerade-linux.args create mode 100644 tests/networkxml2firewalldata/nat-ipv6-masquerade.xml diff --git a/src/network/bridge_driver_linux.c b/src/network/bridge_driver_linux.c index b0bd207250..fcb3803965 100644 --- a/src/network/bridge_driver_linux.c +++ b/src/network/bridge_driver_linux.c @@ -307,7 +307,8 @@ int networkCheckRouteCollision(virNetworkDefPtr def) return ret; } -static const char networkLocalMulticast[] = "224.0.0.0/24"; +static const char networkLocalMulticastIPv4[] = "224.0.0.0/24"; +static const char networkLocalMulticastIPv6[] = "ffx2::/16"; static const char networkLocalBroadcast[] = "255.255.255.255/32"; static int @@ -317,6 +318,7 @@ networkAddMasqueradingFirewallRules(virFirewallPtr fw, { int prefix = virNetworkIPDefPrefix(ipdef); const char *forwardIf = virNetworkDefForwardIf(def, 0); + bool isIPv4 = VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET); if (prefix < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -406,7 +408,8 @@ networkAddMasqueradingFirewallRules(virFirewallPtr fw, return -1; /* exempt local network broadcast address as destination */ - if (iptablesAddDontMasquerade(fw, + if (isIPv4 && + iptablesAddDontMasquerade(fw, &ipdef->address, prefix, forwardIf, @@ -418,7 +421,8 @@ networkAddMasqueradingFirewallRules(virFirewallPtr fw, &ipdef->address, prefix, forwardIf, - networkLocalMulticast) < 0) + isIPv4 ? networkLocalMulticastIPv4 : + networkLocalMulticastIPv6) < 0) return -1; return 0; @@ -431,6 +435,7 @@ networkRemoveMasqueradingFirewallRules(virFirewallPtr fw, { int prefix = virNetworkIPDefPrefix(ipdef); const char *forwardIf = virNetworkDefForwardIf(def, 0); + bool isIPv4 = VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET); if (prefix < 0) return 0; @@ -439,10 +444,12 @@ networkRemoveMasqueradingFirewallRules(virFirewallPtr fw, &ipdef->address, prefix, forwardIf, - networkLocalMulticast) < 0) + isIPv4 ? networkLocalMulticastIPv4 : + networkLocalMulticastIPv6) < 0) return -1; - if (iptablesRemoveDontMasquerade(fw, + if (isIPv4 && + iptablesRemoveDontMasquerade(fw, &ipdef->address, prefix, forwardIf, @@ -769,7 +776,8 @@ networkAddIPSpecificFirewallRules(virFirewallPtr fw, */ if (def->forward.type == VIR_NETWORK_FORWARD_NAT) { - if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET)) + if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET) || + def->forward.natIPv6 == VIR_TRISTATE_BOOL_YES) return networkAddMasqueradingFirewallRules(fw, def, ipdef); else if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) return networkAddRoutingFirewallRules(fw, def, ipdef); @@ -786,7 +794,8 @@ networkRemoveIPSpecificFirewallRules(virFirewallPtr fw, virNetworkIPDefPtr ipdef) { if (def->forward.type == VIR_NETWORK_FORWARD_NAT) { - if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET)) + if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET) || + def->forward.natIPv6 == VIR_TRISTATE_BOOL_YES) return networkRemoveMasqueradingFirewallRules(fw, def, ipdef); else if (VIR_SOCKET_ADDR_IS_FAMILY(&ipdef->address, AF_INET6)) return networkRemoveRoutingFirewallRules(fw, def, ipdef); diff --git a/tests/networkxml2firewalldata/nat-ipv6-masquerade-linux.args b/tests/networkxml2firewalldata/nat-ipv6-masquerade-linux.args new file mode 100644 index 0000000000..4ba4c3da30 --- /dev/null +++ b/tests/networkxml2firewalldata/nat-ipv6-masquerade-linux.args @@ -0,0 +1,228 @@ +iptables \ +--table filter \ +--insert LIBVIRT_INP \ +--in-interface virbr0 \ +--protocol tcp \ +--destination-port 67 \ +--jump ACCEPT +iptables \ +--table filter \ +--insert LIBVIRT_INP \ +--in-interface virbr0 \ +--protocol udp \ +--destination-port 67 \ +--jump ACCEPT +iptables \ +--table filter \ +--insert LIBVIRT_OUT \ +--out-interface virbr0 \ +--protocol tcp \ +--destination-port 68 \ +--jump ACCEPT +iptables \ +--table filter \ +--insert LIBVIRT_OUT \ +--out-interface virbr0 \ +--protocol udp \ +--destination-port 68 \ +--jump ACCEPT +iptables \ +--table filter \ +--insert LIBVIRT_INP \ +--in-interface virbr0 \ +--protocol tcp \ +--destination-port 53 \ +--jump ACCEPT +iptables \ +--table filter \ +--insert LIBVIRT_INP \ +--in-interface virbr0 \ +--protocol udp \ +--destination-port 53 \ +--jump ACCEPT +iptables \ +--table filter \ +--insert LIBVIRT_OUT \ +--out-interface virbr0 \ +--protocol tcp \ +--destination-port 53 \ +--jump ACCEPT +iptables \ +--table filter \ +--insert LIBVIRT_OUT \ +--out-interface virbr0 \ +--protocol udp \ +--destination-port 53 \ +--jump ACCEPT +iptables \ +--table filter \ +--insert LIBVIRT_FWO \ +--in-interface virbr0 \ +--jump REJECT +iptables \ +--table filter \ +--insert LIBVIRT_FWI \ +--out-interface virbr0 \ +--jump REJECT +iptables \ +--table filter \ +--insert LIBVIRT_FWX \ +--in-interface virbr0 \ +--out-interface virbr0 \ +--jump ACCEPT +ip6tables \ +--table filter \ +--insert LIBVIRT_FWO \ +--in-interface virbr0 \ +--jump REJECT +ip6tables \ +--table filter \ +--insert LIBVIRT_FWI \ +--out-interface virbr0 \ +--jump REJECT +ip6tables \ +--table filter \ +--insert LIBVIRT_FWX \ +--in-interface virbr0 \ +--out-interface virbr0 \ +--jump ACCEPT +ip6tables \ +--table filter \ +--insert LIBVIRT_INP \ +--in-interface virbr0 \ +--protocol tcp \ +--destination-port 53 \ +--jump ACCEPT +ip6tables \ +--table filter \ +--insert LIBVIRT_INP \ +--in-interface virbr0 \ +--protocol udp \ +--destination-port 53 \ +--jump ACCEPT +ip6tables \ +--table filter \ +--insert LIBVIRT_OUT \ +--out-interface virbr0 \ +--protocol tcp \ +--destination-port 53 \ +--jump ACCEPT +ip6tables \ +--table filter \ +--insert LIBVIRT_OUT \ +--out-interface virbr0 \ +--protocol udp \ +--destination-port 53 \ +--jump ACCEPT +ip6tables \ +--table filter \ +--insert LIBVIRT_INP \ +--in-interface virbr0 \ +--protocol udp \ +--destination-port 547 \ +--jump ACCEPT +ip6tables \ +--table filter \ +--insert LIBVIRT_OUT \ +--out-interface virbr0 \ +--protocol udp \ +--destination-port 546 \ +--jump ACCEPT +iptables \ +--table filter \ +--insert LIBVIRT_FWO \ +--source 192.168.122.0/24 \ +--in-interface virbr0 \ +--jump ACCEPT +iptables \ +--table filter \ +--insert LIBVIRT_FWI \ +--destination 192.168.122.0/24 \ +--out-interface virbr0 \ +--match conntrack \ +--ctstate ESTABLISHED,RELATED \ +--jump ACCEPT +iptables \ +--table nat \ +--insert LIBVIRT_PRT \ +--source 192.168.122.0/24 '!' \ +--destination 192.168.122.0/24 \ +--jump MASQUERADE +iptables \ +--table nat \ +--insert LIBVIRT_PRT \ +--source 192.168.122.0/24 \ +-p udp '!' \ +--destination 192.168.122.0/24 \ +--jump MASQUERADE \ +--to-ports 1024-65535 +iptables \ +--table nat \ +--insert LIBVIRT_PRT \ +--source 192.168.122.0/24 \ +-p tcp '!' \ +--destination 192.168.122.0/24 \ +--jump MASQUERADE \ +--to-ports 1024-65535 +iptables \ +--table nat \ +--insert LIBVIRT_PRT \ +--source 192.168.122.0/24 \ +--destination 255.255.255.255/32 \ +--jump RETURN +iptables \ +--table nat \ +--insert LIBVIRT_PRT \ +--source 192.168.122.0/24 \ +--destination 224.0.0.0/24 \ +--jump RETURN +ip6tables \ +--table filter \ +--insert LIBVIRT_FWO \ +--source 2001:db8:ca2:2::/64 \ +--in-interface virbr0 \ +--jump ACCEPT +ip6tables \ +--table filter \ +--insert LIBVIRT_FWI \ +--destination 2001:db8:ca2:2::/64 \ +--out-interface virbr0 \ +--match conntrack \ +--ctstate ESTABLISHED,RELATED \ +--jump ACCEPT +ip6tables \ +--table nat \ +--insert LIBVIRT_PRT \ +--source 2001:db8:ca2:2::/64 '!' \ +--destination 2001:db8:ca2:2::/64 \ +--jump MASQUERADE +ip6tables \ +--table nat \ +--insert LIBVIRT_PRT \ +--source 2001:db8:ca2:2::/64 \ +-p udp '!' \ +--destination 2001:db8:ca2:2::/64 \ +--jump MASQUERADE \ +--to-ports 1024-65535 +ip6tables \ +--table nat \ +--insert LIBVIRT_PRT \ +--source 2001:db8:ca2:2::/64 \ +-p tcp '!' \ +--destination 2001:db8:ca2:2::/64 \ +--jump MASQUERADE \ +--to-ports 1024-65535 +ip6tables \ +--table nat \ +--insert LIBVIRT_PRT \ +--source 2001:db8:ca2:2::/64 \ +--destination ffx2::/16 \ +--jump RETURN +iptables \ +--table mangle \ +--insert LIBVIRT_PRT \ +--out-interface virbr0 \ +--protocol udp \ +--destination-port 68 \ +--jump CHECKSUM \ +--checksum-fill diff --git a/tests/networkxml2firewalldata/nat-ipv6-masquerade.xml b/tests/networkxml2firewalldata/nat-ipv6-masquerade.xml new file mode 100644 index 0000000000..03bcc8c67d --- /dev/null +++ b/tests/networkxml2firewalldata/nat-ipv6-masquerade.xml @@ -0,0 +1,17 @@ +<network> + <name>default</name> + <bridge name="virbr0"/> + <forward> + <nat ipv6="yes"/> + </forward> + <ip address="192.168.122.1" netmask="255.255.255.0"> + <dhcp> + <range start="192.168.122.2" end="192.168.122.254"/> + </dhcp> + </ip> + <ip family="ipv6" address="2001:db8:ca2:2::1" prefix="64" > + <dhcp> + <range start="2001:db8:ca2:2:1::10" end="2001:db8:ca2:2:1::ff" /> + </dhcp> + </ip> +</network> diff --git a/tests/networkxml2firewalltest.c b/tests/networkxml2firewalltest.c index 0ad5e2303b..697bfd7e03 100644 --- a/tests/networkxml2firewalltest.c +++ b/tests/networkxml2firewalltest.c @@ -173,6 +173,7 @@ mymain(void) DO_TEST("nat-many-ips"); DO_TEST("nat-no-dhcp"); DO_TEST("nat-ipv6"); + DO_TEST("nat-ipv6-masquerade"); DO_TEST("route-default"); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; -- 2.26.2

On 6/10/20 12:14 AM, Laine Stump wrote: Based on discussion on IRC, apparently the "x" "ffx2" in the standards docs is intended to mean "any value for this digit", but so far only "ff02" is assigned/used, so we're in agreement that we should just change ffx2 (both here and in the test results file) to ff02.