-
Notifications
You must be signed in to change notification settings - Fork 260
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
crypto(feat): Key distribution errors for pin violations #3662
crypto(feat): Key distribution errors for pin violations #3662
Conversation
daf7331
to
1d4cefb
Compare
8fbd9a4
to
e309219
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3662 +/- ##
==========================================
+ Coverage 84.12% 84.18% +0.05%
==========================================
Files 263 263
Lines 27584 27621 +37
==========================================
+ Hits 23205 23252 +47
+ Misses 4379 4369 -10 ☔ View full report in Codecov by Sentry. |
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 can't say much about the crypto logic, but the code LGTM and the docs and comments made it a bit easier to understand, thanks. I have a couple of comments, but feel free to ignore them.
crates/matrix-sdk-crypto/src/session_manager/group_sessions/share_strategy.rs
Outdated
Show resolved
Hide resolved
let shared: BTreeSet<OwnedDeviceId> = shared.keys().cloned().collect(); | ||
let shared: BTreeSet<&DeviceId> = shared.iter().map(|d| d.as_ref()).collect(); |
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.
Do we need to collect it twice?
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.
+1. I fixed another copy of this code recently in 1e58c03
Per #3565 (comment): this needs an update so that an error is not thrown if a TOFU-trusted user has rotated their identity. |
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.
A few comments. Also, needs a changelog entry.
#[error(transparent)] | ||
/// The room key that was to be shared was not shared because the sharing | ||
/// strategy could not be fulfilled. | ||
RoomKeySharingStrategyError(RoomKeySharingStrategyError), |
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.
#3810 added a "SessionRecipientCollectionError" which I think we should be using here, rather than adding a new error type.
let strategy = CollectStrategy::new_identity_based(); | ||
|
||
let encryption_settings = | ||
EncryptionSettings { sharing_strategy: strategy.clone(), ..Default::default() }; |
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.
no need to .clone()
here. Could just inline strategy
} | ||
|
||
pub fn initial_key_query() -> KeyQueryResponse { | ||
let data = response_from_file(&json!({ |
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.
#3823 removed response_from_file
device_id!("NZFSPBRLDO") | ||
} | ||
|
||
pub fn initial_key_query() -> KeyQueryResponse { |
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.
could you give all these methods a doc-comment explaining what they mean?
#[async_test] | ||
async fn test_share_identity_strategy_no_cross_signing() { |
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 would be good to add a doc-comment for each of these tests explaining what they are testing.
) | ||
.await; | ||
|
||
assert!(request_result.is_ok()); |
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.
this is normally spelt .unwrap()
let request_result = machine | ||
.share_room_key( | ||
fake_room_id, | ||
// vec![KeyDistributionTestData::dan_id()].into_iter(), |
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.
dead code?
} | ||
|
||
#[async_test] | ||
async fn test_share_identity_strategy_report_pinning_violation() { |
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.
as well as a all the cleanups suggested for the other test, this test could do with some comments to explain what's going on.
let mut devices: BTreeMap<OwnedUserId, Vec<DeviceData>> = Default::default(); | ||
let mut withheld_devices: Vec<(DeviceData, WithheldCode)> = Default::default(); |
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.
not sure that moving this declaration is helpful?
|
||
for user_id in users { |
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.
This feels like a big refactor, combined with behavioural changes. Please could we have the refactor and the behavioral change as separate commits?
.other() | ||
.unwrap() |
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 think some of these .other().unwrap()
calls become unnecessary with #3847
I "merged" |
…anges (#3884) This is part of #3662, pulled out to into a separate PR. Recent changes in `main` made it pretty much impossible to merge this section of code from `main` into that PR, and Rich wanted to see the refactoring bits separate from the behavioural changes. So I've re-written the refactoring. Pulls the `match` on `sharing_strategy` outside of the `for` loop, and moves any code that is specific to one strategy into the appropriate branch.
Obsoleted by #3896 |
Part of invisible crypto, follow up of #3639
Fixes #3565
Following support for key pinning, we have now to manage pinning violation when encrypting olm messages (room key distribution).
The new encryption errors:
Signed-off-by: