For historic reasons is NMSettingBond implemented differently from other
settings. It uses a strdict, and adds some validation on top of that.
The idea was probably to be able to treat bond options more generically.
But in practice we cannot treat them as opaque values, but need to know,
validate and understand all the options. Thus, this implementation with a
strdict is not nice.
The user can set the GObject property NM_SETTING_BOND_OPTIONS to any
strdict, and the setter performs no validation or normalization. That
is probably good, because g_object_set() cannot return an error to
signalize invalid settings. As often, we have corresponding C API like
nm_setting_bond_add_option() and nm_setting_bond_remove_option(). It
should be possible to get the same result both with the C API and with
the GObject property setting. Since there is already a way to set
certain invalid values, it does not help if the C API tries to prevent
that. That implies, that also add-option does not perform additional
validation and sets whatever the user asks.
Remove all validation from nm_setting_bond_add_option() and
nm_setting_bond_remove_option(). This validation was anyway only very
basic. It was calling nm_setting_bond_validate_option(), which can check
whether the string is (for example) and integer, but it cannot do
validation beyond one option. In most cases, the validation needs to
take into account the bond mode or other options, so validating one
option in isolation is not very useful.
Proper validation should instead be done via nm_connection_verify().
However, due to another historic oddity, that verification is very
forgiving too and doesn't reject many invalid settings when it should.
That is hard to fix, because making validation more strict can break
existing (and working) configurations. However, verify() already contains
basic validation via nm_setting_bond_validate_option(). So in the previous
behavior nm_setting_bond_add_option() would silently do nothing (only
returning %FALSE) for invalid options, while now it would add the
invalid options to the dictionary -- only to have it later fail validation
during nm_connection_verify(). That is a slight change in behavior, however it
seems preferable.
It seems preferable and acceptable because most users that call
nm_setting_bond_add_option() already understand the meaning and valid
values. Keyfile and ifcfg-rh readers are the few exceptions, which really just
parse a string dictionary, without need to understand them. But nmtui
or nmstate already know the option they want to set. They don't expect
a failure there, nor do they need the validation.
Note that this change in behavior could be dangerous for example for the
keyfile/ifcfg-rh readers, which silently ignored errors before. We
don't want them to start failing if they read invalid options from a
file, so instead let those callers explicitly pre-validate the value
and log an warning.
https://bugzilla.redhat.com/show_bug.cgi?id=1887523
If the target hidden network is already recorded by IWD with its SSID
during a previous active scan, use the Network.Connect() API instead of
Station.ConnectHiddenNetwork() which would fail in IWD version up to
1.9. This is a rare corner case scenario though.
Also drop the !nm_wifi_ap_get_supplicant_path(ap) check, I'm not
sure when if ever that condition can be true, more so now that we're
checking nm_wifi_ap_get_fake(ap) before that.
Until now we didn't rely on InterfacesAdded and InterfacesRemoved
signals for tracking when IWD finds new Wi-Fi networks or expires
networks not seen in the latest scans. Instead we'd request the whole
list of networks currently seen by IWD every time the Station.Scanning
property would go from true to false. However the
Station.GetOrderedNetworks() IWD method that we use has a deficiency
up until 1.9 (I plan to fix it soon) where it won't show the hidden
network discovered in the course of the last ConnectHiddenNetwork() call
if that call was unsuccessful, in other words where the new network has
not been saved as a Known Network. A new ConnectHiddenNetwork() will
fail with the "NotHidden" error, so we have to use the Network.Connect()
call for such a network but to find it out we need to track the
InterfacesAdded signals. Doing this may also improve autoconnect speed
in some cases so overall I think it's a good idea.
When IWD asks us for a secret check that we're in NM_DEVICE_STATE_CONFIG
and not for example already in NM_DEVICE_STATE_NEED_AUTH. I believe that
should only happen if IWD is aborting the previous connection attempt and
connecting to a different network due to a timeout or due to somebody
outside NM calling Connect() on an IWD network object...
Guessing what IWD is doing this way is a bit fragile in the long term
but we have to do that as long as we want to override IWD's internal
autoconnect, which I guess we may be able to stop doing at some point.
IWD's Station.State property remains at "connect" or "disconnected"
while IWD is waiting for secrets for a new conncetion, so if we want to
scan only when NM might be in auto-connect (which was the goal) we need
to also look at NMDevice's state. We want to scan whenever wifi is
disconnected and there's no active connection request, which is the same
as saying whever priv->current_ap is unset so for simplicity look at
priv->current_ap. Also in schedule_periodic_scan() don't check whether
Station.State is "disconnected" because priv->can_scan is equivalent to
Station.State being one of ("disconnected", "connected").
Hidden networks are supported in the iwd backend since 1.24.0 but some
places in the code have not been updated to reflect this.
In check_connection_available copy the hidden network check and
corresponding comment from the wpa_supplicant backend. In
act_stage1_prepare drop a straight "hidden networks are unsupported"
comment and a check -- fortunately this check happened to be ineffective
because @mode was more often NULL than NM_SETTING_WIRELESS_MODE_INFRA so
nm_streq0 was not enough. Update comments elsewhere.
There's still one of two corner cases where the user-experience will not
be perfect for hidden networks due to iwd limitations, I'll try to work
around them in another commit.
I first noticed a format string with missing parameters and then that
the compiler wasn't complaining and that's because
nm_utils_error_set_literal doesn't take a format string.
When a connection fails and IWD returns net.connman.iwd.Aborted, we know
whether the abort is caused by us cancelling a secrets request so use
tha knowledge to decide whether to use the NM_DEVICE_STATE_REASON_NO_SECRETS
reason code for the state switch to NM_DEVICE_STATE_FAILED.
Use the NM_UTILS_ERROR_CONNECTION_AVAILABLE_INCOMPATIBLE constant in
place of NM_UTILS_ERROR_CONNECTION_AVAILABLE_TEMPORARY more often in
check_connection_compatible as appropriate.
Heavily rework NML3Cfg's ACD handling.
- the (user facing) API changed, so that we can ask the current ACD
state of an address with nm_l3cfg_get_acd_addr_info(). So, the
acd-event signal is only to notify when the state changes, it does
not carry information that you couldn't fetch anytime.
- add clearer ACD states (NML3AcdAddrState). The current (ACD) state
of an address is important and becomes part of the information that
we expose.
- add new ACD state "USED", when ACD fails. This blocks the address from
being used. Usually the caller would either remove the (used) address
or force reconfigure it (by setting acd_timeout_msec to zero).
- add new ACD state "CONFLICT". Previously conflicts were not handled.
Now the API allows to specify the defend policy. A conflicted address
also gets blocked from being used.
- add new ACD state "EXTERNAL_REMOVED". This happens when we have an
address we wanted to configure, but then the address is no longer
on the interface. For example because the user removed it from the
interface. This also leaves the device indefinitely blocked, and
is important to stop announcing the address.
- add a new ACD state "READY". This indicates that the address is ready
to be configured, but not yet actually configured on the device. This
is the step before "DEFENDING".
ACD is handled by NML3Cfg and it intercepts the IP addresses when
merging the NML3ConfigData.
Originally, I thought that in such a case, the merged l3cd instance
would simply not contain any addresses that ACD have still pending or
which have a conflict.
However, I think it's better (clearer and possibly useful), to still
merge such addresses, but flag them that they are ignored when syncing
the addresses to platform.
It is not yet used, but it will be used to mark instances that
are not supposed to be configured in platform, because ACD is
either still pending of failed.
When a wifi device is in a bridge, the supplicant must be aware of it,
as a socket must be opened on the bridge to receive packets.
Set the BridgeIfname property of the supplicant Interface object
before starting the association. Note that the property was read-only
in the past and recently [1] became read-write. When using a
supplicant version without the patch, writing the property will return
an InvalidArgs error and NetworkManager will print a warning.
[1] https://w1.fi/cgit/hostap/commit/?id=1c58317f56e312576b6872440f125f794e45f991https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/83
The underscore somehow indicated that these would be an internal
function. Which they are in the sense that they are in "shared/nm-glib-aux/".
But they part of our internal helper functions, and in our code base
their use is no discouraged or "private.
Also, next I'll replace the function call with a macro, so, I will
have a need for the underscore name.
Rename.
This is potentially a breaking change, formerly speciyfing 'none|off'
in the kernel cmdline option 'ip' was understood by the dracut
network-module as doing 'ipv6.method=auto' which is clearly incosistent
with the 'off' naming, thus 'off|none' now means to actually disable
both ipv6 and ipv4 (unless a static ip is provided).
Unit test added.
https://bugzilla.redhat.com/show_bug.cgi?id=1883958
Reverts: 440a0b4078 ('initrd: set ipv6.method=auto when the autoconfiguration field is 'none'')
Signed-off-by: Antonio Cardace <acardace@redhat.com>
- use a guint64 variable to avoid wrapping the counter
- cache the used ID in NMDevice. This way, the same NMDevice
instance will get the same UDI path when it realizes
and unrealizes multiple times.
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.
If a VPN has never-default=no but doesn't get a default route (this
can happen for example when the server pushes routes with
openconnect), and there are no search domains, then the name servers
pushed by the server would be unused. It is preferable in this case to
use the VPN DNS server for all queries.
https://bugzilla.redhat.com/show_bug.cgi?id=1863041
The test spawns processes and tries to kill them, with timeouts and retry.
That is inherently racy, and it's hard to deterministically test the
interesting cases, without having unstable tests.
Try to adjust the timeout, to make it more stable:
14:02:27 /builds/NetworkManager/NetworkManager/tools/run-nm-test.sh --called-from-make /builds/NetworkManager/NetworkManager/build --launch-dbus=auto /builds/NetworkManager/NetworkManager/build/src/tests/test-core-with-expect
--- stdout ---
# random seed: R02S7748fae8fc946b7a755b72efb5815250
1..5
# Start of general tests
ok 1 /general/nm_utils_monotonic_timestamp_as_boottime
# NetworkManager-DEBUG: <debug> [1601992953.4091] kill child process 'test-s-1-3' (18615): sending SIGKILL...
# NetworkManager-DEBUG: <debug> [1601992953.4242] kill child process 'test-s-1-3' (18615): waiting for process to terminate after sending no signal (0) and SIGKILL...
# NetworkManager-DEBUG: <debug> [1601992953.4257] kill child process 'test-s-1-3' (18615): after sending no signal (0) and SIGKILL, process 18615 exited by signal 9 (20807 usec elapsed)
Bail out! GLib:ERROR:../src/tests/test-core-with-expect.c:154:test_nm_utils_kill_child_sync_do: Did not see expected message NetworkManager-DEBUG: *<debug> [*] kill child process 'test-s-1-3' (*): waiting up to 1 milliseconds for process to terminate normally after sending no signal (0)...
Bail out! test:ERROR:../src/tests/test-core-with-expect.c:457:test_nm_utils_kill_child: assertion failed (exit_status == 0): (6 == 0)
--- stderr ---
**
GLib:ERROR:../src/tests/test-core-with-expect.c:154:test_nm_utils_kill_child_sync_do: Did not see expected message NetworkManager-DEBUG: *<debug> [*] kill child process 'test-s-1-3' (*): waiting up to 1 milliseconds for process to terminate normally after sending no signal (0)...
**
test:ERROR:../src/tests/test-core-with-expect.c:457:test_nm_utils_kill_child: assertion failed (exit_status == 0): (6 == 0)
/builds/NetworkManager/NetworkManager/tools/run-nm-test.sh: line 279: 18325 Aborted "${NMTST_DBUS_RUN_SESSION[@]}" "${NMTST_LIBTOOL[@]}" "$NMTST_VALGRIND" --quiet --error-exitcode=$VALGRIND_ERROR --leak-check=full --gen-suppressions=all "${NMTST_SUPPRESSIONS[@]}" --num-callers=100 --log-file="$LOGFILE" "$TEST" "$@"
GDBusObjectManagerClient's interface-added and interface-removed signals
are not emitted when the new interfaces are added to a completely new
object or the removal results in the object disappearing. In other
words one interface is never reported both through interface-added and
object-added (or -removed) signals. This kind of makes sense but isn't
documented explicitly so interface-added seemed to correspond to DBus
InterfacesAdded signals which it doesn't.
We need to watch for both kinds of signals and although most things
work without us receiving the signals at all, it causes some race
conditions. For example on hotplug, devices wouldn't transition to
"disconnected" if a device was discovered by NMManager before it
appeared on IWD's dbus interface because that scenario relied on the
dbus signal.
The automatic scanning every 20 seconds while connected has been
annoying users because of the extra connection latency, drop it. The
UIs are supposed to be requesting scans whenever an AP list update is
needed (?).
Fix a crash on device unplugging caused by keeping our signal handlers
for GDBusProxies connected after a call to dispose(). Do this by
replacing most cleanup steps by a nm_device_iwd_set_dbus_object(self, NULL)
call which is more meticulous.
