Add legacy v2-signed keys #131
Conversation
| bytes installation_public_key = 1; | ||
| oneof public_key { | ||
| bytes installation_key = 1; | ||
| bytes unsigned_legacy_create_identity_key = 2; |
There was a problem hiding this comment.
What do we mean by unsigned here?
There was a problem hiding this comment.
This is referring to the key_bytes field on the legacy SignedPublicKey, rather than a serialization of the whole SignedPublicKey. We need to do this because proto serialization is not always consistent, so comparing the bytes of the proto may not always match.
Can see why this might be confusing, open to suggestions!
There was a problem hiding this comment.
Do we care about serialization consistency here? If we've included the exact bytes that were signed, can't we just verify that it deserializes into the right proto. Or are we re-serializing it somewhere?
It seems like it would be helpful to have the wallet signature on the proto, since then you can verify that the wallet doing the revoking matches the wallet that signed the key.
There was a problem hiding this comment.
Totally possible I'm misunderstanding something
There was a problem hiding this comment.
If we've included the exact bytes that were signed, can't we just verify that it deserializes into the right proto
There's two reasons why we might want to attach the unsigned bytes rather than the signed bytes:
- The client can just do
==on the unsigned bytes, whereas the alternative is that the client must deserialize the signed bytes, and then do==on the unsigned bytes anyway. With the latter, the added complexity means clients might screw this up (e.g., do==on the signed bytes) - Attaching the unsigned bytes results in a smaller payload that only contains what is needed
It seems like it would be helpful to have the wallet signature on the proto, since then you can verify that the wallet doing the revoking matches the wallet that signed the key.
I don't think that's the important thing to verify here. Note that a malicious user could sign an existing installation key from someone else using their own wallet, and then revoke it using their own wallet, and then the revocation payload will be internally consistent. I think the important thing to verify here is that within the context of a conversation, the account address on the credential in the conversation matches the account address on the revocation signature.
We're simply saying that this key cannot perform actions on behalf of this account, regardless of how the key was originally registered.
There was a problem hiding this comment.
I do think maybe there's an issue with this field being ambiguous or confusing. I can add an explanatory comment for what it's referring to, or use a different name if you have any thoughts there?
There was a problem hiding this comment.
OK. That sounds reasonable. You've convinced me.
|
🎉 This PR is included in version 3.41.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This adds the legacy credential type. Support for generating these credentials will be added in the next PR. - Apply the changes from xmtp/proto#131 - Add ValidatedLegacySignedPublicKey type and tests - Add LegacyCreateIdentityAssociation - cannot add tests until I have support for serializing into proto, will add in next PR - Significant refactoring. We use separate classes in separate files for different associations. The strong typing ensures we can't mix them up (for example confusing grant messaging access with revoke messaging access). Because the protos have changed, some steps are required to avoid errors for those who already have libxmtp running: - Run `./dev/down && ./dev/up` from `libxmtp`. This rebuilds the validation service, and clears out any data stored on the server. - Remove any existing databases, for example from the CLI and Android/iOS/flutter/RN
Will land xmtp/libxmtp#425 immediately after this PR lands