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 207b877)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ No major issues detected

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 207b877

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Also detect mTLS admin-cert super admin The current check only verifies that the authenticated user is in the adminDNs list, but a proper "super admin" check should also consider requests authenticated via the admin TLS certificate (mTLS super-admin). Consider also checking AdminDNs.isAdminDN for the SSL principal or verifying the OPENDISTRO_SECURITY_SSL_TRANSPORT_PRINCIPAL/injected user, otherwise mTLS admin-cert requests (which the PR explicitly intends to allow) may not be recognized as super admin depending on how the user object is populated. 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; + } + return adminDNs.isAdmin(user) || adminDNs.isAdminDN(user.getName()); } Suggestion importance[1-10]: 3 __ Why: The suggestion is likely incorrect: AdminDNs.isAdmin(User) typically already handles admin cert users because the user's name is set to the SSL principal DN for TLS admin requests. Integration tests in the PR confirm that getAdminCertRestClient() is correctly recognized as super admin via the existing implementation, so the added isAdminDN check is likely redundant.	Low

---

Previous suggestions

Suggestions up to commit 255ac0e

Category	Suggestion	Impact
Possible issue	Also recognize mTLS admin-cert as super-admin The current check only verifies whether the user is in the configured admin DN list, but this misses the mTLS admin-cert case where the request is authenticated via the admin certificate (typically detected via a thread-context flag such as OPENDISTRO_SECURITY_CONF_REQUEST_HEADER or an SSL principal match). Since the PR intent is that the mTLS super-admin bypasses the restapi:admin/* guard, also treat requests carrying the admin-cert marker as super-admin, otherwise TLS admin-cert requests without a populated User will still be blocked. 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 && adminDNs.isAdmin(user)) { + return true; + } + // mTLS admin-cert requests are marked as admin in the thread context + return "true".equals(threadContext.getTransient(ConfigConstants.OPENDISTRO_SECURITY_CONF_REQUEST_HEADER)); } Suggestion importance[1-10]: 4 __ Why: The suggestion raises a plausible concern about mTLS admin-cert detection, but in OpenSearch Security, admin-cert requests typically do populate the User transient and adminDNs.isAdmin handles this. The proposed use of OPENDISTRO_SECURITY_CONF_REQUEST_HEADER is speculative and may not be accurate.	Low
Security	Restrict bypass strictly to mTLS admin Bypassing the restapi:admin/* guard for any caller flagged as super-admin removes the safety net that prevents non-TLS admins from escalating privileges. If isCurrentUserSuperAdmin() ever returns true for a user that authenticated through a non-mTLS path (e.g., via admin_dn matching over a regular REST session), this change silently grants full REST-admin authoring. Consider tightening isCurrentUserSuperAdmin() to strictly require the admin-certificate transport path, or add an explicit assertion here. src/main/java/org/opensearch/security/dlic/rest/validation/EndpointValidator.java [169-171] default ValidationResult<SecurityConfiguration> isAllowedToChangeEntityWithRestAdminPermissions( final SecurityConfiguration securityConfiguration ) throws IOException { + // Only bypass when the caller authenticated via the admin TLS certificate if (isCurrentUserSuperAdmin()) { return ValidationResult.success(securityConfiguration); } Suggestion importance[1-10]: 3 __ Why: The suggestion raises a valid security consideration but the improved_code is essentially identical to the existing_code, only adding a comment. It does not provide a concrete fix.	Low

Suggestions up to commit 255ac0e

Category	Suggestion	Impact
Possible issue	Also detect TLS admin cert callers The current isCurrentUserSuperAdmin only checks if the user is a configured admin user (via adminDNs.isAdmin), but does not account for the mTLS admin certificate case, which is the primary "super admin" bypass path referenced in the tests (getAdminCertRestClient). Consider also checking AdminDNs.isAdminDN on the caller's principal or the SSL principal, otherwise the bypass may not work for TLS admin cert callers as intended. 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 && adminDNs.isAdmin(user)) { + return true; + } + final String sslPrincipal = threadContext.getTransient(ConfigConstants.OPENDISTRO_SECURITY_SSL_PRINCIPAL); + return sslPrincipal != null && adminDNs.isAdminDN(sslPrincipal); } Suggestion importance[1-10]: 6 __ Why: The suggestion raises a legitimate concern: mTLS admin cert callers may authenticate via a different path, and if the injected user isn't recognized by adminDNs.isAdmin, the super-admin bypass could fail. However, in practice the security plugin typically injects a synthetic admin user for TLS admin cert requests, so the current check may already suffice; the suggested SSL principal fallback is plausible but not clearly required, and its correctness depends on codebase specifics not visible in the diff.	Low

Suggestions up to commit 45bb65f

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

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

[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).
⚠️ Report is 19 commits behind head on main.

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.

[github-actions]

Persistent review updated to latest commit 255ac0e

[github-actions]

Persistent review updated to latest commit 255ac0e

[github-actions]

Persistent review updated to latest commit 207b877