- always remember priv->last_state_dir that we received via
D-Bus. Only later, during get_config_path() we will check
whether the path is valid.
- remember in priv->warned_state_dir the full path for
which we warned. We want to print a warning for each
path once, if the path changes, then we also want a new
warning. A boolean flag cannot express that.
Otherwise we see a warning:
<warn> [1622790097.3601] config: unknown key firewall-backend in section [main] of file /etc/NetworkManager/NetworkManager.conf
Fixes: 1da1ad9c99 ('firewall: make firewall-backend configurable via "NetworkManager.conf"')
So far, we didn't verify the secondary connections at all.
But these really are supposed to be UUIDs.
As we now also normalize "connection.uuid" to be in a strict
format, the user might have profiles with non-normalized UUIDs.
In that case, the "connection.uuid" would be normalized, but
"connection.secondaries" no longer matches. We can fix that by
also normalizing "connection.secondaries". OK, this is not a very good
reason, because it's unlikely to affect any users in practice ('though
it's easy to reproduce).
A better reason is that the secondary setting really should be well
defined and verified. As we didn't do that so far, we cannot simply
outright reject invalid settings. What this patch does instead, is
silently changing the profile to only contain valid settings.
That has it's own problems, like that the user setting an invalid
value does not get an error nor the desired(?) outcome.
But of all the bad choices, normalizing seems the most sensible
one.
Note that in practice, most client applications don't rely on setting
arbitrary (invalid) "UUIDs". They simply expect to be able to set valid
UUIDs, which they still are. For example, nm-connection-editor presents
a drop down list of VPN profile, and nmcli also resolves connection IDs
to the UUID. That is, clients already have an intimate understanding of
this setting, and don't blindly set arbitrary values. Hence, this
normalization is unlikely to hit users in practice. But what it gives
is the guarantee that a verified connection only contains valid UUIDs.
Now all UUIDs will be normalized, invalid entries removed, and the list
made unique.
GSList requires an additional allocation for the container struct for each
element. Also, it does not have O(1) direct access. It's a pretty bad
data structure, especially if the underlying data is in form of a strv
array.
Use a GArray instead and the nm_strvarray_*() helpers.
For example for NM_SETTING_CONNECTION_SECONDARIES, the user can set
the GObject property to a string list that includes empty strings.
The C accessors (add/remove-by-value) should also accept any strings that
are accepted otherwise. Asserting against empty strings is wrong. If the
setting wants to reject empty strings, then it should use verify().
Since commit 207cf3d5d4 ('libnm: normalize "connection.uuid"') the
"connection.uuid" is normalized to be a valid UUID and all lower case.
That means, if we have .nmmeta files on disk with a previously valid,
but now invalid UUID, the meta file is no longer going to match.
Reject such file outright as invalid. If we really wanted to preserve
backward compatibility, then we would have to also normalize the
filename when we read it. However, that means, that suddenly we might
have any number of compatible .nmmeta files that normalize to the same
UUID, like the files
71088c75dec54119ab41be71bc10e736aaaabbbb.nmmeta
F95D40B4-578A-5E68-8597-39392249442B.nmmeta
f95d40b4-578a-5e68-8597-39392249442b.nmmeta
Having multiple places for the nmmeta file is complicated to handle.
Also, we often have the connection profile (and the normalized UUID)
first, and then check whether it has a .nmmeta file. If we would support
those unnormalized file names, we would have to visit all file names and
try to normalize it, to find those with a matching UUID.
Non-normalized UUIDs really should not be used and they already are not
working anymore for the .nmmeta file. This commit only outright rejects
them. This is a change in behavior, but the behavior change happened
earlier when we started normalizing "connection.uuid".
"uuid" is returned from nms_keyfile_nmmeta_check_filename(),
and contains "$UUID.nmmeta". We must compare only the first
"uuid_len" bytes.
Fixes: 064544cc07 ('settings: support storing "shadowed-storage" to .nmmeta files')
Most of the time, we care about whether we have a normalized UUID.
nm_uuid_is_valid_full() only exists for a particular case where we want
to use the function in a header, without including "nm-uuid.h". In that
case, we actually also care about normalized UUIDs.
If the TC setting contains no qdiscs and filters, it is lost after a
write-read cycle. Fix this by adding a new property to indicate the
presence of the (empty) setting.
NetworkManager supports a very limited set of qdiscs. If users want to
configure a unsupported qdisc, they need to do it outside of
NetworkManager using tc.
The problem is that NM also removes all qdiscs and filters during
activation if the connection doesn't contain a TC setting. Therefore,
setting TC configuration outside of NM is hard because users need to
do it *after* the connection is up (for example through a dispatcher
script).
Let NM consider the presence (or absence) of a TC setting in the
connection to determine whether NM should configure (or not) qdiscs
and filters on the interface. We already do something similar for
SR-IOV configuration.
Since new connections don't have the TC setting, the new behavior
(ignore existing configuration) will be the default. The impact of
this change in different scenarios is:
- the user previously configured TC settings via NM. This continues
to work as before;
- the user didn't set any qdiscs or filters in the connection, and
expected NM to clear them from the interface during activation.
Here there is a change in behavior, but it seems unlikely that
anybody relied on the old one;
- the user didn't care about qdiscs and filters; NM removed all
qdiscs upon activation, and so the default qdisc from kernel was
used. After this change, NM will not touch qdiscs and the default
qdisc will be used, as before;
- the user set a different qdisc via tc and NM cleared it during
activation. Now this will work as expected.
So, the new default behavior seems better than the previous one.
https://bugzilla.redhat.com/show_bug.cgi?id=1928078
Let's instead add a generic nm_device_get_ports() function.
Also, only adding new API is maybe not sufficient. We should
at the same time deprecate and alias the D-Bus API, like was done
for commit 067a3d6c08 ('nm-device: expose via D-Bus the 'hw-address'
property').
This reverts commit 754143f4e8.
lgtm.com doesn't like this:
Query pack:com.lgtm/cpp-queries
Query ID:cpp/duplicate-include-guard
Using the same include guard macro in more than one header file may
cause unexpected behavior from the compiler.
both for src/libnm-base/nm-ethtool-utils-base.h and
src/libnm-client-public/nm-ethtool-utils.h. But this is intentional,
because these two files are supposed to be identical (but compiled
twice, under different context).
Suppress the warning.
lgtm.com doesn't like this:
Query pack:com.lgtm/cpp-queries
Query ID:cpp/function-in-block
Functions should always be declared at file scope. It is confusing
to declare a function at block scope, and the visibility of the function
is not what would be expected.
Mimic the behaviour of wpa_supplicant where the "secure" identity in
TTLS and PEAP (802-1x.identity) is used as a fallback in the anonymous
identity (802-1x.anonymous_identity) if that is not provided. This is
needed to keep the profiles compatible between the two wifi backends,
for users of poorly configured WPA-Enterprise networks that require the
user login to be sent in phase 1 or in both phases.
The code responsible for this mechanism in wpa_supplicant, at the time
of writing, is
https://w1.fi/cgit/hostap/tree/src/eap_peer/eap.c?id=c733664be9dd3763c03f2da2cb32a23775dde388#n1688
and offers no comment about the privacy implications.
In two similar ``if () {} else if () {} else if () {} else {}`` sequences
the latter two {} blocks were unreachable. In the
identity/anonymous-identity case, anonymous-identity is optional,
wpa_supplicant will fall back to identity, so only check that (a likely
privacy issue because no NM or wpa_s documentation explains that the
"secure" identity is also sent in plaintext if anonymous_identity is
missing.)
In the phase2_auth/phase2_autheap case change the message to make it
clear that exactly one of the properties is expected to be present.
Drop the empty string checks because those cases is validated later in
verify() anyway.
Coverity thinks there is a problem here:
Error: TAINTED_SCALAR (CWE-20): [#def233]
NetworkManager-1.31.5/src/libnm-platform/nm-netlink.c:1437: tainted_argument: Calling function "recvmsg" taints argument "msg".
NetworkManager-1.31.5/src/libnm-platform/nm-netlink.c:1458: tainted_data: Passing tainted expression "msg.msg_controllen" to "g_realloc", which uses it as an allocation size.
NetworkManager-1.31.5/src/libnm-platform/nm-netlink.c:1458: remediation: Ensure that tainted values are properly sanitized, by checking that their values are within a permissible range.
# 1456|
# 1457| msg.msg_controllen *= 2;
# 1458|-> msg.msg_control = g_realloc(msg.msg_control, msg.msg_controllen);
# 1459| goto retry;
# 1460| }
but the problem is not the tainted data. The problem is how should
we handle MSG_CTRUNC? If we reach MSG_CTRUNC we already lost a message.
Retrying to receive the next message is not going to fix that and is
wrong.
Also, there really is no reason why any truncation should happen. The only
ancillary data that should be present is the sender information, and for
that our buffer is supposed to be large enough.
So, simply ignore truncation. It shouldn't happen, if it happened we
cannot recover from it (aside failing an assertion), and all we really
care are the retrieved credentials. If truncation happened, we might
not have retrieved the credentials, but then that is for the caller
to handle (by rejecting the message as untrusted).
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/872
Changing "NetworkManager.conf" is problematic, because the package management
system will detect if the user modified the file and leave .rpmnew files (or
similar).
Still, we only recently modified the file already to mention Libera.Chat.
So now is the time for more rewording.
