[PM-26177] Add methods to create rotateable key set (2/3) #486
Conversation
iinuwa
changed the base branch from
main
to
km/PM-26354/asymmetric-keystore-wrapping
|
Great job! No new security vulnerabilities introduced in this pull request |
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## km/PM-26354/asymmetric-keystore-wrapping #486 +/- ##
============================================================================
+ Coverage 78.25% 78.39% +0.13%
============================================================================
Files 283 283
Lines 27713 27800 +87
============================================================================
+ Hits 21688 21793 +105
+ Misses 6025 6007 -18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I'll defer my review to @quexten. You are introducing a "next version" of these sets in the form of envelopes, right? Did you have a migration plan for the key sets or are they going to have to exist together for a while? |
No, a re-design of RotateableKeySets is not currently in-progress, and they are a supported API. If we do re-design them, then that's a separate thing that's not blocking here. |
| #[serde(rename_all = "camelCase", deny_unknown_fields)] | ||
| #[cfg_attr(feature = "uniffi", derive(uniffi::Record))] | ||
| #[cfg_attr(feature = "wasm", derive(Tsify), tsify(into_wasm_abi, from_wasm_abi))] | ||
| pub struct RotateableKeySet { |
There was a problem hiding this comment.
A thought on the API (open for discussion, as pseudo code):
- Move RotateableKeySet to crypto crate, then:
impl RotateableKeySet<...> {
pub fn new(externalKey: SymmetricCryptoKey, key_slot_to_wrap: KeyId) -> Self;
pub fn unlock(externalKey: SymmetricCryptoKey, key_slot_to_unwrap_to: KeyId) -> Result<KeyId, ...>;
}
That way we would not need to make the functions from the first PR in the set public, and also allow arbitrary keys to be wrapped, not just the user key.
There was a problem hiding this comment.
Yeah, I agree that's more functional.
Some questions:
- These methods would need to take
&KeyStoreContext(or moved toimpl KeyStoreContextand take&self) in order for the KeyId to mean anything, right? - Do we prefer the term "external key" over "wrapping key" in this context?
- Does it make sense to be generic over KeyId? Can we provide the same guarantees with asymmetric keys?
e, d = en-/decapsulation key pair
s = symmetric external key
k = key to wrap (symmetric key, or private key)
KeySet = {
SymmetricWrap(s, d),
Wrap(k, e), // symmetric or asymmetric, depending on type of `k`
Encaps(e, k),
}
In order for Wrap(k, e) to not be tampered by the server, Wrap must provide integrity and authentication, which RSA alone does not, right?
To make sure I understand, and taking in input from the other PRs, we'd want to go with something like (just roughed out):
// caller wrap
let store = client.internal.get_key_store();
let ctx = store.context();
let external_key_id = SymmetricKeyId::Local("external_key");
ctx.derive_symmetric_key_from_prf(prf, external_key_id)?;
let keyset = ctx.create_rotateable_key_set(external_key_id, SymmetricKeyId::User)?;
// or
// let keyset = RotateableKeySet::new(&ctx, external_key_id, SymmetricKeyId::User)?;// caller unlock
let keyset: RotateableKeySet = ...;
let store = client.internal.get_key_store();
let ctx = store.context();
let external_key_id = SymmetricKeyId::Local("external_key");
ctx.derive_symmetric_key_from_prf(prf, external_key_id)?;
let key_to_unwrap = SymmetricKeyId::Local("key_to_unwrap");
ctx.unlock_rotateable_key_set(external_key_id, keyset, key_to_unwrap);
// or
// keyset.unlock(&ctx, external_key_id, key_to_unwrap);// implementation
impl KeyStoreContext<...> {
fn create_rotateable_key_set(external_key_id: SymmetricKeyId, key_to_wrap: KeyId) -> Result<RotateableKeySet> {
// use ephemeral keys to create key set, don't store in context as key IDs
let (encapsulation_key, decapsulation_key) = rsa::make_key_pair()
let wrapped_decapsulation_key = self.encrypt_data_with_symmetric_key(...)
let encapsulated_key = UnsignedSharedKey::encapsulate_key_unsigned(self.get_key(key_to_wrap));
// I only how to do this for symmetric keys, not sure about asymmetric
let wrapped_encapsulation_key = self.encrypt_data_with_symmetric_key(key_to_wrap, encapsulation_key);
return RotateableKeySet {
// ...
}
}
}| // encapsulate user key | ||
| let encapsulation_key_id = AsymmetricKeyId::Local("encapsulation_key"); | ||
| ctx.make_asymmetric_key(encapsulation_key_id)?; | ||
| let encapsulated_user_key = |
There was a problem hiding this comment.
To me, this is unintutive behavior. At least the key_to_wrap should be exposed as a key id parameter here?
There was a problem hiding this comment.
yes, that would make more sense. I can change that and update the documentation.
|
Converting to draft pending further discussion with the KM team. |
🎟️ Tracking
PM-26177
📔 Objective
In order to set up PRF passkeys on mobile clients, we need to create a rotateable key set. This adds an internal method to the SDK that can be used by a future PRF derivation method.
⏰ 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