caddy icon indicating copy to clipboard operation
caddy copied to clipboard

caddyauth: Speed up basicauth provision, deprecate `scrypt`

Open francislavoie opened this issue 3 years ago • 1 comments

When provisioning basicauth, there's a big performance hit on loading/reloading configs because it calculates a fake password; we don't need to do this on-the-fly, it's totally fine to hash the password once offline and just hardcode it.

I did change the Hasher interface unfortunately because it's really the cleanest way to implement this, but I'm not aware of any plugins that implement another hashing algorithm anyways, so the breaking change should probably be fine.


FWIW I still think we should drop scrypt altogether, and stop using base64-encoded strings.

I don't see any compelling reason to keep scrypt, it has way too many knobs, and having the salt not being built-in makes it very error prone to use correctly. See https://passlib.readthedocs.io/en/stable/lib/passlib.hash.scrypt.html for example, which discourages its use.

And no need for base64 because bcrypt already uses a safe string format (called Modular Crypt Format), so we're just base64 encoding an already-ASCII string which just obfuscates the configured work factor (which is plainly visible in the bcrypt header). For example, the bcrypt I hard-coded in here, base64 decoded, is $2a$14$X3ulqf/iGxnf1k6oMZ.RZeJUoqI9PX2PM4rS5lkIKJXduLGXGPrt6 which you can clearly see at the start $14 which means the work factor is 14. That's its only config knob, and can tell you if it's time to re-hash the passwords if that work factor is now too weak (14 is still pretty strong).

francislavoie avatar Apr 22 '22 02:04 francislavoie

I decided to make the commit to deprecate scrypt and add support for Modular Crypt Format for bcrypt (and caddy hash-password now outputs them in that format, without the redundant base64).

This is a big UX simplification and avoids a lot of potential footguns that scrypt carries.

francislavoie avatar Apr 22 '22 03:04 francislavoie

@mholt can I bump this up your queue? I'd really like to get this in for 2.6.0 betas, since it involves a deprecation and a significant performance fix.

francislavoie avatar Aug 23 '22 05:08 francislavoie

Yeah. But events first :)

mholt avatar Aug 23 '22 14:08 mholt