Encrypt rooms' last messages #1718

alfogrillo · 2023-02-13T18:21:51Z

Description

This PR adds the AES encryption for rooms' last messages before they get stored into Core Data.

Before	After

codecov · 2023-02-13T18:35:00Z

Codecov Report

Base: 17.52% // Head: 17.63% // Increases project coverage by +0.11% 🎉

Coverage data is based on head (3117c9c) compared to base (2be6c58).
Patch coverage: 63.82% of modified lines in pull request are covered.

❗ Current head 3117c9c differs from pull request most recent head a3a5ea9. Consider uploading reports for the commit a3a5ea9 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1718      +/-   ##
===========================================
+ Coverage    17.52%   17.63%   +0.11%     
===========================================
  Files          612      612              
  Lines        95993    96148     +155     
  Branches     40294    40353      +59     
===========================================
+ Hits         16826    16960     +134     
- Misses       78658    78674      +16     
- Partials       509      514       +5

Impacted Files	Coverage Δ
MatrixSDK/Data/Store/MXFileStore/MXFileStore.m	`17.37% <ø> (ø)`
MatrixSDK/Data/MXRoomLastMessage.m	`32.70% <63.04%> (+15.14%)`	⬆️
...aryStore/CoreData/Models/MXRoomLastMessageMO.swift	`41.17% <100.00%> (-10.55%)`	⬇️
...o/Algorithms/RoomEvent/MXRoomEventDecryption.swift	`53.77% <0.00%> (-1.57%)`	⬇️
...ypto/Verification/MXKeyVerificationManagerV2.swift	`20.73% <0.00%> (-0.56%)`	⬇️
MatrixSDK/Crypto/MXCrypto.m	`2.06% <0.00%> (-0.01%)`	⬇️
MatrixSDK/Utils/Media/MXMediaLoader.m	`0.00% <0.00%> (ø)`
...SDK/Crypto/Verification/MXKeyVerificationManager.m	`0.00% <0.00%> (ø)`
.../Crypto/KeyBackup/Engine/MXNativeKeyBackupEngine.m	`0.00% <0.00%> (ø)`
...ts/Crypto/Migration/MXCryptoMigrationV2Tests.swift	`0.00% <0.00%> (ø)`
... and 4 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.

MatrixSDK/Data/MXRoomLastMessage.m

Anderas · 2023-02-15T14:39:40Z

