-
Notifications
You must be signed in to change notification settings - Fork 304
feat(crypto): Add EncryptionInfo
to Decrypted
to-device variant
#5074
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5074 +/- ##
==========================================
- Coverage 85.91% 85.91% -0.01%
==========================================
Files 335 335
Lines 36468 36481 +13
==========================================
+ Hits 31331 31342 +11
- Misses 5137 5139 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
223f499
to
f14ecc9
Compare
/// The info if the event was encrypted using m.olm.v1.curve25519-aes-sha2 | ||
OlmV1Curve25519AesSha2 { | ||
// The sender device key, base64 encoded | ||
curve25519_public_key_base64: String, |
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 the stringly typing? Curve25519Publickey
is a Copy
type so it's cheaper to copy it around, provides stronger typing and has a to_base64()
method if you do need a string.
I feel like we've been here already once.
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.
It tried to keep it consistent with the other variant.
I suppose I have then to use serde(deserialize_with = "deserialize_curve_key", .. "serialize_curve_key"
?
I'll do it too for MegolmV1AesSha2
to stay consistant
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.
Actually I am not sure about this change.
To do it, I need to add vodozemac as a dependency of matrix-sdk-common (it is not yet), would I need also to add some feature = "e2e-encryption"
cfg?
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 had another look, and if I just refactor both Olm and Megolm variant to use Curve25519Publickey
instead of strings, I'll need to
- make the
common
crate to depend onvodozemac
- Then for ser/deser compatibility (particularly for Megolm variant as part of
TimelineEventKind
) we need to movedeserialize_curve_key
andserialize_curve_key
from crypto crate down to common crate (that will also bring back a dependency to zeroize).
This is touching several tests files. Looks like a candidate for a separate PR
if json.get("raw").is_some() && json.get("encryption_info").is_some() { | ||
// If the "raw" field is present, it's a NewFormat | ||
#[derive(Deserialize)] | ||
struct NewFormat { | ||
raw: Raw<AnyToDeviceEvent>, | ||
encryption_info: EncryptionInfo, | ||
} |
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.
What happens if someone puts a raw
and encryption_info
field into the JSON of the event?
It seems to me that the sender of the event, perhaps in collusion with the homeserver, might insert an encryption_info
on their own.
Come to think of, why does this need to support Serialize
and Deserialize
? Because the widgets API will transfer this verbatim to the widget using JSON?
In any case, I think it's safer to not have a migration here since we don't seem to need it.
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.
Come to think of, why does this need to support Serialize and Deserialize?
I'll check but I think it mostly to export them to bindings?
But I agree that those should not be persisted, but we previously said that if we put something as serializable we should provide migration. I wish we had another trait for things that can be persisted
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.
So you are right.. Serialize and Deserialize are not used at all (I also checked in wasm bindings and it is not). Removing all that
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.
Alright, I still have a couple of nits I would like to see fixed.
But I think we're good after that.
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 good.
This includes matrix-org/matrix-rust-sdk#5074, which adds an `OlmV1Curve25519AesSha2` variant to `AlgorithmInfo`, meaning some conversions are now fallible.
This includes matrix-org/matrix-rust-sdk#5074, which adds an `OlmV1Curve25519AesSha2` variant to `AlgorithmInfo`, meaning some conversions are now fallible.
This includes matrix-org/matrix-rust-sdk#5074, which adds an `OlmV1Curve25519AesSha2` variant to `AlgorithmInfo`, meaning some conversions are now fallible.
The
ProcessedToDeviceEvent::Decrypted
variant now also have anEncryptionInfo
field.Format changed from
Decrypted(Raw<AnyToDeviceEvent>)
toDecrypted { raw: Raw<AnyToDeviceEvent>, encryption_info: EncryptionInfo) }
It is needed for future work on widget in order to support encrypted to device properly via
add_event_handler
This is a follow-up on
crypto: Add variants for plain text and encrypted to-device events #4935
refactor(crypto): Move
session_id
fromEncryptionInfo
toAlgorithmInfo
as it is megolm specific #4981Public API changes documented in changelogs (optional)
Signed-off-by: