Move it to shared as it's useful for clients as well.
Move and rename nm_dbus_manager_new_auth_subject_from_context() and
nm_dbus_manager_new_auth_subject_from_message() in nm-dbus-manager.c
as they're needed there.
If we know the address used previously, also tell the client to start
from the init-reboot phase, so that it will start with a DHCP request
instead of a discover.
We call svFindFirstNumberedKey() to check whether we have any NETMASK
set. Since commit 9085c5c3a9 ('ifcfg-rh: rename
svFindFirstKeyWithPrefix() to svFindFirstNumberedKey() for finding
NETMASK') that function would no longer find the "NETMASK" without
number.
Fix that, by letting nms_ifcfg_rh_utils_is_numbered_tag() return TRUE
for the tag itself. This also makes more sense, because it matches our
common understanding what numbered tags are.
Adjust the other callers that don't want this behavior to explicitly
check.
Fixes: 9085c5c3a9 ('ifcfg-rh: rename svFindFirstKeyWithPrefix() to svFindFirstNumberedKey() for finding NETMASK')
Previously, setting or getting a variable required to scan all lines.
Note that frequently we would look up variables that didn't actually
exist, which we could only determine after searching the entire list.
Also, since we needed to handle having the same variable specified
multiple times (where the last occurrence wins), we always had to search
all keys and couldn't stop when finding the first key. Well, technically
we could have searched in reverse order for the getter, but that wasn't
done. For the setter we wanted to delete all but the last occurrences,
so to find them, we really had to search them all.
We want to support profiles with hundreds or thousands of addresses and routes.
This does not scale well.
Add an hash table to find the variables in constant time.
Test this commit and the parent commit:
$ git clean -fdx &&
CFLAGS=-O2 ./autogen.sh --with-more-asserts=0 &&
./tools/run-nm-test.sh -m src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh &&
perf stat -r 50 -B src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh 1>/dev/null
Before:
Performance counter stats for 'src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh' (50 runs):
330.94 msec task-clock:u # 0.961 CPUs utilized ( +- 0.33% )
0 context-switches:u # 0.000 K/sec
0 cpu-migrations:u # 0.000 K/sec
1,081 page-faults:u # 0.003 M/sec ( +- 0.07% )
1,035,923,116 cycles:u # 3.130 GHz ( +- 0.29% )
1,800,084,022 instructions:u # 1.74 insn per cycle ( +- 0.01% )
362,313,301 branches:u # 1094.784 M/sec ( +- 0.02% )
6,259,421 branch-misses:u # 1.73% of all branches ( +- 0.13% )
0.34454 +- 0.00116 seconds time elapsed ( +- 0.34% )
Now:
Performance counter stats for 'src/settings/plugins/ifcfg-rh/tests/test-ifcfg-rh' (50 runs):
329.78 msec task-clock:u # 0.962 CPUs utilized ( +- 0.39% )
0 context-switches:u # 0.000 K/sec
0 cpu-migrations:u # 0.000 K/sec
1,084 page-faults:u # 0.003 M/sec ( +- 0.05% )
1,036,130,698 cycles:u # 3.142 GHz ( +- 0.13% )
1,799,851,979 instructions:u # 1.74 insn per cycle ( +- 0.01% )
360,374,338 branches:u # 1092.756 M/sec ( +- 0.01% )
6,160,796 branch-misses:u # 1.71% of all branches ( +- 0.08% )
0.34287 +- 0.00133 seconds time elapsed ( +- 0.39% )
So, not much difference. But this is not surprising, because test-ifcfg-rh loads and
writes predominantly ifcfg files with few variables. The difference should be visible
when having large files.
svFindFirstKeyWithPrefix() only had one caller: to find whether there are
any NETMASK variables set. NETMASK is a numbered variable, so we should only
find variables that indeed follow the pattern. Since there was only
one caller, rename and repurpose the function.
When we write a connection profile to ifcfg-rh file, we first load the
possibly existing file and modify it. The purpose is to preserve
variables that we don't know about, keep comments and preserve the order
of the variables.
Note that the writer sets a bunch of variables according to the
profile's setting. At various places the writer would explicitly
clear variables with svUnsetValue(). However, that was problematic:
- we would not unset all variables that we care about. We really should
not leave previous variables if they make no sense anymore for the
profile. The only thing we want to preserve are entirely unknown keys
and comments. Note that when the writer omits to clear an unset variable,
it usually does so assuming that the reader would anyway ignore the
key, become some other key renders it irrelevant. Given the complexity
of the reader and writer, that is often not the case and hard to ensure.
We might have simply forgotten a svUnsetValue(), which was an easy
to make mistake and hard to find (because you'd have to test with
a pre-existing profile that happens to contain that key, which leaves
countless combinations for testing.
That means, a profile written by the writter might be interpreted
differently by the reader depending on which pre-existing keys were set.
- it was cumbersome to explicitly call svUnsetValue().
Note that for numbered tags in particular we would iterate the keys
trying to unset them. For example for addresses (like "IPADDR5") we
would iterate over the first 256 IPADDR keys, trying to unset them.
That is horrible. For one, it doesn't cover the case where there might
be more than 256 addresses. Also, it adds a significant overhead every
time.
While writing a ifcfg file currently is O(n^2) because setting one key
is O(l), with l being the number of keys/lines. So, if you set n keys
in a file with l lines, you get O(n*l). Which is basically O(n^2),
because the number of lines and the number of keys to set usually
corresponds.
So when setting 256 times IPADDR, the overall complexity was still
O(n^2 + 256 * n) and didn't change. However, the 256 factor here can
be very significant.
We should not explicitly unset variables, we should always unset all
known variables that we don't explicitly set.
The svUnsetValue() calls are still there. They will be dropped next.
Helper function to remove all variables that are still dirty (not
visited) and well-known.
Also add svWriteFileWithoutDirtyWellknown() to clear the lines
before persisting to disk.
This adds a lot of meta-data about how we handle ifcfg-rh.
We will use this to prune/delete all variables that are not explicitly
set (dirty) but also well-known.
We could now easily emit a warning when an ifcfg-rh file contains
unused key.
We also could add more meta-data for each key. For example, we write
different files (ifcfg- and keys- files). We could add flags to indicate
that variables are valid in certain files. Currently that's not done.
Also, for simple properties we could associate the key with the
NMSetting property, and treat does generically, like keyfile does.
Anyway, there are potentials. For now, we will use this to clear dirty
variables.
Previously, IS_NUMBERED_TAG() could only be called with a C literal.
Add is_numbered_tag() which can be called with any C string.
Also, IS_NUMBERED_TAG_PARSE() and IS_NUMBERED_TAG() didn't do exactly
the same. I think they should. The only difference was if the number
was larger than 2^63-1. Now IS_NUMBERED_TAG() starts ignoring such
keys, which is fine.
By default, all lines are now marked as dirty. Whenever we modify/set
a line, it becomes non-dirty. That will be used later to prune lines
that are dirty, that is, not yet visited.
Found by covscan:
NetworkManager-1.22.0/src/dhcp/nm-dhcp-nettools.c:945: check_return:
Calling "g_file_set_contents" without checking return value (as is
done elsewhere 16 out of 20 times).
Fixes: 9f89516928 ('dhcp: nettools: read/write lease files')
The autoconnection for virtual devices currently works in two
phases. First we detect that there is suitable profile that can
autoconnect and we realize the device. Then, when the device becomes
'disconnected', autoconnect kicks in and starts the activation.
However, if autoconnect is blocked for a device, currently we do step
1 without step 2, leaving a stale interface around. Fix this by also
checking that autoconnect is not blocked during step 1.
https://bugzilla.redhat.com/show_bug.cgi?id=1765047https://gitlab.freedesktop.org/NetworkManager/NetworkManager/merge_requests/360
The abbreviations "ns" and "ms" seem not very clear to me. Spell them
out to nsec/msec. Also, in parts we already used the longer abbreviations,
so it wasn't consistent.
If a device is being autoactivated and requires a parent that is
blocked due to user request, the autoactivation attempt should fail
because NM shouldn't overrule the user decision.
https://bugzilla.redhat.com/show_bug.cgi?id=1765566
Many device types take the MTU value from the wired setting; usually
they don't implement the can_reapply_change() method and so the MTU
can't be changed with the Reapply() API.
Instead of implementing the method for all such devices to support the
same property (adding a lot of duplicated code), add a check in
NMDevice to allow the reapply of MTU when we recognize that the device
uses the MTU from the wired setting.
Device types can still decide to implement can_reapply_change() and
support whatever properties they want, even from the wired setting.
The underlying GPtrArray that we use to construct the list of warnings
is more useful than the strv array. For the internal function, don't
let it return the strv array but instead take (and fill) the warnings
as GPtrArray. There is no difference in practice, because also
previously we would always create an empty GPtrArray.
We always build with PolicyKit support enabled, because it has no
additional dependencies, beside some D-Bus calls.
However, in NetworkManager.conf the user could configure
"main.auth-polkit" to disable PolicyKit. However, previously it would
only allow to disable PolicyKit while granting access to all users.
I think it's useful to have an option that disables PolicyKit and grants
access only to root. I think we should not go too far in implementing
our own authorization mechanisms beside PolicyKit (e.g. you cannot
disable PolicyKit and grant access based on group membership of the
user). However, disabling PolicyKit can be useful sometimes, and it's
simple to implement a "root-only" setup.
Note one change is that when NetworkManager now runs without a D-Bus
connection (in initrd), it would deny all non-root requests. Previously
it would grant access. I think there should be little difference in
practice, because if we have no D-Bus we also don't have any requests to
authenticate.
When moving a lease file from initramfs directory to NetworkManager
run directory, SELinux label for that file retains tmpfs_t type.
Fix it by using sendfile() instead of rename(). That way, the
lease file will have the default type: NetworkManager_var_run_t.
Since we take ownership of the lease file, also drop it from the
old location.
* Before the patch:
ls -Z /var/run/NetworkManager/dhclient-*.lease
system_u:object_r:tmpfs_t:s0 dhclient-13162c00-abfb-4e28-bbfb-170187ddd044-ens3.lease
* After:
ls -Z /var/run/NetworkManager/dhclient-*.lease
system_u:object_r:NetworkManager_var_run_t:s0 dhclient-f47d1908-67ae-49c6-bd5e-19a690d85526-ens3.lease
Fixes: f2fe6c03ee ('manager: don't treat the initramfs-configured DHCP connections as generated')
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/merge_requests/353
In NetworkManager we have NMAuthCallResult, which is really a duplicate
of NMClientPermissionResult.
Maybe NMAuthCallResult should be entirely replaced by NMClientPermissionResult.
But the name NMClientPermissionResult is a bit awkward. But then the
duplication is even more awkward... fixing this is left for another day.
Reuse the list of all permissions and don't duplicate it.
Also, now the result of GetPermissions() on D-Bus contains the
permissions sorted by name. We get it almost for free, and it's
a nice property.
For example with
mount -o remount,rw,hidepid=1 /proc/
all permission checks will fail with an error. Internally, we map the
failure to NM_AUTH_CALL_RESULT_UNKNOWN.
<trace> [1575645672.5958] auth: call[1069]: CheckAuthorization(org.freedesktop.NetworkManager.enable-disable-connectivity-check), subject=unix-process[pid=468316, uid=1000, start=1912881]
<trace> [1575645672.6295] auth: call[1069]: completed: failed: GDBus.Error:org.gtk.GDBus.UnmappedGError.Quark._g_2dfile_2derror_2dquark.Code4: Failed to open file “/proc/468316/status”: No such file or directory
<debug> [1575645672.6296] manager: unknown auth chain result 0
First of all, we should not log a debug message about that (we already log the
result of permission checks separately).
Also, we should include the unknown result in the response. The permission was
checked, and omitting it from GetPermissions() result seems wrong (even if we
failed to get the result).
Note that "unknown" is now a new possible return value on D-Bus. But
see how nm_permission_result_to_client() would map such a value to
"unknown" as well. So, it's probably a fine extension of the D-Bus API.
Note that NMClient API is currently quite limited. The user won't know
whether permissions were received (and if they were received, they
could not distinguish between UNKNOWN and absent). Hence, returning
all permissions as unknown (or not at all) causes `nmcli general permissions`
to hang. The solution here is to improve NMClient API to allow the user
to know when the permissions are received. But this patch doesn't
fix the hanging of nmcli nor the limitation of NMClient's API.
If the activation of an assumed device fails, we first set the device
state to FAILED and then to ACTIVATED. In the FAILED state, the active
connection transitions to DEACTIVATED and clears its device pointer;
hence we end up with an inconsistent state which causes assertion
failures in other parts of the code (for example, get_best_ip_config()
assumes that the device of the best active connection is not NULL).
Don't first transition to FAILED and then to ACTIVATED, just set the
latter.
https://bugzilla.redhat.com/show_bug.cgi?id=1737774https://gitlab.freedesktop.org/NetworkManager/NetworkManager/merge_requests/351
