Immutable user object

CHANGELOG.md

@@ -12,6 +12,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 ### Changed
 - Use extendedPlugins in integrationTest framework for sample resource plugin testing ([#5322](https://github.com/opensearch-project/security/pull/5322))
 
+- Performance improvements: Immutable user object ([#5212])
+
 ### Dependencies
 - Bump `guava_version` from 33.4.6-jre to 33.4.8-jre ([#5284](https://github.com/opensearch-project/security/pull/5284))
 - Bump `spring_version` from 6.2.5 to 6.2.6 ([#5283](https://github.com/opensearch-project/security/pull/5283))

src/integrationTest/java/org/opensearch/security/privileges/ActionPrivilegesTest.java

@@ -1071,8 +1071,7 @@ static SecurityDynamicConfiguration<RoleV7> createRoles(int numberOfRoles, int n
     }
 
     static PrivilegesEvaluationContext ctx(String... roles) {
-        User user = new User("test_user");
-        user.addAttributes(ImmutableMap.of("attrs.dept_no", "a11"));
+        User user = new User("test_user").withAttributes(ImmutableMap.of("attrs.dept_no", "a11"));
         return new PrivilegesEvaluationContext(
             user,
             ImmutableSet.copyOf(roles),
@@ -1086,8 +1085,7 @@ static PrivilegesEvaluationContext ctx(String... roles) {
     }
 
     static PrivilegesEvaluationContext ctxByUsername(String username) {
-        User user = new User(username);
-        user.addAttributes(ImmutableMap.of("attrs.dept_no", "a11"));
+        User user = new User(username).withAttributes(ImmutableMap.of("attrs.dept_no", "a11"));
         return new PrivilegesEvaluationContext(
             user,
             ImmutableSet.of(),

src/integrationTest/java/org/opensearch/security/privileges/IndexPatternTest.java

@@ -234,10 +234,7 @@ public void equals() {
     private static PrivilegesEvaluationContext ctx() {
         IndexNameExpressionResolver indexNameExpressionResolver = new IndexNameExpressionResolver(new ThreadContext(Settings.EMPTY));
         IndexResolverReplacer indexResolverReplacer = new IndexResolverReplacer(indexNameExpressionResolver, () -> CLUSTER_STATE, null);
-        User user = new User("test_user");
-        user.addAttributes(ImmutableMap.of("attrs.a11", "a11"));
-        user.addAttributes(ImmutableMap.of("attrs.year", "year"));
-
+        User user = new User("test_user").withAttributes(ImmutableMap.of("attrs.a11", "a11", "attrs.year", "year"));
         return new PrivilegesEvaluationContext(
             user,
             ImmutableSet.of(),

src/integrationTest/java/org/opensearch/security/privileges/dlsfls/DocumentPrivilegesTest.java

@@ -1191,9 +1191,7 @@ UserSpec attribute(String name, String value) {
         }
 
         User buildUser() {
-            User user = new User("test_user_" + description);
-            user.addAttributes(this.attributes);
-            return user;
+            return new User("test_user_" + description).withAttributes(this.attributes);
         }
 
         @Override

src/integrationTest/java/org/opensearch/security/user/UserTest.java

@@ -0,0 +1,79 @@
+/*
+ * 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.user;
+
+import java.util.Arrays;
+
+import com.google.common.collect.ImmutableMap;
+import org.junit.Test;
+
+import org.opensearch.security.support.Base64Helper;
+
+import static org.junit.Assert.assertEquals;
+
+public class UserTest {
+    @Test
+    public void serialization() {
+        User user = new User("serialization_test_user").withRoles(Arrays.asList("br1", "br2", "br3"))
+            .withSecurityRoles(Arrays.asList("sr1", "sr2"))
+            .withAttributes(ImmutableMap.of("a", "v_a", "b", "v_b"));
+
+        String serialized = Base64Helper.serializeObject(user);
+        User user2 = User.fromSerializedBase64(serialized);
+        assertEquals(user, user2);
+
+    }
+
+    @Test
+    public void deserializationFrom2_19() {
+        // The following base64 string was produced by the following code on OpenSearch 2.19
+        // User user = new User("serialization_test_user");
+        // user.addRoles(Arrays.asList("br1", "br2", "br3"));
+        // user.addSecurityRoles(Arrays.asList("sr1", "sr2"));
+        // user.addAttributes(ImmutableMap.of("a", "v_a", "b", "v_b"));
+        // println(Base64JDKHelper.serializeObject(user));
+        String serialized =
+            "rO0ABXNyACFvcmcub3BlbnNlYXJjaC5zZWN1cml0eS51c2VyLlVzZXKzqL2T65dH3AIABloACmlzSW5qZWN0ZWRMAAphdHRyaWJ1dGVzdAAPTGphdmEvdXRpbC9NYXA7TAAEbmFtZXQAEkxqYXZhL2xhbmcvU3RyaW5nO0wAD3JlcXVlc3RlZFRlbmFudHEAfgACTAAFcm9sZXN0AA9MamF2YS91dGlsL1NldDtMAA1zZWN1cml0eVJvbGVzcQB+AAN4cABzcgAlamF2YS51dGlsLkNvbGxlY3Rpb25zJFN5bmNocm9uaXplZE1hcBtz+QlLSzl7AwACTAABbXEAfgABTAAFbXV0ZXh0ABJMamF2YS9sYW5nL09iamVjdDt4cHNyABFqYXZhLnV0aWwuSGFzaE1hcAUH2sHDFmDRAwACRgAKbG9hZEZhY3RvckkACXRocmVzaG9sZHhwP0AAAAAAAAN3CAAAAAQAAAACdAABYXQAA3ZfYXQAAWJ0AAN2X2J4cQB+AAd4dAAXc2VyaWFsaXphdGlvbl90ZXN0X3VzZXJwc3IAJWphdmEudXRpbC5Db2xsZWN0aW9ucyRTeW5jaHJvbml6ZWRTZXQGw8J5Au7fPAIAAHhyACxqYXZhLnV0aWwuQ29sbGVjdGlvbnMkU3luY2hyb25pemVkQ29sbGVjdGlvbiph+E0JnJm1AwACTAABY3QAFkxqYXZhL3V0aWwvQ29sbGVjdGlvbjtMAAVtdXRleHEAfgAGeHBzcgARamF2YS51dGlsLkhhc2hTZXS6RIWVlri3NAMAAHhwdwwAAAAQP0AAAAAAAAN0AANicjF0AANicjN0AANicjJ4cQB+ABJ4c3EAfgAPc3EAfgATdwwAAAAQP0AAAAAAAAJ0AANzcjJ0AANzcjF4cQB+ABh4";
+
+        User user = User.fromSerializedBase64(serialized);
+        assertEquals(
+            new User("serialization_test_user").withRoles(Arrays.asList("br1", "br2", "br3"))
+                .withSecurityRoles(Arrays.asList("sr1", "sr2"))
+                .withAttributes(ImmutableMap.of("a", "v_a", "b", "v_b")),
+            user
+        );
+    }
+
+    @Test
+    public void deserializationLdapUserFrom2_19() {
+        // The following base64 string was produced by the following code on OpenSearch 2.19
+        // LdapUser user = new LdapUser("serialization_test_user",
+        // "original_user_name",
+        // new LdapEntry("cn=test,ou=people,o=TEST", new LdapAttribute("test_ldap_attr", "test_ldap_attr_value")),
+        // new AuthCredentials("test_user", "secret".getBytes(StandardCharsets.UTF_8)),
+        // 100,
+        // WildcardMatcher.ANY);
+        // user.addRoles(Arrays.asList("br1", "br2", "br3"));
+        // user.addSecurityRoles(Arrays.asList("sr1", "sr2"));
+        // user.addAttributes(ImmutableMap.of("a", "v_a", "b", "v_b"));
+        // println(Base64JDKHelper.serializeObject(user));
+        String serialized =
+            "rO0ABXNyACJjb20uYW1hem9uLmRsaWMuYXV0aC5sZGFwLkxkYXBVc2VyAAAAAAAAAAECAAFMABBvcmlnaW5hbFVzZXJuYW1ldAASTGphdmEvbGFuZy9TdHJpbmc7eHIAIW9yZy5vcGVuc2VhcmNoLnNlY3VyaXR5LnVzZXIuVXNlcrOovZPrl0fcAgAGWgAKaXNJbmplY3RlZEwACmF0dHJpYnV0ZXN0AA9MamF2YS91dGlsL01hcDtMAARuYW1lcQB+AAFMAA9yZXF1ZXN0ZWRUZW5hbnRxAH4AAUwABXJvbGVzdAAPTGphdmEvdXRpbC9TZXQ7TAANc2VjdXJpdHlSb2xlc3EAfgAEeHAAc3IAJWphdmEudXRpbC5Db2xsZWN0aW9ucyRTeW5jaHJvbml6ZWRNYXAbc/kJS0s5ewMAAkwAAW1xAH4AA0wABW11dGV4dAASTGphdmEvbGFuZy9PYmplY3Q7eHBzcgARamF2YS51dGlsLkhhc2hNYXAFB9rBwxZg0QMAAkYACmxvYWRGYWN0b3JJAAl0aHJlc2hvbGR4cD9AAAAAAAAGdwgAAAAIAAAABXQAB2xkYXAuZG50ABhjbj10ZXN0LG91PXBlb3BsZSxvPVRFU1R0AAFhdAADdl9hdAAYYXR0ci5sZGFwLnRlc3RfbGRhcF9hdHRydAAUdGVzdF9sZGFwX2F0dHJfdmFsdWV0AAFidAADdl9idAAWbGRhcC5vcmlnaW5hbC51c2VybmFtZXQAEm9yaWdpbmFsX3VzZXJfbmFtZXhxAH4ACHh0ABdzZXJpYWxpemF0aW9uX3Rlc3RfdXNlcnBzcgAlamF2YS51dGlsLkNvbGxlY3Rpb25zJFN5bmNocm9uaXplZFNldAbDwnkC7t88AgAAeHIALGphdmEudXRpbC5Db2xsZWN0aW9ucyRTeW5jaHJvbml6ZWRDb2xsZWN0aW9uKmH4TQmcmbUDAAJMAAFjdAAWTGphdmEvdXRpbC9Db2xsZWN0aW9uO0wABW11dGV4cQB+AAd4cHNyABFqYXZhLnV0aWwuSGFzaFNldLpEhZWWuLc0AwAAeHB3DAAAABA/QAAAAAAAA3QAA2JyMXQAA2JyM3QAA2JyMnhxAH4AGXhzcQB+ABZzcQB+ABp3DAAAABA/QAAAAAAAAnQAA3NyMnQAA3NyMXhxAH4AH3hxAH4AFA==";
+
+        User user = User.fromSerializedBase64(serialized);
+        assertEquals(
+            new User("serialization_test_user").withRoles(Arrays.asList("br1", "br2", "br3"))
+                .withSecurityRoles(Arrays.asList("sr1", "sr2"))
+                .withAttributes(ImmutableMap.of("a", "v_a", "b", "v_b")),
+            user
+        );
+    }
+}

src/main/java/com/amazon/dlic/auth/ldap/LdapUser.java

@@ -11,91 +11,42 @@
 
 package com.amazon.dlic.auth.ldap;
 
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.Map;
-
-import org.opensearch.security.auth.ldap.util.Utils;
-import org.opensearch.security.support.WildcardMatcher;
-import org.opensearch.security.user.AuthCredentials;
-import org.opensearch.security.user.User;
-
-import org.ldaptive.LdapAttribute;
-import org.ldaptive.LdapEntry;
-
 /**
  * This class intentionally remains in the com.amazon.dlic.auth.ldap package
  * to maintain compatibility with serialization/deserialization in mixed cluster
  * environments (nodes running different versions). The class is serialized and
  * passed between nodes, and changing the package would break backward compatibility.
  *
- * Note: This class is planned to be replaced as part of making the User object
- * immutable in a future release or reconsidering java serialization.
+ * This class is only used for deserialization. During deserialization, the readResolve()
+ * method will automatically convert it to a org.opensearch.security.user.User user object.
+ * It will never be used for serialization, only the org.opensearch.security.user.User user object
+ * will be serialized. This is possible because the additional attributes of LdapUser were only
+ * needed during the auth/auth phase, where no inter-node communication is necessary. Afterwards,
+ * the user object is never used as LdapUser, but just as a plain User object.
+ *
+ * This class can be removed as soon as it is no longer possible that a mixed cluster can contain
+ * nodes which send serialized LdapUser objects. This will be the case for OpenSearch 4.0.
  *
  * @see https://github.com/opensearch-project/security/pull/5223
  */
-public class LdapUser extends User {
+public class LdapUser extends org.opensearch.security.user.serialized.User {
 
     private static final long serialVersionUID = 1L;
-    private final transient LdapEntry userEntry;
     private final String originalUsername;
 
-    public LdapUser(
-        final String name,
-        String originalUsername,
-        final LdapEntry userEntry,
-        final AuthCredentials credentials,
-        int customAttrMaxValueLen,
-        WildcardMatcher allowlistedCustomLdapAttrMatcher
-    ) {
-        super(name, null, credentials);
-        this.originalUsername = originalUsername;
-        this.userEntry = userEntry;
-        Map<String, String> attributes = getCustomAttributesMap();
-        attributes.putAll(extractLdapAttributes(originalUsername, userEntry, customAttrMaxValueLen, allowlistedCustomLdapAttrMatcher));
+    public LdapUser() {
+        this.originalUsername = null;
     }
 
     /**
-     * May return null because ldapEntry is transient
-     *
-     * @return ldapEntry or null if object was deserialized
+     * Converts this objects back to User, just after deserialization.
+     * <p>
+     * Note: We do not convert back to LdapUser, but just to User. The additional attributes of
+     * LdapUser were only needed during the auth/auth phase, where no inter-node communication
+     * is necessary. Afterwards, the user object is never used as LdapUser, but just as a plain User
+     * object.
      */
-    public LdapEntry getUserEntry() {
-        return userEntry;
-    }
-
-    public String getDn() {
-        return userEntry.getDn();
-    }
-
-    public String getOriginalUsername() {
-        return originalUsername;
-    }
-
-    public static Map<String, String> extractLdapAttributes(
-        String originalUsername,
-        final LdapEntry userEntry,
-        int customAttrMaxValueLen,
-        WildcardMatcher allowlistedCustomLdapAttrMatcher
-    ) {
-        Map<String, String> attributes = new HashMap<>();
-        attributes.put("ldap.original.username", originalUsername);
-        attributes.put("ldap.dn", userEntry.getDn());
-
-        if (customAttrMaxValueLen > 0) {
-            for (LdapAttribute attr : userEntry.getAttributes()) {
-                if (attr != null && !attr.isBinary() && !attr.getName().toLowerCase().contains("password")) {
-                    final String val = Utils.getSingleStringValue(attr);
-                    // only consider attributes which are not binary and where its value is not
-                    // longer than customAttrMaxValueLen characters
-                    if (val != null && val.length() > 0 && val.length() <= customAttrMaxValueLen) {
-                        if (allowlistedCustomLdapAttrMatcher.test(attr.getName())) {
-                            attributes.put("attr.ldap." + attr.getName(), val);
-                        }
-                    }
-                }
-            }
-        }
-        return Collections.unmodifiableMap(attributes);
+    protected Object readResolve() {
+        return super.readResolve();
     }
 }

src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java

@@ -213,6 +213,7 @@
 import org.opensearch.security.transport.InterClusterRequestEvaluator;
 import org.opensearch.security.transport.SecurityInterceptor;
 import org.opensearch.security.user.User;
+import org.opensearch.security.user.UserFactory;
 import org.opensearch.security.user.UserService;
 import org.opensearch.tasks.Task;
 import org.opensearch.telemetry.tracing.Tracer;
@@ -1118,14 +1119,16 @@ public Collection<Object> createComponents(
             interClusterRequestEvaluator = ReflectionHelper.instantiateInterClusterRequestEvaluator(className, settings);
         }
 
+        UserFactory userFactory = new UserFactory.Caching(settings);
+
         final PrivilegesInterceptor privilegesInterceptor;
 
         namedXContentRegistry.set(xContentRegistry);
         if (SSLConfig.isSslOnlyMode()) {
             auditLog = new NullAuditLog();
             privilegesInterceptor = new PrivilegesInterceptor(resolver, clusterService, localClient, threadPool);
         } else {
-            auditLog = new AuditLogImpl(settings, configPath, localClient, threadPool, resolver, clusterService, environment);
+            auditLog = new AuditLogImpl(settings, configPath, localClient, threadPool, resolver, clusterService, environment, userFactory);
             privilegesInterceptor = new PrivilegesInterceptorImpl(resolver, clusterService, localClient, threadPool);
         }
 
@@ -1226,7 +1229,8 @@ public Collection<Object> createComponents(
             Objects.requireNonNull(sslExceptionHandler),
             Objects.requireNonNull(cih),
             SSLConfig,
-            OpenSearchSecurityPlugin::isActionTraceEnabled
+            OpenSearchSecurityPlugin::isActionTraceEnabled,
+            userFactory
         );
         components.add(principalExtractor);
 
@@ -2142,6 +2146,10 @@ public List<Setting<?>> getSettings() {
                     Property.Filtered
                 )
             );
+
+            settings.add(UserFactory.Caching.MAX_SIZE);
+            settings.add(UserFactory.Caching.MAX_ENTRIES);
+            settings.add(UserFactory.Caching.EXPIRE_AFTER_ACCESS);
         }
 
         return settings;

src/main/java/org/opensearch/security/auditlog/impl/AbstractAuditLog.java

@@ -74,6 +74,7 @@
 import org.opensearch.security.support.Base64Helper;
 import org.opensearch.security.support.ConfigConstants;
 import org.opensearch.security.user.User;
+import org.opensearch.security.user.UserFactory;
 import org.opensearch.tasks.Task;
 import org.opensearch.threadpool.ThreadPool;
 import org.opensearch.transport.TransportRequest;
@@ -96,6 +97,7 @@ public abstract class AbstractAuditLog implements AuditLog {
     private final Environment environment;
     private AtomicBoolean externalConfigLogged = new AtomicBoolean();
     private final Set<String> ignoredUrlParams = new HashSet<>();
+    private final UserFactory userFactory;
 
     protected abstract void enableRoutes();
 
@@ -113,7 +115,8 @@ protected AbstractAuditLog(
         final ThreadPool threadPool,
         final IndexNameExpressionResolver resolver,
         final ClusterService clusterService,
-        final Environment environment
+        final Environment environment,
+        final UserFactory userFactory
     ) {
         super();
         this.threadPool = threadPool;
@@ -125,6 +128,7 @@ protected AbstractAuditLog(
             ConfigConstants.OPENDISTRO_SECURITY_DEFAULT_CONFIG_INDEX
         );
         this.environment = environment;
+        this.userFactory = userFactory;
     }
 
     protected void onAuditConfigFilterChanged(AuditConfig.Filter auditConfigFilter) {
@@ -785,7 +789,7 @@ private TransportAddress getRemoteAddress() {
     private String getUser() {
         User user = threadPool.getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER);
         if (user == null && threadPool.getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER) != null) {
-            user = (User) Base64Helper.deserializeObject(
+            user = this.userFactory.fromSerializedBase64(
                 threadPool.getThreadContext().getHeader(ConfigConstants.OPENDISTRO_SECURITY_USER_HEADER)
             );
         }

src/main/java/org/opensearch/security/auditlog/impl/AuditLogImpl.java

@@ -31,6 +31,7 @@
 import org.opensearch.security.auditlog.config.AuditConfig;
 import org.opensearch.security.auditlog.routing.AuditMessageRouter;
 import org.opensearch.security.filter.SecurityRequest;
+import org.opensearch.security.user.UserFactory;
 import org.opensearch.tasks.Task;
 import org.opensearch.threadpool.ThreadPool;
 import org.opensearch.transport.TransportRequest;
@@ -46,6 +47,9 @@ public final class AuditLogImpl extends AbstractAuditLog {
     private volatile boolean enabled;
     private final Thread shutdownHook;
 
+    /**
+     * Only used for testing.
+     */
     public AuditLogImpl(
         final Settings settings,
         final Path configPath,
@@ -54,7 +58,7 @@ public AuditLogImpl(
         final IndexNameExpressionResolver resolver,
         final ClusterService clusterService
     ) {
-        this(settings, configPath, clientProvider, threadPool, resolver, clusterService, null);
+        this(settings, configPath, clientProvider, threadPool, resolver, clusterService, null, new UserFactory.Simple());
     }
 
     @SuppressWarnings("removal")
@@ -65,9 +69,10 @@ public AuditLogImpl(
         final ThreadPool threadPool,
         final IndexNameExpressionResolver resolver,
         final ClusterService clusterService,
-        final Environment environment
+        final Environment environment,
+        final UserFactory userFactory
     ) {
-        super(settings, threadPool, resolver, clusterService, environment);
+        super(settings, threadPool, resolver, clusterService, environment, userFactory);
         this.settings = settings;
         this.messageRouter = new AuditMessageRouter(settings, clientProvider, threadPool, configPath, clusterService);
         this.messageRouterEnabled = this.messageRouter.isEnabled();

src/main/java/org/opensearch/security/auth/AuthenticationBackend.java

@@ -59,21 +59,10 @@ public interface AuthenticationBackend {
      * <p/>
      * Results of this method are normally cached so that we not need to query the backend for every authentication attempt.
      * <p/>
-     * @param The credentials to be validated, never null
+     * @param context The context of this authentication; contains the auth credentials
      * @return the authenticated User, never null
      * @throws OpenSearchSecurityException in case an authentication failure
      * (when credentials are incorrect, the user does not exist or the backend is not reachable)
      */
-    User authenticate(AuthCredentials credentials) throws OpenSearchSecurityException;
-
-    /**
-     *
-     * Lookup for a specific user in the authentication backend
-     *
-     * @param user The user for which the authentication backend should be queried. If the authentication backend supports
-     * user attributes in combination with impersonation the attributes needs to be added to user by calling {@code user.addAttributes()}
-     * @return true if the user exists in the authentication backend, false otherwise. Before return call {@code user.addAttributes()} as explained above.
-     */
-    boolean exists(User user);
-
+    User authenticate(AuthenticationContext context) throws OpenSearchSecurityException;
 }