Salt Encoding inconsistency between Caddyfile basicauth and hash-password

[0az]

The basicauth directive expects a base64-encoded salt, whereas the hash-password command expects an unencoded salt: it takes raw bytes in from the command line. This is pretty confusing: I had to pull out Delve despite submitting a small patch to the aforementioned command several months ago. I think I might have also run into it then when testing said patch, as well.

I think there's threefour ways to resolve this:

1. Change -salt to take in base64. This is a breaking change, so it's probably a bad idea.
2. Add a -salt-base64 option to take in base64. This is relatively self documenting, and also indicates that the plain -salt argument does not take in base64 by virtue of existing.
3. Add a -use-base64-salt option that changes -salt's behavior. This has the advantages of (2), and easily allows for a deprecation cycle and flag day.
4. Just document it in the help text. This leaves the rather annoying inconsistency, but it's also the easiest to implement.

Thoughts?

[francislavoie]

IMO, we should either simply not support hashing algorithms that require salts (remove scrypt support, keeping only bcrypt), or always randomly generate the salt if the algorithm requires it and embed it in the generated hash.

Currently Caddy only takes base64 encoded strings as input, but the only reason this was necessary was to support scrypt which gives its hash in binary in the Go implementation. The bcrypt alg has a nice format using $ as separators for each part.

The hash-password command also currently doesn't support configuring a the work factor for the algorithms, so bcrypt is always using a work factor of 10 which is arguably kinda weak in 2020. Since the work factor is embedded in the hash though, users can provide their own stronger hashes by generating it elsewhere then base64 encoding the hash before giving it to Caddy... But that's not user-friendly at all.

[mholt]

Thanks for bringing this up.

Turns out there's a proposal to introduce nicer, bcrypt-like APIs for scrypt, argon2, etc, in Go: golang/go#16971

Meanwhile, we can raise the cost factor to 14 in Caddy. That should be plenty future-proof for now.

Edit: I'll circle back on the main topic of this thread when I have a chance. Still swamped trying to get the site upgrades finished.

[francislavoie]

I think increasing the default work factor is viable now that #3465 was implemented. Beforehand, that would've been super infeasible because it would make Caddy's resource usage so high if Caddy gets hammered with auth requests.

[mholt]

We can probably change/fix the hash-password command as it's intended for interactive use only, plus I would guess that not many people use it yet anyway.

Is there any convenient way to pass raw bytes into a command line argument? I feel like it's a bug then and should accept the base64-encoded salt instead.

Or if that's not really a good API, then maybe we should drop the --salt parameter entirely and have Caddy generate one for the user when it is required.

[francislavoie]

Or if that's not really a good API, then maybe we should drop the --salt parameter entirely and have Caddy generate one for the user when it is required.

I would prefer that. (Or drop scrypt altogether but I digress.)

I can do this one soon unless @0az wants to take a crack at it

[mholt]

Scrypt and other modern KDFs are typically more recommended than bcrypt by cryptographers, from my conversations with some.

But scrypt does need better APIs. They're coming, as I linked to above. Maybe we should wait until then, and add a note in the docs that it's possible that the command / config could be changing?

[NightMachinery]

I don't understand; Is the current bcrypt implementation throwing away the salt? I generated my hash using caddy hash-password -algorithm bcrypt -salt "$GARDEN_SALT0" -plaintext "$GARDEN_PASS0", but I can freely change the salt in my caddyfile and it'll accept the requests anyway.

On the lack of tooling in go; Can't you use another language for doing this part? Python has the passlib library, for example. I imagine there must be good C++ libraries, too.

[francislavoie]

bcrypt inherently doesn't allow a salt parameter as input, because it always generates its own salt, and the salt value is built into the output hash. The salt parameter is only meant for hashing algorithms where the API doesn't compute the salt on their own, i.e. scrypt, but my argument here is that that's bad and that the salt should always be randomly generated.

[francislavoie]

I think this'll be resolved by #4720. I don't think we should keep scrypt, and we don't need the extra base64 encoding for bcrypt.