We have bridge min/max/default values in core-internal. Do the same
for bridge port ones.
We will soon use those values to enforce limits when assuming a
bridge port configuration.
In NetworkManager we have a default port path-cost equal to 100.
In the linux kernel the default port cost depends upon the interface
speed: 2 for 10Gb, 4 for 1Gb, 19 for 100Mb and 100 for 10Mb (or when the
interface speed is not available, like current virtio_net driver).
Allow NetworkManager to assume bridge port connections also when the
path-cost differs: this will allow us to assume bridge ports created
outside NetworkManager (e.g. in initrd) that will likely have a different
"cost" value.
Since we already cached the result of getpagesize() in a static variable (at
two places), move the code to nm-shared-utils, so it is reusable.
Also, use sysconf() instead of getpagesize(), like suggested by `man
getpagesize`.
Using strncpy() in the macro directly can result in a compiler warning.
We don't want to replace this with memcpy(), because strncpy() aborts
on the first NUL and fills the rest with NUL. Since nm_strndup_a() is a
replacement for g_strndup(), we want to do that here as well.
In file included from ../shared/nm-default.h:294,
from ../libnm-core/nm-utils.c:22:
../libnm-core/nm-utils.c: In function nm_sock_addr_endpoint_new:
../shared/nm-utils/nm-shared-utils.h:281:4: error: strncpy output truncated before terminating nul copying as many bytes from a string as its length [-Werror=stringop-truncation]
strncpy (_s, _str, _len); \
^~~~~~~~~~~~~~~~~~~~~~~~
../libnm-core/nm-utils.c:154:26: note: in expansion of macro nm_strndup_a
host = _parse_endpoint (nm_strndup_a (200, endpoint, l_endpoint - 1, &host_clone),
^~~~~~~~~~~~
../libnm-core/nm-utils.c:152:15: note: length computed here
l_endpoint = strlen (endpoint) + 1;
^~~~~~~~~~~~~~~~~
Now that the default for the internal client is "mac", we don't need
this snippet anymore. Drop it.
Don't renumber the source files but leave the gap at Source3. Everytime
we add config snippets the numbers need to be shuffled, so don't fill
the gap and maybe use it in the future.
https://bugzilla.redhat.com/show_bug.cgi?id=1661165
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
This is a flags type, not an enum.
This affects the generated glib-mkenums type, changing it from GEnum to
GFlags. That is possibly a change in behavior for language bindings, but
hopefully nobody is badly affected by this.
Fixes: e6f95b50c8
Secret-flags are flags, but most combinations don't actually make sense
and maybe should be rejected. Anyway, that is not done, and most places
just check that there are no unknown flags set.
Add _nm_setting_secret_flags_valid() to perform the check at one place
instead of having the implementation at various places.
- nm_setting_enumerate_values() cannot handle non-GObject based
properties as it cannot abstract them. That means, for gendata
based settings, we need already a special case, and future ways
of handling settings (WireGuard peers) also won't work without
special casing.
- nm_setting_enumerate_values() needs to fetch all properties, although
some properties will not be actually used. E.g. secret properties will
be ignored depending on the flag.
Or compare the read-side with read_one_setting_value(). The reader doesn't
care about the (unset) GObject property. It really just cares about the
GType of the proeprty. Still, nm_setting_enumerate_values() fetches all
(empty) properties.
Or consider, route_writer() as called by nm_setting_enumerate_values()
always needs to deep-clone the entire list of routes in the property
getter for NM_SETTING_IP_CONFIG_ROUTES, then unpack it. This is
unnecesary overhead. This is not yet fixed, but would now be easy to
improve.
- a for-each function like nm_setting_enumerate_values() does make code
harder to read, gives less possibility for optimization, and is in
general less flexible. Don't use it.
Drop another use of nm_setting_enumerate_values().
Using nm_setting_enumerate_values() to duplicate a setting already
didn't work for gendata based settings.
Also, nm_setting_enumerate_values() would iterate the properties
in a particular order. We don't need that, the default order
(asciibetical sorted) is fine.
The property infos are already sorted by name. As nm_setting_enumerate_values()
now uses that information, in most cases there is nothing to sort.
The only instance is NMSettingConnection, which has a different
sort-order. At least for some purposes, not all:
- nm_setting_enumerate_values(), obviously.
- nm_setting_enumerate_values() is called by keyfile writer. That
means, keyfile writer will persist properties in a sorted way.
Cache the property list with alternative sorting also in the
setting-meta data, instead of calculating it each time.
Beside caching the information, this has the additional benefit that
this kind of sorting is now directly available, without calling
nm_setting_enumerate_values(). Meaning, we can implement keyfile writer
without using nm_setting_enumerate_values().
We should no longer use nm_connection_for_each_setting_value() and
nm_setting_for_each_value(). It's fundamentally broken as it does
not work with properties that are not backed by a GObject property
and it cannot be fixed because it is public API.
Add an internal function _nm_connection_aggregate() to replace it.
Compare the implementation of the aggregation functionality inside
libnm with the previous two checks for secret-flags that it replaces:
- previous approach broke abstraction and require detailed knowledge of
secret flags. Meaning, they must special case NMSettingVpn and
GObject-property based secrets.
If we implement a new way for implementing secrets (like we will need
for WireGuard), then this the new way should only affect libnm-core,
not require changes elsewhere.
- it's very inefficient to itereate over all settings. It involves
cloning and sorting the list of settings, and retrieve and clone all
GObject properties. Only to look at secret properties alone.
_nm_connection_aggregate() is supposed to be more flexible then just
the two new aggregate types that perform a "find-any" search. The
@arg argument and boolean return value can suffice to implement
different aggregation types in the future.
Also fixes the check of NMAgentManager for secret flags for VPNs
(NM_CONNECTION_AGGREGATE_ANY_SYSTEM_SECRET_FLAGS). A secret for VPNs
is a property that either has a secret or a secret-flag. The previous
implementation would only look at present secrets and
check their flags. It wouldn't check secret-flags that are
NM_SETTING_SECRET_FLAG_NONE, but have no secret.
There are 3 kinds of secret flag implementations:
1) regular properties have a GObject property and a corresponding
"-flags" property.
2) NMSettingVpn handles this entirely differently
3) NMSettingWirelessSecurity's WEP keys, where the secret keys
share a flags property that does not follow the same naming
scheme as 1).
The getter and setter had a boolean "verifiy_secret", only to
handle 3). Drop that parameter. Don't let NMSettingWirelessSecurity
call the parent's implementation for WEP keys. Just let it handle
it directly.
- most of the time, the secret-name is short and fits in a
stack-allocated buffer.
Optimize for that by using nm_construct_name_a().
- use _nm_utils_ascii_str_to_int64() instead of strtoul().
tmp = strtoul ((const char *) val, NULL, 10);
if ((errno != 0) || (tmp > NM_SETTING_SECRET_FLAGS_ALL)) {
is not the right way to check for errors of strtoul().
- refactor the code to return-early on errors.
- since commit 9b96bfaa72 "setting-vpn: whatever is in vpn.secrets always
is a secrets", we accept secrets without secret-flags as valid too.
However, only do that, when we at least have a corresponding key in
priv->secrets hash. If the secret name is not used at all, it's
clearly not a secret.
- if the secret flags are not a valid number, pretend that the flags
are still set to "none" (zero). That is because we use the presence
of the "*-flags" data item as indication that this is in fact a
secret. The user cannot use data items with such a name for another
purpose, so on failure, we still claim that this is in fact a secret.
We have a concept of setting and property meta-data that extends plain
GObject properties. While most properties are indeed backed by an
implemented as a GObject property, some are not.
Reuse the object property meta-data instead of fetching the list of
properties. Note that there is not much change in behavior, because
at all places where this is done, properties which are not backed by a
GObject property are skipped for the moment.
If nothing else, we save needlessly cloning the property list.
Later possibly we may no longer want to do that and add virtual
functions that can handle all properties.
- previously, writer would use nm_keyfile_plugin_kf_set_integer() for
G_TYPE_UINT types.
That means, values larger than G_MAXINT would be stored as negative
values. On the other hand, the reader would always reject negative
values.
Fix that, by parsing the integer ourself.
Note that we still reject the old (negative) values and there is no
compatibility for accepting such values. They were not accepted by
reader in the past and so they are still rejected.
This affects for example ethernet.mtu setting (arguably, the MTU
is usually set to small values where the issue was not apparent).
This is also covered by a test.
- no longer use nm_keyfile_plugin_kf_set_integer().
nm_keyfile_plugin_kf_set_integer() calls g_key_file_get_integer(), which
uses g_key_file_parse_integer_as_value(). That one has the odd
behavior of accepting "<number><whitespace><bogus>" as valid. Note how that
differs from g_key_file_parse_value_as_double() which rejects trailing data.
Implement the parsing ourself. There are some changes here:
- g_key_file_parse_value_as_integer() uses strtol() with base 10.
We no longer require a certain the base, so '0x' hex values are allowed
now as well.
- bogus suffixes are now rejected but were accepted by g_key_file_parse_value_as_integer().
We however still accept leading and trailing whitespace, as before.
- use nm_g_object_set_property*(). g_object_set() asserts that the value
is in range. We cannot pass invalid values without checking that they
are valid.
- emit warnings when values cannot be parsed. Previously they would
have been silently ignored or fail an assertion during g_object_set().
- don't use "helpers" like nm_keyfile_plugin_kf_set_uint64(). These
merely call GKeyFile's setters (taking care of aliases). The setters
of GKeyFile don't do anything miraculously, they merely call
g_key_file_set_value() with the string that one would expect.
Convert the numbers/boolean ourselfs. For one, we don't require
a heap allocation to convert a number to string. Also, there is
no point in leaving this GKeyFile API, because even if GKeyFile
day would change, we still must continue to support the present
format, as that is what users have on disk. So, even if a new
way would be implemented by GKeyFile, the current way must forever
be accepted too. Hence, we don't need this abstraction.
We will need access to the serialization flags from within the synth_func().
That will be for WireGuard's peers. Peers are a list of complex, structured
elements, and some fields (the peer's preshared-key) are secret and
others are not. So when serializing the peers, we need to know whether
to include secrets or not.
Instead of letting _nm_setting_to_dbus() check the flags, pass them
down.
While at it, don't pass the property_name argument. Instead, pass the
entire meta-data information we have. Most synth functions don't care
about the property or the name either way. But we should not pre-filter
information that we have at hand. Just pass it to the synth function.
If the synth function would be public API, that would be a reason to be
careful about what we pass. But it isn't and it only has one caller.
So passing it along is fine. Also, do it now when adding the flags
argument, as we touch all synth implementations anyway.
We shall not shortcut the synth function. If the synth function is
unhappy about a missing NMConnection argument, then that needs to be
fixed.
So, revert 395c385b9 and fix the issue in nm_setting_wireless_get_security()
differently. I presume that is the only place that caused problems,
since the history of the patch does not clealy show what the problem
was.
This reverts commit 395c385b9b.
Add helper wrappers around nm_g_object_set_property() that take a
native value, construct a GValue of the according type, and call
nm_g_object_set_property().
Commonly, the prefix is a string constant. We don't need to call
g_str_has_prefix() for that, which first requires strlen() on
the prefix. All the information is readily available.
Add a macro for that.
