When subscribing a signal to a singleton, we should ensure that the
source object stays alive. Take a reference.
This is also right in this case, because NMDBusManager (and its dependencies)
should never use NMDhcpListener. So, there is a clear direction of who references
who.
when dhclient is used as the dhcp client in NetworkManager we expose on
D-Bus all the variables that are passed to our script file. In
particular, we use the variable names there as labels (stripping the
heading "new") taking whatever dhclient passes us.
There are few exception to this. Dhclient allows to redefine option
variable names and we use this functionality for a few dhcp options:
dhcp option code 121 --> "rfc3442_classless_static_routes"
dhcp option code 249 --> "ms_classless_static_routes"
dhcp option code 252 --> "wpad"
Note that for private dhcp options (224-254) default dhclient labels are
in the form "unknown_$OPTNUM".
When using the internal dhcp client we skip exporting on D-Bus many of
the dhcp options received from the dhcp server. We instead export almost
all of them when using the dhclient dhcp client, using the variable
names passed by dhclient itself.
Map more DHCP options to dhclient variable names in order to allow the
internal client to retrieve them easily, namely: the server identifier,
the broadcast address, the renewal time, the rebinding time and the timezone.
Note that not all the DHCP options can be exported at this time because
systemd-networkd code drops many it won't process, so we have no way to
retrieve them without changing core systemd-networkd code.
It was already exposed implicity as the expiration time: add also the
explicit option using same format of dhclient dhcp plugin.
In the meanwhile, drop the SD_DHCP_OPTION_CLIENT_IDENTIFIER as not used.
Use DEBUG logging level for the parsing result of lease file.
Moreover, use consistent labels for the dhcp options: same labels of
what is exposed on D-Bus.
Add the reason to error messages to make debugging easier.
Note that n_dhcp4_client_new() also returns positive internal error
values, so we can't use nm_utils_error_set_errno().
I think that artificially slowing down DHCP is not going to make users
happier, so let's decrease it to the minimum allowed value (1 ms).
Note that also dhclient and the internal client have it disabled. From
the dhclient.conf man page:
*initial-delay* parameter sets the maximum time client can wait after
start before commencing first transmission. According to RFC2131
Section 4.4.1, client should wait a random time between startup and
the actual first trans‐ mission. Previous versions of ISC DHCP client
used to wait random time up to 5 seconds, but that was unwanted due
to impact on startup time. As such, new versions have the default
initial delay set to 0. To restore old behavior, please set
initial-delay to 5.
This is inspired by the existing systemd integration, with a few differences:
* This parses the WPAD option, which systemd requested, but did not use.
* We hook into the DAD handling, only making use of the configured address
once DAD has completed successfully, and declining the lease if it fails.
There are still many areas of possible improvement. In particular, we need
to ensure the parsing of all options are compliant, as n-dhcp4 treats all
options as opaque, unlike sd-dhcp4. We probably also need to look at how
to handle failures and retries (in particular if we decline a lease).
We need to query the current MTU at client startu, as well as the hardware
broadcast address. Both these are provided by the kernel over netlink, so
it should simply be a matter of hooking that up with NM's netlink layer.
Contribution under LGPL2.0+, in addition to stated licenses.
The DHCP client is not meant to use the assigned address before DAD
has completed successfully, if enabled. And if DAD fails, the server
should be notified with a DECLINE, in order to potentially blacklist
the address.
Currently, none of the clients support this, but add the required
callbacks, and allow clients to opt in if they want.
Soon a new DHCP backend will be added that will take code from the
internal one. Change its license to LGPL so that the whole new backend
code can also be LGPL, which is the preferred license for new
NetworkManager code.
Acked-by: Dan Williams <dcbw@redhat.com>
Acked-by: Dan Winship <danw@redhat.com>
Acked-by: Lubomir Rintel <lkundrak@v3.sk>
Acked-by: Thomas Haller <thaller@redhat.com>
We no longer add these. If you use Emacs, configure it yourself.
Also, due to our "smart-tab" usage the editor anyway does a subpar
job handling our tabs. However, on the upside every user can choose
whatever tab-width he/she prefers. If "smart-tabs" are used properly
(like we do), every tab-width will work.
No manual changes, just ran commands:
F=($(git grep -l -e '-\*-'))
sed '1 { /\/\* *-\*- *[mM]ode.*\*\/$/d }' -i "${F[@]}"
sed '1,4 { /^\(#\|--\|dnl\) *-\*- [mM]ode/d }' -i "${F[@]}"
Check remaining lines with:
git grep -e '-\*-'
The ultimate purpose of this is to cleanup our files and eventually use
SPDX license identifiers. For that, first get rid of the boilerplate lines.
Maybe DHCP plugins should be configurable per address family and be
re-loadable via SIGHUP. But that just adds complexity.
Nowadays we always have the "internal" DHCP plugin, which is known to
support both IPv4 and IPv6. One day, we should get rid of all plugins
and only use one implementation (that works well). The "internal" plugin
is supposed to be(come) that.
That also means, that we are not going to add more (external) DHCP
plugins and we are not going to invest work in the existing plugins
(except the "internal" plugin).
Some DHCP plugins are known to not support IPv6. If the user selects
"dhcpcd" we should just fallback to the "internal" plugin. What's the
point of letting the activation fail? Probably users shouldn't use
"dhcpcd" plugin anyway, but that's a different story. Doing such fallback
could be a problem with forward compatibility if we ever would add IPv6
support to "dhcpcd". But we won't.
Also, we are going to add "n-dhcp4" as replacement for the systemd based
code. For a time, there will be an experimental plugin "nettools" that
eventually will become the new "internal" plugin. Until that happens,
we want for IPv6 automatically fallback to systemd based "internal"
plugin. This patch will make that simple.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/merge_requests/173
The pid-file is private to NetworkManager. It should reside in NetworkManager's
run directory instead of "/var/run".
I don't think that changing this location can break existing uses. Why
would somebody outside of NetworkManager care about this file?
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/merge_requests/157
"RUNDIR" is set to "$runstatedir/NetworkManager". That is not correct,
we must use "$runstatedir".
I don't understand how this could have ever worked. Commit e2ecf5b808
('dhcp: dhcpcd uses a fixed path for PID files') seems to address this issue,
but already then "RUNDIR" was set to "$(localstatedir)/run/NetworkManager".
The library is called "libnm_core". So the dependency should be called
"libnm_core_dep", like in all other cases.
(cherry picked from commit c27ad37c27)
From the files under "shared/nm-utils" we build an internal library
that provides glib-based helper utilities.
Move the files of that basic library to a new subdirectory
"shared/nm-glib-aux" and rename the helper library "libnm-core-base.la"
to "libnm-glib-aux.la".
Reasons:
- the name "utils" is overused in our code-base. Everything's an
"utils". Give this thing a more distinct name.
- there were additional files under "shared/nm-utils", which are not
part of this internal library "libnm-utils-base.la". All the files
that are part of this library should be together in the same
directory, but files that are not, should not be there.
- the new name should better convey what this library is and what is isn't:
it's a set of utilities and helper functions that extend glib with
funcitonality that we commonly need.
There are still some files left under "shared/nm-utils". They have less
a unifying propose to be in their own directory, so I leave them there
for now. But at least they are separate from "shared/nm-glib-aux",
which has a very clear purpose.
(cherry picked from commit 80db06f768)
"shared/nm-utils" contains general purpose utility functions that only
depend on glib (and extend glib with some helper functions).
We will also add code that does not use glib, hence it would be good
if the part of "shared/nm-utils" that does not depend on glib, could be
used by these future projects.
Also, we use the term "utils" everywhere. While that covers the purpose
and content well, having everything called "nm-something-utils" is not
great. Instead, call this "nm-std-aux", inspired by "c-util/c-stdaux".
(cherry picked from commit b434b9ec07)
- use nm_utils_strsplit_set_full() instead of g_strsplit_set() to avoid allocating
a full strv array.
- refactor the code to return early and use cleanup attribute for freeing
memory.
- return TRUE/FALSE from process_dhclient_rfc3442_route(). It's simpler to
understand than returning the moved pointer and a success output variable.
The defaults for test timeouts in meson is 30 seconds. That is not long
enough when running
$ NMTST_USE_VALGRIND=1 ninja -C build test
Note that meson supports --timeout-multiplier, and automatically
increases the timeout when running under valgrind. However, meson
does not understand that we are running tests under valgrind via
NMTST_USE_VALGRIND=1 environment variable.
Timeouts are really not expected to be reached and are a mean of last
resort. Hence, increasing the timeout to a large value is likely to
have no effect or to fix test failures where the timeout was too rigid.
It's unlikely that the test indeed hangs and the increase of timeout
causes a unnecessary increase of waittime before aborting.
- regarding the DHCP options, we should not suppress them. If the lease
contains such bogus(?) addresses, we still want to expose them on
D-Bus without modification.
- regrading using the DNS server, ignore localhost addresses like done for
systemd-networkd ([1], [2]).
Until recently, the DHCP library would internally suppress such
addresses ([3]). That is no longer the case, and we should handle
them specially.
[1] https://github.com/systemd/systemd/issues/4524
[2] d9ec2e632d
[3] 334d5682ae
Imported from systemd:
The Router DHCP option may contain a list of one or more
routers ([1]). Extend the API of sd_dhcp_lease to return a
list instead of only the first.
Note that networkd still only uses the first router (if present).
Aside from extending the internal API of the DHCP client, there
is almost no change in behavior. The only visible difference in
behavior is that the "ROUTER" variable in the lease file is now a
list of addresses.
Note how RFC 2132 does not define certain IP addresses as invalid for the
router option. Still, previously sd_dhcp_lease_get_router() would never
return a "0.0.0.0" address. In fact, the previous API could not
differenciate whether no router option was present, whether it
was invalid, or whether its first router was "0.0.0.0". No longer let
the DHCP client library impose additional restrictions that are not
part of RFC. Instead, the caller should handle this. The patch does
that, and networkd only consideres the first router entry if it is not
"0.0.0.0".
[1] https://tools.ietf.org/html/rfc2132#section-3.5
This also required adjusting "src/dhcp/nm-dhcp-systemd.c" due to the
changed internal API.
f8862395e8
... and lease_to_ip6_config().
- Handle reasons that render the lease invalid first, before logging
anything. This way, upon invalid lease we don't have partially logged
about the lease.
- prefer logging one line for options that contain multiple values, for
example for search domains.
- reorder statements to consistently log first before calling add_option().
- prefer
g_string_append (nm_gstring_add_space_delimiter (str), ...
over
g_string_append_printf (str, "%s%s", str->len ? " " : "", ...
- use @addr_str buffer directly, instead of assigning to another
temporary variable.
For static functions inside a module, the compiler determines on its own
whether to inline the function.
Also, "inline" was used at some places that don't immediatly look like
candidates for inlining. It was most likely a copy&paste error.
The "ipv4.dhcp-client-id" is configurable per-profile and the default
value can be overwritten via connection defaults in NetworkManager.conf.
For "dhclient" DHCP plugin, the ultimate default for "ipv4.dhcp-client-id"
is determined by dhclient itself, or possibly by user configuration from
"/etc/dhcp".
For the "internal" DHCP plugin, the default must be decided by
NetworkManager. Also, the default here is important, as we preferably
won't change it anymore. That is because a changing the client-id
will result in different IP addresses after upgrade of NetworkManager
version. That should be avoided.
Still, change it now. If a downstream does not want this, it either needs
to patch the sources or add a configuration snippet like:
[connection-internal-dhcp-client-id-duid]
match-device=dhcp-plugin:internal
ipv4.dhcp-client-id=duid
The reason to change from the previous default "duid" to "mac" are the
following:
- "duid" is an RFC 4361 compatible client-id ([1]) and "mac" is defined
in RFC 2132.
- "duid" cannot (easily) be predicated a-priori as it is a hash of the
interface-name and "/etc/machine-id". In particular in cloud and server
environments, admins often prefer "mac" as they do know the MAC address
and pre-configure the DHCP server accordingly.
- with "dhclient" plugin, the default is decided by dhclient package or
user configuration in "/etc/dhcp". However, in fact the default is often
"client-identifier hardware" (for example on RHEL/CentOS).
- for RHEL/CentOS we require a way to select "mac" as default. That was
done by installing a configuration snippet via the NetworkManager-config-server
package. It's confusing to have the default depend on a package. Avoid
that. Also, users required "mac" in certain scenarios, but no users
explicitly asked for "duid" as default.
- our "duid" implementation generates a 32 bit IAID based on a hash of the
interface-name, and only 8 bytes entropy that contains a hash
of "/etc/machine-id". The point is, that is not a lot of entropy to
avoid conflicting client-ids. Another point is, that the choosen algorithm
for "duid" is suitable for RFC 4361, but it's only one of many possibly
implementations. Granted, each possibility has up and downsides but selecting
one of them as default seems wrong (given that it has obvious downsides
already). For "mac" there is only one straight-forward way to implement
it.
- RFC 7844 (Anonymity Profiles for DHCP Clients) is not yet supported by
NetworkManager. But we should not select a default client-id which
counteracts anonymit. Choosing "mac" does not reveal information which
is not already exposed.
[1] https://tools.ietf.org/html/rfc4361#section-4https://bugzilla.redhat.com/show_bug.cgi?id=1661165
