From 84613d7e6d04c97aebea7a843b9396e48102386d Mon Sep 17 00:00:00 2001 From: Gagan Juneja Date: Tue, 10 Oct 2023 16:25:26 +0530 Subject: [PATCH 1/6] Fix TraceableTcpTransportChannel classcast exception Signed-off-by: Gagan Juneja --- .../security/ssl/transport/SecuritySSLRequestHandler.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/opensearch/security/ssl/transport/SecuritySSLRequestHandler.java b/src/main/java/org/opensearch/security/ssl/transport/SecuritySSLRequestHandler.java index c67579e30f..4f71ce29e7 100644 --- a/src/main/java/org/opensearch/security/ssl/transport/SecuritySSLRequestHandler.java +++ b/src/main/java/org/opensearch/security/ssl/transport/SecuritySSLRequestHandler.java @@ -37,9 +37,9 @@ import org.opensearch.security.support.ConfigConstants; import org.opensearch.tasks.Task; import org.opensearch.threadpool.ThreadPool; +import org.opensearch.transport.BaseTcpTransportChannel; import org.opensearch.transport.TaskTransportChannel; import org.opensearch.transport.TcpChannel; -import org.opensearch.transport.TcpTransportChannel; import org.opensearch.transport.TransportChannel; import org.opensearch.transport.TransportRequest; import org.opensearch.transport.TransportRequestHandler; @@ -113,9 +113,9 @@ public final void messageReceived(T request, TransportChannel channel, Task task if (channel instanceof TaskTransportChannel) { final TransportChannel inner = ((TaskTransportChannel) channel).getChannel(); - nettyChannel = (Netty4TcpChannel) ((TcpTransportChannel) inner).getChannel(); - } else if (channel instanceof TcpTransportChannel) { - final TcpChannel inner = ((TcpTransportChannel) channel).getChannel(); + nettyChannel = (Netty4TcpChannel) ((BaseTcpTransportChannel) inner).getChannel(); + } else if (channel instanceof BaseTcpTransportChannel) { + final TcpChannel inner = ((BaseTcpTransportChannel) channel).getChannel(); nettyChannel = (Netty4TcpChannel) inner; } else { throw new Exception("Invalid channel of type " + channel.getClass() + " (" + channel.getChannelType() + ")"); From 49eda17a4480aa3eca07eafac77a5db67fe1cee5 Mon Sep 17 00:00:00 2001 From: Andriy Redko Date: Thu, 12 Oct 2023 08:29:43 -0400 Subject: [PATCH 2/6] Refactor SSL handler retrieval to use HttpChannel / TranportChannel APIs instead of typecasting Signed-off-by: Andriy Redko --- .../security/filter/OpenSearchRequest.java | 15 ++------------ .../transport/SecuritySSLRequestHandler.java | 20 +------------------ 2 files changed, 3 insertions(+), 32 deletions(-) diff --git a/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java b/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java index 85c70b8f7a..26f736e798 100644 --- a/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java +++ b/src/main/java/org/opensearch/security/filter/OpenSearchRequest.java @@ -18,7 +18,6 @@ import javax.net.ssl.SSLEngine; -import org.opensearch.http.netty4.Netty4HttpChannel; import org.opensearch.rest.RestRequest; import org.opensearch.rest.RestRequest.Method; @@ -42,21 +41,11 @@ public Map> getHeaders() { @Override public SSLEngine getSSLEngine() { - if (underlyingRequest == null - || underlyingRequest.getHttpChannel() == null - || !(underlyingRequest.getHttpChannel() instanceof Netty4HttpChannel)) { + if (underlyingRequest == null || underlyingRequest.getHttpChannel() == null) { return null; } - // We look for Ssl_handler called `ssl_http` in the outbound pipeline of Netty channel first, and if its not - // present we look for it in inbound channel. If its present in neither we return null, else we return the sslHandler. - final Netty4HttpChannel httpChannel = (Netty4HttpChannel) underlyingRequest.getHttpChannel(); - SslHandler sslhandler = (SslHandler) httpChannel.getNettyChannel().pipeline().get("ssl_http"); - if (sslhandler == null && httpChannel.inboundPipeline() != null) { - sslhandler = (SslHandler) httpChannel.inboundPipeline().get("ssl_http"); - } - - return sslhandler != null ? sslhandler.engine() : null; + return underlyingRequest.getHttpChannel().get("ssl_http", SslHandler.class).map(SslHandler::engine).orElse(null); } @Override diff --git a/src/main/java/org/opensearch/security/ssl/transport/SecuritySSLRequestHandler.java b/src/main/java/org/opensearch/security/ssl/transport/SecuritySSLRequestHandler.java index 4f71ce29e7..d47ca84a39 100644 --- a/src/main/java/org/opensearch/security/ssl/transport/SecuritySSLRequestHandler.java +++ b/src/main/java/org/opensearch/security/ssl/transport/SecuritySSLRequestHandler.java @@ -37,13 +37,9 @@ import org.opensearch.security.support.ConfigConstants; import org.opensearch.tasks.Task; import org.opensearch.threadpool.ThreadPool; -import org.opensearch.transport.BaseTcpTransportChannel; -import org.opensearch.transport.TaskTransportChannel; -import org.opensearch.transport.TcpChannel; import org.opensearch.transport.TransportChannel; import org.opensearch.transport.TransportRequest; import org.opensearch.transport.TransportRequestHandler; -import org.opensearch.transport.netty4.Netty4TcpChannel; public class SecuritySSLRequestHandler implements TransportRequestHandler { @@ -108,21 +104,7 @@ public final void messageReceived(T request, TransportChannel channel, Task task } try { - - Netty4TcpChannel nettyChannel = null; - - if (channel instanceof TaskTransportChannel) { - final TransportChannel inner = ((TaskTransportChannel) channel).getChannel(); - nettyChannel = (Netty4TcpChannel) ((BaseTcpTransportChannel) inner).getChannel(); - } else if (channel instanceof BaseTcpTransportChannel) { - final TcpChannel inner = ((BaseTcpTransportChannel) channel).getChannel(); - nettyChannel = (Netty4TcpChannel) inner; - } else { - throw new Exception("Invalid channel of type " + channel.getClass() + " (" + channel.getChannelType() + ")"); - } - - final SslHandler sslhandler = (SslHandler) nettyChannel.getNettyChannel().pipeline().get("ssl_server"); - + final SslHandler sslhandler = channel.get("ssl_server", SslHandler.class).orElse(null); if (sslhandler == null) { if (SSLConfig.isDualModeEnabled()) { log.info("Communication in dual mode. Skipping SSL handler check"); From e16fe56550d48223e9766b3599825fa201dd65e8 Mon Sep 17 00:00:00 2001 From: Gagan Juneja Date: Thu, 19 Oct 2023 16:59:00 +0530 Subject: [PATCH 3/6] Address review comment Signed-off-by: Gagan Juneja --- .../java/org/opensearch/security/SearchOperationTest.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/integrationTest/java/org/opensearch/security/SearchOperationTest.java b/src/integrationTest/java/org/opensearch/security/SearchOperationTest.java index a38d26800a..4c44a84e49 100644 --- a/src/integrationTest/java/org/opensearch/security/SearchOperationTest.java +++ b/src/integrationTest/java/org/opensearch/security/SearchOperationTest.java @@ -86,6 +86,7 @@ import org.opensearch.cluster.metadata.IndexMetadata; import org.opensearch.cluster.metadata.IndexTemplateMetadata; import org.opensearch.common.settings.Settings; +import org.opensearch.common.util.FeatureFlags; import org.opensearch.index.query.BoolQueryBuilder; import org.opensearch.index.query.MatchQueryBuilder; import org.opensearch.index.query.QueryBuilders; @@ -94,6 +95,7 @@ import org.opensearch.repositories.RepositoryMissingException; import org.opensearch.core.rest.RestStatus; import org.opensearch.search.builder.SearchSourceBuilder; +import org.opensearch.telemetry.TelemetrySettings; import org.opensearch.test.framework.AuditCompliance; import org.opensearch.test.framework.AuditConfiguration; import org.opensearch.test.framework.AuditFilters; @@ -371,6 +373,9 @@ public class SearchOperationTest { USER_ALLOWED_TO_PERFORM_INDEX_OPERATIONS_ON_SELECTED_INDICES, USER_ALLOWED_TO_CREATE_INDEX ) + .nodeSettings( + Map.of(FeatureFlags.TELEMETRY_SETTING.getKey(), true, TelemetrySettings.TRACER_FEATURE_ENABLED_SETTING.getKey(), true) + ) .audit( new AuditConfiguration(true).compliance(new AuditCompliance().enabled(true)) .filters(new AuditFilters().enabledRest(true).enabledTransport(true)) From db40c642c345e53dcd341e503dc7093c39d89edc Mon Sep 17 00:00:00 2001 From: Gagan Juneja Date: Thu, 19 Oct 2023 17:03:53 +0530 Subject: [PATCH 4/6] Address review comment Signed-off-by: Gagan Juneja --- .../org/opensearch/security/SearchOperationTest.java | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/integrationTest/java/org/opensearch/security/SearchOperationTest.java b/src/integrationTest/java/org/opensearch/security/SearchOperationTest.java index 4c44a84e49..6cc47a56e4 100644 --- a/src/integrationTest/java/org/opensearch/security/SearchOperationTest.java +++ b/src/integrationTest/java/org/opensearch/security/SearchOperationTest.java @@ -374,7 +374,14 @@ public class SearchOperationTest { USER_ALLOWED_TO_CREATE_INDEX ) .nodeSettings( - Map.of(FeatureFlags.TELEMETRY_SETTING.getKey(), true, TelemetrySettings.TRACER_FEATURE_ENABLED_SETTING.getKey(), true) + Map.of( + FeatureFlags.TELEMETRY_SETTING.getKey(), + true, + TelemetrySettings.TRACER_FEATURE_ENABLED_SETTING.getKey(), + true, + TelemetrySettings.METRICS_FEATURE_ENABLED_SETTING, + true + ) ) .audit( new AuditConfiguration(true).compliance(new AuditCompliance().enabled(true)) From 28426889044c51bf09bd357e998c1e8e6f52c2f5 Mon Sep 17 00:00:00 2001 From: Gagan Juneja Date: Thu, 19 Oct 2023 17:07:51 +0530 Subject: [PATCH 5/6] Address review comment Signed-off-by: Gagan Juneja --- .../java/org/opensearch/security/SearchOperationTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/integrationTest/java/org/opensearch/security/SearchOperationTest.java b/src/integrationTest/java/org/opensearch/security/SearchOperationTest.java index 6cc47a56e4..692ac57a2a 100644 --- a/src/integrationTest/java/org/opensearch/security/SearchOperationTest.java +++ b/src/integrationTest/java/org/opensearch/security/SearchOperationTest.java @@ -379,7 +379,7 @@ public class SearchOperationTest { true, TelemetrySettings.TRACER_FEATURE_ENABLED_SETTING.getKey(), true, - TelemetrySettings.METRICS_FEATURE_ENABLED_SETTING, + TelemetrySettings.METRICS_FEATURE_ENABLED_SETTING.getKey(), true ) ) From f7cae910f1a2cb70d8798023cf31e784f1ec11b6 Mon Sep 17 00:00:00 2001 From: Peter Nied Date: Fri, 20 Oct 2023 16:23:14 +0000 Subject: [PATCH 6/6] Create new test class for validation of tracing features Signed-off-by: Peter Nied --- .../security/SearchOperationTest.java | 12 --- .../org/opensearch/security/TracingTests.java | 79 +++++++++++++++++++ 2 files changed, 79 insertions(+), 12 deletions(-) create mode 100644 src/integrationTest/java/org/opensearch/security/TracingTests.java diff --git a/src/integrationTest/java/org/opensearch/security/SearchOperationTest.java b/src/integrationTest/java/org/opensearch/security/SearchOperationTest.java index 692ac57a2a..a38d26800a 100644 --- a/src/integrationTest/java/org/opensearch/security/SearchOperationTest.java +++ b/src/integrationTest/java/org/opensearch/security/SearchOperationTest.java @@ -86,7 +86,6 @@ import org.opensearch.cluster.metadata.IndexMetadata; import org.opensearch.cluster.metadata.IndexTemplateMetadata; import org.opensearch.common.settings.Settings; -import org.opensearch.common.util.FeatureFlags; import org.opensearch.index.query.BoolQueryBuilder; import org.opensearch.index.query.MatchQueryBuilder; import org.opensearch.index.query.QueryBuilders; @@ -95,7 +94,6 @@ import org.opensearch.repositories.RepositoryMissingException; import org.opensearch.core.rest.RestStatus; import org.opensearch.search.builder.SearchSourceBuilder; -import org.opensearch.telemetry.TelemetrySettings; import org.opensearch.test.framework.AuditCompliance; import org.opensearch.test.framework.AuditConfiguration; import org.opensearch.test.framework.AuditFilters; @@ -373,16 +371,6 @@ public class SearchOperationTest { USER_ALLOWED_TO_PERFORM_INDEX_OPERATIONS_ON_SELECTED_INDICES, USER_ALLOWED_TO_CREATE_INDEX ) - .nodeSettings( - Map.of( - FeatureFlags.TELEMETRY_SETTING.getKey(), - true, - TelemetrySettings.TRACER_FEATURE_ENABLED_SETTING.getKey(), - true, - TelemetrySettings.METRICS_FEATURE_ENABLED_SETTING.getKey(), - true - ) - ) .audit( new AuditConfiguration(true).compliance(new AuditCompliance().enabled(true)) .filters(new AuditFilters().enabledRest(true).enabledTransport(true)) diff --git a/src/integrationTest/java/org/opensearch/security/TracingTests.java b/src/integrationTest/java/org/opensearch/security/TracingTests.java new file mode 100644 index 0000000000..b26433c333 --- /dev/null +++ b/src/integrationTest/java/org/opensearch/security/TracingTests.java @@ -0,0 +1,79 @@ +/* +* Copyright OpenSearch Contributors +* 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. +* +*/ +package org.opensearch.security; + +import java.io.IOException; +import java.util.Map; + +import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope; +import org.junit.ClassRule; +import org.junit.Rule; +import org.junit.Test; +import org.junit.runner.RunWith; + +import org.opensearch.action.search.SearchRequest; +import org.opensearch.action.search.SearchResponse; +import org.opensearch.client.Client; +import org.opensearch.client.RestHighLevelClient; +import org.opensearch.common.util.FeatureFlags; +import org.opensearch.telemetry.TelemetrySettings; +import org.opensearch.test.framework.AuditCompliance; +import org.opensearch.test.framework.AuditConfiguration; +import org.opensearch.test.framework.AuditFilters; +import org.opensearch.test.framework.TestSecurityConfig.User; +import org.opensearch.test.framework.audit.AuditLogsRule; +import org.opensearch.test.framework.cluster.ClusterManager; +import org.opensearch.test.framework.cluster.LocalCluster; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.equalTo; +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; +import static org.opensearch.test.framework.TestSecurityConfig.Role.ALL_ACCESS; + +@RunWith(com.carrotsearch.randomizedtesting.RandomizedRunner.class) +@ThreadLeakScope(ThreadLeakScope.Scope.NONE) +public class TracingTests { + + private static final User ADMIN_USER = new User("admin").roles(ALL_ACCESS); + + @ClassRule + public static final LocalCluster cluster = new LocalCluster.Builder().clusterManager(ClusterManager.THREE_CLUSTER_MANAGERS) + .authc(AUTHC_HTTPBASIC_INTERNAL) + .users(ADMIN_USER) + .nodeSettings( + Map.of( + FeatureFlags.TELEMETRY_SETTING.getKey(), + true, + TelemetrySettings.TRACER_FEATURE_ENABLED_SETTING.getKey(), + true, + TelemetrySettings.METRICS_FEATURE_ENABLED_SETTING.getKey(), + true + ) + ) + .build(); + + @Rule + public AuditLogsRule auditLogsRule = new AuditLogsRule(); + + @Test + public void indexDocumentAndSearch() throws IOException { + try (Client internalClient = cluster.getInternalNodeClient()) { + // Create a document to search + internalClient.prepareIndex("index-1").setRefreshPolicy(IMMEDIATE).setSource(Map.of("foo", "bar")).get(); + } + + try (final RestHighLevelClient restClient = cluster.getRestHighLevelClient(ADMIN_USER)) { + final SearchResponse response = restClient.search(new SearchRequest(), DEFAULT); + assertThat(response.getHits().getTotalHits().value, equalTo(1L)); + } + } +}