Skip to content

feat: Remove mostly-unused SignUnencrypted option #8190

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.

Sign up for GitHub

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

Merged
Hocuri merged 4 commits into from
Apr 30, 2026
Merged

feat: Remove mostly-unused SignUnencrypted option #8190

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
36cb7b4
refactor: Remove mostly-unused SignUnencrypted option
Hocuri Apr 26, 2026
813302a
Remove the ability to receive signed-only messages, if the body is in…
Hocuri Apr 28, 2026
d770284
Remove a little bit more code that is not necessary anymore
Hocuri Apr 28, 2026
a4aa11a
Re-add a test
Hocuri Apr 28, 2026
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
4 changes: 0 additions & 4 deletions src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,9 +407,6 @@ pub enum Config {
#[strum(props(default = "1"))]
SyncMsgs,

/// Make all outgoing messages with Autocrypt header "multipart/signed".
SignUnencrypted,

/// Let the core save all events to the database.
/// This value is used internally to remember the MsgId of the logging xdc
#[strum(props(default = "0"))]
Expand Down Expand Up @@ -710,7 +707,6 @@ impl Context {
| Config::Bot
| Config::NotifyAboutWrongPw
| Config::SyncMsgs
| Config::SignUnencrypted
| Config::DisableIdle => {
ensure!(
matches!(value, None | Some("0") | Some("1")),
Expand Down
6 changes: 0 additions & 6 deletions src/context.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -991,12 +991,6 @@ impl Context {
.await?
.to_string(),
);
res.insert(
"sign_unencrypted",
self.get_config_int(Config::SignUnencrypted)
.await?
.to_string(),
);
res.insert(
"debug_logging",
self.get_config_int(Config::DebugLogging).await?.to_string(),
Expand Down
10 changes: 0 additions & 10 deletions src/e2ee.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,16 +79,6 @@ impl EncryptHelper {

Ok(ctext)
}

/// Signs the passed-in `mail` using the private key from `context`.
/// Returns the payload and the signature.
pub async fn sign(self, context: &Context, mail: &MimePart<'static>) -> Result<String> {
let sign_key = load_self_secret_key(context).await?;
let mut buffer = Vec::new();
mail.clone().write_part(&mut buffer)?;
let signature = pgp::pk_calc_signature(buffer, &sign_key)?;
Ok(signature)
}
}

/// Ensures a private key exists for the configured user.
Expand Down
51 changes: 6 additions & 45 deletions src/mimefactory.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1227,53 +1227,18 @@ impl MimeFactory {
message.header(header, value)
});
let message = MimePart::new("multipart/mixed", vec![message]);
let mut message = protected_headers
let message = protected_headers
.iter()
.fold(message, |message, (header, value)| {
message.header(*header, value.clone())
});

if skip_autocrypt || !context.get_config_bool(Config::SignUnencrypted).await? {
// Deduplicate unprotected headers that also are in the protected headers:
let protected: HashSet<&str> =
HashSet::from_iter(protected_headers.iter().map(|(header, _value)| *header));
unprotected_headers.retain(|(header, _value)| !protected.contains(header));
// Deduplicate unprotected headers that also are in the protected headers:
let protected: HashSet<&str> =
HashSet::from_iter(protected_headers.iter().map(|(header, _value)| *header));
unprotected_headers.retain(|(header, _value)| !protected.contains(header));

message
} else {
for (h, v) in &mut message.headers {
if h == "Content-Type"
&& let mail_builder::headers::HeaderType::ContentType(ct) = v
{
let mut ct_new = ct.clone();
ct_new = ct_new.attribute("protected-headers", "v1");
if use_std_header_protection {
ct_new = ct_new.attribute("hp", "clear");
}
*ct = ct_new;
break;
}
}

let signature = encrypt_helper.sign(context, &message).await?;
MimePart::new(
"multipart/signed; protocol=\"application/pgp-signature\"; protected",
vec![
message,
MimePart::new(
"application/pgp-signature; name=\"signature.asc\"",
signature,
)
.header(
"Content-Description",
mail_builder::headers::raw::Raw::<'static>::new(
"OpenPGP digital signature",
),
)
.attachment("signature"),
],
)
}
message
};

let MimeFactory {
Expand Down Expand Up @@ -2192,10 +2157,6 @@ fn group_headers_by_confidentiality(
}
}
} else {
// Copy the header to the protected headers
// in case of signed-only message.
// If the message is not signed, this value will not be used.
protected_headers.push(header.clone());
unprotected_headers.push(header.clone())
}
}
Expand Down
64 changes: 0 additions & 64 deletions src/mimefactory/mimefactory_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -601,70 +601,6 @@ async fn test_selfavatar_unencrypted() -> anyhow::Result<()> {
Ok(())
}

