-
-
Notifications
You must be signed in to change notification settings - Fork 127
fix(send): close SKDM flow gaps for forward secrecy and cache hygiene #604
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
Changes from all commits
79ce0b8
b4fdad2
01b1859
ae0ab11
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -67,6 +67,72 @@ impl Client { | |
| Ok(()) | ||
| } | ||
|
|
||
| /// Forward-secrecy rotation when participants leave a group. Mirrors WA | ||
| /// Web's `removeParticipantInfo` (`GroupParticipantHelpers.js`): if any | ||
| /// removed user had `has_key=true`, delete the bot's own sender key for | ||
| /// the group and wipe `sender_key_devices` so the next send takes the | ||
| /// `force_skdm=true` path (`!key_exists`) and redistributes to all | ||
| /// remaining participants. | ||
| pub(crate) async fn rotate_sender_key_on_participant_remove( | ||
| &self, | ||
| group_jid: &str, | ||
| removed_user_ids: &[&str], | ||
| ) { | ||
| if removed_user_ids.is_empty() { | ||
| return; | ||
| } | ||
|
|
||
| // Read failure → rotate anyway. Better to pay the redistribute cost | ||
| // than leave the sender key in place after a removal we couldn't audit. | ||
| let (rows, read_failed) = match self | ||
| .persistence_manager | ||
| .get_sender_key_devices(group_jid) | ||
| .await | ||
| { | ||
| Ok(r) => (r, false), | ||
| Err(e) => { | ||
| log::warn!( | ||
| "rotate_sender_key_on_participant_remove: read failed for {group_jid}: {e} \ | ||
| — rotating conservatively" | ||
| ); | ||
| (Vec::new(), true) | ||
| } | ||
| }; | ||
|
|
||
| let any_had_key = rows.iter().any(|(jid_str, has_key)| { | ||
| *has_key | ||
| && jid_str | ||
| .parse::<Jid>() | ||
| .ok() | ||
| .is_some_and(|jid| removed_user_ids.iter().any(|u| *u == jid.user.as_str())) | ||
| }); | ||
| if !read_failed && !any_had_key { | ||
| return; | ||
|
Comment on lines
+109
to
+110
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This early return treats an empty Useful? React with 👍 / 👎. |
||
| } | ||
|
|
||
| use wacore::libsignal::store::sender_key_name::SenderKeyName; | ||
| use wacore::types::jid::JidExt; | ||
| let snapshot = self.persistence_manager.get_device_snapshot().await; | ||
| for own_jid in snapshot.lid.iter().chain(snapshot.pn.iter()) { | ||
| let sk_name = | ||
| SenderKeyName::from_parts(group_jid, own_jid.to_protocol_address().as_str()); | ||
| self.signal_cache | ||
| .delete_sender_key(sk_name.cache_key()) | ||
| .await; | ||
| } | ||
| self.flush_signal_cache_logged("rotate_sender_key_on_participant_remove", None) | ||
| .await; | ||
|
|
||
| if let Err(e) = self | ||
| .persistence_manager | ||
| .clear_sender_key_devices(group_jid) | ||
| .await | ||
| { | ||
| log::warn!("rotate_sender_key_on_participant_remove: clear DB failed: {e}"); | ||
| } | ||
| self.sender_key_device_cache.invalidate(group_jid).await; | ||
| } | ||
|
|
||
| /// Take a sent message for retry handling. Checks L1 cache first (if enabled), | ||
| /// then falls back to DB. On miss, tries an alternate PN/LID key to handle | ||
| /// mapping changes between send time and retry time (WAWebLidMigrationUtils | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rotate_sender_key_on_participant_removeonly treats a removed member as having the key when the removed ID exactly matchesjid.userfrom persistedsender_key_devicesrows. Since callers provide a single user form (participant.jid.user), this misses cases where the row is stored under the opposite PN/LID alias (which can happen as mappings are learned/updated). In that caseany_had_keyis false and rotation is skipped, so an actually removed participant who had previously received SKDM can still decrypt subsequent group messages.Useful? React with 👍 / 👎.