New hash strategies

[iegomez]

This PR adds support for bcrypt and argon2 hashing strategies. It also refactors the awfully named commons package: opening DB and matching topics are now part of the backends package, while hashing and comparing are now under the hashing package.

TODO:

• Test hashing package.
• Document changes, both hashing additions and breaking changes to salt encoding options which were moved to the backend hasher.

[garethhcoleman]

Hi! I sat down having finally found time to work on my small part and I see you have leaped ahead! Wow! I've reviewed your code and it seems really nicely done, clean and easy for me to understand despite my newness with Go. I totally missed the salt encoding option - nice. I've looked for ways I could improve it - there are a couple of comments where the search and replace 'common' for 'hashing' has left them slightly difficult to read. I could also usefully clean up my comments left in from my initial work on the argon2 functions. I'll go ahead and try to make a change to this PR to implement this as I think it is fairly uncontroversial and you can always revert them if I made a mistake.

In fact, I can't see how the correct hashing functions are selected and called except in the pw.go file - unless that is also part of the regular execution? (edit - noticed your comment and perhaps this is part of the WIP.) I obviously have a way to go with go yet!

[garethhcoleman]

I've got a minor tidyup commit but I get ERROR: Permission to iegomez/mosquitto-go-auth.git denied to garethhcoleman.
git status gives me:
On branch feature/hashing-options Your branch is ahead of 'origin/feature/hashing-options' by 1 commit.

Also, both argon2 and pbkdf2 have iterations as a param but the sane defaults are like 3 and 10000 respectively! Any bright ideas how to handle that other than just signposting users?

[iegomez]

In fact, I can't see how the correct hashing functions are selected and called except in the pw.go file - unless that is also part of the regular execution? (edit - noticed your comment and perhaps this is part of the WIP.) I obviously have a way to go with go yet!

Yeah, it's still a WIP so a lot is missing right now. I'll try to finish everything tomorrow.

I've got a minor tidyup commit but I get ERROR: Permission to iegomez/mosquitto-go-auth.git denied to garethhcoleman.
git status gives me:
On branch feature/hashing-options Your branch is ahead of 'origin/feature/hashing-options' by 1 commit.

Oops, I forgot about permissions! I'm not sure git or github allow branch permissions, I'll check and see if there's any alternative to usual fork and PR.

[garethhcoleman]

Yeah, it's still a WIP so a lot is missing right now. I'll try to finish everything tomorrow.

I certainly didn't mean to put pressure on you to work to any timescale, I hope you didn't think that!

Would it help if I wrote some updated text documenting the new options for the README ?

Or if you have something else please just suggest it!

[iegomez]

I certainly didn't mean to put pressure on you to work to any timescale, I hope you didn't think that!

Don't worry, I didn't take it that way. As you can see, I didn't work on it yesterday after all. 😄

Would it help if I wrote some updated text documenting the new options for the README ?

Or if you have something else please just suggest it!

Definitely, that'd be very helpful. We'll need to document general hashing options and specificy what happens when missing, e.g.:

# If hasher is not set, the plugin will default to PBKDF2.
auth_opts_hasher argon2

We'll also need to document needed options, valid ranges and defaults for each hasher:

auth_opts_argon2_iterations <some_number>
...
auth_opts_pbkdf2_iterations <some_number>
...
auth_opts_bcrypt_cost <some_number>

Finally, we need to be able to set a specific hasher per backend and provide their options while telling it'll either use a default or plugin wise provided hasher, whatever was configured, if none is specified for a given backend, e.g.:

auth_opts_redis_hasher argon2
auth_opts_redis_argon2_iterations <some_number>
...

I'll leave any changes to the README away from this PR so you may work on it on your fork and open a different PR where we do any documentation changes. That way we won't have any trouble merging it later.

[iegomez]

Hey, @garethhcoleman! cc @coldfire84!

