system/NetworkManager
system/NetworkManager
1
0
Fork 0
mirror of https://gitlab.freedesktop.org/NetworkManager/NetworkManager synced 2024-10-06 16:21:50 +00:00
Code Issues Packages Projects Releases Wiki Activity

libnm/secret-agent: rework NMSecretAgentOld

Browse source 
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.
This commit is contained in:
Thomas Haller 2019-12-24 13:26:50 +01:00
parent 2c30c1a04f
commit c1ec829099
5 changed files with 1393 additions and 745 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
libnm/libnm.ver
View file

@ -1667,7 +1667,11 @@ global:
nm_device_vrf_get_table;
nm_device_vrf_get_type;
nm_object_get_client;
nm_secret_agent_old_destroy;
nm_secret_agent_old_enable;
nm_secret_agent_old_get_context_busy_watcher;
nm_secret_agent_old_get_dbus_connection;
nm_secret_agent_old_get_dbus_name_owner;
nm_secret_agent_old_get_main_context;
nm_setting_vrf_get_table;
nm_setting_vrf_get_type;

1885
libnm/nm-secret-agent-old.c
View file

File diff suppressed because it is too large Load diff

53
libnm/nm-secret-agent-old.h
View file

@ -127,9 +127,12 @@ typedef struct {
/* Called when the subclass should retrieve and return secrets. Subclass
* must copy or reference any arguments it may require after returning from
* this method, as the arguments will freed (except for 'self', 'callback',
* and 'user_data' of course). If the request is canceled, the callback
* should still be called, but with the
* NM_SECRET_AGENT_OLD_ERROR_AGENT_CANCELED error.
* and 'user_data' of course).
*
* Before version 1.24, if the request is canceled, the callback
* should still be called, but with the NM_SECRET_AGENT_ERROR_AGENT_CANCELED
* error. Since 1.24, invoking the callback has no effect during cancellation
* and may be omitted.
*/
void (*get_secrets) (NMSecretAgentOld *self,
NMConnection *connection,
@ -141,10 +144,12 @@ typedef struct {
gpointer user_data);
/* Called when the subclass should cancel an outstanding request to
* get secrets for a given connection. Canceling the request MUST
* call the callback that was passed along with the initial get_secrets
* call, sending the NM_SECRET_AGENT_OLD_ERROR/
* NM_SECRET_AGENT_OLD_ERROR_AGENT_CANCELED error to that callback.
* get secrets for a given connection.
*
* Before version 1.24, canceling the request MUST call the callback that was
* passed along with the initial get_secrets call, sending the NM_SECRET_AGENT_ERROR/
* NM_SECRET_AGENT_ERROR_AGENT_CANCELED error to that callback. Since 1.24,
* the get_secrets callback will be ignored during cancellation and may be omitted.
*/
void (*cancel_get_secrets) (NMSecretAgentOld *self,
const char *connection_path,
@ -186,9 +191,20 @@ GDBusConnection *nm_secret_agent_old_get_dbus_connection (NMSecretAgentOld *self
NM_AVAILABLE_IN_1_24
GMainContext *nm_secret_agent_old_get_main_context (NMSecretAgentOld *self);
gboolean nm_secret_agent_old_register (NMSecretAgentOld *self,
GCancellable *cancellable,
GError **error);
NM_AVAILABLE_IN_1_24
GObject *nm_secret_agent_old_get_context_busy_watcher (NMSecretAgentOld *self);
NM_AVAILABLE_IN_1_24
const char *nm_secret_agent_old_get_dbus_name_owner (NMSecretAgentOld *self);
gboolean nm_secret_agent_old_get_registered (NMSecretAgentOld *self);
/*****************************************************************************/
NM_AVAILABLE_IN_1_24
void nm_secret_agent_old_enable (NMSecretAgentOld *self,
gboolean enable);
void nm_secret_agent_old_register_async (NMSecretAgentOld *self,
GCancellable *cancellable,
GAsyncReadyCallback callback,
@ -197,18 +213,33 @@ gboolean nm_secret_agent_old_register_finish (NMSecretAgentOld *self,
GAsyncResult *result,
GError **error);
NM_AVAILABLE_IN_1_24
void nm_secret_agent_old_destroy (NMSecretAgentOld *self);
/*****************************************************************************/
NM_DEPRECATED_IN_1_24_FOR (nm_secret_agent_old_enable)
gboolean nm_secret_agent_old_register (NMSecretAgentOld *self,
GCancellable *cancellable,
GError **error);
NM_DEPRECATED_IN_1_24_FOR (nm_secret_agent_old_enable)
gboolean nm_secret_agent_old_unregister (NMSecretAgentOld *self,
GCancellable *cancellable,
GError **error);
NM_DEPRECATED_IN_1_24_FOR (nm_secret_agent_old_enable)
void nm_secret_agent_old_unregister_async (NMSecretAgentOld *self,
GCancellable *cancellable,
GAsyncReadyCallback callback,
gpointer user_data);
NM_DEPRECATED_IN_1_24_FOR (nm_secret_agent_old_enable)
gboolean nm_secret_agent_old_unregister_finish (NMSecretAgentOld *self,
GAsyncResult *result,
GError **error);
gboolean nm_secret_agent_old_get_registered (NMSecretAgentOld *self);
/*****************************************************************************/
void nm_secret_agent_old_get_secrets (NMSecretAgentOld *self,
NMConnection *connection,

195
libnm/tests/test-secret-agent.c
View file

@ -122,12 +122,12 @@ test_secret_agent_init (TestSecretAgent *agent)
}
static NMSecretAgentOld *
test_secret_agent_new (void)
test_secret_agent_new (gboolean auto_register)
{
return nmtstc_context_object_new (test_secret_agent_get_type (),
TRUE,
NM_SECRET_AGENT_OLD_IDENTIFIER, "test-secret-agent",
NM_SECRET_AGENT_OLD_AUTO_REGISTER, FALSE,
NM_SECRET_AGENT_OLD_AUTO_REGISTER, auto_register,
NULL);
}
@ -211,7 +211,7 @@ test_setup (TestSecretAgentData *sadata, gconstpointer test_data)
{
static int static_counter = 0;
const int counter = static_counter++;
const char *agent_notes = test_data;
const char *create_agent = test_data;
NMConnection *connection;
NMSettingConnection *s_con;
NMSettingWireless *s_wireless;
@ -273,18 +273,25 @@ test_setup (TestSecretAgentData *sadata, gconstpointer test_data)
g_main_loop_run (sadata->loop);
g_assert (sadata->connection);
if (agent_notes) {
sadata->agent = test_secret_agent_new ();
if (nm_streq (create_agent, "1")) {
gboolean auto_register = nmtst_get_rand_bool ();
if (!strcmp (agent_notes, "sync")) {
sadata->agent = test_secret_agent_new (auto_register);
if (auto_register) {
g_assert (nm_secret_agent_old_get_registered (sadata->agent));
nm_secret_agent_old_register (sadata->agent, NULL, &error);
g_assert_no_error (error);
g_assert (nm_secret_agent_old_get_registered (sadata->agent));
} else {
nm_secret_agent_old_register_async (sadata->agent, NULL,
register_cb, sadata);
g_assert (!nm_secret_agent_old_get_registered (sadata->agent));
nm_secret_agent_old_register_async (sadata->agent,
NULL,
register_cb,
sadata);
g_main_loop_run (sadata->loop);
}
g_assert (nm_secret_agent_old_get_registered (sadata->agent));
}
}
@ -306,6 +313,9 @@ test_cleanup (TestSecretAgentData *sadata, gconstpointer test_data)
nm_client_get_context_busy_watcher (sadata->client));
if (sadata->agent) {
nmtst_context_busy_watcher_add (&watcher_data,
nm_secret_agent_old_get_context_busy_watcher (sadata->agent));
if (nm_secret_agent_old_get_registered (sadata->agent)) {
nm_secret_agent_old_unregister (sadata->agent, NULL, &error);
g_assert_no_error (error);
@ -542,17 +552,18 @@ test_secret_agent_good (TestSecretAgentData *sadata, gconstpointer test_data)
g_assert_cmpint (sadata->secrets_requested, ==, 1);
}
/*****************************************************************************/
static void
async_init_cb (GObject *object, GAsyncResult *result, gpointer user_data)
{
GMainLoop *loop = user_data;
GError *error = NULL;
GObject *agent;
gs_free_error GError *error = NULL;
gs_unref_object GObject *agent = NULL;
agent = g_async_initable_new_finish (G_ASYNC_INITABLE (object), result, &error);
g_assert_error (error, NM_SECRET_AGENT_ERROR, NM_SECRET_AGENT_ERROR_FAILED);
g_assert (agent == NULL);
g_clear_error (&error);
nmtst_assert_success (NM_IS_SECRET_AGENT_OLD (agent), error);
g_assert (!nm_secret_agent_old_get_registered (NM_SECRET_AGENT_OLD (agent)));
g_main_loop_quit (loop);
}
@ -560,89 +571,151 @@ async_init_cb (GObject *object, GAsyncResult *result, gpointer user_data)
static void
test_secret_agent_nm_not_running (void)
{
NMSecretAgentOld *agent;
GMainLoop *loop;
gs_unref_object NMSecretAgentOld *agent = NULL;
nm_auto_unref_gmainloop GMainLoop *loop = NULL;
GError *error = NULL;
agent = g_initable_new (test_secret_agent_get_type (), NULL, &error,
agent = g_initable_new (test_secret_agent_get_type (),
NULL,
&error,
NM_SECRET_AGENT_OLD_IDENTIFIER, "test-secret-agent",
NULL);
g_assert_error (error, NM_SECRET_AGENT_ERROR, NM_SECRET_AGENT_ERROR_FAILED);
g_assert (agent == NULL);
g_clear_error (&error);
nmtst_assert_success (NM_IS_SECRET_AGENT_OLD (agent), error);
g_assert (!nm_secret_agent_old_get_registered (agent));
g_clear_object (&agent);
loop = g_main_loop_new (NULL, FALSE);
g_async_initable_new_async (test_secret_agent_get_type (),
G_PRIORITY_DEFAULT,
NULL, async_init_cb, loop,
NULL,
async_init_cb,
loop,
NM_SECRET_AGENT_OLD_IDENTIFIER, "test-secret-agent",
NULL);
g_main_loop_run (loop);
g_main_loop_unref (loop);
}
/*****************************************************************************/
typedef struct {
int step;
int invoke_count;
} AutoRegisterData;
static void
registered_changed (GObject *object, GParamSpec *pspec, gpointer user_data)
{
GMainLoop *loop = user_data;
NMSecretAgentOld *agent = NM_SECRET_AGENT_OLD (object);
AutoRegisterData *data = user_data;
g_main_loop_quit (loop);
g_assert (data);
g_assert (NM_IS_SECRET_AGENT_OLD (agent));
data->invoke_count++;
g_assert_cmpint (data->invoke_count, ==, data->step);
switch (data->step) {
case 1:
case 3:
g_assert (nm_secret_agent_old_get_registered (agent));
break;
case 2:
case 4:
g_assert (!nm_secret_agent_old_get_registered (agent));
break;
default:
g_assert_not_reached ();
}
}
static void
test_secret_agent_auto_register (void)
{
NMTstcServiceInfo *sinfo;
NMSecretAgentOld *agent;
GMainLoop *loop;
gs_unref_object NMSecretAgentOld *agent = NULL;
GError *error = NULL;
AutoRegisterData auto_register_data = {
.step = 0,
.invoke_count = 0,
};
gulong signal_id;
NMTstContextBusyWatcherData watcher_data = { };
sinfo = nmtstc_service_init ();
if (!nmtstc_service_available (sinfo))
return;
loop = g_main_loop_new (NULL, FALSE);
agent = test_secret_agent_new ();
g_object_set (agent,
NM_SECRET_AGENT_OLD_AUTO_REGISTER, TRUE,
NULL);
g_signal_connect (agent, "notify::" NM_SECRET_AGENT_OLD_REGISTERED,
G_CALLBACK (registered_changed), loop);
agent = test_secret_agent_new (FALSE);
g_assert (!nm_secret_agent_old_get_registered (agent));
signal_id = g_signal_connect (agent, "notify::" NM_SECRET_AGENT_OLD_REGISTERED,
G_CALLBACK (registered_changed), &auto_register_data);
if (nmtst_get_rand_bool ()) {
g_object_set (agent,
NM_SECRET_AGENT_OLD_AUTO_REGISTER, TRUE,
NULL);
} else
nm_secret_agent_old_enable (agent, TRUE);
g_assert (!nm_secret_agent_old_get_registered (agent));
nm_secret_agent_old_register (agent, NULL, &error);
g_assert_no_error (error);
g_assert (nm_secret_agent_old_get_registered (agent));
/* The GLib ObjectManager doesn't like when we drop the service
* in between it sees the service disappear and the call to
* GetManagedObjects. Give it a chance to do its business.
* Arguably a bug. */
g_main_context_iteration (NULL, FALSE);
/* Shut down test service */
nmtstc_service_cleanup (sinfo);
g_main_loop_run (loop);
g_assert (!nm_secret_agent_old_get_registered (agent));
/* Restart test service */
auto_register_data.step = 1;
nmtst_main_context_iterate_until_assert (NULL,
1000,
nm_secret_agent_old_get_registered (agent));
auto_register_data.step = 2;
nm_secret_agent_old_enable (agent, FALSE);
g_assert (!nm_secret_agent_old_get_registered (agent));
nmtst_main_context_iterate_until (NULL,
nmtst_get_rand_uint32 () % 200,
FALSE);
g_assert (!nm_secret_agent_old_get_registered (agent));
nmtstc_service_cleanup (sinfo);
g_assert (!nm_secret_agent_old_get_registered (agent));
nm_secret_agent_old_enable (agent, TRUE);
g_assert (!nm_secret_agent_old_get_registered (agent));
nmtst_main_context_iterate_until (NULL,
nmtst_get_rand_uint32 () % 200,
FALSE);
g_assert (!nm_secret_agent_old_get_registered (agent));
sinfo = nmtstc_service_init ();
g_assert (nmtstc_service_available (sinfo));
g_main_loop_run (loop);
g_assert (nm_secret_agent_old_get_registered (agent));
auto_register_data.step = 3;
nmtst_main_context_iterate_until_assert (NULL,
1000,
nm_secret_agent_old_get_registered (agent));
/* Let ObjectManager initialize (see above). */
g_main_context_iteration (NULL, FALSE);
/* Shut down test service again */
nmtstc_service_cleanup (sinfo);
g_main_loop_run (loop);
g_assert (!nm_secret_agent_old_get_registered (agent));
g_object_unref (agent);
g_main_loop_unref (loop);
auto_register_data.step = 4;
nmtst_main_context_iterate_until_assert (NULL,
1000,
!nm_secret_agent_old_get_registered (agent));
nm_clear_g_signal_handler (agent, &signal_id);
nmtst_context_busy_watcher_add (&watcher_data, nm_secret_agent_old_get_context_busy_watcher (agent));
g_clear_object (&agent);
nmtst_context_busy_watcher_wait (&watcher_data);
nmtst_main_context_assert_no_dispatch (NULL, nmtst_get_rand_uint32 () % 500);
}
/*****************************************************************************/
@ -656,10 +729,10 @@ main (int argc, char **argv)
nmtst_init (&argc, &argv, TRUE);
g_test_add ("/libnm/secret-agent/none", TestSecretAgentData, NULL, test_setup, test_secret_agent_none, test_cleanup);
g_test_add ("/libnm/secret-agent/no-secrets", TestSecretAgentData, "sync", test_setup, test_secret_agent_no_secrets, test_cleanup);
g_test_add ("/libnm/secret-agent/cancel", TestSecretAgentData, "async", test_setup, test_secret_agent_cancel, test_cleanup);
g_test_add ("/libnm/secret-agent/good", TestSecretAgentData, "async", test_setup, test_secret_agent_good, test_cleanup);
g_test_add ("/libnm/secret-agent/none", TestSecretAgentData, "0", test_setup, test_secret_agent_none, test_cleanup);
g_test_add ("/libnm/secret-agent/no-secrets", TestSecretAgentData, "1", test_setup, test_secret_agent_no_secrets, test_cleanup);
g_test_add ("/libnm/secret-agent/cancel", TestSecretAgentData, "1", test_setup, test_secret_agent_cancel, test_cleanup);
g_test_add ("/libnm/secret-agent/good", TestSecretAgentData, "1", test_setup, test_secret_agent_good, test_cleanup);
g_test_add_func ("/libnm/secret-agent/nm-not-running", test_secret_agent_nm_not_running);
g_test_add_func ("/libnm/secret-agent/auto-register", test_secret_agent_auto_register);

1
po/POTFILES.in
View file

@ -134,6 +134,7 @@ libnm/nm-device-wpan.c
libnm/nm-device.c
libnm/nm-object.c
libnm/nm-remote-connection.c
libnm/nm-secret-agent-old.c
libnm/nm-vpn-plugin-old.c
libnm/nm-vpn-service-plugin.c
data/org.freedesktop.NetworkManager.policy.in.in