patchpanel: add iface based forwarding for guest traffic
This patch adds FORWARD ACCEPT rules based on iif and oif interface
names for ARC++ like downstream guests (multinetwork) and Crosvm like
downstream guests (single network tracking). ConnectedNamespaces will be
migrated in a follow-up patch.
Note that these new rules are effectively not doing anything until the
current FORWARD -m match --mark 1/1 -j ACCEPT rule is maintained in the
system. Before relying on these new FORWARD rules, fwmark based routing
should be implemented first.
BUG=b:161507671
BUG=b:161508179
TEST=Unit tests. Flased rammus, checked ARC++ connectivity.
Change-Id: Iaafb5b6060d40fe7f08d223286dd4fa11eb3b273
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2359937
Tested-by: Hugo Benichi <hugobenichi@google.com>
Commit-Queue: Hugo Benichi <hugobenichi@google.com>
Reviewed-by: Abhishek Kumar <kuabhs@chromium.org>
Reviewed-by: Taoyu Li <taoyl@chromium.org>
diff --git a/patchpanel/crostini_service.cc b/patchpanel/crostini_service.cc
index 56a0744..d628677 100644
--- a/patchpanel/crostini_service.cc
+++ b/patchpanel/crostini_service.cc
@@ -88,6 +88,9 @@
LOG(INFO) << "Crostini network service started for {id: " << vm_id << "}";
StartForwarding(shill_client_->default_interface(), tap->host_ifname());
+ auto source = is_termina ? TrafficSource::CROSVM : TrafficSource::PLUGINVM;
+ datapath_->StartRoutingDevice("", tap->host_ifname(),
+ tap->config().host_ipv4_addr(), source);
if (adb_sideloading_enabled_)
StartAdbPortForwarding(tap->phys_ifname());
@@ -105,6 +108,9 @@
}
const auto& ifname = it->second->host_ifname();
+ auto source = is_termina ? TrafficSource::CROSVM : TrafficSource::PLUGINVM;
+ datapath_->StopRoutingDevice("", ifname,
+ it->second->config().host_ipv4_addr(), source);
StopForwarding(shill_client_->default_interface(), ifname);
if (adb_sideloading_enabled_)
StopAdbPortForwarding(ifname);
diff --git a/patchpanel/datapath.cc b/patchpanel/datapath.cc
index 8e3b7a0..7eff599 100644
--- a/patchpanel/datapath.cc
+++ b/patchpanel/datapath.cc
@@ -332,16 +332,13 @@
LOG(ERROR) << "Failed to configure ingress traffic rules for " << ext_ifname
<< "->" << int_ifname;
- // TODO(b/161508179) Explicitly enable IPv4 forwarding for this pair of
- // interfaces for the ingress direction.
- if (!AddOutboundIPv4(int_ifname))
- LOG(ERROR) << "Failed to configure egress traffic rules for " << ext_ifname
- << "<-" << int_ifname;
+ if (StartIpForwarding(IpFamily::IPv4, ext_ifname, int_ifname))
+ LOG(ERROR) << "Failed to enable IP forwarding for " << ext_ifname << "->"
+ << int_ifname;
- // TODO(b/161508179) Explicitly enable IPv4 forwarding for this pair of
- // interfaces for the egress direction and stop relying on the traffic fwmark
- // matching 1/1 once forwarded egress traffic is routed through the fwmark
- // routing tag.
+ if (StartIpForwarding(IpFamily::IPv4, int_ifname, ext_ifname))
+ LOG(ERROR) << "Failed to enable IP forwarding for " << ext_ifname << "<-"
+ << int_ifname;
// TODO(b/161507671) If ext_ifname is not null, mark egress traffic with the
// fwmark routing tag corresponding to |ext_ifname|, and set up strong routing
@@ -360,9 +357,8 @@
TrafficSource source) {
if (!ext_ifname.empty())
RemoveInboundIPv4DNAT(ext_ifname, IPv4AddressToString(int_ipv4_addr));
- RemoveOutboundIPv4(int_ifname);
-
- // TODO(b/161508179) Stops IPv4 forwarding for this pair of interfaces.
+ StopIpForwarding(IpFamily::IPv4, ext_ifname, int_ifname);
+ StopIpForwarding(IpFamily::IPv4, int_ifname, ext_ifname);
// TODO(b/161508179) Remove source marking for egress traffic.
// TODO(b/161507671) Remove routing tag marking for egress traffic.
}
@@ -415,6 +411,8 @@
StopIpForwarding(IpFamily::IPv4, "", ifname);
}
+// TODO(b/161507671) Stop relying on the traffic fwmark 1/1 once forwarded
+// egress traffic is routed through the fwmark routing tag.
bool Datapath::AddSNATMarkRules() {
// chromium:1050579: INVALID packets cannot be tracked by conntrack therefore
// need to be explicitly dropped.
diff --git a/patchpanel/datapath_test.cc b/patchpanel/datapath_test.cc
index f850b78..5eb94f7 100644
--- a/patchpanel/datapath_test.cc
+++ b/patchpanel/datapath_test.cc
@@ -313,8 +313,12 @@
"--to-destination", "1.2.3.4", "-w"),
true, nullptr));
EXPECT_CALL(runner, iptables(StrEq("filter"),
- ElementsAre("-A", "FORWARD", "-o", "arc_eth0",
- "-j", "ACCEPT", "-w"),
+ ElementsAre("-A", "FORWARD", "-i", "eth0", "-o",
+ "arc_eth0", "-j", "ACCEPT", "-w"),
+ true, nullptr));
+ EXPECT_CALL(runner, iptables(StrEq("filter"),
+ ElementsAre("-A", "FORWARD", "-i", "arc_eth0",
+ "-o", "eth0", "-j", "ACCEPT", "-w"),
true, nullptr));
Datapath datapath(&runner, &firewall);
@@ -329,6 +333,10 @@
ElementsAre("-A", "FORWARD", "-o", "vmtap0",
"-j", "ACCEPT", "-w"),
true, nullptr));
+ EXPECT_CALL(runner, iptables(StrEq("filter"),
+ ElementsAre("-A", "FORWARD", "-i", "vmtap0",
+ "-j", "ACCEPT", "-w"),
+ true, nullptr));
Datapath datapath(&runner, &firewall);
datapath.StartRoutingDevice("", "vmtap0", Ipv4Addr(1, 2, 3, 4),
@@ -354,8 +362,12 @@
"--to-destination", "1.2.3.4", "-w"),
true, nullptr));
EXPECT_CALL(runner, iptables(StrEq("filter"),
- ElementsAre("-D", "FORWARD", "-o", "arc_eth0",
- "-j", "ACCEPT", "-w"),
+ ElementsAre("-D", "FORWARD", "-i", "eth0", "-o",
+ "arc_eth0", "-j", "ACCEPT", "-w"),
+ true, nullptr));
+ EXPECT_CALL(runner, iptables(StrEq("filter"),
+ ElementsAre("-D", "FORWARD", "-i", "arc_eth0",
+ "-o", "eth0", "-j", "ACCEPT", "-w"),
true, nullptr));
Datapath datapath(&runner, &firewall);
@@ -370,6 +382,10 @@
ElementsAre("-D", "FORWARD", "-o", "vmtap0",
"-j", "ACCEPT", "-w"),
true, nullptr));
+ EXPECT_CALL(runner, iptables(StrEq("filter"),
+ ElementsAre("-D", "FORWARD", "-i", "vmtap0",
+ "-j", "ACCEPT", "-w"),
+ true, nullptr));
Datapath datapath(&runner, &firewall);
datapath.StopRoutingDevice("", "vmtap0", Ipv4Addr(1, 2, 3, 4),