Bug 1155967 - The "search" parameter is ignored when combined with "Filter: true"
Summary: The "search" parameter is ignored when combined with "Filter: true"
Keywords:
Status: CLOSED DUPLICATE of bug 1163105
Alias: None
Product: oVirt
Classification: Retired
Component: ovirt-engine-core
Version: 4.0
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
: 3.6.0
Assignee: bugs@ovirt.org
QA Contact: Pavel Stehlik
URL:
Whiteboard: infra
: 1132501 (view as bug list)
Depends On:
Blocks: 1132506
TreeView+ depends on / blocked
 
Reported: 2014-10-23 09:50 UTC by Juan Hernández
Modified: 2016-02-10 19:30 UTC (History)
16 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2015-04-12 10:55:59 UTC
oVirt Team: Infra
Embargoed:
tbabej: needinfo-


Attachments (Terms of Use)

Description Juan Hernández 2014-10-23 09:50:59 UTC
Currently when a request is sent to the RESTAPI with the "Filter: true" header the "search" parameter is silently ignored. This may lead to unexpected results, for example it may induce errors like removing the wrong VM, as described in bug 1148715.

Ideally the RESTAPI should support the "search" parameter when combined with "Filter: true" and perform the requested search. If this isn't feasible then it should reject the request, or return an empty set  of objects.

Comment 1 Tareq Alayan 2014-10-26 09:46:58 UTC
Hi Juan,

Why not fix this to rhevm 3.5 ?

Comment 2 Juan Hernández 2014-10-26 11:22:29 UTC
I'm targeting 4.0 because I think that the solution will be complicated, so we won't have time to fix it in the 3.5 or 3.6 time frame. However, if the rest of the stakeholders believe that it should be fixed earlier I'm open to discuss and eventually re-target.

Comment 3 Tomas Babej 2014-11-04 14:52:41 UTC
This bug breaks user-built scripts, i.e. I often use api.templates/vms/something.get() with the python-SDK. If the search parameter is ignored, the get method obtains mutliple results, and raises an exception.

It can be worked around by direct object definition, or by listing all objects and filtering done in Python, however, both approaches require non-trivial changes to already proven and tested scripts.

Comment 4 Petr Spacek 2014-11-04 15:29:33 UTC
Juan,

would it be possible to allocate some reasonable amount of time for initial bug investigation? I would really like to avoiding shifting of this bug to future releases unless absolutelly necessary.

Comment 6 Juan Hernández 2014-11-04 15:41:34 UTC
(In reply to Petr Spacek from comment #4)
> Juan,
> 
> would it be possible to allocate some reasonable amount of time for initial
> bug investigation? I would really like to avoiding shifting of this bug to
> future releases unless absolutelly necessary.

As I wrote in comment 2 I'm open to re-target this bug. I count Tareq's, Tomas' and your comments as positive stakeholder votes. Barak, I would like to know your view on this.

Comment 7 Barak 2014-11-04 16:25:59 UTC
Eli please take (talk to Juan) a look and share your thoughts.
The target version for this bug should depend on the complexity and instabilities it might cause (introducing filters to the search backend). as long as there are workarounds for the deeper problem (using filer in searches ... might lead to a severe situation), we can allow postponing it to a later stage).
BTW it never worked.

Comment 8 Michal Skrivanek 2014-11-05 09:05:27 UTC
the reason for this request actually originates from bug 1160331. If that one works this bug would have less priority...

Comment 9 Petr Spacek 2014-11-05 09:36:47 UTC
Clarification:
We did not notice this bug in our testing environment because we were using 'admin' user for everything.

Now we tried to move from testing to "supported"/"production" RHEV instance where admin is not for everyone anymore and found out that it does not work.

Comment 10 Eli Mesika 2014-11-05 12:10:08 UTC
Adding the following information I had got from Juan in order to make the problem more clear :


If you are user U1 and you try to use the RESTAPI to get
the list of VMs you will by default get a permissions error. This
happens because the RESTAPI uses SearchType.VM to perform this request.

To be able to run the request without permission errors you have to use
the "Filter" header, like this:

#!/bin/sh -ex

url="https://ovirt.example.com/ovirt-engine/api";
user="admin@internal"
password="******"

curl \
--verbose \
--insecure \
--user "${user}:${password}" \
--header "Accept: application/xml" \
--header "Filter: true" \
"${url}/vms"

What will happen when you use this "Filter" header is that the RESTAPI
use VdcQueryType.GetAllVms instead of SearchType.VM. Then you will get
all the VMs that you have permissions for.

The problem is that when you use VdcQueryType.GetAllVms the "search"
parameter is completely ignored. For example, if you do this:

curl \
--verbose \
--insecure \
--user "${user}:${password}" \
--header "Accept: application/xml" \
--header "Filter: true" \
"${url}/vms?search=name%3Dmyvm"

The "search" parameter is ignored, and instead of getting the "myvm"
virtual machine you get all the VMs that you have permissions for. This
is very dangerous, because you request one VM and you may get a
different one as the result. Exactly that happens with the CLI, and
results in removing the wrong VM, for example.

