When subclassing a GObject type, the class and object structs
must be available and defined in the header.
For libnm, and in particular for NMSetting classes, we don't want
users to subclass NMSetting. It also doesn't work, because libnm
has internal code that is necessary to hook up the NMSetting class.
You cannot define your own type and make it work together with
libnm.
Having the structs in public headers limits what we can do with them.
For example, we could embed the private data directly in the structures
and avoid the additional indirection.
This is an API break, but for something that most likely nobody cares
about. Or better, nobody should care about. API is not what is
accidentally defined in a header, API was the library provides to
meaningfully use. Subclassing these types is not meaningful and was
only accidentally possible so far.
Only hide the structs for now. More cleanup is possible later. We shall
however aim to keep the padding and struct layout to not also break ABI.
(cherry picked from commit e46d484fae)
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().
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
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.
However, don't also use the NM_DEPRECATED_IN_1_32 macro, because that
causes annoying compiler warnings.
There is no replacement for the function in libnm, nor is it planned
to add one. So users may still call it, but they are now warned by
documentation that it may not be a good idea.
lgtm.com warns about these uses. They are correct though. Maybe the code should
not use alloca() simply to suppress the warning. Instead, add a comment pointing
out that this is in fact correct.
Setting pause-rx/pause-tx to an explicit value, implies that the user
does not want to enable autoneg. Reject that as invalid value in the
connection profile.
Introducing ethtool PAUSE support with:
* ethtool.pause-autoneg on/off
* ethtool.pause-rx on/off
* ethtool.pause-tx on/off
Limitations:
* When `ethtool.pause-autoneg` is set to true, the `ethtool.pause-rx`
and `ethtool.pause-tx` will be ignored. We don't have warning for
this yet.
Unit test case included.
Signed-off-by: Gris Ge <fge@redhat.com>
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/829
Found by Coverity:
Error: RESOURCE_LEAK (CWE-772):
NetworkManager-1.31.3/src/libnm-core-impl/nm-utils.c:2772: alloc_fn: Storage is returned from allocation function "nm_utils_tc_action_from_str".
NetworkManager-1.31.3/src/libnm-core-impl/nm-utils.c:2772: var_assign: Assigning: "action" = storage returned from "nm_utils_tc_action_from_str(extra_opts, error)".
NetworkManager-1.31.3/src/libnm-core-impl/nm-utils.c:2785: leaked_storage: Variable "action" going out of scope leaks the storage it points to.
# 2783| tfilter = nm_tc_tfilter_new(kind, parent, error);
# 2784| if (!tfilter)
# 2785|-> return NULL;
# 2786|
# 2787| nm_tc_tfilter_set_handle(tfilter, handle);
Fixes: de41c45e61 ('libnm-core: add functionality for dealing with tc-style traffic filter specifiers')
Modern WPA3 authentication methods like SAE and WPA-EAP-SUITE-B-192 need
to have management frame protection set to required according to the
standard. Since the last commit, we enforce this automatically when
key-mgmt is set to 'owe', 'sae' or 'wpa-eap-suite-b-192', so disabling
it manually should not be possible.
Add a check to the pmf property that makes sure it can't be set to
'disabled' or 'optional' when one of those key-mgmt methods is used.
The key-mgmt property of NMSettingWirelessSecurity is slightly confusing
when you know there's also a wpa_supplicant configuration option called
"key_mgmt". Our property is not the same as that supplicant option even
though they do have things in common. NMs key-mgmt is not exactly meant
to configure which AKM suites you want to use, but rather which method
of wifi security is being used (so "wpa2+wpa3 personal", "wpa3 personal
only" or "wpa3 enterprise only").
Try to make this a bit clearer in the documentation of the property by
rewriting it and listing those security methods.
For NetworkManager profiles, "connection.uuid" is the identifier of the
profile. It is supposed to be a UUID, however:
- the UUID was not ensured to be all-lower case. We should make sure
that our UUIDs are in a consistent manner, so that users can rely
on the format of the string.
- the UUID was never actually interpreted as a UUID. It only was some
opaque string, that we use as identifier. We had nm_utils_is_uuid()
which checks that the format is valid, however that did not fully
validate the format, like it would accept "----7daf444dd78741a59e1ef1b3c8b1c0e8"
and "549fac10a25f4bcc912d1ae688c2b4987daf444d" (40 hex characters).
Both invalid UUIDs and non-normalized UUID should be normalized. We
don't want to break existing profiles that use such UUIDs, thus we don't
outright reject them. Let's instead mangle them during
nm_connection_normalize().
This only uses glib's md5 code and has no dependency on our
crypto libraries (gnutls, nss).
Move it to "libnm-glib-aux" so it can be freely used.
Maybe it deserves a better name, but "nm-shared-utils.h" is a heap of
various utility functions, it's hard to name them well.
We use util-linux's libuuid for handling UUIDs. But UUIDs are
really a trivial thing, at least the portion that we use.
Reimplement the parse/unparse/generate_random() methods and drop
the dependency. Note that no other libraries from our dependency chain
was dragging in libuuid, so thereby we really get rid of the dependency.
We still require libuuid for building, because it is used by an example
program. Maybe that should be changed, to avoid the build dependency.
But that can be done at a later time.
A small bug sneaked into commit 3ef3733c81 ('wireless-security: ensure
Mesh networks can't use anything but SAE') during review: Instead of
allowing only "sae" or "none" as key-mgmt, we now disallow "sae" and
"none", but allow anything else. This is obviously not what was
intended, so fix the check.
Also move the valid_key_mgmt check back up to where it was before that
commit, it seems we want to apply that check in all cases.
This patch is introducing the wired setting accept-all-mac-addresses
property. The value corresponds to the kernel flag IFF_PROMISC.
When accept-all-mac-address is enabled, the interface will accept all
the packets without checking the destination mac address.
Signed-off-by: Fernando Fernandez Mancera <ffmancera@riseup.net>
