Add User.isAdminDn to User class #547

cwperks · 2023-10-09T14:05:36Z

Description

Adds a static helper method on the User class to determine if the user is in the security plugin's plugins.security.authcz.admin_dn list.

Issues Resolved

opensearch-project/security#3413

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
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.

Signed-off-by: Craig Perkins <[email protected]>

cwperks · 2023-10-09T14:07:29Z

@dhrubo-os @ylwu-amzn Let me know if this helps. There should be some testing performed with the security plugin installed, but this should let you determine if the current user is in the admin_dn list (i.e. user is a super user)

Edit: This is where the super user gets authenticated in the security plugin: https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/auth/BackendRegistry.java#L215-L220

codecov · 2023-10-09T14:09:07Z

Codecov Report

Merging #547 (dc0047c) into main (2ef47f9) will increase coverage by 0.58%.
Report is 2 commits behind head on main.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main     #547      +/-   ##
============================================
+ Coverage     74.55%   75.14%   +0.58%     
- Complexity      872      874       +2     
============================================
  Files           130      130              
  Lines          5680     5685       +5     
  Branches        698      698              
============================================
+ Hits           4235     4272      +37     
+ Misses         1139     1104      -35     
- Partials        306      309       +3

Files	Coverage Δ
...n/java/org/opensearch/commons/ConfigConstants.java	`87.50% <ø> (ø)`
...ain/java/org/opensearch/commons/authuser/User.java	`88.23% <100.00%> (+0.40%)`	⬆️

... and 1 file with indirect coverage changes

dhrubo-os · 2023-10-10T01:29:34Z

src/main/java/org/opensearch/commons/authuser/User.java

+
+    public static boolean isSuperUser(User user, Settings settings) {
+        List<String> adminDns = settings.getAsList(ConfigConstants.OPENSEARCH_SECURITY_AUTHCZ_ADMIN_DN, Collections.emptyList());
+        return adminDns.contains(user.getName());


I think we might need some backward compatibility? What happens if user is null? then it will throw exception in line 259. And I'm not a security person, so I don't know how to handle this situation. Just wanted to call out here.

Added null check

Usually when the user is super admin, the user object is null. Reference

Yeah same for ml-commons

Please see details here.

There are 2 cases of superadmin:

Plugin stashed the threadcontext (user == null)

User connects to cluster using admin certificate <- scope of this PR

Capturing discussion on this PR:

User is null means 1 of 2 things - 1) Security is not enabled or 2) plugin is operating in a stashContext block

User is non-null then - 1) User is a regular user (including anonymous) or 2) User is a superuser (meaning request comes from admin cert)

This PR pertains to 2) above and also relies on:

When security is enabled a user will always be present in the threadcontext

The plugin dev will ensure they are not making any calls to read the currently authenticated user from within a stashContext block

Signed-off-by: Craig Perkins <[email protected]>

dhrubo-os · 2023-10-12T17:36:48Z

Do we need to mark this PR as draft? Are we planning to merge this?

cwperks · 2023-10-12T18:07:56Z

Do we need to mark this PR as draft? Are we planning to merge this?

Marked as ready for review. @dhrubo-os if there's a companion ml-commons PR please share to show how this change will be utilized.

dhrubo-os · 2023-10-25T21:40:25Z

Do we need to mark this PR as draft? Are we planning to merge this?

Marked as ready for review. @dhrubo-os if there's a companion ml-commons PR please share to show how this change will be utilized.

I'll share within next week.

dhrubo-os · 2023-10-28T00:38:43Z

Do we need to mark this PR as draft? Are we planning to merge this?

Marked as ready for review. @dhrubo-os if there's a companion ml-commons PR please share to show how this change will be utilized.

I'll share within next week.

Just Raised a draft PR to show the example

We are going to use this function similar like this way

Currently for the easy development we added this method in our ml-commons plugin end. But eventually we want to use this method

Please let me know if you have any question or concern. Thanks.

cwperks · 2023-10-30T17:39:51Z

Hey @dhrubo-os, thank you for providing the linked PRs for example usage. If I'm reading the PR correctly it means that if a regular user creates a model its not hidden, but if a superuser creates a model then its hidden?

Have you considered separate transport actions so that those actions are authorized separately? i.e.:

TransportRegisterModelAction
TransportRegisterHiddenModelAction

If you have a separate transport action then you can permission users to perform the register hidden model action through a separate "admin-level" role.

Roles for ml-commons plugin: https://github.com/opensearch-project/security/blob/main/config/roles.yml#L273-L295

While there are already instances in plugins of plugins having to implement their own authz models based on user's backend roles, this will be yet another instance of something that should be the responsibility of the security plugin that is implemented in a plugin.

src/main/java/org/opensearch/commons/authuser/User.java

Signed-off-by: Craig Perkins <[email protected]>