All this is by design of the user API feature, and it is wrong design,
in my opinion.

The fix for this, in my opinion, is to modify the implementation of
SearchType.VM so that it does filtering, like VdcQueryType.GetAllVms
does, so that you get only the VMs that you have permissions for.

Same happens for other objects, not only VMs.

Comment 11 Eli Mesika 2014-11-05 12:16:01 UTC
I agree with Juan, we should extend the Search Engine to support filtering according to the user permissions.

However, this is a major change, IMO , in order to see if we can do that fro 3.6 we should do a POC that takes one of the entities like VM and add filtering support for it in the Search Engine. After that we can estimate the work and decide if to do that for 3.6 or 4.0

Comment 12 Itamar Heim 2014-11-05 12:22:39 UTC
well, considering user level api doesn't support search, i agree its misleading and we should consider failing such a request with an error as search isn't supported.
supporting search is not necessarily trivial.
Petr - is there anything you are doing which the user level api prevents you from doing without the search capabilities?

Comment 13 Itamar Heim 2014-11-05 12:23:53 UTC
(In reply to Michal Skrivanek from comment #8)
> the reason for this request actually originates from bug 1160331. If that
> one works this bug would have less priority...

michal - how would this solve the fact admin queries should be still blocked?
(and why would a non admin should succeed logging in as admin[1])

[1] yes, i know usability of this is very low today.

Comment 14 Juan Hernández 2014-11-05 12:54:14 UTC
(In reply to Michal Skrivanek from comment #8)
> the reason for this request actually originates from bug 1160331. If that
> one works this bug would have less priority...

Note also that fixing bug 1160331 only won't bring any benefit, as even if a normal (non admin) user is allowed to connect to the API without the "Filter: true" header she/he won't be able to get any useful result later, as most queries will just return a permissions error. The fix for that bug has to be combined with the ability to use "search" and "Filter: true" together, otherwise it is useless.

Comment 15 Tomas Babej 2014-11-05 13:28:27 UTC
Responding to Comment 12.

Pretty much nothing more complex can be scripted using the user level API with this bug.

We're using python-sdk, and anytime I call api.<something>.get(key=value), I trigger this bug and I am getting AmbiguousError (only one value was expected, but API returned multiple, since search was ignored).

This prevents you from doing simple things like:

api.template.get('Blank')
api.vms.get(name='myvm')

and such. See my Comment 3 for unacceptable workaround.

Comment 16 Juan Hernández 2014-11-06 15:24:04 UTC
Tomas, note that the currently proposed fix for bug 1155678 may also be helpful for you (if it is eventually merged) as it will make the Python SDK "get" method work as you expect, without the workaround.

Comment 17 Juan Hernández 2014-11-20 14:42:12 UTC
*** Bug 1132501 has been marked as a duplicate of this bug. ***

Comment 18 Michal Skrivanek 2015-01-14 08:14:40 UTC
(In reply to Itamar Heim from comment #13)
> (In reply to Michal Skrivanek from comment #8)
> > the reason for this request actually originates from bug 1160331. If that
> > one works this bug would have less priority...
> 
> michal - how would this solve the fact admin queries should be still blocked?
> (and why would a non admin should succeed logging in as admin[1])

right. I suppose the confusion is about a non-obvious fact that 
we differentiate between admin and non-admin access only by the "Filter" header
The use of this is misleading and should not be needed, right, since we know if the connecting user is an admin or not.

Comment 19 Itamar Heim 2015-01-14 08:44:07 UTC
Because the admin may not want to use his admin rights. Think user portal.

Comment 20 Petr Spacek 2015-01-14 10:37:58 UTC
(In reply to Itamar Heim from comment #19)
> Because the admin may not want to use his admin rights. Think user portal.

I'm sorry but I have to comment on this. This approach really seems as security through obscurity to me. What prevents e.g. rogue Javascript in user's web browser to issue API requests with Filter = False despite the fact that user is logged into User portal?

I would argue that admin principal should simply be separate account with other username/password so access control does not need to depend on user-supplied parameters like 'Filter'.

Comment 21 Itamar Heim 2015-01-14 12:19:04 UTC
filter=false is blocked for non admins.
this is not security by obscurity.
we distingiush between user permissions and admin permissions. we let the (same) user specify the role in which they want to act (if they have both).
this only affects queries/searches.

Comment 22 Petr Spacek 2015-01-14 15:49:18 UTC
(In reply to Itamar Heim from comment #21)
> this is not security by obscurity.
My point is that filter=true gives admins false sense of security while using User Portal.

> we distingiush between user permissions and admin permissions. we let the
> (same) user specify the role in which they want to act (if they have both).
> this only affects queries/searches.
That would be understandable as ugly but workable hack for user interface implementation but it is not only that: It actually changes API behavior outside of query/search:
See https://bugzilla.redhat.com/show_bug.cgi?id=1121144: User doesn't get the UserVmManager permission on a VM when VM is created with using via API with filter=false.

Comment 23 Oved Ourfali 2015-04-12 10:55:59 UTC

*** This bug has been marked as a duplicate of bug 1163105 ***


Note You need to log in before you can comment on or make changes to this bug.