A possible issue is that client_start() has about 136 arguments.
It doesn't get simpler by saving lines of code and writing them
all in the same line.
Wrap the lines.
While at it, use "FALSE" for "enforce_duid" argument, instead of "0".
It's a boolean.
(cherry picked from commit ce1cfd7232)
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.
(cherry picked from commit 7d55b1348b)
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.
(cherry picked from commit cd9e418fbe)
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().
(cherry picked from commit 7e341b73e0)
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.
(cherry picked from commit 025157d597)
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.
(cherry picked from commit 5411fb0cc6)
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.
(cherry picked from commit a55795772a)
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.
(cherry picked from commit 187d356198)
- 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.
(cherry picked from commit 50121ee028)
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.
(cherry picked from commit c51e63feb6)
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.
(cherry picked from commit 581e1c3269)
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.
(cherry picked from commit 8308311264)
- 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.
(cherry picked from commit e1413111a7)
The GChecksum API is cumbersome to use.
For example, g_checksum_get_digest() requires a length input/output
argument. At the same time, GChecksum does not allow you to query its
checksum-type nor the desired digest-length. When you have a GChecksum
at hand, you must always know the digest-length you are going to use.
So, the length parameter is only good for asserting.
Add a macro to make that more convenient.
Benefits: it's less lines of code, and we always do all the asserts
that are due.
(cherry picked from commit 3746845204)
- fix thread-safety by adding a memory barrier (g_atomic_pointer_get())
to the double-checked locking pattern when initializing the hash key.
- generate the random data outside the lock. Calling nm_utils_random_bytes()
within the lock is ugly, because we don't want to assume that the function
has no side effects which are prone to dead-lock. There is no problem attempting
to generate the random data without lock, and only use it when the race is won.
(cherry picked from commit 80220024cc)
We have a fork of a lot of useful systemd helper code.
However, until now we shyed away from using it aside from
the bits that we really need.
That means, although we have some really nice implementations
in our source-tree, we didn't use them. Either we were missing
them, or we had to re-implement them.
Add "nm-sd-utils.h" header to very carefully make internal
systemd API accessible to the rest of core.
This is not intended as a vehicle to access all of internal
API. Instead, this must be used with care, and only a hand picked
selection of functions must be exposed. Use with caution, but where it
makes sense.
(cherry picked from commit eece5aff09)
Add 3 variants of _nm_utils_hexstr2bin*():
- _nm_utils_hexstr2bin_full(), which takes a preallocated
buffer and fills it.
- _nm_utils_hexstr2bin_alloc() which returns a malloc'ed
buffer
- _nm_utils_hexstr2bin_buf(), which fills a preallocated
buffer of a specific size.
(cherry picked from commit be6c7fa5f6)
We already have nm_utils_bin2hexstr() and _nm_utils_bin2hexstr_full().
This is confusing.
- nm_utils_bin2hexstr() is public API of libnm. Also, it has
a last argument @final_len to truncate the string at that
length.
It uses no delimiter and lower-case characters.
- _nm_utils_bin2hexstr_full() does not do any truncation, but
it has options to specify a delimiter, the character case,
and to update a given buffer in-place. Also, like
nm_utils_bin2hexstr() and _nm_utils_bin2hexstr() it can
allocate a new buffer on demand.
- _nm_utils_bin2hexstr() would use ':' as delimiter and make
the case configurable. Also, it would always allocate the returned
buffer.
It's too much and confusing. Drop _nm_utils_bin2hexstr() which is internal
API and just a wrapper around _nm_utils_bin2hexstr_full().
(cherry picked from commit b537c0388a)
It's just more convenient, as it allows better chaining.
Also, allow passing %NULL as @out buffer. It's clear how
large the output buffer must be, so for convenience let the
function (optionally) allocate a new buffer.
This behavior of whether to
- take @out, fill it, and return @out
- take no @out, allocate new buffer, fill and and return it
is slightly error prone. But it was already error prone before, when
it would accept an input buffer without explicit buffer length. I think
this makes it more safe, because in the common case the caller can avoid
pre-allocating a buffer of the right size and the function gets it
right.
(cherry picked from commit 21df8d38ef)
While this is packaged in "NetworkManager-config-server.rpm"
sub-package, it's not in "00-server.conf" file. The reason
is that a convenient way to disable configuration from
"/usr/lib/NetworkManager/conf.d", is by putting a (possibly empty)
file into /etc directory with the same name. If the sub-package
only provides one large "00-server.conf" file, this is no longer
possible at a granular level.
(cherry picked from commit 7a46ccff00)
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
(cherry picked from commit b9eb264efe)
Often, during tests we want to assert against the logged messages.
In fact, most tests enable assertions for all logging and enforce
them with g_test_assert_expected_messages(). So, this is common.
However, sometimes it can be cumbersome to understand which logging
lines will be produced. For example, the next commits will call
nm_dhcp_manager_get() during the tests, which initializes NMDhcpManager
and logs a message which plugin was selected (or an additional warning,
if the selected plugin was not found). The availability of the DHCP plugin
depends on searching the path for "/usr/bin/dhclient", so from testing code
it's hard to determine what will be logged.
Instead, add a way to temporarily disable logging during testing.
(cherry picked from commit 35cecd32fd)
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.
(cherry picked from commit 5f4d8ffa79)
The entire point of using version 3/5 UUIDs is to generate
stable UUIDs based on a string. It's usually important that
we don't change the UUID generation algorithm later on.
Since we didn't have a version 5 implementation, we would always
resort to the MD5 based version 3. Version 5 is recommended by RFC 4122:
o Choose either MD5 [4] or SHA-1 [8] as the hash algorithm; If
backward compatibility is not an issue, SHA-1 is preferred.
Add a version 5 implementation so we can use it in the future.
All test values are generated with python's uuid module or OSSP uuid.
(cherry picked from commit 070a4d9355)
In RFC 4122, this is called "version 3", not "variant 3". While for
UUIDs there is also a concept of "variants", that is something else.
Fix naming.
(cherry picked from commit c150b0fa29)
We link against libuuid.so, but it was entirely internal to
libnm-core. We only exposed UUIDs in string form.
Add API to also handle UUIDs in binary form.
Note that libuuid already defines a type "uuid_t". However,
don't use it and instead use our own typedef NMUuid.
Reasons:
- uuid.h should be internal to libnm-core (nm-utils.c specifically),
and not be used by or exposed it other parts of the code.
- uuid_t is a typedef for a guchar[16] array. Typedefs
for arrays are confusing, because depending on whether
it's an automatic variable or a pointer in a function argument,
they behave differently regarding whether to take their address
or not and usage of "sizeof()".
(cherry picked from commit 88b081fce4)
g_string_new_len() allocates the buffer with length
bytes. Maybe it should be obvious (wasn't to me), but
if a init argument is given, that is taken as containing
length bytes.
So,
str = g_string_new_len (init, len);
is more like
str = g_string_new_len (NULL, len);
g_string_append_len (str, init, len);
and not (how I wrongly thought)
str = g_string_new_len (NULL, len);
g_string_append (str, init);
Fixes: 95b006c244
(cherry picked from commit 511709c54d)
In this way clients will randomly find that the AP list is older than
30 seconds and they will issue a new scan.
(cherry picked from commit 5ba301f4eb)
