-
Notifications
You must be signed in to change notification settings - Fork 274
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
Authorize system index permissions in SecurityIndexAccessEvaluator #2681
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java
Show resolved
Hide resolved
src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opensearch/security/system_indices/SystemIndicesTests.java
Show resolved
Hide resolved
src/test/java/org/opensearch/security/system_indices/SystemIndicesTests.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opensearch/security/system_indices/SystemIndicesTests.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple more comments. Overall looks very close.
src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java
Show resolved
Hide resolved
src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java
Show resolved
Hide resolved
src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java
Outdated
Show resolved
Hide resolved
}); | ||
}); | ||
|
||
for(WildcardMatcher userPermMatcher : userPermMatchers.stream().filter( matcher -> !matcher.toString().equals("*")).collect(Collectors.toSet()) ){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
qq: Why did we remove filtering *
permissions here?
src/main/java/org/opensearch/security/support/ConfigConstants.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/privileges/SecurityIndexAccessEvaluator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/support/ConfigConstants.java
Outdated
Show resolved
Hide resolved
@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? |
@samuelcostae you need to sign-off your commits, otherwise the DCO check will continue blocking this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -175,6 +188,20 @@ public PrivilegesEvaluatorResponse evaluate( | |||
return presponse; | |||
} | |||
|
|||
private boolean checkSystemIndexPermissionsForUser(ConfigModelV7.SecurityRoles securityRoles) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
private static final String extensionUser = "extensions_user"; | ||
private static final Header extensionUserHeader = encodeBasicHeader(extensionUser, allAccessUser); | ||
private static final String extensionUserC = "extensions_user_c"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still see extensionUserC instead of B.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
attributes: {} | ||
description: "User Mapped to role With Access to Admin:System:Indices " | ||
|
||
extensions_user_c: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extensions_user_c: | |
extensions_user_b: |
and_backend_roles: [] | ||
description: "System index permissions" | ||
|
||
extensions_role_c: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extensions_role_c: | |
extensions_role_b: |
- '*' | ||
- 'system:admin/system_index' | ||
|
||
extensions_role_c: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
extensions_role_c: | |
extensions_role_b: |
Hi @samuelcostae, if you can rebase and run |
@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>
@samuelcostae Is this the same as #2887 ? If so can we close this PR out? |
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?
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
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.