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

Implement hashing algorithms + HMac support #193

Merged
merged 8 commits into from
Jun 5, 2024

Conversation

Dekkonot
Copy link
Contributor

@Dekkonot Dekkonot commented May 11, 2024

Implements the basics of hashing algorithm support, along with hmac.

Does so via 2 new functions:

serde.hmac(algorithm: HashAlgorithm, message: string | buffer, secret: string | buffer): string
serde.hash(algorithm: HashAlgoritm, message: string | buffer): string

Algorithm Choice

After the discussion in #105, I elected to include sha1, sha2, sha3, blake3, and md5 for this implementation. This is mostly for practical reasons: algorithms for passwords are more complicated, and other algorithms (like xxhash) don't HMAC well.

I do want to call special attention to GxHash, as it uses aes and thus requires us to mess with our target features if we want to include it. I'm not going to make the call for that, since I'm not Lune's maintainer.


If merged would close #105.

@Dekkonot Dekkonot marked this pull request as draft May 11, 2024 01:47
Copy link
Collaborator

@filiptibell filiptibell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took a quick look and everything seems pretty straightforward. Not actually reviewing yet since this is a draft PR, but I thought I'd mention that this PR will need a bit of file refactoring since it is from before the split-into-crates merger / lune 0.8.4.

Would you like to make those changes in this PR or open a new one? Might be easier to open a new one and copy & paste the files since 0.8.4 was a pretty big change 😬

@Dekkonot
Copy link
Contributor Author

Dekkonot commented Jun 1, 2024

Yeah this'll need a bit of a refactor. I've been putting it off since I got a new laptop and haven't set up a Rust environment on it yet.

I think using this PR is more than feasible since I can just force push this branch, but it might look weird. If you have a preference I'll respect it but otherwise that's what I'll do.

@filiptibell
Copy link
Collaborator

I think using this PR is more than feasible since I can just force push this branch, but it might look weird. If you have a preference I'll respect it but otherwise that's what I'll do.

Force push is no problem! The squash and final changeset looking good is all that matters to me 👍

@Dekkonot Dekkonot marked this pull request as ready for review June 2, 2024 23:49
@Dekkonot Dekkonot requested a review from filiptibell June 2, 2024 23:50
Copy link
Collaborator

@filiptibell filiptibell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything looks great! Thank you for including all the different test cases, too. Maybe we can consider including some conversion utilities for hex, base64 and similar things in serde in the future but for now returning a hex string works well.

@filiptibell filiptibell merged commit 5a292aa into lune-org:main Jun 5, 2024
6 checks passed
@Dekkonot Dekkonot deleted the crypto-again branch June 5, 2024 17:03
@CompeyDev
Copy link
Contributor

For some reason, the changelogs and release notes don't include anything about this being merged? I only found out that this exists today while going through the serde builtin source. Can this be fixed (I doubt now - it might be too late)?

@filiptibell
Copy link
Collaborator

For some reason, the changelogs and release notes don't include anything about this being merged? I only found out that this exists today while going through the serde builtin source. Can this be fixed (I doubt now - it might be too late)?

Both this PR and the main branch have up to date changelogs with the new functions included and example usage for them. They haven't been released yet.

@CompeyDev
Copy link
Contributor

Whoops - thought they were released in 0.8.4!

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.

serde library should have hash functions
3 participants