feat: Remove mostly-unused SignUnencrypted option #8190
Conversation
… a "signed" subpart We have a test for receiving `thunderbird_signed_unencrypted.eml`, so apparently, some signed-only messages can still be received
|
nice! |
|
|
||
| timestamp_sent = | ||
| Self::get_timestamp_sent(&part.headers, timestamp_sent, timestamp_rcvd); | ||
| MimeMessage::merge_headers( |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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/
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
In addition, we already have test_thunderbird_autocrypt_unencrypted(), which tests receiving thunderbird_signed_unencrypted.eml
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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
| } | ||
|
|
||
| /// Tests that timestamp of signed but not encrypted message is protected. | ||
| #[tokio::test(flavor = "multi_thread", worker_threads = 2)] |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
iequidoo
left a comment
iequidoo
left a comment
There was a problem hiding this comment.
Lgtm, but the validate_detached_signature() call should be removed as well probably
4 participants
No description provided.