Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix #13 fix for : DLS overrides broader permissions #1078

Closed

Conversation

chrousto
Copy link

@chrousto chrousto commented Mar 17, 2021

opendistro-for-elasticsearch/security pull request intake form

Please provide as much details as possible to get feedback/acceptance on your PR quickly
This PR fixes the issue #13 that describes how a user with :

  1. one or more role that has some DLS configuration for document filtering
  2. one or more role with no DLS filtering (i.e. has access to everything)

still sees only the conjunction of DLS restriction instead of the conjunction of DLS + everything = everything.

  1. Category: (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation)
    Bug fix

  2. Github Issue # or road-map entry, if available:
    DLS overrides broader permissions #13

  3. Description of changes:
    The idea of the change is to position a dummy dlsfullaccess string inside the DLS object, this way when the whole map is parsed, if this string is found, all other entry are cleared for the specified index pattern.

  4. Why these changes are required?
    This is a a bug the behavior of the plugin does not reflect what has been configured.

  5. What is the old behavior before changes and new behavior after changes? (Please add any example/logs/screen-shot if available)

##############################
Create internal user ...

{
  "status" : "CREATED",
  "message" : "'my_user' created."
}

##############################
Create 'regular' role with dls on index 'my_index' ...

{
  "status" : "CREATED",
  "message" : "'my_role1' created."
}

##############################
Map 'regular' role with dls filtering on index my_index ...

{"status":"CREATED","message":"'my_role1' created."}
##############################
Create 'admin' (all access) role without dls filtering ...

{
  "status" : "CREATED",
  "message" : "'my_admin_role_without_dls_match_all' created."
}

##############################
Map 'admin' (all access) role without dls filtering ...

{"status":"OK","message":"'my_admin_role_without_dls_match_all' updated."}
##############################
Get authinfo ...

{
  "user" : "User [name=my_user, backend_roles=[], requestedTenant=null]",
  "user_name" : "my_user",
  "user_requested_tenant" : null,
  "remote_address" : "[::1]:55724",
  "backend_roles" : [ ],
  "custom_attribute_names" : [ ],
  "roles" : [
    "my_role1",
    "own_index",
    "my_admin_role_without_dls_match_all"
  ],
  "tenants" : {
    "my_user" : true
  },
  "principal" : null,
  "peer_certificates" : "0",
  "sso_logout_url" : null
}

##############################
Get my_index data ...

{
  "took" : 86,
  "timed_out" : false,
  "_shards" : {
    "total" : 1,
    "successful" : 1,
    "skipped" : 0,
    "failed" : 0
  },
  "hits" : {
    "total" : {
      "value" : 1,
      "relation" : "eq"
    },
    "max_score" : 2.0,
    "hits" : [
      {
        "_index" : "my_index",
        "_type" : "_doc",
        "_id" : "1",
        "_score" : 2.0,
        "_source" : {
          "field1" : "value1"
        }
      }
    ]
  }
}

Here the user should see more fields.
I will provide more data/scripts to test against from within the PR.
I have provided proper test units from within the PR

  1. Testing done: (Please provide details of testing done: Unit testing, integration testing and manual testing)
    The testing has, for the moment, been done only with custom script, I have yet to familiarize myself with the testing framework provided with the code.

  2. TO-DOs, if any: (Please describe pending items and provide Github issues# for each of them)
    Not that I know or can think of.

  3. Is it backport from main branch? (If yes, please add backport PR # and commits #)
    No

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or

(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or

(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.

(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.

@chrousto chrousto requested a review from a team as a code owner March 17, 2021 10:36
@chrousto chrousto force-pushed the DLS_all_access_issue_13 branch from 8bd293b to 09b7049 Compare March 18, 2021 07:53
@cliu123
Copy link
Member

cliu123 commented Mar 22, 2021

Thanks for contributing! Can you add Unit Tests?

@cliu123
Copy link
Member

cliu123 commented Apr 9, 2021

Thanks for contributing! Can you add Unit Tests?

@chrousto Thanks for adding UTs for the PR819! Please let us know once the UTs are ready for this PR.

@chrousto
Copy link
Author

Thanks for contributing! Can you add Unit Tests?

@chrousto Thanks for adding UTs for the PR819! Please let us know once the UTs are ready for this PR.

Hey @cliu123 ,
I am in the process of writing UTs for this one too.
I will update you asap.

@chrousto chrousto force-pushed the DLS_all_access_issue_13 branch from bfbb44e to f994a81 Compare April 13, 2021 05:19
@chrousto chrousto force-pushed the DLS_all_access_issue_13 branch from f994a81 to f72f2d2 Compare April 13, 2021 05:21
@chrousto chrousto force-pushed the DLS_all_access_issue_13 branch from f72f2d2 to 2d237ff Compare April 16, 2021 11:59
@chrousto chrousto requested a review from a team August 7, 2021 00:35
@rursprung
Copy link
Contributor

i think this is the same as i reported in #1572 (sorry, didn't spot your ticket at the time) and the solution for that has just been merged (see #1735).

@davidlago
Copy link

Thanks for the connection, @rursprung! Closing this PR.

@davidlago davidlago closed this Apr 13, 2022
gaobinlong pushed a commit to gaobinlong/security that referenced this pull request Aug 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants