Page MenuHomePhabricator
Differential D12750

Implement inbound notif session creation from the NSE with race condition handling
ClosedPublic
Actions

Authored by marcin on Jul 15 2024, 5:18 AM.
Tags
None
Referenced Files
F3197531: D12750.id42946.diff
Sat, Nov 9, 8:52 AM
F3197361: D12750.id42290.diff
Sat, Nov 9, 8:17 AM
Unknown Object (File)
Fri, Nov 8, 2:16 AM
Unknown Object (File)
Tue, Oct 22, 1:16 PM
Unknown Object (File)
Tue, Oct 22, 9:08 AM
Unknown Object (File)
Tue, Oct 22, 8:33 AM
Unknown Object (File)
Tue, Oct 22, 8:33 AM
Unknown Object (File)
Tue, Oct 22, 8:33 AM
View All 82 Files
Subscribers
ashoat

Details

Reviewers
tomek
kamil
bartek
Commits
rCOMM29b725af6115: Implement inbound notif session creation from the NSE with race condition…
Summary

This differential implements inbound notification session creation with race condition handling. The NSE code is modified to use this mechanism when receiving E2EE notification from peer. Android part in CommNotificationsHandler.java will be added shortly after Tunnelbroker <-> FCM diff is out.

Test Plan
  1. Apply this patch: https://gist.github.com/marcinwasowicz/8bdc755abad8ecf81f3bd01c6ca34376
  2. Send notification from web to iOS device.
  3. Ensure that each notification results in two identical notification being displayed. One is from the keyserver. The other is from tunnelbroker and is E2EE encrypted.
  4. I also added some logs in NotificationsCryptoModule so that it is logged whether we create new session or use existing one. I only saw the first type of log once.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

marcin created this revision.Jul 15 2024, 5:18 AM
Herald added a subscriber: ashoat. · View Herald TranscriptJul 15 2024, 5:18 AM
marcin added a parent revision: D12748: Expose function to get notifs inbound keys to C++.Jul 15 2024, 5:19 AM
marcin added 1 blocking reviewer(s): kamil.
marcin added inline comments.Jul 15 2024, 5:22 AM
native/cpp/CommonCpp/Notifications/BackgroundDataStorage/NotificationsCryptoModule.h
114 ↗(On Diff #42290)

When we create inbound session from notification we need to return updated notif account state as well since one time key is removed from the account.

147 ↗(On Diff #42290)

When we win race condition we create an emphemeral one time session to decrypt single message but we don't persist anything. The StatefulPeerConflictDecryptResult implementation of flushState is a np-op.

marcin edited the test plan for this revision. (Show Details)Jul 15 2024, 5:28 AM
Harbormaster returned this revision to the author for changes because remote builds failed.Jul 15 2024, 5:42 AM
Harbormaster failed remote builds in B30328: Diff 42290!
Harbormaster completed remote builds in B30328: Diff 42290.Jul 15 2024, 6:02 AM
marcin requested review of this revision.Jul 15 2024, 6:02 AM
marcin added inline comments.Jul 15 2024, 7:57 AM
native/cpp/CommonCpp/Notifications/BackgroundDataStorage/NotificationsCryptoModule.cpp
613 ↗(On Diff #42290)

We use this way of checking olm internals: https://github.com/CommE2E/comm/blob/6583cde4a78d2981d65d9ec16256f62225af58fb/native/cpp/CommonCpp/CryptoTools/CryptoModule.cpp#L169-L171

marcin added inline comments.Jul 15 2024, 8:26 AM
native/cpp/CommonCpp/Notifications/BackgroundDataStorage/NotificationsCryptoModule.cpp
709 ↗(On Diff #42290)

Decryption with immediate state persistence is used on Android.

marcin added a child revision: D12837: Bump olm version.Jul 22 2024, 7:16 AM
marcin updated this revision to Diff 42717.Jul 23 2024, 11:07 AM

Use common C++ implementation on Android. It was tested the same way as iOS using latest staging Tunnelbroker release

Harbormaster completed remote builds in B30622: Diff 42717.Jul 23 2024, 11:27 AM
marcin mentioned this in D12668: Implement peer notifications session creation as outbound on native.Jul 24 2024, 3:00 AM
kamil accepted this revision.Jul 25 2024, 7:46 AM
kamil added inline comments.
native/cpp/CommonCpp/CryptoTools/Session.cpp
86–91 ↗(On Diff #42717)

this is huge change, I would create a separate diff to test it separately but I think it's okay

This revision is now accepted and ready to land.Jul 25 2024, 7:46 AM
marcin updated this revision to Diff 42946.Jul 30 2024, 3:28 AM

Update phab

Harbormaster completed remote builds in B30801: Diff 42946.Jul 30 2024, 5:16 AM
marcin updated this revision to Diff 43084.Aug 5 2024, 4:18 AM

Rebase before landing

Harbormaster completed remote builds in B30904: Diff 43084.Aug 5 2024, 6:15 AM
Closed by commit rCOMM29b725af6115: Implement inbound notif session creation from the NSE with race condition… (authored by marcin, committed by Marcin Wąsowicz <marcinwasowicz@MacBook-Pro-Marcin.local>). · Explain WhyAug 5 2024, 6:40 AM
This revision was automatically updated to reflect the committed changes.
Marcin Wąsowicz <marcinwasowicz@MacBook-Pro-Marcin.local> added a commit: rCOMM29b725af6115: Implement inbound notif session creation from the NSE with race condition….

Revision Contents
Changeset List

PathSize
native/
android/
app/
src/
cpp/
12 lines
main/
java/
app/
comm/
android/
fbjni/
2 lines
notifications/
46 lines
cpp/
CommonCpp/
CryptoTools/
4 lines
Notifications/
BackgroundDataStorage/
53 lines
190 lines
6 lines
ios/
NotificationService/
31 lines

Diff 43142

View Options

native/android/app/src/cpp/NotificationsCryptoModuleJNIHelper.cpp

Loading...
View Options

native/android/app/src/main/java/app/comm/android/fbjni/NotificationsCryptoModule.java

Loading...
View Options

native/android/app/src/main/java/app/comm/android/notifications/CommNotificationsHandler.java

Loading...
View Options

native/cpp/CommonCpp/CryptoTools/Session.cpp

Loading...
View Options

native/cpp/CommonCpp/Notifications/BackgroundDataStorage/NotificationsCryptoModule.h

Loading...
View Options

native/cpp/CommonCpp/Notifications/BackgroundDataStorage/NotificationsCryptoModule.cpp

Loading...
View Options

native/cpp/CommonCpp/Notifications/BackgroundDataStorage/NotificationsCryptoModuleJNIHelper.h

Loading...
View Options

native/ios/NotificationService/NotificationService.mm

Loading...