Remove static metaFields list and get version-specific values from core

[cwperks]

Description

Remove static metaFields list and get version-specific values from core.

The new list obtained from core is: [_ignored, _routing, _seq_no, _doc_count, _index, _nested_path, _data_stream_timestamp, _field_names, _source, _id, _version, _primary_term]

This doesn't match the hardcoded list because it removes many of the deprecated fields from META_FIELDS_BEFORE_7DOT8

When using FLS Includes rules, this list is used as the starting point for all metaFields to include by default.

• Category

Bug fix

Issues Resolved

• [BUG] Running a nested path query from a user with FLS Includes rules returns empty results #4349

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

All modified and coverable lines are covered by tests ✅

Project coverage is 65.40%. Comparing base (f71d2e6) to head (13d0fd4).
Report is 11 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4370      +/-   ##
==========================================
- Coverage   65.43%   65.40%   -0.04%     
==========================================
  Files         310      310              
  Lines       21992    22001       +9     
  Branches     3554     3554              
==========================================
- Hits        14391    14389       -2     
- Misses       5830     5841      +11     
  Partials     1771     1771              

Files	Coverage Δ
...figuration/SecurityFlsDlsIndexSearcherWrapper.java	97.61% <100.00%> (+0.39%)	⬆️

... and 3 files with indirect coverage changes

[RyanL1997]

Is this a backport 2.x ?

[cwperks]

Is this a backport 2.x ?

Yes it is. I added the label.

opensearch-project/OpenSearch#13822 must be merged first into OS core.

[peternied]

Lets add a test case that verifies that all previous meta items are still covered so we aren't leaking any sensitive data.

I'm curious on your thoughts, how do you think we can build confidence around meta field exposure? I'm not sure we have strong coverage right now.

[cwperks]

@peternied See the PR description. The new list is different and removes the legacy fields: "_size", "_timestamp", "_ttl", "_type" (Fields from this list).

On the backport to 2.x, I plan to keep those fields because they are still referenced here: https://github.com/opensearch-project/OpenSearch/blob/2.x/server/src/main/java/org/opensearch/search/SearchHit.java#L188-L201

This datastructure is used as the default includesSet for FLS when using includes rules. Effectively, that means that these are the fields that are explicitly included without having to list them out in the FLS portion of a role. If keys are removed from this list then they will not be returned in the result set when performing a search query when a user is mapped to a role containing FLS includes rules.

[peternied]

The new list is different and removes the legacy fields: "_size", "_timestamp", "_ttl", "_type" (Fields from this list).

@cwperks Thanks - that's a great reason to tweak the test case coverage based on fields listed above. Is there a reason you think we shouldn't add a test case that would be sure that say _source is specifically filtered as we expect?

[cwperks]

@peternied I will flip this to Draft while a test case is worked on and to understand the legacy fields in greater detail.

[cwperks]

@peternied Setting this back to in review. I can't find a way to assert that the list after this change is a superset of the list before this change, but I also don't think that would be a good test case either. For the fields that have been removed like "_timestamp", "_ttl" and "_type", I have added them back in to keep the list the same as before. I have also added "_size", but if someone installed the mapper-size plugin then "_size" will be added twice (its fine since its a set).

The list of built-in metadata fields can be found here: https://github.com/opensearch-project/OpenSearch/blob/main/server/src/main/java/org/opensearch/indices/IndicesModule.java#L190-L210

I plan to open a PR on the documentation-website to document the mapper-size plugin.

Let me know what you think.

Updated with additional details

[peternied]

@cwperks Thanks for the update on this PR, I'm still on the fence about merging because there isn't any associated test cases, is there an existing test that will catch issues if this list starts behaving strange?

[cwperks]

@peternied I just pushed a new commit that includes a test.

To write a test, I decided to leverage the mapper-size plugin from core which is not installed by default. If this logic is working correctly, "_size" should be included in the list given to the security plugin from core since the MapperSize plugin is installed. I updated the integration test for FlsAndFieldMaskingTests to add a test where it creates an index and enables "_size" on the index. The test does a search request against the index with a user that's only allowed to see a single field "stars". The test verifies that the user can see "stars", but not other fields like "artist" indicating that FLS Includes is working as intended. The test also verifies that "_size" is returned in the response indicating that core gave the security plugin that field.

If you explicitly remove "_size" from the metaFields tracked by SecurityFlsDlsIndexSearcherWrapper the test will fail because it would be filtered out by the FLS restrictions.

FYI "_size" will not be included in the list by default like earlier, however, it will be included for clusters where the MapperSize plugin is installed and this automated test proves it.

[peternied]

@cwperks Nicely done thanks!