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

[Cole Robinson]

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 at 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