From 0b51bb92db0d1eed324c1cc8f8e6ac3468f833b4 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 14 Jan 2025 19:44:42 -0500 Subject: [PATCH 1/8] Cleanup SystemIndexTest with RunAsSubjectClient Signed-off-by: Craig Perkins --- .../systemindex/SystemIndexTests.java | 2 +- .../IndexDocumentIntoSystemIndexAction.java | 5 +- .../IndexDocumentIntoSystemIndexResponse.java | 48 ------------ .../sampleplugin/PluginContextSwitcher.java | 35 --------- ...dexDocumentIntoMixOfSystemIndexAction.java | 27 +++---- ...ulkIndexDocumentIntoSystemIndexAction.java | 31 ++++---- .../RestRunClusterHealthAction.java | 4 +- .../sampleplugin/RunAsSubjectClient.java | 62 ++++++++++++++++ .../sampleplugin/RunClusterHealthAction.java | 5 +- .../RunClusterHealthResponse.java | 42 ----------- .../sampleplugin/SystemIndexPlugin1.java | 16 ++-- ...ortIndexDocumentIntoSystemIndexAction.java | 73 +++++++------------ .../TransportRunClusterHealthAction.java | 52 ++++--------- 13 files changed, 142 insertions(+), 260 deletions(-) delete mode 100644 src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/IndexDocumentIntoSystemIndexResponse.java delete mode 100644 src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/PluginContextSwitcher.java create mode 100644 src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RunAsSubjectClient.java delete mode 100644 src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RunClusterHealthResponse.java diff --git a/src/integrationTest/java/org/opensearch/security/systemindex/SystemIndexTests.java b/src/integrationTest/java/org/opensearch/security/systemindex/SystemIndexTests.java index 3435e6dbdc..13325b3029 100644 --- a/src/integrationTest/java/org/opensearch/security/systemindex/SystemIndexTests.java +++ b/src/integrationTest/java/org/opensearch/security/systemindex/SystemIndexTests.java @@ -100,7 +100,7 @@ public void testPluginShouldBeAbleToIndexDocumentIntoItsSystemIndex() { HttpResponse response = client.put("try-create-and-index/" + SYSTEM_INDEX_1); assertThat(response.getStatusCode(), equalTo(RestStatus.OK.getStatus())); - assertThat(response.getBody(), containsString(SystemIndexPlugin1.class.getCanonicalName())); + assertThat(response.getBody(), containsString("{\"acknowledged\":true}")); } } diff --git a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/IndexDocumentIntoSystemIndexAction.java b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/IndexDocumentIntoSystemIndexAction.java index a78c57e1d1..b272f464c1 100644 --- a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/IndexDocumentIntoSystemIndexAction.java +++ b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/IndexDocumentIntoSystemIndexAction.java @@ -11,12 +11,13 @@ package org.opensearch.security.systemindex.sampleplugin; import org.opensearch.action.ActionType; +import org.opensearch.action.support.master.AcknowledgedResponse; -public class IndexDocumentIntoSystemIndexAction extends ActionType { +public class IndexDocumentIntoSystemIndexAction extends ActionType { public static final IndexDocumentIntoSystemIndexAction INSTANCE = new IndexDocumentIntoSystemIndexAction(); public static final String NAME = "cluster:mock/systemindex/index"; private IndexDocumentIntoSystemIndexAction() { - super(NAME, IndexDocumentIntoSystemIndexResponse::new); + super(NAME, AcknowledgedResponse::new); } } diff --git a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/IndexDocumentIntoSystemIndexResponse.java b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/IndexDocumentIntoSystemIndexResponse.java deleted file mode 100644 index e154742fe2..0000000000 --- a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/IndexDocumentIntoSystemIndexResponse.java +++ /dev/null @@ -1,48 +0,0 @@ -/* - * 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.systemindex.sampleplugin; - -// CS-SUPPRESS-SINGLE: RegexpSingleline It is not possible to use phrase "cluster manager" instead of master here -import java.io.IOException; - -import org.opensearch.action.support.master.AcknowledgedResponse; -import org.opensearch.core.common.io.stream.StreamInput; -import org.opensearch.core.common.io.stream.StreamOutput; -import org.opensearch.core.xcontent.ToXContent; -import org.opensearch.core.xcontent.ToXContentObject; -import org.opensearch.core.xcontent.XContentBuilder; -// CS-ENFORCE-SINGLE - -public class IndexDocumentIntoSystemIndexResponse extends AcknowledgedResponse implements ToXContentObject { - - private String plugin; - - public IndexDocumentIntoSystemIndexResponse(boolean status, String plugin) { - super(status); - this.plugin = plugin; - } - - public IndexDocumentIntoSystemIndexResponse(StreamInput in) throws IOException { - super(in); - } - - @Override - public void writeTo(StreamOutput out) throws IOException { - super.writeTo(out); - out.writeString(plugin); - } - - @Override - public void addCustomFields(XContentBuilder builder, ToXContent.Params params) throws IOException { - super.addCustomFields(builder, params); - builder.field("plugin", plugin); - } -} diff --git a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/PluginContextSwitcher.java b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/PluginContextSwitcher.java deleted file mode 100644 index 4ef420efdb..0000000000 --- a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/PluginContextSwitcher.java +++ /dev/null @@ -1,35 +0,0 @@ -/* - * 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.systemindex.sampleplugin; - -import java.util.Objects; -import java.util.concurrent.Callable; - -import org.opensearch.identity.PluginSubject; - -public class PluginContextSwitcher { - private PluginSubject pluginSubject; - - public PluginContextSwitcher() {} - - public void initialize(PluginSubject pluginSubject) { - Objects.requireNonNull(pluginSubject); - this.pluginSubject = pluginSubject; - } - - public T runAs(Callable callable) { - try { - return pluginSubject.runAs(callable); - } catch (Exception e) { - throw new RuntimeException(e); - } - } -} diff --git a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RestBulkIndexDocumentIntoMixOfSystemIndexAction.java b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RestBulkIndexDocumentIntoMixOfSystemIndexAction.java index e9b569a263..7a9f930c2a 100644 --- a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RestBulkIndexDocumentIntoMixOfSystemIndexAction.java +++ b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RestBulkIndexDocumentIntoMixOfSystemIndexAction.java @@ -34,11 +34,11 @@ public class RestBulkIndexDocumentIntoMixOfSystemIndexAction extends BaseRestHandler { private final Client client; - private final PluginContextSwitcher contextSwitcher; + private final RunAsSubjectClient pluginClient; - public RestBulkIndexDocumentIntoMixOfSystemIndexAction(Client client, PluginContextSwitcher contextSwitcher) { + public RestBulkIndexDocumentIntoMixOfSystemIndexAction(Client client, RunAsSubjectClient pluginClient) { this.client = client; - this.contextSwitcher = contextSwitcher; + this.pluginClient = pluginClient; } @Override @@ -57,19 +57,14 @@ public RestChannelConsumer prepareRequest(RestRequest request, NodeClient client @Override public void accept(RestChannel channel) throws Exception { - contextSwitcher.runAs(() -> { - BulkRequestBuilder builder = client.prepareBulk(); - builder.add(new IndexRequest(SYSTEM_INDEX_1).source("content", 1)); - builder.add(new IndexRequest(SYSTEM_INDEX_2).source("content", 1)); - builder.setRefreshPolicy(WriteRequest.RefreshPolicy.IMMEDIATE); - BulkRequest bulkRequest = builder.request(); - client.bulk(bulkRequest, ActionListener.wrap(r -> { - channel.sendResponse( - new BytesRestResponse(RestStatus.OK, r.toXContent(channel.newBuilder(), ToXContent.EMPTY_PARAMS)) - ); - }, fr -> { channel.sendResponse(new BytesRestResponse(RestStatus.FORBIDDEN, String.valueOf(fr))); })); - return null; - }); + BulkRequestBuilder builder = client.prepareBulk(); + builder.add(new IndexRequest(SYSTEM_INDEX_1).source("content", 1)); + builder.add(new IndexRequest(SYSTEM_INDEX_2).source("content", 1)); + builder.setRefreshPolicy(WriteRequest.RefreshPolicy.IMMEDIATE); + BulkRequest bulkRequest = builder.request(); + pluginClient.bulk(bulkRequest, ActionListener.wrap(r -> { + channel.sendResponse(new BytesRestResponse(RestStatus.OK, r.toXContent(channel.newBuilder(), ToXContent.EMPTY_PARAMS))); + }, fr -> { channel.sendResponse(new BytesRestResponse(RestStatus.FORBIDDEN, String.valueOf(fr))); })); } }; } diff --git a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RestBulkIndexDocumentIntoSystemIndexAction.java b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RestBulkIndexDocumentIntoSystemIndexAction.java index 8b37e54164..a6d729aca4 100644 --- a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RestBulkIndexDocumentIntoSystemIndexAction.java +++ b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RestBulkIndexDocumentIntoSystemIndexAction.java @@ -34,11 +34,11 @@ public class RestBulkIndexDocumentIntoSystemIndexAction extends BaseRestHandler { private final Client client; - private final PluginContextSwitcher contextSwitcher; + private final RunAsSubjectClient pluginClient; - public RestBulkIndexDocumentIntoSystemIndexAction(Client client, PluginContextSwitcher contextSwitcher) { + public RestBulkIndexDocumentIntoSystemIndexAction(Client client, RunAsSubjectClient pluginClient) { this.client = client; - this.contextSwitcher = contextSwitcher; + this.pluginClient = pluginClient; } @Override @@ -58,21 +58,18 @@ public RestChannelConsumer prepareRequest(RestRequest request, NodeClient client @Override public void accept(RestChannel channel) throws Exception { - contextSwitcher.runAs(() -> { - client.admin().indices().create(new CreateIndexRequest(indexName), ActionListener.wrap(r -> { - BulkRequestBuilder builder = client.prepareBulk(); - builder.add(new IndexRequest(indexName).source("{\"content\":1}", XContentType.JSON)); - builder.add(new IndexRequest(indexName).source("{\"content\":2}", XContentType.JSON)); - builder.setRefreshPolicy(WriteRequest.RefreshPolicy.IMMEDIATE); - BulkRequest bulkRequest = builder.request(); - client.bulk(bulkRequest, ActionListener.wrap(r2 -> { - channel.sendResponse( - new BytesRestResponse(RestStatus.OK, r.toXContent(channel.newBuilder(), ToXContent.EMPTY_PARAMS)) - ); - }, fr -> { channel.sendResponse(new BytesRestResponse(RestStatus.FORBIDDEN, String.valueOf(fr))); })); + pluginClient.admin().indices().create(new CreateIndexRequest(indexName), ActionListener.wrap(r -> { + BulkRequestBuilder builder = client.prepareBulk(); + builder.add(new IndexRequest(indexName).source("{\"content\":1}", XContentType.JSON)); + builder.add(new IndexRequest(indexName).source("{\"content\":2}", XContentType.JSON)); + builder.setRefreshPolicy(WriteRequest.RefreshPolicy.IMMEDIATE); + BulkRequest bulkRequest = builder.request(); + pluginClient.bulk(bulkRequest, ActionListener.wrap(r2 -> { + channel.sendResponse( + new BytesRestResponse(RestStatus.OK, r.toXContent(channel.newBuilder(), ToXContent.EMPTY_PARAMS)) + ); }, fr -> { channel.sendResponse(new BytesRestResponse(RestStatus.FORBIDDEN, String.valueOf(fr))); })); - return null; - }); + }, fr -> { channel.sendResponse(new BytesRestResponse(RestStatus.FORBIDDEN, String.valueOf(fr))); })); } }; } diff --git a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RestRunClusterHealthAction.java b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RestRunClusterHealthAction.java index f4674b4377..232f462072 100644 --- a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RestRunClusterHealthAction.java +++ b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RestRunClusterHealthAction.java @@ -24,11 +24,9 @@ public class RestRunClusterHealthAction extends BaseRestHandler { private final Client client; - private final PluginContextSwitcher contextSwitcher; - public RestRunClusterHealthAction(Client client, PluginContextSwitcher contextSwitcher) { + public RestRunClusterHealthAction(Client client) { this.client = client; - this.contextSwitcher = contextSwitcher; } @Override diff --git a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RunAsSubjectClient.java b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RunAsSubjectClient.java new file mode 100644 index 0000000000..39540841cb --- /dev/null +++ b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RunAsSubjectClient.java @@ -0,0 +1,62 @@ +/* + * 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.systemindex.sampleplugin; + +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; + +import org.opensearch.action.ActionRequest; +import org.opensearch.action.ActionType; +import org.opensearch.client.Client; +import org.opensearch.client.FilterClient; +import org.opensearch.common.util.concurrent.ThreadContext; +import org.opensearch.core.action.ActionListener; +import org.opensearch.core.action.ActionResponse; +import org.opensearch.identity.Subject; + +/** + * Implementation of client that will run transport actions in a stashed context and inject the name of the provided + * subject into the context. + */ +public class RunAsSubjectClient extends FilterClient { + + private static final Logger logger = LogManager.getLogger(RunAsSubjectClient.class); + + private Subject subject; + + public RunAsSubjectClient(Client delegate) { + super(delegate); + } + + public RunAsSubjectClient(Client delegate, Subject subject) { + super(delegate); + this.subject = subject; + } + + public void setSubject(Subject subject) { + this.subject = subject; + } + + @Override + protected void doExecute( + ActionType action, + Request request, + ActionListener listener + ) { + try (ThreadContext.StoredContext ctx = threadPool().getThreadContext().newStoredContext(false)) { + subject.runAs(() -> { + logger.info("Running transport action with subject: {}", subject.getPrincipal().getName()); + super.doExecute(action, request, ActionListener.runBefore(listener, ctx::restore)); + return null; + }); + } catch (Exception e) { + throw new RuntimeException(e); + } + } +} diff --git a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RunClusterHealthAction.java b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RunClusterHealthAction.java index dca6b8d2b7..7d028a7ddf 100644 --- a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RunClusterHealthAction.java +++ b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RunClusterHealthAction.java @@ -11,12 +11,13 @@ package org.opensearch.security.systemindex.sampleplugin; import org.opensearch.action.ActionType; +import org.opensearch.action.support.master.AcknowledgedResponse; -public class RunClusterHealthAction extends ActionType { +public class RunClusterHealthAction extends ActionType { public static final RunClusterHealthAction INSTANCE = new RunClusterHealthAction(); public static final String NAME = "cluster:mock/monitor/health"; private RunClusterHealthAction() { - super(NAME, RunClusterHealthResponse::new); + super(NAME, AcknowledgedResponse::new); } } diff --git a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RunClusterHealthResponse.java b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RunClusterHealthResponse.java deleted file mode 100644 index a500755e22..0000000000 --- a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RunClusterHealthResponse.java +++ /dev/null @@ -1,42 +0,0 @@ -/* - * 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.systemindex.sampleplugin; - -// CS-SUPPRESS-SINGLE: RegexpSingleline It is not possible to use phrase "cluster manager" instead of master here -import java.io.IOException; - -import org.opensearch.action.support.master.AcknowledgedResponse; -import org.opensearch.core.common.io.stream.StreamInput; -import org.opensearch.core.common.io.stream.StreamOutput; -import org.opensearch.core.xcontent.ToXContentObject; -import org.opensearch.core.xcontent.XContentBuilder; -// CS-ENFORCE-SINGLE - -public class RunClusterHealthResponse extends AcknowledgedResponse implements ToXContentObject { - - public RunClusterHealthResponse(boolean status) { - super(status); - } - - public RunClusterHealthResponse(StreamInput in) throws IOException { - super(in); - } - - @Override - public void writeTo(StreamOutput out) throws IOException { - super.writeTo(out); - } - - @Override - public void addCustomFields(XContentBuilder builder, Params params) throws IOException { - super.addCustomFields(builder, params); - } -} diff --git a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/SystemIndexPlugin1.java b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/SystemIndexPlugin1.java index 9805619965..9113a50a5f 100644 --- a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/SystemIndexPlugin1.java +++ b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/SystemIndexPlugin1.java @@ -45,7 +45,7 @@ public class SystemIndexPlugin1 extends Plugin implements SystemIndexPlugin, IdentityAwarePlugin { public static final String SYSTEM_INDEX_1 = ".system-index1"; - private PluginContextSwitcher contextSwitcher; + private RunAsSubjectClient pluginClient; private Client client; @@ -64,8 +64,8 @@ public Collection createComponents( Supplier repositoriesServiceSupplier ) { this.client = client; - this.contextSwitcher = new PluginContextSwitcher(); - return List.of(contextSwitcher); + this.pluginClient = new RunAsSubjectClient(client); + return List.of(pluginClient); } @Override @@ -86,9 +86,9 @@ public List getRestHandlers( ) { return List.of( new RestIndexDocumentIntoSystemIndexAction(client), - new RestRunClusterHealthAction(client, contextSwitcher), - new RestBulkIndexDocumentIntoSystemIndexAction(client, contextSwitcher), - new RestBulkIndexDocumentIntoMixOfSystemIndexAction(client, contextSwitcher) + new RestRunClusterHealthAction(client), + new RestBulkIndexDocumentIntoSystemIndexAction(client, pluginClient), + new RestBulkIndexDocumentIntoMixOfSystemIndexAction(client, pluginClient) ); } @@ -102,8 +102,8 @@ public List getRestHandlers( @Override public void assignSubject(PluginSubject pluginSystemSubject) { - if (contextSwitcher != null) { - this.contextSwitcher.initialize(pluginSystemSubject); + if (pluginClient != null) { + this.pluginClient.setSubject(pluginSystemSubject); } } } diff --git a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/TransportIndexDocumentIntoSystemIndexAction.java b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/TransportIndexDocumentIntoSystemIndexAction.java index 6138cfbb54..d9ea722a74 100644 --- a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/TransportIndexDocumentIntoSystemIndexAction.java +++ b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/TransportIndexDocumentIntoSystemIndexAction.java @@ -15,80 +15,57 @@ import org.opensearch.action.support.ActionFilters; import org.opensearch.action.support.HandledTransportAction; import org.opensearch.action.support.WriteRequest; +import org.opensearch.action.support.master.AcknowledgedResponse; import org.opensearch.client.Client; import org.opensearch.common.inject.Inject; import org.opensearch.common.xcontent.XContentType; import org.opensearch.core.action.ActionListener; -import org.opensearch.identity.IdentityService; -import org.opensearch.identity.Subject; -import org.opensearch.security.support.ConfigConstants; -import org.opensearch.security.user.User; import org.opensearch.tasks.Task; -import org.opensearch.threadpool.ThreadPool; import org.opensearch.transport.TransportService; public class TransportIndexDocumentIntoSystemIndexAction extends HandledTransportAction< IndexDocumentIntoSystemIndexRequest, - IndexDocumentIntoSystemIndexResponse> { + AcknowledgedResponse> { private final Client client; - private final ThreadPool threadPool; - private final PluginContextSwitcher contextSwitcher; - private final IdentityService identityService; + private final RunAsSubjectClient pluginClient; @Inject public TransportIndexDocumentIntoSystemIndexAction( final TransportService transportService, final ActionFilters actionFilters, final Client client, - final ThreadPool threadPool, - final PluginContextSwitcher contextSwitcher, - final IdentityService identityService + final RunAsSubjectClient pluginClient ) { super(IndexDocumentIntoSystemIndexAction.NAME, transportService, actionFilters, IndexDocumentIntoSystemIndexRequest::new); this.client = client; - this.threadPool = threadPool; - this.contextSwitcher = contextSwitcher; - this.identityService = identityService; + this.pluginClient = pluginClient; } @Override - protected void doExecute( - Task task, - IndexDocumentIntoSystemIndexRequest request, - ActionListener actionListener - ) { + protected void doExecute(Task task, IndexDocumentIntoSystemIndexRequest request, ActionListener actionListener) { String indexName = request.getIndexName(); String runAs = request.getRunAs(); - Subject userSubject = identityService.getCurrentSubject(); try { - contextSwitcher.runAs(() -> { - client.admin().indices().create(new CreateIndexRequest(indexName), ActionListener.wrap(r -> { - if ("user".equalsIgnoreCase(runAs)) { - userSubject.runAs(() -> { - client.index( - new IndexRequest(indexName).setRefreshPolicy(WriteRequest.RefreshPolicy.IMMEDIATE) - .source("{\"content\":1}", XContentType.JSON), - ActionListener.wrap(r2 -> { - User user = threadPool.getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER); - actionListener.onResponse(new IndexDocumentIntoSystemIndexResponse(true, user.getName())); - }, actionListener::onFailure) - ); - return null; - }); - } else { - client.index( - new IndexRequest(indexName).setRefreshPolicy(WriteRequest.RefreshPolicy.IMMEDIATE) - .source("{\"content\":1}", XContentType.JSON), - ActionListener.wrap(r2 -> { - User user = threadPool.getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER); - actionListener.onResponse(new IndexDocumentIntoSystemIndexResponse(true, user.getName())); - }, actionListener::onFailure) - ); - } - }, actionListener::onFailure)); - return null; - }); + pluginClient.admin().indices().create(new CreateIndexRequest(indexName), ActionListener.wrap(r -> { + if ("user".equalsIgnoreCase(runAs)) { + client.index( + new IndexRequest(indexName).setRefreshPolicy(WriteRequest.RefreshPolicy.IMMEDIATE) + .source("{\"content\":1}", XContentType.JSON), + ActionListener.wrap(r2 -> { + actionListener.onResponse(new AcknowledgedResponse(true)); + }, actionListener::onFailure) + ); + } else { + pluginClient.index( + new IndexRequest(indexName).setRefreshPolicy(WriteRequest.RefreshPolicy.IMMEDIATE) + .source("{\"content\":1}", XContentType.JSON), + ActionListener.wrap(r2 -> { + actionListener.onResponse(new AcknowledgedResponse(true)); + }, actionListener::onFailure) + ); + } + }, actionListener::onFailure)); } catch (Exception ex) { throw new RuntimeException("Unexpected error: " + ex.getMessage()); } diff --git a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/TransportRunClusterHealthAction.java b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/TransportRunClusterHealthAction.java index 310262c947..c7c81d1e53 100644 --- a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/TransportRunClusterHealthAction.java +++ b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/TransportRunClusterHealthAction.java @@ -14,67 +14,43 @@ import org.opensearch.action.admin.cluster.health.ClusterHealthResponse; import org.opensearch.action.support.ActionFilters; import org.opensearch.action.support.HandledTransportAction; +import org.opensearch.action.support.master.AcknowledgedResponse; import org.opensearch.client.Client; import org.opensearch.common.inject.Inject; import org.opensearch.core.action.ActionListener; -import org.opensearch.identity.IdentityService; -import org.opensearch.identity.Subject; import org.opensearch.tasks.Task; -import org.opensearch.threadpool.ThreadPool; import org.opensearch.transport.TransportService; -public class TransportRunClusterHealthAction extends HandledTransportAction { +public class TransportRunClusterHealthAction extends HandledTransportAction { private final Client client; - private final ThreadPool threadPool; - private final PluginContextSwitcher contextSwitcher; - private final IdentityService identityService; + private final RunAsSubjectClient pluginClient; @Inject public TransportRunClusterHealthAction( final TransportService transportService, final ActionFilters actionFilters, final Client client, - final ThreadPool threadPool, - final PluginContextSwitcher contextSwitcher, - final IdentityService identityService + final RunAsSubjectClient pluginClient ) { super(RunClusterHealthAction.NAME, transportService, actionFilters, RunClusterHealthRequest::new); this.client = client; - this.threadPool = threadPool; - this.contextSwitcher = contextSwitcher; - this.identityService = identityService; + this.pluginClient = pluginClient; } @Override - protected void doExecute(Task task, RunClusterHealthRequest request, ActionListener actionListener) { + protected void doExecute(Task task, RunClusterHealthRequest request, ActionListener actionListener) { String runAs = request.getRunAs(); - if ("user".equalsIgnoreCase(runAs)) { - Subject user = identityService.getCurrentSubject(); - try { - user.runAs(() -> { - ActionListener chr = ActionListener.wrap( - r -> { actionListener.onResponse(new RunClusterHealthResponse(true)); }, - actionListener::onFailure - ); - client.admin().cluster().health(new ClusterHealthRequest(), chr); - return null; - }); - } catch (Exception e) { - throw new RuntimeException(e); - } - } else if ("plugin".equalsIgnoreCase(runAs)) { - contextSwitcher.runAs(() -> { - ActionListener chr = ActionListener.wrap( - r -> { actionListener.onResponse(new RunClusterHealthResponse(true)); }, - actionListener::onFailure - ); - client.admin().cluster().health(new ClusterHealthRequest(), chr); - return null; - }); + if ("plugin".equalsIgnoreCase(runAs)) { + ActionListener chr = ActionListener.wrap( + r -> { actionListener.onResponse(new AcknowledgedResponse(true)); }, + actionListener::onFailure + ); + pluginClient.admin().cluster().health(new ClusterHealthRequest(), chr); } else { + // run in the authenticated user context ActionListener chr = ActionListener.wrap( - r -> { actionListener.onResponse(new RunClusterHealthResponse(true)); }, + r -> { actionListener.onResponse(new AcknowledgedResponse(true)); }, actionListener::onFailure ); client.admin().cluster().health(new ClusterHealthRequest(), chr); From adb6cf64dd7df7fed5ab04735df1cf8165d584e1 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 14 Jan 2025 20:05:09 -0500 Subject: [PATCH 2/8] Fix checkstyle error Signed-off-by: Craig Perkins --- .../sampleplugin/IndexDocumentIntoSystemIndexAction.java | 2 ++ .../systemindex/sampleplugin/RunClusterHealthAction.java | 2 ++ .../TransportIndexDocumentIntoSystemIndexAction.java | 2 ++ .../sampleplugin/TransportRunClusterHealthAction.java | 2 ++ 4 files changed, 8 insertions(+) diff --git a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/IndexDocumentIntoSystemIndexAction.java b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/IndexDocumentIntoSystemIndexAction.java index b272f464c1..f9c6abfa04 100644 --- a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/IndexDocumentIntoSystemIndexAction.java +++ b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/IndexDocumentIntoSystemIndexAction.java @@ -11,7 +11,9 @@ package org.opensearch.security.systemindex.sampleplugin; import org.opensearch.action.ActionType; +// CS-SUPPRESS-SINGLE: RegexpSingleline It is not possible to use phrase "cluster manager" instead of master here import org.opensearch.action.support.master.AcknowledgedResponse; +// CS-ENFORCE-SINGLE public class IndexDocumentIntoSystemIndexAction extends ActionType { public static final IndexDocumentIntoSystemIndexAction INSTANCE = new IndexDocumentIntoSystemIndexAction(); diff --git a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RunClusterHealthAction.java b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RunClusterHealthAction.java index 7d028a7ddf..c076936490 100644 --- a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RunClusterHealthAction.java +++ b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RunClusterHealthAction.java @@ -11,7 +11,9 @@ package org.opensearch.security.systemindex.sampleplugin; import org.opensearch.action.ActionType; +// CS-SUPPRESS-SINGLE: RegexpSingleline It is not possible to use phrase "cluster manager" instead of master here import org.opensearch.action.support.master.AcknowledgedResponse; +// CS-ENFORCE-SINGLE public class RunClusterHealthAction extends ActionType { public static final RunClusterHealthAction INSTANCE = new RunClusterHealthAction(); diff --git a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/TransportIndexDocumentIntoSystemIndexAction.java b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/TransportIndexDocumentIntoSystemIndexAction.java index d9ea722a74..1f66dc8bf5 100644 --- a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/TransportIndexDocumentIntoSystemIndexAction.java +++ b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/TransportIndexDocumentIntoSystemIndexAction.java @@ -15,7 +15,9 @@ import org.opensearch.action.support.ActionFilters; import org.opensearch.action.support.HandledTransportAction; import org.opensearch.action.support.WriteRequest; +// CS-SUPPRESS-SINGLE: RegexpSingleline It is not possible to use phrase "cluster manager" instead of master here import org.opensearch.action.support.master.AcknowledgedResponse; +// CS-ENFORCE-SINGLE import org.opensearch.client.Client; import org.opensearch.common.inject.Inject; import org.opensearch.common.xcontent.XContentType; diff --git a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/TransportRunClusterHealthAction.java b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/TransportRunClusterHealthAction.java index c7c81d1e53..0592110831 100644 --- a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/TransportRunClusterHealthAction.java +++ b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/TransportRunClusterHealthAction.java @@ -14,7 +14,9 @@ import org.opensearch.action.admin.cluster.health.ClusterHealthResponse; import org.opensearch.action.support.ActionFilters; import org.opensearch.action.support.HandledTransportAction; +// CS-SUPPRESS-SINGLE: RegexpSingleline It is not possible to use phrase "cluster manager" instead of master here import org.opensearch.action.support.master.AcknowledgedResponse; +// CS-ENFORCE-SINGLE import org.opensearch.client.Client; import org.opensearch.common.inject.Inject; import org.opensearch.core.action.ActionListener; From 0952ac51ba299d75394337afc6b32d0497193e54 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 14 Jan 2025 21:12:42 -0500 Subject: [PATCH 3/8] Surround all imports Signed-off-by: Craig Perkins --- .../sampleplugin/IndexDocumentIntoSystemIndexAction.java | 2 +- .../systemindex/sampleplugin/RunClusterHealthAction.java | 2 +- .../TransportIndexDocumentIntoSystemIndexAction.java | 4 ++-- .../sampleplugin/TransportRunClusterHealthAction.java | 4 ++-- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/IndexDocumentIntoSystemIndexAction.java b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/IndexDocumentIntoSystemIndexAction.java index f9c6abfa04..3be83b39b2 100644 --- a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/IndexDocumentIntoSystemIndexAction.java +++ b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/IndexDocumentIntoSystemIndexAction.java @@ -10,8 +10,8 @@ package org.opensearch.security.systemindex.sampleplugin; -import org.opensearch.action.ActionType; // CS-SUPPRESS-SINGLE: RegexpSingleline It is not possible to use phrase "cluster manager" instead of master here +import org.opensearch.action.ActionType; import org.opensearch.action.support.master.AcknowledgedResponse; // CS-ENFORCE-SINGLE diff --git a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RunClusterHealthAction.java b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RunClusterHealthAction.java index c076936490..e197362254 100644 --- a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RunClusterHealthAction.java +++ b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RunClusterHealthAction.java @@ -10,8 +10,8 @@ package org.opensearch.security.systemindex.sampleplugin; -import org.opensearch.action.ActionType; // CS-SUPPRESS-SINGLE: RegexpSingleline It is not possible to use phrase "cluster manager" instead of master here +import org.opensearch.action.ActionType; import org.opensearch.action.support.master.AcknowledgedResponse; // CS-ENFORCE-SINGLE diff --git a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/TransportIndexDocumentIntoSystemIndexAction.java b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/TransportIndexDocumentIntoSystemIndexAction.java index 1f66dc8bf5..26ae100c89 100644 --- a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/TransportIndexDocumentIntoSystemIndexAction.java +++ b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/TransportIndexDocumentIntoSystemIndexAction.java @@ -10,20 +10,20 @@ package org.opensearch.security.systemindex.sampleplugin; +// CS-SUPPRESS-SINGLE: RegexpSingleline It is not possible to use phrase "cluster manager" instead of master here import org.opensearch.action.admin.indices.create.CreateIndexRequest; import org.opensearch.action.index.IndexRequest; import org.opensearch.action.support.ActionFilters; import org.opensearch.action.support.HandledTransportAction; import org.opensearch.action.support.WriteRequest; -// CS-SUPPRESS-SINGLE: RegexpSingleline It is not possible to use phrase "cluster manager" instead of master here import org.opensearch.action.support.master.AcknowledgedResponse; -// CS-ENFORCE-SINGLE import org.opensearch.client.Client; import org.opensearch.common.inject.Inject; import org.opensearch.common.xcontent.XContentType; import org.opensearch.core.action.ActionListener; import org.opensearch.tasks.Task; import org.opensearch.transport.TransportService; +// CS-ENFORCE-SINGLE public class TransportIndexDocumentIntoSystemIndexAction extends HandledTransportAction< IndexDocumentIntoSystemIndexRequest, diff --git a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/TransportRunClusterHealthAction.java b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/TransportRunClusterHealthAction.java index 0592110831..12e17ae998 100644 --- a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/TransportRunClusterHealthAction.java +++ b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/TransportRunClusterHealthAction.java @@ -10,18 +10,18 @@ package org.opensearch.security.systemindex.sampleplugin; +// CS-SUPPRESS-SINGLE: RegexpSingleline It is not possible to use phrase "cluster manager" instead of master here import org.opensearch.action.admin.cluster.health.ClusterHealthRequest; import org.opensearch.action.admin.cluster.health.ClusterHealthResponse; import org.opensearch.action.support.ActionFilters; import org.opensearch.action.support.HandledTransportAction; -// CS-SUPPRESS-SINGLE: RegexpSingleline It is not possible to use phrase "cluster manager" instead of master here import org.opensearch.action.support.master.AcknowledgedResponse; -// CS-ENFORCE-SINGLE import org.opensearch.client.Client; import org.opensearch.common.inject.Inject; import org.opensearch.core.action.ActionListener; import org.opensearch.tasks.Task; import org.opensearch.transport.TransportService; +// CS-ENFORCE-SINGLE public class TransportRunClusterHealthAction extends HandledTransportAction { From 3a464c1e8c8f91d99408f794c7961d6b2413046d Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 16 Jan 2025 10:34:35 -0500 Subject: [PATCH 4/8] Ensure that plugin can search on system index when utilizing pluginSubject.runAs Signed-off-by: Craig Perkins --- .../systemindex/SystemIndexTests.java | 32 +++++++++ .../RestSearchOnSystemIndexAction.java | 65 +++++++++++++++++++ .../sampleplugin/SystemIndexPlugin1.java | 8 +-- .../security/auditlog/impl/AuditLogImpl.java | 1 + .../SystemIndexSearcherWrapper.java | 6 +- .../privileges/dlsfls/DlsFlsBaseContext.java | 2 +- .../security/support/HeaderHelper.java | 13 ++++ 7 files changed, 117 insertions(+), 10 deletions(-) create mode 100644 src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RestSearchOnSystemIndexAction.java diff --git a/src/integrationTest/java/org/opensearch/security/systemindex/SystemIndexTests.java b/src/integrationTest/java/org/opensearch/security/systemindex/SystemIndexTests.java index 13325b3029..3b09cfedfb 100644 --- a/src/integrationTest/java/org/opensearch/security/systemindex/SystemIndexTests.java +++ b/src/integrationTest/java/org/opensearch/security/systemindex/SystemIndexTests.java @@ -13,6 +13,7 @@ import java.util.Map; import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope; +import com.fasterxml.jackson.databind.JsonNode; import org.junit.Before; import org.junit.ClassRule; import org.junit.Test; @@ -170,6 +171,37 @@ public void testPluginShouldBeAbleToBulkIndexDocumentIntoItsSystemIndex() { } } + @Test + public void testPluginShouldBeAbleSearchOnItsSystemIndex() { + JsonNode searchResponse1; + JsonNode searchResponse2; + try (TestRestClient client = cluster.getRestClient(USER_ADMIN)) { + HttpResponse response = client.put("try-create-and-bulk-index/" + SYSTEM_INDEX_1); + + assertThat(response.getStatusCode(), equalTo(RestStatus.OK.getStatus())); + + HttpResponse searchResponse = client.get("search-on-system-index/" + SYSTEM_INDEX_1); + + assertThat(searchResponse.getStatusCode(), equalTo(RestStatus.OK.getStatus())); + assertThat(searchResponse.getIntFromJsonBody("/hits/total/value"), equalTo(2)); + + searchResponse1 = searchResponse.bodyAsJsonNode(); + } + + try (TestRestClient client = cluster.getRestClient(cluster.getAdminCertificate())) { + HttpResponse searchResponse = client.get(SYSTEM_INDEX_1 + "/_search"); + + assertThat(searchResponse.getStatusCode(), equalTo(RestStatus.OK.getStatus())); + assertThat(searchResponse.getIntFromJsonBody("/hits/total/value"), equalTo(2)); + + searchResponse2 = searchResponse.bodyAsJsonNode(); + } + + JsonNode hits1 = searchResponse1.get("hits"); + JsonNode hits2 = searchResponse2.get("hits"); + assertThat(hits1.toPrettyString(), equalTo(hits2.toPrettyString())); + } + @Test public void testPluginShouldNotBeAbleToBulkIndexDocumentIntoMixOfSystemIndexWhereAtLeastOneDoesNotBelongToPlugin() { try (TestRestClient client = cluster.getRestClient(cluster.getAdminCertificate())) { diff --git a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RestSearchOnSystemIndexAction.java b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RestSearchOnSystemIndexAction.java new file mode 100644 index 0000000000..4f9bcf6cba --- /dev/null +++ b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RestSearchOnSystemIndexAction.java @@ -0,0 +1,65 @@ +/* + * 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.systemindex.sampleplugin; + +import java.util.List; + +import org.opensearch.action.search.SearchRequest; +import org.opensearch.client.node.NodeClient; +import org.opensearch.core.action.ActionListener; +import org.opensearch.core.rest.RestStatus; +import org.opensearch.core.xcontent.ToXContent; +import org.opensearch.index.query.QueryBuilders; +import org.opensearch.rest.BaseRestHandler; +import org.opensearch.rest.BytesRestResponse; +import org.opensearch.rest.RestChannel; +import org.opensearch.rest.RestRequest; +import org.opensearch.search.builder.SearchSourceBuilder; + +import static java.util.Collections.singletonList; +import static org.opensearch.rest.RestRequest.Method.GET; + +public class RestSearchOnSystemIndexAction extends BaseRestHandler { + + private final RunAsSubjectClient pluginClient; + + public RestSearchOnSystemIndexAction(RunAsSubjectClient pluginClient) { + this.pluginClient = pluginClient; + } + + @Override + public List routes() { + return singletonList(new Route(GET, "/search-on-system-index/{index}")); + } + + @Override + public String getName() { + return "test_search_on_system_index_action"; + } + + @Override + public RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) { + String indexName = request.param("index"); + return new RestChannelConsumer() { + + @Override + public void accept(RestChannel channel) throws Exception { + SearchRequest searchRequest = new SearchRequest(indexName); + SearchSourceBuilder sourceBuilder = new SearchSourceBuilder(); + sourceBuilder.query(QueryBuilders.matchAllQuery()); + searchRequest.source(sourceBuilder); + pluginClient.search(searchRequest, ActionListener.wrap(r -> { + channel.sendResponse(new BytesRestResponse(RestStatus.OK, r.toXContent(channel.newBuilder(), ToXContent.EMPTY_PARAMS))); + }, fr -> { channel.sendResponse(new BytesRestResponse(RestStatus.FORBIDDEN, String.valueOf(fr))); })); + } + }; + } +} diff --git a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/SystemIndexPlugin1.java b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/SystemIndexPlugin1.java index 9113a50a5f..7358f93e0c 100644 --- a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/SystemIndexPlugin1.java +++ b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/SystemIndexPlugin1.java @@ -88,16 +88,14 @@ public List getRestHandlers( new RestIndexDocumentIntoSystemIndexAction(client), new RestRunClusterHealthAction(client), new RestBulkIndexDocumentIntoSystemIndexAction(client, pluginClient), - new RestBulkIndexDocumentIntoMixOfSystemIndexAction(client, pluginClient) + new RestBulkIndexDocumentIntoMixOfSystemIndexAction(client, pluginClient), + new RestSearchOnSystemIndexAction(pluginClient) ); } @Override public List> getActions() { - return Arrays.asList( - new ActionHandler<>(IndexDocumentIntoSystemIndexAction.INSTANCE, TransportIndexDocumentIntoSystemIndexAction.class), - new ActionHandler<>(RunClusterHealthAction.INSTANCE, TransportRunClusterHealthAction.class) - ); + return Arrays.asList(new ActionHandler<>(RunClusterHealthAction.INSTANCE, TransportRunClusterHealthAction.class)); } @Override diff --git a/src/main/java/org/opensearch/security/auditlog/impl/AuditLogImpl.java b/src/main/java/org/opensearch/security/auditlog/impl/AuditLogImpl.java index e860ec0d5e..dbd2d867b5 100644 --- a/src/main/java/org/opensearch/security/auditlog/impl/AuditLogImpl.java +++ b/src/main/java/org/opensearch/security/auditlog/impl/AuditLogImpl.java @@ -68,6 +68,7 @@ public AuditLogImpl( final Environment environment ) { super(settings, threadPool, resolver, clusterService, environment); + clusterService.state().metadata(). this.settings = settings; this.messageRouter = new AuditMessageRouter(settings, clientProvider, threadPool, configPath); this.messageRouterEnabled = this.messageRouter.isEnabled(); diff --git a/src/main/java/org/opensearch/security/configuration/SystemIndexSearcherWrapper.java b/src/main/java/org/opensearch/security/configuration/SystemIndexSearcherWrapper.java index b87c92c356..73d462e7fc 100644 --- a/src/main/java/org/opensearch/security/configuration/SystemIndexSearcherWrapper.java +++ b/src/main/java/org/opensearch/security/configuration/SystemIndexSearcherWrapper.java @@ -111,7 +111,6 @@ public void onConfigModelChanged(ConfigModel cm) { @Override public final DirectoryReader apply(DirectoryReader reader) throws IOException { - if (isSecurityIndexRequest() && !isAdminAuthenticatedOrInternalRequest()) { return new EmptyFilterLeafReader.EmptyDirectoryReader(reader); } @@ -163,7 +162,7 @@ protected final boolean isBlockedSystemIndexRequest() { if (systemIndexPermissionEnabled) { final User user = threadContext.getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER); - if (user == null) { + if (HeaderHelper.isInternalOrPluginRequest(threadContext)) { // allow request without user from plugin. return systemIndexMatcher.test(index.getName()) || matchesSystemIndexRegisteredWithCore; } @@ -180,8 +179,7 @@ protected final boolean isBlockedSystemIndexRequest() { protected final boolean isAdminDnOrPluginRequest() { final User user = threadContext.getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER); - if (user == null) { - // allow request without user from plugin. + if (HeaderHelper.isInternalOrPluginRequest(threadContext)) { return true; } else if (adminDns.isAdmin(user)) { return true; diff --git a/src/main/java/org/opensearch/security/privileges/dlsfls/DlsFlsBaseContext.java b/src/main/java/org/opensearch/security/privileges/dlsfls/DlsFlsBaseContext.java index 232e2d7422..8a27fef255 100644 --- a/src/main/java/org/opensearch/security/privileges/dlsfls/DlsFlsBaseContext.java +++ b/src/main/java/org/opensearch/security/privileges/dlsfls/DlsFlsBaseContext.java @@ -39,7 +39,7 @@ public DlsFlsBaseContext(PrivilegesEvaluator privilegesEvaluator, ThreadContext public PrivilegesEvaluationContext getPrivilegesEvaluationContext() { User user = threadContext.getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER); - if (user == null || adminDNs.isAdmin(user)) { + if (HeaderHelper.isInternalOrPluginRequest(threadContext) || adminDNs.isAdmin(user)) { return null; } diff --git a/src/main/java/org/opensearch/security/support/HeaderHelper.java b/src/main/java/org/opensearch/security/support/HeaderHelper.java index bbb44664fa..3b237f5891 100644 --- a/src/main/java/org/opensearch/security/support/HeaderHelper.java +++ b/src/main/java/org/opensearch/security/support/HeaderHelper.java @@ -33,6 +33,7 @@ import com.google.common.base.Strings; import org.opensearch.common.util.concurrent.ThreadContext; +import org.opensearch.security.user.User; public class HeaderHelper { @@ -52,6 +53,18 @@ public static boolean isExtensionRequest(final ThreadContext context) { } // CS-ENFORCE-SINGLE + public static boolean isInternalOrPluginRequest(final ThreadContext threadContext) { + // If user is empty, this indicates a system-level request which should be permitted + // If the requests originates from a plugin this will also return true + final User user = (User) threadContext.getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER); + + if (user == null || user.isPluginUser()) { + return true; + } + + return false; + } + public static String getSafeFromHeader(final ThreadContext context, final String headerName) { if (context == null || headerName == null || headerName.isEmpty()) { From 2d4daab04a9520e847a4e240ea8d970c680fa8b4 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 16 Jan 2025 11:13:10 -0500 Subject: [PATCH 5/8] Remove line inadverantly added in last commit Signed-off-by: Craig Perkins --- .../java/org/opensearch/security/auditlog/impl/AuditLogImpl.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/main/java/org/opensearch/security/auditlog/impl/AuditLogImpl.java b/src/main/java/org/opensearch/security/auditlog/impl/AuditLogImpl.java index dbd2d867b5..e860ec0d5e 100644 --- a/src/main/java/org/opensearch/security/auditlog/impl/AuditLogImpl.java +++ b/src/main/java/org/opensearch/security/auditlog/impl/AuditLogImpl.java @@ -68,7 +68,6 @@ public AuditLogImpl( final Environment environment ) { super(settings, threadPool, resolver, clusterService, environment); - clusterService.state().metadata(). this.settings = settings; this.messageRouter = new AuditMessageRouter(settings, clientProvider, threadPool, configPath); this.messageRouterEnabled = this.messageRouter.isEnabled(); From a06fbf45087f9837ab0a5bedfd24c65008f1740b Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 16 Jan 2025 11:25:50 -0500 Subject: [PATCH 6/8] Add test for GET as well as search Signed-off-by: Craig Perkins --- .../systemindex/SystemIndexTests.java | 36 +++++++++++ .../RestGetOnSystemIndexAction.java | 62 +++++++++++++++++++ .../sampleplugin/SystemIndexPlugin1.java | 3 +- 3 files changed, 100 insertions(+), 1 deletion(-) create mode 100644 src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RestGetOnSystemIndexAction.java diff --git a/src/integrationTest/java/org/opensearch/security/systemindex/SystemIndexTests.java b/src/integrationTest/java/org/opensearch/security/systemindex/SystemIndexTests.java index 3b09cfedfb..8294d912e9 100644 --- a/src/integrationTest/java/org/opensearch/security/systemindex/SystemIndexTests.java +++ b/src/integrationTest/java/org/opensearch/security/systemindex/SystemIndexTests.java @@ -202,6 +202,42 @@ public void testPluginShouldBeAbleSearchOnItsSystemIndex() { assertThat(hits1.toPrettyString(), equalTo(hits2.toPrettyString())); } + @Test + public void testPluginShouldBeAbleGetOnItsSystemIndex() { + JsonNode getResponse1; + JsonNode getResponse2; + try (TestRestClient client = cluster.getRestClient(USER_ADMIN)) { + HttpResponse response = client.put("try-create-and-bulk-index/" + SYSTEM_INDEX_1); + + assertThat(response.getStatusCode(), equalTo(RestStatus.OK.getStatus())); + + HttpResponse searchResponse = client.get("search-on-system-index/" + SYSTEM_INDEX_1); + + assertThat(searchResponse.getStatusCode(), equalTo(RestStatus.OK.getStatus())); + assertThat(searchResponse.getIntFromJsonBody("/hits/total/value"), equalTo(2)); + + String docId = searchResponse.getTextFromJsonBody("/hits/hits/0/_id"); + + HttpResponse getResponse = client.get("get-on-system-index/" + SYSTEM_INDEX_1 + "/" + docId); + + getResponse1 = getResponse.bodyAsJsonNode(); + } + + try (TestRestClient client = cluster.getRestClient(cluster.getAdminCertificate())) { + HttpResponse searchResponse = client.get(SYSTEM_INDEX_1 + "/_search"); + + assertThat(searchResponse.getStatusCode(), equalTo(RestStatus.OK.getStatus())); + assertThat(searchResponse.getIntFromJsonBody("/hits/total/value"), equalTo(2)); + + String docId = searchResponse.getTextFromJsonBody("/hits/hits/0/_id"); + + HttpResponse getResponse = client.get(SYSTEM_INDEX_1 + "/_doc/" + docId); + + getResponse2 = getResponse.bodyAsJsonNode(); + } + assertThat(getResponse1.toPrettyString(), equalTo(getResponse2.toPrettyString())); + } + @Test public void testPluginShouldNotBeAbleToBulkIndexDocumentIntoMixOfSystemIndexWhereAtLeastOneDoesNotBelongToPlugin() { try (TestRestClient client = cluster.getRestClient(cluster.getAdminCertificate())) { diff --git a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RestGetOnSystemIndexAction.java b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RestGetOnSystemIndexAction.java new file mode 100644 index 0000000000..86ed4ce4d7 --- /dev/null +++ b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/RestGetOnSystemIndexAction.java @@ -0,0 +1,62 @@ +/* + * 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.systemindex.sampleplugin; + +import java.util.List; + +import org.opensearch.action.get.GetRequest; +import org.opensearch.client.node.NodeClient; +import org.opensearch.core.action.ActionListener; +import org.opensearch.core.rest.RestStatus; +import org.opensearch.core.xcontent.ToXContent; +import org.opensearch.rest.BaseRestHandler; +import org.opensearch.rest.BytesRestResponse; +import org.opensearch.rest.RestChannel; +import org.opensearch.rest.RestRequest; + +import static java.util.Collections.singletonList; +import static org.opensearch.rest.RestRequest.Method.GET; + +public class RestGetOnSystemIndexAction extends BaseRestHandler { + + private final RunAsSubjectClient pluginClient; + + public RestGetOnSystemIndexAction(RunAsSubjectClient pluginClient) { + this.pluginClient = pluginClient; + } + + @Override + public List routes() { + return singletonList(new Route(GET, "/get-on-system-index/{index}/{docId}")); + } + + @Override + public String getName() { + return "test_get_on_system_index_action"; + } + + @Override + public RestChannelConsumer prepareRequest(RestRequest request, NodeClient client) { + String indexName = request.param("index"); + String docId = request.param("docId"); + return new RestChannelConsumer() { + + @Override + public void accept(RestChannel channel) throws Exception { + GetRequest getRequest = new GetRequest(indexName); + getRequest.id(docId); + pluginClient.get(getRequest, ActionListener.wrap(r -> { + channel.sendResponse(new BytesRestResponse(RestStatus.OK, r.toXContent(channel.newBuilder(), ToXContent.EMPTY_PARAMS))); + }, fr -> { channel.sendResponse(new BytesRestResponse(RestStatus.FORBIDDEN, String.valueOf(fr))); })); + } + }; + } +} diff --git a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/SystemIndexPlugin1.java b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/SystemIndexPlugin1.java index 7358f93e0c..29f1ef3271 100644 --- a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/SystemIndexPlugin1.java +++ b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/SystemIndexPlugin1.java @@ -89,7 +89,8 @@ public List getRestHandlers( new RestRunClusterHealthAction(client), new RestBulkIndexDocumentIntoSystemIndexAction(client, pluginClient), new RestBulkIndexDocumentIntoMixOfSystemIndexAction(client, pluginClient), - new RestSearchOnSystemIndexAction(pluginClient) + new RestSearchOnSystemIndexAction(pluginClient), + new RestGetOnSystemIndexAction(pluginClient) ); } From d0d4474b2e140428c0e661efdbfe9e5a60be58e8 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Thu, 16 Jan 2025 11:29:38 -0500 Subject: [PATCH 7/8] Add back in IndexDocumentIntoSystemIndexAction Signed-off-by: Craig Perkins --- .../systemindex/sampleplugin/SystemIndexPlugin1.java | 5 ++++- .../security/configuration/SystemIndexSearcherWrapper.java | 1 + 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/SystemIndexPlugin1.java b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/SystemIndexPlugin1.java index 29f1ef3271..edd90d0568 100644 --- a/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/SystemIndexPlugin1.java +++ b/src/integrationTest/java/org/opensearch/security/systemindex/sampleplugin/SystemIndexPlugin1.java @@ -96,7 +96,10 @@ public List getRestHandlers( @Override public List> getActions() { - return Arrays.asList(new ActionHandler<>(RunClusterHealthAction.INSTANCE, TransportRunClusterHealthAction.class)); + return Arrays.asList( + new ActionHandler<>(IndexDocumentIntoSystemIndexAction.INSTANCE, TransportIndexDocumentIntoSystemIndexAction.class), + new ActionHandler<>(RunClusterHealthAction.INSTANCE, TransportRunClusterHealthAction.class) + ); } @Override diff --git a/src/main/java/org/opensearch/security/configuration/SystemIndexSearcherWrapper.java b/src/main/java/org/opensearch/security/configuration/SystemIndexSearcherWrapper.java index 73d462e7fc..080e2a8a08 100644 --- a/src/main/java/org/opensearch/security/configuration/SystemIndexSearcherWrapper.java +++ b/src/main/java/org/opensearch/security/configuration/SystemIndexSearcherWrapper.java @@ -111,6 +111,7 @@ public void onConfigModelChanged(ConfigModel cm) { @Override public final DirectoryReader apply(DirectoryReader reader) throws IOException { + if (isSecurityIndexRequest() && !isAdminAuthenticatedOrInternalRequest()) { return new EmptyFilterLeafReader.EmptyDirectoryReader(reader); } From 117855f11c94a9cb047b39b65db403c75afb4dcc Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Fri, 17 Jan 2025 11:10:11 -0500 Subject: [PATCH 8/8] Handle case where system indexes are disabled Signed-off-by: Craig Perkins --- .../systemindex/SystemIndexDisabledTests.java | 88 +++++++++++++++++++ .../SystemIndexAccessEvaluator.java | 2 +- 2 files changed, 89 insertions(+), 1 deletion(-) create mode 100644 src/integrationTest/java/org/opensearch/security/systemindex/SystemIndexDisabledTests.java diff --git a/src/integrationTest/java/org/opensearch/security/systemindex/SystemIndexDisabledTests.java b/src/integrationTest/java/org/opensearch/security/systemindex/SystemIndexDisabledTests.java new file mode 100644 index 0000000000..109e5228cf --- /dev/null +++ b/src/integrationTest/java/org/opensearch/security/systemindex/SystemIndexDisabledTests.java @@ -0,0 +1,88 @@ +/* + * 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.systemindex; + +import java.util.List; +import java.util.Map; + +import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope; +import org.junit.Before; +import org.junit.ClassRule; +import org.junit.Test; +import org.junit.runner.RunWith; + +import org.opensearch.core.rest.RestStatus; +import org.opensearch.security.systemindex.sampleplugin.SystemIndexPlugin1; +import org.opensearch.security.systemindex.sampleplugin.SystemIndexPlugin2; +import org.opensearch.test.framework.TestSecurityConfig.AuthcDomain; +import org.opensearch.test.framework.cluster.ClusterManager; +import org.opensearch.test.framework.cluster.LocalCluster; +import org.opensearch.test.framework.cluster.TestRestClient; +import org.opensearch.test.framework.cluster.TestRestClient.HttpResponse; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.not; +import static org.opensearch.security.support.ConfigConstants.SECURITY_RESTAPI_ROLES_ENABLED; +import static org.opensearch.security.support.ConfigConstants.SECURITY_SYSTEM_INDICES_ENABLED_KEY; +import static org.opensearch.test.framework.TestSecurityConfig.Role.ALL_ACCESS; +import static org.opensearch.test.framework.TestSecurityConfig.User.USER_ADMIN; + +@RunWith(com.carrotsearch.randomizedtesting.RandomizedRunner.class) +@ThreadLeakScope(ThreadLeakScope.Scope.NONE) +public class SystemIndexDisabledTests { + + public static final AuthcDomain AUTHC_DOMAIN = new AuthcDomain("basic", 0).httpAuthenticatorWithChallenge("basic").backend("internal"); + + @ClassRule + public static final LocalCluster cluster = new LocalCluster.Builder().clusterManager(ClusterManager.SINGLENODE) + .anonymousAuth(false) + .authc(AUTHC_DOMAIN) + .users(USER_ADMIN) + .plugin(SystemIndexPlugin1.class, SystemIndexPlugin2.class) + .nodeSettings( + Map.of( + SECURITY_RESTAPI_ROLES_ENABLED, + List.of("user_" + USER_ADMIN.getName() + "__" + ALL_ACCESS.getName()), + SECURITY_SYSTEM_INDICES_ENABLED_KEY, + false + ) + ) + .build(); + + @Before + public void setup() { + try (TestRestClient client = cluster.getRestClient(cluster.getAdminCertificate())) { + client.delete(".system-index1"); + } + } + + @Test + public void testPluginShouldBeAbleToIndexIntoAnySystemIndexWhenProtectionIsDisabled() { + try (TestRestClient client = cluster.getRestClient(cluster.getAdminCertificate())) { + client.put(".system-index1"); + client.put(".system-index2"); + } + try (TestRestClient client = cluster.getRestClient(USER_ADMIN)) { + HttpResponse response = client.put("try-create-and-bulk-mixed-index"); + + response.assertStatusCode(RestStatus.OK.getStatus()); + + assertThat( + response.getBody(), + not( + containsString( + "no permissions for [] and User [name=plugin:org.opensearch.security.systemindex.sampleplugin.SystemIndexPlugin1" + ) + ) + ); + } + } +} diff --git a/src/main/java/org/opensearch/security/privileges/SystemIndexAccessEvaluator.java b/src/main/java/org/opensearch/security/privileges/SystemIndexAccessEvaluator.java index f6a786c514..ba03ff2f25 100644 --- a/src/main/java/org/opensearch/security/privileges/SystemIndexAccessEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/SystemIndexAccessEvaluator.java @@ -304,7 +304,7 @@ private void evaluateSystemIndicesAccess( // cluster actions will return true for requestedResolved.isLocalAll() // the following section should only be run for index actions - if (user.isPluginUser() && !requestedResolved.isLocalAll()) { + if (this.isSystemIndexEnabled && user.isPluginUser() && !requestedResolved.isLocalAll()) { Set matchingSystemIndices = SystemIndexRegistry.matchesPluginSystemIndexPattern( user.getName().replace("plugin:", ""), requestedResolved.getAllIndices()