#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn test_selfavatar_unencrypted_signed() {
// create chat with bob, set selfavatar
let t = TestContext::new_alice().await;
t.set_config(Config::SignUnencrypted, Some("1"))
.await
.unwrap();
let chat = t.create_chat_with_contact("bob", "bob@example.org").await;

let file = t.dir.path().join("avatar.png");
let bytes = include_bytes!("../../test-data/image/avatar64x64.png");
tokio::fs::write(&file, bytes).await.unwrap();
t.set_config(Config::Selfavatar, Some(file.to_str().unwrap()))
.await
.unwrap();

// send message to bob: that should get multipart/signed.
// `Subject:` is protected by copying it.
// make sure, `Subject:` stays in the outer header (imf header)
let mut msg = Message::new_text("this is the text!".to_string());

let sent_msg = t.send_msg(chat.id, &mut msg).await;
let mut payload = sent_msg.payload().splitn(4, "\r\n\r\n");

let part = payload.next().unwrap();
assert_eq!(part.match_indices("multipart/signed").count(), 1);
assert_eq!(part.match_indices("From:").count(), 1);
assert_eq!(part.match_indices("Message-ID:").count(), 1);
assert_eq!(part.match_indices("Subject:").count(), 1);
assert_eq!(part.match_indices("Autocrypt:").count(), 1);
assert_eq!(part.match_indices("Chat-User-Avatar:").count(), 0);

let part = payload.next().unwrap();
assert_eq!(
part.match_indices("multipart/mixed; protected-headers=\"v1\"")
.count(),
1
);
assert_eq!(part.match_indices("From:").count(), 1);
assert_eq!(part.match_indices("Message-ID:").count(), 0);
assert_eq!(part.match_indices("Subject:").count(), 1);
assert_eq!(part.match_indices("Autocrypt:").count(), 1);
assert_eq!(part.match_indices("Chat-User-Avatar:").count(), 0);

let part = payload.next().unwrap();
assert_eq!(part.match_indices("text/plain").count(), 1);
assert_eq!(part.match_indices("From:").count(), 0);
assert_eq!(part.match_indices("Message-ID:").count(), 1);
assert_eq!(part.match_indices("Chat-User-Avatar:").count(), 0);
assert_eq!(part.match_indices("Subject:").count(), 0);

let body = payload.next().unwrap();
assert_eq!(body.match_indices("this is the text!").count(), 1);

let bob = TestContext::new_bob().await;
bob.recv_msg(&sent_msg).await;
let alice_id = Contact::lookup_id_by_addr(&bob.ctx, "alice@example.org", Origin::Unknown)
.await
.unwrap()
.unwrap();
let alice_contact = Contact::get_by_id(&bob.ctx, alice_id).await.unwrap();
assert_eq!(alice_contact.is_key_contact(), false);
}

