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 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 db738ff536..1c57c66870 100644 --- a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java +++ b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java @@ -794,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,7 +816,15 @@ public void clear(String reason) { @Override public Weight doCache(Weight weight, QueryCachingPolicy policy) { 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); + log.debug("doCache: evaluated hasFlsOrFieldMasking({})={}", index().getName(), cached); + } else { + log.debug("doCache: memoized hasFlsOrFieldMasking({})={}", index().getName(), cached); + } + if (cached) { // Do not cache return weight; } else {