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@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