Point in time security changes #2094

bharath-techie · 2022-09-19T16:14:14Z

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

Description

- For 'Delete PIT' and 'PIT segments' API, when PIT IDs are passed as part of request, this custom evaluator decode the PITs to indices and resolve the indices with user permissions.
- If user has permission to all indices of PIT , then PIT is permitted to the user
- Only when the user has permissions for all PITs in the request, then we allow the operation.
- For requests which operates on 'all' PITs, we skip the custom evaluator and evaluate via standard code

Alias and data stream behavior :

PIT IDs always contain the resolved indices ( underlying indices ) when saved.
Based on this,
- For alias, user must have either 'index' or 'alias' permission for any PIT operation.
- For data stream, user must have both 'data stream' AND 'backing indices of data stream' permission ( eg : data-stream-11 + .ds-my-data-stream11-000001 ) for any PIT operation.
- With just data stream permission, user will be able to create pit but will not be able to use the PIT ID for other operations such as search without backing indices permission.

Refer to :
#2087 (comment)

Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation)
New feature - Modification to point in time security model
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?

Issues Resolved

#2087

opensearch-project/OpenSearch#3959

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

Testing

[Please provide details of testing done: unit testing, integration testing and manual testing]

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.

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

peternied

Much simplified version of these checks, great work! Other than the few comments, please add integration tests.

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

peternied · 2022-09-20T14:38:57Z

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

