Fix Document GET with DLS terms query due to Lucene upgrade #3136

cwperks · 2023-08-09T18:25:33Z

Description

Fixes an error on document retrieval for users that are mapped to roles with DLS rules including a terms query. The bug was introduced by a change in Lucene and reported on Slack here. This fix adds a test to catch an error like this at build time and assert the intended behavior.

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

Bug fix

Issues Resolved

#3088

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.

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

cwperks · 2023-08-09T18:26:43Z

This PR adds a test, but I also verified the change locally by creating a local distribution with the demo security config and executing the following API requests:

curl -XPUT https://admin:admin@localhost:9200/examples --insecure

curl -XPOST https://admin:admin@localhost:9200/examples/_doc/1 --insecure -H "Content-Type: application/json" --data '
{ "foo": "bar" }
'

curl -XPOST https://admin:admin@localhost:9200/examples/_doc/2 --insecure -H "Content-Type: application/json" --data '
{ "foo": "baz" }
'

curl -XGET https://admin:admin@localhost:9200/examples/_search --insecure


curl -XGET https://admin:admin@localhost:9200/examples/_search --insecure -H "Content-Type: application/json" --data '
{ "query": {"terms": { "foo" : ["bar"] } } }
'


curl -XPUT https://admin:admin@localhost:9200/_plugins/_security/api/internalusers/craig --insecure -H "Content-Type: application/json" --data '
{  
  "password": "TestPassword123_"
}
'

curl -XPUT https://admin:admin@localhost:9200/_plugins/_security/api/roles/example_role --insecure -H "Content-Type: application/json" --data '
{
    "cluster_permissions":
    [
        "cluster_composite_ops",
        "indices_monitor"
    ],
    "index_permissions":
    [
        {
            "index_patterns":
            [
                "examples*"
            ],
            "allowed_actions":
            [
                "read"
            ],
            "dls": "{ \"terms\": { \"foo\" : [\"bar\"] } }"
        }
    ]
}
'

curl -XPUT https://admin:admin@localhost:9200/_plugins/_security/api/rolesmapping/example_role --insecure -H "Content-Type: application/json" --data '
{  
  "users" : [ "craig" ]
}
'

curl -XGET https://craig:TestPassword123_@localhost:9200/examples/_search --insecure

curl -XGET https://craig:TestPassword123_@localhost:9200/examples/_doc/1 --insecure

curl -XGET https://admin:admin@localhost:9200/examples/_doc/1 --insecure

codecov · 2023-08-09T18:29:39Z

Codecov Report

Merging #3136 (a6be927) into main (4593be7) will decrease coverage by 27.07%.
The diff coverage is 100.00%.

@@              Coverage Diff              @@
##               main    #3136       +/-   ##
=============================================
- Coverage     62.42%   35.36%   -27.07%     
+ Complexity     3353     1826     -1527     
=============================================
  Files           254      254               
  Lines         19749    19749               
  Branches       3334     3334               
=============================================
- Hits          12329     6984     -5345     
- Misses         5788    11701     +5913     
+ Partials       1632     1064      -568

Files Changed	Coverage Δ
...security/configuration/DlsFlsFilterLeafReader.java	`61.52% <100.00%> (ø)`

... and 156 files with indirect coverage changes

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

peternied · 2023-08-09T18:53:33Z

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

