Revert Optimized Privilege Evaluation (#4380) on main

[cwperks]

Description

This PR reverts 2 PRs on main:

• Optimized Privilege Evaluation #4380
• Ensure no active threads in any threadpool for tests in the integrationTest package #4943 <- This was a follow-up bug fix on a bug in metadata version tracking for DLS impl

Discussion on this can be found on the Backport 2.x PR here: #4898

Given the sweeping nature of this change, there is a new plan for rollout to gate the new ActionPrivileges functionality behind a feature flag and temporarily support legacy privilege evaluation (iteration through roles + indices) vs. optimized (constant-time) privilege evaluation. @nibix has opened up a PR to introduce ActionPrivileges only and gate it behind a feature flag: #4998. Note: This PR is currently targeted to 2.x, but plan is to rebase and get it into main to minimize differences between main and 2.x branches for maintenance.

The PR being reverted includes more performance improvements around DLS/FLS/Field Masking that remove the need for carrying FLS/DLS/Field Masking rules in ThreadContext headers. These changes are not included in #4998 and will need to be done independently in a future PR.

Check List

• New functionality includes testing
• New functionality has been documented
• New Roles/Permissions have a corresponding security dashboards plugin PR
• API changes companion pull request created
• 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

Attention: Patch coverage is 68.73990% with 387 lines in your changes missing coverage. Please review.

Project coverage is 69.90%. Comparing base (a3345ef) to head (987962a).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...pensearch/security/securityconf/ConfigModelV7.java	67.84%	93 Missing and 25 partials ⚠️
...security/configuration/DlsFlsFilterLeafReader.java	64.19%	68 Missing and 19 partials ⚠️
...search/security/configuration/DlsFlsValveImpl.java	56.35%	46 Missing and 33 partials ⚠️
...opensearch/security/configuration/MaskedField.java	51.61%	43 Missing and 2 partials ⚠️
...earch/security/privileges/PrivilegesEvaluator.java	66.23%	19 Missing and 7 partials ⚠️
...nsearch/security/configuration/DlsQueryParser.java	81.81%	7 Missing and 3 partials ⚠️
.../opensearch/security/OpenSearchSecurityPlugin.java	86.36%	1 Missing and 5 partials ⚠️
...h/security/securityconf/EvaluatedDlsFlsConfig.java	87.23%	4 Missing and 2 partials ⚠️
...ity/configuration/DlsFilterLevelActionHandler.java	84.61%	0 Missing and 2 partials ⚠️
...java/org/opensearch/security/support/MapUtils.java	88.23%	1 Missing and 1 partial ⚠️
... and 6 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5002      +/-   ##
==========================================
- Coverage   71.50%   69.90%   -1.61%     
==========================================
  Files         334      320      -14     
  Lines       22556    21752     -804     
  Branches     3589     3469     -120     
==========================================
- Hits        16129    15205     -924     
- Misses       4635     4755     +120     
  Partials     1792     1792              

Files with missing lines	Coverage Δ
...ty/configuration/ConfigurationLoaderSecurity7.java	70.00% <ø> (-0.25%)	⬇️
...ecurity/configuration/ConfigurationRepository.java	76.51% <100.00%> (-3.79%)	⬇️
...rch/security/configuration/DlsFlsRequestValve.java	0.00% <ø> (ø)
...urity/configuration/PrivilegesInterceptorImpl.java	57.31% <ø> (-2.46%)	⬇️
...va/org/opensearch/security/configuration/Salt.java	100.00% <ø> (ø)
...rity/configuration/SystemIndexSearcherWrapper.java	91.37% <100.00%> (-0.15%)	⬇️
...nsearch/security/dlic/rest/api/RolesApiAction.java	98.03% <100.00%> (+2.20%)	⬆️
...org/opensearch/security/filter/SecurityFilter.java	65.87% <100.00%> (ø)
...nsearch/security/privileges/DocumentAllowList.java	39.69% <100.00%> (-5.45%)	⬇️
...ch/security/privileges/PitPrivilegesEvaluator.java	96.29% <100.00%> (+0.14%)	⬆️
... and 26 more

... and 6 files with indirect coverage changes

[DarshitChanpura]

Thank you @nibix for promptly raising the new PR and thanks Craig for raising this revert!

[nibix]

I can only join now the conversation, as I was mostly offline for new years celebrations over the previous three days. I am a bit surprised by this.

To be honest, the new plan cited by @cwperks is not entirely clear to me.

This has several reasons (see also #4898 (comment)):

• The changes I have implemented in [2.x Backport] Optimized Privilege Evaluation: Action privileges ONLY, with feature flag #4998 have significant bad impacts to code maintainability, development efficiency, and code safety, as they cause huge amounts of duplicated code. As the original goal was to merge these to the 2.x branch, this seemed to be a good compromise, as the branch had a foreseeably limited life time. However, merging these to the main branch is a different category. As a maintainer of this project, I personally would struggle to approve a PR like [2.x Backport] Optimized Privilege Evaluation: Action privileges ONLY, with feature flag #4998 for the main branch, as the time the bad effects will potentially impact development processes is not so clearly defined. (Note: [2.x Backport] Optimized Privilege Evaluation: Action privileges ONLY, with feature flag #4998 is still not finished, I have yet to duplicate a number of integration tests in order to achieve full test coverage for both the old code and the new code).

• The goal of the feature flag introduced in [2.x Backport] Optimized Privilege Evaluation: Action privileges ONLY, with feature flag #4998 was quality assurance and reduction of risks introduced by the changes. I see in the release planning for 3.0 (see [PROPOSAL] OpenSearch Release Schedule for Year 2025 .github#252 ) that the 3.0 GA release is preceded by an alpha release and a beta release. I am wondering whether this testing phase can be considered as an alternative way to provide confidence and quality assurance in the new code. Usually, major releases receive increased attention and extended testing phases to cater for exactly such situations.

• A couple of details from the plan are still not clear to me: Shall the first part still be released with 2.19.0? If yes, would add 3.0 things on top of 2.19.0 or would it be identical?

[kumargu]

@nibix, I think we should meet again (sooner) to close on this. I will leave my 2 cents on the mental model for taking this path. I'll let Craig reply on how it helps his group to move faster.

IMO, If a PR is merged in main, it must exist in some released-or-non-released version (in this cases 2.x). I don't see a value of something being in main with no existence of it any version. Similarly, if something ever existed as released in some version, it must be moved to main. That's how we maintain consistency or we'll always be in a confused state as to what exists where. (We get often troubled with a similar problem while backporting commits to managed service).

Initially we had an impression that when we merge #4998 into main you'd end up in lot of merge conflicts which will add you churn, and indeed we wanted to reduce the churn. That assumption was based on the fact that you'd branch out the older one #4898, appending changes for the safety flag and removing DLS/FLS handling--and hence no extra work for duplication of test. But i think our assumption was wrong and we also missed talking about it.

I personally would struggle to approve a PR like #4998 for the main branch, as the time the bad effects will potentially impact development processes is not so clearly defined.

Even if there was not a strong reason of "potentially impact to development processes", I would prefer merging it to 2.19 followed by a clean merge to mainline. Sorry, I can't convince myself why the commit with flag should not be carried to main.

Thanks for pointing out the alpha and beta release strategy starting 3.0. I agree with you that we can shield new changes using the strategy. DLS/ FLS changes would be a good starting point? For privileged evaluation part, I think 3.0 would be too late and we still should target it for 2.19.

[cwperks]

When it comes time for the 3.0 release, my understanding is that a new branch will be cut from main called 3.x and then another branch will be cut from that called 3.0. One possibility to consider is keeping the change in main and reverting from 3.x before a release branch is cut if we don't plan to release the change in that particular 3.x release.

There are 2 in-flight projects that hit the authorization path that will need to be doubly implemented to support both the legacy authz code path and the new authz code path with ActionPrivileges:

1. Providing a replacement to ThreadContext.stashContext for plugins to ensure that plugins run with authz checks when changing contexts (you have been involved in this review)
   a. Changes on main that account for ActionPrivileges: Implement new extension points in IdentityPlugin and add ContextProvidingPluginSubject #4896
   b. Changes on 2.x that account for legacy authz impl: [2.x] Implement new extension points in IdentityPlugin and add ContextProvidingPluginSubject #5001

2. API Tokens that @derek-ho has been working on on a feature branch: [RFC] Support for API Keys in OpenSearch Security Plugin #4009
   a. Recent PR that touches authz: Api token authc/z implementation with Cache #4992

[nibix]

@kumargu

Thanks for the replies! Yes, let's have a meeting soon. I am flexible both on Monday and Tuesday. Feel free to suggest a time :)

Sorry, I can't convince myself why the commit with flag should not be carried to main.

The code change will dramatically reduce code quality.

• The flag required the addition of more than 8000 LOC of more or less duplicated code. (See commits nibix@cc1d22a and nibix@ec08e4d ).
• In order to separate old and new implementation, I used different Java packages. However, this made raising of method and attribute visibility from package private to public necessary.
• All authz related integration tests need to be run twice. For some integration tests, this was possible with test params. Other tests are implemented in a way that this is not possible. These needed to be completely duplicated. The total test execution time will significantly increase.
• BWC and release tests won't be able to cover both implementations without significant effort.
• Any future contributor to these code parts need to be made aware that they need to implement the logic in both code bases. This implementation is not trivial, as the old and new code bases use fundamentally different approaches. All reviewers must be aware that they need to verify that both versions are implemented.
• @cwperks already mentioned a couple of projects which will significantly gain in complexity due to this.

The trouble is that we are not talking about "just a feature", but about the core, which has tight coupling to many other parts.

Please do not get me wrong: I do understand your goals and if we do not find any other way, we can do all this; but as I am also here a maintainer, I need to point out that this solution has issues from a maintenance point of view.

@cwperks

When it comes time for the 3.0 release, my understanding is that a new branch will be cut from main called 3.x and then another branch will be cut from that called 3.0.

Not necessarily. There's a proposal to change this: opensearch-project/.github#251