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@xxxxxxxxxx> Signed-off-by: Daniel Henrique Barboza <danielhb413@xxxxxxxxx> --- 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
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
