Immutable user object

[nibix]

Description

Fixes #5168

This PR makes the User object immutable. This brings a number of significant advantages:

• An immutable object means that the binary data created by serialization does not change as well. That means, the serialized data can be computed once per user object and then re-used again and again. The existing authentication cache even allows us to re-use the serialized data across different requests by the same user.

• Additionally, a cache similar to the cache maintained by BackendRegistry can be utilized to also cut down the number of de-serialization operations to a single one per user per node. Such a cache would map the serialized binary data to the actual user object.

• Immutable objects are inherently thread-safe. That makes any synchronization or locking mechanisms on the user object unnecessary.

• Category:
  • Enhancement
• Why these changes are required?
  • The serialization of the user object takes significant time, reducing the throughput. The changes help to minimize serialization operations. Additionally, they make the code more mature.
• What is the old behavior before changes and new behavior after changes?
  • No behavioral changes

Issues Resolved

Fixes #5168

Is this a backport? If so, please add backport PR # and/or commits #, and remove backport-failed label from the original PR.

Do these changes introduce new permission(s) to be displayed in the static dropdown on the front-end? If so, please open a draft PR in the security dashboards plugin and link the draft PR here

Testing

• Unit testing for parsing of serialized user object (from OpenSearch 2.19)
• Integration testing

Check List

• New functionality includes testing
• Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[nibix]

Benchmark Results

We did some initial benchmarks on the new implementation, here are the results.

Disclaimer

The usual benchmarking disclaimer: Of course, these figures can only represent very specific scenarios. Other scenarios can look quite different, as there are so many variables which can be different. Yet, especially the "slow parts" of the benchmark results can give one an impression where real performance issues are.

Test setup

• We used OpenSearch 3.0.0-alpha1 for all tests
• All plugins were removed, except the security plugin
• Audit logging was disabled
• Authentication: Only the internal backend auth was active. All other authentication types were disabled.

K8s resources per node

• Java Heap: 32 GB
• RAM: 64 GB
• CPU: 8 cores
• Kubernetes version: v1.32.2

Tested dimensions

Users

We compared four different users:

• User with 10 internal attributes
• User with 100 internal attributes
• User with 1000 internal attributes
• User authenticated by admin certificate

The number of internal attributes influences the size of the user object, both unserialized and serialized. Thus, it influences the resources needed for serialization, deserialization and also the network traffic for inter-node communication. It would have been also possible to test with different numbers of roles. Both variables would influence the serialization performance in similar ways.

Users with 1000 attributes might be an extreme case. Still, especially users authenticated by Active Directory/LDAP might get (sometimes unknown to administrators) quite a lot of attributes from the LDAP backend.

The user authenticated by admin certificate bypasses most parts of the security plugin code (although not the serialization code) and can be thus seen as a upper threshold for what can be achieved.

Operations

We tested four different operations:

• Bulk indexing with 10 items per request
• Bulk indexing with 100 items per request
• Searching on single index
• Searching on 20 indices (using index pattern)

Test results

A commented summary of the results follows in the upcoming sections.

The raw test results can be also reviewed at https://docs.google.com/spreadsheets/d/1HeJ8HFOqF5S4FEDiLXZ4d8eniU9UCp_-FJ3XBrojVME/edit?usp=sharing

Indexing throughput

Bulk size 10

In this and the following charts, the dashed lines represent OpenSearch with the standard security plugin. The solid lines represent OpenSearch with the security plugin with the optimized privilege evaluation code.

The blue line represents requests authenticated by the super admin certificate, which by-passes most of the security plugin. Thus, the blue line forms a kind of "hull curve", it can be seen as a rough theoretical maximum from a security plugin point of view.

We see that users with 10 attributes experience a throughput improvement of about 6% with the new implementation. Users with 100 attributes get an 18% improvement. The extreme case where users have 1000 attributes sees a 46% improvement.

