Add a new key management option to support WPA3 Enteprise wifi
connection.
Only supported with wpa_supplicant for the time being.
Signed-off-by: Antonio Cardace <acardace@redhat.com>
In public libnm headers we include some libc/linux headers, although
libnm doesn't strictly need them.
The <linux/*.h> headers conflict with some network headers provided by
libc and they need to be included in the right order. As
<NetworkManager.h> drags in some linux headers, this makes it
unnecessarily complicated.
It also feels ugly to include headers we don't need, only for the
sake of convenience. Allow to opt out.
Also, for internal build, don't do this. When building NetworkManager
we need control about the headers and their order of inclusion.
"in_addr_t" and "struct in6_addr" require headers from libc (or linux).
In particular, some libc headers conflict with the linux headers
(or they have to be included in a specific order). To avoid that
we want that our libnm headers include a minimum of other headers
(and only drag in glib headers, which we anyway need).
- instead of "in_addr_t", use guint32. For all practical purposes,
"in_addr_t" is a plain 32 bit integers and we can do this replacement
in our public headers.
- forward declare "struct in6_addr".
Currently libnm headers include <linux/if_{ether,infiniband,vlan}.h>.
These are public headers, that means we drag in the linux header to all
users of <NetworkManager.h>.
Often the linux headers work badly together with certain headers from libc.
Depending on the libc version, you have to order linux headers in the right
order with respect to libc headers.
We should do better about libnm headers. As a first step, assume that
the linux headers don't get included by libnm, and explicitly include
them where they are needed.
Including <netinet/ether.h> with musl leads to a conflict with <linux/if_ether.h>,
due to redefining ethhdr struct. As we include <linux/if_ether.h> in "nm-utils.h",
that is a problem.
Avoid that, by including other headers.
gcc-11.0.0-0.7.fc34 warns here:
CC libnm-core/libnm_core_la-nm-setting-team.lo
libnm-core/nm-setting-team.c: In function ‘nm_team_link_watcher_new_ethtool’:
libnm-core/nm-setting-team.c:127:33: error: array subscript ‘NMTeamLinkWatcher[0]’ is partly outside array bounds of ‘unsigned char[16]’ [-Werror=array-bounds]
127 | watcher->ref_count = 1;
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~
libnm-core/nm-setting-team.c:125:15: note: referencing an object of size 16 allocated by ‘g_malloc’
125 | watcher = g_malloc(nm_offsetofend(NMTeamLinkWatcher, ethtool));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
libnm-core/nm-setting-team.c:128:33: error: array subscript ‘NMTeamLinkWatcher[0]’ is partly outside array bounds of ‘unsigned char[16]’ [-Werror=array-bounds]
128 | watcher->type = LINK_WATCHER_ETHTOOL;
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~
libnm-core/nm-setting-team.c:125:15: note: referencing an object of size 16 allocated by ‘g_malloc’
125 | watcher = g_malloc(nm_offsetofend(NMTeamLinkWatcher, ethtool));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
libnm-core/nm-setting-team.c:129:33: error: array subscript ‘NMTeamLinkWatcher[0]’ is partly outside array bounds of ‘unsigned char[16]’ [-Werror=array-bounds]
129 | watcher->ethtool.delay_up = delay_up;
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~
libnm-core/nm-setting-team.c:125:15: note: referencing an object of size 16 allocated by ‘g_malloc’
125 | watcher = g_malloc(nm_offsetofend(NMTeamLinkWatcher, ethtool));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
libnm-core/nm-setting-team.c:130:33: error: array subscript ‘NMTeamLinkWatcher[0]’ is partly outside array bounds of ‘unsigned char[16]’ [-Werror=array-bounds]
130 | watcher->ethtool.delay_down = delay_down;
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
libnm-core/nm-setting-team.c:125:15: note: referencing an object of size 16 allocated by ‘g_malloc’
125 | watcher = g_malloc(nm_offsetofend(NMTeamLinkWatcher, ethtool));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Maybe we should not use this trick and just malloc() a struct of the
intended size, however:
- the code below does a similar thing, doing it differently for ethtool
watcher is confusing.
- the NMTeamLinkWatcher is a union which cannot alter its type. In no
case is it correct to access the fields of the wrong union type. By
allocating a smaller chunk, valgrind might catch such bugs.
Also, NMTeamLinkWatcher's definition is private to the C source file,
in no case must anybody assume that the rest of the buffer actually
exists.
Hence, workaround the warning by suppressing it.
We allow normalizing the slave-type, but sometimes, we may want to
validate a profile according to the set slave-type.
For example, a "ovs-external-ids" setting should only be allowed for
"connection.slave-type=ovs-interface". But during verify, the slave-type
may be missing and not yet normalized. We need to be able to obtain
the actually used slave-type.
NetworkManager is now able to configure veth interfaces throught the
NMSettingVeth. Veth interfaces only have "peer" property.
In order to support Veth interfaces in NetworkManager the design need
to pass the following requirements:
* Veth setting only has "peer" attribute.
* Ethernet profiles must be applicable to Veth interfaces.
* When creating a veth interface, the peer will be managed by
NetworkManager but will not have a profile.
* Veth connection can reapply only if the peer has not been modified.
* In order to modify the veth peer, NetworkManager must deactivate the
connection and create a new one with peer modified.
In general, it should support the basis of veth interfaces but without
breaking any existing feature or use case. The users that are using veth
interfaces as ethernet should not notice anything changed unless they
specified the veth peer setting.
Creating a Veth interface in NetworkManager is useful even without the
support for namespaces for some use cases, e.g "connecting one side of
the veth to an OVS bridge and the other side to a Linux bridge" this is
done when using OVN kubernetes [1][2]. In addition, it would provide
persistent configuration and rollback support for Veth interfaces.
[1] https://bugzilla.redhat.com/show_bug.cgi?id=1885605
[2] https://bugzilla.redhat.com/show_bug.cgi?id=1894139
Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
Revert this change. One problem is that none of the current GUIs
(nm-connection-editor, gnome-control-center, plasma-nm) expose the
dns-priority option. So, users tend to have their profile value set to
0. Changing the default means for them not only a change in behavior,
but its hard to fix via the GUI.
Also, what other call DNS leaks, is Split DNS to some. Both uses make
sense, but have conflicting goals. The default cannot accommodate both
at the same time.
Also, with split DNS enabled (dnsmasq, systemd-resolved), the concern
for DNS leaks is smaller. Imagine:
Wi-Fi profile with ipv4.dns-priority (effectively) 100, domain "example.com".
VPN profile with ipv4.dns-priority (effectively) 50 and a default route.
That is a common setup that one gets by default (and what probably many
users have today). In such a case with split DNS enabled, the Wi-Fi's DNS
server only sees requests for "*.example.com". So, it does not leak
everything.
Hence, revert this change before 1.28.0 release to the earlier behavior.
This reverts commit af13081bec.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/688
C casts unconditionally force the type, and as such they don't
necessarily improve type safety, but rather overcome restrictions
from the compiler when necessary.
Casting a void pointer is unnecessary (in C), it does not make the
code more readable nor more safe. In particular for g_object_new(),
which is known to return a void pointer of the right type.
Drop such casts.
sed 's/([A-Za-z_0-9]\+ *\* *) *g_object_new/g_object_new/g' $(git grep -l g_object_new) -i
./contrib/scripts/nm-code-format-container.sh
We have:
- nm_utils_hashtable_cmp(): this does a full cmp of two hash
tables, with the intent to provide a stable sort order.
It thus takes a GCompareDataFunc() argument.
- nm_utils_hashtable_cmp_equal(): this is like nm_utils_hashtable_cmp(),
except that the caller won't get a compare value, only a boolean
value that indicates equality.
This was previously called nm_utils_hashtable_equal().
- nm_utils_hashtable_equal(): this takes a GEqualFunc function
for comparing the values for equality. It takes thus
a different kind of predicate, but otherwise is similar to
nm_utils_hashtable_cmp_equal().
This was previously called nm_utils_hash_table_equal().
Unify the naming of these functions.
Historically, keyfile read/write code was part of core, and thus
GPL-2.0+ licensed. Keyfile is the native file format for NetworkManager
connection profiles, and code to handle that should be part of libnm.
This would unlock many interesting features, like tools being able
to import/export connection profiles in the native file format.
However, libnm is LGPL-2.1+ licensed, so this is a problem.
The alternative would be to add a separate, GPL licensed library
(libnm-keyfile.so or libnm-gpl.so). However that also requires a larger
rework, because the current keyfile implementation uses internal API
from libnm-core and it would need to be reworked to only use public
API of libnm.
Relicense the code instead. According to research and "keyfile-history.sh"
script, the following individuals and companies possibly hold copyright
on the code:
<bgalvani(at)redhat.com>
<blueowl(at)centrum.cz>
<daniel(at)gnoutcheff.name>
<danw(at)redhat.com>
<dcantrell(at)redhat.com>
<dcbw(at)redhat.com>
<evan(at)ebroder.net>
<fgiudici(at)redhat.com>
<floe(at)butterbrot.org>
<j(at)bootlab.org>
<kmaraas(at)gnome.org>
<lkundrak(at)v3.sk>
<luzpaz(at)users.noreply.github.com>
<martinpitt(at)gnome.org>
<michael.i.doherty(at)intel.com>
<pavlix(at)pavlix.net>
<pmarti(at)warp.es>
<rafaelff(at)gnome.org>
<rstrode(at)redhat.com>
<tambet(at)gmail.com>
<tgraf(at)redhat.com>
<thaller(at)redhat.com>
<walters(at)verbum.org>
<yurchor(at)ukr.net>
Intel Corporation
Novell, Inc.
Red Hat, Inc.
Ximian, Inc.
Most contributors on this list agreed to relicensing according to RELICENSE.md.
The following copyright holders did not answer the request for agreeing
to relicensing:
- <j(at)bootlab.org>: no contributions were made that are related to
keyfile implementation. The script just gives a false positive.
- <pmarti(at)warp.es>: the contribution is a fix of a spelling error
(commit 6029288ffb).
- <tgraf(at)redhat.com>: the contribution to keyfile code are small
(I only identified commit 5b7503e95e).
Also, Thomas worked for Red Hat at the time.
After research, I think it's fair to conclude that everybody who holds
non-trivial copyright on the keyfile code agreed to the relicensing.
>>>
H0=a3e75f329446a93a61ca4c458a7657bd919f4fe6
commit_has_file() {
git ls-tree -r "$1" | grep -q "\\s$2"\$
}
print_commit_authors() {
git --no-pager log --full-history --follow --no-merges --pretty='format:<%ae>' $H0 -- "$1" | sort | uniq
}
print_blame_authors() {
local LAST_H
if commit_has_file $H0 "$1"; then
LAST_H=$H0
else
LAST_H="$(git log --full-history --no-merges -n1 --pretty='format:%H' $H0 -- "$1")"^1
fi
git blame --no-progress -C -C -C20 -M -M10 -e "$LAST_H" -- "$1" | sed 's/.*\(<[^>]\+@[^>]\+>\).*/\1/' | sort | uniq
}
print_grep() {
git --no-pager log -p --full-history --follow $H0 -- "$1" | grep -i '[a-z0-9]@\|author\|copyright' | sort | uniq
}
prefix() {
sed "s/^/>>>$1 /"
}
collect_all() {
for F; do
print_commit_authors "$F" | prefix 1
echo
print_blame_authors "$F" | prefix 2
echo
print_grep "$F" | prefix 3
done |
sort |
uniq |
sed 's/@/(at)/'
}
collect_all \
include/NetworkManager.h \
libnm-core/NetworkManager.h \
libnm-core/nm-dbus-interface.h \
libnm-util/NetworkManager.h \
NetworkManager.h \
;
<<<
>>>1 <alfonso.sanchez-beato(at)canonical.com>
>>>1 <bberg(at)redhat.com>
>>>1 <bgalvani(at)redhat.com>
>>>1 <caillon(at)redhat.com>
>>>1 <daniel(at)gnoutcheff.name>
>>>1 <danw(at)gnome.org>
>>>1 <dcbw(at)redhat.com>
>>>1 <dsd(at)laptop.org>
>>>1 <gcampagna(at)src.gnome.org>
>>>1 <jarteaga(at)jbeta.is>
>>>1 <jiri(at)resnulli.us>
>>>1 <jklimes(at)redhat.com>
>>>1 <jlu(at)pengutronix.de>
>>>1 <lkundrak(at)v3.sk>
>>>1 <luzpaz(at)users.noreply.github.com>
>>>1 <mvollmer(at)redhat.com>
>>>1 <pktoss(at)gmail.com>
>>>1 <radykal(at)radykal.com>
>>>1 <rml(at)novell.com>
>>>1 <rodrigo(at)gnome-db.org>
>>>1 <tambet(at)gmail.com>
>>>1 <tambet(at)ximian.com>
>>>1 <tgraf(at)redhat.com>
>>>1 <thaller(at)redhat.com>
>>>1 <thomasbechtold(at)jpberlin.de>
>>>1 <tredaelli(at)redhat.com>
>>>2 <bberg(at)redhat.com>
>>>2 <bgalvani(at)redhat.com>
>>>2 <blueowl(at)centrum.cz>
>>>2 <daniel(at)gnoutcheff.name>
>>>2 <danw(at)redhat.com>
>>>2 <dcbw(at)redhat.com>
>>>2 <jarteaga(at)jbeta.is>
>>>2 <jiri(at)resnulli.us>
>>>2 <jlu(at)pengutronix.de>
>>>2 <lkundrak(at)v3.sk>
>>>2 <luzpaz(at)users.noreply.github.com>
>>>2 <mvollmer(at)redhat.com>
>>>2 <pktoss(at)gmail.com>
>>>2 <radykal(at)radykal.com>
>>>2 <rml(at)novell.com>
>>>2 <tambet(at)gmail.com>
>>>2 <tgraf(at)redhat.com>
>>>2 <thaller(at)redhat.com>
>>>2 <thomasbechtold(at)jpberlin.de>
>>>2 <tredaelli(at)redhat.com>
>>>3 [...] Red Hat, Inc.
>>>3 Thiago Bauermann <thiago.bauermann(at)gmail.com>
>>>3 <j(at)bootlab.org>
Most contributors on this list agreed to relicensing according to RELICENSE.md.
The following copyright holders did not answer the request for agreeing to
relicensing:
- <caillon(at)redhat.com>: the only contributions are removing code.
Also, Christopher was working for Red Hat at that time.
- <j(at)bootlab.org>: the patch was trivial, but the relevant code also got
removed by commit f003ba8ef7.
- <jiri(at)resnulli.us>: a trivial contribution to the header. Also,
Jiri was working for Red Hat at that time.
- <radykal(at)radykal.com>: fix spelling error in code comment.
- <rodrigo(at)gnome-db.org>: trivial contribution of adding a #define for
NM_DBUS_PATH_CONNECTION_SETTINGS.
- <tgraf(at)redhat.com>: a trivial contribution to the header. Also,
Thomas was working fro Red Hat at that time.
- <thiago.bauermann(at)gmail.com>: add a define to the header. This
was later removed by commit 7a8f33aa3d.
While not everybody agreed to this relicensing, I think the not covered
contributions are trivial additions to our header file. Also, this file
was always part of libnm. While it had the wrong license comment, it was
never intended to be GPL licensed.
For historic reasons is NMSettingBond implemented differently from other
settings. It uses a strdict, and adds some validation on top of that.
The idea was probably to be able to treat bond options more generically.
But in practice we cannot treat them as opaque values, but need to know,
validate and understand all the options. Thus, this implementation with a
strdict is not nice.
The user can set the GObject property NM_SETTING_BOND_OPTIONS to any
strdict, and the setter performs no validation or normalization. That
is probably good, because g_object_set() cannot return an error to
signalize invalid settings. As often, we have corresponding C API like
nm_setting_bond_add_option() and nm_setting_bond_remove_option(). It
should be possible to get the same result both with the C API and with
the GObject property setting. Since there is already a way to set
certain invalid values, it does not help if the C API tries to prevent
that. That implies, that also add-option does not perform additional
validation and sets whatever the user asks.
Remove all validation from nm_setting_bond_add_option() and
nm_setting_bond_remove_option(). This validation was anyway only very
basic. It was calling nm_setting_bond_validate_option(), which can check
whether the string is (for example) and integer, but it cannot do
validation beyond one option. In most cases, the validation needs to
take into account the bond mode or other options, so validating one
option in isolation is not very useful.
Proper validation should instead be done via nm_connection_verify().
However, due to another historic oddity, that verification is very
forgiving too and doesn't reject many invalid settings when it should.
That is hard to fix, because making validation more strict can break
existing (and working) configurations. However, verify() already contains
basic validation via nm_setting_bond_validate_option(). So in the previous
behavior nm_setting_bond_add_option() would silently do nothing (only
returning %FALSE) for invalid options, while now it would add the
invalid options to the dictionary -- only to have it later fail validation
during nm_connection_verify(). That is a slight change in behavior, however it
seems preferable.
It seems preferable and acceptable because most users that call
nm_setting_bond_add_option() already understand the meaning and valid
values. Keyfile and ifcfg-rh readers are the few exceptions, which really just
parse a string dictionary, without need to understand them. But nmtui
or nmstate already know the option they want to set. They don't expect
a failure there, nor do they need the validation.
Note that this change in behavior could be dangerous for example for the
keyfile/ifcfg-rh readers, which silently ignored errors before. We
don't want them to start failing if they read invalid options from a
file, so instead let those callers explicitly pre-validate the value
and log an warning.
https://bugzilla.redhat.com/show_bug.cgi?id=1887523
The underscore somehow indicated that these would be an internal
function. Which they are in the sense that they are in "shared/nm-glib-aux/".
But they part of our internal helper functions, and in our code base
their use is no discouraged or "private.
Also, next I'll replace the function call with a macro, so, I will
have a need for the underscore name.
Rename.
Change the default DNS priority of VPNs to -50, to avoid leaking
queries out of full-tunnel VPNs.
This is a change in behavior. In particular:
- when using dns=default (i.e. no split-dns) before this patch both
VPN and the local name server were added (in this order) to
resolv.conf; the result was that depending on resolv.conf options
and resolver implementation, the name servers were tried in a
certain manner which does not prevent DNS leaks.
With this change, only the VPN name server is added to resolv.conf.
- When using a split-dns plugin (systemd-resolved or dnsmasq), before
this patch the full-tunnel VPN would get all queries except those
ending in a local domain, that would instead be directed to the
local server.
After this patch, the VPN gets all queries.
To revert to the old behavior, set the DNS priority to 50 in the
connection profile.
Previously, both nm_setting_connection_add_permission() and the GObject
property setter would merely assert that the provided values are valid
(and otherwise don't do anything). That is bad for handling errors.
For example, we use the property setter to initialize the setting from
keyfile and GVariant (D-Bus). That means, if a user provides an invalid
permissions value, we would emit a g_critical() assertion failure, but
otherwise ignore the configuration. What we instead need to do is to
accept the value, and afterwards fail verification. That way, a proper error
message can be generated.
$ mcli connection add type ethernet autoconnect no ifname bogus con-name x connection.permissions 'bogus:'
(process:429514): libnm-CRITICAL **: 12:12:00.359: permission_new: assertion 'strchr (uname, ':') == NULL' failed
(process:429514): libnm-CRITICAL **: 12:12:00.359: nm_setting_connection_add_permission: assertion 'p != NULL' failed
Connection 'x' (2802d117-f84e-44d9-925b-bfe26fd85da1) successfully added.
$ $ nmcli -f connection.permissions connection show x
connection.permissions: --
While at it, also don't track the permissions in a GSList. Tracking one
permission in a GSList requires 3 allocations (one for the user string,
one for the Permission struct, and one for the GSList struct). Instead,
use a GArray. That is still not great, because GArray cannot be embedded
inside NMSettingConnectionPrivate, so tracking one permission also
requires 3 allocations (which is really a fault of GArray). So, GArray
is not better in the common case where there is only one permissions. But even
in the worst case (only one entry), GArray is no worse than GSList.
Also change the API of nm_setting_connection_add_permission().
Previously, the function would assert that the arguments are in
a certain form (strcmp (ptype, "user") == 0), but still document
the such behaviors like regular operation ("[returns] %FALSE if @ptype
or @pitem was invalid"). Don't assert against the function arguments.
Also, if you first set the user to "fo:o", then
nm_setting_connection_add_permission() would accept it -- only at
a later phase, the property setter would assert against such values.
Also, the function would return %FALSE both if the input value was
invalid (an error) and if the value already existed. I think the
function should not treat a duplicate entry like a badly formatted
input.
Now the function does much less asserting of the arguments, but will
return %FALSE only if the values are invalid. And it will silently ignore
duplicate entries.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/636
