Change all DBUS_TYPE_G_MAP_OF_STRING properties to G_TYPE_HASH_TABLE,
with annotations indicating they are string->string. Not much outside
libnm-core needs to changed for this, since DBUS_TYPE_G_MAP_OF_STRING
was already represented as a hash table.
(One change needed within libnm-core is that we now need to copy the
hash tables in get_property(), or else the caller will receive a
reffed copy of the object's own hash table, which we don't want.)
Rename nm_connection_to_hash() to nm_connection_to_dbus(), and
nm_connection_new_from_hash() to nm_connection_new_from_dbus(). In
addition to clarifying that this is specifically the D-Bus
serialization format, these names will also work better in the
GDBus-based future where the serialization format is GVariant, not
GHashTable.
Also, move NMSettingHashFlags to nm-connection.h, and rename it
NMConnectionSerializationFlags.
The fact that NMRemoteConnection has to be an NMConnection and
therefore can't be an NMObject means that it needs to reimplement bits
of NMObject functionality (and likewise NMObject needs some special
magic to deal with it). Likewise, we will need a daemon-side
equivalent of NMObject as part of the gdbus port, and we would want
NMSettingsConnection to be able to inherit from this as well.
Solve this problem by making NMConnection into an interface, and
having NMRemoteConnection and NMSettingsConnection implement it. (We
use some hacks to keep the GHashTable of NMSettings objects inside
nm-connection.c rather than having to be implemented by the
implementations.)
Since NMConnection is no longer an instantiable type, this adds
NMSimpleConnection to replace the various non-D-Bus-based uses of
NMConnection throughout the code. nm_connection_new() becomes
nm_simple_connection_new(), nm_connection_new_from_hash() becomes
nm_simple_connection_new_from_hash(), and nm_connection_duplicate()
becomes nm_simple_connection_new_clone().
"NetworkManager.h"'s name (and non-standard capitalization) suggest
that it's some sort of high-level super-important header, but it's
really just low-level D-Bus stuff. Rename it to "nm-dbus-interface.h"
and likewise "NetworkManagerVPN.h" to "nm-vpn-dbus-interface.h"
For some reason, the flags used by o.fd.NM.SecretAgent.GetSecrets were
defined as both NMSecretAgentGetSecretsFlags in
libnm{,-glib}/nm-secret-agent.h, and then separately as
NMSettingsGetSecretsFlags in include/nm-settings-flags.h.
(NMSettingsGetSecretsFlags also had an additional internal-use-only
value, but that was added later after the duplication already
existed.)
Fix this by moving NMSecretAgentGetSecretsFlags from libnm to
nm-dbus-interface.h, adding the internal-use-only value to it as well,
updating the core code to use that, and then removing
nm-settings-flags.h.
Clean up some of the cross-includes between headers (which made it so
that, eg, if you included NetworkManagerUtils.h in a test program, you
would need to build the test with -I$(top_srcdir)/src/platform, and if
you included nm-device.h you'd need $(POLKIT_CFLAGS)) by moving all
GObject struct definitions for src/ and src/settings/ into nm-types.h
(which already existed to solve the NMDevice/NMActRequest circular
references).
Update various .c files to explicitly include the headers they used to
get implicitly, and remove some now-unnecessary -I options from
Makefiles.
NMAgentManager was supposed to be trying multiple secret agents on any
error except UserCanceled, but due to a botched last-minute rewrite,
it was actually doing the reverse.
When an activation request requires secrets, if there is a secret
agent in the process that made the request, then prefer that to all
other secret agents.
Rather than explicitly passing around a UID and a flag saying whether
or not it's relevant.
(This also fixes a bug where the wrong UID was being recorded in
nm-settings-connection.c::auth_start(), which caused problems such as
agent-owned secrets not getting saved because of a perceived UID
mismatch.)
When request for getting secrets is being freed in request_free(),
cancel_callback is get_cancel_cb(). It uses parent->current as a secret agent
object. However, this object can be already freed and thus there is a problem
getting priv in nm_secret_agent_cancel_secrets:
g_return_if_fail (self != NULL);
priv = NM_SECRET_AGENT_GET_PRIVATE (self);
(gdb) p self
$66 = (NMSecretAgent *) 0x7fae9afd42e0
(gdb) p *self
$67 = {parent = {g_type_instance = {g_class = 0x0}, ref_count = 0, qdata = 0x0}}
#0 nm_secret_agent_cancel_secrets (self=0x7fae9afd42e0, call=0x1) at settings/nm-secret-agent.c:325
#1 0x00007fae9a774882 in request_free (req=0x7fae9afc48f0) at settings/nm-agent-manager.c:496
#2 0x00007fae967b251a in g_hash_table_remove_internal (hash_table=0x7fae9aefdf00, key=0x2, notify=1) at ghash.c:1276
#3 0x00007fae9a72b340 in dispose (object=0x7fae9af77200) at nm-activation-request.c:446
#4 0x00007fae96cbeee8 in g_object_unref (_object=0x7fae9af77200) at gobject.c:3160
#5 0x00007fae9a73d87c in _active_connection_cleanup (user_data=<optimized out>) at nm-manager.c:359
#6 0x00007fae967c32a6 in g_main_dispatch (context=0x7fae9aedb180) at gmain.c:3066
#7 g_main_context_dispatch (context=context@entry=0x7fae9aedb180) at gmain.c:3642
#8 0x00007fae967c3628 in g_main_context_iterate (context=0x7fae9aedb180, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at gmain.c:3713
#9 0x00007fae967c3a3a in g_main_loop_run (loop=0x7fae9aedb860) at gmain.c:3907
So we need to ref() 'agent' when adding it to pending list, so that the object
is not freed if the secret agent unregisters and is removed.
Test case:
1. run NM and nm-applet
2. activate a Wi-Fi network
3. nm-applet will ask for a password; ignore the popup window and kill nm-applet
4. start nm-applet again
5. click the same Wi-Fi network in nm-applet
6. NM will experience problems in nm_secret_agent_cancel_secrets() or crashes
(the procedure may not be 100%, but reproduces most of the time)
https://bugzilla.redhat.com/show_bug.cgi?id=922855
GLib-CRITICAL **: g_hash_table_iter_next: assertion 'ri->version == ri->hash_table->version' failed
It is not allowed to modify hash table while it is iterated. Unfortunately,
request_remove_agent() may remove the request from the 'requests' hash table,
making it not usable in the loop hash table looping.
We need to store the request into a temporary list and call request_next_agent()
on them later (after the hash loop).
Test case:
1. start NM and nm-applet
2. activate a Wi-Fi WPA connection
3. nm-applet displays a dialog asking for a password
4. kill nm-applet
5. NetworkManager removes the nm-applet's secret agent
and runs into removing the request from hash table in the
iterating loop (via get_complete_cb)
#0 get_complete_cb (parent=0x7f3f250f2970, secrets=0x0, agent_dbus_owner=0x0, agent_username=0x0, error=0x7f3f250f7830, user_data=0x7f3f25020e10)
at settings/nm-agent-manager.c:1111
#1 0x00007f3f23b46ea5 in req_complete_error (error=0x7f3f250f7830, req=0x7f3f250f2970) at settings/nm-agent-manager.c:509
#2 request_next_agent (req=0x7f3f250f2970) at settings/nm-agent-manager.c:615
#3 0x00007f3f23b48596 in request_remove_agent (agent=0x7f3f250f4a20, req=0x7f3f250f2970) at settings/nm-agent-manager.c:631
#4 remove_agent (self=<optimized out>, owner=0x7f3f250dbff0 ":1.275") at settings/nm-agent-manager.c:130
#5 0x00007f3f23b4868d in impl_agent_manager_unregister (self=0x7f3f25020e10, context=0x7f3f250f5480) at settings/nm-agent-manager.c:374
#0 0x00007f3f1fb9c4e9 in g_logv (log_domain=0x7f3f1fbfef4e "GLib", log_level=G_LOG_LEVEL_CRITICAL, format=<optimized out>, args=args@entry=0x7fff156b77c0) at gmessages.c:989
#1 0x00007f3f1fb9c63f in g_log (log_domain=log_domain@entry=0x7f3f1fbfef4e "GLib", log_level=log_level@entry=G_LOG_LEVEL_CRITICAL,
format=format@entry=0x7f3f1fc0889a "%s: assertion '%s' failed") at gmessages.c:1025
#2 0x00007f3f1fb9c679 in g_return_if_fail_warning (log_domain=log_domain@entry=0x7f3f1fbfef4e "GLib",
pretty_function=pretty_function@entry=0x7f3f1fc03c30 <__PRETTY_FUNCTION__.4571> "g_hash_table_iter_next",
expression=expression@entry=0x7f3f1fc038f0 "ri->version == ri->hash_table->version") at gmessages.c:1034
#3 0x00007f3f1fb849c0 in g_hash_table_iter_next (iter=<optimized out>, key=<optimized out>, value=<optimized out>) at ghash.c:733
#4 0x00007f3f23b484e5 in remove_agent (self=<optimized out>, owner=0x7f3f250dbff0 ":1.275") at settings/nm-agent-manager.c:129
#5 0x00007f3f23b4868d in impl_agent_manager_unregister (self=0x7f3f25020e10, context=0x7f3f250f5480) at settings/nm-agent-manager.c:374
These are (most likely) only warnings and not severe bugs.
Some of these changes are mostly made to get a clean run of
Coverity without any warnings.
Error found by running Coverity scan
https://bugzilla.redhat.com/show_bug.cgi?id=1025894
Co-Authored-By: Jiří Klimeš <jklimes@redhat.com>
Signed-off-by: Thomas Haller <thaller@redhat.com>
This fixes a glib assertion:
"dbus_g_proxy_cancel_call: assertion `pending != NULL' failed"
Backtrace:
#0 0x00007f962dad9e0d in g_logv () from /lib64/libglib-2.0.so.0
#1 0x00007f962dad9ff2 in g_log () from /lib64/libglib-2.0.so.0
#2 0x00000000004a84bd in nm_secret_agent_cancel_secrets (self=0x213b300, call=0x1) at settings/nm-secret-agent.c:331
#3 0x00000000004a4068 in request_free (req=0x216a490) at settings/nm-agent-manager.c:479
#4 0x00007f962dac25fa in g_hash_table_remove_internal () from /lib64/libglib-2.0.so.0
Signed-off-by: Thomas Haller <thaller@redhat.com>
Turns out this function is useless, because it's only called when the
agent has dropped off the bus or when the whole request is being
freed. If the agent has dropped off the bus then there's no point
in asking it to cancel the request because there's nothing to ask.
So we can collapse request_cancel() into request_free().
If all agents can handle VPN hints, then we'll try to use
ConnectInteractive() to let the VPN plugin ask for secrets
interactively via the SecretsRequired signal. These hints
are then passed to agents during the connection process if
the plugin needs more secrets or different secrets, and when
the new secrets are returned, they are passed back to the VPN
plugin.
If at least one agent does not have the VPN hints capability,
we can't use ConnectInteractive(), but fall back to the old
Connect call, because that agent won't be able to send the
hints to the VPN plugin's authentication dialog, and thus
we won't get back the secrets the VPN plugin is looking for.
So, for interactive secrets to work correctly, you need:
1) A VPN plugin updated for interactive secrets requests
2) NM updated for interactive secrets requests
3) all agents to set the VPN_HINTS capability when
registering with NetworkManager and to pass hints
along to the VPN authentication dialog
4) a VPN authentication dialog updated to look for hints
and only return secrets corresponding to the hints
requested by the plugin
Previously I didn't think they'd be used for anything other than connection secrets
which only have one hint, but in the future we'll want to pass more information.
Split the agent secrets request tracking structure into a generic
structure for tracking any agent request, and a connection-specific
subclass. We'll use the generic structure in the future for device
secrets and other stuff.
Most callers of nm_auth_chain_new() call nm_dbus_manager_get_caller_info()
right before that, so just fold the get_caller_info() call into
nm_auth_chain_new() to reduce code complexity in callers. Yes, this
means sometimes we call nm_dbus_manager_get_caller_info() twice,
but that's not really a problem.
Normally, users which are not part of a login session can't access
connections. Root won't always be part of a login session, so
allow root to bypass visibility checks. The code already bypassed
the ACL checks for root, but in multiple places. Consolidate those
checks into one function.
Instead of doing something like
<get caller UID>
if (root) {
perform_operation()
other boilerplate stuff
return;
}
nm_auth_chain_new(perform_operation)
...
just have root also go through the auth chain, which is now
short circuited for root. This ensures we always use the same
code paths for root and non-root, and that fixes made in one path
are also executed for the other.
When providing a service on the bus daemon and a private connection,
we'll need to track objects so we can register them with the
private connection too. Thus all registration/unregistration
calls have to go through the NMDBusManager, not straight to
dbus-glib.
The ctype macros (eg, isalnum(), tolower()) are locale-dependent. Use
glib's ASCII-only versions instead.
Also, replace isascii() with g_ascii_isprint(), since isascii()
accepts control characters, which isn't what the code wanted in any of
the places where it was using it.
Rather than generating enum classes by hand (and complaining in each
file that "this should really be standard"), use glib-mkenums.
Unfortunately, we need a very new version of glib-mkenums in order to
deal with NM's naming conventions and to fix a few other bugs, so just
import that into the source tree temporarily.
Also, to simplify the use of glib-mkenums, import Makefile.glib from
https://bugzilla.gnome.org/654395.
To avoid having to run glib-mkenums for every subdirectory of src/,
add a new "generated" directory, and put the generated enums files
there.
Finally, use Makefile.glib for marshallers too, and generate separate
ones for libnm-glib and NetworkManager.
Use case:
A user has an auto-activatable connection with secrets in a keyring. While
booting NM starts and tries to activate the connection, but it fails because of
missing secrets. Then the user logs in, but the connection is marked as invalid
and is not tried again.
This commit solves the issue by removing invalid flag and activating the
connection when a secret agent registers.
Signed-off-by: Jiří Klimeš <jklimes@redhat.com>
The core problem was the nm_connection_need_secrets() call in
nm-agent-manager.c's get_start() function; for VPN settings this
always returns TRUE. Thus if a VPN connection had only system
secrets, when the agent manager checked if additional secrets
were required, they would be, and agents would be asked for
secrets they didn't have and couldn't provide. Thus the
connection would fail. nm_connection_need_secrets() simply
can't know if VPN secrets are really required because it
doesn't know anything about the internal VPN private data;
only the plugin itself can tell us if secrets are required.
If the system secrets are sufficient we shouldn't be asking any
agents for secrets at all. So implement a three-step secrets
path for VPN connections. First we retrieve existing system
secrets, and ask the plugin if these are sufficient. Second we
request both existing system secrets and existing agent secrets
and again ask the plugin if these are sufficient. If both those
fail, we ask agents for new secrets.
All non-VPN secrets are considered system-owned if they do not
have any explicitly set secret flags, and this makes VPN secrets
treated the same way. As part of the import process plugins and
the applet already update secret flags. This ensures that VPN
secrets are treated consistently throughout the codebase.
Use one global PolkitAuthority object; we only really need to use it
in one place anyway. So consolidate the code that uses polkit into
nm-manager-auth.c.
If the connection had system secrets, previously the settings core
would consider those sufficient even if the device code had requested
new secrets because the old ones didn't work.
