If the client-id has been set to "none", the DHCP client-id option
(option 61) mustn't be sent. Honor this when the dhclient plugin is
used.
If dhclient has been called with the -i option (Use a DUID with DHCPv4
clients), it will send a Client-ID even without setting one in dhclient.conf.
In this case, this option needs to be explicitly overwritten with:
send dhcp-client-identifier = "";
At least in RHEL 8, dhclient is launched with `-i` turned on by default.
The function merge_dhclient_config was called only once from
create_dhclient_config. The content of both of them is short and simple,
so moving the content from merge_dhclient_config to the caller
improves the readability and makes the functions call chain easier to
follow. Also, both functions takes a long list of arguments which are
almost the same, so we can avoid having to pass them over and over in a
long call chain.
Sending a client-id is not mandatory according to RFC2131. It is
mandatory according to RFC4361 that superseedes it.
Some weird DHCP servers conforming RFC2131 can get confused and break
existing DHCP leases if they start receiving a client-id when it was not
being previously received. Users that were using other DHCP client like
dhclient, but want to use NetworkManager's internal DHCP client, can
suffer this problem.
Add "none" as accepted value in ipv4.dhcp-client-id to specify that
client-id must not be sent. Note that this is generally not recommended
unless it's explicitly needed for some reason like the explained above.
Client-id is mandatory in DHCPv6.
This commit allow to set the "none" value and properly parse it in the
NMDhcpClientConfig struct. Next commits will modify the different DHCP
plugins to honor it.
We don't know the reason why the DHCP client is being stopped. It is
wrong to schedule a commit of type "update" because the device could
be now unmanaged. Schedule instead a commit of type "auto", which
automatically determines the type of commit based on registered
handles.
Arguably, a kernel link is needed for DHCP and so the interface name
univocally identifies a device (for example, the OVS interface). But
for consistency and clarity, store the device type to be used for
logging.
If the client was waiting for IPv6 DAD to complete and the lease was
updated or lost, `wait_ipv6_dad` needs to be cleared; otherwise, at
the next platform change the client will try to evaluate the DAD state
with a different or no lease. In particular if there is no lease the
client will try to decline it because there are no valid addresses,
leading to an assertion failure:
../src/core/dhcp/nm-dhcp-client.c:997:_dhcp_client_decline: assertion failed: (l3cd)
Backtrace:
__GI_raise ()
__GI_abort ()
g_assertion_message ()
g_assertion_message_expr ()
_dhcp_client_decline (self=0x1af13b0, l3cd=0x0, error_message=0x8e25e1 "DAD failed", error=0x7ffec2c45cb0) at ../src/core/dhcp/nm-dhcp-client.c:997
l3_cfg_notify_cb (l3cfg=0x1bc47f0, notify_data=0x7ffec2c46c60, self=0x1af13b0) at ../src/core/dhcp/nm-dhcp-client.c:1190
g_closure_invoke ()
g_signal_emit_valist ()
g_signal_emit ()
_nm_l3cfg_emit_signal_notify () at ../src/core/nm-l3cfg.c:629
_nm_l3cfg_notify_platform_change_on_idle () at ../src/core/nm-l3cfg.c:1390
_platform_signal_on_idle_cb () at ../src/core/nm-netns.c:411
g_idle_dispatch ()
Fixes: 393bc628ff ('dhcp: wait DAD completion for DHCPv6 addresses')
https://bugzilla.redhat.com/show_bug.cgi?id=2179890https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1594
We already get the IAID from the dhclient environment. This is actually
rather useful, because dhclient plugin does not support setting the
value (that is, what we request in "config.v6.iaid" is not actually
used). Already previously, was the IAID for dhclient present in the
lease information. Now also normalize/verify it.
Expose the used IAID also with the internal (systemd) plugin. There we
explicitly set the IAID and know it.
Our lease is tracked in a plain string dictionary. For dhclient plugin
and similar, the keys are received via the environment, they are thus
unlimited. For the internal plugins they are known at compile time and
static strings. We thus sometimes need to clone the string, and
sometimes not.
Unfortunately, we cannot ask the GHashTable whether it has a free
function for the key, so we need to explicitly tell it. Add a parameter
for that.
New files must be written to the build directory, not to the source
one.
Fixes: 5ee2f3d1dc ('dhcp/tests: refactor tests for nm_dhcp_dhclient_save_duid()')
When the connection setting changes at the first place, then calling
the device reapply, the ip address got temporarily removed when DHCP
restarted. To avoid the ip address got temporarily removed, we should
preserve the previous lease and keep using it until the new lease comes
along.
Previously, we only set the "default-duid" line in the lease file. That
means, if the lease already contained a matching entry with a
"dhcp6.client-id" option, it was not honored. That is wrong.
If the profile has "ipv6.dhcp-duid" set, then we must use it and get
rid of those options from the lease.
It's easy to reproduce:
PROFILE=eth1
nmcli connection down "$PROFILE"
rm -f /var/lib/NetworkManager/*lease
nmcli connection modify "$PROFILE" ipv6.dhcp-duid "aa:bb:cc:dd:00:00:11"
nmcli connection up "$PROFILE"
# Verify the expected duid in /var/lib/NetworkManager/*lease and "/run/NetworkManager/devices/$IFINDEX"
nmcli connection modify "$PROFILE" ipv6.dhcp-duid "aa:bb:cc:dd:00:00:22"
nmcli connection up "$PROFILE"
# Check the DUID again.
Splitting by any of "\r\n" and then joining the lines with "\n"
leads to double-newlines. That's certainly wrong.
Maybe we shouldn't care about "\r", I don't know why this was done. But
handle it differently.
Of course, the old "priv->effective_client_id" and the new
"client_id" instances are truly separate, that is, they don't
share data, and destroying "priv->effective_client_id" before
taking a reference on "client_id" causes no problem.
It's still a code smell. It makes the function unnecessarily unsafe
under (very unusual) circumstances.
The point of using this trivial helper function is to have one function
that is related to the construction of the options dictionary, that we
can search for.
It answers the question, where do we create a option hash (at `git grep
nm_dhcp_option_create_options_dict`).
The "lease" mode is unusual, because it means to prefer the DUID
configuration from the DHCP plugin over the explicit configuration in
NetworkManager. It is only for the DHCPv6 DUID and not for the IPv4
client-id. It also is only special for the "dhclient" plugin, because
with the internal plugin, this always corresponds to a generated, stable
DUID.
Commit 58287cbcc0 ('core: rework IP configuration in NetworkManager
using layer 3 configuration') broke this. The commit refactored the code
to track the effective-client-id separately. Previously, the client-id which
was read from the dhclient lease, was overwriting NMDhcpClient.client_id. But
with the refactor, it broke because nm_dhcp_client_get_effective_client_id()
was never called.
Fix that.
Fixes: 58287cbcc0 ('core: rework IP configuration in NetworkManager using layer 3 configuration')
Note that there are no callers of nm_dhcp_client_get_effective_client_id(),
hence calling the setter had no effect. This is a bug, that we will fix
later.
But before fixing the bug, change how this works. Drop the get_duid() hook.
It's only confusing and backward.
We will keep the nm_dhcp_client_[gs]et_effective_client_id() functions.
They will be used later.
The "effective-client-id" is handled wrongly. Step 1 to clean this up.
Note that NMDhcpClientPrivate.effective_client_id is only ever get/set
via the nm_dhcp_client_[gs]et_effective_client_id() functions.
Note that only a NMDhcpDhclient instance ever calls
nm_dhcp_client_set_effective_client_id().
Hence, for NMDhcpSystemd the effective-client-id is really just the DUID
from the config. Clean this up by not calling nm_dhcp_client_get_effective_client_id()
but use the config directly. There is no change in behavior here.
G_TYPE_CHECK_INSTANCE_CAST() can trigger a "-Wcast-align":
src/core/devices/nm-device-macvlan.c: In function 'parent_changed_notify':
/usr/include/glib-2.0/gobject/gtype.h:2421:42: error: cast increases required alignment of target type [-Werror=cast-align]
2421 | # define _G_TYPE_CIC(ip, gt, ct) ((ct*) ip)
| ^
/usr/include/glib-2.0/gobject/gtype.h:501:66: note: in expansion of macro '_G_TYPE_CIC'
501 | #define G_TYPE_CHECK_INSTANCE_CAST(instance, g_type, c_type) (_G_TYPE_CIC ((instance), (g_type), c_type))
| ^~~~~~~~~~~
src/core/devices/nm-device-macvlan.h:13:6: note: in expansion of macro 'G_TYPE_CHECK_INSTANCE_CAST'
13 | (G_TYPE_CHECK_INSTANCE_CAST((obj), NM_TYPE_DEVICE_MACVLAN, NMDeviceMacvlan))
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
Avoid that by using _NM_G_TYPE_CHECK_INSTANCE_CAST().
This can only be done for our internal usages. The public headers
of libnm are not changed.
"connection" variable might be NULL, which fails an assertion in
g_dbus_connection_flush_sync(). Consequently, "error_flush" is also
NULL which leads to a crash of "nm-dhcp-helper".
Reported-by: Jules Maselbas <jmaselbas@zdiv.net>
Fixes: 240ec7f891 ('dhcp: implement ACD (address collision detection) for DHCPv4')
This is the version shipped in Fedora 37. As Fedora 37 is now out, the
core developers switch to it. Our gitlab-ci will also use that as base
image for the check-{patch.tree} tests and to generate the pages. There
is a need that everybody agrees on which clang-format version to use,
and that version should be the one of the currently used Fedora release.
Also update the used Fedora image in "contrib/scripts/nm-code-format-container.sh"
script.
The gitlab-ci still needs update in the following commit. The change
in isolation will break the "check-tree" test.
The DNS name can now also contain the DoT server name. It's not longer a
binary IP address only.
Extend NML3ConfigData to account for that. To track the additional
data, use the string representation. The alternative to have a separate
type that contains the parsed information would be cumbersome too.
Instead of assuming any address that disappeared was because of a DAD
failure, check explicitly that either:
- the address is still present with DADFAILED flag (in case it was a
permanent address), or
- the address was removed and platform recorded that it had the
DADFAILED flag.
These variants provide additional nm_assert() checks, and are thus
preferable.
Note that we cannot just blindly replace &g_array_index() with
&nm_g_array_index(), because the latter would not allow getting a
pointer at index [arr->len]. That might be a valid (though uncommon)
usecase. The correct replacement of &g_array_index() is thus
nm_g_array_index_p().
I checked the code manually and replaced uses of nm_g_array_index_p()
with &nm_g_array_index(), if that was a safe thing to do. The latter
seems preferable, because it is familar to &g_array_index().
The dhclient plugin already supports sending a decline when IPv4 ACD
fails. Also implement support for IPv6 DAD.
See-also: 156d84217c ("dhcp/dhclient: implement accept/decline (ACD) for dhclient plugin")
Currently we accept the DHCPv6 just after addresses are configured on
kernel, without waiting DAD result. Instead, wait that DAD completes
and decline the lease if all addresses are detected as duplicate.
Note that when an address has non-infinite lifetime and fails DAD,
kernel removes it automatically. With iproute2 we see something like:
602: testX6 inet6 2620:🔢5678/128 scope global tentative dynamic noprefixroute
valid_lft 7500sec preferred_lft 7200sec
Deleted 602: testX6 inet6 2620:🔢5678/128 scope global dadfailed tentative dynamic noprefixroute
valid_lft 7500sec preferred_lft 7200sec
Since the address gets removed from the platform cache, at the moment
we don't have a way to check the flags of the removal
message. Therefore, we assume that any address that goes away in
tentative state was detected as duplicate.
https://bugzilla.redhat.com/show_bug.cgi?id=2096386
- name things related to `in_addr_t`, `struct in6_addr`, `NMIPAddr` as
`nm_ip4_addr_*()`, `nm_ip6_addr_*()`, `nm_ip_addr_*()`, respectively.
- we have a wrapper `nm_inet_ntop()` for `inet_ntop()`. This name
of our wrapper is chosen to be familiar with the libc underlying
function. With this, also name functions that are about string
representations of addresses `nm_inet_*()`, `nm_inet4_*()`,
`nm_inet6_*()`. For example, `nm_inet_parse_str()`,
`nm_inet_is_normalized()`.
<<<<
R() {
git grep -l "$1" | xargs sed -i "s/\<$1\>/$2/g"
}
R NM_CMP_DIRECT_IN4ADDR_SAME_PREFIX NM_CMP_DIRECT_IP4_ADDR_SAME_PREFIX
R NM_CMP_DIRECT_IN6ADDR_SAME_PREFIX NM_CMP_DIRECT_IP6_ADDR_SAME_PREFIX
R NM_UTILS_INET_ADDRSTRLEN NM_INET_ADDRSTRLEN
R _nm_utils_inet4_ntop nm_inet4_ntop
R _nm_utils_inet6_ntop nm_inet6_ntop
R _nm_utils_ip4_get_default_prefix nm_ip4_addr_get_default_prefix
R _nm_utils_ip4_get_default_prefix0 nm_ip4_addr_get_default_prefix0
R _nm_utils_ip4_netmask_to_prefix nm_ip4_addr_netmask_to_prefix
R _nm_utils_ip4_prefix_to_netmask nm_ip4_addr_netmask_from_prefix
R nm_utils_inet4_ntop_dup nm_inet4_ntop_dup
R nm_utils_inet6_ntop_dup nm_inet6_ntop_dup
R nm_utils_inet_ntop nm_inet_ntop
R nm_utils_inet_ntop_dup nm_inet_ntop_dup
R nm_utils_ip4_address_clear_host_address nm_ip4_addr_clear_host_address
R nm_utils_ip4_address_is_link_local nm_ip4_addr_is_link_local
R nm_utils_ip4_address_is_loopback nm_ip4_addr_is_loopback
R nm_utils_ip4_address_is_zeronet nm_ip4_addr_is_zeronet
R nm_utils_ip4_address_same_prefix nm_ip4_addr_same_prefix
R nm_utils_ip4_address_same_prefix_cmp nm_ip4_addr_same_prefix_cmp
R nm_utils_ip6_address_clear_host_address nm_ip6_addr_clear_host_address
R nm_utils_ip6_address_same_prefix nm_ip6_addr_same_prefix
R nm_utils_ip6_address_same_prefix_cmp nm_ip6_addr_same_prefix_cmp
R nm_utils_ip6_is_ula nm_ip6_addr_is_ula
R nm_utils_ip_address_same_prefix nm_ip_addr_same_prefix
R nm_utils_ip_address_same_prefix_cmp nm_ip_addr_same_prefix_cmp
R nm_utils_ip_is_site_local nm_ip_addr_is_site_local
R nm_utils_ipaddr_is_normalized nm_inet_is_normalized
R nm_utils_ipaddr_is_valid nm_inet_is_valid
R nm_utils_ipx_address_clear_host_address nm_ip_addr_clear_host_address
R nm_utils_parse_inaddr nm_inet_parse_str
R nm_utils_parse_inaddr_bin nm_inet_parse_bin
R nm_utils_parse_inaddr_bin_full nm_inet_parse_bin_full
R nm_utils_parse_inaddr_prefix nm_inet_parse_with_prefix_str
R nm_utils_parse_inaddr_prefix_bin nm_inet_parse_with_prefix_bin
R test_nm_utils_ip6_address_same_prefix test_nm_ip_addr_same_prefix
./contrib/scripts/nm-code-format.sh -F
n-dhcp4 only supports calling ACCEPT during the GRANTED state.
Not during a EXTENDED event. So usually, we would not want
to call accept in that case.
And we didn't. During EXTENDED event, we would usually skip ACD (because
it's either not enabled or we already passed ACD for the current address).
In that case, in _nm_dhcp_client_notify() we hit the line
if (client_event_type == NM_DHCP_CLIENT_EVENT_TYPE_BOUND && priv->l3cd_curr
&& nm_l3_config_data_get_num_addresses(priv->l3cd_curr, priv->config.addr_family) > 0)
priv->l3cfg_notify.wait_dhcp_commit = TRUE;
else
priv->l3cfg_notify.wait_dhcp_commit = FALSE;
and would not set `wait_dhpc_commit`. That means, we never called _dhcp_client_accept().
For nettools, that doesn't really matter because calling ACCEPT during EXTENDED
is invalid anyway. However, for dhclient that is fatal because we wouldn't reply the
D-Bus request from nm-dhcp-helper. The helper times out after 60 seconds and dhclient
would misbehave.
We need to fix that by also calling _dhcp_client_accept() in the case when we don't
need to wait (the EXTENDED case).
However, previously _dhcp_client_accept() was rather peculiar and didn't like to be
called in an unexpected state. Relax that. Now, when calling accept in an unexpected
state, just do nothing and signal success. That frees the caller from the complexity
to understand when they must/must not call accept.
https://bugzilla.redhat.com/show_bug.cgi?id=2109285
Fixes: 156d84217c ('dhcp/dhclient: implement accept/decline (ACD) for dhclient plugin')
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/1308
In the past, nmp_lookup_init_object() could both lookup all object for a
certain ifindex, and lookup all objects of a type. That fallback path
already leads to an assertion failure fora while now, so nobody should
be using this function to lookup all objects of a certain type (for
what, we have nmp_lookup_init_obj_type()).
Now, remove the fallback path, and rename the function to what it really
does.
