Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
77 commits
Select commit Hold shift + click to select a range
9694ba3
[WIP] QR codes and symmetric encryption for broadcast channels
Hocuri Oct 2, 2025
10091a7
Merge remote-tracking branch 'origin/main' into hoc/channels-encrypti…
Hocuri Oct 2, 2025
62d7441
test: Fix test_broadcast_multidev
Hocuri Oct 2, 2025
a434e6e
Fix some things, so that all tests pass sometimes. Some tests are sti…
Hocuri Oct 6, 2025
7f3d0cc
Merge remote-tracking branch 'origin/main' into hoc/channels-encrypti…
Hocuri Oct 6, 2025
17fdac2
clippy
Hocuri Oct 6, 2025
8b3b597
test: Fix the flaky tests. All tests pass now!
Hocuri Oct 6, 2025
915bad9
refactor: Remove unused MarkVerified sync action
Hocuri Oct 7, 2025
d755f61
improve comment
Hocuri Oct 7, 2025
e001067
Some things I noticed while self-reviewing
Hocuri Oct 7, 2025
c602586
Small comment improvements
Hocuri Oct 7, 2025
7d92fcd
fix: Don't invent a secret when receiving an outgoing broadcast message
Hocuri Oct 7, 2025
faa149a
small comment improvement
Hocuri Oct 8, 2025
5a91887
more comment improvements
Hocuri Oct 8, 2025
41b4953
more tweaks
Hocuri Oct 8, 2025
6114a6a
Merge remote-tracking branch 'origin/main' into hoc/channels-encrypti…
Hocuri Oct 8, 2025
27d1d35
small tweaks
Hocuri Oct 8, 2025
89f1e52
clippy
Hocuri Oct 8, 2025
7fcbf39
Simon's review
Hocuri Oct 15, 2025
e00db35
Merge branch 'main' into hoc/channels-encryption-3
Hocuri Oct 15, 2025
a3a932a
clippy
Hocuri Oct 15, 2025
7bd1ced
Update src/securejoin/bob.rs
Hocuri Oct 20, 2025
07a6062
Merge remote-tracking branch 'origin/main' into hoc/channels-encrypti…
Hocuri Oct 20, 2025
e6bb254
Update golden tests
Hocuri Oct 20, 2025
24cbcbe
fix: Don't add duplicate member-added message
Hocuri Oct 21, 2025
691887f
iequidoo's review
Hocuri Oct 21, 2025
e74f312
clippy
Hocuri Oct 21, 2025
fcc3363
Merge remote-tracking branch 'origin/main' into hoc/channels-encrypti…
Hocuri Oct 21, 2025
f336add
iequidoo's review
Hocuri Oct 23, 2025
24d5335
Fix compilation error
Hocuri Oct 23, 2025
a836775
Merge remote-tracking branch 'origin/main' into hoc/channels-encrypti…
Hocuri Oct 23, 2025
8a4044e
Another compilation error
Hocuri Oct 23, 2025
1d671d2
Fix test
Hocuri Oct 23, 2025
cafc2c7
Merge remote-tracking branch 'origin/main' into hoc/channels-encrypti…
Hocuri Oct 23, 2025
efc323f
fix: Show an error message when trying to send into an old channel
Hocuri Oct 23, 2025
ab1844d
Merge remote-tracking branch 'origin/main' into hoc/channels-encrypti…
Hocuri Oct 24, 2025
9776cd7
refactor: rename broadcast_shared_secret -> broadcast_secret
Hocuri Oct 25, 2025
745d38c
refactor: inline handle_sync_create_chat()
Hocuri Oct 25, 2025
1029707
Bail if added/removed member is missing
Hocuri Oct 25, 2025
205ca0f
refactor: Rename src/internals_for_benchmarks.rs -> src/internals_for…
Hocuri Oct 26, 2025
4d615d6
comments
Hocuri Oct 26, 2025
9f87a53
Make some code shorter
Hocuri Oct 26, 2025
c101f2a
Small changes from iequidoo's review
Hocuri Oct 26, 2025
93b8763
more of iequidoo's suggestions
Hocuri Oct 26, 2025
ea82fe9
Rename encryption_keys -> encryption_pubkeys
Hocuri Oct 26, 2025
0e0c534
clippy
Hocuri Oct 26, 2025
9e9ed67
Merge remote-tracking branch 'origin/main' into hoc/channels-encrypti…
Hocuri Oct 26, 2025
501b5a7
remaining review comments
Hocuri Oct 26, 2025
9e0ff87
more review
Hocuri Oct 28, 2025
717d6bb
review, again
Hocuri Oct 28, 2025
0a69a55
Make all rust tests pass again
Hocuri Oct 28, 2025
7fdee26
clippy
Hocuri Oct 28, 2025
1a85ad7
Update src/test_utils.rs
Hocuri Oct 28, 2025
05cacc0
small test improvements
Hocuri Oct 28, 2025
9048977
feat: Add and use stock string "You joined the channel"
Hocuri Oct 28, 2025
bd12139
Merge remote-tracking branch 'origin/main' into hoc/channels-encrypti…
Hocuri Oct 28, 2025
c3be22f
Update src/mimefactory.rs
Hocuri Oct 28, 2025
bc116bd
fix compilation
Hocuri Oct 28, 2025
d9e38c6
Try to fix Python tests
Hocuri Oct 28, 2025
c71f698
fix: Delete broadcast secret when leaving or being removed from channel
Hocuri Oct 28, 2025
dd3cffd
don't prematurely optimize SQL statement
Hocuri Oct 28, 2025
7be7ba0
feat: If someone who is not the admin tries to send into brodacast, a…
Hocuri Oct 29, 2025
1d3ae6d
small fixes
Hocuri Oct 29, 2025
327e0cc
test: Fix test flakiness
Hocuri Oct 29, 2025
af20e3e
fix: Delete request-with-auth messages immediately; some tests still …
Hocuri Oct 29, 2025
ca986cc
Update src/receive_imf.rs
Hocuri Oct 31, 2025
644d6e5
fix: stop using `leftgrps` table
link2xt Oct 28, 2025
1bbd3ec
feat(backwards-compat): For now, send Chat-Verified header (instead o…
Hocuri Oct 29, 2025
3723cd1
fix remaining test failures
Hocuri Oct 31, 2025
c1b53b1
Merge remote-tracking branch 'origin/main' into hoc/channels-encrypti…
Hocuri Oct 31, 2025
c0aa31c
fix: Set chat_created = true when creating outgoing broadcast
Hocuri Oct 31, 2025
2409431
Merge remote-tracking branch 'origin/main' into hoc/channels-encrypti…
Hocuri Nov 1, 2025
28d1ca9
fix: Use Chat-List-Id rather than List-Id header
Hocuri Nov 1, 2025
36a8f20
fix: Make sure broadcast members can't see each other
Hocuri Nov 3, 2025
8b300b8
fix: Use j= instead of i= in broadcast invites, so that old devices d…
Hocuri Nov 3, 2025
788765f
Fix compilation error
Hocuri Nov 3, 2025
47b8a2d
iequidoo's review
Hocuri Nov 3, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions benches/decrypting.rs
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ fn criterion_benchmark(c: &mut Criterion) {
vec![black_box(key_pair.public.clone())],
Some(key_pair.secret.clone()),
true,
true,
)
.await
.unwrap()
Expand Down
2 changes: 2 additions & 0 deletions deltachat-rpc-client/tests/test_securejoin.py
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,8 @@ def check_account(ac, contact, inviter_side, please_wait_info_msg=False):

check_account(fiona, fiona.create_contact(alice), inviter_side=False, please_wait_info_msg=True)

# For Bob, the channel must not have changed:
check_account(bob, bob.create_contact(alice), inviter_side=False, please_wait_info_msg=True)

def test_qr_securejoin_contact_request(acfactory) -> None:
"""Alice invites Bob to a group when Bob's chat with Alice is in a contact request mode."""
Expand Down
120 changes: 116 additions & 4 deletions src/chat/chat_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2627,6 +2627,121 @@ async fn test_can_send_group() -> Result<()> {
Ok(())
}

/// Tests that in a broadcast channel,
/// the recipients can't see the identity of their fellow recipients.
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn test_broadcast_members_cant_see_each_other() -> Result<()> {
fn contains(parsed: &MimeMessage, s: &str) -> bool {
assert_eq!(parsed.decrypting_failed, false);
let decoded_str = str::from_utf8(&parsed.decoded_data).unwrap();
decoded_str.contains(s)
}

let mut tcm = TestContextManager::new();
let alice = &tcm.alice().await;
let bob = &tcm.bob().await;
let charlie = &tcm.charlie().await;

tcm.section("Alice creates a channel, Bob joins.");
let alice_broadcast_id = create_broadcast(alice, "Channel".to_string()).await?;
let qr = get_securejoin_qr(alice, Some(alice_broadcast_id))
.await
.unwrap();
tcm.exec_securejoin_qr(bob, alice, &qr).await;

tcm.section("Charlie scans the QR code and sends request.");
{
join_securejoin(charlie, &qr).await.unwrap();

let request = charlie.pop_sent_msg().await;
assert_eq!(request.recipients, "[email protected] [email protected]");

alice.recv_msg_trash(&request).await;
}

tcm.section("Alice sends auth-required");
{
let auth_required = alice.pop_sent_msg().await;
assert_eq!(
auth_required.recipients,
"[email protected] [email protected]"
);
let parsed = charlie.parse_msg(&auth_required).await;
assert!(parsed.header_exists(HeaderDef::AutocryptGossip));
assert!(contains(&parsed, "[email protected]"));
assert_eq!(contains(&parsed, "[email protected]"), false);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assert!(contains(&parsed, "[email protected]") == false); reads better to me (and is better aligned with the assert-s above)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of these is great, and I do miss pytest in these moments, but sure, I can replace it

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, no, clippy will complain:

warning: equality checks against false can be replaced by a negation
    --> src/chat/chat_tests.rs:2672:17
     |
2672 |         assert!(contains(&parsed, "[email protected]") == false);
     |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try simplifying it as shown: `!contains(&parsed, "[email protected]")`
     |
     = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#bool_comparison

...and signifying just by a ! which asserts are negated seems pretty unreadable to me.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since in most other tests, we do the assert_eq!(..., false) thing, I'll go with this for now

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means that later clippy will maybe complain about assert_eq!(..., false) and recommend to replace it with assert!(!...). Probably it's better to define some not() function once, and it will be more readable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can deal with this problem if and when it arrives.


let parsed_by_bob = bob.parse_msg(&auth_required).await;
assert!(parsed_by_bob.decrypting_failed);

charlie.recv_msg_trash(&auth_required).await;
}

tcm.section("Charlie sends request-with-auth");
{
let request_with_auth = charlie.pop_sent_msg().await;
assert_eq!(
request_with_auth.recipients,
"[email protected] [email protected]"
);

alice.recv_msg_trash(&request_with_auth).await;
}

tcm.section("Alice adds member");
{
let member_added = alice.pop_sent_msg().await;
assert_eq!(
member_added.recipients,
"[email protected] [email protected]"
);
let parsed = charlie.parse_msg(&member_added).await;
assert!(parsed.header_exists(HeaderDef::AutocryptGossip));
assert!(contains(&parsed, "[email protected]"));
assert_eq!(contains(&parsed, "[email protected]"), false);

let parsed_by_bob = bob.parse_msg(&member_added).await;
assert!(parsed_by_bob.decrypting_failed);

let rcvd = charlie.recv_msg(&member_added).await;
assert_eq!(rcvd.param.get_cmd(), SystemMessage::MemberAddedToGroup);
}

tcm.section("Alice sends into the channel.");
{
let hi_msg = alice.send_text(alice_broadcast_id, "hi").await;
let parsed = charlie.parse_msg(&hi_msg).await;
assert_eq!(parsed.header_exists(HeaderDef::AutocryptGossip), false);
assert_eq!(contains(&parsed, "[email protected]"), false);
assert_eq!(contains(&parsed, "[email protected]"), false);

let parsed_by_bob = bob.parse_msg(&hi_msg).await;
assert_eq!(parsed_by_bob.decrypting_failed, false);
}

tcm.section("Alice removes Charlie. Bob must not see it.");
{
let alice_charlie_contact = alice.add_or_lookup_contact_id(charlie).await;
remove_contact_from_chat(alice, alice_broadcast_id, alice_charlie_contact).await?;
let member_removed = alice.pop_sent_msg().await;
assert_eq!(
member_removed.recipients,
"[email protected] [email protected]"
);
let parsed = charlie.parse_msg(&member_removed).await;
assert!(contains(&parsed, "[email protected]"));
assert_eq!(contains(&parsed, "[email protected]"), false);

let parsed_by_bob = bob.parse_msg(&member_removed).await;
assert!(parsed_by_bob.decrypting_failed);

let rcvd = charlie.recv_msg(&member_removed).await;
assert_eq!(rcvd.param.get_cmd(), SystemMessage::MemberRemovedFromGroup);
}

Ok(())
}

#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn test_broadcast_change_name() -> Result<()> {
let mut tcm = TestContextManager::new();
Expand Down Expand Up @@ -3174,10 +3289,7 @@ async fn test_leave_broadcast_multidevice() -> Result<()> {

alice.recv_msg_trash(&request_with_auth).await;
let member_added = alice.pop_sent_msg().await;
assert_eq!(
request_with_auth.recipients,
"[email protected] [email protected]"
);
assert_eq!(member_added.recipients, "[email protected] [email protected]");

tcm.section("Bob receives the member-added message answer, and processes it");
let rcvd = bob0.recv_msg(&member_added).await;
Expand Down
50 changes: 29 additions & 21 deletions src/mimefactory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
use std::collections::{BTreeSet, HashSet};
use std::io::Cursor;

use anyhow::{Context as _, Result, bail};
use anyhow::{Context as _, Result, bail, format_err};
use base64::Engine as _;
use data_encoding::BASE32_NOPAD;
use deltachat_contact_tools::sanitize_bidi_characters;
Expand Down Expand Up @@ -290,6 +290,14 @@ impl MimeFactory {
for row in rows {
let (authname, addr, fingerprint, id, add_timestamp, remove_timestamp, public_key_bytes_opt) = row?;

// In a broadcast channel, only send member-added/removed messages
// to the affected member:
if let Some(fp) = should_only_send_to_one_recipient(&msg, &chat){
if fp? != fingerprint {
continue;
}
}

let public_key_opt = if let Some(public_key_bytes) = &public_key_bytes_opt {
Some(SignedPublicKey::from_slice(public_key_bytes)?)
} else {
Expand Down Expand Up @@ -415,20 +423,6 @@ impl MimeFactory {
req_mdn = true;
}

// If undisclosed_recipients, and this is a member-added/removed message,
// only send to the added/removed member
if undisclosed_recipients
&& matches!(
msg.param.get_cmd(),
SystemMessage::MemberRemovedFromGroup | SystemMessage::MemberAddedToGroup
)
{
let Some(member) = msg.param.get(Param::Arg) else {
bail!("Missing removed/added member");
};
recipients.retain(|addr| addr == member);
}

encryption_pubkeys = if !is_encrypted {
None
} else if should_encrypt_symmetrically(&msg, &chat) {
Expand Down Expand Up @@ -1975,12 +1969,7 @@ fn hidden_recipients() -> Address<'static> {
}

fn should_encrypt_with_broadcast_secret(msg: &Message, chat: &Chat) -> bool {
chat.typ == Chattype::OutBroadcast
// We encrypt securejoin messages asymmetrically
&& msg.param.get_cmd() != SystemMessage::SecurejoinMessage
// The member-added message in a broadcast must be asymmetrically encrypted,
// because the newly-added member doesn't know the broadcast shared secret yet:
&& msg.param.get_cmd() != SystemMessage::MemberAddedToGroup
chat.typ == Chattype::OutBroadcast && should_only_send_to_one_recipient(msg, chat).is_none()
}

fn should_hide_recipients(msg: &Message, chat: &Chat) -> bool {
Expand All @@ -1991,6 +1980,25 @@ fn should_encrypt_symmetrically(msg: &Message, chat: &Chat) -> bool {
should_encrypt_with_broadcast_secret(msg, chat)
}

/// Some messages sent into outgoing broadcast channels (member-added/member-removed)
/// should only go to a single recipient,
/// rather than all recipients.
/// This function returns the fingerprint of the recipient the message should be sent to.
fn should_only_send_to_one_recipient<'a>(msg: &'a Message, chat: &Chat) -> Option<Result<&'a str>> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

must_have_only_one_recipient() is shorter (and i'd say that "should" is rather a recommendation). And why not Result<Option<...>> as usually, does it cause compilation issues?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With Result<Option<..>>, the 3 functions that use this function would also need to be resultified.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's still better to return Result<Option<...>> everywhere for consistency and if some function doesn't want to be resultified, you can use Result::transpose()

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either way to write it is fine - Result<Option<...>> is a bit more consistent, while some readers won't know Result::transpose(), so, each has its pros and cons.

if chat.typ == Chattype::OutBroadcast
&& matches!(
msg.param.get_cmd(),
SystemMessage::MemberRemovedFromGroup | SystemMessage::MemberAddedToGroup
)
{
let Some(fp) = msg.param.get(Param::Arg4) else {
return Some(Err(format_err!("Missing removed/added member")));
};
return Some(Ok(fp));
}
None
}

async fn build_body_file(context: &Context, msg: &Message) -> Result<MimePart<'static>> {
let file_name = msg.get_filename().context("msg has no file")?;
let blob = msg
Expand Down
Loading