[libvirt] [PATCH 0/3] Add capability for text based polkit authentication for virsh
https://bugzilla.redhat.com/show_bug.cgi?id=872166 As an alternative to commit id 'e94979e90' which allows polkit authentication by adding users to the 'libvirt' group, add the ability to start and utilize a text based authentication agent for virsh. At the very least patch 1 will suffice part of the issue listed in the bz - the opaque error message related to "some agent". For patch 2, it was far easier to utilize what polkit provides in pkttyagent and pkcheck utilities, than adding some code which requires POLKIT_AGENT_I_KNOW_API_IS_SUBJECT_TO_CHANGE being #defined for compilation. I chose 'pkauth' to mean polkit authentication - figured it was a workable shorthand, but if there's better suggestions those can be considered. John Ferlan (3): polkit: Adjust message when action-id isn't found util: Introduce API's for Polkit text authentication virsh: Add support for text based polkit authentication src/libvirt_private.syms | 3 ++ src/util/virpolkit.c | 104 +++++++++++++++++++++++++++++++++++++++++++++-- src/util/virpolkit.h | 7 ++++ tests/virpolkittest.c | 3 +- tools/virsh.c | 22 ++++++++-- tools/virsh.h | 2 + tools/virsh.pod | 10 +++++ tools/vsh.h | 1 + 8 files changed, 144 insertions(+), 8 deletions(-) -- 2.5.0
When there isn't a ssh -X type session running and a user has not been added to the libvirt group, attempts to run 'virsh -c qemu:///system' commands from an otherwise unprivileged user will fail with rather generic or opaque error message: "error: authentication failed: no agent is available to authenticate" This patch will adjust the error message to help reflect not only that it was a polkit agent, but which 'action-id' was not found. It does so carefully, by adding to the text rather than adjusting it since virpolkittest is looking for a specific string. The result on a failure then becomes: "error: authentication failed: no agent is available to authenticate - polkit agent 'org.libvirt.unix.manage' is not available" A bit more history on this - at one time a failure generated the following type message when running the 'pkcheck' as a subprocess: "error: authentication failed: polkit\56retains_authorization_after_challenge=1 Authorization requires authentication but no agent is available." but, a patch was generated to adjust the error message to help provide more details about what failed. This was pushed as commit id '96a108c99'. That patch prepended a "polkit: " to the output. It really didn't solve the problem, but gave a hint. After some time it was deemed using DBus API calls directly was a better way to go (since pkcheck calls them anyway). So, commit id '1b854c76' (more or less) copied the code from remoteDispatchAuthPolkit and adjusted it. Then commit id 'c7542573' adjusted the remote.c code to call the new API (virPolkitCheckAuth). Finally, commit id '308c0c5a' altered the code to call DBus APIs directly. In doing so, it reverted the failing error message to the generic message that would have been received from DBus anyway. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virpolkit.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/util/virpolkit.c b/src/util/virpolkit.c index 8da91f2..56b1c31 100644 --- a/src/util/virpolkit.c +++ b/src/util/virpolkit.c @@ -1,7 +1,7 @@ /* * virpolkit.c: helpers for using polkit APIs * - * Copyright (C) 2013, 2014 Red Hat, Inc. + * Copyright (C) 2013, 2014, 2016 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -121,8 +121,10 @@ int virPolkitCheckAuth(const char *actionid, virReportError(VIR_ERR_AUTH_CANCELLED, "%s", _("user cancelled authentication process")); else if (is_challenge) - virReportError(VIR_ERR_AUTH_FAILED, "%s", - _("no agent is available to authenticate")); + virReportError(VIR_ERR_AUTH_FAILED, + _("no agent is available to authenticate - " + "polkit agent '%s' is not available"), + actionid); else virReportError(VIR_ERR_AUTH_FAILED, "%s", _("access denied by policy")); -- 2.5.0
On Wed, Feb 10, 2016 at 02:46:34PM -0500, John Ferlan wrote:
When there isn't a ssh -X type session running and a user has not been added to the libvirt group, attempts to run 'virsh -c qemu:///system' commands from an otherwise unprivileged user will fail with rather generic or opaque error message:
"error: authentication failed: no agent is available to authenticate"
This patch will adjust the error message to help reflect not only that it was a polkit agent, but which 'action-id' was not found. It does so carefully, by adding to the text rather than adjusting it since virpolkittest is looking for a specific string. The result on a failure then becomes:
"error: authentication failed: no agent is available to authenticate - polkit agent 'org.libvirt.unix.manage' is not available"
A bit more history on this - at one time a failure generated the following type message when running the 'pkcheck' as a subprocess:
"error: authentication failed: polkit\56retains_authorization_after_challenge=1 Authorization requires authentication but no agent is available."
but, a patch was generated to adjust the error message to help provide more details about what failed. This was pushed as commit id '96a108c99'. That patch prepended a "polkit: " to the output. It really didn't solve the problem, but gave a hint.
After some time it was deemed using DBus API calls directly was a better way to go (since pkcheck calls them anyway). So, commit id '1b854c76' (more or less) copied the code from remoteDispatchAuthPolkit and adjusted it. Then commit id 'c7542573' adjusted the remote.c code to call the new API (virPolkitCheckAuth). Finally, commit id '308c0c5a' altered the code to call DBus APIs directly. In doing so, it reverted the failing error message to the generic message that would have been received from DBus anyway.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virpolkit.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/src/util/virpolkit.c b/src/util/virpolkit.c index 8da91f2..56b1c31 100644 --- a/src/util/virpolkit.c +++ b/src/util/virpolkit.c @@ -1,7 +1,7 @@ /* * virpolkit.c: helpers for using polkit APIs * - * Copyright (C) 2013, 2014 Red Hat, Inc. + * Copyright (C) 2013, 2014, 2016 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -121,8 +121,10 @@ int virPolkitCheckAuth(const char *actionid, virReportError(VIR_ERR_AUTH_CANCELLED, "%s", _("user cancelled authentication process")); else if (is_challenge) - virReportError(VIR_ERR_AUTH_FAILED, "%s", - _("no agent is available to authenticate")); + virReportError(VIR_ERR_AUTH_FAILED, + _("no agent is available to authenticate - " + "polkit agent '%s' is not available"), + actionid);
That message doesn't make sense I'm afraid. The 'actionid' is referring to a permission name, not an agent name. 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 :|
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 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); + + return cmd; + + error: + virCommandFree(cmd); + return NULL; +} + + +/* virPolkitAgentCheck: + * + * Execute the pkcheck program against the running Polkit Agent + * + * returns 0 on success and -1 on failure. + */ +int +virPolkitAgentCheck(void) +{ + int ret = -1; + virCommandPtr cmd = virCommandNewArgList(PKCHECK, + "--action-id", + "org.libvirt.unix.manage", + "--process", + NULL); + + virCommandAddArgFormat(cmd, "%lld,0,%u", + (long long int) getpid(), (unsigned int) getuid()); + virCommandAddArg(cmd, "--allow-user-interaction"); + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; + + ret = 0; + cleanup: + virCommandFree(cmd); + return ret; +} + + #elif WITH_POLKIT0 int virPolkitCheckAuth(const char *actionid, pid_t pid, @@ -254,4 +325,27 @@ 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; +} + + +int +virPolkitAgentCheck(void) +{ + virReportError(VIR_ERR_AUTH_FAILED, "%s", + _("polkit text authentication agent unavailable")); + return -1; +} #endif /* WITH_POLKIT1 */ diff --git a/src/util/virpolkit.h b/src/util/virpolkit.h index 36122d0..fcfc855 100644 --- a/src/util/virpolkit.h +++ b/src/util/virpolkit.h @@ -24,6 +24,9 @@ # include "internal.h" +# define PKTTYAGENT "/usr/bin/pkttyagent" +# define PKCHECK "/usr/bin/pkcheck" + int virPolkitCheckAuth(const char *actionid, pid_t pid, unsigned long long startTime, @@ -31,4 +34,8 @@ int virPolkitCheckAuth(const char *actionid, const char **details, bool allowInteraction); +void virPolkitAgentDestroy(virCommandPtr cmd); +virCommandPtr virPolkitAgentCreate(void); +int virPolkitAgentCheck(void); + #endif /* __VIR_POLKIT_H__ */ diff --git a/tests/virpolkittest.c b/tests/virpolkittest.c index cdf78f5..b5bebf9 100644 --- a/tests/virpolkittest.c +++ b/tests/virpolkittest.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2013, 2014 Red Hat, Inc. + * Copyright (C) 2013, 2014, 2016 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -27,6 +27,7 @@ # include <stdlib.h> # include <dbus/dbus.h> +# include "vircommand.h" # include "virpolkit.h" # include "virdbus.h" # include "virlog.h" -- 2.5.0
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.
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.
+ + return cmd; + + error: + virCommandFree(cmd); + return NULL; +}
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 :|
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
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 - polkit agent 'org.libvirt.unix.manage' is not available In order to handle the local authentication, add a new switch "--pkauth" to virsh which will make use of the virPolkitAgent* API's in order to handle the text authentication when not a readonly request. With this patch in place, the following occurs: $ virsh --pkauth -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@redhat.com> --- tools/virsh.c | 22 +++++++++++++++++++--- tools/virsh.h | 2 ++ tools/virsh.pod | 10 ++++++++++ tools/vsh.h | 1 + 4 files changed, 32 insertions(+), 3 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index b96dbda..83cebe0 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -143,6 +143,7 @@ virshConnect(vshControl *ctl, const char *uri, bool readonly) int interval = 5; /* Default */ int count = 6; /* Default */ bool keepalive_forced = false; + virCommandPtr pkagent = NULL; if (ctl->keepalive_interval >= 0) { interval = ctl->keepalive_interval; @@ -153,10 +154,17 @@ virshConnect(vshControl *ctl, const char *uri, bool readonly) keepalive_forced = true; } + if (!readonly && ctl->pkauth) { + if (!(pkagent = virPolkitAgentCreate())) + return NULL; + if (virPolkitAgentCheck() < 0) + goto cleanup; + } + c = virConnectOpenAuth(uri, virConnectAuthPtrDefault, readonly ? VIR_CONNECT_RO : 0); if (!c) - return NULL; + goto cleanup; if (interval > 0 && virConnectSetKeepAlive(c, interval, count) != 0) { @@ -165,12 +173,15 @@ virshConnect(vshControl *ctl, const char *uri, bool readonly) _("Cannot setup keepalive on connection " "as requested, disconnecting")); virConnectClose(c); - return NULL; + c = NULL; + goto cleanup; } vshDebug(ctl, VSH_ERR_INFO, "%s", _("Failed to setup keepalive on connection\n")); } + cleanup: + virPolkitAgentDestroy(pkagent); return c; } @@ -490,6 +501,7 @@ virshUsage(void) " -K | --keepalive-count=NUM\n" " number of possible missed keepalive messages\n" " -l | --log=FILE output logging to file\n" + " -p | --pkauth start background polkit text authentication agent\n" " -q | --quiet quiet mode\n" " -r | --readonly connect readonly\n" " -t | --timing print timing information\n" @@ -701,13 +713,14 @@ virshParseArgv(vshControl *ctl, int argc, char **argv) {"readonly", no_argument, NULL, 'r'}, {"timing", no_argument, NULL, 't'}, {"version", optional_argument, NULL, 'v'}, + {"pkauth", no_argument, NULL, 'p'}, {NULL, 0, NULL, 0} }; /* Standard (non-command) options. The leading + ensures that no * argument reordering takes place, so that command options are * not confused with top-level virsh options. */ - while ((arg = getopt_long(argc, argv, "+:c:d:e:hk:K:l:qrtvV", opt, &longindex)) != -1) { + while ((arg = getopt_long(argc, argv, "+:c:d:e:hk:K:l:pqrtvV", opt, &longindex)) != -1) { switch (arg) { case 'c': VIR_FREE(ctl->connname); @@ -779,6 +792,9 @@ virshParseArgv(vshControl *ctl, int argc, char **argv) ctl->logfile = vshStrdup(ctl, optarg); vshOpenLogFile(ctl); break; + case 'p': + ctl->pkauth = true; + break; case 'q': ctl->quiet = true; break; diff --git a/tools/virsh.h b/tools/virsh.h index 8b5e5ba..a6c7289 100644 --- a/tools/virsh.h +++ b/tools/virsh.h @@ -36,6 +36,8 @@ # include "internal.h" # include "virerror.h" # include "virthread.h" +# include "vircommand.h" +# include "virpolkit.h" # include "vsh.h" # define VIRSH_PROMPT_RW "virsh # " diff --git a/tools/virsh.pod b/tools/virsh.pod index 435c649..5368de7 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -90,6 +90,16 @@ no effect to this setting in case the I<INTERVAL> is set to 0. Output logging details to I<FILE>. +=item B<-p>, B<--pkauth> + +Start a local Polkit Authentication Agent in order to authenticate an +unprivileged user for write access from a text session that cannot +autolaunch a graphical Polkit Agent. Using this mechanism is an +alternative to adding the user to the libvirt group list in /etc/group. +The authentication processing requires the ability to view stdout and +read from stdin. If Polkit is not available on the host, then the +command will fail. + =item B<-q>, B<--quiet> Avoid extra informational messages. diff --git a/tools/vsh.h b/tools/vsh.h index 0c5abdc..151fdcc 100644 --- a/tools/vsh.h +++ b/tools/vsh.h @@ -202,6 +202,7 @@ struct _vshControl { vshCmd *cmd; /* the current command */ char *cmdstr; /* string with command */ bool imode; /* interactive mode? */ + bool pkauth; /* start polkit authentication agent */ bool quiet; /* quiet mode */ bool timing; /* print timing info? */ int debug; /* print debug messages? */ -- 2.5.0
On Wed, Feb 10, 2016 at 02:46:36PM -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 - polkit agent 'org.libvirt.unix.manage' is not available
In order to handle the local authentication, add a new switch "--pkauth" to virsh which will make use of the virPolkitAgent* API's in order to handle the text authentication when not a readonly request.
With this patch in place, the following occurs:
$ virsh --pkauth -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@redhat.com> --- tools/virsh.c | 22 +++++++++++++++++++--- tools/virsh.h | 2 ++ tools/virsh.pod | 10 ++++++++++ tools/vsh.h | 1 + 4 files changed, 32 insertions(+), 3 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index b96dbda..83cebe0 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -143,6 +143,7 @@ virshConnect(vshControl *ctl, const char *uri, bool readonly) int interval = 5; /* Default */ int count = 6; /* Default */ bool keepalive_forced = false; + virCommandPtr pkagent = NULL;
if (ctl->keepalive_interval >= 0) { interval = ctl->keepalive_interval; @@ -153,10 +154,17 @@ virshConnect(vshControl *ctl, const char *uri, bool readonly) keepalive_forced = true; }
+ if (!readonly && ctl->pkauth) { + if (!(pkagent = virPolkitAgentCreate())) + return NULL; + if (virPolkitAgentCheck() < 0) + goto cleanup;
I'm not sure why you need to run pkcheck here - the auth check is something the server side does - it doesn't make sense for the client to be doing that.
@@ -701,13 +713,14 @@ virshParseArgv(vshControl *ctl, int argc, char **argv) {"readonly", no_argument, NULL, 'r'}, {"timing", no_argument, NULL, 't'}, {"version", optional_argument, NULL, 'v'}, + {"pkauth", no_argument, NULL, 'p'},
I think it would nice if we did without this extra argument and made things "just work". The way polkit agents work is that they have to claim a well known path on the system dbus instance. If we connect to DBus and check for existance of the "/org/freedesktop/PolicyKit1/AuthenticationAgent" we will know whether we should try to provide an auth agent or not. That said I wonder if there's cause to support a scenario where polkit agent is not running and people want to run virsh without the agent still. 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 :|
On Wed, Feb 10, 2016 at 02:46:33PM -0500, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=872166
As an alternative to commit id 'e94979e90' which allows polkit authentication by adding users to the 'libvirt' group, add the ability to start and utilize a text based authentication agent for virsh.
At the very least patch 1 will suffice part of the issue listed in the bz - the opaque error message related to "some agent".
For patch 2, it was far easier to utilize what polkit provides in pkttyagent and pkcheck utilities, than adding some code which requires POLKIT_AGENT_I_KNOW_API_IS_SUBJECT_TO_CHANGE being #defined for compilation.
Sigh, that define is a bit of a bad joke really. polkit was first added in Fedora 12, and comparing the header files between then and now, they've never broken their ABI. They're merely added new APIs. IMHO, we can just define that, and use the API from libvirt without trouble.
I chose 'pkauth' to mean polkit authentication - figured it was a workable shorthand, but if there's better suggestions those can be considered.
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 :|
On 02/11/2016 05:11 AM, Daniel P. Berrange wrote:
On Wed, Feb 10, 2016 at 02:46:33PM -0500, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=872166
As an alternative to commit id 'e94979e90' which allows polkit authentication by adding users to the 'libvirt' group, add the ability to start and utilize a text based authentication agent for virsh.
At the very least patch 1 will suffice part of the issue listed in the bz - the opaque error message related to "some agent".
For patch 2, it was far easier to utilize what polkit provides in pkttyagent and pkcheck utilities, than adding some code which requires POLKIT_AGENT_I_KNOW_API_IS_SUBJECT_TO_CHANGE being #defined for compilation.
Sigh, that define is a bit of a bad joke really. polkit was first added in Fedora 12, and comparing the header files between then and now, they've never broken their ABI. They're merely added new APIs. IMHO, we can just define that, and use the API from libvirt without trouble.
I had code generated that tried to use those API's, but couldn't find the correct magic incantation to convince the build to find the polkitagent/polkitagent.h file. #define POLKIT_AGENT_I_KNOW_API_IS_SUBJECT_TO_CHANGE #include <polkitagent/polkitagent.h> ... util/virpolkit.c:30:37: fatal error: polkitagent/polkitagent.h: No such file or directory ... /usr/include/polkit-1/polkitagent/polkitagent.h That is, how do I ensure that somehow automagically add that -I/usr/include/polkit-1 ? I did try to "follow" examples of adding POLKIT_AGENT_CFLAGS and POLKIT_AGENT_LIBS to configure.ac and src/Makefile.am, but still no luck. Tks - John
I chose 'pkauth' to mean polkit authentication - figured it was a workable shorthand, but if there's better suggestions those can be considered.
Regards, Daniel
On Thu, Feb 11, 2016 at 12:22:12PM -0500, John Ferlan wrote:
On 02/11/2016 05:11 AM, Daniel P. Berrange wrote:
On Wed, Feb 10, 2016 at 02:46:33PM -0500, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=872166
As an alternative to commit id 'e94979e90' which allows polkit authentication by adding users to the 'libvirt' group, add the ability to start and utilize a text based authentication agent for virsh.
At the very least patch 1 will suffice part of the issue listed in the bz - the opaque error message related to "some agent".
For patch 2, it was far easier to utilize what polkit provides in pkttyagent and pkcheck utilities, than adding some code which requires POLKIT_AGENT_I_KNOW_API_IS_SUBJECT_TO_CHANGE being #defined for compilation.
Sigh, that define is a bit of a bad joke really. polkit was first added in Fedora 12, and comparing the header files between then and now, they've never broken their ABI. They're merely added new APIs. IMHO, we can just define that, and use the API from libvirt without trouble.
I had code generated that tried to use those API's, but couldn't find the correct magic incantation to convince the build to find the polkitagent/polkitagent.h file.
#define POLKIT_AGENT_I_KNOW_API_IS_SUBJECT_TO_CHANGE #include <polkitagent/polkitagent.h>
... util/virpolkit.c:30:37: fatal error: polkitagent/polkitagent.h: No such file or directory ...
/usr/include/polkit-1/polkitagent/polkitagent.h
That is, how do I ensure that somehow automagically add that -I/usr/include/polkit-1 ?
I did try to "follow" examples of adding POLKIT_AGENT_CFLAGS and POLKIT_AGENT_LIBS to configure.ac and src/Makefile.am, but still no luck.
If you show your complete patch for this, I can take a look and see what's missing 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 :|
On 02/11/2016 12:45 PM, Daniel P. Berrange wrote:
On Thu, Feb 11, 2016 at 12:22:12PM -0500, John Ferlan wrote:
On 02/11/2016 05:11 AM, Daniel P. Berrange wrote:
On Wed, Feb 10, 2016 at 02:46:33PM -0500, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=872166
As an alternative to commit id 'e94979e90' which allows polkit authentication by adding users to the 'libvirt' group, add the ability to start and utilize a text based authentication agent for virsh.
At the very least patch 1 will suffice part of the issue listed in the bz - the opaque error message related to "some agent".
For patch 2, it was far easier to utilize what polkit provides in pkttyagent and pkcheck utilities, than adding some code which requires POLKIT_AGENT_I_KNOW_API_IS_SUBJECT_TO_CHANGE being #defined for compilation.
Sigh, that define is a bit of a bad joke really. polkit was first added in Fedora 12, and comparing the header files between then and now, they've never broken their ABI. They're merely added new APIs. IMHO, we can just define that, and use the API from libvirt without trouble.
I had code generated that tried to use those API's, but couldn't find the correct magic incantation to convince the build to find the polkitagent/polkitagent.h file.
#define POLKIT_AGENT_I_KNOW_API_IS_SUBJECT_TO_CHANGE #include <polkitagent/polkitagent.h>
... util/virpolkit.c:30:37: fatal error: polkitagent/polkitagent.h: No such file or directory ...
/usr/include/polkit-1/polkitagent/polkitagent.h
That is, how do I ensure that somehow automagically add that -I/usr/include/polkit-1 ?
I did try to "follow" examples of adding POLKIT_AGENT_CFLAGS and POLKIT_AGENT_LIBS to configure.ac and src/Makefile.am, but still no luck.
If you show your complete patch for this, I can take a look and see what's missing
There's really not much to show... I took src/util/polkit.c and added: #define POLKIT_AGENT_I_KNOW_API_IS_SUBJECT_TO_CHANGE #include <polkitagent/polkitagent.h> For configure.ac, I tried adding : POLKIT_AGENT_CFLAGS= POLKIT_AGENT_LIBS= Under the existing POLKIT_CFLAGS= POLKIT_LIBS= and AC_SUBST([POLKIT_AGENT_CFLAGS]) AC_SUBST([POLKIT_AGENT_LIBS]) after AC_SUBST([POLKIT_CFLAGS]) AC_SUBST([POLKIT_LIBS]) Then in src/Makefile.am I added $(POLKIT_AGENT_CFLAGS) after $(POLKIT_CFLAGS) and $(POLKIT_AGENT_LIBS) after $(POLKIT_LIBS) I usually try to avoid the configure.ac and Makefile.am - it's all black magic to me. Happy when it works... John
participants (2)
-
Daniel P. Berrange -
John Ferlan