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

Support password hashes in addition to plain text passwords #39

Closed
clach04 opened this issue Jun 11, 2023 · 3 comments
Closed

Support password hashes in addition to plain text passwords #39

clach04 opened this issue Jun 11, 2023 · 3 comments
Assignees
Labels
enhancement New feature or request

Comments

@clach04
Copy link
Contributor

clach04 commented Jun 11, 2023

Config files and command line parameters accept plaintext passwords. Be nice to have hashed passwords (with salt).

  • BKDF2, Argon2, bcrypt, is not part of stdlib but would be a good choice.
  • blake2 is in Python 3.6 (but not earlier :-()
  • sha-256 is everywhere

OWASP best practice link - https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html

@clach04 clach04 added the enhancement New feature or request label Jun 11, 2023
@9001
Copy link
Owner

9001 commented Jun 13, 2023

You're not the first to bring this up :-) we briefly considered it while doing the nixos packaging, but we settled with just agenix instead.

Just for context, the reasoning at the time was:

  • as copyparty doesn't do anything like sessions or jwt, the passwords will be sent continuously over the wire regardless (which is mostly fine, thanks to TLS)
  • adding support for hashed passwords felt like a sizable undertaking
  • it would be extremely taxing on the cpu, as each request would need to have the password hashed and compared
  • it would probably be fully incompatible with a few features
  • and if you manage to obtain the list of passwords, you probably have bigger problems :-p

But it's true that an astray list of hashed passwords would usually cause much less damage, for example if the config file was left public by accident -- and the performance concern could be partially mitigated by caching recent passwords, and there is already a bruteforce protection thing which should catch the worst offenders.

Regarding stuff that would be incompatible with hashed passwords, so far I can think of the following:

  • the smb server (but I hope nobody actually uses that...)

If / when we end up adding this, I think we should do a few rounds of sha512 + salts by default, but also add some optional dependencies to enable stronger choices (argon2/bcrypt and so), as I don't think python really offers the horsepower to do proper hashing on its own. But I'd love to be proven on this :-)

So while this may not be top priority, it's definitely worth reconsidering at some point 👍

@clach04
Copy link
Contributor Author

clach04 commented Jun 17, 2023

I see, yep that's tricky :-)

Any change would require either extra overhead for processing on each (web) request or a switch to a token mechanism. A well as not working for SMB.

This does sound like nice to have but low priority.

Possibly alternative would be a plugin mechanism (likely with deficiencies with respect to things like SMB) so people can come up with their own approach (sort of like a poor mans PAM support). I've not looked into the code to see if this is feasible.

I would probably be happy if I could use basic auth with a reverse proxy and then skip password for my use cases but was unable to get that to work (not sure if it's an issue with my redirection headers). Right now for my public server I'm using https with reverse proxy and then a CopyParty password. If I get time I may look into a plugin idea.

For regular use case, this is probably over kill :-)

@9001 9001 closed this as completed in e197895 Jun 25, 2023
@9001
Copy link
Owner

9001 commented Jun 25, 2023

Found out it wasn't that hard to add this after all!

There is support for sha2-512 (always avialable), scrypt (needs openssl 1.1), and argon2id (needs argon2-cffi) -- they can be configured to fit a particular usecase, but the defaults spend about 0.4 sec when validating a new password.

The hashes are cached, so there won't be too much of a performance hit from legit users, and the bruteforce limits will hopefully prevent too much damage from someone being funny ;-)

This probably carries a tiny bit of a performance hit even when the feature is not enabled, but nothing noticeable... although I haven't checked yet hehe

EDIT: but I'm still curious why your basic-auth setup was failing -- if you want to handle authentication on the reverse-proxy side, you probably want to prevent the authorization header from hitting copyparty at all, since otherwise it'll see a password it doesn't recognize and ask the client to clear it. You can see what headers copyparty are getting from the reverse-proxy with --ihead '*' if maybe that helps...

Let me know if you hit any issues!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants