AddPolicyAction - Recursion removal

[petardz]

Issue #, if available: #774

Description of changes:

Removed recursion when checking permissions on indices.

Fixed index pattern permission check: injected "injected_user" property into threadContext

CheckList:

• 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.

[petardz]

I had inject user in context for index permissions(patterns) to be verified by Security plugin. This information being tied to a role, I would expect by just injecting roles, that it would be enough.

Without injecting user, index permission check seem to be skipped.

We're checking index permission by restoring roles in threadContext and then calling our dummy Transport Action which ActionRequest extends BroadcastRequest which implements IndicesRequest.Replaceable.

@bowenlan-amzn

[cwperks]

It should not be necessary to call on inject user directly. The security plugin also does not support injecting user directly into the thread context by default, this is a feature that is turned off and the setting to enable the feature makes it clear that it is an unsupported feature: https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/auth/UserInjector.java#L62

I'm puzzled to why its not working with only using injectRoles - I will continue looking at this further, but here's the analysis I have done so far:

• inject.injectRoles injects a threadcontext header: opendistro_security_injected_roles - example looks like plugin|role1,role2. plugin in this user string is a special username where the permissions check is evaluated on the plugin user that has the roles the plugin would like to evaluate privileges on.
• Within the security plugin it hits here - https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/filter/SecurityFilter.java#L169
• Digging into rolesInjector.injectUserAndRoles it populates the opendistro_security_user at the very end inside the addUser method: https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/auth/RolesInjector.java#L48-L86
• Since injected roles are non-null it should bypass this block that allows local direct requests from plugins to directly pass through the SecurityFilter: https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/filter/SecurityFilter.java#L258-L266

By calling on injectUser and passing the username it is populating a threadcontext header as injected_user: <username>

If the plugins.security.unsupported.inject_user.enabled is turned on inside opensearch.yml then it would hit this block in the SecurityFilter, but only if the opendistro_security_user transient header is null (which it shouldn't be after a roles injection) and the feature is enabled.

@petardz What list of roles are you testing with?

[petardz]

We use 1 role in security tests. This is where we create role and assign role to user: link and implementations of those util functions: link

[cwperks]

Could it have been hitting this block in InjectSecurity where it does nothing if the list of roles is empty?

[petardz]

As of today I cannot reproduce this issue anymore... Two days ago I was flipping between "with injectUser" and "without injectUser" and was able to reproduce it every time.. My testing setup was official 2.7 docker image with installed security plugin where I installed ISM through script.

Thanks for looking into this and sorry for false alarm. I'll revert my change regarding threadContext

[cwperks]

@petardz Were you testing locally or via a Github action? If there's a github action log available, could you provide a link? I'm glad its working now :)

[petardz]

@cwperks It was locally tested with my own scripts.

[codecov]

Codecov Report

Merging #779 (f93e7eb) into main (2c84ffb) will decrease coverage by 0.15%.
The diff coverage is 13.33%.

@@             Coverage Diff              @@
##               main     #779      +/-   ##
============================================
- Coverage     75.46%   75.31%   -0.15%     
- Complexity     2591     2595       +4     
============================================
  Files           335      335              
  Lines         15174    15176       +2     
  Branches       2189     2188       -1     
============================================
- Hits          11451    11430      -21     
- Misses         2448     2472      +24     
+ Partials       1275     1274       -1     

Impacted Files	Coverage Δ
...exmanagement/opensearchapi/OpenSearchExtensions.kt	76.06% <ø> (ø)
...sport/action/addpolicy/TransportAddPolicyAction.kt	64.63% <13.33%> (+2.42%)	⬆️

... and 11 files with indirect coverage changes

[opensearch-trigger-bot]

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-779-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 45b14765c2139095da9ce12c835a8bac35fa5fbb
# Push it to GitHub
git push --set-upstream origin backport/backport-779-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-779-to-2.x.

[bowenlan-amzn]

@petardz pls do a manual backport... not sure what's conflict that keeps failing the auto backport