signumsoftware / framework

Open Source framework for writing data-centric applications using the latest versions of .Net Core, C# (not-nullable), ASP.NET Web API, Typescript (strict), React, D3 and Sql Server or PostgreeSQL
https://www.signumsoftware.com/en/Framework
MIT License
221 stars 84 forks source link

Implemented password hash migration for local Signum users #604

Closed pbeckmannCE closed 1 year ago

pbeckmannCE commented 1 year ago

At first I extended PasswordEncoding.EncodePassword() to return an IList<byte[]> instead of byte[]. This way multiple password hashes based on different algorithms can be returned by a Signum application, of which the last should be the most secure.

Now AuthLogic.RetrieveUser() checks if any password hash matches to authorize and updates User.PasswordHash if it's not the latest hash. This is where the actual password hash migration takes place and the code is executed on every login.

I wanted to use a some salt. However, this would require more extensive changes. Therefore I have decided to pass the user name, which can be used to derive a salt from it using SHA256 for example. Of course this is not ideal and a best effort solution.

All other changes are just calls to PasswordEncoding.EncodePassword().

olmobrutall commented 1 year ago

Thanks @pbeckmannCE! Looks like a great addition 👍🏼

One question: using the username as salt will make problems of the username changes, right ? Maybe using the id is a better idea ?

pbeckmannCE commented 1 year ago

Thanks for accepting the PR. 😄

You are right, the hash will differ if the username is used for it. I was not aware that it is subject to change and looking into it again, the id is not available at that point either.

However, this will not break at the moment. If you look into the default implementation of PasswordEncoding.EncodePassword(), you will see that it still passes the password to MD5Hash() without any changes.

This may break for us in the future, as we are now using the username to derive a salt from it. Is there a routine in Signum to change the username or can this only be done on the database level?