* Add User.isSuperUser to User class Signed-off-by: Craig Perkins <[email protected]> * Add null check Signed-off-by: Craig Perkins <[email protected]> * Add another test Signed-off-by: Craig Perkins <[email protected]> * Make method non-static and require a user to exist to call method Signed-off-by: Craig Perkins <[email protected]> * Change to isAdminDn Signed-off-by: Craig Perkins <[email protected]> --------- Signed-off-by: Craig Perkins <[email protected]> Signed-off-by: Craig Perkins <[email protected]> (cherry picked from commit 107be59) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* Add User.isSuperUser to User class * Add null check * Add another test * Make method non-static and require a user to exist to call method * Change to isAdminDn --------- (cherry picked from commit 107be59) Signed-off-by: Craig Perkins <[email protected]> Signed-off-by: Craig Perkins <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

…-project#562) * Add User.isSuperUser to User class * Add null check * Add another test * Make method non-static and require a user to exist to call method * Change to isAdminDn --------- (cherry picked from commit 107be59) Signed-off-by: Craig Perkins <[email protected]> Signed-off-by: Craig Perkins <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Signed-off-by: AWSHurneyt <[email protected]>

Add User.isSuperUser to User class

0a87fdd

Signed-off-by: Craig Perkins <[email protected]>

dhrubo-os reviewed Oct 10, 2023

View reviewed changes

cwperks and others added 3 commits October 9, 2023 23:21

Add null check

e5aaee8

Signed-off-by: Craig Perkins <[email protected]>

Add another test

afdf043

Signed-off-by: Craig Perkins <[email protected]>

Make method non-static and require a user to exist to call method

527e2d2

Signed-off-by: Craig Perkins <[email protected]>

cwperks marked this pull request as ready for review October 12, 2023 18:06

cwperks requested review from qreshi, bowenlan-amzn, rishabhmaurya, getsaurabh02, eirsep, sbcd90 and AWSHurneyt as code owners October 12, 2023 18:06

shikharj05 reviewed Nov 8, 2023

View reviewed changes

src/main/java/org/opensearch/commons/authuser/User.java Outdated Show resolved Hide resolved

Change to isAdminDn

dc0047c

Signed-off-by: Craig Perkins <[email protected]>

cwperks changed the title ~~Add User.isSuperUser to User class~~ Add User.isAdminDn to User class Nov 13, 2023

shikharj05 approved these changes Nov 14, 2023

View reviewed changes

DarshitChanpura approved these changes Nov 14, 2023

View reviewed changes

lezzago approved these changes Nov 14, 2023

View reviewed changes

AWSHurneyt approved these changes Nov 14, 2023

View reviewed changes

AWSHurneyt merged commit 107be59 into opensearch-project:main Nov 14, 2023
9 checks passed

lezzago added the backport 2.x label Nov 14, 2023

opensearch-trigger-bot bot mentioned this pull request Nov 14, 2023

[Backport 2.x] Add User.isAdminDn to User class #562

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add User.isAdminDn to User class #547

Add User.isAdminDn to User class #547

cwperks commented Oct 9, 2023

cwperks commented Oct 9, 2023 •

edited

Loading

codecov bot commented Oct 9, 2023 •

edited

Loading

dhrubo-os Oct 10, 2023

cwperks Oct 10, 2023

lezzago Oct 10, 2023

dhrubo-os Oct 10, 2023

cwperks Oct 10, 2023 •

edited

Loading

cwperks Oct 10, 2023

dhrubo-os commented Oct 12, 2023

cwperks commented Oct 12, 2023

dhrubo-os commented Oct 25, 2023

dhrubo-os commented Oct 28, 2023

cwperks commented Oct 30, 2023

Add User.isAdminDn to User class #547

Add User.isAdminDn to User class #547

Conversation

cwperks commented Oct 9, 2023

Description

Issues Resolved

Check List

cwperks commented Oct 9, 2023 • edited Loading

codecov bot commented Oct 9, 2023 • edited Loading

Codecov Report

dhrubo-os Oct 10, 2023

Choose a reason for hiding this comment

cwperks Oct 10, 2023

Choose a reason for hiding this comment

lezzago Oct 10, 2023

Choose a reason for hiding this comment

dhrubo-os Oct 10, 2023

Choose a reason for hiding this comment

cwperks Oct 10, 2023 • edited Loading

Choose a reason for hiding this comment

cwperks Oct 10, 2023

Choose a reason for hiding this comment

dhrubo-os commented Oct 12, 2023

cwperks commented Oct 12, 2023

dhrubo-os commented Oct 25, 2023

dhrubo-os commented Oct 28, 2023

cwperks commented Oct 30, 2023

cwperks commented Oct 9, 2023 •

edited

Loading

codecov bot commented Oct 9, 2023 •

edited

Loading

cwperks Oct 10, 2023 •

edited

Loading