/// Test that removed member address does not go into the `To:` field.
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn test_remove_member_bcc() -> Result<()> {
Expand Down
36 changes: 2 additions & 34 deletions src/mimeparser.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,37 +304,9 @@ impl MimeMessage {

// Parse hidden headers.
let mimetype = mail.ctype.mimetype.parse::<Mime>()?;
let (part, mimetype) =
if mimetype.type_() == mime::MULTIPART && mimetype.subtype().as_str() == "signed" {
if let Some(part) = mail.subparts.first() {
// We don't remove "subject" from `headers` because currently just signed
// messages are shown as unencrypted anyway.

timestamp_sent =
Self::get_timestamp_sent(&part.headers, timestamp_sent, timestamp_rcvd);
MimeMessage::merge_headers(
Copy link
Copy Markdown
Collaborator

@iequidoo iequidoo Apr 28, 2026

Choose a reason for hiding this comment

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

This means that we don't even look at signed-only headers anymore. I'd split this PR into two because SignUnencrypted and support for receiving signed-only messages are separate things. SignUnencrypted can definitely be removed

Copy link
Copy Markdown
Contributor

@r10s r10s Apr 28, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

looking at signed-only is questionable as well. see k-9 blog eg.

from a high-level view, questionable, in general, and in special with our focus on chatmail, and not focusing on pgp specials. not only maintainance and impact, also the continuous discussions these special cases raise all the time are questionable already :)

Copy link
Copy Markdown
Collaborator

@iequidoo iequidoo Apr 28, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

If you're talking about https://k9mail.app/2016/11/24/OpenPGP-Considerations-Part-I and part II as well, i've read already. But everyone has different opinion on this topic, you can ask dkg for example.

We can remove signature checks for signed-only emails (btw, not removed in this PR) because we don't display them in any special way, but not parsing headers in all kinds of messages looks strange.

Copy link
Copy Markdown
Collaborator

@link2xt link2xt Apr 28, 2026

Choose a reason for hiding this comment

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

We should at least make sure we don't break parsing of multipart/signed messages so we don't end up with the mail being received as a file or something like that if we receive such signed-only message.

I'd just keep this code to unwrap the message early, but remove all the comments like "Currently we do not sign unencrypted messages by default.". We are not going to start signing anything with multipart/signed, even thunderbird is now working on switching to https://datatracker.ietf.org/doc/draft-ietf-mailmaint-unobtrusive-signatures/

Copy link
Copy Markdown
Collaborator

@link2xt link2xt Apr 28, 2026

Choose a reason for hiding this comment

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

As for whether to parse protected headers or not, in signed messages they are duplicating the unprotected headers anyway, so it does not matter which one to use, if we don't process signed-only messages differently then can as well use unprotected headers because it is anyway possible to just remove the signature and modify headers. Whatever is simpler to implement, as long as we have some test that incoming multipart/signed messages are displayed normally the same way as unencrypted, the whole code block can be removed as well if mimeparser handles it later.

Copy link
Copy Markdown
Collaborator Author

@Hocuri Hocuri Apr 28, 2026

Choose a reason for hiding this comment

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

Being compatible with every imaginable MUA implementation doing weird stuff is a non-goal. And there are definitely many MUA implementations out there that are not able to process inner headers in a signed mime part, so, if a MUA doesn't duplicate headers into the outer part then they will need to live with incompatibilities.

Whatever is simpler to implement, as long as we have some test that incoming multipart/signed messages are displayed normally the same way as unencrypted, the whole code block can be removed as well if mimeparser handles it later.

I restored part of the tests, renaming it to test_receive_signed_only(). It passes fine without any further adaptions to the code.

Copy link
Copy Markdown
Collaborator Author

@Hocuri Hocuri Apr 28, 2026

Choose a reason for hiding this comment

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

In addition, we already have test_thunderbird_autocrypt_unencrypted(), which tests receiving thunderbird_signed_unencrypted.eml

Copy link
Copy Markdown
Collaborator Author

@Hocuri Hocuri Apr 28, 2026

Choose a reason for hiding this comment

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

We can remove signature checks for signed-only emails (btw, not removed in this PR) because we don't display them in any special way

Where is the code that does these checks? I agree, would be great to be able to remove them, too!

Copy link
Copy Markdown
Collaborator

@link2xt link2xt Apr 28, 2026

Choose a reason for hiding this comment

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

Are you sure all implementations duplicate protected headers in signed-only messages in the outer section?

I think we are the only client that produces RFC 9788 messages. Still waiting for Thunderbird to implement it, and there work is focused on https://datatracker.ietf.org/doc/draft-ietf-mailmaint-unobtrusive-signatures/ (see first bugzilla issue), likely by the time RFC 9788 is implemented multipart/signed signatures will be even less interesting:

Copy link
Copy Markdown
Collaborator

@iequidoo iequidoo Apr 29, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

And there are definitely many MUA implementations out there that are not able to process inner headers in a signed mime part

So we're going to add one more such implementation... EDIT: Ok, after reading a bit https://datatracker.ietf.org/doc/draft-ietf-mailmaint-unobtrusive-signatures/ i agree that support for "multipart/signed" may be removed.

Maybe just parsing headers in all parts recursively (w/o any additional handling of "multipart/signed") is better.

Where is the code that does these checks?

grep validate_detached_signature

context,
&mut headers,
&mut headers_removed,
&mut recipients,
&mut past_members,
&mut from,
&mut list_post,
&mut chat_disposition_notification_to,
part,
);
(part, part.ctype.mimetype.parse::<Mime>()?)
} else {
// Not a valid signed message, handle it as plaintext.
(&mail, mimetype)
}
} else {
// Currently we do not sign unencrypted messages by default.
(&mail, mimetype)
};
if mimetype.type_() == mime::MULTIPART
&& mimetype.subtype().as_str() == "mixed"
&& let Some(part) = part.subparts.first()
&& let Some(part) = mail.subparts.first()
{
for field in &part.headers {
let key = field.get_key().to_lowercase();
Expand All @@ -358,8 +330,7 @@ impl MimeMessage {
);
}

// Remove headers that are allowed _only_ in the encrypted+signed part. It's ok to leave
// them in signed-only emails, but has no value currently.
// Remove headers that are allowed _only_ in the encrypted+signed part
let encrypted = false;
Self::remove_secured_headers(&mut headers, &mut headers_removed, encrypted);

Expand Down Expand Up @@ -2217,9 +2188,6 @@ pub(crate) fn parse_message_id(ids: &str) -> Result<String> {
/// Returns whether the outer header value must be ignored if the message contains a signed (and
/// optionally encrypted) part. This is independent from the modern Header Protection defined in
/// <https://www.rfc-editor.org/rfc/rfc9788.html>.
///
/// NB: There are known cases when Subject and List-ID only appear in the outer headers of
/// signed-only messages. Such messages are shown as unencrypted anyway.
fn is_protected(key: &str) -> bool {
key.starts_with("chat-")
|| matches!(
Expand Down
33 changes: 13 additions & 20 deletions src/mimeparser/mimeparser_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use crate::{
chat,
chatlist::Chatlist,
constants::{self, Blocked, DC_DESIRED_TEXT_LEN, DC_ELLIPSIS},
contact::Contact,
key,
message::{MessageState, MessengerMessage},
receive_imf::receive_imf,
Expand Down Expand Up @@ -2041,32 +2042,24 @@ async fn test_multiple_autocrypt_hdrs() -> Result<()> {
Ok(())
}

/// Tests that timestamp of signed but not encrypted message is protected.
/// Tests receiving a simple signed-unencrypted message
/// that was generated by an old version of Core that supported sending such messages.
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
Copy link
Copy Markdown
Collaborator

@iequidoo iequidoo Apr 28, 2026

Choose a reason for hiding this comment

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

Not sure we should just remove this test, this means that we don't test that signed "Date" is taken from signed-only messages sent by other MUAs anymore. I'd like to have such a message in test-data/ then and use it here. Probably we already have such a test message, then need to change the outer Date in it and add a check to the corresponding test

Copy link
Copy Markdown
Collaborator Author

@Hocuri Hocuri Apr 28, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

If an attacker wants to spoof the date, they can just remove the signature, because we don't process signed-only messages differently than unsigned messages.

But I will restore the rest of the test, putting the message in test-data, in order to have a test that receiving signed-only messages works at all.

async fn test_protected_date() -> Result<()> {
async fn test_receive_signed_only() -> Result<()> {
let mut tcm = TestContextManager::new();
let alice = &tcm.alice().await;
let bob = &tcm.bob().await;

alice.set_config(Config::SignUnencrypted, Some("1")).await?;
let imf_raw = include_bytes!("../../test-data/message/unencrypted_signed_simple.eml");
let msg = receive_imf(bob, imf_raw, false).await?.unwrap();
assert_eq!(msg.msg_ids.len(), 1);
let msg = Message::load_from_db(bob, msg.msg_ids[0]).await?;
assert_eq!(msg.get_text(), "Hello!");
assert_eq!(msg.viewtype, Viewtype::Text);
assert_eq!(msg.get_timestamp(), 1615987853);

let alice_chat = alice.create_email_chat(bob).await;
let alice_msg_id = chat::send_text_msg(alice, alice_chat.id, "Hello!".to_string()).await?;
let alice_msg = Message::load_from_db(alice, alice_msg_id).await?;
assert_eq!(alice_msg.get_showpadlock(), false);

let mut sent_msg = alice.pop_sent_msg().await;
sent_msg.payload = sent_msg.payload.replacen(
"Date:",
"Date: Wed, 17 Mar 2021 14:30:53 +0100 (CET)\r\nX-Not-Date:",
1,
);
let bob_msg = bob.recv_msg(&sent_msg).await;
assert_eq!(alice_msg.get_text(), bob_msg.get_text());
let alice_contact = Contact::get_by_id(bob, msg.from_id).await.unwrap();
assert_eq!(alice_contact.is_key_contact(), false);

// Timestamp that the sender has put into the message
// should always be displayed as is on the receiver.
assert_eq!(alice_msg.get_timestamp(), bob_msg.get_timestamp());
Ok(())
}

Expand Down
49 changes: 4 additions & 45 deletions src/pgp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,15 @@ use std::io::Cursor;
use anyhow::{Context as _, Result, ensure};
use deltachat_contact_tools::{EmailAddress, may_be_valid_addr};
use pgp::composed::{
ArmorOptions, Deserializable, DetachedSignature, EncryptionCaps, KeyType as PgpKeyType,
MessageBuilder, SecretKeyParamsBuilder, SignedKeyDetails, SignedPublicKey, SignedPublicSubKey,
SignedSecretKey, SubkeyParamsBuilder, SubpacketConfig,
Deserializable, DetachedSignature, EncryptionCaps, KeyType as PgpKeyType, MessageBuilder,
SecretKeyParamsBuilder, SignedKeyDetails, SignedPublicKey, SignedPublicSubKey, SignedSecretKey,
SubkeyParamsBuilder, SubpacketConfig,
};
use pgp::crypto::aead::{AeadAlgorithm, ChunkSize};
use pgp::crypto::ecc_curve::ECCCurve;
use pgp::crypto::hash::HashAlgorithm;
use pgp::crypto::sym::SymmetricKeyAlgorithm;
use pgp::packet::{Signature, SignatureConfig, SignatureType, Subpacket, SubpacketData};
use pgp::packet::{Signature, Subpacket, SubpacketData};
use pgp::types::{
CompressionAlgorithm, Imprint, KeyDetails, KeyVersion, Password, SignedUser, SigningKey as _,
StringToKey,
Expand Down Expand Up @@ -202,47 +202,6 @@ pub async fn pk_encrypt(
.await?
}

/// Produces a detached signature for `plain` text using `private_key_for_signing`.
pub fn pk_calc_signature(
plain: Vec<u8>,
private_key_for_signing: &SignedSecretKey,
) -> Result<String> {
let rng = thread_rng();

let mut config = SignatureConfig::from_key(
rng,
&private_key_for_signing.primary_key,
SignatureType::Binary,
)?;

config.hashed_subpackets = vec![
Subpacket::regular(SubpacketData::IssuerFingerprint(
private_key_for_signing.fingerprint(),
))?,
Subpacket::critical(SubpacketData::SignatureCreationTime(
pgp::types::Timestamp::now(),
))?,
];
config.unhashed_subpackets = vec![];
if private_key_for_signing.version() <= KeyVersion::V4 {
config
.unhashed_subpackets
.push(Subpacket::regular(SubpacketData::IssuerKeyId(
private_key_for_signing.legacy_key_id(),
))?);
}

let signature = config.sign(
&private_key_for_signing.primary_key,
&Password::empty(),
plain.as_slice(),
)?;

let sig = DetachedSignature::new(signature);

Ok(sig.to_armored_string(ArmorOptions::default())?)
}

/// Returns fingerprints
/// of all keys from the `public_keys_for_validation` keyring that
/// have valid signatures in `msg` and corresponding intended recipient fingerprints
Expand Down
Loading