Skip to content
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.

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

(POSTPONED) Synchronize "verification" events between devices and simplify securejoin.rs #4914

Closed
4 tasks
hpk42 opened this issue Nov 2, 2023 · 7 comments
Closed
4 tasks

Comments

@hpk42
Copy link
Contributor

hpk42 commented Nov 2, 2023

Both for setup-contact and group join protocol we want to synchronize "verification" states in order to simplify securejoin code and reduce the network protocol. First we introduce two new sync messages for Alice and bob respectively:

  • when Alice determines that Bob is verified, send a sync message to Alice's other devices about it
  • when Bob determines Alice is verified, send a sync message to Bob's other devices about it

Once verifications are explicitely synced we can:

Note: maybe Alice does not need to send a sync message on verified group-join because alice's "vg-member-added" message is observable on her other devices anyway. But it probably makes sense to not try too hard to avoid verification-sync here, and rather have the "verification" sync independently from vg-member-added. In the case of setup-contact the verifications need to be synced because the other device doesn't have an observable message like the in vg-member-added group-join case.

@link2xt
Copy link
Collaborator

link2xt commented Nov 2, 2023

We can send a sync message with a key fingerprint and contact identifier (email address) every time we directly verify someone (there is no third-party verifier). But we can also keep it in the form of self-sent contact-confirmed (every time, even for group join) instead of JSON sync item if this allows to keep compatibility and then reduce observe_securejoin to handling this message and even inline it..

@iequidoo
Copy link
Collaborator

iequidoo commented Nov 4, 2023

Moving my reasoning to the right place:

... afaiu there are 3 messages to be removed from the protocol:

But sync messages still can simplify the code because observe_securejoin_on_other_device() can be removed, but i think not much and in favor of some new code, so i'm unsure we should implement them. The mentioned PR also removes 100+ LOCs. But if i'm wrong and sync messages actually can simplify the protocol itself, that's another deal

UPD: Sync messages can simplify the code by that the necessary checks are already done by another device, so we can skip the checks and just apply the new state. On the other hand, it's not bad to have all the checks done on all devices, there can be different client versions and different configs, and probably the proper refactoring can simplify the code as well. All in all i think that sync messages should be avoided if they don't bring new information (which means they can't simplify the network protocol).

@hpk42 hpk42 changed the title Synchronize "verification" events between devices and simplify securejoin.rs (POSTPONED) Synchronize "verification" events between devices and simplify securejoin.rs Nov 9, 2023
@link2xt
Copy link
Collaborator

link2xt commented Nov 17, 2023

I am currently looking at observe_securejoin_on_other_device.

PR #3982 (fixing issue #3836) was the latest change to observe_securejoin_on_other_device. It added handling of vg-request-with-auth and vc-request-with-auth so Alice is already marked as verified because sending such message means that Alices fingerprint matched the key from *-auth-required.

The most interesting part of 39601be security-wise is not the added if, but the addition of *-request-with-auth to the existing condition.

@iequidoo
Copy link
Collaborator

iequidoo commented Nov 17, 2023

... It added handling of vg-request-with-auth and vc-request-with-auth so Alice is already marked as verified because sending such message means that Alices fingerprint matched the key from *-auth-required.

Yes, and i'd not replace this logic with sync messages, they would only complicate things (two different messages would need to be sent instead of one). EDIT: Some redundant and apparently wrongly understood text was here.

@hpk42
Copy link
Contributor Author

hpk42 commented Nov 18, 2023 via email

@link2xt
Copy link
Collaborator

link2xt commented Nov 28, 2023

why does Alice need to know Bob has verified her?

It is the other way round, Bob needs to know that Alice has verified Bob. Alice verifies Bob when vc-request-with-auth is processed, but Bob will only learn that Alice verified Bob when vc-contact-confirm is received.

@link2xt
Copy link
Collaborator

link2xt commented Dec 18, 2023

Closing in favor of #5089
I have reviewed observe_securejoin_on_other_device logic there. It is simple enough and compatible to existing deployed core, unlike sync messages.

@link2xt link2xt closed this as not planned Won't fix, can't repro, duplicate, stale Dec 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants