3:44 p.m.
This is a code repetition that I crossed a few times, then
I noticed that Cole Robinson suggested a solution for it
in the wiki. Here it is.
changes from v1:
- handle QEMU and vbox cases separately inside the validation
function
v1: https://www.redhat.com/archives/libvir-list/2019-September/msg00983.html
Daniel Henrique Barboza (11):
src/driver.c: add virConnectValidateURIPath()
interface_backend_netcf.c: use virConnectValidateURIPath()
interface_backend_udev.c: use virConnectValidateURIPath()
bridge_driver.c: virConnectValidateURIPath()
node_device_driver.c: use virConnectValidateURIPath()
secret_driver.c: use virConnectValidateURIPath()
storage_driver.c: use virConnectValidateURIPath()
driver.c: change URI validation to handle QEMU and vbox case
qemu_driver.c: use virConnectValidateURIPath()
vbox_common.c: use virConnectValidateURIPath()
vbox_driver.c: use virConnectValidateURIPath()
src/driver.c | 40 +++++++++++++++++++++++++
src/driver.h | 4 +++
src/interface/interface_backend_netcf.c | 17 ++---------
src/interface/interface_backend_udev.c | 17 ++---------
src/libvirt_private.syms | 1 +
src/network/bridge_driver.c | 19 +++---------
src/node_device/node_device_driver.c | 17 ++---------
src/qemu/qemu_driver.c | 20 +++----------
src/secret/secret_driver.c | 17 ++---------
src/storage/storage_driver.c | 17 ++---------
src/vbox/vbox_common.c | 16 ++--------
src/vbox/vbox_driver.c | 16 ++--------
12 files changed, 67 insertions(+), 134 deletions(-)
--
2.21.0
3:44 p.m.
New subject: [libvirt] [PATCH v2 01/11] src/driver.c: add virConnectValidateURIPath()
The code to validate the URI path is repeated across several
files. This patch creates a common validation code to be
used across all of them.
Suggested-by: Cole Robinson <crobinso(a)redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
---
src/driver.c | 24 ++++++++++++++++++++++++
src/driver.h | 4 ++++
src/libvirt_private.syms | 1 +
3 files changed, 29 insertions(+)
diff --git a/src/driver.c b/src/driver.c
index 5e8f68f6df..e627b0c1d7 100644
--- a/src/driver.c
+++ b/src/driver.c
@@ -269,3 +269,27 @@ virSetConnectStorage(virConnectPtr conn)
VIR_DEBUG("Override storage connection with %p", conn);
return virThreadLocalSet(&connectStorage, conn);
}
+
+bool
+virConnectValidateURIPath(const char *uriPath,
+ const char *entityName,
+ bool privileged)
+{
+ if (privileged) {
+ if (STRNEQ(uriPath, "/system")) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("unexpected %s URI path '%s', try
%s:///system"),
+ entityName, uriPath, entityName);
+ return false;
+ }
+ } else {
+ if (STRNEQ(uriPath, "/session")) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("unexpected %s URI path '%s', try
%s:///session"),
+ entityName, uriPath, entityName);
+ return false;
+ }
+ }
+
+ return true;
+}
diff --git a/src/driver.h b/src/driver.h
index f7d667a03c..68c0004d86 100644
--- a/src/driver.h
+++ b/src/driver.h
@@ -127,3 +127,7 @@ int virSetConnectNWFilter(virConnectPtr conn);
int virSetConnectNodeDev(virConnectPtr conn);
int virSetConnectSecret(virConnectPtr conn);
int virSetConnectStorage(virConnectPtr conn);
+
+bool virConnectValidateURIPath(const char *uriPath,
+ const char *entityName,
+ bool privileged);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 39812227aa..eb9c5c22ee 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1343,6 +1343,7 @@ virStreamClass;
# driver.h
+virConnectValidateURIPath;
virGetConnectInterface;
virGetConnectNetwork;
virGetConnectNodeDev;
--
2.21.0
3:44 p.m.
New subject: [libvirt] [PATCH v2 02/11] interface_backend_netcf.c: use virConnectValidateURIPath()
Suggested-by: Cole Robinson <crobinso(a)redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
---
src/interface/interface_backend_netcf.c | 17 ++---------------
1 file changed, 2 insertions(+), 15 deletions(-)
diff --git a/src/interface/interface_backend_netcf.c
b/src/interface/interface_backend_netcf.c
index 9659e9fcf1..7fe8f230b6 100644
--- a/src/interface/interface_backend_netcf.c
+++ b/src/interface/interface_backend_netcf.c
@@ -200,21 +200,8 @@ netcfConnectOpen(virConnectPtr conn,
return VIR_DRV_OPEN_ERROR;
}
- if (driver->privileged) {
- if (STRNEQ(conn->uri->path, "/system")) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unexpected interface URI path '%s', try
interface:///system"),
- conn->uri->path);
- return VIR_DRV_OPEN_ERROR;
- }
- } else {
- if (STRNEQ(conn->uri->path, "/session")) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unexpected interface URI path '%s', try
interface:///session"),
- conn->uri->path);
- return VIR_DRV_OPEN_ERROR;
- }
- }
+ if (!virConnectValidateURIPath(conn->uri->path, "interface",
driver->privileged))
+ return VIR_DRV_OPEN_ERROR;
if (virConnectOpenEnsureACL(conn) < 0)
return VIR_DRV_OPEN_ERROR;
--
2.21.0
3:44 p.m.
New subject: [libvirt] [PATCH v2 03/11] interface_backend_udev.c: use virConnectValidateURIPath()
Suggested-by: Cole Robinson <crobinso(a)redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
---
src/interface/interface_backend_udev.c | 17 ++---------------
1 file changed, 2 insertions(+), 15 deletions(-)
diff --git a/src/interface/interface_backend_udev.c
b/src/interface/interface_backend_udev.c
index ddc3de5347..d870e3d1b1 100644
--- a/src/interface/interface_backend_udev.c
+++ b/src/interface/interface_backend_udev.c
@@ -1250,21 +1250,8 @@ udevConnectOpen(virConnectPtr conn,
return VIR_DRV_OPEN_ERROR;
}
- if (driver->privileged) {
- if (STRNEQ(conn->uri->path, "/system")) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unexpected interface URI path '%s', try
interface:///system"),
- conn->uri->path);
- return VIR_DRV_OPEN_ERROR;
- }
- } else {
- if (STRNEQ(conn->uri->path, "/session")) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unexpected interface URI path '%s', try
interface:///session"),
- conn->uri->path);
- return VIR_DRV_OPEN_ERROR;
- }
- }
+ if (!virConnectValidateURIPath(conn->uri->path, "interface",
driver->privileged))
+ return VIR_DRV_OPEN_ERROR;
if (virConnectOpenEnsureACL(conn) < 0)
return VIR_DRV_OPEN_ERROR;
--
2.21.0
3:44 p.m.
New subject: [libvirt] [PATCH v2 04/11] bridge_driver.c: virConnectValidateURIPath()
Suggested-by: Cole Robinson <crobinso(a)redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
---
src/network/bridge_driver.c | 19 ++++---------------
1 file changed, 4 insertions(+), 15 deletions(-)
diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c
index c54be96407..c617bbb58f 100644
--- a/src/network/bridge_driver.c
+++ b/src/network/bridge_driver.c
@@ -938,21 +938,10 @@ networkConnectOpen(virConnectPtr conn,
return VIR_DRV_OPEN_ERROR;
}
- if (network_driver->privileged) {
- if (STRNEQ(conn->uri->path, "/system")) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unexpected network URI path '%s', try
network:///system"),
- conn->uri->path);
- return VIR_DRV_OPEN_ERROR;
- }
- } else {
- if (STRNEQ(conn->uri->path, "/session")) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unexpected network URI path '%s', try
network:///session"),
- conn->uri->path);
- return VIR_DRV_OPEN_ERROR;
- }
- }
+ if (!virConnectValidateURIPath(conn->uri->path,
+ "network",
+ network_driver->privileged))
+ return VIR_DRV_OPEN_ERROR;
if (virConnectOpenEnsureACL(conn) < 0)
return VIR_DRV_OPEN_ERROR;
--
2.21.0
3:44 p.m.
New subject: [libvirt] [PATCH v2 05/11] node_device_driver.c: use virConnectValidateURIPath()
Suggested-by: Cole Robinson <crobinso(a)redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
---
src/node_device/node_device_driver.c | 17 ++---------------
1 file changed, 2 insertions(+), 15 deletions(-)
diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
index 8fb00d0c86..06febacd96 100644
--- a/src/node_device/node_device_driver.c
+++ b/src/node_device/node_device_driver.c
@@ -58,21 +58,8 @@ nodeConnectOpen(virConnectPtr conn,
return VIR_DRV_OPEN_ERROR;
}
- if (driver->privileged) {
- if (STRNEQ(conn->uri->path, "/system")) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unexpected nodedev URI path '%s', try
nodedev:///system"),
- conn->uri->path);
- return VIR_DRV_OPEN_ERROR;
- }
- } else {
- if (STRNEQ(conn->uri->path, "/session")) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unexpected nodedev URI path '%s', try
nodedev:///session"),
- conn->uri->path);
- return VIR_DRV_OPEN_ERROR;
- }
- }
+ if (!virConnectValidateURIPath(conn->uri->path, "nodedev",
driver->privileged))
+ return VIR_DRV_OPEN_ERROR;
if (virConnectOpenEnsureACL(conn) < 0)
return VIR_DRV_OPEN_ERROR;
--
2.21.0
3:44 p.m.
New subject: [libvirt] [PATCH v2 06/11] secret_driver.c: use virConnectValidateURIPath()
Suggested-by: Cole Robinson <crobinso(a)redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
---
src/secret/secret_driver.c | 17 ++---------------
1 file changed, 2 insertions(+), 15 deletions(-)
diff --git a/src/secret/secret_driver.c b/src/secret/secret_driver.c
index 7512a51c74..07ba679541 100644
--- a/src/secret/secret_driver.c
+++ b/src/secret/secret_driver.c
@@ -552,21 +552,8 @@ secretConnectOpen(virConnectPtr conn,
return VIR_DRV_OPEN_ERROR;
}
- if (driver->privileged) {
- if (STRNEQ(conn->uri->path, "/system")) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unexpected secret URI path '%s', try
secret:///system"),
- conn->uri->path);
- return VIR_DRV_OPEN_ERROR;
- }
- } else {
- if (STRNEQ(conn->uri->path, "/session")) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unexpected secret URI path '%s', try
secret:///session"),
- conn->uri->path);
- return VIR_DRV_OPEN_ERROR;
- }
- }
+ if (!virConnectValidateURIPath(conn->uri->path, "secret",
driver->privileged))
+ return VIR_DRV_OPEN_ERROR;
if (virConnectOpenEnsureACL(conn) < 0)
return VIR_DRV_OPEN_ERROR;
--
2.21.0
3:44 p.m.
New subject: [libvirt] [PATCH v2 07/11] storage_driver.c: use virConnectValidateURIPath()
Suggested-by: Cole Robinson <crobinso(a)redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
---
src/storage/storage_driver.c | 17 ++---------------
1 file changed, 2 insertions(+), 15 deletions(-)
diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c
index ce10b55ed0..1bec2d964f 100644
--- a/src/storage/storage_driver.c
+++ b/src/storage/storage_driver.c
@@ -411,21 +411,8 @@ storageConnectOpen(virConnectPtr conn,
return VIR_DRV_OPEN_ERROR;
}
- if (driver->privileged) {
- if (STRNEQ(conn->uri->path, "/system")) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unexpected storage URI path '%s', try
storage:///system"),
- conn->uri->path);
- return VIR_DRV_OPEN_ERROR;
- }
- } else {
- if (STRNEQ(conn->uri->path, "/session")) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unexpected storage URI path '%s', try
storage:///session"),
- conn->uri->path);
- return VIR_DRV_OPEN_ERROR;
- }
- }
+ if (!virConnectValidateURIPath(conn->uri->path, "storage",
driver->privileged))
+ return VIR_DRV_OPEN_ERROR;
if (virConnectOpenEnsureACL(conn) < 0)
return VIR_DRV_OPEN_ERROR;
--
2.21.0
3:44 p.m.
New subject: [libvirt] [PATCH v2 08/11] driver.c: change URI validation to handle QEMU and vbox case
The existing QEMU and vbox URI path validation consider
that a privileged user can use both a "/system" and a
"/session" URI. This differs from all the other drivers
that forbids the root user to use "/session" URI.
Let's update virConnectValidateURIPath() to handle these
cases as exceptions, using the already existent 'entityName'
value to handle "QEMU" and "vbox" differently. This allows
us to use the validateURI function in these cases without
changing the existing behavior of other drivers.
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
---
src/driver.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/src/driver.c b/src/driver.c
index e627b0c1d7..c89a410dd1 100644
--- a/src/driver.c
+++ b/src/driver.c
@@ -276,7 +276,23 @@ virConnectValidateURIPath(const char *uriPath,
bool privileged)
{
if (privileged) {
- if (STRNEQ(uriPath, "/system")) {
+ /* TODO: QEMU and vbox drivers allow '/session'
+ * connections as root. This is not ideal, but changing
+ * these drivers to refuse privileged '/session'
+ * connections, like everyone else is already doing, can
+ * break existing applications. Until we decide what to do,
+ * for now we can handle them as exception in this validate
+ * function.
+ */
+ if (STREQ(entityName, "QEMU") || STREQ(entityName, "vbox"))
{
+ if (STRNEQ(uriPath, "/system") &&
+ STRNEQ(uriPath, "/session")) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("unexpected %s URI path '%s', try
%s:///system"),
+ entityName, uriPath, entityName);
+ return false;
+ }
+ } else if (STRNEQ(uriPath, "/system")) {
virReportError(VIR_ERR_INTERNAL_ERROR,
_("unexpected %s URI path '%s', try
%s:///system"),
entityName, uriPath, entityName);
--
2.21.0
1:09 p.m.
New subject: [libvirt] [PATCH v2 08/11] driver.c: change URI validation to handle QEMU and vbox case
On 9/23/19 4:44 PM, Daniel Henrique Barboza wrote:
The existing QEMU and vbox URI path validation consider
that a privileged user can use both a "/system" and a
"/session" URI. This differs from all the other drivers
that forbids the root user to use "/session" URI.
Let's update virConnectValidateURIPath() to handle these
cases as exceptions, using the already existent 'entityName'
value to handle "QEMU" and "vbox" differently. This allows
us to use the validateURI function in these cases without
changing the existing behavior of other drivers.
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
---
src/driver.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/src/driver.c b/src/driver.c
index e627b0c1d7..c89a410dd1 100644
--- a/src/driver.c
+++ b/src/driver.c
@@ -276,7 +276,23 @@ virConnectValidateURIPath(const char *uriPath,
bool privileged)
{
if (privileged) {
- if (STRNEQ(uriPath, "/system")) {
+ /* TODO: QEMU and vbox drivers allow '/session'
+ * connections as root. This is not ideal, but changing
+ * these drivers to refuse privileged '/session'
+ * connections, like everyone else is already doing, can
+ * break existing applications. Until we decide what to do,
+ * for now we can handle them as exception in this validate
+ * function.
+ */
+ if (STREQ(entityName, "QEMU") || STREQ(entityName, "vbox"))
{
+ if (STRNEQ(uriPath, "/system") &&
+ STRNEQ(uriPath, "/session")) {
+ virReportError(VIR_ERR_INTERNAL_ERROR,
+ _("unexpected %s URI path '%s', try
%s:///system"),
+ entityName, uriPath, entityName);
+ return false;
+ }
I like the approach of hiding the behavior in this function, it will
make it harder for new usages to slip in. I don't think it should
duplicate the whole block though. You could do something like
bool compatSessionRoot = (STREQ(entityName, "QEMU") ||
STREQ(entityName, "vbox");
And use that to conditionalize the /session check in those block
Also, break up the > 80 long lines in the other patches, besides the
error strings those ConnectOpen functions don't seem to have any long
lines. After that I think the series will be good
Thanks,
Cole
3:44 p.m.
New subject: [libvirt] [PATCH v2 09/11] qemu_driver.c: use virConnectValidateURIPath()
Suggested-by: Cole Robinson <crobinso(a)redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
---
src/qemu/qemu_driver.c | 20 ++++----------------
1 file changed, 4 insertions(+), 16 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index f7f059b6d6..fe4d50c372 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -1295,22 +1295,10 @@ static virDrvOpenStatus qemuConnectOpen(virConnectPtr conn,
return VIR_DRV_OPEN_ERROR;
}
- if (virQEMUDriverIsPrivileged(qemu_driver)) {
- if (STRNEQ(conn->uri->path, "/system") &&
- STRNEQ(conn->uri->path, "/session")) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unexpected QEMU URI path '%s', try
qemu:///system"),
- conn->uri->path);
- return VIR_DRV_OPEN_ERROR;
- }
- } else {
- if (STRNEQ(conn->uri->path, "/session")) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unexpected QEMU URI path '%s', try
qemu:///session"),
- conn->uri->path);
- return VIR_DRV_OPEN_ERROR;
- }
- }
+ if (!virConnectValidateURIPath(conn->uri->path,
+ "QEMU",
+ virQEMUDriverIsPrivileged(qemu_driver)))
+ return VIR_DRV_OPEN_ERROR;
if (virConnectOpenEnsureACL(conn) < 0)
return VIR_DRV_OPEN_ERROR;
--
2.21.0
3:44 p.m.
New subject: [libvirt] [PATCH v2 10/11] vbox_common.c: use virConnectValidateURIPath()
Suggested-by: Cole Robinson <crobinso(a)redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
---
src/vbox/vbox_common.c | 16 ++--------------
1 file changed, 2 insertions(+), 14 deletions(-)
diff --git a/src/vbox/vbox_common.c b/src/vbox/vbox_common.c
index ddabcb80ca..d3b8fb625f 100644
--- a/src/vbox/vbox_common.c
+++ b/src/vbox/vbox_common.c
@@ -517,20 +517,8 @@ vboxConnectOpen(virConnectPtr conn,
virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR);
- if (uid != 0) {
- if (STRNEQ(conn->uri->path, "/session")) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unknown driver path '%s' specified (try
vbox:///session)"), conn->uri->path);
- return VIR_DRV_OPEN_ERROR;
- }
- } else { /* root */
- if (STRNEQ(conn->uri->path, "/system") &&
- STRNEQ(conn->uri->path, "/session")) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unknown driver path '%s' specified (try
vbox:///system)"), conn->uri->path);
- return VIR_DRV_OPEN_ERROR;
- }
- }
+ if (!virConnectValidateURIPath(conn->uri->path, "vbox", uid == 0))
+ return VIR_DRV_OPEN_ERROR;
if (!(driver = vboxGetDriverConnection()))
return VIR_DRV_OPEN_ERROR;
--
2.21.0
3:44 p.m.
New subject: [libvirt] [PATCH v2 11/11] vbox_driver.c: use virConnectValidateURIPath()
Suggested-by: Cole Robinson <crobinso(a)redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413(a)gmail.com>
---
src/vbox/vbox_driver.c | 16 ++--------------
1 file changed, 2 insertions(+), 14 deletions(-)
diff --git a/src/vbox/vbox_driver.c b/src/vbox/vbox_driver.c
index 1f31fa28df..d7e80828ab 100644
--- a/src/vbox/vbox_driver.c
+++ b/src/vbox/vbox_driver.c
@@ -58,20 +58,8 @@ static virDrvOpenStatus dummyConnectOpen(virConnectPtr conn,
virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR);
- if (uid != 0) {
- if (STRNEQ(conn->uri->path, "/session")) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unknown driver path '%s' specified (try
vbox:///session)"), conn->uri->path);
- return VIR_DRV_OPEN_ERROR;
- }
- } else { /* root */
- if (STRNEQ(conn->uri->path, "/system") &&
- STRNEQ(conn->uri->path, "/session")) {
- virReportError(VIR_ERR_INTERNAL_ERROR,
- _("unknown driver path '%s' specified (try
vbox:///system)"), conn->uri->path);
- return VIR_DRV_OPEN_ERROR;
- }
- }
+ if (!virConnectValidateURIPath(conn->uri->path, "vbox", uid == 0))
+ return VIR_DRV_OPEN_ERROR;
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("unable to initialize VirtualBox driver API"));
--
2.21.0
1922
days inactive
1923
days old
12 comments
2 participants
participants (2)
-
Cole Robinson -
Daniel Henrique Barboza