While the new implementation shows significant improvements, we still see a declining throughput with a growing number of user attributes. This is expected, as we only optimize the process of serialization/deserialization. The increased network load stays unchanged compared to the old implementation.

Bulk size 100

Bulk requests with more items can be processed more efficiently. Thus, as we are looking at a docs/s throughput, the improvements are a bit smaller in this case.

Users with 10 attributes experience a 2.5% improvement with the new implementation. Users with 100 attributes see an 8% improvement. For 1000 attributes, we get a 32% improvement.

Search throughput

Single index

In this benchmark, we compare the operation throughput for search requests performed on a single index.

For users with 10 attributes, we only see a small throughput improvement of 1.2%. Users with 100 attributes get an improvement of 13%. Users with 1000 attributes get an improvement of 83%.

20 indices

This benchmark tests searches operations that search through 20 indices at once (specified by an index pattern).

Compared to the single index case, we see much more pronounced improvements. This is likely due to the increased inter-node communication required by searching on several indices with several shards distributed over a cluster.

Users with 10 attributes see a 21% improvement. Users with 100 attributes get a throughput that is 39% higher than the on the old version. Users with 1000 attributes see an improvement of 384%.

@cwperks @kkhatua

[nibix]

@cwperks @kkhatua @DarshitChanpura

I'd like to hear an opinion from you regarding the following:

In the immutable user object implementation, we need to use the following quite inefficient code to create serialized forms of the user object that are backwards compatible. That's especially the wrapping of collections inside Collections.synchronizedSet(new HashSet<>(...)). The new code does not need this any more, but nodes on old versions need this.

security/src/main/java/org/opensearch/security/user/User.java

Lines 361 to 384 in 7a49f7c

	@Serial
	private static final ObjectStreamField[] serialPersistentFields = {
	new ObjectStreamField("name", String.class),
	new ObjectStreamField("roles", Collections.synchronizedSet(Collections.emptySet()).getClass()),
	new ObjectStreamField("securityRoles", Collections.synchronizedSet(Collections.emptySet()).getClass()),
	new ObjectStreamField("requestedTenant", String.class),
	new ObjectStreamField("attributes", Collections.synchronizedMap(Collections.emptyMap()).getClass()),
	new ObjectStreamField("isInjected", Boolean.TYPE) };
	/**
	* Creates a backwards compatible object that can be used for serialization
	*/
	@Serial
	private void writeObject(ObjectOutputStream out) throws IOException {
	ObjectOutputStream.PutField fields = out.putFields();
	fields.put("name", name);
	fields.put("roles", Collections.synchronizedSet(new HashSet<>(this.roles)));
	fields.put("securityRoles", Collections.synchronizedSet(new HashSet<>(this.securityRoles)));
	fields.put("requestedTenant", requestedTenant);
	fields.put("attributes", Collections.synchronizedMap(new HashMap<>(this.attributes)));
	fields.put("isInjected", this.isInjected);
	out.writeFields();
	}

Due to this, I am leaning towards again using a custom serialization protocol for the immutable user object. Thus, in perspective, we could retire the wrapping inside Collections.synchronizedSet(new HashSet<>(...)). I am aware of the previous issues with custom serialization formats. However, in my perception, the issues all came from the huge data structures created by DLS/FLS. The user object, on the other hand, has a quite bounded size. Thus, I believe that this would work well and bring us benefits.

[cwperks]

However, in my perception, the issues all came from the huge data structures created by DLS/FLS.

yes, that was the case and led to large duplication when using custom serialization for that data structure: opensearch-project/OpenSearch#11430

Due to this, I am leaning towards again using a custom serialization protocol for the immutable user object.

+1 to this if you are seeing noticeable improvements with the new format.

[nibix]

@cwperks

+1 to this if you are seeing noticeable improvements with the new format.

I believe the performance improvements will be noticeable in graphs, but not as pronounced as the improvements we see already now.

I rather look at this from a code quality perspective: Without changing the protocol at one point, we have to carry the baggage of doing this ...

