Add support for DLS Term Lookup Queries #1541

jochenkressin · 2021-12-21T14:25:57Z

Signed-off-by: Jochen Kressin [email protected]

opensearch-security pull request intake form

This PR adds support for Term Lookup Queries (TLQ) in Document-level security. It introduces two different modes for executing DLS filters which can either be set in opensearch.yml or are chosen automatically by Opensearch Security.

Lucene-level DLS
This mode implements DLS by modifying Lucene queries and data structures directly. This is the most efficient mode but does not allow certain advanced constructs in DLS queries, including TLQ. This is how DLS works currently.

Filter-level DLS
With this mode, DLS is applied by modifying queries as received by OpenSearch. This allows the use of term lookup queries in DLS queries but limits the set of operations that can be used to retrieve data from the protected index to get, search, mget, msearch. Also, the use of Cross Cluster Searches is limited in this mode.

By default, OpenSearch Security will detect if a DLS query contains a TLQ or not, and chooses the appropriate mode automatically at runtime. The mode can also be set explicitly in opensearch.yml:

plugins.security.dls.mode: <lucene-level | filter-level | adaptive>

lucene-level: enforces that all DLS queries are applied on Lucene-level. This is the current implementation
filter-level: enforces that all DLS queries are applied on filter level
adaptive (default): OpenSearch chooses the mode automatically. DLS queries without TLQ are executed on Lucene level, while DLS queries that contain TLQ are executed on filter level.

Category: (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation)

Enhancement / New feature

Github Issue # or road-map entry, if available:

#1508

Description of changes:

As described above, this PR introduces two different approaches to executing DLS queries.

Why these changes are required?

TLQ in DLS queries can only be supported if the query is executed on filter level.

What is the old behavior before changes and new behavior after changes? (Please add any example/logs/screen-shot if available)

If a DLS query contains a TLQ, an exception is thrown. Now a DLS query can also contain a TLQ. Per default, Opensearch Security detects a TLQ and sets the corresponding mode automatically.

Testing done: (Please provide details of testing done: Unit testing, integration testing and manual testing)

Executed existing tests and added new tests for TLQ (DlsTermLookupQueryTest).

