Changes to make PIT security model granular #2053

bharath-techie · 2022-08-28T17:26:14Z

Signed-off-by: Bharathwaj G bharath78910@gmail.com

Description

As part of point in time opensearch review, we got comments which will help PIT security model more granular - where each PIT can be permitted or rejected based on user's indices permissions.
opensearch-project/OpenSearch#4064 (comment)

Design doc - opensearch-project/OpenSearch#3960
This PR contains the changes for same.
As part of this change ,

All PIT APIs which are user facing will be using 'indices:data/read' action that is inline with search semantics
For 'List all PITs' API, we'll return only the PITs for which the user has access to.
- If user has access to all indices part of the PIT, then the PIT will be permitted
- If user has no access to any index of PIT, then we'll not give access to that PIT
- If user has no access to any PITs or there are no PITs in cluster, then we'll throw permission error for the API request
For 'Delete list of PITs' API
- User needs to have access for all indices of all PITs, only then we'll allow deletion otherwise we'll fail the operation. ( This is inline with other APIs where explicit values are given instead of 'all' or '*' )
For 'Delete all PITs' API
- If user has access to all indices part of the PIT, then the PIT will be permitted and deleted
- If user has no access to any index of PIT, then we'll not delete the PIT
- If user has no access to any PITs or there are no PITs in cluster,, then we'll throw permission error for the API request
For 'PIT segments API for list of PITs' API
- User needs to have access for all indices of all PITs, only then we'll allow PIT segments API otherwise we'll fail the operation. ( This is inline with other APIs where explicit values are given instead of 'all' or '*' )
For 'PIT segments API for all PITs' API
- If user has access to all indices part of the PIT, then the PIT will be permitted and segments info will be shown
- If user has no access to any index of PIT, then we'll not permit the PIT
- If user has no access to any PITs or there are no PITs in cluster,, then we'll throw permission error for the API request

Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation)
New feature - Modification to point in time security model - It is a new feature and not released.
Why these changes are required?
We're making these changes to enhance the security model of PIT as per review comments from opensearch community
What is the old behavior before changes and new behavior after changes?
Add security changes for point in time API #2033 -- this is the old behavior and the above description gives an idea of new behavior. Basically old model gives user access to all PIT contexts or none of PIT contexts and the new model allows permission for PITs based on the underlying indices.

Issues Resolved

opensearch-project/OpenSearch#3960

Is this a backport? If so, please add backport PR # and/or commits #
2.3 / 2.x backport

Testing

[Please provide details of testing done: unit testing, integration testing and manual testing]
Tested all the changes with opensearch + security plugin locally. All changes are working as expected.

Check List

New functionality includes testing
New functionality has been documented
Commits are signed per the DCO using --signoff

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.

bharath-techie · 2022-08-28T17:33:00Z

@reta @Bukhtawar please review

bharath-techie · 2022-08-28T17:34:19Z

Build is failing because all the dependencies are in opensearch main branch ( 3.0.0 )

Signed-off-by: Bharathwaj G <bharath78910@gmail.com>

bharath-techie · 2022-09-02T15:04:50Z

@peternied @cliu123 please review this PR.

Bukhtawar · 2022-09-02T15:14:40Z

src/main/resources/static_config/static_action_groups.yml

+    - "indices:data/read/point_in_time/delete"
+    - "indices:data/read/point_in_time/readall"
+    - "indices:data/read/search"
+    - "cluster:admin/point_in_time/read_from_nodes"


Sorry, for my understanding what is read_from_nodes?

This is the parent action that basically reads all active Point in time searches from nodes.

This the action for NodesGetAllPitAction.

We can rename it as well, if the name is confusing.

cliu123 · 2022-09-02T21:40:13Z

src/main/java/org/opensearch/security/resolver/IndexResolverReplacer.java

