[libvirt] [PATCH 08/10] qemu_driver.c: use virConnectValidateURIPath()
Daniel Henrique Barboza
danielhb413 at gmail.com
Mon Sep 23 15:14:52 UTC 2019
On 9/23/19 11:51 AM, Daniel P. Berrangé wrote:
> On Mon, Sep 23, 2019 at 11:03:41AM -0300, Daniel Henrique Barboza wrote:
>> The existing QEMU URI path validation considers that a
>> privileged user can use a "/session" URI as well. Let's update
>> virConnectValidateURIPath() to consider this usage, allowing us
>> to use the function inside qemu_driver.c.
>>
>> Although the existing callers of virConnectValidateURIPath()
>> didn't consider that a privileged user could connect with
>> "/session", it is sensible to consider that this should be
>> the case. thus no serious harm is done.
> That we allowed use of '/session' while connecting as root is a bug
> IMHO. Ideally we're kill that off entirely, so there's potential for
> us to allow a genuine session instance daemon as root at a later date.
>
> I'm just concerned about whether there might be apps relying on this
> bug.
In QEMU I believe this would be considered a sort of "API break",
then it would be put in a deprecation cycle and, after the cycle (last
time I checked it was about 2 years, but I might be wrong) it would be
permanently changed.
Perhaps a similar approach can be taken here in Libvirt. I mean, in
theory we could simply change it, claiming that it is a bug fix and every
other driver is forbidding root with 'session' already. In reality,
people will get mad about it ...
>
> I certainly don't want to see the bug spread across all drivers
> though.
I'll resubmit the patch series, creating a new function to emulate
the current behavior of qemu_driver, vbox_common and vbox_driver,
without changing the behavior of everyone else. I'll put a comment
in it to document the problem you mentioned above. At least
we have this issue documented in the code (perhaps a TODO/FIXME
tag is warranted?) and it's centralized in one place.
Thanks,
DHB
>
>> Suggested-by: Cole Robinson <crobinso at redhat.com>
>> Signed-off-by: Daniel Henrique Barboza <danielhb413 at gmail.com>
>> ---
>> src/driver.c | 3 ++-
>> src/qemu/qemu_driver.c | 20 ++++----------------
>> 2 files changed, 6 insertions(+), 17 deletions(-)
>>
>> diff --git a/src/driver.c b/src/driver.c
>> index e627b0c1d7..f9c41383dc 100644
>> --- a/src/driver.c
>> +++ b/src/driver.c
>> @@ -276,7 +276,8 @@ virConnectValidateURIPath(const char *uriPath,
>> bool privileged)
>> {
>> if (privileged) {
>> - if (STRNEQ(uriPath, "/system")) {
>> + if (STRNEQ(uriPath, "/system") &&
>> + STRNEQ(uriPath, "/session")) {
>> virReportError(VIR_ERR_INTERNAL_ERROR,
>> _("unexpected %s URI path '%s', try %s:///system"),
>> entityName, uriPath, entityName);
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 0753904472..05025e7251 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
>>
>> --
>> libvir-list mailing list
>> libvir-list at redhat.com
>> https://www.redhat.com/mailman/listinfo/libvir-list
> Regards,
> Daniel
More information about the libvir-list
mailing list