fields.put("roles", Collections.synchronizedSet(new HashSet<>(this.roles))); 
fields.put("securityRoles", Collections.synchronizedSet(new HashSet<>(this.securityRoles))); 
fields.put("attributes", Collections.synchronizedMap(new HashMap<>(this.attributes)));

... without a real reason to use Collections.synchronizedSet() and Collections.synchronizedMap() other than backwards compatibility - forever. Such "magic code" (in the terms of "do not touch the following code, even though it fulfils no real function") is IMHO something one should avoid.

[codecov]

Codecov Report

Attention: Patch coverage is 86.47799% with 43 lines in your changes missing coverage. Please review.

Project coverage is 71.95%. Comparing base (0cd23e7) to head (1dadf8a).
Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
.../org/opensearch/security/user/serialized/User.java	53.33%	5 Missing and 2 partials ⚠️
...y/auth/internal/InternalAuthenticationBackend.java	78.57%	6 Missing ⚠️
.../org/opensearch/security/auth/BackendRegistry.java	80.00%	3 Missing and 2 partials ⚠️
...y/auth/ldap/backend/LDAPAuthenticationBackend.java	82.60%	2 Missing and 2 partials ⚠️
...c/main/java/org/opensearch/security/user/User.java	93.84%	1 Missing and 3 partials ⚠️
...ecurity/auth/ldap2/LDAPAuthenticationBackend2.java	86.36%	3 Missing ⚠️
...security/auth/ldap2/LDAPAuthorizationBackend2.java	76.92%	3 Missing ⚠️
...ch/security/securityconf/DynamicConfigFactory.java	25.00%	1 Missing and 2 partials ⚠️
.../main/java/com/amazon/dlic/auth/ldap/LdapUser.java	33.33%	2 Missing ⚠️
...java/org/opensearch/security/user/UserFactory.java	88.88%	1 Missing and 1 partial ⚠️
... and 4 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5212      +/-   ##
==========================================
+ Coverage   71.90%   71.95%   +0.04%     
==========================================
  Files         380      383       +3     
  Lines       23677    23725      +48     
  Branches     3636     3628       -8     
==========================================
+ Hits        17026    17071      +45     
- Misses       4836     4854      +18     
+ Partials     1815     1800      -15     

Files with missing lines	Coverage Δ
.../opensearch/security/OpenSearchSecurityPlugin.java	84.68% <100.00%> (+0.05%)	⬆️
...earch/security/auditlog/impl/AbstractAuditLog.java	76.68% <100.00%> (+0.05%)	⬆️
...pensearch/security/auditlog/impl/AuditLogImpl.java	89.28% <100.00%> (ø)
...pensearch/security/auth/AuthenticationContext.java	100.00% <100.00%> (ø)
...va/org/opensearch/security/auth/RolesInjector.java	92.00% <100.00%> (+3.53%)	⬆️
...urity/auth/internal/NoOpAuthenticationBackend.java	100.00% <100.00%> (ø)
...earch/security/dlic/rest/api/AccountApiAction.java	98.64% <ø> (ø)
...org/opensearch/security/filter/SecurityFilter.java	66.98% <100.00%> (+0.47%)	⬆️
...earch/security/privileges/PrivilegesEvaluator.java	74.04% <100.00%> (+0.07%)	⬆️
...opensearch/security/privileges/UserAttributes.java	80.00% <ø> (+8.57%)	⬆️
... and 22 more

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[nibix]

@cwperks @kkhatua @DarshitChanpura @shikharj05 @kumargu

This should be now ready, I would be curious for your opinions :)

Regarding the serialization format discussed in #5212 (comment) : I did not change anything in this regard so far. If we want to change it, we can do it both in this PR or also in a separate PR.

[kumargu]

I'll start taking a look at this tomorrow.

[willyborankin]

Thank you, @nibix, for the excellent work. If all comments have been addressed, I'll go ahead and merge it