Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[2.x Backport] Optimized Privilege Evaluation: Action privileges ONLY, with feature flag #4998

Conversation

nibix
Copy link
Collaborator

@nibix nibix commented Dec 30, 2024

Description

This implements the optimized privilege evaluation as described in #3870 and partially backports the changes from #4380 to the 2.x branch.

This supersedes the PR #4898 . The changes are an outcome from the requests that came up in the review of #4898.

Particularly, this PR now contains both the new privilege evaluation implementation and the old privilege evaluation implementation. The new implementation is active by default; the old implementation can be restored by setting the opensearch.yml setting plugins.security.privileges_evaluation.use_legacy_impl to true.

Additionally, this contains ONLY the changes related to action privileges. The DLS/FLS implementation is still the old one.

  • Category: Enhancement
  • Why these changes are required?

Performance tests indicate that the OpenSearch security layer adds a noticeable overhead to the indexing throughput of an OpenSearch cluster. The overhead may vary depending on the number of indices, the use of aliases, the number of roles and the size of the user object. The goal of these changes is to improve privilege evaluation performance and to make it less dependent on the number of indices, etc.

The additional preservation of the old implementation has been added on request in order to have an option to roll back to the old implementation in case issues are found in the new implementation. Unfortunately, this makes it necessary to double up quite a lot of code.

  • What is the old behavior before changes and new behavior after changes?

No significant behavioral changes in the "happy case", when privileges are present.

The undocumented config option config.dynamic.multi_rolespan_enabled is no longer evaluated. The code now behaves like it is always set to true - that is the former default. See #4495 for details.

Some slight changes are present in error cases:

  • More detailed error messages for missing privileges, showing a index/action matrix of missing privileges
  • Errors in the role configuration might be reported (as error log messages) more early, directly after the configuration was applied

Issues Resolved

Partially #3870

This is a partial backport from #4380

Testing

  • Each new component is accompanied by its own unit test.
  • The high-level functionality is validated by the existing integration tests

Check List

  • New functionality includes testing
  • 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.

nibix added 10 commits December 30, 2024 15:13
Signed-off-by: Nils Bandener <nils.bandener@eliatra.com>
Signed-off-by: Nils Bandener <nils.bandener@eliatra.com>
Signed-off-by: Nils Bandener <nils.bandener@eliatra.com>
Signed-off-by: Nils Bandener <nils.bandener@eliatra.com>
Signed-off-by: Nils Bandener <nils.bandener@eliatra.com>
Signed-off-by: Nils Bandener <nils.bandener@eliatra.com>
Signed-off-by: Nils Bandener <nils.bandener@eliatra.com>
…d_privileges.include_indices

See discussion in opensearch-project#4380 (comment)

Signed-off-by: Nils Bandener <nils.bandener@eliatra.com>
Signed-off-by: Nils Bandener <nils.bandener@eliatra.com>
Signed-off-by: Nils Bandener <nils.bandener@eliatra.com>
@kumargu
Copy link

kumargu commented Dec 30, 2024

Thanks @nibix !

Signed-off-by: Nils Bandener <nils.bandener@eliatra.com>
Copy link

codecov bot commented Jan 3, 2025

Codecov Report

Attention: Patch coverage is 73.90777% with 430 lines in your changes missing coverage. Please review.

Project coverage is 64.45%. Comparing base (15c0b19) to head (ec08e4d).
Report is 9 commits behind head on 2.x.

Files with missing lines Patch % Lines
...ity/privileges/legacy/PrivilegesEvaluatorImpl.java 60.45% 76 Missing and 47 partials ⚠️
...h/security/privileges/PrivilegesEvaluatorImpl.java 73.13% 44 Missing and 46 partials ⚠️
...ensearch/security/privileges/ActionPrivileges.java 87.72% 30 Missing and 18 partials ⚠️
...g/opensearch/security/privileges/IndexPattern.java 51.08% 43 Missing and 2 partials ⚠️
.../privileges/legacy/SystemIndexAccessEvaluator.java 70.63% 30 Missing and 7 partials ⚠️
...y/privileges/legacy/TermsAggregationEvaluator.java 53.57% 4 Missing and 9 partials ⚠️
...ivileges/legacy/ProtectedIndexAccessEvaluator.java 74.46% 8 Missing and 4 partials ⚠️
...rity/privileges/PrivilegesEvaluationException.java 0.00% 9 Missing ⚠️
...curity/privileges/PrivilegesEvaluatorResponse.java 78.57% 7 Missing and 2 partials ⚠️
...leges/ClusterStateMetadataDependentPrivileges.java 76.66% 6 Missing and 1 partial ⚠️
... and 13 more
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##              2.x    #4998      +/-   ##
==========================================
+ Coverage   63.95%   64.45%   +0.49%     
==========================================
  Files         330      344      +14     
  Lines       23185    24433    +1248     
  Branches     3755     4052     +297     
==========================================
+ Hits        14828    15748     +920     
- Misses       6526     6749     +223     
- Partials     1831     1936     +105     
Files with missing lines Coverage Δ
.../opensearch/security/OpenSearchSecurityPlugin.java 84.68% <100.00%> (+0.14%) ⬆️
...search/security/configuration/DlsFlsValveImpl.java 59.93% <ø> (ø)
...rity/configuration/SystemIndexSearcherWrapper.java 91.37% <100.00%> (ø)
...org/opensearch/security/filter/SecurityFilter.java 65.87% <100.00%> (-0.48%) ⬇️
...ch/security/privileges/PitPrivilegesEvaluator.java 96.15% <100.00%> (-0.15%) ⬇️
...earch/security/privileges/PrivilegesEvaluator.java 100.00% <100.00%> (+28.12%) ⬆️
...rch/security/privileges/PrivilegesInterceptor.java 88.23% <ø> (ø)
...ch/security/securityconf/DynamicConfigFactory.java 55.88% <ø> (ø)
...ecurityconf/impl/SecurityDynamicConfiguration.java 81.15% <100.00%> (+2.62%) ⬆️
...ecurity/privileges/SystemIndexAccessEvaluator.java 71.31% <83.33%> (+0.68%) ⬆️
... and 22 more

... and 6 files with indirect coverage changes

Signed-off-by: Nils Bandener <nils.bandener@eliatra.com>
@cwperks
Copy link
Member

cwperks commented Jan 10, 2025

Primary review performed in #4380

The changes in this PR look reasonable to me. Thank you for making sure that any tests that can are run on both code paths. I don't have any major concerns. @kumargu can you take a look as well?

@kumargu
Copy link

kumargu commented Jan 10, 2025

sure. I will take a look at it.

@DarshitChanpura DarshitChanpura merged commit 4af1d07 into opensearch-project:2.x Jan 13, 2025
81 checks passed
@DarshitChanpura
Copy link
Member

Thank you @nibix @cwperks @kumargu for bringing this home!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants