NM didn't support wpa-none for years because kernel drivers used to be
broken. Note that it wasn't even possible to *add* a connection with
wpa-none because it was rejected in nm_settings_add_connection_dbus().
Given that wpa-none is also deprecated in wpa_supplicant and is
considered insecure, drop altogether any reference to it.
This file causes a crash [1], add it to the tests.
Note that the test only check parsing the file and the
crash happens in the "upper" layers. So, it's not really
a test for the crash. But at least have such a file in
our repository.
[1] https://gitlab.freedesktop.org/NetworkManager/NetworkManager/issues/235
Logging pointer values might reveal information that can be used to defeat
ASLR. We should avoid that.
On the other hand, it's useful to tag a logging message with the pointer
value of the "source" of the message. It helps to correlate messages and
search for relevant messages in the log.
As a compromise, use NM_HASH_OBFUSCATE_PTR(), like we do at several places
already. For example, we also log
<debug> [1566550899.7901] setup NMPlatform singleton (29a6af9867f2e5d0)
This obfuscated value is a 64 bit unsigned integer with the siphash24
hash of the raw value with a randomized seed. Of course, contrary to the
pointer value, there is a tiny chance that two different pointers hash
to the same identifier. However, that seems unlikely enough to be of no
concern. Note that this pointer value is only logged to aid debugging.
It is sufficiently unlikely that this causes confusion.
One other downside of printed the obfuscated value, is that you can no
longer read the pointer from the log and use it in gdb directly. That
might be sometimes convenient, but making this impossible is kinda the
purpose of this change.
As such, nm_log_ptr() becomes a bit of a misnomer. But not too bad, it
still is a good name. For example, if we wanted we could redefine the
NM_HASH_OBFUSCATE_PTR* macros when building "--with-more-asserts".
Previously we only cared whether supplicant is build with support for
FT. In that case we would pass FT-PSK to supplicant, like
Config: added 'key_mgmt' value 'WPA-PSK WPA-PSK-SHA256 FT-PSK'
Supplicant would then always try FT with preference, regardless whether
the interface/driver support it. That results in a failure to associate, if
the driver does not support it.
NetworkManager[1356]: <info> [1566296144.9940] Config: added 'key_mgmt' value 'WPA-PSK WPA-PSK-SHA256 FT-PSK'
...
wpa_supplicant[1348]: wlan0: WPA: AP key_mgmt 0x42 network profile key_mgmt 0x142; available key_mgmt 0x42
wpa_supplicant[1348]: wlan0: WPA: using KEY_MGMT FT/PSK
...
wpa_supplicant[1348]: * akm=0xfac04
...
kernel: ERROR @wl_set_key_mgmt :
kernel: invalid cipher group (1027076)
Since we pass a list of acceptable "key_mgmt" options to supplicant,
FT-PSK should not be used when supplicant knows it's not supported.
That is a supplicant bug.
Regardless, work around it by checking the per-interface capability, and
avoid it if support is apparently not present.
Up until now, a default-route (with prefix length zero) could not
be configured directly. The user could only set ipv4.gateway,
ipv4.never-default, ipv4.route-metric and ipv4.route-table to influence
the setting of the default-route (respectively for IPv6).
That is a problematic limitation. For one, whether a route has prefix
length zero or non-zero does not make a fundamental difference. Also,
it makes it impossible to configure all the routing attributes that one can
configure otherwise for static routes. For example, the default-route could
not be configured as "onlink", could not have a special MTU, nor could it be
placed in a dedicated routing table.
Fix that by lifting the restriction. Note that "ipv4.never-default" does
not apply to /0 manual routes. Likewise, the previous manners of
configuring default-routes ("ipv4.gateway") don't conflict with manual
default-routes.
Server-side this all the pieces are already in place to accept a default-route
as static routes. This was done by earlier commits like 5c299454b4
('core: rework tracking of gateway/default-route in ip-config').
A long time ago, NMIPRoute would assert that the prefix length is
positive. That was relaxed by commit a2e93f2de4 ('libnm: allow zero
prefix length for NMIPRoute'), already before 1.0.0. Using libnm from
before 1.0.0 would result in assertion failures.
Note that the default-route-metric-penalty based on connectivity
checking applies to all /0 routes, even these static routes. Be they
added due to DHCP, "ipv4.gateway", "ipv4.routes" or "wireguard.peer-routes".
I wonder whether doing that unconditionally is desirable, and maybe
there should be a way to opt-out/opt-in for the entire profile or even
per-routes.
https://bugzilla.redhat.com/show_bug.cgi?id=1714438
Minor refactoring so that there is only a one-line change necessary to
flip the implementation of the "internal" DHCP plugin for IPv4 from
"systemd" to "nettools".
We don't do that yet, because there are still some issues (e.g. the
lease is not persisted for nettools plugin). Eventually we want to
switch, so prepare the code to be almost there.
We have the "internal" DHCP plugin. That's our preferred plugin,
and eventually we may drop all other plugins.
Currently, the "internal" plugin is based on code from systemd-networkd
and implemented in "src/dhcp/nm-dhcp-systemd.c". As this code is forked
we eventually want to switch to nettools' n-dhcp4 library (for IPv4).
For that reason we already have "src/dhcp/nm-dhcp-nettools.c".
Note that "nettools" can be configured as a DHCP plugin, but this configuration
is only experimental and for testing. There is never supposed to be a
"nettools" plugin, but eventually the "internal" plugin will switch
implementation.
We don't want to replace systemd-based implementation right away. Not until
we are sure that nettools works well. For that reason we keep them
both in parallel for a while.
This commit makes "systemd" DHCP plugin explicitly configurable
in NetworkManager.conf. Like "nettools" this is an undocumented option,
only for testing.
If you choose "internal" (the default), you get one of the
implementations (currently the "systemd" one). But by selecting
"systemd" or "nettools" explicitly, you can select the exact plugin.
BlueZ 5.0 was released in December 2012 and broke API with
BlueZ 4. NetworkManager supports Bluez 5 for years already.
Of course, version 4 is long gone by now, so remove it.
We now only call the idle action with the same reason: authorized.
That is since we no longer use GDBusProxy, there are no other reasons
where we would fail.
Drop the unused code.
I encountered a failure in the log
<trace> [1564647990.7822] keyfile: commit: deleting nmmeta file "/etc/NetworkManager/system-connections/35370b0b-e53b-42ea-9fe3-f1b1d552343b.nmmeta" failed
<trace> [1564647990.7822] keyfile: commit: deleting nmmeta file "/etc/NetworkManager/system-connections/35370b0b-e53b-42ea-9fe3-f1b1d552343b.nmmeta" simulated
I think that was due to SELinux (rh #1738010).
Let nms_keyfile_nmmeta_write() return an errno code so we can log
more information about the failure.
... and nm_utils_fd_get_contents() and nm_utils_file_set_contents().
Don't mix negative errno return value with a GError output. Instead,
return a boolean result indicating success or failure.
Also, optionally
- output GError
- set out_errsv to the positive errno (or 0 on success)
Obviously, the return value and the output arguments (contents, length,
out_errsv, error) must all agree in their success/failure result.
That means, you may check any of the return value, out_errsv, error, and
contents to reliably detect failure or success.
Also note that out_errsv gives the positive(!) errno. But you probably
shouldn't care about the distinction and use nm_errno_native() either
way to normalize the value.
nm_utils_file_set_contents() is a re-implementation of g_file_set_contents(),
as such it returned merely a boolean success value.
It's sometimes interesting to get the native error code. Let the function
deviate from glib's original g_file_set_contents() and return the error code
(as negative value) instead.
This requires all callers to change. Also, it's potentially a dangerous
change, as this is easy to miss.
Note that nm_utils_file_get_contents() also returns an errno, and
already deviates from g_file_get_contents() in the same way. This patch
resolves at least the inconsistency with nm_utils_file_get_contents().
The secret-agent D-Bus API knows 4 methods: GetSecrets, SaveSecrets,
DeleteSecrets and CancelGetSecrets. When we cancel a GetSecrets
request, we must issue another CancelGetSecrets to tell the agent
that the request was aborted. This is also true during shutdown.
Well, technically, during shutdown we anyway drop off the bus and
it woudn't matter. In practice, I think we should get this right and
always cancel properly.
To better handle shutdown change the following:
- each request now takes a reference on NMSecretAgent. That means,
as long as there are pending requests, the instance stays alive.
The way to get this right during shutdown, is that NMSecretAgent
registers itself via nm_shutdown_wait_obj_register() and
NetworkManager is supposed to keep running as long as requests
are keeping the instance alive.
- now, the 3 regular methods are cancellable (which means: we are
no longer interested in the result). CancelGetSecrets is not
cancellable, but it has a short timeout NM_SHUTDOWN_TIMEOUT_MS
to handle this. We anyway don't really care about the result,
aside logging and to be sure that the request fully completed.
- this means, a request (NMSecretAgentCallId) can now immediately
be cancelled and destroyed, both when the request returns and
when the caller cancels it. The exception is GetSecrets which
keeps the request alive while waiting for CancelGetSecrets. But
this is easily handled by unlinking the call-id and pass it on
to the CancelGetSecrets callback.
Previously, the NMSecretAgentCallId was only destroyed when
the D-Bus call returns, even if it was cancelled earlier. That's
unnecessary complicated.
- previously, D-Bus requests SaveSecrets and DeleteSecrets were not cancellable.
That is a problem. We need to be able to cancel them in order to shutdown in
time.
- use GDBusConnection instead of GDBusProxy. As most of the time, GDBusProxy
provides features we don't use.
- again, don't log direct pointer values, but obfuscate the indentifiers.
In the past, we had a private unix socket. That is long gone.
Drop the remains in "nm-secret-agent.c". The request here really
always comes from the main D-Bus connection.
Maybe the private unix socket makes sense and we might resurrect it one
day. But at that point it would be an entire rewrite and the existing
code is probably not useful either way. Drop it.
- add nm_c_list_elem_find_first() macro that takes a predicate
and returns the first match.
This macro has a non-function-like behavior, which we often try to
avoid because macros should behave like functions. In this case it's
however convenient, so let's do it.
Also, despite being non-function-like, it should be pretty hard to
use wrongly.
- rename nm_c_list_elem_find_first() to nm_c_list_elem_find_first_ptr().
- Don't use GDBusProxy but plain GDBusConnection. NMFirewallManager
is very simple, it doesn't use any of the features that GDBusProxy
provides.
- make NMFirewallManagerCallId typedef a pointer to the opaque call-id
struct, instead of the struct itself. It's confusing to have a
variable that does not look like a pointer and assigning %NULL to
it.
- internally drop the CBInfo typename and name the call-id variable
constsistantly as "call_id".
- no need to keep the call-id struct alive after cancelling it. That
simplifies the lifetime managment of the pending call because the
completion callback is always invoked shortly before destroying
the call-id.
- note that the caller is no longer allowed to cancel a call-id from
inside the completion callback. That just complicates the
implementation and is not necessary. Assert against that.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/merge_requests/230
They should be "static" and only visible to this source file.
Also, they should be "const", that allows the linker to place them
into read-only memory.
For better or worse, the API does not require the value to be a
UTF-8 string. We cannot just concatenate binary to a string.
Instead, backslash escape it with utf8safe-escape.
Also, this will shut up a (wrong) coverity warning at this place.
Seen on gitlab-ci.
NMPlatformSignalAssert: ../src/platform/tests/test-link.c:260, test_slave(): failure to accept signal [0,2] times: link-changed-changed ifindex 15 (3 times received)
ERROR: src/platform/tests/test-link-linux - too few tests run (expected 76, got 6)
ERROR: src/platform/tests/test-link-linux - exited with status 133 (terminated by signal 5?)
CID 59391 (#1 of 1): Copy into fixed size buffer (STRING_OVERFLOW)
31. fixed_size_dest: You might overrun the 16-character fixed-size string be.ifspec.spec.ifname by copying priv->nas_ifname without checking the length.
get_word() only moves the "argument" pointer forward. It never sets it
to %NULL. Also, above we already dereference argument, so Coverity thinks
that this NULL check indicates a bug.
Drop it to silence Coverity.
Warned by coverity: we assert above that brfd is -1, so we must always
restore it to -1 in the error case.
Technically, not a problem because socket() is documented to return
only -1 on error already. Apparently coverity does not believe that.