I just pushed changes that - besides adding some refactoring, cleanup and other stuff -, inject a hasher to each backend that needs it: that's all but grpc, http and custom since we don't control how their auth works (we don't in remote jwt either, but since it may use a local DB, it needs a hasher for those cases).

Now, there are some subtleties here: there are general hashing options as well as per backend ones, but the way it works is that in practice every backend gets a hasher and whether it uses specific options or general ones depends on the auth opts passed (see https://github.com/iegomez/mosquitto-go-auth/blob/feature/hashing-options/hashing/hashing.go#L60, and please do comment on an idea I had of adding a specific prefix for hashing options but didn't dare to implement because auth_opts_hashing_hasher looked ugly 😅 ).
This means that if there are general hashing options available but no backend ones, then every backend will use those general ones for its hasher; if there are no options available in general and none for a given backend either, that backend will use defaults; if there's options for a given backend but no general ones, the backend will use its own and any backend that doesn't register a hasher will use defaults. This will need some seriously clear docs for it to be understood and tests to back it up.

The PR also brings a related breaking change: salt encoding options are deprecated (sorry @coldfire84) in favor of per hasher salt encoding. The most relevant detail here is that if you don't care about the hasher and just want to use PBKDF2 or whatever is set as the general hasher with default or given values, you'd normally avoid declaring a hasher for the backend or even the whole plugin, but if you want to use a salt encoding for a backend that's not the default base64 one (right now that's only utf-8, but it may be extended), then you'll need to at least tell what hasher you're gonna use for that plugin and pass the salt encoding for it (see Mongo tests for an example: https://github.com/iegomez/mosquitto-go-auth/blob/feature/hashing-options/backends/mongo_test.go#L201). Since @coldfire84 worked this one out along with his Mongo additions, I'm pulling him in too. I'm absolutely not sure this is the best option and am very open to better alternatives (e.g., we could still declare the salt encoding per backend and pass it to the hasher, but it makes pw more cumbersome), it's just the best I could come up with during a really tight sprint at work.

Another notable issue that you mentioned earlier has to do with the pw utility and the iterations default (apart from some work that could be done to integrate it better and docs efforts): the hashing package has reasonable defaults for both PBKDF2 and Argon2, but the pw only has a flag hint: https://github.com/iegomez/mosquitto-go-auth/blob/feature/hashing-options/pw-gen/pw.go#L14. I'm too tired/lazy to do something about it right now (i.e., I couldn't come up with any good short flag options to differenciate them while I was working on this PR), so any suggestions are very welcome.

Finally, there are 2 unrelated but notable plugin issues remaining: #64 and #31. The first one needs infrastructure and, most importantly, time from me (sorry @sanhardik, now it's really on the top of my list); in the latter I'm not so sure about my initial intent: sure, Files backend needed some work for ACLs that was done when supporting new MOSQ_ACL options, but I now believe every other backend may handle subscription/read access on their own, so it may not be an issue at all. As for #60 and #67, the former is already merged and awaiting for confirmation from OP, while the latter is just cleanup and won't break anything.

Why do I mention this? Because if we manage to resolve the reported CPU issues, or they turn out to be unrelated, then once this hashing effort is done I think the related release would be a candidate to reach version 1.0! 🚀 That's why I'm adding you both, @garethhcoleman and @coldfire84, as collaborators (check your emails and ping me if you didn't receive the invitation, and @garethhcoleman, you may add any docs you were working on directly now), and asking if you can review this PR before I feel comfortable merging it and reaching that release promise. Of course, feel no pressure and instead free to say no to this request. In any case, I'm really grateful for your contributions.

Cheers!

[iegomez]

I just pushed some changes that rename argon2 to the correct argon2id name and fix an error in the implementation. I also removed salt encoding for that hasher since it expects base64 in the string representation anyway.

I added tests for the hashing package that cover both initializing hasher from options and generating hashes and then checking passwords against them.

I've seen a random PBKDF2 failure come up from time to time when using UTF8 salt encoding, so I guessed some salts when converted to string generate the $ separator char (which makes a point for using base64 encoded salts and makes me wonder why some language or store implementations that inspired the original issue and utf8 option use it at all). I put a print in the generation to check and, sure enough, that's the case:

salt bytes: [229 226 227 45 61 36 228 123 95 10 38 93 236 212 193 106]
salt string:���-=$�{_
&]���j
ERRO[0000] invalid PBKDF2 hash supplied, expected length 5, got: 6 
    TestPBKDF2: hashing_test.go:145: 
        	Error Trace:	hashing_test.go:145
        	Error:      	Should be true
        	Test:       	TestPBKDF2

So yay for tests, we caught a live bug! The naive solution would be to check the string form of the salt and regenerate before hashing if a $ comes up, but it'd probably be better to just encode/decode to a format that prevents it as a middle step or generate safe salts from the start. Not sure yet, so I won't fix it for now but will surely do before merging.

[garethhcoleman]

Hiya @iegomez - happy to review, I've had a quick look just now and things are looking pretty spiffy to me, but I'll go over it more carefully in the next couple of days.

Very impressed also that you managed to do this 'during a busy week at work'!! It shows the truth to the motto - "If you want something done, give it to a busy person".

I debated the question of argon2 vs argon2id but for some reason decided not to mention it but I'm glad you decided to give it the correct name as we don't provide facilities to use the other variants, it is a more accurate name.

I feel like I'm lucky to be involved in live development where the principles of good software (e.g. unit testing) are being demonstrated right in front of me! And I really appreciate you taking the trouble to invite me in as a collaborator when (up to now) you have put as much work into that as I am returning! But hopefully I can produce some good docs that make me a net asset! Thank you again!

Its likely to take me three or four days to get an opportunity to work on this more, I hope that doesn't block anything but ping me if it does.

[iegomez]

@garethhcoleman @coldfire84 I've just pushed a commit that solves the salt problem and documents the hashing options.

Somewhat unrelated, it solves a security issue where cache keys weren't being hashed (I know, I'm an idiot 😅), and since that's a critical one, I feel it should be merged ASAP.

As I mentioned, this is a breaking change. Also, I did some testing regarding CPU problems and couldn't reproduce the issue, so I'll also release this as 1.0 once I get an approval from either of you to merge it. 🚀 🎉 But please, even if you approve so I may merge, let's keep discussing anything related to hashing and let me know about any concerns you may have about it.

Thanks and cheers!