@secrets is unreferenced at the end of request_secrets_from_ui() and
so try_spawn_vpn_auth_helper() must take a reference to it.
Fixes: 1a0fc8d437
(cherry picked from commit b57a3a4cc6)
For now only add the core settings, no peers' data.
To support peers and the allowed-ips of the peers is more complicated
and will be done later. It's more complicated because these are nested
lists (allowed-ips) inside a list (peers). That is quite unusual and to
conveniently support that in D-Bus API, in keyfile format, in libnm,
and nmcli, is a effort.
Also, it's further complicated by the fact that each peer has a secret (the
preshared-key). Thus we probably need secret flags for each peer, which
is a novelty as well (until now we require a fixed set of secrets per
profile that is well known).
It's not really used, but we shouldn't just forget about it.
Currently, we fill requests only based on the connection-type, ignoring
the setting-name. I guess, the concept of requesting secrets for a setting
is utterly broken. But equally broken it is to just look at the connection
(type). At least, don't just throw parts of the request away but keep
it.
The callback must be invoked, as also documented.
Otherwise, the tracked info gets leaked.
Let NMSecretAgentOld (the caller) be a bit resilient against
bugs in the client, and avoid a crash by prematurely remove
the request-info from the pending list. That does not fully
workaround the bug (it leads to a leak), but at least it does
not cause other "severe" issues.
The leak was present earlier as well.
NMSecretAgentOld's get_secrets_cb() gets this right and takes
a floating reference. So this was correct.
However, make this a bit more robust, and don't pass on
floating references. This was, we don't require the callee
to consume the reference.
Most of the times we actually need a NMSecretAgentSimple typed pointer.
This way, need need to cast less.
But even if we would need to cast more, it's better to have pointers
point to the actual type, not merely to avoid shortcomings of C.
No caller cared about the NM_SECRET_AGENT_ERROR_AGENT_CANCELED reason.
In particular, because previously the requests would keep the secret-agent
instance alive, and this never happend.
Also, NM_SECRET_AGENT_ERROR_AGENT_CANCELED precicley exists for
NMSecretAgentOld:cancel_get_secrets() (as documented). During finalize
we are not cancelled -- at least not the same way as
cancel_get_secrets(). Setting NM_SECRET_AGENT_ERROR_AGENT_CANCELED
is wrong.
Anyway, we have a default error for such cases already.
The code was correct. But it's hard to follow when and whether
the child-watch-id was destroyed at the right time.
Instead, always let _auth_dialog_data_free() clear the signal handlers.
Don't let RequestData keep the parent NMSecretAgentSimple instance
alive. Previously, the code in finalize() was never actually reached.
Also, move the final callback from finalize() to dispose(). It feels
wrong to invoke callbacks from finalize().
We must actually cancel the GCancellable. Otherwise, the pending async
operations are not cancelled. _auth_dialog_write_done() doesn't care
about that, but _auth_dialog_read_done() does. It must not touch the
destroyed data, after the operation is cancelled.
Note that previously the @requests hash took the request-id as key and
the RequestData as value. Likewise, the destroy functions of the head
would destroy the key and the value.
However, RequestData also had a field "request_id". But that pointer was
not owned (nor freed) by the RequestData structure. Instead, it was
relied that the hash kept the request-id alive long enough.
That is confusing. Let RequestData own the request-id.
Also, we don't need to track a separate key. Just move the request-id
as first filed in RequestData, and use compare/hash functions that
handle that correctly (nm_pstr_*()).
Code that is internal to a source file should not have a "nm" prefix.
That is what differenciates it from declarations in header files. It
makes it clearer that these names are only defined in the current file.
Also, our implementations of virtual functions shall have the same
name as the function pointer of the VTable (or at least, it shouldn't
have a "nm" prefix).
This makes it possible to utilize agents in the "external UI" mode
instead of hardcoded handling of VPN secrets requests.
Ideally this would be turned into a library so that nm-applet can share
the code, but figuring out the right API might be a non-trivial
undertaking.
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[@]}"
If the hints parameter to the agent request wasn't empty, ask
specifically for the 802-1x keys listed in the hints and skip the
guessing. I didn't add human readable names for all of the 802-1x
settings, it could be useful to do for at least the three 802-1x
properties that add_8021x_secrets already knows about because
those may have translations.
(cherry picked from commit 1a6e53808d)
Next we will use siphash24() instead of the glib version g_direct_hash() or
g_str_hash(). Hence, the "nm-utils/nm-hash-utils.h" header becomes very
fundamental and will be needed basically everywhere.
Instead of requiring the users to include them, let it be included via
"nm-default.h" header.
Replace the usage of g_str_hash() with our own nm_str_hash().
GLib's g_str_hash() uses djb2 hashing function, just like we
do at the moment. The only difference is, that we use a diffrent
seed value.
Note, that we initialize the hash seed with random data (by calling
getrandom() or reading /dev/urandom). That is a change compared to
before.
This change of the hashing function and accessing the random pool
might be undesired for libnm/libnm-core. Hence, the change is not
done there as it possibly changes behavior for public API. Maybe
we should do that later though.
At this point, there isn't much of a change. This patch becomes
interesting, if we decide to use a different hashing algorithm.
Branch f9b1bc16e9 added bluetooth NAP
support. A NAP connection is of connection.type "bluetooth", but it
also has a "bridge" setting. Also, it is primarily handled by NMDeviceBridge
and NMBridgeDeviceFactory (with help from NMBluezManager).
However, don't let nm_connection_get_connection_type() and
nm_connnection_is_type() lie about what the connection.type is.
The type is "bluetooth" for most purposes -- at least, as far as
the client is concerned (and the public API of libnm). This restores
previous API behavior, where nm_connection_get_connection_type()
and nm_connection_is_type() would be simple accessors to the
"connection.type" property.
Only a few places care about the bridge aspect, and those places need special
treatment. For example NMDeviceBridge needs to be fully aware that it can
handle bluetooth NAP connection. That is nothing new: if you handle a
connection of any type, you must know which fields matter and what they
mean. It's not enough that nm_connection_get_connection_type() for bluetooth
NAP connectins is claiming to be a bridge.
Counter examples, where the original behavior is right:
src/nm-manager.c- g_set_error (error,
src/nm-manager.c- NM_MANAGER_ERROR,
src/nm-manager.c- NM_MANAGER_ERROR_FAILED,
src/nm-manager.c- "NetworkManager plugin for '%s' unavailable",
src/nm-manager.c: nm_connection_get_connection_type (connection));
the correct message is: "no bluetooth plugin available", not "bridge".
src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c: if ( ( nm_connection_is_type (connection, NM_SETTING_WIRED_SETTING_NAME)
src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c: && !nm_connection_get_setting_pppoe (connection))
src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c: || nm_connection_is_type (connection, NM_SETTING_VLAN_SETTING_NAME)
src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c: || nm_connection_is_type (connection, NM_SETTING_WIRELESS_SETTING_NAME)
src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c: || nm_connection_is_type (connection, NM_SETTING_INFINIBAND_SETTING_NAME)
src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c: || nm_connection_is_type (connection, NM_SETTING_BOND_SETTING_NAME)
src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c: || nm_connection_is_type (connection, NM_SETTING_TEAM_SETTING_NAME)
src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c: || nm_connection_is_type (connection, NM_SETTING_BRIDGE_SETTING_NAME))
src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c- return TRUE;
the correct behavior is for ifcfg-rh plugin to reject bluetooth NAP
connections, not proceed and store it.
Since we use g_str_has_prefix() to match a request_id with the
connection path, there can be wrong matches. For example:
request_id: /org/freedesktop/NetworkManager/Settings/10/802-1x
connection: /org/freedesktop/NetworkManager/Settings/1
would match. Add a trailing slash to the connection path stored in the
agent to prevent this.
