- add more error handling and free/unref data.
- split code in 3 functions: vpn_connection_import(),
connection_add() and main(). These steps are mostly
independent, and having them in separate functions
makes their separation clearer.
- handle error from nm_client_add_connection_async() to
exit program with non zero exit code.
Review and replace usages of the two nm_connection_to_dbus() flags
marked deprecated in commit 84648e562c98 ('libnm: Refactor
NM_CONNECTION_SERIALIZE_* flags'):
NM_CONNECTION_SERIALIZE_NO_SECRETS and
NM_CONNECTION_SERIALIZE_ONLY_SECRETS.
Use the new nm_connection_to_dbus() flags to filter secrets instead of
cloning connections and using
_nm_connection_clear_secrets_by_secret_flags() and then serializing all
secrets in NMSettingsConnection. Fix a related comment.
nm-settings-connection.c has code similar to this in two places:
/* FIXME: improve NMConnection API so we can avoid the overhead of cloning the connection,
* in particular if there are no secrets to begin with. */
connection_cloned = nm_simple_connection_new_clone(new);
/* Clear out unwanted secrets */
_nm_connection_clear_secrets_by_secret_flags(connection_cloned,
NM_SETTING_SECRET_FLAG_NOT_SAVED
| NM_SETTING_SECRET_FLAG_AGENT_OWNED);
secrets = nm_g_variant_ref_sink(
nm_connection_to_dbus(connection_cloned, NM_CONNECTION_SERIALIZE_ONLY_SECRETS));
It seems the secrets filtering can be done by nm_connection_to_dbus() if
the NM_CONNECTION_SERIALIZE_* flags are extended. The current set of
flags contains flags that start with NO, ONLY and WITH prefixes, which
makes it useless for combining the flags because most combinations of
more than one flag don't have a clear interpretation. So they're mostly
useful when used alone, i.e. you'd need to add a new enum value for
each new subset of settings to be serialized.
To get the most flexibility from a small set of flags they should
either all be of the WITH_* type or NO_* type. In the former case they
could be combined to extend the subset of properties serialized, in the
latter case each flag would reduce the subset. After trying both
options I found it's easier to adapt the current set of flags to the
WITH_* schema while keeping binary and source compatibility. This
commit changes the set of flags in the following way:
NM_CONNECTION_SERIALIZE_ALL is kept for compatibility but is equivalent
to a combination of other flags.
NM_CONNECTION_SERIALIZE_WITH_NON_SECRET is added with the same value as
NM_CONNECTION_SERIALIZE_NO_SECRETS, it implies that non-secret
properties are included but doesn't prevent including other properties.
Since it couldn't be meaningfully combined with any other flag this
change shouldn't break compatibility.
Similarly NM_CONNECTION_SERIALIZE_WITH_SECRETS is added with the same
value as existing NM_CONNECTION_SERIALIZE_ONLY_SECRETS with the same
consideration about compatibility.
NM_CONNECTION_SERIALIZE_WITH_SECRETS_AGENT_OWNED and the new
NM_CONNECTION_SERIALIZE_WITH_SECRETS_SYSTEM_OWNED and
NM_CONNECTION_SERIALIZE_WITH_SECRETS_NOT_SAVED add only subsets of
secrets and can be combined. For backwards compatibility
NM_CONNECTION_SERIALIZE_ONLY_SECRETS is basically ignored when either of
these three is present, so that the value:
..ONLY_SECRETS | ..AGENT_OWNED works as previously.
This introduces no change. On systemd branch files were moved
around to follow what also happened on master. The merge commit
just makes git aware of this part of history.
initscripts don't support "$VLAN_ID". They actually support "$VID",
which NetworkManager doesn't.
"$VLAN_ID" was introduced by commit 10b32be37b ('ifcfg-rh: various VLAN
cleanups'). It has a comment about "backward compatibility" for the case
where the reader would ignore "$VLAN_ID" if "$DEVICE"'s name contains
a suffix that is parsable as VLAN ID.
That is wrong. If a new feature gets introduce (like NetworkManager
supporting "$VLAN_ID"), then there is no way that an older version of the
tool -- which doesn't know the new feature yet (initscripts) -- supports it.
This is not what backward compatibility means. Backward compatibility
means that if a user has an old ifcfg-file without "$VLAN_ID", then we
continue parsing it as before.
Consider, when a user (or NetworkManager) writes a configuration
DEVICE=vlan9
PHYSDEV=eth0
VLAN_ID=10
then it makes no sense to ignore VLAN_ID=10 and use "9" instead.
Otherwise the user (or NetworkManager) should not have written the
file this way.
Also, NetworkManager profiles support "connection.interface-name=vlan9"
together with "vlan.id=10". Such a configuration is valid and must be
expressible in ifcfg-rh format. The ifcfg-rh writer code did not somehow
restrict the setting of "$VLAN_ID" to account for this odd behavior. Whenever
NetworkManager in the past wrote VLAN_ID variable to file, it really meant
it.
https://bugzilla.redhat.com/show_bug.cgi?id=1907960https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/794
This reverts commit 389575a6b1.
When the command line contains BOOTIF and there is another ip=
argument specifying an interface name, we can follow 2 approaches:
a) BOOTIF creates a new distinct connection with DHCP
(the behaviour before the commit)
b) the connection generated for ip= will be also be bound to the
BOOTIF MAC (the behavior introduced by the commit)
Restore a) because we can't be sure that the MAC address refers to the
same interface. In that case it's preferable to generate a different
connection.
https://bugzilla.redhat.com/show_bug.cgi?id=1915493#c35
It's not entirely clear how to treat %NULL.
Clearly "match.interface-name=eth0" should not
match with an interface %NULL. But what about
"match.interface-name=!eth0"? It's now implemented
that negative matches still succeed against %NULL.
What about "match.interface-name=*"? That probably
should also match with %NULL. So we treat %NULL really
like "".
Against commit 11cd443448bc ('iwd: Don't call IWD methods when device
unmanaged'), we got this backtrace:
#0 0x00007f1c164069f1 in __strnlen_avx2 () at ../sysdeps/x86_64/multiarch/strlen-avx2.S:62
#1 0x00007f1c1637ac9e in __fnmatch (pattern=<optimized out>, string=<optimized out>, string@entry=0x0, flags=flags@entry=0) at fnmatch.c:379
p = 0x0
res = <optimized out>
orig_pattern = <optimized out>
n = <optimized out>
wpattern = 0x7fff8d860730 L"pci-0000:03:00.0"
ps = {__count = 0, __value = {__wch = 0, __wchb = "\000\000\000"}}
wpattern_malloc = 0x0
wstring_malloc = 0x0
wstring = <optimized out>
alloca_used = 80
__PRETTY_FUNCTION__ = "__fnmatch"
#2 0x0000564484a978bf in nm_wildcard_match_check (str=0x0, patterns=<optimized out>, num_patterns=<optimized out>) at src/core/nm-core-utils.c:1959
is_inverted = 0
is_mandatory = 0
match = <optimized out>
p = 0x564486c43fa0 "pci-0000:03:00.0"
has_optional = 0
has_any_optional = 0
i = <optimized out>
#3 0x0000564484bf4797 in check_connection_compatible (self=<optimized out>, connection=<optimized out>, error=0x0) at src/core/devices/nm-device.c:7499
patterns = <optimized out>
device_driver = 0x564486c76bd0 "veth"
num_patterns = 1
priv = 0x564486cbe0b0
__func__ = "check_connection_compatible"
device_iface = <optimized out>
local = 0x564486c99a60
conn_iface = 0x0
klass = <optimized out>
s_match = 0x564486c63df0 [NMSettingMatch]
#4 0x0000564484c38491 in check_connection_compatible (device=0x564486cbe590 [NMDeviceVeth], connection=0x564486c6b160, error=0x0) at src/core/devices/nm-device-ethernet.c:348
self = 0x564486cbe590 [NMDeviceVeth]
s_wired = <optimized out>
Fixes: 3ced486f41 ('libnm/match: extend syntax for match patterns with '|', '&', '!' and '\\'')
https://bugzilla.redhat.com/show_bug.cgi?id=1942741
Let's shortcut the test by consistently checking whether num_patterns
is positive before matching.
It's more about having a consistent form of the "if" checks, than
anything else.
When adding an IPv4 address, kernel automatically adds a local route.
This is done by fib_add_ifaddr(). Note that if the address is
IFA_F_SECONDARY, then the "src" is the primary address. That means, with
nmcli connection add con-name t type ethernet ifname t autoconnect no \
ipv4.method manual ipv6.method disabled \
ipv4.addresses '192.168.77.10/24, 192.168.77.11/24'
we get two routes:
"local 192.168.77.10 dev t table local proto kernel scope host src 192.168.77.10"
"local 192.168.77.11 dev t table local proto kernel scope host src 192.168.77.10"
Our code would only generate instead:
"local 192.168.77.10 dev t table local proto kernel scope host src 192.168.77.10"
"local 192.168.77.11 dev t table local proto kernel scope host src 192.168.77.11"
Afterwards, this artificial route will be leaked:
#!/bin/bash
set -vx
nmcli connection delete t || :
ip link delete t || :
ip link add name t type veth peer t-veth
nmcli connection add con-name t type ethernet ifname t autoconnect no ipv4.method manual ipv4.addresses '192.168.77.10/24, 192.168.77.11/24' ipv6.method disabled
nmcli connection up t
ip route show table all dev t | grep --color '^\|192.168.77.11'
sleep 1
nmcli device modify t -ipv4.addresses 192.168.77.11/24
ip route show table all dev t | grep --color '^\|192.168.77.11'
ip route show table all dev t | grep -q 192.168.77.11 && echo "the local route 192.168.77.11 is still there, because NM adds a local route with wrong pref-src"
It will also be leaked because in the example above ipv4.route-table is
unset, so we are not in full route sync mode and the local table is not
synced.
This was introduced by commit 3e5fc04df3 ('core: add dependent local
routes configured by kernel'), but it's unclear to me why we really need
this. Drop it again and effectively revert commit 3e5fc04df3 ('core:
add dependent local routes configured by kernel').
I think this "solution" is still bad. We need to improve our route sync
approach with L3Cfg rework. For now, it's probably good enough.
https://bugzilla.redhat.com/show_bug.cgi?id=1907661
This effectively reverts commit cd89026c5f ('core: add dependent
multicast route configured by kernel for IPv6').
It's not clear to me why this was done or why it would be correct.
True, kernel automatically adds multicast route like
multicast ff00::/8 dev $IFACE table local proto kernel metric 256 pref medium
But NetworkManager ignores all multicast routes for now. So the dependent
routes cannot contain multicast routes as they are not handled. Also,
the code added a unicast route, so I don't understand why the comment
is talking about multicast.
This seems just wrong. Drop it.
Watch for NMSettingConnection changes and creation signals and convert
them to IWD format and write them to the configured IWD profile storage
directory. The logic is off by default and gets enabled when the new
iwd-config-path setting in nm.conf's [main] group is set to a path to
an existing directory.
The idea here is that when a user edits an NM connection profile, the
change is immediately mirrored in IWD since IWD watches its
configuration directory using inotify. This way NM clients can be used
to edit 802.1x settings, the PSK passphrase or the SSID -- changes that
would previously not take effect with the IWD backend.
Some precautions are taken to not make connections owned by a user
available to other users, such connections are not converted at all.
In all other cases where a connection cannot be converted sufficiently
well to the IWD format, for various reasons, we also give up and not
mirror these connections.
Due to IWD limitations and design differences with NM this logic has
many problems where it may not do its task properly. It's meant to work
on a best-effort and "better than nothing" basis, but it should be safe
in that it shouldn't delete users data or reveal secrets, etc. The most
obvious limitation is that there can be multiple NM connections
referring to the same SSID+Security tuple and only one IWD profile can
exist because the filename is based on only the SSID+Security type. We
already had one NM connection selected for each IWD KnownNetwork and
referenced by a pointer, so we ignore changes in NM connections other
than that selected one.
Add code that can take an NMConnection and convert it to the IWD
network config file format so as to be able to mirror NM connection
profiles to IWD connection profiles and make basic editing IWD
profile possible from nm-connection-editor. The focus here is on 802.1x
settings.
