I, Frédéric Danis, agree to relicense my contributions to NetworkManager
as LGPL-2.1+ as proposed by Thomas Haller.
Some of my work may be held under copyright by Sigfox or Collabora Ltd.
I do not speak for those entities.
Signed-off-by: Frédéric Danis <frederic.danis.oss@gmail.com>
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/462
Hide the object and class structures from public API.
This is an API and ABI break, but of something that is very likely
unused.
This is mainly done to embed the private structure in the object itself.
This has benefits for performance and debugability.
This is obviously a change in behavior, as we now honor backslash
escape sequences. With this change, all string values can be expressed,
both as option keys and values.
Previously, you could for example not set vpn.secrets to have a ','
and you could not set vpn.data to
nmcli connection modify "$PROFILE" +vpn.data 'ipsec-ike = aes256-sha2_256-modp2048,aes256-sha2_256-modp1536'
Use a relatively simple backslash escaping scheme. The main goal of
the scheme is that it doesn't change behavior for the majority of cases.
It only changes behavior for setting an option if:
- the string contains a backslash
- and if the backslash proceeds one of the few characters that support
escaping now (white space, ',', '\\', and '=').
The only downside here is that backslash is only treated special, if it
preceeds a character that requires escaping. That makes the behavior
non intuitive. However, it allows to write most backslashes without
escaping them as "\\\\" and thus keep previous behavior.
The nmcli getters now also escape the options accordingly. That means,
the string printed by the getter is also a valid input for the setter.
https://gitlab.freedesktop.org/NetworkManager/NetworkManager/issues/390
The two modes VALUE_STRSPLIT_MODE_OBJLIST and VALUE_STRSPLIT_MODE_MULTILIST
basically do regular split and afterwards g_strstrip() all values and
remove empty tokens.
That is what the NM_UTILS_STRSPLIT_SET_FLAGS_STRSTRIP flag already does.
Use it.
There should be no change in behavior.
Add flags to explicitly escape leading or trailing spaces. Note
that we were already escaping trailing spaces.
This will be used later when supporting backslash escapes for
option parameters for nmcli (vpn.data).
We should generate the GVariant in a stable manner. That implies
to sort the keys first.
Also, don't use the NM_SETTING_VPN_SECRETS getter, which first needs
to clone all secrets.
When we use _nm_utils_strdict_from_dbus(), we clone the secrets, but don't use
nm_free_secret() for freeing them.
And in fact, we clone the setings twice. It't really not too nice. Implement
this without the property setter.
Don't use _nm_utils_copy_strdict().
On a minor note, that function will always allocate a GHashTable, even if
it only contains "" as only key. Later we would throw that out again,
so it was unnecessary.
Worse, using _nm_utils_copy_strdict() does not use nm_free_secrets as
destroy function. While it's in general difficult to clear all places
in memory where we copy the secrets around, we can easily avoid that.
Also skip over %NULL keys and values. It probably would be a bug passing
such arguments to the property. Better ignore them and not crash.
Like for data, now also allow empty secrets to be added to the VPN
setting.
For one, this avoids an assertion failure, where keyfile reader wouldn't
check whether a secret key is set to the empty word.
For data, it's more clear that we want to allow setting empty data
values. VPN settings are only interpreted by the VPN plugin, so libnm
and the daemon shouldn't prevent empty settings. It can be useful to
distinguish between unset (NULL) and empty values.
For secrets, it's less clear that empty secrets shall be allowed. I
think it should. Of course, the empty secret likely isn't a correct
nor valid secret. But libnm cannot validate the secrets anyway. It's
up to the VPN plugin to handle this in any way they see fit.
Also, already before, the user could set NM_SETTING_VPN_SECRETS to
a string dictionary with empty passwords. So, the API didn't fully
prevent that. Only certain API wouldn't play along.
Until now, nm_setting_vpn_add_data_item() would reject empty data values.
This leads for example to an assertion failure, if you write a keyfile
that assigns an empty value to a key. Keyfile reader would not check that
the value is non-empty before calling nm_setting_vpn_add_data_item().
Anyway, I think we should not require having non-empty data elements. It's
an unnecessary and sometimes harmful restriction. NetworkManager doesn't understand
not care about the content of the vpn data. That is up the VPN plugins. Sometimes
and empty value may be desirable.
Also, the NM_SETTING_VPN_DATA property setter wouldn't filter out empty
values either. So it was always possible to use some libnm API to set data
with empty values. The restriction in nm_setting_vpn_add_data_item() was
inconsistent.
Also drop the g_warn_if_fail() from update_secret_dict(). We
may get the variant from D-Bus, so avoiding this assertion (g_warn*() is
an assertion!) would require us to prevalidate the variant.
That would be very cumbersome, and we would probably not want to
handle that as an error and silently ignore them anyway. Just shut
up.
Ensure that the data hash doesn't contain keys with empty key-name.
It just doesn't make sense, and will lead to potential problems later,
if we would allow this to happen.
For example, keyfile writer may naively try to set all keys, without
checking for empty keys. That may lead to assertion failures or worse,
later on. Don't allow that.
Also, assert against non-empty keys in nm_setting_vpn_get_data_item()
and nm_setting_vpn_remove_data_item(). This stricter handling is a
change in behavior, however it would have always been a bug trying to
refer to empty key names. So, this assertion will only help to find
those bugs.
We always initialized priv->data in nm_setting_vpn_init(), but usually
soon after this hash would be replaced by NM_SETTING_VPN_DATA property
setter.
Avoid that. Allow for priv->data to be %NULL, which of course has the
meaning that no keys are set.
When we invoke the user's callback, be more robust about the user
modifying or unreferencing the NMSettingVpn, while iterating.
While it would be odd to modify the NMSettingVpn from inside the foreach
callback, we should behave consistently and sensibly.
That means, to ensure that the NMSettingVpn instance stays alive all
the time, that we don't crash, and that we always iterate over the
previously determined, planned list of keys.
Also, avoid some unnecessary string copies, like the clone of the first key.
For one, this code was unreachable, because we checked these conditions
already before.
That aside, g_warn_*() is for all intended purposes an assertion.
The caller probably gets the GVariant from an untrusted source (e.g. via
D-Bus). Asserting here is not helpful.
It's up to the caller to validate the argument. Or, in case the caller
doesn't care, update_secret_dict() should just do the sensible thing.
But be silent about it!
Yes, the compiler probably can optimize strlen() in these cases. That
still doesn't make strlen() the right API to check whether a NUL
terminated string is empty.
We should not use global variables, and we should minimize the state
that we pass around. Instead of requiring the full NmCli struct in
nm_cli_spawn_pager(), pass only the necessary data.
This reduces our use of global variables.
Of course, we later pass the point on, where we need to cast the constness away
again. This is more a reminder that we aren't suppost to change the variable.
We should try to avoid access to global variables. For libreadline
callbacks we still need a global variable.
Introduce a global variable nm_cli_global_readline, specially for this
use. It makes the places clear where we use it, and discourages
the use at other places, where we better avoid global variables.
In the next commit, GString will be replaced by NMStrBuf. Then, we will
pre-allocate a string buffer with 16 bytes, and measure the performance
difference. To have it comparable, adjust the pre-allocation size also
with GString.
nm_utils_buf_utf8safe_unescape() is almost the same as g_strcompress(),
with the only difference is that if the string contains NUL escapes "\000",
it will be handled correctly.
In other words, g_strcompress() and nm_utils_str_utf8safe_unescape() can only
unescape values, that contain no NUL escapes. That's why we added our
own binary unescape function.
As we already have our g_strcompress() variant, use it. It just gives it more
testing and usage. Also, we have full control over it's behavior. For example,
g_strcompress() issues a g_warning() when encountering a trailing '\\'. I
think this makes it unsuitable to unescape untrusted data. Either the function
should fail, or just make the best of it. Currently, our implementation
does the latter.
