[libvirt] [PATCH v3 2/3] util: Introduce API's for Polkit text authentication

[John Ferlan]

On 02/25/2016 09:25 AM, Daniel P. Berrange wrote:
> On Fri, Feb 12, 2016 at 12:12:32PM -0500, John Ferlan wrote:
>> Introduce virPolkitAgentCreate and virPolkitAgentDestroy
>>
>> virPolkitAgentCreate will run the polkit pkttyagent image as an asynchronous
>> command in order to handle the local agent authentication via stdin/stdout.
>> The code makes use of the pkttyagent --notify-fd mechanism to let it know
>> when the agent is successfully registered.
>>
>> virPolkitAgentDestroy will close the command effectively reaping our
>> child process
>>
>> Needed to move around or add the "#include vircommand.h" since,
>> virpolkit.h now uses it.

This part of the commit message was removed with Martin's request/change.

>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>>  src/libvirt_private.syms |  2 ++
>>  src/util/virpolkit.c     | 77 +++++++++++++++++++++++++++++++++++++++++++++++-
>>  src/util/virpolkit.h     |  5 ++++
>>  tests/virpolkittest.c    |  1 +
>>  4 files changed, 84 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index 4cfaed5..8f2358f 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -2029,6 +2029,8 @@ virPidFileWritePath;
>>  
>>  
>>  # util/virpolkit.h
>> +virPolkitAgentCreate;
>> +virPolkitAgentDestroy;
>>  virPolkitCheckAuth;
>>  
>>  
>> diff --git a/src/util/virpolkit.c b/src/util/virpolkit.c
>> index df707f1..6941d74 100644
>> --- a/src/util/virpolkit.c
>> +++ b/src/util/virpolkit.c
>> @@ -20,20 +20,22 @@
>>   */
>>  
>>  #include <config.h>
>> +#include <poll.h>
>>  
>>  #if WITH_POLKIT0
>>  # include <polkit/polkit.h>
>>  # include <polkit-dbus/polkit-dbus.h>
>>  #endif
>>  
>> -#include "virpolkit.h"
>>  #include "vircommand.h"
>> +#include "virpolkit.h"
>>  #include "virerror.h"
>>  #include "virlog.h"
>>  #include "virstring.h"
>>  #include "virprocess.h"
>>  #include "viralloc.h"
>>  #include "virdbus.h"
>> +#include "virfile.h"
>>  
>>  #define VIR_FROM_THIS VIR_FROM_POLKIT
>>  
>> @@ -136,6 +138,65 @@ int virPolkitCheckAuth(const char *actionid,
>>  }
>>  
>>  
>> +/* virPolkitAgentDestroy:
>> + * @cmd: Pointer to the virCommandPtr created during virPolkitAgentCreate
>> + *
>> + * Destroy resources used by Polkit Agent
>> + */
>> +void
>> +virPolkitAgentDestroy(virCommandPtr cmd)
>> +{
>> +    virCommandFree(cmd);
>> +}
>> +
>> +/* virPolkitAgentCreate:
>> + *
>> + * Allocate and setup a polkit agent
>> + *
>> + * Returns a virCommandPtr on success and NULL on failure
>> + */
>> +virCommandPtr
>> +virPolkitAgentCreate(void)
>> +{
>> +    virCommandPtr cmd = virCommandNewArgList(PKTTYAGENT, "--process", NULL);
>> +    int pipe_fd[2] = {-1, -1};
>> +    struct pollfd pollfd;
>> +    int outfd = STDOUT_FILENO;
>> +    int errfd = STDERR_FILENO;
>> +
>> +    if (!isatty(STDIN_FILENO))
>> +        goto error;
>> +
>> +    if (pipe2(pipe_fd, 0) < 0)
>> +        goto error;
>> +
>> +    virCommandAddArgFormat(cmd, "%lld", (long long int) getpid());
>> +    virCommandAddArg(cmd, "--notify-fd");
>> +    virCommandAddArgFormat(cmd, "%d", pipe_fd[1]);
>> +    virCommandAddArg(cmd, "--fallback");
>> +    virCommandSetInputFD(cmd, STDIN_FILENO);
>> +    virCommandSetOutputFD(cmd, &outfd);
>> +    virCommandSetErrorFD(cmd, &errfd);
>> +    virCommandPassFD(cmd, pipe_fd[1], VIR_COMMAND_PASS_FD_CLOSE_PARENT);
>> +    if (virCommandRunAsync(cmd, NULL) < 0)
>> +        goto error;
>> +
>> +    pollfd.fd = pipe_fd[0];
>> +    pollfd.events = POLLHUP;
>> +
>> +    if (poll(&pollfd, 1, -1) < 0)
>> +        goto error;
>> +
>> +    return cmd;
>> +
>> + error:
>> +    VIR_FORCE_CLOSE(pipe_fd[0]);
>> +    VIR_FORCE_CLOSE(pipe_fd[1]);
>> +    virCommandFree(cmd);
>> +    return NULL;
>> +}
>> +
>> +
>>  #elif WITH_POLKIT0
>>  int virPolkitCheckAuth(const char *actionid,
>>                         pid_t pid,
>> @@ -254,4 +315,18 @@ int virPolkitCheckAuth(const char *actionid ATTRIBUTE_UNUSED,
>>  }
>>  
>>  
>> +void
>> +virPolkitAgentDestroy(virCommandPtr cmd ATTRIBUTE_UNUSED)
>> +{
>> +    return; /* do nothing */
>> +}
>> +
>> +
>> +virCommandPtr
>> +virPolkitAgentCreate(void)
>> +{
>> +    virReportError(VIR_ERR_AUTH_FAILED, "%s",
>> +                   _("polkit text authentication agent unavailable"));
>> +    return NULL;
>> +}
>>  #endif /* WITH_POLKIT1 */
>> diff --git a/src/util/virpolkit.h b/src/util/virpolkit.h
>> index 36122d0..f0aea37 100644
>> --- a/src/util/virpolkit.h
>> +++ b/src/util/virpolkit.h
>> @@ -24,6 +24,8 @@
>>  
>>  # include "internal.h"

Based on Martin's comments - I included "vircommand.h" here

>>  
>> +# define PKTTYAGENT "/usr/bin/pkttyagent"
>> +
>>  int virPolkitCheckAuth(const char *actionid,
>>                         pid_t pid,
>>                         unsigned long long startTime,
>> @@ -31,4 +33,7 @@ int virPolkitCheckAuth(const char *actionid,
>>                         const char **details,
>>                         bool allowInteraction);
>>  
>> +void virPolkitAgentDestroy(virCommandPtr cmd);
>> +virCommandPtr virPolkitAgentCreate(void);
> 
> Rather than exposing use of virCommand in the API, I'd
> suggest you create a
> 
> 
>  typedef struct virPolkitAgent virPolkitAgent;
>  typedef virPolkitAgent *virPolkitAgentPtr;

okidoke... Funny I had done it this way at some point, but when
virCommandPtr was the only thing in the structure, I just opted to use
virCommandPtr directly.  Anyway, the following is now defined:

typedef struct _virPolkitAgent virPolkitAgent;
typedef virPolkitAgent *virPolkitAgentPtr;

void virPolkitAgentDestroy(virPolkitAgentPtr cmd);
virPolkitAgentPtr virPolkitAgentCreate(void);

> 
> in the header file here
> 
> and in the .c do
> 
>   struct virPolitAgent {
>       virCommandPtr;
>   };
> 

Inside the "#if WITH_POLKIT1" there is now a :


struct _virPolkitAgent {
    virCommandPtr cmd;
};

results in mods to virPolkitAgentDestroy:

virPolkitAgentDestroy(virPolkitAgentPtr agent)
{
    if (!agent)
        return;

    virCommandFree(agent->cmd);
    VIR_FREE(agent);
}

and the VIR_ALLOC(agent) in virPolkitAgentCreate as well as calling
virPolkitAgentDestroy(agent) instead of virCommandFree in error:

> so we hide use of virCommand
> 
> 
>> diff --git a/tests/virpolkittest.c b/tests/virpolkittest.c
>> index 73f001b..b46e65f 100644
>> --- a/tests/virpolkittest.c
>> +++ b/tests/virpolkittest.c
>> @@ -27,6 +27,7 @@
>>  # include <stdlib.h>
>>  # include <dbus/dbus.h>
>>  
>> +# include "vircommand.h"

Forgot to note in my response to Martin that virpolkittest.c doesn't
need a change here since virpolkit.h now includes vircommand.h.  Same of
course for virsh.h.

Should I post a v4 of patches 2 & 3?

I'll still wait for the release to push the patches though.

Tks -

John