Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated PBKDF2 hasher with more complex format handling #319

Merged
merged 2 commits into from
Mar 8, 2024
Merged

Updated PBKDF2 hasher with more complex format handling #319

merged 2 commits into from
Mar 8, 2024

Conversation

loffa
Copy link
Contributor

@loffa loffa commented Feb 27, 2024

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.

@loffa loffa requested a review from iegomez as a code owner February 27, 2024 15:13
@iegomez
Copy link
Owner

iegomez commented Mar 1, 2024

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 #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
Copy link
Contributor Author

loffa commented Mar 2, 2024

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.

hashing/pbkdf2.go Dismissed Show dismissed Hide dismissed
hashing/pbkdf2.go Dismissed Show dismissed Hide dismissed
@iegomez
Copy link
Owner

iegomez commented Mar 7, 2024

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
Copy link
Contributor Author

loffa commented Mar 8, 2024

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.

@iegomez iegomez merged commit afd1bd7 into iegomez:master Mar 8, 2024
4 checks passed
@loffa loffa deleted the update-pbkdf2-hash branch March 8, 2024 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants