feat: re-establish EAR support - WPB-23473#4299
feat: re-establish EAR support - WPB-23473#4299David-Henner wants to merge 27 commits intorelease/cycle-4.16from
Conversation
# Conflicts: # WireDomain/Sources/WireDomain/Helpers/ProtobufMessageDecoder.swift
feb4b98 to
268f4a8
Compare
Test Results 8 files 982 suites 9m 50s ⏱️ Results for commit 18f63c3. ♻️ This comment has been updated with latest results. Summary: workflow run #22218036080 |
|
| // @SF.Storage @TSFI.FS-IOS @TSFI.Enclave-IOS @S0.1 @S0.2 | ||
| // Make sure that message content is encrypted when EAR is enabled | ||
| func test_ExistingMessageContentIsEncrypted_WhenEarIsEnabled() throws { | ||
| // Given | ||
| uiMOC.encryptMessagesAtRest = false | ||
| uiMOC.databaseKey = nil | ||
|
|
||
| let conversation = createConversation(in: uiMOC) | ||
| try conversation.appendText(content: "Beep bloop") | ||
|
|
||
| let results: [ZMGenericMessageData] = try uiMOC.fetchObjects() | ||
|
|
||
| guard let messageData = results.first else { | ||
| XCTFail("Could not find message data.") | ||
| return | ||
| } | ||
|
|
||
| // Then | ||
| XCTAssertFalse(messageData.isEncrypted) | ||
| XCTAssertEqual(messageData.unencryptedContent, "Beep bloop") | ||
| XCTAssertFalse(uiMOC.encryptMessagesAtRest) | ||
|
|
||
| // Mock | ||
| mockKeyGeneration() | ||
|
|
||
| // When | ||
| XCTAssertNoThrow(try sut.enableEncryptionAtRest(context: uiMOC)) | ||
|
|
||
| // Then migration was run | ||
| XCTAssertEqual(prepareForMigrationCalls, 1) | ||
| XCTAssertTrue(messageData.isEncrypted) | ||
| XCTAssertEqual(messageData.unencryptedContent, "Beep bloop") | ||
|
|
||
| // Then EAR is enabled on the context | ||
| XCTAssertTrue(uiMOC.encryptMessagesAtRest) | ||
| } | ||
|
|
||
| // @SF.Storage @TSFI.FS-IOS @TSFI.Enclave-IOS @S0.1 @S0.2 | ||
| // Make sure that message content normalized for text search is also encrypted when EAR is enabled | ||
| func test_NormalizedMessageContentIsCleared_WhenEarIsEnabled() throws { | ||
| // Given | ||
| uiMOC.encryptMessagesAtRest = false | ||
| uiMOC.databaseKey = nil | ||
|
|
||
| let conversation = createConversation(in: uiMOC) | ||
| let message = try conversation.appendText(content: "Beep bloop") as! ZMMessage | ||
|
|
||
| // Then | ||
| XCTAssertNotNil(message.normalizedText) | ||
| XCTAssertEqual(message.normalizedText?.isEmpty, false) | ||
|
|
||
| // Mock | ||
| mockKeyGeneration() | ||
|
|
||
| // When | ||
| XCTAssertNoThrow(try sut.enableEncryptionAtRest(context: uiMOC)) | ||
|
|
||
| // Then | ||
| XCTAssertNotNil(message.normalizedText) | ||
| XCTAssertEqual(message.normalizedText?.isEmpty, true) | ||
| XCTAssertTrue(uiMOC.encryptMessagesAtRest) | ||
| } | ||
|
|
||
| // @SF.Storage @TSFI.FS-IOS @TSFI.Enclave-IOS @S0.1 @S0.2 | ||
| // Make sure that message content that is drafted but not send by the user yet is also encrypted | ||
| // when EAR is enabled | ||
| func test_DraftMessageContentIsEncrypted_WhenEarIsEnabled() throws { | ||
| // Given | ||
| uiMOC.encryptMessagesAtRest = false | ||
| uiMOC.databaseKey = nil | ||
|
|
||
| let conversation = createConversation(in: uiMOC) | ||
| conversation.draftMessage = DraftMessage( | ||
| text: "Beep bloop", | ||
| mentions: [], | ||
| quote: nil | ||
| ) | ||
|
|
||
| // Then | ||
| XCTAssertTrue(conversation.hasDraftMessage) | ||
| XCTAssertFalse(conversation.hasEncryptedDraftMessageData) | ||
| XCTAssertEqual(conversation.unencryptedDraftMessageContent, "Beep bloop") | ||
|
|
||
| // Mock | ||
| mockKeyGeneration() | ||
|
|
||
| // When | ||
| XCTAssertNoThrow(try sut.enableEncryptionAtRest(context: uiMOC)) | ||
|
|
||
| // Then | ||
| XCTAssertTrue(conversation.hasEncryptedDraftMessageData) | ||
| XCTAssertEqual(conversation.unencryptedDraftMessageContent, "Beep bloop") | ||
| XCTAssertTrue(uiMOC.encryptMessagesAtRest) | ||
| } |
There was a problem hiding this comment.
Moved these security tests to EARServiceIntegrationTests
|
|
||
| // MARK: - Security tests | ||
|
|
||
| // @SF.Storage @TSFI.FS-IOS @TSFI.Enclave-IOS @S0.1 @S0.2 |
There was a problem hiding this comment.
moved to EARServiceIntegrationTests
| XCTAssertEqual(keyRepository.deleteDatabaseKeyDescription_Invocations.count, 1) | ||
| } | ||
|
|
||
| // @SF.Storage @TSFI.UserInterface @S0.1 @S0.2 |
There was a problem hiding this comment.
moved to EARServiceIntegrationTests
| import WireDataModelSupport | ||
| @testable import WireSyncEngine | ||
|
|
||
| // TODO: [WPB-23474] Fix the tests setup |
There was a problem hiding this comment.
This test class is skipped temporarily
netbe
left a comment
There was a problem hiding this comment.
left a couple questions before approving
wire-ios-data-model/Source/Authentication/EAR/EARMessageEncryptionService.swift
Outdated
Show resolved
Hide resolved
wire-ios-data-model/Source/Authentication/EAR/EARMessageEncryptionService.swift
Show resolved
Hide resolved
| lock.unlock() | ||
| return cached | ||
| } | ||
|
|
||
| defer { | ||
| lock.unlock() | ||
| } |
There was a problem hiding this comment.
question: can't we just do defer { lock.unlock() from the top?
| #endif | ||
|
|
||
| // Set the EARMessageEncryptionService on the new background context | ||
| context.performAndWait { |
There was a problem hiding this comment.
suggestion: what about making the method async
There was a problem hiding this comment.
My intuition is that since this context has just been created and not returned, in no place else could there be a simultaneous performAndWait which could lead to a deadlock.
| // MARK: - Security Tests | ||
|
|
||
| // @SF.Storage @TSFI.FS-IOS @TSFI.Enclave-IOS @S0.1 @S0.2 | ||
| func test_ItStoresAndClearsDatabaseKeyOnAllContexts() async throws { |
There was a problem hiding this comment.
todo: check the security tests is enabled the Security Tests plan
| keyRepository: EARKeyRepository(), // Real keychain access | ||
| keyEncryptor: EARKeyEncryptor(), // Real crypto |
There was a problem hiding this comment.
question: why do we need real objects here?
There was a problem hiding this comment.
They're used to generate and store real keys. The test is verifying that the service is replacing keys after disabling / enabling EAR.
johnxnguyen
left a comment
There was a problem hiding this comment.
Looks good! I'll approve when the current conversations are resolved. Also, are we able to playtest these changes as is or is there something missing?
wire-ios-data-model/Source/Authentication/EAR/EARMessageEncryptionService.swift
Show resolved
Hide resolved
|
|
||
| let contextData = try context.earContextData() | ||
|
|
||
| context.saveOrRollback() |
There was a problem hiding this comment.
suggestion: a comment explaining this save would be nice.
There was a problem hiding this comment.
TBH I'm not sure why it's there. I've moved this part of the code from the NSManagedObjecContext extension.
| #endif | ||
|
|
||
| // Set the EARMessageEncryptionService on the new background context | ||
| context.performAndWait { |
There was a problem hiding this comment.
My intuition is that since this context has just been created and not returned, in no place else could there be a simultaneous performAndWait which could lead to a deadlock.
@johnxnguyen besides incremental sync support coming in a separate PR, there's nothing missing and it can be playtested |
wire-ios-sync-engine/Source/SessionManager/SessionManager+EncryptionAtRest.swift
Show resolved
Hide resolved
| do { | ||
| return try moc.encryptData(data: data) | ||
| } catch let error as NSManagedObjectContext.EncryptionError { | ||
| let service = try moc.getEarMessageEncryptionService() |
There was a problem hiding this comment.
getEarMessageEncryptionService() will throw if this context wasn’t wired with the EAR service. Any ad‑hoc context that skips setEARMessageEncryptionService will now fail encryption at runtime when EAR is enabled - can we enforce injection or assert here?
| guard moc.encryptMessagesAtRest else { return (data, nonce: nil) } | ||
| return try moc.encryptData(data: data) | ||
|
|
||
| let service = try moc.getEarMessageEncryptionService() |
There was a problem hiding this comment.
Same as above: this assumes earMessageEncryptionService is injected into the context. If any context is created outside CoreDataStack (or before injection), draft encryption will throw - can we assert/guard or centralize context creation to guarantee injection?
There was a problem hiding this comment.
same as above, I added an assertion. Additionally, the context creation is centralized through CoreDataStack. Although there's nothing restricting from creating a context differently
wire-ios-data-model/Source/Authentication/EAR/EARMessageEncryptionService.swift
Show resolved
Hide resolved
wire-ios-data-model/Source/ManagedObjectContext/NSManagedObjectContext+EncryptionAtRest.swift
Show resolved
Hide resolved
wire-ios-data-model/Source/Authentication/EAR/EARMessageEncryptionService.swift
Show resolved
Hide resolved
|
| "skippedTests" : [ | ||
| "AvailabilityTests", | ||
| "EventProcessingPerformanceTests", | ||
| "SessionManagerEncryptionAtRestMigrationTests", |
There was a problem hiding this comment.
question: why is it skipped ?
There was a problem hiding this comment.
disabled tests will be re-enabled in another PR I guess



Issue
This PR re-establishes Encryption at Rest after it was broken by the introduction of dynamic background contexts. The implementation decouples encryption logic from
NSManagedObjectContextand centralizes database key management through a service-based architecture.The previous EAR implementation had critical issues:
CoreDataStack.newBackgroundContext()lacked the database key, causing decryption errors and disappearing messagesChanges
EARMessageEncryptionServicefor database key management and centralized encryptionEARMigratorfor migrations when enabling / disabling EAREARServiceinitialization asyncEARServiceTestsandEARServiceIntegrationTestsChecklist
[WPB-XXX].