We already had "${DEVICE}" which uses the interface name.
In times of predictable interface naming, that works well.
It allows the user to generate IDs per device which don't
change when the hardware is replaced.
"${MAC}" is similar, except that is uses the permanent MAC
address of the device. The substitution results in the empty
word, if the device has no permanent MAC address (like software
devices).
The per-device substitutions "${DEVICE}" and "${MAC}" are especially
interesting with "connection.multi-connect=multiple".
Our internal DHCP client (from systemd) defaults to a particular client ID.
It is currently exposed as nm_sd_utils_generate_default_dhcp_client_id()
and is based on the systemd implementation.
One problem with that is, that it internally looks up the interface name
with if_indextoname() and reads /etc/machine-id. Both makes it harder
for testing.
Another problem is, that this way of generating the client-id is
currently limited to internal client. Why? If you use dhclient plugin,
you may still want to use the same algorithm. Also, there is no explict
"ipv4.dhcp-client-id" mode to select this client-id (so that it could
be used in combination with "dhclient" plugin).
As such, this code will be useful also aside systemd DHCP plugin.
Hence, the function should not be obviously tied to systemd code.
The implementation is simple enough, and since we already have a
unit-test, refactor the code to our own implementation.
Internal DHCP client generates a default client ID. For one,
we should ensure that this algorithm does not change without
us noticing, for example, when upgrading systemd code. Add
a test, that the generation algorithm works as we expect.
Also note, that the generation algorithm uses siphash24().
That means, siphash24() implementation also must not change
in the future, to ensure the client ID doesn't change. As we
patch systemd sources to use shared/c-siphash, this is not
obviously the case. Luckily c-siphash and systemd's siphash24 do
agree, so all is good. The test is here to ensure that.
Also, previously the generation algorithm is not exposed as a
function, sd_dhcp_client will just generate a client-id when
it needs it. However, later we want to know (and set) the client
id before starting DHCP and not leave it unspecified to an
implementation detail.
This patch only adds a unit-test for the existing DHCP client
ID generation to have something for comparison. In the next
commit this will change further.
Tests might access the secret-key.
For CI builds we may very well build NM as root and also run
unit tests. In such a situation it's bad to persist the secret
key. For example, the SELinux label may be wrong, and subsequently
starting NetworkManager may cause errors. Avoid persisting the secret
key for tests.
Previously, whenever we needed /etc/machine-id we would re-load it
from file. The are 3 downsides of that:
- the smallest downside is the runtime overhead of repeatedly
reading the file and parse it.
- as we read it multiple times, it may change anytime. Most
code in NetworkManager does not expect or handle a change of
the machine-id.
Generally, the admin should make sure that the machine-id is properly
initialized before NetworkManager starts, and not change it. As such,
a change of the machine-id should never happen in practice.
But if it would change, we would get odd behaviors. Note for example
how generate_duid_from_machine_id() already cached the generated DUID
and only read it once.
It's better to pick the machine-id once, and rely to use the same
one for the remainder of the program.
If the admin wants to change the machine-id, NetworkManager must be
restarted as well (in case the admin cares).
Also, as we now only load it once, it makes sense to log an error
(once) when we fail to read the machine-id.
- previously, loading the machine-id could fail each time. And we
have to somehow handle that error. It seems, the best thing what we
anyway can do, is to log an error once and continue with a fake
machine-id. Here we add a fake machine-id based on the secret-key
or the boot-id. Now obtaining a machine-id can no longer fail
and error handling is no longer necessary.
Also, ensure that a machine-id of all zeros is not valid.
Technically, a machine-id is not an RFC 4122 UUID. But it's
the same size, so we also use NMUuid data structure for it.
While at it, also refactor caching of the boot-id and the secret
key. In particular, fix the thread-safety of the double-checked
locking implementations.
The need for this is the following:
"ipv4.dhcp-client-id" can be specified via global connection defaults.
In absence of any configuration in NetworkManager, the default depends
on the DHCP client plugin. In case of "dhclient", the default further
depends on /etc/dhcp.
For "internal" plugin, we may very well want to change the default
client-id to "mac" by universally installing a configuration
snippet
[connection-use-mac-client-id]
ipv4.dhcp-client-id=mac
However, if we the user happens to enable "dhclient" plugin, this also
forces the client-id and overrules configuration from /etc/dhcp. The real
problem is, that dhclient can be configured via means outside of NetworkManager,
so our defaults shall not overwrite defaults from /etc/dhcp.
With the new device spec, we can avoid this issue:
[connection-dhcp-client-id]
match-device=except:dhcp-plugin:dhclient
ipv4.dhcp-client-id=mac
This will be part of the solution for rh#1640494. Note that merely
dropping a configuration snippet is not yet enough. More fixes for
DHCP will follow. Also, bug rh#1640494 may have alternative solutions
as well. The nice part of this new feature is that it is generally
useful for configuring connection defaults and not specifically for
the client-id issue.
Note that this match spec is per-device, although the plugin is selected
globally. That makes some sense, because in the future we may or may not
configure the DHCP plugin per-device or per address family.
https://bugzilla.redhat.com/show_bug.cgi?id=1640494
Add a helper function for the common check whether a file is
inside a path. Also, this function handles special cases like
repeated file separators. However, as it is still entirely text
based, it also cannot recognize if two (literally) different
paths reference the same inode/file.
Ok, I changed my mind.
The new behavior seems to make more sense to me. Not that it matters,
because we always use nm_utils_strbuf*() API with buffers that we expect
to be large enough to contain the result. And when truncation occurs,
we usually don't care much about it. That is, there is no code that
uses nm_utils_strbuf*() API and handles string truncation in particular.
We commonly don't use the glib typedefs for char/short/int/long,
but their C types directly.
$ git grep '\<g\(char\|short\|int\|long\|float\|double\)\>' | wc -l
587
$ git grep '\<\(char\|short\|int\|long\|float\|double\)\>' | wc -l
21114
One could argue that using the glib typedefs is preferable in
public API (of our glib based libnm library) or where it clearly
is related to glib, like during
g_object_set (obj, PROPERTY, (gint) value, NULL);
However, that argument does not seem strong, because in practice we don't
follow that argument today, and seldomly use the glib typedefs.
Also, the style guide for this would be hard to formalize, because
"using them where clearly related to a glib" is a very loose suggestion.
Also note that glib typedefs will always just be typedefs of the
underlying C types. There is no danger of glib changing the meaning
of these typedefs (because that would be a major API break of glib).
A simple style guide is instead: don't use these typedefs.
No manual actions, I only ran the bash script:
FILES=($(git ls-files '*.[hc]'))
sed -i \
-e 's/\<g\(char\|short\|int\|long\|float\|double\)\>\( [^ ]\)/\1\2/g' \
-e 's/\<g\(char\|short\|int\|long\|float\|double\)\> /\1 /g' \
-e 's/\<g\(char\|short\|int\|long\|float\|double\)\>/\1/g' \
"${FILES[@]}"
Add new stable-id specifier "${DEVICE}" to explicitly declare that the
connection's identity differs per-device.
Note that for settings like "ipv6.addr-gen-mode=stable" we already hash
the interface's name. So, in combination with addr-gen-mode, using this
specifier has no real use. But for example, we don't do that for
"ipv4.dhcp-client-id=stable".
Point being, in various context we possibly already include a per-device
token into the generation algorithm. But that is not the case for all
contexts and uses.
Especially the DHCPv4 client identifier is supposed to differ between interfaces
(according to RFC). We don't do that by default with "ipv4.dhcp-client-id=stable",
but with "${DEVICE}" can can now be configured by the user.
Note that the fact that the client-id is the same accross interfaces, is not a
common problem, because profiles are usually restricted to one device via
connection.interface-name.
No longer rely on nm_connection_get_path() being meaningful in server.
It also was wrong. During update, nm_settings_connection_update()
would call
nm_utils_log_connection_diff (replace_connection, NM_CONNECTION (self), ...
where replace_connection has no path set, and nothing was logged.
Fix it, by explicitly passing the D-Bus path. Also, because
nm-core-utils.c should be independent of nm-dbus-object.h.
The matching works fuzzy and is not reliable. That is why we store
which connection should be assumed after restart in the state file
of NetworkManager.
In that case, we don't need to do a full check (with the possibility
of a false-reject). Just check for the minimum required properties:
the type and slave-type.
Yes, if the user modifies the connection while restarting NM, then
we might wrongly assume a connection that no longer would match.
But NM should not read minds, it should do as indicated.
nm_utils_exp10() is a better name, because it reminds of the function
exp10() from <math.h> which has a similar purpose (but whose argument
is double, not gint16).
Now we only search for a candiate with matching UUID. No need to
first lookup all activatable connections, just find the candidate
by UUID and see if it is activatable.
NMPolicy's auto_activate_device() wants to sort by autoconnect-priority,
nm_utils_cmp_connection_by_autoconnect_priority() but fallback to the default
nm_settings_connection_cmp_default(), which includes the timestamp.
Extend nm_settings_connection_cmp_default() to consider the
autoconnect-priority as well. Thus change behavior so that
nm_settings_connection_cmp_default() is the sort order that
auto_activate_device() wants. That makes sense, as
nm_settings_connection_cmp_default() already considered the
ability to autoconnect as first. Hence, it should also honor
the autoconnect priority.
When doing that, rename nm_settings_connection_cmp_default()
to nm_settings_connection_cmp_autoconnect_priority().
Have a proper cmp() function and a wrapper *_p_with_data() that can be
used for g_qsort_with_data().
Thus, establish a naming scheme (*_p_with_data()) for these compare
wrappers that we need all over the place. Note, we also have
nm_strcmp_p_with_data() for the same reason and later more such
functions will follow.
The -Wimplicit-fallthrough=3 warning is quite flexible of accepting
a fall-through warning.
Some comments were missing or not detected correctly.
Thereby, also change all other comments to follow the exact
same pattern.
Previously, we would have different functions like
- nm_match_spec_device_type()
- nm_match_spec_hwaddr()
- nm_match_spec_s390_subchannels()
- nm_match_spec_interface_name()
which all would handle one type of match-spec.
So, to get the overall result whether the arguments
match or not, nm_device_spec_match_list() had to stich
them together and iterate the list multiple times.
Refactor the code to have one nm_match_spec_device()
function that gets all relevant paramters.
The upside is:
- the logic how to evaluate the match-spec is all at one place
(match_device_eval()) instead of spread over multiple
functions.
- It requires iterating the list at most twice. Twice, because
we do a fast pre-search for "*".
One downside could be, that we have to pass all 4 arguments
for the evaluation, even if the might no be needed. That is,
because "nm-core-utils.c" shall be independend from NMDevice, it
cannot receive a device instance to get the parameters as needed.
As we would add new match-types, the argument list would grow.
However, all arguments are cached and fetching them from the
device's private data is very cheap.
(cherry picked from commit b957403efd)
Usecase: when connecting to a public Wi-Fi with MAC address randomization
("wifi.cloned-mac-address=random") you get on every re-connect a new
IP address due to the changing MAC address.
"wifi.cloned-mac-address=stable" is the solution for that. But that
means, every time when reconnecting to this network, the same ID will
be reused. We want an ID that is stable for a while, but at a later
point a new ID should e generated when revisiting the Wi-Fi network.
Extend the stable-id to become dynamic and support templates/substitutions.
Currently supported is "${CONNECTION}", "${BOOT}" and "${RANDOM}".
Any unrecognized pattern is treated verbaim/untranslated.
"$$" is treated special to allow escaping the '$' character. This allows
the user to still embed verbatim '$' characters with the guarantee that
future versions of NetworkManager will still generate the same ID.
Of course, a user could just avoid '$' in the stable-id unless using
it for dynamic substitutions.
Later we might want to add more recognized substitutions. For example, it
could be useful to generate new IDs based on the current time. The ${} syntax
is extendable to support arguments like "${PERIODIC:weekly}".
Also allow "connection.stable-id" to be set as global default value.
Previously that made no sense because the stable-id was static
and is anyway strongly tied to the identity of the connection profile.
Now, with dynamic stable-ids it gets much more useful to specify
a global default.
Note that pre-existing stable-ids don't change and still generate
the same addresses -- unless they contain one of the new ${} patterns.
We'll soon not only do the router discovery, but announce ourselves as a
reouter. "Neighbor discovery" sounds to be a more appropriate name for
the class than "Router discovery".
When a reverse DNS entry must be added to dnsmasq, instead of
considering IP addresses as classful use the prefix to compute one or
more "in-addr.arpa" according to CIDR rules.
https://bugzilla.gnome.org/show_bug.cgi?id=767174
A large part of "nm-test-utils.h" is only relevant for tests inside "src/"
directory, as they are helpers related to NetworkManager core part.
Split this part out of "nm-test-utils.h" header.
Duplicated const specifiers are allowed by C99 and can easily
happen in macros. Also, systemd's interal code will use them.
Disable this warning, it doesn't seem useful.
- All internal source files (except "examples", which are not internal)
should include "config.h" first. As also all internal source
files should include "nm-default.h", let "config.h" be included
by "nm-default.h" and include "nm-default.h" as first in every
source file.
We already wanted to include "nm-default.h" before other headers
because it might contains some fixes (like "nm-glib.h" compatibility)
that is required first.
- After including "nm-default.h", we optinally allow for including the
corresponding header file for the source file at hand. The idea
is to ensure that each header file is self contained.
- Don't include "config.h" or "nm-default.h" in any header file
(except "nm-sd-adapt.h"). Public headers anyway must not include
these headers, and internal headers are never included after
"nm-default.h", as of the first previous point.
- Include all internal headers with quotes instead of angle brackets.
In practice it doesn't matter, because in our public headers we must
include other headers with angle brackets. As we use our public
headers also to compile our interal source files, effectively the
result must be the same. Still do it for consistency.
- Except for <config.h> itself. Include it with angle brackets as suggested by
https://www.gnu.org/software/autoconf/manual/autoconf.html#Configuration-Headers
When NM tries to match a generated connection to a persistent one, it
considers also the metric of static routes. However, if the field is
set to -1 (use default value for the device) on the persistent
connection, the comparison will always fail because the generated
connection contains the actual value read from kernel.
To fix the issue, modify check_possible_match() to deal correctly with
-1 and translate it to the expected value for the current device when
performing the comparison.
This allows connections with static routes and default metric to
properly be re-assumed when NM is restarted.
https://bugzilla.redhat.com/show_bug.cgi?id=1302532
When a connection should be assumed and the generated connection did not
contain a wired setting, the connection did not match due to S390 properties.
Such a connection should be allowed to match to a connection with a wired
setting with default (empty) S390 properties.
This can happen when there is a VLAN profile configured that contains a wired
setting in it and NetworkManager is (re)started.
Example/reproducer:
$ nmcli con add type vlan con-name vlan-test autoconnect no dev em1 id 44
$ nmcli con mod vlan-test eth.mtu 1450 (modify the connection, so that it has a wired setting)
$ nmcli con up vlan-test (activate the connection)
$ sudo systemctl restart NetworkManager
$ nmcli device
check that 'vlan-test' connection is active on em1.44 device
(and not the auto-generated em1.44)
https://bugzilla.redhat.com/show_bug.cgi?id=1276343
