After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 739413 - nmcli: implement a polkit agent
nmcli: implement a polkit agent
Status: RESOLVED FIXED
Product: NetworkManager
Classification: Platform
Component: nmcli
git master
Other Linux
: Normal normal
: ---
Assigned To: NetworkManager maintainer(s)
NetworkManager maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2014-10-30 16:49 UTC by Jiri Klimes
Modified: 2014-11-07 12:35 UTC
See Also:
GNOME target: ---
GNOME version: ---



Description Jiri Klimes 2014-10-30 16:49:58 UTC
NetworkManager secures some operations via polkit framework. When nmcli performs those operations and no polkit agent is available for the session (the agents are usually GUI based), the operation may fail due to insufficient permissions.

Implement a polkit agent within nmcli that can ask for authorization when required.
Comment 1 Jiri Klimes 2014-10-30 16:56:20 UTC
I have pushed branch jk/clients-pkagent with polkit agent implementation.

(It is based on jk/nmcli-secret-agent branch because it uses clients/common directory added by that branch.)
Comment 2 Dan Winship 2014-10-30 17:50:37 UTC
> clients: add common code for polkit agent listener

two FIXMES:

>+	//FIXME: polkit_agent_session_cancel() should emit "completed", but it doesn't work for me ???

Should figure out what's up with that?

>+	//FIXME
>+	/* Choose identity */
>+	identity = identities->data;

Do we need to be cleverer here?

>+typedef char * (*NmcPolkitListenerOnRequestFunc) (const char *request,
>...
>+                                                  gpointer user_data,
>+                                                  GError *error);

Should be **error, although if it's not going to be used maybe it should just go away.

