Fix role creation failure for restapi:admin/* permissions

[itsmevichu]

Description

This PR allows superadmin user to create roles with restapi:admin/* permissions.

• Category: Bug fix
• Why these changes are required?
  Creating a role with restapi:admin/* permissions was failing with an `Access denied error, even when the request was made by a superadmin user;
• What is the old behavior before changes and new behavior after changes?
  • Before:
    • Creating a role containing restapi:admin/* permissions would fail with Access denied, even when the request was made by a superadmin user.
  • After:
    • Superadmin users can successfully create roles containing restapi:admin/* permissions.

Issues Resolved

#6178

Testing

Unit testing and manual testing.

Check List

• New functionality includes testing
• New functionality has been documented
• New Roles/Permissions have a corresponding security dashboards plugin PR
• API changes companion pull request created
• 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.

[github-actions]

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit b85cfd9.

Path	Line	Severity	Description
src/main/java/org/opensearch/security/dlic/rest/validation/EndpointValidator.java	163	low	Super admin bypass added to isAllowedToChangeEntityWithRestAdminPermissions: any user recognized as a super admin via adminDNs can skip the REST admin permission checks entirely. The check itself uses the established adminDNs certificate mechanism (high trust), so this is likely intentional, but it creates a broader privilege escalation path if admin certificates are ever compromised. No signs of malicious intent — the logic is consistent, tests cover both branches, and no external calls or obfuscation are present.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 0 | High: 0 | Medium: 0 | Low: 1

---

Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.

---

⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 45bb65f)

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 Broad Bypass Scope The super-admin bypass in isAllowedToChangeEntityWithRestAdminPermissions short-circuits both the check for existing entities containing restapi:admin/* permissions and the check for the new payload. This means a super-admin can now not only create/modify entities with restapi:admin/* permissions, but also modify/delete any pre-existing entity that has such permissions, regardless of other guard logic downstream. Confirm this expanded scope matches the intent of issue #6178 (which describes creation only). If the intent was creation-only, the bypass should be scoped more narrowly. if (isCurrentUserSuperAdmin()) { return ValidationResult.success(securityConfiguration); }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 45bb65f

Explore these optional code suggestions:

Category	Suggestion	Impact
Security	Detect super-admin via TLS principal, not username The check adminDNs.isAdmin(user) matches any user whose name is in the admin DN list, but "super admin" here should mean the request came via the admin TLS certificate (mTLS), not merely a user with a matching name. Consider checking AdminDNs.isAdminDN against the actual authenticated principal/TLS DN from the thread context (e.g., ConfigConstants.OPENDISTRO_SECURITY_SSL_PRINCIPAL) so a regular user cannot spoof super-admin privileges by choosing a matching username. src/main/java/org/opensearch/security/dlic/rest/api/RestApiAuthorizationEvaluator.java [258-261] public boolean isCurrentUserSuperAdmin() { - final Pair<User, TransportAddress> userAndRemoteAddress = Utils.userAndRemoteAddressFrom(threadContext); - return userAndRemoteAddress.getLeft() != null && adminDNs.isAdmin(userAndRemoteAddress.getLeft()); + final String sslPrincipal = threadContext.getTransient(ConfigConstants.OPENDISTRO_SECURITY_SSL_PRINCIPAL); + return sslPrincipal != null && adminDNs.isAdminDN(sslPrincipal); } Suggestion importance[1-10]: 3 __ Why: The concern about spoofing is largely mitigated because adminDNs.isAdmin(user) compares against admin DNs, and normal users cannot authenticate with an admin DN as their name without the TLS cert. The suggestion's rationale is somewhat speculative and the proposed alternative may not be a drop-in replacement.	Low
	Ensure super-admin bypass is authenticated correctly Bypassing the rest-admin-permissions check for super admin here allows unrestricted mutation of entities carrying restapi:admin/* permissions. Ensure that isCurrentUserSuperAdmin() truly reflects mTLS admin-cert usage and not merely an admin DN match; otherwise a regular user with a spoofed username could bypass this guard. Also consider auditing/logging this bypass for traceability. src/main/java/org/opensearch/security/dlic/rest/validation/EndpointValidator.java [169-171] default ValidationResult<SecurityConfiguration> isAllowedToChangeEntityWithRestAdminPermissions( final SecurityConfiguration securityConfiguration ) throws IOException { if (isCurrentUserSuperAdmin()) { + // Super-admin (mTLS admin cert) bypasses rest-admin permission guard return ValidationResult.success(securityConfiguration); } Suggestion importance[1-10]: 2 __ Why: The suggestion mostly asks for verification and adds a comment, without providing a substantive code change. The improved_code differs from existing_code only by an added comment.	Low

---

Previous suggestions

Suggestions up to commit 45bb65f

Category	Suggestion	Impact
Security	Harden super-admin detection against spoofing The current check only verifies whether the user's DN is in the admin DN list, but mTLS super-admin identification typically also requires that the request was made via the admin certificate (i.e., isAdminDN on the certificate/SSL principal), not just any user whose name happens to match. Consider also checking the SSL principal / admin certificate context (e.g., via sslInfo or existing AdminDNs.isAdminDN on the transport principal) to avoid incorrectly granting super-admin bypass to a regular user whose username coincides with an admin DN entry. src/main/java/org/opensearch/security/dlic/rest/api/RestApiAuthorizationEvaluator.java [258-261] public boolean isCurrentUserSuperAdmin() { final Pair<User, TransportAddress> userAndRemoteAddress = Utils.userAndRemoteAddressFrom(threadContext); - return userAndRemoteAddress.getLeft() != null && adminDNs.isAdmin(userAndRemoteAddress.getLeft()); + final User user = userAndRemoteAddress.getLeft(); + if (user == null) { + return false; + } + // Ensure the user was authenticated via the admin certificate, not just name-matched. + return adminDNs.isAdmin(user); } Suggestion importance[1-10]: 5 __ Why: The suggestion raises a valid security concern about relying solely on adminDNs.isAdmin(user) for super-admin detection, but the improved_code is essentially equivalent to the existing code (same check, just refactored with an early return) and doesn't actually add the SSL principal verification it recommends. The concern has merit but the concrete improvement is marginal.	Low

Suggestions up to commit 39e7667

Category	Suggestion	Impact
Possible issue	Prevent NPE when retrieving current user Guard against a null userAndRemoteAddress pair to avoid a potential NullPointerException if Utils.userAndRemoteAddressFrom returns null. This is consistent with defensive null-handling used elsewhere when retrieving the current user from the thread context. src/main/java/org/opensearch/security/dlic/rest/api/RestApiAuthorizationEvaluator.java [258-261] public boolean isCurrentUserSuperAdmin() { final Pair<User, TransportAddress> userAndRemoteAddress = Utils.userAndRemoteAddressFrom(threadContext); - return userAndRemoteAddress.getLeft() != null && adminDNs.isAdmin(userAndRemoteAddress.getLeft()); + return userAndRemoteAddress != null + && userAndRemoteAddress.getLeft() != null + && adminDNs.isAdmin(userAndRemoteAddress.getLeft()); } Suggestion importance[1-10]: 3 __ Why: Utils.userAndRemoteAddressFrom typically returns a non-null Pair, and existing usages in the same file (e.g., isCurrentUserAdminFor) don't null-check the pair itself. The defensive check is minor and likely unnecessary.	Low

Suggestions up to commit f719d6e

Category	Suggestion	Impact
General	Add null check for safety Guard against a potential NullPointerException when userAndRemoteAddressFrom returns null. While unlikely, defensively checking the pair itself prevents an unexpected NPE that could break the role creation flow for super admins. src/main/java/org/opensearch/security/dlic/rest/api/RestApiAuthorizationEvaluator.java [258-261] public boolean isCurrentUserSuperAdmin() { final Pair<User, TransportAddress> userAndRemoteAddress = Utils.userAndRemoteAddressFrom(threadContext); - return userAndRemoteAddress.getLeft() != null && adminDNs.isAdmin(userAndRemoteAddress.getLeft()); + return userAndRemoteAddress != null + && userAndRemoteAddress.getLeft() != null + && adminDNs.isAdmin(userAndRemoteAddress.getLeft()); } Suggestion importance[1-10]: 2 __ Why: Utils.userAndRemoteAddressFrom is used elsewhere in the codebase without null checks (as shown by the same pattern usage in this PR's context), suggesting it doesn't return null. The defensive check offers minimal value.	Low

Suggestions up to commit d4caeab

Category	Suggestion	Impact
Possible issue	Add null check for adminDNs Add null safety check for adminDNs before calling isAdmin(). If adminDNs is null, the method will throw a NullPointerException, causing the authorization check to fail unexpectedly. src/main/java/org/opensearch/security/dlic/rest/api/RestApiAuthorizationEvaluator.java [258-261] public boolean isCurrentUserSuperAdmin() { final Pair<User, TransportAddress> userAndRemoteAddress = Utils.userAndRemoteAddressFrom(threadContext); - return userAndRemoteAddress.getLeft() != null && adminDNs.isAdmin(userAndRemoteAddress.getLeft()); + return userAndRemoteAddress.getLeft() != null && adminDNs != null && adminDNs.isAdmin(userAndRemoteAddress.getLeft()); } Suggestion importance[1-10]: 7 __ Why: Adding a null check for adminDNs prevents potential NullPointerException in the authorization flow. However, this is a defensive programming practice rather than a critical bug fix, as adminDNs is likely initialized during object construction in the existing codebase.	Medium

Suggestions up to commit b85cfd9

Category	Suggestion	Impact
Possible issue	Add null check for adminDNs Add null safety check for adminDNs before calling isAdmin(). If adminDNs is null, the method will throw a NullPointerException when attempting to verify admin status. src/main/java/org/opensearch/security/dlic/rest/api/RestApiAuthorizationEvaluator.java [258-261] public boolean isCurrentUserSuperAdmin() { final Pair<User, TransportAddress> userAndRemoteAddress = Utils.userAndRemoteAddressFrom(threadContext); - return userAndRemoteAddress.getLeft() != null && adminDNs.isAdmin(userAndRemoteAddress.getLeft()); + return userAndRemoteAddress.getLeft() != null && adminDNs != null && adminDNs.isAdmin(userAndRemoteAddress.getLeft()); } Suggestion importance[1-10]: 7 __ Why: Adding a null check for adminDNs prevents potential NullPointerException. However, if adminDNs is a required dependency that should never be null in normal operation, this might mask initialization issues rather than fix them.	Medium
General	Replace real method calls in mocks The mock setup calls thenCallRealMethod() which may invoke actual implementation code during testing. This could lead to unexpected behavior or test failures if the real methods have dependencies. Consider using thenReturn() with appropriate values instead. src/test/java/org/opensearch/security/dlic/rest/api/RolesApiActionValidationTest.java [65-70] @Test public void nonSuperAdminIsNotAllowedToCreateRoleWithRestAdminPermissions() throws Exception { when(restApiAuthorizationEvaluator.isCurrentUserSuperAdmin()).thenReturn(false); Mockito.doReturn(CType.ROLES).when(configuration).getCType(); - when(configuration.getImplementingClass()).thenCallRealMethod(); - when(restApiAuthorizationEvaluator.containsRestApiAdminPermissions(any(Object.class))).thenCallRealMethod(); + when(configuration.getImplementingClass()).thenReturn(RoleV7.class); + when(restApiAuthorizationEvaluator.containsRestApiAdminPermissions(any(Object.class))).thenReturn(true); ... } Suggestion importance[1-10]: 4 __ Why: While using thenCallRealMethod() can introduce dependencies in tests, the suggestion to replace with hardcoded return values may not accurately test the actual behavior. The current approach might be intentional to test integration between components.	Low

[github-actions]

Persistent review updated to latest commit d4caeab

[DarshitChanpura]

Changes LGTM. Will approve once CI is green

[github-actions]

Persistent review updated to latest commit f719d6e

[github-actions]

Persistent review updated to latest commit 39e7667

[DarshitChanpura]

Can you please these test failures?
specifically, RolesApiActionValidationTest.java‎

[github-actions]

Persistent review updated to latest commit 45bb65f

[github-actions]

Persistent review updated to latest commit 45bb65f

[codecov]

Codecov Report

❌ Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 75.37%. Comparing base (283edfc) to head (45bb65f).

Files with missing lines	Patch %	Lines
...y/dlic/rest/api/RestApiAuthorizationEvaluator.java	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6226      +/-   ##
==========================================
- Coverage   75.39%   75.37%   -0.02%     
==========================================
  Files         456      456              
  Lines       29924    29929       +5     
  Branches     4530     4531       +1     
==========================================
  Hits        22560    22560              
- Misses       5265     5273       +8     
+ Partials     2099     2096       -3     

Files with missing lines	Coverage Δ
...curity/dlic/rest/validation/EndpointValidator.java	94.59% <100.00%> (+0.22%)	⬆️
...y/dlic/rest/api/RestApiAuthorizationEvaluator.java	69.00% <50.00%> (-0.16%)	⬇️

... and 8 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.