9001 / copyparty

Portable file server with accelerated resumable uploads, dedup, WebDAV, FTP, TFTP, zeroconf, media indexer, thumbnails++ all in one file, no deps
MIT License
571 stars 34 forks source link

Support password hashes in addition to plain text passwords #39

Closed clach04 closed 1 year ago

clach04 commented 1 year ago

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

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

9001 commented 1 year ago

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:

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:

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 commented 1 year ago

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 commented 1 year ago

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!