feat: support EAR with incremental sync - WPB-17302#4310
feat: support EAR with incremental sync - WPB-17302#4310David-Henner wants to merge 43 commits intofeat/ear-support-without-new-syncfrom
Conversation
Test Results2 tests 2 ✅ 0s ⏱️ Results for commit e0296f0. ♻️ This comment has been updated with latest results. Summary: workflow run #22235654623 |
There was a problem hiding this comment.
Pull request overview
This pull request adds comprehensive support for Encryption at Rest (EAR) with incremental synchronization. The implementation encrypts update events when storing them in the events database and decrypts them when fetching for processing. The key innovation is using dual encryption keys (primary and secondary) to enable background processing of calling events while maintaining security for other event types.
Changes:
- Core Data schema upgraded to v7.0 with
isEncryptedandisBackgroundAccessibleflags for event envelopes - IncrementalSync enhanced with database unlock waiting mechanism and encryption key management
- UpdateEventsLocalStore modified to encrypt events on write and decrypt on read using appropriate keys
- New helper files for encryption operations and protobuf message decoding
- SelfPostingNotification protocol and DatabaseEncryptionLockNotification moved to WireDataModel for shared access
Reviewed changes
Copilot reviewed 39 out of 41 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| IncrementalSync.swift | Added database unlock waiting logic, key fetching, and passing encryption keys through sync pipeline |
| UpdateEventsLocalStore.swift | Implemented encryption on persist and decryption on fetch with primary/secondary key selection |
| UpdateEventEnvelope+BackgroundAccessible.swift | Logic to determine if events contain calling content for secondary key usage |
| EAREncryptionHelper.swift | Wrapper functions for SecKey encryption/decryption operations |
| ProtobufMessageDecoder.swift | Helper to decode and validate protobuf messages for event type detection |
| StoredUpdateEventEnvelope.swift | Added isEncrypted and isBackgroundAccessible Core Data attributes |
| ZMEventModel7.0 | New Core Data schema version with encryption-related attributes |
| UpdateEventMigrator.swift | Updated to fetch and use private keys during legacy event migration |
| NSEClientScope.swift | Modified to fetch public keys for encrypting events pulled in NSE |
| Multiple test files | Updated mocks and added comprehensive tests for encryption/decryption scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
WireDomain/Sources/WireDomain/Repositories/UpdateEvents/UpdateEventsLocalStore.swift
Outdated
Show resolved
Hide resolved
.../Sources/WireDomain/Repositories/UpdateEvents/UpdateEventEnvelope+BackgroundAccessible.swift
Show resolved
Hide resolved
WireDomain/Sources/WireDomain/Notifications/Components/NSEClientScope.swift
Show resolved
Hide resolved
WireDomain/Sources/WireDomain/Helpers/EAREncryptionHelper.swift
Outdated
Show resolved
Hide resolved
netbe
left a comment
There was a problem hiding this comment.
left some comments, the main point I am a bit fuzzy about is when DatabaseEncryptionLockNotification is posted, and if we could use publisher instead of notification
| accountID: accountID, | ||
| coreDataStack: coreDataStack, | ||
| sharedUserDefaults: dependency.sharedUserDefaults, | ||
| authenticationContext: AuthenticationContext(storage: LAContextStorage()) |
There was a problem hiding this comment.
question: isn't the LAContextStorage() requiring a biometric prompt?
There was a problem hiding this comment.
it's LAContext() that does IIRC
| enum Error: Swift.Error { | ||
| case failedToFetchStoredEvents(Swift.Error) | ||
| case failedToDeleteStoredEvents(Swift.Error) | ||
| case failedToEncryptEventData |
There was a problem hiding this comment.
suggestion: add associated error?
| data: data, | ||
| privateKey: key | ||
| ) else { | ||
| WireLogger.ear.error("failed to decrypt stored event", attributes: .safePublic) |
There was a problem hiding this comment.
question: can we log the eventId as envelope_event_id attribute?
There was a problem hiding this comment.
not available on the stored event
| storedEventEnvelope.isEncrypted = true | ||
| storedEventEnvelope.isBackgroundAccessible = isBackgroundAccessible | ||
| } else { | ||
| WireLogger.ear.error("failed to encrypt event", attributes: .safePublic) |
There was a problem hiding this comment.
suggestion: add event id in attributes if possible
| "timed out waiting for database unlock, cancelling sync", | ||
| attributes: .incrementalSyncV2, .safePublic | ||
| ) | ||
| throw Failure.databaseUnlockTimeout |
There was a problem hiding this comment.
question: is the failure propagated?
does it mean that if it timesout? the incremental sync is cancelled?
why would it timeout?
…ew-sync # Conflicts: # wire-ios-data-model/Source/Authentication/EAR/EARService.swift
| import Foundation | ||
| import GenericMessageProtocol | ||
|
|
||
| struct ProtobufMessageDecoder { |
There was a problem hiding this comment.
question: it looks like this was restored to its original implementation, but changes were made recently to throw errors. I think we should preserve these changes.
| "failed to decrypt stored event: no private keys", | ||
| attributes: .safePublic | ||
| ) | ||
| return nil |
There was a problem hiding this comment.
issue: returning nil here means we just skip over this event and will proceed to process the rest. This could lead to some unexpected issues given the assumption that events are processed in order. What do you think about throwing an error and aborting? Perhaps this comes with other issues. what do you think @netbe ?
There was a problem hiding this comment.
added critical logs and threw errors 6a2e408
| } | ||
|
|
||
| func perform(appState: UIApplication.State) async throws -> Token { | ||
| let inBackground = appState == .background |
There was a problem hiding this comment.
question: why do we need to check if we're in the BG?
There was a problem hiding this comment.
made an attempt to update the flow as discussed 65fd560
| publicKeys: EARPublicKeys?, | ||
| backgroundAccessibleOnly: Bool | ||
| ) async { | ||
| guard !backgroundAccessibleOnly || (backgroundAccessibleOnly && envelope.isBackgroundAccessible) else { |
There was a problem hiding this comment.
question: why do we abort if we get a live event that is not background accessible? To avoid processing it before we processed earlier stored events?
WireDomain/Sources/WireDomain/Synchronization/IncrementalSync.swift
Outdated
Show resolved
Hide resolved
| let isLocked = earService.isLocked | ||
| let privateKeys = try earService.fetchPrivateKeys(includingPrimary: !isLocked) |
There was a problem hiding this comment.
question: so we will attempt to migrate event if the database is locked? I would expect that a prerequisite would be to have an unlocked database.
WireDomain/Tests/WireDomainTests/LocalStores/UpdateEventsLocalStoreTests.swift
Show resolved
Hide resolved
…ounding with an active call
|



Issue
This PR add support for EAR with the incremental sync. This means we encrypt the update events when saving them in the events database, and decrypt them when fetching them for processing.
Most of the events can be accessible only when the app is unlocked. However, calling events need to be processed while in the background, so their access level is less restrictive and can be encrypted / decrypted without the app being unlocked. This is why we have primary and secondary encryption keys.
Changes
IncrementalSync
earServiceandnotificationContextdependenciesNew:waitForDatabaseUnlock()method with 3-second timeoutbackgroundAccessibleOnlyto live & stored event processingUpdateEventsLocalStore
persistEventEnvelope):publicKeysparameterisEncryptedandisBackgroundAccessiblepropertiesfetchStoredEventEnvelopes):privateKeysandbackgroundAccessibleOnlyparametersCore Data
StoredUpdateEventEnvelopeentity now includes:isEncrypted: indicates if event data is encryptedisBackgroundAccessible: indicates if the event should be accessible in the background (for calling events)New Files Added
EAREncryptionHelper.swift- Encryption/decryption utilities for event dataProtobufMessageDecoder.swift- Helper for decoding protobuf messagesUpdateEventEnvelope+BackgroundAccessible.swift- Extension determining if events are calling-relatedDatabaseEncryptionLockNotification.swift- Notification type for database lock/unlock events (moved from sync-engine)Testing
Settings > Account > Encrypt messages at rest- enableChecklist
[WPB-XXX].