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

Allow multiple BobStates #2777

Closed
r10s opened this issue Oct 29, 2021 · 12 comments
Closed

Allow multiple BobStates #2777

r10s opened this issue Oct 29, 2021 · 12 comments
Labels
enhancement New feature or request verified-chats

Comments

@r10s
Copy link
Contributor

r10s commented Oct 29, 2021

BobState is currently kept in memory only and there is only one BobState. this has the following disadvantages:

  1. if the app is terminated before Alice answers, the secure join will get stale. eg. iOS suspends app in the background already after 30 seconds, usually the memory is restored on coming to foreground again, but that depends: apps may also be terminated already after 30 seconds.
    android may have similar situations, but also a simple user-exit may terminate a handshake unexpectedly, esp. it that runs for several minutes.
    EDIT this is fixed by #2920

  2. since instant success on simple qr-setup-contact #1215 and non-blocking group QR joins #2508 all handshakes run in background and dc_join_securejoin() always returns immediately with the chat-id.
    however, if dc_join_securejoin() is called again before the old one has finished, the old one is aborted (which is already better than showing an error :)

the idea is to target both issues by handling BobState completely in the database, eg. in a separate table, one row per state.

@r10s r10s added the enhancement New feature or request label Oct 29, 2021
@flub
Copy link
Contributor

flub commented Nov 1, 2021

https://github.com/deltachat/deltachat-core-rust/blob/5e26b5bfdce8080512bab26cb95703f20b45e872/src/securejoin.rs#L283

This... is no longer true? Is this still the right way to notify the user?

@r10s
Copy link
Contributor Author

r10s commented Nov 1, 2021

This... is no longer true? Is this still the right way to notify the user?

the comment is wrong, yes. join_securejoin() function is now not much different from creating a chat and works even when both devices are offline.

the end-user is now informed by info-messages added to the corresponding chat, so, from view of the ui, there is nothing more to do anymore.

@flub
Copy link
Contributor

flub commented Nov 1, 2021

This... is no longer true? Is this still the right way to notify the user?

the comment is wrong, yes. join_securejoin() function is now not much different from creating a chat and works even when both devices are offline.

the end-user is now informed by info-messages added to the corresponding chat, so, from view of the ui, there is nothing more to do anymore.

Ok, I think the error!() call below that line needs to be fixed in that case.

@flub
Copy link
Contributor

flub commented Nov 1, 2021

Do we want to allow alice to invite bob with multiple QR codes in parallel, e.g. "here join this and this groups". Or do we only want to allow bob to join multiple alices? From a user point of view the latter is probably a weird case that's different to understand.

This matters because if we allow multiple joining processes at the same time we have to know which bit of state to load from the db. The securejoin protocol took care of this on the alice side with an invite code but the bob side doesn't really do this. We can probably amend the protocol a little, e.g. bob sends an additional header which alice needs to send back, but we'd need to think through the security implications.

@r10s
Copy link
Contributor Author

r10s commented Nov 1, 2021

Do we want to allow alice to invite bob with multiple QR codes in parallel

i think, in the long run, it would be nice if Bob can just scan multiple qr-codes, no matter if they come from the same or from different Alices. usecase could be eg. a gathering prepared by Alice - "scan here and here and here to join the groups 'kitchen', 'chore' and 'fun'" :)

but if the "same Alice" turns out to be complicated and needs adaptions in the protocol, maybe just start with the simpler parts - persistence and allow Bob to scan different Alices. scanning the same Alice again, will abort pervious handshakes with that Alice (as now with all handshakes).

maybe that's already "good enough". in any case it would already be a huge improvement over the current state.

EDIT: and we can iterate from there - maybe we could also serialize different handshakes with the same Alice. that would save some messages as subsequent joins will take the shortcut in the protocol.

@flub
Copy link
Contributor

flub commented Nov 20, 2021

now that the group chat is created up front, does it make sense to re-direct (some) info messages about the progress and/or failure of joining a group to this chat instead of the 1:1 chat of the inviter? In any case I'll do this separately, but makes sense to think about already.

