NM_UTILS_LOOKUP_STR() uses alloca(). Partly to avoid the overhead of
malloc(), but more important because it's convenient to use. It does
not require to declare a varible to manage the lifetime of the heap
allocation.
It's quite safe, because the stack allocation is of a fixed size of only
a few bytes. Overall, I think the convenience that we get (resulting in
simpler code) outweighs the danger of stack allocation in this case. It's
still worth it.
However, as it uses alloca(), it still must not be used inside a (unbound)
loop and it is obviously a macro.
Rename the macros to have a _A() suffix. This should make the
peculiarities more apparent.
The only purpose of using alloca() to avoid the overhead of heap-allocation
and possible save a line in source code for managing/freeing the heap allocation.
For tests we don't care about performance, and (in this case)
the code does not get any shorter.
Avoid alloca() in tests, because alloca() is something to search for
when reviewing code for stack overflows. No need to have such false
positives show up in tests.
Add a version of nm_utils_strbuf_append_*() that does not care
about NUL terminate strings, but accept any binary data. That makes
it useful for writing a binary buffer.
Add a "a{sv}" output argument to "AddAndActivate2" D-Bus API.
"AddAndActivate2" replaces "AddAndActivate" with more options.
It also has a dictionary argument to be forward compatible so that we
hopefully won't need an "AddAndActivate3". However, it lacked a similar
output dictionary. Add it for future extensibility. I think this is
really to workaround a shortcoming of D-Bus, which does provide strong
typing and type information about its API, but does not allow to extend
an existing API in a backward compatible manner. So we either resort to
Method(), Method2(), Method3() variants, or a catch-all variant with a
generic "a{sv}" input/output argument.
In libnm, rename "nm_client_add_and_activate_connection_options()" to
"nm_client_add_and_activate_connection2()". I think libnm API should have
an obvious correspondence with D-Bus API. Or stated differently, if
"AddAndActivateOptions" would be a better name, then the D-Bus API should
be renamed. We should prefer one name over the other, but regardless
of which is preferred, the naming for D-Bus and libnm API should
correspond.
In this case, I do think that AddAndActivate2() is a better name than
AddAndActivateOptions(). Hence I rename the libnm API.
Also, unless necessary, let libnm still call "AddAndActivate" instead of
"AddAndActivate2". Our backward compatibility works the way that libnm
requires a server version at least as new as itself. As such, libnm
theoretically could assume that server version is new enough to support
"AddAndActivate2" and could always use the more powerful variant.
However, we don't need to break compatibility intentionally and for
little gain. Here, it's easy to let libnm also handle old server API, by
continuing to use "AddAndActivate" for nm_client_add_and_activate_connection().
Note that during package update, we don't restart the currently running
NetworkManager instance. In such a scenario, it can easily happen that
nmcli/libnm is newer than the server version. Let's try a bit harder
to not break that.
Changes as discussed in [1].
[1] https://gitlab.freedesktop.org/NetworkManager/NetworkManager/merge_requests/37#note_79876
Don't return success from a nested code path. Handle all errors
first, and return early. Well, we cannot really return right away
because we need to handle the failure. So, at least, check for errors
and "goto fail".
If the child is respawning too fast, consider the plugin failed so
that upstream servers are written to resolv.conf until the plugin gets
restarted after the delay.
When the dnsmasq process dies, two events are generated:
(1) a NM_DNS_PLUGIN_FAILED signal in nm-dns-dnsmasq.c:name_owner_changed()
(2) a NM_DNS_PLUGIN_CHILD_QUIT signal in nm-dns-plugin.c:from watch_cb()
Event (1) is handled by updating resolv.conf with upstream servers,
(2) by restarting the child process.
The order in which the two signals are received is not deterministic,
so when (1) comes after (2) the manager leaves upstream servers in
resolv.conf even if a dnsmasq instance is running.
When dnsmasq disappears from D-Bus and we know that the process is not
running, we should not emit a FAILED signal because the disappearing
is caused by the process termination, and that event is already
handled by the manager.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/issues/105
wpa_supplicant is going to change the global default for PMF from 0
(disabled) to 1 (optional) [1], so NM code needs to be adjusted to
work with all wpa_supplicant versions. Furthermore, it is better to
set optional PMF using the 'Pmf' property instead of the 'ieee80211w'
configuration option because the former better handles missing support
in driver [2].
Note that each interface in wpa_supplicant has its own copy of global
configuration and so 'global' options must still be set on each
interface. So, let's set Pmf=1 when each interface gets created and
override it with ieee80211w={0,2} if needed during association.
[1] http://lists.infradead.org/pipermail/hostap/2018-November/039009.html
[2] http://lists.infradead.org/pipermail/hostap/2019-January/039215.htmlhttps://gitlab.freedesktop.org/NetworkManager/NetworkManager/issues/104
_log_connection_get_property() is a hack, as it cannot meaningfully print complex
properties. Also, it uses _nm_setting_get_property() which can only work with GObject
base properties.
Don't assert against _nm_setting_get_property() returning success. Eventually
we should replace _nm_setting_get_property() by something better. But for the moment,
it's fine to being unable to print a property value.
When STP is disabled, the bridge parameters 'priority', 'forward-delay',
'hello-time' and 'max-age' are irrelevant.
We already skip them when loading a connection profile from a ifcfg file.
Do the same when generating a connection from a configured device, in
order to possibly assume the connection.
...also when the connection is created at NetworkManager
startup to map an already configured bridge.
Ensure the device has configuration values that fall inside
NetworkManager boundaries, otherwise map the value with a default.
Since we already cached the result of getpagesize() in a static variable (at
two places), move the code to nm-shared-utils, so it is reusable.
Also, use sysconf() instead of getpagesize(), like suggested by `man
getpagesize`.
The "ipv4.dhcp-client-id" is configurable per-profile and the default
value can be overwritten via connection defaults in NetworkManager.conf.
For "dhclient" DHCP plugin, the ultimate default for "ipv4.dhcp-client-id"
is determined by dhclient itself, or possibly by user configuration from
"/etc/dhcp".
For the "internal" DHCP plugin, the default must be decided by
NetworkManager. Also, the default here is important, as we preferably
won't change it anymore. That is because a changing the client-id
will result in different IP addresses after upgrade of NetworkManager
version. That should be avoided.
Still, change it now. If a downstream does not want this, it either needs
to patch the sources or add a configuration snippet like:
[connection-internal-dhcp-client-id-duid]
match-device=dhcp-plugin:internal
ipv4.dhcp-client-id=duid
The reason to change from the previous default "duid" to "mac" are the
following:
- "duid" is an RFC 4361 compatible client-id ([1]) and "mac" is defined
in RFC 2132.
- "duid" cannot (easily) be predicated a-priori as it is a hash of the
interface-name and "/etc/machine-id". In particular in cloud and server
environments, admins often prefer "mac" as they do know the MAC address
and pre-configure the DHCP server accordingly.
- with "dhclient" plugin, the default is decided by dhclient package or
user configuration in "/etc/dhcp". However, in fact the default is often
"client-identifier hardware" (for example on RHEL/CentOS).
- for RHEL/CentOS we require a way to select "mac" as default. That was
done by installing a configuration snippet via the NetworkManager-config-server
package. It's confusing to have the default depend on a package. Avoid
that. Also, users required "mac" in certain scenarios, but no users
explicitly asked for "duid" as default.
- our "duid" implementation generates a 32 bit IAID based on a hash of the
interface-name, and only 8 bytes entropy that contains a hash
of "/etc/machine-id". The point is, that is not a lot of entropy to
avoid conflicting client-ids. Another point is, that the choosen algorithm
for "duid" is suitable for RFC 4361, but it's only one of many possibly
implementations. Granted, each possibility has up and downsides but selecting
one of them as default seems wrong (given that it has obvious downsides
already). For "mac" there is only one straight-forward way to implement
it.
- RFC 7844 (Anonymity Profiles for DHCP Clients) is not yet supported by
NetworkManager. But we should not select a default client-id which
counteracts anonymit. Choosing "mac" does not reveal information which
is not already exposed.
[1] https://tools.ietf.org/html/rfc4361#section-4https://bugzilla.redhat.com/show_bug.cgi?id=1661165
We should no longer use nm_connection_for_each_setting_value() and
nm_setting_for_each_value(). It's fundamentally broken as it does
not work with properties that are not backed by a GObject property
and it cannot be fixed because it is public API.
Add an internal function _nm_connection_aggregate() to replace it.
Compare the implementation of the aggregation functionality inside
libnm with the previous two checks for secret-flags that it replaces:
- previous approach broke abstraction and require detailed knowledge of
secret flags. Meaning, they must special case NMSettingVpn and
GObject-property based secrets.
If we implement a new way for implementing secrets (like we will need
for WireGuard), then this the new way should only affect libnm-core,
not require changes elsewhere.
- it's very inefficient to itereate over all settings. It involves
cloning and sorting the list of settings, and retrieve and clone all
GObject properties. Only to look at secret properties alone.
_nm_connection_aggregate() is supposed to be more flexible then just
the two new aggregate types that perform a "find-any" search. The
@arg argument and boolean return value can suffice to implement
different aggregation types in the future.
Also fixes the check of NMAgentManager for secret flags for VPNs
(NM_CONNECTION_AGGREGATE_ANY_SYSTEM_SECRET_FLAGS). A secret for VPNs
is a property that either has a secret or a secret-flag. The previous
implementation would only look at present secrets and
check their flags. It wouldn't check secret-flags that are
NM_SETTING_SECRET_FLAG_NONE, but have no secret.
It's not yet used, but it will be. We will need nm_sd_utils_unbase64mem()
to strictly validate WireGuard settings, which contain keys in base64 encoding.
Note that we also need a stub implementation for logging. This will do
nothing for all logging from "libnm-systemd-shared.a". This makes
sense because "libnm.so" as a library should not log directly. Also,
"libnm.so" will only use a small portion of "libnm-systemd-shared.a" which
doesn't log anything. Thus this code is unused and dropped by the linker
with "--gc-sections".
glib has an base64 implementation, but g_base64_decode() et al. gives
no way to detect invalid encodings. All invalid codes are silently
ignored. That is not suitable for strictly validating user input.
Instead of reimplementing of copy-pasting the code from somewhere,
reuse systemd's unbase64mem().
But don't use "hexdecoct.h" directly. Instead, add a single accessor
function to our "nm-sd-utils-shared.h" gateway. We want to be careful
about which bits from systemd we use, because otherwise re-importing
systemd code becomes fragile as you don't know which relevant parts
changed.
For better or worse, we already pull in large parts of systemd sources.
I need a base64 decode implementation (because glib's g_base64_decode()
cannot reject invalid encodings). Instead of coming up with my own or
copy-paste if from somewhere, reuse systemd's unbase64mem().
But for that, make systemd's basic bits an independent static library
first because I will need it in libnm-core.
This doesn't really change anything except making "libnm-systemd-core.la"
an indpendent static library that could be used from "libnm-core". We
shall still be mindful about which internal code of systemd we use, and only
access functionality that is exposed via "systemd/nm-sd-utils-shared.h".
In core ("src/"), we use "nm-logging.h" for all logging. This dispatches
for logging to syslog, glog or systemd-journald.
If we want to log from a shared component under "shared/", we need to
use a common logging function. Add "nm-utils/nm-logging-fwd.h" for
forward declaring the used logging mechaism.
The shared library will still need to link with "src/nm-logging.c"
or an alternative implementation, depending on whether it is used
inside core or not.
We need more information what failed. Don't only return success/failure,
but an error number.
Note that we still don't actually return an error number. Only
the link_add() function is changed to return an nm-error integer.
Platform had it's own scheme for reporting errors: NMPlatformError.
Before, NMPlatformError indicated success via zero, negative integer
values are numbers from <errno.h>, and positive integer values are
platform specific codes. This changes now according to nm-error:
success is still zero. Negative values indicate a failure, where the
numeric value is either from <errno.h> or one of our error codes.
The meaning of positive values depends on the functions. Most functions
can only report an error reason (negative) and success (zero). For such
functions, positive values should never be returned (but the caller
should anticipate them).
For some functions, positive values could mean additional information
(but still success). That depends.
This is also what systemd does, except that systemd only returns
(negative) integers from <errno.h>, while we merge our own error codes
into the range of <errno.h>.
The advantage is to get rid of one way how to signal errors. The other
advantage is, that these error codes are compatible with all other
nm-errno values. For example, previously negative values indicated error
codes from <errno.h>, but it did not entail error codes from netlink.
There are various functions to set the DUID of a DHCPv6 client.
However, none of them allows to set arbitrary data. The closest is
sd_dhcp6_client_set_duid(), which would still do validation of the
DUID's content via dhcp_validate_duid_len().
Relax the validation and only log a debug message if the DUID
does not validate.
Note that dhcp_validate_duid_len() already is not very strict. For example
with DUID_TYPE_LLT it only ensures that the length is suitable to contain
hwtype and time. It does not further check that the length of hwaddr is non-zero
or suitable for hwtype. Also, non-well-known DUID types are accepted for
extensibility. Why reject certain DUIDs but allowing clearly wrong formats
otherwise?
The validation and failure should happen earlier, when accepting the
unsuitable DUID. At that point, there is more context of what is wrong,
and a better failure reason (or warning) can be reported to the user. Rejecting
the DUID when setting up the DHCPv6 client seems not optimal, in particular
because the DHCPv6 client does not care about actual content of the
DUID and treats it as opaque blob.
Also, NetworkManager (which uses this code) allows to configure the entire
binary DUID in binary. It intentionally does not validate the binary
content any further. Hence, it needs to be able to set _invalid_ DUIDs,
provided that some basic constraints are satisfied (like the maximum length).
sd_dhcp6_client_set_duid() has two callers: both set the DUID obtained
from link_get_duid(), which comes from configuration.
`man networkd.conf` says: "The configured DHCP DUID should conform to
the specification in RFC 3315, RFC 6355.". It does not not state that
it MUST conform.
Note that dhcp_validate_duid_len() has another caller: DHCPv4's
dhcp_client_set_iaid_duid_internal(). In this case, continue with
strict validation, as the callers are more controlled. Also, there is
already sd_dhcp_client_set_client_id() which can be used to bypass
this check and set arbitrary client identifiers.
ab4a88bc29
sd_dhcp_client_set_client_id() is the only API for setting a raw client-id.
All other setters are more restricted and only allow to set a type 255 DUID.
Also, dhcp4_set_client_identifier() is the only caller, which already
does:
r = sd_dhcp_client_set_client_id(link->dhcp_client,
ARPHRD_ETHER,
(const uint8_t *) &link->mac,
sizeof(link->mac));
and hence ensures that the data length is indeed ETH_ALEN.
Drop additional input validation from sd_dhcp_client_set_client_id(). The client-id
is an opaque blob, and if a caller wishes to set type 1 (ethernet) or type 32
(infiniband) with unexpected address length, it should be allowed. The actual
client-id is not relevant to the DHCP client, and it's the responsibility of the
caller to generate a suitable client-id.
For example, in NetworkManager you can configure all the bytes of the
client-id, including such _invalid_ settings. I think it makes sense,
to allow the user to fully configure the identifier. Even if such configuration
would be rejected, it would be the responsibility of the higher layers (including
a sensible error message to the user) and not fail later during
sd_dhcp_client_set_client_id().
Still log a debug message if the length is unexpected.
bfda0d0f09
Infiniband addresses are 20 bytes (INFINIBAND_ALEN), but only the last
8 bytes are suitable for putting into the client-id.
This bug had no effect for networkd, because sd_dhcp_client_set_client_id()
has only one caller which always uses ARPHRD_ETHER type.
I was unable to find good references for why this is correct ([1]). Fedora/RHEL
has patches for ISC dhclient that also only use the last 8 bytes ([2], [3]).
RFC 4390 (Dynamic Host Configuration Protocol (DHCP) over InfiniBand) [4] does
not discuss the content of the client-id either.
[1] https://bugzilla.redhat.com/show_bug.cgi?id=1658057#c29
[2] https://bugzilla.redhat.com/show_bug.cgi?id=660681
[3] 3ccf3c8d81/f/dhcp-lpf-ib.patch
[4] https://tools.ietf.org/html/rfc4390b9d8071458
When the link is up and goes down link_changed_cb() schedules
device_link_changed() to be run later. If the function is dispatched
when the link is already up again, it does not detect that the link
was down.
Fix this by storing in the device state that we saw the link down so
that device_link_changed() can properly restore the IP configuration.
https://bugzilla.redhat.com/show_bug.cgi?id=1636715https://github.com/NetworkManager/NetworkManager/pull/264