@@ -232,7 +232,7 @@ public DlsGetEvaluator(final Query dlsQuery, final LeafReader in, boolean applyD
                // https://github.com/apache/lucene-solr/blob/branch_6_3/lucene/misc/src/java/org/apache/lucene/index/PKIndexSplitter.java
                final IndexSearcher searcher = new IndexSearcher(DlsFlsFilterLeafReader.this);
                searcher.setQueryCache(null);
-                final Weight preserveWeight = searcher.createWeight(dlsQuery, ScoreMode.COMPLETE_NO_SCORES, 1f);
+                final Weight preserveWeight = searcher.rewrite(dlsQuery).createWeight(searcher, ScoreMode.COMPLETE_NO_SCORES, 1f);


Yikes! What a terrible side effect. It doesn't seem clear how we can identify issues like this if we have them elsewhere other than having had a functional test case fail. How confident are you in this fix, and our coverage on these scenarios?

This was not caught with CI because there is no instance of a document GET request from a user mapped to a role with a DLS terms query associated with it. The test added in this PR would ensure that this would be caught at build time.

We could probably enhance this test suite by running different kinds of requests in addition to search requests and aggregations.

Could you create a follow up task to analyze what test we should have / are missing?

Will it work for indices that were created prior to 2.9 or 2.10 with the shard version, e.g., 7.10.x?

@peternied I filed an issue to asses the current state of FLS, DLS and Field Masking tests and determine what other scenarios should be tested: #3139

Hey @willyborankin , I plan to test this out today. Thank you for pointing out potential issues with backward compatibility depending on the version of Lucene the shards are created with.

@willyborankin I tested this by creating an index with ES 7, doing a full cluster upgrade to 2.8 w/ this change included and verified that it works.

DarshitChanpura · 2023-08-09T19:01:11Z

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

        System.out.println("q");
        System.out.println(Strings.toString(XContentType.JSON, tc.search(new SearchRequest().indices(".opendistro_security")).actionGet()));


out of scope of this PR; but we can get rid of these sysouts to boost test performance.

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

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

RyanL1997 · 2023-08-10T04:36:06Z

src/test/java/org/opensearch/security/test/helper/rest/RestHelper.java

+            return getJsonNodeAt(jsonPointer).asText();
+        }
+
+        private JsonNode getJsonNodeAt(String jsonPointer) {


qq: If the method getJsonNodeAt(jsonPointer) does not find the specified node, what is returned? Is it null? If it's a possible scenario, we might want to handle it or at least document the expected behavior.

I ran into this when testing to figure out the correct JSON path. It returns an empty string.

This method is used in assertion statements like this:

Assert.assertEquals(res.getTextFromJsonBody("/hits/total/value"), "1");

so the assertion will fail.

opensearch-trigger-bot · 2023-08-14T16:50:58Z

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-3136-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 88b6d23f0c84d83f138cf1a61bbe0145d8dd007e
# Push it to GitHub
git push --set-upstream origin backport/backport-3136-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-3136-to-2.x.

cwperks · 2023-08-14T16:51:38Z

I will create a manual backport for this

Fixes an error on document retrieval for users that are mapped to roles with DLS rules including a terms query. The bug was introduced by a change in Lucene and reported on Slack [here](https://opensearch.slack.com/archives/C0539F41Z5X/p1690714779140659). This fix adds a test to catch an error like this at build time and assert the intended behavior. * Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation) Bug fix opensearch-project#3088 - [ ] 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](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin). --------- Signed-off-by: Craig Perkins <[email protected]> (cherry picked from commit 88b6d23)

Backport #3136 to 2.x

Fix Document GET with DLS terms query

7c7ad9c

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

cwperks requested review from cliu123, DarshitChanpura, davidlago, peternied, RyanL1997, stephen-crawford, reta and willyborankin as code owners August 9, 2023 18:25

cwperks added the backport 2.x backport to 2.x branch label Aug 9, 2023

peternied reviewed Aug 9, 2023

View reviewed changes

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

peternied reviewed Aug 9, 2023

View reviewed changes

DarshitChanpura reviewed Aug 9, 2023

View reviewed changes

Use getTextFromJsonBody

c233b07

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

peternied approved these changes Aug 9, 2023

View reviewed changes

RyanL1997 reviewed Aug 10, 2023

View reviewed changes

Merge branch 'main' into fix-dls-term-query

a6be927

willyborankin approved these changes Aug 14, 2023

View reviewed changes

cwperks merged commit 88b6d23 into opensearch-project:main Aug 14, 2023

opensearch-trigger-bot bot added the backport-failed label Aug 14, 2023

cwperks mentioned this pull request Aug 14, 2023

[Backport 2.x] Fix Document GET with DLS terms query (#3136) #3175

Merged

cwperks added a commit that referenced this pull request Aug 15, 2023

[Backport 2.x] Fix Document GET with DLS terms query (#3136) (#3175)

4a14af4

Backport #3136 to 2.x

peternied changed the title ~~Fix Document GET with DLS terms query~~ Fix Document GET with DLS terms query due to Lucene upgrade Aug 31, 2023

peternied removed the backport-failed label Aug 31, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Document GET with DLS terms query due to Lucene upgrade #3136

Fix Document GET with DLS terms query due to Lucene upgrade #3136

cwperks commented Aug 9, 2023

cwperks commented Aug 9, 2023

codecov bot commented Aug 9, 2023 •

edited

Loading

peternied Aug 9, 2023

cwperks Aug 9, 2023

peternied Aug 9, 2023

willyborankin Aug 10, 2023 •

edited

Loading

cwperks Aug 10, 2023

cwperks Aug 14, 2023

cwperks Aug 14, 2023 •

edited

Loading

DarshitChanpura Aug 9, 2023

RyanL1997 Aug 10, 2023

cwperks Aug 14, 2023

opensearch-trigger-bot bot commented Aug 14, 2023

cwperks commented Aug 14, 2023

		System.out.println("q");
		System.out.println(Strings.toString(XContentType.JSON, tc.search(new SearchRequest().indices(".opendistro_security")).actionGet()));

Fix Document GET with DLS terms query due to Lucene upgrade #3136

Fix Document GET with DLS terms query due to Lucene upgrade #3136

Conversation

cwperks commented Aug 9, 2023

Description

Issues Resolved

Check List

cwperks commented Aug 9, 2023

codecov bot commented Aug 9, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willyborankin Aug 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwperks Aug 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented Aug 14, 2023

cwperks commented Aug 14, 2023

codecov bot commented Aug 9, 2023 •

edited

Loading

willyborankin Aug 10, 2023 •

edited

Loading

cwperks Aug 14, 2023 •

edited

Loading