From 6e9da227a273144c0c829dd8be99a8348d26b618 Mon Sep 17 00:00:00 2001
From: Lee Hinman <lee@writequit.org>
Date: Mon, 4 May 2020 15:05:46 -0600
Subject: [PATCH 1/3] Validate V2 templates more strictly

This commit changes the validation for V2 index and component templates to re-use the same
validation that V1 templates used. This includes things like invalid template names, index patterns,
etc.

This also adds validation that template names do not contain `*`.

Supercedes #53970
Relates to #53101
---
 .../TransportSimulateIndexTemplateAction.java |   3 +-
 .../metadata/MetadataCreateIndexService.java  |   2 +-
 .../MetadataIndexTemplateService.java         | 118 +++++++++++++-----
 .../MetadataIndexTemplateServiceTests.java    |  16 +--
 .../cluster/shards/ClusterShardLimitIT.java   |   2 +-
 5 files changed, 100 insertions(+), 41 deletions(-)

diff --git a/server/src/main/java/org/elasticsearch/action/admin/indices/template/post/TransportSimulateIndexTemplateAction.java b/server/src/main/java/org/elasticsearch/action/admin/indices/template/post/TransportSimulateIndexTemplateAction.java
index 0bc91c9c0276a..746da562dee42 100644
--- a/server/src/main/java/org/elasticsearch/action/admin/indices/template/post/TransportSimulateIndexTemplateAction.java
+++ b/server/src/main/java/org/elasticsearch/action/admin/indices/template/post/TransportSimulateIndexTemplateAction.java
@@ -57,6 +57,7 @@
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
+import java.util.Locale;
 import java.util.Map;
 import java.util.Set;
 import java.util.function.Function;
