Curl's CURLOPT_RESOLVE expects one list entry per host. That
documentation ([1]) also makes that clear that the form is
"[+]HOST:PORT:ADDRESS[,ADDRESS]".
The way we constructed the list, only the last entry was honored:
<trace> [1647551393.5362] connectivity: (eth0,IPv4,25) adding 'fedoraproject.org:80:18.159.254.57' to curl resolve list
<trace> [1647551393.5363] connectivity: (eth0,IPv4,25) adding 'fedoraproject.org:80:152.19.134.142' to curl resolve list
<trace> [1647551393.5363] connectivity: (eth0,IPv4,25) adding 'fedoraproject.org:80:18.192.40.85' to curl resolve list
...
<trace> [1647551393.5366] connectivity: (eth0,IPv4,25) adding 'fedoraproject.org:80:85.236.55.6' to curl resolve list
<trace> [1647551393.5366] connectivity: (eth0,IPv4,25) adding 'fedoraproject.org:80:38.145.60.20' to curl resolve list
...
<trace> [1647551393.5415] connectivity: (eth0,IPv4,25) libcurl: == Info: Added fedoraproject.org:80:18.159.254.57 to DNS cache\012
<trace> [1647551393.5416] connectivity: (eth0,IPv4,25) libcurl: == Info: RESOLVE fedoraproject.org:80 is - old addresses discarded!\012
<trace> [1647551393.5416] connectivity: (eth0,IPv4,25) libcurl: == Info: Added fedoraproject.org:80:152.19.134.142 to DNS cache\012
<trace> [1647551393.5417] connectivity: (eth0,IPv4,25) libcurl: == Info: RESOLVE fedoraproject.org:80 is - old addresses discarded!\012
...
<trace> [1647551393.5422] connectivity: (eth0,IPv4,25) libcurl: == Info: RESOLVE fedoraproject.org:80 is - old addresses discarded!\012
<trace> [1647551393.5423] connectivity: (eth0,IPv4,25) libcurl: == Info: Added fedoraproject.org:80:38.145.60.20 to DNS cache\012
<trace> [1647551393.5424] connectivity: (eth0,IPv4,25) libcurl: == Info: Hostname fedoraproject.org was found in DNS cache\012
<trace> [1647551393.5424] connectivity: (eth0,IPv4,25) libcurl: == Info: Trying 38.145.60.20:80...\012
There are two possible fixes. Either join all addresses in one
entry, or use the '+' modifier. Do the former.
Now we get:
<trace> [1647551967.0378] connectivity: (eth0,IPv4,25) set curl resolve list to 'fedoraproject.org:80:38.145.60.21,152.19.134.142,152...
...
<trace> [1647551967.0559] connectivity: (eth0,IPv4,25) libcurl: == Info: Added fedoraproject.org:80:38.145.60.21,152.19.134.142,152.1...
<trace> [1647551967.0560] connectivity: (eth0,IPv4,25) libcurl: == Info: Hostname fedoraproject.org was found in DNS cache\012
<trace> [1647551967.0561] connectivity: (eth0,IPv4,25) libcurl: == Info: Trying 38.145.60.21:80...\012
[1] https://curl.se/libcurl/c/CURLOPT_RESOLVE.html
Reported-by: Bastien Nocera <hadess@hadess.net>
Fixes: 2cec94bacc ('connectivity: use systemd-resolved for resolving the check endpoint')
This allows us to reject activation of WEP profiles very early,
also providing a reasonable error code to the client:
$ nmcli d wifi connect test
Error: Failed to add/activate new connection: wpa_supplicant does not support WEP encryption
Since version 2.10, it's possible to build wpa_supplicant without WEP
support. In fact, it's disabled by default. Regrettably, there's no
indication in that version as to whether WEP is enabled or not.
A patch has been sent upstream that exposes the information on D-Bus:
https://patchwork.ozlabs.org/project/hostap/patch/20220307085446.706024-1-lkundrak@v3.sk/
This makes use of the above to indicate presence or absence of WEP
support.
I think we should move away from using the source-ids.
Having a "GSource*" pointer makes it clearer what this is, compared to a
guint source ID. Also, g_source_remove() always needs to first do a hash
lookup (with locking) to resolve the source ID to the GSource. This is
unnecessary.
I got a report of a scenario where multiple servers reply to a REQUEST
in SELECTING, and all servers send NAKs except the one which sent the
offer, which replies with a ACK. In that scenario, n-dhcp4 is not able
to obtain a lease because it restarts from INIT as soon as the first
NAK is received. For comparison, dhclient can get a lease because it
ignores all NAKs in SELECTING.
Arguably, the network is misconfigured there, but it would be great if
n-dhcp4 could still work in such scenario.
According to RFC 2131, ACK and NAK messages from server must contain a
server-id option. The RFC doesn't explicitly say that the client
should check the option, but I think it's a reasonable thing to do, at
least for NAKs.
This patch stores the server-id of the REQUEST in SELECTING, and
compares it with the server-id from NAKs, to discard other servers'
replies.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1144
When a NMDevice is involved in a PPPoE activation, it means that the
connection has connection.interface-name=<ethernet-interface>. In such
case, the ppp ifindex should be set as ip-ifindex of the ethernet
device.
Fixes: 58287cbcc0 ('core: rework IP configuration in NetworkManager using layer 3 configuration')
This is needed to get "/usr/share/gettext/its/polkit.its",
otherwise msgfmt will fail on Debian:
/usr/bin/msgfmt: cannot locate ITS rules for data/org.freedesktop.NetworkManager.policy.in
Clang 14 has a new warning "-Wbitwise-instead-of-logical", and it warns
about our usage with NM_IN_SET_SE()/NM_IN_STRSET_SE(). It complains that we
are using '|' with boolean operands. Which is true (and intended), as we bitwise-or
the result of the '==' comparisons.
Work around the warning by casting the operands to "int". Note that
in C, the comparison operators have already a type "int", so this cast
should not result in any changes in the compiled code.
../src/libnm-core-impl/tests/test-general.c:9415:17: error: use of bitwise '|' with boolean operands [-Werror,-Wbitwise-instead-of-logical]
_ASSERT(2, !NM_IN_SET_SE(-1, G(1), G(2)));
~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../src/libnm-std-aux/nm-std-aux.h:800:30: note: expanded from macro 'NM_IN_SET_SE'
#define NM_IN_SET_SE(x, ...) _NM_IN_SET(|, typeof(x), x, __VA_ARGS__)
^
../src/libnm-std-aux/nm-std-aux.h:789:39: note: expanded from macro '_NM_IN_SET'
!!(NM_VA_ARGS_FOREACH(, , op, _NM_IN_SET_OP, __VA_ARGS__)); \
^
../src/libnm-std-aux/nm-std-aux.h:772:20: note: expanded from macro 'NM_VA_ARGS_FOREACH'
op, \
^
note: (skipping 7 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)
../src/libnm-glib-aux/nm-macros-internal.h:1603:47: note: expanded from macro '_G_BOOLEAN_EXPR'
#define _G_BOOLEAN_EXPR(expr) NM_BOOLEAN_EXPR(expr)
~~~~~~~~~~~~~~~~^~~~~
../src/libnm-std-aux/nm-std-aux.h:167:62: note: expanded from macro 'NM_BOOLEAN_EXPR'
#define NM_BOOLEAN_EXPR(expr) _NM_BOOLEAN_EXPR_IMPL(NM_UNIQ, expr)
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~
../src/libnm-std-aux/nm-std-aux.h:161:13: note: expanded from macro '_NM_BOOLEAN_EXPR_IMPL'
if (expr) \
^~~~
../src/libnm-core-impl/tests/test-general.c:9415:17: note: cast one or both operands to int to silence this warning
../src/libnm-std-aux/nm-std-aux.h:800:30: note: expanded from macro 'NM_IN_SET_SE'
#define NM_IN_SET_SE(x, ...) _NM_IN_SET(|, typeof(x), x, __VA_ARGS__)
^
../src/libnm-std-aux/nm-std-aux.h:789:39: note: expanded from macro '_NM_IN_SET'
!!(NM_VA_ARGS_FOREACH(, , op, _NM_IN_SET_OP, __VA_ARGS__)); \
^
Frequencies with the 'disabled' flag are supported by the driver but
disabled in the current regulatory domain. Don't add them to the list
of supported frequencies since they are not usable.
This is especially needed since commit f18bf17dea ('wifi: cleanup
ensure_hotspot_frequency()'), as now NetworkManager explicitly sets a
random, stable channel for Wi-Fi hotspots. If the choosen channel is
disabled, the hotspot fails to start.
Disabled channels are displayed in the 'iw phy' output as '(disabled)':
[...]
Frequencies:
* 2412 MHz [1] (30.0 dBm)
* 2417 MHz [2] (30.0 dBm)
* 2422 MHz [3] (30.0 dBm)
* 2427 MHz [4] (30.0 dBm)
* 2432 MHz [5] (30.0 dBm)
* 2437 MHz [6] (30.0 dBm)
* 2442 MHz [7] (30.0 dBm)
* 2447 MHz [8] (30.0 dBm)
* 2452 MHz [9] (30.0 dBm)
* 2457 MHz [10] (30.0 dBm)
* 2462 MHz [11] (30.0 dBm)
* 2467 MHz [12] (disabled)
* 2472 MHz [13] (disabled)
* 2484 MHz [14] (disabled)
Note that currently NM loads the list only at startup and therefore,
in case of a change of regulatory domain, a restart of the daemon is
needed to have the list updated. This needs to be improved.
https://bugzilla.redhat.com/show_bug.cgi?id=2062785
Fixes: f18bf17dea ('wifi: cleanup ensure_hotspot_frequency()')
This makes it more obvious in client when a feature that will not work
(now or in future) is being used. The motivation is to phase out WEP nicely.
To keep this simple this does not impose any policy decisions, nor is
configurable (contrary to what i've suggested before).
The policy on whether a connection activation will succeed or not is left
to the daemon. For WEP the idea is to delegate the decision further to the
supplicant while also provide a reasonable error handling [1].
[1] https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1138
The lack of configurability is due to the fact that WEP is going away
everywhere, regardless of whether it is enabled at the moment (Fedora 36)
or not (RHEL 9).
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1139https://bugzilla.redhat.com/show_bug.cgi?id=2030997
We sometimes emit warnings after a connection is added. Currently
there's a warning when the connection ID collides with another one (and
a suggestion to use an UUID instead).
Let's move the check into a separate routine, so that we can reuse it
elsewhere, such as on connection "modify" (in a following commit).
Check if a connection uses something that is likely not to work --
either now or in future.
The ultimate decision on whether it's going to work is up to the daemon.
We just use the result to color the connection differently to provide
slight visual cue to the user.
Follow-up commits are going color Wi-Fi networks and connections that rely
on deprecated features differently, to provide a visual cue.
Add color definitions for those.
We have nm_device_master_add_slave(). This should be mirrored by
nm_device_master_release_slave() (not release-one-slave).
Thereby, also rename nm_device_master_release_slaves() to
nm_device_master_release_slaves_all() to make it clearer.
I find the two (dependent) booleans "configure" and "force" confusing.
nm_device_master_release_one_slave() has many callers, it's interesting
to be able to grep for the release-type. Add an enum to make this more
readable.
This makes the non-obvious fact clearer, that when you look up an object
by an untrusted, user-provided path, it might not be the object type you
are looking for. In basically all cases, you need to check that the
result is of the expected type. This helper makes that clearer.
We often create the source with default priority, no destroy function and
attach it to the default context (g_main_context_default()). For that
case, we have wrapper functions like nm_g_timeout_add_source()
and nm_g_idle_add_source(). Use those.
There should be no change in behavior.
g_idle_add() uses G_PRIORITY_DEFAULT_IDLE priority. Most of the time we don't
care much about the priority.
But at the places that this patch changes, I think that using
G_PRIORITY_DEFAULT_IDLE (and following g_idle_add()) is more correct. The
reason for this is not very strong, except that it's probably the better
choice. And the old choice was made because I didn't realize that
g_idle_add() uses another default priority. Hence, the old choice was not
for good reasons either.
