[lib] add session version to EncryptedData
ClosedPublic
Actions

Authored by kamil on Aug 8 2024, 8:50 AM.

Details

Reviewers

tomek
marcin

Commits

rCOMM28edf256514a: [lib] add session version to EncryptedData

Summary

ENG-6982.

When there are multiple messages queued for the device, and we start resetting the session the messages already queued will for sure fail to be decrypted - to distinguish that and avoid resetting the session multiple times we need to add the session version to each message. This is optional to backward compatibility and to avoid adding this for notif when this is not needed.

Depends on D13032

Test Plan

Create a session between devices A and B
On B comment out sending confirmation.
Send some messages from A to B
Receive couple of messages from B
Reset session
Make sure other messages throws INVALID_SESSION_VERSION error when decrypting

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

kamil created this revision.Aug 8 2024, 8:50 AM

kamil held this revision as a draft.

Herald added a subscriber: ashoat. · View Herald TranscriptAug 8 2024, 8:50 AM

Harbormaster completed remote builds in B31015: Diff 43264.Aug 8 2024, 9:07 AM

update

kamil edited the summary of this revision. (Show Details)Aug 12 2024, 2:36 AM

kamil edited the test plan for this revision. (Show Details)

kamil added a reviewer: marcin.Aug 12 2024, 2:39 AM

kamil published this revision for review.Aug 12 2024, 2:47 AM

kamil added a child revision: D13046: [lib] reset session and resend message when device is unable to decrypt messages.Aug 12 2024, 2:49 AM

Harbormaster completed remote builds in B31039: Diff 43303.Aug 12 2024, 2:51 AM

tomek accepted this revision.Mon, Aug 19, 5:34 AM

tomek added inline comments.

native/cpp/CommonCpp/CryptoTools/CryptoModule.cpp
402 ↗	(On Diff #43303)	Shouldn't we check for equality?
native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
1850 ↗	(On Diff #43303)	Why do we round?

marcin accepted this revision.Tue, Aug 20, 2:05 AM

This revision is now accepted and ready to land.Tue, Aug 20, 2:05 AM

kamil added inline comments.Tue, Aug 20, 3:48 AM

native/cpp/CommonCpp/CryptoTools/CryptoModule.cpp
402 ↗	(On Diff #43303)	This is on purpose: `encryptedData.sessionVersion == session->getVersion()` -> The same session, we should be able to decrypt `encryptedData.sessionVersion < session->getVersion()` -> We have a newer session, we should ignore this message, that's why we're throwing a specific error to ignore in JS `encryptedData.sessionVersion > session->getVersion()` -> We have an older session, this is a very rare edge case when there was a session reset from the other client but the session init message was not delivered, in that case, we want to throw an error to heal session state.
native/cpp/CommonCpp/NativeModules/CommCoreModule.cpp
1850 ↗	(On Diff #43303)	we do it always for numbers because JSI uses `double` when passing numbers from JS

ashoat added inline comments.Tue, Aug 20, 12:59 PM

native/cpp/CommonCpp/CryptoTools/CryptoModule.cpp
402 ↗	(On Diff #43303)	I'm confused... it seems like you're saying that for both the `<` and `>` case, we should throw an error... but we're only throwing an error here for the `<` case, no?

kamil added inline comments.Wed, Aug 21, 2:27 AM

native/cpp/CommonCpp/CryptoTools/CryptoModule.cpp
402 ↗	(On Diff #43303)	For case `<` we should throw specific error, other than Olm's error to ignore this fact and avoid resetting the session. For case `>` we should keep the default behavior which is decrypt, or on Olm error reset session. We're not throwing an error explicitly but Olm will do it (`OLM_BAD_MESSAGE_MAC`). If you think it's better to introduce another specific error - not ignored but causing resetting session like internal Olm errors I can do it as a follow-up, the logic will remain unchanged, this is just about code readability/avoiding calling the `decrypt` method.