+                        final ImmutableSet<String> allIndices,
+                        final ImmutableSet<String> originalRequested,
+                        final ImmutableSet<String> remoteIndices,
+                        IndicesOptions indicesOptions) {


Please avoid the format changes if not necessary.

src/main/java/org/opensearch/security/privileges/PitAccessEvaluator.java

cliu123 · 2022-09-04T05:28:29Z

src/main/java/org/opensearch/security/privileges/PitAccessEvaluator.java

+        // Skip pit evaluation for "NodesGetAllPITs" action, since it fetches all PITs across the cluster
+        // for privilege evaluation
+        if(action.startsWith("cluster:admin")) {
+            return presponse;


How is the permission for NodesGetAllPITs evaluated if requests bypass privilege evaluator? Or is there no need to have access control on NodesGetAllPITs?

It'll still get evaluated, we are just not evaluating it as part of pit access evaluator.

We're not marking presponse as complete, so it'll continue down the privilegeEvaluator code flow.

In short, User needs the action permission to execute nodes get all pits.

@bharath-techie looks like the permission for NodesGetAllPITs is designed to be evaluated in PitPrivilegesEvaluator.java? Would you please point me to where it gets evaluated and explain a bit more about how it gets evaluated?

As long as we don't mark the presponse as complete, it'll continue down the flow in PrivilegesEvaluator

// check access for point in time requests if(pitAccessEvaluator.evaluate(request, clusterService, user, securityRoles, action0, resolver, dcm.isDnfofForEmptyResultsEnabled(), presponse).isComplete()) { return presponse; }

It'll go inside this if block for evaluation after we return without marking it complete https://github.com/opensearch-project/security/pull/2053/files#diff-e746c76b234a31b6f95d428b7c40dabf805b609b110c560902c967db8f83fa7aR303

src/main/java/org/opensearch/security/privileges/PitAccessEvaluator.java

cliu123 · 2022-09-04T05:47:59Z

src/main/java/org/opensearch/security/privileges/PitAccessEvaluator.java

+        List<ListPitInfo> pitInfos = getAllPitInfos((GetAllPitNodesRequest) request);
+        // if cluster has no PITs, then allow the operation to pass with empty response
+        if(pitInfos.isEmpty()) {
+            presponse.allowed = true;


The authorization on a request should not be decided by whether there are PITs in a cluster. It should always be evaluated with permissions instead. If the user does not have right permissions, the request should be denied no matter whether there are PITs in a cluster or how many PITs there are.

If the user does not have permissions, we do throw exception. I'll add some ITs which can assert the same.

Now this logic makes sure that we don't throw permission exception when there are simply no PITs created in the cluster.

Now this logic makes sure that we don't throw permission exception when there are simply no PITs created in the cluster.

If a user does not have PIT search permissions, even if there are not PITs in the cluster, the request still should be denied. As I said, authorizing a request or not should only be decided by the permissions the user has(privilege evaluator).

Addressed the comment, we're denying if there are no pits so resolving this and similar ones

I agree with @cliu123 - we are divulging information to the user they should not have - if PITs exist or not.

If you were building a client to connect with OpenSearch and it called this API and got a 200 because it was empty, but then someone added a PIT your requests would suddenly fail due to a side effect - best to be as consistent as possible.

so as long as the user does not have PITs , we throw 403. The behavior is consistent and the above does not happen.

User might not have PITs because :

PITs are present in the cluster but the user does not have access

PITs are not present in the cluster

cliu123 · 2022-09-04T05:56:06Z

src/main/java/org/opensearch/security/privileges/PitAccessEvaluator.java

+                permittedPits.add(pitToPitInfoMap.get(pitId));
+            }
+        }
+        if (permittedPits.size() > 0) {


When a user has right permissions, even if permittedPits.size() equals to 0, the request also should be authorized and get empty in the response. There could be 0 PITs in the cluster.

That is the case we've handled in the line 124, we authorize the user. Because the whole custom logic just ensures if PIT IDs are present, the user should have associated indices permissions.

If not, at this point the user does have action permissions and there are no PITs present in cluster, so we mark operation as success.

src/main/java/org/opensearch/security/privileges/PitAccessEvaluator.java

cliu123 · 2022-09-04T06:01:56Z

src/main/java/org/opensearch/security/privileges/PitAccessEvaluator.java

+            }
+        }
+        // If there are any PITs for which the user has access to, then allow operation otherwise fail.
+        if(permittedPits.size() > 0) {


When a user has right permissions, even if permittedPits.size() equals to 0, the request also should be authorized and get empty in the response. There could be 0 PITs in the cluster.

@cliu123 i tried making this work, but looks like for index actions, there is no way to authorize the user when there are no indices to validate against.

dnfOfEnabled flag is used for indices actions where for empty we permit , otherwise we throw forbidden error.

can you please suggest if there are any alternate approaches ?

Here is a potential alternative approach:

Collect authorization information for the user

Does the user have access to any PITs? (If no- permission is denied)

Does the user have access to the PITs in the request? (If no- permission is denied)

That is what we are doing currently when the config is false ( which is default ).
This config is used in search and other index actions as well, so can we please take a look at this config? it is designed not to throw forbidden in case of empty results.

src/main/java/org/opensearch/security/privileges/PitAccessEvaluator.java

Signed-off-by: Bharathwaj G <bharath78910@gmail.com>

Bukhtawar · 2022-09-05T17:21:59Z

src/main/java/org/opensearch/security/privileges/PitPrivilegesEvaluator.java

+
+        OpenSearchSecurityPlugin.GuiceHolder.getPitService().getAllPits(latchedActionListener);
+
+        if(!latch.await(15, TimeUnit.SECONDS)) {


@reta as per this discussion we need to get rid of this latch since this has the potential to block http request handler threads.

Do you have thoughts on how we could have a reference to the response listener or perform non-blocking waits,

Sorry @Bukhtawar , I came a bit late, seems like this code is already gone 🥳

@bharath-techie would you be able to shed some light if you still have that commit handy

@reta apologies let's see if we that commit

Signed-off-by: Bharathwaj G <bharath78910@gmail.com>

bharath-techie · 2022-09-06T15:08:25Z

@elfisher @cliu123 please prioritize reviewing this PR as this is a 2.3 release blocker.

peternied

I have concerns about the processing in the pit privileges evaluator, if there are updates or bugs create security issues. Please take a look at the individual comments, happy to discuss if there are alternatives or other considerations to weight.

src/main/java/org/opensearch/security/privileges/PitPrivilegesEvaluator.java

src/test/java/org/opensearch/security/PitIntegrationTests.java

Signed-off-by: Bharathwaj G <bharath78910@gmail.com>

elfisher · 2022-09-07T14:25:11Z

Hi I want to confirm this will work with relevant predefined roles. This should at minimum work with all_access, read_all, and read_all_and_monitor. Thanks!

peternied

Thanks for the updates, there are still many unresolved threads please take a look.

One way we could see about reviewing/merging this change faster, I think there is less discussion around the workflows that intersect with handleExplicitPitsAccess. Consider splitting this PR into multiple parts to so we can make progress independently

peternied · 2022-09-08T14:45:44Z

src/main/java/org/opensearch/security/privileges/PitPrivilegesEvaluator.java

+                permittedPits.add(pitToPitInfoMap.get(pitId));
+            }
+        }
+        if (permittedPits.size() > 0) {


By hiding the inaccessible PITs we are making it harder to understand what "ALL" means. While we are just getting this feature off the ground I'd like us to use the most restrictive framing of permissions that we could open up in future revisions.

I understand this change requires more work from the user configuring the cluster; however, I think adding a role like the following better communicates what is secured and what isn't to the operator of the cluster.

point_in_time_all_index_manage: index_permissions: - index_patterns: - "*" allowed_actions: - "manage_point_in_time"

src/test/java/org/opensearch/security/PitIntegrationTests.java

peternied · 2022-09-08T14:48:17Z

src/test/resources/roles.yml

+  hidden: false
+  description: "Migrated from v6 (all types mapped)"
+  cluster_permissions:
+    - "cluster:admin/point_in_time/read_from_nodes"


I am concerned about adding cluster level permission it blurs the line between the index based permissions and PIT which are tightly coupled to indices, I know there is some discussion around it and I'd like to focus that discussion separately from the rest of this work to help get this PR merged more quickly - is that possible?

Without cluster level permission we'd not be able to get all PITs from the cluster, as the request doesn't have indices as part of it. Any suggestions ?

Is there a place where all of the different Rest APIs are documented in the form below? I would like to be able to cross-reference the actions and associated permissions that are needed to perform them, I think that might help our understanding.

Create point in time

POST "/<target indices>/point_in_time?*keep_alive*=1h&routing=&expand_wildcards=&preference="

opensearch-project/OpenSearch#3960 -- is the design doc

The doc and the comments on this PR have evolved over time and it is not clear to me what the permission setup, user action, and result should be for the many scenarios embodied - please make this as clear as possible.

Not only would this help us understand the specific scope and bounds of your request, this would be of great value for writing the end-user facing permissions documentation for this feature

peternied · 2022-09-08T14:56:16Z

src/main/java/org/opensearch/security/privileges/PitAccessEvaluator.java

+        List<ListPitInfo> pitInfos = getAllPitInfos((GetAllPitNodesRequest) request);
+        // if cluster has no PITs, then allow the operation to pass with empty response
+        if(pitInfos.isEmpty()) {
+            presponse.allowed = true;


I agree with @cliu123 - we are divulging information to the user they should not have - if PITs exist or not.

If you were building a client to connect with OpenSearch and it called this API and got a 200 because it was empty, but then someone added a PIT your requests would suddenly fail due to a side effect - best to be as consistent as possible.

peternied · 2022-09-08T15:02:32Z

src/main/java/org/opensearch/security/privileges/PitAccessEvaluator.java

+            }
+        }
+        // If there are any PITs for which the user has access to, then allow operation otherwise fail.
+        if(permittedPits.size() > 0) {


Here is a potential alternative approach:

Collect authorization information for the user

Does the user have access to any PITs? (If no- permission is denied)

Does the user have access to the PITs in the request? (If no- permission is denied)

src/main/java/org/opensearch/security/privileges/PitPrivilegesEvaluator.java

bharath-techie force-pushed the pitsecurity branch from 62c6a15 to 66058f1 Compare August 28, 2022 17:43

Changes to make PIT security model granular

d5c482e

Signed-off-by: Bharathwaj G <bharath78910@gmail.com>

bharath-techie force-pushed the pitsecurity branch from 66058f1 to d5c482e Compare September 2, 2022 14:45

bharath-techie marked this pull request as ready for review September 2, 2022 14:47

bharath-techie requested a review from a team September 2, 2022 14:47

Bukhtawar reviewed Sep 2, 2022

View reviewed changes

cliu123 reviewed Sep 2, 2022

View reviewed changes

src/main/java/org/opensearch/security/privileges/PitAccessEvaluator.java Outdated Show resolved Hide resolved

cliu123 reviewed Sep 4, 2022

View reviewed changes

src/main/java/org/opensearch/security/privileges/PitAccessEvaluator.java Outdated Show resolved Hide resolved

cliu123 reviewed Sep 4, 2022

View reviewed changes

src/main/java/org/opensearch/security/privileges/PitAccessEvaluator.java Outdated Show resolved Hide resolved

cliu123 reviewed Sep 4, 2022

View reviewed changes

src/main/java/org/opensearch/security/privileges/PitAccessEvaluator.java Outdated Show resolved Hide resolved

cliu123 reviewed Sep 4, 2022

View reviewed changes

src/main/java/org/opensearch/security/privileges/PitAccessEvaluator.java Outdated Show resolved Hide resolved

cliu123 reviewed Sep 4, 2022

View reviewed changes

src/main/java/org/opensearch/security/privileges/PitAccessEvaluator.java Outdated Show resolved Hide resolved

cliu123 reviewed Sep 4, 2022

View reviewed changes

src/main/java/org/opensearch/security/privileges/PitAccessEvaluator.java Outdated Show resolved Hide resolved

Addressing comments , using dnfOfEnabled

953748f

Signed-off-by: Bharathwaj G <bharath78910@gmail.com>

bharath-techie force-pushed the pitsecurity branch from 2e94765 to 70c0d02 Compare September 5, 2022 14:59

Addressing comments , using dnfOfEmptyResultsEnabled

dd830b4

Signed-off-by: Bharathwaj G <bharath78910@gmail.com>

bharath-techie force-pushed the pitsecurity branch from 70c0d02 to dd830b4 Compare September 5, 2022 15:10

Bukhtawar reviewed Sep 5, 2022

View reviewed changes

Changes to separate list all pits call from security plugin

eeff45e

Signed-off-by: Bharathwaj G <bharath78910@gmail.com>

peternied requested changes Sep 6, 2022

View reviewed changes

Addressing comments

4a8ad4c

Signed-off-by: Bharathwaj G <bharath78910@gmail.com>

bharath-techie force-pushed the pitsecurity branch from 7463e43 to 4a8ad4c Compare September 6, 2022 15:47

bharath-techie added 3 commits September 6, 2022 22:10

Addressing comments

4380e77

Signed-off-by: Bharathwaj G <bharath78910@gmail.com>

Addressing comments - enhancing integ tests

e440c35

Signed-off-by: Bharathwaj G <bharath78910@gmail.com>

Addressing comments - correcting roles permissions

54d80cf

Signed-off-by: Bharathwaj G <bharath78910@gmail.com>

bharath-techie force-pushed the pitsecurity branch from 3b222ef to 54d80cf Compare September 6, 2022 17:15

reta mentioned this pull request Sep 6, 2022

Added rest layer changes for List all PITs and PIT segments opensearch-project/OpenSearch#4388

Merged

6 tasks

peternied requested changes Sep 8, 2022

View reviewed changes

bharath-techie closed this Sep 19, 2022


		OpenSearchSecurityPlugin.GuiceHolder.getPitService().getAllPits(latchedActionListener);

		if(!latch.await(15, TimeUnit.SECONDS)) {

Changes to make PIT security model granular #2053

Changes to make PIT security model granular #2053

Conversation

bharath-techie commented Aug 28, 2022 • edited Loading

Description

Issues Resolved

Testing

Check List

bharath-techie commented Aug 28, 2022

bharath-techie commented Aug 28, 2022 • edited Loading

bharath-techie commented Sep 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cliu123 Sep 4, 2022 • edited Loading

Choose a reason for hiding this comment

bharath-techie Sep 4, 2022 • edited Loading

Choose a reason for hiding this comment

cliu123 Sep 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bharath-techie commented Sep 6, 2022

peternied left a comment

Choose a reason for hiding this comment

elfisher commented Sep 7, 2022

peternied left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Create point in time

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bharath-techie commented Aug 28, 2022 •

edited

Loading

bharath-techie commented Aug 28, 2022 •

edited

Loading

cliu123 Sep 4, 2022 •

edited

Loading

bharath-techie Sep 4, 2022 •

edited

Loading

cliu123 Sep 4, 2022 •

edited

Loading