Change the default DNS priority of VPNs to -50, to avoid leaking
queries out of full-tunnel VPNs.
This is a change in behavior. In particular:
- when using dns=default (i.e. no split-dns) before this patch both
VPN and the local name server were added (in this order) to
resolv.conf; the result was that depending on resolv.conf options
and resolver implementation, the name servers were tried in a
certain manner which does not prevent DNS leaks.
With this change, only the VPN name server is added to resolv.conf.
- When using a split-dns plugin (systemd-resolved or dnsmasq), before
this patch the full-tunnel VPN would get all queries except those
ending in a local domain, that would instead be directed to the
local server.
After this patch, the VPN gets all queries.
To revert to the old behavior, set the DNS priority to 50 in the
connection profile.
(cherry picked from commit af13081bec)
With "connection.multi-connect", a profile can be activated multiple
times on a device with `nmcli connection show`. Also, a profile may be
in the process of deactivating on one device, while activating on
another one. So, in general it's possible that `nmcli connection show`
lists the same profile on multiple lines (reflecting their multiple
activation states).
If the user requests no fields that are part of the activation state,
then the active connections are ignored. For example with `nmcli
-f UUID,NAME connection show`. In that case, each profile is listed only
once.
On the other hand, with `nmcli -g UUID,NAME,DEVICE connection show` the
user again requested also to see the activation state, and a profile can
appear multiple times.
To handle that, we need to consider which fields were requested.
There was a bug where the "ACTIVE" field was not treated as part of the
activation state. That results in `nmcli -f UUID,NAME,ACTIVE connection
show` always returning "no". Fix that.
Fixes: a1b25a47b0 ('cli: rework printing of `nmcli connection` for multiple active connections')
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/547https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/642
(cherry picked from commit 4eb3b5b9dd)
Previously, both nm_setting_connection_add_permission() and the GObject
property setter would merely assert that the provided values are valid
(and otherwise don't do anything). That is bad for handling errors.
For example, we use the property setter to initialize the setting from
keyfile and GVariant (D-Bus). That means, if a user provides an invalid
permissions value, we would emit a g_critical() assertion failure, but
otherwise ignore the configuration. What we instead need to do is to
accept the value, and afterwards fail verification. That way, a proper error
message can be generated.
$ mcli connection add type ethernet autoconnect no ifname bogus con-name x connection.permissions 'bogus:'
(process:429514): libnm-CRITICAL **: 12:12:00.359: permission_new: assertion 'strchr (uname, ':') == NULL' failed
(process:429514): libnm-CRITICAL **: 12:12:00.359: nm_setting_connection_add_permission: assertion 'p != NULL' failed
Connection 'x' (2802d117-f84e-44d9-925b-bfe26fd85da1) successfully added.
$ $ nmcli -f connection.permissions connection show x
connection.permissions: --
While at it, also don't track the permissions in a GSList. Tracking one
permission in a GSList requires 3 allocations (one for the user string,
one for the Permission struct, and one for the GSList struct). Instead,
use a GArray. That is still not great, because GArray cannot be embedded
inside NMSettingConnectionPrivate, so tracking one permission also
requires 3 allocations (which is really a fault of GArray). So, GArray
is not better in the common case where there is only one permissions. But even
in the worst case (only one entry), GArray is no worse than GSList.
Also change the API of nm_setting_connection_add_permission().
Previously, the function would assert that the arguments are in
a certain form (strcmp (ptype, "user") == 0), but still document
the such behaviors like regular operation ("[returns] %FALSE if @ptype
or @pitem was invalid"). Don't assert against the function arguments.
Also, if you first set the user to "fo:o", then
nm_setting_connection_add_permission() would accept it -- only at
a later phase, the property setter would assert against such values.
Also, the function would return %FALSE both if the input value was
invalid (an error) and if the value already existed. I think the
function should not treat a duplicate entry like a badly formatted
input.
Now the function does much less asserting of the arguments, but will
return %FALSE only if the values are invalid. And it will silently ignore
duplicate entries.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/636
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>
clang-format will re-format this in multiple lines, use C comment
to not break compilation after applying code-style with clang-format.
Signed-off-by: Antonio Cardace <acardace@redhat.com>
nm_utils_hexstr2bin_full() is our general hexstr to binary parsing
method. It uses (either mandatory or optional) delimiters. Before,
if delimiters are in use, it would accept individual hexdigits.
E.g. "a:b" would be accepted as "0a:0b:.
Add an argument that prevents accepting such single digits.
"active_slave" option is a deprecated alias for "primary". nmtui can configure
the "primary" option, so whenever it configures a profile the "active_slave"
option should be unset.
"active_slave" is by now deprecated and became an alias for "primary".
If a profile specifies both properties, only "primary" is honored, without
failing validation (to not break existing behavior).
Maybe we should introduce a normalization for such cases. But normalize
might not do the right thing, if a profile currently has "primary" set,
and the user modifies it to set "active_slave" to a different value,
normalize would not know which setting was set first and remove
"active_slave" again.
In the past, nm_setting_bond_add_option() performed some simple
normalization, but this was dropped, because (such incompatible) settings
can also be created via the GObject property. Our C accessor function
should not be less flexible than other ways of creating a profile.
In the end, whenever a user (or a tool) creates a profile, the tool must
be aware of the semantics. E.g. setting an IP route without a suitable
IP address is unlike to make sense, the tool must understand what it's
doing. The same is true for the bond options. When a tool (or user) sets
the "active_slave" property, then it must clear out the redundant
information from the "primary" setting. There is no alternative to this
problem than having tools smart enough to understand what they are
doing.
WireGuard's wg-quick primarily wants to avoid DNS leaks, and thus also
our import code should generate profiles that configure exclusive DNS
servers. This is done by setting "ipv[46].dns-priority" to a negative
value.
Note that if a profile leaves the DNS priority at zero (which in many
regard is the default), then the zero translates to 50 (for VPN
profiles) and 100 (for other profiles).
Instead of setting the DNS priority to -10, set it to -50. This gives
some more room so that the user can choose priorities that are worse
than the WireGuard's one, but still negative (exclusive). Also, since
the positive range defaults to 50 and 100, let's stretch the range a
bit.
Since this only affects import and creation of new profiles, such a
change in behavior seems acceptable.
Use a macro that uses NM_CAST_STRV_CC() to cast the strv argument. Note that
NM_CAST_STRV_CC() uses C11's _Generic() to check whether the argument is
of a valid type.
Seems with LTO the compiler can sometimes think that thes variables are
uninitialized. Usually those code paths are only after an assertion was
hit (g_return*()), but we still need to workaround the warning.
A profile can configure "connection.wait-device-timeout" to indicate
that startup complete is blocked until a suitable device around.
This is useful for NetworkManager-wait-online and initrd mode.
Previously, we looked at NMPlatform whether a link with matching
interface-name was present. That is wrong because it cannot handle
profiles that rely on "ethernet.mac-address" setting or other "match"
settings. Also, the mere presence of the link does not yet mean
that the NMDevice was created and ready. In fact, there is a race here:
NMPlatform indicates that the device is ready (unblocking NMSettings),
but there is no corresponding NMDevice yet which keeps NetworkManager
busy to block startup complete.
Rework this. Now, only check whether there is a compatible device for
the profile.
Since we wait for compatible devices, it works now not only for the
interface name. Note that we do some optimizations so that we don't have
to re-evaluate all profiles (w.r.t. all devices) whenever something on the
device changes: we only care about this when all devices finally become
ready.
Also, we no longer start the timeout for "connection.wait-device-timeout"
when the profile appears. Instead, there is one system-wide start time
(NMSettingsPrivate.startup_complete_start_timestamp_msec). That simplifies
code and makes sense: we start waiting when NetworkManager is starting, not
when the profile gets added. Also, we wait for all profiles to become
ready together.
Make the error handling similar to the other provider implementations.
- only actually return once all callbacks completed.
- cache the first error and report it.
- drop AzureData.success field. It is redundant to have AzureData.error set.
Also it was actually unused.
- ensure that we keep the first error passed during
_get_config_maybe_task_return(). Once we set an error, that error gets
returned. There is a twist here, that we prefer cancellation errors
over other error reasons.
- drop GCPData.success field. It is redundant to have GCPData.error set.
Also, it's meaningless to indicate failure, if we don't have an error
at hand.
- ensure that we keep the first error passed during
_get_config_maybe_task_return(). Once we set an error, that error gets
returned. There is a twist here, that we prefer cancellation errors
over other error reasons.
- in _get_config_fip_cb(), ensure to call _get_config_maybe_task_return()
even if we are not yet ready. That is useful to record a potential
error.
If the list of addresses, routes and rules is empty, we still want to mangle
the applied connection, to also have an empty list.
nm-cloud-setup has certain expectations. For example, that the static addresses,
routes and rules of the active connection is entirely under the control of the
tool. For example, so it usually replaces the lists entirely. It also should do
that, if the new list is empty.
Maybe, one day there could be more complex merging strategies, where the user could
also add static addresses, routes, or rules to the profile, and nm-cloud-setup
would preserve them. However, that is not implemented, nor is it clear how exactly
that would work.
Without this, `nmcli device modify "$DEVICE"` leads to a crash. At least
since commit c5d45848dd ('cli: mark argv argument for command line
parsing as const'), when this happens.
That is, because it passes a NULL strv array with argc being set to
zero. nmc_process_connection_properties() is not supposed to access
the array, if there are no elements there.
Fixes: c5d45848dd ('cli: mark argv argument for command line parsing as const')
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/492
When configuring miimon settings, the updelay/downdelay fields with
value zero may not be stored in the setting.
For example:
- have a profile with "mode=balance-rr,arp_interval=11,arp_ip_target=10.10.10.1,miimon=10"
Switch the link monitoring mode to "MII" and press <OK>. Previously,
the change of the link monitoring did not update the settings, and
nothing was changed.
- when loading settings, initialize all fields with the values from the
settings, regardless whether they are currently visible or not.
Otherwise, if you edit a profile with
"mode=balance-rr,arp_interval=11,arp_ip_target=10.10.10.1,miimon=10"
and switch link monitoring mode to "MII", the miimon setting was not
initialized to 10.
- accept empty bond settings, for example for updelay. In that case,
initialize the text input to "0". Likewise, when the text entry is
empty, set the bond option to the respective default.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/488
Before 1.24, nm_setting_bond_add_option() would clear
miimon/arp_interval settings when the respective other was set.
That was no longer done, with the effect that enabling (for example)
miimon on a bond profile that has arp_interval enabled, sets both
conflicting options.
That is not a severe problem, because the profile still validates.
However, at runtime only one of the settings can be actually configured.
Fix that, by restoring the previous behavior for the client. But note
that this time it's implemented in the client, and not in libnm's
nm_setting_bond_add_option().
$ nmcli -f GENERAL.HWADDR device show ovsport0
GENERAL.HWADDR: (unknown)
but:
$ nmcli -f GENERAL.HWADDR --terse device show ovsport0
GENERAL.HWADDR:
This is an API change of nmcli.
nm_http_client_poll_get_finish() can only either succeed (returning TRUE
and setting no GError), or failing (returning FALSE and setting GError).
Checking for both is redundant and unnecessary.
