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

[Daniel Henrique Barboza]

On 9/25/19 7:58 PM, Cole Robinson wrote:
> On 9/25/19 9:50 AM, 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.
>>
>> Suggested-by: Cole Robinson <crobinso at redhat.com>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413 at gmail.com>
>> ---
>>   src/driver.c | 26 ++++++++++++++++++++------
>>   1 file changed, 20 insertions(+), 6 deletions(-)
>>
>> diff --git a/src/driver.c b/src/driver.c
>> index e627b0c1d7..8f9da35f78 100644
>> --- a/src/driver.c
>> +++ b/src/driver.c
>> @@ -276,16 +276,30 @@ virConnectValidateURIPath(const char *uriPath,
>>                             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;
>> +        /* 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.
>> +         */
>> +        bool compatSessionRoot = (STREQ(entityName, "QEMU") ||
>> +                                  STREQ(entityName, "vbox"));
>> +
>> +        if ((compatSessionRoot && STRNEQ(uriPath, "/session")) ||
>> +            STRNEQ(uriPath, "/system")) {
>> +                virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                               _("unexpected %s URI path '%s', try "
>> +                                 "%s:///system"),
>> +                               entityName, uriPath, entityName);
>> +                return false;
>>           }
>
> The logic is incorrect here. Try connecting to qemu:///system after 
> this change, it will be rejected.

I assumed that any of the checks in 'make check' would fail if I messed
up here. Never assume huh.

(/me wondering if this rewards a unit test)

>
> Also passing in the 'QEMU' instead of 'qemu string means we print the 
> wrong URI in the error. I think this needs a v4 :/

I'll fix that and re-spin a v4. I'll also squash the below formatting like
you suggested in patch 01.


Thanks,

DHB


>
>>       } else {
>>           if (STRNEQ(uriPath, "/session")) {
>>               virReportError(VIR_ERR_INTERNAL_ERROR,
>> -                           _("unexpected %s URI path '%s', try 
>> %s:///session"),
>> +                           _("unexpected %s URI path '%s', try "
>> +                             "%s:///session"),
>>                              entityName, uriPath, entityName);
>
> This formatting should be squashed into patch #1 IMO
>
> Thanks,
> Cole