Conceptionally, the MUD URL really depends on the device, and not so
much the connection profile. That is, when you have a specific IoT
device, then this device probably should use the same MUD URL for all
profiles (at least by default).
We already have a mechanism for that: global connection defaults. Use
that. This allows a vendor drop pre-install a file
"/usr/lib/NetworkManager/conf.d/10-mud-url.conf" with
[connection-10-mud-url]
connection.mud-url=https://example.com
Note that we introduce the special "connection.mud-url" value "none", to
indicate not to use a MUD URL (but also not to consult the global connection
default).
For one, the setters sd_dhcp_client_set_mud_url() and sd_dhcp6_client_set_request_mud_url()
assert that the value honors these settings. So, we must never pass such values to the
function. Also, before calling n_dhcp4_client_probe_config_append_option()
the code doesn't check whether the URL is short enough. That would be
a bug (unless we ensure that the property is valid from the beginning).
In general, it is necessary to strictly validate the parameter.
Also, returning NM_SETTING_VERIFY_NORMALIZABLE_ERROR for a property that does
not get normalized is a bug.
Since commit c1907a218a ('libnm-core: remove gateway when
never-default=yes in NMSettingIPConfig'), the gateway gets normalized
away when the profile has never-default set.
That means,
$ nmcli connection modify "$PROFILE" ipv4.never-default yes ipv4.gateway 192.168.77.1
does not set the gateway. Likewise, if your profile has already never-default
enabled,
$ nmcli connection modify "$PROFILE" ipv4.gateway 192.168.77.1
will have no effect. That is confusing and undesirable.
Note that we don't adjust the GObject property setter for "gateway" to clear
never-default. I feel, setting one property in libnm should preferably
not unset another (there are exceptions to the rule, like for team
properties). However, for nmcli it's clear in which order properties
are set, so this change is right for the client tool.
https://bugzilla.redhat.com/show_bug.cgi?id=1785039https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/475
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.
Hide the object and class structures from public API.
This is an API and ABI break, but of something that is very likely
unused.
This is mainly done to embed the private structure in the object itself.
This has benefits for performance and debugability.
We should generate the GVariant in a stable manner. That implies
to sort the keys first.
Also, don't use the NM_SETTING_VPN_SECRETS getter, which first needs
to clone all secrets.
When we use _nm_utils_strdict_from_dbus(), we clone the secrets, but don't use
nm_free_secret() for freeing them.
And in fact, we clone the setings twice. It't really not too nice. Implement
this without the property setter.
Don't use _nm_utils_copy_strdict().
On a minor note, that function will always allocate a GHashTable, even if
it only contains "" as only key. Later we would throw that out again,
so it was unnecessary.
Worse, using _nm_utils_copy_strdict() does not use nm_free_secrets as
destroy function. While it's in general difficult to clear all places
in memory where we copy the secrets around, we can easily avoid that.
Also skip over %NULL keys and values. It probably would be a bug passing
such arguments to the property. Better ignore them and not crash.
Like for data, now also allow empty secrets to be added to the VPN
setting.
For one, this avoids an assertion failure, where keyfile reader wouldn't
check whether a secret key is set to the empty word.
For data, it's more clear that we want to allow setting empty data
values. VPN settings are only interpreted by the VPN plugin, so libnm
and the daemon shouldn't prevent empty settings. It can be useful to
distinguish between unset (NULL) and empty values.
For secrets, it's less clear that empty secrets shall be allowed. I
think it should. Of course, the empty secret likely isn't a correct
nor valid secret. But libnm cannot validate the secrets anyway. It's
up to the VPN plugin to handle this in any way they see fit.
Also, already before, the user could set NM_SETTING_VPN_SECRETS to
a string dictionary with empty passwords. So, the API didn't fully
prevent that. Only certain API wouldn't play along.
Until now, nm_setting_vpn_add_data_item() would reject empty data values.
This leads for example to an assertion failure, if you write a keyfile
that assigns an empty value to a key. Keyfile reader would not check that
the value is non-empty before calling nm_setting_vpn_add_data_item().
Anyway, I think we should not require having non-empty data elements. It's
an unnecessary and sometimes harmful restriction. NetworkManager doesn't understand
not care about the content of the vpn data. That is up the VPN plugins. Sometimes
and empty value may be desirable.
Also, the NM_SETTING_VPN_DATA property setter wouldn't filter out empty
values either. So it was always possible to use some libnm API to set data
with empty values. The restriction in nm_setting_vpn_add_data_item() was
inconsistent.
Also drop the g_warn_if_fail() from update_secret_dict(). We
may get the variant from D-Bus, so avoiding this assertion (g_warn*() is
an assertion!) would require us to prevalidate the variant.
That would be very cumbersome, and we would probably not want to
handle that as an error and silently ignore them anyway. Just shut
up.
Ensure that the data hash doesn't contain keys with empty key-name.
It just doesn't make sense, and will lead to potential problems later,
if we would allow this to happen.
For example, keyfile writer may naively try to set all keys, without
checking for empty keys. That may lead to assertion failures or worse,
later on. Don't allow that.
Also, assert against non-empty keys in nm_setting_vpn_get_data_item()
and nm_setting_vpn_remove_data_item(). This stricter handling is a
change in behavior, however it would have always been a bug trying to
refer to empty key names. So, this assertion will only help to find
those bugs.
We always initialized priv->data in nm_setting_vpn_init(), but usually
soon after this hash would be replaced by NM_SETTING_VPN_DATA property
setter.
Avoid that. Allow for priv->data to be %NULL, which of course has the
meaning that no keys are set.
When we invoke the user's callback, be more robust about the user
modifying or unreferencing the NMSettingVpn, while iterating.
While it would be odd to modify the NMSettingVpn from inside the foreach
callback, we should behave consistently and sensibly.
That means, to ensure that the NMSettingVpn instance stays alive all
the time, that we don't crash, and that we always iterate over the
previously determined, planned list of keys.
Also, avoid some unnecessary string copies, like the clone of the first key.
For one, this code was unreachable, because we checked these conditions
already before.
That aside, g_warn_*() is for all intended purposes an assertion.
The caller probably gets the GVariant from an untrusted source (e.g. via
D-Bus). Asserting here is not helpful.
It's up to the caller to validate the argument. Or, in case the caller
doesn't care, update_secret_dict() should just do the sensible thing.
But be silent about it!
Yes, the compiler probably can optimize strlen() in these cases. That
still doesn't make strlen() the right API to check whether a NUL
terminated string is empty.
Sometimes these function may set errno to unexpected values like EAGAIN.
This causes confusion. Avoid that by using our own wrappers that retry
in that case. For example, in rhbz#1797915 we have failures like:
errno = 0;
v = g_ascii_strtoll ("10", 0, &end);
if (errno != 0)
g_assert_not_reached ();
as g_ascii_strtoll() would return 10, but also set errno to EAGAIN.
Work around that by using wrapper functions that retry. This certainly
should be fixed in glib (or glibc), but the issues are severe enough to
warrant a workaround.
Note that our workarounds are very defensive. We only retry 2 times, if
we get an unexpected errno value. This is in the hope to recover from
a spurious EAGAIN. It won't recover from other errors.
https://bugzilla.redhat.com/show_bug.cgi?id=1797915
NMTST_SWAP() used memcpy() for copying the value, while NM_SWAP() uses
a temporary variable with typeof(). I think the latter is preferable.
Also, the macro is essentially doing the same thing.
