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

[John Ferlan]

On 02/11/2016 05:02 AM, Daniel P. Berrange wrote:
> On Wed, Feb 10, 2016 at 02:46:35PM -0500, John Ferlan wrote:
>> Introduce virPolkitAgentCreate, virPolkitAgentCheck, and virPolkitAgentDestroy
>>
>> virPolkitAgentCreate will run the polkit pkttyagent image as an asynchronous
>> command in order to handle the local agent authentication via stdin/stdout.
>>
>> virPolkitAgentCheck will run the polkit pkcheck command against the async
>> command process in order to perform the authentication
> 
> Err, we already have virPolkitCheckAuth which does this via the DBus
> API. Using pkcheck is a security flaw in many versions of Linux because
> it didn't accept the full set of args required for race-free auth
> checking.
> 

oh - right.  duh.

>> 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.
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>>  src/libvirt_private.syms |  3 ++
>>  src/util/virpolkit.c     | 96 +++++++++++++++++++++++++++++++++++++++++++++++-
>>  src/util/virpolkit.h     |  7 ++++
>>  tests/virpolkittest.c    |  3 +-
>>  4 files changed, 107 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
>> index 5ae3618..e4d791d 100644
>> --- a/src/libvirt_private.syms
>> +++ b/src/libvirt_private.syms
>> @@ -2028,6 +2028,9 @@ virPidFileWritePath;
>>  
>>  
>>  # util/virpolkit.h
>> +virPolkitAgentCheck;
>> +virPolkitAgentCreate;
>> +virPolkitAgentDestroy;
>>  virPolkitCheckAuth;
>>  
>>  
>> diff --git a/src/util/virpolkit.c b/src/util/virpolkit.c
>> index 56b1c31..e7c8603 100644
>> --- a/src/util/virpolkit.c
>> +++ b/src/util/virpolkit.c
>> @@ -26,8 +26,8 @@
>>  # 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"
>> @@ -136,6 +136,77 @@ 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 outfd = STDOUT_FILENO;
>> +    int errfd = STDERR_FILENO;
>> +
>> +    virCommandAddArgFormat(cmd, "%lld", (long long int) getpid());
>> +    virCommandAddArg(cmd, "--fallback");
>> +    virCommandSetInputFD(cmd, STDIN_FILENO);
>> +    virCommandSetOutputFD(cmd, &outfd);
>> +    virCommandSetErrorFD(cmd, &errfd);
>> +    if (virCommandRunAsync(cmd, NULL) < 0)
>> +        goto error;
>> +    /* Give it a chance to get started */
>> +    sleep(1);
> 
> Sigh, needing a sleep(1) is a sure sign that we'd be better off
> doing this via the Polkit API and not spawning programs. This
> sleep is just asking for bug reports about race conditions.
> 

So after more than a few cycles spent trying to figure out the build
system - I came back to this code...

I found that by changing this to a usleep(10 * 1000); also did the trick
(although 1 * 1000 fails).

Of course if I change the virsh code to utilize a retry loop that checks
on "no agent is available to authenticate", then the sleep isn't
necessary, although it could cut down on the retries...

I'll send out something shortly.

John

>> +
>> +    return cmd;
>> +
>> + error:
>> +    virCommandFree(cmd);
>> +    return NULL;
>> +}
> 
> Regards,
> Daniel
>