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

Define MXCrypto and MXCrossSigning as protocols #1614

Merged
merged 3 commits into from
Oct 25, 2022
Merged

Conversation

Anderas
Copy link
Contributor

@Anderas Anderas commented Oct 20, 2022

Convert MXCrypto and MXCrossSigning into protocols and define ..Legacy.. implementations for both. These two types are the last two that were previously overriden by rust-based V2 implementations, but can now be relatively safely converted into protocols.

To make the conversion possible a number of changes were made:

  • Expose protocol types instead of implementation where possible
  • Legacy crypto functionality (MXLegacyKeyVerificationManager, MXKeyVerificationRequest, MXDecrypting ...) need to access the legacy implementation directly, as they depend on symbols defined inMXCrypto_Private.
  • A number of integration tests depend on private legacy symbols, not compatible with V2. This means those tests cannot be currently run against V2 implementation and will need to be fixed in the future.

Note that this PR does minimum changes to the actual protocols to ease the review of this PR. Future PRs will further tweak the protocol requirements and add nullability support for MXCrypto

@Anderas Anderas requested review from a team and pixlwave and removed request for a team October 20, 2022 11:22
@Anderas Anderas force-pushed the andy/crypto_protocol branch 2 times, most recently from 092d867 to 26301f3 Compare October 20, 2022 11:30
@codecov
Copy link

codecov bot commented Oct 20, 2022

Codecov Report

Base: 15.77% // Head: 36.35% // Increases project coverage by +20.57% 🎉

Coverage data is based on head (2be4bb0) compared to base (8fbd5d1).
Patch coverage: 46.26% of modified lines in pull request are covered.

❗ Current head 2be4bb0 differs from pull request most recent head 85a5d5a. Consider uploading reports for the commit 85a5d5a to get more accurate results

Additional details and impacted files
@@             Coverage Diff              @@
##           develop    #1614       +/-   ##
============================================
+ Coverage    15.77%   36.35%   +20.57%     
============================================
  Files          575      577        +2     
  Lines        90921    90959       +38     
  Branches     38355    39578     +1223     
============================================
+ Hits         14347    33068    +18721     
+ Misses       76107    56909    -19198     
- Partials       467      982      +515     
Impacted Files Coverage Δ
...xSDK/Crypto/Algorithms/Megolm/MXMegolmDecryption.m 87.00% <ø> (+68.92%) ⬆️
...xSDK/Crypto/Algorithms/Megolm/MXMegolmEncryption.m 91.66% <ø> (+90.80%) ⬆️
MatrixSDK/Crypto/Algorithms/Olm/MXOlmDecryption.m 43.06% <ø> (+41.62%) ⬆️
MatrixSDK/Crypto/Algorithms/Olm/MXOlmEncryption.m 5.26% <ø> (ø)
MatrixSDK/Crypto/CrossSigning/MXCrossSigning.m 80.00% <ø> (+80.00%) ⬆️
...atrixSDK/Crypto/Dehydration/MXDehydrationService.m 0.00% <0.00%> (ø)
MatrixSDK/Crypto/Devices/MXDeviceList.h 100.00% <ø> (+100.00%) ⬆️
MatrixSDK/Crypto/Devices/MXDeviceList.m 93.11% <ø> (+93.11%) ⬆️
...KeyBackup/Data/Aes256/MXAes256KeyBackupAlgorithm.m 35.71% <ø> (ø)
...p/Data/Curve25519/MXCurve25519KeyBackupAlgorithm.m 70.58% <ø> (+35.88%) ⬆️
... and 222 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@pixlwave pixlwave 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. The only question I have is whether you might want to rename some of the files to …Legacy… to match updated class names?

@@ -179,6 +174,20 @@ typedef NS_ENUM(NSInteger, MXCrossSigningErrorCode)
onPrivateKeysReceived:(void (^)(void))onPrivateKeysReceived
failure:(void (^)(NSError *error))failure;

/**
Does given secret, interpreted as a private key, match given public cross-signing key
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Does given secret, interpreted as a private key, match given public cross-signing key
Does the given secret, interpreted as a private key, match given public cross-signing key

// These methods and properties will not be available in MXCrossSigningV2 and therefore given
// integration tests cannot be run (or have to be re-written).
var legacyCrossSigning: MXLegacyCrossSigning? {
guard let crossSignign = crossSigning else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
guard let crossSignign = crossSigning else {
guard let crossSigning = crossSigning else {

@Anderas
Copy link
Contributor Author

Anderas commented Oct 24, 2022

Looks good to me. The only question I have is whether you might want to rename some of the files to …Legacy… to match updated class names?

I do plan to align all the naming (and potentially replace V2 with something more sensible), but I think adding Legacy only makes sense to those classes that have at least two implementations (eg. Crypto, KeyVerificationManager). For others (eg MegolmDecryption, OlmEncryption etc) this is unnecessary: there is currently no confusion and once fully migrated these files will be simply removed.

Base automatically changed from andy/trust_devices to develop October 25, 2022 13:07
Cross-sign self after restoring session with Crypto V2
@Anderas Anderas merged commit 2aa4808 into develop Oct 25, 2022
@Anderas Anderas deleted the andy/crypto_protocol branch October 25, 2022 14:19
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.

None yet

2 participants