For profiles in "/etc/NetworkManager/system-connections", we did not enforce
that the keyfiles have a special suffix, nor did we generate the
filenames in such a manner. In hindsight, I think that was a mistake.
Recently we added "/run/NetworkManager/system-connections" as additional
keyfile directory. Enforce a suffix and write keyfiles with such a name.
In principle, we could also start writing keyfiles in /etc with the
same suffix. But let's not do that, because we anyway cannot enforce
it.
An ugly part is, that during `nmcli connection load` we need to
determine whether the to-be-loaded connection is under /etc or /run.
Preferably, we would allow any kind of symlinking as what matters
is the file object (inode) and not the path. Anyway, we don't do
that but compare plain paths. That means, paths which are not
in an expected form, will be rejected. In particular, the paths
starting with "/run/..." and "/var/run/..." will be treated differently,
and one of them will be rejected.
Note that ifcfg-rh plugin strictly enforces that the path
starts with IFCFG_DIR as well. So, while this is a breaking
change for keyfile, I think it's reasonable.
keyfile already supports omitting the "connection.id" and
"connection.uuid". In that case, the ID would be taken from the
keyfile's name, and the UUID was generated by md5 hashing the
full filename.
No longer do this during nm_keyfile_read(), instead let all
callers call nm_keyfile_read_ensure_*() to their liking. This is done
for two reasons:
- a minor reason is, that one day we want to expose keyfile API
as public API. That means, we also want to read keyfiles from
stdin, where there is no filename available. The implementation
which parses stdio needs to define their own way of auto-generating
ID and UUID. Note how nm_keyfile_read()'s API no longer takes a
filename as argument, which would be awkward for the stdin case.
- Currently, we only support one keyfile directory, which (configurably)
is "/etc/NetworkManager/system-connections".
In the future, we want to support multiple keyfile dirctories, like
"/var/run/NetworkManager/profiles" or "/usr/lib/NetworkManager/profiles".
Here we want that a file "foo" (which does not specify a UUID) gets the
same UUID regardless of the directory it is in. That seems better, because
then the UUID won't change as you move the file between directories.
Yes, that means, that the same UUID will be provided by multiple
files, but NetworkManager must already cope with that situation anyway.
Unfortunately, the UUID generation scheme hashes the full path. That
means, we must hash the path name of the file "foo" inside the
original "system-connections" directory.
Refactor the code so that it accounds for a difference between the
filename of the keyfile, and the profile_dir used for generating
the UUID.
Add 3 variants of _nm_utils_hexstr2bin*():
- _nm_utils_hexstr2bin_full(), which takes a preallocated
buffer and fills it.
- _nm_utils_hexstr2bin_alloc() which returns a malloc'ed
buffer
- _nm_utils_hexstr2bin_buf(), which fills a preallocated
buffer of a specific size.
char[] is not a char *; it can not go NULL.
test-ifupdown.c:147:27: error: address of array 'n->name' will always evaluate
to 'true' [-Werror,-Wpointer-bool-conversion]
g_assert (b->name && n->name);
test-ifupdown.c:156:27: error: address of array 'm->key' will always evaluate
to 'true' [-Werror,-Wpointer-bool-conversion]
g_assert (k->key && m->key);
This is useful for in-memory connections to persist NetworkManager
restarts (as opposed to machine restarts).
Perhaps most improtantly, this allows generating in-memory connections outside
NetworkManager, e.g. passing configuration from early boot firmware in initrd.
Note that this does *not* aspire to do more than it says on the tin:
Notably, it doesn't touch the problem of provisioning connections in multiple
persistent connection directories and thus doesn't have to deal with the
problem of deleting or overlaying the connections tha (rh #772414) deals
with.
As we accept addr_family %AF_UNSPEC to detect the address family,
we also need to return it. Just returning the binary address without
the address family makes no sense.
Note that NMSettingEthtool and NMSettingMatch don't have such
functions either.
We have API
nm_connection_get_setting (NMConnection *, GType)
nm_connection_get_setting_by_name (NMConnection *, const char *)
which can be used generically, meaning: the requested setting type
is an argument to the function. That is generally more useful and
flexible.
Don't add API which duplicates existing functionality and is (arguably)
inferiour. Drop it now. This is an ABI/API break for the current development
cycle where the 1.14.0 API is still unstable. Indeed it's already after
1.14-rc1, which is ugly. But it's also unlikely that somebody already uses
this API/ABI and is badly impacted by this change.
Note that nm_connection_get_setting() and nm_connection_get_setting_by_name()
are slightly inconvenient in C still, because they usually require a cast.
We should fix that by changing the return type to "void *". Such
a change may be possibly any time without breaking API/ABI (almost, it'd
be an API change when taking a function pointer without casting).
(cherry picked from commit a10156f516)
Does not address the issues that the existing logging is much too verbose
and does not provide necessary context for what it's complaining. The
logging messages should be improved.
At least, the logging macro gives all messages a "ifupdown: " prefix.
Don't listen to udev to find out about devices. First of all, using udev
for this is already very wrong, because we have the platform cache.
Anyway, all that the device information is used, is pointless as well.
Drop it.
It's pointless because:
The entires in eni_ifaces are already indexed by the interface name.
Likewise, all NMIfupdownConnection set "connection.interface-name" to
restict the profile by name.
/e/n/i matches devices is by name, that's it. We don't need udev to
look up the MAC address (by name!!) to later ignore/match devices
by MAC address. Especially, because NetworkMaanger can already
restrict profiles to devices based on the interface name.
Likewise, NetworkMaanger can use the interface name for the
unmanaged-specs.
It's wrong to extend the on-disk configuration from /e/n/i with runtime
information from udev, especially, because other NetworkMaanger layers
are perfectly content using the interface name for this purpose.
Also, bind_device_to_connection() was fundamentally wrong. It's wrong
to modify the settings connection at random moments (on udev event).
If at all, that should only happen during connection load/reload.
This may have been necessary a long time ago, when unmanaged devices were
not expressed by device-match-specs, but by the HAL UDI. That was since
improved, for example by commit c9067d8fed.
The "connections" hash contains a mapping of block->name (iface) to the
NMSettingsConnection. The "eni_ifaces" hash contains the name of all
blocks which are mentioned, but for which no connection was created.
Merge the two hashes. We don't need to keep track of whether a
connections was successfully created ("connections"), but the
same name also has a non-connection block. This information is
unnecessary, so one hash is enough.
@unmanage_well_known directly depends on the "ifupdown.managed" setting from
NetworkManager.conf. Rename it (and invert the meaning) so that this
relation ship becomes clearer.
Also, the double negation of "if (!unmanaged_well_known)" hurts the
brain.
This is only useful for testing. But since the managed flag is configurable
via NetworkManager.conf, there is no point in having a define as well. If
you want to test it, just configure it.
And if you really want to patch the code, then patch
"priv->unmanage_well_known" to be always TRUE.
- use _NMLOG() macro and give logging message a sensible prefix
- downgrade logging severity. Most of these messages are not
important to warrant <info> or <warn> level.
- the logging is generally rather bad. Messages like
"bind-to-connection: locking wired connection setting"
don't indicate which profile is locked to which MAC address.
TODO.
- use gs_unref_hashtable for managing lifetime
- only allocate the hashtable if necessary, and use g_hash_table_add()
which is optimized by HashTable.
- actually copy the block->name that is used as key. While not
necessary at the moment, it is very ugly how ifparser_getfirst()
returns static data. Optimally, this would be fixed and we create
and destroy the parser results. Hence, ensure the lifetime of
the key.
- initialize the hash tables in the plugins constructor, not during
initialize().
- let all dictionaries own a copy/reference of the keys and values, and
properly free them when the values are removed. In general, avoid
leaks by properly managing lifetimes.
- in @eni_ifaces, don't add a pointless dummy value "known". It has
overhead for no benefit.
NMSettingsPlugin was a glib interface, not a regular GObject
instance. Accordingly, settings plugins would implement this interface
instead of subclassing a parent type.
Refactor the code, and make NMSettingsPlugin a GObject type. Plugins
are now required to subclass this type.
Glib interfaces are more cumbersome than helpful. At least, unless
there is a good reason for using them.
Our settings plugins are all internal API and are entirely under
our control. It also means, this change is fine, as there are no
implementations outside of this source tree.
Using interfaces do would allow more flexibility in implementing the
settings plugin.
For example, the plugin would be able to derive from any other GObject
type, like NMKimchiRefrigerator. But why would we even? Let's not add monster
classes that implement house appliances beside NMSettingsPluginInterface.
The settings plugin should have one purpose only: being a settings plugin.
Hence, requiring it to subclass NMSettingsPlugin is more than resonable. We
don't need interfaces for this.
Now that NMSettingsPlugin is a regular object instance, it may also have
state, and potentially could provide common functionality for the plugin
implementation -- if that turns out to be useful. Arguably, an interface can
have state too, for example by attaching the state somewhere else (like
NMConnection does). But let's just say no.
On a minor note, this also avoids some tiny overhead that comes with
glib interfaces.
The virtual function init() naturally leads to calling the wrapper
function nm_settings_plugin_init(). However, such ${TYPE}_init() functions
are generated by G_DEFINE_TYPE().
Rename to avoid the naming conflict, which will matter next, when the
interface will be converted to a regular GObject class.
Note that while these are settings plugin, there is no public
or stable API which we need to preserve.
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.
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.
