Keep hybrid DLS safe across mixed versions

[sharp-pixel]

Summary

• Replace the hybrid DLS sentinel value in the filter-level completion header with a dedicated header.
  - Continue accepting the legacy sentinel from unpatched nodes during rolling upgrades.
• Remove the cluster-wide version gate, allowing safe backports and mixed-version rollout behavior.
• Preserve reader-level DLS and strip both marker formats before dispatch to non-local clusters.

Security review

No security finding under the mixed patched-node and unpatched-node cluster assumption.

• Untrusted REST callers cannot forge either internal header: security-prefixed headers are rejected at ingress.
• Unpatched coordinator to patched data node: the patched node recognizes the legacy sentinel as hybrid-query-only completion, does not treat it as full filter-level completion, and retains reader-level DLS.
• Patched coordinator to unpatched data node: the unpatched node ignores the unfamiliar dedicated marker and performs normal DLS processing. This can repeat DLS work but is fail-closed.
• Patched nodes preserve both marker formats during local fan-out. If an unpatched intermediate node drops the unfamiliar dedicated marker, downstream nodes fall back to normal DLS rather than bypassing it.
• Cross-cluster dispatch strips both marker formats on patched nodes. Unpatched nodes already strip the legacy sentinel and do not copy the unfamiliar dedicated header.
• Only the exact legacy sentinel is reclassified; the ordinary filter-level completion value continues to suppress reader-level DLS as before.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 81a742a)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Legacy sentinel not treated as completion The mixed-version compatibility story in the PR description states that patched data nodes should recognize the legacy sentinel from unpatched coordinators as hybrid-query completion (avoiding duplicate filter-level DLS work while retaining reader-level DLS). However, the re-entry guard here only checks OPENDISTRO_SECURITY_FILTER_LEVEL_DLS_DONE and the new OPENDISTRO_SECURITY_DLS_QUERY_FILTER_APPLIED headers - the legacy hybrid sentinel value (previously OPENDISTRO_SECURITY_HYBRID_QUERY_DLS_DONE) constant has been removed entirely. If an unpatched coordinator sends the legacy hybrid sentinel value in the FILTER_LEVEL_DLS_DONE header, a patched node will still short-circuit via the first check (because the header key is present), which does prevent re-entry but also means the code path that ‘recognizes legacy sentinel as hybrid-only completion’ described in the PR summary is not actually implemented — reader-level DLS state depends on isDlsQueryFilterApplied(), which now only inspects the new header. Verify this is intentional; otherwise reader-level DLS may be incorrectly retained/skipped for legacy-sentinel-bearing requests from unpatched coordinators. if (threadContext.getHeader(ConfigConstants.OPENDISTRO_SECURITY_FILTER_LEVEL_DLS_DONE) != null || threadContext.getHeader(ConfigConstants.OPENDISTRO_SECURITY_DLS_QUERY_FILTER_APPLIED) != null) { return true; } CCS stripping regression risk The previous code removed FILTER_LEVEL_DLS_DONE when it carried the hybrid sentinel value before dispatch outside the local cluster. The new code only removes DLS_QUERY_FILTER_APPLIED for outside-cluster dispatch, and relies on a separate CCS branch below to remove FILTER_LEVEL_DLS_DONE. But that separate branch is gated by isCrossClusterSearchEnabled() && !action.startsWith("internal:") && !action.equals(ClusterSearchShardsAction.NAME) && isDestinationOutsideLocalCluster. If CCS is not enabled but the destination is still an unrecognized node (e.g., during cluster topology changes), a stale FILTER_LEVEL_DLS_DONE=true header set locally could now leak to an unrecognized destination where previously the hybrid-sentinel variant would have been stripped. Confirm this leakage path is unreachable in practice or extend the strip to cover FILTER_LEVEL_DLS_DONE for all outside-cluster destinations. if (isDestinationOutsideLocalCluster) { // The top-level query filter is only valid within the coordinating cluster. Strip its marker from every // action sent to an unrecognized destination, even if RemoteClusterService does not report CCS as enabled. headerMap.remove(ConfigConstants.OPENDISTRO_SECURITY_DLS_QUERY_FILTER_APPLIED); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 81a742a

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Restore min-node-version guard for hybrid DLS Removing the hybridQueryDlsFilterSupported version check means a coordinating node running the new code will apply the hybrid-query DLS filter and send the OPENDISTRO_SECURITY_DLS_QUERY_FILTER_APPLIED header to peers in a mixed cluster; older nodes will not recognize this header and may reapply DLS or fail to honor filtering, breaking rolling upgrades. Reintroduce a min-node-version guard (or otherwise restrict the new marker to clusters where every node understands it) so the behavior only activates once all nodes support it. src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java [452-462] static boolean shouldApplyDlsFilterToHybridQueryInAdaptiveMode( ActionRequest request, boolean hasDlsRestrictions, boolean containsTermLookupQuery, + boolean hybridQueryDlsFilterSupported, boolean localOnlyRequest ) { return hasDlsRestrictions && !containsTermLookupQuery + && hybridQueryDlsFilterSupported && localOnlyRequest && isTopLevelHybridQueryWithoutParentChildClauses(request); } Suggestion importance[1-10]: 7 __ Why: Legitimate concern regarding mixed-cluster rolling upgrade compatibility, as removing the version check may cause issues if older nodes don't understand the new header. However, the PR author may have intentional reasons for this removal.	Medium
	Guard header stripping with initialization check The condition was tightened from isDestinationOutsideLocalCluster && marker present to only isDestinationOutsideLocalCluster, so the removal now runs for every outbound request to any unrecognized destination. Also, clusterInfoHolder.isInitialized() is no longer checked, meaning when the holder is uninitialized isDestinationOutsideLocalCluster is false and the header will NOT be stripped for genuinely remote destinations. Re-add the initialization check and preferably keep this as a no-op when the header is absent to preserve the original safety guarantee for CCS during startup. src/main/java/org/opensearch/security/transport/SecurityInterceptor.java [226-230] -if (isDestinationOutsideLocalCluster) { +if (clusterInfoHolder.isInitialized() && isDestinationOutsideLocalCluster) { // The top-level query filter is only valid within the coordinating cluster. Strip its marker from every // action sent to an unrecognized destination, even if RemoteClusterService does not report CCS as enabled. headerMap.remove(ConfigConstants.OPENDISTRO_SECURITY_DLS_QUERY_FILTER_APPLIED); } Suggestion importance[1-10]: 6 __ Why: Valid concern: isDestinationOutsideLocalCluster already requires clusterInfoHolder.isInitialized() in its definition, so the header won't be stripped when uninitialized. The suggestion is technically redundant but improves clarity. Moderate impact.	Low
General	Audit all consumers of both markers Using two distinct headers means downstream code paths that historically checked only OPENDISTRO_SECURITY_FILTER_LEVEL_DLS_DONE will no longer see completion when the hybrid path fires. Verify every consumer of OPENDISTRO_SECURITY_FILTER_LEVEL_DLS_DONE (including transport-level stripping in CCS and any external plugin integrations) also inspects the new OPENDISTRO_SECURITY_DLS_QUERY_FILTER_APPLIED marker, otherwise DLS may be re-executed or skipped incorrectly. src/main/java/org/opensearch/security/configuration/DlsFilterLevelActionHandler.java [183-188] +threadContext.putHeader( + applyDlsFilterToHybridQuery + ? ConfigConstants.OPENDISTRO_SECURITY_DLS_QUERY_FILTER_APPLIED + : ConfigConstants.OPENDISTRO_SECURITY_FILTER_LEVEL_DLS_DONE, + "true" +); - Suggestion importance[1-10]: 3 __ Why: The suggestion only asks to verify consumers of the headers without providing a concrete code change; existing_code equals improved_code. Low actionable impact.	Low

---

Previous suggestions

Suggestions up to commit beec2a1

Category	Suggestion	Impact
General	Prevent hybrid marker leak when cluster info uninitialized The guard isDestinationOutsideLocalCluster requires clusterInfoHolder.isInitialized(), but the previous behavior additionally checked the header value before removing anything. The new code unconditionally removes OPENDISTRO_SECURITY_DLS_QUERY_FILTER_APPLIED for any outside-cluster destination even when it was not set by this node's hybrid path — this is fine, but note that when clusterInfoHolder is not initialized, isDestinationOutsideLocalCluster is false and the marker will leak to remote nodes. Consider treating an uninitialized cluster info holder as "unknown/remote" for stripping the hybrid markers to avoid leaking the coordinator-only marker across clusters. src/main/java/org/opensearch/security/transport/SecurityInterceptor.java [226-235] -if (isDestinationOutsideLocalCluster) { - // The top-level query filter is only valid within the coordinating cluster. Strip its marker from every - // action sent to an unrecognized destination, even if RemoteClusterService does not report CCS as enabled. +boolean destinationLikelyOutsideLocalCluster = !clusterInfoHolder.isInitialized() + || !clusterInfoHolder.hasNode(connection.getNode()); +if (destinationLikelyOutsideLocalCluster) { headerMap.remove(ConfigConstants.OPENDISTRO_SECURITY_DLS_QUERY_FILTER_APPLIED); if (ConfigConstants.OPENDISTRO_SECURITY_HYBRID_QUERY_DLS_DONE.equals( headerMap.get(ConfigConstants.OPENDISTRO_SECURITY_FILTER_LEVEL_DLS_DONE) )) { headerMap.remove(ConfigConstants.OPENDISTRO_SECURITY_FILTER_LEVEL_DLS_DONE); } } Suggestion importance[1-10]: 5 __ Why: The suggestion raises a legitimate concern about the marker potentially leaking when clusterInfoHolder is not initialized, since isDestinationOutsideLocalCluster requires initialization. However, this reflects existing design behavior across the codebase, and changing the semantics may have broader implications.	Low
	Keep filter-level and hybrid markers disjoint isDlsDoneOnFilterLevel() only inspects the legacy FILTER_LEVEL_DLS_DONE header and ignores the new OPENDISTRO_SECURITY_DLS_QUERY_FILTER_APPLIED marker. When a patched node applies the hybrid filter (setting only the new header), isDlsDoneOnFilterLevel() returns false — which is correct — but if some code path sets FILTER_LEVEL_DLS_DONE=true alongside the new hybrid marker, isDlsDoneOnFilterLevel() would incorrectly return true. Consider explicitly excluding the case where OPENDISTRO_SECURITY_DLS_QUERY_FILTER_APPLIED is set to keep the two states cleanly disjoint. src/main/java/org/opensearch/security/privileges/dlsfls/DlsFlsBaseContext.java [66-69] public boolean isDlsDoneOnFilterLevel() { String filterLevelDlsDone = threadContext.getHeader(ConfigConstants.OPENDISTRO_SECURITY_FILTER_LEVEL_DLS_DONE); - return filterLevelDlsDone != null && !ConfigConstants.OPENDISTRO_SECURITY_HYBRID_QUERY_DLS_DONE.equals(filterLevelDlsDone); + if (filterLevelDlsDone == null || ConfigConstants.OPENDISTRO_SECURITY_HYBRID_QUERY_DLS_DONE.equals(filterLevelDlsDone)) { + return false; + } + return threadContext.getHeader(ConfigConstants.OPENDISTRO_SECURITY_DLS_QUERY_FILTER_APPLIED) == null; } Suggestion importance[1-10]: 4 __ Why: The suggestion addresses a theoretical edge case where both headers could be set simultaneously. It's a defensive improvement but the scenario is unlikely given the PR's design where these markers are mutually exclusive by the writing logic.	Low

Suggestions up to commit c237a53

Category	Suggestion	Impact
Possible issue	Handle legacy hybrid marker from older nodes A mixed-version coordinator running an older node may send the legacy value OPENDISTRO_SECURITY_HYBRID_QUERY_DLS_DONE on the FILTER_LEVEL_DLS_DONE header. The new code treats any non-null value on FILTER_LEVEL_DLS_DONE as "filter-level DLS done" (via isDlsDoneOnFilterLevel), which will incorrectly suppress the hybrid-query filter path. Consider preserving legacy compatibility by detecting the legacy hybrid marker string and mapping it to the new DLS_QUERY_FILTER_APPLIED semantics during rolling upgrades. src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java [236-240] -if (threadContext.getHeader(ConfigConstants.OPENDISTRO_SECURITY_FILTER_LEVEL_DLS_DONE) != null - || threadContext.getHeader(ConfigConstants.OPENDISTRO_SECURITY_DLS_QUERY_FILTER_APPLIED) != null) { +String filterLevelDone = threadContext.getHeader(ConfigConstants.OPENDISTRO_SECURITY_FILTER_LEVEL_DLS_DONE); +boolean legacyHybridMarker = "opendistro_security_hybrid_query".equals(filterLevelDone); +if ((filterLevelDone != null && !legacyHybridMarker) + || threadContext.getHeader(ConfigConstants.OPENDISTRO_SECURITY_DLS_QUERY_FILTER_APPLIED) != null + || legacyHybridMarker) { if (log.isDebugEnabled()) { log.debug("DLS query handling is already done for this request"); } Suggestion importance[1-10]: 5 __ Why: Raises a potentially valid concern about backward compatibility during rolling upgrades where older nodes may still send the legacy OPENDISTRO_SECURITY_HYBRID_QUERY_DLS_DONE value. However, the impact depends on whether mixed-version support with the previous release is required, which is not evident from the diff.	Low
General	Strip DLS marker also when cluster info uninitialized The guard isDestinationOutsideLocalCluster depends on clusterInfoHolder.isInitialized() being true; if the holder is not initialized, isDestinationOutsideLocalCluster is false and the marker will leak to remote destinations. Preserve the previous safety by also stripping the marker when the destination cannot be verified as local, or explicitly handle the uninitialized case to avoid regressing CCS isolation. src/main/java/org/opensearch/security/transport/SecurityInterceptor.java [226-230] -if (isDestinationOutsideLocalCluster) { +if (isDestinationOutsideLocalCluster + || (clusterInfoHolder.isInitialized() ? false : !clusterInfoHolder.hasNode(connection.getNode()))) { // The top-level query filter is only valid within the coordinating cluster. Strip its marker from every // action sent to an unrecognized destination, even if RemoteClusterService does not report CCS as enabled. headerMap.remove(ConfigConstants.OPENDISTRO_SECURITY_DLS_QUERY_FILTER_APPLIED); } Suggestion importance[1-10]: 4 __ Why: The concern about clusterInfoHolder.isInitialized() returning false is valid, but the proposed improved_code is convoluted and logically equivalent to the original in the uninitialized case (it still evaluates to false). The suggestion identifies a potential issue but doesn't provide a correct fix.	Low

[github-actions]

Persistent review updated to latest commit beec2a1

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.79%. Comparing base (b8c667f) to head (81a742a).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6451      +/-   ##
==========================================
+ Coverage   75.66%   75.79%   +0.13%     
==========================================
  Files         456      457       +1     
  Lines       30414    30508      +94     
  Branches     4605     4615      +10     
==========================================
+ Hits        23012    23123     +111     
+ Misses       5281     5267      -14     
+ Partials     2121     2118       -3     

Files with missing lines	Coverage Δ
...ity/configuration/DlsFilterLevelActionHandler.java	72.72% <100.00%> (+0.25%)	⬆️
...security/configuration/DlsFlsFilterLeafReader.java	64.39% <100.00%> (-0.14%)	⬇️
...search/security/configuration/DlsFlsValveImpl.java	72.75% <100.00%> (-0.15%)	⬇️
.../security/privileges/dlsfls/DlsFlsBaseContext.java	100.00% <100.00%> (ø)
...g/opensearch/security/support/ConfigConstants.java	96.55% <ø> (ø)
...search/security/transport/SecurityInterceptor.java	80.85% <100.00%> (+0.42%)	⬆️

... and 55 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[cwperks]

Continue accepting the legacy sentinel from unpatched nodes during rolling upgrades.

We don't need to do this if this code hasn't been released yet

[sharp-pixel]

Continue accepting the legacy sentinel from unpatched nodes during rolling upgrades.

We don't need to do this if this code hasn't been released yet

I think we do to support mixed mode because the coordinator can either be an unpatched or a patched node.

Here is the analysis of GPT5.6 Daybreak Blue:
In a mixed patched/unpatched cluster, one direction is unsafe:

Coordinator	Data node	Result
Patched	Unpatched	Fail-closed. The old node ignores the new dedicated header and performs normal DLS again. It may duplicate filtering or fail the query, but should not expose documents.
Unpatched	Patched	Potential fail-open. The old coordinator sends the legacy hybrid sentinel in FILTER_LEVEL_DLS_DONE. The patched data node treats any value in that header as “DLS fully complete,” disabling reader-level DLS. Hits remain filtered, but global aggregations and other reader-level paths could expose unauthorized data.

[cwperks]

@sharp-pixel that would matter if your last PR (#6416) was in a released version, but its only in mainline and unreleased. Given that, bwc checks like this block:

String filterLevelDlsDone = threadContext.getHeader(ConfigConstants.OPENDISTRO_SECURITY_FILTER_LEVEL_DLS_DONE);
return filterLevelDlsDone != null && !ConfigConstants.OPENDISTRO_SECURITY_HYBRID_QUERY_DLS_DONE.equals(filterLevelDlsDone);

Can be simplified to:

return threadContext.getHeader(ConfigConstants.OPENDISTRO_SECURITY_FILTER_LEVEL_DLS_DONE) != null;

since we don't actually check the value in that header, only that its not-null (or having the value of "true").

[github-actions]

Persistent review updated to latest commit 81a742a