opensearch-project / OpenSearch

🔎 Open source distributed and RESTful search engine.
https://opensearch.org/docs/latest/opensearch/index/
Apache License 2.0
9.73k stars 1.8k forks source link

Internal User Management APIs #5882

Open peternied opened 1 year ago

peternied commented 1 year ago

Use case:

There are actions that can create, retrieve, update, delete an internal user account.

Acceptance Criteria:

RyanL1997 commented 1 year ago

Password Reset API

Me and @cwperks we went through some general ideas about the password reset API, and here are some thoughts and using scenarios that I would like to share with you guys:

Since we were thinking backwards, so here is the general idea of how the password resetting API should be called:

POST _identity/internaluser/{username}/reset/password
    { 
        oldpassword: xxx 
        newpassword: yyy
        newpasswordverify: yyy
    }

Scenario of Success:

--> 200 successful + "The password has been reset" 

(User are able to login with new password)

Scenarios of Failure:

dblock commented 1 year ago

I feel pretty strongly that we should never store a user's actual password, so any new API should be transporting a hash.

There's no need to send anything twice (password verify) in an API IMO or require any kind of password complexity checks server-side.

cwperks commented 1 year ago

@dblock The user's password is never stored in OpenSearch, the encrypted password is stored using a bcrypt hash of the password. The security plugin accepts both hash and password via the API to create/edit a user and if password is supplied then it performs the hashing of the password in the controller: https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/dlic/rest/api/InternalUsersApiAction.java#L129-L133

An Authorization header for basic auth sends password info in an http header: Authorization: <Base64.encode(username:password)>. Should a user be required to use a hashing tool before calling on an API to change their password?

dblock commented 1 year ago

Your statement is confusing. You say both "encrypted password is stored" and "the user's password is never stored". So which one is it? I think you mean to say that "bcrypt hash of the password is stored".

Basic auth requires the user's actual password (the server can verify it by hashing it). That's why one shouldn't use basic auth, but that's beyond the point here.

Yes, no API should be taking the actual user's password IMHO.

cwperks commented 1 year ago

Should a user be required to use a hashing tool like hash.sh before calling the API to update their password? That way the API param is hash and the plain text password is not exchanged. The user would authenticate with their current password to the cluster through any configured authentication backend.

Edit: I'm adding this for informational context, not to influence the design of the API in core. There is an existing API (PUT _plugins/_security/api/account) that takes in current_password and password (https://opensearch.org/docs/latest/security/access-control/api/#change-password):

PUT _plugins/_security/api/account
{
    "current_password" : "old-password",
    "password" : "new-password"
}
dblock commented 1 year ago

Fair enough, and there are lot of applications that send passwords back and forth over the wire. Can we guarantee that the password won't be logged? Can we implement both ways (maybe version 2)?

cwperks commented 1 year ago

Absolutely, I think we could support both ways (hash or password as a param) in v1. In implementing this endpoint, the implementer and reviewers will ensure it is not logged, but I'm not sure how to create an enforcement mechanism in the code around it.

DarshitChanpura commented 1 year ago

In feature/identity branch, current design of /user PUT api allows to create as user with password or update its password via PUT. However, only authorized users will be able to create or update the user record. Should this design be changed to reflect that a user can reset their password themselves and only allow passing hash in doing so? Or should we completely block password updates via PUT /user API and only allow it via POST /resetPassword API?

RyanL1997 commented 1 year ago

Fair enough, and there are lot of applications that send passwords back and forth over the wire. Can we guarantee that the password won't be logged? Can we implement both ways (maybe version 2)?

@dblock Yes, we will not log the password. I'm working on the password reset API for now, and once it's done, I will update the admin password PR.

RyanL1997 commented 1 year ago

In feature/identity branch, current design of /user PUT api allows to create as user with password or update its password via PUT. However, only authorized users will be able to create or update the user record. Should this design be changed to reflect that a user can reset their password themselves and only allow passing hash in doing so? Or should we completely block password updates via PUT /user API and only allow it via POST /resetPassword API?

@DarshitChanpura, If thats the case I think it should block the regular users only and the admin users can help the other regular internal users for the password update. @cwperks what do you think? Is this the case that the admin users can help the other regular users to reset password?

cwperks commented 1 year ago

@DarshitChanpura Any user should be able to update their own password. The API to Create/Update a user is reserved for admins.

I did some analysis of the security plugin and this is what I found:

There are currently 3 API endpoints capable of updating a password in the security plugin.

  1. Create User - PUT _plugins/_security/api/internalusers/<username> - https://opensearch.org/docs/latest/security/access-control/api/#create-user
  2. Patch User - PATCH _plugins/_security/api/internalusers/<username> - https://opensearch.org/docs/latest/security/access-control/api/#patch-user (It's probably possible with `Patch Users as well)
  3. Account Change Password - PUT _plugins/_security/api/account - https://opensearch.org/docs/latest/security/access-control/api/#patch-user

I did some digging into the history of Account Change Password and it was created to support security-kibana-plugin's reset password screen.

  1. Security PR - https://github.com/opensearch-project/security/pull/200
  2. security-kibana-plugin doesn't exist anymore, but the issue is in security-dashboards-plugin here: https://github.com/opensearch-project/security-dashboards-plugin/pull/126

I wanted to look into the history to provide context of the existence of the different APIs. Regarding the work we are doing in identity, I'm not crazy about the fact that Create and Update are overloaded because a user could be inadvertently created if a caller has a typo in the username of the API. There shouldn't be a different endpoint to update each different attribute of a user (there can certainly be an endpoint that does a lot of work similar to PATCH user above) but password is special and should have a dedicated endpoint to update so any user is able to reset their own password. Special permission is needed to perform PATCH User and PUT User because these endpoints are intended for administrators.