deltachat-core-rust
deltachat-core-rust copied to clipboard
deltachat
fix: Create new Peerstate for unencrypted message with already known Autocrypt key, but a new address
See commit messages.
This is aimed to solve the @adbenitez's problem with sharing the same key by several accounts.
One more occurence of #5201: https://github.com/deltachat/deltachat-core-rust/actions/runs/7926082001/job/21640415854?pr=5269
Could you rebase it and test with #5274 merged?
vc-request cannot do AEAP because it is not encrypted. It was only encrypted due to a bug.
Could you rebase it and test with #5274 merged?
vc-request cannot do AEAP because it is not encrypted. It was only encrypted due to a bug.
It still doesn't work. Apparently AEAP doesn't happen, but a Peerstate isn't created for the new address and that should be fixed. So, instead of looking at the SecureJoin message type, we should check if the message is encrypted.
Finally i think that relying on encryption here is not right. vc-request can easily be signed (w/o encryption) and that would be a good reason for doing AEAP. Of course we can state in the SecureJoin doc that vc-request should never be encrypted or signed (as now), but i'd better exculde it explicitly for AEAP.
#5280 again: https://github.com/deltachat/deltachat-core-rust/actions/runs/7999217931/job/21846663121?pr=5269
I still don't understand why vc-request is special. What if it is a vg-request? What if first message coming from a new address uses existing key, will it also fail to create peerstate?
@adbenitez has scanned a QR code and sent vc-request's, but if just a "hello!" text message is sent, will it fail the same way?
I still don't understand why vc-request is special.
I thought it's a good heuristic because vc-request means that it's a new contact, not the existing one changed its address. But not only vc-request means that, you're right. We should check if a message is encrypted. And do AEAP only if the message is encrypted + signed with a verified key. Otherwise a new peerstate must be created, but we must make sure that this doesn't break the possibility of a further AEAP because this may be an attack if the message isn't signed.
What if it is a vg-request?
It won't work the same way.
What if first message coming from a new address uses existing key, will it also fail to create peerstate?
Yes.
@adbenitez has scanned a QR code and sent vc-request's, but if just a "hello!" text message is sent, will it fail the same way?
Almost. The difference is that Alice will answer unencrypted because a new peerstate isn't created. But anyway this is wrong. So, the solution is either do AEAP or create a new peerstate, there is no third option.
With AEAP it is unclear what should happen if we have two contacts with the same key in a verified group and then someone writes to this group with the same key from a third address. Do we remove all contacts and replace with a single one? Currently I think Delta Chat will only AEAP-port one of them arbitrarily selected.
With AEAP it is unclear what should happen if we have two contacts with the same key in a verified group and then someone writes to this group with the same key from a third address. Do we remove all contacts and replace with a single one? Currently I think Delta Chat will only AEAP-port one of them arbitrarily selected.
Yes, but not arbitrarily selected -- the recently appeared one. Anyway this looks strange, yes. But i decided not to change this right now. Probably removing all old contacts should be done, need to think a little of it and fix in a separate PR.
With AEAP it is unclear what should happen if we have two contacts with the same key in a verified group and then someone writes to this group with the same key from a third address. Do we remove all contacts and replace with a single one? Currently I think Delta Chat will only AEAP-port one of them arbitrarily selected.
Yes, but not arbitrarily selected -- the recently appeared one.
Finally i think that in such a case it's better not to port any contact at all. We don't know which one to port. If we port both (i.e. replace them with the new one), then it's a question why they both exist before (probably because the key is shared). Sharing a key isn't a common scenario, if we want AEAP to work in this case too, we better add an additional attribute to the Autocrypt header, smth like setup-id.
So, for now i suggest to avoid doing AEAP if the key is shared.