@@ -106,7 +107,7 @@ protected void masterOperation(Task task, SimulateIndexTemplateRequest request,
         ClusterState simulateOnClusterState = state;
         if (request.getIndexTemplateRequest() != null) {
             // we'll "locally" add the template defined by the user in the cluster state (as if it existed in the system)
-            String simulateTemplateToAdd = "simulate_new_template_" + UUIDs.randomBase64UUID();
+            String simulateTemplateToAdd = "simulate_new_template_" + UUIDs.randomBase64UUID().toLowerCase(Locale.ROOT);
             simulateOnClusterState = indexTemplateService.addIndexTemplateV2(state, request.getIndexTemplateRequest().create(),
                 simulateTemplateToAdd, request.getIndexTemplateRequest().indexTemplate());
         }
diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java
index c2c9b9b8640f2..0f734a5aee9cc 100644
--- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java
+++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataCreateIndexService.java
@@ -981,7 +981,7 @@ private static List<String> validatePrivateSettingsNotExplicitlySet(Settings set
         for (final String key : settings.keySet()) {
             final Setting<?> setting = indexScopedSettings.get(key);
             if (setting == null) {
-                assert indexScopedSettings.isPrivateSetting(key);
+                assert indexScopedSettings.isPrivateSetting(key) : "expected [" + key + "] to be private but it was not";
             } else if (setting.isPrivateIndex()) {
                 validationErrors.add("private index setting [" + key + "] can not be set explicitly");
             }
diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java
index 280e546f983a7..846a9205680d7 100644
--- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java
+++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java
@@ -233,6 +233,7 @@ ClusterState addComponentTemplate(final ClusterState currentState, final boolean
         final Template finalTemplate = new Template(finalSettings,
             stringMappings == null ? null : new CompressedXContent(stringMappings), template.template().aliases());
         final ComponentTemplate finalComponentTemplate = new ComponentTemplate(finalTemplate, template.version(), template.metadata());
+        validate(name, finalComponentTemplate);
         logger.info("adding component template [{}]", name);
         return ClusterState.builder(currentState)
             .metadata(Metadata.builder(currentState.metadata()).put(name, finalComponentTemplate))
@@ -402,6 +403,7 @@ public ClusterState addIndexTemplateV2(final ClusterState currentState, final bo
                 template.priority(), template.version(), template.metadata());
         }
 
+        validate(name, finalIndexTemplate);
         logger.info("adding index template [{}]", name);
         return ClusterState.builder(currentState)
             .metadata(Metadata.builder(currentState.metadata()).put(name, finalIndexTemplate))
@@ -922,70 +924,126 @@ private static void validateTemplate(Settings validateSettings, String mappings,
         }
     }
 
-    private void validate(PutRequest request) {
+    private void validate(String name, ComponentTemplate template) {
+        validate(name,
+            template.template().settings(),
+            Collections.emptyList(),
+            Optional.ofNullable(template.template().aliases())
+                .map(aliases -> aliases.values().stream()
+                    .map(aliasMeta -> {
+                        Alias a = new Alias(aliasMeta.alias());
+                        if (aliasMeta.filter() != null) {
+                            a.filter(aliasMeta.filter().string());
+                        }
+                        a.searchRouting(aliasMeta.searchRouting());
+                        a.indexRouting(aliasMeta.indexRouting());
+                        a.isHidden(aliasMeta.isHidden());
+                        a.writeIndex(aliasMeta.writeIndex());
+                        return a;
+                    })
+                    .collect(Collectors.toList()))
+                .orElse(Collections.emptyList()));
+    }
+
+    private void validate(String name, IndexTemplateV2 template) {
+        Optional<Template> maybeTemplate = Optional.ofNullable(template.template());
+        validate(name,
+            maybeTemplate.map(Template::settings).orElse(Settings.EMPTY),
+            template.indexPatterns(),
+            maybeTemplate
+                .map(Template::aliases)
+                .map(aliasMap -> aliasMap.values().stream()
+                    .map(aliasMeta -> {
+                        Alias a = new Alias(aliasMeta.alias());
+                        if (aliasMeta.filter() != null) {
+                            a.filter(aliasMeta.filter().string());
+                        }
+                        a.searchRouting(aliasMeta.searchRouting());
+                        a.indexRouting(aliasMeta.indexRouting());
+                        a.isHidden(aliasMeta.isHidden());
+                        a.writeIndex(aliasMeta.writeIndex());
+                        return a;
+                    })
+                    .collect(Collectors.toList()))
+                .orElse(Collections.emptyList()));
+    }
+
+    private void validate(PutRequest putRequest) {
+        validate(putRequest.name, putRequest.settings, putRequest.indexPatterns, putRequest.aliases);
+    }
+
+    private void validate(String name, @Nullable Settings settings, List<String> indexPatterns, List<Alias> aliases) {
         List<String> validationErrors = new ArrayList<>();
-        if (request.name.contains(" ")) {
+        if (name.contains(" ")) {
             validationErrors.add("name must not contain a space");
         }
-        if (request.name.contains(",")) {
+        if (name.contains(",")) {
             validationErrors.add("name must not contain a ','");
         }
-        if (request.name.contains("#")) {
+        if (name.contains("#")) {
             validationErrors.add("name must not contain a '#'");
         }
-        if (request.name.startsWith("_")) {
+        if (name.contains("*")) {
+            validationErrors.add("name must not contain a '*'");
+        }
+        if (name.startsWith("_")) {
             validationErrors.add("name must not start with '_'");
         }
-        if (!request.name.toLowerCase(Locale.ROOT).equals(request.name)) {
+        if (name.toLowerCase(Locale.ROOT).equals(name) == false) {
             validationErrors.add("name must be lower cased");
         }
-        for(String indexPattern : request.indexPatterns) {
+        for(String indexPattern : indexPatterns) {
             if (indexPattern.contains(" ")) {
-                validationErrors.add("template must not contain a space");
+                validationErrors.add("index_patterns [" + indexPattern + "] must not contain a space");
             }
             if (indexPattern.contains(",")) {
-                validationErrors.add("template must not contain a ','");
+                validationErrors.add("index_pattern [" + indexPattern + "] must not contain a ','");
             }
             if (indexPattern.contains("#")) {
-                validationErrors.add("template must not contain a '#'");
+                validationErrors.add("index_pattern [" + indexPattern + "] must not contain a '#'");
             }
             if (indexPattern.startsWith("_")) {
-                validationErrors.add("template must not start with '_'");
+                validationErrors.add("index_pattern [" + indexPattern + "] must not start with '_'");
             }
-            if (!Strings.validFileNameExcludingAstrix(indexPattern)) {
-                validationErrors.add("template must not contain the following characters " + Strings.INVALID_FILENAME_CHARS);
+            if (Strings.validFileNameExcludingAstrix(indexPattern) == false) {
+                validationErrors.add("index_pattern [" + indexPattern + "] must not contain the following characters " +
+                    Strings.INVALID_FILENAME_CHARS);
             }
         }
 
-        try {
-            indexScopedSettings.validate(request.settings, true); // templates must be consistent with regards to dependencies
-        } catch (IllegalArgumentException iae) {
-            validationErrors.add(iae.getMessage());
-            for (Throwable t : iae.getSuppressed()) {
-                validationErrors.add(t.getMessage());
+
+        if (settings != null) {
+            try {
+                // templates must be consistent with regards to dependencies
+                indexScopedSettings.validate(settings, true);
+            } catch (IllegalArgumentException iae) {
+                validationErrors.add(iae.getMessage());
+                for (Throwable t : iae.getSuppressed()) {
+                    validationErrors.add(t.getMessage());
+                }
             }
+            List<String> indexSettingsValidation = metadataCreateIndexService.getIndexSettingsValidationErrors(settings, true);
+            validationErrors.addAll(indexSettingsValidation);
         }
-        List<String> indexSettingsValidation = metadataCreateIndexService.getIndexSettingsValidationErrors(request.settings, true);
-        validationErrors.addAll(indexSettingsValidation);
 
-        if (request.indexPatterns.stream().anyMatch(Regex::isMatchAllPattern)) {
-            if (IndexMetadata.INDEX_HIDDEN_SETTING.exists(request.settings)) {
+        if (indexPatterns.stream().anyMatch(Regex::isMatchAllPattern)) {
+            if (settings != null && IndexMetadata.INDEX_HIDDEN_SETTING.exists(settings)) {
                 validationErrors.add("global templates may not specify the setting " + IndexMetadata.INDEX_HIDDEN_SETTING.getKey());
             }
         }
 
-        if (!validationErrors.isEmpty()) {
+        if (validationErrors.size() > 0) {
             ValidationException validationException = new ValidationException();
             validationException.addValidationErrors(validationErrors);
-            throw new InvalidIndexTemplateException(request.name, validationException.getMessage());
+            throw new InvalidIndexTemplateException(name, validationException.getMessage());
         }
 
-        for (Alias alias : request.aliases) {
-            //we validate the alias only partially, as we don't know yet to which index it'll get applied to
+        for (Alias alias : aliases) {
+            // we validate the alias only partially, as we don't know yet to which index it'll get applied to
             aliasValidator.validateAliasStandalone(alias);
-            if (request.indexPatterns.contains(alias.name())) {
-                throw new IllegalArgumentException("Alias [" + alias.name() +
-                    "] cannot be the same as any pattern in [" + String.join(", ", request.indexPatterns) + "]");
+            if (indexPatterns.contains(alias.name())) {
+                throw new IllegalArgumentException("alias [" + alias.name() +
+                    "] cannot be the same as any pattern in [" + String.join(", ", indexPatterns) + "]");
             }
         }
     }
diff --git a/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateServiceTests.java b/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateServiceTests.java
index 09eeb4da3094f..2029d169dcd88 100644
--- a/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateServiceTests.java
+++ b/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateServiceTests.java
@@ -146,7 +146,7 @@ public void testIndexTemplateValidationAccumulatesValidationErrors() {
         assertEquals(throwables.size(), 1);
         assertThat(throwables.get(0), instanceOf(InvalidIndexTemplateException.class));
         assertThat(throwables.get(0).getMessage(), containsString("name must not contain a space"));
-        assertThat(throwables.get(0).getMessage(), containsString("template must not start with '_'"));
+        assertThat(throwables.get(0).getMessage(), containsString("index_pattern [_test_shards*] must not start with '_'"));
         assertThat(throwables.get(0).getMessage(),
                 containsString("Failed to parse value [0] for setting [index.number_of_shards] must be >= 1"));
     }
@@ -159,7 +159,7 @@ public void testIndexTemplateWithAliasNameEqualToTemplatePattern() {
         List<Throwable> errors = putTemplate(xContentRegistry(), request);
         assertThat(errors.size(), equalTo(1));
         assertThat(errors.get(0), instanceOf(IllegalArgumentException.class));
-        assertThat(errors.get(0).getMessage(), equalTo("Alias [foobar] cannot be the same as any pattern in [foo, foobar]"));
+        assertThat(errors.get(0).getMessage(), equalTo("alias [foobar] cannot be the same as any pattern in [foo, foobar]"));
     }
 
     public void testIndexTemplateWithValidateMapping() throws Exception {
@@ -309,15 +309,15 @@ public void testUpdateComponentTemplateWithIndexHiddenSetting() throws Exception
 
         IndexTemplateV2 firstGlobalIndexTemplate =
             new IndexTemplateV2(List.of("*"), template, List.of("foo"), 1L, null, null);
-        state = metadataIndexTemplateService.addIndexTemplateV2(state, true, "globalIndexTemplate1", firstGlobalIndexTemplate);
+        state = metadataIndexTemplateService.addIndexTemplateV2(state, true, "globalindextemplate1", firstGlobalIndexTemplate);
 
         IndexTemplateV2 secondGlobalIndexTemplate =
             new IndexTemplateV2(List.of("*"), template, List.of("foo"), 2L, null, null);
-        state = metadataIndexTemplateService.addIndexTemplateV2(state, true, "globalIndexTemplate2", secondGlobalIndexTemplate);
+        state = metadataIndexTemplateService.addIndexTemplateV2(state, true, "globalindextemplate2", secondGlobalIndexTemplate);
 
         IndexTemplateV2 fooPatternIndexTemplate =
             new IndexTemplateV2(List.of("foo-*"), template, List.of("foo"), 3L, null, null);
-        state = metadataIndexTemplateService.addIndexTemplateV2(state, true, "fooPatternIndexTemplate", fooPatternIndexTemplate);
+        state = metadataIndexTemplateService.addIndexTemplateV2(state, true, "foopatternindextemplate", fooPatternIndexTemplate);
 
         // update the component template to set the index.hidden setting
         Template templateWithIndexHiddenSetting = new Template(Settings.builder().put(IndexMetadata.SETTING_INDEX_HIDDEN, true).build(),
@@ -328,9 +328,9 @@ public void testUpdateComponentTemplateWithIndexHiddenSetting() throws Exception
             fail("expecting an exception as updating the component template would yield the global templates to include the index.hidden " +
                 "setting");
         } catch (IllegalArgumentException e) {
-            assertThat(e.getMessage(), containsStringIgnoringCase("globalIndexTemplate1"));
-            assertThat(e.getMessage(), containsStringIgnoringCase("globalIndexTemplate2"));
-            assertThat(e.getMessage(), not(containsStringIgnoringCase("fooPatternIndexTemplate")));
+            assertThat(e.getMessage(), containsStringIgnoringCase("globalindextemplate1"));
+            assertThat(e.getMessage(), containsStringIgnoringCase("globalindextemplate2"));
+            assertThat(e.getMessage(), not(containsStringIgnoringCase("foopatternindextemplate")));
         }
     }
 
diff --git a/server/src/test/java/org/elasticsearch/cluster/shards/ClusterShardLimitIT.java b/server/src/test/java/org/elasticsearch/cluster/shards/ClusterShardLimitIT.java
index c51f46457afff..143711d9726be 100644
--- a/server/src/test/java/org/elasticsearch/cluster/shards/ClusterShardLimitIT.java
+++ b/server/src/test/java/org/elasticsearch/cluster/shards/ClusterShardLimitIT.java
@@ -121,7 +121,7 @@ public void testIndexCreationOverLimitFromTemplate() {
 
         assertAcked(client().admin()
             .indices()
-            .preparePutTemplate("should-fail*")
+            .preparePutTemplate("should-fail")
             .setPatterns(Collections.singletonList("should-fail"))
             .setOrder(1)
             .setSettings(Settings.builder()

From 25ee3ced9469205c234a0c900900963b324d01f2 Mon Sep 17 00:00:00 2001
From: Lee Hinman <lee@writequit.org>
Date: Mon, 4 May 2020 17:14:50 -0600
Subject: [PATCH 2/3] Disallow ':' in index patterns also

---
 .../cluster/metadata/MetadataIndexTemplateService.java         | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java
index 846a9205680d7..12dcfc3e9fca1 100644
--- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java
+++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java
@@ -1002,6 +1002,9 @@ private void validate(String name, @Nullable Settings settings, List<String> ind
             if (indexPattern.contains("#")) {
                 validationErrors.add("index_pattern [" + indexPattern + "] must not contain a '#'");
             }
+            if (indexPattern.contains(":")) {
+                validationErrors.add("index_pattern [" + indexPattern + "] must not contain a ':'");
+            }
             if (indexPattern.startsWith("_")) {
                 validationErrors.add("index_pattern [" + indexPattern + "] must not start with '_'");
             }

From 2ce90bb6c8d2d70eff498fe58f01a5bc97a75d22 Mon Sep 17 00:00:00 2001
From: Lee Hinman <lee@writequit.org>
Date: Tue, 5 May 2020 08:15:35 -0600
Subject: [PATCH 3/3] Refactor validation methods to DRY them up

---
 .../MetadataIndexTemplateService.java         | 60 ++++++++-----------
 1 file changed, 24 insertions(+), 36 deletions(-)

diff --git a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java
index 12dcfc3e9fca1..9861b688d4d0b 100644
--- a/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java
+++ b/server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexTemplateService.java
@@ -925,47 +925,35 @@ private static void validateTemplate(Settings validateSettings, String mappings,
     }
 
     private void validate(String name, ComponentTemplate template) {
-        validate(name,
-            template.template().settings(),
-            Collections.emptyList(),
-            Optional.ofNullable(template.template().aliases())
-                .map(aliases -> aliases.values().stream()
-                    .map(aliasMeta -> {
-                        Alias a = new Alias(aliasMeta.alias());
-                        if (aliasMeta.filter() != null) {
-                            a.filter(aliasMeta.filter().string());
-                        }
-                        a.searchRouting(aliasMeta.searchRouting());
-                        a.indexRouting(aliasMeta.indexRouting());
-                        a.isHidden(aliasMeta.isHidden());
-                        a.writeIndex(aliasMeta.writeIndex());
-                        return a;
-                    })
-                    .collect(Collectors.toList()))
-                .orElse(Collections.emptyList()));
+        validate(name, template.template(), Collections.emptyList());
     }
 
     private void validate(String name, IndexTemplateV2 template) {
-        Optional<Template> maybeTemplate = Optional.ofNullable(template.template());
+        validate(name, template.template(), template.indexPatterns());
+    }
+
+    private void validate(String name, Template template, List<String> indexPatterns) {
+        Optional<Template> maybeTemplate = Optional.ofNullable(template);
         validate(name,
             maybeTemplate.map(Template::settings).orElse(Settings.EMPTY),
-            template.indexPatterns(),
-            maybeTemplate
-                .map(Template::aliases)
-                .map(aliasMap -> aliasMap.values().stream()
-                    .map(aliasMeta -> {
-                        Alias a = new Alias(aliasMeta.alias());
-                        if (aliasMeta.filter() != null) {
-                            a.filter(aliasMeta.filter().string());
-                        }
-                        a.searchRouting(aliasMeta.searchRouting());
-                        a.indexRouting(aliasMeta.indexRouting());
-                        a.isHidden(aliasMeta.isHidden());
-                        a.writeIndex(aliasMeta.writeIndex());
-                        return a;
-                    })
-                    .collect(Collectors.toList()))
-                .orElse(Collections.emptyList()));
+            indexPatterns,
+            maybeTemplate.map(Template::aliases)
+                .orElse(Collections.emptyMap())
+                .values().stream()
+                .map(MetadataIndexTemplateService::toAlias)
+                .collect(Collectors.toList()));
+    }
+
+    private static Alias toAlias(AliasMetadata aliasMeta) {
+        Alias a = new Alias(aliasMeta.alias());
+        if (aliasMeta.filter() != null) {
+            a.filter(aliasMeta.filter().string());
+        }
+        a.searchRouting(aliasMeta.searchRouting());
+        a.indexRouting(aliasMeta.indexRouting());
+        a.isHidden(aliasMeta.isHidden());
+        a.writeIndex(aliasMeta.writeIndex());
+        return a;
     }
 
     private void validate(PutRequest putRequest) {