Previously, nms_keyfile_writer_connection() would only return @out_path, if
it differed from @existing_path. That might make sense, if we could thereby
avoid duplicating @existing_path, however, we never did that
optimization.
Just consistently always return the path, let the caller deal with this.
When NM has to rebuild the platform cache, it first generates ADD and
then REMOVE events for the links. So, if an interface is removed and
readded, platform will emit the ADDED event with a new ifindex while
the device with old ifindex still exists.
In such case the manager currently updates the device's ifindex but
this causes problems as the DNS manager tracks configurations by their
ifindex and so the configurations for the old device will become
stale.
Fix this by removing the device and adding it again when we detect a
change of ifindex on a device that already had valid one.
https://bugzilla.redhat.com/show_bug.cgi?id=1542366
- since commit d17d26887c, a
NMSettingsConnection no longer "is-a" NMConnection. Instead,
we must call nm_settings_connection_get_connection() to obtain
the NMConnection instance. Adjust this in mirror_8021x_connection()
- don't leak "ssid" in mirror_8021x_connection()
- move deletion of the mirror-connection to known_network_data_free().
Previously, we must have made sure that every g_hash_table_remove()
and g_hash_table_insert()(!!) first deletes the mirror connection.
Likewise, in got_object_manager() when we call g_hash_table_remove_all(),
delete created mirror connections.
- rework interface_added() to make it robust against calling
interface_added() more than once without removing the interface
in between. Essentially, this just means that we first look into
"priv->known_networks" to see whether the @id is already tracked.
And if so, delete an existing mirror-connection as necessary.
Calling g_hash_table_insert() with a key which is already hashed
will destroy the *new* key. Since @id is used below, that would
be use after free.
Fixes: d635caf940551f8f5b52683b8379a1f81c58f8fc
IWD's mechanism for connecting to EAP networks requires a network config
file to be present in IWD's storage. NM and its clients however won't
allow a connection to be attempted until a valid NMConnection is created
on the NM side for the network. To avoid duplicating the settings from
the IWD-side profiles in NM, automatically create NMSettingConnections
for EAP networks preconfigured on the IWD side, unless a matching
connection already exists. These connections will use the "external"
EAP method to mean their EAP settings can't be modified through NM, also
they won't be valid for devices configured to use the wpa_supplicant
backend unfortunately.
Those nm-generated connections can be modified by NM users (makes sense
for settings not related to the wifi authentication) in which case they
get saved as normal profiles and will not be recreated as nm-generated
connections on the next run.
I want to additionally handle deleting connections from NM clients so
that they're also forgotten by IWD, in a later patch.
To allow connections that mirror IWD's configured WPA-Enterprise
networks to be seen as valid by NM, add a new value for the eap key in
802-1x settings. 802-1x.eap stores EAP method names. In the IWD
connections we don't know what EAP method is configured and we don't
have any of the other 802-1x properties that would be required for the
settings to verify.
These connections can't be activated on devices managed by wpa_supplicant.
Make this function public. I'm not sure if at this point it makes
much sense to add a new file for iwd-specific utilities.
While there add a way for the function to return error if security
type can't be mapped to an IWD-supported security type.
The known networks hash table is indexed by the (ssid, security) tuple
for fast lookups both on DBus signals related to an IWD known network
and local NMConnection signals such as on removal.
There's no need anymore for NMIwdManager to know when a network has been
connected to, InterfaceAdded signals are now emitted when a network is
saved as a Known Network.
Before 0.5 IWD has changed the known networks API to expose separate
objects for each known network and dropped the KnownNetworks
manager-like interface so stop using that interface. Following
patches will add tracking of the known networks through
ObjectManager.
Instead of passing the return values to g_variant_get_string or
g_variant_boolean and then checking the return value of that call,
add wrappers that first check's whether the variant is non-NULL
and of the right type.
g_variant_get_string doesn't allow a NULL parameter and will also never
return NULL according to the docs.
For the State property we assume a state "unknown" and emit a warning
if the property can't be read, "unknown" is also a string in IWD itself
which would be returned if something went really wrong. In any case
this shouldn't happen.
[thaller@redhat.com: fix missing initialization of nm_auto() variable
interfaces.]
The certificate setter function like nm_setting_802_1x_set_ca_cert()
actually load the file from disk, and validate whether it is a valid
certificate. That is very wrong to do.
For one, the certificates are external files, which are not embedded
into the NMConnection. That means, strongly validating the files while
loading the ifcfg files, is wrong because:
- if validation fails, loading the file fails in its entirety with
a warning in the log. That is not helpful to the user, who now
can no longer use nmcli to fix the path of the certificate (because
the profile failed to load in the first place).
- even if the certificate is valid at load-time, there is no guarantee
that it is valid later on, when we actually try to use the file. What
good does such a validation do? nm_setting_802_1x_set_ca_cert() might
make sense during nmcli_connection_modify(). At the moment when we
create or update the profile, we do want to validate the input and
be helpful to the user. Validating the file later on, when reloading
the profile from disk seems undesirable.
- note how keyfile also does not perform such validations (for good
reasons, I presume).
Also, there is so much wrong with how ifcfg reader handles EAP files.
There is a lot of duplication, and trying to be too smart. I find it
wrong how the "eap_readers" are nested. E.g. both eap_peap_reader() and
"tls" method call to eap_tls_reader(), making it look like that
NMSetting8021x can handle multiple EAP profiles separately. But it cannot. The
802-1x profile is a flat set of properties like ca-cert and others. All
EAP methods share these properties, so having this complex parsing
is not only complicated, but also wrong. The reader should simply parse
the shell variables, and let NMSetting8021x::verify() handle validation
of the settings. Anyway, the patch does not address that.
Also, the setting of the likes of NM_SETTING_802_1X_CLIENT_CERT_PASSWORD was
awkwardly only done when
privkey_format != NM_SETTING_802_1X_CK_FORMAT_PKCS12
&& scheme == NM_SETTING_802_1X_CK_SCHEME_PKCS11
It is too smart. Just read it from file, if it contains invalid data, let
verify() reject it. That is only partly addressed.
Also note, how writer never actually writes the likes of
IEEE_8021X_CLIENT_CERT_PASSWORD. That is another bug and not fixed
either.
- rename secret related functions to have a "_secret" prefix.
Also, rename read_8021x_password() because it's not only useful
for 802-1x.
- In particular, this patch adds _secret_read_ifcfg() helper (formerly
read_8021x_password()), which is smart enough to obtain secrets from
the keys ifcfg file. We have other places where we don't get this
right.
- on a minor note, the patch also makes an effort to clear passwords
and certifcate data from memory. Yes, there are countless places
where we don't do that, but in this case, it's done and is as simple
as replacing gs_free with nm_auto_free_secret, etc.
The term "keys" is used ambigiously. Rename occurances which reference
the "keys" ifcfg-rh file.
While at it, rename the file "parsed" to "main_ifcfg". It follows the
same pattern as the "keys_ifcfg" name.
If the library is available, let's at least compile both
crypto backends.
That is helpful when developing on crypto backends, so that
one does not have to configure the build twice.
With autotools, the build is only run during `make check`.
Not for meson, but that is generally the case with our meson
setup, that it also builds tests during the regular build step.
When reading a file, we may allocate intermediate buffers (realloc()).
Also, reading might fail halfway through the process.
Add a new flag that makes sure that this memory is cleared. The
point is when reading secrets, that we don't accidentally leave
private sensitive material in memory.
NMConnection is an interface, which is implemented by the types
NMSimpleConnection (libnm-core), NMSettingsConnection (src) and
NMRemoteConnection (libnm).
NMSettingsConnection does a lot of things already:
1) it "is-a" NMDBusObject and exports the API of a connection profile
on D-Bus
2) it interacts with NMSettings and contains functionality
for tracking the profiles.
3) it is the base-class of types like NMSKeyfileConnection and
NMIfcfgConnection. These handle how the profile is persisted
on disk.
4) it implements NMConnection interface, to itself track the
settings of the profile.
3) and 4) would be better implemented via delegation than inheritance.
Address 4) and don't let NMSettingsConnection implemente the NMConnection
interface. Instead, a settings-connection references now a NMSimpleConnection
instance, to which it delegates for keeping the actual profiles.
Advantages:
- by delegating, there is a clearer separation of what
NMSettingsConnection does. For example, in C we often required
casts from NMSettingsConnection to NMConnection. NMConnection
is a very trivial object with very little logic. When we have
a NMConnection instance at hand, it's good to know that it is
*only* that simple instead of also being an entire
NMSettingsConnection instance.
The main purpose of this patch is to simplify the code by separating
the NMConnection from the NMSettingsConnection. We should generally
be aware whether we handle a NMSettingsConnection or a trivial
NMConnection instance. Now, because NMSettingsConnection no longer
"is-a" NMConnection, this distinction is apparent.
- NMConnection is implemented as an interface and we create
NMSimpleConnection instances whenever we need a real instance.
In GLib, interfaces have a performance overhead, that we needlessly
pay all the time. With this change, we no longer require
NMConnection to be an interface. Thus, in the future we could compile
a version of libnm-core for the daemon, where NMConnection is not an
interface but a GObject implementation akin to NMSimpleConnection.
- In the previous implementation, we cannot treat NMConnection immutable
and copy-on-write.
For example, when NMDevice needs a snapshot of the activated
profile as applied-connection, all it can do is clone the entire
NMSettingsConnection as a NMSimpleConnection.
Likewise, when we get a NMConnection instance and want to keep
a reference to it, we cannot do that, because we never know
who also references and modifies the instance.
By separating NMSettingsConnection we could in the future have
NMConnection immutable and copy-on-write, to avoid all unnecessary
clones.
The limit of trying up to 10000 was arbitrary. In practice, we are not expected
that we need that many searches. If that would be the case (and we would have
10000 conflicting connections that take all the names), then we anyway would
need to refactor the code not to scale with O(n^2).
Replace the arbitrary limit with an even larger one. The new limit is so
large that in practice it's impossible to reach it.
Add a helper function nm_device_parent_find_for_connection() to
unify implementations of setting the parent in update_connection().
There is some change in behavior, in particular for nm-device-vlan.c,
which no longer compares the link information from platform. But
update_connection() is anyway a questionable concept, only used
for external assumed connection (which itself, is questionable). Meaning,
update_connection() is a hack not science, and it's not at all clear
what the correct behavior is.
Also, note how vlan's implementation differs from all others. Why?
Should we always resort to also check the information from platform?
Either way, one of the two approaches should be used consistently and
nm_device_parent_find_for_connection() opts to not consult platform
cache.
- always define the SESSION_TRACKING_* defines to replace
"#ifdef" with "#if".
- drop defining the consolekit database path CKDB_PATH in
config.h. The path was not customizable via configure/meson.
- fix meson build to enable consolekit support for session tracking
without also enabling logind/elogind session tracking.
logind/elogind is mutually exclusive, but consolekit session tracking
goes together just fine.
Using '#ifdef' is generally error prone. It's better to always define
a define and check for it explicitly. This way, the compiler can issue
a warning if the define does not exist.
Also, note how meson would always define NM_MORE_LOGGING, possibly to
"0". That means, for meson, we unintentionally always enabled more
logging because the define was always present.
Fix that.
gboolean is a typedef for "int".
While older compilers might treat such bitfields as unsigned ([1]),
commonly such a bitfield is signed and can only contain the values 0
and -1.
We only want to use numeric 1 for TRUE, hence, creating such bitfields
is wrong, or at least error prone.
In fact, in this case it's a bug, because later we compare
it with a regular gboolean
if (priv->scanning != new_scanning)
[1] https://lgtm.com/rules/1506024027114/
Fixes: e0f9677018
- have two variants of functions to set the SSID of an access point:
one that passes SSID as GBytes, and one that passes it as plain
data with length. Accepting a GBytes allows to share the immutable
GBytes instance.
- both functions now also support clearing the SSID. In
nm_wifi_ap_update_from_properties(), if the GVariant specifies
a "SSID", we always update the access point. We already support
chaging the SSID, so why not support changing it to *no* SSID
(hidden).
GBytes makes more sense, because it's immutable.
Also, since at other places we use GBytes, having
different types is combersome and requires needless
conversions.
Also:
- avoid nm_utils_escape_ssid() instead of _nm_utils_ssid_to_string().
We use nm_utils_escape_ssid() when we want to log the SSID. However, it
does not escape newlines, which is bad.
- also no longer use nm_utils_same_ssid(). Since it no longer
treated trailing NUL special, it is not different from
g_bytes_equal().
- also, don't use nm_utils_ssid_to_utf8() for logging anymore.
For logging, _nm_utils_ssid_escape_utf8safe() is better because
it is loss-less escaping which can be unambigously reverted.
nm_utils_same_ssid() has a comment
* Earlier versions of the Linux kernel added a NULL byte to the end of the
* SSID to enable easy printing of the SSID on the console or in a terminal,
* but this behavior was problematic (SSIDs are simply byte arrays, not strings)
* and thus was changed. This function compensates for that behavior at the
* cost of some compatibility with odd SSIDs that may legitimately have trailing
* NULLs, even though that is functionally pointless.
and the functionality was introduced by commit
ccb13f0bdd.
There was only place left that calls nm_utils_same_ssid().
I really don't think this is the right approach, nor is it clear
that this is still necessary. Also, it seems to only matter with
WEXT, and we should not have such an ugly hack in all cases.
Use GBytes instead of GBytesArray. GBytes is immutable and
can be shared.
It is also the type that we natively get from
nm_setting_wireless_get_ssid(). This way we avoid some
conversions.
These should be logged on DEBUG level:
<warn> platform-linux: do-change-link[2]: failure changing link: failure 97 (Address family not supported by protocol)
<warn> device (wlo1): failed to enable userspace IPv6LL address handling (unspecified)
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/issues/10
