Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix dovecot passwdfile change #8145

Merged
merged 4 commits into from
Aug 1, 2021
Merged

Fix dovecot passwdfile change #8145

merged 4 commits into from
Aug 1, 2021

Conversation

fribergr
Copy link
Contributor

@fribergr fribergr commented Jul 26, 2021

Currently there is a few problems with the dovecot_passwdfile password-driver.

  • Trying to use two variables that are not initialized ($handle and $newhash - introduced in commit ca523bd7facf29aaabf08718252314fca05c83a5)
  • If the password::hash_password-function returns false (which could happen when using dovecot as password_algorithm), the code would still proceed to change the password to an empty string even though hashing failed. It now raises an error instead.
  • Escaping the hashed password result is not needed and actually breaks if option 'password_dovecotpw_with_method' is set to true resulting in an escaped entry in the passwd-file, i.e. \{SSHA256\}...

Let me know if something needs to be changed.

Thanks for developing an awesome mail client!

@alecpl alecpl added this to the 1.5.0 milestone Jul 29, 2021
@alecpl
Copy link
Member

alecpl commented Jul 29, 2021

I think it will be better to log the error and stop execution from inside the hash_password() method. This way it will be working for any driver that uses it.

@alecpl alecpl merged commit e4d7b9b into roundcube:master Aug 1, 2021
alecpl pushed a commit that referenced this pull request Aug 1, 2021
* Fix dovecot passwd-file handler
* Set new password hash correctly
* If password hashing failes during password change request, abort trying to change the password.
* Escaping hashed password is not needed and actually breaks if 'password_dovecotpw_with_method' is set to true (escaping the {METHOD} in the passwd file)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants