From eecba3ad5b9aded6f79695186bebd6e848ace2f9 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Mon, 25 Nov 2024 15:33:30 -0500 Subject: [PATCH 1/7] Only run DlsFlsValveImpl.invoke on indices requests Signed-off-by: Craig Perkins --- .../opensearch/security/configuration/DlsFlsValveImpl.java | 6 ++++++ .../opensearch/security/privileges/PrivilegesEvaluator.java | 4 ++++ 2 files changed, 10 insertions(+) diff --git a/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java b/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java index 498b908e5d..c4bfa54c31 100644 --- a/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java +++ b/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java @@ -83,6 +83,8 @@ import org.opensearch.security.support.ConfigConstants; import org.opensearch.threadpool.ThreadPool; +import static org.opensearch.security.privileges.PrivilegesEvaluator.isIndexPerm; + public class DlsFlsValveImpl implements DlsFlsRequestValve { private static final String MAP_EXECUTION_HINT = "map"; @@ -135,6 +137,10 @@ public DlsFlsValveImpl( */ @Override public boolean invoke(PrivilegesEvaluationContext context, final ActionListener listener) { + if (!isIndexPerm(context.getAction())) { + return true; + } + DlsFlsProcessedConfig config = this.dlsFlsProcessedConfig.get(); ActionRequest request = context.getRequest(); IndexResolverReplacer.Resolved resolved = context.getResolvedRequest(); diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java index 36666972ec..56a60bc35d 100644 --- a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java @@ -709,6 +709,10 @@ public static boolean isClusterPerm(String action0) { || (action0.equals(RenderSearchTemplateAction.NAME))); } + public static boolean isIndexPerm(String action0) { + return (action0.startsWith("indices:") && !isClusterPerm(action0)); + } + @SuppressWarnings("unchecked") private boolean checkFilteredAliases(Resolved requestedResolved, String action, boolean isDebugEnabled) { final String faMode = dcm.getFilteredAliasMode();// getConfigSettings().dynamic.filtered_alias_mode; From 47691449e10a92a0b0a49d6a1f8a12fe74274cd7 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Mon, 25 Nov 2024 16:00:45 -0500 Subject: [PATCH 2/7] Only check prefix Signed-off-by: Craig Perkins --- .../opensearch/security/configuration/DlsFlsValveImpl.java | 4 +--- .../opensearch/security/privileges/PrivilegesEvaluator.java | 4 ---- 2 files changed, 1 insertion(+), 7 deletions(-) diff --git a/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java b/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java index c4bfa54c31..3bac72890b 100644 --- a/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java +++ b/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java @@ -83,8 +83,6 @@ import org.opensearch.security.support.ConfigConstants; import org.opensearch.threadpool.ThreadPool; -import static org.opensearch.security.privileges.PrivilegesEvaluator.isIndexPerm; - public class DlsFlsValveImpl implements DlsFlsRequestValve { private static final String MAP_EXECUTION_HINT = "map"; @@ -137,7 +135,7 @@ public DlsFlsValveImpl( */ @Override public boolean invoke(PrivilegesEvaluationContext context, final ActionListener listener) { - if (!isIndexPerm(context.getAction())) { + if (!context.getAction().startsWith("indices:")) { return true; } diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java index 56a60bc35d..36666972ec 100644 --- a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java @@ -709,10 +709,6 @@ public static boolean isClusterPerm(String action0) { || (action0.equals(RenderSearchTemplateAction.NAME))); } - public static boolean isIndexPerm(String action0) { - return (action0.startsWith("indices:") && !isClusterPerm(action0)); - } - @SuppressWarnings("unchecked") private boolean checkFilteredAliases(Resolved requestedResolved, String action, boolean isDebugEnabled) { final String faMode = dcm.getFilteredAliasMode();// getConfigSettings().dynamic.filtered_alias_mode; From d6037f99c13320c79149eece793088be22abcc17 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Wed, 27 Nov 2024 13:25:55 -0500 Subject: [PATCH 3/7] Only set once Signed-off-by: Craig Perkins --- .../opensearch/security/configuration/DlsFlsValveImpl.java | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java b/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java index 3bac72890b..1afb0f36b8 100644 --- a/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java +++ b/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java @@ -683,7 +683,7 @@ static Mode get(Settings settings) { public void updateConfiguration(SecurityDynamicConfiguration rolesConfiguration) { try { if (rolesConfiguration != null) { - DlsFlsProcessedConfig oldConfig = this.dlsFlsProcessedConfig.getAndSet( + this.dlsFlsProcessedConfig.set( new DlsFlsProcessedConfig( DynamicConfigFactory.addStatics(rolesConfiguration.clone()), clusterService.state().metadata().getIndicesLookup(), @@ -692,10 +692,6 @@ public void updateConfiguration(SecurityDynamicConfiguration rolesConfig fieldMaskingConfig ) ); - - if (oldConfig != null) { - oldConfig.shutdown(); - } } } catch (Exception e) { log.error("Error while updating DLS/FLS configuration with {}", rolesConfiguration, e); From 9de9c24b23c764e213e9d9049b72bd8edaa824ff Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Wed, 27 Nov 2024 13:32:54 -0500 Subject: [PATCH 4/7] Increment metadata version Signed-off-by: Craig Perkins --- .../security/privileges/dlsfls/DlsFlsProcessedConfig.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/main/java/org/opensearch/security/privileges/dlsfls/DlsFlsProcessedConfig.java b/src/main/java/org/opensearch/security/privileges/dlsfls/DlsFlsProcessedConfig.java index b217b59df3..d7ade17e19 100644 --- a/src/main/java/org/opensearch/security/privileges/dlsfls/DlsFlsProcessedConfig.java +++ b/src/main/java/org/opensearch/security/privileges/dlsfls/DlsFlsProcessedConfig.java @@ -71,6 +71,7 @@ protected void updateClusterStateMetadata(Metadata metadata) { long duration = System.currentTimeMillis() - start; log.debug("Updating DlsFlsProcessedConfig took {} ms", duration); + this.metadataVersionEffective = metadata.version(); } @Override From 136972652e56caae56fe77ec074616e3f40ef90f Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Wed, 27 Nov 2024 16:29:30 -0500 Subject: [PATCH 5/7] Match action name Signed-off-by: Craig Perkins --- .../opensearch/security/configuration/DlsFlsValveImpl.java | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java b/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java index 1afb0f36b8..ede36819ed 100644 --- a/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java +++ b/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java @@ -38,6 +38,7 @@ import org.opensearch.action.bulk.BulkItemRequest; import org.opensearch.action.bulk.BulkShardRequest; import org.opensearch.action.search.SearchRequest; +import org.opensearch.action.update.UpdateAction; import org.opensearch.action.update.UpdateRequest; import org.opensearch.client.Client; import org.opensearch.cluster.metadata.IndexNameExpressionResolver; @@ -135,10 +136,6 @@ public DlsFlsValveImpl( */ @Override public boolean invoke(PrivilegesEvaluationContext context, final ActionListener listener) { - if (!context.getAction().startsWith("indices:")) { - return true; - } - DlsFlsProcessedConfig config = this.dlsFlsProcessedConfig.get(); ActionRequest request = context.getRequest(); IndexResolverReplacer.Resolved resolved = context.getResolvedRequest(); @@ -268,7 +265,7 @@ public boolean invoke(PrivilegesEvaluationContext context, final ActionListener< } } - if (request instanceof UpdateRequest) { + if (UpdateAction.NAME.equals(context.getAction())) { listener.onFailure(new OpenSearchSecurityException("Update is not supported when FLS or DLS or Fieldmasking is activated")); return false; } From e133b399a9f43b301d348c59e4704c2fd5a40fd5 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 3 Dec 2024 16:15:08 -0500 Subject: [PATCH 6/7] Assert no active threads Signed-off-by: Craig Perkins --- .../cluster/LocalOpenSearchCluster.java | 27 +++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/src/integrationTest/java/org/opensearch/test/framework/cluster/LocalOpenSearchCluster.java b/src/integrationTest/java/org/opensearch/test/framework/cluster/LocalOpenSearchCluster.java index 8570c3d398..d54bbe0f10 100644 --- a/src/integrationTest/java/org/opensearch/test/framework/cluster/LocalOpenSearchCluster.java +++ b/src/integrationTest/java/org/opensearch/test/framework/cluster/LocalOpenSearchCluster.java @@ -55,6 +55,9 @@ import org.apache.logging.log4j.Logger; import org.opensearch.action.admin.cluster.health.ClusterHealthResponse; +import org.opensearch.action.admin.cluster.node.stats.NodeStats; +import org.opensearch.action.admin.cluster.node.stats.NodesStatsRequest; +import org.opensearch.action.admin.cluster.node.stats.NodesStatsResponse; import org.opensearch.client.AdminClient; import org.opensearch.client.Client; import org.opensearch.cluster.health.ClusterHealthStatus; @@ -67,6 +70,7 @@ import org.opensearch.plugins.Plugin; import org.opensearch.test.framework.certificate.TestCertificates; import org.opensearch.test.framework.cluster.ClusterManager.NodeSettings; +import org.opensearch.threadpool.ThreadPoolStats; import org.opensearch.transport.BindTransportException; import static java.util.Objects.requireNonNull; @@ -219,7 +223,26 @@ public boolean isStarted() { return started; } - public void stop() { + public void stop() throws IOException { + Client client = clientNode().getInternalNodeClient(); + AdminClient adminClient = client.admin(); + + final NodesStatsResponse nodesStatsResponse = adminClient.cluster() + .nodesStats(new NodesStatsRequest().addMetric(NodesStatsRequest.Metric.THREAD_POOL.metricName())) + .actionGet(); + + for (NodeStats stats : nodesStatsResponse.getNodes()) { + for (ThreadPoolStats.Stats stat : requireNonNull(stats.getThreadPool())) { + if ("management".equals(stat.getName())) { + continue; + } + if (stat.getActive() > 0) { + log.warn("Thread pool {} has {} active threads", stat.getName(), stat.getActive()); + throw new IOException("Thread pool " + stat.getName() + " has " + stat.getActive() + " active threads"); + } + } + } + List> stopFutures = new ArrayList<>(); for (Node node : nodes) { stopFutures.add(node.stop(2, TimeUnit.SECONDS)); @@ -227,7 +250,7 @@ public void stop() { CompletableFuture.allOf(stopFutures.toArray(CompletableFuture[]::new)).join(); } - public void destroy() { + public void destroy() throws IOException { try { stop(); nodes.clear(); From d24ca832384fa6d7fe518bb36d306fcb23b2a542 Mon Sep 17 00:00:00 2001 From: Craig Perkins Date: Tue, 3 Dec 2024 17:12:59 -0500 Subject: [PATCH 7/7] Keep oldConfig Signed-off-by: Craig Perkins --- .../opensearch/security/configuration/DlsFlsValveImpl.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java b/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java index ede36819ed..abe65a717a 100644 --- a/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java +++ b/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java @@ -680,7 +680,7 @@ static Mode get(Settings settings) { public void updateConfiguration(SecurityDynamicConfiguration rolesConfiguration) { try { if (rolesConfiguration != null) { - this.dlsFlsProcessedConfig.set( + DlsFlsProcessedConfig oldConfig = this.dlsFlsProcessedConfig.getAndSet( new DlsFlsProcessedConfig( DynamicConfigFactory.addStatics(rolesConfiguration.clone()), clusterService.state().metadata().getIndicesLookup(), @@ -689,6 +689,10 @@ public void updateConfiguration(SecurityDynamicConfiguration rolesConfig fieldMaskingConfig ) ); + + if (oldConfig != null) { + oldConfig.shutdown(); + } } } catch (Exception e) { log.error("Error while updating DLS/FLS configuration with {}", rolesConfiguration, e);