From 0672dad588315f6b096609b20b300d4712376283 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 10 Mar 2026 21:58:17 -0400 Subject: [PATCH 1/4] Skip logic in doCache if index request cache is disabled Signed-off-by: Craig Perkins --- .../org/opensearch/security/OpenSearchSecurityPlugin.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java index db738ff536..bc58846324 100644 --- a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java +++ b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java @@ -112,6 +112,7 @@ import org.opensearch.identity.Subject; import org.opensearch.index.IndexModule; import org.opensearch.index.cache.query.QueryCache; +import org.opensearch.indices.IndicesRequestCache; import org.opensearch.indices.IndicesService; import org.opensearch.indices.SystemIndexDescriptor; import org.opensearch.plugins.ClusterPlugin; @@ -812,6 +813,9 @@ public void clear(String reason) { @Override public Weight doCache(Weight weight, QueryCachingPolicy policy) { + if (!indexSettings.getValue(IndicesRequestCache.INDEX_CACHE_REQUEST_ENABLED_SETTING)) { + return weight; + } try { if (dlsFlsValve.hasFlsOrFieldMasking(index().getName())) { // Do not cache From f845f1c9bfad6ecfe34a1c9a5f9a23df7e52289b Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 10 Mar 2026 22:02:57 -0400 Subject: [PATCH 2/4] Add to CHANGELOG Signed-off-by: Craig Perkins --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a39eb7669b..60001a38af 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), ### Bug Fixes - Fix the issue of unprocessed X-Request-Id ([#5954](https://github.com/opensearch-project/security/pull/5954)) +- Skip logic in doCache if index request cache is disabled ([#6001](https://github.com/opensearch-project/security/pull/6001)) + ### Refactoring ### Maintenance From df3b963d43517f2c4852de442df9caf4f5924f69 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 12 Mar 2026 15:34:35 -0400 Subject: [PATCH 3/4] Ensure that doCache is called once per request Signed-off-by: Craig Perkins --- .../security/OpenSearchSecurityPlugin.java | 14 +++++++++----- .../security/configuration/DlsFlsValveImpl.java | 4 ++++ 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java index bc58846324..35d4efa2e5 100644 --- a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java +++ b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java @@ -112,7 +112,6 @@ import org.opensearch.identity.Subject; import org.opensearch.index.IndexModule; import org.opensearch.index.cache.query.QueryCache; -import org.opensearch.indices.IndicesRequestCache; import org.opensearch.indices.IndicesService; import org.opensearch.indices.SystemIndexDescriptor; import org.opensearch.plugins.ClusterPlugin; @@ -795,6 +794,9 @@ public void onIndexModule(IndexModule indexModule) { } indexModule.forceQueryCacheProvider((indexSettings, nodeCache) -> new QueryCache() { + // Transient key used to cache the hasFlsOrFieldMasking result for the duration of a single request, + // avoiding repeated alias traversal for every sub-clause of a BooleanQuery. + private final String cacheKey = "_opendistro_security_flsfm_" + indexSettings.getIndex().getName(); @Override public Index index() { @@ -813,11 +815,13 @@ public void clear(String reason) { @Override public Weight doCache(Weight weight, QueryCachingPolicy policy) { - if (!indexSettings.getValue(IndicesRequestCache.INDEX_CACHE_REQUEST_ENABLED_SETTING)) { - return weight; - } try { - if (dlsFlsValve.hasFlsOrFieldMasking(index().getName())) { + Boolean cached = threadPool.getThreadContext().getTransient(cacheKey); + if (cached == null) { + cached = dlsFlsValve.hasFlsOrFieldMasking(index().getName()); + threadPool.getThreadContext().putTransient(cacheKey, cached); + } + if (cached) { // Do not cache return weight; } else { diff --git a/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java b/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java index 122ef92c86..e8afa3e775 100644 --- a/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java +++ b/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java @@ -16,6 +16,7 @@ import java.util.List; import java.util.Objects; import java.util.Set; +import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; import java.util.stream.StreamSupport; @@ -537,8 +538,11 @@ public DlsFlsProcessedConfig getCurrentConfig() { return dlsFlsProcessedConfig.get(); } + public static final AtomicLong HAS_FLS_OR_FIELD_MASKING_CALL_COUNT = new AtomicLong(); + @Override public boolean hasFlsOrFieldMasking(String index) throws PrivilegesEvaluationException { + HAS_FLS_OR_FIELD_MASKING_CALL_COUNT.incrementAndGet(); PrivilegesEvaluationContext privilegesEvaluationContext = this.dlsFlsBaseContext.getPrivilegesEvaluationContext(); if (privilegesEvaluationContext == null) { return false; From 0e8791a757f2bded993b20673e81682355d9b739 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 12 Mar 2026 16:14:55 -0400 Subject: [PATCH 4/4] Use LogsRules for assertion Signed-off-by: Craig Perkins --- .../dlsfls/DoCacheMemoizationTest.java | 107 ++++++++++++++++++ .../resources/log4j2-test.properties | 5 + .../security/OpenSearchSecurityPlugin.java | 3 + .../configuration/DlsFlsValveImpl.java | 4 - 4 files changed, 115 insertions(+), 4 deletions(-) create mode 100644 src/integrationTest/java/org/opensearch/security/dlsfls/DoCacheMemoizationTest.java diff --git a/src/integrationTest/java/org/opensearch/security/dlsfls/DoCacheMemoizationTest.java b/src/integrationTest/java/org/opensearch/security/dlsfls/DoCacheMemoizationTest.java new file mode 100644 index 0000000000..89aebdf945 --- /dev/null +++ b/src/integrationTest/java/org/opensearch/security/dlsfls/DoCacheMemoizationTest.java @@ -0,0 +1,107 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + * + * Modifications Copyright OpenSearch Contributors. See + * GitHub history for details. + */ +package org.opensearch.security.dlsfls; + +import java.util.Map; +import java.util.stream.IntStream; + +import org.junit.BeforeClass; +import org.junit.ClassRule; +import org.junit.Rule; +import org.junit.Test; + +import org.opensearch.action.index.IndexRequest; +import org.opensearch.action.search.SearchRequest; +import org.opensearch.action.search.SearchResponse; +import org.opensearch.client.RestHighLevelClient; +import org.opensearch.index.query.QueryBuilders; +import org.opensearch.search.builder.SearchSourceBuilder; +import org.opensearch.test.framework.TestSecurityConfig; +import org.opensearch.test.framework.cluster.ClusterManager; +import org.opensearch.test.framework.cluster.LocalCluster; +import org.opensearch.test.framework.log.LogCapturingAppender; +import org.opensearch.test.framework.log.LogsRule; +import org.opensearch.transport.client.Client; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.lessThan; +import static org.opensearch.action.support.WriteRequest.RefreshPolicy.IMMEDIATE; +import static org.opensearch.client.RequestOptions.DEFAULT; +import static org.opensearch.test.framework.TestSecurityConfig.AuthcDomain.AUTHC_HTTPBASIC_INTERNAL; + +/** + * Regression test verifying that hasFlsOrFieldMasking() is memoized per request in doCache(). + * + * Without the fix, doCache() calls hasFlsOrFieldMasking() once per BooleanQuery filter clause + * per shard. With the fix, the result is cached in the ThreadContext for the duration of the + * request so only 1 call is made per shard per request, and subsequent calls hit the memoized path. + */ +public class DoCacheMemoizationTest { + + static final String INDEX_NAME = "test-index"; + static final int NUM_FILTER_CLAUSES = 50; + + // Field masking causes hasFlsOrFieldMasking() to return true, exercising the memoization path. + static final TestSecurityConfig.User MASKED_USER = new TestSecurityConfig.User("masked_user").roles( + new TestSecurityConfig.Role("masked_role").clusterPermissions("cluster_composite_ops_ro") + .indexPermissions("read") + .maskedFields("field_a") + .on(INDEX_NAME) + ); + + @ClassRule + public static final LocalCluster cluster = new LocalCluster.Builder().clusterManager(ClusterManager.SINGLENODE) + .anonymousAuth(false) + .authc(AUTHC_HTTPBASIC_INTERNAL) + .users(MASKED_USER) + .build(); + + @Rule + public LogsRule logsRule = new LogsRule("org.opensearch.security.OpenSearchSecurityPlugin"); + + @BeforeClass + public static void setupIndex() { + try (Client client = cluster.getInternalNodeClient()) { + client.index(new IndexRequest(INDEX_NAME).setRefreshPolicy(IMMEDIATE).source(Map.of("field_a", "value", "field_b", "other"))) + .actionGet(); + } + } + + /** + * filter clauses → scoreMode=COMPLETE_NO_SCORES → doCache() called per clause per shard. + * should/must clauses would set needsScores=true and bypass doCache() entirely. + */ + private SearchRequest buildFilterQuery() { + var bool = QueryBuilders.boolQuery(); + IntStream.range(0, NUM_FILTER_CLAUSES).forEach(i -> bool.filter(QueryBuilders.termQuery("field_b", "other_" + i))); + return new SearchRequest(INDEX_NAME).source(new SearchSourceBuilder().query(bool).size(10)); + } + + @Test + public void doCacheUsedMemoizedValueForSubsequentClauses() throws Exception { + try (RestHighLevelClient client = cluster.getRestHighLevelClient(MASKED_USER)) { + SearchResponse response = client.search(buildFilterQuery(), DEFAULT); + assertThat("search must succeed", response.getFailedShards(), lessThan(1)); + } + + // First clause evaluates fresh and populates the ThreadContext transient + logsRule.assertThatContain("doCache: evaluated hasFlsOrFieldMasking(test-index)=true"); + // Subsequent clauses (49 of them) hit the memoized value instead of re-evaluating + logsRule.assertThatContain("doCache: memoized hasFlsOrFieldMasking(test-index)=true"); + // Exactly 1 fresh evaluation and NUM_FILTER_CLAUSES-1 memoized hits + long evaluatedCount = LogCapturingAppender.getLogMessagesAsString() + .stream() + .filter(m -> m.contains("doCache: evaluated hasFlsOrFieldMasking")) + .count(); + assertThat("expected exactly 1 fresh evaluation per request", evaluatedCount, equalTo(1L)); + } +} diff --git a/src/integrationTest/resources/log4j2-test.properties b/src/integrationTest/resources/log4j2-test.properties index d0bb23fa3f..a9208792bd 100644 --- a/src/integrationTest/resources/log4j2-test.properties +++ b/src/integrationTest/resources/log4j2-test.properties @@ -53,3 +53,8 @@ logger.securenetty4transport.name = org.opensearch.transport.netty4.ssl.SecureNe logger.securenetty4transport.level = error logger.securenetty4transport.appenderRef.capturing.ref = logCapturingAppender +# Logger required by test org.opensearch.security.dlsfls.DoCacheMemoizationTest +logger.opensearchsecurityplugin.name = org.opensearch.security.OpenSearchSecurityPlugin +logger.opensearchsecurityplugin.level = debug +logger.opensearchsecurityplugin.appenderRef.capturing.ref = logCapturingAppender + diff --git a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java index 35d4efa2e5..1c57c66870 100644 --- a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java +++ b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java @@ -820,6 +820,9 @@ public Weight doCache(Weight weight, QueryCachingPolicy policy) { if (cached == null) { cached = dlsFlsValve.hasFlsOrFieldMasking(index().getName()); threadPool.getThreadContext().putTransient(cacheKey, cached); + log.debug("doCache: evaluated hasFlsOrFieldMasking({})={}", index().getName(), cached); + } else { + log.debug("doCache: memoized hasFlsOrFieldMasking({})={}", index().getName(), cached); } if (cached) { // Do not cache diff --git a/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java b/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java index e8afa3e775..122ef92c86 100644 --- a/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java +++ b/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java @@ -16,7 +16,6 @@ import java.util.List; import java.util.Objects; import java.util.Set; -import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; import java.util.stream.StreamSupport; @@ -538,11 +537,8 @@ public DlsFlsProcessedConfig getCurrentConfig() { return dlsFlsProcessedConfig.get(); } - public static final AtomicLong HAS_FLS_OR_FIELD_MASKING_CALL_COUNT = new AtomicLong(); - @Override public boolean hasFlsOrFieldMasking(String index) throws PrivilegesEvaluationException { - HAS_FLS_OR_FIELD_MASKING_CALL_COUNT.incrementAndGet(); PrivilegesEvaluationContext privilegesEvaluationContext = this.dlsFlsBaseContext.getPrivilegesEvaluationContext(); if (privilegesEvaluationContext == null) { return false;