A NetworkManager client requires an API to validate and decode
a base64 secret -- like it is used by WireGuard. If we don't have
this as part of the API, it's inconvenient. Expose it.
Rename it from _nm_utils_wireguard_decode_key(), to give it a more
general name.
Also, rename _nm_utils_wireguard_normalize_key() to
nm_utils_base64secret_normalize(). But this one we keep as internal
API. The user will care more about validating and decoding the base64
key. To convert the key back to base64, we don't need a public API in
libnm.
This is another ABI change since 1.16-rc1.
(cherry picked from commit e46ba01867)
This is an API break since 1.16-rc1.
The functions like _nm_utils_wireguard_decode_key() are internal API
and not accessible to a libnm user. Maybe this should be public API,
but for now it is not.
That makes it cumbersome for a client to validate the setting. The client
could only reimplement the validation (bad) or go ahead and set invalid
value.
When setting an invalid value, the user can afterwards detect it via
nm_wireguard_peer_is_valid(), but at that point, it's not clear which
exact property is invalid.
First I wanted to keep the API conservative and not promissing too much.
For example, not promising to do any validation when setting the key.
However, libnm indeed validates the key at the time of setting it
instead of doing lazy validation later. This makes sense, so we can
keep this promise and just expose the validation result to the caller.
Another downside of this is that the API just got more complicated.
But it not provides a validation API, that we previously did not have.
(cherry picked from commit d7bc1750c1)
- nm_setting_enumerate_values() cannot handle non-GObject based
properties as it cannot abstract them. That means, for gendata
based settings, we need already a special case, and future ways
of handling settings (WireGuard peers) also won't work without
special casing.
- nm_setting_enumerate_values() needs to fetch all properties, although
some properties will not be actually used. E.g. secret properties will
be ignored depending on the flag.
Or compare the read-side with read_one_setting_value(). The reader doesn't
care about the (unset) GObject property. It really just cares about the
GType of the proeprty. Still, nm_setting_enumerate_values() fetches all
(empty) properties.
Or consider, route_writer() as called by nm_setting_enumerate_values()
always needs to deep-clone the entire list of routes in the property
getter for NM_SETTING_IP_CONFIG_ROUTES, then unpack it. This is
unnecesary overhead. This is not yet fixed, but would now be easy to
improve.
- a for-each function like nm_setting_enumerate_values() does make code
harder to read, gives less possibility for optimization, and is in
general less flexible. Don't use it.
There are 3 kinds of secret flag implementations:
1) regular properties have a GObject property and a corresponding
"-flags" property.
2) NMSettingVpn handles this entirely differently
3) NMSettingWirelessSecurity's WEP keys, where the secret keys
share a flags property that does not follow the same naming
scheme as 1).
The getter and setter had a boolean "verifiy_secret", only to
handle 3). Drop that parameter. Don't let NMSettingWirelessSecurity
call the parent's implementation for WEP keys. Just let it handle
it directly.
- previously, writer would use nm_keyfile_plugin_kf_set_integer() for
G_TYPE_UINT types.
That means, values larger than G_MAXINT would be stored as negative
values. On the other hand, the reader would always reject negative
values.
Fix that, by parsing the integer ourself.
Note that we still reject the old (negative) values and there is no
compatibility for accepting such values. They were not accepted by
reader in the past and so they are still rejected.
This affects for example ethernet.mtu setting (arguably, the MTU
is usually set to small values where the issue was not apparent).
This is also covered by a test.
- no longer use nm_keyfile_plugin_kf_set_integer().
nm_keyfile_plugin_kf_set_integer() calls g_key_file_get_integer(), which
uses g_key_file_parse_integer_as_value(). That one has the odd
behavior of accepting "<number><whitespace><bogus>" as valid. Note how that
differs from g_key_file_parse_value_as_double() which rejects trailing data.
Implement the parsing ourself. There are some changes here:
- g_key_file_parse_value_as_integer() uses strtol() with base 10.
We no longer require a certain the base, so '0x' hex values are allowed
now as well.
- bogus suffixes are now rejected but were accepted by g_key_file_parse_value_as_integer().
We however still accept leading and trailing whitespace, as before.
- use nm_g_object_set_property*(). g_object_set() asserts that the value
is in range. We cannot pass invalid values without checking that they
are valid.
- emit warnings when values cannot be parsed. Previously they would
have been silently ignored or fail an assertion during g_object_set().
- don't use "helpers" like nm_keyfile_plugin_kf_set_uint64(). These
merely call GKeyFile's setters (taking care of aliases). The setters
of GKeyFile don't do anything miraculously, they merely call
g_key_file_set_value() with the string that one would expect.
Convert the numbers/boolean ourselfs. For one, we don't require
a heap allocation to convert a number to string. Also, there is
no point in leaving this GKeyFile API, because even if GKeyFile
day would change, we still must continue to support the present
format, as that is what users have on disk. So, even if a new
way would be implemented by GKeyFile, the current way must forever
be accepted too. Hence, we don't need this abstraction.
- in nm_keyfile_read(), unify _read_setting() and
_read_setting_vpn_secret() in they way they are called
(that is, they no longer return any value and don't accept
any arguments aside @info).
- use cleanup attributes
- use nm_streq() instead of strcmp().
- wrap lines that have multiple statements or conditions.
Several callers access the length output argument, expecting
it to be zero also on failure. That is a bug, because glib does
not guarantee that.
Fix that by making a stronger promise from our wrappers: the output
argument should also be set on failure.
Also ensure that calls to g_keyfile_get_groups() and
g_keyfile_get_keys() don't rely on the length output to be valid,
when the function call fails.
Actually, these issues were not severe because:
- `g_key_file_get_*_list()`'s implementation always sets the output length
even on failure (undocumented).
- `g_key_file_get_groups()` cannot fail and always set the length.
- `g_key_file_get_keys()` is called under circumstances where it won't
fail.
Still, be explicit about it.
Report an error when the user tries to add an unknown attribute
instead of silently accepting (and ignoring) it.
Note that this commit also changes the behavior of public API
nm_utils_sriov_vf_from_str() to return an error when an unknown
attribute is found. I think the previous behavior was buggy as wrong
attributes were simply ignored without any way for the user to know.
Fixes: a9b4532fa7
NM_CONFIG_KEYFILE_PATH_IN_MEMORY is now called NMS_KEYFILE_PATH_NAME_RUN.
This name seems odd in the current context, it will be more suitable
when we also have NMS_KEYFILE_PATH_NAME_LIB (for /usr/lib).
These utilities are concerned with valid file names (as NetworkManager
daemon requires it). This is relevant for everybody who wants to write
keyfile files directly. Hence, move it to libnm-core. Still as internal
API.
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.
Split out the functionality for auto-detecting the ID and UUID of
a connection. First of all, nm_keyfile_read() is already overcomplicated.
The next commit will require the caller to explicitly call these
functions.
Of course, there are countless places where we get it wrong to clear
the memory. In particular, glib's GKeyFile implementation does
not care about that.
Anyway, the keyfile do contain private sensitive data. Adjust
a few places to zero out such data from memory.
Add a new way how NMSetting subclasses can be implemented.
Currently, most NMSetting implementations realize all their properties
via GObject properties. That has some downsides:
- the biggest one, is the large effort to add new properties.
Most of them are implemented on a one-by-one basis and they come
with additional API (like native getter functions).
It makes it cumbersome to add more properties.
- for certain properties, it's hard to encode them entirely in
a GObject property. That results in unusable API like
NM_SETTING_IP_CONFIG_ADDRESSES, NM_SETTING_BOND_OPTIONS,
NM_SETTING_USER_DATA. These complex valued properties only
exist, because we currently always need GObject properties
to even implement simple functionality. For example,
nm_setting_duplicate() is entirely implemented via
nm_setting_enumerate_values(), which can only iterate
GObject properies. There is no reason why this is necessary.
Note also how nmcli badly handles bond options and VPN
data. That is only a shortcoming of nmcli and wouldn't
need to be that way. But it happend, because we didn't
keep an open mind that settings might be more than just
accessing GObject properties.
- a major point of NMSetting is to convert to/from a GVariant
from the D-Bus API. As NMSetting needs to squeeze all values
into the static GObject structure, there is no place to
encode invalid or unknown properties. Optimally,
_nm_setting_new_from_dbus() does not loose any information
and a subsequent _nm_setting_to_dbus() can restore the original
variant. That is interesting, because we want that an older
libnm client can talk to a newer NetworkManager version. The
client needs to handle unknown properties gracefully to stay
forward compatible. However, it also should not just drop the
properties on the floor.
Note however, optimally we want that nm_setting_verify() still
can reject settings that have such unknown/invalid values. So,
it should be possible to create an NMSetting instance without
error or loosing information. But verify() should be usable to
identify such settings as invalid.
They also have a few upsides.
- libnm is heavily oriented around GObject. So, we generate
our nm-settings manual based on the gtk-doc. Note however,
how we fail to generate a useful manual for bond.options.
Also note, that there is no reason we couldn't generate
great documentation, even if the properties are not GObject
properties.
- GObject properties do give some functionality like meta-data,
data binding and notification. However, the meta-data is not
sufficient on its own. Note how keyfile and nmcli need extensive
descriptor tables on top of GObject properties, to make this
useful. Note how GObject notifications for NMSetting instances
are usually not useful, aside for data binding like nmtui does.
Also note how NMSettingBond already follows a different paradigm
than using GObject properties. Nowdays, NMSettingBond is considered
a mistake (related bug rh#1032808). Many ideas of NMSettingBond
are flawed, like exposing an inferiour API that reduces everything
to a string hash. Also, it only implemented the options hash inside
NMSettingBond. That means, if we would consider this a good style,
we would have to duplicate this approach in each new setting
implementation.
Add a new style to track data for NMSetting subclasses. It keeps
an internal hash table with all GVariant properies. Also, the
functionality is hooked into NMSetting base class, so all future
subclasses that follow this way, can benefit from this. This approach
has a few similiarties with NMSettingBond, but avoids its flaws.
With this, we also no longer need GObject properties (if we would
also implement generating useful documentation based on non-gkt-doc).
They may be added as accessors if they are useful, but there is no
need for them.
Also, handling the properties as a hash of variants invites for a
more generic approach when handling them. While we still could add
accessors that operate on a one-by-one bases, this leads to a more
generic usage where we apply common functionality to a set of properties.
Also, this is for the moment entirely internal and an implementation
detail. It's entirely up to the NMSetting subclass to make use of this
new style. Also, there are little hooks for the subclass available.
If they turn out to be necessary, they might be added. However, for
the moment, the functionality is restricted to what is useful and
necessary.
We have NMMetaSettingType enum, which is an enum of all setting types.
We also have an efficient way to get the enum (and its NMMetaSettingInfo)
from an NMSetting, setting-name and GType.
No longer maintain the vtable for keyfile by "setting-name". Instead,
index it by NMMetaSettingType enum.
That way, we get efficient lookup, and don't need to duplicate the
functionality of finding the vtable entry for a setting.
We commonly don't use the glib typedefs for char/short/int/long,
but their C types directly.
$ git grep '\<g\(char\|short\|int\|long\|float\|double\)\>' | wc -l
587
$ git grep '\<\(char\|short\|int\|long\|float\|double\)\>' | wc -l
21114
One could argue that using the glib typedefs is preferable in
public API (of our glib based libnm library) or where it clearly
is related to glib, like during
g_object_set (obj, PROPERTY, (gint) value, NULL);
However, that argument does not seem strong, because in practice we don't
follow that argument today, and seldomly use the glib typedefs.
Also, the style guide for this would be hard to formalize, because
"using them where clearly related to a glib" is a very loose suggestion.
Also note that glib typedefs will always just be typedefs of the
underlying C types. There is no danger of glib changing the meaning
of these typedefs (because that would be a major API break of glib).
A simple style guide is instead: don't use these typedefs.
No manual actions, I only ran the bash script:
FILES=($(git ls-files '*.[hc]'))
sed -i \
-e 's/\<g\(char\|short\|int\|long\|float\|double\)\>\( [^ ]\)/\1\2/g' \
-e 's/\<g\(char\|short\|int\|long\|float\|double\)\> /\1 /g' \
-e 's/\<g\(char\|short\|int\|long\|float\|double\)\>/\1/g' \
"${FILES[@]}"
Coccinelle:
@@
expression a, b;
@@
-a ? a : b
+a ?: b
Applied with:
spatch --sp-file ternary.cocci --in-place --smpl-spacing --dir .
With some manual adjustments on spots that Cocci didn't catch for
reasons unknown.
Thanks to the marvelous effort of the GNU compiler developer we can now
spare a couple of bits that could be used for more important things,
like this commit message. Standards commitees yet have to catch up.
Do not have multiple ways of expressing a certain thing. There is
a way how to express that the parser shouldn't check for keys, and
that is via the parse-information. No extra hacks.
Rework this to have a value "parser_no_check_key" so that:
- the default value for this is FALSE, so that we don't need to
explicitly set it in @parse_infos to only get the default.
Contrary to check_for_key.
- check_for_key only had meaning when also "parser" was set.
That means, the value was really "pip->parser && pip->check_for_key".
That came from the fact, that orginally this was tracked as
key_parsers array, which had "parser" always set.
That is confusing, don't do that. The field "parser_no_check_key"
has it's meaning, regardless of whether "parser" is set.
Splitting keyfile handling in two "reader.c" and "writer.c" files
is not helpful. What is most interesting, is to see how property XYZ
is serialized to keyfile, and to verify that the parser does the
inverse. For that, it's easier if both the write_xzy() and parse_xyz()
function are beside each other, and not split accross files.
The more important reason is, that both reader and writer have their
separate handler arrays, for special handling of certain properties:
@key_parsers and @key_writers. These two should not be separate but will
be merged. Since they reference static functions, these functions must
all be in the same source file (unless, we put them into headers, which
would be unnecessary complex).
No code was changed, only moved.
