We don't do that for ip4_start() either. The duid/client-id
is stored inside the NMDhcpClient instance, and the function can
access it from there.
Maybe, it is often preferable to have stateless objects and not
relying on ip4_start() to obtain the client ID from the client's
state. However, the purpose of the NMDhcpClient object is to
hold state about DHCP. To simplify the complexity of objects that
inherrently have state, we should be careful about mutating the state.
It adds little additional complexity of only reading the state when
needed anyway. In fact, it adds complexity, because previously
it wasn't enough to check all callers of nm_dhcp_client_get_client_id()
to see where the client-id is used. Instead, one would also need to
follow the @duid argument several layers of the call stack.
There should be lower layers that are concerned with writing
and reading dhclient configuration files. It's wrong to
have a nm_dhcp_dhclient_save_duid() function which requires
the caller to pre-escape the string to write. The caller shouldn't
be concerned with the file format, that's why the function
is used in the first place.
We only used "client_id" for IPv4 and "duid" for IPv6. Merge them.
Another advantage is, that we can share the logging functionality
of _set_client_id().
Drop unused function.
Aside from that, dhclient configuration files support a very complex
syntax. The parser was very naive and insufficient in parsing such
files. It's good we can just drop it.
Why would we do this? The configuration file we are reading back was
written by NetworkManager in the first place.
Maybe when assuming a connection after restart, this information could
be interesting. It however is not actually relevant.
Note how nm_dhcp_client_get_client_id() has only very few callers.
- nm_device_spawn_iface_helper() in 'nm-device.c'. In this case,
we either should use the client-id which we used when starting
DHCP, or none at all.
- ip4_start() in 'nm-dhcp-dhclient.c', but this is before starting
DHCP client and before it was re-read from configuration file.
- in "src/dhcp/nm-dhcp-systemd.c", but this has no effect for
the dhclient plugin.
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.
- use NMUuid type where appropriate.
- no error handling for generate_duid_from_machine_id().
It cannot fail anymore.
- add thread-safety to generate_duid_from_machine_id() with
double-checked locking.
- use unions for converting the sha256 digest to the target
type.
For testing purpose, it's bad to let nm_utils_stable_id_parse()
directly access nm_utils_get_boot_id_str(). Instead, the function
should have no side-effects.
Since the boot-id is anyway cached, accessing it is cheap. Even
if it likely won't be needed.
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.
- add a commnt about thread-safety.
- minor refactoring initializing the value in nm_utils_get_testing().
Instead of returning the flags we just set, go back to the begin
and re-read the value (which must be initialized by now). No big
difference, but feels a bit nicer to me.
In the past, the headers "linux/if.h" and "net/if.h" were incompatible.
That means, we can either include one or the other, but not both.
This is fixed in the meantime, however the issue still exists when
building against older kernel/glibc.
That means, including one of these headers from a header file
is problematic. In particular if it's a header like "nm-platform.h",
which itself is dragged in by many other headers.
Avoid that by not including these headers from "platform.h", but instead
from the source files where needed (or possibly from less popular header
files).
Currently there is no problem. However, this allows an unknowing user to
include <net/if.h> at the same time with "nm-platform.h", which is easy
to get wrong.
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
Most singletons can only be instantiated once (unless NM_DEFINE_SINGLETON_ALLOW_MULTIPLE
is defined). Otherwise, an assertion will be triggered if the singleton is destroyed
and another instance is requested.
For testing, we want to create multiple singleton instances and being able to reset
the singleton getter. Add a function for that.
This doesn't change anything in the generated source, but I think makes
semantically more sense, as these structures have undefined size, and we
only want to know the size up to the data field in these cases.
(cherry picked from commit 20b55f853847378b85561a4e299604d27b5cd25b)
For upstream, we changed behavior here. However, I think certain
downstream don't want to do that, and revert patch "d37ad15f12 keyfile:
also add ".nmconnection" extension when writing keyfiles in /etc".
For that to make easier, keep the upstream sources closer to what
was. Revert.
This reverts commit e93d8cdb74.
is_available would recently return true after IWD had disconnected
if a connection was active because it would check that
priv->dbus_station_proxy was non-NULL (i.e. that the DBus interface was
still visible, which it wasn't) but that check would be overridden
if the NMDevice state was activated. Now require priv->dbus_obj to be
non-NULL, which would even be enough on its own although I'm leaving the
previous check there too to catch potential IWD states we don't support
in which priv->dbus_station_proxy is NULL without an active connection.
Sanity check networks received from the Station.GetOrderedNetworks()
DBus method. Duplicates shouldn't happen but the code should be safe
against bogus data received over DBus. There was a recent bug in a
library used by IWD causing occasional duplicates to be returned which
would cause invalid memory accesses reported by valgrind in NM because
g_hash_table_insert would free what we passed as the key.
Watch for connection-removed events and delete corresponding IWD network
configs if found. This mainly changes anything for 802.1X networks
where the deleted NM connections might annoyingly re-appear after a
restart. For PSK networks though it'll make IWD forget the password
which, until now, would be remembered by IWD even if it was removed or
changed in the NM profile, which is a bug.
This is still fragile because we don't handle "connection-updated"
events so the data->mirror_connection pointer for a known network record
may after some time point to an NMSettingsConnection with a different
SSID or security type and there are corner cases where the IWD-side
profile will not be forgotten. At least I'm trying to make sure we
don't crash and don't wrongly remove any IWD profile which could also be
annoying for complicated EAP configs.
Something is possibly wrong with the DBus signal handling if a newly
added KnownNetwork interface already has an entry in
priv->known_networks, but since we handle this case add a warning and
update the GDBusProxy pointer for that existing entry.
interface_added expects mirror_8021x_connection() to return the pointer to
the existing connection if one exists, and NULL on error, rather than
NULL if a conneciton exists. While touching that, add logic to return
specifically a connection with EAP method set to 'external' if one
exists even though this should not affect any other logic we have
currently.
Some internal logic causes the secrets in a connection to be
occasionally moved to NMSettingConnection's priv->system_secrets after a
connection attempt so we need to use nm_act_request_get_secrets to get
them added to the device's settings connection and applied connection if
the PSK is missing during an AP or AdHoc mode activation (in
infrastructure mode we already do secret requests though they're cached
by IWD in most cases).
The common steps for the PSK available and unavailable scenarios is moved
from act_stage2_config to act_set_mode.
This simplifies the code by using g_variant_lookup. In this handler
where we parse more than one property this is probably slower although
the number of string comparisons will be the same.
