Expose PBKDF2#46
Conversation
Not-Nik
requested review from
beurdouche,
jschanck,
larseggert and
martinthomson
as code owners
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #46 +/- ##
=======================================
Coverage ? 68.89%
=======================================
Files ? 26
Lines ? 3244
Branches ? 3244
=======================================
Hits ? 2235
Misses ? 806
Partials ? 203
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean, well-structured addition that follows existing patterns (HKDF, HMAC). The FFI boundary is handled correctly — algid is manually destroyed and SymKey::from_ptr guards the key pointer. One resource-leak bug on an early-return path needs fixing before merge (see inline). Minor nits on naming and annotation consistency.
Good test coverage with RFC vectors; would benefit from exercising the SHA-384/SHA-512 code paths.
There was a problem hiding this comment.
Clean addition that follows established codebase patterns (HMAC, HKDF). The FFI boundary is handled correctly for the happy path, and test coverage includes RFC 7914 known-answer vectors.
The second commit ("Claude fixes") addressed the #[must_use] annotation on hmac_alg_to_prf_oid. The resource leak on the algid early-return path (lines 51–52) from the prior review remains open and should be resolved before merge — the suggested reordering is straightforward.
|
Keeping my approve so that you can make the suggested fixes and push. |
There was a problem hiding this comment.
Clean, well-structured addition that follows the existing HKDF/HMAC patterns. The FFI boundary is handled correctly — algid is manually destroyed after use, and SymKey's Drop impl handles the derived key.
The three issues from the prior review (resource leak on algid early-return, missing #[must_use], RFC 6070→7914 test naming) are all resolved across the follow-up commits.
Two items from the prior review remain open and should be addressed before merge:
-
hmac_alg_to_prf_oidvisibility (existing thread) —publeaks an FFI return type (SECOidTag::Type) into the crate's public API. The only consumer ispbkdf2::pbkdf2()within the crate, sopub(crate)is the correct visibility. This is consistent with the privatehmac_alg_to_ckmhelper. -
SHA-384/SHA-512 test coverage (existing thread) — all four tests exercise only
HMAC_SHA2_256. At least one known-answer vector for SHA-384 or SHA-512 would verify the OID mapping end-to-end.
There was a problem hiding this comment.
Clean, well-structured PBKDF2 wrapper that follows the crate's established HKDF/HMAC patterns. FFI boundary is handled correctly — algid lifetime is properly managed (the resource-leak issue from the prior review is resolved), SymKey::from_ptr guards the key pointer, and SECOID_DestroyAlgorithmID runs unconditionally after PK11_PBEKeyGen.
Prior review items addressed in this revision: resource leak on algid early-return, #[must_use] on hmac_alg_to_prf_oid, pub(crate) visibility, RFC 6070→7914 test naming, and SHA-384/SHA-512 test coverage.
Blocking: The latest commit has a syntax error — missing } closing rfc_7914_vector_iter_2 — which prevents compilation (see inline).
There was a problem hiding this comment.
Clean PBKDF2 wrapper that follows the crate's established HKDF/HMAC patterns. FFI boundary is correctly managed — algid is allocated after all fallible setup, destroyed unconditionally after PK11_PBEKeyGen, and SymKey::from_ptr guards the key pointer. Test coverage now spans all three HMAC variants with known-answer vectors.
All blocking issues from prior reviews are resolved (resource leak on algid early-return, missing }, pub(crate) visibility, #[must_use], RFC naming, SHA-384/512 coverage).
One prior suggestion remains open: extracting hmac_alg_to_prf_oid(alg) to a local variable to clarify that passing the same OID for both cipherAlgTag and prfAlgTag in PK11_CreatePBEV2AlgorithmID is intentional (thread).
There was a problem hiding this comment.
Clean PBKDF2 wrapper that follows the crate's established HKDF/HMAC patterns well. FFI boundary is correctly managed: all fallible operations (Slot::internal, SECItemBorrowed::wrap) are hoisted before algid allocation, SECOID_DestroyAlgorithmID runs unconditionally after PK11_PBEKeyGen, and SymKey::from_ptr guards the key pointer. Test coverage spans all three HMAC variants with known-answer vectors (RFC 7914 §11 for SHA-256, standard vectors for SHA-384/SHA-512).
All blocking issues from prior reviews are resolved (resource leak on algid early-return, pub(crate) visibility, #[must_use], test naming, SHA-384/512 coverage, syntax error).
One minor suggestion remains open (see inline): extracting the PRF OID to a local variable to clarify the PK11_CreatePBEV2AlgorithmID parameter semantics.
3 participants
Fixes #44