Authorize system index permissions in SecurityIndexAccessEvaluator #2681

samuelcostae · 2023-04-14T18:18:49Z

Description

SecurityIndexAccessEvaluator was modified to check if an accounting trying to access a system index specifically has the "system:admin/system_index" permission for the index, and block the access if it doesn't.

A Permission needs to be set explicitly and a "*" permission will not allow access to the sistem indices.

Category: Enhancement
Why these changes are required?
Extensions do not have the ability to elevate privileges, which means they cannot access system indexes. To overcome this limitation, extensions will require a way to access and store protected metadata.
What is the old behavior before changes and new behavior after changes?
- Before: Plugins would elevate privileges to access system indices
- After: An account needs to have the additional index permission "system:admin/system_index" in order to modify indices defined as "System Indices".

Issues Resolved

#2553

Testing

Unit Testing - Added tests to SystemIndexTests.java
Draft PR - Existing tests still need to be updated to account for new beahaviour

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.

stephen-crawford

Hi @samuelcostae, thank you for taking the time to put together this draft. It looks like you have a great start. I left a few comments but overall I think things are pretty close. I also left some tricks to get the github runners to not yell about the formatting.

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

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

src/test/java/org/opensearch/security/system_indices/SystemIndicesTests.java

src/test/resources/internal_users_system_indices.yml

src/test/resources/roles_system_indices.yml

stephen-crawford

Just a couple more comments. Overall looks very close.

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

src/main/java/org/opensearch/security/securityconf/ConfigModelV7.java

peternied

Please do not use auto formatters on pre-existing files, it makes it hard to detect legitimate changes, can you revert all changes of this kind so only your changes are visible in the GitHub diff view?

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

DarshitChanpura · 2023-05-18T14:33:39Z

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

