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(a)redhat.com>
> Signed-off-by: Daniel Henrique Barboza <danielhb413(a)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