Tenant Permissions : added the possibility to specify tenants via par… by chrousto · Pull Request #819 · opensearch-project/security

chrousto · 2020-11-01T11:35:16Z

Please provide as much details as possible to get feedback/acceptance on your PR quickly

Category: (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation)
Enhancement / New Feature : specify tenants in roles via parameter substitution (like for indexes and DLS)

Github Issue # or road-map entry, if available: 
#817

Description of changes:
The idea would be that this user :

new-user:
  hash: "*************"
  reserved: false
  hidden: false
  opendistro_security_roles:
  - "role-tenant1"
  attributes:
    attribute1: "tenant1"
  static: false

Would only have access to the tenant tenant1 if the role role-tenant1 was defined like this :

role-tenant1:
 reserved: false
 hidden: false
 cluster_permissions:
 - "read"
 - "cluster:monitor/nodes/stats"
 - "cluster:monitor/task/get"
 tenant_permissions:
 - tenant_patterns:
   - ${attr.internal.attribute1}
   allowed_actions:
   - "kibana_all_write"
 static: false
_meta:
 type: "roles"
 config_version: 2

Why these changes are required?
Enhancement of the way tenants are specified in roles definitions. Allows for more generic roles to be created with information being received from the authorization backend for instance.

What is the old behavior before changes and new behavior after changes? (Please add any example/logs/screen-shot if available)
Before the change, tenants could only be specified statically or via patterns in role definitions

Testing done: (Please provide details of testing done: Unit testing, integration testing and manual testing)
Testing done manually

