[PM-26354] Add methods to create rotateable key sets from PRF #494
Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #494 +/- ##
==========================================
+ Coverage 78.36% 78.44% +0.07%
==========================================
Files 291 293 +2
Lines 29343 29519 +176
==========================================
+ Hits 22994 23155 +161
- Misses 6349 6364 +15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
iinuwa
force-pushed
the
km/PM-26177/create-prf-user-key-set
branch
from
2514070 to
32ca764
Compare
| }) | ||
| } | ||
|
|
||
| fn unlock<Ids: KeyIds>( |
There was a problem hiding this comment.
This method is currently unused since we don't yet have plans to use the rotateable key set on mobile, only to create them. However, it implements functionality that is currently done in TypeScript that could be moved to the SDK.
I can remove it until we decide to either migrate the browser apps to this SDK method or start using it in the mobile apps.
| let pub_key = SpkiPublicKeyBytes::from(pub_key_bytes); | ||
| let encapsulation_key = AsymmetricPublicCryptoKey::from_der(&pub_key)?; | ||
| // TODO: There is no method to store only the public key in the store, so we | ||
| // have pull out the encryption key to encapsulate it manually. |
There was a problem hiding this comment.
Note: I think this needs a KM-owned tech debt follow-up ticket. I'll make a jira ticket when this merges, but adding the APIs for this feels out of scope for an external PR.
|
Thank you for the review!
I looked around the clients codebase, but I don't see a full test vector for an end-to-end (PRF, User Key) -> RotateableKeySet and back. There are mocked values that don't verify the actual cryptography in https://github.com/bitwarden/clients/blob/main/libs/auth/src/common/login-strategies/webauthn-login.strategy.spec.ts and https://github.com/bitwarden/clients/blob/main/libs/common/src/auth/services/webauthn-login/webauthn-login.service.spec.ts. The Rust types guarantee some of the other invariants tested in. Did you have some specific test vectors in mind? |
There was a problem hiding this comment.
Note: I'd recommend moving RotateableKeySet into it's own file, since it is a different cryptographic construction. key_rotation.rs is really just meant for "User Key Rotation", which may interact with RotateableKeySets, but is a different use-case.
(As a KM note, key_rotation.rs should probably be renamed to user_key_rotation.rs)
|
Claude finished @iinuwa's task —— View job Code Review CompleteReviewing PR #494: Add methods to create rotateable key sets from PRF Summary of ChangesThis PR implements methods to create rotateable key sets derived from PRF (Pseudo-Random Function) values for unlock passkeys on mobile clients. The implementation adds:
Critical Issues🔴 Security: File Organization Violates Repository GuidelinesLocation: Issue: Per the review discussion and CLAUDE.md guidelines,
However, I notice the file is in its own file ( 🟡 Test Coverage: Missing Cross-Platform VerificationLocation: Issue: Tests verify internal correctness but lack cross-platform test vectors. The author noted they couldn't find full end-to-end test vectors in the TypeScript codebase. Recommendation: Create explicit test vectors documenting the expected PRF → key derivation to ensure compatibility between Rust SDK and TypeScript clients. Consider: #[test]
fn test_prf_compatibility_with_typescript() {
// Known PRF value from TS implementation
let prf = hex::decode("...").unwrap();
let expected_key_bytes = hex::decode("...").unwrap();
let key = derive_symmetric_key_from_prf(&prf).unwrap();
// Verify key matches TS-derived key
}This ensures cryptographic compatibility across platforms and serves as regression protection. Suggested Improvements📝 Documentation: Function Purpose UnclearLocation: Suggestion: Enhance documentation to explain the broader context: /// Takes the output of a PRF and derives a symmetric key for protecting rotateable key sets.
///
/// This is used in WebAuthn PRF-based authentication flows where the PRF output
/// from a security key is used to derive an upstream key that protects the user's
/// encryption key through a RotateableKeySet.
///
/// The PRF output must be at least 32 bytes long. If longer, only the first 32 bytes
/// are used. The key is derived using HKDF-SHA256 to produce an AES-256-CBC-HMAC key.
///
/// # Security
/// - Returns an error if the PRF is all zeros (uninitialized)
/// - Uses the same key stretching as master key and PIN key derivation
///
/// # Arguments
/// * `prf` - Raw PRF output from WebAuthn authenticator (≥32 bytes)
pub fn derive_symmetric_key_from_prf(prf: &[u8]) -> Result<SymmetricCryptoKey, CryptoError>🎨 Code Quality: Test Helper NamingLocation: Issue: The test helper Suggestion: Rename for clarity: /// Returns deterministic test bytes for a given length.
/// Each byte equals (index % 255).
fn deterministic_test_bytes(len: usize) -> Vec<u8> {
(0..len).map(|x| (x % 255) as u8).collect()
}🎨 Code Quality: Inconsistent Test OrganizationLocation: Issue: The test Current code: fn test_long_prf_truncated_to_proper_length() {
let long_prf = pseudorandom_bytes(33);
let prf = pseudorandom_bytes(32); // Different bytes!
let key1 = derive_symmetric_key_from_prf(&long_prf).unwrap();
let key2 = derive_symmetric_key_from_prf(&prf).unwrap();
assert_eq!(key1, key2); // This happens to pass due to deterministic generation
}Should be: fn test_long_prf_truncated_to_32_bytes() {
let long_prf = pseudorandom_bytes(33);
let truncated_prf = &long_prf[..32];
let key1 = derive_symmetric_key_from_prf(&long_prf).unwrap();
let key2 = derive_symmetric_key_from_prf(truncated_prf).unwrap();
assert_eq!(key1, key2, "Long PRF should be truncated to 32 bytes");
}🟡 API Design: Unused Method Should Be Removed or Feature-GatedLocation: Issue: The Recommendation: Either:
/// Unlocks a rotateable key set using the upstream key.
///
/// # Future Migration
/// This method will be used when webauthn-login-strategy is migrated from
/// TypeScript to the SDK. See: [ticket reference]
///
/// Until then, this is tested but unused in production code.
#[cfg(test)]
pub(crate) fn unlock<Ids: KeyIds>(Given the PR description states this is only for creating key sets (not unlocking), I recommend removing the unused method to keep the PR focused. 🎨 Code Quality: Inconsistent Error Enum Variant NamingLocation: Observation: Changed Good Practices Observed✓ Proper zero-key validation - Prevents uninitialized PRF values from being used Status of Previous Review FeedbackAddressed:
Pending:
Action Items for Author
Overall AssessmentThis is a well-structured implementation that correctly ports TypeScript functionality to Rust. The cryptographic approach is sound, following established patterns in the codebase. The code quality is high with good test coverage, proper error handling, and thoughtful use of Rust's type system. The main concerns are around documentation completeness and cross-platform compatibility verification. The implementation itself appears secure and correct. Recommendation: Approve with minor improvements suggested above. |
|
iinuwa
force-pushed
the
km/PM-26177/create-prf-user-key-set
branch
from
b9bb1f0 to
d33e203
Compare
iinuwa
force-pushed
the
km/PM-26177/create-prf-user-key-set
branch
from
d33e203 to
fef1668
Compare
iinuwa
changed the title
|
I rebased and incorporated the changes mentioned by Bernd, squashed the lint fixes, and added some other commits to make CI all happy. This is ready for review. (I updated the Jira ticket to PM-26354 to separate it from the mobile work.) |
|
Looks like there are some new merge conflicts, other than that LGTM. |
| /// The PRF output must be at least 32 bytes long. | ||
| pub fn derive_symmetric_key_from_prf(prf: &[u8]) -> Result<SymmetricCryptoKey, CryptoError> { | ||
| let (secret, _) = prf.split_at_checked(32).ok_or(CryptoError::InvalidKeyLen)?; |
There was a problem hiding this comment.
We should probably mention in the docs that only the first 32 bytes of the PRF output are processed:
/// The PRF output must be at least 32 bytes long. If longer, only the first 32 bytes are used and the remainder is discarded.
pub fn derive_symmetric_key_from_prf(prf: &[u8]) -> Result<SymmetricCryptoKey, CryptoError> {4 participants
🎟️ Tracking
PM-26354
📔 Objective
In order to set up unlock passkeys on mobile clients, this PR adds a method to create a rotateable key set derived from a PRF value.
This is based on existing code in the TypeScript library and web vault:
⏰ Reminders before review
team
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmedissue and could potentially benefit from discussion
:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes