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
Use vodozemac for the E2EE support #494
Conversation
9c13a10
to
641138e
Compare
b78b0b5
to
e6926ed
Compare
42bf08a
to
beb2d3c
Compare
I think this should be ready for review. Couple of things that weren't done as part of this, since it would blow up the PR:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have only looked superficially over the crypto-part of things, I don't really know enough to give that a proper in-depth review. However, I've found a few remarks, I'd like to see addressed...
|
||
// We need to add the new session to the session cache, otherwise | ||
// we might try to create the same session again. | ||
// TODO separate the session cache from the storage so we only add |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Proposal: Let's make a conscious effort to remove ToDo- and FIXME from the codebase and instead put them into the issue tracker. Therefore let's only merge FIXMEs that link to a specific issue tracked somewhere.
Rational: Inline ToDo- and FIXME comments are a bad habit of not really doing something we think at that time should be done. However, as these are hidden in the code base somewhere, they are hard to track and often way to short - not even talking about the ability to comment and discuss them. Often we find incomplete and - because of changes in the code - now incorrect TODO- and FIXME-remarks that just confuse the reader as they are everything but accurate. On the other hand, they are hidden in the code base somewhere rather than made explicit in the issue tracker and thus we have an incomplete view on the technical debt we have in our system.
Using these markers during development for oneself is totally fine, but when merging, and we still have them in there, they should be moved into the appropriate issue tracker.
What I have seen to work well is to limit usage of FIXME at merge to thing that link to an issue, either here for us to fix, or related to a problem that this code base works around elsewhere by posting the link in between. TODOs are entirely removed and, if appropriate, put into an issue on our own tracker, but as they are often not bound to a specific small piece of code, have no reason to stay in there.
Nice side effect to keep in mind when making these explicit: we do have a nice community of people, who like to contribute, in particular around specific and clearly defined, useful work like these. By putting them into concrete issues, we raise the chances of them actually being addressed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks sensible overall. Left some minor style tweaks and a few questions/comments. After addressing those, it's good to go.
@@ -534,15 +535,14 @@ impl ReadOnlyDevice { | |||
} | |||
|
|||
pub(crate) fn is_signed_by_device(&self, json: &mut Value) -> Result<(), SignatureError> { | |||
let signing_key = | |||
let key = | |||
self.get_key(DeviceKeyAlgorithm::Ed25519).ok_or(SignatureError::MissingSigningKey)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we keep the key serialized here in the first place? Could we be storing it in a deserialized form already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. The inner type here comes from ruma-common so either Ruma needs to depend on vodozemac or we need to have a customized version of this type.
Now I do plan on introducing a customized version of this type, but there are still a couple of problems with deserializing the BTreeMap<Box<DeviceKeyId>, String>,
into a BTreeSet<DeviceKey>
where DeviceKey
would be something like
enum DeviceKey {
Ed25519(Ed25519PublicKey),
Curve25519(Curve25519PublicKey),
Other(String)
}
-
DeviceKeyId
doesn't contain only the algorithm (ed25519
,curve25519
), it also contains the device id of the device that owns the key. This makes it hard to use theDeviceKeyId
for theSerde
tag. -
Serde
doesn't support theOther
variant on non-unit enum variants nicely: accept #[serde(other)] on non-unit enum variants serde-rs/serde#1973.
Overall, maybe we'll end up with a customized type that will do this, though not immediately. It'll depend on how annoying the serialization/deserialization code will be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a bunch of types like that in Ruma. We should definitely have a tracking issue for these Vodozemac / Ruma integration points. I think a dependency either direction could be problematic and we'll more likely end up having a lower-level crate underneath both, but all of that can be discussed after this PR is merged 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's actually what I had planned. There are a bunch of customized types in the crypto crate already, more will come soon. So the plan is to have some vodozemac powered types in a small crate that both the crypto crate and other weird e2ee related projects can use.
The test didn't check if we can actually save/restore multiple devices, this patch fixes this.
This PR intends to replaces olm-rs with vodozemac. It's still in a messy state, but can already be tested. I have been using this for a couple of days in a custom weechat-matrix-rs build.
The missing things here are:
One notable exception here is the backup support. Backup support requires support for
PkEncrypt
andPkDecrypt
structs from libolm, the whole asymmetric encryption scheme is deemed to be insecure and has been abandoned in favor of MSC3270.In conclusion, if backup support is needed, libolm will still be used.