MatrixSDK/Data/RoomSummaryStore/CoreData/Models/MXRoomLastMessageMO.swift

        } else {
-            s_others = nil
+            s_attributedText = lastMessage.attributedText.map(NSKeyedArchiver.archivedData(withRootObject:))


I think its a bit confusing that archiving of encrypted data happens in MXRoomLastMessage but unencrypted in MXRoomLastMessageMO ... it creates some assumptions about responsibilities that are not obvious from the API.

An alternative approach would be to simply expose archivedAttributedText getter on MXRoomLastMessage which would be used by MXRoomLastMessageMO directly. That way the latter will not need to perform any additional encryption checks

Anderas · 2023-02-15T14:41:40Z

MatrixSDK/Data/MXRoomLastMessage.h

@@ -69,6 +69,8 @@ FOUNDATION_EXPORT NSString *const MXRoomLastMessageDataType;

 - (instancetype)initWithEvent:(MXEvent *)event;

+- (nullable NSData*)encryptedAttributedString;


As suggested below, I would rename this archivedAttributedText or attributedTextData and let it handle both encrypted and unencrypted variant, otherwise we are handling the archiving in two separate objects, depending on the encryption status.

Also I'd convert this into property (nonatomic, readonly, nullable) NSData *archivedAttributedText and move just below attributedText to make the API more consistent and clearer

Anderas · 2023-02-15T14:50:02Z

MatrixSDK/Data/MXRoomLastMessage.m

+
+        if (_attributedText == nil && model.s_text)
+        {
+            _attributedText = [[NSAttributedString alloc] initWithString:model.s_text];


Does this not change the existing behaviour? Previously if _attributedText was unset, it would not be created from model.s_text. It is not obvious to me whether text and attributedText are always variants of the same data or not. And if we always end up using attributedText whether it would be simpler to remove text entirely

pixlwave

I think that changing the file store version would probably add enough changes that I'll leave finishing the review until then.

Also, I'm not super keen on the sensitiveData naming tbh - I think being more explicit and saying encryptedData when its encrypted and simply lastMessageData/Dictionary when its unencrypted would be clearer here.

MatrixSDK/Data/MXRoomLastMessage.h

MatrixSDK/Data/MXRoomLastMessage.m

pixlwave · 2023-02-16T17:40:48Z

MatrixSDK/Data/MXRoomLastMessage.m

+            _attributedText = sensitiveDataDictionary[kCodingKeyAttributedText];
+            _others = sensitiveDataDictionary[kCodingKeyOthers];
+        }
+        else // fallback logic for old database versions


You can completely avoid having to deal with old versions by incrementing kMXFileVersion in MXFileStore. It'll wipe the data and force a fresh sync when the user upgrades. Probably better than running the old way in parallel and avoids having to think about migrations etc.

Technically I didn't change the MXFileStore so I'm not sure this is clean.
Btw does it mean people will need to do a full sync potentially taking a lot of time (for account big as Matthew's one)?

...ata/MXCoreDataRoomSummaryStore.xcdatamodeld/MXRoomSummaryCoreDataStore2.xcdatamodel/contents

pixlwave · 2023-02-16T18:10:36Z

I've just twigged about the naming as sensitiveData is only encrypted for rooms with encryption enabled. Probably just lastMessageData or messageData to be clearer, otherwise ignore that comment 🙃

Co-authored-by: Doug <[email protected]>

alfogrillo · 2023-02-16T18:25:23Z

I've just twigged about the naming as sensitiveData is only encrypted for rooms with encryption enabled. Probably just lastMessageData or messageData to be clearer, otherwise ignore that comment 🙃

I don't know, names like these look confusing to me on types that already call themself MXRoomLastMessage. 😅
Btw the reasoning is that the sensitive data is always sensitive, in encrypted rooms we encrypt it.

pixlwave

LGTM, chatted about the file store versions offline.

Anderas

This looks so much clearer now 👍

Anderas · 2023-02-22T10:06:28Z

MatrixSDK/Data/MXRoomLastMessage.m

+                                                             error:&error];
+
+    if (error) {
+        MXLogDebug(@"[MXRoomLastMessage] did fail to archive sensitiveDataDictionary. Error: %@", error.description);


For future reference: errors are best logged via MXLogErrorDetails which will also send them to Sentry (if enabled), or even MXLogFailureDetails if we do not expect this error to ever occur

alfogrillo added 3 commits February 13, 2023 19:07

Add AES encryption

e4e7b2b

Cleanup code style

9b8b629

Add changelog.d file

056afdd

alfogrillo changed the title ~~Encrypt room's last messages~~ Encrypt rooms' last messages Feb 13, 2023

alfogrillo requested a review from Anderas February 13, 2023 18:24

Anderas reviewed Feb 15, 2023

View reviewed changes

alfogrillo added 3 commits February 15, 2023 16:20

Improve encryptedAttributedString

1e1c243

Add sensitiveData method

e032582

Encrypt/decrypt last message’s sensitive data

9c2a2ce

alfogrillo force-pushed the alfogrillo/fix_clear_last_message branch from 3d120bf to 9c2a2ce Compare February 16, 2023 15:53

alfogrillo added 4 commits February 16, 2023 16:59

Depecrate core data vars

10d4a6c

Fix mapping in MXRoomLastMessage

54aa4d9

Switch to legacy unarchive api

7bc3e2e

Amend comments

6a5f3b9

pixlwave reviewed Feb 16, 2023

View reviewed changes

Update MatrixSDK/Data/MXRoomLastMessage.h

e03289a

Co-authored-by: Doug <[email protected]>

Improve logging

e6a0aee

pixlwave approved these changes Feb 17, 2023

View reviewed changes

alfogrillo added 2 commits February 20, 2023 11:02

Implement hard migration

3117c9c

Refine changelog.d file

a3a5ea9

alfogrillo merged commit e90403a into develop Feb 21, 2023

alfogrillo deleted the alfogrillo/fix_clear_last_message branch February 21, 2023 09:33

Anderas reviewed Feb 22, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Encrypt rooms' last messages #1718

Encrypt rooms' last messages #1718

alfogrillo commented Feb 13, 2023 •

edited

Loading

codecov bot commented Feb 13, 2023 •

edited

Loading

Anderas Feb 15, 2023 •

edited

Loading

Anderas Feb 15, 2023

Anderas Feb 15, 2023

pixlwave left a comment

pixlwave Feb 16, 2023

alfogrillo Feb 16, 2023

pixlwave commented Feb 16, 2023

alfogrillo commented Feb 16, 2023

pixlwave left a comment

Anderas left a comment

Anderas Feb 22, 2023

		@@ -69,6 +69,8 @@ FOUNDATION_EXPORT NSString *const MXRoomLastMessageDataType;

		- (instancetype)initWithEvent:(MXEvent *)event;

		- (nullable NSData*)encryptedAttributedString;

Encrypt rooms' last messages #1718

Encrypt rooms' last messages #1718

Conversation

alfogrillo commented Feb 13, 2023 • edited Loading

Description

codecov bot commented Feb 13, 2023 • edited Loading

Codecov Report

Anderas Feb 15, 2023 • edited Loading

Choose a reason for hiding this comment

Anderas Feb 15, 2023

Choose a reason for hiding this comment

Anderas Feb 15, 2023

Choose a reason for hiding this comment

pixlwave left a comment

Choose a reason for hiding this comment

pixlwave Feb 16, 2023

Choose a reason for hiding this comment

alfogrillo Feb 16, 2023

Choose a reason for hiding this comment

pixlwave commented Feb 16, 2023

alfogrillo commented Feb 16, 2023

pixlwave left a comment

Choose a reason for hiding this comment

Anderas left a comment

Choose a reason for hiding this comment

Anderas Feb 22, 2023

Choose a reason for hiding this comment

alfogrillo commented Feb 13, 2023 •

edited

Loading

codecov bot commented Feb 13, 2023 •

edited

Loading

Anderas Feb 15, 2023 •

edited

Loading