Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

hash_password should fail if you don't supply a configuration file #11548

Closed
reivilibre opened this issue Dec 9, 2021 · 2 comments · Fixed by #12789
Closed

hash_password should fail if you don't supply a configuration file #11548

reivilibre opened this issue Dec 9, 2021 · 2 comments · Fixed by #12789
Labels
good first issue Good for newcomers T-Enhancement New features, changes in functionality, improvements in performance, or user-facing enhancements.

Comments

@reivilibre
Copy link
Contributor

reivilibre commented Dec 9, 2021

if "config" in args and args.config:
config = yaml.safe_load(args.config)
bcrypt_rounds = config.get("bcrypt_rounds", bcrypt_rounds)
password_config = config.get("password_config", None) or {}
password_pepper = password_config.get("pepper", password_pepper)

It seems to have some defaults but I somewhat doubt they are actually useful to anyone who generated their homeserver config 'properly'.
I guess it's too late to make it fail in case the current behaviour is useful to someone, but I wish it would at least print a warning.

Bit me just now (well, since last night...) as I needed to reset a LP.net user's password, and come to think of it it's not the first time either!

@richvdh
Copy link
Member

richvdh commented Dec 9, 2021

I don't think it would be at all unreasonable to make it fail without a config file.

@reivilibre reivilibre added the T-Enhancement New features, changes in functionality, improvements in performance, or user-facing enhancements. label Dec 10, 2021
@reivilibre reivilibre added the good first issue Good for newcomers label May 12, 2022
@reivilibre reivilibre changed the title hash_password should print out a warning if you don't supply a configuration file hash_password should fail if you don't supply a configuration file May 12, 2022
@DMRobertson DMRobertson linked a pull request May 19, 2022 that will close this issue
4 tasks
@madduck
Copy link

madduck commented Sep 12, 2022

I would find it very beneficial if the situation was clarified a bit more in the manpage:

  1. The defaults are 12 and "" and they cannot be changed without invalidating existing passwords;
  2. The example YAML file in the manpage should probably be changed to these defaults;
  3. The YAML file to use should be homeserver.yaml or the appropriate file in conf.d;
  4. homseserver.yaml does not need to specify these values, as the defaults will be used in case of absence.

Thanks,

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
good first issue Good for newcomers T-Enhancement New features, changes in functionality, improvements in performance, or user-facing enhancements.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants