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@xxxxxxxxxx>
>> ---
>> 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
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list