-
Notifications
You must be signed in to change notification settings - Fork 279
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
Only run DlsFlsValveImpl.invoke on indices requests #4937
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Craig Perkins <cwperx@amazon.com>
FYI @nibix |
Signed-off-by: Craig Perkins <cwperx@amazon.com>
@@ -135,6 +137,10 @@ public DlsFlsValveImpl( | |||
*/ | |||
@Override | |||
public boolean invoke(PrivilegesEvaluationContext context, final ActionListener<?> listener) { | |||
if (!isIndexPerm(context.getAction())) { |
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.
I would guess that the valve code needs to be at least also called for scroll requests, otherwise FLS and field masking might be not correctly applied.
Generally, as the issue seems to be of a very specific kind, I am wondering whether the fix should be equally specific.
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.
I just pushed a commit to check the action prefix alone. If the action prefix is not indices:
, then DlsFlsValveImpl would be skipped.
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, I think the fault is rather on the ISM side: It defines an action which has a request which extends UpdateRequest
and thus in turn extends IndicesRequest
- while the type string is cluster:admin/opendistro/transform/stop
, i.e. it indicates it to be a cluster request. That seems to be a contradiction in itself.
I am a bit surprised that this should only occur after the merge of #4380. At least this basic behavior of DlsFlsValveImpl did not change. If this is really related to #4380, we should find out why #4380 changes this behavior.
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.
Before #4380 , it would get short circuited here because the helpdesk_role
has no restrictions. The new code does not use that data structure and instead calls on IndexNameExpressionResolver.getConcreteIndices
and passes in *
because the resolved request shows *
as you pointed out in #4937 (comment)
Two things are interesting IMHO:
|
For the second one, that's actually because of the test itself: https://github.com/opensearch-project/index-management/blob/main/src/test/kotlin/org/opensearch/indexmanagement/TransformSecurityBehaviorIT.kt#L260-L261 It calls it first with a user that doesn't have permission and then again with a user that does. This is where the |
Ok, I start to understand now. Still, I think the best fix would be to remove the If that's not feasible, one could change this line to security/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java Line 267 in c22002a
|
Looks like this pattern is prevalent in the ISM codebase. StartRollupRequest, StopRollupRequest, StartSMRequest, StopSMRequest, StartTransformRequest and StopTransformRequest |
@nibix There's another issue in ISM tests w/ security around cluster state updates:
When dumping out hot threads I see this:
During the tests its failing due to active threads in the generic threadpool. ISM tests perform index operations frequently like creating indices and deleting all indices in the cluster. Edit: At a certain point its getting reset to -1
Example test to run:
|
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4937 +/- ##
==========================================
- Coverage 71.46% 71.43% -0.03%
==========================================
Files 334 334
Lines 22517 22550 +33
Branches 3586 3589 +3
==========================================
+ Hits 16091 16108 +17
- Misses 4634 4648 +14
- Partials 1792 1794 +2
|
There's one other flavor of failure too: Example test:
It fails on this line: https://github.com/opensearch-project/index-management/blob/main/src/test/kotlin/org/opensearch/indexmanagement/indexstatemanagement/resthandler/RestRemovePolicyActionIT.kt#L203-L207 The response is an InternalServerError and its making the test exit early.
Edit: This last error may be caused by having
at the top of |
Signed-off-by: Craig Perkins <cwperx@amazon.com>
@@ -264,7 +265,7 @@ public boolean invoke(PrivilegesEvaluationContext context, final ActionListener< | |||
} | |||
} | |||
|
|||
if (request instanceof UpdateRequest) { | |||
if (UpdateAction.NAME.equals(context.getAction())) { |
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.
This change wouldn't be needed if opensearch-project/index-management#1311 is merged
Interesting. I tried to find more context in the logs linked at opensearch-project/index-management#1305 but could not really find the failure. Where did you see it? |
Good catch regarding 9de9c24 ... that might be already fixing the thread pool issue? |
After running the test in the comment
you will see a thread remaining the generic threadpool when calling |
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Description
Companion PR in ISM: opensearch-project/index-management#1311
Fixes an issue seen in ISM's tests after the merge of #4380.
Failing test example: https://github.com/opensearch-project/index-management/actions/runs/12013427116/job/33486922892?pr=1310
Failing test error message:
ISM has actions like the StopTransformRequest that subclass UpdateRequest since behind the scenes, an entry in the ISM system index is updated and to run the request requires passing the ID of the transform with the request. This request is a cluster request meaning that is granted based on the action name alone without having to associate it with indices (behind the scenes the plugin updates its system index entry).
DlsFlsValveImpl.invoke is being called on this request and it is failing because the resolved request is resolving to all indices and the user calling that Stop Transform API has partial index (airlines-*) access.
This PR updates the logic in DlsFlsValveImpl.invoke to not run if the action is not prefixed with
indices:
Bug fix
Issues Resolved
Resolves: opensearch-project/index-management#1305
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.