Noobaa Account: Remove password hashing

[aspandey]

Explain the changes

1. This PR is attempting to achieve the same target but with different approach.
2. We are removing bcrypt code completely and NOT replacing it with any hashing.
3. As we have observed that this login/password is not being used by users, we are removing this field completely.
4. We are providing the upgrade script - This script will remove "password" field from all the existing accounts, including admin and support.
5. At this point of time we are not removing the code which implements password. For example removing password, has_login and other fields from schema.
6. Attaching screenshot. This screenshot shows entries in Database. First with 5.17.0 and second is with changes and upgrade script .
7. There are few changes which have been done to make sure existing test on password succeed.
8. noobaa-operator also sends admin password, which needs to be handled to avoid failure of installation.

[jackyalbo]

I think this print was removed by accident, please leave it also after your change.

[aspandey]

done

[jackyalbo]

shouldn't this be changed to try and catch?

[aspandey]

done

[guymguym]

@aspandey I don't follow how we plan to integrate with #8252 and #8213 but in any case there are two issues with this PR that I think should be addressed - first that we should not allow empty password to authenticate. second that we should make the schema changes backward compatible so we can't change the meaning of a field without introducing another field that describes the different meanings. see more info in code comments above. Thanks

[guymguym]

wait this doesn't make sense - why would we allow an empty password to pass authentication?
this should return false. if password authentication is not used that's one thing, but we shouldn't allow it to be bypassed and allow unintended access.

Suggested change

	if (!password || !target_account.password) {
	return true;
	}
	if (!password || !target_account.password) {
	return false;
	}

[guymguym]

changing the meaning of a field in schema is breaking backward compatibility.
also notice that the upgrade script will forever delete the password to all users, which means we cannot ever introduce it again... Instead I would prefer to add a new field called password_hash_type with enum of ['bcrypt'] which we can also add 'PBKDF2-HMAC-SHA512-ITER100000' or 'argon2'. this way, the upgrade script just needs to fill for every account with password that will also have value for password_hash_type='bcrypt', and our authentication logic can decide to reject any password authentication with bcrypt, since we do not support it anylonger, so every old account will be forced to renew the password to use it - we should set the account.next_password_change = new Date() to force it.

[aspandey]

@guymguym Let me explain some context before making any changes in code as per your comment.

What is our target for this task and the limitations to achieve it.

Target - “We need to get rid of bcrypt”.

Over the period of the last 3 weeks, our discussion on slack, PR and calls has resulted in following two approaches.

Approach 1 - Remove password hashing code completely. Also, remove the password field completely.
This is an easy approach and removing the password field would not be difficult if we know that users don’t use this password. We need to remove schema also but I thought of doing it into parts and sending different PR for removing schema and other code. This might also require changes in operator.

This is implemented in #8259

Approach 2 - Replace bcrypt with other hashing [argon2, Crypto Module]

This does not require a lot of code changes. However, the MOST important issue is how can we remove bcrypt completely. We have to have a code to authenticate using bcrypt to even reset passwords.

Guy’s suggestion to place an extra field (like password_hash_type) using the upgrade script might work to enforce a password reset. However, password reset too requires to first authenticating with an old password which can only be done using bcrypt hashing.

The other option in my mind is without any extra field. We can implement the new hashing and wipe out the old password and have an empty string. Whenever a user logs in, we can reset password and empty string in stored DB would be allowed for the first time.
I don’t like this idea even while writing it as this goes against the practice. However, if we can allow this, it looks less complicated.

I don’t see any other clean and logical way of removing/replacing bcrypt without having bcrypt code itself in the next few releases.

These are two PR’s one implementing a node crypto module and the other argon2. This will not be integrated and only one PR will be merged if we go with the second approach.
#8252 - Replace bcrypt password hashing by crypto
#8213 - Replace bcrypt password hashing by argon2

@liranmauda @jackyalbo @vh05 @achouhan09 @dannyzaken

[guymguym]

@aspandey The operator account has a token that can reset the password to any user without checking for the verification password so it can send empty string and set (just tested it):

noobaa api account reset_password '{ "email":"guy-test", "password":"123456", "verification_password":"" }'

Also how critical it is for us to remove bcrypt immediately? can't we deprecate it for 1-2 versions before we remvoe it from the codebase? this would allow smoother transition if we need to support users self-service password updates. The only password we currently have is for the admin user that the operator creates. So perhaps it would be easy enough to have that transition done by the operator. In any case, I'm not too worried even if the previous password_hash_type="bcrypt" and we reject it, the operator token can still be used to reset it if needed.

[aspandey]

@aspandey The operator account has a token that can reset the password to any user without checking for the verification password so it can send empty string and set (just tested it):

noobaa api account reset_password '{ "email":"guy-test", "password":"123456", "verification_password":"" }'

Also how critical it is for us to remove bcrypt immediately? can't we deprecate it for 1-2 versions before we remvoe it from the codebase? this would allow smoother transition if we need to support users self-service password updates. The only password we currently have is for the admin user that the operator creates. So perhaps it would be easy enough to have that transition done by the operator. In any case, I'm not too worried even if the previous password_hash_type="bcrypt" and we reject it, the operator token can still be used to reset it if needed.

@guymguym Thanks Guy, your test results and suggestions actually unblocks many things.

1 - First thing your test suggest that we can reset any password with operator account, including users and admin. That solves many things as I was not able to find a way to reset a password without authenticating it with previous hashing method and which could be a bcrypted.

2 - Depreciating bcrypt in 1-2 versions would be a good point but now I think we can remove it from the start as we have found that we can reset any password even if we do not have bcrypt in our code. You had also suggested that this password is not being used much by users so it is better to get rid of it asap. Just a thought.

3 - About password_hash_type="bcrypt" . I think you suggested this flag to mainly identifying if the stored password is bcrypt or some other hash and accordingly FORCE user to reset it.
What we have observed suggests that we do not need it. All the bcrypt passwords strings starts with algo identifier "$2b$" (There could be 2a or 2y but we use 2b). So we can just fetch this password and based on this string we can say if it is bcrypt or not and force reset password.

[nimrod-becker]

@guymguym I might be mistaken here, but we only use password for the UI login.
The UI has been deprecated on 4.11 which is 6 versions very soon, what is the reason you think we should wait for 2 more versions?