Re: [PATCH v2 08/11] driver.c: change URI validation to handle QEMU and vbox case

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx>
---
  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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux