From 115291a46f52ee4adfe85264b9566d216bcb25e8 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 22 Apr 2020 10:46:17 +0200 Subject: [PATCH] wireguard: don't let explicit gateway override WireGuard's peer route The profile's "ipv4.gateway" and "ipv6.gateway" has only one real purpose: to define the next hop of a static default route. Usually, when specifying a gateway in this way, the default route from other addressing methods (like DHCPv4 or IPv6 autoconf) gets ignored. If you have a WireGuard peer with "AllowedIPs=0.0.0.0/0" and "wireguard.peer-routes" enabled, NetworkManager would automatically add a route to the peer. Previously, if the user also set a gateway, that route was suppressed. That doesn't feel right. Note that configuring a gateway on a WireGuard profile is likely to be wrong to begin with. At least, unless you take otherwise care to avoid routing loops. If you take care, setting a gateway may work, but it would feel clearer to instead just add an explicit /0 manual route instead. Also, note that usually you don't need a gateway anyway. WireGuard is a Layer 3 (IP) tunnel, where the next hop is alway just the other side of the tunnel. The next hop has little effect on the routes that you configure on a WireGuard interface. What however matters is whether a default route is present or not. Also, an explicit gateway probably works badly with "ipv[46].ip4-auto-default-route", because in that case the automatism should add a /0 peer-route route in a separate routing table. The explicit gateway interferes with that too. Nonetheless, without this patch it's not obvious why the /0 peer route gets suppressed when a gateway is set. Don't allow for that, and always add the peer-route. Probably the profile's gateway setting is still wrong and causes the profile not to work. But at least, you see all routes configured, and it's clearer where the (wrong) default route to the gateway comes from. --- clients/common/settings-docs.h.in | 2 +- libnm-core/nm-setting-wireguard.c | 14 +++++++++----- src/devices/nm-device-wireguard.c | 6 +++++- src/nm-ip4-config.c | 3 ++- src/nm-ip4-config.h | 6 +++++- src/nm-ip6-config.c | 3 ++- src/nm-types.h | 4 +++- 7 files changed, 27 insertions(+), 11 deletions(-) diff --git a/clients/common/settings-docs.h.in b/clients/common/settings-docs.h.in index 3b8e235963..def9d284c5 100644 --- a/clients/common/settings-docs.h.in +++ b/clients/common/settings-docs.h.in @@ -386,7 +386,7 @@ #define DESCRIBE_DOC_NM_SETTING_WIMAX_MAC_ADDRESS N_("If specified, this connection will only apply to the WiMAX device whose MAC address matches. This property does not change the MAC address of the device (known as MAC spoofing). Deprecated: 1") #define DESCRIBE_DOC_NM_SETTING_WIMAX_NETWORK_NAME N_("Network Service Provider (NSP) name of the WiMAX network this connection should use. Deprecated: 1") #define DESCRIBE_DOC_NM_SETTING_WIREGUARD_FWMARK N_("The use of fwmark is optional and is by default off. Setting it to 0 disables it. Otherwise it is a 32-bit fwmark for outgoing packets. Note that \"ip4-auto-default-route\" or \"ip6-auto-default-route\" enabled, implies to automatically choose a fwmark.") -#define DESCRIBE_DOC_NM_SETTING_WIREGUARD_IP4_AUTO_DEFAULT_ROUTE N_("Whether to enable special handling of the IPv4 default route. If enabled, the IPv4 default route will be placed to a dedicated routing-table and two policy routing rules will be added. The fwmark number is also used as routing-table for the default-route, and if fwmark is zero, a unused fwmark/table is chosen automatically. This corresponds to what wg-quick does with Table=auto. Leaving this at the default will enable this option automatically if ipv4.never-default is not set and there are any peers that use a default-route as allowed-ips.") +#define DESCRIBE_DOC_NM_SETTING_WIREGUARD_IP4_AUTO_DEFAULT_ROUTE N_("Whether to enable special handling of the IPv4 default route. If enabled, the IPv4 default route from wireguard.peer-routes will be placed to a dedicated routing-table and two policy routing rules will be added. The fwmark number is also used as routing-table for the default-route, and if fwmark is zero, an unused fwmark/table is chosen automatically. This corresponds to what wg-quick does with Table=auto and what WireGuard calls \"Improved Rule-based Routing\". Note that for this automatism to work, you usually don't want to set ipv4.gateway, because that will result in a conflicting default route. Leaving this at the default will enable this option automatically if ipv4.never-default is not set and there are any peers that use a default-route as allowed-ips.") #define DESCRIBE_DOC_NM_SETTING_WIREGUARD_IP6_AUTO_DEFAULT_ROUTE N_("Like ip4-auto-default-route, but for the IPv6 default route.") #define DESCRIBE_DOC_NM_SETTING_WIREGUARD_LISTEN_PORT N_("The listen-port. If listen-port is not specified, the port will be chosen randomly when the interface comes up.") #define DESCRIBE_DOC_NM_SETTING_WIREGUARD_MTU N_("If non-zero, only transmit packets of the specified size or smaller, breaking larger packets up into multiple fragments. If zero a default MTU is used. Note that contrary to wg-quick's MTU setting, this does not take into account the current routes at the time of activation.") diff --git a/libnm-core/nm-setting-wireguard.c b/libnm-core/nm-setting-wireguard.c index 1f6940a898..2bd633cecb 100644 --- a/libnm-core/nm-setting-wireguard.c +++ b/libnm-core/nm-setting-wireguard.c @@ -2534,11 +2534,15 @@ nm_setting_wireguard_class_init (NMSettingWireGuardClass *klass) * NMSettingWireGuard:ip4-auto-default-route: * * Whether to enable special handling of the IPv4 default route. - * If enabled, the IPv4 default route will be placed to a dedicated - * routing-table and two policy routing rules will be added. - * The fwmark number is also used as routing-table for the default-route, - * and if fwmark is zero, a unused fwmark/table is chosen automatically. - * This corresponds to what wg-quick does with Table=auto. + * If enabled, the IPv4 default route from wireguard.peer-routes + * will be placed to a dedicated routing-table and two policy routing rules + * will be added. The fwmark number is also used as routing-table for the default-route, + * and if fwmark is zero, an unused fwmark/table is chosen automatically. + * This corresponds to what wg-quick does with Table=auto and what WireGuard + * calls "Improved Rule-based Routing". + * + * Note that for this automatism to work, you usually don't want to set + * ipv4.gateway, because that will result in a conflicting default route. * * Leaving this at the default will enable this option automatically * if ipv4.never-default is not set and there are any peers that use diff --git a/src/devices/nm-device-wireguard.c b/src/devices/nm-device-wireguard.c index 8dffb7f957..9f01a76323 100644 --- a/src/devices/nm-device-wireguard.c +++ b/src/devices/nm-device-wireguard.c @@ -1639,8 +1639,12 @@ _get_dev2_ip_config (NMDeviceWireGuard *self, continue; } - if (!ip_config) + if (!ip_config) { ip_config = nm_device_ip_config_new (NM_DEVICE (self), addr_family); + nm_ip_config_set_config_flags (ip_config, + NM_IP_CONFIG_FLAGS_IGNORE_MERGE_NO_DEFAULT_ROUTES, + 0); + } nm_utils_ipx_address_clear_host_address (addr_family, &addrbin, NULL, prefix); diff --git a/src/nm-ip4-config.c b/src/nm-ip4-config.c index 8c16d0bfae..0feb7b2c3c 100644 --- a/src/nm-ip4-config.c +++ b/src/nm-ip4-config.c @@ -1203,7 +1203,8 @@ nm_ip4_config_merge (NMIP4Config *dst, nm_ip_config_iter_ip4_route_for_each (&ipconf_iter, src, &r_src) { if (NM_PLATFORM_IP_ROUTE_IS_DEFAULT (r_src)) { - if (NM_FLAGS_HAS (merge_flags, NM_IP_CONFIG_MERGE_NO_DEFAULT_ROUTES)) + if ( NM_FLAGS_HAS (merge_flags, NM_IP_CONFIG_MERGE_NO_DEFAULT_ROUTES) + && !NM_FLAGS_HAS (src_priv->config_flags, NM_IP_CONFIG_FLAGS_IGNORE_MERGE_NO_DEFAULT_ROUTES)) continue; if (default_route_metric_penalty) { NMPlatformIP4Route r = *r_src; diff --git a/src/nm-ip4-config.h b/src/nm-ip4-config.h index b2845da3bb..d4694d9360 100644 --- a/src/nm-ip4-config.h +++ b/src/nm-ip4-config.h @@ -16,7 +16,11 @@ /*****************************************************************************/ typedef enum _NMIPConfigFlags { - NM_IP_CONFIG_FLAG_NONE = 0, + NM_IP_CONFIG_FLAG_NONE = 0, + + /* if set, then the merge flag NM_IP_CONFIG_MERGE_NO_DEFAULT_ROUTES gets + * ignored during merge. */ + NM_IP_CONFIG_FLAGS_IGNORE_MERGE_NO_DEFAULT_ROUTES = (1ull << 0), } NMIPConfigFlags; typedef struct { diff --git a/src/nm-ip6-config.c b/src/nm-ip6-config.c index 3f2c465bda..b60e8057cb 100644 --- a/src/nm-ip6-config.c +++ b/src/nm-ip6-config.c @@ -876,7 +876,8 @@ nm_ip6_config_merge (NMIP6Config *dst, nm_ip_config_iter_ip6_route_for_each (&ipconf_iter, src, &r_src) { if (NM_PLATFORM_IP_ROUTE_IS_DEFAULT (r_src)) { - if (NM_FLAGS_HAS (merge_flags, NM_IP_CONFIG_MERGE_NO_DEFAULT_ROUTES)) + if ( NM_FLAGS_HAS (merge_flags, NM_IP_CONFIG_MERGE_NO_DEFAULT_ROUTES) + && !NM_FLAGS_HAS (src_priv->config_flags, NM_IP_CONFIG_FLAGS_IGNORE_MERGE_NO_DEFAULT_ROUTES)) continue; if (default_route_metric_penalty) { NMPlatformIP6Route r = *r_src; diff --git a/src/nm-types.h b/src/nm-types.h index 4db9a19b13..d2f6fb4093 100644 --- a/src/nm-types.h +++ b/src/nm-types.h @@ -233,7 +233,9 @@ typedef enum { * NMIPConfigMergeFlags: * @NM_IP_CONFIG_MERGE_DEFAULT: no flags set * @NM_IP_CONFIG_MERGE_NO_ROUTES: don't merge routes - * @NM_IP_CONFIG_MERGE_NO_DEFAULT_ROUTES: don't merge default routes + * @NM_IP_CONFIG_MERGE_NO_DEFAULT_ROUTES: don't merge default routes. + * Note that if the source IP config has NM_IP_CONFIG_FLAGS_IGNORE_MERGE_NO_DEFAULT_ROUTES + * set, this flag gets ignored during merge. * @NM_IP_CONFIG_MERGE_NO_DNS: don't merge DNS information * @NM_IP_CONFIG_MERGE_EXTERNAL: mark new addresses as external */