PR opensearch-project#6416 signalled "DLS was applied to the top-level hybrid query" by
storing a sentinel value in the existing
OPENDISTRO_SECURITY_FILTER_LEVEL_DLS_DONE header, and redefined
isDlsDoneOnFilterLevel() to exclude that value.
That marker is transport-propagated to data nodes. A node that predates
the sentinel cannot distinguish it from "true", so isDlsDoneOnFilterLevel()
returns true, SecurityFlsDlsIndexSearcherWrapper takes DlsRestriction.NONE
and builds no DLS query, and reader-level DLS is skipped on that shard.
Hits are still protected by the injected hybrid filter, but global
aggregations are not, because a global aggregation ignores the top-level
query by definition. The design therefore fails open and needs the
Version.V_3_9_0 gate to be correct at all.
Use a dedicated OPENDISTRO_SECURITY_HYBRID_QUERY_DLS_APPLIED header
instead. A node that does not know the header sees no marker and runs its
normal reader-level DLS path, so the change fails closed by construction
and the version gate is no longer required:
- isDlsDoneOnFilterLevel() returns to a plain null check, so its four
callers keep their original meaning.
- Remove HYBRID_QUERY_DLS_FILTER_SUPPORTED_SINCE and
isHybridQueryDlsFilterSupported, dropping one gate from
shouldApplyDlsFilterToHybridQueryInAdaptiveMode.
- Add isDlsAlreadyHandled() for the two coordinator-side re-entry guards,
which now consult both markers. These run only on the coordinating node.
- Copy the new header to local destinations and strip it for destinations
outside the local cluster.
Without a version constant the identical patch applies to every release
line, which matters for backports: a Version comparison cannot express
"has the fix" across 3.5.x, 3.7.x, and 3.9 at once, and would disable the
fix on exactly the mixed-version clusters a backport is meant to serve.
On a mixed-version cluster the hybrid request now fails on shards hosted
by nodes without the fix, as it does today, rather than silently dropping
reader-level DLS.
The runtime verification in applyFilterLevelDls is unchanged and remains
what actually guarantees the Neural Search hybrid-query contract.
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Description
Follow-up to #6416. That PR is correct on a fully-upgraded cluster, but it signals its state through a sentinel value stored in an existing header, and that choice is what forces the
Version.V_3_9_0gate to exist. This replaces the sentinel with a dedicated header so the mechanism is fail-safe on its own, and removes the version gate.No behavior change on a cluster where every node has the fix.
Why the sentinel value needs a version gate
#6416 wrote the marker as a value of
OPENDISTRO_SECURITY_FILTER_LEVEL_DLS_DONEand redefinedisDlsDoneOnFilterLevel()to exclude it:That header is transport-propagated to data nodes. On a node that predates the sentinel, the original definition applies — any non-null value means "filter-level DLS handled this request" — and the consumer that matters is
SecurityFlsDlsIndexSearcherWrapper:So that shard takes
DlsRestriction.NONE, builds nodlsQuery, and applies no reader-level DLS. Hits are still protected by the DLS filter pushed into the hybrid subqueries, but aglobalaggregation is not, because a global aggregation ignores the top-level query by definition — and retaining reader-level DLS for exactly that case is the stated purpose of the marker.The design fails open, which is why it needs the version gate to be correct.
What this changes
Use a dedicated
OPENDISTRO_SECURITY_HYBRID_QUERY_DLS_APPLIEDheader instead of overloading the existing one. A node that does not know the header sees absence, and absence means "do the normal thing" — compute the restriction, build the DLS query, apply reader-level DLS. The mechanism fails closed by construction, so there is nothing left to version-gate.isDlsDoneOnFilterLevel()returns to a plain null check. Its four callers keep their original meaning, including the untouchedSecurityFlsDlsIndexSearcherWrapper.HYBRID_QUERY_DLS_FILTER_SUPPORTED_SINCEandisHybridQueryDlsFilterSupportedare removed, dropping one gate fromshouldApplyDlsFilterToHybridQueryInAdaptiveModeand theVersionimport fromDlsFlsValveImpl.isDlsAlreadyHandled()backs the two coordinator-side re-entry guards, which now consult both markers. These stop the child request dispatched throughnodeClientfrom being reprocessed and run only on the coordinating node, so they carry no compatibility exposure.SecurityInterceptorcopies the new header to local destinations and strips it for destinations outside the local cluster. The strip is now an unconditionalremoveof our own key rather than a conditional edit of a shared header's value.The runtime verification in
applyFilterLevelDlsis unchanged. It is still what actually guarantees the Neural Search hybrid-query contract, and it needs no version information.Why this matters for backports
A
Versioncomparison cannot express "has the fix" across several release lines at once:DlsFlsValveImpldiverges and conflicts on every later backport touching the file.3.5.0GA would not contain the fix, soV_3_5_0is too permissive and reopens the gap, whileV_3_5_4requires a constant that OpenSearch core may not have declared yet.main,minNodeVersion.onOrAfter(V_3_9_0)is false for a 3.7-with-fix + 3.9 cluster, disabling the fix on exactly the mixed-version clusters a backport is meant to serve.Without a version constant, the identical patch applies to every release line.
Mixed-version behavior
Shards on nodes with the fix return correct results. Shards on nodes without it wrap the parsed query as they do today and fail the hybrid-query shape, so the request surfaces a shard failure — the same class of failure as the bug #6416 fixed, and never a silent loss of reader-level DLS. This is a documentable rolling-upgrade limitation rather than a security regression, which the sentinel-value design could not offer.
Issues Resolved
Follow-up to #6416. No new issue; this hardens and simplifies the mechanism introduced there and unblocks backporting it.
Testing
./gradlew testfor the five affected suites: 87 tests, 0 failures, 0 skipped../gradlew test \ --tests org.opensearch.security.configuration.DlsFlsFilterLeafReaderTest \ --tests org.opensearch.security.configuration.DlsFilterLevelActionHandlerTest \ --tests org.opensearch.security.configuration.DlsFlsValveImplTest \ --tests org.opensearch.security.privileges.dlsfls.DlsFlsBaseContextTest \ --tests org.opensearch.security.transport.SecurityInterceptorTestsTest changes:
hybridQueryDlsFilterRequiresOpenSearchThreeNineOnEveryNodeanddoesNotApplyHybridQueryFilterWhenClusterContainsOlderNode, which covered the deleted gate.SecurityInterceptorTestsasserts the new header is copied to local nodes and stripped for an unrecognized destination../gradlew precommitpasses except for two failures that reproduce identically on unmodifiedmain(verified by stashing this change and re-running) and involve no file touched here::licenseHeaders— 38 pre-existing unapproved headers underauth/**andssl/**.:opensearch-sample-resource-plugin:forbiddenApisMain—URL#openStream()inCreateResourceTransportAction:77andCreateResourceGroupTransportAction:70.Cross-plugin verification
Published this branch to Maven Local and ran opensearch-project/neural-search#1957's
HybridQueryDlsITon a three-node secured cluster, which is what actually exercises the header crossing the transport layer:testDlsFiltersHybridHitsAndAggregationstestDlsSupportsSuggestionBearingHybridRequesttestDlsCombinesWithExistingHybridFiltertestDlsSupportsSingleClauseHybridQuerytestDlsFiltersHybridKnnQuerytestDlsFiltersHybridNeuralQuerytestDlsFiltersHybridHitsAndAggregationsis the important one: it covers the DLS-protected global aggregation, which is precisely the behavior the version gate existed to protect, and it passes across three nodes with the dedicated header. That confirms the header propagates through theSecurityInterceptorcopy allowlist and that reader-level DLS stays active on non-coordinating shards.testDlsFiltersHybridKnnQueryfails on this branch withHybrid query did not apply the DLS filter to every subquery. I re-ran it against an unmodifiedmainbuild and got a byte-identical failure reason, so it is pre-existing and depends on the neural/k-NN filter traversal in #6428, not on this refactor.Interaction with #6428 (verified, not assumed)
I merged #6428 into this branch on a scratch branch and ran the full suite, rather than just eyeballing the overlap.
The merge produced exactly two conflicts, both trivial:
DlsFlsValveImpl— Support neural queries with hybrid DLS #6428 rewrites the javadoc ofisHybridQueryDlsFilterSupported, a method this PR deletes. Resolved by taking the deletion.DlsFilterLevelActionHandlerTest— Support neural queries with hybrid DLS #6428 adds stub helpers adjacent toassertDlsMarkerPreventsReentry, whose signature this PR changed. Resolved by keeping both.DlsFilterLevelActionHandleritself — #6428's substantive +120 lines — auto-merged with no conflict, because its neural/k-NN filter discovery does not care how the marker is stored.Stacked results:
DlsFilterLevelActionHandlerTest).HybridQueryDlsIT: 5 of 6 pass, includingtestDlsFiltersHybridKnnQuery. The remaining failure istestDlsFiltersHybridNeuralQuery, which fails during ML model registration withDirect self-reference leading to cycle ... org.opensearch.security.user.User— that is [BUG] User principal creates a Jackson self-reference in ML Commons #6433, unrelated to DLS.That 5-of-6 result matches what #1957 reports for #6428 on its own, so this refactor neither improves nor regresses #6428's coverage. Whichever of the two merges first, the second rebases with a two-line resolution.
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.