7:50 a.m.
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
7:50 a.m.
New subject: [libvirt] [PATCH v2 1/3] Revert "access: Modify the VIR_ERR_ACCESS_DENIED to include driverName"
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(a)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
7:50 a.m.
New subject: [libvirt] [PATCH v2 2/3] access: Modify the VIR_ERR_ACCESS_DENIED to include driverName
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(a)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
7:50 a.m.
New subject: [libvirt] [PATCH v2 3/3] qemu: Set identity for the reconnect all thread
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(a)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
10:33 a.m.
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
2200
days inactive
2202
days old
4 comments
2 participants
participants (2)
-
John Ferlan -
Michal Privoznik