diff --git a/CHANGELOG.md b/CHANGELOG.md index 99fe66f160..56624c10eb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), * Added new option skip_users to client cert authenticator (clientcert_auth_domain.http_authenticator.config.skip_users in config.yml)([#4378](https://github.com/opensearch-project/security/pull/5525)) * [Resource Sharing] Fixes accessible resource ids search by marking created_by.user field as keyword search instead of text ([#5574](https://github.com/opensearch-project/security/pull/5574)) * [Resource Sharing] Reverts @Inject pattern usage for ResourceSharingExtension to client accessor pattern. ([#5576](https://github.com/opensearch-project/security/pull/5576)) +* Inject user custom attributes when injecting user and role information to the thread context ([#5560](https://github.com/opensearch-project/security/pull/5560)) ### Refactoring diff --git a/src/integrationTest/java/org/opensearch/security/privileges/dlsfls/DocumentPrivilegesTest.java b/src/integrationTest/java/org/opensearch/security/privileges/dlsfls/DocumentPrivilegesTest.java index a63db960df..7182b22ed6 100644 --- a/src/integrationTest/java/org/opensearch/security/privileges/dlsfls/DocumentPrivilegesTest.java +++ b/src/integrationTest/java/org/opensearch/security/privileges/dlsfls/DocumentPrivilegesTest.java @@ -69,6 +69,7 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotEquals; +import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -94,7 +95,6 @@ DocumentPrivilegesTest.DataStreams_getRestriction.class, DocumentPrivilegesTest.DlsQuery.class }) public class DocumentPrivilegesTest { - static NamedXContentRegistry xContentRegistry = new NamedXContentRegistry( ImmutableList.of( new NamedXContentRegistry.Entry( @@ -364,23 +364,25 @@ public void queryPatternTemplate() throws Exception { new TestSecurityConfig.Role("non_dls_role").indexPermissions("*").on("index_a*") ); - DocumentPrivileges subject = createSubject(roleConfig); + Exception exception = null; + DlsRestriction dlsRestriction = null; + DocumentPrivileges subject = null; - DlsRestriction dlsRestriction = subject.getRestriction(context, index.getName()); + try { + subject = createSubject(roleConfig); + dlsRestriction = subject.getRestriction(context, index.getName()); + } catch (PrivilegesEvaluationException ex) { + exception = ex; + } if (index == index_b1) { // This test case never grants privileges to index_b1 assertThat(dlsRestriction, isFullyRestricted()); } else if (userSpec.attributes.isEmpty()) { - // If a role uses undefined user attributes for DLS queries, the attribute templates - // remain unchanged in the resulting query. This is a property of the current attribute handling code. - // It would be probably better if an error would be raised in that case. + // If a role uses undefined user attributes for DLS queries, an exception should be raised if (index == index_a1) { if (userSpec.roles.contains("dls_role_1") && userSpec.roles.contains("dls_role_2")) { - assertThat( - dlsRestriction, - isRestricted(termQuery("dept", "${attr.attr_a}1"), termQuery("dept", "${attr.attr_a}2")) - ); + assertNotNull(exception); } } } else if (userSpec.roles.contains("non_dls_role") && dfmEmptyOverridesAll) { @@ -417,7 +419,7 @@ public void queryPatternTemplate() throws Exception { } boolean isUnrestricted = subject.isUnrestricted(context, index.getName()); - if (dlsRestriction.isUnrestricted()) { + if (dlsRestriction != null && dlsRestriction.isUnrestricted()) { assertTrue("isUnrestricted() should return true according to " + dlsRestriction, isUnrestricted); } else { assertFalse("isUnrestricted() should return false according to " + dlsRestriction, isUnrestricted); diff --git a/src/main/java/org/opensearch/security/auth/RolesInjector.java b/src/main/java/org/opensearch/security/auth/RolesInjector.java index 76d2e46614..42afc77ad2 100644 --- a/src/main/java/org/opensearch/security/auth/RolesInjector.java +++ b/src/main/java/org/opensearch/security/auth/RolesInjector.java @@ -15,6 +15,7 @@ package org.opensearch.security.auth; +import java.util.Map; import java.util.Set; import com.google.common.collect.ImmutableSet; @@ -70,7 +71,11 @@ public Set injectUserAndRoles(final ThreadPool threadPool) { return null; } Set roles = ImmutableSet.copyOf(strs[1].split(",")); - User user = new User(strs[0]).withSecurityRoles(roles); + + Map customAttributes = threadPool.getThreadContext() + .getTransient(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_USER_CUSTOM_ATTRIBUTES); + + User user = new User(strs[0]).withSecurityRoles(roles).withAttributes(customAttributes); addUser(user, threadPool); return roles; diff --git a/src/main/java/org/opensearch/security/privileges/IndexPattern.java b/src/main/java/org/opensearch/security/privileges/IndexPattern.java index 9ce65dea40..014af99e54 100644 --- a/src/main/java/org/opensearch/security/privileges/IndexPattern.java +++ b/src/main/java/org/opensearch/security/privileges/IndexPattern.java @@ -220,7 +220,7 @@ public void add(List source) { if (indexPattern.startsWith("<") && indexPattern.endsWith(">")) { this.dateMathExpressions.add(indexPattern); - } else if (!containsPlaceholder(indexPattern)) { + } else if (!UserAttributes.needsAttributeSubstitution(indexPattern)) { this.constantPatterns.add(WildcardMatcher.from(indexPattern)); } else { this.patternTemplates.add(indexPattern); @@ -241,10 +241,6 @@ public IndexPattern build() { } } - static boolean containsPlaceholder(String indexPattern) { - return indexPattern.indexOf("${") != -1; - } - public static IndexPattern from(List source) { Builder builder = new Builder(); builder.add(source); diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java index 65c98d7165..9cfe99de94 100644 --- a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java +++ b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java @@ -26,7 +26,6 @@ package org.opensearch.security.privileges; -import java.io.Serializable; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; @@ -307,7 +306,7 @@ private void setUserInfoInThreadContext(PrivilegesEvaluationContext context) { log.debug(joiner); if (this.isUserAttributeSerializationEnabled()) { - joiner.add(Base64Helper.serializeObject((Serializable) context.getUser().getCustomAttributesMap())); + joiner.add(Base64Helper.serializeObject(new HashMap<>(context.getUser().getCustomAttributesMap()))); } threadContext.putTransient(OPENDISTRO_SECURITY_USER_INFO_THREAD_CONTEXT, joiner.toString()); diff --git a/src/main/java/org/opensearch/security/privileges/TenantPrivileges.java b/src/main/java/org/opensearch/security/privileges/TenantPrivileges.java index 2395ba548e..9320abfb45 100644 --- a/src/main/java/org/opensearch/security/privileges/TenantPrivileges.java +++ b/src/main/java/org/opensearch/security/privileges/TenantPrivileges.java @@ -104,7 +104,7 @@ public TenantPrivileges( for (RoleV7.Tenant tenantPermissions : role.getTenant_permissions()) { List actionTypes = resolveActionType(tenantPermissions.getAllowed_actions(), actionGroups); for (String tenantPattern : tenantPermissions.getTenant_patterns()) { - if (isDynamic(tenantPattern)) { + if (UserAttributes.needsAttributeSubstitution(tenantPattern)) { // If a tenant pattern contains a user attribute (like ${user.name}), we can only // do the tenant pattern matching during the actual tenant privilege evaluation, when the user is known. // Thus, we just keep these patterns here unprocessed @@ -252,10 +252,6 @@ static List resolveActionType(Collection allowedActions, Fla } } - static boolean isDynamic(String tenantPattern) { - return tenantPattern.contains("${"); - } - private static ImmutableMap> build( Map> source, DeduplicatingCompactSubSetBuilder.Completed completedRoleSetBuilder diff --git a/src/main/java/org/opensearch/security/privileges/UserAttributes.java b/src/main/java/org/opensearch/security/privileges/UserAttributes.java index 2f5dfeed35..f1a8acf148 100644 --- a/src/main/java/org/opensearch/security/privileges/UserAttributes.java +++ b/src/main/java/org/opensearch/security/privileges/UserAttributes.java @@ -25,6 +25,10 @@ * This code was moved over from ConfigModelV7. */ public class UserAttributes { + public static boolean needsAttributeSubstitution(String patternString) { + return patternString.contains("${"); + } + public static String replaceProperties(String orig, PrivilegesEvaluationContext context) { User user = context.getUser(); diff --git a/src/main/java/org/opensearch/security/privileges/dlsfls/DocumentPrivileges.java b/src/main/java/org/opensearch/security/privileges/dlsfls/DocumentPrivileges.java index 4c2cdc211e..0252fb2772 100644 --- a/src/main/java/org/opensearch/security/privileges/dlsfls/DocumentPrivileges.java +++ b/src/main/java/org/opensearch/security/privileges/dlsfls/DocumentPrivileges.java @@ -18,6 +18,7 @@ import org.apache.logging.log4j.util.Strings; +import org.opensearch.OpenSearchSecurityException; import org.opensearch.cluster.metadata.IndexAbstraction; import org.opensearch.common.settings.Settings; import org.opensearch.common.xcontent.json.JsonXContent; @@ -44,7 +45,6 @@ * Instances of this class are managed by DlsFlsProcessedConfig. */ public class DocumentPrivileges extends AbstractRuleBasedPrivileges { - private final NamedXContentRegistry xContentRegistry; public DocumentPrivileges( @@ -134,7 +134,7 @@ protected QueryBuilder parseQuery(String queryString, NamedXContentRegistry xCon static DlsQuery create(String queryString, NamedXContentRegistry xContentRegistry) throws PrivilegesConfigurationValidationException { - if (queryString.contains("${")) { + if (UserAttributes.needsAttributeSubstitution(queryString)) { return new DlsQuery.Dynamic(queryString, xContentRegistry); } else { return new DlsQuery.Constant(queryString, xContentRegistry); @@ -174,6 +174,12 @@ static class Dynamic extends DlsQuery { @Override RenderedDlsQuery evaluate(PrivilegesEvaluationContext context) throws PrivilegesEvaluationException { String effectiveQueryString = UserAttributes.replaceProperties(this.queryString, context); + if (UserAttributes.needsAttributeSubstitution(effectiveQueryString)) { + throw new PrivilegesEvaluationException( + "Invalid DLS query: " + effectiveQueryString, + new OpenSearchSecurityException("User attribute substitution failed") + ); + } try { return new RenderedDlsQuery(parseQuery(effectiveQueryString, xContentRegistry), effectiveQueryString); } catch (Exception e) { diff --git a/src/main/java/org/opensearch/security/support/ConfigConstants.java b/src/main/java/org/opensearch/security/support/ConfigConstants.java index 1f9ac97090..81202e47fe 100644 --- a/src/main/java/org/opensearch/security/support/ConfigConstants.java +++ b/src/main/java/org/opensearch/security/support/ConfigConstants.java @@ -131,6 +131,7 @@ public class ConfigConstants { public static final String OPENDISTRO_SECURITY_INJECTED_USER = "injected_user"; public static final String OPENDISTRO_SECURITY_INJECTED_USER_HEADER = "injected_user_header"; + public static final String OPENDISTRO_SECURITY_INJECTED_USER_CUSTOM_ATTRIBUTES = "injected_user_custom_attributes"; public static final String OPENDISTRO_SECURITY_XFF_DONE = OPENDISTRO_SECURITY_CONFIG_PREFIX + "xff_done"; diff --git a/src/test/java/org/opensearch/security/RolesInjectorIntegTest.java b/src/test/java/org/opensearch/security/RolesInjectorIntegTest.java index 89664b6a51..683808caf0 100644 --- a/src/test/java/org/opensearch/security/RolesInjectorIntegTest.java +++ b/src/test/java/org/opensearch/security/RolesInjectorIntegTest.java @@ -18,6 +18,7 @@ import java.nio.file.Path; import java.util.ArrayList; import java.util.Collection; +import java.util.Map; import java.util.function.Supplier; import com.google.common.collect.Lists; @@ -30,11 +31,18 @@ import org.opensearch.action.admin.indices.create.CreateIndexResponse; import org.opensearch.action.admin.indices.exists.indices.IndicesExistsRequest; import org.opensearch.action.admin.indices.exists.indices.IndicesExistsResponse; +import org.opensearch.action.admin.indices.refresh.RefreshResponse; +import org.opensearch.action.index.IndexRequest; +import org.opensearch.action.index.IndexResponse; +import org.opensearch.action.search.SearchRequest; +import org.opensearch.action.search.SearchResponse; import org.opensearch.cluster.health.ClusterHealthStatus; import org.opensearch.cluster.metadata.IndexNameExpressionResolver; import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.settings.Settings; +import org.opensearch.common.util.concurrent.ThreadContext; import org.opensearch.core.common.io.stream.NamedWriteableRegistry; +import org.opensearch.core.rest.RestStatus; import org.opensearch.core.xcontent.NamedXContentRegistry; import org.opensearch.env.Environment; import org.opensearch.env.NodeEnvironment; @@ -55,12 +63,13 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.is; +import static org.opensearch.action.support.WriteRequest.RefreshPolicy.IMMEDIATE; public class RolesInjectorIntegTest extends SingleClusterTest { - public static class RolesInjectorPlugin extends Plugin implements ActionPlugin { Settings settings; public static String injectedRoles = null; + public static Map injectedCustomAttributes = null; public RolesInjectorPlugin(final Settings settings, final Path configPath) { this.settings = settings; @@ -82,6 +91,8 @@ public Collection createComponents( ) { if (injectedRoles != null) threadPool.getThreadContext() .putTransient(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES, injectedRoles); + if (injectedCustomAttributes != null) threadPool.getThreadContext() + .putTransient(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_USER_CUSTOM_ATTRIBUTES, injectedCustomAttributes); return new ArrayList<>(); } } @@ -172,5 +183,83 @@ public void testRolesInject() throws Exception { IndicesExistsResponse ier = node.client().admin().indices().exists(new IndicesExistsRequest("captain-logs-3")).actionGet(); Assert.assertTrue(ier.isExists()); } + + // 4. With a role using DLS and attribute substitution, but with no attributes specified in the thread context + RolesInjectorPlugin.injectedRoles = "valid_user|role_with_dls"; + try ( + Node node = new PluginAwareNode( + false, + tcSettings, + Lists.newArrayList(Netty4ModulePlugin.class, OpenSearchSecurityPlugin.class, RolesInjectorPlugin.class) + ).start() + ) { + waitForInit(node.client()); + + CreateIndexResponse cir = node.client().admin().indices().create(new CreateIndexRequest("captain-logs-4")).actionGet(); + Assert.assertTrue(cir.isAcknowledged()); + } catch (OpenSearchSecurityException ex) { + exception = ex; + log.warn(ex.toString()); + } + Assert.assertNotNull(exception); + Assert.assertTrue(exception.getMessage().contains("Error while evaluating DLS/FLS privileges")); + + // 5. With a role using DLS and attribute substitution and with attributes specified in the thread context + // First we need to use a role without DLS to write data to the index. Roles with DLS restrictions cannot perform writes. + RolesInjectorPlugin.injectedRoles = "valid_user|role_without_dls_write"; + try ( + Node node = new PluginAwareNode( + false, + tcSettings, + Lists.newArrayList(Netty4ModulePlugin.class, OpenSearchSecurityPlugin.class, RolesInjectorPlugin.class) + ).start() + ) { + waitForInit(node.client()); + + CreateIndexResponse cir = node.client().admin().indices().create(new CreateIndexRequest("captain-logs-5")).actionGet(); + Assert.assertTrue(cir.isAcknowledged()); + + IndicesExistsResponse ier = node.client().admin().indices().exists(new IndicesExistsRequest("captain-logs-5")).actionGet(); + Assert.assertTrue(ier.isExists()); + + Map document = Map.of("starship", "enterprise"); + Map document2 = Map.of("starship", "voyager"); + + IndexResponse idr = node.client() + .index(new IndexRequest().setRefreshPolicy(IMMEDIATE).index("captain-logs-5").id("1").source(document)) + .actionGet(); + Assert.assertEquals(idr.status(), RestStatus.CREATED); + + IndexResponse idr2 = node.client() + .index(new IndexRequest().setRefreshPolicy(IMMEDIATE).index("captain-logs-5").id("2").source(document2)) + .actionGet(); + Assert.assertEquals(idr2.status(), RestStatus.CREATED); + + RefreshResponse rer = node.client().admin().indices().prepareRefresh("captain-logs-5").get(); + Assert.assertEquals(rer.getStatus(), RestStatus.OK); + + SearchResponse ser = clusterHelper.nodeClient().search(new SearchRequest("captain-logs-5")).actionGet(); + Assert.assertEquals(RestStatus.OK, ser.status()); + Assert.assertEquals(2, ser.getHits().getTotalHits().value()); + Assert.assertTrue(ser.toString().contains("enterprise")); + Assert.assertTrue(ser.toString().contains("voyager")); + + // Now use a role with DLS and custom attributes to test that attribute substitution works + // and searched documents are filtered correctly. + ThreadPool tp = clusterHelper.nodeClient().threadPool(); + try (ThreadContext.StoredContext ignored = tp.getThreadContext().stashContext()) { + ThreadContext tc = tp.getThreadContext(); + tc.putTransient(ConfigConstants.OPENDISTRO_SECURITY_INJECTED_ROLES, "valid_user|role_with_dls"); + tc.putTransient( + ConfigConstants.OPENDISTRO_SECURITY_INJECTED_USER_CUSTOM_ATTRIBUTES, + Map.of("attr.proxy.starship", "enterprise") + ); + + SearchResponse serDls = clusterHelper.nodeClient().search(new SearchRequest("captain-logs-5")).actionGet(); + Assert.assertEquals(RestStatus.OK, serDls.status()); + Assert.assertEquals(1, serDls.getHits().getTotalHits().value()); + Assert.assertTrue(serDls.toString().contains("enterprise")); + } + } } } diff --git a/src/test/java/org/opensearch/security/privileges/UserAttributesUnitTest.java b/src/test/java/org/opensearch/security/privileges/UserAttributesUnitTest.java new file mode 100644 index 0000000000..05da8d5419 --- /dev/null +++ b/src/test/java/org/opensearch/security/privileges/UserAttributesUnitTest.java @@ -0,0 +1,74 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + * + * Modifications Copyright OpenSearch Contributors. See + * GitHub history for details. + */ +package org.opensearch.security.privileges; + +import java.util.List; +import java.util.Map; + +import com.google.common.collect.ImmutableSet; +import org.junit.Test; + +import org.opensearch.security.user.User; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +public class UserAttributesUnitTest { + @Test + public void testNeedsAttributeSubstitution() { + assertTrue(UserAttributes.needsAttributeSubstitution("{\"foo\": \"${user.name}}\"")); + assertTrue(UserAttributes.needsAttributeSubstitution("${attr1.proxy.foo}")); + assertFalse(UserAttributes.needsAttributeSubstitution("{\"foo\": \"bar\"}")); + } + + @Test + public void testReplaceProperties() { + User user = new User("test_user").withAttributes(Map.of("attr.proxy.attr1", "value1")) + .withSecurityRoles(List.of("role1")) + .withRoles(List.of("role2")); + PrivilegesEvaluationContext ctx = new PrivilegesEvaluationContext( + user, + ImmutableSet.copyOf(List.of("mapped_role1")), + null, + null, + null, + null, + null, + null, + null + ); + + String stringWithPlaceholders = """ + { + "name": "${user.name}", + "name2": "${user_name}", + "bar": "${attr.proxy.attr1}", + "roles": [${user.roles}], + "roles2": [${user_roles}], + "security_roles": [${user.securityRoles}], + "security_roles2": [${user_securityRoles}], + } + """; + String expectedString = """ + { + "name": "test_user", + "name2": "test_user", + "bar": "value1", + "roles": ["role2"], + "roles2": ["role2"], + "security_roles": ["role1","mapped_role1"], + "security_roles2": ["role1","mapped_role1"], + } + """; + assertEquals(expectedString, UserAttributes.replaceProperties(stringWithPlaceholders, ctx)); + } +} diff --git a/src/test/resources/roles.yml b/src/test/resources/roles.yml index 47371e1abe..a937062879 100644 --- a/src/test/resources/roles.yml +++ b/src/test/resources/roles.yml @@ -1411,3 +1411,35 @@ who_am_i: hidden: false description: "Test role to grant access to /whoami endpoint" cluster_permissions: [ "security:whoamiprotected" ] + +role_with_dls: + reserved: false + hidden: false + cluster_permissions: + - "read" + index_permissions: + - index_patterns: + - "captain-logs-*" + dls: "{\"term\": {\"starship\": \"${attr.proxy.starship}\" }}" + fls: null + allowed_actions: + - "indices:admin/create" + - "indices:admin/exists" + - "read" + tenant_permissions: [] + static: false + +role_without_dls_write: + reserved: false + hidden: false + cluster_permissions: + - "cluster_composite_ops" + index_permissions: + - index_patterns: + - "captain-logs-*" + dls: null + fls: null + allowed_actions: + - "indices_all" + tenant_permissions: [] + static: false