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

Add security changes for point in time API #2033

Merged
merged 1 commit into from
Aug 18, 2022

Conversation

bharath-techie
Copy link
Contributor

@bharath-techie bharath-techie commented Aug 17, 2022

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

Description

The existing model requires indices read access to 'pit delete' and 'list all pits' to all users.
So changing the action names to 'cluster:admin/<action_name>".

Now , we can't add these cluster permissions to 'cluster_composite_ops' or 'ops_ro' since user has access to 'my_index' role which has access to 'cluster_composite_ops' which will make all users to access list and delete pit.

So changing the approach to new one below.
Add point in time permissions to 'manage_point_in_time' which is part of default static action groups.

Point in time apis - and associated action names :

  1. Create PIT - - "indices:data/read/point_in_time/create" ( indices:data/read is chosen because this will make sure users have read permission to the passed indices )
  2. Delete PIT - "cluster:admin/point_in_time/delete" ( cluster:admin/* is used because the apis are not dependent on indices like create pit and should just work based on this standalone permission )
  3. List all PITs - "cluster:admin/point_in_time/read*"
  4. PIT segments API - "indices:monitor/point_in_time/segments"

All the above actions are added to new action group 'manage_point_in_time'

  • Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation)
    New feature - Point in time feature
  • Why these changes are required?
    These changes are required for access of various point in time APIs
  • What is the old behavior before changes and new behavior after changes?
    This is a new feature

Design document:
opensearch-project/OpenSearch#3960

Api changes:
opensearch-project/OpenSearch#4064 - create pit and delete pit api
opensearch-project/OpenSearch#4016 - list all

Issues Resolved

opensearch-project/OpenSearch#3959

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

Testing

Tested locally by running opensearch server alongside security plugin.
Only when index permissions and 'manage_point_in_time' action group permission is present , create api succeeds
For rest of the APIs, 'manage_point_in_time' action group permission controls whether the api can be accessible or not, which has been tested as well.

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>
@codecov-commenter
Copy link

codecov-commenter commented Aug 17, 2022

Codecov Report

Merging #2033 (abfabe1) into main (f4b3a3a) will increase coverage by 0.00%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##               main    #2033   +/-   ##
=========================================
  Coverage     61.06%   61.07%           
- Complexity     3229     3230    +1     
=========================================
  Files           256      256           
  Lines         18070    18070           
  Branches       3220     3220           
=========================================
+ Hits          11035    11036    +1     
+ Misses         5463     5461    -2     
- Partials       1572     1573    +1     
Impacted Files Coverage Δ
...iance/ComplianceIndexingOperationListenerImpl.java 63.23% <0.00%> (+1.47%) ⬆️

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

@bharath-techie bharath-techie changed the title Point in time API security changes Add security changes for point in time API Aug 17, 2022
@bharath-techie bharath-techie marked this pull request as ready for review August 17, 2022 10:54
@bharath-techie bharath-techie requested a review from a team August 17, 2022 10:54
@bharath-techie
Copy link
Contributor Author

@cliu123 @peternied please review

@peternied
Copy link
Member

@bharath-techie Have these changes been tested, could you describe how this was done and what was covered? There was a previous permissions pull request that didn't cover the full scenarios.

@bharath-techie
Copy link
Contributor Author

@bharath-techie Have these changes been tested, could you describe how this was done and what was covered? There was a previous permissions pull request that didn't cover the full scenarios.

Added testing section of PR. Tested the changes locally to verify the changes.

Copy link

@Bukhtawar Bukhtawar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for those updates

@cliu123 cliu123 merged commit 6b7a586 into opensearch-project:main Aug 18, 2022
@cwperks cwperks added the backport 2.x backport to 2.x branch label Aug 19, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Aug 19, 2022
Signed-off-by: Bharathwaj G <bharath78910@gmail.com>
(cherry picked from commit 6b7a586)
cwperks pushed a commit that referenced this pull request Aug 22, 2022
Signed-off-by: Bharathwaj G <bharath78910@gmail.com>
(cherry picked from commit 6b7a586)

Co-authored-by: Bharathwaj G <58062316+bharath-techie@users.noreply.github.com>
stephen-crawford pushed a commit to stephen-crawford/security that referenced this pull request Nov 10, 2022
Signed-off-by: Bharathwaj G <bharath78910@gmail.com>
Signed-off-by: Stephen Crawford <steecraw@amazon.com>
wuychn pushed a commit to ochprince/security that referenced this pull request Mar 16, 2023
…rch-project#2037)

Signed-off-by: Bharathwaj G <bharath78910@gmail.com>
(cherry picked from commit 6b7a586)

Co-authored-by: Bharathwaj G <58062316+bharath-techie@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants