shill: connection: Add dst rules for additional routes
Connection allows for additional routes to be added to a particular
Device's routing table through the use of
IPConfig::Properties::routes. However, physical interfaces have
conservative routing policy that prevents traffic (and source
selection) from using those additional routes unless the interface is
the default physical interface (in which case this happens because of
the catchall routing rule).
This change adds dst routing policy rules right before the catchall
routing rule so that these added routes will not be overlooked but
existing traffic going through a different interface will not be
stolen by the added rules.
BUG=b:152322923
TEST=All unit tests are passing.
Change-Id: I3e1fc7a98e79666594525c84ccf714a6d6e57ae1
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2118994
Tested-by: Alex Khouderchah <akhouderchah@chromium.org>
Reviewed-by: Hugo Benichi <hugobenichi@google.com>
Commit-Queue: Alex Khouderchah <akhouderchah@chromium.org>
(cherry picked from commit 75d60ff801b35fe062163c88a72b363292dba0c7)
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2137580
Reviewed-by: Alex Khouderchah <akhouderchah@chromium.org>
diff --git a/shill/connection.cc b/shill/connection.cc
index 1780a60..e08686e 100644
--- a/shill/connection.cc
+++ b/shill/connection.cc
@@ -39,13 +39,18 @@
// static
const uint32_t Connection::kDefaultPriority = 10;
-// static
-//
+// Allowed dsts rules are added right before the catchall rule. In this way,
+// existing traffic from a different interface will not be "stolen" by these
+// rules and sent out of the wrong interface, but the routes added to
+// |table_id| will not be ignored.
+const uint32_t Connection::kDstRulePriority =
+ RoutingTable::kRulePriorityMain - 2;
+const uint32_t Connection::kCatchallPriority =
+ RoutingTable::kRulePriorityMain - 1;
// UINT_MAX is also a valid priority, but we reserve this as a sentinel
// value, as in RoutingTable::GetDefaultRouteInternal.
const uint32_t Connection::kLeastPriority =
std::numeric_limits<uint32_t>::max() - 1;
-// static
const uint32_t Connection::kPriorityStep = 10;
Connection::Connection(int interface_index,
@@ -94,6 +99,7 @@
bool Connection::SetupIncludedRoutes(const IPConfig::Properties& properties) {
bool ret = true;
+ allowed_dsts_.clear();
IPAddress::Family address_family = properties.address_family;
for (const auto& route : properties.routes) {
SLOG(this, 2) << "Installing route:"
@@ -122,6 +128,17 @@
.SetTable(table_id_))) {
ret = false;
}
+ // While we have added routes to this Device's routing table, if there are
+ // not appropriate routing policy rules to send traffic to that routing
+ // table, these routes will essentially be ignored. VPNs have particular
+ // routing policy that is set by its VPNDriver, which ensures that the VPN's
+ // routing table is always used when appropriate. This is necessary for
+ // physical technologies because their routing policy is inherently more
+ // conservative and its table might not be used even when it contains a
+ // prefix route for the destination of the traffic.
+ if (technology_.IsPrimaryConnectivityTechnology()) {
+ allowed_dsts_.push_back(destination_address);
+ }
}
return ret;
}
@@ -242,15 +259,17 @@
interface_index_, IPAddress::kFamilyIPv6, 0, blackhole_table_id_);
}
- UpdateRoutingPolicy();
-
- SetupIncludedRoutes(config->properties());
-
if (properties.blackhole_ipv6) {
routing_table_->CreateBlackholeRoute(interface_index_,
IPAddress::kFamilyIPv6, 0, table_id_);
}
+ if (!SetupIncludedRoutes(properties)) {
+ LOG(WARNING) << "Failed to set up additional routes";
+ }
+
+ UpdateRoutingPolicy();
+
// Save a copy of the last non-null DNS config.
if (!config->properties().dns_servers.empty()) {
dns_servers_ = config->properties().dns_servers;
@@ -323,7 +342,7 @@
auto catch_all_rule =
RoutingPolicyEntry::CreateFromSrc(IPAddress(IPAddress::kFamilyIPv4))
.SetTable(table_id_)
- .SetPriority(RoutingTable::kRulePriorityMain - 1);
+ .SetPriority(kCatchallPriority);
routing_table_->AddRule(interface_index_, catch_all_rule);
routing_table_->AddRule(interface_index_, catch_all_rule.FlipFamily());
}
@@ -349,13 +368,20 @@
routing_table_->AddRule(interface_index_, entry.FlipFamily());
}
- for (const auto& source_address : allowed_addrs_) {
+ for (const auto& source_address : allowed_srcs_) {
routing_table_->AddRule(interface_index_,
RoutingPolicyEntry::CreateFromSrc(source_address)
.SetPriority(base_priority)
.SetTable(table_id));
}
+ for (const auto& dst_address : allowed_dsts_) {
+ routing_table_->AddRule(interface_index_,
+ RoutingPolicyEntry::CreateFromDst(dst_address)
+ .SetPriority(kDstRulePriority)
+ .SetTable(table_id));
+ }
+
// Add output interface rule for all interfaces, such that SO_BINDTODEVICE can
// be used without explicitly binding the socket.
auto oif_rule =
@@ -461,8 +487,8 @@
"%s/%d", local().GetNetworkPart().ToString().c_str(), local().prefix());
}
-void Connection::set_allowed_addrs(std::vector<IPAddress> addresses) {
- allowed_addrs_ = std::move(addresses);
+void Connection::set_allowed_srcs(std::vector<IPAddress> addresses) {
+ allowed_srcs_ = std::move(addresses);
}
bool Connection::FixGatewayReachability(const IPAddress& local,
diff --git a/shill/connection.h b/shill/connection.h
index 4ffea2a..407df06 100644
--- a/shill/connection.h
+++ b/shill/connection.h
@@ -27,13 +27,18 @@
class Resolver;
class RoutingTable;
-// The Conneciton maintains the implemented state of an IPConfig, e.g,
+// The Connection maintains the implemented state of an IPConfig, e.g,
// the IP address, routing table and DNS table entries.
class Connection : public base::RefCounted<Connection> {
public:
// The routing rule priority used for the default service, whether physical or
// VPN.
static const uint32_t kDefaultPriority;
+ // Priority for rules corresponding to IPConfig::Properties::routes.
+ static const uint32_t kDstRulePriority;
+ // Priority for the rule sending any remaining traffic to the default physical
+ // interface.
+ static const uint32_t kCatchallPriority;
// The lowest priority value that is still valid.
static const uint32_t kLeastPriority;
// Space between the priorities of services. The Nth highest priority service
@@ -113,7 +118,7 @@
virtual const IPAddress& local() const { return local_; }
virtual const IPAddress& gateway() const { return gateway_; }
virtual Technology technology() const { return technology_; }
- void set_allowed_addrs(std::vector<IPAddress> addresses);
+ void set_allowed_srcs(std::vector<IPAddress> addresses);
virtual const std::string& tethering() const { return tethering_; }
void set_tethering(const std::string& tethering) { tethering_ = tethering; }
@@ -178,14 +183,13 @@
// True if this device should have rules sending traffic whose src address
// matches one of the interface's addresses to the per-device table.
bool use_if_addrs_;
- // If |allowed_uids_|, |allowed_iifs_|, and/or |allowed_addrs_| is set, IP
- // policy rules will be created so that only traffic from the whitelisted
- // UIDs, input interfaces, and/or source IP addresses can use this connection,
- // with the exception of the interface's own IP addresses, which can always
- // use a connection when it corresponds to a physical interface.
+ // |allowed_{uids,iifs,dsts,srcs}_| allow for this connection to serve more
+ // traffic than it would by default.
+ // TODO(crbug.com/1022028) Replace this with a RoutingPolicy.
std::vector<uint32_t> allowed_uids_;
std::vector<std::string> allowed_iifs_;
- std::vector<IPAddress> allowed_addrs_;
+ std::vector<IPAddress> allowed_srcs_;
+ std::vector<IPAddress> allowed_dsts_;
std::vector<uint32_t> blackholed_uids_;
// Do not reconfigure the IP addresses, subnet mask, broadcast, etc.
diff --git a/shill/connection_test.cc b/shill/connection_test.cc
index 77d46e3..038eb3a 100644
--- a/shill/connection_test.cc
+++ b/shill/connection_test.cc
@@ -95,6 +95,10 @@
arg.oif_name == oif;
}
+MATCHER_P3(IsValidDstRule, family, priority, dst, "") {
+ return arg.family == family && arg.priority == priority && arg.dst == dst;
+}
+
MATCHER_P(IsLinkRouteTo, dst, "") {
return dst.HasSameAddressAs(arg.dst) &&
arg.dst.prefix() ==
@@ -209,6 +213,7 @@
properties_.routes = routes;
UpdateProperties();
+ included_route_dsts_.clear();
// Add expectations for the added routes.
auto address_family = properties_.address_family;
for (const auto& route : routes) {
@@ -227,6 +232,7 @@
source_address, gateway_address)
.SetMetric(connection_->priority_)
.SetTable(connection_->table_id_)));
+ included_route_dsts_.push_back(destination_address);
}
}
@@ -269,17 +275,15 @@
IsValidRoutingRule(IPAddress::kFamilyIPv6, priority - 1)))
.WillOnce(Return(true));
- EXPECT_CALL(
- routing_table_,
- AddRule(device->interface_index(),
- IsValidRoutingRule(IPAddress::kFamilyIPv4,
- RoutingTable::kRulePriorityMain - 1)))
+ EXPECT_CALL(routing_table_,
+ AddRule(device->interface_index(),
+ IsValidRoutingRule(IPAddress::kFamilyIPv4,
+ Connection::kCatchallPriority)))
.WillOnce(Return(true));
- EXPECT_CALL(
- routing_table_,
- AddRule(device->interface_index(),
- IsValidRoutingRule(IPAddress::kFamilyIPv6,
- RoutingTable::kRulePriorityMain - 1)))
+ EXPECT_CALL(routing_table_,
+ AddRule(device->interface_index(),
+ IsValidRoutingRule(IPAddress::kFamilyIPv6,
+ Connection::kCatchallPriority)))
.WillOnce(Return(true));
}
@@ -290,6 +294,13 @@
IsValidRoutingRule(address.family(), priority)))
.WillOnce(Return(true));
}
+ for (const auto& dst : included_route_dsts_) {
+ EXPECT_CALL(routing_table_,
+ AddRule(device->interface_index(),
+ IsValidDstRule(dst.family(),
+ Connection::kDstRulePriority, dst)))
+ .WillOnce(Return(true));
+ }
// Physical interfaces will have both iif and oif rules to send to the
// per-interface table if the interface name matches.
EXPECT_CALL(routing_table_,
@@ -338,6 +349,7 @@
IPAddress gateway_address_;
IPAddress default_address_;
IPAddress local_ipv6_address_;
+ std::vector<IPAddress> included_route_dsts_;
StrictMock<MockResolver> resolver_;
StrictMock<MockRoutingTable> routing_table_;
StrictMock<MockRTNLHandler> rtnl_handler_;
diff --git a/shill/manager.cc b/shill/manager.cc
index efd186b..58f2853 100644
--- a/shill/manager.cc
+++ b/shill/manager.cc
@@ -1748,7 +1748,7 @@
// physical connection. Android VPNs route traffic using interface
// mappings set up by arc-networkd.
if (conn)
- conn->set_allowed_addrs(vpn_addresses);
+ conn->set_allowed_srcs(vpn_addresses);
}
if (conn) {
if (!found_dns && !conn->dns_servers().empty()) {
