12:38 a.m.
v1: http://www.redhat.com/archives/libvir-list/2016-February/msg00545.html
Differences to v1:
- Patch 1, adjust error message slightly
- Patch 2, remove the unnecessary virPolkitAgentCheck and the sleep
- Patch 3, remove the --pkauth parameter
adjust the logic to attempt the authentications in a loop
with the first being one for the graphical agent. If that
fails because there was no agent to be found, then start
up the text based agent. Also add logic to wait for the
text based authentication agent to start (just in case).
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 | 2 ++
src/util/virpolkit.c | 64 +++++++++++++++++++++++++++++++++++++++++++++---
src/util/virpolkit.h | 5 ++++
tests/virpolkittest.c | 3 ++-
tools/virsh.c | 49 ++++++++++++++++++++++++++++++++----
tools/virsh.h | 2 ++
6 files changed, 115 insertions(+), 10 deletions(-)
--
2.5.0
12:38 a.m.
New subject: [libvirt] [PATCH v2 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 -
no polkit agent for action 'org.libvirt.unix.manage'"
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..d837a14 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 - "
+ "no polkit agent for action '%s'"),
+ actionid);
else
virReportError(VIR_ERR_AUTH_FAILED, "%s",
_("access denied by policy"));
--
2.5.0
10:47 a.m.
New subject: [libvirt] [PATCH v2 1/3] polkit: Adjust message when action-id isn't found
On Thu, Feb 11, 2016 at 06:38:12PM -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 -
no polkit agent for action 'org.libvirt.unix.manage'"
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..d837a14 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 - "
+ "no polkit agent for action '%s'"),
This is a littled repetitive still. ACK if you make it say
"No polkit agent available to authenticate action '%s'"
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 :|
12:48 p.m.
New subject: [libvirt] [PATCH v2 1/3] polkit: Adjust message when action-id isn't found
On 02/12/2016 04:47 AM, Daniel P. Berrange wrote:
On Thu, Feb 11, 2016 at 06:38:12PM -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 -
> no polkit agent for action 'org.libvirt.unix.manage'"
>
> 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..d837a14 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 - "
> + "no polkit agent for action '%s'"),
This is a littled repetitive still. ACK if you make it say
"No polkit agent available to authenticate action '%s'"
OK - that will require a change to virpolkittest.c too (adding the
domain and code checks as well):
index 1ef7635..56f98b2 100644
--- a/tests/virpolkittest.c
+++ b/tests/virpolkittest.c
@@ -220,8 +220,9 @@ static int testPolkitAuthChallenge(const void
*opaque ATTRIBUTE_UNUSED)
}
err = virGetLastError();
- if (!err || !strstr(err->message,
- _("no agent is available to authenticate"))) {
+ if (!err || err->domain != VIR_FROM_POLKIT ||
+ err->code != VIR_ERR_AUTH_FAILED || !strstr(err->message,
+ _("no polkit agent available to authenticate"))) {
fprintf(stderr, "Incorrect error response\n");
goto cleanup;
}
Although my thoughts in reply to the patch3 review may need to tweak
this more...
John
12:38 a.m.
New subject: [libvirt] [PATCH v2 2/3] util: Introduce API's for Polkit text authentication
Introduce virPolkitAgentCreate and virPolkitAgentDestroy
virPolkitAgentCreate will run the polkit pkttyagent image as an asynchronous
command in order to handle the local agent authentication via stdin/stdout.
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 | 2 ++
src/util/virpolkit.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++-
src/util/virpolkit.h | 5 +++++
tests/virpolkittest.c | 3 ++-
4 files changed, 64 insertions(+), 2 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 4cfaed5..8f2358f 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -2029,6 +2029,8 @@ virPidFileWritePath;
# util/virpolkit.h
+virPolkitAgentCreate;
+virPolkitAgentDestroy;
virPolkitCheckAuth;
diff --git a/src/util/virpolkit.c b/src/util/virpolkit.c
index d837a14..48d214a 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,46 @@ 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;
+
+ return cmd;
+
+ error:
+ virCommandFree(cmd);
+ return NULL;
+}
+
+
#elif WITH_POLKIT0
int virPolkitCheckAuth(const char *actionid,
pid_t pid,
@@ -254,4 +294,18 @@ 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;
+}
#endif /* WITH_POLKIT1 */
diff --git a/src/util/virpolkit.h b/src/util/virpolkit.h
index 36122d0..f0aea37 100644
--- a/src/util/virpolkit.h
+++ b/src/util/virpolkit.h
@@ -24,6 +24,8 @@
# include "internal.h"
+# define PKTTYAGENT "/usr/bin/pkttyagent"
+
int virPolkitCheckAuth(const char *actionid,
pid_t pid,
unsigned long long startTime,
@@ -31,4 +33,7 @@ int virPolkitCheckAuth(const char *actionid,
const char **details,
bool allowInteraction);
+void virPolkitAgentDestroy(virCommandPtr cmd);
+virCommandPtr virPolkitAgentCreate(void);
+
#endif /* __VIR_POLKIT_H__ */
diff --git a/tests/virpolkittest.c b/tests/virpolkittest.c
index b39beed..3ccb779 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
12:38 a.m.
New subject: [libvirt] [PATCH v2 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 -
no polkit agent for action 'org.libvirt.unix.manage'
In order to handle the local authentication, we will use the new
virPolkitAgentCreate API in order to create a text based authentication
agent for our non readonly session to authenticate with.
The new code will execute in a loop allowing 5 failures to authenticate
before failing out. It also has a check for a failure to start the text
polkit authentication agent as testing as shown the agent startup isn't
necessarily immediate.
With this patch in place, the following occurs:
$ virsh -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 | 49 ++++++++++++++++++++++++++++++++++++++++++++-----
tools/virsh.h | 2 ++
2 files changed, 46 insertions(+), 5 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c
index b96dbda..c9da9f1 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -143,6 +143,9 @@ virshConnect(vshControl *ctl, const char *uri, bool readonly)
int interval = 5; /* Default */
int count = 6; /* Default */
bool keepalive_forced = false;
+ virCommandPtr pkagent = NULL;
+ int authfail = 0;
+ int agentstart = 0;
if (ctl->keepalive_interval >= 0) {
interval = ctl->keepalive_interval;
@@ -153,10 +156,43 @@ virshConnect(vshControl *ctl, const char *uri, bool readonly)
keepalive_forced = true;
}
- c = virConnectOpenAuth(uri, virConnectAuthPtrDefault,
- readonly ? VIR_CONNECT_RO : 0);
- if (!c)
- return NULL;
+ do {
+ virErrorPtr err;
+
+ if ((c = virConnectOpenAuth(uri, virConnectAuthPtrDefault,
+ readonly ? VIR_CONNECT_RO : 0)))
+ break;
+
+ if (readonly)
+ goto cleanup;
+
+ err = virGetLastError();
+ if (err && strstr(err->message,
+ _("no agent is available to authenticate"))) {
+ if (!pkagent) {
+ if (!(pkagent = virPolkitAgentCreate()))
+ goto cleanup;
+ }
+ agentstart++;
+ } else if (err && strstr(err->message, _("authentication
failed:"))) {
+ authfail++;
+ } else {
+ goto cleanup;
+ }
+ virResetLastError();
+ /* If we fail to authenticate 5 times or we fail to successfully
+ * connect with the agent after 10 attempts, then fail out.
+ * No sense prolonging the agony
+ */
+ } while (authfail < 5 && agentstart < 10);
+
+ if (!c) {
+ /* If we failed because we couldn't start the agent, give a reason */
+ if (agentstart == 10)
+ vshError(ctl, "%s",
+ _("Cannot setup a polkit text authentication agent"));
+ goto cleanup;
+ }
if (interval > 0 &&
virConnectSetKeepAlive(c, interval, count) != 0) {
@@ -165,12 +201,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;
}
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 # "
--
2.5.0
10:50 a.m.
New subject: [libvirt] [PATCH v2 3/3] virsh: Add support for text based polkit authentication
On Thu, Feb 11, 2016 at 06:38:14PM -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 -
no polkit agent for action 'org.libvirt.unix.manage'
In order to handle the local authentication, we will use the new
virPolkitAgentCreate API in order to create a text based authentication
agent for our non readonly session to authenticate with.
The new code will execute in a loop allowing 5 failures to authenticate
before failing out. It also has a check for a failure to start the text
polkit authentication agent as testing as shown the agent startup isn't
necessarily immediate.
With this patch in place, the following occurs:
$ virsh -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 | 49 ++++++++++++++++++++++++++++++++++++++++++++-----
tools/virsh.h | 2 ++
2 files changed, 46 insertions(+), 5 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c
index b96dbda..c9da9f1 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -143,6 +143,9 @@ virshConnect(vshControl *ctl, const char *uri, bool readonly)
int interval = 5; /* Default */
int count = 6; /* Default */
bool keepalive_forced = false;
+ virCommandPtr pkagent = NULL;
+ int authfail = 0;
+ int agentstart = 0;
if (ctl->keepalive_interval >= 0) {
interval = ctl->keepalive_interval;
@@ -153,10 +156,43 @@ virshConnect(vshControl *ctl, const char *uri, bool readonly)
keepalive_forced = true;
}
- c = virConnectOpenAuth(uri, virConnectAuthPtrDefault,
- readonly ? VIR_CONNECT_RO : 0);
- if (!c)
- return NULL;
+ do {
+ virErrorPtr err;
+
+ if ((c = virConnectOpenAuth(uri, virConnectAuthPtrDefault,
+ readonly ? VIR_CONNECT_RO : 0)))
+ break;
+
+ if (readonly)
+ goto cleanup;
+
+ err = virGetLastError();
+ if (err && strstr(err->message,
+ _("no agent is available to authenticate"))) {
+ if (!pkagent) {
+ if (!(pkagent = virPolkitAgentCreate()))
+ goto cleanup;
+ }
+ agentstart++;
+ } else if (err && strstr(err->message, _("authentication
failed:"))) {
String matching is pretty unpleasant. I think we can match on
err->domain == VIR_FROM_POLKIT && err->code == VIR_ERR_AUTH_FAILED
for this.
Also instead of trying to match for the agent message, you can just
do
if (!virDBusIsServiceRegistered('....polkit service name....'))
to decide whether to then start the agent after an auth failure
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 :|
12:49 p.m.
New subject: [libvirt] [PATCH v2 3/3] virsh: Add support for text based polkit authentication
[...]
> + err = virGetLastError();
> + if (err && strstr(err->message,
> + _("no agent is available to authenticate"))) {
> + if (!pkagent) {
> + if (!(pkagent = virPolkitAgentCreate()))
> + goto cleanup;
> + }
> + agentstart++;
> + } else if (err && strstr(err->message, _("authentication
failed:"))) {
String matching is pretty unpleasant. I think we can match on
err->domain == VIR_FROM_POLKIT && err->code == VIR_ERR_AUTH_FAILED
for this.
Using VIR_ERR_AUTH_FAILED I cannot distinguish between the failure of
available agent or access denied by policy from virPolkitCheckAuth.
Adjusting what virPolkitCheckAuth returns means more code modification
since the assumption is -2 has 3 possible issues of which 2 currently
are tested by a err->message comparison.
I would think in this case, I wouldn't want to create a text agent if
access is denied by policy. So should I bite the bullet and adjust the
return value checking? Or should I add a new error code
"VIR_ERR_AUTH_DENY" and likewise adjust the code/tests to use that
rather than the current string comparisons.
John
Also instead of trying to match for the agent message, you can just
do
if (!virDBusIsServiceRegistered('....polkit service name....'))
to decide whether to then start the agent after an auth failure
12:57 p.m.
New subject: [libvirt] [PATCH v2 3/3] virsh: Add support for text based polkit authentication
On Fri, Feb 12, 2016 at 06:49:22AM -0500, John Ferlan wrote:
[...]
>> + err = virGetLastError();
>> + if (err && strstr(err->message,
>> + _("no agent is available to
authenticate"))) {
>
>> + if (!pkagent) {
>> + if (!(pkagent = virPolkitAgentCreate()))
>> + goto cleanup;
>> + }
>> + agentstart++;
>> + } else if (err && strstr(err->message,
_("authentication failed:"))) {
>
> String matching is pretty unpleasant. I think we can match on
> err->domain == VIR_FROM_POLKIT && err->code == VIR_ERR_AUTH_FAILED
> for this.
>
Using VIR_ERR_AUTH_FAILED I cannot distinguish between the failure of
available agent or access denied by policy from virPolkitCheckAuth.
Adjusting what virPolkitCheckAuth returns means more code modification
since the assumption is -2 has 3 possible issues of which 2 currently
are tested by a err->message comparison.
My point is that you don't actually need to distinguish those two
cases directly. You can do this:
if (err && err->code == VIR_FROM_POLKIT && err->code ==
VIR_ER_AUTH_FAILED) {
if (!virDBusIsServiceRegistered(...polkit...)) {
....start agent...
}
....retry auth...
}
I would think in this case, I wouldn't want to create a text
agent if
access is denied by policy. So should I bite the bullet and adjust the
return value checking? Or should I add a new error code
"VIR_ERR_AUTH_DENY" and likewise adjust the code/tests to use that
rather than the current string comparisons.
It is actually generally bad security practice to tell users /why/ auth
failed - that we return different error messages for these two cases
is probably something we should in fact fix.
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:53 p.m.
New subject: [libvirt] [PATCH v2 3/3] virsh: Add support for text based polkit authentication
On 02/12/2016 06:57 AM, Daniel P. Berrange wrote:
On Fri, Feb 12, 2016 at 06:49:22AM -0500, John Ferlan wrote:
> [...]
>
>>> + err = virGetLastError();
>>> + if (err && strstr(err->message,
>>> + _("no agent is available to
authenticate"))) {
>>
>>> + if (!pkagent) {
>>> + if (!(pkagent = virPolkitAgentCreate()))
>>> + goto cleanup;
>>> + }
>>> + agentstart++;
>>> + } else if (err && strstr(err->message,
_("authentication failed:"))) {
>>
>> String matching is pretty unpleasant. I think we can match on
>> err->domain == VIR_FROM_POLKIT && err->code == VIR_ERR_AUTH_FAILED
>> for this.
>>
>
> Using VIR_ERR_AUTH_FAILED I cannot distinguish between the failure of
> available agent or access denied by policy from virPolkitCheckAuth.
> Adjusting what virPolkitCheckAuth returns means more code modification
> since the assumption is -2 has 3 possible issues of which 2 currently
> are tested by a err->message comparison.
My point is that you don't actually need to distinguish those two
cases directly. You can do this:
if (err && err->code == VIR_FROM_POLKIT && err->code ==
VIR_ER_AUTH_FAILED) {
if (!virDBusIsServiceRegistered(...polkit...)) {
Including "virdbus.h" to get virDBusIsServiceRegistered from virsh.c
sends me down the build system rabbit hole again:
In file included from virsh.c:59:0:
../src/util/virdbus.h:27:25: fatal error: dbus/dbus.h: No such file or
directory
compilation terminated.
Adding "$(DBUS_CFLAGS)" to the virsh_CLFAGS in Makefile.am still leaves
me with:
virsh-virsh.o: In function `virshConnect':
/home/jferlan/git/libvirt.work/tools/virsh.c:183: undefined reference to
`virDBusIsServiceRegistered'
collect2: error: ld returned 1 exit status
...
Even if/once figured out, wouldn't that a dependency to virsh?
John
....start agent...
}
....retry auth...
}
> I would think in this case, I wouldn't want to create a text agent if
> access is denied by policy. So should I bite the bullet and adjust the
> return value checking? Or should I add a new error code
> "VIR_ERR_AUTH_DENY" and likewise adjust the code/tests to use that
> rather than the current string comparisons.
It is actually generally bad security practice to tell users /why/ auth
failed - that we return different error messages for these two cases
is probably something we should in fact fix.
2:22 p.m.
New subject: [libvirt] [PATCH v2 3/3] virsh: Add support for text based polkit authentication
On Fri, Feb 12, 2016 at 07:53:40AM -0500, John Ferlan wrote:
On 02/12/2016 06:57 AM, Daniel P. Berrange wrote:
> On Fri, Feb 12, 2016 at 06:49:22AM -0500, John Ferlan wrote:
>> [...]
>>
>>>> + err = virGetLastError();
>>>> + if (err && strstr(err->message,
>>>> + _("no agent is available to
authenticate"))) {
>>>
>>>> + if (!pkagent) {
>>>> + if (!(pkagent = virPolkitAgentCreate()))
>>>> + goto cleanup;
>>>> + }
>>>> + agentstart++;
>>>> + } else if (err && strstr(err->message,
_("authentication failed:"))) {
>>>
>>> String matching is pretty unpleasant. I think we can match on
>>> err->domain == VIR_FROM_POLKIT && err->code ==
VIR_ERR_AUTH_FAILED
>>> for this.
>>>
>>
>> Using VIR_ERR_AUTH_FAILED I cannot distinguish between the failure of
>> available agent or access denied by policy from virPolkitCheckAuth.
>> Adjusting what virPolkitCheckAuth returns means more code modification
>> since the assumption is -2 has 3 possible issues of which 2 currently
>> are tested by a err->message comparison.
>
> My point is that you don't actually need to distinguish those two
> cases directly. You can do this:
>
> if (err && err->code == VIR_FROM_POLKIT && err->code ==
VIR_ER_AUTH_FAILED) {
> if (!virDBusIsServiceRegistered(...polkit...)) {
Including "virdbus.h" to get virDBusIsServiceRegistered from virsh.c
sends me down the build system rabbit hole again:
In file included from virsh.c:59:0:
../src/util/virdbus.h:27:25: fatal error: dbus/dbus.h: No such file or
directory
compilation terminated.
Adding "$(DBUS_CFLAGS)" to the virsh_CLFAGS in Makefile.am still leaves
me with:
virsh-virsh.o: In function `virshConnect':
/home/jferlan/git/libvirt.work/tools/virsh.c:183: undefined reference to
`virDBusIsServiceRegistered'
collect2: error: ld returned 1 exit status
Oh we missed it from src/libvirt_private.syms
...
Even if/once figured out, wouldn't that a dependency to virsh?
Yes, that's no problem really.
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:04 p.m.
New subject: [libvirt] [PATCH v2 3/3] virsh: Add support for text based polkit authentication
On 02/12/2016 08:22 AM, Daniel P. Berrange wrote:
On Fri, Feb 12, 2016 at 07:53:40AM -0500, John Ferlan wrote:
>
>
> On 02/12/2016 06:57 AM, Daniel P. Berrange wrote:
>> On Fri, Feb 12, 2016 at 06:49:22AM -0500, John Ferlan wrote:
>>> [...]
>>>
>>>>> + err = virGetLastError();
>>>>> + if (err && strstr(err->message,
>>>>> + _("no agent is available to
authenticate"))) {
>>>>
>>>>> + if (!pkagent) {
>>>>> + if (!(pkagent = virPolkitAgentCreate()))
>>>>> + goto cleanup;
>>>>> + }
>>>>> + agentstart++;
>>>>> + } else if (err && strstr(err->message,
_("authentication failed:"))) {
>>>>
>>>> String matching is pretty unpleasant. I think we can match on
>>>> err->domain == VIR_FROM_POLKIT && err->code ==
VIR_ERR_AUTH_FAILED
>>>> for this.
>>>>
>>>
>>> Using VIR_ERR_AUTH_FAILED I cannot distinguish between the failure of
>>> available agent or access denied by policy from virPolkitCheckAuth.
>>> Adjusting what virPolkitCheckAuth returns means more code modification
>>> since the assumption is -2 has 3 possible issues of which 2 currently
>>> are tested by a err->message comparison.
>>
>> My point is that you don't actually need to distinguish those two
>> cases directly. You can do this:
>>
>> if (err && err->code == VIR_FROM_POLKIT && err->code ==
VIR_ER_AUTH_FAILED) {
>> if (!virDBusIsServiceRegistered(...polkit...)) {
>
> Including "virdbus.h" to get virDBusIsServiceRegistered from virsh.c
> sends me down the build system rabbit hole again:
>
> In file included from virsh.c:59:0:
> ../src/util/virdbus.h:27:25: fatal error: dbus/dbus.h: No such file or
> directory
> compilation terminated.
>
> Adding "$(DBUS_CFLAGS)" to the virsh_CLFAGS in Makefile.am still leaves
> me with:
>
> virsh-virsh.o: In function `virshConnect':
> /home/jferlan/git/libvirt.work/tools/virsh.c:183: undefined reference to
> `virDBusIsServiceRegistered'
> collect2: error: ld returned 1 exit status
Oh we missed it from src/libvirt_private.syms
Ahh... that's it - didn't even consider that option...
However, virDBusIsServiceRegistered:
"Retruns 0 if service is registered, -1 on fatal error, or -2 if service
is not registered"
I found passing "org.freedesktop.PolicyKit1" returns 0 every time even
whether or not virPolkitAgentCreate has been called... Feels like
something like the machine name code that searches by pid would be what
would work.
As an alternative (since this is the I want to make sure the agent is
running path), the pkttyagent also takes a --notify-fd fd parameter. I
can work something up to use that.
John
4:08 p.m.
New subject: [libvirt] [PATCH v2 3/3] virsh: Add support for text based polkit authentication
On Fri, Feb 12, 2016 at 10:04:58AM -0500, John Ferlan wrote:
On 02/12/2016 08:22 AM, Daniel P. Berrange wrote:
> On Fri, Feb 12, 2016 at 07:53:40AM -0500, John Ferlan wrote:
>>
>>
>> On 02/12/2016 06:57 AM, Daniel P. Berrange wrote:
>>> On Fri, Feb 12, 2016 at 06:49:22AM -0500, John Ferlan wrote:
>>>> [...]
>>>>
>>>>>> + err = virGetLastError();
>>>>>> + if (err && strstr(err->message,
>>>>>> + _("no agent is available to
authenticate"))) {
>>>>>
>>>>>> + if (!pkagent) {
>>>>>> + if (!(pkagent = virPolkitAgentCreate()))
>>>>>> + goto cleanup;
>>>>>> + }
>>>>>> + agentstart++;
>>>>>> + } else if (err && strstr(err->message,
_("authentication failed:"))) {
>>>>>
>>>>> String matching is pretty unpleasant. I think we can match on
>>>>> err->domain == VIR_FROM_POLKIT && err->code ==
VIR_ERR_AUTH_FAILED
>>>>> for this.
>>>>>
>>>>
>>>> Using VIR_ERR_AUTH_FAILED I cannot distinguish between the failure of
>>>> available agent or access denied by policy from virPolkitCheckAuth.
>>>> Adjusting what virPolkitCheckAuth returns means more code modification
>>>> since the assumption is -2 has 3 possible issues of which 2 currently
>>>> are tested by a err->message comparison.
>>>
>>> My point is that you don't actually need to distinguish those two
>>> cases directly. You can do this:
>>>
>>> if (err && err->code == VIR_FROM_POLKIT &&
err->code == VIR_ER_AUTH_FAILED) {
>>> if (!virDBusIsServiceRegistered(...polkit...)) {
>>
>> Including "virdbus.h" to get virDBusIsServiceRegistered from virsh.c
>> sends me down the build system rabbit hole again:
>>
>> In file included from virsh.c:59:0:
>> ../src/util/virdbus.h:27:25: fatal error: dbus/dbus.h: No such file or
>> directory
>> compilation terminated.
>>
>> Adding "$(DBUS_CFLAGS)" to the virsh_CLFAGS in Makefile.am still
leaves
>> me with:
>>
>> virsh-virsh.o: In function `virshConnect':
>> /home/jferlan/git/libvirt.work/tools/virsh.c:183: undefined reference to
>> `virDBusIsServiceRegistered'
>> collect2: error: ld returned 1 exit status
>
> Oh we missed it from src/libvirt_private.syms
>
Ahh... that's it - didn't even consider that option...
However, virDBusIsServiceRegistered:
"Retruns 0 if service is registered, -1 on fatal error, or -2 if service
is not registered"
I found passing "org.freedesktop.PolicyKit1" returns 0 every time even
whether or not virPolkitAgentCreate has been called... Feels like
something like the machine name code that searches by pid would be what
would work.
As an alternative (since this is the I want to make sure the agent is
running path), the pkttyagent also takes a --notify-fd fd parameter. I
can work something up to use that.
Oh, I'm an idiot in suggesting this route. So, even when logging in via
ssh, chances are that there *is* an agent running - one run by GNOME
in your desktop login. The problem is that agent isn't able to display
prompts on the ssh text console.
So I think we would have to go back to the approach of returning a
special error code to let the client detect the problem.
So when libvirtd is able to auth due to missing agent, then we should
return a new code VIR_ERR_AUTH_UNAVAILABLE, and virsh can check for
that to see if it should launch pkttyagent.
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 :|
3142
days inactive
3143
days old
12 comments
2 participants
participants (2)
-
Daniel P. Berrange -
John Ferlan