Skip to content

refactorings/preparations for introduce asymetrically encrypted keyupdates - #8620

Open
hpk42 wants to merge 3 commits into
mainfrom
hpk/prepare-keyupdates
Open

refactorings/preparations for introduce asymetrically encrypted keyupdates #8620
hpk42 wants to merge 3 commits into
mainfrom
hpk/prepare-keyupdates

Conversation

@hpk42

@hpk42 hpk42 commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

This PR does not change any behaviour for code on main but contains the refactoring parts for introducing asynchronously encrypted key updates in #8621

IT should not be merged without also merging the actual keyupdate branch.

@hpk42 hpk42 changed the title refactorings/preparations for introduce asynchronously encrypted keyupdates refactorings/preparations for introduce asymetrically encrypted keyupdates Aug 22, 2026
@hpk42
hpk42 force-pushed the hpk/prepare-keyupdates branch from a871291 to 7c7c14e Compare August 22, 2026 21:44
Comment thread src/receive_imf.rs
// and nothing else should come out of it: no contact, no chat,
// and no `last_seen` update lighting up an online dot.
info!(context, "Report without message reference (TRASH).");
return trash().await;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

not strictly neccessary but a short-cut to simplify reasoning about keyupdates in the future, to ensure the subsequent receive_imf code never sees this empty MDN.

@link2xt link2xt Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This code is only for users who upgraded, so we still need to check that nothing really bad happens on versions that don't have this code (contacts/chats appearing etc., not just the green dot). Having this code before key updates may hide problems if something other than green dot on the address-contact appears as a result.

EDIT: it is tested with test_keyupdate_against_core_2_48_march_2026 in #8621

@hpk42
hpk42 force-pushed the hpk/prepare-keyupdates branch from 7c7c14e to 715e1cc Compare August 22, 2026 22:02
Comment thread src/pgp.rs Outdated
@Hocuri
Hocuri requested review from Hocuri and link2xt August 26, 2026 07:55
Comment thread src/mimefactory.rs Outdated
Comment thread src/mimefactory.rs

/// Returns the protected headers shared by encrypted messages
/// that are not part of a chat.
async fn non_chat_protected_headers(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

All headers are protected in some way except for Chat-Is-Post-Message, so not sure what protected means here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed the comment.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I guess the function should be renamed too, then? To non_chat_headers or headers_for_non_chat_msg?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I actually meant the function name, it is still _protected_. It's fine, i just don't know why it has _protected_ in the name.

Comment thread src/receive_imf.rs
Ok(mime_parser) => mime_parser,
};

if !mime_parser.mdn_reports.is_empty()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I tried commenting this out (if false &&), the test test_mdn_without_message_reference fails then but only because a contact is created that is not visible.

Without this "address-contact" probably still gets a green dot, not a problem because the user needs to create a visible address-contact chat first to even see this contact.

hpk42 added 3 commits August 27, 2026 14:23
No functional changes:
Add a relay_addrs helper, share the protected headers and self-key rendering
of non-chat messages, and move insert_into_smtp from securejoin to smtp.
…ages

An unsigned message carries no intended recipient fingerprints,
so recipients of an encrypted unsigned message
learn nothing about other recipients from the PGP packets.
A report referencing no message can never be applied to one,
so it must not create a contact, a chat or a `last_seen` update
on its way to the trash.
@hpk42
hpk42 force-pushed the hpk/prepare-keyupdates branch from 715e1cc to 36c0231 Compare August 27, 2026 12:54
Comment thread src/pgp.rs
Comment thread src/pgp.rs
Comment thread src/mimefactory.rs Outdated
}

/// Returns the protected headers shared by encrypted messages
/// that are not part of a chat.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I didn't understand this comment until I looked at what the function is used for

Suggested change
/// that are not part of a chat.
/// Returns the protected headers shared by
/// encrypted messages that are not part of a chat,
/// i.e. key update messages and
/// symmetrically encrypted securejoin messages.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is outdated, comment is already changed: #8620 (comment)

Comment thread src/mimefactory.rs
Comment thread src/pgp/pgp_tests.rs
/// Tests that a message encrypted without a signing key has no signature,
/// and therefore no intended recipient fingerprints naming the other recipients.
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn test_encrypt_unsigned() {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This test is tautological, and doesn't test anything - it tests that if you don't pass any signing key to pk_encrypt, then it doesn't sign the message. And even if pk_encrypt somehow managed to sign the message with some dummy key (and include intended recipients), then the test would still pass, because pk_decrypt_and_validate ignores any signature that does not validate against sig_check_keyring.

I think the test can just be removed. I'm not sure if rpgp exposes a way to check if there is any signature inside a message; if so, then such a check could go into test_send_and_receive_keyupdate.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It is possible to check if the message is a signed message after decrypting or not, e.g. here:

Some(pgp::composed::Message::Signed { reader, .. }) => reader.signature(0),

Decrypting the message should result in pgp::composed::Message::Literal message (because keyupdates are not compressed). But this is something for #8621 or PR on top of it.

@link2xt link2xt Aug 28, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

test_send_and_receive_keyupdate already has this test, there is this code:

    // A keyupdate is trashed on old cores because it's unsigned MDN
    // without a message reference. See also cross-core Python tests.
    let mail = mailparse::parse_mail(keyupdate.payload.as_bytes())?;
    let (mut decrypted, _fingerprint) = decrypt(bob, &mail).await?.unwrap();
    // The next line is important: A key update message must NOT be signed,
    // as the signature might contain intended recipient fingerprints,
    // leaking all of the sender's contacts to all the other contacts.
    assert!(!decrypted.is_signed());

decrypted is a pgp::composed::Message

Comment on lines +274 to +278
/// Tests that an MDN referencing no message is trashed early:
/// there is nothing it could ever be applied to,
/// so it must not create a contact or a chat on the way.
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn test_mdn_without_message_reference() -> Result<()> {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Just from looking at the test and its documentation, it's unclear why we need this test, i.e. why it's important that an MDN referencing no message is trashed early.

I see two options:

  • adapt the test documentation to explain that this is important for key update messages
  • or replace it with a test testing that a key update message from an unknown contact doesn't create a contact or chat.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Just from looking at the test and its documentation, it's unclear why we need this test, i.e. why it's important that an MDN referencing no message is trashed early.

User-visible effect is that "last seen" is not updated for address-contact and address-contact does not get a green dot.
See also #8620 (comment)

Comment thread src/mimefactory.rs

/// Returns the protected headers shared by encrypted messages
/// that are not part of a chat.
async fn non_chat_protected_headers(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I guess the function should be renamed too, then? To non_chat_headers or headers_for_non_chat_msg?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants