Use User directly as authenticated Subject

[cwperks]

Description

Make the Security plugin user model implement Core Subject and Principal directly, store User in persistent thread context, and remove the redundant UserSubjectImpl wrapper.

The removed wrapper only supplied user-level runAs() behavior, and no production caller uses it. Existing plugin execution remains unchanged: SecurePluginSubject continues to provide runAs() with the plugin identity and thread-context handling.

This prepares Security for opensearch-project/OpenSearch#22796, which removes the unused Core UserSubject specialization. This PR remains compatible with the current Core API and can merge first.

Testing

./gradlew spotlessApply compileJava test --tests 'org.opensearch.security.user.UserTests' --tests 'org.opensearch.security.resources.ResourceAccessHandlerTests' --tests 'org.opensearch.security.privileges.ResourceAccessEvaluatorTest' --tests 'org.opensearch.security.identity.SecurePluginSubjectTests' -Pcrypto.standard=FIPS-140-3

./gradlew precommit -Pcrypto.standard=FIPS-140-3 reaches an unrelated existing forbidden-APIs failure in opensearch-sample-resource-plugin for two uses of URL.openStream().

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit c537928)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Behavior Change The comparison origUser.equals(authSubject) compares a User to a User now (previously authSubject.getUser()). Since User now implements Principal, ensure User.equals semantics still match the intent (same user identity). If User.equals includes tenant/attributes that may differ between the transient USER and persistent AUTHENTICATED_USER (e.g., after withRequestedTenant), the SAME_AS_SUBJECT_HEADER short-circuit may not trigger when it previously did, causing the full auth subject header to be serialized instead. Worth verifying with a test that exercises tenant/impersonation flows over transport. if (origUser != null && origUser.equals(authSubject)) { Serialization Compatibility Adding Principal and Subject interfaces to User changes the class hierarchy. Since User is Serializable and is serialized/deserialized via Base64 headers across nodes during rolling upgrades, mixed-version clusters where one side has the old User class and the other has the new one could have differing serialVersionUID or class descriptor mismatches. Confirm serialVersionUID is explicitly declared and unchanged, and that the ObjectStreamField set is unaffected, to preserve wire compatibility. public class User implements Serializable, CustomAttributesAware, Principal, Subject {

[github-actions]

Persistent review updated to latest commit a0ced03

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to c537928
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Implement required Subject methods on User Subject interface typically requires implementing authenticate(AuthToken) and runAs(CheckedRunnable) methods (as previously done in UserSubjectImpl). Making User implement Subject without these implementations will fail to compile unless Subject provides defaults. Additionally, the removed runAs semantics (stashing context and setting OPENDISTRO_SECURITY_USER transient) are no longer available, which may break plugin code that invoked subject.runAs(...) on the authenticated subject. src/main/java/org/opensearch/security/user/User.java [61] public class User implements Serializable, CustomAttributesAware, Principal, Subject { + // ensure authenticate(AuthToken) and runAs(CheckedRunnable) are implemented, + // e.g. by preserving prior UserSubjectImpl.runAs behavior: + // @Override public <E extends Exception> void runAs(CheckedRunnable<E> r) throws E { ... stash + put user + run ... } + // @Override public void authenticate(AuthToken t) { /* not implemented */ } Suggestion importance[1-10]: 6 __ Why: The suggestion raises a valid concern about whether Subject interface methods (authenticate, runAs) are implemented on User. However, the PR likely compiles (implying defaults or implementations exist elsewhere), and the improved_code is just a comment placeholder rather than actual code. Still, the concern about lost runAs semantics has merit.	Low
General	Return a stable named principal instead of this Returning this as the Principal couples the returned principal's identity (equals/hashCode/name) to the full User object, including roles, tenant, and attributes. Consumers expecting a stable principal identity (e.g. audit logs, caches keyed by principal) may behave incorrectly when User copies with different attributes but the same name are compared. Consider returning a lightweight NamedPrincipal(name) instead, matching the previous UserSubjectImpl behavior. src/main/java/org/opensearch/security/user/User.java [160-163] @Override public Principal getPrincipal() { - return this; + return new org.opensearch.identity.NamedPrincipal(name); } Suggestion importance[1-10]: 5 __ Why: Valid concern about principal identity stability. Returning this couples principal identity to the full User object including mutable-looking attributes. Using a NamedPrincipal matches previous UserSubjectImpl behavior and could avoid subtle bugs in audit/caching contexts.	Low
	Verify equality semantics for same-subject check Since User now implements Principal, ensure User.equals correctly compares two User instances (previously the comparison was origUser.equals(authSubject.getUser()) — same type). If Principal default equality or added interfaces changed hashing/equals expectations, this comparison could silently misidentify the same-subject case and cause incorrect header propagation. Verify User.equals still returns true only when the underlying users match. src/main/java/org/opensearch/security/transport/SecurityInterceptor.java [344] -if (origUser != null && origUser.equals(authSubject)) { +if (origUser != null && authSubject != null && origUser.getName().equals(authSubject.getName()) && origUser.equals(authSubject)) { Suggestion importance[1-10]: 3 __ Why: The suggestion asks to verify equality semantics, which is a mild concern. The improved_code adds redundant name comparison before the equals check, which doesn't materially improve correctness since User.equals should already handle this properly.	Low

---

Previous suggestions

Suggestions up to commit a0ced03

Category	Suggestion	Impact
General	Handle null authenticated user explicitly The previous code implicitly threw a NullPointerException via userSubject.getUser() when userSubject was null, but this call chain is now split. If user is null here, the subsequent Objects.requireNonNull(user) is inside the try block, but the direct cast is safe. However, this change means a null persistent value no longer throws NPE at the cast site (User cast tolerates null). Ensure the null handling behavior is preserved — the Objects.requireNonNull(user) inside try will still throw, but consider logging/handling it explicitly to avoid silent failures. src/main/java/org/opensearch/security/resources/ResourceIndexListener.java [107-110] final User user = (User) threadPool.getThreadContext().getPersistent(ConfigConstants.OPENDISTRO_SECURITY_AUTHENTICATED_USER); - +if (user == null) { + log.warn("No authenticated user found in thread context; skipping resource sharing entry creation for {}", resourceId); + return; +} try { Objects.requireNonNull(user); Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that the previous code would have NPE'd earlier on userSubject.getUser() when subject was null, and proposes explicit null handling. However, Objects.requireNonNull(user) still preserves failure semantics, so the impact is moderate.	Low
Possible issue	Verify serialization compatibility after adding interfaces Making User implement Principal requires implementing equals/hashCode semantics compatible with Principal contract, and importantly User is Serializable with a specific serialVersionUID. Adding Principal/Subject interfaces can affect serialization compatibility across cluster nodes running mixed versions. Verify that serialized User instances from older nodes (which serialized as plain User) remain deserializable, and that the new interfaces do not alter the serialized form. src/main/java/org/opensearch/security/user/User.java [61] +public class User implements Serializable, CustomAttributesAware, Principal, Subject { - Suggestion importance[1-10]: 4 __ Why: Raises a valid concern about serialization compatibility when adding interfaces to a Serializable class, but it is a verification-only suggestion with identical existing_code and improved_code.	Low
	Ensure runAs semantics preserved in User Previously, the authenticated subject wrapped the user in a UserSubjectImpl that provided runAs functionality via stashContext. Storing the raw User now relies on User (via Subject) implementing an equivalent runAs. Confirm that User.runAs (from the Subject interface) properly stashes and restores the ThreadContext and sets OPENDISTRO_SECURITY_USER, otherwise callers depending on the previous runAs semantics will break. src/main/java/org/opensearch/security/auth/BackendRegistry.java [541-542] +threadPool.getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_USER, anonymousUser); +threadPool.getThreadContext().putPersistent(ConfigConstants.OPENDISTRO_SECURITY_AUTHENTICATED_USER, anonymousUser); - Suggestion importance[1-10]: 4 __ Why: Highlights a legitimate concern about runAs semantics after replacing UserSubjectImpl, but is verification-only and provides identical existing_code and improved_code.	Low

[github-actions]

Persistent review updated to latest commit c537928