[libvirt] [PATCH v2 3/3] virsh: Add support for text based polkit authentication

[Daniel P. Berrange]

On Thu, Feb 11, 2016 at 06:38:14PM -0500, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=872166
> 
> When the login session doesn't have an ssh -X type display agent in
> order for libvirtd to run the polkit session authentication, attempts
> to run 'virsh -c qemu:///system list' from an unauthorized user (or one
> that isn't part of the libvirt /etc/group) will fail with the following
> error from libvirtd:
> 
> error: authentication failed: no agent is available to authenticate -
>        no polkit agent for action 'org.libvirt.unix.manage'
> 
> In order to handle the local authentication, we will use the new
> virPolkitAgentCreate API in order to create a text based authentication
> agent for our non readonly session to authenticate with.
> 
> The new code will execute in a loop allowing 5 failures to authenticate
> before failing out. It also has a check for a failure to start the text
> polkit authentication agent as testing as shown the agent startup isn't
> necessarily immediate.
> 
> With this patch in place, the following occurs:
> 
> $ virsh -c qemu:///system list
> ==== AUTHENTICATING FOR org.libvirt.unix.manage ===
> System policy prevents management of local virtualized systems
> Authenticating as: Some User (SUser)
> Password:
> ==== AUTHENTICATION COMPLETE ===
>  Id    Name                           State
>  ----------------------------------------------------
>   1     somedomain                     running
> 
> $
> 
> Signed-off-by: John Ferlan <jferlan at redhat.com>
> ---
>  tools/virsh.c | 49 ++++++++++++++++++++++++++++++++++++++++++++-----
>  tools/virsh.h |  2 ++
>  2 files changed, 46 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/virsh.c b/tools/virsh.c
> index b96dbda..c9da9f1 100644
> --- a/tools/virsh.c
> +++ b/tools/virsh.c
> @@ -143,6 +143,9 @@ virshConnect(vshControl *ctl, const char *uri, bool readonly)
>      int interval = 5; /* Default */
>      int count = 6;    /* Default */
>      bool keepalive_forced = false;
> +    virCommandPtr pkagent = NULL;
> +    int authfail = 0;
> +    int agentstart = 0;
>  
>      if (ctl->keepalive_interval >= 0) {
>          interval = ctl->keepalive_interval;
> @@ -153,10 +156,43 @@ virshConnect(vshControl *ctl, const char *uri, bool readonly)
>          keepalive_forced = true;
>      }
>  
> -    c = virConnectOpenAuth(uri, virConnectAuthPtrDefault,
> -                           readonly ? VIR_CONNECT_RO : 0);
> -    if (!c)
> -        return NULL;
> +    do {
> +        virErrorPtr err;
> +
> +        if ((c = virConnectOpenAuth(uri, virConnectAuthPtrDefault,
> +                                    readonly ? VIR_CONNECT_RO : 0)))
> +            break;
> +
> +        if (readonly)
> +            goto cleanup;
> +
> +        err = virGetLastError();
> +        if (err && strstr(err->message,
> +                          _("no agent is available to authenticate"))) {

> +            if (!pkagent) {
> +                if (!(pkagent = virPolkitAgentCreate()))
> +                    goto cleanup;
> +            }
> +            agentstart++;
> +        } else if (err && strstr(err->message, _("authentication failed:"))) {

String matching is pretty unpleasant. I think we can match on
err->domain == VIR_FROM_POLKIT && err->code == VIR_ERR_AUTH_FAILED
for this.

Also instead of trying to match for the agent message, you can just
do

  if (!virDBusIsServiceRegistered('....polkit service name....'))

to decide whether to then start the agent after an auth failure


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|