Page MenuHomePhabricator

Encrypt Android notifications on the keyserver before sending them
ClosedPublic

Authored by marcin on Fri, May 19, 7:17 AM.

Details

Summary

This differential implements Android notification encryption before sending them.

Test Plan

Send notifications to Android device. On each notification issue query select * from olm_sessions;. Ensure that version column of session associated with current user increments its value on each
notification.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

keyserver/src/push/send.js
834–860 ↗(On Diff #26671)

Same feedback as on iOS diffs – we should merge these so we only have to do a single pass

tomek requested changes to this revision.Mon, May 22, 2:55 AM
tomek added inline comments.
keyserver/src/push/send.js
791 ↗(On Diff #26671)

Why do we check if codeVersion > 0?

826–827 ↗(On Diff #26671)

Is it a good idea to encrypt both of these? Is the session state affected by that?

keyserver/src/push/utils.js
137–143 ↗(On Diff #26671)

Is it guaranteed that device tokes match notifications? Can we make the API safer by merging these into one object with deviceToken and notification fields? (and add an array of these as a parameter)

keyserver/src/session/cookies.js
834 ↗(On Diff #26671)

Why 10?

This revision now requires changes to proceed.Mon, May 22, 2:55 AM
keyserver/src/push/send.js
791 ↗(On Diff #26671)

This is an artifact from debugging. I forgot to set it to FUTURE_CODE_VERSION.

keyserver/src/push/utils.js
137–143 ↗(On Diff #26671)

Is it guaranteed that device tokes match notifications?

It is guaranted since:

  1. Function prepareEncryptedIOSNotifications returns notifications that are encrypted in the order of cookieIDs array that is passed to this function.
  2. Array cookieIDs matches device tokens since it is constructed by iterating over rows from the db. Each row that we iterate contains one cookieID and related device token.

Can we make the API safer by merging these into one object with deviceToken and notification fields? (and add an array of these as a parameter)

I think we can

keyserver/src/session/cookies.js
834 ↗(On Diff #26671)

Artifact from debugging. I forgot to set it to FUTURE_CODE_VERSION.

keyserver/src/push/send.js
826–827 ↗(On Diff #26671)

Is it a good idea to encrypt both of these?

We must encrypt both since it is the only way to get to know if adding messageInfos does not overflow fcm push limit.

Is the session state affected by that?

It is but not in a dangerous way. Only one of those will be sent to client, and the other can be viewed as skipped messages. The client can handle skipped messages by advancing its ratchet keys. Every ratchet-encrypted message contains a header with the counter, so that the recipent can advance their keys if they received messages no. 1, 2 and 5 but didn't receive message 3 and 4. It can therefore decrypt 5 when it arrives and 3, 4 in case they happen to arrive later.

  1. Use FUTURE_CODE_VERSION in everu codeVersion comparison
  2. Ona pass instead of three when evaluating size of notification with message infos.
  3. Change types and refactor so that fcmPush taks an array of pairs of devicetokens and notifications, so that we are sure notifications are sent to the device token associated with cookie id they were encrypted with
ashoat requested changes to this revision.Tue, May 23, 7:26 AM
ashoat added inline comments.
keyserver/src/push/send.js
245 ↗(On Diff #26869)

This diff appears to depend on an updated version of getDevicesByPlatform which is implemented in D7815. However, D7815 is not listed as a dependency of this diff – instead, it appears later in the stack.

Can you update the diff stack to reflect the state of your local git branch?

266–270 ↗(On Diff #26869)

Shorthand

268 ↗(On Diff #26869)

We appear to rely on the assumption that cookieIDs and deviceTokens will have the same length, but I'm not sure that this is guaranteed in D7815. If for some reason two cookieIDs exist for the same deviceToken (should not be possible), this assumption will be broken.

Furthermore, I could imagine a future developer updating getDevicesByPlatform and breaking this assumption. The assumption is not clear in the code.

We have two options here:

  1. Enforce the assumption here with an invariant.
  2. Probably better: update getDevicesByPlatform to avoid this assumption. In particular, I think we could replace the following part of the return type:
{ cookieIDs: Set<string>, deviceTokens: Set<string> }

With the following:

Array<{ cookieID: string, deviceToken: string }>
1271–1277 ↗(On Diff #26869)

await is an important keyword, and we should always be clear about it, and make sure it's visible. We always make sure that await appears as the first keyword on the right side of an assigment, eg.:

const leftSide = await rightSide;

We can rewrite this code to follow this rule.

1273 ↗(On Diff #26869)

Your use of await here is very bad. You're using it inside a loop, forcing every single invocation of the loop to await one-at-a-time. This has come up before – I urge you to make sure you understand async / await and Promises going forward, so that you are able to write performant async code without needing reviewers to rewrite it for you

This revision now requires changes to proceed.Tue, May 23, 7:26 AM

Refactor to create delivery promise from android badhe notifications encryption and notification transport

(don't have much context here, feel free to re-add me if there's anything specific I can be helpful w/)

tomek requested changes to this revision.Thu, May 25, 8:03 AM
tomek added inline comments.
keyserver/src/push/send.js
776 ↗(On Diff #26980)

We should probably prefer $ReadOnlyArray. If we need mutability, we can use a shorthand AndroidNotification[]

864 ↗(On Diff #26980)

This is really strange. We're iterating over cookies but not use them. Is it a mistake? We're returning the same notif multiple times.

This revision now requires changes to proceed.Thu, May 25, 8:03 AM
ashoat requested changes to this revision.Fri, May 26, 8:59 AM
ashoat removed a reviewer: tomek.

Taking over for @tomek on the review, since he is on vacation now. Thought about his feedback and have a proposed approach – @marcin, what do you think?

keyserver/src/push/send.js
864 ↗(On Diff #26980)

I thought this was weird on my initial read too, but after looking at it more closely I think it's correct.

The caller of prepareAndroidNotification expects it to return one result for each cookieID. This is because it later pairs each notification with the corresponding deviceToken for that cookieID based on the notification's order in the array.

This is arguably a brittle design. We could reconsider the approach, and instead pass the devices array (containing pairs of deviceTokens and cookieIDs) into prepareAndroidNotification, instead of just the cookieIDs. Then we could make sure that this function returns the format we want (notifications paired with deviceTokens).

I think that design would be more readable (less weird lines like the one @tomek pointed out), and less brittle (if a future developer changes something, they are less likely to break an assumption).

@marcin, what do you think? If we make this change, we should make it for both iOS and Android, assuming it applies for both.

marcin added inline comments.
keyserver/src/push/send.js
776 ↗(On Diff #26980)

Sure - I will replace it with $ReadOnlyArray. Mutability is not necessary here.

864 ↗(On Diff #26980)

We're iterating over cookies but not use them. Is it a mistake? We're returning the same notif multiple times.

Looking at changes made in this differential it can be concluded that this is intentional. It was introduced in order to meet requirements of new typing. New typing here is that send{APNs/Android}Notification requires the array of pairs of deviceToken and notification to ensure that each encrypted notification is delivered to the particular deviceToken associated with cookieID used to encrypt notif (olm session related to this cookieID, to be precise). Therefore in order to meet this typing we have to repeat the same notification for each deviceToken in case notification is not encrypted.

I think that design would be more readable (less weird lines like the one @tomek pointed out), and less brittle (if a future developer changes something, they are less likely to break an assumption).

I agree that this code might be of better quality and change suggested by @ashoat should be made.

However the fact that this snippet of code was enough to the reviewer to request changes is concerning for me. Looking at this differential holistically was enough to figure out that it is not a mistake, but rather a consequence of types we agreed on. Additionally, it is worth to note that this differential already had many cycles during which we detected and addressed much more serious issues. It could be a flag for the reviewer that it is unlikely there is a mistake here.

The fact that code quality is not perfect here could well be handled by a simple comment with suggestion on how to beautify this code. This team has a strong culture of not landing a diff with unaddressed comments. We should be taking advantage of this good convention to speed-up the process of delivering features where we know the code was tested and works correctly but there are some minor issues left about code quality.

@marcin, what do you think? If we make this change, we should make it for both iOS and Android, assuming it applies for both.

I agree we shouldn't leave discrepancy between platforms and apply new approach to both. That said I will introduce those changes to iOS diff. However I would pursue landing the entire stack, apart from this diff, once I am back to work on Monday in order not to block entire goal, other people who might be interested in some parts of this stack and my future work regarding e2e rescinds and large notif download.

keyserver/src/push/send.js
864 ↗(On Diff #26980)

I think I should add, that I expressed my concerns before I noticed there has been a change in reviewers and believed the stack will be blocked until June.

Change typing so that notifications are packaged with relevant device token in advance

Rebase before landing. Use real code version

Rebase before landing. Use real code version

Last refactor: Use TargetedAndrooidNotification in fcmPush

This revision is now accepted and ready to land.Mon, May 29, 1:02 PM