[libvirt] [PATCH 0/2] Augment access denied error message and adjust polkit docs
Details in the patches John Ferlan (2): access: Modify the VIR_ERR_ACCESS_DENIED to include driverName docs: Enhance polkit documentation to describe secondary connection docs/aclpolkit.html.in | 117 ++++++++++++++++++++++++++++++++++ docs/libvirt.css | 1 + src/access/viraccessmanager.c | 25 ++++---- src/rpc/gendispatch.pl | 2 +- src/util/virerror.c | 4 +- 5 files changed, 134 insertions(+), 15 deletions(-) -- 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 returne 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. 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, 16 insertions(+), 15 deletions(-) diff --git a/src/access/viraccessmanager.c b/src/access/viraccessmanager.c index e7b5bf38da..1dfff32b9d 100644 --- a/src/access/viraccessmanager.c +++ b/src/access/viraccessmanager.c @@ -196,11 +196,12 @@ 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, driverName, NULL); } return ret; @@ -217,7 +218,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 +234,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 +249,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 +264,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 +279,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 +294,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 +309,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 +324,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 +339,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 +355,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..f599002056 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, NULL);\n"; + print " virReportError(VIR_ERR_ACCESS_DENIED, conn->driver->name, NULL);\n"; print " return $fail;\n"; } else { print " virResetLastError();\n"; diff --git a/src/util/virerror.c b/src/util/virerror.c index 683e51aa19..10f1b55c5f 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"); + errmsg = _("access denied from '%s'"); else - errmsg = _("access denied: %s"); + errmsg = _("access denied from '%s': %s"); break; case VIR_ERR_DBUS_SERVICE: if (info == NULL) -- 2.17.2
https://bugzilla.redhat.com/show_bug.cgi?id=1631608 Since commit 8259255 usage of a primary connection driver for a virConnect has been modified to open (virConnectOpen) and use a connection to the specific driver in order to handle the API calls to/for that driver. This causes some confusion and issues for ACL polkit rule scripts to know exactly which driver by name will be used. Add some documentation describing the processing of the primary and secondary connection as well as the list of the connect_driver names used for each driver. Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/aclpolkit.html.in | 117 +++++++++++++++++++++++++++++++++++++++++ docs/libvirt.css | 1 + 2 files changed, 118 insertions(+) diff --git a/docs/aclpolkit.html.in b/docs/aclpolkit.html.in index ee00b98461..ac54f125da 100644 --- a/docs/aclpolkit.html.in +++ b/docs/aclpolkit.html.in @@ -287,6 +287,123 @@ </tbody> </table> + <h2><a id="connect_driver">Hypervisor Driver connect_driver</a></h2> + <p> + The <code>connect_driver</code> parameter describes the + client's <a href="remote.html">remote Connection Driver</a> + name based on the <a href="uri.html">URI</a> used for the + connection. + </p> + <p> + <span class="since">Since 4.1.0</span>, when calling an API + outside the scope of the primary connection driver, the + primary driver will attempt to open a secondary connection + to the specific API driver in order to process the API. For + example, when hypervisor domain processing needs to make an + API call within the storage driver or the network filter driver + an attempt to open a connection to the "storage" or "nwfilter" + driver will be made. Similarly, a "storage" primary connection + may need to create a connection to the "secret" driver in order + to process secrets for the API. If successful, then calls to + those API's will occur in the <code>connect_driver</code> context + of the secondary connection driver rather than in the context of + the primary driver. This affects the <code>connect_driver</code> + returned from rule generation from the <code>action.loookup</code> + function. The following table provides a list of the various + connection drivers and the <code>connect_driver</code> name + used by each regardless of primary or secondary connection. + The access denied error message from libvirt will list the + connection driver by name that denied the access. + </p> + + <h3><a id="object_connect_driver">Connection Driver Name</a></h3> + <table class="acl"> + <thead> + <tr> + <th>Connection Driver</th> + <th><code>connect_driver</code> name</th> + </tr> + </thead> + <tbody> + <tr> + <td>bhyve</td> + <td>bhyve</td> + </tr> + <tr> + <td>esx</td> + <td>ESX</td> + </tr> + <tr> + <td>hyperv</td> + <td>Hyper-V</td> + </tr> + <tr> + <td>interface</td> + <td>interface</td> + </tr> + <tr> + <td>libxl</td> + <td>xenlight</td> + </tr> + <tr> + <td>lxc</td> + <td>LXC</td> + </tr> + <tr> + <td>network</td> + <td>network</td> + </tr> + <tr> + <td>nodedev</td> + <td>nodedev</td> + </tr> + <tr> + <td>nwfilter</td> + <td>NWFilter</td> + </tr> + <tr> + <td>openvz</td> + <td>OPENVZ</td> + </tr> + <tr> + <td>phyp</td> + <td>PHYP</td> + </tr> + <tr> + <td>qemu</td> + <td>QEMU</td> + </tr> + <tr> + <td>secret</td> + <td>secret</td> + </tr> + <tr> + <td>storage</td> + <td>storage</td> + </tr> + <tr> + <td>uml</td> + <td>UML</td> + </tr> + <tr> + <td>vbox</td> + <td>VBOX</td> + </tr> + <tr> + <td>vmware</td> + <td>VMWARE</td> + </tr> + <tr> + <td>vz</td> + <td>vz</td> + </tr> + <tr> + <td>xenapi</td> + <td>XenAPI</td> + </tr> + </tbody> + </table> + <h2><a id="user">User identity attributes</a></h2> diff --git a/docs/libvirt.css b/docs/libvirt.css index b2ed33926a..e590b33cfb 100644 --- a/docs/libvirt.css +++ b/docs/libvirt.css @@ -393,6 +393,7 @@ table.acl { table.acl tr, table.acl td { padding: 0.3em; + border: 1px solid #ccc; } table.acl thead { -- 2.17.2
ping? Thoughts at all - it's more doc than code and I'd really like to be sure the words are proper and/or make sense. Tks - John On 10/15/18 10:26 AM, John Ferlan wrote:
Details in the patches
John Ferlan (2): access: Modify the VIR_ERR_ACCESS_DENIED to include driverName docs: Enhance polkit documentation to describe secondary connection
docs/aclpolkit.html.in | 117 ++++++++++++++++++++++++++++++++++ docs/libvirt.css | 1 + src/access/viraccessmanager.c | 25 ++++---- src/rpc/gendispatch.pl | 2 +- src/util/virerror.c | 4 +- 5 files changed, 134 insertions(+), 15 deletions(-)
On 10/15/2018 04:26 PM, John Ferlan wrote:
Details in the patches
John Ferlan (2): access: Modify the VIR_ERR_ACCESS_DENIED to include driverName docs: Enhance polkit documentation to describe secondary connection
docs/aclpolkit.html.in | 117 ++++++++++++++++++++++++++++++++++ docs/libvirt.css | 1 + src/access/viraccessmanager.c | 25 ++++---- src/rpc/gendispatch.pl | 2 +- src/util/virerror.c | 4 +- 5 files changed, 134 insertions(+), 15 deletions(-)
ACK Michal
participants (2)
-
John Ferlan -
Michal Privoznik