-            });
-        });
-
-        for(WildcardMatcher userPermMatcher : userPermMatchers.stream().filter( matcher ->  !matcher.toString().equals("*")).collect(Collectors.toSet()) ){


qq: Why did we remove filtering * permissions here?

src/main/java/org/opensearch/security/support/ConfigConstants.java

src/test/resources/roles_mapping_system_indices.yml

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

src/main/java/org/opensearch/security/support/ConfigConstants.java

davidlago · 2023-06-15T20:06:50Z

@samuelcostae where do we stand with this? I just noticed it has not been updated for a few weeks now... was something blocking this work?

davidlago · 2023-06-16T12:39:33Z

@samuelcostae you need to sign-off your commits, otherwise the DCO check will continue blocking this PR.

stephen-crawford

Hi Sam, great work with this. I left some comments. I am marking this for ready for review since it seems close. Please let me know if you have any questions about my comments.

stephen-crawford · 2023-06-19T13:57:03Z

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

@@ -175,6 +188,20 @@ public PrivilegesEvaluatorResponse evaluate(
        return presponse;
    }

+    private  boolean checkSystemIndexPermissionsForUser(ConfigModelV7.SecurityRoles securityRoles) {


Are these system indices permissions supposed to be specific to a given index or generalized? This looks like we check for any permissions which match any of the system index permission privilege strings. This would only work if we were using system indices access as a boolean versus a specific target index which an account has permissions with.

I think we should go with the latter option as that will provide much higher levels of certainty over what an account is doing and what operations they can perform. Giving them complete access versus no-access causes problems when you want an account to be able to modify its own system index but no others.

stephen-crawford · 2023-06-19T14:03:55Z

src/test/java/org/opensearch/security/system_indices/SystemIndicesTests.java

+
+    private static final String extensionUser = "extensions_user";
+    private static final Header extensionUserHeader = encodeBasicHeader(extensionUser, allAccessUser);
+    private static final String extensionUserC = "extensions_user_c";


Still see extensionUserC instead of B.

stephen-crawford · 2023-06-19T14:07:54Z

src/test/java/org/opensearch/security/system_indices/SystemIndicesTests.java

Can you add some verification of the response messages and the logging messages? The tests are good, but it would be a good idea to verify that we get the expected response messages when being told forbidden or being allowed. The response codes could be from various things so this will make sure the responses come from what we expect.

stephen-crawford · 2023-06-19T14:08:26Z

src/test/resources/internal_users_system_indices.yml

+  attributes: {}
+  description: "User Mapped to role With Access to Admin:System:Indices "
+
+extensions_user_c:


Suggested change

extensions_user_c:

extensions_user_b:

stephen-crawford · 2023-06-19T14:08:47Z

src/test/resources/roles_mapping_system_indices.yml

+  and_backend_roles: []
+  description: "System index permissions"
+
+extensions_role_c:


Suggested change

extensions_role_c:

extensions_role_b:

stephen-crawford · 2023-06-19T14:08:56Z

src/test/resources/roles_system_indices.yml

+        - '*'
+        - 'system:admin/system_index'
+
+extensions_role_c:


Suggested change

extensions_role_c:

extensions_role_b:

stephen-crawford · 2023-06-19T14:10:31Z

Hi @samuelcostae, if you can rebase and run ./gradlew spotlessApply it should fix the hygiene issues.

DarshitChanpura · 2023-06-19T15:29:09Z

@samuelcostae Can you please sign-off commits to fix the DCO checks?

samuelcostae · 2023-06-19T15:34:47Z

@samuelcostae Can you please sign-off commits to fix the DCO checks?

I thought I had fixed all, spent a good amount of time today merging after signing the old ones xD, i will double check

Signed-off-by: Abhi Kalra <abhivka@amazon.com> Co-authored-by: Abhi Kalra <abhivka@amazon.com> Signed-off-by: Sam <samuel.costa@eliatra.com>

Signed-off-by: Andriy Redko <andriy.redko@aiven.io> Signed-off-by: Sam <samuel.costa@eliatra.com>

…t#2679) * Update certs for SecuritySSLReloadCertsActionTests Signed-off-by: Craig Perkins <cwperx@amazon.com> * Add otherName back in Signed-off-by: Craig Perkins <cwperx@amazon.com> * Ensure files end in new line Signed-off-by: Craig Perkins <cwperx@amazon.com> --------- Signed-off-by: Craig Perkins <cwperx@amazon.com> Signed-off-by: Sam <samuel.costa@eliatra.com>

…t#2687) * Fix NPE and add additional graceful error handling Signed-off-by: Craig Perkins <cwperx@amazon.com> * Add new lines at end of file Signed-off-by: Craig Perkins <cwperx@amazon.com> * Run spotlessApply Signed-off-by: Craig Perkins <cwperx@amazon.com> * Remove unused import Signed-off-by: Craig Perkins <cwperx@amazon.com> * volatile to final Signed-off-by: Craig Perkins <cwperx@amazon.com> --------- Signed-off-by: Craig Perkins <cwperx@amazon.com> Signed-off-by: Sam <samuel.costa@eliatra.com>

* Add release notes for 2.7.0 Signed-off-by: Ryan Liang <jiallian@amazon.com> Signed-off-by: Sam <samuel.costa@eliatra.com>

--------- Signed-off-by: Stephen Crawford <steecraw@amazon.com> Signed-off-by: Stephen Crawford <65832608+scrawfor99@users.noreply.github.com> Signed-off-by: Sam <samuel.costa@eliatra.com>

Typos Co-authored-by: Stephen Crawford <65832608+scrawfor99@users.noreply.github.com> Signed-off-by: Sam <128482925+samuelcostae@users.noreply.github.com> Signed-off-by: Sam <samuel.costa@eliatra.com>

Signed-off-by: scosta <samuel.costa@eliatra.com> Signed-off-by: Sam <samuel.costa@eliatra.com>

* Match version of zstd-jni from core Signed-off-by: Craig Perkins <cwperx@amazon.com> * Add zstd version from core to force resolutions section Signed-off-by: Craig Perkins <cwperx@amazon.com> --------- Signed-off-by: Craig Perkins <cwperx@amazon.com> Signed-off-by: Sam <samuel.costa@eliatra.com>

…security/m* (opensearch-project#2826) * Update Sec/action to Sec/Filter Signed-off-by: Stephen Crawford <steecraw@amazon.com> * Update Security/* Signed-off-by: Stephen Crawford <steecraw@amazon.com> * Update Security/* and test/opensearch/security/* Signed-off-by: Stephen Crawford <steecraw@amazon.com> * Half sec test changes Signed-off-by: Stephen Crawford <steecraw@amazon.com> * Half sec test changes Signed-off-by: Stephen Crawford <steecraw@amazon.com> --------- Signed-off-by: Stephen Crawford <steecraw@amazon.com> Signed-off-by: Sam <samuel.costa@eliatra.com>

* Update test/security/s* java files Signed-off-by: Stephen Crawford <steecraw@amazon.com> * Update test/security/s* java files Signed-off-by: Stephen Crawford <steecraw@amazon.com> --------- Signed-off-by: Stephen Crawford <steecraw@amazon.com> Signed-off-by: Sam <samuel.costa@eliatra.com>

…#2837) * Update test/security/s* java files Signed-off-by: Stephen Crawford <steecraw@amazon.com> * Update test/security/s* java files Signed-off-by: Stephen Crawford <steecraw@amazon.com> * Update all tests under security Signed-off-by: Stephen Crawford <steecraw@amazon.com> --------- Signed-off-by: Stephen Crawford <steecraw@amazon.com> Signed-off-by: Sam <samuel.costa@eliatra.com>

…t#2843) * Add Andrey to Maintainers Signed-off-by: Stephen Crawford <steecraw@amazon.com> * Update MAINTAINERS.md Signed-off-by: Stephen Crawford <65832608+scrawfor99@users.noreply.github.com> * Update codeowners Signed-off-by: Stephen Crawford <steecraw@amazon.com> --------- Signed-off-by: Stephen Crawford <steecraw@amazon.com> Signed-off-by: Stephen Crawford <65832608+scrawfor99@users.noreply.github.com> Signed-off-by: Sam <samuel.costa@eliatra.com>

Signed-off-by: Craig Perkins <cwperx@amazon.com> Signed-off-by: Sam <samuel.costa@eliatra.com>

…roject#2840) * Update all tests Signed-off-by: Stephen Crawford <steecraw@amazon.com> * spotless Signed-off-by: Stephen Crawford <steecraw@amazon.com> --------- Signed-off-by: Stephen Crawford <steecraw@amazon.com> Signed-off-by: Sam <samuel.costa@eliatra.com>

…project#2855) * add search model group permission to ml_read_access role Signed-off-by: Bhavana Ramaram <rbhavna@amazon.com> Signed-off-by: Sam <samuel.costa@eliatra.com>

Signed-off-by: Stephen Crawford <steecraw@amazon.com> Signed-off-by: Sam <samuel.costa@eliatra.com>

Signed-off-by: Sam <samuel.costa@eliatra.com>

…AccessEvaluator.java Signed-off-by: scosta <samuel.costa@eliatra.com> opensearch-project#2553 gSmall refactor on logic on SecurityIndexAccessEvaluator.java Signed-off-by: scosta <samuel.costa@eliatra.com> opensearch-project#2553 formatting fixes and correcting merge error Signed-off-by: scosta <samuel.costa@eliatra.com> opensearch-project#2553 removing some leftover changes from SecurityIndexAccessEvaluator.java opensearch-project#2553 adding a few more tests to SystemIndicesTests.java and fixing some merge issues on ConfigConstants.java opensearch-project#2553 Refactoring and adding tests to SystemIndicesTests.java opensearch-project#2553 Changes to SecurityIndexAccessEvaluator and initial tests(still failing) Signed-off-by: scosta <samuel.costa@eliatra.com>

* Isolate spotless config changes Signed-off-by: Craig Perkins <cwperx@amazon.com> * update dlic/auth package style Signed-off-by: Stephen Crawford <steecraw@amazon.com> * Update formatterConfig.xml --------- Signed-off-by: Craig Perkins <cwperx@amazon.com> Signed-off-by: Stephen Crawford <steecraw@amazon.com> Co-authored-by: Craig Perkins <cwperx@amazon.com>

…search-project#2823) * rebase Signed-off-by: Stephen Crawford <steecraw@amazon.com> * Update java style under **/auth Signed-off-by: Stephen Crawford <steecraw@amazon.com> * Update Util dir Signed-off-by: Stephen Crawford <steecraw@amazon.com> * readd formatting Signed-off-by: Stephen Crawford <steecraw@amazon.com> --------- Signed-off-by: Stephen Crawford <steecraw@amazon.com>

Typos Co-authored-by: Stephen Crawford <65832608+scrawfor99@users.noreply.github.com> Signed-off-by: Sam <128482925+samuelcostae@users.noreply.github.com>

…security/m* (opensearch-project#2826) * Update Sec/action to Sec/Filter Signed-off-by: Stephen Crawford <steecraw@amazon.com> * Update Security/* Signed-off-by: Stephen Crawford <steecraw@amazon.com> * Update Security/* and test/opensearch/security/* Signed-off-by: Stephen Crawford <steecraw@amazon.com> * Half sec test changes Signed-off-by: Stephen Crawford <steecraw@amazon.com> * Half sec test changes Signed-off-by: Stephen Crawford <steecraw@amazon.com> --------- Signed-off-by: Stephen Crawford <steecraw@amazon.com> Signed-off-by: Sam <samuel.costa@eliatra.com>

* Update test/security/s* java files Signed-off-by: Stephen Crawford <steecraw@amazon.com> * Update test/security/s* java files Signed-off-by: Stephen Crawford <steecraw@amazon.com> --------- Signed-off-by: Stephen Crawford <steecraw@amazon.com>

…#2837) * Update test/security/s* java files Signed-off-by: Stephen Crawford <steecraw@amazon.com> * Update test/security/s* java files Signed-off-by: Stephen Crawford <steecraw@amazon.com> * Update all tests under security Signed-off-by: Stephen Crawford <steecraw@amazon.com> --------- Signed-off-by: Stephen Crawford <steecraw@amazon.com>

…roject#2840) * Update all tests Signed-off-by: Stephen Crawford <steecraw@amazon.com> * spotless Signed-off-by: Stephen Crawford <steecraw@amazon.com> --------- Signed-off-by: Stephen Crawford <steecraw@amazon.com>

Signed-off-by: Stephen Crawford <steecraw@amazon.com> Signed-off-by: Sam <samuel.costa@eliatra.com>

…ect#2864) Use BouncyCastle PEMReader instead of regular expression to read and parse private key pem files. Signed-off-by: Andrey Pleskach <ples@aiven.io> Signed-off-by: Sam <samuel.costa@eliatra.com>

Signed-off-by: Sam <samuel.costa@eliatra.com>

# Conflicts: # src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java # src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java # src/main/java/org/opensearch/security/securityconf/ConfigModelV7.java # src/main/java/org/opensearch/security/support/ConfigConstants.java # src/test/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluatorTest.java # src/test/java/org/opensearch/security/system_indices/SystemIndicesTests.java

Signed-off-by: Sam <samuel.costa@eliatra.com>

peternied · 2023-06-21T16:17:52Z

@samuelcostae Is this the same as #2887 ? If so can we close this PR out?

samuelcostae mentioned this pull request Apr 14, 2023

[Feature] Update SecurityIndexAccessEvaluator to support new index permission #2553

Closed

stephen-crawford changed the title ~~#2553~~ Authorize system index permissions in SecurityIndexAccessEvaluator Apr 17, 2023

stephen-crawford reviewed Apr 18, 2023

View reviewed changes

stephen-crawford reviewed Apr 25, 2023

View reviewed changes

peternied reviewed May 2, 2023

View reviewed changes

samuelcostae force-pushed the #2553 branch from a3ca6c8 to 106b552 Compare May 16, 2023 13:35

samuelcostae closed this May 16, 2023

samuelcostae force-pushed the #2553 branch from 106b552 to 15860b6 Compare May 16, 2023 13:36

samuelcostae reopened this May 16, 2023

DarshitChanpura reviewed May 18, 2023

View reviewed changes

stephen-crawford reviewed Jun 1, 2023

View reviewed changes

stephen-crawford reviewed Jun 19, 2023

View reviewed changes

stephen-crawford marked this pull request as ready for review June 19, 2023 14:10

stephen-crawford requested review from cliu123, cwperks, davidlago, RyanL1997, reta and willyborankin as code owners June 19, 2023 14:10

abhivka7 and others added 6 commits June 19, 2023 16:37

Dynamic_Tenancy_Configurations changes (opensearch-project#2607)

5e2ea59

Signed-off-by: Abhi Kalra <abhivka@amazon.com> Co-authored-by: Abhi Kalra <abhivka@amazon.com> Signed-off-by: Sam <samuel.costa@eliatra.com>

Gradle plugin updates (spotless, ospackage) (opensearch-project#2670)

15a29bd

Signed-off-by: Andriy Redko <andriy.redko@aiven.io> Signed-off-by: Sam <samuel.costa@eliatra.com>

Add release notes for 2.7.0 (opensearch-project#2690)

b2e7898

* Add release notes for 2.7.0 Signed-off-by: Ryan Liang <jiallian@amazon.com> Signed-off-by: Sam <samuel.costa@eliatra.com>

Security User Refactor (opensearch-project#2594)

61f3dac

--------- Signed-off-by: Stephen Crawford <steecraw@amazon.com> Signed-off-by: Stephen Crawford <65832608+scrawfor99@users.noreply.github.com> Signed-off-by: Sam <samuel.costa@eliatra.com>

samuelcostae and others added 24 commits June 19, 2023 16:37

Apply suggestions from code review

53f17a6

Typos Co-authored-by: Stephen Crawford <65832608+scrawfor99@users.noreply.github.com> Signed-off-by: Sam <128482925+samuelcostae@users.noreply.github.com> Signed-off-by: Sam <samuel.costa@eliatra.com>

Log fixes and restoring some ConfigConstants

2314a4d

Signed-off-by: scosta <samuel.costa@eliatra.com> Signed-off-by: Sam <samuel.costa@eliatra.com>

Fix JwtAuthenticationTests assertion (opensearch-project#2844)

1b2573d

Signed-off-by: Craig Perkins <cwperx@amazon.com> Signed-off-by: Sam <samuel.costa@eliatra.com>

add search model group permission to ml_read_access role (opensearch-…

b7ddc18

…project#2855) * add search model group permission to ml_read_access role Signed-off-by: Bhavana Ramaram <rbhavna@amazon.com> Signed-off-by: Sam <samuel.costa@eliatra.com>

IntegrationTest spotless (opensearch-project#2863)

3a39112

Signed-off-by: Stephen Crawford <steecraw@amazon.com> Signed-off-by: Sam <samuel.costa@eliatra.com>

Format everything (opensearch-project#2866)

bfe3760

Signed-off-by: Sam <samuel.costa@eliatra.com>

Apply suggestions from code review

f8ef616

Typos Co-authored-by: Stephen Crawford <65832608+scrawfor99@users.noreply.github.com> Signed-off-by: Sam <128482925+samuelcostae@users.noreply.github.com>

IntegrationTest spotless (opensearch-project#2863)

b4bfa76

Signed-off-by: Stephen Crawford <steecraw@amazon.com> Signed-off-by: Sam <samuel.costa@eliatra.com>

Format everything (opensearch-project#2866)

c52b681

Use boucycastle PEM reader instead of reg expression (opensearch-proj…

191dc9d

…ect#2864) Use BouncyCastle PEMReader instead of regular expression to read and parse private key pem files. Signed-off-by: Andrey Pleskach <ples@aiven.io> Signed-off-by: Sam <samuel.costa@eliatra.com>

Gradlew spotless

e286014

Signed-off-by: Sam <samuel.costa@eliatra.com>

samuelcostae force-pushed the #2553 branch from e1cf3b0 to e286014 Compare June 19, 2023 15:46

samuelcostae added 2 commits June 19, 2023 17:04

Gradlew spotless

a4a5a4d

Signed-off-by: Sam <samuel.costa@eliatra.com>

samuelcostae mentioned this pull request Jun 20, 2023

System index permissions #2887

Merged

3 tasks

samuelcostae closed this Jun 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Authorize system index permissions in SecurityIndexAccessEvaluator #2681

Authorize system index permissions in SecurityIndexAccessEvaluator #2681

samuelcostae commented Apr 14, 2023 •

edited

Loading

stephen-crawford left a comment

stephen-crawford left a comment

peternied left a comment

DarshitChanpura May 18, 2023

davidlago commented Jun 15, 2023

davidlago commented Jun 16, 2023

stephen-crawford left a comment

stephen-crawford Jun 19, 2023

stephen-crawford Jun 19, 2023

stephen-crawford Jun 19, 2023

stephen-crawford Jun 19, 2023

stephen-crawford Jun 19, 2023

stephen-crawford Jun 19, 2023

stephen-crawford commented Jun 19, 2023

DarshitChanpura commented Jun 19, 2023

samuelcostae commented Jun 19, 2023

peternied commented Jun 21, 2023

Authorize system index permissions in SecurityIndexAccessEvaluator #2681

Authorize system index permissions in SecurityIndexAccessEvaluator #2681

Conversation

samuelcostae commented Apr 14, 2023 • edited Loading

Description

Issues Resolved

Testing

Check List

stephen-crawford left a comment

Choose a reason for hiding this comment

stephen-crawford left a comment

Choose a reason for hiding this comment

peternied left a comment

Choose a reason for hiding this comment

DarshitChanpura May 18, 2023

Choose a reason for hiding this comment

davidlago commented Jun 15, 2023

davidlago commented Jun 16, 2023

stephen-crawford left a comment

Choose a reason for hiding this comment

stephen-crawford Jun 19, 2023

Choose a reason for hiding this comment

stephen-crawford Jun 19, 2023

Choose a reason for hiding this comment

stephen-crawford Jun 19, 2023

Choose a reason for hiding this comment

stephen-crawford Jun 19, 2023

Choose a reason for hiding this comment

stephen-crawford Jun 19, 2023

Choose a reason for hiding this comment

stephen-crawford Jun 19, 2023

Choose a reason for hiding this comment

stephen-crawford commented Jun 19, 2023

DarshitChanpura commented Jun 19, 2023

samuelcostae commented Jun 19, 2023

peternied commented Jun 21, 2023

samuelcostae commented Apr 14, 2023 •

edited

Loading