Improved index resolution and revised index authorization

[nibix]

Description

This introduces all the necessary changes for the improved index resolution and revised index authorization described in #5814.

This has several advantages:

• The old index authorization and index resolution system had many inconsistencies and oddities. These are fixed by the new approach.
• The responsibility for index resolution is moved from the security plugin to the individual transport actions. This makes the whole authorization process much more robust and the code less messy and fragile.
• As each transport action knows its indices it is operating on very well, it can do the resolution in a more efficient manner (often actually no resolution takes place at all, as all indices are already determined as constant values). This reduces the performance overhead induced by the security plugin.
• The security plugin no longer needs to resolve data streams to its individual backing indices to check privileges; this brings further performance improvements.

• Category: Enhancement
• Why these changes are required?
  • Fundamental improvements for index authorization were due
• What is the old behavior before changes and new behavior after changes?
  • By default, no behavioral changes are introduced; if the new index authorization system is enabled in config.yml, quite fundamental behavioral changes take place. These are described in [RFC] Revised index action authorization scheme #5814.

Issues Resolved

#5814

Testing

• extensive integration tests

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.

[codecov]

Codecov Report

❌ Patch coverage is 86.90373% with 151 lines in your changes missing coverage. Please review.
✅ Project coverage is 74.17%. Comparing base (7466266) to head (83b7f1e).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
...s/actionlevel/nextgen/PrivilegesEvaluatorImpl.java	86.71%	12 Missing and 24 partials ⚠️
...tgen/DashboardsMultitenancySystemIndexHandler.java	88.41%	8 Missing and 11 partials ⚠️
...nsearch/security/privileges/DocumentAllowList.java	52.17%	10 Missing and 1 partial ⚠️
...curity/privileges/PrivilegesEvaluatorResponse.java	73.17%	6 Missing and 5 partials ⚠️
.../actionlevel/RuntimeOptimizedActionPrivileges.java	90.83%	3 Missing and 8 partials ⚠️
...eges/actionlevel/SubjectBasedActionPrivileges.java	85.29%	5 Missing and 5 partials ⚠️
...vileges/actionlevel/RoleBasedActionPrivileges.java	85.93%	4 Missing and 5 partials ⚠️
...leges/actionlevel/nextgen/ActionConfiguration.java	86.20%	5 Missing and 3 partials ⚠️
...search/security/configuration/DlsFlsValveImpl.java	75.86%	4 Missing and 3 partials ⚠️
...earch/security/privileges/PrivilegesEvaluator.java	75.00%	5 Missing and 2 partials ⚠️
... and 11 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5827      +/-   ##
==========================================
+ Coverage   73.81%   74.17%   +0.36%     
==========================================
  Files         439      444       +5     
  Lines       27087    27997     +910     
  Branches     4018     4227     +209     
==========================================
+ Hits        19993    20768     +775     
- Misses       5188     5251      +63     
- Partials     1906     1978      +72     

Files with missing lines	Coverage Δ
.../opensearch/security/OpenSearchSecurityPlugin.java	85.05% <100.00%> (-0.02%)	⬇️
...rity/configuration/SystemIndexSearcherWrapper.java	94.82% <100.00%> (ø)
...ensearch/security/privileges/ActionPrivileges.java	100.00% <100.00%> (ø)
...leges/ClusterStateMetadataDependentPrivileges.java	93.33% <100.00%> (ø)
...g/opensearch/security/privileges/IndexPattern.java	97.08% <100.00%> (+0.42%)	⬆️
...ch/security/privileges/IndicesRequestResolver.java	100.00% <100.00%> (ø)
...curity/privileges/PrivilegesEvaluationContext.java	97.29% <100.00%> (+4.98%)	⬆️
...ges/actionlevel/legacy/PitPrivilegesEvaluator.java	96.29% <100.00%> (ø)
...es/actionlevel/legacy/PrivilegesEvaluatorImpl.java	84.74% <100.00%> (ø)
...ionlevel/legacy/ProtectedIndexAccessEvaluator.java	75.55% <ø> (ø)
... and 28 more

... and 9 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[cwperks]

@nibix apologies for a delay in comments. I plan to look at this in earnest over today and the coming days.

[cwperks]

TY for this change @nibix and introducing a dynamic feature flag through the security configuration.

I think this will greatly enhance the user experience, particularly for clusters where users are given fine-grained permissions over a subset of indices.

[cwperks]

@nibix When a securityconfig update is made will it add an entry for privileges_evaluation_type with the v3 default?

When preparing for OpenSearch v4, what code change would need to take place in the security repo?

[cwperks]

on second thought, what do you think about making this a dynamic cluster setting instead of securityconfig?

I'm a bit concerned about the persistence part of the securityconfig and generally prefer to make things dynamic cluster settings where its easier to change the default from one version to the next without concern about whether its been persisted with the default from previous major version

[nibix]

When a securityconfig update is made will it add an entry for privileges_evaluation_type with the v3 default?

Good question. Need to check that as well, will update later.

When preparing for OpenSearch v4, what code change would need to take place in the security repo?

It depends on the question whether we just want to flip the feature flag default for v4 or whether the old logic shall be completely retired and deleted then.

If just the feature flag default value is flipped, the changes in the main code are limited to flipping the default value. Test code would need to make sure to use explicit feature flag values where it is targeting the old behavior.

If the old logic shall be completely deleted, of course, we would need to delete quite a lot of main code. However, that should be fairly well separated from the new code. Regarding tests, we'd need to delete quite a lot of legacy tests which only cover the old behavior. But we should already have new tests which cover the new behavior, so it should be fine to delete such tests.

[nibix]

@cwperks @shikharj05

I have now changed the value of the feature flag to v4 and adapted the implementation to the changes performed in #5858 .

The changes are in these commits:

• 5279383
• e0c1c27

Can you re-review please?

I will also try to write some docs.

[cwperks]

@nibix looks good to me. I've noticed that our integration tests have been running a lot longer lately (unrelated to this PR). Any idea which change we can attribute the increase to?

[nibix]

@cwperks

I've noticed that our integration tests have been running a lot longer lately (unrelated to this PR). Any idea which change we can attribute the increase to?

I also had the feeling ... I will try to scan the logs to see if I find any indication.

Of course, we added quite a few new integration tests which might add maybe three to five minutes of additional runtime. For the time being, I did not remove older integration tests which cover the same thing - just in order to be sure that the new tests agree with the old tests. But, after having merged the new int tests, we could consider cleaning up the old int tests a bit.

Edit: Some findings at #5915

[nibix]

BTW, before merging this, we might want to have a closer look at the snapshot test issues described in #5915. This PR brings a few more test runs with snapshot/restore, which might make the issue worse.

[nibix]

I have rebased this and fixed a conflict. With the merged test fixes, this is now IMHO ready for merging. @cwperks @shikharj05 wdyt?

[nibix]

@shikharj05 @DarshitChanpura @derek-ho @RyanL1997 @reta @willyborankin

I kind this is a big PR; is there something I can facilitate the approval? It is already sitting around for a while.