Note that the server always returns TRUE for the boolean return value
of ReloadConnections. Hence, this should not change in behavior, because
the server would never have returned FALSE.
However, change behavior of the API. It's odd that the function might
return %FALSE without setting the error output. It's also not clear
what the boolean value of the "ReloadConnections" D-Bus would mean
anyway.
Refactor reading the phase2 authentication method for 802.1X.
Previously the reader only considered the first item of the
space-separated list; but since the 802.1x setting can hold distinct
phase2-auth and phase2-autheap properties - both mapped to the same
ifcfg-rh variable - we should parse the whole list. We only emit a
warning when multiple methods of the same type are found to avoid
breaking existing manually written ifcfg files.
Moreover, the reader implemented different checks for each of the
outer tunneled methods (PEAP, TTLS and FAST); drop those checks and
accept whatever the 802.1X setting also consider as valid. Note that
some combinations that are in principle valid, like PEAP + EAP-MD5,
were dropped before.
An extra variable is used for sources of
`libnm-settings-plugin-ifupdown` module. However, it only contains
one source file and using it directly avoiding the creation of the
extra variable doesn't hurt readibility.
The `ifcfg-rh` meson build file installs a new post install script
to create the `network-config` directory.
This has been moved to the main post install file so it's easier to
find because all post install steps are together and it avoids and
extra post install script execution.
The file has been fixed to be consistent with the rest of the files.
The data files to be installed have been grouped together. The
sourc files has been listed vertically and the link target in
`nm-settings-plugin-ifcfg-rh` does not use an array anymore.
The targets that involve the use of the `NetworkManager` library,
built in the `src` build file have been improved by applying a set
of changes:
- Indentation has been fixed.
- Set of objects used in targets have been grouped together.
- Aritificial dependencies used to group dependencies and custom
compiler flags have been removed and their use replaced with
proper dependencies and compiler flags to avoid any confussion.
The `libnm-core` build file has been improved by applying a set of
changes:
- Indentation has been fixed to be consistent.
- Library variable names have been changed to `lib{name}` pattern
following their filename pattern.
- `shared` prefix has been removed from all variables using it.
- Dependencies have been reviewed to store the necessary data.
- The use of the libraries and dependencies created in this file
has been reviewed through the entire source code. This has
required the addition or the removal of different libraries and
dependencies in different targets.
- Some files used directly with the `files` function have been moved
to their nearest path build file because meson stores their full
path seamessly and they can be used anywhere later.
The `nm-default.h` header is used widely in the code by many
targets. This header includes different headers and needs different
libraries depending the compilation flags.
A new set of `*nm_default_dep` dependencies have been created to
ease the inclusion of different directorires and libraries.
This allows cleaner build files and avoiding linking unnecessary
libraries so this has been applied allowing the removal of some
dependencies involving the linking of unnecessary libraries.
have_connection_for_device() really should just call nm_device_check_connection_compatible().
Note that nm_device_check_connection_compatible() of course checks the
connection type already, so this is redundant.
This check is only useful for devices that implement new_default_connection.
We can shortcut the possibly expensive checks like have_connection_for_device(),
which need to iterate all profiles.
If a profile has only "ethernet.mac-address" set, but
"connection.interface-name" not, then the previous check
iface = nm_setting_connection_get_interface_name (s_con);
if (!nm_streq0 (iface, nm_device_get_iface (device)))
continue;
would wrongly consider the profile not matching for the device.
As a result, we would wrongly create a auto-default connection.
Fix that. We already call nm_device_check_connection_compatible()
above. That is fully suitable to compare the interface name and
the MAC address. We don't need to duplicate this check (wrongly).
See also commit 77d01c9094 ('settings: ignore incompatible connections
when looking for existing ones') for how this code changed.
https://bugzilla.redhat.com/show_bug.cgi?id=1727909
Now nm_shutdown_wait_obj_*() supports two styles:
- NM_SHUTDOWN_WAIT_TYPE_OBJECT: this just registers a weak pointer
on a source GObject. As long as the object is not destroyed
(and the object is not unregistered), the shutdown gets blocked.
- now new is NM_SHUTDOWN_WAIT_TYPE_CANCELLABLE: this source object
is a GCancellable, and during shutdown, the system will cancel
the instances to notify about the shutdown. That aside, the GCancellable
is tracked exactly like a regular NM_SHUTDOWN_WAIT_TYPE_OBJECT (meaning:
a weak pointer is registered and shutdown gets delayed as long as the instance
lives).
As the rest of the shutdown, it's not yet implemented on the shutdown-side.
What is now possible is to register such cancellables, so that users can make
use of this API before we fix shutdown. We cannot fix it all at the same time,
so first users must be ready for this approach.
All D-Bus method call implementations use similar error messages when
authenticating requests; add defines for them to ensure the same exact
message is reused.
First of all, keyfile writer (and reader) are supposed to be able to store
every profile to disk and re-read a valid profile back. Note that the profile
might be modified in the process, for example, blob certificates are written
to a file. So, the result might no be exactly the same, but it must still be
valid (and should only diverge in expected ways from the original, like mangled
certificates).
Previously, we would re-read the profile after writing to disk. If that failed,
we would only fail an assertion but otherwise proceeed. It is a bug
after all. However, it's bad to check only after writing to file,
because it results in a unreadable profile on disk, and in the first
moment it appears that noting went wrong. Instead, we should fail early.
Note that nms_keyfile_reader_from_keyfile() must entirely operate on the in-memory
representation of the keyfile. It must not actually access any files on disk. Hence,
moving this check before writing the profile must work. Otherwise, that would be
a separate bug. Actually, keyfile reader and writer violate this. I
added FIXME comments for that. But it doesn't interfere with this
patch.
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
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
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.
This triggers a coverity warning because we above already
check that not all relevant keys are NULL together.
Work around warning by modifying the code.
(cherry picked from commit 210d7eb528)
Tombstones in /etc are not only to hide storages of type keyfile. They
are for hiding/shadowing any profile from persistant storage. That's
why we need to compare them already in _sett_conn_entry_sds_update().
Fix the prioriy of storages for the same UUID.
Note that the "$UUID.nmmeta" files (the tombstones) are handled by
keyfile plugin. But that is only to load them together during `nmcli
connection reload` when we iterate the files of the system-connections
directory. For the most part, nmmeta/tombstones are not keyfile specific
and handled by NMSettings. A tombstone in /run hides any profile (regardless
of the settings plugin). And a tombstones in /etc hides any profile, except
in-memory connections from keyfile /run directory.
Note that we now support keyfiles from read-only storage in /usr/lib.
Note also that we do support modifying and deleting these profiles.
That works by placing a shadowing profile to /etc or /run.
Surely this is questionable. It means that once the user uses D-Bus
to modify/delete a profile in /usr/lib, that profile becomes forever
shadowed by a different file, and there is no D-Bus API to return
to the original file. The user would have to drop the shadowing storages
from the file system. That is a problem.
But on the other hand, disallowing changes to such read-only profiles
is not very useful either. If you no longer can use D-Bus to modify such
profiles, what's the value here? How are applications supposed to handle
such profiles if there is no D-Bus API to do something sensible to them?
So, whatever problems read-only profiles and this shadowing causes, I don't
think that the solution is to entirely disallow changes via D-Bus.
At that point, we can just as well allow changes to profiles from
ifupdown. Note that you still cannot modify the profile directly (as the
ifupdown plugin does not support that). But you can delete the profile
(either temporarily via a tombstone in /run or permanently via a
tombstone in /etc). You also can make the profile in-memory, and take
it from there. Note that if you try to later store the in-memory profile
to disk again, then it depends on the order of settings plugins whether
that succeeds. If you have "plugins=keyfile,ifupdown", then the profile
will be stored as keyfile to /etc. If you have "plugins=ifupdown,keyfile",
then the modification will only be done in /run and the "to-disk" command
silently ignored (there really is no better solution).
Via Update2() D-Bus API there are three ways how a profile can be stored
(or migrated) to in-memory:
- NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY
- NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_DETACHED
- NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_ONLY
With the recent rework of settings I dropped NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY
and it had the same meaning as NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_DETACHED.
However, the way NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_DETACHED was implemented is
problematic. The problem is that it leaves the profile on disk but creates an
in-memory representation which shadows the persistent storage. Later,
when storing the profile to disk again, a new filename is chosen.
This allows via D-Bus API to toggle between NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_DETACHED
and NM_SETTINGS_UPDATE2_FLAG_TO_DISK, and thereby pilling up profiles on disk.
Also, there is no D-Bus API to do anything sensible with these leaked, shadowed
profiles on disk.
Note that if we have a read-only profile in /usr/lib or in ifupdown
plugin, then the problem is not made any worse. That is, because via D-Bus
API such profiles can be made in-memory, and afterwards stored to /etc.
Thereby too the profile gets duplicate on disk, but this game only
works once. Afterwards, you cannot repeat it to create additional
profiles on disk. It means, you can only leak profiles once, and only
if they already exist in read-only storage to begin with.
This problem with NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_DETACHED already existed
before the settings-delegate-storage rework, and is unrelated to whether in-memory
profiles now happen to be persisted to /run.
Note that NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_ONLY is simple and does not suffer
from this problem. When you move a profile to in-memory-only, it gets deleted from
persistent storage and no duplication happens.
The problem is that NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_DETACHED used to
forget about the profile that it shadows, and that is wrong.
So, first re-add proper support for NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY. This
works by remembering the "shadowed-storage" path for in-memory profiles.
When later saving such a profile to disk again, the shadowed-storage
will be re-used. Likewise, when deleting such a profile, the shadowed
storage will be deleted.
Note that we keep NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_DETACHED and it
also remembers the shadowed storage (but without "owning" it). That means,
when such a profile gets saved to disk again, the orginal storage is
reused too. As such, during future updates it behaves just like
NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY. The difference is when deleting
such a profile. In this case, the profile is left on storage and a
tombstone gets written. So, how is this better than before and why even
keep this complicated flag?
First, we keep this flag because we really want the ansible role to be
able to do in-memory changes only. That implies being able to delete a
profile from NetworkManager's view, but not from persistent storage. Without
this flag there is no way to do that. You can only modify an on-disk profile
by shadowing it, but you could not delete it form NetworkManager's view
while keeping it on disk.
The new form of NM_SETTINGS_UPDATE2_FLAG_IN_MEMORY_DETACHED is safe and avoids
the duplication problem because also for tombstones it remembers the original
"shadowed-storage". That is, when the profile gets recreated later via
D-Bus API AddConnection, then the re-created profile will still reference
and reuse the shadowed storage that it had before deletion.