>+void nmc_polkit_listener_set_request_callback    (NmcPolkitListener *self,

any reason you didn't just make these be signals?

>+		user = strchr (priv->identity, ':');

That makes assumptions about polkit internals...

I think it would be better to do

  if (POLKIT_IS_UNIX_USER (identity))
      priv->identity = polkit_unix_user_get_name (POLKIT_UNIX_USER (identity));
  else
      priv->identity = polkit_identity_to_string (identity);

and then just pass priv->identity to the on_request_callback.



> cli: add a polkit agent support for nmcli

>+	g_print (_("Warning: nmcli does not ask for polkit authorization without '--ask' option.\n"));

Maybe it should print the message from polkit too? Or else, maybe it should wait to print that warning until it has gotten back the failure error? Right now, it prints that warning for no apparent reason, then does nothing for several seconds, then prints an error.

>+	if (!echo_on) {

If you ^C at the password prompt, the terminal is left in a broken state.

>+	/* Ask user for polkit autorization password */

Missing "h" in "authorization"

>+	if (user) {
>+		tmp = g_strdup (request);
>+		p = strrchr (tmp, ':');
>+		if (p)
>+			*p = '\0';

I had no idea what this was supposed to be doing at first (and assumed that @request must contain two different strings, separated by a ':' for some reason). Can you change "if (p)" to

    if (p && !strcmp (p, ": "))

>+	g_print (_("polkit: %s\n"), text);

"polkit" is kinda technical. Though it's hard to say what else we should say here since the polkit docs don't give any information about what sort of message the message might be... but maybe "Authentication message:" and "Authentication error:" ?

>+	if (gained_authorization)
>+		g_print (_("polkit authentication succeeded!\n"));
>+	else
>+		g_print (_("Error: polkit authentication failed!\n"));

I don't think these are needed. They're going to be followed up by an operation-level succeeded/failed message anyway, making the messages here redundant:

    Error: polkit authentication failed!
    Error: failed to set hostname: Insufficient privileges.

>+	if (for_session)
>+		session = polkit_unix_session_new_for_process_sync (getpid (), NULL, NULL);
>+	else
>+		session = polkit_unix_process_new_for_owner (getpid (), 0, getuid ());
>+
>+	nmc->pk_reg_handle = polkit_agent_listener_register (listener, POLKIT_AGENT_REGISTER_FLAGS_NONE,
>+	                                                       session, NULL, NULL, error);

This would all be the same in nmtui, so it should go into NmcPolkitListener.
Comment 3 Thomas Haller 2014-10-31 12:11:51 UTC
I dislike that clients/common and nmcli both use nmc_ as prefix. Seems non-obvious to me. nmco_*?



>> clients: add common code for polkit agent listener


I'd prefer to use gobject signals instead of the nmc_polkit_listener_set_*_callback() functions because they seem more idiomatic for us.



 * @user_data: location to store errors
                                 ^^^^^^

also, all the other callbacks don't allow you to pass on user_data, which makes it chumbersome to use.


on_request only allows for synchronous requests. That is quite a limitation, because often you cannot block the main loop and you have to ask the user.


»···gobject_class->finalize = nmc_polkit_listener_finalize;

»···pkal_class->initiate_authentication = initiate_authentication;
»···pkal_class->initiate_authentication_finish = initiate_authentication_finish;

I prefer that we call our virtual function implementations as the field in the class structure. e.g. "finalize" better then "nmc_polkit_listener_finalize". "src/" does that (mostly). "nmtui/" does the other style. But maybe not mix the styles in new code?


»···if (priv->active_session != NULL) {
»···»···g_simple_async_result_set_error (simple,
»···»···                                 POLKIT_ERROR,
»···»···                                 POLKIT_ERROR_FAILED,
»···»···                                 _("An authentication session is already underway."));
»···»···g_simple_async_result_complete_in_idle (simple);
»···»···g_object_unref (simple);
»···»···return;
»···}

should we not support more auth-sessions at a time? I think, we should not hold the data in priv->active_session,etc. Instead we should support parallel sessions and keep the data in some ~struct SessionData~.


»···g_cancellable_disconnect (priv->cancellable, priv->cancel_id);
»···g_object_unref (priv->cancellable);
if (priv->cancellable) {




»···//FIXME
»···/* Choose identity */
»···identity = identities->data;

hm, I think we also should support different identities.



>> cli: add a polkit agent support for nmcli



whitespace:

+    nmc->pk_reg_handle = polkit_agent_listener_register (listener, POLKIT_AGENT_REGISTER_FLAGS_NONE,
+                                                           session, NULL, NULL, error);



polkit_request() reads synchronously from stdin, blocking the main loop. I think that is wrong, but I don't see how to fix that easily.
Comment 4 Dan Winship 2014-10-31 13:21:21 UTC
(In reply to comment #3)
> I dislike that clients/common and nmcli both use nmc_ as prefix. Seems
> non-obvious to me. nmco_*?

I eventually want NmcSecretAgent to end up in libnm, and maybe NmcPolkitListener too (we could port it to the polkit D-Bus interface so libnm wouldn't have to depend on the polkit libs). So maybe we should just go with "nm_" (and rename the secret agent to not conflict with the base NMSecretAgent class. "NMSecretAgentSimple" or something.)

> on_request only allows for synchronous requests. That is quite a limitation,
> because often you cannot block the main loop and you have to ask the user.

but since this is currently internal-only API, we don't need to worry about cases nmcli and nmtui don't need yet.

> I prefer that we call our virtual function implementations as the field in the
> class structure. e.g. "finalize" better then "nmc_polkit_listener_finalize".
> "src/" does that (mostly). "nmtui/" does the other style. But maybe not mix the
> styles in new code?

The "long" style is more commonly used in new code. I'm not sure what other people's reasoning is, but the reason I prefer the long style now (and the reason I changed all of libsoup over to use it) is that with the long names, you can do "break nmc_polkit_listener_finalize", etc, in gdb, whereas with the short names, you'd have to figure out the line number of that function if you wanted to set a breakpoint there as opposed to any of the 50 other "finalize" functions.
Comment 5 Thomas Haller 2014-10-31 13:41:41 UTC
(In reply to comment #4)
> (In reply to comment #3)

> > I prefer that we call our virtual function implementations as the field in the
> > class structure. e.g. "finalize" better then "nmc_polkit_listener_finalize".
> > "src/" does that (mostly). "nmtui/" does the other style. But maybe not mix the
> > styles in new code?
> 
> The "long" style is more commonly used in new code. I'm not sure what other
> people's reasoning is, but the reason I prefer the long style now (and the
> reason I changed all of libsoup over to use it) is that with the long names,
> you can do "break nmc_polkit_listener_finalize", etc, in gdb, whereas with the
> short names, you'd have to figure out the line number of that function if you
> wanted to set a breakpoint there as opposed to any of the 50 other "finalize"
> functions.

I like the identical (short) names, because then I can navigate with ctags/cscope from the call site to all the implementations. 

Cscope tag: ip4_route_add
   #   line  filename / context / line
   1   1043  src/platform/nm-fake-platform.c <<ip4_route_add>>
             ip4_route_add (NMPlatform *platform, int ifindex, NMIPConfigSource source,
   2   3752  src/platform/nm-linux-platform.c <<ip4_route_add>>
             ip4_route_add (NMPlatform *platform, int ifindex, NMIPConfigSource source,


I also prefer static functions not to have the nm_TYPE_* prefix, as we do mostly.



But anyway, not that important to me
Comment 6 Jiri Klimes 2014-10-31 21:26:04 UTC
(In reply to comment #2)
> > clients: add common code for polkit agent listener
> 
> two FIXMES:
> 
> >+	//FIXME: polkit_agent_session_cancel() should emit "completed", but it doesn't work for me ???
> 
> Should figure out what's up with that?
Does it work for you?
The current code does effectively the same thing as _cancel(). So I give debugging the issue low priority and it can be done later.

> 
> >+	//FIXME
> >+	/* Choose identity */
> >+	identity = identities->data;
> 
> Do we need to be cleverer here?
> 
Probably we could ask user what identity he wants (when there are more) and then ask for authorization to that identity. But, how common is that there are more identities, especially for NM operations.

> >+typedef char * (*NmcPolkitListenerOnRequestFunc) (const char *request,
> >...
> >+                                                  gpointer user_data,
> >+                                                  GError *error);
> 
> Should be **error, although if it's not going to be used maybe it should just
> go away.
> 
Fixed.

> >+void nmc_polkit_listener_set_request_callback    (NmcPolkitListener *self,
> 
> any reason you didn't just make these be signals?
>
Not any particular reason except it seemed more straightforward to than installing the signals. (And there are two set of signals.)

> >+		user = strchr (priv->identity, ':');
> 
> That makes assumptions about polkit internals...
> 
> I think it would be better to do
> 
>   if (POLKIT_IS_UNIX_USER (identity))
>       priv->identity = polkit_unix_user_get_name (POLKIT_UNIX_USER (identity));
>   else
>       priv->identity = polkit_identity_to_string (identity);
> 
> and then just pass priv->identity to the on_request_callback.
> 
Fixed.

> 
> > cli: add a polkit agent support for nmcli
> 
> >+	g_print (_("Warning: nmcli does not ask for polkit authorization without '--ask' option.\n"));
> 
> Maybe it should print the message from polkit too? Or else, maybe it should
> wait to print that warning until it has gotten back the failure error? Right
> now, it prints that warning for no apparent reason, then does nothing for
> several seconds, then prints an error.
> 
Well, I have changed it to not even registering the agent without '--ask'.


> >+	if (!echo_on) {
> 
> If you ^C at the password prompt, the terminal is left in a broken state.
>
Fixed.
 
> >+	/* Ask user for polkit autorization password */
> 
> Missing "h" in "authorization"
> 
Fixed.

> >+	if (user) {
> >+		tmp = g_strdup (request);
> >+		p = strrchr (tmp, ':');
> >+		if (p)
> >+			*p = '\0';
> 
> I had no idea what this was supposed to be doing at first (and assumed that
> @request must contain two different strings, separated by a ':' for some
> reason). Can you change "if (p)" to
> 
>     if (p && !strcmp (p, ": "))
> 
Fixed.

> >+	g_print (_("polkit: %s\n"), text);
> 
> "polkit" is kinda technical. Though it's hard to say what else we should say
> here since the polkit docs don't give any information about what sort of
> message the message might be... but maybe "Authentication message:" and
> "Authentication error:" ?
> 
Fixed.

> >+	if (gained_authorization)
> >+		g_print (_("polkit authentication succeeded!\n"));
> >+	else
> >+		g_print (_("Error: polkit authentication failed!\n"));
> 
> I don't think these are needed. They're going to be followed up by an
> operation-level succeeded/failed message anyway, making the messages here
> redundant:
> 
>     Error: polkit authentication failed!
>     Error: failed to set hostname: Insufficient privileges.
> 
Fixed.

> >+	if (for_session)
> >+		session = polkit_unix_session_new_for_process_sync (getpid (), NULL, NULL);
> >+	else
> >+		session = polkit_unix_process_new_for_owner (getpid (), 0, getuid ());
> >+
> >+	nmc->pk_reg_handle = polkit_agent_listener_register (listener, POLKIT_AGENT_REGISTER_FLAGS_NONE,
> >+	                                                       session, NULL, NULL, error);
> 
> This would all be the same in nmtui, so it should go into NmcPolkitListener.
Fixed.
Comment 7 Jiri Klimes 2014-10-31 21:45:23 UTC
(In reply to comment #4)
> (In reply to comment #3)
> > I dislike that clients/common and nmcli both use nmc_ as prefix. Seems
> > non-obvious to me. nmco_*?
> 
> I eventually want NmcSecretAgent to end up in libnm, and maybe
> NmcPolkitListener too (we could port it to the polkit D-Bus interface so libnm
> wouldn't have to depend on the polkit libs). So maybe we should just go with
> "nm_" (and rename the secret agent to not conflict with the base NMSecretAgent
> class. "NMSecretAgentSimple" or something.)
> 

I renamed polkit code to use just Nm prefix. I am going to do that for secret agent as well, as Dan suggest. nmco would start to be too cryptic.

> > I prefer that we call our virtual function implementations as the field in the
> > class structure. e.g. "finalize" better then "nmc_polkit_listener_finalize".
> > "src/" does that (mostly). "nmtui/" does the other style. But maybe not mix the
> > styles in new code?
> 
> The "long" style is more commonly used in new code. I'm not sure what other
> people's reasoning is, but the reason I prefer the long style now (and the
> reason I changed all of libsoup over to use it) is that with the long names,
> you can do "break nmc_polkit_listener_finalize", etc, in gdb, whereas with the
> short names, you'd have to figure out the line number of that function if you
> wanted to set a breakpoint there as opposed to any of the 50 other "finalize"
> functions.

I don't have a strong preference. Both namings have valid arguments. The unique name certainly help with debugging, which is nice. We use finalize more often on the other hand.
Comment 8 Dan Winship 2014-11-06 19:22:30 UTC
(In reply to comment #6)
> > >+	//FIXME: polkit_agent_session_cancel() should emit "completed", but it doesn't work for me ???

> The current code does effectively the same thing as _cancel(). So I give
> debugging the issue low priority and it can be done later.

ok

> Probably we could ask user what identity he wants (when there are more) and
> then ask for authorization to that identity. But, how common is that there are
> more identities, especially for NM operations.

gnome-shell's rule is: pick $USER if it's in the list, else pick 'root' if it's in the list, else pick identities[0]. That seems like a pretty good rule.

> > >+void nmc_polkit_listener_set_request_callback    (NmcPolkitListener *self,
> > 
> > any reason you didn't just make these be signals?
> >
> Not any particular reason except it seemed more straightforward to than
> installing the signals. (And there are two set of signals.)

Using signals would really be much better I think. But this is internal API, so that can happen later.


(In reply to comment #7)
> I renamed polkit code to use just Nm prefix. I am going to do that for secret
> agent as well, as Dan suggest. nmco would start to be too cryptic.

"NM" please, not "Nm". To be consistent with everywhere else.


everything else looks good
Comment 9 Dan Williams 2014-11-07 04:28:00 UTC
everything looks (and works! neat stuff...) good to me after danw's comments.  I'd vote for "NM" too where we need capitals/studly-caps.
Comment 10 Jiri Klimes 2014-11-07 12:35:13 UTC
I changed the prefix to "NM" and (In reply to comment #8)
> (In reply to comment #6)
> > Probably we could ask user what identity he wants (when there are more) and
> > then ask for authorization to that identity. But, how common is that there are
> > more identities, especially for NM operations.
> 
> gnome-shell's rule is: pick $USER if it's in the list, else pick 'root' if it's
> in the list, else pick identities[0]. That seems like a pretty good rule.
> 
Done.

> 
> (In reply to comment #7)
> > I renamed polkit code to use just Nm prefix. I am going to do that for secret
> > agent as well, as Dan suggest. nmco would start to be too cryptic.
> 
> "NM" please, not "Nm". To be consistent with everywhere else.
> 
Done.

> 
> everything else looks good

Committed to master:
cc7dc01 merge: implement a polkit agent and use it in nmcli (bgo #739413)
e517061 cli: add a polkit agent support for nmcli
c7aaee1 configure: check whether polkit-agent-1 is available
ca5d6be clients: add common code for polkit agent listener