There is really no problem here, allow it.
Previously we would assert against a non-zero prefix length.
But I am not sure that all callers really ensured that this
couldn't happen. Anyway, there is no problem we such addresses,
really.
Only we need to make sure that nm_ip4_config_add_dependent_routes()
and nm_ip6_config_add_dependent_routes() don't add prefix routes for
such addresses (which is the case now).
Preface: RFC 3442 (The Classless Static Route Option for Dynamic Host
Configuration Protocol (DHCP) version 4) states:
If the DHCP server returns both a Classless Static Routes option and
a Router option, the DHCP client MUST ignore the Router option.
Similarly, if the DHCP server returns both a Classless Static Routes
option and a Static Routes option, the DHCP client MUST ignore the
Static Routes option.
Changes:
- sd_dhcp_lease_get_routes() returns the combination of both option 33
(static routes) and 121 (classless static routes). If classless static
routes are provided, the state routes must be ignored.
- we collect the options hash that we expose on D-Bus. For that purpose,
we must not merge both option types as classless static routes. Instead,
we want to expose the values like we received them originally: as two
different options.
- we continue our deviation from RFC 3442, when receiving classless static
routes with option 3 (Router), we only ignore the router if we didn't
already receive a default route via classless static routes.
- in the past, NetworkManager treated the default route specially, and
one device could only have one default route. That limitation was
already (partly) lifted by commit 5c299454b4
(core: rework tracking of gateway/default-route in ip-config). However,
from DHCP we still would only accept one default route. Fix that for
internal client. Installing multiple default routes might make sense, as
kernel apparently can skip unreachable routers (as it notes via ICMP
messages) (rh#1634657).
https://bugzilla.redhat.com/show_bug.cgi?id=1634657
In ip4_start(), we iterate over @dhcp4_requests array and add the
options that are to be included. We do so, by calling
sd_dhcp_client_set_request_option().
Note that sd_dhcp_client_set_request_option() only appends the options
to a list, not taking special care about the order in which options are
added.
RFC 3442 (The Classless Static Route Option for Dynamic Host Configuration
Protocol (DHCP) version 4) says:
DHCP clients that support this option and send a parameter request
list MAY also request the Static Routes option, for compatibility
with older servers that don't support Classless Static Routes. The
Classless Static Routes option code MUST appear in the parameter
request list prior to both the Router option code and the Static
Routes option code, if present.
Compare to RFC 2132 (DHCP Options and BOOTP Vendor Extensions) which says
about the parameter request list:
The client MAY list the options in order of preference.
Note, with RFC 7844 (Anonymity Profiles for DHCP Clients), the order
should be randomized. But since we don't follow RFC 7844 (yet), let's follow
at least RFC 3442.
- check errors when accessing the lease. Some errors, like a failure
from sd_dhcp_lease_get_address() are fatal.
- while parsing the individual options, don't incrementally build the
NMPlatformIP4Address instance (and similar). Instead, parse the
options to individual variales, and only package them as platform
structure at the point where they are needed. It makes the code simpler,
because all individual bits (like "r_plen" variable) are only
initialized and used at one place. That is clearer than incrementally
building a platform structure, where you have to follow the code to
see how the structure mutates.
- drop redundant comments that only serve as a visual separator
for structuring the code. Instead, structure the code.
lease_to_ip4_config() can fail, if the lease is broken. As such, a function
that fails should not modifiy an in/out parameter. Avoid that, by not
having the caller pre-allocate the options hash, but instead allocate it
by the lease_to_ip*_config() functions, and return it only on success.
- use proper data types "guint16" and "bool" in static
option list. It saves a few bytes, but also it's the appropriate
type. Well, at least, it's the appropriate type for DHCPv6,
not for DHCPv4 (which is guint8).
- assert against failure of sd_dhcp_client_set_request_option() and
sd_dhcp6_client_set_request_option().
sd_dhcp6_client_set_request_option() only accepts a white-listed
set of options. Unexpected options are rejected with -EINVAL.
Currently supported are only:
- SD_DHCP6_OPTION_DNS_SERVERS
- SD_DHCP6_OPTION_DOMAIN_LIST
- SD_DHCP6_OPTION_SNTP_SERVERS
- SD_DHCP6_OPTION_NTP_SERVER
- SD_DHCP6_OPTION_RAPID_COMMIT
As such, SD_DHCP6_OPTION_CLIENTID is not accepted and requesting it
was silently ignored.
Fixes: d2dd3b2c90
While nm_utils_inet*_ntop() accepts a %NULL buffer to fallback
to a static buffer, don't do that.
I find the possibility of using a static buffer here error prone
and something that should be avoided. There is of course the downside,
that in some cases it requires an additional line of code to allocate
the buffer on the stack as auto-variable.
During start, sd_dhcp6_client would call client_ensure_iaid(),
to initialize the IAID.
First of all, the IAID is important to us, we should not leave the
used IAID up to an implementation detail. In the future, we possibly
want to make this configurable.
The problem is that client_ensure_iaid() calls dhcp_identifier_set_iaid()
which looks up the interface name via if_indextoname() (in our fork).
The problem is that dhcp_identifier_set_iaid() looks up information by
quering the system, which makes it hard for testing and also makes it
unpredictable. It's better to use our implementation nm_utils_create_dhcp_iaid(),
which is solely based on the interface-name, which is given as a well
defined parameter to the DHCP client instance.
- use nm_auto to return early when something goes wrong
- don't modify NMDhcpClient's state until the end, when it looks
like we are (almost) started successfully.
- for IPv4, only attempt to load the lease if we actually are
interested in the address. Also, reduce the scope of the lease
variable, to the one place where we need it.
The client-id is something that we want to determine top-down.
Meaning, if the user specifies it via ipv4.dhcp-client-id, then it
should be used. If the user leaves it unspecified, we choose a
default stable client-id. For the internal DHCP plugin, this is
a node specific client-id based on
- the predictable interface name
- and /etc/machine-id
It's not clear, why we should allow specifying the client-id in
the lease file as a third source of configuration. It really pushes
the configuration first down (when we do DHCP without lease file),
to store an additional bit of configuration for future DHCP attempts.
If the machine-id or the interface-name changes, then so does the
default client-id. In this case, also "ipv4.dhcp-client-id=stable"
changes. It's fair to require that the user keeps the machine-id
stable, if the machine identity doesn't change.
Also, the lease files are stored in /var/lib/NetworkManager, which
is more volatile than /etc/machine-id. So, if we think that machine-id
and interface-name is not stable, why would we assume that we have
a suitable lease file?
Also, if you do:
nmcli connection add con-name "$PROFILE" ... ipv4.dhcp-client-id ''
nmcli connection up $PROFILE
nmcli connection modify "$PROFILE" ipv4.dhcp-client-id mac
nmcli connection up $PROFILE
nmcli connection modify "$PROFILE" ipv4.dhcp-client-id ''
nmcli connection up $PROFILE
wouldn't you expect that the original (default) client-id is used again?
Also, this works badly with global connection defaults in
NetworkManager.conf. If you configure a connection default, previously
already this would always force the client-id and overrule the lease.
That is reasonable, but in which case would you ever want to use
the client-id from the lease?
- if we leave the client-id of sd_dhcp_client unset, it will
anyway generate a node-specific client-id (and may fail if
"/etc/machine-id" is invalid).
Anticipate that, and don't let the client-id unset. In case
we have no client-id from configuration or lease, just generate
the id ourself (using the same algorithm). The advantage is,
that we know it upfront and can store the client-id in the
NMDhcpClient instance. We no longer need to peel it out from
the lease later.
- to generate the IPv4 client-id, we need a valid MAC address. Also,
sd_dhcp_client needs a MAC address for dhcp_network_bind_raw_socket()
as well. Just require that a MAC address is always needed. Likewise,
we need a valid ifindex and ifname set.
- likewise for IPv6 and IPv4, cleanup detecting the arptype and
checking MAC address length. sd_dhcp_client_set_mac() is overly
strict at asserting input arguments, so we must validate them anyway.
- also, now that we always initialize the client-id when starting
the DHCP client, there is no need to retroactively extract it
again when we receive the first lease.
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.
We need a mode that:
* doesn't leave processes behind
* doesn't force an internal dhclient
* doesn't auto-generate default connections
* doesn't write out files into libdir, only /run
The original configure-and-quit mode doesn't really fit the initrd use. But
it's proobably not a good idea to just change its behavior.
On networked boot we need to somehow communicate this to the early boot
machinery. Sadly, no DBus there and we're running in configure-and-quit
mode.
Abusing the state file for this sounds almost reasonable and is
reasonably straightforward thing to do.
Requesting broadcast replies from the DHCP server can be problematic in
filtered environments like some wireless networks. Don't override the
default of using unicast. This matches the behaviour of the external DHCP
clients.
https://github.com/NetworkManager/NetworkManager/pull/93
It probably was no problem in practice, because very likely the
chunk of memory was aligned already.
Also, drop non helpful comment and fix whitespace.
GByteArray is a mutable array of bytes. For every practical purpose, the hwaddr
property of NMDhcpClient is an immutable sequence of bytes. Thus, make it a
GBytes.
Optimally, NMDhcpClient would be stateless and all paramters would
be passed on as argument. Clearly that is not feasable, because there
are so many paramters, and in many cases they need to be cached for the
lifetime of the client instance.
Instead of passing info_only paramter to ip6_start() and cache it
both in NMDhcpClient and NMDhcpSystemd, keep it in NMDhcpClient at
one place.
In the next commit, we will initialize info-only only once during the
constructor, so it is immutable and somewhat stateless.
The parent's stop() implementation does nothing interesting
for NMDhcpSystem. Still, call it, it's just unexpected to
not chain up the parent implementation, if all other subclasses
do it.
In general, if the parent's implementation is not suitable to be called
by the derived class, that should be handled differently then just not
chaining up. Otherwise it's inconsistent and confusing.
The internal client asserts that the length of the client ID is not more
than MAX_CLIENT_ID_LEN. Avoid that assert by truncating the string.
Also add new nm_dhcp_client_set_client_id_*() setters, that either
set the ID based on a string (in our common dhclient specific
format), or based on the binary data (as obtained from systemd client).
Also, add checks and assertions that the client ID which is
set via nm_dhcp_client_set_client_id() is always of length
of at least 2 (as required by rfc2132, section-9.14).
Replace the usage of g_str_hash() with our own nm_str_hash().
GLib's g_str_hash() uses djb2 hashing function, just like we
do at the moment. The only difference is, that we use a diffrent
seed value.
Note, that we initialize the hash seed with random data (by calling
getrandom() or reading /dev/urandom). That is a change compared to
before.
This change of the hashing function and accessing the random pool
might be undesired for libnm/libnm-core. Hence, the change is not
done there as it possibly changes behavior for public API. Maybe
we should do that later though.
At this point, there isn't much of a change. This patch becomes
interesting, if we decide to use a different hashing algorithm.
Instead of having 3 properties @gateway, @never_default and @has_gateway
on NMIP4Config/NMIP6Config that determine the default-route, track the
default-route as a regular route.
The gateway setting is the configuration knob for the default-route.
Since an NMIP4Config/NMIP6Config instance only has one gateway property,
it cannot track more then one default-routes (see related bug rh#1445417).
Especially with policy routing, it might be interesting to configure a
default-route in multiple tables.
Also, later it might be interesting to allow adding default-routes as
regular static routes in a connection, so that the user can configure additional
route parameters for the default-route or add default-routes in multiple tables.
With this patch, default-routes now have a rt_source property according to their
origin.
Also, the previous commits of this branch broke handling of the
default-route :) . That should be working now again.
The name "priority" is well established for routes (e.g. kernel's
RTA_PRIORITY netlink attribute).
However, we call it at most places "metric" or "route_metric".
Rename it, not to use two different names for the same thing.
In many cases we want to treat IPv4 and IPv6 generically. That looks nicer
if we distingish by an @addr_family integer, instead of a boolean.
Replace the @is_ipv6 boolean with an @addr_family paramter. The @is_ipv6
boolean is inconsistent with other places where we use @is_ipv4 to
indicate the opposite. Eventually, we should use @addr_family
everywhere.
Also, at the call site it's not immediately clear what TRUE/FALSE means,
here AF_INET/AF_INET6 is better.
Kernel requires that the host part of a route (based on network/plen)
is zero. Routes with non-zero host part don't really exist.
In settings (NMIPRoute), we don't enforce that. Hence we must ensure
that we don't let such invalid routes into NMIP4Config/NMIP6Config.
Also at other places where we obtain routes from untrusted sources,
we must sanitize them first.
Also add an assertion to catch such bugs.
NMIP4Config, NMIP6Config, and NMPlatform shall share one
NMDedupMultiIndex instance.
For that, pass an NMDedupMultiIndex instance to NMPlatform and NMNetns.
NMNetns than passes it on to NMDevice, NMDhcpClient, NMIP4Config and NMIP6Config.
So currently NMNetns is the access point to the shared NMDedupMultiIndex
instance, and it gets it from it's NMPlatform instance.
The NMDedupMultiIndex instance is really a singleton, we don't want
multiple instances of it. However, for testing, instead of adding a
singleton instance, pass the instance explicitly around.
lease_to_ip6_config() calls the GString temporary buffer "str".
That makes sense, use the same name in lease_to_ip4_config().
For that, we have to rename other local variables too.
If users wrote a FQDN in ipv4.dhcp-hostname presumably it's because
they really want to send the full value, not only the host part, so
let's send it as-is.
This obviously is a change in behavior, but only for users that have a
FQDN in ipv4.dhcp-hostname, where it's not clear if they really want the
domain to be stripped.
When the property is unset, we keep sending only the host part of the
system hostname to maintain backwards compatibility.
This commit aligns NM behavior to initscripts.
(cherry picked from commit cf5fab8f55)
