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
inet_aton() is very accepting when parsing the address. For example,
it accepts addresses with fewer octets (interpreting the last octet
as a number in network byte order for multiple bytes). It also ignores
any trailing garbage after the first delimiting whitespace (at least,
the glibc implementation). It also accepts octets in hex and octal
notation.
For the initrd reader we want to be more forgiving than inet_pton()
and also accept addresses like 255.000.000.000 (octal notation). For
that we would want to use inet_aton(). But we should not accept all the
craziness that inet_aton() otherwise accepts.
Use nm_utils_parse_inaddr_bin_full() instead. This function implements
our way how we want to interpret IP addresses in string representation.
Under the hood, of course it also uses inet_pton() and even inet_aton(),
but it is stricter than inet_aton() and only accepts certain formats.
The @family argument is an input and output argument.
Initially, the family is set to AF_UNSPEC, in which case the family
gets detected based on the IP address. However, we call
dt_get_ipaddr_property() multiple times to parse the netmask, the
gateway and the IP address.
That means, after the first successfull call, the @family is set to
AF_INET or AF_INET6.
Note that the previous code (in the switch block) would only check that
the family is set to AF_UNSPEC, but it would not check that the @family
matches the expected binary address length @len. Later, we then might call
nm_ip_address_new_binary() with a family and a binary address of
unexpected length.
Also drop the error checking for nm_ip_address_new_binary().
nm_ip_address_new_binary() can only fail if the prefix length is larger
than 32/128. The function has no way to validate the input arguments
beyond that and can thus not fail (short of undefined behavior).
This is necessary on Travis/Ubuntu 16.04, otherwise the test
fails with
# NetworkManager-MESSAGE: <warn> [1575301791.7600] platform-linux: do-add-link[nm-test-device/team]: failure 95 (Operation not supported)
Aborted (core dumped)
# test:ERROR:../src/platform/tests/test-link.c:353:test_software: assertion failed: (software_add (link_type, DEVICE_NAME))
ERROR: src/platform/tests/test-link-linux - too few tests run (expected 76, got 6)
