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.

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

feat(crypto): Customized event types #767

Merged
merged 1 commit into from
Jul 7, 2022
Merged

Conversation

poljar
Copy link
Contributor

@poljar poljar commented Jun 16, 2022

This patch adds customized event types, currently only for the m.room_key and m.secret.send to-device events.

This allows us to:
a) Deserialize the session_key field into a vodozemac type
b) Control when we zeroize secrets better

@codecov
Copy link

codecov bot commented Jun 16, 2022

Codecov Report

Merging #767 (4a198d1) into main (f0190b4) will increase coverage by 0.02%.
The diff coverage is 68.77%.

❗ Current head 4a198d1 differs from pull request most recent head e6bde16. Consider uploading reports for the commit e6bde16 to get more accurate results

@@            Coverage Diff             @@
##             main     #767      +/-   ##
==========================================
+ Coverage   77.92%   77.95%   +0.02%     
==========================================
  Files          92       96       +4     
  Lines       13930    14088     +158     
==========================================
+ Hits        10855    10982     +127     
- Misses       3075     3106      +31     
Impacted Files Coverage Δ
crates/matrix-sdk-crypto/src/store/mod.rs 53.73% <0.00%> (+0.39%) ⬆️
crates/matrix-sdk-crypto/src/types/mod.rs 81.53% <ø> (ø)
crates/matrix-sdk-crypto/src/machine.rs 80.02% <60.00%> (+3.24%) ⬆️
...es/matrix-sdk-crypto/src/types/events/to_device.rs 61.73% <61.73%> (ø)
crates/matrix-sdk-crypto/src/gossiping/machine.rs 80.40% <70.58%> (-0.06%) ⬇️
...tes/matrix-sdk-crypto/src/types/events/room_key.rs 74.28% <74.28%> (ø)
.../matrix-sdk-crypto/src/types/events/secret_send.rs 82.35% <82.35%> (ø)
.../matrix-sdk-crypto/src/verification/event_enums.rs 67.51% <89.47%> (-0.24%) ⬇️
crates/matrix-sdk-crypto/src/olm/account.rs 82.29% <100.00%> (-0.05%) ⬇️
...atrix-sdk-crypto/src/olm/group_sessions/inbound.rs 88.32% <100.00%> (-0.09%) ⬇️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f0190b4...e6bde16. Read the comment docs.

@poljar poljar requested a review from a team June 17, 2022 11:45
@@ -763,7 +761,7 @@ impl GossipMachine {
&self,
sender_key: &str,
event: &mut SecretSendEvent,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this code working with mutable events in the first place? That strikes me as a little odd.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is explained here:
https://github.com/matrix-org/matrix-rust-sdk/pull/767/files#diff-723b3a87aa508f41a037f9c4222c6c2e125abca051e3d60c43cb99e275ebcfe7R783=

// Set the secret name so other consumers of the event know
// what this event is about.

That event type doesn't really carry enough information to let users know what the secret is about. You only get to know this via previous state, from the time you made the request.

For convenience we set the secret name.


/// An enum over the various to-device events we support.
#[derive(Debug)]
pub enum ToDeviceEvents {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this is not a collection type (i.e. an instance only contains one event), its name should be singular, right? We could use the same naming as Ruma (AnyToDeviceEvent).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, but having two AnyToDeviceEvent types in the crate would confuse me to no end. Eventually we'll use only our customized one in the matrix-sdk-crypto crate and we'll rename it then.

So bear with me until we get to that point.

@poljar
Copy link
Contributor Author

poljar commented Jul 5, 2022

Rebased onto main to get CI going.

@poljar poljar requested a review from a team July 6, 2022 13:15
Copy link
Member

@Hywan Hywan left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for having taking time to do it.

crates/matrix-sdk-crypto/src/types/events/mod.rs Outdated Show resolved Hide resolved
crates/matrix-sdk-crypto/src/types/events/to_device.rs Outdated Show resolved Hide resolved
This patch adds customized event types, currently only for the
m.room_key and m.secret.send to-device events.

This allows us to:
    a) Deserialize the session_key field into a vodozemac type
    b) Control when we zeroize secrets better
@poljar poljar force-pushed the poljar/vodozemac-room-key-event branch from 4a198d1 to e6bde16 Compare July 7, 2022 16:55
@poljar poljar merged commit a7af96d into main Jul 7, 2022
@poljar poljar deleted the poljar/vodozemac-room-key-event branch July 7, 2022 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants