-
Notifications
You must be signed in to change notification settings - Fork 7
Implement HKDF #110
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
base: main
Are you sure you want to change the base?
Implement HKDF #110
Conversation
Adjust the bound for the `Clone` implementation of `Hmac<H>` from `H: Clone` to `H::Context: Clone` because `H: Hash` ins't guaranteed to implement `Clone`, wheras `H: HashContext` is.
CodSpeed Performance ReportMerging #110 will not alter performanceComparing Summary
Benchmarks breakdown
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #110 +/- ##
========================================
Coverage 99.14% 99.14%
========================================
Files 171 172 +1
Lines 39074 39258 +184
========================================
+ Hits 38738 38921 +183
- Misses 336 337 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
|
Sorry for not looking at this yet. But I think HKDF is a reasonable thing to live here, so I will review this soon. |
I wanted to use Graviola in a personal toy project and noticed the crate didn’t include HKDF, so I took the time to implement it.
I drew strong inspiration from
ring::hkdf, both in the implementation and the API.However, I chose not to implement extract and expand as methods on
SaltandPrk. Instead, they are provided as free functions in the module (hkdf::extractandhkdf::expand). I find this approach more intuitive. One potential downside is that the result of extract (Prk) is not tied to a specific hashing algorithm. ring avoids this issue by binding thePrkto the algorithm at extraction time. From my understanding, this does not create any crypto weakness.Questions/Considerations
Prkenum. It feels a bit hacky.saltparameter ofhkdf::extractshould be optional. According to RFC 5869, "the use of salt adds significantly to the strength of HKDF" and "designers of applications are therefore encouraged to provide salt values." Making the parameter non‑optional would force users to supply a salt (similar to how ring behaves).Should I add the same SPDX‑License‑Identifier header inDone in 9e040c7.hkdf.rs?hkdf::expandwill panic ifokm.len() > 255 * hash_len. Maybe we want to return an error instead. In my opinion, users will most often call this function with a fixed‑length array, so an error should be rare and panicking seems appropriate. Note that RustCrypto's and ring's APIs return aResult.Please, feel free to comment on any of these points.