From e57fae6aa5043f6be6f506ea6066d3211129520d Mon Sep 17 00:00:00 2001 From: Michael Basnight Date: Tue, 25 Jun 2019 10:11:29 -0500 Subject: [PATCH 1/2] Validate read priv of enrich source indices This commit adds permissions validation on the indices provided in the enrich policy. These indices should be validated at store time so as not to have cryptic error messages in the event the user does not have permissions to access said indices. --- .../core/security/authz/RoleDescriptor.java | 4 ++ .../xpack/enrich/EnrichSecurityFailureIT.java | 2 +- .../xpack/enrich/EnrichSecurityIT.java | 13 +++++ .../xpack/enrich/EnrichPlugin.java | 4 ++ .../TransportPutEnrichPolicyAction.java | 58 +++++++++++++++++-- .../xpack/enrich/EnrichMultiNodeIT.java | 2 +- .../xpack/enrich/EnrichPolicyUpdateTests.java | 4 +- .../xpack/enrich/LocalStateEnrich.java | 26 +++++++++ 8 files changed, 104 insertions(+), 9 deletions(-) create mode 100644 x-pack/plugin/enrich/src/test/java/org/elasticsearch/xpack/enrich/LocalStateEnrich.java diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/RoleDescriptor.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/RoleDescriptor.java index 15304ff85dbd9..4fa171cf6247d 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/RoleDescriptor.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authz/RoleDescriptor.java @@ -761,6 +761,10 @@ public Builder indices(String... indices) { return this; } + public Builder indices(Collection indices) { + return indices(indices.toArray(new String[indices.size()])); + } + public Builder privileges(String... privileges) { indicesPrivileges.privileges = privileges; return this; diff --git a/x-pack/plugin/enrich/qa/rest-with-security/src/test/java/org/elasticsearch/xpack/enrich/EnrichSecurityFailureIT.java b/x-pack/plugin/enrich/qa/rest-with-security/src/test/java/org/elasticsearch/xpack/enrich/EnrichSecurityFailureIT.java index 198b2a254993d..4dcc9886438fa 100644 --- a/x-pack/plugin/enrich/qa/rest-with-security/src/test/java/org/elasticsearch/xpack/enrich/EnrichSecurityFailureIT.java +++ b/x-pack/plugin/enrich/qa/rest-with-security/src/test/java/org/elasticsearch/xpack/enrich/EnrichSecurityFailureIT.java @@ -36,7 +36,7 @@ public void testFailure() { Request putPolicyRequest = new Request("PUT", "/_enrich/policy/my_policy"); putPolicyRequest.setJsonEntity("{\"type\": \"exact_match\",\"indices\": [\"my-source-index\"], \"enrich_key\": \"host\", " + "\"enrich_values\": [\"globalRank\", \"tldRank\", \"tld\"]}"); - ResponseException exc = expectThrows(ResponseException.class, () -> assertOK(client().performRequest(putPolicyRequest))); + ResponseException exc = expectThrows(ResponseException.class, () -> client().performRequest(putPolicyRequest)); assertTrue(exc.getMessage().contains("action [cluster:admin/xpack/enrich/put] is unauthorized for user [test_enrich_no_privs]")); } } diff --git a/x-pack/plugin/enrich/qa/rest-with-security/src/test/java/org/elasticsearch/xpack/enrich/EnrichSecurityIT.java b/x-pack/plugin/enrich/qa/rest-with-security/src/test/java/org/elasticsearch/xpack/enrich/EnrichSecurityIT.java index be99ca893efd7..23b1c94678151 100644 --- a/x-pack/plugin/enrich/qa/rest-with-security/src/test/java/org/elasticsearch/xpack/enrich/EnrichSecurityIT.java +++ b/x-pack/plugin/enrich/qa/rest-with-security/src/test/java/org/elasticsearch/xpack/enrich/EnrichSecurityIT.java @@ -5,6 +5,8 @@ */ package org.elasticsearch.xpack.enrich; +import org.elasticsearch.client.Request; +import org.elasticsearch.client.ResponseException; import org.elasticsearch.common.settings.SecureString; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.util.concurrent.ThreadContext; @@ -29,4 +31,15 @@ protected Settings restAdminSettings() { .put(ThreadContext.PREFIX + ".Authorization", token) .build(); } + + public void testInsufficientPermissionsOnNonExistentIndex() { + // This test is here because it requires a valid user that has permission to execute policy PUTs but should fail if the user + // does not have access to read the backing indices used to enrich the data. + Request putPolicyRequest = new Request("PUT", "/_enrich/policy/my_policy"); + putPolicyRequest.setJsonEntity("{\"type\": \"exact_match\",\"indices\": [\"some-other-index\"], \"enrich_key\": \"host\", " + + "\"enrich_values\": [\"globalRank\", \"tldRank\", \"tld\"]}"); + ResponseException exc = expectThrows(ResponseException.class, () -> client().performRequest(putPolicyRequest)); + assertTrue(exc.getMessage().contains("Could not store policy because an index specified [some-other-index] did not exist or user" + + " test_enrich lacks permission to access it.")); + } } diff --git a/x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/EnrichPlugin.java b/x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/EnrichPlugin.java index 2e77065264e49..7567148898cd3 100644 --- a/x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/EnrichPlugin.java +++ b/x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/EnrichPlugin.java @@ -24,6 +24,7 @@ import org.elasticsearch.env.Environment; import org.elasticsearch.env.NodeEnvironment; import org.elasticsearch.ingest.Processor; +import org.elasticsearch.license.XPackLicenseState; import org.elasticsearch.plugins.ActionPlugin; import org.elasticsearch.plugins.IngestPlugin; import org.elasticsearch.plugins.Plugin; @@ -32,6 +33,7 @@ import org.elasticsearch.script.ScriptService; import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.watcher.ResourceWatcherService; +import org.elasticsearch.xpack.core.XPackPlugin; import org.elasticsearch.xpack.core.enrich.action.DeleteEnrichPolicyAction; import org.elasticsearch.xpack.core.enrich.action.ExecuteEnrichPolicyAction; import org.elasticsearch.xpack.core.enrich.action.GetEnrichPolicyAction; @@ -75,6 +77,8 @@ public Map getProcessors(Processor.Parameters paramet return Map.of(EnrichProcessorFactory.TYPE, factory); } + protected XPackLicenseState getLicenseState() { return XPackPlugin.getSharedLicenseState(); } + public List> getActions() { if (enabled == false) { return List.of(); diff --git a/x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/action/TransportPutEnrichPolicyAction.java b/x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/action/TransportPutEnrichPolicyAction.java index 9c0c428c32d0b..95a68d3944d54 100644 --- a/x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/action/TransportPutEnrichPolicyAction.java +++ b/x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/action/TransportPutEnrichPolicyAction.java @@ -9,30 +9,48 @@ import org.elasticsearch.action.support.ActionFilters; import org.elasticsearch.action.support.master.AcknowledgedResponse; import org.elasticsearch.action.support.master.TransportMasterNodeAction; +import org.elasticsearch.client.Client; import org.elasticsearch.cluster.ClusterState; import org.elasticsearch.cluster.block.ClusterBlockException; import org.elasticsearch.cluster.block.ClusterBlockLevel; import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver; import org.elasticsearch.cluster.service.ClusterService; +import org.elasticsearch.common.Strings; import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.io.stream.StreamInput; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.license.XPackLicenseState; import org.elasticsearch.threadpool.ThreadPool; import org.elasticsearch.transport.TransportService; +import org.elasticsearch.xpack.core.XPackSettings; import org.elasticsearch.xpack.core.enrich.action.PutEnrichPolicyAction; +import org.elasticsearch.xpack.core.security.SecurityContext; +import org.elasticsearch.xpack.core.security.action.user.HasPrivilegesAction; +import org.elasticsearch.xpack.core.security.action.user.HasPrivilegesRequest; +import org.elasticsearch.xpack.core.security.action.user.HasPrivilegesResponse; +import org.elasticsearch.xpack.core.security.authz.RoleDescriptor; +import org.elasticsearch.xpack.core.security.support.Exceptions; import org.elasticsearch.xpack.enrich.EnrichStore; import java.io.IOException; public class TransportPutEnrichPolicyAction extends TransportMasterNodeAction { + private final XPackLicenseState licenseState; + private final SecurityContext securityContext; + private final Client client; + @Inject - public TransportPutEnrichPolicyAction(TransportService transportService, - ClusterService clusterService, - ThreadPool threadPool, - ActionFilters actionFilters, + public TransportPutEnrichPolicyAction(Settings settings, TransportService transportService, + ClusterService clusterService, ThreadPool threadPool, Client client, + XPackLicenseState licenseState, ActionFilters actionFilters, IndexNameExpressionResolver indexNameExpressionResolver) { super(PutEnrichPolicyAction.NAME, transportService, clusterService, threadPool, actionFilters, PutEnrichPolicyAction.Request::new, indexNameExpressionResolver); + this.licenseState = licenseState; + this.securityContext = XPackSettings.SECURITY_ENABLED.get(settings) ? + new SecurityContext(settings, threadPool.getThreadContext()) : null; + this.client = client; } @Override @@ -52,6 +70,38 @@ protected AcknowledgedResponse read(StreamInput in) throws IOException { @Override protected void masterOperation(PutEnrichPolicyAction.Request request, ClusterState state, ActionListener listener) { + + if (licenseState.isAuthAllowed()) { + RoleDescriptor.IndicesPrivileges privileges = RoleDescriptor.IndicesPrivileges.builder() + .indices(request.getPolicy().getIndices()) + .privileges("read") + .build(); + + String username = securityContext.getUser().principal(); + + HasPrivilegesRequest privRequest = new HasPrivilegesRequest(); + privRequest.applicationPrivileges(new RoleDescriptor.ApplicationResourcePrivileges[0]); + privRequest.username(username); + privRequest.clusterPrivileges(Strings.EMPTY_ARRAY); + privRequest.indexPrivileges(privileges); + + ActionListener wrappedListener = ActionListener.wrap( + r -> { + if (r.isCompleteMatch()) { + putPolicy(request, listener); + } else { + listener.onFailure(Exceptions.authorizationError("Could not store policy because an index specified {} did not" + + " exist or user {} lacks permission to access it.", request.getPolicy().getIndices(), username)); + } + }, + listener::onFailure); + client.execute(HasPrivilegesAction.INSTANCE, privRequest, wrappedListener); + } else { + putPolicy(request, listener); + } + } + + private void putPolicy(PutEnrichPolicyAction.Request request, ActionListener listener ) { EnrichStore.putPolicy(request.getName(), request.getPolicy(), clusterService, e -> { if (e == null) { listener.onResponse(new AcknowledgedResponse(true)); diff --git a/x-pack/plugin/enrich/src/test/java/org/elasticsearch/xpack/enrich/EnrichMultiNodeIT.java b/x-pack/plugin/enrich/src/test/java/org/elasticsearch/xpack/enrich/EnrichMultiNodeIT.java index 5503ddf2884bb..20eccccbd91b7 100644 --- a/x-pack/plugin/enrich/src/test/java/org/elasticsearch/xpack/enrich/EnrichMultiNodeIT.java +++ b/x-pack/plugin/enrich/src/test/java/org/elasticsearch/xpack/enrich/EnrichMultiNodeIT.java @@ -50,7 +50,7 @@ public class EnrichMultiNodeIT extends ESIntegTestCase { @Override protected Collection> nodePlugins() { - return List.of(EnrichPlugin.class, ReindexPlugin.class); + return List.of(LocalStateEnrich.class, ReindexPlugin.class); } public void testEnrichAPIs() { diff --git a/x-pack/plugin/enrich/src/test/java/org/elasticsearch/xpack/enrich/EnrichPolicyUpdateTests.java b/x-pack/plugin/enrich/src/test/java/org/elasticsearch/xpack/enrich/EnrichPolicyUpdateTests.java index 64c49836b27e8..ee6388cf61e7b 100644 --- a/x-pack/plugin/enrich/src/test/java/org/elasticsearch/xpack/enrich/EnrichPolicyUpdateTests.java +++ b/x-pack/plugin/enrich/src/test/java/org/elasticsearch/xpack/enrich/EnrichPolicyUpdateTests.java @@ -27,7 +27,7 @@ public class EnrichPolicyUpdateTests extends ESSingleNodeTestCase { @Override protected Collection> getPlugins() { - return List.of(EnrichPlugin.class); + return List.of(LocalStateEnrich.class); } public void testUpdatePolicyOnly() { @@ -56,6 +56,4 @@ public void testUpdatePolicyOnly() { Pipeline pipelineInstance2 = ingestService.getPipeline("1"); assertThat(pipelineInstance2, sameInstance(pipelineInstance1)); } - - } diff --git a/x-pack/plugin/enrich/src/test/java/org/elasticsearch/xpack/enrich/LocalStateEnrich.java b/x-pack/plugin/enrich/src/test/java/org/elasticsearch/xpack/enrich/LocalStateEnrich.java new file mode 100644 index 0000000000000..ca8c429935591 --- /dev/null +++ b/x-pack/plugin/enrich/src/test/java/org/elasticsearch/xpack/enrich/LocalStateEnrich.java @@ -0,0 +1,26 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ +package org.elasticsearch.xpack.enrich; + +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.license.XPackLicenseState; +import org.elasticsearch.xpack.core.LocalStateCompositeXPackPlugin; + +import java.nio.file.Path; + +public class LocalStateEnrich extends LocalStateCompositeXPackPlugin { + + public LocalStateEnrich(final Settings settings, final Path configPath) throws Exception { + super(settings, configPath); + + plugins.add(new EnrichPlugin(settings) { + @Override + protected XPackLicenseState getLicenseState() { + return LocalStateEnrich.this.getLicenseState(); + } + }); + } +} From 8622ef3b19e61b6fb58e59e69fe71789fd61d0dc Mon Sep 17 00:00:00 2001 From: Michael Basnight Date: Thu, 27 Jun 2019 14:14:03 -0500 Subject: [PATCH 2/2] Fix the error msg --- .../org/elasticsearch/xpack/enrich/EnrichSecurityIT.java | 5 +++-- .../xpack/enrich/action/TransportPutEnrichPolicyAction.java | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/x-pack/plugin/enrich/qa/rest-with-security/src/test/java/org/elasticsearch/xpack/enrich/EnrichSecurityIT.java b/x-pack/plugin/enrich/qa/rest-with-security/src/test/java/org/elasticsearch/xpack/enrich/EnrichSecurityIT.java index 23b1c94678151..87a7c7bb48dc0 100644 --- a/x-pack/plugin/enrich/qa/rest-with-security/src/test/java/org/elasticsearch/xpack/enrich/EnrichSecurityIT.java +++ b/x-pack/plugin/enrich/qa/rest-with-security/src/test/java/org/elasticsearch/xpack/enrich/EnrichSecurityIT.java @@ -13,6 +13,7 @@ import org.elasticsearch.test.enrich.CommonEnrichRestTestCase; import static org.elasticsearch.xpack.core.security.authc.support.UsernamePasswordToken.basicAuthHeaderValue; +import static org.hamcrest.CoreMatchers.containsString; public class EnrichSecurityIT extends CommonEnrichRestTestCase { @@ -39,7 +40,7 @@ public void testInsufficientPermissionsOnNonExistentIndex() { putPolicyRequest.setJsonEntity("{\"type\": \"exact_match\",\"indices\": [\"some-other-index\"], \"enrich_key\": \"host\", " + "\"enrich_values\": [\"globalRank\", \"tldRank\", \"tld\"]}"); ResponseException exc = expectThrows(ResponseException.class, () -> client().performRequest(putPolicyRequest)); - assertTrue(exc.getMessage().contains("Could not store policy because an index specified [some-other-index] did not exist or user" + - " test_enrich lacks permission to access it.")); + assertThat(exc.getMessage(), + containsString("unable to store policy because no indices match with the specified index patterns [some-other-index]")); } } diff --git a/x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/action/TransportPutEnrichPolicyAction.java b/x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/action/TransportPutEnrichPolicyAction.java index 95a68d3944d54..fbfdd1aaad53e 100644 --- a/x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/action/TransportPutEnrichPolicyAction.java +++ b/x-pack/plugin/enrich/src/main/java/org/elasticsearch/xpack/enrich/action/TransportPutEnrichPolicyAction.java @@ -90,8 +90,8 @@ protected void masterOperation(PutEnrichPolicyAction.Request request, ClusterSta if (r.isCompleteMatch()) { putPolicy(request, listener); } else { - listener.onFailure(Exceptions.authorizationError("Could not store policy because an index specified {} did not" + - " exist or user {} lacks permission to access it.", request.getPolicy().getIndices(), username)); + listener.onFailure(Exceptions.authorizationError("unable to store policy because no indices match with the " + + "specified index patterns {}", request.getPolicy().getIndices(), username)); } }, listener::onFailure);