[libvirt] [PATCH] PolicyKit: Check auth before asking client to obtain it
I previously mentioned [1] a PolicyKit issue where libvirt would proceed with authentication even though polkit-auth failed: testusr xen134:~> virsh list --all Attempting to obtain authorization for org.libvirt.unix.manage. polkit-grant-helper: given auth type (8 -> yes) is bogus Failed to obtain authorization for org.libvirt.unix.manage. Id Name State ---------------------------------- 0 Domain-0 running - sles11sp1-pv shut off AFAICT, libvirt attempts to obtain a privilege it already has, causing polkit-auth to fail with above message. Instead of calling obtain and then checking auth, IMO the workflow should be for the server to check auth first, and if that fails ask the client to obtain it and check again. This workflow also allows for checking only successful exit of polkit-auth in virConnectAuthGainPolkit(). [1] https://www.redhat.com/archives/libvir-list/2011-December/msg00837.html --- src/libvirt.c | 2 +- src/remote/remote_driver.c | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index feb3ca6..9e04a38 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -119,7 +119,7 @@ static int virConnectAuthGainPolkit(const char *privilege) { cmd = virCommandNewArgList(POLKIT_AUTH, "--obtain", privilege, NULL); if (virCommandRun(cmd, &status) < 0 || - status > 1) + status > 0) goto cleanup; ret = 0; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 7580477..e28840b 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -3121,6 +3121,14 @@ remoteAuthPolkit (virConnectPtr conn, struct private_data *priv, }; VIR_DEBUG("Client initialize PolicyKit-0 authentication"); + /* Check auth first and if it succeeds we are done. */ + memset (&ret, 0, sizeof ret); + if (call (conn, priv, 0, REMOTE_PROC_AUTH_POLKIT, + (xdrproc_t) xdr_void, (char *)NULL, + (xdrproc_t) xdr_remote_auth_polkit_ret, (char *) &ret) == 0) + goto out; + + /* Auth failed. Ask client to obtain it and check again. */ if (auth && auth->cb) { /* Check if the necessary credential type for PolicyKit is supported */ for (i = 0 ; i < auth->ncredtype ; i++) { @@ -3138,9 +3146,11 @@ remoteAuthPolkit (virConnectPtr conn, struct private_data *priv, } } else { VIR_DEBUG("Client auth callback does not support PolicyKit"); + return -1; } } else { VIR_DEBUG("No auth callback provided"); + return -1; } memset (&ret, 0, sizeof ret); @@ -3150,6 +3160,7 @@ remoteAuthPolkit (virConnectPtr conn, struct private_data *priv, return -1; /* virError already set by call */ } +out: VIR_DEBUG("PolicyKit-0 authentication complete"); return 0; } -- 1.7.7
On 01/03/2012 03:35 PM, Jim Fehlig wrote:
I previously mentioned [1] a PolicyKit issue where libvirt would proceed with authentication even though polkit-auth failed:
testusr xen134:~> virsh list --all Attempting to obtain authorization for org.libvirt.unix.manage. polkit-grant-helper: given auth type (8 -> yes) is bogus Failed to obtain authorization for org.libvirt.unix.manage. Id Name State ---------------------------------- 0 Domain-0 running - sles11sp1-pv shut off
AFAICT, libvirt attempts to obtain a privilege it already has, causing polkit-auth to fail with above message. Instead of calling obtain and then checking auth, IMO the workflow should be for the server to check auth first, and if that fails ask the client to obtain it and check again. This workflow also allows for checking only successful exit of polkit-auth in virConnectAuthGainPolkit().
[1] https://www.redhat.com/archives/libvir-list/2011-December/msg00837.html --- src/libvirt.c | 2 +- src/remote/remote_driver.c | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletions(-)
This looks reasonable to me, but I'd like a second opinion from someone more familiar with the PolicyKit code before you push anything (that would probably be DV or danpb). If they agree, then I think it can go in 0.9.9. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Thu, Jan 05, 2012 at 01:12:37PM -0700, Eric Blake wrote:
On 01/03/2012 03:35 PM, Jim Fehlig wrote:
I previously mentioned [1] a PolicyKit issue where libvirt would proceed with authentication even though polkit-auth failed:
testusr xen134:~> virsh list --all Attempting to obtain authorization for org.libvirt.unix.manage. polkit-grant-helper: given auth type (8 -> yes) is bogus Failed to obtain authorization for org.libvirt.unix.manage. Id Name State ---------------------------------- 0 Domain-0 running - sles11sp1-pv shut off
AFAICT, libvirt attempts to obtain a privilege it already has, causing polkit-auth to fail with above message. Instead of calling obtain and then checking auth, IMO the workflow should be for the server to check auth first, and if that fails ask the client to obtain it and check again. This workflow also allows for checking only successful exit of polkit-auth in virConnectAuthGainPolkit().
[1] https://www.redhat.com/archives/libvir-list/2011-December/msg00837.html --- src/libvirt.c | 2 +- src/remote/remote_driver.c | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletions(-)
This looks reasonable to me, but I'd like a second opinion from someone more familiar with the PolicyKit code before you push anything (that would probably be DV or danpb). If they agree, then I think it can go in 0.9.9.
ACK Out of interest, what Suse distro releases are still relying on the old policy kit code, as opposed to the new style ? 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 :|
Daniel P. Berrange wrote:
On Thu, Jan 05, 2012 at 01:12:37PM -0700, Eric Blake wrote:
On 01/03/2012 03:35 PM, Jim Fehlig wrote:
I previously mentioned [1] a PolicyKit issue where libvirt would proceed with authentication even though polkit-auth failed:
testusr xen134:~> virsh list --all Attempting to obtain authorization for org.libvirt.unix.manage. polkit-grant-helper: given auth type (8 -> yes) is bogus Failed to obtain authorization for org.libvirt.unix.manage. Id Name State ---------------------------------- 0 Domain-0 running - sles11sp1-pv shut off
AFAICT, libvirt attempts to obtain a privilege it already has, causing polkit-auth to fail with above message. Instead of calling obtain and then checking auth, IMO the workflow should be for the server to check auth first, and if that fails ask the client to obtain it and check again. This workflow also allows for checking only successful exit of polkit-auth in virConnectAuthGainPolkit().
[1] https://www.redhat.com/archives/libvir-list/2011-December/msg00837.html --- src/libvirt.c | 2 +- src/remote/remote_driver.c | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletions(-)
This looks reasonable to me, but I'd like a second opinion from someone more familiar with the PolicyKit code before you push anything (that would probably be DV or danpb). If they agree, then I think it can go in 0.9.9.
ACK
Thanks. Should I push this for 0.9.9?
Out of interest, what Suse distro releases are still relying on the old policy kit code, as opposed to the new style ?
SLES11 contains the old PolicyKit package, so I'll need the libvirt integration to work for quite some time :-/. All supported openSUSE distros use the new polkit packages. Regards, Jim
Daniel P. Berrange wrote:
On Thu, Jan 05, 2012 at 01:12:37PM -0700, Eric Blake wrote:
On 01/03/2012 03:35 PM, Jim Fehlig wrote:
I previously mentioned [1] a PolicyKit issue where libvirt would proceed with authentication even though polkit-auth failed:
testusr xen134:~> virsh list --all Attempting to obtain authorization for org.libvirt.unix.manage. polkit-grant-helper: given auth type (8 -> yes) is bogus Failed to obtain authorization for org.libvirt.unix.manage. Id Name State ---------------------------------- 0 Domain-0 running - sles11sp1-pv shut off
AFAICT, libvirt attempts to obtain a privilege it already has, causing polkit-auth to fail with above message. Instead of calling obtain and then checking auth, IMO the workflow should be for the server to check auth first, and if that fails ask the client to obtain it and check again. This workflow also allows for checking only successful exit of polkit-auth in virConnectAuthGainPolkit().
[1] https://www.redhat.com/archives/libvir-list/2011-December/msg00837.html --- src/libvirt.c | 2 +- src/remote/remote_driver.c | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletions(-)
This looks reasonable to me, but I'd like a second opinion from someone more familiar with the PolicyKit code before you push anything (that would probably be DV or danpb). If they agree, then I think it can go in 0.9.9.
ACK
I've pushed this now that 0.9.9 has been released. Thanks, Jim
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Jim Fehlig