[libvirt] [PATCH v2 0/3] Add capability for text based polkit authentication for virsh
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
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@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
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@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 :|
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@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
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@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
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@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
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@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 :|
[...]
+ 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
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 :|
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.
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 :|
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
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 :|
participants (2)
-
Daniel P. Berrange -
John Ferlan