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.

Sign up for GitHub

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

Jump to bottom

Validate Credential #689

Merged
merged 20 commits into from
May 8, 2024
Merged

Validate Credential #689

merged 20 commits into from
May 8, 2024

Conversation

insipx
Copy link
Contributor

@insipx insipx commented Apr 29, 2024 •
edited
Loading

closes #675

  • validate_inbox_ids
    • validate that a Memberidentifier belongs to an inbox_id
  • validate_inbox_id_key_packages
    • verify MLS key packages and pull out the MLS Credential (InboxID) and installation public key
  • e2e test inbox_id_key_packages
  • e2e test validating whether member identifier belongs to a set of inbox id identities

@tuddman tuddman added the inbox-id Support for Inbox ID label May 3, 2024
@insipx insipx marked this pull request as ready for review May 8, 2024 22:30
@insipx insipx requested a review from a team as a code owner May 8, 2024 22:30
neekolas
neekolas reviewed May 8, 2024
View reviewed changes
dev/gen_protos.sh Outdated Show resolved Hide resolved
neekolas
neekolas reviewed May 8, 2024
View reviewed changes
xmtp_mls/src/verified_key_package_v2.rs
Decode(#[from] DecodeError),
}

pub struct VerifiedKeyPackageV2 {
Copy link
Contributor

Choose a reason for hiding this comment

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

The old VerifiedKeyPackage actually checked the signatures in the credential, so you knew you could trust it.

I wonder if we want to either

A) Rename this so that it's clear users still need to check the identity update history for the inbox id to make sure it's valid
B) Add those checks in here and take a reference to the client to access the DB/API

Not a blocker. Just food for thought.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hm,

That's a good catch. I think we should probably go with B), I think A might still lead to users glossing over that. If users dont have a use for a key package that's actually invalid w.r.t InboxId, I think we should definitely include this check, despite added complexity with a Client

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it's a bit of both A and B.

This is kinda a half-verified key package. The MLS checks have passed, but not our Inbox ID authentication. That's still valuable for places like the MLS Validation Service, where you need to parse the key package and then later verify the inbox ID stuff.

So, maybe we have some sort of ParsedKeyPackage and VerifiedKeyPackage or whatever you want to name it. Then you can move from raw -> parsed -> verified, or stop in the middle.

@insipx insipx requested a review from 37ng May 8, 2024 23:01
@insipx insipx merged commit b19a633 into main May 8, 2024
6 checks passed
@insipx insipx deleted the insipx/validate-cred branch May 8, 2024 23:57
37ng pushed a commit that referenced this pull request May 10, 2024
@insipx @37ng
Validate Credential (#689)
557cdf5 
* gen protos

* trait method

* scaffold out validation fn, comment next steps in validation fn

* validation of inbox ids

* no expiration

* key package validation

* add files

* restore proto

* remove println

* do not break the wasm bindings
nplasterer added a commit that referenced this pull request May 24, 2024
@neekolas @insipx
@tuddman @37ng @richardhuaaa @nplasterer
Identity Release Branch (#718)
613d13e 
* Create validated commit for MLS

* Create validated commit for MLS

* Add/Remove members using GroupMembership

* Add/Remove members using GroupMembership

* Remove unused import

* initial commit

* initial commit

* Use latest protos

* Add missing installations

* Validate Credential (#689)

* gen protos

* trait method

* scaffold out validation fn, comment next steps in validation fn

* validation of inbox ids

* no expiration

* key package validation

* add files

* restore proto

* remove println

* do not break the wasm bindings

* Message History: handle Reply better (#723)

* new identity in client (#728)

* Consolidate on single method

* initial commit

* initial commit

* new identity in client (#728)

* Hack together something that compiles

* Fix straggling issues

* New DB schema

* Stub validated commit v2

* Start on mutable metadata

* Add some metadata changes

* initial commit

* initial commit

* new identity in client (#728)

* Hack together something that compiles

* initial commit

* initial commit

* new identity in client (#728)

* Fix straggling issues

* Add inbox_id fixes

* Remove dead code

* Fix lints

* Derive default for MutableMetadataChanges

* Rename identity table

* Stub validated commit v2

* Identity DB Schema (#739)

* New DB schema

* Rename identity table

* Cache association state (#733)

This caches the association state for a given inbox_id and sequence_id, allowing us to skip recomputing it/issuing network requests for smart wallet associations.

* Adding and removing group members

* More cleanup

* Remove after add

* Lint

* Update xmtp_mls/src/groups/sync.rs

Co-authored-by: 37ng <wang7ong@gmail.com>

* bindings_ffi: new client & signatureRequest. (#748)

* Gather group members from DB

* Implement inbox_sequence_id method in client (#737)

* Lint

* Remove unused import

* Get tests passing

* Add new GroupUpdated proto

* Use new group updated codec

* More loose ends

* Move back to main

* Make pub

* More tests passing

* Remove a few more todos

* Fix up bindings

* Fix up bindings

* Fix up subscriptions tests

* Add hack for unexpected installations

* Fix issue with group creation

* Fix lints

* Use specific docker image

* Fix commit validation logic

* Update tests since we now have transcript messages

* Unignore more tests

* Lower concurrency

* Change test-threads

* Lint

* Fix args

* Ignore failing test

* Update log line

* Un-ignore test

* Update docker image

* Lower number of test threads

* Lower number of test threads in bindings tests

* Ignore flaky test

* Update lock files

* Use latest openmls branch

* Fix some errors

* Fix key package reference

* No more compile errors

* Remove unused import

* Use latest node go

* Store key package hash ref

* Get inbox ID for address method

* Add test

* Remove unused import

* Lint

* Legacy key issues (#767)

* add log

* add log

* bump uniffi version to fix logging issues

* force push to hopefully clean CI cache

* All String lower case (#777)

* lowercase for inbox id

* lowercase for in create_client

* lint

* cargo

* bump the openmls version to a merged commit

* fix format linter

* inbox id not account address anymore

* Remove Await Helper in favor of Async Transactions (#779)

* remove await helper

* do not load mls group on every iteration

* don't need to clone provider

* keep provider as reference

* remove process_for_id_async and just make process_for_id async

* bump to the latest jna library

* update the makefile with the updated jna

* update the binaries

---------

Co-authored-by: Andrew Plaza <github.tech@liquidthink.net>
Co-authored-by: tuddman <tuddman@users.noreply.github.com>
Co-authored-by: yoduyodu <wang7ong@gmail.com>
Co-authored-by: Richard Hua <rich@xmtp.com>
Co-authored-by: Naomi Plasterer <naomi@xmtp.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@neekolas neekolas neekolas approved these changes

@37ng 37ng 37ng approved these changes

Assignees
No one assigned
Labels
inbox-id Support for Inbox ID
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Validate credentials with inbox_id
4 participants
@insipx @neekolas @37ng @tuddman