Inject user custom attributes (#5560)

Signed-off-by: Mark Boyd <[email protected]>

Author: markdboyd

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
 

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);

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<String> injectUserAndRoles(final ThreadPool threadPool) {
             return null;
         }
         Set<String> roles = ImmutableSet.copyOf(strs[1].split(","));
-        User user = new User(strs[0]).withSecurityRoles(roles);
+
+        Map<String, String> 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;

src/main/java/org/opensearch/security/privileges/IndexPattern.java

@@ -220,7 +220,7 @@ public void add(List<String> 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<String> source) {
         Builder builder = new Builder();
         builder.add(source);

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());

src/main/java/org/opensearch/security/privileges/TenantPrivileges.java

@@ -104,7 +104,7 @@ public TenantPrivileges(
                 for (RoleV7.Tenant tenantPermissions : role.getTenant_permissions()) {
                     List<ActionType> 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<ActionType> resolveActionType(Collection<String> allowedActions, Fla
         }
     }
 
-    static boolean isDynamic(String tenantPattern) {
-        return tenantPattern.contains("${");
-    }
-
     private static ImmutableMap<ActionType, ImmutableCompactSubSet<String>> build(
         Map<ActionType, DeduplicatingCompactSubSetBuilder.SubSetBuilder<String>> source,
         DeduplicatingCompactSubSetBuilder.Completed<String> completedRoleSetBuilder

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();
 

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<DocumentPrivileges.DlsQuery, DlsRestriction> {
-
     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) {

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";
 

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<String, String> injectedCustomAttributes = null;
 
         public RolesInjectorPlugin(final Settings settings, final Path configPath) {
             this.settings = settings;
@@ -82,6 +91,8 @@ public Collection<Object> 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<String, String> document = Map.of("starship", "enterprise");
+            Map<String, String> 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"));
+            }
+        }
     }
 }