TO-DOs, if any: (Please describe pending items and provide Github issues# for each of them)

If this PR is accepted the documentation needs to be updated accordingly.

Is it backport from main branch? (If yes, please add backport PR # and commits #)

No.

Additional notes:

I excluded the transitive dependency to JUnit5 from spring-kafka-test in pom.xml. This was causing issues when running the Junit4 unit and integration tests in Eclipse
For the integration tests, I disabled the high disk watermark checks. I think it is not necessary to have this on during the tests, and it caused some hard-to-spot issues when running the tests locally.

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or

(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or

(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.

(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.

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.

rursprung

thanks a lot for this PR!

i only gave this a cursory glance (i do not have the necessary understanding of the code for anything more + i'm theoretically on vacation).

mainly i did nit-pickings in two areas:

formatting (i raised add .editorconfig #1547 to streamline this - i'm a big fan of using tools to avoid even having to talk about these topics 🙂)
atomic commits (you mentioned two things also in the PR description which were unrelated)

furthermore: this is a huge commit, yet the commit-msg has one single line. but at the end, the commit-msg will be the only thing which'll survive (the commit history might well live longer than the github repo, due to renamings, moves, etc.). i'm a (very) big proponent of proper commit messages which explain all relevant things. some commit messages can be short, but with commits as complex as this one here a one-liner isn't cutting it. at the very least the description which you've now only added in the PR description should be (properly formatted!) in the commit message. and maybe this'll also show that the commit should be split further up (if that's possible, they should be atomic - but that also means that each should be consistent in itself) so that the necessary descriptions can be added to individual commits, making things clearer.

pom.xml

src/main/java/org/opensearch/security/configuration/DlsFilterLevelActionHandler.java

src/test/java/org/opensearch/security/dlic/dlsfls/DlsTermLookupQueryTest.java

src/test/java/org/opensearch/security/test/AbstractSecurityUnitTest.java

src/test/resources/dlsfls/roles_mapping_tlq.yml

src/test/resources/dlsfls/roles_tlq.yml

src/test/resources/dlsfls/securityconfig_tlq.yml

src/test/java/org/opensearch/security/dlic/dlsfls/DlsTermLookupQueryTest.java

davidlago · 2022-01-06T01:07:07Z

Thanks @jochenkressin! In addition to addressing the comments above, this test seems to be failing (and based on the name could be related to these changes)

src/main/java/org/opensearch/security/privileges/DocumentWhitelist.java

src/test/resources/dlsfls/securityconfig_tlq.yml

src/main/java/org/opensearch/security/securityconf/ConfigModelV7.java

src/test/java/org/opensearch/security/dlic/dlsfls/AbstractDlsFlsTest.java

src/test/resources/dlsfls/internal_users_tlq.yml

src/test/resources/dlsfls/securityconfig_tlq.yml

src/test/java/org/opensearch/security/dlic/dlsfls/DlsTermsLookupAsserts.java

src/test/java/org/opensearch/security/dlic/dlsfls/DlsTermLookupQueryTest.java

src/test/resources/dlsfls/roles_tlq.yml

src/test/java/org/opensearch/security/dlic/dlsfls/DlsTermLookupQueryTest.java

src/test/java/org/opensearch/security/dlic/dlsfls/AbstractDlsFlsTest.java

src/main/java/org/opensearch/security/transport/SecurityInterceptor.java

peternied

I know there is some follow up, but I don't have any outstanding feedback. Thank you for this contribution!

codecov-commenter · 2022-03-04T14:17:28Z

Codecov Report

Merging #1541 (897ca1b) into main (d96e2d2) will decrease coverage by 0.79%.
The diff coverage is 49.55%.

@@             Coverage Diff              @@
##               main    #1541      +/-   ##
============================================
- Coverage     63.68%   62.88%   -0.80%     
- Complexity     3165     3259      +94     
============================================
  Files           249      253       +4     
  Lines         17479    18127     +648     
  Branches       3096     3258     +162     
============================================
+ Hits          11131    11399     +268     
- Misses         4795     5079     +284     
- Partials       1553     1649      +96

Impacted Files	Coverage Δ
...rch/security/configuration/DlsFlsRequestValve.java	`0.00% <ø> (ø)`
...opensearch/security/filter/SecurityRestFilter.java	`76.66% <ø> (ø)`
...pensearch/security/securityconf/ConfigModelV6.java	`27.81% <0.00%> (+0.31%)`	⬆️
...g/opensearch/security/support/ConfigConstants.java	`94.44% <ø> (ø)`
...search/security/queries/QueryBuilderTraverser.java	`19.08% <19.08%> (ø)`
...security/support/ReflectiveAttributeAccessors.java	`22.22% <22.22%> (ø)`
...nsearch/security/privileges/DocumentAllowList.java	`39.69% <39.69%> (ø)`
...search/security/configuration/DlsFlsValveImpl.java	`58.17% <54.28%> (-12.06%)`	⬇️
...ity/configuration/DlsFilterLevelActionHandler.java	`55.50% <55.50%> (ø)`
...search/security/transport/SecurityInterceptor.java	`75.38% <57.69%> (+1.43%)`	⬆️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d96e2d2...897ca1b. Read the comment docs.

Please resolve the conflicts before merging.

peternied · 2022-03-08T16:05:59Z

@jochenkressin Could you resolve the merge conflicts so we can merge this PR?

exclude replication requests from DLS/FLS, removed unused cluster action cleanup: removed sg references and outdated comments Make sure that SG_FILTER_LEVEL_DLS_DONE is reset after request finished handle masked field aggregations, reactivate buckeg merging Signed-off-by: Jochen Kressin <[email protected]>

Signed-off-by: Jochen Kressin <[email protected]>

jochenkressin · 2022-03-08T16:57:14Z

@peternied I rebased to main

cliu123 · 2022-03-10T19:24:57Z

LGTM!
Re-trigerring CI.
Please squash the commits and remove all unnecessary texts in the commit message upon merging.

src/main/java/org/opensearch/security/configuration/DlsQueryParser.java

Sorry, forgot to post the comments on Log4j.

src/main/java/org/opensearch/security/configuration/DlsFilterLevelActionHandler.java

src/test/java/org/opensearch/security/dlic/dlsfls/AbstractDlsFlsTest.java

Signed-off-by: Jochen Kressin <[email protected]>

jochenkressin · 2022-03-11T13:24:51Z

@cliu123 @peternied I removed the references to log4j from the PR

opensearch-trigger-bot · 2022-03-11T17:48:52Z

The backport to 1.3 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-1.3 1.3
# Navigate to the new working tree
cd .worktrees/backport-1.3
# Create a new branch
git switch --create backport/backport-1541-to-1.3
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 920701e236680b83e8aa25ef5d027884d3cf4768
# Push it to GitHub
git push --set-upstream origin backport/backport-1541-to-1.3
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-1.3

Then, create a pull request where the base branch is 1.3 and the compare/head branch is backport/backport-1541-to-1.3.

Signed-off-by: Jochen Kressin <[email protected]>

…1541) Signed-off-by: Craig Perkins <[email protected]>

jochenkressin requested a review from a team December 21, 2021 14:25

rursprung reviewed Dec 23, 2021

View reviewed changes

src/test/java/org/opensearch/security/dlic/dlsfls/DlsTermLookupQueryTest.java Outdated Show resolved Hide resolved

peternied reviewed Feb 28, 2022

View reviewed changes

src/main/java/org/opensearch/security/privileges/DocumentWhitelist.java Outdated Show resolved Hide resolved