System index permissions

[samuelcostae]

Description

This PR is based on the work done on #2681 . As the branch got stale and there were some issues with the sign-off of the commits, I created a new clean branch and generating a new PR

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?
To allow delegation of admin related work w.r.t system indices and allow more granular approach when working with system indices.

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 under allowed_actions and the pattern (except "*") or name clearly defined under index_pattern section of a role, in order to read/write to indices defined as "System Indices".

Issues Resolved

• Resolves [Feature] Update SecurityIndexAccessEvaluator to support new index permission #2553
• Resolve Add configuration flag to disable system index permissions #2845
• Resolves [FEATURE] Allow super-admin to control which system indices are not permission-able #3038

Testing
Automated Tests added.

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.

[peternied]

Thanks for the contribution. I've added several structural comments and a broader question about how the permission works with index pattern matching.

[stephen-crawford]

I think this is good to go once Peter's comments are addressed. I did not see anything that he did not comment on that looked blocking.

[DarshitChanpura]

FYI for maintainers: This is undergoing security review and will be merged backported once the review is complete.

Backport should be blocked until the security review is complete.

[codecov]

Codecov Report

Merging #2887 (98832d9) into main (1034cef) will increase coverage by 0.95%.
The diff coverage is 78.89%.

@@             Coverage Diff              @@
##               main    #2887      +/-   ##
============================================
+ Coverage     63.24%   64.20%   +0.95%     
- Complexity     3452     3471      +19     
============================================
  Files           263      263              
  Lines         20053    20112      +59     
  Branches       3348     3359      +11     
============================================
+ Hits          12683    12912     +229     
+ Misses         5741     5523     -218     
- Partials       1629     1677      +48     

Files Changed	Coverage Δ
...pensearch/security/securityconf/ConfigModelV6.java	31.71% <52.63%> (+31.71%)	⬆️
...pensearch/security/securityconf/ConfigModelV7.java	67.90% <66.66%> (-0.45%)	⬇️
...urity/privileges/SecurityIndexAccessEvaluator.java	79.80% <87.87%> (+4.48%)	⬆️
.../opensearch/security/OpenSearchSecurityPlugin.java	84.60% <100.00%> (+0.07%)	⬆️
...earch/security/privileges/PrivilegesEvaluator.java	73.29% <100.00%> (+0.08%)	⬆️
...g/opensearch/security/support/ConfigConstants.java	95.00% <100.00%> (+0.26%)	⬆️

... and 8 files with indirect coverage changes

[DarshitChanpura]

QQ: Are we preventing access to security index? If not should there be a follow-up that prevents access to security-index with this permission scheme?

Also @samuelcostae Could you please address CI failures?

[cwperks]

QQ: Are we preventing access to security index? If not should there be a follow-up that prevents access to security-index with this permission scheme?

IMO Security index should only ever be limited to requests from the admin certificate. The admin certificate is configured in opensearch.yml and is not dependent on the security index at all. i.e. If a user connecting with admin certificate deletes the security index, they can connect again in a following request to create the security index.

Any other user would not be able to reconnect after the security index is nuked because their permissions rely on the security index.

[DarshitChanpura]

Another question @samuelcostae, are we toggling this feature with a feature flag or is this enabled by default?

Core issue with SecurityRoles has been resolved

the license header changes requested in this review have been addressed

[DarshitChanpura]

@cwperks @peternied @RyanL1997 @scrawfor99 All outstanding comments have been addressed. Please review it once you get a chance.

[stephen-crawford]

Looks good to me. Nice job with the testing.

[peternied]

Thanks for the work on this @DarshitChanpura and @samuelcostae

[opensearch-trigger-bot]

The backport to 2.x failed:

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

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/security/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/security/backport-2.x
# Create a new branch
git switch --create backport/backport-2887-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 1379234ccd4ecf19a2c7de570724aeace7664f99
# Push it to GitHub
git push --set-upstream origin backport/backport-2887-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/security/backport-2.x

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