Note that the name "NMSecretAgentOld" comes from when libnm was forked
from libnm-glib. There was a plan to rework the secret agent API and
replace it by a better one. That didn't happen (yet), instead our one
and only agent implementation is still lacking. Don't add a new API, instead
try to improve the existing one, without breaking existing users. Just
get over the fact that the name "NMSecretAgentOld" is ugly.
Also note how nm-applet uses NMSecretAgentOld. It subtypes a class
AppletAgent. The constructor applet_agent_new() is calling the synchronous
g_initable_init() initialization with auto-register enabled. As it was,
g_initable_init() would call nm_secret_agent_old_register(), and if the
"Register" call failed, initialization failed for good. There are even
unit tests that test this behavior. This is bad behavior. It means, when
you start nm-applet without NetworkManager running, it will fail to create
the AppletAgent instance. It would hence be the responsibility of the applet
to recover from this situation (e.g. by retrying after timeout or watching
the D-Bus name owner). Of course, nm-applet doesn't do that and won't recover
from such a failure.
NMSecretAgentOld must try hard not to fail and recover automatically. The
user of the API is not interested in implementing the registration,
unregistration and retry handling. Instead, it should just work best
effort and transparently to the user of the API.
Differences:
- no longer use gdbus-codegen generate bindings. Use GDBusConnection
directly instead. These generated proxies complicate the code by
introducing an additional, stateful layer.
- properly handle GMainContext and synchronous initialization by using an
internal GMainContext.
With this NMSecretAgentOld can be used in a multi threaded context
with separate GMainContext. This does not mean that the object
itself became thread safe, but that the GMainContext gives the means
to coordinate multi-threaded access.
- there are no more blocking calls except g_initiable_init() which
iterates an internal GMainContext until initialization completes.
- obtaining the Unix user ID with "GetConnectionUnixUser" to authenticate
the server is now done asynchronously and only once per name-owner.
- NMSecretAgentOld will now register/export the Agent D-Bus object
already during initialization and stay registered as long as the
instance is alive. This is because usually registering a D-Bus
object would not fail, unless the D-Bus path is already taken.
Such an error would mean that another agent is registered for the same
GDBusConnection, that likely would be a bug in the caller. Hence,
such an issue is truly non-recoverable and should be reported early to
the user. There is a change in behavior compared to before, where previously
the D-Bus object would only be registered while the instance is enabled.
This makes a difference if the user intended to keep the NMSecretAgentOld
instance around in an unregistered state.
Note that nm_secret_agent_old_destroy() was added to really unregister
the D-Bus object. A destroyed instance can no longer be registered.
- the API no longer fully exposes the current registration state. The
user either enables or disables the agent. Then, in the background
NMSecretAgentOld will register, and serve requests as they come. It
will also always automatically re-register and it can de-facto no
longer fail. That is, there might be a failure to register, or the
NetworkManager peer might not be authenticated (non-root) or there
might be some other error, or NetworkManager might not be running.
But such errors are not exposed to the user. The instance is just not
able to provide the secrets in those cases, but it may recover if the
problem can be resolved.
- In particular, it makes no sense that nm_secret_agent_old_register*()
fails, returns an error, or waits until registration is complete. This
API is now only to enable/disable the agent. It is idempotent and
won't fail (there is a catch, see next point).
In particular, nm_secret_agent_old_unregister*() cannot fail anymore.
- However, with the previous point there is a problem/race. When you create
a NMSecretAgentOld instance and immediately afterwards activate a
profile, then you want to be sure that the registration is complete
first. Otherwise, NetworkManager might fail the activation because
no secret agent registered yet. A partial solution for this is
that g_initiable_init()/g_async_initable_init_async() will block
until registration is complete (or with or without success). That means,
if NetworkManager is running, initializing the NMSecretAgentOld will
wait until registration is complete (or failed). However, that does not
solve the race if NetworkManager was not running when creating the
instance.
To solve that race, the user may call nm_secret_agent_old_register_async()
and wait for the command to finish before starting activating. While
async registration no longer fails (in the sense of leaving the agent
permanently disconnected), it will try to ensure that we are
successfully registered and ready to serve requests. By using this
API correctly, a race can be avoided and the user can know that the
instance is now ready to serve request.
The NMSecretAgentOld is build very much around a GDBusConnection, and GDBusConnection
is build around GMainContext. That means, a NMSecretAgentOld instance is
strongly related to these two. That is because NMSecretAgentOld will register
to signals on D-Bus, using GDBusConnection. Hence, that GDBusConnection instance
and the calling GMainContext becomes central to the NMSecretAgentOld instance.
Also, the GMainContext is the way to synchronize access to the
NMSecretAgentOld. Used properly, this allows using the API in multi
threaded context.
Expose these two in the public API. Since NMSecretAgentOld is part of
libnm and supposed to provide a flexible API, this is just useful to
have.
Also, allow to provide a GDBusConnection as construct-only property. This way,
the instance can be used independent of g_bus_get() and the user has full control.
There is no setter for the GMainContext, because it just takes the
g_main_context_get_thread_default() instance at the time of
construction.
When NetworkManager starts, NMSecretAgentOld gets a name-owner changed
signal and registers right away.
Especially since commit ce0e898fb4 ('libnm: refactor caching of D-Bus
objects in NMClient') this hits a race where NetworkManager does not yet
export the org.freedesktop.NetworkManager.AgentManager interface and
the registration fails:
GDBus.Error:org.freedesktop.DBus.Error.UnknownMethod: No such interface “org.freedesktop.NetworkManager.AgentManager” on object at path /org/freedesktop/NetworkManager/AgentManager
Previously, when NMClient recevied a name-owner changed, that would
block the main loop long enough to avoid the race. Note that NMClient
has nothing to do with NMSecretAgentOld, however in practice all
applications that use NMSecretAgentOld also use NMClient.
While we should fix the race server-side, we also need to work around it
in the client. Retry.
Also, make the async request actually cancellable and actually honor the passed
GCancellable.
Check output:
$ LIBNM_CLIENT_DEBUG=trace ./clients/cli/nmcli agent secret |& grep secret-agent
libnm-dbus: <trace> [21399.04862] secret-agent[2f2af4ee102d7570]: create new instance
libnm-dbus: <trace> [21399.04863] secret-agent[2f2af4ee102d7570]: init-sync
libnm-dbus: <trace> [21404.08147] secret-agent[2f2af4ee102d7570]: name owner changed: (null)
libnm-dbus: <trace> [21404.09085] secret-agent[2f2af4ee102d7570]: name owner changed: ":1.2504"
libnm-dbus: <trace> [21404.09085] secret-agent[2f2af4ee102d7570]: register: starting asynchronous registration...
libnm-dbus: <trace> [21404.09178] secret-agent[2f2af4ee102d7570]: register: registration failed with error "GDBus.Error:org.freedesktop.DBus.Error.UnknownMethod: No such interface “org.freedesktop.NetworkManager.AgentManager” on object at path /org/freedesktop/NetworkManager/AgentManager". Retry in 0 msec...
libnm-dbus: <trace> [21404.09178] secret-agent[2f2af4ee102d7570]: register: retry registration...
libnm-dbus: <trace> [21404.09195] secret-agent[2f2af4ee102d7570]: register: registration failed with error "GDBus.Error:org.freedesktop.DBus.Error.UnknownMethod: No such interface “org.freedesktop.NetworkManager.AgentManager” on object at path /org/freedesktop/NetworkManager/AgentManager". Retry in 4 msec...
libnm-dbus: <trace> [21404.09236] secret-agent[2f2af4ee102d7570]: register: retry registration...
[...]
libnm-dbus: <trace> [21405.01782] secret-agent[2f2af4ee102d7570]: register: registration failed with error "GDBus.Error:org.freedesktop.DBus.Error.UnknownMethod: No such interface “org.freedesktop.NetworkManager.AgentManager” on object at path /org/freedesktop/NetworkManager/AgentManager". Retry in 128 msec...
libnm-dbus: <trace> [21405.03063] secret-agent[2f2af4ee102d7570]: register: retry registration...
libnm-dbus: <trace> [21405.03068] secret-agent[2f2af4ee102d7570]: register: registration failed with error "GDBus.Error:org.freedesktop.DBus.Error.UnknownMethod: No such interface “org.freedesktop.NetworkManager.AgentManager” on object at path /org/freedesktop/NetworkManager/AgentManager". Retry in 128 msec...
libnm-dbus: <trace> [21405.04354] secret-agent[2f2af4ee102d7570]: register: retry registration...
libnm-dbus: <trace> [21406.01097] secret-agent[2f2af4ee102d7570]: register: registration succeeded
RegisterWithCapabilities() is supported since NetworkManager 0.9.9.1. Of course,
we don't support such old server anymore (also, because we require the standard
D-Bus interfaces like ObjectManager).
We don't need a wrapper around g_bus_get*(). Just use
it directly.
I guess in the past this had some use when we were using
a private socket too. Those days are gone. If we are going
to re-introduce private socket support, then we probably should
come up with a better solution.
We no longer add these. If you use Emacs, configure it yourself.
Also, due to our "smart-tab" usage the editor anyway does a subpar
job handling our tabs. However, on the upside every user can choose
whatever tab-width he/she prefers. If "smart-tabs" are used properly
(like we do), every tab-width will work.
No manual changes, just ran commands:
F=($(git grep -l -e '-\*-'))
sed '1 { /\/\* *-\*- *[mM]ode.*\*\/$/d }' -i "${F[@]}"
sed '1,4 { /^\(#\|--\|dnl\) *-\*- [mM]ode/d }' -i "${F[@]}"
Check remaining lines with:
git grep -e '-\*-'
The ultimate purpose of this is to cleanup our files and eventually use
SPDX license identifiers. For that, first get rid of the boilerplate lines.
For static functions inside a module, the compiler determines on its own
whether to inline the function.
Also, "inline" was used at some places that don't immediatly look like
candidates for inlining. It was most likely a copy&paste error.
Always always when we want a linked list, CList is a better choice than
GSList. It's more convenient to use and is more efficient.
Also, use GSlice allocator for GetSecretRequest data.
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.
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.
If an operation is cancelled through the GCancellable, then the idiom is
that the operation is always cancelled, even if it has finished
successfully. To ensure this is the case, add calls to
g_simple_async_result_set_check_cancellable everywhere.
Without this, e.g. gnome-control-center will crash when switching away
from the power panel quickly, as the NMClient creation finishes
asynchronously and g-c-c assume that G_IO_ERROR_CANCELLED is returned to
ensure it doesn't access the now invalid user_data parameter.
https://bugzilla.gnome.org/show_bug.cgi?id=794088
From valgrind:
==21921== Invalid free() / delete / delete[] / realloc()
==21921== at 0x4C2CD5A: free (vg_replace_malloc.c:530)
==21921== by 0x81C4F2D: g_free (gmem.c:189)
==21921== by 0x81AB021: g_error_free (gerror.c:491)
==21921== by 0x81AB325: g_clear_error (gerror.c:674)
==21921== by 0x767B555: reg_request_cb (nm-secret-agent-old.c:616)
==21921== by 0x7A211F2: g_task_return_now (gtask.c:1107)
==21921== by 0x7A21228: complete_in_idle_cb (gtask.c:1121)
==21921== by 0x81BF6B9: g_main_dispatch (gmain.c:3154)
==21921== by 0x81BF6B9: g_main_context_dispatch (gmain.c:3769)
==21921== by 0x81BFA6F: g_main_context_iterate.isra.29 (gmain.c:3840)
==21921== by 0x81BFB1B: g_main_context_iteration (gmain.c:3901)
==21921== by 0x7A4748C: g_application_run (gapplication.c:2381)
==21921== by 0x118AEF: main (main.c:81)
It caused memory corruption and may result in strange nm-applet crashes.
This makes it easier to install the files with proper names.
Also, it makes the makefile rules slightly simpler.
Lastly, the documentation is now generated into docs/api, which makes it
possible to get rid of the awkward relative file names in docbook.
The name_owner_chagned() unregisters the agent if NetworkManager goes away and
nmc_cleanup() also tries to unregister an agent, resulting in an assertion
failure:
# nmcli c up conn666
<daemon terminates>
Error: Connection activation failed: Message recipient disconnected from message bus without replying
(process:8746): libnm-CRITICAL **: nm_secret_agent_old_unregister: assertion 'priv->registered == TRUE' failed
_internal_unregister() already contains a priv->registered check and raising an
error on duplicate unregister attempt from a daemon after a restart is not a
problem either, since nmc_cleanup() doesn't care about the error returned
on teardown anyway.
When we receive a connection from NetworkManager it is not guaranteed
that the connection verifies. For example, if the current libnm version
is older then the NetworkManager version.
Be more accepting and don't do any verification of the connection.
This is a change in behavior in that we accept also invalid connections
and pass them down to the sub-classes.
- All internal source files (except "examples", which are not internal)
should include "config.h" first. As also all internal source
files should include "nm-default.h", let "config.h" be included
by "nm-default.h" and include "nm-default.h" as first in every
source file.
We already wanted to include "nm-default.h" before other headers
because it might contains some fixes (like "nm-glib.h" compatibility)
that is required first.
- After including "nm-default.h", we optinally allow for including the
corresponding header file for the source file at hand. The idea
is to ensure that each header file is self contained.
- Don't include "config.h" or "nm-default.h" in any header file
(except "nm-sd-adapt.h"). Public headers anyway must not include
these headers, and internal headers are never included after
"nm-default.h", as of the first previous point.
- Include all internal headers with quotes instead of angle brackets.
In practice it doesn't matter, because in our public headers we must
include other headers with angle brackets. As we use our public
headers also to compile our interal source files, effectively the
result must be the same. Still do it for consistency.
- Except for <config.h> itself. Include it with angle brackets as suggested by
https://www.gnu.org/software/autoconf/manual/autoconf.html#Configuration-Headers
Rather than randomly including one or more of <glib.h>,
<glib-object.h>, and <gio/gio.h> everywhere (and forgetting to include
"nm-glib-compat.h" most of the time), rename nm-glib-compat.h to
nm-glib.h, include <gio/gio.h> from there, and then change all .c
files in NM to include "nm-glib.h" rather than including the glib
headers directly.
(Public headers files still have to include the real glib headers,
since nm-glib.h isn't installed...)
Also, remove glib includes from header files that are already
including a base object header file (which must itself already include
the glib headers).
The previous patch 9ffcecf86a was
completely wrong.
It tried to fix callers that provided a floating GVariant reference.
We require the caller to unref @secrets, so the correct fix it to
ensure that the reference is not floating.
Fixes: 9ffcecf86a
Fixes: 6793a32a8c
VPN connections always return true for nm_connection_need_secrets(), but the
documented behavior of GetSecrets() is just to return any secrets we have
(otherwise nmcli c --show-secrets would not be useful for VPN connections).
