1:46 p.m.
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
1:46 p.m.
New subject: [libvirt] [PATCH 1/3] polkit: Adjust message when action-id isn't found
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(a)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
3:53 a.m.
New subject: [libvirt] [PATCH 1/3] polkit: Adjust message when action-id isn't found
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(a)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 :|
1:46 p.m.
New subject: [libvirt] [PATCH 2/3] util: Introduce API's for Polkit text authentication
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(a)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
4:02 a.m.
New subject: [libvirt] [PATCH 2/3] util: Introduce API's for Polkit text authentication
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(a)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 :|
5:31 p.m.
New subject: [libvirt] [PATCH 2/3] util: Introduce API's for Polkit text authentication
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(a)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
1:46 p.m.
New subject: [libvirt] [PATCH 3/3] virsh: Add support for text based polkit authentication
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(a)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
4:05 a.m.
New subject: [libvirt] [PATCH 3/3] virsh: Add support for text based polkit authentication
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(a)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 :|
4:11 a.m.
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 :|
11:22 a.m.
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
11:45 a.m.
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 :|
11:53 a.m.
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
3207
days inactive
3208
days old
11 comments
2 participants
participants (2)
-
Daniel P. Berrange -
John Ferlan