From af2b21a38f14ae6f309ceba36af55df5841b712c Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Mon, 17 Apr 2023 11:40:33 -0400 Subject: [PATCH 1/5] Fix NPE and add additional graceful error handling Signed-off-by: Craig Perkins --- .../security/OpenSearchSecurityPlugin.java | 13 +- .../GuardedSearchOperationWrapper.java | 84 ++++++++++ .../GuardedSearchOperationWrapperTest.java | 146 ++++++++++++++++++ 3 files changed, 238 insertions(+), 5 deletions(-) create mode 100644 src/main/java/org/opensearch/security/support/GuardedSearchOperationWrapper.java create mode 100644 src/test/java/org/opensearch/security/support/GuardedSearchOperationWrapperTest.java diff --git a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java index ffd7f730ed..4161e57f43 100644 --- a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java +++ b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java @@ -45,6 +45,7 @@ import java.util.Map; import java.util.Objects; import java.util.Set; +import java.util.concurrent.atomic.AtomicReference; import java.util.function.BiFunction; import java.util.function.Function; import java.util.function.Predicate; @@ -165,6 +166,7 @@ import org.opensearch.security.ssl.transport.SecuritySSLNettyTransport; import org.opensearch.security.ssl.util.SSLConfigConstants; import org.opensearch.security.support.ConfigConstants; +import org.opensearch.security.support.GuardedSearchOperationWrapper; import org.opensearch.security.support.HeaderHelper; import org.opensearch.security.support.ModuleInfo; import org.opensearch.security.support.ReflectionHelper; @@ -215,7 +217,7 @@ public final class OpenSearchSecurityPlugin extends OpenSearchSecuritySSLPlugin private final List demoCertHashes = new ArrayList(3); private volatile SecurityFilter sf; private volatile IndexResolverReplacer irr; - private volatile NamedXContentRegistry namedXContentRegistry = null; + private volatile AtomicReference namedXContentRegistry = new AtomicReference<>(NamedXContentRegistry.EMPTY);; private volatile DlsFlsRequestValve dlsFlsValve = null; private volatile Salt salt; private volatile OpensearchDynamicSetting transportPassiveAuthSetting; @@ -569,11 +571,11 @@ public Weight doCache(Weight weight, QueryCachingPolicy policy) { } }); - indexModule.addSearchOperationListener(new SearchOperationListener() { + indexModule.addSearchOperationListener(new GuardedSearchOperationWrapper() { @Override public void onPreQueryPhase(SearchContext context) { - dlsFlsValve.handleSearchContext(context, threadPool, namedXContentRegistry); + dlsFlsValve.handleSearchContext(context, threadPool, namedXContentRegistry.get()); } @Override @@ -643,7 +645,7 @@ public void onQueryPhase(SearchContext searchContext, long tookInNanos) { } } } - }); + }.toListener()); } } @@ -798,6 +800,7 @@ public Collection createComponents(Client localClient, ClusterService cl final PrivilegesInterceptor privilegesInterceptor; + namedXContentRegistry.set(xContentRegistry); if (SSLConfig.isSslOnlyMode()) { dlsFlsValve = new DlsFlsRequestValve.NoopDlsFlsRequestValve(); auditLog = new NullAuditLog(); @@ -822,7 +825,7 @@ public Collection createComponents(Client localClient, ClusterService cl // DLS-FLS is enabled if not client and not disabled and not SSL only. final boolean dlsFlsEnabled = !SSLConfig.isSslOnlyMode(); evaluator = new PrivilegesEvaluator(clusterService, threadPool, cr, resolver, auditLog, - settings, privilegesInterceptor, cih, irr, dlsFlsEnabled, namedXContentRegistry); + settings, privilegesInterceptor, cih, irr, dlsFlsEnabled, namedXContentRegistry.get()); sf = new SecurityFilter(settings, evaluator, adminDns, dlsFlsValve, auditLog, threadPool, cs, compatConfig, irr, xffResolver); diff --git a/src/main/java/org/opensearch/security/support/GuardedSearchOperationWrapper.java b/src/main/java/org/opensearch/security/support/GuardedSearchOperationWrapper.java new file mode 100644 index 0000000000..1ea219d4d4 --- /dev/null +++ b/src/main/java/org/opensearch/security/support/GuardedSearchOperationWrapper.java @@ -0,0 +1,84 @@ +/* + * 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.support; + +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.opensearch.index.shard.SearchOperationListener; +import org.opensearch.search.internal.ReaderContext; +import org.opensearch.search.internal.SearchContext; +import org.opensearch.transport.TransportRequest; + +/** + * Guarded version of Search Operation Listener to ensure critical request paths succeed + */ +public interface GuardedSearchOperationWrapper { + + static final Logger log = LogManager.getLogger(GuardedSearchOperationWrapper.class); + + void onPreQueryPhase(final SearchContext context); + + void onNewReaderContext(final ReaderContext readerContext); + + void onNewScrollContext(final ReaderContext readerContext); + + void validateReaderContext(final ReaderContext readerContext, final TransportRequest transportRequest); + + void onQueryPhase(final SearchContext searchContext, final long tookInNanos); + + default SearchOperationListener toListener() { + return new InnerSearchOperationListener(this); + } + + static class InnerSearchOperationListener implements SearchOperationListener { + + private GuardedSearchOperationWrapper that; + InnerSearchOperationListener(GuardedSearchOperationWrapper that) { + this.that = that; + } + + @Override + public void onPreQueryPhase(final SearchContext searchContext) { + try { + that.onPreQueryPhase(searchContext); + } catch (final Exception e) { + searchContext.setTask(null); + log.error("Cancelled request due to internal error", e); + } + } + + @Override + public void onNewReaderContext(final ReaderContext readerContext) { + that.onNewReaderContext(readerContext); + } + + @Override + public void onNewScrollContext(final ReaderContext readerContext) { + that.onNewScrollContext(readerContext); + } + + @Override + public void validateReaderContext(final ReaderContext readerContext, final TransportRequest transportRequest) { + that.validateReaderContext(readerContext, transportRequest); + } + + @Override + public void onQueryPhase(final SearchContext searchContext, final long tookInNanos) { + try { + that.onQueryPhase(searchContext, tookInNanos); + } catch (final Exception e) { + searchContext.setTask(null); + log.error("Cancelled request due to internal error", e); + } + } + } +} \ No newline at end of file diff --git a/src/test/java/org/opensearch/security/support/GuardedSearchOperationWrapperTest.java b/src/test/java/org/opensearch/security/support/GuardedSearchOperationWrapperTest.java new file mode 100644 index 0000000000..1fa0cd31e4 --- /dev/null +++ b/src/test/java/org/opensearch/security/support/GuardedSearchOperationWrapperTest.java @@ -0,0 +1,146 @@ +/* + * 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.support; + +import static org.junit.Assert.assertThrows; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.notNullValue; + +import org.junit.Test; +import java.util.concurrent.atomic.AtomicReference; + +import org.opensearch.index.shard.SearchOperationListener; +import org.opensearch.search.internal.ReaderContext; +import org.opensearch.search.internal.SearchContext; +import org.opensearch.transport.TransportRequest; + + +public class GuardedSearchOperationWrapperTest { + + @Test + public void onNewReaderContextCanThrowException() { + final String expectedExceptionText = "abcd1234"; + + DefaultingGuardedSearchOperationWrapper testWrapper = new DefaultingGuardedSearchOperationWrapper() { + @Override + public void onNewReaderContext(ReaderContext readerContext) { + throw new RuntimeException(expectedExceptionText); + } + }; + + final RuntimeException expectedException = assertThrows(RuntimeException.class, testWrapper::exerciseAllMethods); + + assertThat(expectedException.getMessage(), equalTo(expectedExceptionText)); + } + + @Test + public void onNewScrollContextCanThrowException() { + final String expectedExceptionText = "qwerty978"; + + DefaultingGuardedSearchOperationWrapper testWrapper = new DefaultingGuardedSearchOperationWrapper() { + @Override + public void onNewScrollContext(ReaderContext readerContext) { + throw new RuntimeException(expectedExceptionText); + } + }; + + final RuntimeException expectedException = assertThrows(RuntimeException.class, testWrapper::exerciseAllMethods); + + assertThat(expectedException.getMessage(), equalTo(expectedExceptionText)); + } + + @Test + public void validateReaderContextCanThrowException() { + final String expectedExceptionText = "validationException"; + + DefaultingGuardedSearchOperationWrapper testWrapper = new DefaultingGuardedSearchOperationWrapper() { + @Override + public void validateReaderContext(ReaderContext readerContext, TransportRequest transportRequest) { + throw new RuntimeException(expectedExceptionText); + } + }; + + final RuntimeException expectedException = assertThrows(RuntimeException.class, testWrapper::exerciseAllMethods); + + assertThat(expectedException.getMessage(), equalTo(expectedExceptionText)); + } + + @Test + public void onPreQueryPhaseCannotThrow() { + AtomicReference calledSearchContext = new AtomicReference<>(); + DefaultingGuardedSearchOperationWrapper testWrapper = new DefaultingGuardedSearchOperationWrapper() { + @Override + public void onPreQueryPhase(SearchContext context) { + calledSearchContext.set(context); + throw new RuntimeException("EXCEPTIONAL!"); + } + }; + + testWrapper.exerciseAllMethods(); + + assertThat(calledSearchContext.get(), notNullValue()); + verify(calledSearchContext.get()).setTask(null); + } + + @Test + public void onQueryPhaseCannotThrow() { + AtomicReference calledSearchContext = new AtomicReference<>(); + DefaultingGuardedSearchOperationWrapper testWrapper = new DefaultingGuardedSearchOperationWrapper() { + @Override + public void onQueryPhase(SearchContext context, long tookInNanos) { + calledSearchContext.set(context); + throw new RuntimeException("EXCEPTIONAL!"); + } + }; + + testWrapper.exerciseAllMethods(); + + assertThat(calledSearchContext.get(), notNullValue()); + verify(calledSearchContext.get()).setTask(null); + } + + /** Only use to make testing easier */ + private static class DefaultingGuardedSearchOperationWrapper implements GuardedSearchOperationWrapper { + + @Override + public void onNewReaderContext(ReaderContext readerContext) { + } + + @Override + public void onNewScrollContext(ReaderContext readerContext) { + } + + @Override + public void onPreQueryPhase(SearchContext context) { + } + + @Override + public void onQueryPhase(SearchContext searchContext, long tookInNanos) { + } + + @Override + public void validateReaderContext(ReaderContext readerContext, TransportRequest transportRequest) { + } + + void exerciseAllMethods(){ + final SearchOperationListener sol = this.toListener(); + sol.onNewReaderContext(mock(ReaderContext.class)); + sol.onNewScrollContext(mock(ReaderContext.class)); + sol.onPreQueryPhase(mock(SearchContext.class)); + sol.onQueryPhase(mock(SearchContext.class), 12345L); + sol.validateReaderContext(mock(ReaderContext.class), mock(TransportRequest.class)); + } + } +} \ No newline at end of file From 9a31a0113c0cc064c249aae228f41ccbc3b9c613 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Mon, 17 Apr 2023 11:43:39 -0400 Subject: [PATCH 2/5] Add new lines at end of file Signed-off-by: Craig Perkins --- .../security/support/GuardedSearchOperationWrapper.java | 2 +- .../security/support/GuardedSearchOperationWrapperTest.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/opensearch/security/support/GuardedSearchOperationWrapper.java b/src/main/java/org/opensearch/security/support/GuardedSearchOperationWrapper.java index 1ea219d4d4..95e48be072 100644 --- a/src/main/java/org/opensearch/security/support/GuardedSearchOperationWrapper.java +++ b/src/main/java/org/opensearch/security/support/GuardedSearchOperationWrapper.java @@ -81,4 +81,4 @@ public void onQueryPhase(final SearchContext searchContext, final long tookInNan } } } -} \ No newline at end of file +} diff --git a/src/test/java/org/opensearch/security/support/GuardedSearchOperationWrapperTest.java b/src/test/java/org/opensearch/security/support/GuardedSearchOperationWrapperTest.java index 1fa0cd31e4..ea1373e038 100644 --- a/src/test/java/org/opensearch/security/support/GuardedSearchOperationWrapperTest.java +++ b/src/test/java/org/opensearch/security/support/GuardedSearchOperationWrapperTest.java @@ -143,4 +143,4 @@ void exerciseAllMethods(){ sol.validateReaderContext(mock(ReaderContext.class), mock(TransportRequest.class)); } } -} \ No newline at end of file +} From eb98d35bb3a4dd9e1e1bab0dfaba1eb705abc3d9 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Mon, 17 Apr 2023 11:48:45 -0400 Subject: [PATCH 3/5] Run spotlessApply Signed-off-by: Craig Perkins --- .../support/GuardedSearchOperationWrapper.java | 1 + .../GuardedSearchOperationWrapperTest.java | 16 ++++++++-------- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/main/java/org/opensearch/security/support/GuardedSearchOperationWrapper.java b/src/main/java/org/opensearch/security/support/GuardedSearchOperationWrapper.java index 95e48be072..76d316de2d 100644 --- a/src/main/java/org/opensearch/security/support/GuardedSearchOperationWrapper.java +++ b/src/main/java/org/opensearch/security/support/GuardedSearchOperationWrapper.java @@ -13,6 +13,7 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; + import org.opensearch.index.shard.SearchOperationListener; import org.opensearch.search.internal.ReaderContext; import org.opensearch.search.internal.SearchContext; diff --git a/src/test/java/org/opensearch/security/support/GuardedSearchOperationWrapperTest.java b/src/test/java/org/opensearch/security/support/GuardedSearchOperationWrapperTest.java index ea1373e038..982d1108ad 100644 --- a/src/test/java/org/opensearch/security/support/GuardedSearchOperationWrapperTest.java +++ b/src/test/java/org/opensearch/security/support/GuardedSearchOperationWrapperTest.java @@ -10,22 +10,22 @@ */ package org.opensearch.security.support; -import static org.junit.Assert.assertThrows; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.verify; - -import static org.hamcrest.MatcherAssert.assertThat; -import static org.hamcrest.Matchers.equalTo; -import static org.hamcrest.Matchers.notNullValue; +import java.util.concurrent.atomic.AtomicReference; import org.junit.Test; -import java.util.concurrent.atomic.AtomicReference; import org.opensearch.index.shard.SearchOperationListener; import org.opensearch.search.internal.ReaderContext; import org.opensearch.search.internal.SearchContext; import org.opensearch.transport.TransportRequest; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.notNullValue; +import static org.junit.Assert.assertThrows; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; + public class GuardedSearchOperationWrapperTest { From d392035be96b2ae1e0b3a397f356561a35b94488 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Mon, 17 Apr 2023 12:03:52 -0400 Subject: [PATCH 4/5] Remove unused import Signed-off-by: Craig Perkins --- .../java/org/opensearch/security/OpenSearchSecurityPlugin.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java index 4161e57f43..da107f32c8 100644 --- a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java +++ b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java @@ -99,7 +99,6 @@ import org.opensearch.index.Index; import org.opensearch.index.IndexModule; import org.opensearch.index.cache.query.QueryCache; -import org.opensearch.index.shard.SearchOperationListener; import org.opensearch.indices.IndicesService; import org.opensearch.indices.SystemIndexDescriptor; import org.opensearch.indices.breaker.CircuitBreakerService; From 8f4905a5d9c845b38fa858afd3ddf29fa300f8bf Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Mon, 17 Apr 2023 12:42:27 -0400 Subject: [PATCH 5/5] volatile to final Signed-off-by: Craig Perkins --- .../java/org/opensearch/security/OpenSearchSecurityPlugin.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java index da107f32c8..362a46843e 100644 --- a/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java +++ b/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java @@ -216,7 +216,7 @@ public final class OpenSearchSecurityPlugin extends OpenSearchSecuritySSLPlugin private final List demoCertHashes = new ArrayList(3); private volatile SecurityFilter sf; private volatile IndexResolverReplacer irr; - private volatile AtomicReference namedXContentRegistry = new AtomicReference<>(NamedXContentRegistry.EMPTY);; + private final AtomicReference namedXContentRegistry = new AtomicReference<>(NamedXContentRegistry.EMPTY);; private volatile DlsFlsRequestValve dlsFlsValve = null; private volatile Salt salt; private volatile OpensearchDynamicSetting transportPassiveAuthSetting;