Run:
./contrib/scripts/nm-code-format.sh -i
./contrib/scripts/nm-code-format.sh -i
Yes, it needs to run twice because the first run doesn't yet produce the
final result.
Signed-off-by: Antonio Cardace <acardace@redhat.com>
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.
(cherry picked from commit 115291a46f)
Enabling both peer-routes and never-default conflicts with having
AllowedIPs set to a default route. Let never-default win.
(cherry picked from commit 5da82ee3ea)
There was only API to schedule the stage on an idle handler.
Sometimes, we are just in the right situation to schedule the stage
right away. It should be possibly to avoid going through the extra hop.
For now, none of the caller makes use of this. So, there isn't any
actual change in behavior. But by adding this possibility, we may do
use in the future.
Several macros are used to define function. They had a "_STATIC" variant,
to define the function as static.
I think those macros should not try to abstract entirely what they do.
They should not accept the function scope as argument (or have two
variants per scope). This also because it might make sense to add
additional __attribute__(()) to the function. That only works, if
the macro does not pretend to *not* define a plain function.
Instead, embrace what the function does and let the users place the
function scope as they see fit.
This also follows what is already done with
static NM_CACHED_QUARK_FCN ("autoconnect-root", autoconnect_root_quark)
Most callers would pass FALSE to nm_utils_error_is_cancelled(). That's
not very useful. Split the two functions and have nm_utils_error_is_cancelled()
and nm_utils_error_is_cancelled_is_disposing().
The abbreviations "ns" and "ms" seem not very clear to me. Spell them
out to nsec/msec. Also, in parts we already used the longer abbreviations,
so it wasn't consistent.
Allow a reapply of the connection when the device is still activating
and ensure that each reapply action is performed only at a given
activation stage. For example, the IP configuration is not reactivated
if the device is in the prepare stage.
https://bugzilla.redhat.com/show_bug.cgi?id=1763062
We try to set only one time the MTU from the connection to not
interfere with manual user changes.
If at some point the parent interface changes temporarily MTU to a
lower value (for example, because the connection was reactivated), the
kernel will also lower the MTU on child interface and we will not
update it ever again.
Add a workaround to this. If we detect that the MTU we want to set
from connection is higher that the allowed one, go into a state where
we follow the parent MTU until it is possible to set again the desired
MTU. This is a bit ugly, but I can't think of any nicer way to do it.
https://bugzilla.redhat.com/show_bug.cgi?id=1751079
I am about to change the when stage1 gets postponed, then the way to
proceed it is to schedule stage1 again (instead of scheduling stage2).
The reason is that stage1 handling should be reentrant and we should
keep entering it until there is no more reason to postpone it. If
a subclass postpones stage1 and then later progresses it by directly
scheduling stage2, then only the subclass is in control over postponing
stage 2.
Instead, anybody should be able to delay stage2 independently. That can
only work if everybody signals readyness to proceed by scheduling stage1
again.
act_stage1_prepare() should become re-entrant. That means, we should not clear the state
there. Instead, we clear it where necessary or on deactivate (which we do already).
With "wireguard.ip4-auto-default-route" and "wireguard.ip6-auto-default-route",
NetworkManager automatically adds policy routing rules for the default
route.
For that, it needs to pick (up to) two numbers:
- the fwmark. This is used both for WireGuard's fwmark setting and
is also the number of the routing table where the default-route is
added.
- the rule priority. NetworkManager adds two policy routing rules, and
we need to place them somewhere before the default rules (at 32766).
Previously, we looked at exiting platform configuration and picked
numbers that were not yet used. However, during restart of
NetworkManager, we leave the interface up and after restart we will
take over the previous configuration. At that point, we need to choose
the same fwmark/priority, otherwise the configuration changes.
But since we picked numbers that were not yet used, we would always choose
different numbers. For routing rules that meant that after restart a second
pair of rules was added.
We possibly could store this data in the device's state-file. But that
is complex. Instead, just pick numbers deterministically based on the
connection's UUID.
If the picked numbers are not suitable, then the user can still work
around that:
- for fwmark, the user can explicitly configure wireguard.fwmark
setting.
- for the policy routes, the user can explicitly add the rules with
the desired priorirites (arguably, currently the default-route cannot
be added like a regular route, so the table cannot be set. Possibly
the user would have to add two /1 routes instead with
suppress_prefixlength=1).
(cherry picked from commit cfb497e499)
We call _auto_default_route_init() at various places, for example during
coerce_route_table(). We cannot be sure that we don't call it before
activation starts (before stage1) or after device-cleanup.
That means, upon activation, we need to clear the state first. Do that in
act_stage1_prepare().
(cherry picked from commit dc219662fa)
#3 0x00007fb0aa9e7d3d in g_return_if_fail_warning
(log_domain=log_domain@entry=0x562295fd5ee3 "libnm", pretty_function=pretty_function@entry=0x562295fd71d0 <__func__.35180> "_connection_get_setting_check", expression=expression@entry=0x562295f8edf7 "NM_IS_CONNECTION (connection)") at ../glib/gmessages.c:2767
#4 0x0000562295df151a in _connection_get_setting_check (connection=0x0, setting_type=0x562297b17050 [NMSettingWireGuard/NMSetting]) at libnm-core/nm-connection.c:207
#5 0x0000562295df151a in _connection_get_setting_check (connection=0x0, setting_type=0x562297b17050 [NMSettingWireGuard/NMSetting]) at libnm-core/nm-connection.c:205
#6 0x0000562295ef132a in _nm_connection_get_setting (type=<optimized out>, connection=0x0) at ./libnm-core/nm-core-internal.h:483
#7 0x0000562295ef132a in _auto_default_route_init (self=self@entry=0x562297bf82b0 [NMDeviceWireGuard]) at src/devices/nm-device-wireguard.c:443
#8 0x0000562295ef1b98 in coerce_route_table (device=0x562297bf82b0 [NMDeviceWireGuard], addr_family=2, route_table=0, is_user_config=<optimized out>)
at src/devices/nm-device-wireguard.c:565
#9 0x0000562295ea42ae in _get_route_table (self=self@entry=0x562297bf82b0 [NMDeviceWireGuard], addr_family=addr_family@entry=2) at src/devices/nm-device.c:2311
#10 0x0000562295ea4593 in nm_device_get_route_table (self=0x562297bf82b0 [NMDeviceWireGuard], addr_family=2) at src/devices/nm-device.c:2338
#11 0x0000562295eabde0 in ip_config_merge_and_apply (self=0x562297bf82b0 [NMDeviceWireGuard], addr_family=2, commit=1) at src/devices/nm-device.c:7590
#12 0x0000562295ed2f0b in device_link_changed (self=self@entry=0x562297bf82b0 [NMDeviceWireGuard]) at src/devices/nm-device.c:3939
#13 0x00007fb0aa9dc7db in g_idle_dispatch (source=source@entry=0x562297bf0b30, callback=0x562295ed2880 <device_link_changed>, user_data=0x562297bf82b0) at ../glib/gmain.c:5627
#14 0x00007fb0aa9dfedd in g_main_dispatch (context=0x562297a28090) at ../glib/gmain.c:3189
#15 0x00007fb0aa9dfedd in g_main_context_dispatch (context=context@entry=0x562297a28090) at ../glib/gmain.c:3854
#16 0x00007fb0aa9e0270 in g_main_context_iterate (context=0x562297a28090, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at ../glib/gmain.c:3927
#17 0x00007fb0aa9e05a3 in g_main_loop_run (loop=0x562297a0b380) at ../glib/gmain.c:4123
#18 0x0000562295d0b147 in main (argc=<optimized out>, argv=<optimized out>) at src/main.c:465
https://bugzilla.redhat.com/show_bug.cgi?id=1734383
(cherry picked from commit 47fc1a4293)
For WireGuard (like for all IP-tunnels and IP-based VPNs), the IP addresses of
the peers must be reached outside the tunnel/VPN itself.
For VPN connections, NetworkManager usually adds a direct /32 route to
the external VPN gateway to the underlying device. For WireGuard that is
not done, because injecting a route to another device is ugly and error
prone. Worse: WireGuard with automatic roaming and multiple peers makes this
more complicated.
This is commonly a problem when setting the default-route via the VPN,
but there are also other subtle setups where special care must be taken
to prevent such routing loops.
WireGuard's wg-quick provides a simple, automatic solution by adding two policy
routing rules and relying on the WireGuard packets having a fwmark set (see [1]).
Let's also do that. Add new properties "wireguard.ip4-auto-default-route"
and "wireguard.ip6-auto-default-route" to enable/disable this. Note that
the default value lets NetworkManager automatically choose whether to
enable it (depending on whether there are any peers that have a default
route). This means, common scenarios should now work well without additional
configuration.
Note that this is also a change in behavior and upon package upgrade
NetworkManager may start adding policy routes (if there are peers that
have a default-route). This is a change in behavior, as the user already
clearly had this setup working and configured some working solution
already.
The new automatism picks the rule priority automatically and adds the
default-route to the routing table that has the same number as the fwmark.
If any of this is unsuitable, then the user is free to disable this
automatism. Note that since 1.18.0 NetworkManager supports policy routing (*).
That means, what this automatism does can be also achieved via explicit
configuration of the profile, which gives the user more flexibility to
adjust all parameters explicitly).
(*) but only since 1.20.0 NetworkManager supports the "suppress_prefixlength"
rule attribute, which makes it impossible to configure exactly this rule-based
solution with 1.18.0 NetworkManager.
[1] https://www.wireguard.com/netns/#improved-rule-based-routing
Changing "ipv4.route-table" and "ipv6.route-table" was not allowed
during reapply.
The main difficulty for supporting that is changing the sync-mode.
With route-table 0, we don't sync all tables but only the main table.
So, when reapply changes from full-sync to no-full-sync, it's slightly
more complicated.
But it's probably not too complicated either. The change from
no-full-sync to full-sync is simple: we just start doing a full-sync.
The reverse change is slightly more complicated, because we need to
do one last full-sync, to get rid of routes that we configured on those
other tables.
From the files under "shared/nm-utils" we build an internal library
that provides glib-based helper utilities.
Move the files of that basic library to a new subdirectory
"shared/nm-glib-aux" and rename the helper library "libnm-core-base.la"
to "libnm-glib-aux.la".
Reasons:
- the name "utils" is overused in our code-base. Everything's an
"utils". Give this thing a more distinct name.
- there were additional files under "shared/nm-utils", which are not
part of this internal library "libnm-utils-base.la". All the files
that are part of this library should be together in the same
directory, but files that are not, should not be there.
- the new name should better convey what this library is and what is isn't:
it's a set of utilities and helper functions that extend glib with
funcitonality that we commonly need.
There are still some files left under "shared/nm-utils". They have less
a unifying propose to be in their own directory, so I leave them there
for now. But at least they are separate from "shared/nm-glib-aux",
which has a very clear purpose.
(cherry picked from commit 80db06f768)
A NetworkManager client requires an API to validate and decode
a base64 secret -- like it is used by WireGuard. If we don't have
this as part of the API, it's inconvenient. Expose it.
Rename it from _nm_utils_wireguard_decode_key(), to give it a more
general name.
Also, rename _nm_utils_wireguard_normalize_key() to
nm_utils_base64secret_normalize(). But this one we keep as internal
API. The user will care more about validating and decoding the base64
key. To convert the key back to base64, we don't need a public API in
libnm.
This is another ABI change since 1.16-rc1.
(cherry picked from commit e46ba01867)
That is slightly complex, because we need to (DNS) resolve the endpoints,
and we also have to retry periodically. For example, initially we may be
unable to resolve an endpoint, but later we may be.
What is also interesting is that during assume and reapply, we may not
have all information in the profile. Most notably, the private keys will
be missing. We need to cope with that and not reconfigure keys. However,
we still need to resolve names and update the endpoints.
For now only add the core settings, no peers' data.
To support peers and the allowed-ips of the peers is more complicated
and will be done later. It's more complicated because these are nested
lists (allowed-ips) inside a list (peers). That is quite unusual and to
conveniently support that in D-Bus API, in keyfile format, in libnm,
and nmcli, is a effort.
Also, it's further complicated by the fact that each peer has a secret (the
preshared-key). Thus we probably need secret flags for each peer, which
is a novelty as well (until now we require a fixed set of secrets per
profile that is well known).
