all_projects doesn't limit on resource type

[Steve McLellan]

Looking at the code in v1/search.py that constructs the eventual elasticsearch query, 'all_projects' passes the query straight through, bypassing the plugin RBAC filter entirely. That appears not to be correct. Using for OS::Nova::Server as an example, which has a simple project-id filter, the query without all_projects will look like:

  filtered: {
    filter: {
       indices: {
          filter: {
             and: [
                {term: {tenant_id: 'abc'}},
                {type: OS::Nova::Server}
             ]
         },
        index_name: <index>
      }
    },
   query: { <whatever the query was> }
  }

When all_projects is applied, the 'filtered' part is missing entirely. The resource_type IS restricted by an argument to the search API function, but I don't think that's restrictive enough (imagine two live indices, both containing OS::Nova::Server types).

This isn't tremendously serious because it only affects admin queries with all_projects (so there's no security implication, for instance), but I think the code could be more consistent; it seems more accurate to always construct the 'indices' filters but drop the 'term' filter if all_projects is given.

[Steve McLellan (sjmc7)]

Looking a bit more at this, i think the query creation code is unnecessarily complicated. There's no reason to run a filtered query against each resource type; the filtering should look like (pseudo code):

  FILTER:
      SHOULD:
          INDEX:searchlight TYPE:OS::Nova::Server TERM:tenant_id=abc
          INDEX:searchlight TYPE:OS::Nova::Server TERM:tenant_id=abc
  QUERY: <whatever query is>

The 'common' filters that are getting added (deletion flag, role based) can either be added as a top level MUST to the FILTER part (meaning, 'all of these common things plus one of the SHOULDs plus the query') or inside each SHOULD piece. The former is probably slightly more performant; the latter makes more sense given that facet querying has to use the same information and is self contained in plugins.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/286829

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/286829
Committed: https://git.openstack.org/cgit/openstack/searchlight/commit/?id=845f760ddf5879e04c86147b3bc403c26c2e1507
Submitter: Jenkins
Branch: master

commit 845f760ddf5879e04c86147b3bc403c26c2e1507
Author: Steve McLellan <email address hidden>
Date: Tue Mar 1 14:37:08 2016 -0600

    Apply rbac more consistently

    Previously, all_projects meant that no RBAC was applied at all, which
    in general was not dangerous but did suffer from a potential edge
    condition where inactive documents in an index could be returned from
    searches. More problematically, the logic for all_projects was very
    different from the usual user-level flow which could lead to difficulty
    debugging.

    Even more problematically, the queries were much more complex than
    necessary. This patch simplifies the query construction logic such that
    the query is specified once, and the filters (the user/admin split plus
    the rbac and type filters) are applied in a single filter. This should
    mean that the score makes more sense as it is purely a result of the
    actual query. This does NOT change functional behavior (and thus no
    changes to tests).

    The pseudo logic for search is now:

      ((index= AND type= AND <all_rbac_filters>) OR
       (index= AND type= AND <all_rbac_filters>) OR
       ..... # Repeat for each eligible plugin type)
      AND <user-role-field>=ADMIN/USER
      AND <user-provided-query>

    In addition, this patch adds the ability to enforce RBAC filtering
    even for administrative all_projects queries on a per plugin basis
    with a property allow_admin_ignore_rbac.

    Change-Id: Idef1ef000b7c14fb5bd4810b034b426ba48872fb
    Closes-Bug: #1541115

[Thierry Carrez (ttx)]

This issue was fixed in the openstack/searchlight 0.2.0.0rc1 release candidate.