Only run DlsFlsValveImpl.invoke on indices requests #4937
Conversation
Signed-off-by: Craig Perkins <[email protected]>
|
FYI @nibix |
Signed-off-by: Craig Perkins <[email protected]>
| @@ -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.
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.
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.
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.
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 |
Looks like this pattern is prevalent in the ISM codebase. StartRollupRequest, StopRollupRequest, StartSMRequest, StopSMRequest, StartTransformRequest and StopTransformRequest Code search: https://github.com/search?q=repo%3Aopensearch-project%2Findex-management%20%22%3A%20UpdateRequest%22&type=code |
|
@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 <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
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 <[email protected]>
| @@ -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.
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 |
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.