GNOME Bugzilla – Bug 739413
nmcli: implement a polkit agent
Last modified: 2014-11-07 12:35:13 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.
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.)
> 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.
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.
(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.
(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
(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.
(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.
(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
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.
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