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

Sandbox syncing with removed members #21

Open
HerbCaudill opened this issue Dec 9, 2021 · 1 comment
Open

Sandbox syncing with removed members #21

HerbCaudill opened this issue Dec 9, 2021 · 1 comment
Projects

Comments

@HerbCaudill
Copy link
Member

We'll still open a connection and sync with someone who appears to have been removed from the team, in case they have information that would invalidate their removal (e.g. whoever removed them was concurrently demoted or removed themselves).

However, when we're syncing we don't want to provide them any information until we're sure they're still on the team. I think right now a removed member can sync up and continue to get updates to the signature chain.

@HerbCaudill HerbCaudill created this issue from a note in Release (To do) Dec 9, 2021
@HerbCaudill HerbCaudill changed the title When syncing, don't send new links to removed members Sandbox syncing with removed members Dec 14, 2021
@HerbCaudill
Copy link
Member Author

Just to be clear why need to connect with removed members (or call them "maybe-removed" members):

  • Alice, Bob, and Charlie are on the team; Alice and Bob are admins.
  • Alice and Bob concurrently remove each other from the team.
  • Charlie connects to Bob. He now believes that Alice has been removed from the team.
  • However, Alice wins the conflicting removals, because she is senior to Bob.
  • Charlie connects to Alice and learns that Alice concurrently removed Bob, so it is Bob who should be removed and not Alice. So Charlie keeps the connection to Alice and disconnects from Bob.

This scenario is reflected in this test, which needs to continue to pass.

it(`Alice and Bob demote each other concurrently`, () => {
show('Bob:laptop')
alice().addToTeam('Bob')
// Only Alice is admin
aliceToAlice().should('be.admin')
bobToAlice().should('not.be.admin')
aliceToBob().should('be.admin')
bobToBob().should('not.be.admin')
// Alice promotes Bob
alice().promote('Bob')
// Now both are admins
aliceToAlice().should('be.admin')
bobToAlice().should('be.admin')
aliceToBob().should('be.admin')
bobToBob().should('be.admin')
// Alice and Bob both disconnect
alice().toggleOnline()
bob().toggleOnline()
// They demote each other
alice().demote('Bob')
bob().demote('Alice')
// They both reconnect
alice().toggleOnline()
bob().toggleOnline()
// Alice is admin, Bob is not
aliceToAlice().should('be.admin')
bobToAlice().should('not.be.admin')
aliceToBob().should('be.admin')
bobToBob().should('not.be.admin')
})

However, this makes it possible for a removed member to keep syncing indefinitely with the remaining members — both providing them new links and receiving links that were added after they were removed. So we need to handle syncing with a maybe-removed member differently.

The only reason to sync with a maybe-removed member is to see if they have links that would invalidate their removal. If that's not the case, I should neither accept new links from them, nor give them any links they don't have.

Plan:

  • Create a failing test showing a removed user exploiting this vulnerability to add links to the chain
  • Create a failing test showing a removed user exploiting this vulnerability to receive new links from the chain
  • Create a separate, sandboxed sync process for maybe-removed members. This is a one-sided sync — Charlie receives links from Alice, but initially doesn't provide any links. And Charlie syncs against a clone of the chain, which he only keeps if Alice's removal turns out to be invalidated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Release
To do
Development

No branches or pull requests

1 participant