Legacy key issues #767
Merged
Legacy key issues #767
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
neekolas
commented
May 23, 2024
neekolas
commented
May 23, 2024
| } | ||
|
|
||
| #[async_trait] | ||
| impl Signature for LegacyDelegatedSignature { | ||
| async fn recover_signer(&self) -> Result<MemberIdentifier, SignatureError> { | ||
| // 1. Verify the RecoverableEcdsaSignature | ||
| let legacy_signer = self.legacy_key_signature.recover_signer().await?; | ||
| // let legacy_signer = self.legacy_key_signature.recover_signer().await?; |
There was a problem hiding this comment.
We still need to verify the signature from the private key. The only validation this function does is check which wallet signed the public key. But we also need to make sure that the signature of the signature text, using the private key, is valid as well. If either signature is wrong we should return an error.
neekolas
commented
May 23, 2024
| @@ -179,6 +179,27 @@ mod tests { | |||
| assert!(!client.installation_public_key().is_empty()); | |||
| } | |||
|
|
|||
| #[tokio::test] | |||
| async fn legacy_keys() { | |||
There was a problem hiding this comment.
There is a quirk with this test case. It uses the legacy key the first time it is run, but after it has been run once on the local DB you will get a different result in calls to get_inbox_ids for that account address. That's because the first test run will register 0x0bD00B21aF9a2D538103c3AAf95Cb507f8AF1B28 with the network. If the address is already registered, the IdentityStrategy won't use the legacy key and will prompt for a wallet signature.
We should probably replace this with a dynamically generated legacy key so that we don't get confusing results in local testing where you might not drop the DB after each test run.
The test won't fail right now, but it's testing totally different code paths on subsequent runs.
nplasterer
added a commit
that referenced
this pull request
May 24, 2024
* 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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
I've found issues creating a client with a legacy key. I've created a new test case that reproduces an issue. The legacy key I'm using was exported from xmtp-js.
legacy_key_to_addressmatches the address provided. Butlegacy_key_to_addressreturns the XMTP V2 key converted to an eth address, when it should be returning the wallet that signed the XMTP v2 key. I have a fix for that.Identity(SignatureRequestBuilder(Signature(Invalid)))errors coming from my test case