@r10s
Copy link
Contributor Author

r10s commented Nov 20, 2021

now that the group chat is created up front, does it make sense to re-direct (some) info messages about the progress and/or failure of joining a group to this chat instead of the 1:1 chat of the inviter?

for Bob, this already happens. for group-joins all info messages go to the group-chat (info messages are added to BobState::chat_id() - that is the group-chat for secure-join or the one-to-one-chat for setup-contact

to reduce noise and to focus on the gist of the scan - joining a group -, the one-to-one chat is not even shown to the user (however, the contact is added and verified, of course) (technically, the one-to-one chat is there, as it is needed for the messages currently, however, unless it was present before, it is not added to the chatlist the user sees)

flub added a commit that referenced this issue Dec 24, 2021
The state bob needs to maintain during a secure-join process when
exchanging messages used to be stored on the context.  This means if
the process was killed this state was lost and the securejoin process
would fail.  Moving this state into the database should help this.

This still only allows a single securejoin process at a time, this may
be relaxed in the future.  For now any previous securejoin process
that was running is killed if a new one is started (this was already
the case).

This can remove some of the complexity around BobState handling: since
the state is in the database we can already make state interactions
transactional and correct.  We no longer need the mutex around the
state handling.  This means the BobStateHandle construct that was
handling the interactions between always having a valid state and
handling the mutex is no longer needed, resulting in some nice
simplifications.

Part of #2777.
@flub flub self-assigned this Dec 28, 2021
flub added a commit that referenced this issue Dec 28, 2021
The state bob needs to maintain during a secure-join process when
exchanging messages used to be stored on the context.  This means if
the process was killed this state was lost and the securejoin process
would fail.  Moving this state into the database should help this.

This still only allows a single securejoin process at a time, this may
be relaxed in the future.  For now any previous securejoin process
that was running is killed if a new one is started (this was already
the case).

This can remove some of the complexity around BobState handling: since
the state is in the database we can already make state interactions
transactional and correct.  We no longer need the mutex around the
state handling.  This means the BobStateHandle construct that was
handling the interactions between always having a valid state and
handling the mutex is no longer needed, resulting in some nice
simplifications.

Part of #2777.
flub added a commit that referenced this issue Jan 16, 2022
The state bob needs to maintain during a secure-join process when
exchanging messages used to be stored on the context.  This means if
the process was killed this state was lost and the securejoin process
would fail.  Moving this state into the database should help this.

This still only allows a single securejoin process at a time, this may
be relaxed in the future.  For now any previous securejoin process
that was running is killed if a new one is started (this was already
the case).

This can remove some of the complexity around BobState handling: since
the state is in the database we can already make state interactions
transactional and correct.  We no longer need the mutex around the
state handling.  This means the BobStateHandle construct that was
handling the interactions between always having a valid state and
handling the mutex is no longer needed, resulting in some nice
simplifications.

Part of #2777.
flub added a commit that referenced this issue Jan 16, 2022
The state bob needs to maintain during a secure-join process when
exchanging messages used to be stored on the context.  This means if
the process was killed this state was lost and the securejoin process
would fail.  Moving this state into the database should help this.

This still only allows a single securejoin process at a time, this may
be relaxed in the future.  For now any previous securejoin process
that was running is killed if a new one is started (this was already
the case).

This can remove some of the complexity around BobState handling: since
the state is in the database we can already make state interactions
transactional and correct.  We no longer need the mutex around the
state handling.  This means the BobStateHandle construct that was
handling the interactions between always having a valid state and
handling the mutex is no longer needed, resulting in some nice
simplifications.

Part of #2777.
flub added a commit that referenced this issue Mar 1, 2022
The state bob needs to maintain during a secure-join process when
exchanging messages used to be stored on the context.  This means if
the process was killed this state was lost and the securejoin process
would fail.  Moving this state into the database should help this.

This still only allows a single securejoin process at a time, this may
be relaxed in the future.  For now any previous securejoin process
that was running is killed if a new one is started (this was already
the case).

This can remove some of the complexity around BobState handling: since
the state is in the database we can already make state interactions
transactional and correct.  We no longer need the mutex around the
state handling.  This means the BobStateHandle construct that was
handling the interactions between always having a valid state and
handling the mutex is no longer needed, resulting in some nice
simplifications.

Part of #2777.
flub added a commit that referenced this issue Mar 1, 2022
The state bob needs to maintain during a secure-join process when
exchanging messages used to be stored on the context.  This means if
the process was killed this state was lost and the securejoin process
would fail.  Moving this state into the database should help this.

This still only allows a single securejoin process at a time, this may
be relaxed in the future.  For now any previous securejoin process
that was running is killed if a new one is started (this was already
the case).

This can remove some of the complexity around BobState handling: since
the state is in the database we can already make state interactions
transactional and correct.  We no longer need the mutex around the
state handling.  This means the BobStateHandle construct that was
handling the interactions between always having a valid state and
handling the mutex is no longer needed, resulting in some nice
simplifications.

Part of #2777.
flub added a commit that referenced this issue Mar 1, 2022
The state bob needs to maintain during a secure-join process when
exchanging messages used to be stored on the context.  This means if
the process was killed this state was lost and the securejoin process
would fail.  Moving this state into the database should help this.

This still only allows a single securejoin process at a time, this may
be relaxed in the future.  For now any previous securejoin process
that was running is killed if a new one is started (this was already
the case).

This can remove some of the complexity around BobState handling: since
the state is in the database we can already make state interactions
transactional and correct.  We no longer need the mutex around the
state handling.  This means the BobStateHandle construct that was
handling the interactions between always having a valid state and
handling the mutex is no longer needed, resulting in some nice
simplifications.

Part of #2777.
@flub
Copy link
Contributor

flub commented May 10, 2022

Proposal to allow bob to scan multiple QR codes from Alice concurrently:

In each response from Alice, vc-auth-required & vc-contact-confirm etc, Alice adds the Secure-Join-Invitenumber header which currently is only sent in Bob's vc-request message. While this code may have been sent in plain text, Bob only accepts messages which are verified to be from Alice_FP for these messages so it does not matter it is known by an attacker. This header allows Bob to identify which invite the messages pertain to.

For cases where Bob supports multiple concurrent joins but Alice doesn't yet Alice would not include this header. If Bob has multiple joins with Alice at this point he aborts all of them with a suitable error message in the chats.

@link2xt link2xt changed the title persist BobState, allow multiple BobStates Allow multiple BobStates Sep 24, 2023
@link2xt
Copy link
Collaborator

link2xt commented Sep 24, 2023

Currrently BobState is persisted: #2920
Renamed the issue, the only missing part is scanning multiple QR codes in parallel.

@link2xt
Copy link
Collaborator

link2xt commented Nov 14, 2024

Looking at the protocol, it seems there are no protocol changes required. In the worst case Bob scans two QR codes from Alice trying to join two different groups with the same inviter.

Bob sends two vc-request messages with different invite numbers. Alice receives both messages and sends back two identical vc-auth-required with the same key. The only problem is that now Bob who has two states does not know which AUTH code to send when vc-auth-required is received. But he can send two vc-request-with-auth messages out already when the first vc-auth-required message is received and advance both bob states to the next step. Then when the second vc-auth-required arrives, Bob will do nothing.

Bob states should also be cleaned up eventually to remove stale processes, but this can be done in housekeeping, like e.g. after 30 days, if not done already (did not check).

@iequidoo
Copy link
Collaborator

Code-wise it looks like Alice is also prepared to receiving v*-request-with-auth before v*-request, so Bob indeed may send multiple v*-request-with-auth messages if he doesn't know which to send.

@iequidoo iequidoo self-assigned this Jan 6, 2025
@link2xt
Copy link
Collaborator

link2xt commented Feb 11, 2025

Replaced with a less cluttered #6531

@link2xt link2xt closed this as completed Feb 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request verified-chats
Projects
None yet
Development

No branches or pull requests

4 participants