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(sdk-crypto): Add Identity based room key sharing strategy #3607

Merged
merged 4 commits into from
Jul 3, 2024

Conversation

BillCarsonFr
Copy link
Member

@BillCarsonFr BillCarsonFr commented Jun 25, 2024

Draft because based on #3605
Fixes #3563

This PR just introduce a new way to distribute room keys based on identities, by only sharing with devices signed by their owner.
This is a first PR to introduce this mode, but it will be really usable once we introduce some support for identity TOFU.

  • Public API changes documented in changelogs (optional)

Signed-off-by:

@BillCarsonFr BillCarsonFr changed the base branch from main to valere/invisible_crypto/refactor_share_2 June 25, 2024 14:07
@BillCarsonFr BillCarsonFr force-pushed the valere/invisible_crypto/indentity_base_sharing branch from 9c325a2 to e3b8900 Compare June 25, 2024 14:08
@BillCarsonFr BillCarsonFr requested a review from andybalaam June 25, 2024 14:08
@BillCarsonFr BillCarsonFr changed the title Valere/invisible crypto/indentity base sharing feat(sdk-crypto): Add Identity based room key sharing strategy Jun 25, 2024
@BillCarsonFr BillCarsonFr force-pushed the valere/invisible_crypto/indentity_base_sharing branch from e3b8900 to a12e281 Compare June 25, 2024 14:38
Base automatically changed from valere/invisible_crypto/refactor_share_2 to main June 25, 2024 14:54
@BillCarsonFr BillCarsonFr marked this pull request as ready for review June 25, 2024 14:57
@BillCarsonFr BillCarsonFr requested a review from a team as a code owner June 25, 2024 14:57
Copy link

codecov bot commented Jun 25, 2024

Codecov Report

Attention: Patch coverage is 90.47619% with 2 lines in your changes missing coverage. Please review.

Project coverage is 83.87%. Comparing base (ca6537b) to head (9294618).
Report is 138 commits behind head on main.

Files Patch % Lines
...c/session_manager/group_sessions/share_strategy.rs 85.71% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3607      +/-   ##
==========================================
+ Coverage   83.86%   83.87%   +0.01%     
==========================================
  Files         254      254              
  Lines       25892    25909      +17     
==========================================
+ Hits        21714    21731      +17     
  Misses       4178     4178              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@poljar poljar left a comment

Choose a reason for hiding this comment

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

Much better PR, thanks for taking the time to make this reviewable. I left some nits.

@@ -764,6 +764,18 @@ impl ReadOnlyDevice {
)
}

pub(crate) fn is_cross_signed_by_owner(
Copy link
Contributor

Choose a reason for hiding this comment

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

This method exists for Device

/// Is this device cross signed by its owner?
pub fn is_cross_signed_by_owner(&self) -> bool {
self.device_owner_identity.as_ref().is_some_and(|device_identity| match device_identity {
// If it's one of our own devices, just check that
// we signed the device.
ReadOnlyUserIdentities::Own(identity) => identity.is_device_signed(&self.inner).is_ok(),
// If it's a device from someone else, check
// if the other user has signed this device.
ReadOnlyUserIdentities::Other(device_identity) => {
device_identity.is_device_signed(&self.inner).is_ok()
}
})
}

Please just move the body of that function over here instead of duplicating it.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

None => {
// withheld all the users devices, we need to have an identity for this
// distribution mode
denied_devices_with_code
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's return in each branch of that match a RecipientDevices struct, makes it a bit clearer that we're not really mutating multiple times the above vectors and might get rid of a couple of allocations.

Suggested change
denied_devices_with_code
RecipientDevices {
denied_devices_with_code: user_devices
.into_values()
.map(|d| (d, WithheldCode::Unauthorised)),
..Default::default()
}

(You won't be able to apply that suggestion because I can't modify multiple lines and you need further modifications in the rest of the function).

.find(|(d, _)| d.device_id() == KeyDistributionTestData::dan_unsigned_device_id())
.expect("This dan's device should receive a withheld code");

assert_eq!(code.as_str(), WithheldCode::Unauthorised.as_str());
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the as_str() on both sides? This should be the same type, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

updated

@BillCarsonFr BillCarsonFr requested a review from poljar June 27, 2024 07:58
device_owner_identity: &ReadOnlyUserIdentities,
) -> bool {
match device_owner_identity {
ReadOnlyUserIdentities::Own(identity) => identity.is_device_signed(self).is_ok(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please preserve the comments from the other body of this function?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@BillCarsonFr BillCarsonFr requested a review from poljar July 2, 2024 15:07
@poljar poljar merged commit d8b2b74 into main Jul 3, 2024
39 checks passed
@poljar poljar deleted the valere/invisible_crypto/indentity_base_sharing branch July 3, 2024 14:18
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.

InvisibleCrypto | Share Room Keys: new RoomKeyShareStrategy based on user identities instead of per device
2 participants