search

iegomez / mosquitto-go-auth

Auth plugin for mosquitto.
MIT License
495 stars 165 forks source link

Updated PBKDF2 hasher with more complex format handling #319

Closed loffa closed 4 months ago

loffa commented 5 months ago

This change adds compatibility with the hash-format used in Chirpstack 4.

Since Chirpstack 4 is rewritten in Rust the format for storing passwords has changed a bit. The new format now follows the specification PHC-String-Format.

The changes in this PR identifies if the new or old format is used in the supplied hash and then extracts parameters from that. Comparison is done only on the password part of the hash since that is equal between both old and new structure.

An update to Go 1.21 was needed to utilize the new built in slices package for comparison. This could be removed and rewritten for the older go version as well if preferred.

iegomez commented 5 months ago

Hey, @loffa! Thanks for this, I haven't followed ChirpStack's development in years and didn't realize the format change.

I haven't reviewed your code yet, but I'm a bit hesitant on upgrading to Go 1.21 at the moment as I think that'd merit a major version bump. Just as an example, although Go is almost non existent within my team at work, we have a small service I wrote that was limited to Go 1.19 because of the client's restrictions. I can see people facing something similar at their work places.

So, if you don't mind, I'd prefer to stick to 1.18 for the time being. I'm working on improving the caching system (see https://github.com/iegomez/mosquitto-go-auth/issues/310) and I want to completely redo the whole options mechanism, aka auth_opt_.... When all that's done, releasing a new major version and bumping Go and dependencies would make sense.

loffa commented 5 months ago

Hey, I removed the need for the Go version update and implemented the slices compare function instead. The PR has been updated with these changes.

iegomez commented 4 months ago

Sorry for the delay, @loffa. Tests are passing and the two cases flagged by CodeQL are not real concerns IMO, it's just the i/l keys and the hashing format that are getting logged, not something sensible.

If you agree, I'll merge.

loffa commented 4 months ago

I agree!

I can not see why it would be a security risk to log the hash-format or keys that can't be read in the hash text when they are unsupported either way.