[libvirt] [PATCH v2 0/3] Fix some errors around VIR_ACCESS_DENIED
v1: https://www.redhat.com/archives/libvir-list/2018-November/msg00339.html Changes since v1: * Do the right thing, revert the bad patch and rework it. Thus patch1 is the revert and patch2 is the rework. If it's decided that patch2 is unnecessary or undesired, that's perfectly fine, but would then require a slight modification to the documentation from commit 4f1107614 to remove the reference about the access denied message. * From review - add the virObjectUnref for the data->identity for which a virIdentityGetCurrent reference was obtained. v1 cover: Patch 1 fixes my own error made in this release fortunately, but only seen because I was invesigating Patch 2 Patch 2 is perhaps a longer term issue, but perhaps coming more to light now that the nwfilter bindings have been separated and use a virConnectOpen for nwfilter:///system during reconnection processing; whereas, previously the filter bindings would have been "hidden" within various nwfilter, domain, and network driver callbacks and throughs. John Ferlan (3): Revert "access: Modify the VIR_ERR_ACCESS_DENIED to include driverName" access: Modify the VIR_ERR_ACCESS_DENIED to include driverName qemu: Set identity for the reconnect all thread src/access/viraccessmanager.c | 3 ++- src/qemu/qemu_process.c | 7 +++++++ src/rpc/gendispatch.pl | 3 ++- src/util/virerror.c | 4 ++-- 4 files changed, 13 insertions(+), 4 deletions(-) -- 2.17.2
This reverts commit ccc72d5cbdd85f66cb737134b3be40aac1df03ef. Based on upstream comment to a follow-up patch, this didn't take the right approach and the right thing to do is revert and rework. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/access/viraccessmanager.c | 25 ++++++++++++------------- src/rpc/gendispatch.pl | 2 +- src/util/virerror.c | 4 ++-- 3 files changed, 15 insertions(+), 16 deletions(-) diff --git a/src/access/viraccessmanager.c b/src/access/viraccessmanager.c index 1dfff32b9d..e7b5bf38da 100644 --- a/src/access/viraccessmanager.c +++ b/src/access/viraccessmanager.c @@ -196,12 +196,11 @@ static void virAccessManagerDispose(void *object) * should the admin need to debug things */ static int -virAccessManagerSanitizeError(int ret, - const char *driverName) +virAccessManagerSanitizeError(int ret) { if (ret < 0) { virResetLastError(); - virAccessError(VIR_ERR_ACCESS_DENIED, driverName, NULL); + virAccessError(VIR_ERR_ACCESS_DENIED, NULL); } return ret; @@ -218,7 +217,7 @@ int virAccessManagerCheckConnect(virAccessManagerPtr manager, if (manager->drv->checkConnect) ret = manager->drv->checkConnect(manager, driverName, perm); - return virAccessManagerSanitizeError(ret, driverName); + return virAccessManagerSanitizeError(ret); } @@ -234,7 +233,7 @@ int virAccessManagerCheckDomain(virAccessManagerPtr manager, if (manager->drv->checkDomain) ret = manager->drv->checkDomain(manager, driverName, domain, perm); - return virAccessManagerSanitizeError(ret, driverName); + return virAccessManagerSanitizeError(ret); } int virAccessManagerCheckInterface(virAccessManagerPtr manager, @@ -249,7 +248,7 @@ int virAccessManagerCheckInterface(virAccessManagerPtr manager, if (manager->drv->checkInterface) ret = manager->drv->checkInterface(manager, driverName, iface, perm); - return virAccessManagerSanitizeError(ret, driverName); + return virAccessManagerSanitizeError(ret); } int virAccessManagerCheckNetwork(virAccessManagerPtr manager, @@ -264,7 +263,7 @@ int virAccessManagerCheckNetwork(virAccessManagerPtr manager, if (manager->drv->checkNetwork) ret = manager->drv->checkNetwork(manager, driverName, network, perm); - return virAccessManagerSanitizeError(ret, driverName); + return virAccessManagerSanitizeError(ret); } int virAccessManagerCheckNodeDevice(virAccessManagerPtr manager, @@ -279,7 +278,7 @@ int virAccessManagerCheckNodeDevice(virAccessManagerPtr manager, if (manager->drv->checkNodeDevice) ret = manager->drv->checkNodeDevice(manager, driverName, nodedev, perm); - return virAccessManagerSanitizeError(ret, driverName); + return virAccessManagerSanitizeError(ret); } int virAccessManagerCheckNWFilter(virAccessManagerPtr manager, @@ -294,7 +293,7 @@ int virAccessManagerCheckNWFilter(virAccessManagerPtr manager, if (manager->drv->checkNWFilter) ret = manager->drv->checkNWFilter(manager, driverName, nwfilter, perm); - return virAccessManagerSanitizeError(ret, driverName); + return virAccessManagerSanitizeError(ret); } int virAccessManagerCheckNWFilterBinding(virAccessManagerPtr manager, @@ -309,7 +308,7 @@ int virAccessManagerCheckNWFilterBinding(virAccessManagerPtr manager, if (manager->drv->checkNWFilterBinding) ret = manager->drv->checkNWFilterBinding(manager, driverName, binding, perm); - return virAccessManagerSanitizeError(ret, driverName); + return virAccessManagerSanitizeError(ret); } int virAccessManagerCheckSecret(virAccessManagerPtr manager, @@ -324,7 +323,7 @@ int virAccessManagerCheckSecret(virAccessManagerPtr manager, if (manager->drv->checkSecret) ret = manager->drv->checkSecret(manager, driverName, secret, perm); - return virAccessManagerSanitizeError(ret, driverName); + return virAccessManagerSanitizeError(ret); } int virAccessManagerCheckStoragePool(virAccessManagerPtr manager, @@ -339,7 +338,7 @@ int virAccessManagerCheckStoragePool(virAccessManagerPtr manager, if (manager->drv->checkStoragePool) ret = manager->drv->checkStoragePool(manager, driverName, pool, perm); - return virAccessManagerSanitizeError(ret, driverName); + return virAccessManagerSanitizeError(ret); } int virAccessManagerCheckStorageVol(virAccessManagerPtr manager, @@ -355,5 +354,5 @@ int virAccessManagerCheckStorageVol(virAccessManagerPtr manager, if (manager->drv->checkStorageVol) ret = manager->drv->checkStorageVol(manager, driverName, pool, vol, perm); - return virAccessManagerSanitizeError(ret, driverName); + return virAccessManagerSanitizeError(ret); } diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl index f599002056..0c4648c0fb 100755 --- a/src/rpc/gendispatch.pl +++ b/src/rpc/gendispatch.pl @@ -2199,7 +2199,7 @@ elsif ($mode eq "client") { print " virObjectUnref(mgr);\n"; if ($action eq "Ensure") { print " if (rv == 0)\n"; - print " virReportError(VIR_ERR_ACCESS_DENIED, conn->driver->name, NULL);\n"; + print " virReportError(VIR_ERR_ACCESS_DENIED, NULL);\n"; print " return $fail;\n"; } else { print " virResetLastError();\n"; diff --git a/src/util/virerror.c b/src/util/virerror.c index 10f1b55c5f..683e51aa19 100644 --- a/src/util/virerror.c +++ b/src/util/virerror.c @@ -1442,9 +1442,9 @@ virErrorMsg(virErrorNumber error, const char *info) break; case VIR_ERR_ACCESS_DENIED: if (info == NULL) - errmsg = _("access denied from '%s'"); + errmsg = _("access denied"); else - errmsg = _("access denied from '%s': %s"); + errmsg = _("access denied: %s"); break; case VIR_ERR_DBUS_SERVICE: if (info == NULL) -- 2.17.2
https://bugzilla.redhat.com/show_bug.cgi?id=1631606 Changes made to manage and utilize a secondary connection driver to APIs outside the scope of the primary connection driver have resulted in some confusion processing polkit rules since the simple "access denied" error message doesn't provide enough of a clue when combined with the "authentication failed: access denied by policy" as to which connection driver refused or failed the ACL check. In order to provide some context, let's modify the existing "access denied" error returned from the various vir*EnsureACL API's to provide the connection driver name that is causing the failure. This should provide the context for writing the polkit rules that would allow access via the driver, but yet still adhere to the virAccessManagerSanitizeError commentary regarding not telling the user why access was denied. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/access/viraccessmanager.c | 26 ++++++++++++++------------ src/rpc/gendispatch.pl | 3 ++- 2 files changed, 16 insertions(+), 13 deletions(-) diff --git a/src/access/viraccessmanager.c b/src/access/viraccessmanager.c index e7b5bf38da..f5d62604cf 100644 --- a/src/access/viraccessmanager.c +++ b/src/access/viraccessmanager.c @@ -196,11 +196,13 @@ static void virAccessManagerDispose(void *object) * should the admin need to debug things */ static int -virAccessManagerSanitizeError(int ret) +virAccessManagerSanitizeError(int ret, + const char *driverName) { if (ret < 0) { virResetLastError(); - virAccessError(VIR_ERR_ACCESS_DENIED, NULL); + virAccessError(VIR_ERR_ACCESS_DENIED, + _("'%s' denied access"), driverName); } return ret; @@ -217,7 +219,7 @@ int virAccessManagerCheckConnect(virAccessManagerPtr manager, if (manager->drv->checkConnect) ret = manager->drv->checkConnect(manager, driverName, perm); - return virAccessManagerSanitizeError(ret); + return virAccessManagerSanitizeError(ret, driverName); } @@ -233,7 +235,7 @@ int virAccessManagerCheckDomain(virAccessManagerPtr manager, if (manager->drv->checkDomain) ret = manager->drv->checkDomain(manager, driverName, domain, perm); - return virAccessManagerSanitizeError(ret); + return virAccessManagerSanitizeError(ret, driverName); } int virAccessManagerCheckInterface(virAccessManagerPtr manager, @@ -248,7 +250,7 @@ int virAccessManagerCheckInterface(virAccessManagerPtr manager, if (manager->drv->checkInterface) ret = manager->drv->checkInterface(manager, driverName, iface, perm); - return virAccessManagerSanitizeError(ret); + return virAccessManagerSanitizeError(ret, driverName); } int virAccessManagerCheckNetwork(virAccessManagerPtr manager, @@ -263,7 +265,7 @@ int virAccessManagerCheckNetwork(virAccessManagerPtr manager, if (manager->drv->checkNetwork) ret = manager->drv->checkNetwork(manager, driverName, network, perm); - return virAccessManagerSanitizeError(ret); + return virAccessManagerSanitizeError(ret, driverName); } int virAccessManagerCheckNodeDevice(virAccessManagerPtr manager, @@ -278,7 +280,7 @@ int virAccessManagerCheckNodeDevice(virAccessManagerPtr manager, if (manager->drv->checkNodeDevice) ret = manager->drv->checkNodeDevice(manager, driverName, nodedev, perm); - return virAccessManagerSanitizeError(ret); + return virAccessManagerSanitizeError(ret, driverName); } int virAccessManagerCheckNWFilter(virAccessManagerPtr manager, @@ -293,7 +295,7 @@ int virAccessManagerCheckNWFilter(virAccessManagerPtr manager, if (manager->drv->checkNWFilter) ret = manager->drv->checkNWFilter(manager, driverName, nwfilter, perm); - return virAccessManagerSanitizeError(ret); + return virAccessManagerSanitizeError(ret, driverName); } int virAccessManagerCheckNWFilterBinding(virAccessManagerPtr manager, @@ -308,7 +310,7 @@ int virAccessManagerCheckNWFilterBinding(virAccessManagerPtr manager, if (manager->drv->checkNWFilterBinding) ret = manager->drv->checkNWFilterBinding(manager, driverName, binding, perm); - return virAccessManagerSanitizeError(ret); + return virAccessManagerSanitizeError(ret, driverName); } int virAccessManagerCheckSecret(virAccessManagerPtr manager, @@ -323,7 +325,7 @@ int virAccessManagerCheckSecret(virAccessManagerPtr manager, if (manager->drv->checkSecret) ret = manager->drv->checkSecret(manager, driverName, secret, perm); - return virAccessManagerSanitizeError(ret); + return virAccessManagerSanitizeError(ret, driverName); } int virAccessManagerCheckStoragePool(virAccessManagerPtr manager, @@ -338,7 +340,7 @@ int virAccessManagerCheckStoragePool(virAccessManagerPtr manager, if (manager->drv->checkStoragePool) ret = manager->drv->checkStoragePool(manager, driverName, pool, perm); - return virAccessManagerSanitizeError(ret); + return virAccessManagerSanitizeError(ret, driverName); } int virAccessManagerCheckStorageVol(virAccessManagerPtr manager, @@ -354,5 +356,5 @@ int virAccessManagerCheckStorageVol(virAccessManagerPtr manager, if (manager->drv->checkStorageVol) ret = manager->drv->checkStorageVol(manager, driverName, pool, vol, perm); - return virAccessManagerSanitizeError(ret); + return virAccessManagerSanitizeError(ret, driverName); } diff --git a/src/rpc/gendispatch.pl b/src/rpc/gendispatch.pl index 0c4648c0fb..a8b9f5aeca 100755 --- a/src/rpc/gendispatch.pl +++ b/src/rpc/gendispatch.pl @@ -2199,7 +2199,8 @@ elsif ($mode eq "client") { print " virObjectUnref(mgr);\n"; if ($action eq "Ensure") { print " if (rv == 0)\n"; - print " virReportError(VIR_ERR_ACCESS_DENIED, NULL);\n"; + print " virReportError(VIR_ERR_ACCESS_DENIED,\n"; + print" _(\"'%s' denied access\"), conn->driver->name);\n"; print " return $fail;\n"; } else { print " virResetLastError();\n"; -- 2.17.2
https://bugzilla.redhat.com/show_bug.cgi?id=1631622 If polkit authentication is enabled, an attempt to open the connection failed during virAccessDriverPolkitGetCaller when the call to virIdentityGetCurrent returned NULL resulting in the errors: virAccessDriverPolkitGetCaller:87 : access denied: Policy kit denied action org.libvirt.api.connect.getattr from <anonymous> Because qemuProcessReconnect runs in a thread during daemonRunStateInit processing it doesn't have the thread local identity. Thus when the virGetConnectNWFilter is called as part of the qemuProcessFiltersInstantiate when virDomainConfNWFilterInstantiate is run the attempt to get the idenity fails and results in the anonymous error above. To fix this, let's grab/use the virIdenityPtr of the process that will be creating the thread, e.g. what daemonRunStateInit has set and use that for our thread. That way any other similar processing that uses/requires an identity for any other call that would have previously been successfully run won't fail in a similar manner. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_process.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 1850923914..df7f0bfafb 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -81,6 +81,7 @@ #include "netdev_bandwidth_conf.h" #include "virresctrl.h" #include "virvsock.h" +#include "viridentity.h" #define VIR_FROM_THIS VIR_FROM_QEMU @@ -7705,6 +7706,7 @@ qemuProcessRefreshCPU(virQEMUDriverPtr driver, struct qemuProcessReconnectData { virQEMUDriverPtr driver; virDomainObjPtr obj; + virIdentityPtr identity; }; /* * Open an existing VM's monitor, re-detect VCPU threads @@ -7742,6 +7744,8 @@ qemuProcessReconnect(void *opaque) bool retry = true; bool tryMonReconn = false; + virIdentitySetCurrent(data->identity); + virObjectUnref(data->identity); VIR_FREE(data); qemuDomainObjRestoreJob(obj, &oldjob); @@ -7968,6 +7972,7 @@ qemuProcessReconnect(void *opaque) virObjectUnref(cfg); virObjectUnref(caps); virNWFilterUnlockFilterUpdates(); + virIdentitySetCurrent(NULL); return; error: @@ -8011,6 +8016,7 @@ qemuProcessReconnectHelper(virDomainObjPtr obj, memcpy(data, src, sizeof(*data)); data->obj = obj; + data->identity = virIdentityGetCurrent(); virNWFilterReadLockFilterUpdates(); @@ -8034,6 +8040,7 @@ qemuProcessReconnectHelper(virDomainObjPtr obj, virDomainObjEndAPI(&obj); virNWFilterUnlockFilterUpdates(); + virObjectUnref(data->identity); VIR_FREE(data); return -1; } -- 2.17.2
On 11/12/2018 02:50 PM, John Ferlan wrote:
v1: https://www.redhat.com/archives/libvir-list/2018-November/msg00339.html
Changes since v1:
* Do the right thing, revert the bad patch and rework it. Thus patch1 is the revert and patch2 is the rework. If it's decided that patch2 is unnecessary or undesired, that's perfectly fine, but would then require a slight modification to the documentation from commit 4f1107614 to remove the reference about the access denied message.
* From review - add the virObjectUnref for the data->identity for which a virIdentityGetCurrent reference was obtained.
v1 cover:
Patch 1 fixes my own error made in this release fortunately, but only seen because I was invesigating Patch 2
Patch 2 is perhaps a longer term issue, but perhaps coming more to light now that the nwfilter bindings have been separated and use a virConnectOpen for nwfilter:///system during reconnection processing; whereas, previously the filter bindings would have been "hidden" within various nwfilter, domain, and network driver callbacks and throughs.
John Ferlan (3): Revert "access: Modify the VIR_ERR_ACCESS_DENIED to include driverName" access: Modify the VIR_ERR_ACCESS_DENIED to include driverName qemu: Set identity for the reconnect all thread
src/access/viraccessmanager.c | 3 ++- src/qemu/qemu_process.c | 7 +++++++ src/rpc/gendispatch.pl | 3 ++- src/util/virerror.c | 4 ++-- 4 files changed, 13 insertions(+), 4 deletions(-)
ACK Michal
participants (2)
-
John Ferlan -
Michal Privoznik