[libvirt] [PATCH v2 00/11] remove repetition of URI path validation
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
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@redhat.com> Signed-off-by: Daniel Henrique Barboza <danielhb413@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
Suggested-by: Cole Robinson <crobinso@redhat.com> Signed-off-by: Daniel Henrique Barboza <danielhb413@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
Suggested-by: Cole Robinson <crobinso@redhat.com> Signed-off-by: Daniel Henrique Barboza <danielhb413@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
Suggested-by: Cole Robinson <crobinso@redhat.com> Signed-off-by: Daniel Henrique Barboza <danielhb413@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
Suggested-by: Cole Robinson <crobinso@redhat.com> Signed-off-by: Daniel Henrique Barboza <danielhb413@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
Suggested-by: Cole Robinson <crobinso@redhat.com> Signed-off-by: Daniel Henrique Barboza <danielhb413@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
Suggested-by: Cole Robinson <crobinso@redhat.com> Signed-off-by: Daniel Henrique Barboza <danielhb413@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
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@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
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@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
Suggested-by: Cole Robinson <crobinso@redhat.com> Signed-off-by: Daniel Henrique Barboza <danielhb413@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
Suggested-by: Cole Robinson <crobinso@redhat.com> Signed-off-by: Daniel Henrique Barboza <danielhb413@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
Suggested-by: Cole Robinson <crobinso@redhat.com> Signed-off-by: Daniel Henrique Barboza <danielhb413@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
participants (2)
-
Cole Robinson -
Daniel Henrique Barboza