From 31a7be60fd6973e0a207a4b786736a61028bee93 Mon Sep 17 00:00:00 2001 From: Slobodan Adamovic Date: Fri, 28 Mar 2025 14:25:16 +0100 Subject: [PATCH 1/4] [Failure Store] Prevent explicit selectors in role index name patterns Adding basic validation to prevent using `::` selectors when defining index permissions. For example, the `test-*::failures` will not be allowed for `test-role`: ``` PUT /_security/role/test-role { "indices": [ { "names": ["test-*::failures"], "privileges": ["read"] } ] } ``` --- .../role/RoleDescriptorRequestValidator.java | 10 ++ .../RoleDescriptorRequestValidatorTests.java | 103 ++++++++++++++++++ 2 files changed, 113 insertions(+) create mode 100644 x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/role/RoleDescriptorRequestValidatorTests.java diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/role/RoleDescriptorRequestValidator.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/role/RoleDescriptorRequestValidator.java index 761c2258d3d3f..0feaa985630d2 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/role/RoleDescriptorRequestValidator.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/role/RoleDescriptorRequestValidator.java @@ -8,6 +8,7 @@ package org.elasticsearch.xpack.core.security.action.role; import org.elasticsearch.action.ActionRequestValidationException; +import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver; import org.elasticsearch.xpack.core.security.authz.RoleDescriptor; import org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivilege; import org.elasticsearch.xpack.core.security.authz.privilege.ClusterPrivilegeResolver; @@ -53,6 +54,15 @@ public static ActionRequestValidationException validate( } catch (IllegalArgumentException ile) { validationException = addValidationError(ile.getMessage(), validationException); } + for (final String indexName : idp.getIndices()) { + if (IndexNameExpressionResolver.hasSelectorSuffix(indexName)) { + validationException = addValidationError( + IndexNameExpressionResolver.SelectorResolver.SELECTOR_SEPARATOR + + " selectors are not allowed in the index name expression", + validationException + ); + } + } } } final RoleDescriptor.RemoteIndicesPrivileges[] remoteIndicesPrivileges = roleDescriptor.getRemoteIndicesPrivileges(); diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/role/RoleDescriptorRequestValidatorTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/role/RoleDescriptorRequestValidatorTests.java new file mode 100644 index 0000000000000..e091ca4260c23 --- /dev/null +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/role/RoleDescriptorRequestValidatorTests.java @@ -0,0 +1,103 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0; you may not use this file except in compliance with the Elastic License + * 2.0. + */ + +package org.elasticsearch.xpack.core.security.action.role; + +import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.xpack.core.security.authz.RoleDescriptor; +import org.elasticsearch.xpack.core.security.authz.RoleDescriptor.IndicesPrivileges; +import org.elasticsearch.xpack.core.security.authz.privilege.IndexPrivilege; + +import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.hamcrest.Matchers.notNullValue; +import static org.hamcrest.Matchers.nullValue; + +public class RoleDescriptorRequestValidatorTests extends ESTestCase { + + public void testSelectorsValidation() { + String[] invalidIndexNames = { + "index::failures", + ".fs-*::failures", + ".ds-*::data", + "*::failures", + "*::", + "?::?", + "test?-*::data", + "test-*::*", // actual selector is not relevant and not validated + "test::irrelevant", + "::test", + "test::", + "::", + ":: ", + " ::", + ":::", + "::::", + randomAlphaOfLengthBetween(5, 10) + "\u003a\u003afailures", + randomAlphaOfLengthBetween(5, 10) + "\072\072failures" }; + for (String indexName : invalidIndexNames) { + validateAndAssertSelectorNotAllowed(indexName); + } + + // these are not necessarily valid index names, but they should not trigger the selector validation + String[] validIndexNames = { + "index:failures", // single colon is allowed + ":failures", + "no double colon", + ":", + ": :", + "", + " ", + ":\n:", + null, + "a:", + ":b:", + "*", + "c?-*", + "d-*e", + "f:g:h", + "/[a-b]*test:[a-b]*:failures/", // while this regex can match test::failures, it is not rejected - doing so would be too complex + randomIntBetween(-10, 10) + "", + randomAlphaOfLengthBetween(1, 10), + randomAlphanumericOfLength(10) }; + for (String indexName : validIndexNames) { + validateAndAssertNoException(indexName); + } + } + + private static void validateAndAssertSelectorNotAllowed(String indexName) { + var validationException = RoleDescriptorRequestValidator.validate(roleWithIndexPrivileges(indexName)); + assertThat("expected validation exception for " + indexName, validationException, notNullValue()); + assertThat(validationException.validationErrors(), containsInAnyOrder(":: selectors are not allowed in the index name expression")); + } + + private static void validateAndAssertNoException(String indexName) { + var validationException = RoleDescriptorRequestValidator.validate(roleWithIndexPrivileges(indexName)); + assertThat("expected no validation exception for " + indexName, validationException, nullValue()); + } + + private static RoleDescriptor roleWithIndexPrivileges(String... indices) { + return new RoleDescriptor( + "test-role", + null, + new IndicesPrivileges[] { + IndicesPrivileges.builder() + .allowRestrictedIndices(randomBoolean()) + .indices(indices) + .privileges(randomSubsetOf(randomIntBetween(1, IndexPrivilege.names().size()), IndexPrivilege.names())) + .build() }, + null, + null, + null, + null, + null, + null, + null, + null, + null + ); + } +} From 9266bd014145332333f96a89001f989f3acc1d8f Mon Sep 17 00:00:00 2001 From: Slobodan Adamovic Date: Fri, 28 Mar 2025 15:29:15 +0100 Subject: [PATCH 2/4] validate remote index names as well and hide everything behind feature flag --- .../role/RoleDescriptorRequestValidator.java | 33 +++++++++--- .../RoleDescriptorRequestValidatorTests.java | 53 ++++++++++++++++--- 2 files changed, 72 insertions(+), 14 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/role/RoleDescriptorRequestValidator.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/role/RoleDescriptorRequestValidator.java index 0feaa985630d2..e6d29473c440a 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/role/RoleDescriptorRequestValidator.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/role/RoleDescriptorRequestValidator.java @@ -8,6 +8,7 @@ package org.elasticsearch.xpack.core.security.action.role; import org.elasticsearch.action.ActionRequestValidationException; +import org.elasticsearch.cluster.metadata.DataStream; import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver; import org.elasticsearch.xpack.core.security.authz.RoleDescriptor; import org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivilege; @@ -54,13 +55,9 @@ public static ActionRequestValidationException validate( } catch (IllegalArgumentException ile) { validationException = addValidationError(ile.getMessage(), validationException); } - for (final String indexName : idp.getIndices()) { - if (IndexNameExpressionResolver.hasSelectorSuffix(indexName)) { - validationException = addValidationError( - IndexNameExpressionResolver.SelectorResolver.SELECTOR_SEPARATOR - + " selectors are not allowed in the index name expression", - validationException - ); + if (DataStream.isFailureStoreFeatureFlagEnabled()) { + for (final String indexName : idp.getIndices()) { + validationException = validateIndexNameExpression(indexName, validationException); } } } @@ -81,6 +78,11 @@ public static ActionRequestValidationException validate( } catch (IllegalArgumentException ile) { validationException = addValidationError(ile.getMessage(), validationException); } + if (DataStream.isFailureStoreFeatureFlagEnabled()) { + for (String indexName : ridp.indicesPrivileges().getIndices()) { + validationException = validateIndexNameExpression(indexName, validationException); + } + } } if (roleDescriptor.hasRemoteClusterPermissions()) { try { @@ -128,4 +130,21 @@ public static ActionRequestValidationException validate( } return validationException; } + + private static ActionRequestValidationException validateIndexNameExpression( + String indexNameExpression, + ActionRequestValidationException validationException + ) { + if (IndexNameExpressionResolver.hasSelectorSuffix(indexNameExpression)) { + validationException = addValidationError( + "selectors [" + + IndexNameExpressionResolver.SelectorResolver.SELECTOR_SEPARATOR + + "] are not allowed in the index name expression [" + + indexNameExpression + + "]", + validationException + ); + } + return validationException; + } } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/role/RoleDescriptorRequestValidatorTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/role/RoleDescriptorRequestValidatorTests.java index e091ca4260c23..d1e0f546e0d63 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/role/RoleDescriptorRequestValidatorTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/role/RoleDescriptorRequestValidatorTests.java @@ -7,11 +7,15 @@ package org.elasticsearch.xpack.core.security.action.role; +import org.elasticsearch.cluster.metadata.DataStream; import org.elasticsearch.test.ESTestCase; import org.elasticsearch.xpack.core.security.authz.RoleDescriptor; import org.elasticsearch.xpack.core.security.authz.RoleDescriptor.IndicesPrivileges; import org.elasticsearch.xpack.core.security.authz.privilege.IndexPrivilege; +import java.util.Set; +import java.util.stream.Collectors; + import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.notNullValue; import static org.hamcrest.Matchers.nullValue; @@ -19,6 +23,7 @@ public class RoleDescriptorRequestValidatorTests extends ESTestCase { public void testSelectorsValidation() { + assumeTrue("failure store feature flag must be enabled", DataStream.isFailureStoreFeatureFlagEnabled()); String[] invalidIndexNames = { "index::failures", ".fs-*::failures", @@ -39,7 +44,8 @@ public void testSelectorsValidation() { randomAlphaOfLengthBetween(5, 10) + "\u003a\u003afailures", randomAlphaOfLengthBetween(5, 10) + "\072\072failures" }; for (String indexName : invalidIndexNames) { - validateAndAssertSelectorNotAllowed(indexName); + validateAndAssertSelectorNotAllowed(roleWithIndexPrivileges(indexName), indexName); + validateAndAssertSelectorNotAllowed(roleWithRemoteIndexPrivileges(indexName), indexName); } // these are not necessarily valid index names, but they should not trigger the selector validation @@ -64,18 +70,22 @@ public void testSelectorsValidation() { randomAlphaOfLengthBetween(1, 10), randomAlphanumericOfLength(10) }; for (String indexName : validIndexNames) { - validateAndAssertNoException(indexName); + validateAndAssertNoException(roleWithIndexPrivileges(indexName), indexName); + validateAndAssertNoException(roleWithRemoteIndexPrivileges(indexName), indexName); } } - private static void validateAndAssertSelectorNotAllowed(String indexName) { - var validationException = RoleDescriptorRequestValidator.validate(roleWithIndexPrivileges(indexName)); + private static void validateAndAssertSelectorNotAllowed(RoleDescriptor roleDescriptor, String indexName) { + var validationException = RoleDescriptorRequestValidator.validate(roleDescriptor); assertThat("expected validation exception for " + indexName, validationException, notNullValue()); - assertThat(validationException.validationErrors(), containsInAnyOrder(":: selectors are not allowed in the index name expression")); + assertThat( + validationException.validationErrors(), + containsInAnyOrder("selectors [::] are not allowed in the index name expression [" + indexName + "]") + ); } - private static void validateAndAssertNoException(String indexName) { - var validationException = RoleDescriptorRequestValidator.validate(roleWithIndexPrivileges(indexName)); + private static void validateAndAssertNoException(RoleDescriptor roleDescriptor, String indexName) { + var validationException = RoleDescriptorRequestValidator.validate(roleDescriptor); assertThat("expected no validation exception for " + indexName, validationException, nullValue()); } @@ -100,4 +110,33 @@ private static RoleDescriptor roleWithIndexPrivileges(String... indices) { null ); } + + private static RoleDescriptor roleWithRemoteIndexPrivileges(String... indices) { + Set privileges = IndexPrivilege.names() + .stream() + .filter(p -> false == (p.equals("read_failure_store") || p.equals("manage_failure_store"))) + .collect(Collectors.toSet()); + return new RoleDescriptor( + "remote-test-role", + null, + null, + null, + null, + null, + null, + null, + new RoleDescriptor.RemoteIndicesPrivileges[] { + new RoleDescriptor.RemoteIndicesPrivileges( + IndicesPrivileges.builder() + .allowRestrictedIndices(randomBoolean()) + .indices(indices) + .privileges(randomSubsetOf(randomIntBetween(1, privileges.size()), privileges)) + .build(), + "my-remote-cluster" + ) }, + null, + null, + null + ); + } } From 47330be4b8fff15d7db0b410f09767d4678bebd2 Mon Sep 17 00:00:00 2001 From: Slobodan Adamovic Date: Fri, 28 Mar 2025 16:30:38 +0100 Subject: [PATCH 3/4] fix IT test --- .../FailureStoreSecurityRestIT.java | 124 +++++++++--------- 1 file changed, 63 insertions(+), 61 deletions(-) diff --git a/x-pack/plugin/security/qa/security-trial/src/javaRestTest/java/org/elasticsearch/xpack/security/failurestore/FailureStoreSecurityRestIT.java b/x-pack/plugin/security/qa/security-trial/src/javaRestTest/java/org/elasticsearch/xpack/security/failurestore/FailureStoreSecurityRestIT.java index 5800ed25712aa..5bb523893935a 100644 --- a/x-pack/plugin/security/qa/security-trial/src/javaRestTest/java/org/elasticsearch/xpack/security/failurestore/FailureStoreSecurityRestIT.java +++ b/x-pack/plugin/security/qa/security-trial/src/javaRestTest/java/org/elasticsearch/xpack/security/failurestore/FailureStoreSecurityRestIT.java @@ -17,6 +17,7 @@ import org.elasticsearch.common.Strings; import org.elasticsearch.common.settings.SecureString; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.util.Maps; import org.elasticsearch.common.util.concurrent.ThreadContext; import org.elasticsearch.common.xcontent.XContentHelper; import org.elasticsearch.common.xcontent.support.XContentMapValues; @@ -47,6 +48,7 @@ import java.util.stream.Collectors; import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasItem; import static org.hamcrest.Matchers.is; @@ -876,62 +878,43 @@ public void testHasPrivilegesWithApiKeys() throws IOException { public void testRoleWithSelectorInIndexPattern() throws Exception { setupDataStream(); - createUser("user", PASSWORD, "role"); - upsertRole(""" - { - "cluster": ["all"], - "indices": [ - { - "names": ["*::failures"], - "privileges": ["read"] - } - ] - }""", "role"); - createAndStoreApiKey("user", null); - - expectThrows("user", new Search("test1::failures"), 403); - expectSearch("user", new Search("*::failures")); - - upsertRole(""" - { - "cluster": ["all"], - "indices": [ - { - "names": ["test1::failures"], - "privileges": ["read"] - } - ] - }""", "role"); - - expectThrows("user", new Search("test1::failures"), 403); - expectSearch("user", new Search("*::failures")); + expectThrowsSelectorsNotAllowed( + () -> upsertRole( + Strings.format(""" + { + "cluster": ["all"], + "indices": [ + { + "names": ["%s"], + "privileges": ["%s"] + } + ] + }""", randomFrom("*::failures", "test1::failures", "test1::data", "*::data"), randomFrom("read", "read_failure_store")), + "role", + false + ) + ); - upsertRole(""" - { - "cluster": ["all"], - "indices": [ - { - "names": ["*::failures"], - "privileges": ["read_failure_store"] - } - ] - }""", "role"); - expectThrows("user", new Search("test1::failures"), 403); - expectSearch("user", new Search("*::failures")); + AssertionError bulkFailedError = expectThrows( + AssertionError.class, + () -> upsertRole( + Strings.format(""" + { + "cluster": ["all"], + "indices": [ + { + "names": ["%s"], + "privileges": ["%s"] + } + ] + }""", randomFrom("*::failures", "test1::failures", "test1::data", "*::data"), randomFrom("read", "read_failure_store")), + "role", + true + ) + ); + assertThat(bulkFailedError.getMessage(), containsString("selectors [::] are not allowed in the index name expression")); - upsertRole(""" - { - "cluster": ["all"], - "indices": [ - { - "names": ["test1::failures"], - "privileges": ["read_failure_store"] - } - ] - }""", "role"); - expectThrows("user", new Search("test1::failures"), 403); - expectSearch("user", new Search("*::failures")); } public void testFailureStoreAccess() throws Exception { @@ -2396,7 +2379,7 @@ protected void createUser(String username, SecureString password, String... role protected String createAndStoreApiKey(String username, @Nullable String roleDescriptors) throws IOException { assertThat("API key already registered for user: " + username, apiKeys.containsKey(username), is(false)); apiKeys.put(username, createApiKey(username, roleDescriptors)); - return createApiKey(username, roleDescriptors); + return apiKeys.get(username); } private String createApiKey(String username, String roleDescriptors) throws IOException { @@ -2421,22 +2404,35 @@ private String createApiKey(String username, String roleDescriptors) throws IOEx return (String) responseAsMap.get("encoded"); } - protected void upsertRole(String roleDescriptor, String roleName) throws IOException { - Request createRoleRequest = roleRequest(roleDescriptor, roleName); + protected Response upsertRole(String roleDescriptor, String roleName) throws IOException { + return upsertRole(roleDescriptor, roleName, randomBoolean()); + } + + protected Response upsertRole(String roleDescriptor, String roleName, boolean bulk) throws IOException { + Request createRoleRequest = roleRequest(roleDescriptor, roleName, bulk); Response createRoleResponse = adminClient().performRequest(createRoleRequest); assertOK(createRoleResponse); + if (bulk) { + Map flattenedResponse = Maps.flatten(responseAsMap(createRoleResponse), true, true); + if (flattenedResponse.containsKey("errors.count") && (int) flattenedResponse.get("errors.count") > 0) { + throw new AssertionError( + "Failed to create role [" + roleName + "], reason: " + flattenedResponse.get("errors.details." + roleName + ".reason") + ); + } + } + return createRoleResponse; } - protected Request roleRequest(String roleDescriptor, String roleName) { + protected Request roleRequest(String roleDescriptor, String roleName, boolean bulk) { Request createRoleRequest; - if (randomBoolean()) { - createRoleRequest = new Request(randomFrom(HttpPut.METHOD_NAME, HttpPost.METHOD_NAME), "/_security/role/" + roleName); - createRoleRequest.setJsonEntity(roleDescriptor); - } else { + if (bulk) { createRoleRequest = new Request(HttpPost.METHOD_NAME, "/_security/role"); createRoleRequest.setJsonEntity(org.elasticsearch.core.Strings.format(""" {"roles": {"%s": %s}} """, roleName, roleDescriptor)); + } else { + createRoleRequest = new Request(randomFrom(HttpPut.METHOD_NAME, HttpPost.METHOD_NAME), "/_security/role/" + roleName); + createRoleRequest.setJsonEntity(roleDescriptor); } return createRoleRequest; } @@ -2499,4 +2495,10 @@ private void expectHasPrivilegesWithApiKey(String apiKey, String requestBody, St Response response = performRequestWithApiKey(apiKey, req); assertThat(responseAsMap(response), equalTo(mapFromJson(expectedResponse))); } + + private static void expectThrowsSelectorsNotAllowed(ThrowingRunnable runnable) { + ResponseException exception = expectThrows(ResponseException.class, runnable); + assertThat(exception.getResponse().getStatusLine().getStatusCode(), equalTo(400)); + assertThat(exception.getMessage(), containsString("selectors [::] are not allowed in the index name expression")); + } } From 78699f4c9e59bfdb8cfb01a8d2a5188f1dc4d106 Mon Sep 17 00:00:00 2001 From: Slobodan Adamovic Date: Fri, 28 Mar 2025 18:24:01 +0100 Subject: [PATCH 4/4] test API key role validation as well --- .../failurestore/FailureStoreSecurityRestIT.java | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/x-pack/plugin/security/qa/security-trial/src/javaRestTest/java/org/elasticsearch/xpack/security/failurestore/FailureStoreSecurityRestIT.java b/x-pack/plugin/security/qa/security-trial/src/javaRestTest/java/org/elasticsearch/xpack/security/failurestore/FailureStoreSecurityRestIT.java index 5bb523893935a..e4ccab8f6f79c 100644 --- a/x-pack/plugin/security/qa/security-trial/src/javaRestTest/java/org/elasticsearch/xpack/security/failurestore/FailureStoreSecurityRestIT.java +++ b/x-pack/plugin/security/qa/security-trial/src/javaRestTest/java/org/elasticsearch/xpack/security/failurestore/FailureStoreSecurityRestIT.java @@ -915,6 +915,19 @@ public void testRoleWithSelectorInIndexPattern() throws Exception { ); assertThat(bulkFailedError.getMessage(), containsString("selectors [::] are not allowed in the index name expression")); + expectThrowsSelectorsNotAllowed(() -> createApiKey("user", Strings.format(""" + { + "role": { + "cluster": ["all"], + "indices": [ + { + "names": ["%s"], + "privileges": ["%s"] + } + ] + } + }""", randomFrom("*::failures", "test1::failures", "test1::data", "*::data"), randomFrom("read", "read_failure_store")))); + } public void testFailureStoreAccess() throws Exception {