From 5782d93e7d130bddffcb89de870fe4b7b2278204 Mon Sep 17 00:00:00 2001 From: William Brafford Date: Tue, 26 Jan 2021 13:40:14 -0500 Subject: [PATCH 1/6] Add breaking test --- .../20_destructive_wildcard.yml | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 rest-api-spec/src/main/resources/rest-api-spec/test/indices.delete/20_destructive_wildcard.yml diff --git a/rest-api-spec/src/main/resources/rest-api-spec/test/indices.delete/20_destructive_wildcard.yml b/rest-api-spec/src/main/resources/rest-api-spec/test/indices.delete/20_destructive_wildcard.yml new file mode 100644 index 0000000000000..b1fb9b3b333be --- /dev/null +++ b/rest-api-spec/src/main/resources/rest-api-spec/test/indices.delete/20_destructive_wildcard.yml @@ -0,0 +1,22 @@ +--- +setup: + - do: + cluster.put_settings: + body: + transient: + action.destructive_requires_name: "true" + flat_settings: true +--- +teardown: + - do: + cluster.put_settings: + body: + transient: + action.destructive_requires_name: "false" + flat_settings: true +--- +"Delete nonexistent concrete index with wildcard expansion disallowed": + - do: + indices.delete: + index: index3 + ignore_unavailable: true From ef96cd004721dfb9188f6bcb60466dbddd0ebcbb Mon Sep 17 00:00:00 2001 From: William Brafford Date: Tue, 26 Jan 2021 13:41:08 -0500 Subject: [PATCH 2/6] Allow *,-* pattern in destructive actions Since the "*,-*" pattern resolves to "no indices", it makes a normally destructive action into a non-destructive one. Rather than throwing a wildcards-not-allowed exception, we can allow this pattern to pass without triggering an exception. This allows the security layer to safely use a "*,-*" pattern to indicate a "no indices" result for its index resolution step, which is important because otherwise we get wildcards-not-allowed exceptions when trying to delete nonexistent concrete indices. --- .../action/support/DestructiveOperations.java | 12 +++++++++++- .../action/filter/DestructiveOperationsTests.java | 6 ++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/server/src/main/java/org/elasticsearch/action/support/DestructiveOperations.java b/server/src/main/java/org/elasticsearch/action/support/DestructiveOperations.java index 583c34e09641a..57884511417ae 100644 --- a/server/src/main/java/org/elasticsearch/action/support/DestructiveOperations.java +++ b/server/src/main/java/org/elasticsearch/action/support/DestructiveOperations.java @@ -60,11 +60,21 @@ public void failDestructive(String[] aliasesOrIndices) { throw new IllegalArgumentException("Wildcard expressions or all indices are not allowed"); } } else { + boolean matchNoneFound = false; for (String aliasesOrIndex : aliasesOrIndices) { if (hasWildcardUsage(aliasesOrIndex)) { - throw new IllegalArgumentException("Wildcard expressions or all indices are not allowed"); + if (aliasesOrIndex.equals("-*")) { + matchNoneFound = true; + } else if (aliasesOrIndex.equals("*")) { + // do nothing + } else { + throw new IllegalArgumentException("Wildcard expressions or all indices are not allowed"); + } } } + if (matchNoneFound == false) { + throw new IllegalArgumentException("Wildcard expressions or all indices are not allowed"); + } } } diff --git a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/action/filter/DestructiveOperationsTests.java b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/action/filter/DestructiveOperationsTests.java index c0b83f9fc6ea7..ff6b74f3bf642 100644 --- a/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/action/filter/DestructiveOperationsTests.java +++ b/x-pack/plugin/security/src/internalClusterTest/java/org/elasticsearch/xpack/security/action/filter/DestructiveOperationsTests.java @@ -49,6 +49,8 @@ public void testDeleteIndexDestructiveOperationsRequireName() { assertEquals("index1", indices[0]); } + // the "*,-*" pattern is specially handled because it makes a destructive action non-destructive + assertAcked(client().admin().indices().prepareDelete("*", "-*")); assertAcked(client().admin().indices().prepareDelete("index1")); } @@ -114,6 +116,10 @@ public void testOpenCloseIndexDestructiveOperationsRequireName() { assertEquals("Wildcard expressions or all indices are not allowed", illegalArgumentException.getMessage()); } + // the "*,-*" pattern is specially handled because it makes a destructive action non-destructive + assertAcked(client().admin().indices().prepareClose("*", "-*")); + assertAcked(client().admin().indices().prepareOpen("*", "-*")); + createIndex("index1"); assertAcked(client().admin().indices().prepareClose("index1")); assertAcked(client().admin().indices().prepareOpen("index1")); From 9cd643ae306d5604e9a32ff8a67b441b460497a7 Mon Sep 17 00:00:00 2001 From: William Brafford Date: Wed, 27 Jan 2021 11:22:46 -0500 Subject: [PATCH 3/6] Check for exactly "*,-*", where order matters --- .../action/support/DestructiveOperations.java | 17 ++-- .../support/DestructiveOperationsTest.java | 93 +++++++++++++++++++ 2 files changed, 99 insertions(+), 11 deletions(-) create mode 100644 server/src/test/java/org/elasticsearch/action/support/DestructiveOperationsTest.java diff --git a/server/src/main/java/org/elasticsearch/action/support/DestructiveOperations.java b/server/src/main/java/org/elasticsearch/action/support/DestructiveOperations.java index 57884511417ae..66e15e35445cf 100644 --- a/server/src/main/java/org/elasticsearch/action/support/DestructiveOperations.java +++ b/server/src/main/java/org/elasticsearch/action/support/DestructiveOperations.java @@ -24,6 +24,8 @@ import org.elasticsearch.common.settings.Setting.Property; import org.elasticsearch.common.settings.Settings; +import java.util.Arrays; + /** * Helper for dealing with destructive operations and wildcard usage. */ @@ -59,22 +61,15 @@ public void failDestructive(String[] aliasesOrIndices) { if (hasWildcardUsage(aliasesOrIndices[0])) { throw new IllegalArgumentException("Wildcard expressions or all indices are not allowed"); } + } else if (Arrays.equals(aliasesOrIndices, new String[]{"*", "-*"})) { + // do nothing, allowing the use of the "matchNone" pattern, "*,-*", + // which will never actually be destructive as it operates on no indices } else { - boolean matchNoneFound = false; for (String aliasesOrIndex : aliasesOrIndices) { if (hasWildcardUsage(aliasesOrIndex)) { - if (aliasesOrIndex.equals("-*")) { - matchNoneFound = true; - } else if (aliasesOrIndex.equals("*")) { - // do nothing - } else { - throw new IllegalArgumentException("Wildcard expressions or all indices are not allowed"); - } + throw new IllegalArgumentException("Wildcard expressions or all indices are not allowed"); } } - if (matchNoneFound == false) { - throw new IllegalArgumentException("Wildcard expressions or all indices are not allowed"); - } } } diff --git a/server/src/test/java/org/elasticsearch/action/support/DestructiveOperationsTest.java b/server/src/test/java/org/elasticsearch/action/support/DestructiveOperationsTest.java new file mode 100644 index 0000000000000..17861a61b8a37 --- /dev/null +++ b/server/src/test/java/org/elasticsearch/action/support/DestructiveOperationsTest.java @@ -0,0 +1,93 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.action.support; + +import org.elasticsearch.common.settings.ClusterSettings; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.test.ESTestCase; + +import java.util.Set; + +import static org.hamcrest.Matchers.equalTo; + +public class DestructiveOperationsTest extends ESTestCase { + + private DestructiveOperations destructiveOperations; + + @Override + public void setUp() throws Exception { + super.setUp(); + Settings nodeSettings = Settings.builder() + .put(DestructiveOperations.REQUIRES_NAME_SETTING.getKey(), "true") + .build(); + destructiveOperations = new DestructiveOperations( + nodeSettings, + new ClusterSettings(nodeSettings, Set.of(DestructiveOperations.REQUIRES_NAME_SETTING))); + } + + public void testDestructive() { + { + // requests that might resolve to all indices + assertFailsDestructive(null); + assertFailsDestructive(new String[]{}); + assertFailsDestructive(new String[]{"_all"}); + assertFailsDestructive(new String[]{"*"}); + } + { + // various wildcards + assertFailsDestructive(new String[] {"-*"}); + assertFailsDestructive(new String[] {"index*"}); + assertFailsDestructive(new String[] {"index", "*"}); + assertFailsDestructive(new String[] {"index", "-*"}); + assertFailsDestructive(new String[] {"index", "test-*-index"}); + } + { + // near versions of the "matchNone" pattern + assertFailsDestructive(new String[]{"-*", "*"}); + assertFailsDestructive(new String[]{"*", "-*", "*"}); + } + } + + /** + * Test that non-wildcard expressions or the special "*,-*" don't throw an + * exception. Since {@link DestructiveOperations#failDestructive(String[])} + * has no return value, we run the statements without asserting anything + * about them. + */ + public void testNonDestructive() { + { + // no wildcards + destructiveOperations.failDestructive(new String[]{"index"}); + destructiveOperations.failDestructive(new String[]{"index", "-index2"}); + } + { + // special "matchNone" pattern + destructiveOperations.failDestructive(new String[]{"*", "-*"}); + } + } + + private void assertFailsDestructive(String[] aliasesOrIndices) { + IllegalArgumentException e = expectThrows( + IllegalArgumentException.class, + () -> destructiveOperations.failDestructive(aliasesOrIndices)); + + assertThat(e.getMessage(), equalTo("Wildcard expressions or all indices are not allowed")); + } +} From b7bd9f44f9f9940e5d1c6a1cbb0bb6f0266e0c20 Mon Sep 17 00:00:00 2001 From: William Brafford Date: Wed, 27 Jan 2021 14:12:53 -0500 Subject: [PATCH 4/6] Add integration testing for match-none in destructive operations --- .../DestructiveOperationsIT.java | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/server/src/internalClusterTest/java/org/elasticsearch/operateAllIndices/DestructiveOperationsIT.java b/server/src/internalClusterTest/java/org/elasticsearch/operateAllIndices/DestructiveOperationsIT.java index 356e012ff166e..2239e2cf866c7 100644 --- a/server/src/internalClusterTest/java/org/elasticsearch/operateAllIndices/DestructiveOperationsIT.java +++ b/server/src/internalClusterTest/java/org/elasticsearch/operateAllIndices/DestructiveOperationsIT.java @@ -27,6 +27,7 @@ import org.elasticsearch.test.ESIntegTestCase; import org.junit.After; +import static org.elasticsearch.cluster.metadata.IndexMetadata.APIBlock.WRITE; import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked; import static org.hamcrest.Matchers.equalTo; @@ -48,6 +49,8 @@ public void testDeleteIndexIsRejected() throws Exception { // Should succeed, since no wildcards assertAcked(client().admin().indices().prepareDelete("1index").get()); + // Special "match none" pattern succeeds, since non-destructive + assertAcked(client().admin().indices().prepareDelete("*", "-*").get()); expectThrows(IllegalArgumentException.class, () -> client().admin().indices().prepareDelete("i*").get()); expectThrows(IllegalArgumentException.class, () -> client().admin().indices().prepareDelete("_all").get()); @@ -82,6 +85,8 @@ public void testCloseIndexIsRejected() throws Exception { // Should succeed, since no wildcards assertAcked(client().admin().indices().prepareClose("1index").get()); + // Special "match none" pattern succeeds, since non-destructive + assertAcked(client().admin().indices().prepareClose("*", "-*").get()); expectThrows(IllegalArgumentException.class, () -> client().admin().indices().prepareClose("i*").get()); expectThrows(IllegalArgumentException.class, () -> client().admin().indices().prepareClose("_all").get()); @@ -118,6 +123,9 @@ public void testOpenIndexIsRejected() throws Exception { createIndex("index1", "1index"); assertAcked(client().admin().indices().prepareClose("1index", "index1").get()); + // Special "match none" pattern succeeds, since non-destructive + assertAcked(client().admin().indices().prepareOpen("*", "-*").get()); + expectThrows(IllegalArgumentException.class, () -> client().admin().indices().prepareOpen("i*").get()); expectThrows(IllegalArgumentException.class, () -> client().admin().indices().prepareOpen("_all").get()); } @@ -144,4 +152,46 @@ public void testOpenIndexDefaultBehaviour() throws Exception { assertEquals(IndexMetadata.State.OPEN, indexMetadataObjectObjectCursor.value.getState()); } } + + public void testAddIndexBlockIsRejected() throws Exception { + Settings settings = Settings.builder() + .put(DestructiveOperations.REQUIRES_NAME_SETTING.getKey(), true) + .build(); + assertAcked(client().admin().cluster().prepareUpdateSettings().setTransientSettings(settings)); + + createIndex("index1", "1index"); + + // Should succeed, since no wildcards + assertAcked(client().admin().indices().prepareAddBlock(WRITE,"1index").get()); + // Special "match none" pattern succeeds, since non-destructive + assertAcked(client().admin().indices().prepareAddBlock(WRITE,"*", "-*").get()); + + expectThrows(IllegalArgumentException.class, + () -> client().admin().indices().prepareAddBlock(WRITE,"i*").get()); + expectThrows(IllegalArgumentException.class, + () -> client().admin().indices().prepareAddBlock(WRITE, "_all").get()); + } + + public void testAddIndexBlockDefaultBehaviour() throws Exception { + if (randomBoolean()) { + Settings settings = Settings.builder() + .put(DestructiveOperations.REQUIRES_NAME_SETTING.getKey(), false) + .build(); + assertAcked(client().admin().cluster().prepareUpdateSettings().setTransientSettings(settings)); + } + + createIndex("index1", "1index"); + + if (randomBoolean()) { + assertAcked(client().admin().indices().prepareAddBlock(WRITE, "_all").get()); + } else { + assertAcked(client().admin().indices().prepareAddBlock(WRITE, "*").get()); + } + + ClusterState state = client().admin().cluster().prepareState().get().getState(); + assertTrue("write block is set on index1", + state.getBlocks().hasIndexBlock("index1", IndexMetadata.INDEX_WRITE_BLOCK)); + assertTrue("write block is set on 1index", + state.getBlocks().hasIndexBlock("1index", IndexMetadata.INDEX_WRITE_BLOCK)); + } } From e7eb32805ac6123c4f7e58af404718470e1367f1 Mon Sep 17 00:00:00 2001 From: William Brafford Date: Wed, 27 Jan 2021 14:30:09 -0500 Subject: [PATCH 5/6] Rename class for testing conventions --- ...ctiveOperationsTest.java => DestructiveOperationsTests.java} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename server/src/test/java/org/elasticsearch/action/support/{DestructiveOperationsTest.java => DestructiveOperationsTests.java} (98%) diff --git a/server/src/test/java/org/elasticsearch/action/support/DestructiveOperationsTest.java b/server/src/test/java/org/elasticsearch/action/support/DestructiveOperationsTests.java similarity index 98% rename from server/src/test/java/org/elasticsearch/action/support/DestructiveOperationsTest.java rename to server/src/test/java/org/elasticsearch/action/support/DestructiveOperationsTests.java index 17861a61b8a37..7b5b1f12fa62c 100644 --- a/server/src/test/java/org/elasticsearch/action/support/DestructiveOperationsTest.java +++ b/server/src/test/java/org/elasticsearch/action/support/DestructiveOperationsTests.java @@ -27,7 +27,7 @@ import static org.hamcrest.Matchers.equalTo; -public class DestructiveOperationsTest extends ESTestCase { +public class DestructiveOperationsTests extends ESTestCase { private DestructiveOperations destructiveOperations; From 8546587b091c3eb9f5730515b40c26fc2618e3c1 Mon Sep 17 00:00:00 2001 From: William Brafford Date: Thu, 28 Jan 2021 09:42:50 -0500 Subject: [PATCH 6/6] Simplify logic and make String array a constant --- .../action/support/DestructiveOperations.java | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/server/src/main/java/org/elasticsearch/action/support/DestructiveOperations.java b/server/src/main/java/org/elasticsearch/action/support/DestructiveOperations.java index 66e15e35445cf..bcc47f16200f7 100644 --- a/server/src/main/java/org/elasticsearch/action/support/DestructiveOperations.java +++ b/server/src/main/java/org/elasticsearch/action/support/DestructiveOperations.java @@ -36,6 +36,15 @@ public final class DestructiveOperations { */ public static final Setting REQUIRES_NAME_SETTING = Setting.boolSetting("action.destructive_requires_name", false, Property.Dynamic, Property.NodeScope); + /** + * The "match none" pattern, "*,-*", will never actually be destructive + * because it operates on no indices. If plugins or other components add + * their own index resolution layers, they can pass this pattern to the + * core code in order to indicate that an operation won't run on any + * indices, relying on the core code to handle this situation. + */ + private static final String[] MATCH_NONE_PATTERN = {"*", "-*"}; + private volatile boolean destructiveRequiresName; public DestructiveOperations(Settings settings, ClusterSettings clusterSettings) { @@ -61,10 +70,7 @@ public void failDestructive(String[] aliasesOrIndices) { if (hasWildcardUsage(aliasesOrIndices[0])) { throw new IllegalArgumentException("Wildcard expressions or all indices are not allowed"); } - } else if (Arrays.equals(aliasesOrIndices, new String[]{"*", "-*"})) { - // do nothing, allowing the use of the "matchNone" pattern, "*,-*", - // which will never actually be destructive as it operates on no indices - } else { + } else if (Arrays.equals(aliasesOrIndices, MATCH_NONE_PATTERN) == false) { for (String aliasesOrIndex : aliasesOrIndices) { if (hasWildcardUsage(aliasesOrIndex)) { throw new IllegalArgumentException("Wildcard expressions or all indices are not allowed");