> core: convert DHCP client communication to use a private socket

OK, again I don't know low-level D-Bus well enough, but the idea makes sense.

One comment (which actually belongs back at "core: add a root-only private D-Bus socket" but I didn't know it then) is that for NMDBusManager::private-connection-new and ::private-connection-disconnected, you should put the tag in the signal detail rather than as a signal argument. Then nm-dhcp-manager can connect to NM_DBUS_MANAGER_PRIVATE_CONNECTION_NEW "::" PRIV_SOCK_TAG, and it doesn't need to do a strcmp() at the top of new_connection_cb, because it will only get called for its own connections.

(nm-dbus-manager's private_connection_new() default handler would still be called for all emissions... it would have to call g_signal_get_invocation_hint(self) to find the detail it was being invoked with and check that against PRIV_SOCK_TAG.)

Just a side note, it's not too important now...

But for non-root NetworkManager testing, I can also benefit from not having to use the system bus. Using a private bus will be much better than using the session bus, so the only thing I will need is to alter the location of the private bus for both NetworkManager command-line at runtime.

(In reply to comment #0)
> >-	g_assert (pending->chain);
> >+	if (pending->chain) {
> 
> So there seems to be some sort of "error condition X which used to be fatal is
> now no longer fatal" thing going on here, but the commit message doesn't
> mention it, and I'm not 100% sure what it is (or if it's even really related to
> the nm-manager-auth.c part of the patch...)

Correct; I've moved these bits of the patch to later in the series where the "else" condition is actually added to make things clearer.

> > core: add nm_dbus_g_method_invocation_get_g_connection()
> > 
> > In lieu of https://bugs.freedesktop.org/show_bug.cgi?id=55729; should
> > be removed when that patch gets committed.
> 
> neither the summary nor the body correctly describes the current state of the
> patch.

commit message and code comments clarified.

> > core: add a root-only private D-Bus socket
> >@@ -281,7 +501,7 @@ nm_dbus_manager_init_bus (NMDBusManager *self)
> >-		nm_log_err (LOGD_CORE, "Could not get the system bus.  Make sure "
> >+		nm_log_dbg (LOGD_CORE, "Could not get the system bus.  Make sure "
> 
> dbg is a little bit low. I think it ought to log something that will be visible
> at the default logging levels.

Done.  The change was originally because we'll now be running in cases where we *don't* expect the system bus, so it should no longer be an error, and the message should be more informational than a red flag.

> > libnm-glib: look for private bus connection before using system bus
> 
> >@@ -1454,15 +1454,18 @@ init_sync (GInitable *initable, GCancellable *cancellable, GError **error)
> >-	if (priv->manager_running && !get_permissions_sync (client, error))
> >+	if (   (priv->manager_running || priv->private_bus)
> >+	    && !get_permissions_sync (client, error))
> 
> Do you actually need to check priv->private_bus there? Is (priv->private_bus &&
> !priv->manager_running) possible?

You're right, fixed.

> >+dbus_g_connection_open_private (const gchar *address,
> > ...
> >+	/* This code is evil.  Since dbus-glib doesn't give us a corresponding
> 
> Yes, and why bother? Just make the client side not support private buses unless
> HAVE_DBUS_GLIB_GC_OPEN_PRIVATE. (All the code in nm-client/nm-remote-settings
> would still be there, it's just that _nm_get_bus() would never actually return
> a private bus.)

We'd like to use the private bus stuff with older dbus-glib though, so I kept this bit but reworded the comment.

> > core: simplify object set property filter
> >
> > dbus-glib has had dbus_g_connection_lookup_g_object() since 2006,
> 
> Or maybe more relevantly, since way before 0.94, which is the version we
> currently require in configure.

reworded.

> >@@ -3800,14 +3773,12 @@ prop_filter (DBusConnection *connection,
> >+	g_object_ref (obj);
> 
> I think this leaks this ref if caller_uid == 0. Might as well just put the
> g_object_ref() in the nm_auth_chain_set_data() call, like you do for
> "connection".

Fixed.

> > core: use same codepaths for root and non-root during authentication
> 
> >@@ -3121,22 +3112,8 @@ impl_manager_deactivate_connection (NMManager *self,
> > 	/* Otherwise validate the user request */
> 
> It's not "Otherwise" any more

Fixed that stuff up too.

> >@@ -225,7 +226,6 @@ agent_register_permissions_done (NMAuthChain *chain,
> >-		dbus_g_method_return (context);
> >...
> >+		dbus_g_method_return (context);
> 
> not clear why you moved this (is it fixing a (potential?) bug?)

Nah, just cleanup. But I moved it back.

> > core: use DBusManager caller info/auth functions
> >
> > Only the DBusManager can get the sender for private connections.
> 
> You should move this to before the commit that adds the private bus. Otherwise
> all the commits in between are broken and will thwart "git bisect".

Can't move this code before private bus since it depends on some bits of it, but to preserve bisect I moved all the libnm-glib changes later in the series.

> > agents: enforce one-agent-per-identifier-per-UID restriction
> 
> any reason this is here or is it just a drive-by bugfix?

Drive-by.

> > agents: don't require root agents to be part of a login session
> 
> would it make more sense to put the uid check into
> nm_session_monitor_uid_has_session(), like with nm_auth_uid_in_acl()?

If we did that we'd have to dupe the code into each session monitor backend, so I was trying to keep the check in one place in the generic code instead of in each of the [systemd, ConsoleKit, null] backends.

> > auth: move common nm_dbus_manager_get_caller_info() functionality into nm_auth_chain_new()
> 
> The error codes seem a bit random in some places
> (NM_SETTINGS_ERROR_NOT_PRIVILEGED vs NM_SETTINGS_ERROR_PERMISSION_DENIED?), but
> I'll assume you've thought about them.

Not at all.  It's a bug and I've fixed up the cases that used it, and finally removed NOT_PRIVILEGED completely.  No idea why we had two.

(In reply to comment #1)
> > core: convert DHCP client communication to use a private socket
> 
> OK, again I don't know low-level D-Bus well enough, but the idea makes sense.
> 
> One comment (which actually belongs back at "core: add a root-only private
> D-Bus socket" but I didn't know it then) is that for
> NMDBusManager::private-connection-new and ::private-connection-disconnected,
> you should put the tag in the signal detail rather than as a signal argument.
> Then nm-dhcp-manager can connect to NM_DBUS_MANAGER_PRIVATE_CONNECTION_NEW "::"
> PRIV_SOCK_TAG, and it doesn't need to do a strcmp() at the top of
> new_connection_cb, because it will only get called for its own connections.
> 
> (nm-dbus-manager's private_connection_new() default handler would still be
> called for all emissions... it would have to call
> g_signal_get_invocation_hint(self) to find the detail it was being invoked with
> and check that against PRIV_SOCK_TAG.)

So I fixed all that up and instead of checking the invocation hint (which requires some GQuark stuff) I just killed the default handler and made the dbus manager connect to its own signal with a detail.

One question I had... so I moved the DHCP helper over to use the private-bus exclusively.  This means that if you upgrade NM, it'll write the new helper to disk, but the running NM won't know about private bus.  So until you restart NM, DHCP would be broken.  Is that a problem?

I supposed we make the DHCP helper try private bus first and fall back to public bus to make sure upgrades work.  Is it worth it?  Or should we just expect that people who upgrade NM also restart it within a reasonable amount of time?

(In reply to comment #5)
> Or should we just
> expect that people who upgrade NM also restart it within a reasonable amount of
> time?

No one except developers would ever be upgrading from a non-private-bus version to a private-bus version on a live system (as opposed to during a system upgrade), right? In that case, yes, I think it's fine for things to be broken until they restart.

Ok, all good to merge then?

yes, looks good now

Ok, substantial rework now posted to the branch.  The main changes are:

1) many new libnm-glib changes - using a private connection requires that we use dbus_g_proxy_new_for_peer() because the other DBusGProxy functions have logic to talk to the system bus, which may or may not exist.  Besides that, we need skip various code that talks to the system bus

2) new changes the NM core agent interface to use dbus_g_proxy_new_for_peer() when an agent connects; same reason as (1).  In addition, since the agent  manager uses name-owner-changed signals to manage the list of active agents, we need to synthesize that signal when we get a disconnect event for a private connection

Also, private connection cannot be used with dbus-glib < 0.100 due to bugs in dbus-glib, so it is disabled there.

One open question: if any client code used to use nm_object_get_connection() and then create their own proxies from that, this code will break if the connection is a private connection because the client code was likely not calling dbus_g_proxy_new_for_peer().  So should we make private connection stuff opt-in by adding a method like nm_enable_private_connections(TRUE); that clients call?

(In reply to comment #9)
> One open question: if any client code used to use nm_object_get_connection()
> and then create their own proxies from that, this code will break if the
> connection is a private connection because the client code was likely not
> calling dbus_g_proxy_new_for_peer().  So should we make private connection
> stuff opt-in by adding a method like nm_enable_private_connections(TRUE); that
> clients call?

I believe that if we properly document it, we can afford to do it by default (or always, if that matters). But we should document it and make nm_object_get_connection() issue a runtime warning and also a compiler warning.

If you don't like it, an alternative could be to drop the private bus connection on nm_object_get_connection() and replace it with a system bus connection before handing it over. It is an ugly hack but unless there is a problem with it, it would deliver backwards compatibility while keeping the correct behavior default.

> core: use wrappers for DBus object registration/unregistration

>@@ -67,6 +68,7 @@ nm_dbus_manager_get (void)
> 
> 	if (!singleton) {
> 		singleton = NM_DBUS_MANAGER (g_object_new (NM_TYPE_DBUS_MANAGER, NULL));
>+		g_assert (singleton);

unnecessary, the call can't fail

>@@ -87,6 +92,9 @@ nm_dbus_manager_dispose (GObject *object)
> {
> 	NMDBusManagerPrivate *priv = NM_DBUS_MANAGER_GET_PRIVATE (object);
> 
>+	g_hash_table_destroy (priv->exported);
>+	priv->exported = NULL;

If it's actually possible for the NMDBusManager to be destroyed, then you need to drop all the weak refs here too, or it will crash later. But it's not really possible is it? In which case you should just g_assert_not_reached() or g_warn_if_reached() here.

>+nm_dbus_manager_unregister_object (NMDBusManager *self, gpointer object)

Need to g_object_weak_unref() here too

(Actually, priv->exported and the weak refs aren't needed at all in this patch anyway; maybe that should all be moved to the patch where you add private connections?)



> core: add nm_dbus_g_method_invocation_get_g_connection()

The dbus check section of configure.ac is pretty messy by the end of this patch series...



> core: add a root-only private D-Bus socket

>- * Copyright (C) 2006 - 2010 Red Hat, Inc.
>+ * Copyright (C) 2006 - 2012 Red Hat, Inc.

just noticed this here; you might have done this in other patches too, but anyway, should be 2013 now.



> core: use same codepaths for root and non-root during authentication

This doesn't get rid of all of them though. Eg, the one in nm-manager.c:pending_activation_check_authorized() that gets removed later in "auth: move common nm_dbus_manager_get_caller_info() functionality into nm_auth_chain_new()"



> agents: enforce one-agent-per-identifier-per-UID restriction

>+	/* Only one identifier per user is allowed */

That's not right. "Only one agent for each identifier is allowed per user" or something.



> core: convert DHCP client communication to use a private socket

If you're going to do all those code style fixes you should switch the comments from doxygen to gtk-doc too :)

>-	connection = dbus_bus_get (DBUS_BUS_SYSTEM, &error);

you can't get rid of the old code now; you need it #ifndef HAVE_DBUS_GLIB_100



> libnm-glib: add helpers to create object proxies
> libnm-glib: use object proxy creation helpers

Half of this ends up getting undone by "libnm-glib: add helper to connect to NM's private D-Bus socket"... seems like it would be simple enough to just change everything to call _nm_dbus_new_proxy_for_connection() rather than dbus_g_proxy_new_for_name(), and forget about the NMObject helper method.



> libnm-glib: use private connection before trying the system bus

The private bus codepaths don't deal with the server exiting/restarting. Actually, you don't call dbus_connection_set_exit_on_disconnect() on the private connection, so I think they'll just die in that case...



> libnm-glib: never call NM D-Bus methods if NM isn't running

Note that NMRemoteSettings and NMSecretAgent have the same problem NMClient does.



> cli: use nm_client_get_manager_running() instead of nmc_is_nm_running()

Can't it just check at the top level rather than checking in every single function?

(In reply to comment #11)
> > core: use wrappers for DBus object registration/unregistration
> 
> >@@ -67,6 +68,7 @@ nm_dbus_manager_get (void)
> > 
> > 	if (!singleton) {
> > 		singleton = NM_DBUS_MANAGER (g_object_new (NM_TYPE_DBUS_MANAGER, NULL));
> >+		g_assert (singleton);
> 
> unnecessary, the call can't fail

Fixed.

> >@@ -87,6 +92,9 @@ nm_dbus_manager_dispose (GObject *object)
> > {
> > 	NMDBusManagerPrivate *priv = NM_DBUS_MANAGER_GET_PRIVATE (object);
> > 
> >+	g_hash_table_destroy (priv->exported);
> >+	priv->exported = NULL;
> 
> If it's actually possible for the NMDBusManager to be destroyed, then you need
> to drop all the weak refs here too, or it will crash later. But it's not really
> possible is it? In which case you should just g_assert_not_reached() or
> g_warn_if_reached() here.

Fixed, weak unrefed.

> >+nm_dbus_manager_unregister_object (NMDBusManager *self, gpointer object)
> 
> Need to g_object_weak_unref() here too

Fixed.

> (Actually, priv->exported and the weak refs aren't needed at all in this patch
> anyway; maybe that should all be moved to the patch where you add private
> connections?)

They aren't needed yet, but I think keeping the code in this patch makes the series clearer than putting it all into the bits that actually use the private bus.

> > core: add nm_dbus_g_method_invocation_get_g_connection()
> 
> The dbus check section of configure.ac is pretty messy by the end of this patch
> series...

Cleaned up.

> > core: add a root-only private D-Bus socket
> 
> >- * Copyright (C) 2006 - 2010 Red Hat, Inc.
> >+ * Copyright (C) 2006 - 2012 Red Hat, Inc.
> 
> just noticed this here; you might have done this in other patches too, but
> anyway, should be 2013 now.

Fixed.

> > core: use same codepaths for root and non-root during authentication
> 
> This doesn't get rid of all of them though. Eg, the one in
> nm-manager.c:pending_activation_check_authorized() that gets removed later in
> "auth: move common nm_dbus_manager_get_caller_info() functionality into
> nm_auth_chain_new()"

Fixed.

> > agents: enforce one-agent-per-identifier-per-UID restriction
> 
> >+	/* Only one identifier per user is allowed */
> 
> That's not right. "Only one agent for each identifier is allowed per user" or
> something.

Fixed.

> > core: convert DHCP client communication to use a private socket
> 
> If you're going to do all those code style fixes you should switch the comments
> from doxygen to gtk-doc too :)

Done.

> >-	connection = dbus_bus_get (DBUS_BUS_SYSTEM, &error);
> 
> you can't get rid of the old code now; you need it #ifndef HAVE_DBUS_GLIB_100

Fixed.

> > libnm-glib: add helpers to create object proxies
> > libnm-glib: use object proxy creation helpers
> 
> Half of this ends up getting undone by "libnm-glib: add helper to connect to
> NM's private D-Bus socket"... seems like it would be simple enough to just
> change everything to call _nm_dbus_new_proxy_for_connection() rather than
> dbus_g_proxy_new_for_name(), and forget about the NMObject helper method.

I've made these bits clearer now by squashing some of the patches.

> > libnm-glib: use private connection before trying the system bus
> 
> The private bus codepaths don't deal with the server exiting/restarting.
> Actually, you don't call dbus_connection_set_exit_on_disconnect() on the
> private connection, so I think they'll just die in that case...

Partially fixed; the private bus doesn't handle server restart because there's no good way to do that which I can find right now.  But also because the private socket is root-only, the service isn't supposed to be restarted often, and we don't expect long-running programs (like nm-applet) to be run as root.

This could be fixed by monitoring the system bus for NameOwnerChanged signals and doing the right thing with the private connections when NM goes away or comes back though, since the bus daemon will exist outside of minimal environments where this might actually be an issue.

> > libnm-glib: never call NM D-Bus methods if NM isn't running
> 
> Note that NMRemoteSettings and NMSecretAgent have the same problem NMClient
> does.

Fixed.

> > cli: use nm_client_get_manager_running() instead of nmc_is_nm_running()
> 
> Can't it just check at the top level rather than checking in every single
> function?

I opted to not rearchitect nmcli yet,  though clearly this is better to do.

looks good to me

This branch has been merged.