Add end-to-end encryption for push notifications #15229

hashirsarwar · 2020-06-05T20:48:31Z

Closes #6954.

timabbott · 2020-06-05T23:10:19Z

So, this is a fine start to the encryption part, but we first need to change the data model, likely in a separate PR, to create a PushDeviceToken locally when using the push notification bouncer (the current system only creates a RemotePushDeviceToken on the remove server). There's no point in doing further work on this until we've integrated that change (as discussed in the old PR thread).

Hypro999

Hey @hashirsarwar I'm leaving a few change requests for the first few commits right now so we can keep this project moving. I'll add some follow up reviews on the second half of commits, the tests, etc. soon but separately since they will be a bit heavier (I don't want to suggest too many changes at once).

zerver/lib/encryption.py

zerver/tests/test_encryption.py

zerver/lib/encryption.py

zerver/lib/push_notifications.py

Hypro999

I left just a few small change requests - nothing major. What I think we should do now is:

Finish discussing the details of the refactor I mentioned (in PMs)
~~Actually perform the refactor.~~ (if you're not comfortable with it then fine, stick to whatever is personally comfortable)
Look at the failing test and get that fixed.
4.Give this PR one more round of review and look more carefully at the tests you've added.
Get Tim Abbott and Greg Price involved for some rounds of final review.
Get this merged.

zerver/tests/test_push_notifications.py

zerver/lib/push_notifications.py

Hypro999

I left a few small change requests but otherwise this is pretty much ready for review by others, so I'm going to go ahead and approve it. There's still the issue of deciding if the discussed refactor needs to be made or not. But that's not going to get resolved until after the upcoming release AFAIK. So we'll wait until the release is over then get Tim Abbott and Greg Price to review this.

zerver/tests/test_push_notifications.py

gnprice

Thanks @hashirsarwar . Discussion in chat, and a few comments below.

zilencer/views.py

zerver/lib/push_notifications.py

gnprice

See comments below. Most importantly: this actually leaves much of the data unencrypted.

zerver/lib/encryption.py

gnprice · 2020-09-05T04:42:44Z

zerver/lib/encryption.py

+    # AESGCM.encrypt() expects data in bytes and returns ciphertext
+    # bytes with the 16 byte tag appended.
+    encrypted_data = aesgcm.encrypt(nonce, data.encode('utf-8'), None)
+    return bytes_to_b64(encrypted_data), bytes_to_b64(nonce)


Similarly -- why base64 here?

I'd prefer to handle things in their natural form, and encode as base64 or whatever only where needed.

We also need to send this information i.e. encrypted_data and nonce to other devices via bouncer. Sending these fields as bytes will cause the following exception when we try to covert encrypted_payloads to JSON format.

TypeError: Type is not JSON serializable: bytes

Am I missing something?

Right. At the moment when we're sending them over the network, they need to be encoded somehow, and base64 is a fine choice there. But the best point to do that encoding is just before we send them over the network, so that inside our code they can have their natural form, which is a plain bytes.

gnprice · 2020-09-05T04:45:30Z

zerver/lib/encryption.py

+        # Make sure the generated key doesn't collide with an existing key.
+        if PushDeviceToken.objects.filter(notification_encryption_key=key_str).count() == 0:
+            return key_str


What's the motivation for this bit?

If we generate the same key twice... something is very badly wrong already.

I think that AESGCM.generate_key can generate the same keys twice though the chances are very little. We are not generating the encryption key through our code, instead we are relying on AESGCM.generate_key to do so.

This change was suggested by @Hypro999 in a resolved conversation -- so he might want to add something here.

I don't really have anything to add here. This was just added as extremely defensive code. We can remove this if we want to, since it involves a DB query for a highly unlikely event.

Yes, let's take this out.

In the absence of a bug in how we generate the key (including in the system's random-number generator which we'll be relying on), the probability of a collision is far smaller than the probability of, say, the system going completely wild due to a hardware failure. Even if there are a billion records here, the probability of a collision is less than 1/2^68. The probability that the machine will take a direct hit from a meteor is several orders of magnitude bigger than that.

If OTOH there is a bug in the key generation... this would not be an effective way to detect that (and the Zulip app in general isn't the right layer to be attempting to defend against such a thing anyway.)

zerver/lib/push_notifications.py

gnprice · 2020-09-05T05:00:33Z

zerver/lib/push_notifications.py

+def send_android_encrypted_push_notification(encrypted_data: EncryptedData,
+                                             options: Dict[str, Any], remote: bool=False) -> None:
+    for device, payload in encrypted_data:
+        send_android_push_notification([device], payload, options, remote)
+


Why a loop here? We discussed this in chat in a previous round. A single request with an encrypted_payloads field would make the code simpler. It would also make it easier to manage the case where a Zulip server for some reason ends up with lots of device records for a given user.

I guess that approach would end up having us send a single large payload, rather than several small ones, in that buggy case? Probably better since there's a high minimum latency, assuming we don't risk hitting a low limit for how large a payload can be.

Ah I see, this isn't in the path leading to the bouncer and is only used when actually talking to FCM. I was thinking of the request the app server makes to the bouncer.

When actually talking to FCM or APNs, we have to make a separate request per distinct notification-message payload anyway, which means one per device when they're encrypted. So the loop here is appropriate -- it should just be inlined rather than being its own function, as mentioned in another comment.

zerver/lib/push_notifications.py

gnprice · 2020-09-05T05:05:20Z

zerver/lib/push_notifications.py

+def send_android_encrypted_push_notification(encrypted_data: EncryptedData,
+                                             options: Dict[str, Any], remote: bool=False) -> None:
+    for device, payload in encrypted_data:
+        send_android_push_notification([device], payload, options, remote)
+


As a matter of style and code clarity, if we do have this loop, the code would be easier to read if it's simply inlined at the one place this function is used -- this function doesn't really add anything. (But as mentioned in a separate comment, we should arrange the internal API so that we don't need this loop at all.)

timabbott · 2020-09-10T00:59:48Z

@hashirsarwar can you also respond individually to the comments I posted? It wasn't clear to me whether your last push is expected to address them.

hashirsarwar · 2020-09-10T07:01:43Z

@hashirsarwar can you also respond individually to the comments I posted? It wasn't clear to me whether your last push is expected to address them.

The last push only addressed one of the comments. I am working on the rest.

gnprice

The discussion here above caused me to look closer at the bouncer side of this and at the device-registration side. Some comments on those below. In particular there are some issues at registration time that are essential to fix.

gnprice · 2020-09-10T07:51:06Z

zerver/lib/push_notifications.py

+def prepare_encrypted_payload(data: EncryptedData) -> EncryptedPayload:
+    payload = []
+    for device, content in data:
+        payload.append((device.token, content))
+
+    return payload


This would be much simpler and clearer as a list comprehension, inlined at its one call site.

gnprice · 2020-09-10T07:51:39Z

zerver/lib/push_notifications.py

+                                                  apns_payload,
+                                                  encrypt_apns_payload)
+
+    encrypted_payloads = prepare_encrypted_payload(encrypted_gcm_data)


It looks like this is completely missing the data for Apple devices.

gnprice · 2020-09-10T08:05:00Z

zilencer/views.py

+def prepare_encrypted_data(devices: List[RemotePushDeviceToken],
+                           payload: EncryptedPayload) -> Tuple[EncryptedData, EncryptedData]:
+    device_map = {d.token: d for d in devices}
+    encrypted_android_data = []
+    encrypted_apple_data = []
+    for token, data in payload:
+        if token in device_map:
+            if device_map[token].kind == RemotePushDeviceToken.GCM:
+                encrypted_android_data.append((device_map[token], data))
+            else:
+                encrypted_apple_data.append((device_map[token], data))
+
+    return encrypted_android_data, encrypted_apple_data


This code can be made a lot simpler by taking out the if device_map[token].kind == … conditional and producing just a single list. Then it can just be a list comprehension, which can just be inlined at the one place it's used.

Then instead of having separate functions send_android_push_notifications and send_apple_push_notifications which are just simple loops that are very similar to each other, those two can be a single loop, with the if device.kind == … conditional inside that loop.

gnprice · 2020-09-10T08:06:34Z

zilencer/views.py

+    encrypted_android_data, encrypted_apple_data = prepare_encrypted_data(android_devices + apple_devices,
+                                                                          encrypted_payloads)
+
+    android_devices = [d for d in android_devices if not d.encrypted]


This new value has a quite different meaning from the old value called android_devices. It should therefore have a different name which expresses that different meaning.

(Same for apple_devices below.)

Alternatively, this list comprehension could go directly in the argument to the function call, rather than give it a name at all.

gnprice · 2020-09-10T08:11:49Z

zilencer/models.py

@@ -32,6 +32,7 @@ class RemotePushDeviceToken(AbstractPushDeviceToken):
    server: RemoteZulipServer = models.ForeignKey(RemoteZulipServer, on_delete=models.CASCADE)
    # The user id on the remote server for this device device this is
    user_id: int = models.BigIntegerField(db_index=True)
+    encrypted: bool = models.BooleanField(default=False)


I don't like this name; it sounds like it's saying the device is encrypted (whatever that would mean).

One good name could be encrypt_notifications -- same as in the request the client makes when registering its token.

Another could be require_encryption -- reflecting the effect of the flag, which is to say "don't send plaintext messages here."

zerver/views/push_notifications.py

timabbott · 2020-09-14T23:17:26Z

zilencer/models.py

@@ -32,6 +32,7 @@ class RemotePushDeviceToken(AbstractPushDeviceToken):
    server: RemoteZulipServer = models.ForeignKey(RemoteZulipServer, on_delete=models.CASCADE)
    # The user id on the remote server for this device device this is
    user_id: int = models.BigIntegerField(db_index=True)
+    encrypt_notifications: bool = models.BooleanField(default=False)


This name is confusing -- this field represents whether notifications ARE encrypted; the push bouncer shouldn't care. We should add a comment explaining it and also pick a better name. (I'm not sure actually what this is needed for?)

zulipbot · 2020-11-02T20:18:16Z

Heads up @hashirsarwar, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/master branch and resolve your pull request's merge conflicts accordingly.

zulipbot · 2021-09-08T19:17:19Z

Heads up @hashirsarwar, we just merged some commits that conflict with the changes your made in this pull request! You can review this repository's recent commits to see where the conflicts occur. Please rebase your feature branch against the upstream/main branch and resolve your pull request's merge conflicts accordingly.

timabbott · 2024-01-15T01:50:43Z

Closing in favor of the more current #26262 for this issue.

zulipbot added the size: M label Jun 5, 2020

hashirsarwar force-pushed the encryption branch from 6928008 to 64aa5df Compare June 5, 2020 21:05

zulipbot added the has conflicts label Jun 10, 2020

hashirsarwar force-pushed the encryption branch 4 times, most recently from 67c36dd to 102d311 Compare June 24, 2020 14:21

hashirsarwar force-pushed the encryption branch from 102d311 to a7a03e3 Compare June 25, 2020 17:19

zulipbot added size: XL and removed size: M labels Jun 25, 2020

hashirsarwar force-pushed the encryption branch 7 times, most recently from 5c18f5c to 51e343f Compare June 30, 2020 22:43

hashirsarwar force-pushed the encryption branch 2 times, most recently from bc499a6 to 6172f47 Compare July 3, 2020 20:57

Hypro999 suggested changes Jul 4, 2020

View reviewed changes

hashirsarwar force-pushed the encryption branch from 6172f47 to 5d431d8 Compare July 5, 2020 03:04

Hypro999 suggested changes Jul 5, 2020

View reviewed changes

zerver/tests/test_push_notifications.py Outdated Show resolved Hide resolved

zerver/tests/test_push_notifications.py Outdated Show resolved Hide resolved

zerver/lib/push_notifications.py Outdated Show resolved Hide resolved

Hypro999 reviewed Jul 6, 2020

View reviewed changes

zerver/lib/push_notifications.py Outdated Show resolved Hide resolved

hashirsarwar force-pushed the encryption branch 2 times, most recently from d69ac6d to 830d4cf Compare July 6, 2020 09:32

Hypro999 approved these changes Jul 9, 2020

View reviewed changes

zerver/tests/test_push_notifications.py Show resolved Hide resolved

zerver/tests/test_push_notifications.py Outdated Show resolved Hide resolved

zerver/tests/test_push_notifications.py Outdated Show resolved Hide resolved

gnprice reviewed Jul 11, 2020

View reviewed changes

zilencer/views.py Outdated Show resolved Hide resolved

zerver/lib/push_notifications.py Outdated Show resolved Hide resolved

hashirsarwar force-pushed the encryption branch 2 times, most recently from f2ddba6 to 0c1515a Compare July 16, 2020 15:44

gnprice requested changes Sep 5, 2020

View reviewed changes

gnprice requested changes Sep 10, 2020

View reviewed changes

hashirsarwar force-pushed the encryption branch 9 times, most recently from 6412287 to 5d5df13 Compare September 14, 2020 20:34

timabbott reviewed Sep 14, 2020

View reviewed changes

hashirsarwar force-pushed the encryption branch from 5d5df13 to 7a8b247 Compare September 20, 2020 18:14

zulipbot added the has conflicts label Nov 2, 2020

hashirsarwar added 6 commits November 27, 2020 16:03

models: Add encryption related database fileds.

c239177

encryption: Add symmetric key cryptography lib.

c4beac1

settings: Add PUSH_NOTIFICATION_ENCRYPTION setting.

7489686

push_notifications: Encrypt push notification data.

a195c78

push_notifications: Return encryption key in reg. response.

00ed21e

push_notifications: Upgrade bouncer to handle encryption.

4879f71

hashirsarwar force-pushed the encryption branch from 7a8b247 to 4879f71 Compare November 27, 2020 20:19

PIG208 mentioned this pull request Jul 14, 2023

Support end-to-end encryption for push notifications #26262

Draft

12 tasks

timabbott force-pushed the main branch 2 times, most recently from 4ec3636 to 88b200c Compare August 18, 2023 23:52

timabbott closed this Jan 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add end-to-end encryption for push notifications #15229

Add end-to-end encryption for push notifications #15229

hashirsarwar commented Jun 5, 2020

timabbott commented Jun 5, 2020

Hypro999 left a comment

Hypro999 left a comment •

edited

Hypro999 left a comment

gnprice left a comment

gnprice left a comment

gnprice Sep 5, 2020

hashirsarwar Sep 5, 2020

gnprice Sep 7, 2020

gnprice Sep 5, 2020

hashirsarwar Sep 5, 2020

Hypro999 Sep 5, 2020

gnprice Sep 7, 2020

timabbott Sep 10, 2020

gnprice Sep 5, 2020

timabbott Sep 10, 2020

gnprice Sep 10, 2020

gnprice Sep 5, 2020

timabbott Sep 10, 2020

timabbott commented Sep 10, 2020

hashirsarwar commented Sep 10, 2020

gnprice left a comment

gnprice Sep 10, 2020

gnprice Sep 10, 2020

gnprice Sep 10, 2020

gnprice Sep 10, 2020

gnprice Sep 10, 2020

timabbott Sep 14, 2020 •

edited

zulipbot commented Nov 2, 2020

zulipbot commented Sep 8, 2021

timabbott commented Jan 15, 2024

Add end-to-end encryption for push notifications #15229

Add end-to-end encryption for push notifications #15229

Conversation

hashirsarwar commented Jun 5, 2020

timabbott commented Jun 5, 2020

Hypro999 left a comment

Choose a reason for hiding this comment

Hypro999 left a comment • edited

Choose a reason for hiding this comment

Hypro999 left a comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timabbott commented Sep 10, 2020

hashirsarwar commented Sep 10, 2020

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timabbott Sep 14, 2020 • edited

Choose a reason for hiding this comment

zulipbot commented Nov 2, 2020

zulipbot commented Sep 8, 2021

timabbott commented Jan 15, 2024

Hypro999 left a comment •

edited

timabbott Sep 14, 2020 •

edited