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

Add detail and address feedback on identity doc #353

Merged
merged 11 commits into from
Dec 5, 2023

Conversation

richardhuaaa
Copy link
Contributor

Preview the doc here: https://github.com/xmtp/libxmtp/blob/rich/identity-doc-updates/xmtp_mls/IDENTITY.md

Adding more detail that was requested during review, as well as addressing feedback.

  • Add context on ethereum wallets
  • Add detail on signature formats and signature labelling
  • Add installation_public_key to credentials
  • Clean up time representation
  • Add detail on credential validation and installation synchronization
  • Add detail on revocation
  • Add account-level membership description


At any time, the user may enumerate active installations by querying for all identity updates under the account. The user may identify each installation by the creation time as well as the installation key from the signing text.
1. Query the server for identity updates on the sending wallet address and verify that the sending installation has not been revoked.
Copy link
Contributor

@neekolas neekolas Nov 30, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is going to be viable as part of the message processing flow. Any race condition here could lead to part of the group accepting a commit and part of a group rejecting it. Even if we are only checking whether the installation was revoked at the time the message was sent, and not the time it was read, it still opens the door to potential race conditions given that revocation state does not have the same total ordering properties as topic contents.

The good news is that revocations don't need to be instant.

I propose we don't check for revocation when reading payloads and instead do it before synchronizing the group state. Anyone with privileges allowing them to remove users would check the network for any revoked members of the group and submit a commit to remove the revoked installation from the group before processing any other intents.

How this interacts with group permissions is TBD. Maybe we want a carve-out allowing anyone to remove a revoked installation so that we don't have to wait for an admin to come online. Maybe we have each user go through all their groups and remove themselves as part of revocation.

Copy link
Contributor Author

@richardhuaaa richardhuaaa Dec 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My preference is to have total ordering between revocations and commits, which is a little more robust, but I concede that the current direction of decentralization work doesn't seem to be heading that way.

Updated the revocation approach in 18c7f93!

@richardhuaaa richardhuaaa requested a review from neekolas December 3, 2023 22:47
@richardhuaaa richardhuaaa force-pushed the rich/identity-doc-updates branch from a54dbad to 18c7f93 Compare December 3, 2023 22:51
Copy link

@karthikbhargavan karthikbhargavan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These clarifications address all the security considerations we considered, so it looks great. The only thing I would add is an explicit note that we the installation_key should only be used in MLS and not elsewhere. If it is, then we need to review those use cases to ensure signature non-collisions.

@richardhuaaa richardhuaaa enabled auto-merge (squash) December 5, 2023 23:25
@richardhuaaa richardhuaaa merged commit bdbe0ab into main Dec 5, 2023
7 checks passed
@richardhuaaa richardhuaaa deleted the rich/identity-doc-updates branch December 5, 2023 23:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants