Move back to INIT state after the lease expires, as per section 4.4.5
of RFC 2131. Previously the client just moved to EXPIRED, closed the
connection and cleared the probe, leaving to the caller of the library
the choice to create a new client instance and to start from
scratch. However, it seems more useful that the client, once
initialized, always tries to get a lease even after an expiration.
We should use the same "is-valid" function everywhere.
Since nm_utils_ipaddr_valid() is part of libnm, it does not qualify.
Use nm_utils_ipaddr_is_valid() instead.
This should give the compiler more possibilities to warn about wrong
use of the API.
In practice, my current compiler wouldn't flag any issues. However,
some compilers (or compile options) might.
nmtstc_client_new() exists to test creating a GInitiable/GAsyncInitiable
in different GMainContext combinations.
This is not only useful for NMClient but will also be useful for
NMSecretAgentOld. Add nmtstc_context_object_new() to allow for that.
Also, allow passing parameters when creating the object.
The resulting nmtstc_context_object_new() is relatively complex. But
this is only testing code, that aims to construct the respective GObject
instance in various manners (randomly using the sync or async initialization).
It is complex, but delivers at testing various code paths of the
underlying code. The API that it provides however is simple.
Also drop _nmtstc_client_new_extra_context() to create the instance with
a different context. For one, this requires that the internal context is
integrated as long as the context-busy-watcher exists. That was not
handled correctly. Also, creating a NMClient instance with a different
context than the current thread default at construct time has
implications to the test later. The tests don't want this variant, and
don't handle them properly. So drop this.
nmtst_main_context_iterate_until*() iterates until the condition is
satisfied. If that doesn't happen within timeout, it fails an assertion.
Rename the function to make that clearer.
If the server sends a packet with multiple instances of the same
option, they are concatenated during n_dhcp4_incoming_linearize() and
evaluated as a single option as per section 7 of RFC 3396.
However, there are broken server implementations that send
self-contained options in multiple copies. They are reassembled to
form a single instance by the nettools client, which then fails to
parse them because they have a length greater than the expected one.
This problem can be reproduced by starting a server with:
dnsmasq --bind-interfaces --interface veth1 -d
--dhcp-range=172.25.1.100,172.25.1.200,1m
--dhcp-option=54,172.25.1.1
In this way dnsmasq sends a duplicate option 54 (server-id) when the
client requests it in the 'parameter request list' option, as
dhcp=systemd and dhcp=nettools currently do.
While this is a violation of the RFC by the server, both isc-dhcp and
systemd-networkd client implementations have mechanisms to deal with
this situation. dhclient simply takes the first bytes of the
aggregated option. systemd-networkd doesn't follow RFC 3396 and
doesn't aggregate multiple options; it considers only the last
occurrence of each option.
Change the parsing code to accept options that are longer than
necessary.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/issues/324
It seems to complicate things more than helping. Drop it. What we still have
is a wrapper around plain g_dbus_connection_signal_subscribe(). That one is
trivial and helpful. The previous wrapper seems to add more complexity.
"nm-dbus-compat.h" was GPL licensed. That is a problem, because we use it from
libnm (which is LGPL).
The history of this file in NetworkManager source tree:
$ git shortlog -n -s e055bdbbc3 -- shared/nm-std-aux/nm-dbus-compat.h include/nm-dbus-compat.h shared/nm-dbus-compat.h
5 Thomas Haller
1 Dan Winship
1 Lubomir Rintel
Note that commit dd0e198955 ('include: add nm-dbus-compat.h')
introduced this file from dbus sources ([1]). Hence, originally
the file is (like all of dbus sources) dual-licensed under GPL-2.0+
and Academic Free License 2.1 (AFL-2.1). That makes it problematic to
change the license of this file to LGPL also because of the old history
of the file.
Instead, drop everything from the header except the bits that we
actually use. I claim the remainder is trivial and only contains
defines for documented D-Bus API. I don't think that the remainder
is copyrightable and hence get rid of the copy-right notice and the
GPL license.
[1] 39ea37b587/dbus/dbus-shared.h
The purpose is to clear the entire available buffer, not only
up to the first '\0'. This is done, because otherwise we might
leak sensitive data that happens to be after the first '\0',
or we might give away the length of the secrets.
Of course, those are very (very) minor concerns. But avoiding them is
easy enough.
The latter requires __auto_type which is not available in GCC versions
older than 4.9. Fix the following compile error on RHEL 7.8:
CC src/src_libNetworkManagerBase_la-NetworkManagerUtils.lo
shared/n-dhcp4/src/n-dhcp4-c-probe.c: In function 'n_dhcp4_client_probe_transition_nak':
shared/n-dhcp4/src/n-dhcp4-c-probe.c:1008:17: error: unknown type name '__auto_type'
probe->ns_nak_restart_delay = c_clamp(probe->ns_nak_restart_delay * 2,
^
shared/n-dhcp4/src/n-dhcp4-c-probe.c:1008:17: error: unknown type name '__auto_type'
shared/n-dhcp4/src/n-dhcp4-c-probe.c:1008:17: error: unknown type name '__auto_type'
Fixes: 218782a9a3 ('n-dhcp4: restart the transaction after a NAK')
It is not enough to set the INIT state after a NAK; a timeout
(ns_deferred) must be set so that it is added to the event fd. The
client retries immediately the first time, so that in the successful
case it gets an address quickly. To avoid flooding the network in case
of servers always replying with NAKs, next attempts are done with
intervals from 2 seconds to 5 minutes using exponential backoff. See
also systemd commit [1].
[1] 1d1a3e0afbhttps://gitlab.freedesktop.org/NetworkManager/NetworkManager/issues/325
When the client enters the INIT state, it calls listen() on the
connection connection to create the packet socket. However, if the
client is coming from the REBOOTING state after a NAK, the connection
is already in the listening state; do nothing in such case.
Keyfile support was initially added under GPL-2.0+ license as part of
core. It was moved to "libnm-core" in commit 59eb5312a5 ('keyfile: merge
branch 'th/libnm-keyfile-bgo744699'').
"libnm-core" is statically linked with by core and "libnm". In
the former case under terms of GPL-2.0+ (good) and in the latter case
under terms of LGPL-2.1+ (bad).
In fact, to this day, "libnm" doesn't actually use the code. The linker
will probably remove all the GPL-2.0+ symbols when compiled with
gc-sections or LTO. Still, linking them together in the first place
makes "libnm" only available under GPL code (despite the code
not actually being used).
Instead, move the GPL code to a separate static library
"shared/nm-keyfile/libnm-keyfile.la" and only link it to the part
that actually uses the code (and which is GPL licensed too).
This fixes the license violation.
Eventually, it would be very useful to be able to expose keyfile
handling via "libnm". However that is not straight forward due to the
licensing conflict.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/merge_requests/381
We cannot know the key/value free functions, hence, our compat implementation
cannot free the values if they are not requested. The "solution" is to require
the caller to fetch all values, always.
Move it to shared as it's useful for clients as well.
Move and rename nm_dbus_manager_new_auth_subject_from_context() and
nm_dbus_manager_new_auth_subject_from_message() in nm-dbus-manager.c
as they're needed there.
Instead of terminating the program when the dispatch function returns
an invalid return code, log an error message and convert the error
code to a valid, generic one.
https://bugs.archlinux.org/task/64880
After t1, the client tries to renew the lease by contacting via the
udp socket the server specified in the server-id option. If this
fails, after t2 it tries to contact any server using broadcast. For
this to work, the packet socket must be used.
Currently the client always starts from the INIT state (i.e. sending a
discover message). If a requested-ip was specified by the caller, it
is added as an option in the discover.
It was reported that some DHCP servers don't respond to discover
messages with the requested-ip option set [1][2].
The RFC allows to skip the discover by entering the INIT-REBOOT state
and starting directly with a broadcast request message containing the
requested IP address. Implement that.
[1] https://bugzilla.redhat.com/show_bug.cgi?id=1781856
[2] https://gitlab.freedesktop.org/NetworkManager/NetworkManager/issues/310
Found by covscan:
NetworkManager-1.22.0/shared/nm-glib-aux/nm-dbus-aux.c:361:
missing_va_end: va_end was not called for "ap".
Fixes: ce36494c0a ('shared: add nm_dbus_error_is() helper')
"nm-glib-aux/nm-logging-fwd.h" provides macros like _LOGD() to be reused
by various parts which implement logging (by defining _NMLOG() accordingly).
libnm also has logging, however it uses different logging levels
aside LOGD_DEBUG.
Instead, implement _LOGD() using a define _LOGL_DEBUG, so that libnm can
redefine thos _LOGL_DEBUG defines and use the _LOGD() macro.