+                                            User user, SecurityRoles securityRoles, final String action,
+                                            IndexNameExpressionResolver resolver) {
+        final ImmutableSet<String> pitImmutableIndices = ImmutableSet.copyOf(pitIndices);
+        final IndexResolverReplacer.Resolved pitResolved =


How does this resolve with aliases or data streams?

Good question - don't think this logic handles other than local indices.

final ResolvedIndicesProvider resolvedIndicesProvider = new ResolvedIndicesProvider(request); getOrReplaceAllIndices(request, resolvedIndicesProvider, false); return resolvedIndicesProvider.resolved(indicesOptionsFrom(request))

I think i have to use this logic instead. Let me try debugging and update this.

bharath-techie · 2022-09-20T15:08:20Z

Much simplified version of these checks, great work! Other than the few comments, please add integration tests.

One problem with integration tests is that current framework doesn't let me pass body to 'delete
or 'get' REST requests. Any suggestions ?

I can validate only '_all' requests.

peternied · 2022-09-21T03:25:03Z

For these scenarios that aren't included in the test framework, could you add overloads that cover these scenarios?

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

peternied

Thanks for the update for the tests, There is still an open question about data streams/aliases. Could you add tests for these scenarios - this will confirm the behavior is as expected.

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

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

bharath-techie · 2022-09-26T07:21:14Z

Thanks for the update for the tests, There is still an open question about data streams/aliases. Could you add tests for these scenarios - this will confirm the behavior is as expected.

I've addressed the question about data stream / aliases. And also have added the tests validating the same.

peternied · 2022-09-26T19:19:03Z

@bharath-techie It looks like there are build issues impacting the CI. I'm happy to approve once we see those tests are passing

bharath-techie · 2022-09-27T14:58:54Z

@peternied
Thanks for the review, builds will pass post the 2.4 backport of opensearch PRs. I'll update this PR post that.

codecov-commenter · 2022-10-06T18:20:15Z

Codecov Report

Merging #2094 (1d4365d) into main (ca4917c) will increase coverage by 0.06%.
The diff coverage is 97.22%.

@@             Coverage Diff              @@
##               main    #2094      +/-   ##
============================================
+ Coverage     61.01%   61.08%   +0.06%     
- Complexity     3229     3241      +12     
============================================
  Files           256      257       +1     
  Lines         18101    18135      +34     
  Branches       3229     3235       +6     
============================================
+ Hits          11044    11077      +33     
  Misses         5475     5475              
- Partials       1582     1583       +1

Impacted Files	Coverage Δ
...ch/security/privileges/PitPrivilegesEvaluator.java	`96.42% <96.42%> (ø)`
.../opensearch/security/OpenSearchSecurityPlugin.java	`79.95% <100.00%> (+0.08%)`	⬆️
...earch/security/privileges/PrivilegesEvaluator.java	`72.38% <100.00%> (+0.35%)`	⬆️
...earch/security/resolver/IndexResolverReplacer.java	`65.34% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

bharath-techie · 2022-10-06T18:32:01Z

Hi @peternied,
Build is green finally, please review the changes.

peternied

@bharath-techie Excellent work! LGTM

DarshitChanpura

LGTM! Thanks @bharath-techie !

Description For 'Delete PIT' and 'PIT segments' API, when PIT IDs are passed as part of request, this custom evaluator decode the PITs to indices and resolve the indices with user permissions. If user has permission to all indices of PIT, then PIT is permitted to the user. Only when the user has permissions for all PITs in the request, then we allow the operation. For requests which operates on 'all' PITs, we skip the custom evaluator and evaluate via standard code Alias and data stream behavior : PIT IDs always contain the resolved indices ( underlying indices ) when saved. Based on this, For alias, user must have either 'index' or 'alias' permission for any PIT operation. For data stream, user must have both 'data stream' AND 'backing indices of data stream' permission ( eg : data-stream-11 + .ds-my-data-stream11-000001 ) for any PIT operation. With just data stream permission, user will be able to create pit but will not be able to use the PIT ID for other operations such as search without backing indices permission. Signed-off-by: Bharathwaj G <bharath78910@gmail.com> Signed-off-by: Bharathwaj G <58062316+bharath-techie@users.noreply.github.com> (cherry picked from commit 207cfcc)

Description For 'Delete PIT' and 'PIT segments' API, when PIT IDs are passed as part of request, this custom evaluator decode the PITs to indices and resolve the indices with user permissions. If user has permission to all indices of PIT, then PIT is permitted to the user. Only when the user has permissions for all PITs in the request, then we allow the operation. For requests which operates on 'all' PITs, we skip the custom evaluator and evaluate via standard code Alias and data stream behavior : PIT IDs always contain the resolved indices ( underlying indices ) when saved. Based on this, For alias, user must have either 'index' or 'alias' permission for any PIT operation. For data stream, user must have both 'data stream' AND 'backing indices of data stream' permission ( eg : data-stream-11 + .ds-my-data-stream11-000001 ) for any PIT operation. With just data stream permission, user will be able to create pit but will not be able to use the PIT ID for other operations such as search without backing indices permission. Signed-off-by: Bharathwaj G <bharath78910@gmail.com> Signed-off-by: Bharathwaj G <58062316+bharath-techie@users.noreply.github.com> (cherry picked from commit 207cfcc) Co-authored-by: Bharathwaj G <58062316+bharath-techie@users.noreply.github.com>

Description For 'Delete PIT' and 'PIT segments' API, when PIT IDs are passed as part of request, this custom evaluator decode the PITs to indices and resolve the indices with user permissions. If user has permission to all indices of PIT, then PIT is permitted to the user. Only when the user has permissions for all PITs in the request, then we allow the operation. For requests which operates on 'all' PITs, we skip the custom evaluator and evaluate via standard code Alias and data stream behavior : PIT IDs always contain the resolved indices ( underlying indices ) when saved. Based on this, For alias, user must have either 'index' or 'alias' permission for any PIT operation. For data stream, user must have both 'data stream' AND 'backing indices of data stream' permission ( eg : data-stream-11 + .ds-my-data-stream11-000001 ) for any PIT operation. With just data stream permission, user will be able to create pit but will not be able to use the PIT ID for other operations such as search without backing indices permission. Signed-off-by: Bharathwaj G <bharath78910@gmail.com> Signed-off-by: Bharathwaj G <58062316+bharath-techie@users.noreply.github.com> Signed-off-by: Stephen Crawford <steecraw@amazon.com>

…project#2224) Description For 'Delete PIT' and 'PIT segments' API, when PIT IDs are passed as part of request, this custom evaluator decode the PITs to indices and resolve the indices with user permissions. If user has permission to all indices of PIT, then PIT is permitted to the user. Only when the user has permissions for all PITs in the request, then we allow the operation. For requests which operates on 'all' PITs, we skip the custom evaluator and evaluate via standard code Alias and data stream behavior : PIT IDs always contain the resolved indices ( underlying indices ) when saved. Based on this, For alias, user must have either 'index' or 'alias' permission for any PIT operation. For data stream, user must have both 'data stream' AND 'backing indices of data stream' permission ( eg : data-stream-11 + .ds-my-data-stream11-000001 ) for any PIT operation. With just data stream permission, user will be able to create pit but will not be able to use the PIT ID for other operations such as search without backing indices permission. Signed-off-by: Bharathwaj G <bharath78910@gmail.com> Signed-off-by: Bharathwaj G <58062316+bharath-techie@users.noreply.github.com> (cherry picked from commit 207cfcc) Co-authored-by: Bharathwaj G <58062316+bharath-techie@users.noreply.github.com>

bharath-techie requested a review from a team September 19, 2022 16:14

bharath-techie force-pushed the main branch from ddfe5d2 to 98cb17f Compare September 19, 2022 16:16

Point in time security changes

0e0affc

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

bharath-techie force-pushed the main branch from 98cb17f to 0e0affc Compare September 19, 2022 16:17

peternied requested changes Sep 20, 2022

View reviewed changes

Addressing comments

4d10bb3

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

peternied requested changes Sep 22, 2022

View reviewed changes

src/test/java/org/opensearch/security/PitIntegrationTests.java Outdated Show resolved Hide resolved

Tests for alias and data stream

22264ce

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

peternied added the v2.4.0 'Issues and PRs related to version v2.4.0' label Sep 27, 2022

dhruv16dhr mentioned this pull request Sep 30, 2022

[META] Point In Time Search Issue List opensearch-project/OpenSearch#3959

Open

47 tasks

Merge branch 'main' of https://github.com/opensearch-project/security

1d4365d

peternied approved these changes Oct 6, 2022

View reviewed changes

peternied requested review from cwperks and DarshitChanpura October 6, 2022 18:42

DarshitChanpura approved these changes Oct 7, 2022

View reviewed changes

DarshitChanpura merged commit 207cfcc into opensearch-project:main Oct 7, 2022

peternied added backport 2.x backport to 2.x branch backport 2.4 labels Nov 2, 2022

opensearch-trigger-bot bot mentioned this pull request Nov 2, 2022

[Backport 2.4] Point in time security changes #2223

Merged

opensearch-trigger-bot bot mentioned this pull request Nov 2, 2022

[Backport 2.x] Point in time security changes #2224

Merged

peternied mentioned this pull request Nov 2, 2022

Release Version 2.4.0 #2160

Closed

23 tasks

cwperks mentioned this pull request Nov 3, 2022

2.4 Release Notes for Security Plugin #2231

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Point in time security changes #2094

Point in time security changes #2094

bharath-techie commented Sep 19, 2022 •

edited

Loading

peternied left a comment

peternied Sep 20, 2022

bharath-techie Sep 22, 2022

bharath-techie commented Sep 20, 2022

peternied commented Sep 21, 2022

peternied left a comment

bharath-techie commented Sep 26, 2022

peternied commented Sep 26, 2022

bharath-techie commented Sep 27, 2022

codecov-commenter commented Oct 6, 2022 •

edited

Loading

bharath-techie commented Oct 6, 2022

peternied left a comment

DarshitChanpura left a comment

Point in time security changes #2094

Point in time security changes #2094

Conversation

bharath-techie commented Sep 19, 2022 • edited Loading

Description

Issues Resolved

Testing

Check List

peternied left a comment

Choose a reason for hiding this comment

peternied Sep 20, 2022

Choose a reason for hiding this comment

bharath-techie Sep 22, 2022

Choose a reason for hiding this comment

bharath-techie commented Sep 20, 2022

peternied commented Sep 21, 2022

peternied left a comment

Choose a reason for hiding this comment

bharath-techie commented Sep 26, 2022

peternied commented Sep 26, 2022

bharath-techie commented Sep 27, 2022

codecov-commenter commented Oct 6, 2022 • edited Loading

Codecov Report

bharath-techie commented Oct 6, 2022

peternied left a comment

Choose a reason for hiding this comment

DarshitChanpura left a comment

Choose a reason for hiding this comment

bharath-techie commented Sep 19, 2022 •

edited

Loading

codecov-commenter commented Oct 6, 2022 •

edited

Loading