New 'password' account property and Accounts endpoint refactoring

[otsakir]

A new password property should be added to the account entity. The new property will be used for initial authentication and security critical operations like updating an account. Further use of the RESTapi will require AuthToken.

1. All REST endpoints except AccountsEndpoint will require an AuthToken.
2. Accounts' endpoint create/update/delete methods will require a password.
3. Retrieving sub-accounts list should not contain AuthToken.
4. Retrieving a single sub-account should not contain AuthToken. Actually, the AuthToken will only be contained in login requests i.e. requests where the Account SID in the Authorization Header is the same as the Account being requested.
5. Restcomm SIP Client's endpoint create/update/delete methods will require a password.
6. Need to refactor HTTP DELETE to update the status to close and not delete the account

Also, AuthToken should be a random value instead of a hashed one.

This also relates to password hashing strength issue - https://github.com/RestComm/Restcomm-Connect/issues/1497

[deruelle]

This will require AdminUI to have a new popup that requests the user to reenter his credentials for security measures in the case of:

1. Update or Delete your own account
2. Create, Update or Delete a sub account

[otsakir]

@deruelle , @gvagenas , i'm was thinking of adding another property that will hold the type of password hashing if any. Sth like passwordEncType. It could be 'plain', 'md5', 'sha-salted' etc.

wdyt ?

This will also help in the transition from the old installations. It will work like this:

• The db upgrade script will create the new 'password' column and initialize it with the existing AuthToken (md5'd stuff). It will also set he passwordEncType to 'md5'.
• Authentication will be able to handle various types of passwords and of course md5. So it will still work.
• At some point we set accounts to 'uninitialized'. That will force users to update their passwords. Setting a new password will now default to 'plain' passwordEncType. Thus, further authentication attempts will work.

Thoughts ?

[otsakir]

Following the discussion with @gvagenas , a new passwordAlgorithm property will be added to Account.

Suggested values will be: plain, md5

[deruelle]

@otsakir was there a Pull Request ever done for this ?

[otsakir]

Yes @deruelle, it's here: https://github.com/RestComm/Restcomm-Connect/pull/1574

Note, It contains UI Dashboard adjustments in the old dashboard :-( so there will some (lots of?) conflicts there.

Also, there are a lot of comments in this issue that refer to rolling upgrades. Back then, we decided to introduce and test rolling upgrades in the new account password attribute. Probably a wrong decision. In any case, the issue1496_account_password branch (PR1574) does NOT contain them.

[deruelle]

@otsakir any chances we can update the PR for the new dashboard. Where is the document related to rolling upgrades for this one ? I think this was just a new column addition that would be backward compatible ?

[scottbarstow]

@otsakir you need to be aware of #2164 as well when finishing this off.

[otsakir]

@deruelle , i suggest forgetting about rolling upgrades for now and stick to adding the password property. And it's not only about the password property. It's the password_algorithm also and a lot of other considerations too.

https://docs.google.com/a/telestax.com/document/d/1ePVbuWfHDp4uxh9dH1Ht13vHTw754XDBUebSoNhTEs0/edit?usp=sharing

IMO rolling upgrades should be stripped off. And the issue (the password part) should be handled in a new PR.

[scottbarstow]

@otsakir @arslan70 (who is working on #2164 is very interested in working on this task as a part of his work on the other task. Is this somehting you'd be ok with overseeing?

[otsakir]

Sorry @scottbarstow, i can't oversee @arslan70's work . Of course it's fine with me if he wants to dive in #1496 too.

[deruelle]

thanks @otsakir we will follow the recommendation of starting over from scratch on this one plus potentially cherry pick what makes sense.

[arslan70]

Hi @deruelle and @scottbarstow I was working on a related issue so I thought I may be able to lend a hand on this as well. Can I pitch in somehow?

[deruelle]

@arslan70 Most Definitely. Let's do this one as a separate standalone PR from the other one though.

[arslan70]

@otsakir If we keep the UI separate from this issue, the work you have done is still re-usable. Is there a way to use the underlying schema change and DAO layer and go ahead finishing it? Basically I'm trying to understand the need of a new pull request, are there still too many conflicts if we take UI out of the equation right now?

[arslan70]

@otsakir why is there a need to have multiple password algorithms, doesn't it unnecessarily complicate the design?

[otsakir]

@arslan70 , yes it does complicate things more.

There was the idea of supporting other algorithms too - #1497 - so it would help users and system making the transition from one to the other. If you know what algorithm is used in your password you also know how to verify it. So, you can start supporting move advanced hashing algorithms and still your old passwords will work. When you ask your users to change their password the new algorithm will be used.

That was the idea back then of course.

[arslan70]

@otsakir thanks for the reply. I'm trying to understand the scope of change required and trying to avoid the pitfalls you came upon. In your previous comment

i suggest forgetting about rolling upgrades for now and stick to adding the password property. And it's not only about the password property. It's the password_algorithm also and a lot of other considerations too.

Are you suggesting to disregard backward compatibility for the moment? If this the case, adding the password field in database, using a single hashing algorithm(bcrypt) along with the DAO changes and changes in the validation algorithm seems to be the scope.

It will be great if @otsakir or @deruelle can validate my understanding.

[deruelle]

@arslan70 yes that seems safe to start with the smallest number of changes (ie only adding password and letting the algorithm changes for #1497) and go incremental from there in other separate issues as required.

[arslan70]

@deruelle thanks Jean. That sounds great. Can I use the pull request for Multi API keys issue. My current branch can incrementally accommodate this issue otherwise I fear there will be a lot of code conflicts.

[deruelle]

@arslan70 let's not do the PR for this one in the same PR as Multi API keys issue or this would be too big to review but you can reuse the same code base to make a new PR based on it. Let's wrap up Multi API keys issue first.

[arslan70]

@deruelle Multi API keys PR is ready to be merged from my side. A review will be a good idea.

[arslan70]

I have come upon an interesting design decision, I think it needs some discussion whether to completely rely on HTTPS or use client side hashing of password.

Here is the existing logic behind logging in. The client send credentials as the following string.

YWRtaW5pc3RyYXRvckBjb21wYW55LmNvbTo3N2Y4YzEyY2M3YjhmODQyM2U1YzM4YjAzNTI0OTE2Ng

Decoding it as base64 we get

administrator@company.com:77f8c12cc7b8f8423e5c38b035249166

The password is sent as MD5 hashed after the colon. With the current design it is a piece of cake to crack the password if using HTTP connection.

Now I assume we need to handle the case without HTTPS and sending password in plain text is not an option. We will need to handle client side Bcrypt hashing, which is not recommended.

An interesting argument against sending hashed passwords is if somehow the database gets hacked. The attacker can login with every account by simply using the hashed password. Why because the server will be string matching the already hashed password against what is saved in the database. As opposed to applying the hash first to the provided password and then matching. For the later case when server apply a hash and then match with the database, the attacker need to reverse the compromised hash in order to login.

@deruelle @scottbarstow thoughts on this?

[deruelle]

@arslan70 yes we always use HTTPs for production system that's a given.

I would actually break down this issue into 2, one that is focused solely on account password property and then on improving the security of the password by upgrading the algorithm to secure password by using a library such as libsodium by example.

[arslan70]

@deruelle thanks for the reply. I'm reading up on libsodium. Meanwhile if we can rely on HTTPS then I'll change the UI to send password in plain text. Rest of the backend is finished.

[scottbarstow]

@arslan70 @gvagenas and I were just discussing this and had two questions:

1. When you update password on the account we should no longer be updating the tokens. Is this the case?

2. Is this ready for PR and review?