Security User Refactor

[stephen-crawford]

Description

This PR is meant to refactor the concept of a user from the Security Plugin's point of view. This will allow us to introduce the basics of service accounts on the Security plugin side.

Issues Resolved

This PR will contribute to resolving #2584.

Testing

The existing InternalUsersTests are being used to verify the previous functionality is maintained. After merging this change, specific Service Account operations can be performed and tests added to exercise service account logic.

Check List

• New functionality includes testing
• New functionality has been documented
• 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.

[codecov-commenter]

Codecov Report

Merging #2594 (3a106c5) into main (bbd43ec) will increase coverage by 0.13%.
The diff coverage is 76.31%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@             Coverage Diff              @@
##               main    #2594      +/-   ##
============================================
+ Coverage     61.38%   61.51%   +0.13%     
- Complexity     3383     3397      +14     
============================================
  Files           269      272       +3     
  Lines         18668    18740      +72     
  Branches       3279     3284       +5     
============================================
+ Hits          11459    11528      +69     
  Misses         5613     5613              
- Partials       1596     1599       +3     

Impacted Files	Coverage Δ
...security/dlic/rest/api/InternalUsersApiAction.java	86.88% <57.14%> (+2.88%)	⬆️
...java/org/opensearch/security/user/UserService.java	78.18% <78.18%> (ø)
.../opensearch/security/OpenSearchSecurityPlugin.java	80.27% <100.00%> (+0.23%)	⬆️
...security/dlic/rest/api/SecurityRestApiActions.java	95.00% <100.00%> (ø)
...opensearch/security/user/UserServiceException.java	100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[cwperks]

Hi @scrawfor99 , I think you are on the right track with the extensions registration process and creation of the service account at that time for extensions that require one, but I think we should start by introducing the notion of a service account.

I see in this PR that service: true is added in the attribute map of the internal user and I think this should be a field of a user in the style of reserved: true if the service accounts will be stored in the internal user list.

For introduction of the concept of a service account, I would expect:

1. That service accounts are designated by a special property service on a user. If this property exists and is set to true than it is a service account.
2. Service accounts can only be mapped to at most one role - that role will dictate how a service account token can interact with the cluster. This can be used to implement system indices - i.e. service account tokens scoped to a system index pattern can create, delete and modify the index.
3. Service accounts cannot be mapped to backend roles
4. Service accounts are passwordless and only become useful once a token is generated for the account
5. internaluser APIs should account for service accounts
   • List internalusers should not include service accounts
   • Create internaluser prohibits creation of account
   • Delete internaluser prohibits deletion of account - once they are created programmatically on registration of extension they will exist and be useless until a token is created
   • Patch user/users prohibits patching the account
6. There should be a convention in naming the role that can be associated with a service account. If by convention we have a service account named <extension/extensionId> then maybe a role can exist with the same name for quick retrieval?

Once the concept of the service account is introduced, an API to list service accounts should be created and we can also start linking it to the extensions use-case like you are doing here to create one on extension registration.

[stephen-crawford]

Hi @cwperks, thank you for your thoughtful comments. For specifications about defining service accounts, I would suggest we use the associated decision doc/question #2597. I think that some of the expectations you listed for service accounts sound like great ideas, but I also think we may need to come to a better understanding about some others. For instance, having an API to list service accounts sounds great. However, I am not sure why we need to restrict the number of roles a service account can have.

How about we head over to the decision doc to hash things out and then we can come back here as I implement based on our decision?

[cwperks]

@scrawfor99 I will move the comments over to the decision doc. I was thinking that service account tokens would be narrowly used in order to allow extensions to write to reserved indices for those extensions that have special protections - similar to plugins with a system index, but without the need for stashing the thread context.

It would be possible to increase the scope of the service account token for use-cases where an extension runs as a daemon if its been granted by the cluster admin. Single roles makes it a one stop place to look up the permissions. User's may have multiple roles, but for a service account limiting it to a single role would make it explicitly clear what it can do. I have seen other systems that implement service accounts hardcode the roles definition in code and the permissions are not modifiable at all.

[reta]

@RyanL1997, @DarshitChanpura, @reta can I get a review here so that we can potentially merge this? I addressed Craig's comments but he is not around this week.

@scrawfor99 my apologies, I do not (yet) know this part of the plugin well enough, looking nonetheless but would let others to approve with confidence

[RyanL1997]

Nice work! Thanks @scrawfor99

[RyanL1997]

Btw, I saw Peter was requesting some changes, but for some reason I couldn't see that thru the log. Just wanna double check with you to make sure all the comments have been resolved.

[stephen-crawford]

Hi @RyanL1997, thank you. Yeah, Peter's changes were addressed a while ago, I will dismiss if we get two approvals.

[DarshitChanpura]

why not use securityJsonNode.put('name')... here to avoid duplicate declaration on line 115?
Having said that, I know that put() doesn't exist in SecurityJsonNode for some unknown reason (probably to make it a read-only node). Do you think creating it might be worthwhile?

[stephen-crawford]

See above. I can remove use of the contentAsNode pattern by adding methods to the SecurityJsonNode object. I was not sure which implementation would be preferred.

I am interpreting this as you would prefer that change so I will go that route.

[stephen-crawford]

EDIT: Hi @DarshitChanpura Unless it is going to block this PR, I am going to suggest we leave it as is. I know it is not the most convenient solution but the securityJsonNode class has 40+ usages and was not intended to be directly modifiable. I have been working on modifying it but it requires changing the class from creating final instances of itself to creating mutable versions. I suspect a part of the reason the class was created how it is, is so that there would be an immutable version of a JSON node for implementations. Let me know if this will be a blocker and I can look more into it but I am going to suggest we leave this as is otherwise.

P.S. I did make the other changes you suggested.

[DarshitChanpura]

Sounds good, we can leave it as is. Thank you for taking time and going through SecurityJsonNode 😃. We can cover this in a separate PR in future

Peter is not here this week or last.

[DarshitChanpura]

Thank you for addressing all the comments. I only have one last comment. The changes look good to me otherwise!

[stephen-crawford]

@DarshitChanpura, exactly, it is so that an account can be turned on/off and auth tokens will not be created for any disabled accounts.

[DarshitChanpura]

Thank you for addressing all my comments @scrawfor99 ! I'm happy to merge this.