TO-DOs, if any: (Please describe pending items and provide Github issues# for each of them)
Provide proper tests units.

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

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

cliu123 · 2020-11-10T23:35:35Z

src/main/java/com/amazon/opendistroforelasticsearch/security/securityconf/ConfigModelV7.java

+
+                                //for(String matchingTenant: ) {
+                                //  tuples.add(new Tuple<String, Boolean>(matchingTenant, agr.resolvedActions(tenant.getAllowed_actions()).contains("kibana:saved_objects/*/write")));
+                                //}


Should these be removed if it's not needed?

Hello @cliu123 and thanks for reviewing my PR.
Indeed those lines were useless and I have removed them, please check it out.
Regards,
Christophe.

@cliu123 : did you have a chance to have a look at my reviewed code ?
Is there something I need to improve in order to start the discussion.

Please let me know.
Thanks in advance
Christophe

…thub.com/opendistro-for-elasticsearch/security/pull/819#discussion_r520942228

codecov · 2020-11-11T07:23:11Z

Codecov Report

Merging #819 (91effce) into main (c0b1bac) will increase coverage by 0.00%.
The diff coverage is 90.00%.

@@            Coverage Diff            @@
##               main     #819   +/-   ##
=========================================
  Coverage     64.54%   64.54%           
+ Complexity     3171     3170    -1     
=========================================
  Files           244      244           
  Lines         17124    17131    +7     
  Branches       3036     3038    +2     
=========================================
+ Hits          11052    11057    +5     
  Misses         4531     4531           
- Partials       1541     1543    +2

Impacted Files	Coverage Δ	Complexity Δ
...ticsearch/security/securityconf/ConfigModelV7.java	`63.78% <90.00%> (+0.28%)`	`22.00 <0.00> (ø)`
.../dlic/auth/ldap2/LDAPConnectionFactoryFactory.java	`57.25% <0.00%> (-0.77%)`	`23.00% <0.00%> (-1.00%)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c0b1bac...91effce. Read the comment docs.

…thub.com/opendistro-for-elasticsearch/security/pull/819#discussion_r520942228

francisco-hoo · 2021-02-25T22:18:27Z

Hey @chrousto, do you have any news over this? The ability to take advantage of user attributes for tenant patterns are really useful.
I take a look at your commit code chrousto/security@342707e and the it seems fine (to me), basically you just added the new Tuples<String, Boolean> related to the parameter substitution

chrousto · 2021-02-26T07:25:06Z

Hello @francisco-hoo : No news at all on this, after removing comments according to the first answer, I did not receive any updates on this, even after posting in the forum or sending emails :(
Btw, the last CI build did fail but the error seems to be unrelated to the code I have submitted in the PR, do you know a way to re-run the CI without re-committing ?

Thanks in advance.
Christophe.

…thub.com/opendistro-for-elasticsearch/security/pull/819#discussion_r520942228 / Trigger CI

hardik-k-shah · 2021-03-04T01:06:21Z

@chrousto Thanks for these changes.

Can you please add test cases to cover this new feature?
Also please sync your branch from main with new latest changes. Also, please update your description using new PR-Intake form.

hardik-k-shah · 2021-03-05T00:18:06Z

src/main/java/com/amazon/opendistroforelasticsearch/security/securityconf/ConfigModelV7.java

+                    // passed on as values of users' attributes, we have to make
+                    // sure that we don't allow them to select tenants that do not exist.
+                    if(ConfigModelV7.this.tenants.getCEntries().keySet().contains(tenant)) {
+                        result.put(replaceProperties(tenant, user), rw);


why do we need to call replaceProperties() again? Value was already substitued and stored in tenant variable on line 1068

Hello @hardik-k-shah I have integrated the changes in chrousto/security@a5683bf

hardik-k-shah · 2021-03-05T00:30:40Z

src/main/java/com/amazon/opendistroforelasticsearch/security/securityconf/ConfigModelV7.java

-                                    tuples.add(new Tuple<String, Boolean>(matchingTenant, agr.resolvedActions(tenant.getAllowed_actions()).contains("kibana:saved_objects/*/write")));
+
+                                // find Wildcarded tenant patterns
+                                List<String> wildcardMatchingTenants = WildcardMatcher.from(tenant.getTenant_patterns()).getMatchAny(definedTenants.getCEntries().keySet(), Collectors.toList()) ;


not a wildcard (*).
But it is matching pattern configured in role, so better to rename wildcardMatchingTenants to original variable matchingTenants

Also update comments accordingly.

Hello @hardik-k-shah I have integrated the changes in chrousto/security@a5683bf

hardik-k-shah · 2021-03-11T17:35:51Z

@chrousto There are lot of changes which are not related to your PR topic and it is difficult to review. Can you fix that? If needed, please create new PR.

vrozov · 2021-03-11T18:00:59Z

@chrousto please rebase your PR against upstream/master and force push to chrousto:TenantsParameterSubstitutions branch.

chrousto · 2021-03-11T18:52:36Z

@chrousto please rebase your PR against upstream/master and force push to chrousto:TenantsParameterSubstitutions branch.
Hello @vrozov,
Sorry I was too quick the other day and forgot to rebase , I'll do that tomorrow.
You mean upstream/main instead of upstream/master right ?

hardik-k-shah · 2021-03-11T19:03:03Z

@chrousto please rebase your PR against upstream/master and force push to chrousto:TenantsParameterSubstitutions branch.
Hello @vrozov,
Sorry I was too quick the other day and forgot to rebase , I'll do that tomorrow.
You mean upstream/main instead of upstream/master right ?

Yes, that's right.

…thub.com/opendistro-for-elasticsearch/security/pull/819#discussion_r520942228 / Trigger CI

chrousto · 2021-03-18T14:22:06Z

@cliu123 : do you need more information on this ?
Thanks.

cliu123 · 2021-03-22T16:55:52Z

@chrousto Thanks for contributing!

Can you add Unit Test to cover all the test cases?
Can you use the PR intake form to provide the information needed for review? This PR is one of the examples.

chrousto · 2021-03-24T05:54:15Z

@chrousto Thanks for contributing!

1. Can you add Unit Test to cover all the test cases?

2. Can you use the [PR intake form](https://github.com/opendistro-for-elasticsearch/security/blob/main/.github/pull_request_template.md) to provide the information needed for review? [This PR](https://github.com/opendistro-for-elasticsearch/security/pull/1000) is one of the examples.

Hello

I will find time to write those.
I have updated the initial PR comment with the new form, tell me if that is ok with you.

…ants via parameter substitution.

chrousto · 2021-04-06T17:49:17Z

@chrousto Thanks for contributing!

1. Can you add Unit Test to cover all the test cases?

2. Can you use the [PR intake form](https://github.com/opendistro-for-elasticsearch/security/blob/main/.github/pull_request_template.md) to provide the information needed for review? [This PR](https://github.com/opendistro-for-elasticsearch/security/pull/1000) is one of the examples.

Hello there, I have updated the PR with everything you requested (tell me if that is ok).
Things to note :

I have split the PR in two commits : one for the code and one for the tets, let me know if that works for you or if you prefer a single commit.
When it comes to the code, I have chosen, due the way the securityModel is built, to not be able to specify a user attribute AND a wildcard. This would be possible but would require more code. Indeed, when it comes to tenants, wildcards are evaluated and replaced when the security model is built after roles definitions, but parameter substitution is evaluated dynamically when the user performs a request because the attributes are relative to every users.

Be able to have parameters AND wildcards would be a future improvement (something like this : ${attr.internal.plop}* matching value1 and value1-2 if the user attribute plop was set to value1.

Thanks for your feedback.

cliu123 · 2021-05-03T16:16:21Z

@chrousto Thanks for taking care the requests! As you might have noticed, the repo has been moved from ODFE org to opensearch-project org. We are working on migration from ODFE to OpenSearch currently. The main branch now has a part of the migration(renaming) changes. All changes to be merged into main branch need to be compatible with the OpenSearch naming convention. Could you please look into the changes if it has any conflicts with the new naming convention or any test failures? If not, please rebase it against the main branch. Thanks!

chrousto · 2021-05-04T04:23:02Z

@chrousto Thanks for taking care the requests! As you might have noticed, the repo has been moved from ODFE org to opensearch-project org. We are working on migration from ODFE to OpenSearch currently. The main branch now has a part of the migration(renaming) changes. All changes to be merged into main branch need to be compatible with the OpenSearch naming convention. Could you please look into the changes if it has any conflicts with the new naming convention or any test failures? If not, please rebase it against the main branch. Thanks!

Hello, could you please point me to the naming conventions, I can't seem to find them.
Thanks.

cliu123 · 2021-05-04T04:26:45Z

@chrousto Thanks for taking care the requests! As you might have noticed, the repo has been moved from ODFE org to opensearch-project org. We are working on migration from ODFE to OpenSearch currently. The main branch now has a part of the migration(renaming) changes. All changes to be merged into main branch need to be compatible with the OpenSearch naming convention. Could you please look into the changes if it has any conflicts with the new naming convention or any test failures? If not, please rebase it against the main branch. Thanks!

Hello, could you please point me to the naming conventions, I can't seem to find them.
Thanks.

I believe you can rebase against main branch. If no conflicts, then the PR should be fine. Otherwise, please resovle the conflicts. Most of the naming convention changes are introduced by this PR

peternied · 2022-04-25T19:17:01Z

@chrousto I've recently looked over this change and I think its valuable. I've created a branch main...peternied:TenantsParameterSubstitutions in my fork, you can force push onto your branch if you would like to make progress towards merging.

chrousto · 2022-04-26T08:16:11Z

Hello Peter, I am happy to see interest In this change. I will have to dive into it again in order be able to push properly. Thanks Christophe Le lun. 25 avr. 2022, 21:17, Peter Nied ***@***.***> a écrit :

…

@chrousto <https://github.com/chrousto> I've recently looked over this change and I think its valuable. I've created a branch main...peternied:TenantsParameterSubstitutions <main...peternied:TenantsParameterSubstitutions> in my fork, you can force push onto your branch if you would like to make progress towards merging. — Reply to this email directly, view it on GitHub <#819 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFFOMOXNDFXLYAAY3ZJL5M3VG3VTTANCNFSM4TGPBICA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

peternied · 2022-04-27T22:30:27Z

@chrousto If you'd like, I'm happy to get this pull request merged doing some of the resolving/merging on my own. However, we require a Developer Certificate of Origin.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

If you are willing to reply with the above statement and your permission, I can get this merged - what would you like to do?

chrousto · 2022-04-29T08:00:18Z

Hello, @peternied,

indeed I give you my permission to proceed with the merge :

By submitting this pull request, I (Christophe Chazeau christophe.chazeau@gmail.com) confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Thanks in advance.
Don't hesitate if you need more info.

Christophe.

peternied · 2022-04-29T20:52:40Z

I am migrating this pull request to #1813 so I can rebase and add signatures for DCO

FYI
@cliu123
@francisco-hoo
@hardik-k-shah
@vrozov

chrousto requested a review from a team as a code owner November 1, 2020 11:35

cliu123 reviewed Nov 10, 2020

View reviewed changes

chrousto referenced this pull request in chrousto/security Nov 11, 2020

PR 819 : Remove commented and unused lines as requested in https://gi…

02760e0

…thub.com/opendistro-for-elasticsearch/security/pull/819#discussion_r520942228

chrousto referenced this pull request in chrousto/security Jan 26, 2021

PR 819 : Remove commented and unused lines as requested in https://gi…

7ed4b48

…thub.com/opendistro-for-elasticsearch/security/pull/819#discussion_r520942228

chrousto force-pushed the TenantsParameterSubstitutions branch from 02760e0 to 7ed4b48 Compare January 26, 2021 07:32

cliu123 changed the base branch from master to main February 6, 2021 00:10

chrousto referenced this pull request in chrousto/security Feb 28, 2021

PR 819 : Remove commented and unused lines as requested in https://gi…

07c61c3

…thub.com/opendistro-for-elasticsearch/security/pull/819#discussion_r520942228 / Trigger CI

chrousto force-pushed the TenantsParameterSubstitutions branch from 7ed4b48 to 07c61c3 Compare February 28, 2021 09:58

hardik-k-shah mentioned this pull request Mar 4, 2021

Feature Request : parameter substitution in tenant patterns. #817

Closed

hardik-k-shah reviewed Mar 5, 2021

View reviewed changes

chrousto referenced this pull request in chrousto/security Mar 12, 2021

PR 819 : Remove commented and unused lines as requested in https://gi…

2f1a853

…thub.com/opendistro-for-elasticsearch/security/pull/819#discussion_r520942228 / Trigger CI

chrousto force-pushed the TenantsParameterSubstitutions branch from a5683bf to 12f0a42 Compare March 12, 2021 06:18

cliu123 linked an issue Mar 15, 2021 that may be closed by this pull request

Feature Request : parameter substitution in tenant patterns. #817

Closed

chrousto force-pushed the TenantsParameterSubstitutions branch 4 times, most recently from 17acfe0 to 7d95dd5 Compare April 5, 2021 16:35

chrousto added 2 commits April 6, 2021 19:37

Issue #817 : Tenant Permissions, added the possibility to specify ten…

be2842a

…ants via parameter substitution.

Issue #817 : Tests for tenants parameters substitutions

91effce

chrousto force-pushed the TenantsParameterSubstitutions branch from 7d95dd5 to 91effce Compare April 6, 2021 17:43

chrousto requested a review from a team August 7, 2021 00:35

peternied mentioned this pull request Apr 29, 2022

Tenant Permissions : added the possibility to specify tenants via parameter #1813

Merged

3 tasks

peternied closed this Apr 29, 2022

Conversation

chrousto commented Nov 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cliu123 Nov 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrousto Nov 11, 2020

Choose a reason for hiding this comment

Uh oh!

chrousto Dec 7, 2020

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Nov 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

francisco-hoo commented Feb 25, 2021

Uh oh!

chrousto commented Feb 26, 2021

Uh oh!

hardik-k-shah commented Mar 4, 2021

Uh oh!

hardik-k-shah Mar 5, 2021

Choose a reason for hiding this comment

Uh oh!

chrousto Mar 6, 2021

Choose a reason for hiding this comment

Uh oh!

hardik-k-shah Mar 5, 2021

Choose a reason for hiding this comment

Uh oh!

chrousto Mar 6, 2021

Choose a reason for hiding this comment

Uh oh!

hardik-k-shah commented Mar 11, 2021

Uh oh!

vrozov commented Mar 11, 2021

Uh oh!

chrousto commented Mar 11, 2021

Uh oh!

hardik-k-shah commented Mar 11, 2021

Uh oh!

chrousto commented Mar 18, 2021

Uh oh!

cliu123 commented Mar 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrousto commented Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrousto commented Apr 6, 2021

Uh oh!

cliu123 commented May 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chrousto commented May 4, 2021

Uh oh!

cliu123 commented May 4, 2021

Uh oh!

peternied commented Apr 25, 2022

Uh oh!

chrousto commented Apr 26, 2022 via email

Uh oh!

peternied commented Apr 27, 2022

Uh oh!

chrousto commented Apr 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

peternied commented Apr 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

chrousto commented Nov 1, 2020 •

edited

Loading

cliu123 Nov 10, 2020 •

edited

Loading

codecov bot commented Nov 11, 2020 •

edited

Loading

cliu123 commented Mar 22, 2021 •

edited

Loading

chrousto commented Mar 24, 2021 •

edited

Loading

cliu123 commented May 3, 2021 •

edited

Loading

chrousto commented Apr 29, 2022 •

edited

Loading